From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Yinbo Zhu <zhuyinbo@loongson.cn>
Cc: Minas Harutyunyan <hminas@synopsys.com>,
Bjorn Helgaas <bhelgaas@google.com>,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-pci@vger.kernel.org, Jianmin Lv <lvjianmin@loongson.cn>,
wanghongliang@loongson.cn, Liu Peibao <liupeibao@loongson.cn>,
loongson-kernel@lists.loongnix.cn
Subject: Re: [PATCH v1] usb: dwc2: add pci_device_id driver_data parse support
Date: Thu, 18 May 2023 12:32:32 +0200 [thread overview]
Message-ID: <2023051843-scruffy-gush-cdec@gregkh> (raw)
In-Reply-To: <20230518092240.8023-1-zhuyinbo@loongson.cn>
On Thu, May 18, 2023 at 05:22:40PM +0800, Yinbo Zhu wrote:
> The dwc2 driver has everything we need to run in PCI mode except
> for pci_device_id driver_data parse. With that to set Loongson
> dwc2 element and added identified as PCI_VENDOR_ID_LOONGSON
> and PCI_DEVICE_ID_LOONGSON_DWC2 in dwc2_pci_ids, the Loongson
> dwc2 controller will work.
>
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
> drivers/usb/dwc2/core.h | 1 +
> drivers/usb/dwc2/params.c | 33 +++++++++++++++++++++++++++++++--
> drivers/usb/dwc2/pci.c | 14 +-------------
> include/linux/pci_ids.h | 2 ++
> 4 files changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 0bb4c0c845bf..c92a1da46a01 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -1330,6 +1330,7 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev);
> /* The device ID match table */
> extern const struct of_device_id dwc2_of_match_table[];
> extern const struct acpi_device_id dwc2_acpi_match[];
> +extern const struct pci_device_id dwc2_pci_ids[];
>
> int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg);
> int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg);
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 21d16533bd2f..f7550d293c2d 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -7,6 +7,8 @@
> #include <linux/module.h>
> #include <linux/of_device.h>
> #include <linux/usb/of.h>
> +#include <linux/pci_ids.h>
> +#include <linux/pci.h>
>
> #include "core.h"
>
> @@ -55,6 +57,14 @@ static void dwc2_set_jz4775_params(struct dwc2_hsotg *hsotg)
> !device_property_read_bool(hsotg->dev, "disable-over-current");
> }
>
> +static void dwc2_set_loongson_params(struct dwc2_hsotg *hsotg)
> +{
> + struct dwc2_core_params *p = &hsotg->params;
> +
> + p->phy_utmi_width = 8;
> + p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> +}
> +
> static void dwc2_set_x1600_params(struct dwc2_hsotg *hsotg)
> {
> struct dwc2_core_params *p = &hsotg->params;
> @@ -281,6 +291,22 @@ const struct acpi_device_id dwc2_acpi_match[] = {
> };
> MODULE_DEVICE_TABLE(acpi, dwc2_acpi_match);
>
> +const struct pci_device_id dwc2_pci_ids[] = {
> + {
> + PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG),
> + },
> + {
> + PCI_DEVICE(PCI_VENDOR_ID_STMICRO,
> + PCI_DEVICE_ID_STMICRO_USB_OTG),
> + },
> + {
> + PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_DWC2),
> + .driver_data = (unsigned long)dwc2_set_loongson_params,
> + },
> + { /* end: all zeroes */ }
> +};
> +MODULE_DEVICE_TABLE(pci, dwc2_pci_ids);
> +
> static void dwc2_set_param_otg_cap(struct dwc2_hsotg *hsotg)
> {
> switch (hsotg->hw_params.op_mode) {
> @@ -929,10 +955,13 @@ int dwc2_init_params(struct dwc2_hsotg *hsotg)
> set_params(hsotg);
> } else {
> const struct acpi_device_id *amatch;
> + const struct pci_device_id *pmatch;
>
> amatch = acpi_match_device(dwc2_acpi_match, hsotg->dev);
> - if (amatch && amatch->driver_data) {
> - set_params = (set_params_cb)amatch->driver_data;
> + pmatch = pci_match_id(dwc2_pci_ids, to_pci_dev(hsotg->dev->parent));
Ick, this means this is not a "real" PCI driver, right? Why not?
Please tie into the PCI device probe call, don't walk all PCI devices
like this.
How are you _sure_ that the parent is really a PCI device? That is very
very fragile and will break.
Do this properly instead.
> +
> + if ((amatch && amatch->driver_data) || (pmatch && pmatch->driver_data)) {
> + set_params = (set_params_cb)pmatch->driver_data;
> set_params(hsotg);
> }
> }
> diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
> index b7306ed8be4c..f3a1e4232a31 100644
> --- a/drivers/usb/dwc2/pci.c
> +++ b/drivers/usb/dwc2/pci.c
> @@ -24,7 +24,7 @@
> #include <linux/platform_device.h>
> #include <linux/usb/usb_phy_generic.h>
>
> -#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0
> +#include "core.h"
>
> static const char dwc2_driver_name[] = "dwc2-pci";
>
> @@ -122,18 +122,6 @@ static int dwc2_pci_probe(struct pci_dev *pci,
> return ret;
> }
>
> -static const struct pci_device_id dwc2_pci_ids[] = {
> - {
> - PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG),
> - },
> - {
> - PCI_DEVICE(PCI_VENDOR_ID_STMICRO,
> - PCI_DEVICE_ID_STMICRO_USB_OTG),
> - },
> - { /* end: all zeroes */ }
> -};
> -MODULE_DEVICE_TABLE(pci, dwc2_pci_ids);
> -
> static struct pci_driver dwc2_pci_driver = {
> .name = dwc2_driver_name,
> .id_table = dwc2_pci_ids,
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index e43ab203054a..6481f648695a 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -157,6 +157,7 @@
> #define PCI_VENDOR_ID_PCI_SIG 0x0001
>
> #define PCI_VENDOR_ID_LOONGSON 0x0014
> +#define PCI_DEVICE_ID_LOONGSON_DWC2 0x7a04
>
> #define PCI_VENDOR_ID_SOLIDIGM 0x025e
>
> @@ -2356,6 +2357,7 @@
> #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI 0xabce
> #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB31 0xabcf
> #define PCI_DEVICE_ID_SYNOPSYS_EDDA 0xedda
> +#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0
Please read the top of this file for why you should not add new ids
here.
thanks,
greg k-h
next prev parent reply other threads:[~2023-05-18 10:32 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-18 9:22 [PATCH v1] usb: dwc2: add pci_device_id driver_data parse support Yinbo Zhu
2023-05-18 10:32 ` Greg Kroah-Hartman [this message]
2023-05-20 3:24 ` zhuyinbo
2023-06-08 7:54 ` zhuyinbo
2023-06-08 8:01 ` Greg Kroah-Hartman
2023-06-08 8:57 ` zhuyinbo
2023-06-08 9:11 ` Greg Kroah-Hartman
2023-06-09 1:15 ` zhuyinbo
2023-05-18 17:52 ` kernel test robot
2023-05-19 7:13 ` zhuyinbo
2023-05-19 9:49 ` Greg Kroah-Hartman
2023-05-20 3:33 ` zhuyinbo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2023051843-scruffy-gush-cdec@gregkh \
--to=gregkh@linuxfoundation.org \
--cc=bhelgaas@google.com \
--cc=hminas@synopsys.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=liupeibao@loongson.cn \
--cc=loongson-kernel@lists.loongnix.cn \
--cc=lvjianmin@loongson.cn \
--cc=wanghongliang@loongson.cn \
--cc=zhuyinbo@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox