[PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu

[Michael Ellerman]

On Mon, 2008-04-21 at 18:01 +0800, Jin Zhengxiong wrote:
> Hi, Michael,
> 
> Thank you very much for you input, please see my inline answer.

No worries.

> > > +static int fsl_msi_reserve_dt_hwirqs(struct fsl_msi *msi)
> > > +{
> > > +	int i, len;
> > > +	const u32 *p;
> > > +
> > > +	p = of_get_property(msi->of_node, "msi-available-ranges", &len);
> > > +	if (!p) {
> > > +		pr_debug("fsl_msi: no msi-available-ranges 
> > property found \
> > > +				on %s\n", msi->of_node->full_name);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	if (len & 0x8 != 0) {
> > > +		printk(KERN_WARNING "fsl_msi: Malformed 
> > msi-available-ranges "
> > > +		       "property on %s\n", msi->of_node->full_name);
> > > +		return -EINVAL;
> > > +	}
> > 
> > Do you really want a bitwise and with 0x8?
> > 
> The range for the msi interrupt can be seperated to several part. 
> This can used to check the if the ranges is correct. 

I don't see how. AFAIK the "msi-available-ranges" property is just a
list of u32 pairs, so the only thing that makes sense is to check that
the length is a multiple of 8, not that it has the 3rd bit set.

> > > +static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq,
> > > +				  struct msi_msg *msg)
> > > +{
> > > +	unsigned int srs;
> > > +	unsigned int ibs;
> > > +	struct fsl_msi *msi = fsl_msi;
> > > +
> > > +	srs = hwirq / INT_PER_MSIR;
> > > +	ibs = hwirq % INT_PER_MSIR;
> > > +
> > > +	msg->address_lo = msi->msi_addr_lo;
> > > +	msg->address_hi = msi->msi_addr_hi;
> > > +	msg->data = (srs << 5) | (ibs & 0x1F);
> > 
> > Is the 5 and 0x1F independent of the INT_PER_MSIR value? Given the
> > current values isn't this a no-op, or am I missing something?
> > 
> Do you mean there're another way to get the msg->data from the hwirq?  

No I mean I'm confused about the maths here. If we pull out the
variables it boils down to:

data = ((hwirq / 32) << 5) | ((hwirq % 32) & 0x1F)

Which doesn't seem to actually do anything?

> > > +static int fsl_setup_msi_irqs(struct pci_dev *pdev, int 
> > nvec, int type)
> > > +{
> > > +	irq_hw_number_t hwirq;
> > > +	int rc;
> > > +	unsigned int virq;
> > > +	struct msi_desc *entry;
> > > +	struct msi_msg msg;
> > > +	struct fsl_msi *msi = fsl_msi;
> > 
> > A couple of places you put this into a local called "msi" 
> > which is not the
> > greatest name in the world IMHO :)
> > 
> Thank you, I'll try to use another name, Do you have any suggestion?

Oh I dunno, maybe msi_data or msi_state ? 


cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20080422/ffbc4eab/attachment.pgp>