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

Huang Rui
2014-10-27T03:19:26Z

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