Messages in this thread | | | Subject | Re: [PATCH v2] usb: dwc2: add pci_device_id driver_data parse support | From | Yinbo Zhu <> | Date | Fri, 21 Jul 2023 16:13:54 +0800 |
| |
Hi greg k-h,
I'm sorry to bother you, I want to request your opinion on what I have said.
Thanks,
Yinbo
在 2023/6/14 上午11:55, zhuyinbo 写道: > > > 在 2023/6/13 下午6:19, Greg Kroah-Hartman 写道: >> On Mon, Jun 12, 2023 at 02:51:05PM +0800, zhuyinbo wrote: >>> >>> >>> 在 2023/6/9 下午5:31, Greg Kroah-Hartman 写道: >>>> On Fri, Jun 09, 2023 at 11:25:15AM +0200, Greg Kroah-Hartman wrote: >>>>> On Fri, Jun 09, 2023 at 05:07:03PM +0800, zhuyinbo wrote: >>>>>> >>>>>> >>>>>> 在 2023/6/9 下午2:13, Greg Kroah-Hartman 写道: >>>>>>> On Fri, Jun 09, 2023 at 10:50:47AM +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> >>>>>>>> --- >>>>>>>> Change in v2: >>>>>>>> 1. Move the dwc2 pci ID from pci_ids.h to params.c. >>>>>>>> 2. Add some code logic to ensure that the current >>>>>>>> device is >>>>>>>> a PCI device. >>>>>>>> 3. Fix the compile issue when dwc2 pci driver as module. >>>>>>>> >>>>>>>> drivers/usb/dwc2/core.h | 1 + >>>>>>>> drivers/usb/dwc2/params.c | 39 >>>>>>>> ++++++++++++++++++++++++++++++++++++++- >>>>>>>> drivers/usb/dwc2/pci.c | 14 +------------- >>>>>>>> 3 files changed, 40 insertions(+), 14 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..6b68a8830781 100644 >>>>>>>> --- a/drivers/usb/dwc2/params.c >>>>>>>> +++ b/drivers/usb/dwc2/params.c >>>>>>>> @@ -7,9 +7,14 @@ >>>>>>>> #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" >>>>>>>> +#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0 >>>>>>>> +#define PCI_DEVICE_ID_LOONGSON_DWC2 0x7a04 >>>>>>>> + >>>>>>>> static void dwc2_set_bcm_params(struct dwc2_hsotg *hsotg) >>>>>>>> { >>>>>>>> struct dwc2_core_params *p = &hsotg->params; >>>>>>>> @@ -55,6 +60,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_PARTIAL; >>>>>>>> +} >>>>>>>> + >>>>>>>> static void dwc2_set_x1600_params(struct dwc2_hsotg *hsotg) >>>>>>>> { >>>>>>>> struct dwc2_core_params *p = &hsotg->params; >>>>>>>> @@ -281,6 +294,23 @@ 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); >>>>>>>> +EXPORT_SYMBOL_GPL(dwc2_pci_ids); >>>>>>>> + >>>>>>>> static void dwc2_set_param_otg_cap(struct dwc2_hsotg *hsotg) >>>>>>>> { >>>>>>>> switch (hsotg->hw_params.op_mode) { >>>>>>>> @@ -927,13 +957,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)); >>>>>>> >>>>>>> At this point in time, how can you guarantee that the parent device >>>>>>> really is a PCI one? This function is being called from a platform >>>>>>> device callback, and platform devices should NEVER be a child of >>>>>>> a PCI >>>>>>> device, as that's not how PCI or platform devices work. >>>>>>> >>>>>>> So how is this even possible? >>>>>>> >>>>>>> confused, >>>>>>> >>>>>> >>>>>> >>>>>> Hi greg k-h, >>>>>> >>>>>> My current considerations are based on that the dwc2 drivers support >>>>>> three types of devices, they are Platform device / ACPI device / PCI >>>>>> device, and The dwc2/platform.c can all cover the three type dwc2 >>>>>> device, no matter how it is registered. >>>>>> >>>>>> So, when a dwc2 device wasn't platform device and acpi device and It >>>>>> will be a PCI device, and the the dwc2/pci.c will register a dwc2 >>>>>> platform device that dwc2 device is a device that in >>>>>> dwc2/platform.c's >>>>>> dwc2_driver_probe. the "&dwc2->dev->parent" is the >>>>>> "hsotg->dev->parent" >>>>>> and it was also the &pci->dev. >>>>> >>>>> That's wrong, a PCI device should NEVER register a platform device >>>>> under >>>>> it, as obviously it's not a platform device at all. >>>>> >>>>> So please work to fix that layering violation here, as that's not >>>>> correct and will cause problems as you are showing here. >>>>> >>>>>> So, We can use "to_pci_dev(hsotg->dev->parent)" to gain a dwc2 pci >>>>>> device. >>>>> >>>>> No, not if the parent is something else as you have no way of knowing >>>>> this at this point in time. >>>>> >>>>>> >>>>>> 1) DWC2 PCI device driver (drivers/usb/dwc2/pci.c) : >>>>>> >>>>>> static int dwc2_pci_probe(struct pci_dev *pci, >>>>>> const struct pci_device_id *id) >>>>>> { >>>>>> ... >>>>>> struct platform_device *dwc2; >>>>>> ... >>>>>> struct device *dev = &pci->dev; >>>>>> ... >>>>>> dwc2 = platform_device_alloc("dwc2", PLATFORM_DEVID_AUTO); >>>>>> //"dwc2" was used to match dwc2 platform driver >>>>> >>>>> This needs to be fixed, it's not ok at all. Just use the real PCI >>>>> device here please. >>>> >>>> Also, to be fair, you didn't cause this problem, I missed it >>>> previously. >>>> But fixing it is essential, and will be required in order to get your >>>> changes to work properly. Sorry about that. >>>> >>> >>> >>> Hi greg k-h, >>> >>> Actually, the dwc2/pci.c's platform_device_add(dwc2) was create a >>> platform device called "dwc2" ensure dwc2/platform's dwc2_driver_probe >>> function can be called. After all, the dwc2/platform's >>> dwc2_driver_probe >>> is the one that truly initializes the dwc2 controller. In addition, the >>> same applies to dwc3 code. >> >> Yes, it does create a platform device, but ALSO a real platform device >> could be present in the system for the driver, one that is NOT a child >> of a PCI device, right? > > > I don't get it that why need dwc2 was a child of pci. and I just need > the dwc2->dev.parent's parent device was a child of dwc2 pci device. > If my patch conditions are met (match and amatch was NULL), this always > seems to hold true. > > dwc2_pci_probe () > { > struct platform_device *dwc2; > struct device *dev = &pci->dev; > dwc2->dev.parent = dev; > platform_device_add(dwc2); > } > >> >> It's not this way with your system, but it could be that way for many >> others, right? And if so, the assumption that you can just poke around >> in the parent device and assume that you know what bus it is on is just >> wrong and will break things. > > > What you said about "system" that is unrelated to the kernel driver? I > think it should be system independent and it depends entirely on the > kernel driver, for the driver, everyone is the same. the behavior of the > driver is known, and currently I have not found any other risks it > poses. Additionally, the key is that the issue that my patch is > addressing is a common issue. For example, if the following devices are > designed as PCI devices on hardware, they will always encounter the same > problem. > > const struct of_device_id dwc2_of_match_table[] = { > { .compatible = "brcm,bcm2835-usb", .data = > dwc2_set_bcm_params }, > { .compatible = "hisilicon,hi6220-usb", .data = > dwc2_set_his_params }, > { .compatible = "ingenic,jz4775-otg", .data = > dwc2_set_jz4775_params }, > { .compatible = "ingenic,jz4780-otg", .data = > dwc2_set_jz4775_params }, > { .compatible = "ingenic,x1000-otg", .data = > dwc2_set_jz4775_params }, > { .compatible = "ingenic,x1600-otg", .data = > dwc2_set_x1600_params }, > { .compatible = "ingenic,x1700-otg", .data = > dwc2_set_x1600_params }, > { .compatible = "ingenic,x1830-otg", .data = > dwc2_set_x1600_params }, > { .compatible = "ingenic,x2000-otg", .data = > dwc2_set_x2000_params }, > { .compatible = "rockchip,rk3066-usb", .data = > dwc2_set_rk_params }, > { .compatible = "lantiq,arx100-usb", .data = > dwc2_set_ltq_params }, > { .compatible = "lantiq,xrx200-usb", .data = > dwc2_set_ltq_params }, > >> >> You should NEVER be poking around in the information for the parent >> device, UNLESS you have just created the child device yourself and the >> ONLY code path is one that you have full control over. >> >> Does that make more sense? > > > Yes, but it seems inappropriate for dwc2. For this, please allow me to > parse some of my comments in v1: > > > Actually, I use pci_match_id 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); > > > Thanks, > > Yinbo
| |