[v2] usb: dwc2: add pci_device_id driver_data parse support

  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(-)
  

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,

greg k-h
  

在 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.

So, We can use "to_pci_dev(hsotg->dev->parent)" to gain a dwc2 pci
device.

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
...
         dwc2->dev.parent = dev;
...

}

2)   DWC2 PLATFROM driver (drivers/usb/dwc2/platform.c) :
// This driver cover pci type device.

static const char dwc2_driver_name[] = "dwc2";
static struct platform_driver dwc2_platform_driver = {
         .driver = {
                 .name = dwc2_driver_name,
			//"dwc2" was used to match pci type dwc2 device
...
         },
         .probe = dwc2_driver_probe,
         .remove = dwc2_driver_remove,
         .shutdown = dwc2_driver_shutdown,
};
static int dwc2_driver_probe(struct platform_device *dev)
{
// The dev is the "dwc2" platform device in dwc2/pci.c driver.
         struct dwc2_hsotg *hsotg;
...
         hsotg->dev = &dev->dev;

// The hsotg->dev->parent or &dev->dev->parent is the &pci->dev in
// dwc2/pci.c driver.
...
}


Thanks,
Yinbo
  

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.

> ...
>         dwc2->dev.parent = dev;
> ...
> 
> }
> 
> 2)   DWC2 PLATFROM driver (drivers/usb/dwc2/platform.c) :
> // This driver cover pci type device.
> 
> static const char dwc2_driver_name[] = "dwc2";
> static struct platform_driver dwc2_platform_driver = {
>         .driver = {
>                 .name = dwc2_driver_name,
> 			//"dwc2" was used to match pci type dwc2 device
> ...
>         },
>         .probe = dwc2_driver_probe,
>         .remove = dwc2_driver_remove,
>         .shutdown = dwc2_driver_shutdown,
> };
> static int dwc2_driver_probe(struct platform_device *dev)
> {
> // The dev is the "dwc2" platform device in dwc2/pci.c driver.
>         struct dwc2_hsotg *hsotg;
> ...
>         hsotg->dev = &dev->dev;

Yes, and if this was a real platform device, the parent would NOT be a
PCI device, and so your check will cause all sorts of problems, right?

thanks,

greg k-h
  

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.

greg k-h
  

Hi greg k-h,

在 2023/6/9 下午5:25, Greg Kroah-Hartman 写道:
> 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.
> 


It doesn't seems like that for current dwc2 code.  The dwc2/pci.c driver
register a platform device called "dwc2" was to match dwc2/platform.c's
dwc2 driver called "dwc2" and 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.

Do we need to refactor all the dwc (dwc2/dwc3) code ?

> 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.


Do we need to support legacy dwc2 device drivers in the future ? which
legacy device driver was that dwc2 as a platform device but not support
dts.  If that's not the case, I think my logic seems to guarantee it.

> 
>>
>> 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.
> 
>> ...
>>          dwc2->dev.parent = dev;
>> ...
>>
>> }
>>
>> 2)   DWC2 PLATFROM driver (drivers/usb/dwc2/platform.c) :
>> // This driver cover pci type device.
>>
>> static const char dwc2_driver_name[] = "dwc2";
>> static struct platform_driver dwc2_platform_driver = {
>>          .driver = {
>>                  .name = dwc2_driver_name,
>> 			//"dwc2" was used to match pci type dwc2 device
>> ...
>>          },
>>          .probe = dwc2_driver_probe,
>>          .remove = dwc2_driver_remove,
>>          .shutdown = dwc2_driver_shutdown,
>> };
>> static int dwc2_driver_probe(struct platform_device *dev)
>> {
>> // The dev is the "dwc2" platform device in dwc2/pci.c driver.
>>          struct dwc2_hsotg *hsotg;
>> ...
>>          hsotg->dev = &dev->dev;
> 
> Yes, and if this was a real platform device, the parent would NOT be a
> PCI device, and so your check will cause all sorts of problems, right?


When the "match" and "amatch" was all NULL and considering that the
current driver does not support legacy dwc2 device and the current
platform device seems that it must be created by dwc2/pci.c. Is there
anything else I don't know about ?


Thanks,
Yinbo
  

在 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.

So, Do we need to refactor all the dwc (dwc2/dwc3) code ?  For dwc2, It
seems need to rework dwc2/pci.c, dwc2/platform.c and dwc2/params.c.
And it is necessary to decouple dwc2/platform.c and dwc2/params.c and
this is to ensure that dwc2/pci. c and dwc2/platform. c are two
relatively independent drivers, but, from my personal perspective, this
doesn't seem like a good choice.  Do you think so ?  Or did I
misunderstand your meaning?

Thanks,
Yinbo
  

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?

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.

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?

thanks,

greg k-h
  

在 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
  

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
  

Friendly ping ?


在 2023/7/21 下午4:13, Yinbo Zhu 写道:
> 
> 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
  

On Mon, Aug 14, 2023 at 04:25:56PM +0800, Yinbo Zhu wrote:
> 
> 
> Friendly ping ?

Sorry, missed this.

> 在 2023/7/21 下午4:13, Yinbo Zhu 写道:
> > 
> > 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);

I don't remember at all here anymore, sorry.  I suggest doing what you
think is best here and resubmit and I will be glad to review it then.

thanks,

greg k-h
  

在 2023/8/14 下午11:12, Greg Kroah-Hartman 写道:
> On Mon, Aug 14, 2023 at 04:25:56PM +0800, Yinbo Zhu wrote:
>>
>>
>> Friendly ping ?
> 
> Sorry, missed this.
> 
>> 在 2023/7/21 下午4:13, Yinbo Zhu 写道:
>>>
>>> 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);
> 
> I don't remember at all here anymore, sorry.  I suggest doing what you
> think is best here and resubmit and I will be glad to review it then.


Okay, I will resend v2, please help review.

Thanks,
Yinbo.