lkml.org 
[lkml]   [2023]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2] usb: dwc2: add pci_device_id driver_data parse support
    From
    Date

    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

    \
     
     \ /
      Last update: 2023-07-21 10:15    [W:8.912 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site