Re: [PATCH RESEND] PCI: Check for USB xHCI class for HAPS platform

Re: [PATCH RESEND] PCI: Check for USB xHCI class for HAPS platform

[Bjorn Helgaas]

[+cc Richard, Lucas]

On Tue, Feb 05, 2019 at 01:04:28PM -0800, Thinh Nguyen wrote:
> The Synopsys HAPS USB controller has a VID PID (16c3,abcd) that matches
> to an existing PCIe controller. This quirk is intended for USB HAPS
> devices only. To fix this, check for the PCI class USB xHCI to prevent
> matching the PCIe controllers.

So there are at least three different parts with the same Vendor &
Device ID ([16c3:abcd]):

  1) Synopsys HAPS USB3 controller
  2) Synopsys PCIe IP in the NXP i.MX6QP (reported by Lukas)
  3) Synopsys PCIe IP in the NXP i.MX7D (reported by Trent)

I don't know if Synopsys is to blame for 2 & 3, or if NXP was expected
to change the Vendor ID when incorporating the Synopsys IP into the
i.MX designs.  But even leaving the default Device ID of the PCIe IP
the same as another Synopsys device was probably a bad idea.

dwc3-haps claims by Vendor & Device ID; it doesn't look at the Class
Code.  What happens when it tries to claim the PCIe IP in i.MX?

lspci also doesn't look at the Class Code when it looks up the text
name of devices, so it will print the same name for both.  Of course,
the Linux PCI ID database at https://pci-ids.ucw.cz/read/PC/16c3 shows
basically no information for the 0x16c3 Vendor ID, so it won't print
anything useful no matter what.  You could add some things there :)

