Re: [PATCH v1] usb: dwc2: add pci_device_id driver_data parse support
From: zhuyinbo
Date: Thu Jun 08 2023 - 03:55:09 EST
Friendly ping ?
在 2023/5/20 上午11:24, zhuyinbo 写道:
Hi greg k-h,
I'm sorry for giving you feedback so late, for your suggestion that I
have a some analysis.
2023 May 18, 2023 at 6:32 PM, Greg Kroah-Hartman wrote:
On Thu, May 18, 2023 at 05:22:40PM +0800, Yinbo Zhu wrote:
The dwc2 driver has everything we need to run in PCI mode except
for pci_device_id driver_data parse. With that to set Loongson
dwc2 element and added identified as PCI_VENDOR_ID_LOONGSON
and PCI_DEVICE_ID_LOONGSON_DWC2 in dwc2_pci_ids, the Loongson
dwc2 controller will work.
Signed-off-by: Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>
---
drivers/usb/dwc2/core.h | 1 +
drivers/usb/dwc2/params.c | 33 +++++++++++++++++++++++++++++++--
drivers/usb/dwc2/pci.c | 14 +-------------
include/linux/pci_ids.h | 2 ++
4 files changed, 35 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 0bb4c0c845bf..c92a1da46a01 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1330,6 +1330,7 @@ irqreturn_t dwc2_handle_common_intr(int irq,
void *dev);
/* The device ID match table */
extern const struct of_device_id dwc2_of_match_table[];
extern const struct acpi_device_id dwc2_acpi_match[];
+extern const struct pci_device_id dwc2_pci_ids[];
int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg);
int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg);
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 21d16533bd2f..f7550d293c2d 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -7,6 +7,8 @@
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/usb/of.h>
+#include <linux/pci_ids.h>
+#include <linux/pci.h>
#include "core.h"
@@ -55,6 +57,14 @@ static void dwc2_set_jz4775_params(struct
dwc2_hsotg *hsotg)
!device_property_read_bool(hsotg->dev,
"disable-over-current");
}
+static void dwc2_set_loongson_params(struct dwc2_hsotg *hsotg)
+{
+ struct dwc2_core_params *p = &hsotg->params;
+
+ p->phy_utmi_width = 8;
+ p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
+}
+
static void dwc2_set_x1600_params(struct dwc2_hsotg *hsotg)
{
struct dwc2_core_params *p = &hsotg->params;
@@ -281,6 +291,22 @@ const struct acpi_device_id dwc2_acpi_match[] = {
};
MODULE_DEVICE_TABLE(acpi, dwc2_acpi_match);
+const struct pci_device_id dwc2_pci_ids[] = {
+ {
+ PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG),
+ },
+ {
+ PCI_DEVICE(PCI_VENDOR_ID_STMICRO,
+ PCI_DEVICE_ID_STMICRO_USB_OTG),
+ },
+ {
+ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON,
PCI_DEVICE_ID_LOONGSON_DWC2),
+ .driver_data = (unsigned long)dwc2_set_loongson_params,
+ },
+ { /* end: all zeroes */ }
+};
+MODULE_DEVICE_TABLE(pci, dwc2_pci_ids);
+
static void dwc2_set_param_otg_cap(struct dwc2_hsotg *hsotg)
{
switch (hsotg->hw_params.op_mode) {
@@ -929,10 +955,13 @@ int dwc2_init_params(struct dwc2_hsotg *hsotg)
set_params(hsotg);
} else {
const struct acpi_device_id *amatch;
+ const struct pci_device_id *pmatch;
amatch = acpi_match_device(dwc2_acpi_match, hsotg->dev);
- if (amatch && amatch->driver_data) {
- set_params = (set_params_cb)amatch->driver_data;
+ pmatch = pci_match_id(dwc2_pci_ids,
to_pci_dev(hsotg->dev->parent));
Ick, this means this is not a "real" PCI driver, right? Why not?
The params.c and platform.c was a part of pci dwc2 device driver. This
pci.c was only play a role that register device resource but not operate
dwc2 hardware. in other words, the params.c seems unrelated to the
device type. Whether this device is a PCI device, platform device, or
PCI device, it is best to use params.c for operational dwc2 elements.
Failure to do so seems to break the original design.
Please tie into the PCI device probe call, don't walk all PCI devices
like this.
I learn about that you strongly disagree with using pci_match_id, May
I ask you the reason ? actually, I use it was due to I noticed that
xhci-pci.c, ehci-pci.c and ohci-pci.c was all use it. and I don't use it
in dwc2/pci.c was considering set dwc2 element need dpend on elements.c
and platform.c, and usb driver (ohci,echi,xhci) was a relatively
indepent device driver when to operate usb controler. but dwc2 was not.
If I fource the element setting of dwc2 element in dwc2/pci.c. It will
be following case. This will cause problems with element-initial
function or element-check function.
1. initial dwc2 element.
2. check the setting of dwc2 element whether was suitable
3. set dwc2 element.
or
1. set dwc2 element.
2. initial dwc2 element.
3. check the setting of dwc2 element whether was suitable
The corresponding code call process as follows:
1. dwc2_set_default_params(hsotg);
2. dwc2_get_device_properties(hsotg);
3. dwc2_check_params(hsotg);
4. dwc2_set_loongson_params;
or
1. dwc2_set_loongson_params;
2. dwc2_set_default_params(hsotg);
3. dwc2_get_device_properties(hsotg);
4. dwc2_check_params(hsotg);
But the platform dwc2 device or acpi dwc2 device was all following case
and It seems was correct order.
1. dwc2_set_default_params(hsotg);
2. dwc2_get_device_properties(hsotg);
3. dwc2_set_loongson_params;
4. dwc2_check_params(hsotg);
How are you _sure_ that the parent is really a PCI device? That is very
very fragile and will break.
Do this properly instead.
Thank you for your reminder. There was indeed an issue with my previous
code, and the modified code is as follows, then it seems to ensure that
device is a PCI device.
@@ -927,13 +954,20 @@ int dwc2_init_params(struct dwc2_hsotg *hsotg)
if (match && match->data) {
set_params = match->data;
set_params(hsotg);
- } else {
+ } else if (!match) {
const struct acpi_device_id *amatch;
+ const struct pci_device_id *pmatch = NULL;
amatch = acpi_match_device(dwc2_acpi_match, hsotg->dev);
if (amatch && amatch->driver_data) {
set_params = (set_params_cb)amatch->driver_data;
set_params(hsotg);
+ } else if (!amatch)
+ pmatch = pci_match_id(dwc2_pci_ids,
to_pci_dev(hsotg->dev->parent));
+
+ if (pmatch && pmatch->driver_data) {
+ set_params = (set_params_cb)pmatch->driver_data;
+ set_params(hsotg);
}
+
+ if ((amatch && amatch->driver_data) || (pmatch &&
pmatch->driver_data)) {
+ set_params = (set_params_cb)pmatch->driver_data;
set_params(hsotg);
}
}
diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
index b7306ed8be4c..f3a1e4232a31 100644
--- a/drivers/usb/dwc2/pci.c
+++ b/drivers/usb/dwc2/pci.c
@@ -24,7 +24,7 @@
#include <linux/platform_device.h>
#include <linux/usb/usb_phy_generic.h>
-#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0
+#include "core.h"
static const char dwc2_driver_name[] = "dwc2-pci";
@@ -122,18 +122,6 @@ static int dwc2_pci_probe(struct pci_dev *pci,
return ret;
}
-static const struct pci_device_id dwc2_pci_ids[] = {
- {
- PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG),
- },
- {
- PCI_DEVICE(PCI_VENDOR_ID_STMICRO,
- PCI_DEVICE_ID_STMICRO_USB_OTG),
- },
- { /* end: all zeroes */ }
-};
-MODULE_DEVICE_TABLE(pci, dwc2_pci_ids);
-
static struct pci_driver dwc2_pci_driver = {
.name = dwc2_driver_name,
.id_table = dwc2_pci_ids,
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index e43ab203054a..6481f648695a 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -157,6 +157,7 @@
#define PCI_VENDOR_ID_PCI_SIG 0x0001
#define PCI_VENDOR_ID_LOONGSON 0x0014
+#define PCI_DEVICE_ID_LOONGSON_DWC2 0x7a04
#define PCI_VENDOR_ID_SOLIDIGM 0x025e
@@ -2356,6 +2357,7 @@
#define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI 0xabce
#define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB31 0xabcf
#define PCI_DEVICE_ID_SYNOPSYS_EDDA 0xedda
+#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0
Please read the top of this file for why you should not add new ids
here.
okay, I will remove it.
Thanks!
Yinbo.