| Message ID | 1364954598-31914-2-git-send-email-hongtao.jia@freescale.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
On Wed, Apr 03, 2013 at 10:03:18AM +0800, Hongtao Jia wrote: > The MPIC 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. > > Signed-off-by: Liu Shuo <soniccat.liu@gmail.com> > Signed-off-by: Li Yang <leoli@freescale.com> > Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com> Building on 83xx: arch/powerpc/sysdev/built-in.o: In function `fsl_of_msi_probe': fsl_msi.c:(.text+0x1464): undefined reference to `fsl_mpic_primary_get_version' make[1]: *** [vmlinux] Error 1 make: *** [sub-make] Error 2 fsl_msi.c supports IPIC as well. -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Friday, June 28, 2013 10:29 AM > To: Jia Hongtao-B38951 > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood Scott- > B07421 > Subject: Re: [V2,2/2] powerpc/85xx: workaround for chips with MSI > hardware errata > > On Wed, Apr 03, 2013 at 10:03:18AM +0800, Hongtao Jia wrote: > > The MPIC 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. > > > > Signed-off-by: Liu Shuo <soniccat.liu@gmail.com> > > Signed-off-by: Li Yang <leoli@freescale.com> > > Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com> > > Building on 83xx: > > arch/powerpc/sysdev/built-in.o: In function `fsl_of_msi_probe': > fsl_msi.c:(.text+0x1464): undefined reference to > `fsl_mpic_primary_get_version' > make[1]: *** [vmlinux] Error 1 > make: *** [sub-make] Error 2 > > fsl_msi.c supports IPIC as well. > > -Scott Hi Scott, I updated the patch to fix this compile error just now. please refer to: http://patchwork.ozlabs.org/patch/256018/ Thanks. -Hongtao
> -----Original Message----- > From: Jia Hongtao-B38951 > Sent: Monday, July 01, 2013 5:36 PM > To: Wood Scott-B07421 > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org > Subject: RE: [V2,2/2] powerpc/85xx: workaround for chips with MSI > hardware errata > > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Friday, June 28, 2013 10:29 AM > > To: Jia Hongtao-B38951 > > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood > > Scott- > > B07421 > > Subject: Re: [V2,2/2] powerpc/85xx: workaround for chips with MSI > > hardware errata > > > > On Wed, Apr 03, 2013 at 10:03:18AM +0800, Hongtao Jia wrote: > > > The MPIC 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. > > > > > > Signed-off-by: Liu Shuo <soniccat.liu@gmail.com> > > > Signed-off-by: Li Yang <leoli@freescale.com> > > > Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com> > > > > Building on 83xx: > > > > arch/powerpc/sysdev/built-in.o: In function `fsl_of_msi_probe': > > fsl_msi.c:(.text+0x1464): undefined reference to > > `fsl_mpic_primary_get_version' > > make[1]: *** [vmlinux] Error 1 > > make: *** [sub-make] Error 2 > > > > fsl_msi.c supports IPIC as well. > > > > -Scott > > Hi Scott, > I updated the patch to fix this compile error just now. > please refer to: > http://patchwork.ozlabs.org/patch/256018/ > > Thanks. > -Hongtao Hi Scott, The 83xx compile issue has already been fixed. Please have a review on this patch. Thanks. -Hongtao
On Wed, 2013-09-04 at 23:00 -0500, Jia Hongtao-B38951 wrote: > > -----Original Message----- > > From: Jia Hongtao-B38951 > > Sent: Monday, July 01, 2013 5:36 PM > > To: Wood Scott-B07421 > > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org > > Subject: RE: [V2,2/2] powerpc/85xx: workaround for chips with MSI > > hardware errata > > > > > -----Original Message----- > > > From: Wood Scott-B07421 > > > Sent: Friday, June 28, 2013 10:29 AM > > > To: Jia Hongtao-B38951 > > > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood > > > Scott- > > > B07421 > > > Subject: Re: [V2,2/2] powerpc/85xx: workaround for chips with MSI > > > hardware errata > > > > > > On Wed, Apr 03, 2013 at 10:03:18AM +0800, Hongtao Jia wrote: > > > > The MPIC 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. > > > > > > > > Signed-off-by: Liu Shuo <soniccat.liu@gmail.com> > > > > Signed-off-by: Li Yang <leoli@freescale.com> > > > > Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com> > > > > > > Building on 83xx: > > > > > > arch/powerpc/sysdev/built-in.o: In function `fsl_of_msi_probe': > > > fsl_msi.c:(.text+0x1464): undefined reference to > > > `fsl_mpic_primary_get_version' > > > make[1]: *** [vmlinux] Error 1 > > > make: *** [sub-make] Error 2 > > > > > > fsl_msi.c supports IPIC as well. > > > > > > -Scott > > > > Hi Scott, > > I updated the patch to fix this compile error just now. > > please refer to: > > http://patchwork.ozlabs.org/patch/256018/ > > > > Thanks. > > -Hongtao > > Hi Scott, > > The 83xx compile issue has already been fixed. > Please have a review on this patch. Oh, sorry -- I missed it because it was marked "Changes Requested". I've changed the status and will consider it for the next batch of "next" patches. In the future, if a patch is miscategorized in patchwork (e.g. says "changes requested" when there is no longer a need to submit a new patch) please mention that specifically and provide the patchwork URL. -Scott
On Apr 2, 2013, at 9:03 PM, Jia Hongtao wrote: > The MPIC 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. > > Signed-off-by: Liu Shuo <soniccat.liu@gmail.com> > Signed-off-by: Li Yang <leoli@freescale.com> > Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com> > --- > Changes for V2: > * change the name of function mpic_has_errata() to mpic_has_erratum_pic1(). > * move MSI_HW_ERRATA_ENDIAN define to fsl_msi.h with all other defines. > > arch/powerpc/sysdev/fsl_msi.c | 40 +++++++++++++++++++++++++++++++++++++--- > arch/powerpc/sysdev/fsl_msi.h | 2 ++ > 2 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c > index 178c994..ca1157a 100644 > --- a/arch/powerpc/sysdev/fsl_msi.c > +++ b/arch/powerpc/sysdev/fsl_msi.c > @@ -98,8 +98,18 @@ 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) > { > - if (type == PCI_CAP_ID_MSIX) > - pr_debug("fslmsi: MSI-X untested, trying anyway.\n"); > + struct fsl_msi *msi; > + > + if (type == PCI_CAP_ID_MSI) { > + /* > + * MPIC version 2.0 has erratum PIC1. For now MSI > + * could not work. So check to prevent MSI from > + * being used on the board with this erratum. > + */ > + list_for_each_entry(msi, &msi_head, list) > + if (msi->feature & MSI_HW_ERRATA_ENDIAN) > + return -EINVAL; > + } > > return 0; > } > @@ -142,7 +152,17 @@ 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; > + /* > + * MPIC version 2.0 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. It only works for MSI-X, we prevent > + * MSI on buggy chips 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,6 +381,15 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev, > return 0; > } > > +/* MPIC version 2.0 has erratum PIC1 */ > +static int mpic_has_erratum_pic1(void) > +{ > + if (fsl_mpic_primary_get_version() == 0x0200) > + return 1; > + > + return 0; > +} > + > static const struct of_device_id fsl_of_msi_ids[]; > static int fsl_of_msi_probe(struct platform_device *dev) > { > @@ -423,6 +452,11 @@ 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) { > + if (mpic_has_erratum_pic1()) Get ride of the mpic_has_erratum_pic1() function and just put the test here > + msi->feature |= MSI_HW_ERRATA_ENDIAN; > + } > + > /* > * 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 > + Why does this need to be in the header, why not just have it in the .c only > struct fsl_msi { > struct irq_domain *irqhost; > > -- > 1.8.0 >
On Thu, 2013-09-05 at 13:34 -0500, Kumar Gala wrote: > On Apr 2, 2013, at 9:03 PM, Jia Hongtao wrote: > > + msi->feature |= MSI_HW_ERRATA_ENDIAN; > > + } > > + > > /* > > * 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 > > + > > Why does this need to be in the header, why not just have it in the .c only Didn't you ask this last time around? :-) This flag is part of the same numberspace as FSL_PIC_IP_xxx and thus should be defined in the same place. -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Friday, September 06, 2013 1:57 AM > To: Jia Hongtao-B38951 > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; > galak@kernel.crashing.org > Subject: Re: [V2,2/2] powerpc/85xx: workaround for chips with MSI > hardware errata > > On Wed, 2013-09-04 at 23:00 -0500, Jia Hongtao-B38951 wrote: > > > -----Original Message----- > > > From: Jia Hongtao-B38951 > > > Sent: Monday, July 01, 2013 5:36 PM > > > To: Wood Scott-B07421 > > > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org > > > Subject: RE: [V2,2/2] powerpc/85xx: workaround for chips with MSI > > > hardware errata > > > > > > > -----Original Message----- > > > > From: Wood Scott-B07421 > > > > Sent: Friday, June 28, 2013 10:29 AM > > > > To: Jia Hongtao-B38951 > > > > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood > > > > Scott- > > > > B07421 > > > > Subject: Re: [V2,2/2] powerpc/85xx: workaround for chips with MSI > > > > hardware errata > > > > > > > > On Wed, Apr 03, 2013 at 10:03:18AM +0800, Hongtao Jia wrote: > > > > > The MPIC 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. > > > > > > > > > > Signed-off-by: Liu Shuo <soniccat.liu@gmail.com> > > > > > Signed-off-by: Li Yang <leoli@freescale.com> > > > > > Signed-off-by: Jia Hongtao <hongtao.jia@freescale.com> > > > > > > > > Building on 83xx: > > > > > > > > arch/powerpc/sysdev/built-in.o: In function `fsl_of_msi_probe': > > > > fsl_msi.c:(.text+0x1464): undefined reference to > > > > `fsl_mpic_primary_get_version' > > > > make[1]: *** [vmlinux] Error 1 > > > > make: *** [sub-make] Error 2 > > > > > > > > fsl_msi.c supports IPIC as well. > > > > > > > > -Scott > > > > > > Hi Scott, > > > I updated the patch to fix this compile error just now. > > > please refer to: > > > http://patchwork.ozlabs.org/patch/256018/ > > > > > > Thanks. > > > -Hongtao > > > > Hi Scott, > > > > The 83xx compile issue has already been fixed. > > Please have a review on this patch. > > Oh, sorry -- I missed it because it was marked "Changes Requested". > I've changed the status and will consider it for the next batch of "next" > patches. > > In the future, if a patch is miscategorized in patchwork (e.g. says > "changes requested" when there is no longer a need to submit a new > patch) please mention that specifically and provide the patchwork URL. > > -Scott > Ok, got it. Sorry for your inconvenient. -Hongtao
On Sep 5, 2013, at 1:37 PM, Scott Wood wrote: > On Thu, 2013-09-05 at 13:34 -0500, Kumar Gala wrote: >> On Apr 2, 2013, at 9:03 PM, Jia Hongtao wrote: >>> + msi->feature |= MSI_HW_ERRATA_ENDIAN; >>> + } >>> + >>> /* >>> * 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 >>> + >> >> Why does this need to be in the header, why not just have it in the .c only > > Didn't you ask this last time around? :-) > > This flag is part of the same numberspace as FSL_PIC_IP_xxx and thus > should be defined in the same place. I probably did, if its part of the FSL_PIC_IP_xxx namespace, than lets remove blank line between things to make that a bit more clear - k
On Fri, 2013-09-06 at 10:01 -0500, Kumar Gala wrote: > On Sep 5, 2013, at 1:37 PM, Scott Wood wrote: > > > On Thu, 2013-09-05 at 13:34 -0500, Kumar Gala wrote: > >> On Apr 2, 2013, at 9:03 PM, Jia Hongtao wrote: > >>> + msi->feature |= MSI_HW_ERRATA_ENDIAN; > >>> + } > >>> + > >>> /* > >>> * 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 > >>> + > >> > >> Why does this need to be in the header, why not just have it in the .c only > > > > Didn't you ask this last time around? :-) > > > > This flag is part of the same numberspace as FSL_PIC_IP_xxx and thus > > should be defined in the same place. > > I probably did, if its part of the FSL_PIC_IP_xxx namespace, than lets remove blank line between things to make that a bit more clear It's not part of the FSL_PIC_IP_MASK subnumberspace though, just the overall msi->features numberspace. It would be nice if these symbols could have some sort of prefix in common, though. -Scott
On Sep 6, 2013, at 10:36 AM, Scott Wood wrote: > On Fri, 2013-09-06 at 10:01 -0500, Kumar Gala wrote: >> On Sep 5, 2013, at 1:37 PM, Scott Wood wrote: >> >>> On Thu, 2013-09-05 at 13:34 -0500, Kumar Gala wrote: >>>> On Apr 2, 2013, at 9:03 PM, Jia Hongtao wrote: >>>>> + msi->feature |= MSI_HW_ERRATA_ENDIAN; >>>>> + } >>>>> + >>>>> /* >>>>> * 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 >>>>> + >>>> >>>> Why does this need to be in the header, why not just have it in the .c only >>> >>> Didn't you ask this last time around? :-) >>> >>> This flag is part of the same numberspace as FSL_PIC_IP_xxx and thus >>> should be defined in the same place. >> >> I probably did, if its part of the FSL_PIC_IP_xxx namespace, than lets remove blank line between things to make that a bit more clear > > It's not part of the FSL_PIC_IP_MASK subnumberspace though, just the > overall msi->features numberspace. > > It would be nice if these symbols could have some sort of prefix in > common, though. > > -Scott Maybe we should do something like MSI_FTR_ as a prefix - k
diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c index 178c994..ca1157a 100644 --- a/arch/powerpc/sysdev/fsl_msi.c +++ b/arch/powerpc/sysdev/fsl_msi.c @@ -98,8 +98,18 @@ 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) { - if (type == PCI_CAP_ID_MSIX) - pr_debug("fslmsi: MSI-X untested, trying anyway.\n"); + struct fsl_msi *msi; + + if (type == PCI_CAP_ID_MSI) { + /* + * MPIC version 2.0 has erratum PIC1. For now MSI + * could not work. So check to prevent MSI from + * being used on the board with this erratum. + */ + list_for_each_entry(msi, &msi_head, list) + if (msi->feature & MSI_HW_ERRATA_ENDIAN) + return -EINVAL; + } return 0; } @@ -142,7 +152,17 @@ 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; + /* + * MPIC version 2.0 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. It only works for MSI-X, we prevent + * MSI on buggy chips 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,6 +381,15 @@ static int fsl_msi_setup_hwirq(struct fsl_msi *msi, struct platform_device *dev, return 0; } +/* MPIC version 2.0 has erratum PIC1 */ +static int mpic_has_erratum_pic1(void) +{ + if (fsl_mpic_primary_get_version() == 0x0200) + return 1; + + return 0; +} + static const struct of_device_id fsl_of_msi_ids[]; static int fsl_of_msi_probe(struct platform_device *dev) { @@ -423,6 +452,11 @@ 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) { + if (mpic_has_erratum_pic1()) + msi->feature |= MSI_HW_ERRATA_ENDIAN; + } + /* * 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;