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?
Yes this error code can be returned by related to phy function if CONFIG_GENERIC_PHY is disabled.
I have seen this warning in 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.
Ok.
>
>> +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()
Ok,
>
>> 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.
I agree,
It would be nice to have a set of decoding function in some generic library. It could be used
also by other drivers.
Who should do this ?
>
>> 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
If address is zero then state should be USB_STATE_DEFAULT. Driver enters to this branch
only if gadget.state = USB_STATE_ADDRESS (address != 0). This state can be changed in composite.c file
after delegating request to gadget driver. Because this state could be changed driver simple
restore USB_STATE_ADDRESS if config = 0.
>
>> + }
>> + break;
>> + case USB_STATE_CONFIGURED:
>
>where do you set this state?
It's set in set_config in composite.c file.
>
>> +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?
>From USB spec:
"The transition to test mode must be complete no later than 3 ms after the completion of the status stage of the
request."
But I don't remember any issues with this test on other ours drivers. Maybe status stage
is armed in this case by controller. I have to confirm how it works with hardware team.
Driver doesn't know when status stage was completed. We don't have
any event on status stage completion. I haven't checked it yet with tester on this driver.
>
>> 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()
Ok.
>
>> +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?
Test mode should be reset automatically on exit.
W can't clear this mode in register. It's WAC (write only and auto clear) bit.
This function only reset endpoint configuration in controller register.
Ep0 max_packet_size should be unchanged here. Ep0 max_packet it's updated on
Connect/Disconnect/Reset events.
Maybe this function should be called cdns3_hw_reset_eps_config.
It doesn't unconfigure whole gadget but only reset endpoints configuration kept by
controller.
I will change this function.
>
>> +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.
Yes, spin_lock_irqsave is not necessary here.
Do you mean replacing devm_request_irq with a request_threaded_irq ?
I have single interrupt line shared between Host, Driver, DRD/OTG.
I'm not sure if it will work more efficiently.
>
>> + /* 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.
Ok, It's not necessary in this place, especially, that there is invoked trace point
inside cdns3_check_usb_interrupt_proceed which makes the same.
>
>> + 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.
Ok
>
>> +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
Ok.
>
>> + 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));
>> +}
>
>--
Thanks
Pawell
