Re: [PATCH v3 2/4] usb: cdns2: Add main part of Cadence USBHS driver
From: Christophe JAILLET
Date: Thu May 25 2023 - 16:47:12 EST
Le 25/05/2023 à 07:49, Pawel Laszczak a écrit :
This patch introduces the main part of Cadence USBHS driver
to Linux kernel.
To reduce the patch size a little bit, the header file gadget.h was
intentionally added as separate patch.
The Cadence USB 2.0 Controller is a highly configurable IP Core which
supports both full and high speed data transfer.
The current driver has been validated with FPGA platform. We have
support for PCIe bus, which is used on FPGA prototyping.
Signed-off-by: Pawel Laszczak <pawell-vna1KIf7WgpBDgjK7y7TUQ@xxxxxxxxxxxxxxxx>
---
drivers/usb/gadget/udc/Kconfig | 2 +
drivers/usb/gadget/udc/Makefile | 1 +
drivers/usb/gadget/udc/cdns2/Kconfig | 11 +
drivers/usb/gadget/udc/cdns2/Makefile | 5 +
drivers/usb/gadget/udc/cdns2/cdns2-ep0.c | 638 +++++
drivers/usb/gadget/udc/cdns2/cdns2-gadget.c | 2426 +++++++++++++++++++
drivers/usb/gadget/udc/cdns2/cdns2-pci.c | 149 ++
7 files changed, 3232 insertions(+)
create mode 100644 drivers/usb/gadget/udc/cdns2/Kconfig
create mode 100644 drivers/usb/gadget/udc/cdns2/Makefile
create mode 100644 drivers/usb/gadget/udc/cdns2/cdns2-ep0.c
create mode 100644 drivers/usb/gadget/udc/cdns2/cdns2-gadget.c
create mode 100644 drivers/usb/gadget/udc/cdns2/cdns2-pci.c
diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
index 83cae6bb12eb..aae1787320d4 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -463,6 +463,8 @@ config USB_ASPEED_UDC
source "drivers/usb/gadget/udc/aspeed-vhub/Kconfig"
+source "drivers/usb/gadget/udc/cdns2/Kconfig"
+
#
# LAST -- dummy/emulated controller
#
diff --git a/drivers/usb/gadget/udc/Makefile b/drivers/usb/gadget/udc/Makefile
index ee569f63c74a..b52f93e9c61d 100644
--- a/drivers/usb/gadget/udc/Makefile
+++ b/drivers/usb/gadget/udc/Makefile
@@ -42,3 +42,4 @@ obj-$(CONFIG_USB_ASPEED_VHUB) += aspeed-vhub/
obj-$(CONFIG_USB_ASPEED_UDC) += aspeed_udc.o
obj-$(CONFIG_USB_BDC_UDC) += bdc/
obj-$(CONFIG_USB_MAX3420_UDC) += max3420_udc.o
+obj-$(CONFIG_USB_CDNS2_UDC) += cdns2/
diff --git a/drivers/usb/gadget/udc/cdns2/Kconfig b/drivers/usb/gadget/udc/cdns2/Kconfig
new file mode 100644
index 000000000000..310db4788353
--- /dev/null
+++ b/drivers/usb/gadget/udc/cdns2/Kconfig
@@ -0,0 +1,11 @@
+config USB_CDNS2_UDC
+ tristate "Cadence USBHS Device Controller"
+ depends on USB_PCI && ACPI && HAS_DMA
+ help
+ Cadence USBHS Device controller is a PCI based USB peripheral
+ controller which supports both full and high speed USB 2.0
+ data transfers.
+
+ Say "y" to link the driver statically, or "m" to build a
+ dynamically linked module called "cdns2-pci.ko" and to
I'm not expert in module naming, but isn't it cdns2-udc-pci?
+ force all gadget drivers to also be dynamically linked.
[...]
+static void cdns2_ep_tx_isoc(struct cdns2_endpoint *pep,
+ struct cdns2_request *preq,
+ int num_trbs)
+{
+ struct scatterlist *sg = NULL;
+ u32 remaining_packet_size = 0;
+ struct cdns2_trb *trb;
+ bool first_trb = true;
+ dma_addr_t trb_dma;
+ u32 trb_buff_len;
+ u32 block_length;
+ int sg_iter = 0;
Not need to init.
+ int sent_len;
+ int td_idx = 0;
+ int split_size;
+ u32 control;
[...]
+/* Prepare and start transfer for all not started requests. */
+static int cdns2_start_all_request(struct cdns2_device *pdev,
+ struct cdns2_endpoint *pep)
+{
+ struct cdns2_request *preq;
+ int ret = 0;
+
+ while (!list_empty(&pep->deferred_list)) {
+ preq = cdns2_next_preq(&pep->deferred_list);
+
+ ret = cdns2_ep_run_transfer(pep, preq);
+ if (ret)
+ return ret;
+
+ list_move_tail(&preq->list, &pep->pending_list);
+ }
+
+ pep->ep_state &= ~EP_RING_FULL;
+
+ return ret;
Maybe return 0; would be more explicit? (and would remove the "= 0" above)
+}
[...]
diff --git a/drivers/usb/gadget/udc/cdns2/cdns2-pci.c b/drivers/usb/gadget/udc/cdns2/cdns2-pci.c
new file mode 100644
index 000000000000..ab2891c79b5c
--- /dev/null
+++ b/drivers/usb/gadget/udc/cdns2/cdns2-pci.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Cadence USBHS-DEV controller - PCI Glue driver.
+ *
+ * Copyright (C) 2023 Cadence.
+ *
+ * Author: Pawel Laszczak <pawell-vna1KIf7WgpBDgjK7y7TUQ@xxxxxxxxxxxxxxxx>
+ *
+ */
+
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/pci.h>
+
+#include "cdns2-gadget.h"
+
+#define PCI_DRIVER_NAME "cdns-pci-usbhs"
+#define CDNS_VENDOR_ID 0x17cd
+#define CDNS_DEVICE_ID 0x0120
+#define PCI_BAR_DEV 0
+#define PCI_DEV_FN_DEVICE 0
+
+static int cdns2_pci_probe(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ resource_size_t rsrc_start, rsrc_len;
+ struct device *dev = &pdev->dev;
+ struct cdns2_device *priv_dev;
+ struct resource *res;
+ int ret;
+
+ /* For GADGET PCI (devfn) function number is 0. */
+ if (!id || pdev->devfn != PCI_DEV_FN_DEVICE ||
+ pdev->class != PCI_CLASS_SERIAL_USB_DEVICE)
+ return -EINVAL;
+
+ ret = pcim_enable_device(pdev);
+ if (ret)
+ dev_err(&pdev->dev, "Enabling PCI device has failed %d\n", ret);
Should we bail out in this case?
+
+ pci_set_master(pdev);
+
+ priv_dev = kzalloc(sizeof(*priv_dev), GFP_KERNEL);
+ if (!priv_dev) {
+ ret = -ENOMEM;
+ goto disable_pci;
Any reason, not to use devm_kzalloc() and manually hanfle kfree() in the
error handling path and in the removbe function ?
+ }
+
+ dev_dbg(dev, "Initialize resources\n");
+ rsrc_start = pci_resource_start(pdev, PCI_BAR_DEV);
+ rsrc_len = pci_resource_len(pdev, PCI_BAR_DEV);
+
+ res = devm_request_mem_region(dev, rsrc_start, rsrc_len, "dev");
+ if (!res) {
+ dev_dbg(dev, "controller already in use\n");
+ ret = -EBUSY;
+ goto free_priv_dev;
+ }
+
+ priv_dev->regs = devm_ioremap(dev, rsrc_start, rsrc_len);
+ if (!priv_dev->regs) {
+ dev_dbg(dev, "error mapping memory\n");
+ ret = -EFAULT;
+ goto free_priv_dev;
+ }
+
+ priv_dev->irq = pdev->irq;
+ dev_dbg(dev, "USBSS-DEV physical base addr: %pa\n",
+ &rsrc_start);
+
+ priv_dev->dev = dev;
+
+ priv_dev->eps_supported = 0x000f000f;
+ priv_dev->onchip_tx_buf = 16;
+ priv_dev->onchip_rx_buf = 16;
+
+ ret = cdns2_gadget_init(priv_dev);
+ if (ret)
+ goto free_priv_dev;
+
+ pci_set_drvdata(pdev, priv_dev);
+
+ device_wakeup_enable(&pdev->dev);
+ if (pci_dev_run_wake(pdev))
+ pm_runtime_put_noidle(&pdev->dev);
+
+ return 0;
+
+free_priv_dev:
+ kfree(priv_dev);
+
+disable_pci:
+ pci_disable_device(pdev);
+
+ return ret;
+}
+
+static void cdns2_pci_remove(struct pci_dev *pdev)
+{
+ struct cdns2_device *priv_dev = pci_get_drvdata(pdev);
+
+ if (pci_dev_run_wake(pdev))
+ pm_runtime_get_noresume(&pdev->dev);
+
+ cdns2_gadget_remove(priv_dev);
+ kfree(priv_dev);
There is a pci_disable_device() in the error handling path of the probe,
but not in the remove function.
Is it on purpose?
Since pcim_enable_device() is used, is it needed above?
CJ
[...]
+static struct pci_driver cdns2_pci_driver = {
+ .name = "cdns2-pci",
+ .id_table = &cdns2_pci_ids[0],
+ .probe = cdns2_pci_probe,
+ .remove = cdns2_pci_remove,
+ .driver = {
+ .pm = pm_ptr(&cdns2_pci_pm_ops),
+ }
+};
+
+module_pci_driver(cdns2_pci_driver);
+MODULE_DEVICE_TABLE(pci, cdns2_pci_ids);
+
+MODULE_ALIAS("pci:cdns2");
+MODULE_AUTHOR("Pawel Laszczak <pawell-vna1KIf7WgpBDgjK7y7TUQ@xxxxxxxxxxxxxxxx>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Cadence CDNS2 PCI driver");