> Fixes: 03e6742584af ("PCI: Override Synopsys USB 3.x HAPS device class")
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
>  drivers/pci/quirks.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index b0a413f3f7ca..e2a879e93d86 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -639,8 +639,9 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
>  		break;
>  	}
>  }
> -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
> -			 quirk_synopsys_haps);
> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
> +			       PCI_CLASS_SERIAL_USB_XHCI, 0,
> +			       quirk_synopsys_haps);
>  
>  /*
>   * Let's make the southbridge information explicit instead of having to
> -- 
> 2.11.0
> 

Re: [PATCH RESEND] PCI: Check for USB xHCI class for HAPS platform

[John Youn]

On 02/05/2019 03:32 PM, Bjorn Helgaas wrote:
> [+cc Richard, Lucas]
> 
> On Tue, Feb 05, 2019 at 01:04:28PM -0800, Thinh Nguyen wrote:
>> The Synopsys HAPS USB controller has a VID PID (16c3,abcd) that matches
>> to an existing PCIe controller. This quirk is intended for USB HAPS
>> devices only. To fix this, check for the PCI class USB xHCI to prevent
>> matching the PCIe controllers.
> 
> So there are at least three different parts with the same Vendor &
> Device ID ([16c3:abcd]):
> 
>    1) Synopsys HAPS USB3 controller
>    2) Synopsys PCIe IP in the NXP i.MX6QP (reported by Lukas)
>    3) Synopsys PCIe IP in the NXP i.MX7D (reported by Trent)
> 
> I don't know if Synopsys is to blame for 2 & 3, or if NXP was expected
> to change the Vendor ID when incorporating the Synopsys IP into the
> i.MX designs.  But even leaving the default Device ID of the PCIe IP
> the same as another Synopsys device was probably a bad idea.


Hi Bjorn,

 From talking with our PCIe folks, our best guess is a vendor
misconfiguration. The PCIe IP ships with a different PID by default
and does not collide with USB.

> dwc3-haps claims by Vendor & Device ID; it doesn't look at the Class
> Code.  What happens when it tries to claim the PCIe IP in i.MX?

We can add the class code to dwc3-haps to account for this.

John

Re: [PATCH RESEND] PCI: Check for USB xHCI class for HAPS platform

[Bjorn Helgaas]

On Tue, Feb 05, 2019 at 05:01:18PM -0800, John Youn wrote:
> On 02/05/2019 03:32 PM, Bjorn Helgaas wrote:
> > On Tue, Feb 05, 2019 at 01:04:28PM -0800, Thinh Nguyen wrote:
> > > The Synopsys HAPS USB controller has a VID PID (16c3,abcd) that matches
> > > to an existing PCIe controller. This quirk is intended for USB HAPS
> > > devices only. To fix this, check for the PCI class USB xHCI to prevent
> > > matching the PCIe controllers.
> > 
> > So there are at least three different parts with the same Vendor &
> > Device ID ([16c3:abcd]):
> > 
> >    1) Synopsys HAPS USB3 controller
> >    2) Synopsys PCIe IP in the NXP i.MX6QP (reported by Lukas)
> >    3) Synopsys PCIe IP in the NXP i.MX7D (reported by Trent)
> > 
> > I don't know if Synopsys is to blame for 2 & 3, or if NXP was expected
> > to change the Vendor ID when incorporating the Synopsys IP into the
> > i.MX designs.  But even leaving the default Device ID of the PCIe IP
> > the same as another Synopsys device was probably a bad idea.
> 
> Hi Bjorn,
> 
> From talking with our PCIe folks, our best guess is a vendor
> misconfiguration. The PCIe IP ships with a different PID by default
> and does not collide with USB.

If that's true, your vendor was very unlucky in choosing 0xabcd.  They
also have a pretty serious misunderstanding of how PCI IDs work, which
will cause you major headaches if they continue allocating Device IDs
from the Synopsys space.

In this particular case it's probably not a disaster because the
i.MX6QP and i.MX7D devices are both bridges (Root Ports, I think), and
we don't currently have any drivers that claim those by ID.  The
portdrv claims them by Class Code.

> > dwc3-haps claims by Vendor & Device ID; it doesn't look at the Class
> > Code.  What happens when it tries to claim the PCIe IP in i.MX?
> 
> We can add the class code to dwc3-haps to account for this.

I think that'd be a good idea.  I think portdrv will claim the Root
Port first, before dwc3-haps has a chance, so the driver core won't
even call dwc3_haps_probe().

But if you unset CONFIG_PCIEPORTBUS or boot with "pcie_ports=compat",
I bet dwc3-haps *will* claim it, and things will fail a little less
gracefully.

Bjorn

Re: [PATCH RESEND] PCI: Check for USB xHCI class for HAPS platform

[Lucas Stach]

Hi Bjorn,

Am Dienstag, den 05.02.2019, 17:31 -0600 schrieb Bjorn Helgaas:
> [+cc Richard, Lucas]
> 
> On Tue, Feb 05, 2019 at 01:04:28PM -0800, Thinh Nguyen wrote:
> > The Synopsys HAPS USB controller has a VID PID (16c3,abcd) that
> > matches
> > to an existing PCIe controller. This quirk is intended for USB HAPS
> > devices only. To fix this, check for the PCI class USB xHCI to
> > prevent
> > matching the PCIe controllers.
> 
> So there are at least three different parts with the same Vendor &
> Device ID ([16c3:abcd]):
> 
>   1) Synopsys HAPS USB3 controller
>   2) Synopsys PCIe IP in the NXP i.MX6QP (reported by Lukas)

The first incorporation of this IP in a Freescale SoC (i.MX6Q) also
uses the device ID 0xabcd.

So this seems to be a major miscommunication between NXP/Freescale and
Synopsys.

Regards,
Lucas

>   3) Synopsys PCIe IP in the NXP i.MX7D (reported by Trent)
> 
> I don't know if Synopsys is to blame for 2 & 3, or if NXP was
> expected
> to change the Vendor ID when incorporating the Synopsys IP into the
> i.MX designs.  But even leaving the default Device ID of the PCIe IP
> the same as another Synopsys device was probably a bad idea.
> 
> dwc3-haps claims by Vendor & Device ID; it doesn't look at the Class
> Code.  What happens when it tries to claim the PCIe IP in i.MX?
> 
> lspci also doesn't look at the Class Code when it looks up the text
> name of devices, so it will print the same name for both.  Of course,
> the Linux PCI ID database at https://pci-ids.ucw.cz/read/PC/16c3
> shows
> basically no information for the 0x16c3 Vendor ID, so it won't print
> anything useful no matter what.  You could add some things there :)
> 
> > Fixes: 03e6742584af ("PCI: Override Synopsys USB 3.x HAPS device
> > class")
> > Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> > ---
> >  drivers/pci/quirks.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index b0a413f3f7ca..e2a879e93d86 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -639,8 +639,9 @@ static void quirk_synopsys_haps(struct pci_dev
> > *pdev)
> >  		break;
> >  	}
> >  }
> > -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
> > -			 quirk_synopsys_haps);
> > +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
> > +			       PCI_CLASS_SERIAL_USB_XHCI, 0,
> > +			       quirk_synopsys_haps);
> >  
> >  /*
> >   * Let's make the southbridge information explicit instead of
> > having to
> > -- 
> > 2.11.0
> > 

Re: [PATCH RESEND] PCI: Check for USB xHCI class for HAPS platform

[Bjorn Helgaas]

On Tue, Feb 05, 2019 at 01:04:28PM -0800, Thinh Nguyen wrote:
> The Synopsys HAPS USB controller has a VID PID (16c3,abcd) that matches
> to an existing PCIe controller. This quirk is intended for USB HAPS
> devices only. To fix this, check for the PCI class USB xHCI to prevent
> matching the PCIe controllers.
> 
> Fixes: 03e6742584af ("PCI: Override Synopsys USB 3.x HAPS device class")
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>

I applied this as below to for-linus for v5.0, thanks!

I *suspect* that this pending patch [1] would make the Root Ports work
correctly as bridges even if they had the wrong class code, e.g., if
we didn't have this fix to the quirk.

But the portdrv wouldn't claim the Root Ports, so PCIe services still
wouldn't work correctly, so we absolutely still need this patch.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?id=b2fb5cc57469

> ---
>  drivers/pci/quirks.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index b0a413f3f7ca..e2a879e93d86 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -639,8 +639,9 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
>  		break;
>  	}
>  }
> -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
> -			 quirk_synopsys_haps);
> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
> +			       PCI_CLASS_SERIAL_USB_XHCI, 0,
> +			       quirk_synopsys_haps);
>  
>  /*
>   * Let's make the southbridge information explicit instead of having to

commit f57a98e1b71357713e44c57268a53d9c803f0626
Author: Thinh Nguyen <thinh.nguyen@synopsys.com>
Date:   Wed Feb 6 17:17:27 2019 -0600

    PCI: Work around Synopsys duplicate Device ID (HAPS USB3, NXP i.MX)
    
    There are at least four different parts with the same Vendor and Device
    ID ([16c3:abcd]):
    
      1) Synopsys HAPS USB3 controller
      2) Synopsys PCIe Root Port in Freescale/NXP i.MX6Q (reported by Lucas)
      3) Synopsys PCIe Root Port in Freescale/NXP i.MX6QP (reported by Lukas)
      4) Synopsys PCIe Root Port in Freescale/NXP i.MX7D (reported by Trent)
    
    The HAPS USB3 controller has a Class Code of PCI_CLASS_SERIAL_USB_XHCI,
    which means the XHCI driver would normally claim it.  Previously,
    quirk_synopsys_haps() changed the Class Code of all [16c3:abcd] devices,
    including the Root Ports, to PCI_CLASS_SERIAL_USB_DEVICE to prevent the
    XHCI driver from claiming them so dwc3-haps can claim them instead.
    
    Changing the Class Code of the Root Ports prevents the PCI core from
    handling them as bridges, so devices below them don't work.
    
    Restrict the quirk so it only changes the Class Code for devices that start
    with the PCI_CLASS_SERIAL_USB_XHCI Class Code, leaving the Root Ports
    alone.
    
    Fixes: 03e6742584af ("PCI: Override Synopsys USB 3.x HAPS device class")
    Reported-by: Lukas F. Hartmann <lukas@mntmn.com>
    Reported-by: Trent Piepho <tpiepho@impinj.com>
    Reported-by: Lucas Stach <l.stach@pengutronix.de>
    Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
    [bhelgaas: changelog]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b0a413f3f7ca..e2a879e93d86 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -639,8 +639,9 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
 		break;
 	}
 }
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
-			 quirk_synopsys_haps);
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
+			       PCI_CLASS_SERIAL_USB_XHCI, 0,
+			       quirk_synopsys_haps);
 
 /*
  * Let's make the southbridge information explicit instead of having to