Patchwork [2/2] powerpc/85xx: workaround for chips with MSI hardware errata

login
register
mail settings
Submitter Hongtao Jia
Date March 26, 2013, 3:28 a.m.
Message ID <1364268527-32068-2-git-send-email-hongtao.jia@freescale.com>
Download mbox | patch
Permalink /patch/231060/
State Superseded
Headers show

Comments

Hongtao Jia - March 26, 2013, 3:28 a.m.
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(-)
Hongtao Jia - April 2, 2013, 6:35 a.m.
> -----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.
Scott Wood - April 2, 2013, 5:49 p.m.
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
Kumar Gala - April 2, 2013, 6:01 p.m.
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

Patch

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.