[openib-general] [PATCH] mthca - command interface

[Roland Dreier]

Thanks, looks good.  A few comments:

 > +	int                       can_post_doorbells;
 > +	int                       dbell_post;

I think these should be combined with struct mthca_cmd.use_events with
some bit masks defined.  It feels too bloaty to use 3 ints to hold
what's really 3 bits.

 > +	void __iomem             *dbell_map;
 > +	void __iomem             *dbell_ptrs[8];
 > +	u64                       dbell_base;
 > +	u16                       dbell_offsets[8];

The magic number 8 should be given a name as an enum value.

It seems that we don't need to keep these values around for the whole
lifetime of the driver.  How about holding dbell_base and
dbell_offsets somewhere temporary just long enough to create dbell_map
and dbell_ptrs?  Or maybe it's not even worth having dbell_ptrs --
just keep dbell_offsets and use it along with dbell_map when writing
commands into the doorbell page.

 > +	MTHCA_GET(tmp, outbox, QUERY_FW_CMD_DB_EN_OFFSET);
 > +	dev->cmd.can_post_doorbells = tmp & 0x1;

Please add an mthca_dbg() line that says whether the HW/FW can post
commands through doorbells, and whether enabling the feature
succeeded.

 > +static void mthca_cmd_post_dbell(struct mthca_dev *dev,
 > +				 u64 in_param,
 > +				 u64 out_param,
 > +				 u32 in_modifier,
 > +				 u8 op_modifier,
 > +				 u16 op,
 > +				 u16 token)
 > +{
 > +	void __iomem **ptr = dev->cmd.dbell_ptrs;
 > +
 > +	writel((__force u32) cpu_to_be32(in_param >> 32),           ptr[0]);
 > +	writel((__force u32) cpu_to_be32(in_param & 0xfffffffful),  ptr[1]);

Does this work on a big-endian system?  It seems that writel() will do
one swap too many; I think you need __raw_writel() as in the existing
mthca_cmd_post().  I don't know what the precise ordering requirements
for commands through the UAR are, but you need explicit wmb()s for
__raw_writel() to guarantee order.

 - R.