Hi, Pawel Laszczak <pawell@xxxxxxxxxxx> 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() > 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@xxxxxxxxxxx> > + */ > +#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. > 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@xxxxxxxxxxx>, > + * Pawel Laszczak <pawell@xxxxxxxxxxx> > + * Peter Chen <peter.chen@xxxxxxx> > + */ > + > +#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? > 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
Attachment:
signature.asc
Description: PGP signature