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

[Jin Zhengxiong]

> -----Original Message-----
> From: Michael Ellerman [mailto:michael at ellerman.id.au] 
> Sent: Tuesday, April 22, 2008 12:35 PM
> To: Jin Zhengxiong
> Cc: linuxppc-dev list; Gala Kumar
> Subject: RE: [PATCH 1/3] MSI driver for Freescale 83xx/85xx/86xx cpu
> 
> 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.
> 
I found the problem. I'll check the length if it's a multiple of 8.
Thanks

> > > > +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?
> 
Thanks, The hwirq number can stand for the data to the msiir in this
case.
I'll change this.