All of lore.kernel.org
 help / color / mirror / Atom feed
From: Felipe Balbi <balbi@kernel.org>
To: Pawel Laszczak <pawell@cadence.com>, devicetree@vger.kernel.org
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
	rogerq@ti.com, linux-kernel@vger.kernel.org,
	adouglas@cadence.com, jbergsagel@ti.com, nsekhar@ti.com,
	nm@ti.com, sureshp@cadence.com, peter.chen@nxp.com,
	pjez@cadence.com, kurahul@cadence.com,
	Pawel Laszczak <pawell@cadence.com>
Subject: Re: [PATCH v1 2/2] usb:cdns3 Add Cadence USB3 DRD Driver
Date: Tue, 11 Dec 2018 14:14:04 +0200	[thread overview]
Message-ID: <87h8fkmfar.fsf@linux.intel.com> (raw)
In-Reply-To: <1544445555-17325-3-git-send-email-pawell@cadence.com>

[-- Attachment #1: Type: text/plain, Size: 19657 bytes --]


Hi,

Pawel Laszczak <pawell@cadence.com> 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@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.

> 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, &regs->ep_sts);
> +	writel(EP_TRADDR_TRADDR(priv_dev->ep0_trb_dma), &regs->ep_traddr);
> +	trace_cdns3_doorbell_ep0(priv_dev->ep0_data_dir ? "ep0in" : "ep0out");
> +
> +	/* TRB should be prepared before starting transfer */
> +	writel(EP_CMD_DRDY, &regs->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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: Felipe Balbi <balbi@kernel.org>
To: devicetree@vger.kernel.org
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
	rogerq@ti.com, linux-kernel@vger.kernel.org,
	adouglas@cadence.com, jbergsagel@ti.com, nsekhar@ti.com,
	nm@ti.com, sureshp@cadence.com, peter.chen@nxp.com,
	pjez@cadence.com, kurahul@cadence.com,
	Pawel Laszczak <pawell@cadence.com>
Subject: Re: [PATCH v1 2/2] usb:cdns3 Add Cadence USB3 DRD Driver
Date: Tue, 11 Dec 2018 14:14:04 +0200	[thread overview]
Message-ID: <87h8fkmfar.fsf@linux.intel.com> (raw)
In-Reply-To: <1544445555-17325-3-git-send-email-pawell@cadence.com>

[-- Attachment #1: Type: text/plain, Size: 19657 bytes --]


Hi,

Pawel Laszczak <pawell@cadence.com> 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@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.

> 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, &regs->ep_sts);
> +	writel(EP_TRADDR_TRADDR(priv_dev->ep0_trb_dma), &regs->ep_traddr);
> +	trace_cdns3_doorbell_ep0(priv_dev->ep0_data_dir ? "ep0in" : "ep0out");
> +
> +	/* TRB should be prepared before starting transfer */
> +	writel(EP_CMD_DRDY, &regs->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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: Felipe Balbi <balbi@kernel.org>
To: Pawel Laszczak <pawell@cadence.com>, devicetree@vger.kernel.org
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
	rogerq@ti.com, linux-kernel@vger.kernel.org,
	adouglas@cadence.com, jbergsagel@ti.com, nsekhar@ti.com,
	nm@ti.com, sureshp@cadence.com, peter.chen@nxp.com,
	pjez@cadence.com, kurahul@cadence.com
Subject: [v1,2/2] usb:cdns3 Add Cadence USB3 DRD Driver
Date: Tue, 11 Dec 2018 14:14:04 +0200	[thread overview]
Message-ID: <87h8fkmfar.fsf@linux.intel.com> (raw)

Hi,

Pawel Laszczak <pawell@cadence.com> 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@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.

> 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, &regs->ep_sts);
> +	writel(EP_TRADDR_TRADDR(priv_dev->ep0_trb_dma), &regs->ep_traddr);
> +	trace_cdns3_doorbell_ep0(priv_dev->ep0_data_dir ? "ep0in" : "ep0out");
> +
> +	/* TRB should be prepared before starting transfer */
> +	writel(EP_CMD_DRDY, &regs->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));
> +}

  parent reply	other threads:[~2018-12-11 12:14 UTC|newest]

