| Message ID | 1413536021-4886-3-git-send-email-ray.huang@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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. > 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". > 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? > + > +/* > + * 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)". Bjorn > + pdev->class = 0x0c03fe; > +} > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_NL, > + quirk_amd_nl_class); > + > /* > * Let's make the southbridge information explicit instead > * of having to worry about people probing the ACPI areas, > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 26, 2014 at 8:55 PM, Huang Rui <ray.huang@amd.com> wrote: > 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: >> > +/* 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. Just read the text at the beginning of pci_ids.h. If you have to use the same ID in two different files, you should add it to pci_ids.h. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 + +/* + * 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 */ + pdev->class = 0x0c03fe; +} +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_NL, + quirk_amd_nl_class); + /* * Let's make the southbridge information explicit instead * of having to worry about people probing the ACPI areas,
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. 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 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(+)