| Message ID | 20230518092240.8023-1-zhuyinbo@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] usb: dwc2: add pci_device_id driver_data parse support | expand |
On Thu, May 18, 2023 at 05:22:40PM +0800, Yinbo Zhu wrote: > The dwc2 driver has everything we need to run in PCI mode except > for pci_device_id driver_data parse. With that to set Loongson > dwc2 element and added identified as PCI_VENDOR_ID_LOONGSON > and PCI_DEVICE_ID_LOONGSON_DWC2 in dwc2_pci_ids, the Loongson > dwc2 controller will work. > > Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn> > --- > drivers/usb/dwc2/core.h | 1 + > drivers/usb/dwc2/params.c | 33 +++++++++++++++++++++++++++++++-- > drivers/usb/dwc2/pci.c | 14 +------------- > include/linux/pci_ids.h | 2 ++ > 4 files changed, 35 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > index 0bb4c0c845bf..c92a1da46a01 100644 > --- a/drivers/usb/dwc2/core.h > +++ b/drivers/usb/dwc2/core.h > @@ -1330,6 +1330,7 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev); > /* The device ID match table */ > extern const struct of_device_id dwc2_of_match_table[]; > extern const struct acpi_device_id dwc2_acpi_match[]; > +extern const struct pci_device_id dwc2_pci_ids[]; > > int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg); > int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg); > diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c > index 21d16533bd2f..f7550d293c2d 100644 > --- a/drivers/usb/dwc2/params.c > +++ b/drivers/usb/dwc2/params.c > @@ -7,6 +7,8 @@ > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/usb/of.h> > +#include <linux/pci_ids.h> > +#include <linux/pci.h> > > #include "core.h" > > @@ -55,6 +57,14 @@ static void dwc2_set_jz4775_params(struct dwc2_hsotg *hsotg) > !device_property_read_bool(hsotg->dev, "disable-over-current"); > } > > +static void dwc2_set_loongson_params(struct dwc2_hsotg *hsotg) > +{ > + struct dwc2_core_params *p = &hsotg->params; > + > + p->phy_utmi_width = 8; > + p->power_down = DWC2_POWER_DOWN_PARAM_NONE; > +} > + > static void dwc2_set_x1600_params(struct dwc2_hsotg *hsotg) > { > struct dwc2_core_params *p = &hsotg->params; > @@ -281,6 +291,22 @@ const struct acpi_device_id dwc2_acpi_match[] = { > }; > MODULE_DEVICE_TABLE(acpi, dwc2_acpi_match); > > +const struct pci_device_id dwc2_pci_ids[] = { > + { > + PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG), > + }, > + { > + PCI_DEVICE(PCI_VENDOR_ID_STMICRO, > + PCI_DEVICE_ID_STMICRO_USB_OTG), > + }, > + { > + PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_DWC2), > + .driver_data = (unsigned long)dwc2_set_loongson_params, > + }, > + { /* end: all zeroes */ } > +}; > +MODULE_DEVICE_TABLE(pci, dwc2_pci_ids); > + > static void dwc2_set_param_otg_cap(struct dwc2_hsotg *hsotg) > { > switch (hsotg->hw_params.op_mode) { > @@ -929,10 +955,13 @@ int dwc2_init_params(struct dwc2_hsotg *hsotg) > set_params(hsotg); > } else { > const struct acpi_device_id *amatch; > + const struct pci_device_id *pmatch; > > amatch = acpi_match_device(dwc2_acpi_match, hsotg->dev); > - if (amatch && amatch->driver_data) { > - set_params = (set_params_cb)amatch->driver_data; > + pmatch = pci_match_id(dwc2_pci_ids, to_pci_dev(hsotg->dev->parent)); Ick, this means this is not a "real" PCI driver, right? Why not? Please tie into the PCI device probe call, don't walk all PCI devices like this. How are you _sure_ that the parent is really a PCI device? That is very very fragile and will break. Do this properly instead. > + > + if ((amatch && amatch->driver_data) || (pmatch && pmatch->driver_data)) { > + set_params = (set_params_cb)pmatch->driver_data; > set_params(hsotg); > } > } > diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c > index b7306ed8be4c..f3a1e4232a31 100644 > --- a/drivers/usb/dwc2/pci.c > +++ b/drivers/usb/dwc2/pci.c > @@ -24,7 +24,7 @@ > #include <linux/platform_device.h> > #include <linux/usb/usb_phy_generic.h> > > -#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0 > +#include "core.h" > > static const char dwc2_driver_name[] = "dwc2-pci"; > > @@ -122,18 +122,6 @@ static int dwc2_pci_probe(struct pci_dev *pci, > return ret; > } > > -static const struct pci_device_id dwc2_pci_ids[] = { > - { > - PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG), > - }, > - { > - PCI_DEVICE(PCI_VENDOR_ID_STMICRO, > - PCI_DEVICE_ID_STMICRO_USB_OTG), > - }, > - { /* end: all zeroes */ } > -}; > -MODULE_DEVICE_TABLE(pci, dwc2_pci_ids); > - > static struct pci_driver dwc2_pci_driver = { > .name = dwc2_driver_name, > .id_table = dwc2_pci_ids, > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index e43ab203054a..6481f648695a 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -157,6 +157,7 @@ > #define PCI_VENDOR_ID_PCI_SIG 0x0001 > > #define PCI_VENDOR_ID_LOONGSON 0x0014 > +#define PCI_DEVICE_ID_LOONGSON_DWC2 0x7a04 > > #define PCI_VENDOR_ID_SOLIDIGM 0x025e > > @@ -2356,6 +2357,7 @@ > #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI 0xabce > #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB31 0xabcf > #define PCI_DEVICE_ID_SYNOPSYS_EDDA 0xedda > +#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0 Please read the top of this file for why you should not add new ids here. thanks, greg k-h
在 2023/5/19 上午1:52, kernel test robot 写道: > Hi Yinbo, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on pci/next] > [also build test ERROR on pci/for-linus westeri-thunderbolt/next linus/master v6.4-rc2 next-20230518] > [cannot apply to usb/usb-testing usb/usb-next usb/usb-linus] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Yinbo-Zhu/usb-dwc2-add-pci_device_id-driver_data-parse-support/20230518-173721 > base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next > patch link: https://lore.kernel.org/r/20230518092240.8023-1-zhuyinbo%40loongson.cn > patch subject: [PATCH v1] usb: dwc2: add pci_device_id driver_data parse support > config: powerpc-allmodconfig > compiler: powerpc-linux-gcc (GCC) 12.1.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/intel-lab-lkp/linux/commit/3ff56448e1442fe8b1e72651a8d4d6e1086ece32 > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Yinbo-Zhu/usb-dwc2-add-pci_device_id-driver_data-parse-support/20230518-173721 > git checkout 3ff56448e1442fe8b1e72651a8d4d6e1086ece32 > # save the config file > mkdir build_dir && cp config build_dir/.config > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc olddefconfig > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202305190105.O6ycxCti-lkp@intel.com/ > > All errors (new ones prefixed by >>, old ones prefixed by <<): > >>> ERROR: modpost: "dwc2_pci_ids" [drivers/usb/dwc2/dwc2_pci.ko] undefined! I test it was set dwc2 pci driver as built-in, so no error, this compile error was that dwc2_pci_ids not export when driver as module and I will add EXPORT_SYMBOL_GPL(dwc2_pci_ids) to fix that compile issue. Thanks. >
On Fri, May 19, 2023 at 03:13:20PM +0800, zhuyinbo wrote: > > > 在 2023/5/19 上午1:52, kernel test robot 写道: > > Hi Yinbo, > > > > kernel test robot noticed the following build errors: > > > > [auto build test ERROR on pci/next] > > [also build test ERROR on pci/for-linus westeri-thunderbolt/next linus/master v6.4-rc2 next-20230518] > > [cannot apply to usb/usb-testing usb/usb-next usb/usb-linus] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use '--base' as documented in > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > url: https://github.com/intel-lab-lkp/linux/commits/Yinbo-Zhu/usb-dwc2-add-pci_device_id-driver_data-parse-support/20230518-173721 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next > > patch link: https://lore.kernel.org/r/20230518092240.8023-1-zhuyinbo%40loongson.cn > > patch subject: [PATCH v1] usb: dwc2: add pci_device_id driver_data parse support > > config: powerpc-allmodconfig > > compiler: powerpc-linux-gcc (GCC) 12.1.0 > > reproduce (this is a W=1 build): > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # https://github.com/intel-lab-lkp/linux/commit/3ff56448e1442fe8b1e72651a8d4d6e1086ece32 > > git remote add linux-review https://github.com/intel-lab-lkp/linux > > git fetch --no-tags linux-review Yinbo-Zhu/usb-dwc2-add-pci_device_id-driver_data-parse-support/20230518-173721 > > git checkout 3ff56448e1442fe8b1e72651a8d4d6e1086ece32 > > # save the config file > > mkdir build_dir && cp config build_dir/.config > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc olddefconfig > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash > > > > If you fix the issue, kindly add following tag where applicable > > | Reported-by: kernel test robot <lkp@intel.com> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202305190105.O6ycxCti-lkp@intel.com/ > > > > All errors (new ones prefixed by >>, old ones prefixed by <<): > > > > > > ERROR: modpost: "dwc2_pci_ids" [drivers/usb/dwc2/dwc2_pci.ko] undefined! > > > I test it was set dwc2 pci driver as built-in, so no error, this compile > error was that dwc2_pci_ids not export when driver as module and I will > add EXPORT_SYMBOL_GPL(dwc2_pci_ids) to fix that compile issue. Again, no, please do this properly, no one should ever be walking a pci id list by hand like this... thanks, greg k-h
Hi greg k-h, I'm sorry for giving you feedback so late, for your suggestion that I have a some analysis. 2023 May 18, 2023 at 6:32 PM, Greg Kroah-Hartman wrote: > On Thu, May 18, 2023 at 05:22:40PM +0800, Yinbo Zhu wrote: >> The dwc2 driver has everything we need to run in PCI mode except >> for pci_device_id driver_data parse. With that to set Loongson >> dwc2 element and added identified as PCI_VENDOR_ID_LOONGSON >> and PCI_DEVICE_ID_LOONGSON_DWC2 in dwc2_pci_ids, the Loongson >> dwc2 controller will work. >> >> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn> >> --- >> drivers/usb/dwc2/core.h | 1 + >> drivers/usb/dwc2/params.c | 33 +++++++++++++++++++++++++++++++-- >> drivers/usb/dwc2/pci.c | 14 +------------- >> include/linux/pci_ids.h | 2 ++ >> 4 files changed, 35 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >> index 0bb4c0c845bf..c92a1da46a01 100644 >> --- a/drivers/usb/dwc2/core.h >> +++ b/drivers/usb/dwc2/core.h >> @@ -1330,6 +1330,7 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev); >> /* The device ID match table */ >> extern const struct of_device_id dwc2_of_match_table[]; >> extern const struct acpi_device_id dwc2_acpi_match[]; >> +extern const struct pci_device_id dwc2_pci_ids[]; >> >> int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg); >> int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg); >> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c >> index 21d16533bd2f..f7550d293c2d 100644 >> --- a/drivers/usb/dwc2/params.c >> +++ b/drivers/usb/dwc2/params.c >> @@ -7,6 +7,8 @@ >> #include <linux/module.h> >> #include <linux/of_device.h> >> #include <linux/usb/of.h> >> +#include <linux/pci_ids.h> >> +#include <linux/pci.h> >> >> #include "core.h" >> >> @@ -55,6 +57,14 @@ static void dwc2_set_jz4775_params(struct dwc2_hsotg *hsotg) >> !device_property_read_bool(hsotg->dev, "disable-over-current"); >> } >> >> +static void dwc2_set_loongson_params(struct dwc2_hsotg *hsotg) >> +{ >> + struct dwc2_core_params *p = &hsotg->params; >> + >> + p->phy_utmi_width = 8; >> + p->power_down = DWC2_POWER_DOWN_PARAM_NONE; >> +} >> + >> static void dwc2_set_x1600_params(struct dwc2_hsotg *hsotg) >> { >> struct dwc2_core_params *p = &hsotg->params; >> @@ -281,6 +291,22 @@ const struct acpi_device_id dwc2_acpi_match[] = { >> }; >> MODULE_DEVICE_TABLE(acpi, dwc2_acpi_match); >> >> +const struct pci_device_id dwc2_pci_ids[] = { >> + { >> + PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG), >> + }, >> + { >> + PCI_DEVICE(PCI_VENDOR_ID_STMICRO, >> + PCI_DEVICE_ID_STMICRO_USB_OTG), >> + }, >> + { >> + PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_DWC2), >> + .driver_data = (unsigned long)dwc2_set_loongson_params, >> + }, >> + { /* end: all zeroes */ } >> +}; >> +MODULE_DEVICE_TABLE(pci, dwc2_pci_ids); >> + >> static void dwc2_set_param_otg_cap(struct dwc2_hsotg *hsotg) >> { >> switch (hsotg->hw_params.op_mode) { >> @@ -929,10 +955,13 @@ int dwc2_init_params(struct dwc2_hsotg *hsotg) >> set_params(hsotg); >> } else { >> const struct acpi_device_id *amatch; >> + const struct pci_device_id *pmatch; >> >> amatch = acpi_match_device(dwc2_acpi_match, hsotg->dev); >> - if (amatch && amatch->driver_data) { >> - set_params = (set_params_cb)amatch->driver_data; >> + pmatch = pci_match_id(dwc2_pci_ids, to_pci_dev(hsotg->dev->parent)); > > Ick, this means this is not a "real" PCI driver, right? Why not? The params.c and platform.c was a part of pci dwc2 device driver. This pci.c was only play a role that register device resource but not operate dwc2 hardware. in other words, the params.c seems unrelated to the device type. Whether this device is a PCI device, platform device, or PCI device, it is best to use params.c for operational dwc2 elements. Failure to do so seems to break the original design. > > Please tie into the PCI device probe call, don't walk all PCI devices > like this. I learn about that you strongly disagree with using pci_match_id, May I ask you the reason ? actually, I use it was due to I noticed that xhci-pci.c, ehci-pci.c and ohci-pci.c was all use it. and I don't use it in dwc2/pci.c was considering set dwc2 element need dpend on elements.c and platform.c, and usb driver (ohci,echi,xhci) was a relatively indepent device driver when to operate usb controler. but dwc2 was not. If I fource the element setting of dwc2 element in dwc2/pci.c. It will be following case. This will cause problems with element-initial function or element-check function. 1. initial dwc2 element. 2. check the setting of dwc2 element whether was suitable 3. set dwc2 element. or 1. set dwc2 element. 2. initial dwc2 element. 3. check the setting of dwc2 element whether was suitable The corresponding code call process as follows: 1. dwc2_set_default_params(hsotg); 2. dwc2_get_device_properties(hsotg); 3. dwc2_check_params(hsotg); 4. dwc2_set_loongson_params; or 1. dwc2_set_loongson_params; 2. dwc2_set_default_params(hsotg); 3. dwc2_get_device_properties(hsotg); 4. dwc2_check_params(hsotg); But the platform dwc2 device or acpi dwc2 device was all following case and It seems was correct order. 1. dwc2_set_default_params(hsotg); 2. dwc2_get_device_properties(hsotg); 3. dwc2_set_loongson_params; 4. dwc2_check_params(hsotg); > > How are you _sure_ that the parent is really a PCI device? That is very > very fragile and will break. > > Do this properly instead. Thank you for your reminder. There was indeed an issue with my previous code, and the modified code is as follows, then it seems to ensure that device is a PCI device. @@ -927,13 +954,20 @@ int dwc2_init_params(struct dwc2_hsotg *hsotg) if (match && match->data) { set_params = match->data; set_params(hsotg); - } else { + } else if (!match) { const struct acpi_device_id *amatch; + const struct pci_device_id *pmatch = NULL; amatch = acpi_match_device(dwc2_acpi_match, hsotg->dev); if (amatch && amatch->driver_data) { set_params = (set_params_cb)amatch->driver_data; set_params(hsotg); + } else if (!amatch) + pmatch = pci_match_id(dwc2_pci_ids, to_pci_dev(hsotg->dev->parent)); + + if (pmatch && pmatch->driver_data) { + set_params = (set_params_cb)pmatch->driver_data; + set_params(hsotg); } > > >> + >> + if ((amatch && amatch->driver_data) || (pmatch && pmatch->driver_data)) { >> + set_params = (set_params_cb)pmatch->driver_data; >> set_params(hsotg); >> } >> } >> diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c >> index b7306ed8be4c..f3a1e4232a31 100644 >> --- a/drivers/usb/dwc2/pci.c >> +++ b/drivers/usb/dwc2/pci.c >> @@ -24,7 +24,7 @@ >> #include <linux/platform_device.h> >> #include <linux/usb/usb_phy_generic.h> >> >> -#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0 >> +#include "core.h" >> >> static const char dwc2_driver_name[] = "dwc2-pci"; >> >> @@ -122,18 +122,6 @@ static int dwc2_pci_probe(struct pci_dev *pci, >> return ret; >> } >> >> -static const struct pci_device_id dwc2_pci_ids[] = { >> - { >> - PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG), >> - }, >> - { >> - PCI_DEVICE(PCI_VENDOR_ID_STMICRO, >> - PCI_DEVICE_ID_STMICRO_USB_OTG), >> - }, >> - { /* end: all zeroes */ } >> -}; >> -MODULE_DEVICE_TABLE(pci, dwc2_pci_ids); >> - >> static struct pci_driver dwc2_pci_driver = { >> .name = dwc2_driver_name, >> .id_table = dwc2_pci_ids, >> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h >> index e43ab203054a..6481f648695a 100644 >> --- a/include/linux/pci_ids.h >> +++ b/include/linux/pci_ids.h >> @@ -157,6 +157,7 @@ >> #define PCI_VENDOR_ID_PCI_SIG 0x0001 >> >> #define PCI_VENDOR_ID_LOONGSON 0x0014 >> +#define PCI_DEVICE_ID_LOONGSON_DWC2 0x7a04 >> >> #define PCI_VENDOR_ID_SOLIDIGM 0x025e >> >> @@ -2356,6 +2357,7 @@ >> #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI 0xabce >> #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB31 0xabcf >> #define PCI_DEVICE_ID_SYNOPSYS_EDDA 0xedda >> +#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0 > > Please read the top of this file for why you should not add new ids > here. okay, I will remove it. Thanks! Yinbo.
在 2023/5/19 下午5:49, Greg Kroah-Hartman 写道: > On Fri, May 19, 2023 at 03:13:20PM +0800, zhuyinbo wrote: >> >> >> 在 2023/5/19 上午1:52, kernel test robot 写道: >>> Hi Yinbo, >>> >>> kernel test robot noticed the following build errors: >>> >>> [auto build test ERROR on pci/next] >>> [also build test ERROR on pci/for-linus westeri-thunderbolt/next linus/master v6.4-rc2 next-20230518] >>> [cannot apply to usb/usb-testing usb/usb-next usb/usb-linus] >>> [If your patch is applied to the wrong git tree, kindly drop us a note. >>> And when submitting patch, we suggest to use '--base' as documented in >>> https://git-scm.com/docs/git-format-patch#_base_tree_information] >>> >>> url: https://github.com/intel-lab-lkp/linux/commits/Yinbo-Zhu/usb-dwc2-add-pci_device_id-driver_data-parse-support/20230518-173721 >>> base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next >>> patch link: https://lore.kernel.org/r/20230518092240.8023-1-zhuyinbo%40loongson.cn >>> patch subject: [PATCH v1] usb: dwc2: add pci_device_id driver_data parse support >>> config: powerpc-allmodconfig >>> compiler: powerpc-linux-gcc (GCC) 12.1.0 >>> reproduce (this is a W=1 build): >>> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >>> chmod +x ~/bin/make.cross >>> # https://github.com/intel-lab-lkp/linux/commit/3ff56448e1442fe8b1e72651a8d4d6e1086ece32 >>> git remote add linux-review https://github.com/intel-lab-lkp/linux >>> git fetch --no-tags linux-review Yinbo-Zhu/usb-dwc2-add-pci_device_id-driver_data-parse-support/20230518-173721 >>> git checkout 3ff56448e1442fe8b1e72651a8d4d6e1086ece32 >>> # save the config file >>> mkdir build_dir && cp config build_dir/.config >>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc olddefconfig >>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash >>> >>> If you fix the issue, kindly add following tag where applicable >>> | Reported-by: kernel test robot <lkp@intel.com> >>> | Closes: https://lore.kernel.org/oe-kbuild-all/202305190105.O6ycxCti-lkp@intel.com/ >>> >>> All errors (new ones prefixed by >>, old ones prefixed by <<): >>> >>>>> ERROR: modpost: "dwc2_pci_ids" [drivers/usb/dwc2/dwc2_pci.ko] undefined! >> >> >> I test it was set dwc2 pci driver as built-in, so no error, this compile >> error was that dwc2_pci_ids not export when driver as module and I will >> add EXPORT_SYMBOL_GPL(dwc2_pci_ids) to fix that compile issue. > > Again, no, please do this properly, no one should ever be walking a pci > id list by hand like this... okay, I got it. But I don't seem to have found a good way to set dwc2 elements yet for pci device, in addition, I have some alalysis in another mail loop. please you check in your free time. Thanks! Yinbo.
Friendly ping ? 在 2023/5/20 上午11:24, zhuyinbo 写道: > > Hi greg k-h, > > I'm sorry for giving you feedback so late, for your suggestion that I > have a some analysis. > > 2023 May 18, 2023 at 6:32 PM, Greg Kroah-Hartman wrote: >> On Thu, May 18, 2023 at 05:22:40PM +0800, Yinbo Zhu wrote: >>> The dwc2 driver has everything we need to run in PCI mode except >>> for pci_device_id driver_data parse. With that to set Loongson >>> dwc2 element and added identified as PCI_VENDOR_ID_LOONGSON >>> and PCI_DEVICE_ID_LOONGSON_DWC2 in dwc2_pci_ids, the Loongson >>> dwc2 controller will work. >>> >>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn> >>> --- >>> drivers/usb/dwc2/core.h | 1 + >>> drivers/usb/dwc2/params.c | 33 +++++++++++++++++++++++++++++++-- >>> drivers/usb/dwc2/pci.c | 14 +------------- >>> include/linux/pci_ids.h | 2 ++ >>> 4 files changed, 35 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >>> index 0bb4c0c845bf..c92a1da46a01 100644 >>> --- a/drivers/usb/dwc2/core.h >>> +++ b/drivers/usb/dwc2/core.h >>> @@ -1330,6 +1330,7 @@ irqreturn_t dwc2_handle_common_intr(int irq, >>> void *dev); >>> /* The device ID match table */ >>> extern const struct of_device_id dwc2_of_match_table[]; >>> extern const struct acpi_device_id dwc2_acpi_match[]; >>> +extern const struct pci_device_id dwc2_pci_ids[]; >>> int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg); >>> int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg); >>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c >>> index 21d16533bd2f..f7550d293c2d 100644 >>> --- a/drivers/usb/dwc2/params.c >>> +++ b/drivers/usb/dwc2/params.c >>> @@ -7,6 +7,8 @@ >>> #include <linux/module.h> >>> #include <linux/of_device.h> >>> #include <linux/usb/of.h> >>> +#include <linux/pci_ids.h> >>> +#include <linux/pci.h> >>> #include "core.h" >>> @@ -55,6 +57,14 @@ static void dwc2_set_jz4775_params(struct >>> dwc2_hsotg *hsotg) >>> !device_property_read_bool(hsotg->dev, >>> "disable-over-current"); >>> } >>> +static void dwc2_set_loongson_params(struct dwc2_hsotg *hsotg) >>> +{ >>> + struct dwc2_core_params *p = &hsotg->params; >>> + >>> + p->phy_utmi_width = 8; >>> + p->power_down = DWC2_POWER_DOWN_PARAM_NONE; >>> +} >>> + >>> static void dwc2_set_x1600_params(struct dwc2_hsotg *hsotg) >>> { >>> struct dwc2_core_params *p = &hsotg->params; >>> @@ -281,6 +291,22 @@ const struct acpi_device_id dwc2_acpi_match[] = { >>> }; >>> MODULE_DEVICE_TABLE(acpi, dwc2_acpi_match); >>> +const struct pci_device_id dwc2_pci_ids[] = { >>> + { >>> + PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG), >>> + }, >>> + { >>> + PCI_DEVICE(PCI_VENDOR_ID_STMICRO, >>> + PCI_DEVICE_ID_STMICRO_USB_OTG), >>> + }, >>> + { >>> + PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, >>> PCI_DEVICE_ID_LOONGSON_DWC2), >>> + .driver_data = (unsigned long)dwc2_set_loongson_params, >>> + }, >>> + { /* end: all zeroes */ } >>> +}; >>> +MODULE_DEVICE_TABLE(pci, dwc2_pci_ids); >>> + >>> static void dwc2_set_param_otg_cap(struct dwc2_hsotg *hsotg) >>> { >>> switch (hsotg->hw_params.op_mode) { >>> @@ -929,10 +955,13 @@ int dwc2_init_params(struct dwc2_hsotg *hsotg) >>> set_params(hsotg); >>> } else { >>> const struct acpi_device_id *amatch; >>> + const struct pci_device_id *pmatch; >>> amatch = acpi_match_device(dwc2_acpi_match, hsotg->dev); >>> - if (amatch && amatch->driver_data) { >>> - set_params = (set_params_cb)amatch->driver_data; >>> + pmatch = pci_match_id(dwc2_pci_ids, >>> to_pci_dev(hsotg->dev->parent)); >> >> Ick, this means this is not a "real" PCI driver, right? Why not? > > > The params.c and platform.c was a part of pci dwc2 device driver. This > pci.c was only play a role that register device resource but not operate > dwc2 hardware. in other words, the params.c seems unrelated to the > device type. Whether this device is a PCI device, platform device, or > PCI device, it is best to use params.c for operational dwc2 elements. > Failure to do so seems to break the original design. > >> >> Please tie into the PCI device probe call, don't walk all PCI devices >> like this. > > > I learn about that you strongly disagree with using pci_match_id, May > I ask you the reason ? actually, I use it was due to I noticed that > xhci-pci.c, ehci-pci.c and ohci-pci.c was all use it. and I don't use it > in dwc2/pci.c was considering set dwc2 element need dpend on elements.c > and platform.c, and usb driver (ohci,echi,xhci) was a relatively > indepent device driver when to operate usb controler. but dwc2 was not. > > If I fource the element setting of dwc2 element in dwc2/pci.c. It will > be following case. This will cause problems with element-initial > function or element-check function. > > 1. initial dwc2 element. > 2. check the setting of dwc2 element whether was suitable > 3. set dwc2 element. > > or > > 1. set dwc2 element. > 2. initial dwc2 element. > 3. check the setting of dwc2 element whether was suitable > > The corresponding code call process as follows: > > 1. dwc2_set_default_params(hsotg); > 2. dwc2_get_device_properties(hsotg); > 3. dwc2_check_params(hsotg); > 4. dwc2_set_loongson_params; > > or > > 1. dwc2_set_loongson_params; > 2. dwc2_set_default_params(hsotg); > 3. dwc2_get_device_properties(hsotg); > 4. dwc2_check_params(hsotg); > > But the platform dwc2 device or acpi dwc2 device was all following case > and It seems was correct order. > > 1. dwc2_set_default_params(hsotg); > 2. dwc2_get_device_properties(hsotg); > 3. dwc2_set_loongson_params; > 4. dwc2_check_params(hsotg); > >> >> How are you _sure_ that the parent is really a PCI device? That is very >> very fragile and will break. >> >> Do this properly instead. > > > Thank you for your reminder. There was indeed an issue with my previous > code, and the modified code is as follows, then it seems to ensure that > device is a PCI device. > > @@ -927,13 +954,20 @@ int dwc2_init_params(struct dwc2_hsotg *hsotg) > if (match && match->data) { > set_params = match->data; > set_params(hsotg); > - } else { > + } else if (!match) { > const struct acpi_device_id *amatch; > + const struct pci_device_id *pmatch = NULL; > > amatch = acpi_match_device(dwc2_acpi_match, hsotg->dev); > if (amatch && amatch->driver_data) { > set_params = (set_params_cb)amatch->driver_data; > set_params(hsotg); > + } else if (!amatch) > + pmatch = pci_match_id(dwc2_pci_ids, > to_pci_dev(hsotg->dev->parent)); > + > + if (pmatch && pmatch->driver_data) { > + set_params = (set_params_cb)pmatch->driver_data; > + set_params(hsotg); > } > >> >> >>> + >>> + if ((amatch && amatch->driver_data) || (pmatch && >>> pmatch->driver_data)) { >>> + set_params = (set_params_cb)pmatch->driver_data; >>> set_params(hsotg); >>> } >>> } >>> diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c >>> index b7306ed8be4c..f3a1e4232a31 100644 >>> --- a/drivers/usb/dwc2/pci.c >>> +++ b/drivers/usb/dwc2/pci.c >>> @@ -24,7 +24,7 @@ >>> #include <linux/platform_device.h> >>> #include <linux/usb/usb_phy_generic.h> >>> -#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0 >>> +#include "core.h" >>> static const char dwc2_driver_name[] = "dwc2-pci"; >>> @@ -122,18 +122,6 @@ static int dwc2_pci_probe(struct pci_dev *pci, >>> return ret; >>> } >>> -static const struct pci_device_id dwc2_pci_ids[] = { >>> - { >>> - PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG), >>> - }, >>> - { >>> - PCI_DEVICE(PCI_VENDOR_ID_STMICRO, >>> - PCI_DEVICE_ID_STMICRO_USB_OTG), >>> - }, >>> - { /* end: all zeroes */ } >>> -}; >>> -MODULE_DEVICE_TABLE(pci, dwc2_pci_ids); >>> - >>> static struct pci_driver dwc2_pci_driver = { >>> .name = dwc2_driver_name, >>> .id_table = dwc2_pci_ids, >>> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h >>> index e43ab203054a..6481f648695a 100644 >>> --- a/include/linux/pci_ids.h >>> +++ b/include/linux/pci_ids.h >>> @@ -157,6 +157,7 @@ >>> #define PCI_VENDOR_ID_PCI_SIG 0x0001 >>> #define PCI_VENDOR_ID_LOONGSON 0x0014 >>> +#define PCI_DEVICE_ID_LOONGSON_DWC2 0x7a04 >>> #define PCI_VENDOR_ID_SOLIDIGM 0x025e >>> @@ -2356,6 +2357,7 @@ >>> #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI 0xabce >>> #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB31 0xabcf >>> #define PCI_DEVICE_ID_SYNOPSYS_EDDA 0xedda >>> +#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0 >> >> Please read the top of this file for why you should not add new ids >> here. > > okay, I will remove it. > > Thanks! > Yinbo.
On Thu, Jun 08, 2023 at 03:54:36PM +0800, zhuyinbo wrote: > > Friendly ping ? For what? I thought a new series was going to be submitted, I don't have anything in my queue to review for this at the moment. thanks, greg k-h
在 2023/6/8 下午4:01, Greg Kroah-Hartman 写道: > On Thu, Jun 08, 2023 at 03:54:36PM +0800, zhuyinbo wrote: >> >> Friendly ping ? > > For what? I thought a new series was going to be submitted, I don't > have anything in my queue to review for this at the moment. I have some comments that need your consent or opinion. such as, I have a explanation why I using pci_match_id() in dwc2/params.c. Do you agree with my explanation ? Thanks, Yinbo
On Thu, Jun 08, 2023 at 04:57:04PM +0800, zhuyinbo wrote: > > > 在 2023/6/8 下午4:01, Greg Kroah-Hartman 写道: > > On Thu, Jun 08, 2023 at 03:54:36PM +0800, zhuyinbo wrote: > > > > > > Friendly ping ? > > > > For what? I thought a new series was going to be submitted, I don't > > have anything in my queue to review for this at the moment. > > > I have some comments that need your consent or opinion. > such as, I have a explanation why I using pci_match_id() in > dwc2/params.c. Do you agree with my explanation ? I have no context here, sorry. Remember, we review hundreds of patches a week, when you cut off all context that makes it impossible to remember. Submit your updated patchset based on what you feel is correct and we can review it from there. thanks, greg k-h
在 2023/6/8 下午5:11, Greg Kroah-Hartman 写道: > On Thu, Jun 08, 2023 at 04:57:04PM +0800, zhuyinbo wrote: >> >> >> 在 2023/6/8 下午4:01, Greg Kroah-Hartman 写道: >>> On Thu, Jun 08, 2023 at 03:54:36PM +0800, zhuyinbo wrote: >>>> >>>> Friendly ping ? >>> >>> For what? I thought a new series was going to be submitted, I don't >>> have anything in my queue to review for this at the moment. >> >> >> I have some comments that need your consent or opinion. >> such as, I have a explanation why I using pci_match_id() in >> dwc2/params.c. Do you agree with my explanation ? > > I have no context here, sorry. Remember, we review hundreds of patches > a week, when you cut off all context that makes it impossible to > remember. > > Submit your updated patchset based on what you feel is correct and we > can review it from there. okay, I got it. Thanks, Yinbo
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 0bb4c0c845bf..c92a1da46a01 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -1330,6 +1330,7 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev); /* The device ID match table */ extern const struct of_device_id dwc2_of_match_table[]; extern const struct acpi_device_id dwc2_acpi_match[]; +extern const struct pci_device_id dwc2_pci_ids[]; int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg); int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg); diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c index 21d16533bd2f..f7550d293c2d 100644 --- a/drivers/usb/dwc2/params.c +++ b/drivers/usb/dwc2/params.c @@ -7,6 +7,8 @@ #include <linux/module.h> #include <linux/of_device.h> #include <linux/usb/of.h> +#include <linux/pci_ids.h> +#include <linux/pci.h> #include "core.h" @@ -55,6 +57,14 @@ static void dwc2_set_jz4775_params(struct dwc2_hsotg *hsotg) !device_property_read_bool(hsotg->dev, "disable-over-current"); } +static void dwc2_set_loongson_params(struct dwc2_hsotg *hsotg) +{ + struct dwc2_core_params *p = &hsotg->params; + + p->phy_utmi_width = 8; + p->power_down = DWC2_POWER_DOWN_PARAM_NONE; +} + static void dwc2_set_x1600_params(struct dwc2_hsotg *hsotg) { struct dwc2_core_params *p = &hsotg->params; @@ -281,6 +291,22 @@ const struct acpi_device_id dwc2_acpi_match[] = { }; MODULE_DEVICE_TABLE(acpi, dwc2_acpi_match); +const struct pci_device_id dwc2_pci_ids[] = { + { + PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG), + }, + { + PCI_DEVICE(PCI_VENDOR_ID_STMICRO, + PCI_DEVICE_ID_STMICRO_USB_OTG), + }, + { + PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_DWC2), + .driver_data = (unsigned long)dwc2_set_loongson_params, + }, + { /* end: all zeroes */ } +}; +MODULE_DEVICE_TABLE(pci, dwc2_pci_ids); + static void dwc2_set_param_otg_cap(struct dwc2_hsotg *hsotg) { switch (hsotg->hw_params.op_mode) { @@ -929,10 +955,13 @@ int dwc2_init_params(struct dwc2_hsotg *hsotg) set_params(hsotg); } else { const struct acpi_device_id *amatch; + const struct pci_device_id *pmatch; amatch = acpi_match_device(dwc2_acpi_match, hsotg->dev); - if (amatch && amatch->driver_data) { - set_params = (set_params_cb)amatch->driver_data; + pmatch = pci_match_id(dwc2_pci_ids, to_pci_dev(hsotg->dev->parent)); + + if ((amatch && amatch->driver_data) || (pmatch && pmatch->driver_data)) { + set_params = (set_params_cb)pmatch->driver_data; set_params(hsotg); } } diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c index b7306ed8be4c..f3a1e4232a31 100644 --- a/drivers/usb/dwc2/pci.c +++ b/drivers/usb/dwc2/pci.c @@ -24,7 +24,7 @@ #include <linux/platform_device.h> #include <linux/usb/usb_phy_generic.h> -#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0 +#include "core.h" static const char dwc2_driver_name[] = "dwc2-pci"; @@ -122,18 +122,6 @@ static int dwc2_pci_probe(struct pci_dev *pci, return ret; } -static const struct pci_device_id dwc2_pci_ids[] = { - { - PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG), - }, - { - PCI_DEVICE(PCI_VENDOR_ID_STMICRO, - PCI_DEVICE_ID_STMICRO_USB_OTG), - }, - { /* end: all zeroes */ } -}; -MODULE_DEVICE_TABLE(pci, dwc2_pci_ids); - static struct pci_driver dwc2_pci_driver = { .name = dwc2_driver_name, .id_table = dwc2_pci_ids, diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index e43ab203054a..6481f648695a 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -157,6 +157,7 @@ #define PCI_VENDOR_ID_PCI_SIG 0x0001 #define PCI_VENDOR_ID_LOONGSON 0x0014 +#define PCI_DEVICE_ID_LOONGSON_DWC2 0x7a04 #define PCI_VENDOR_ID_SOLIDIGM 0x025e @@ -2356,6 +2357,7 @@ #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI 0xabce #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB31 0xabcf #define PCI_DEVICE_ID_SYNOPSYS_EDDA 0xedda +#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0 #define PCI_VENDOR_ID_USR 0x16ec
The dwc2 driver has everything we need to run in PCI mode except for pci_device_id driver_data parse. With that to set Loongson dwc2 element and added identified as PCI_VENDOR_ID_LOONGSON and PCI_DEVICE_ID_LOONGSON_DWC2 in dwc2_pci_ids, the Loongson dwc2 controller will work. Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn> --- drivers/usb/dwc2/core.h | 1 + drivers/usb/dwc2/params.c | 33 +++++++++++++++++++++++++++++++-- drivers/usb/dwc2/pci.c | 14 +------------- include/linux/pci_ids.h | 2 ++ 4 files changed, 35 insertions(+), 15 deletions(-)