| Message ID | 1363074482-868-1-git-send-email-B38951@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kumar Gala |
| Headers | show |
On Tue, Mar 12, 2013 at 03:48:02PM +0800, Jia Hongtao wrote: > The MPIC chip with version 2.0 has a MSI errata (errata PIC1 of mpc8544), > It causes that neither MSI nor MSI-X can work fine. This is a workaround > to allow MSI-X to function properly. You say "neither MSI nor MSI-X can work fine", which I take to mean "both MSI and MSI-X do not work". But then you say this is a workaround to allow MSI-X to work. So what I think you mean is, the erratum prevents both MSI and MSI-X from working. This is a workaround that allows MSI-X to work, and in addition the patch prevents MSI from being used on chips with the erratum - because there is no workaround for MSI. > diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c > index 178c994..0dea680 100644 > --- a/arch/powerpc/sysdev/fsl_msi.c > +++ b/arch/powerpc/sysdev/fsl_msi.c > @@ -98,8 +98,20 @@ static int fsl_msi_init_allocator(struct fsl_msi *msi_data) > > static int fsl_msi_check_device(struct pci_dev *pdev, int nvec, int type) > { > + struct fsl_msi *msi; > + > if (type == PCI_CAP_ID_MSIX) > pr_debug("fslmsi: MSI-X untested, trying anyway.\n"); Seeing as this patch is enabling a workaround for MSI-X you've obviously tested MSI-X, so you should remove the two lines above. > + else if (type == PCI_CAP_ID_MSI) > + /* > + * MPIC chip with 2.0 version has erratum PIC1. It > + * causes that neither MSI nor MSI-X can work fine. > + * This is a workaround to allow MSI-X to function > + * properly. > + */ This is not a workaround. This is a check to prevent MSI from being used on buggy chipsets. > + list_for_each_entry(msi, &msi_head, list) > + if (msi->feature & MSI_HW_ERRATA_ENDIAN) > + return -EINVAL; I take it you're happy preventing MSI for all devices even if only a single chip in the machine has the erratum? In practice you probably have all or none with the erratum right? I would suggest brackets on an if with such a large body, even though it is OK as it is. > > return 0; > } > @@ -142,7 +154,11 @@ static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq, > msg->address_lo = lower_32_bits(address); > msg->address_hi = upper_32_bits(address); > > - msg->data = hwirq; > + /* See the comment in fsl_msi_check_device() */ > + if (msi_data->feature & MSI_HW_ERRATA_ENDIAN) > + msg->data = __swab32(hwirq); > + else > + msg->data = hwirq; This is the workaround. The comment here should say, "this only works for MSI-X, we prevent MSI in on buggy chips in fsl_msi_check_device()". > > pr_debug("%s: allocated srs: %d, ibs: %d\n", > __func__, hwirq / IRQS_PER_MSI_REG, hwirq % IRQS_PER_MSI_REG); > @@ -361,13 +377,43 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev, > return 0; > } > > +/* MPIC chip with 2.0 version has erratum PIC1 */ > +static int mpic_has_errata(struct platform_device *dev) > +{ > + struct device_node *mpic_node; > + > + mpic_node = of_irq_find_parent(dev->dev.of_node); > + if (mpic_node) { > + u32 *reg_base, brr1 = 0; > + /* Get the PIC reg base */ > + reg_base = of_iomap(mpic_node, 0); > + of_node_put(mpic_node); > + if (!reg_base) { > + dev_err(&dev->dev, "ioremap problem failed.\n"); > + return -EIO; > + } > + > + /* Get the mpic chip version from block revision register 1 */ > + brr1 = in_be32(reg_base + MPIC_FSL_BRR1); > + iounmap(reg_base); > + if ((brr1 & MPIC_FSL_BRR1_VER) == 0x0200) > + return 1; > + } else { > + dev_err(&dev->dev, "MSI can't find his parent mpic node.\n"); > + of_node_put(mpic_node); You don't need the put here, you know it's NULL (you just checked). > + return -ENODEV; > + } > + > + return 0; > +} > + > static const struct of_device_id fsl_of_msi_ids[]; > static int fsl_of_msi_probe(struct platform_device *dev) > { > const struct of_device_id *match; > struct fsl_msi *msi; > struct resource res; > - int err, i, j, irq_index, count; > + int err, i, j, irq_index, count, errata; > int rc; > const u32 *p; > const struct fsl_msi_feature *features; > @@ -423,6 +469,16 @@ static int fsl_of_msi_probe(struct platform_device *dev) > > msi->feature = features->fsl_pic_ip; > > + if ((features->fsl_pic_ip & FSL_PIC_IP_MASK) == FSL_PIC_IP_MPIC) { > + errata = mpic_has_errata(dev); > + if (errata > 0) { > + msi->feature |= MSI_HW_ERRATA_ENDIAN; > + } else if (errata < 0) { > + err = errata; > + goto error_out; > + } I don't think you need errata here, "rc" would be fine. cheers
> -----Original Message----- > From: Michael Ellerman [mailto:michael@ellerman.id.au] > Sent: Wednesday, March 13, 2013 1:04 PM > To: Jia Hongtao-B38951 > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood Scott- > B07421 > Subject: Re: [PATCH] powerpc/85xx: workaround for chips with MSI hardware > errata > > On Tue, Mar 12, 2013 at 03:48:02PM +0800, Jia Hongtao wrote: > > The MPIC chip with version 2.0 has a MSI errata (errata PIC1 of > mpc8544), > > It causes that neither MSI nor MSI-X can work fine. This is a > workaround > > to allow MSI-X to function properly. > > You say "neither MSI nor MSI-X can work fine", which I take to mean > "both MSI and MSI-X do not work". > > But then you say this is a workaround to allow MSI-X to work. > > So what I think you mean is, the erratum prevents both MSI and MSI-X > from working. This is a workaround that allows MSI-X to work, and in > addition > the patch prevents MSI from being used on chips with the erratum - > because there is no workaround for MSI. I'm happy that you understand the patch. > > > diff --git a/arch/powerpc/sysdev/fsl_msi.c > b/arch/powerpc/sysdev/fsl_msi.c > > index 178c994..0dea680 100644 > > --- a/arch/powerpc/sysdev/fsl_msi.c > > +++ b/arch/powerpc/sysdev/fsl_msi.c > > @@ -98,8 +98,20 @@ static int fsl_msi_init_allocator(struct fsl_msi > *msi_data) > > > > static int fsl_msi_check_device(struct pci_dev *pdev, int nvec, int > type) > > { > > + struct fsl_msi *msi; > > + > > if (type == PCI_CAP_ID_MSIX) > > pr_debug("fslmsi: MSI-X untested, trying anyway.\n"); > > Seeing as this patch is enabling a workaround for MSI-X you've obviously > tested MSI-X, so you should remove the two lines above. Right, will be removed. > > > + else if (type == PCI_CAP_ID_MSI) > > + /* > > + * MPIC chip with 2.0 version has erratum PIC1. It > > + * causes that neither MSI nor MSI-X can work fine. > > + * This is a workaround to allow MSI-X to function > > + * properly. > > + */ > > This is not a workaround. This is a check to prevent MSI from being used > on buggy chipsets. Yes, I will move the comments to the right place. > > > + list_for_each_entry(msi, &msi_head, list) > > + if (msi->feature & MSI_HW_ERRATA_ENDIAN) > > + return -EINVAL; > > I take it you're happy preventing MSI for all devices even if only a > single chip in the machine has the erratum? In practice you probably > have all or none with the erratum right? Need more investigations for this comment. > > I would suggest brackets on an if with such a large body, even though it > is OK as it is. > Actually, I'd like to brackets on *if body* too. I just follow the kernel *CodingStyle*: "Do not unnecessarily use braces where a single statement will do." In this case, I will use braces to make code clearer. > > > > return 0; > > } > > @@ -142,7 +154,11 @@ static void fsl_compose_msi_msg(struct pci_dev > *pdev, int hwirq, > > msg->address_lo = lower_32_bits(address); > > msg->address_hi = upper_32_bits(address); > > > > - msg->data = hwirq; > > + /* See the comment in fsl_msi_check_device() */ > > + if (msi_data->feature & MSI_HW_ERRATA_ENDIAN) > > + msg->data = __swab32(hwirq); > > + else > > + msg->data = hwirq; > > This is the workaround. The comment here should say, "this only works > for MSI-X, we prevent MSI in on buggy chips in fsl_msi_check_device()". Very helpful comment. > > > > > pr_debug("%s: allocated srs: %d, ibs: %d\n", > > __func__, hwirq / IRQS_PER_MSI_REG, hwirq % IRQS_PER_MSI_REG); > > @@ -361,13 +377,43 @@ static int fsl_msi_setup_hwirq(struct fsl_msi > *msi, struct platform_device *dev, > > return 0; > > } > > > > +/* MPIC chip with 2.0 version has erratum PIC1 */ > > +static int mpic_has_errata(struct platform_device *dev) > > +{ > > + struct device_node *mpic_node; > > + > > + mpic_node = of_irq_find_parent(dev->dev.of_node); > > + if (mpic_node) { > > + u32 *reg_base, brr1 = 0; > > + /* Get the PIC reg base */ > > + reg_base = of_iomap(mpic_node, 0); > > + of_node_put(mpic_node); > > + if (!reg_base) { > > + dev_err(&dev->dev, "ioremap problem failed.\n"); > > + return -EIO; > > + } > > + > > + /* Get the mpic chip version from block revision register 1 > */ > > + brr1 = in_be32(reg_base + MPIC_FSL_BRR1); > > + iounmap(reg_base); > > + if ((brr1 & MPIC_FSL_BRR1_VER) == 0x0200) > > + return 1; > > + } else { > > + dev_err(&dev->dev, "MSI can't find his parent mpic node.\n"); > > + of_node_put(mpic_node); > > You don't need the put here, you know it's NULL (you just checked). Yes. > > > + return -ENODEV; > > + } > > + > > + return 0; > > +} > > + > > static const struct of_device_id fsl_of_msi_ids[]; > > static int fsl_of_msi_probe(struct platform_device *dev) > > { > > const struct of_device_id *match; > > struct fsl_msi *msi; > > struct resource res; > > - int err, i, j, irq_index, count; > > + int err, i, j, irq_index, count, errata; > > int rc; > > const u32 *p; > > const struct fsl_msi_feature *features; > > @@ -423,6 +469,16 @@ static int fsl_of_msi_probe(struct platform_device > *dev) > > > > msi->feature = features->fsl_pic_ip; > > > > + if ((features->fsl_pic_ip & FSL_PIC_IP_MASK) == FSL_PIC_IP_MPIC) { > > + errata = mpic_has_errata(dev); > > + if (errata > 0) { > > + msi->feature |= MSI_HW_ERRATA_ENDIAN; > > + } else if (errata < 0) { > > + err = errata; > > + goto error_out; > > + } > > I don't think you need errata here, "rc" would be fine. Sounds reasonable. > > cheers Very grateful for all the constructive comments. -Hongtao.
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Thursday, March 14, 2013 4:24 AM > To: Michael Ellerman > Cc: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; > galak@kernel.crashing.org; Wood Scott-B07421 > Subject: Re: [PATCH] powerpc/85xx: workaround for chips with MSI hardware > errata > > On 03/13/2013 12:04:03 AM, Michael Ellerman wrote: > > On Tue, Mar 12, 2013 at 03:48:02PM +0800, Jia Hongtao wrote: > > > The MPIC chip with version 2.0 has a MSI errata (errata PIC1 of > > mpc8544), > > > It causes that neither MSI nor MSI-X can work fine. This is a > > workaround > > > to allow MSI-X to function properly. > > > > You say "neither MSI nor MSI-X can work fine", which I take to mean > > "both MSI and MSI-X do not work". > > > > But then you say this is a workaround to allow MSI-X to work. > > > > So what I think you mean is, the erratum prevents both MSI and MSI-X > > from working. This is a workaround that allows MSI-X to work, and in > > addition the patch prevents MSI from being used on chips with the > > erratum - because there is no workaround for MSI. > > There actually is a workaround for MSI, but it's more complicated and not > implemented by this patch. > > We could also possibly get away with allowing exactly one MSI > (byteswapping doesn't matter if the value is zero) -- not sure how hard > that would be. For now we have no plan to support MSI workaround. Also MSI workaround deserve a new patch :) I will sent V2 of this patch soon. -Hongtao. > > > > + list_for_each_entry(msi, &msi_head, list) > > > + if (msi->feature & MSI_HW_ERRATA_ENDIAN) > > > + return -EINVAL; > > > > I take it you're happy preventing MSI for all devices even if only a > > single chip in the machine has the erratum? In practice you probably > > have all or none with the erratum right? > > Yes, it's all integrated onto one chip (the SoC itself). In fact there > should only be one MSI block on these chips. > > -Scott
diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c index 178c994..0dea680 100644 --- a/arch/powerpc/sysdev/fsl_msi.c +++ b/arch/powerpc/sysdev/fsl_msi.c @@ -98,8 +98,20 @@ static int fsl_msi_init_allocator(struct fsl_msi *msi_data) static int fsl_msi_check_device(struct pci_dev *pdev, int nvec, int type) { + struct fsl_msi *msi; + if (type == PCI_CAP_ID_MSIX) pr_debug("fslmsi: MSI-X untested, trying anyway.\n"); + else if (type == PCI_CAP_ID_MSI) + /* + * MPIC chip with 2.0 version has erratum PIC1. It + * causes that neither MSI nor MSI-X can work fine. + * This is a workaround to allow MSI-X to function + * properly. + */ + list_for_each_entry(msi, &msi_head, list) + if (msi->feature & MSI_HW_ERRATA_ENDIAN) + return -EINVAL; return 0; } @@ -142,7 +154,11 @@ static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq, msg->address_lo = lower_32_bits(address); msg->address_hi = upper_32_bits(address); - msg->data = hwirq; + /* See the comment in fsl_msi_check_device() */ + if (msi_data->feature & MSI_HW_ERRATA_ENDIAN) + msg->data = __swab32(hwirq); + else + msg->data = hwirq; pr_debug("%s: allocated srs: %d, ibs: %d\n", __func__, hwirq / IRQS_PER_MSI_REG, hwirq % IRQS_PER_MSI_REG); @@ -361,13 +377,43 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev, return 0; } +/* MPIC chip with 2.0 version has erratum PIC1 */ +static int mpic_has_errata(struct platform_device *dev) +{ + struct device_node *mpic_node; + + mpic_node = of_irq_find_parent(dev->dev.of_node); + if (mpic_node) { + u32 *reg_base, brr1 = 0; + /* Get the PIC reg base */ + reg_base = of_iomap(mpic_node, 0); + of_node_put(mpic_node); + if (!reg_base) { + dev_err(&dev->dev, "ioremap problem failed.\n"); + return -EIO; + } + + /* Get the mpic chip version from block revision register 1 */ + brr1 = in_be32(reg_base + MPIC_FSL_BRR1); + iounmap(reg_base); + if ((brr1 & MPIC_FSL_BRR1_VER) == 0x0200) + return 1; + } else { + dev_err(&dev->dev, "MSI can't find his parent mpic node.\n"); + of_node_put(mpic_node); + return -ENODEV; + } + + return 0; +} + static const struct of_device_id fsl_of_msi_ids[]; static int fsl_of_msi_probe(struct platform_device *dev) { const struct of_device_id *match; struct fsl_msi *msi; struct resource res; - int err, i, j, irq_index, count; + int err, i, j, irq_index, count, errata; int rc; const u32 *p; const struct fsl_msi_feature *features; @@ -423,6 +469,16 @@ static int fsl_of_msi_probe(struct platform_device *dev) msi->feature = features->fsl_pic_ip; + if ((features->fsl_pic_ip & FSL_PIC_IP_MASK) == FSL_PIC_IP_MPIC) { + errata = mpic_has_errata(dev); + if (errata > 0) { + msi->feature |= MSI_HW_ERRATA_ENDIAN; + } else if (errata < 0) { + err = errata; + goto error_out; + } + } + /* * Remember the phandle, so that we can match with any PCI nodes * that have an "fsl,msi" property. diff --git a/arch/powerpc/sysdev/fsl_msi.h b/arch/powerpc/sysdev/fsl_msi.h index 8225f86..7389e8e 100644 --- a/arch/powerpc/sysdev/fsl_msi.h +++ b/arch/powerpc/sysdev/fsl_msi.h @@ -25,6 +25,8 @@ #define FSL_PIC_IP_IPIC 0x00000002 #define FSL_PIC_IP_VMPIC 0x00000003 +#define MSI_HW_ERRATA_ENDIAN 0x00000010 + struct fsl_msi { struct irq_domain *irqhost;