| Message ID | 1364268527-32068-2-git-send-email-hongtao.jia@freescale.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Saturday, March 30, 2013 5:55 AM > To: Jia Hongtao-B38951 > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood Scott- > B07421; Li Yang-R58472; Jia Hongtao-B38951 > Subject: Re: [PATCH 2/2] powerpc/85xx: workaround for chips with MSI > hardware errata > > On 03/25/2013 10:28:47 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> > > --- > > arch/powerpc/sysdev/fsl_msi.c | 47 > > ++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 44 insertions(+), 3 deletions(-) > > > > diff --git a/arch/powerpc/sysdev/fsl_msi.c > > b/arch/powerpc/sysdev/fsl_msi.c index 178c994..d2f8040 100644 > > --- a/arch/powerpc/sysdev/fsl_msi.c > > +++ b/arch/powerpc/sysdev/fsl_msi.c > > @@ -28,6 +28,8 @@ > > #include "fsl_msi.h" > > #include "fsl_pci.h" > > > > +#define MSI_HW_ERRATA_ENDIAN 0x00000010 It seems Kumar like put this just in fsl_msi.c. Here is the comments from Kumar few days ago: "Is there any reason to put this in fsl_msi.h rather than just in fsl_msi.c itself?" I think the all the #defines should be together. Ether all in .h or all in .c. In this case I prefer your idea. > > This should probably be kept in the same place as the other > msi->features definitions (e.g. FSL_PIC_IP_*). > > > +/* MPIC version 2.0 has erratum PIC1 */ static int > > +mpic_has_errata(void) { > > + if (mpic_primary_get_version() == 0x0200) > > + return 1; > > + > > + return 0; > > +} > > mpic_has_erratum_pic1() You are right. Good advice. > > > + if ((features->fsl_pic_ip & FSL_PIC_IP_MASK) == > > FSL_PIC_IP_MPIC) { > > + rc = mpic_has_errata(); > > + if (rc > 0) { > > + msi->feature |= MSI_HW_ERRATA_ENDIAN; > > + } else if (rc < 0) { > > + err = rc; > > + goto error_out; > > + } > > When would mpic_has_errata() ever return a negative value (maybe > mpic_primary_get_version could fail, but you don't allow for that in the > interface)? > > If you're not going to add a way for errors to be returned back, just > do: > > if (mpic_has_erratum_pic1()) > msi->feature |= MSI_HW_ERRATA_ENDIAN; > > -Scott I will update the mpic_primary_get_version() and it will return 0 if failed. Based on the new mpic_primary_get_version() I agree with this. Thanks. -Hongtao.
On 04/02/2013 01:35:05 AM, Jia Hongtao-B38951 wrote: > > > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Saturday, March 30, 2013 5:55 AM > > To: Jia Hongtao-B38951 > > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood > Scott- > > B07421; Li Yang-R58472; Jia Hongtao-B38951 > > Subject: Re: [PATCH 2/2] powerpc/85xx: workaround for chips with MSI > > hardware errata > > > > On 03/25/2013 10:28:47 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> > > > --- > > > arch/powerpc/sysdev/fsl_msi.c | 47 > > > ++++++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 44 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/powerpc/sysdev/fsl_msi.c > > > b/arch/powerpc/sysdev/fsl_msi.c index 178c994..d2f8040 100644 > > > --- a/arch/powerpc/sysdev/fsl_msi.c > > > +++ b/arch/powerpc/sysdev/fsl_msi.c > > > @@ -28,6 +28,8 @@ > > > #include "fsl_msi.h" > > > #include "fsl_pci.h" > > > > > > +#define MSI_HW_ERRATA_ENDIAN 0x00000010 > > It seems Kumar like put this just in fsl_msi.c. > Here is the comments from Kumar few days ago: > > "Is there any reason to put this in fsl_msi.h rather than just in > fsl_msi.c itself?" > > I think the all the #defines should be together. > Ether all in .h or all in .c. > > In this case I prefer your idea. I don't care which file they go in (though .c is probably better if they don't need wider visibility), just as long as they're together. -Scott
On Apr 2, 2013, at 12:49 PM, Scott Wood wrote: > On 04/02/2013 01:35:05 AM, Jia Hongtao-B38951 wrote: >> > -----Original Message----- >> > From: Wood Scott-B07421 >> > Sent: Saturday, March 30, 2013 5:55 AM >> > To: Jia Hongtao-B38951 >> > Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood Scott- >> > B07421; Li Yang-R58472; Jia Hongtao-B38951 >> > Subject: Re: [PATCH 2/2] powerpc/85xx: workaround for chips with MSI >> > hardware errata >> > >> > On 03/25/2013 10:28:47 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> >> > > --- >> > > arch/powerpc/sysdev/fsl_msi.c | 47 >> > > ++++++++++++++++++++++++++++++++++++++++--- >> > > 1 file changed, 44 insertions(+), 3 deletions(-) >> > > >> > > diff --git a/arch/powerpc/sysdev/fsl_msi.c >> > > b/arch/powerpc/sysdev/fsl_msi.c index 178c994..d2f8040 100644 >> > > --- a/arch/powerpc/sysdev/fsl_msi.c >> > > +++ b/arch/powerpc/sysdev/fsl_msi.c >> > > @@ -28,6 +28,8 @@ >> > > #include "fsl_msi.h" >> > > #include "fsl_pci.h" >> > > >> > > +#define MSI_HW_ERRATA_ENDIAN 0x00000010 >> It seems Kumar like put this just in fsl_msi.c. >> Here is the comments from Kumar few days ago: >> "Is there any reason to put this in fsl_msi.h rather than just in >> fsl_msi.c itself?" >> I think the all the #defines should be together. >> Ether all in .h or all in .c. >> In this case I prefer your idea. > > I don't care which file they go in (though .c is probably better if they don't need wider visibility), just as long as they're together. > > -Scott Agreed, I didn't realize it was with mixed with the FSL_PIC_IP_* defines. So this should be with: #define FSL_PIC_IP_MASK 0x0000000F #define FSL_PIC_IP_MPIC 0x00000001 #define FSL_PIC_IP_IPIC 0x00000002 #define FSL_PIC_IP_VMPIC 0x00000003 in fsl_msi.h - k
diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c index 178c994..d2f8040 100644 --- a/arch/powerpc/sysdev/fsl_msi.c +++ b/arch/powerpc/sysdev/fsl_msi.c @@ -28,6 +28,8 @@ #include "fsl_msi.h" #include "fsl_pci.h" +#define MSI_HW_ERRATA_ENDIAN 0x00000010 + static LIST_HEAD(msi_head); struct fsl_msi_feature { @@ -98,8 +100,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 +154,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 +383,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_errata(void) +{ + if (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 +454,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) { + rc = mpic_has_errata(); + if (rc > 0) { + msi->feature |= MSI_HW_ERRATA_ENDIAN; + } else if (rc < 0) { + err = rc; + goto error_out; + } + } + /* * Remember the phandle, so that we can match with any PCI nodes * that have an "fsl,msi" property.