Re: [PATCH v1 2/2] usb:cdns3 Add Cadence USB3 DRD Driver
From: Felipe Balbi <balbi@kernel.org>
Date: 2018-12-11 12:14:19
Also in:
linux-usb, lkml
Hi, Pawel Laszczak [off-list ref] writes:
+static int cdns3_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ struct cdns3 *cdns;
+ void __iomem *regs;
+ int ret;
+
+ cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
+ if (!cdns)
+ return -ENOMEM;
+
+ cdns->dev = dev;
+
+ platform_set_drvdata(pdev, cdns);
+
+ res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (!res) {
+ dev_err(dev, "missing IRQ\n");
+ return -ENODEV;
+ }
+ cdns->irq = res->start;
+
+ cdns->xhci_res[0] = *res;
+
+ /*
+ * Request memory region
+ * region-0: xHCI
+ * region-1: Peripheral
+ * region-2: OTG registers
+ */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ cdns->xhci_res[1] = *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(regs))
+ return PTR_ERR(regs);
+ cdns->dev_regs = regs;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+ regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(regs))
+ return PTR_ERR(regs);
+ cdns->otg_regs = regs;
+
+ mutex_init(&cdns->mutex);
+
+ cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
+ if (IS_ERR(cdns->phy)) {
+ ret = PTR_ERR(cdns->phy);
+ if (ret == -ENOSYS || ret == -ENODEV) {Are you sure you can get ENOSYS here? Have you checked output of checkpatch --strict? -:852: WARNING: ENOSYS means 'invalid syscall nr' and nothing else
+#ifdef CONFIG_PM_SLEEP
+static int cdns3_suspend(struct device *dev)
+{
+ /* TODO: Implements this function. */
+ return 0;
+}
+
+static int cdns3_resume(struct device *dev)
+{
+ /* TODO: Implements this function. */
+ return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+static int cdns3_runtime_suspend(struct device *dev)
+{ /* TODO: Implements this function. */
+ return 0;
+}
+
+static int cdns3_runtime_resume(struct device *dev)
+{
+ /* TODO: Implements this function. */
+ return 0;
+}
+#endif /* CONFIG_PM */please no TODO stubs. Just get rid of your dev_pm_ops if you don't implement them. Come up with a later patch adding a proper implementation for PM.
+static int __init cdns3_driver_platform_register(void)
+{
+ return platform_driver_register(&cdns3_driver);
+}
+module_init(cdns3_driver_platform_register);
+
+static void __exit cdns3_driver_platform_unregister(void)
+{
+ platform_driver_unregister(&cdns3_driver);
+}
+module_exit(cdns3_driver_platform_unregister);module_platform_driver()
quoted hunk ↗ jump to hunk
diff --git a/drivers/usb/cdns3/debug.h b/drivers/usb/cdns3/debug.h new file mode 100644 index 000000000000..afb81d224718 --- /dev/null +++ b/drivers/usb/cdns3/debug.h@@ -0,0 +1,346 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Cadence USBSS DRD Driver. + * Debug header file. + * + * Copyright (C) 2018 Cadence. + * + * Author: Pawel Laszczak <pawell@cadence.com> + */ +#ifndef __LINUX_CDNS3_DEBUG +#define __LINUX_CDNS3_DEBUG +#include "gadget.h" + +static inline void cdns3_decode_get_status(u8 bRequestType, u16 wIndex, + u16 wLength, char *str) +{ + switch (bRequestType & USB_RECIP_MASK) { + case USB_RECIP_INTERFACE: + sprintf(str, "Get Interface Status Intf = %d, L: = %d", + wIndex, wLength); + break; + case USB_RECIP_ENDPOINT: + sprintf(str, "Get Endpoint Status ep%d%s", + wIndex & ~USB_DIR_IN, + wIndex & USB_DIR_IN ? "in" : "out"); + break; + } +} + +static inline const char *cdns3_decode_device_feature(u16 wValue) +{ + switch (wValue) { + case USB_DEVICE_SELF_POWERED: + return "Self Powered"; + case USB_DEVICE_REMOTE_WAKEUP: + return "Remote Wakeup"; + case USB_DEVICE_TEST_MODE: + return "Test Mode"; + case USB_DEVICE_U1_ENABLE: + return "U1 Enable"; + case USB_DEVICE_U2_ENABLE: + return "U2 Enable"; + case USB_DEVICE_LTM_ENABLE: + return "LTM Enable"; + default: + return "UNKNOWN"; + } +} + +static inline const char *cdns3_decode_test_mode(u16 wIndex) +{ + switch (wIndex) { + case TEST_J: + return ": TEST_J"; + case TEST_K: + return ": TEST_K"; + case TEST_SE0_NAK: + return ": TEST_SE0_NAK"; + case TEST_PACKET: + return ": TEST_PACKET"; + case TEST_FORCE_EN: + return ": TEST_FORCE_EN"; + default: + return ": UNKNOWN"; + } +} + +static inline void cdns3_decode_set_clear_feature(u8 bRequestType, u8 bRequest, + u16 wValue, u16 wIndex, + char *str) +{ + switch (bRequestType & USB_RECIP_MASK) { + case USB_RECIP_DEVICE: + sprintf(str, "%s Device Feature(%s%s)", + bRequest == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set", + cdns3_decode_device_feature(wValue), + wValue == USB_DEVICE_TEST_MODE ? + cdns3_decode_test_mode(wIndex) : ""); + break; + case USB_RECIP_INTERFACE: + sprintf(str, "%s Interface Feature(%s)", + bRequest == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set", + wIndex == USB_INTRF_FUNC_SUSPEND ? + "Function Suspend" : "UNKNOWN"); + break; + case USB_RECIP_ENDPOINT: + sprintf(str, "%s Endpoint Feature(%s ep%d%s)", + bRequest == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set", + wIndex == USB_ENDPOINT_HALT ? "Halt" : "UNKNOWN", + wIndex & ~USB_DIR_IN, + wIndex & USB_DIR_IN ? "in" : "out"); + break; + } +} + +static inline const char *cdns3_decode_descriptor(u16 wValue) +{ + switch (wValue >> 8) { + case USB_DT_DEVICE: + return "Device"; + case USB_DT_CONFIG: + return "Configuration"; + case USB_DT_STRING: + return "String"; + case USB_DT_INTERFACE: + return "Interface"; + case USB_DT_ENDPOINT: + return "Endpoint"; + case USB_DT_DEVICE_QUALIFIER: + return "Device Qualifier"; + case USB_DT_OTHER_SPEED_CONFIG: + return "Other Speed Config"; + case USB_DT_INTERFACE_POWER: + return "Interface Power"; + case USB_DT_OTG: + return "OTG"; + case USB_DT_DEBUG: + return "Debug"; + case USB_DT_INTERFACE_ASSOCIATION: + return "Interface Association"; + case USB_DT_BOS: + return "BOS"; + case USB_DT_DEVICE_CAPABILITY: + return "Device Capability"; + case USB_DT_SS_ENDPOINT_COMP: + return "SS Endpoint Companion"; + case USB_DT_SSP_ISOC_ENDPOINT_COMP: + return "SSP Isochronous Endpoint Companion"; + default: + return "UNKNOWN"; + } +} + +/** + * cdns3_decode_ctrl - returns a string represetion of ctrl request + */ +static inline const char *cdns3_decode_ctrl(char *str, u8 bRequestType, + u8 bRequest, u16 wValue, + u16 wIndex, u16 wLength) +{ + switch (bRequest) { + case USB_REQ_GET_STATUS: + cdns3_decode_get_status(bRequestType, wIndex, + wLength, str); + break; + case USB_REQ_CLEAR_FEATURE: + case USB_REQ_SET_FEATURE: + cdns3_decode_set_clear_feature(bRequestType, bRequest, + wValue, wIndex, str); + break; + case USB_REQ_SET_ADDRESS: + sprintf(str, "Set Address Addr: %02x", wValue); + break; + case USB_REQ_GET_DESCRIPTOR: + sprintf(str, "GET %s Descriptor I: %d, L: %d", + cdns3_decode_descriptor(wValue), + wValue & 0xff, wLength); + break; + case USB_REQ_SET_DESCRIPTOR: + sprintf(str, "SET %s Descriptor I: %d, L: %d", + cdns3_decode_descriptor(wValue), + wValue & 0xff, wLength); + break; + case USB_REQ_GET_CONFIGURATION: + sprintf(str, "Get Configuration L: %d", wLength); + break; + case USB_REQ_SET_CONFIGURATION: + sprintf(str, "Set Configuration Config: %d ", wValue); + break; + case USB_REQ_GET_INTERFACE: + sprintf(str, "Get Interface Intf: %d, L: %d", wIndex, wLength); + break; + case USB_REQ_SET_INTERFACE: + sprintf(str, "Set Interface Intf: %d, Alt: %d", wIndex, wValue); + break; + case USB_REQ_SYNCH_FRAME: + sprintf(str, "Synch Frame Ep: %d, L: %d", wIndex, wLength); + break; + case USB_REQ_SET_SEL: + sprintf(str, "Set SEL L: %d", wLength); + break; + case USB_REQ_SET_ISOCH_DELAY: + sprintf(str, "Set Isochronous Delay Delay: %d ns", wValue); + break; + default: + sprintf(str, + "SETUP BRT: %02x BR: %02x V: %04x I: %04x L: %04x\n", + bRequestType, bRequest, + wValue, wIndex, wLength); + } + + return str; +}
All of these are a flat out copy of dwc3's implementation. It's much, much better to turn dwc3's implementation into a generic, reusable library function then spinning your own as a duplicated effort.
quoted hunk ↗ jump to hunk
diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c new file mode 100644 index 000000000000..1ef0e9f73e3e --- /dev/null +++ b/drivers/usb/cdns3/ep0.c@@ -0,0 +1,864 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Cadence USBSS DRD Driver - gadget side. + * + * Copyright (C) 2018 Cadence Design Systems. + * Copyright (C) 2017-2018 NXP + * + * Authors: Pawel Jez <pjez@cadence.com>, + * Pawel Laszczak <pawell@cadence.com> + * Peter Chen <peter.chen@nxp.com> + */ + +#include <linux/usb/composite.h> + +#include "gadget.h" +#include "trace.h" + +static struct usb_endpoint_descriptor cdns3_gadget_ep0_desc = { + .bLength = USB_DT_ENDPOINT_SIZE, + .bDescriptorType = USB_DT_ENDPOINT, + .bmAttributes = USB_ENDPOINT_XFER_CONTROL, +}; + +/** + * cdns3_ep0_run_transfer - Do transfer on default endpoint hardware + * @priv_dev: extended gadget object + * @dma_addr: physical address where data is/will be stored + * @length: data length + * @erdy: set it to 1 when ERDY packet should be sent - + * exit from flow control state + */ +static void cdns3_ep0_run_transfer(struct cdns3_device *priv_dev, + dma_addr_t dma_addr, + unsigned int length, int erdy) +{ + struct cdns3_usb_regs __iomem *regs = priv_dev->regs; + struct cdns3_endpoint *priv_ep = ep_to_cdns3_ep(priv_dev->gadget.ep0); + + priv_dev->ep0_trb->buffer = TRB_BUFFER(dma_addr); + priv_dev->ep0_trb->length = TRB_LEN(length); + priv_dev->ep0_trb->control = TRB_CYCLE | TRB_IOC | TRB_TYPE(TRB_NORMAL); + + trace_cdns3_prepare_trb(priv_ep, priv_dev->ep0_trb); + + cdns3_select_ep(priv_dev, priv_dev->ep0_data_dir); + + writel(EP_STS_TRBERR, ®s->ep_sts); + writel(EP_TRADDR_TRADDR(priv_dev->ep0_trb_dma), ®s->ep_traddr); + trace_cdns3_doorbell_ep0(priv_dev->ep0_data_dir ? "ep0in" : "ep0out"); + + /* TRB should be prepared before starting transfer */ + writel(EP_CMD_DRDY, ®s->ep_cmd); + + if (erdy) + writel(EP_CMD_ERDY, &priv_dev->regs->ep_cmd); +} + +/** + * cdns3_ep0_delegate_req - Returns status of handling setup packet + * Setup is handled by gadget driver + * @priv_dev: extended gadget object + * @ctrl_req: pointer to received setup packet + * + * Returns zero on success or negative value on failure + */ +static int cdns3_ep0_delegate_req(struct cdns3_device *priv_dev, + struct usb_ctrlrequest *ctrl_req) +{ + int ret; + + spin_unlock(&priv_dev->lock); + priv_dev->setup_pending = 1; + ret = priv_dev->gadget_driver->setup(&priv_dev->gadget, ctrl_req); + priv_dev->setup_pending = 0; + spin_lock(&priv_dev->lock); + return ret; +} + +static void cdns3_prepare_setup_packet(struct cdns3_device *priv_dev) +{ + priv_dev->ep0_data_dir = 0; + cdns3_ep0_run_transfer(priv_dev, priv_dev->setup_dma, + sizeof(struct usb_ctrlrequest), 0); +} + +/** + * cdns3_req_ep0_set_configuration - Handling of SET_CONFIG standard USB request + * @priv_dev: extended gadget object + * @ctrl_req: pointer to received setup packet + * + * Returns 0 if success, USB_GADGET_DELAYED_STATUS on deferred status stage, + * error code on error + */ +static int cdns3_req_ep0_set_configuration(struct cdns3_device *priv_dev, + struct usb_ctrlrequest *ctrl_req) +{ + enum usb_device_state device_state = priv_dev->gadget.state; + struct cdns3_endpoint *priv_ep; + u32 config = le16_to_cpu(ctrl_req->wValue); + int result = 0; + int i; + + switch (device_state) { + case USB_STATE_ADDRESS: + /* Configure non-control EPs */ + for (i = 0; i < CDNS3_ENDPOINTS_MAX_COUNT; i++) { + priv_ep = priv_dev->eps[i]; + if (!priv_ep) + continue; + + if (priv_ep->flags & EP_CLAIMED) + cdns3_ep_config(priv_ep); + } + + result = cdns3_ep0_delegate_req(priv_dev, ctrl_req); + + if (result) + return result; + + if (config) { + cdns3_set_hw_configuration(priv_dev); + } else { + cdns3_gadget_unconfig(priv_dev); + usb_gadget_set_state(&priv_dev->gadget, + USB_STATE_ADDRESS);
this is wrong. If address is zero, state should be default, not addressed. Addressed would be used on the other branch here, when you have a non-zero address
+ } + break; + case USB_STATE_CONFIGURED:
where do you set this state?
+static int cdns3_ep0_feature_handle_device(struct cdns3_device *priv_dev,
+ struct usb_ctrlrequest *ctrl,
+ int set)
+{
+ enum usb_device_state state;
+ enum usb_device_speed speed;
+ int ret = 0;
+ u32 wValue;
+ u32 wIndex;
+ u16 tmode;
+
+ wValue = le16_to_cpu(ctrl->wValue);
+ wIndex = le16_to_cpu(ctrl->wIndex);
+ state = priv_dev->gadget.state;
+ speed = priv_dev->gadget.speed;
+
+ switch (ctrl->wValue) {
+ case USB_DEVICE_REMOTE_WAKEUP:
+ priv_dev->wake_up_flag = !!set;
+ break;
+ case USB_DEVICE_U1_ENABLE:
+ if (state != USB_STATE_CONFIGURED || speed != USB_SPEED_SUPER)
+ return -EINVAL;
+
+ priv_dev->u1_allowed = !!set;
+ break;
+ case USB_DEVICE_U2_ENABLE:
+ if (state != USB_STATE_CONFIGURED || speed != USB_SPEED_SUPER)
+ return -EINVAL;
+
+ priv_dev->u2_allowed = !!set;
+ break;
+ case USB_DEVICE_LTM_ENABLE:
+ ret = -EINVAL;
+ break;
+ case USB_DEVICE_TEST_MODE:
+ if (state != USB_STATE_CONFIGURED || speed > USB_SPEED_HIGH)
+ return -EINVAL;
+
+ tmode = le16_to_cpu(ctrl->wIndex);
+
+ if (!set || (tmode & 0xff) != 0)
+ return -EINVAL;
+
+ switch (tmode >> 8) {
+ case TEST_J:
+ case TEST_K:
+ case TEST_SE0_NAK:
+ case TEST_PACKET:
+ cdns3_set_register_bit(&priv_dev->regs->usb_cmd,
+ USB_CMD_STMODE |
+ USB_STS_TMODE_SEL(tmode - 1));I'm 90% sure this won't work. There's a reason why we only enter the requested test mode from status stage. How have you tested this?
quoted hunk ↗ jump to hunk
diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c new file mode 100644 index 000000000000..a021eaf07aee --- /dev/null +++ b/drivers/usb/cdns3/gadget.c
<snip>
+struct usb_request *cdns3_next_request(struct list_head *list)
+{
+ if (list_empty(list))
+ return NULL;
+ return list_first_entry(list, struct usb_request, list);list_first_entry_or_null()
+void cdns3_gadget_unconfig(struct cdns3_device *priv_dev)
+{
+ /* RESET CONFIGURATION */
+ writel(USB_CONF_CFGRST, &priv_dev->regs->usb_conf);
+
+ cdns3_allow_enable_l1(priv_dev, 0);
+ priv_dev->hw_configured_flag = 0;
+ priv_dev->onchip_mem_allocated_size = 0;clear all test modes? Reset ep0 max_packet_size to 512?
+static irqreturn_t cdns3_device_irq_handler(int irq, void *data)
+{
+ struct cdns3_device *priv_dev;
+ struct cdns3 *cdns = data;
+ irqreturn_t ret = IRQ_NONE;
+ unsigned long flags;
+ u32 reg;
+
+ priv_dev = cdns->gadget_dev;
+ spin_lock_irqsave(&priv_dev->lock, flags);you're already running in hardirq context. Why do you need this lock at all? I would be better to use the hardirq handler to mask your interrupts, so they don't fire again, then used the top-half (softirq) handler to actually handle the interrupts.
+ /* check USB device interrupt */
+ reg = readl(&priv_dev->regs->usb_ists);
+ writel(reg, &priv_dev->regs->usb_ists);
+
+ if (reg) {
+ dev_dbg(priv_dev->dev, "IRQ: usb_ists: %08X\n", reg);I strongly advise against using dev_dbg() for debugging. Even more so inside your IRQ handler.
+ cdns3_check_usb_interrupt_proceed(priv_dev, reg);
+ ret = IRQ_HANDLED;
+ }
+
+ /* check endpoint interrupt */
+ reg = readl(&priv_dev->regs->ep_ists);
+
+ /* handle default endpoint OUT */
+ if (reg & EP_ISTS_EP_OUT0) {
+ cdns3_check_ep0_interrupt_proceed(priv_dev, USB_DIR_OUT);
+ ret = IRQ_HANDLED;
+ }
+
+ /* handle default endpoint IN */
+ if (reg & EP_ISTS_EP_IN0) {
+ cdns3_check_ep0_interrupt_proceed(priv_dev, USB_DIR_IN);
+ ret = IRQ_HANDLED;
+ }
+
+ /* check if interrupt from non default endpoint, if no exit */
+ reg &= ~(EP_ISTS_EP_OUT0 | EP_ISTS_EP_IN0);
+ if (!reg)
+ goto irqend;
+
+ do {
+ unsigned int bit_pos = ffs(reg);
+ u32 bit_mask = 1 << (bit_pos - 1);
+ int index;
+
+ index = cdns3_ep_reg_pos_to_index(bit_pos);
+ cdns3_check_ep_interrupt_proceed(priv_dev->eps[index]);
+ reg &= ~bit_mask;
+ ret = IRQ_HANDLED;
+ } while (reg);use for_each_set_bit() here.
+void cdns3_ep_config(struct cdns3_endpoint *priv_ep)
+{
+ bool is_iso_ep = (priv_ep->type == USB_ENDPOINT_XFER_ISOC);
+ struct cdns3_device *priv_dev = priv_ep->cdns3_dev;
+ u32 bEndpointAddress = priv_ep->num | priv_ep->dir;
+ u32 interrupt_mask = EP_STS_EN_TRBERREN;
+ u32 max_packet_size = 0;
+ u32 ep_cfg = 0;
+ int ret;
+
+ if (!priv_ep->dir)
+ interrupt_mask |= EP_STS_EN_DESCMISEN;
+
+ if (priv_ep->type == USB_ENDPOINT_XFER_INT) {you can turn tis into a switch statement. It'll look nicer
+ ep_cfg = EP_CFG_EPTYPE(USB_ENDPOINT_XFER_INT);
+ } else if (priv_ep->type == USB_ENDPOINT_XFER_BULK) {
+ ep_cfg = EP_CFG_EPTYPE(USB_ENDPOINT_XFER_BULK);
+ } else {
+ ep_cfg = EP_CFG_EPTYPE(USB_ENDPOINT_XFER_ISOC);
+ interrupt_mask = 0xFFFFFFFF;
+ }
+
+ switch (priv_dev->gadget.speed) {
+ case USB_SPEED_FULL:
+ max_packet_size = is_iso_ep ? 1023 : 64;
+ break;
+ case USB_SPEED_HIGH:
+ max_packet_size = is_iso_ep ? 1024 : 512;
+ break;
+ case USB_SPEED_SUPER:
+ max_packet_size = 1024;
+ break;
+ default:
+ /* all other speed are not supported */
+ return;
+ }
+
+ ret = cdns3_ep_onchip_buffer_reserve(priv_dev, CDNS3_EP_BUF_SIZE);
+ if (ret) {
+ dev_err(priv_dev->dev, "onchip mem is full, ep is invalid\n");
+ return;
+ }
+
+ ep_cfg |= EP_CFG_MAXPKTSIZE(max_packet_size) |
+ EP_CFG_BUFFERING(CDNS3_EP_BUF_SIZE - 1) |
+ EP_CFG_MAXBURST(priv_ep->endpoint.maxburst);
+
+ cdns3_select_ep(priv_dev, bEndpointAddress);
+
+ writel(ep_cfg, &priv_dev->regs->ep_cfg);
+ writel(interrupt_mask, &priv_dev->regs->ep_sts_en);
+
+ dev_dbg(priv_dev->dev, "Configure %s: with val %08x\n",
+ priv_ep->name, ep_cfg);
+
+ /* enable interrupt for selected endpoint */
+ cdns3_set_register_bit(&priv_dev->regs->ep_ien,
+ cdns3_ep_addr_to_bit_pos(bEndpointAddress));
+}-- balbi
Attachments
- signature.asc [application/pgp-signature] 832 bytes