Re: [PATCH v2 02/16] pci: quirks: add quirk to avoid AMD NL to bind with xhci

[Huang Rui <ray.huang@amd.com>]

On Fri, Oct 24, 2014 at 10:35:29AM -0600, Bjorn Helgaas wrote:
> On Fri, Oct 17, 2014 at 04:53:27PM +0800, Huang Rui wrote:
> > The dwc3 controller is the PCI-E device in AMD NL platform, but the class code
> > of PCI header is 0x0c0330, the same with xHC. That's because it needs to meet
> > the windows enviroment. The dwc3 controller acted as only host mode to bind with
> > windows xhci driver.
> > But on linux, sometimes, it would auto-bind with xhci driver as well (class code
> > 0x0c0330) despite it uses Pid/Vid on dwc3 driver.
> 
> This changelog really doesn't make any sense unless the reader already
> knows everything.  I think you mean something like this:
> 
>   The AMD NL (please explain what "NL" is; Google has no clue) SoC contains
>   a DesignWare USB3 Dual-Role Device that can be operated either as a USB
>   Host or a USB Device.  In the AMD NL platform, this device ([1022:7912])
>   has a class code of PCI_CLASS_SERIAL_USB_XHCI (0x0c0330), which means the
>   xhci driver will claim it.
> 
>   But the dwc3 driver is a more specific driver for this device, and we'd
>   prefer to use it instead of xhci.  To prevent xhci from claiming the
>   device, change the class code to 0x0c03fe, which the PCI r3.0 spec
>   defines as "USB device (not host controller)".  The dwc3 driver can then
>   claim it based on its Vendor and Device ID.
> 
> Obviously, this means the device won't work at all unless dwc3 is enabled.
> Previously, it probably would work as a host controller with the xhci
> driver.  I assume this change is what you want.
> 

OK, I will update to this changelog in V3.

> > Heikki suggested to use the quirk to fix this issue, and the detailed discussion
> > is at below thread:
> > http://marc.info/?l=linux-usb&m=141197934712824&w=2
> 
> This changelog needs to be wrapped so no line is longer than 75 characters.
> 
> Please run "git log --oneline --no-merges drivers/pci/quirks.c" and make
> your subject line match the rest.  In particular, "PCI" (not "pci"), drop
> the "quirks:" part, and capitalize "Add".
> 

I got it, will update subject.

> > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >  drivers/pci/quirks.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 90acb32..3c911b7 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -378,6 +378,26 @@ static void quirk_ati_exploding_mce(struct pci_dev *dev)
> >  }
> >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI,	PCI_DEVICE_ID_ATI_RS100,   quirk_ati_exploding_mce);
> >  
> > +/* FIXME should define in <linux/pci_ids.h> */
> > +#define PCI_DEVICE_ID_AMD_NL	0x7912
> 
> If you KNOW this should be in linux/pci_ids.h (and it looks like it COULD
> go there, since you do use the same definition in patch [01/16]), why don't
> you just do it?  Why are you wasting our time reviewing trivial stuff like
> this?
> 

Apology, I was told that it could not add device id into pci_ids.h
from my buddy with below thread discussion last year, but I don't
confirm at current. I check the git log at pci_ids.h, it looks like
it's able to add device id marco again, is that right?

http://marc.info/?l=linux-pci&m=137028614924258&w=2

If I misunderstand your meaning, please correct me.

> > +
> > +/*
> > + * AMD NL SoC 7912 PCI device is dwc3 controller, but the class code of PCI
> > + * header is 0x0c0330, the same with xHC. That's because it need to meet the
> > + * windows enviroment. The dwc3 controller acted as only host mode to bind with
> > + * windows xhci driver. But on linux, sometimes, we auto-bind with xhci driver
> > + * as well (class code 0x0c0330) despite we use Pid/Vid on dwc3 driver. So this
> > + * quirk used a dummy class code to avoid to bind with xhci driver at booting
> > + * phase.
> > + */
> > +static void quirk_amd_nl_class(struct pci_dev *pdev)
> > +{
> > +	/* Use a dummy class value instead of PCI header provided */
> 
> When you say "dummy class value," that makes it sound like you just chose
> a random invalid value.  But this value is actually defined by the spec, so
> you should use the description from the spec, namely, "USB device (not host
> controller)".
> 

Yes, you're right. Will change this description.

Thanks,
Rui