Re: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
On Mon, Jun 29, 2020 at 11:35:32AM +, Pawel Laszczak wrote: > > > > >On Fri, Jun 26, 2020 at 06:54:47AM +0200, Pawel Laszczak wrote: > >> Patch adds PCI specific glue driver that creates and registers in-system > >> cdns-usbssp platform device. > > > >Ick, no. > > > >Platform devices are ONLY to be used for when you have a real platform > >device. If your device is a PCI device, use that. Don't try to be cute > >and make a platform device when you don't have one please. > > > >> Thanks to that we will be able to use > >> the cdns-usbssp platform driver for USBSS-DEV controller > >> build on PCI board. > > > >No, fix up that driver to not care about it being a platform device, > >make it just accept any type of 'struct device'. > > > Ok, I will remove it form this driver but probably in a few months I will > have to add > such support. For a real platform device, that's fine, but that is not what you are doing here. thanks, greg k-h
RE: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
>
>On Fri, Jun 26, 2020 at 03:10:32PM +, Pawel Laszczak wrote:
>> >> +static int cdnsp_pci_probe(struct pci_dev *pdev,
>> >> +const struct pci_device_id *id)
>> >> +{
>> >> + struct platform_device_info plat_info;
>> >> + struct cdnsp_wrap *wrap;
>> >> + struct resource *res;
>> >> + struct pci_dev *func;
>> >> + int err;
>> >> +
>> >> + /*
>> >> + * For GADGET/HOST PCI (devfn) function number is 0,
>> >> + * for OTG PCI (devfn) function number is 1.
>> >> + */
>> >> + if (!id || (pdev->devfn != PCI_DEV_FN_HOST_DEVICE &&
>> >> + pdev->devfn != PCI_DEV_FN_OTG))
>> >> + return -EINVAL;
>> >> +
>> >> + func = cdnsp_get_second_fun(pdev);
>> >> + if (unlikely(!func))
>> >> + return -EINVAL;
>> >> +
>> >> + if (func->class == PCI_CLASS_SERIAL_USB_XHCI ||
>> >> + pdev->class == PCI_CLASS_SERIAL_USB_XHCI)
>> >> + return -EINVAL;
>> >
>> >
>> >Do we need call pci_put_device(func) before returning?
>>
>> We don't need.
>> Such function doesn't exist.
>>
>
>I meant pci_dev_put(). I'm pretty sure that we do need it to match the
>pci_get_device() in cdnsp_get_second_fun().
Right, I will add this,
>
>regards,
>dan carpenter
regards,
pawel
RE: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
> >On Fri, Jun 26, 2020 at 06:54:47AM +0200, Pawel Laszczak wrote: >> Patch adds PCI specific glue driver that creates and registers in-system >> cdns-usbssp platform device. > >Ick, no. > >Platform devices are ONLY to be used for when you have a real platform >device. If your device is a PCI device, use that. Don't try to be cute >and make a platform device when you don't have one please. > >> Thanks to that we will be able to use >> the cdns-usbssp platform driver for USBSS-DEV controller >> build on PCI board. > >No, fix up that driver to not care about it being a platform device, >make it just accept any type of 'struct device'. > Ok, I will remove it form this driver but probably in a few months I will have to add such support. So I'm going to merge patch 2/5 "usb:cdns3: Add pci to platform driver wrapper" with main part of driver and remove patch 1/5 "dt-bindings: add binding for CDNSP-DRD controller". thanks, pawel
Re: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
On Fri, Jun 26, 2020 at 03:10:32PM +, Pawel Laszczak wrote:
> >> +static int cdnsp_pci_probe(struct pci_dev *pdev,
> >> + const struct pci_device_id *id)
> >> +{
> >> + struct platform_device_info plat_info;
> >> + struct cdnsp_wrap *wrap;
> >> + struct resource *res;
> >> + struct pci_dev *func;
> >> + int err;
> >> +
> >> + /*
> >> + * For GADGET/HOST PCI (devfn) function number is 0,
> >> + * for OTG PCI (devfn) function number is 1.
> >> + */
> >> + if (!id || (pdev->devfn != PCI_DEV_FN_HOST_DEVICE &&
> >> + pdev->devfn != PCI_DEV_FN_OTG))
> >> + return -EINVAL;
> >> +
> >> + func = cdnsp_get_second_fun(pdev);
> >> + if (unlikely(!func))
> >> + return -EINVAL;
> >> +
> >> + if (func->class == PCI_CLASS_SERIAL_USB_XHCI ||
> >> + pdev->class == PCI_CLASS_SERIAL_USB_XHCI)
> >> + return -EINVAL;
> >
> >
> >Do we need call pci_put_device(func) before returning?
>
> We don't need.
> Such function doesn't exist.
>
I meant pci_dev_put(). I'm pretty sure that we do need it to match the
pci_get_device() in cdnsp_get_second_fun().
regards,
dan carpenter
RE: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
>
>On Fri, Jun 26, 2020 at 06:54:47AM +0200, Pawel Laszczak wrote:
>> +static struct pci_dev *cdnsp_get_second_fun(struct pci_dev *pdev)
>> +{
>> +struct pci_dev *func;
>> +
>> +/*
>> + * Gets the second function.
>> + * It's little tricky, but this platform has two function.
>> + * The fist keeps resources for Host/Device while the second
>> + * keeps resources for DRD/OTG.
>> + */
>> +func = pci_get_device(pdev->vendor, pdev->device, NULL);
>> +if (unlikely(!func))
>
>Delete all likely/unlikely annotations. Likely and unlikely annotations
>make the code less readable. We are willing to sacrifice readability on
>fast paths.
>
>They're only supposed to be used where they're supported by benchmarking.
>Probably it's pretty tricky to benchmark probe. The other rule of thumb
>is don't add them to drivers. Another thing to consider is that this
>error path is probably obvious enough for the compiler to figure out
>without help.
Ok, I will remove unlikely.
>
>> +return NULL;
>> +
>> +if (func->devfn == pdev->devfn) {
>> +func = pci_get_device(pdev->vendor, pdev->device, func);
>> +if (unlikely(!func))
>> +return NULL;
>> +}
>> +
>> +return func;
>> +}
>> +
>> +static int cdnsp_pci_probe(struct pci_dev *pdev,
>> + const struct pci_device_id *id)
>> +{
>> +struct platform_device_info plat_info;
>> +struct cdnsp_wrap *wrap;
>> +struct resource *res;
>> +struct pci_dev *func;
>> +int err;
>> +
>> +/*
>> + * For GADGET/HOST PCI (devfn) function number is 0,
>> + * for OTG PCI (devfn) function number is 1.
>> + */
>> +if (!id || (pdev->devfn != PCI_DEV_FN_HOST_DEVICE &&
>> +pdev->devfn != PCI_DEV_FN_OTG))
>> +return -EINVAL;
>> +
>> +func = cdnsp_get_second_fun(pdev);
>> +if (unlikely(!func))
>> +return -EINVAL;
>> +
>> +if (func->class == PCI_CLASS_SERIAL_USB_XHCI ||
>> +pdev->class == PCI_CLASS_SERIAL_USB_XHCI)
>> +return -EINVAL;
>
>
>Do we need call pci_put_device(func) before returning?
We don't need.
Such function doesn't exist.
>
> ret = -EINVAL;
> goto put_pci;
>
>> +
>> +err = pcim_enable_device(pdev);
>> +if (err) {
>> +dev_err(>dev, "Enabling PCI device has failed %d\n", err);
>> +return err;
>
> goto put_pci;
>
>> +}
>> +
>> +pci_set_master(pdev);
>> +
>> +if (pci_is_enabled(func)) {
>> +wrap = pci_get_drvdata(func);
>> +} else {
>> +wrap = kzalloc(sizeof(*wrap), GFP_KERNEL);
>> +if (!wrap) {
>> +pci_disable_device(pdev);
>> +return -ENOMEM;
>
>goto disable_pci;
>
>> +}
>> +}
>> +
>> +res = wrap->dev_res;
>> +
>> +/* For GADGET device function number is 0. */
>> +if (pdev->devfn == 0) {
>> +/* Function 0: host(BAR_0) + device(BAR_1).*/
>> +dev_dbg(>dev, "Initialize Device resources\n");
>> +res[RES_DEV_ID].start = pci_resource_start(pdev, PCI_BAR_DEV);
>> +res[RES_DEV_ID].end = pci_resource_end(pdev, PCI_BAR_DEV);
>> +res[RES_DEV_ID].name = "dev";
>> +res[RES_DEV_ID].flags = IORESOURCE_MEM;
>> +dev_dbg(>dev, "USBSS-DEV physical base addr: %pa\n",
>> +[RES_DEV_ID].start);
>> +
>> +res[RES_HOST_ID].start = pci_resource_start(pdev, PCI_BAR_HOST);
>> +res[RES_HOST_ID].end = pci_resource_end(pdev, PCI_BAR_HOST);
>> +res[RES_HOST_ID].name = "xhci";
>> +res[RES_HOST_ID].flags = IORESOURCE_MEM;
>> +dev_dbg(>dev, "USBSS-XHCI physical base addr: %pa\n",
>> +[RES_HOST_ID].start);
>> +
>> +/* Interrupt for XHCI, */
>> +wrap->dev_res[RES_IRQ_HOST_ID].start = pdev->irq;
>> +wrap->dev_res[RES_IRQ_HOST_ID].name = "host";
>> +wrap->dev_res[RES_IRQ_HOST_ID].flags = IORESOURCE_IRQ;
>> +
>> +/* Interrupt device. It's the same as for HOST. */
>> +wrap->dev_res[RES_IRQ_PERIPHERAL_ID].start = pdev->irq;
>> +wrap->dev_res[RES_IRQ_PERIPHERAL_ID].name = "peripheral";
>> +wrap->dev_res[RES_IRQ_PERIPHERAL_ID].flags = IORESOURCE_IRQ;
>> +} else {
>> +res[RES_DRD_ID].start = pci_resource_start(pdev, PCI_BAR_OTG);
>> +res[RES_DRD_ID].end = pci_resource_end(pdev, PCI_BAR_OTG);
>> +res[RES_DRD_ID].name = "otg";
>> +res[RES_DRD_ID].flags = IORESOURCE_MEM;
>> +dev_dbg(>dev, "CDNSP-DRD physical base addr: %pa\n",
>> +[RES_DRD_ID].start);
>> +
>> +/* Interrupt for OTG/DRD. */
>> +wrap->dev_res[RES_IRQ_OTG_ID].start = pdev->irq;
>> +wrap->dev_res[RES_IRQ_OTG_ID].name = "otg";
>> +
Re: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
On Fri, Jun 26, 2020 at 06:54:47AM +0200, Pawel Laszczak wrote: > Patch adds PCI specific glue driver that creates and registers in-system > cdns-usbssp platform device. Ick, no. Platform devices are ONLY to be used for when you have a real platform device. If your device is a PCI device, use that. Don't try to be cute and make a platform device when you don't have one please. > Thanks to that we will be able to use > the cdns-usbssp platform driver for USBSS-DEV controller > build on PCI board. No, fix up that driver to not care about it being a platform device, make it just accept any type of 'struct device'. thanks, greg k-h
Re: [PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
On Fri, Jun 26, 2020 at 06:54:47AM +0200, Pawel Laszczak wrote:
> +static struct pci_dev *cdnsp_get_second_fun(struct pci_dev *pdev)
> +{
> + struct pci_dev *func;
> +
> + /*
> + * Gets the second function.
> + * It's little tricky, but this platform has two function.
> + * The fist keeps resources for Host/Device while the second
> + * keeps resources for DRD/OTG.
> + */
> + func = pci_get_device(pdev->vendor, pdev->device, NULL);
> + if (unlikely(!func))
Delete all likely/unlikely annotations. Likely and unlikely annotations
make the code less readable. We are willing to sacrifice readability on
fast paths.
They're only supposed to be used where they're supported by benchmarking.
Probably it's pretty tricky to benchmark probe. The other rule of thumb
is don't add them to drivers. Another thing to consider is that this
error path is probably obvious enough for the compiler to figure out
without help.
> + return NULL;
> +
> + if (func->devfn == pdev->devfn) {
> + func = pci_get_device(pdev->vendor, pdev->device, func);
> + if (unlikely(!func))
> + return NULL;
> + }
> +
> + return func;
> +}
> +
> +static int cdnsp_pci_probe(struct pci_dev *pdev,
> +const struct pci_device_id *id)
> +{
> + struct platform_device_info plat_info;
> + struct cdnsp_wrap *wrap;
> + struct resource *res;
> + struct pci_dev *func;
> + int err;
> +
> + /*
> + * For GADGET/HOST PCI (devfn) function number is 0,
> + * for OTG PCI (devfn) function number is 1.
> + */
> + if (!id || (pdev->devfn != PCI_DEV_FN_HOST_DEVICE &&
> + pdev->devfn != PCI_DEV_FN_OTG))
> + return -EINVAL;
> +
> + func = cdnsp_get_second_fun(pdev);
> + if (unlikely(!func))
> + return -EINVAL;
> +
> + if (func->class == PCI_CLASS_SERIAL_USB_XHCI ||
> + pdev->class == PCI_CLASS_SERIAL_USB_XHCI)
> + return -EINVAL;
Do we need call pci_put_device(func) before returning?
ret = -EINVAL;
goto put_pci;
> +
> + err = pcim_enable_device(pdev);
> + if (err) {
> + dev_err(>dev, "Enabling PCI device has failed %d\n", err);
> + return err;
goto put_pci;
> + }
> +
> + pci_set_master(pdev);
> +
> + if (pci_is_enabled(func)) {
> + wrap = pci_get_drvdata(func);
> + } else {
> + wrap = kzalloc(sizeof(*wrap), GFP_KERNEL);
> + if (!wrap) {
> + pci_disable_device(pdev);
> + return -ENOMEM;
goto disable_pci;
> + }
> + }
> +
> + res = wrap->dev_res;
> +
> + /* For GADGET device function number is 0. */
> + if (pdev->devfn == 0) {
> + /* Function 0: host(BAR_0) + device(BAR_1).*/
> + dev_dbg(>dev, "Initialize Device resources\n");
> + res[RES_DEV_ID].start = pci_resource_start(pdev, PCI_BAR_DEV);
> + res[RES_DEV_ID].end = pci_resource_end(pdev, PCI_BAR_DEV);
> + res[RES_DEV_ID].name = "dev";
> + res[RES_DEV_ID].flags = IORESOURCE_MEM;
> + dev_dbg(>dev, "USBSS-DEV physical base addr: %pa\n",
> + [RES_DEV_ID].start);
> +
> + res[RES_HOST_ID].start = pci_resource_start(pdev, PCI_BAR_HOST);
> + res[RES_HOST_ID].end = pci_resource_end(pdev, PCI_BAR_HOST);
> + res[RES_HOST_ID].name = "xhci";
> + res[RES_HOST_ID].flags = IORESOURCE_MEM;
> + dev_dbg(>dev, "USBSS-XHCI physical base addr: %pa\n",
> + [RES_HOST_ID].start);
> +
> + /* Interrupt for XHCI, */
> + wrap->dev_res[RES_IRQ_HOST_ID].start = pdev->irq;
> + wrap->dev_res[RES_IRQ_HOST_ID].name = "host";
> + wrap->dev_res[RES_IRQ_HOST_ID].flags = IORESOURCE_IRQ;
> +
> + /* Interrupt device. It's the same as for HOST. */
> + wrap->dev_res[RES_IRQ_PERIPHERAL_ID].start = pdev->irq;
> + wrap->dev_res[RES_IRQ_PERIPHERAL_ID].name = "peripheral";
> + wrap->dev_res[RES_IRQ_PERIPHERAL_ID].flags = IORESOURCE_IRQ;
> + } else {
> + res[RES_DRD_ID].start = pci_resource_start(pdev, PCI_BAR_OTG);
> + res[RES_DRD_ID].end = pci_resource_end(pdev, PCI_BAR_OTG);
> + res[RES_DRD_ID].name = "otg";
> + res[RES_DRD_ID].flags = IORESOURCE_MEM;
> + dev_dbg(>dev, "CDNSP-DRD physical base addr: %pa\n",
> + [RES_DRD_ID].start);
> +
> + /* Interrupt for OTG/DRD. */
> + wrap->dev_res[RES_IRQ_OTG_ID].start = pdev->irq;
> + wrap->dev_res[RES_IRQ_OTG_ID].name = "otg";
> + wrap->dev_res[RES_IRQ_OTG_ID].flags = IORESOURCE_IRQ;
> + }
> +
> + if (pci_is_enabled(func)) {
> + /* Set up platform
[PATCH RFC 2/5] usb:cdns3: Add pci to platform driver wrapper
Patch adds PCI specific glue driver that creates and registers in-system
cdns-usbssp platform device. Thanks to that we will be able to use
the cdns-usbssp platform driver for USBSS-DEV controller
build on PCI board.
Signed-off-by: Pawel Laszczak
---
drivers/usb/Kconfig | 1 +
drivers/usb/Makefile | 1 +
drivers/usb/cdnsp/Kconfig | 26 +
drivers/usb/cdnsp/Makefile| 3 +
drivers/usb/cdnsp/cdnsp-pci.c | 214 ++
5 files changed, 245 insertions(+)
create mode 100644 drivers/usb/cdnsp/Kconfig
create mode 100644 drivers/usb/cdnsp/Makefile
create mode 100644 drivers/usb/cdnsp/cdnsp-pci.c
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 26475b409b53..555c4a4cb465 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -112,6 +112,7 @@ source "drivers/usb/usbip/Kconfig"
endif
source "drivers/usb/cdns3/Kconfig"
+source "drivers/usb/cdnsp/Kconfig"
source "drivers/usb/mtu3/Kconfig"
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index 1c1c1d659394..84727f7a4b92 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_USB_DWC2)+= dwc2/
obj-$(CONFIG_USB_ISP1760) += isp1760/
obj-$(CONFIG_USB_CDNS3)+= cdns3/
+obj-$(CONFIG_USB_CDNSP)+= cdnsp/
obj-$(CONFIG_USB_MON) += mon/
obj-$(CONFIG_USB_MTU3) += mtu3/
diff --git a/drivers/usb/cdnsp/Kconfig b/drivers/usb/cdnsp/Kconfig
new file mode 100644
index ..5796a19bb8a0
--- /dev/null
+++ b/drivers/usb/cdnsp/Kconfig
@@ -0,0 +1,26 @@
+config USB_CDNSP
+ tristate "Cadence CDNSP Dual-Role Controller"
+ depends on USB_SUPPORT && (USB || USB_GADGET) && HAS_DMA
+ select USB_XHCI_PLATFORM if USB_XHCI_HCD
+ select USB_ROLE_SWITCH
+ help
+ Say Y here if your system has a Cadence CDNSP dual-role controller.
+ It supports: dual-role switch Host-only, and Peripheral-only.
+
+ If you choose to build this driver is a dynamically linked
+ module, the module will be called cdnsp.ko.
+
+if USB_CDNSP
+
+config USB_CDNSP_PCI
+ tristate "Cadence CDNSP support on PCIe-based platforms"
+ depends on USB_PCI && ACPI
+ default USB_CDNSP
+ help
+ If you're using the CDNSP Core IP with a PCIe, please say
+ 'Y' or 'M' here.
+
+ If you choose to build this driver as module it will
+ be dynamically linked and module will be called cdnsp-pci.ko
+
+endif
diff --git a/drivers/usb/cdnsp/Makefile b/drivers/usb/cdnsp/Makefile
new file mode 100644
index ..21adf3eb2f7d
--- /dev/null
+++ b/drivers/usb/cdnsp/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_USB_CDNSP_PCI)+= cdnsp-pci.o
diff --git a/drivers/usb/cdnsp/cdnsp-pci.c b/drivers/usb/cdnsp/cdnsp-pci.c
new file mode 100644
index ..7e05d584d57c
--- /dev/null
+++ b/drivers/usb/cdnsp/cdnsp-pci.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Cadence PCI Glue driver.
+ *
+ * Copyright (C) 2019 Cadence.
+ *
+ * Author: Pawel Laszczak
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct cdnsp_wrap {
+ struct platform_device *plat_dev;
+ struct resource dev_res[6];
+ int devfn;
+};
+
+#define RES_IRQ_HOST_ID0
+#define RES_IRQ_PERIPHERAL_ID 1
+#define RES_IRQ_OTG_ID 2
+#define RES_HOST_ID3
+#define RES_DEV_ID 4
+#define RES_DRD_ID 5
+
+#define PCI_BAR_HOST 0
+#define PCI_BAR_OTG0
+#define PCI_BAR_DEV2
+
+#define PCI_DEV_FN_HOST_DEVICE 0
+#define PCI_DEV_FN_OTG 1
+
+#define PCI_DRIVER_NAME"cdns-pci-usbssp"
+#define PLAT_DRIVER_NAME "cdns-usbssp"
+
+#define CDNS_VENDOR_ID 0x17cd
+#define CDNS_DEVICE_ID 0x0100
+#define CDNS_DRD_IF(PCI_CLASS_SERIAL_USB << 8 | 0x80)
+
+static struct pci_dev *cdnsp_get_second_fun(struct pci_dev *pdev)
+{
+ struct pci_dev *func;
+
+ /*
+* Gets the second function.
+* It's little tricky, but this platform has two function.
+* The fist keeps resources for Host/Device while the second
+* keeps resources for DRD/OTG.
+*/
+ func = pci_get_device(pdev->vendor, pdev->device, NULL);
+ if (unlikely(!func))
+ return NULL;
+
+ if (func->devfn == pdev->devfn) {
+ func = pci_get_device(pdev->vendor, pdev->device, func);
+ if (unlikely(!func))
+ return NULL;
+ }
+
+ return func;
+}
+
+static int cdnsp_pci_probe(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ struct platform_device_info plat_info;
+ struct cdnsp_wrap *wrap;
+ struct resource *res;
+ struct pci_dev *func;
+ int err;
+
+ /*
+* For GADGET/HOST