Thread overview: 79+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-10 12:39 [PATCH v1 0/2] Introduced new Cadence USBSS DRD Driver Pawel Laszczak
2018-12-10 12:39 ` Pawel Laszczak
2018-12-10 12:39 ` [PATCH v1 1/2] dt-bindings: add binding for USBSS-DRD controller Pawel Laszczak
2018-12-10 12:39   ` [v1,1/2] " Pawel Laszczak
2018-12-10 12:39   ` [PATCH v1 1/2] " Pawel Laszczak
2018-12-11 10:16   ` Roger Quadros
2018-12-11 10:16     ` [v1,1/2] " Roger Quadros
2018-12-11 10:16     ` [PATCH v1 1/2] " Roger Quadros
2018-12-13  9:20     ` Peter Chen
2018-12-13  9:20       ` [v1,1/2] " Peter Chen
2018-12-13  9:25       ` [PATCH v1 1/2] " Pawel Laszczak
2018-12-13  9:25         ` [v1,1/2] " Pawel Laszczak
2018-12-20 20:01   ` [PATCH v1 1/2] " Rob Herring
2018-12-20 20:01     ` [v1,1/2] " Rob Herring
2018-12-22 22:24     ` [PATCH v1 1/2] " Pawel Laszczak
2018-12-22 22:24       ` [v1,1/2] " Pawel Laszczak
2018-12-27 21:01       ` [PATCH v1 1/2] " Rob Herring
2018-12-27 21:01         ` [v1,1/2] " Rob Herring
2018-12-31  5:35         ` [PATCH v1 1/2] " Pawel Laszczak
2018-12-31  5:35           ` [v1,1/2] " Pawel Laszczak
2018-12-10 12:39 ` [PATCH v1 2/2] usb:cdns3 Add Cadence USB3 DRD Driver Pawel Laszczak
2018-12-10 12:39   ` [v1,2/2] " Pawel Laszczak
2018-12-10 12:39   ` [PATCH v1 2/2] " Pawel Laszczak
2018-12-11  9:39   ` Roger Quadros
2018-12-11  9:39     ` [v1,2/2] " Roger Quadros
2018-12-11  9:39     ` [PATCH v1 2/2] " Roger Quadros
2018-12-11 10:01     ` Pawel Laszczak
2018-12-11 10:01       ` [v1,2/2] " Pawel Laszczak
2018-12-11 12:15       ` [PATCH v1 2/2] " Felipe Balbi
2018-12-11 12:15         ` [v1,2/2] " Felipe Balbi
2018-12-11 12:15         ` [PATCH v1 2/2] " Felipe Balbi
2018-12-11 11:46     ` Felipe Balbi
2018-12-11 11:46       ` [v1,2/2] " Felipe Balbi
2018-12-11 12:14   ` Felipe Balbi [this message]
2018-12-11 12:14     ` Felipe Balbi
2018-12-11 12:14     ` [PATCH v1 2/2] " Felipe Balbi
2018-12-11 19:04     ` Pawel Laszczak
2018-12-11 19:04       ` [v1,2/2] " Pawel Laszczak
2018-12-11 19:04       ` [PATCH v1 2/2] " Pawel Laszczak
2018-12-12  2:04       ` Peter Chen
2018-12-12  2:04         ` [v1,2/2] " Peter Chen
2018-12-12  6:55         ` [PATCH v1 2/2] " Felipe Balbi
2018-12-12  6:55           ` [v1,2/2] " Felipe Balbi
2018-12-12  7:38           ` [PATCH v1 2/2] " Peter Chen
2018-12-12  7:38             ` [v1,2/2] " Peter Chen
2018-12-12  8:34             ` [PATCH v1 2/2] " Felipe Balbi
2018-12-12  8:34               ` [v1,2/2] " Felipe Balbi
2018-12-12  8:34               ` [PATCH v1 2/2] " Felipe Balbi
2018-12-12  9:24               ` Peter Chen
2018-12-12  9:24                 ` [v1,2/2] " Peter Chen
2018-12-12 15:53         ` [PATCH v1 2/2] " Bin Liu
2018-12-12 15:53           ` [v1,2/2] " Bin Liu
2018-12-12 15:53           ` [PATCH v1 2/2] " Bin Liu
2018-12-13  1:21           ` Peter Chen
2018-12-13  1:21             ` [v1,2/2] " Peter Chen
2018-12-12  6:52       ` [PATCH v1 2/2] " Felipe Balbi
2018-12-12  6:52         ` [v1,2/2] " Felipe Balbi
2018-12-12  6:52         ` [PATCH v1 2/2] " Felipe Balbi
2018-12-14  3:46         ` Kishon Vijay Abraham I
2018-12-14  3:46           ` [v1,2/2] " Kishon Vijay Abraham I
2018-12-17  5:46           ` [PATCH v1 2/2] " Pawel Laszczak
2018-12-17  5:46             ` [v1,2/2] " Pawel Laszczak
2018-12-17 11:25         ` [PATCH v1 2/2] " Pawel Laszczak
2018-12-17 11:25           ` [v1,2/2] " Pawel Laszczak
2018-12-17 11:34           ` [PATCH v1 2/2] " Felipe Balbi
2018-12-17 11:34             ` [v1,2/2] " Felipe Balbi
2018-12-17 11:34             ` [PATCH v1 2/2] " Felipe Balbi
2018-12-17 11:51         ` Pawel Laszczak
2018-12-17 11:51           ` [v1,2/2] " Pawel Laszczak
2018-12-17 11:56           ` [PATCH v1 2/2] " Felipe Balbi
2018-12-17 11:56             ` [v1,2/2] " Felipe Balbi
2018-12-17 11:56             ` [PATCH v1 2/2] " Felipe Balbi
2018-12-13  9:35   ` Peter Chen
2018-12-13  9:35     ` [v1,2/2] " Peter Chen
2018-12-16 13:01     ` [PATCH v1 2/2] " Pawel Laszczak
2018-12-16 13:01       ` [v1,2/2] " Pawel Laszczak
2018-12-14 22:56   ` [PATCH v1 2/2] " kbuild test robot
2018-12-14 22:56     ` [v1,2/2] " kbuild test robot
2018-12-14 22:56     ` [PATCH v1 2/2] " kbuild test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87h8fkmfar.fsf@linux.intel.com \
    --to=balbi@kernel.org \
    --cc=adouglas@cadence.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jbergsagel@ti.com \
    --cc=kurahul@cadence.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=nm@ti.com \
    --cc=nsekhar@ti.com \
    --cc=pawell@cadence.com \
    --cc=peter.chen@nxp.com \
    --cc=pjez@cadence.com \
    --cc=rogerq@ti.com \
    --cc=sureshp@cadence.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.