Message-ID: <BYAPR07MB4709F7183F252D83BD6994F5DDD30@BYAPR07MB4709.namprd07.prod.outlook.com>
Date:   Fri, 30 Nov 2018 14:36:40 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Roger Quadros <rogerq@...com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Felipe Balbi <balbi@...nel.org>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Alan Douglas <adouglas@...ence.com>,
        "jbergsagel@...com" <jbergsagel@...com>,
        "nsekhar@...com" <nsekhar@...com>, "nm@...com" <nm@...com>,
        Suresh Punnoose <sureshp@...ence.com>,
        "peter.chen@....com" <peter.chen@....com>,
        Pawel Jez <pjez@...ence.com>, Rahul Kumar <kurahul@...ence.com>
Subject: RE: [RFC PATCH v2 07/15] usb:cdns3: Adds Device mode support -
 initialization.

Hi, 
>
>+Felipe.
>
>Pawel,
>
>Please copy Felipe Balbi as he maintains the USB gadget stack.
>
>On 18/11/18 12:09, Pawel Laszczak wrote:
>> Patch implements a set of functions responsible for initialization,
>> configuration, starting and stopping device mode.
>> This patch also adds new ep0.c that holds all functions related
>> to endpoint 0.
>>
>> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
>> ---
>>  drivers/usb/cdns3/Kconfig         |  10 +
>>  drivers/usb/cdns3/Makefile        |   1 +
>>  drivers/usb/cdns3/core.c          |   5 +-
>>  drivers/usb/cdns3/ep0.c           | 105 ++++++++
>>  drivers/usb/cdns3/gadget-export.h |  27 +++
>>  drivers/usb/cdns3/gadget.c        | 390 ++++++++++++++++++++++++++++++
>>  drivers/usb/cdns3/gadget.h        |   4 +
>>  7 files changed, 541 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/usb/cdns3/ep0.c
>>  create mode 100644 drivers/usb/cdns3/gadget-export.h
>>  create mode 100644 drivers/usb/cdns3/gadget.c
>>
>> diff --git a/drivers/usb/cdns3/Kconfig b/drivers/usb/cdns3/Kconfig
>> index d92bc3d68eb0..b7d71b5c4f60 100644
>> --- a/drivers/usb/cdns3/Kconfig
>> +++ b/drivers/usb/cdns3/Kconfig
>> @@ -10,6 +10,16 @@ config USB_CDNS3
>>
>>  if USB_CDNS3
>>
>> +config USB_CDNS3_GADGET
>> +        bool "Cadence USB3 device controller"
>> +        depends on USB_GADGET
>> +        help
>> +          Say Y here to enable device controller functionality of the
>> +          cadence USBSS-DEV driver.
>> +
>> +          This controller support FF, HS and SS mode. It doeasn't support
>
>s/support/supports
>s/doeasn't/doesn't
>
>> +          LS and SSP mode
>> +
>>  config USB_CDNS3_HOST
>>          bool "Cadence USB3 host controller"
>>          depends on USB_XHCI_HCD
>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>> index 976117ba67ff..bea6173bf37f 100644
>> --- a/drivers/usb/cdns3/Makefile
>> +++ b/drivers/usb/cdns3/Makefile
>> @@ -2,5 +2,6 @@ obj-$(CONFIG_USB_CDNS3)			+= cdns3.o
>>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)	+= cdns3-pci.o
>>
>>  cdns3-y					:= core.o drd.o
>> +cdns3-$(CONFIG_USB_CDNS3_GADGET)	+= gadget.o ep0.o
>>  cdns3-$(CONFIG_USB_CDNS3_HOST)          += host.o
>>  cdns3-pci-y		 		:= cdns3-pci-wrap.o
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index 4cb820be9ff3..1fa233415901 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -18,6 +18,7 @@
>>  #include "gadget.h"
>>  #include "core.h"
>>  #include "host-export.h"
>> +#include "gadget-export.h"
>>  #include "drd.h"
>>
>>  static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
>> @@ -104,7 +105,8 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>  	}
>>
>>  	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
>> -		//TODO: implements device initialization
>> +		if (cdns3_gadget_init(cdns))
>> +			dev_info(dev, "doesn't support gadget\n");
>
>dev_err() and we should should error out with error code returned by cdns3_gadget_init().
Ok, 
>
>>  	}
>>
>>  	if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
>> @@ -144,6 +146,7 @@ static irqreturn_t cdns3_irq(int irq, void *data)
>>
>>  static void cdns3_remove_roles(struct cdns3 *cdns)
>>  {
>
>if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL)

I had to  little change this  fragment. In the latest version only single driver (role) can be 
initialized and started so this code has changed to :

static void cdns3_exit_roles(struct cdns3 *cdns)
{
	cdns3_role_stop(cdns);
	cdns3_drd_exit(cdns);
}

Functions  cdns3_gadget_remove and cdns3_host_remove are no longer needed.

>> +	cdns3_gadget_remove(cdns);
>>  	cdns3_host_remove(cdns);
>>  }
>>
>> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
>> new file mode 100644
>> index 000000000000..c08d02665f9d
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/ep0.c
>> @@ -0,0 +1,105 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Cadence USBSS DRD Driver - gadget side.
>> + *
>> + * Copyright (C) 2018 Cadence Design Systems.
>> + * Copyright (C) 2017 NXP
>> + *
>> + * Authors: Pawel Jez <pjez@...ence.com>,
>> + *          Pawel Laszczak <pawell@...ence.com>
>> + *	    Peter Chen <peter.chen@....com>
>> + */
>> +
>> +#include "gadget.h"
>> +
>> +static struct usb_endpoint_descriptor cdns3_gadget_ep0_desc = {
>> +	.bLength = USB_DT_ENDPOINT_SIZE,
>> +	.bDescriptorType = USB_DT_ENDPOINT,
>> +	.bmAttributes =	USB_ENDPOINT_XFER_CONTROL,
>> +};
>> +
>> +static void cdns3_prepare_setup_packet(struct cdns3_device *priv_dev)
>> +{
>> +	//TODO: Implements this function
>> +}
>> +
>> +/**
>> + * cdns3_ep0_config - Configures default endpoint
>> + * @priv_dev: extended gadget object
>> + *
>> + * Functions sets parameters: maximal packet size and enables interrupts
>> + */
>> +void cdns3_ep0_config(struct cdns3_device *priv_dev)
>> +{
>> +	struct cdns3_usb_regs __iomem *regs;
>> +	u32 max_packet_size = 64;
>> +
>> +	regs = priv_dev->regs;
>> +
>> +	if (priv_dev->gadget.speed == USB_SPEED_SUPER)
>> +		max_packet_size = 512;
>
>is gadget.speed known at this point? I think it will only be known after a connection
>is made.

This function is called on connection, on reset event and during initialization gadget. 
During initialization the speed will be set to 64 and then will be updated to correct value. 

>> +
>> +	if (priv_dev->ep0_request) {
>> +		list_del_init(&priv_dev->ep0_request->list);
>> +		priv_dev->ep0_request = NULL;
>> +	}
>> +
>> +	priv_dev->gadget.ep0->maxpacket = max_packet_size;
>> +	cdns3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(max_packet_size);
>> +
>> +	/* init ep out */
>> +	cdns3_select_ep(priv_dev, USB_DIR_OUT);
>> +
>> +	writel(EP_CFG_ENABLE | EP_CFG_MAXPKTSIZE(max_packet_size),
>> +	       &regs->ep_cfg);
>> +
>> +	writel(EP_STS_EN_SETUPEN | EP_STS_EN_DESCMISEN | EP_STS_EN_TRBERREN,
>> +	       &regs->ep_sts_en);
>> +
>> +	/* init ep in */
>> +	cdns3_select_ep(priv_dev, USB_DIR_IN);
>> +
>> +	writel(EP_CFG_ENABLE | EP_CFG_MAXPKTSIZE(max_packet_size),
>> +	       &regs->ep_cfg);
>> +
>> +	writel(EP_STS_EN_SETUPEN | EP_STS_EN_TRBERREN, &regs->ep_sts_en);
>> +
>> +	cdns3_set_register_bit(&regs->usb_conf, USB_CONF_U1DS | USB_CONF_U2DS);
>> +	cdns3_prepare_setup_packet(priv_dev);
>
>What happens if gadget.speed is USB_SPEED_SUPER, but was connected to a high-speed host?
>Are you updating the max_packet_size on connection done?

As above.
>
>> +}
>> +
>> +/**
>> + * cdns3_init_ep0 Initializes software endpoint 0 of gadget
>> + * @cdns3: extended gadget object
>> + *
>> + * Returns 0 on success, error code elsewhere
>> + */
>> +int cdns3_init_ep0(struct cdns3_device *priv_dev)
>> +{
>> +	struct cdns3_endpoint *ep0;
>> +
>> +	ep0 = devm_kzalloc(&priv_dev->dev, sizeof(struct cdns3_endpoint),
>> +			   GFP_KERNEL);
>> +
>> +	if (!ep0)
>> +		return -ENOMEM;
>> +
>> +	ep0->cdns3_dev = priv_dev;
>> +	sprintf(ep0->name, "ep0");
>> +
>> +	/* fill linux fields */
>> +	//TODO: implements cdns3_gadget_ep0_ops object
>> +	//ep0->endpoint.ops = &cdns3_gadget_ep0_ops;
>> +	ep0->endpoint.maxburst = 1;
>> +	usb_ep_set_maxpacket_limit(&ep0->endpoint, ENDPOINT0_MAX_PACKET_LIMIT);
>> +	ep0->endpoint.address = 0;
>> +	ep0->endpoint.caps.type_control = 1;
>> +	ep0->endpoint.caps.dir_in = 1;
>> +	ep0->endpoint.caps.dir_out = 1;
>> +	ep0->endpoint.name = ep0->name;
>> +	ep0->endpoint.desc = &cdns3_gadget_ep0_desc;
>> +	priv_dev->gadget.ep0 = &ep0->endpoint;
>> +	INIT_LIST_HEAD(&ep0->request_list);
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/usb/cdns3/gadget-export.h b/drivers/usb/cdns3/gadget-export.h
>> new file mode 100644
>> index 000000000000..257e5e0eef31
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/gadget-export.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Cadence USBSS DRD Driver -Gadget Export APIs
>> + *
>> + * Copyright (C) 2017 NXP
>> + *
>> + * Authors: Peter Chen <peter.chen@....com>
>> + */
>> +#ifndef __LINUX_CDNS3_GADGET_EXPORT
>> +#define __LINUX_CDNS3_GADGET_EXPORT
>> +
>> +#ifdef CONFIG_USB_CDNS3_GADGET
>> +
>> +int cdns3_gadget_init(struct cdns3 *cdns);
>> +void cdns3_gadget_remove(struct cdns3 *cdns);
>> +#else
>> +
>> +static inline int cdns3_gadget_init(struct cdns3 *cdns)
>> +{
>> +	return -ENXIO;
>> +}
>> +
>> +static inline void cdns3_gadget_remove(struct cdns3 *cdns) { }
>> +
>> +#endif
>> +
>> +#endif /* __LINUX_CDNS3_GADGET_EXPORT */
>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>> new file mode 100644
>> index 000000000000..376b68b13d1b
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/gadget.c
>> @@ -0,0 +1,390 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Cadence USBSS DRD Driver - gadget side.
>> + *
>> + * Copyright (C) 2018 Cadence Design Systems.
>> + * Copyright (C) 2017 NXP
>> + *
>> + * Authors: Pawel Jez <pjez@...ence.com>,
>> + *          Pawel Laszczak <pawell@...ence.com>
>> + *	    Peter Chen <peter.chen@....com>
>> + */
>> +
>> +#include <linux/dma-mapping.h>
>> +#include <linux/usb/gadget.h>
>> +
>> +#include "core.h"
>> +#include "gadget-export.h"
>> +#include "gadget.h"
>> +
>> +/**
>> + * cdns3_set_register_bit - set bit in given register.
>> + * @ptr: address of device controller register to be read and changed
>> + * @mask: bits requested to set
>> + */
>> +void cdns3_set_register_bit(void __iomem *ptr, u32 mask)
>> +{
>> +	mask = readl(ptr) | mask;
>> +	writel(mask, ptr);
>> +}
>
>static inline?
>I'd get rid of this function if possible. You are using it only once
>and it is easier to read code if setting the bitmask is done plainly
>instead of hiding it behind a function.

It will be used at least 6 times in the entire driver. 

>
>> +
>> +/**
>> + * select_ep - selects endpoint
>> + * @priv_dev:  extended gadget object
>> + * @ep: endpoint address
>> + */
>> +void cdns3_select_ep(struct cdns3_device *priv_dev, u32 ep)
>> +{
>> +	if (priv_dev->selected_ep == ep)
>> +		return;
>
>I didn't understand the purpose of this function. You can only select the EP once?

Yes I can, but the endpoint is selected in many place in the driver and sometimes the same endpoint 
Is selected many times subsequently. 
Access to register can be slow, so this solution can improve performance. 
If I'm wrong please correct me. 
>
>> +
>> +	dev_dbg(&priv_dev->dev, "Ep sel: 0x%02x\n", ep);
>
>You should move to use trace-points instead of dev_dbg.

Yes, I know in next patch I will add trace-point
>
>> +	priv_dev->selected_ep = ep;
>> +	writel(ep, &priv_dev->regs->ep_sel);
>> +}
>> +
>> +/**
>> + * cdns3_irq_handler - irq line interrupt handler
>> + * @cdns: cdns3 instance
>> + *
>> + * Returns IRQ_HANDLED when interrupt raised by USBSS_DEV,
>> + * IRQ_NONE when interrupt raised by other device connected
>> + * to the irq line
>> + */
>> +static irqreturn_t cdns3_irq_handler_thread(struct cdns3 *cdns)
>> +{
>> +	irqreturn_t ret = IRQ_NONE;
>> +	//TODO: implements this function
>> +	return ret;
>> +}
>> +
>> +static void cdns3_gadget_config(struct cdns3_device *priv_dev)
>> +{
>> +	struct cdns3_usb_regs __iomem *regs = priv_dev->regs;
>> +
>> +	cdns3_ep0_config(priv_dev);
>> +
>> +	/* enable interrupts for endpoint 0 (in and out) */
>> +	writel(EP_IEN_EP_OUT0 | EP_IEN_EP_IN0, &regs->ep_ien);
>> +
>> +	/* enable generic interrupt*/
>> +	writel(USB_IEN_INIT, &regs->usb_ien);
>> +	writel(USB_CONF_CLK2OFFDS | USB_CONF_L1DS, &regs->usb_conf);
>> +	writel(USB_CONF_DMULT, &regs->usb_conf);
>> +	writel(USB_CONF_DEVEN, &regs->usb_conf);
>
>If you are enabling interrupts in this patch you should handle them in the ISR.
>
>> +}
>> +
>> +/**
>> + * cdns3_init_ep Initializes software endpoints of gadget
>> + * @cdns3: extended gadget object
>> + *
>> + * Returns 0 on success, error code elsewhere
>> + */
>> +static int cdns3_init_ep(struct cdns3_device *priv_dev)
>> +{
>> +	u32 ep_enabled_reg, iso_ep_reg;
>> +	struct cdns3_endpoint *priv_ep;
>> +	int found_endpoints = 0;
>> +	int ep_dir, ep_number;
>> +	u32 ep_mask;
>> +	int i;
>> +
>> +	/* Read it from USB_CAP3 to USB_CAP5 */
>> +	ep_enabled_reg = readl(&priv_dev->regs->usb_cap3);
>> +	iso_ep_reg = readl(&priv_dev->regs->usb_cap4);
>> +
>> +	dev_dbg(&priv_dev->dev, "Initializing non-zero endpoints\n");
>> +
>> +	for (i = 0; i < USB_SS_ENDPOINTS_MAX_COUNT; i++) {
>
>CDNS3_USB_SS_ENDPOINTS_MAX_COUNT

Ok, consistence in naming is important. 
>
>> +		ep_number = (i / 2) + 1;
>> +		ep_dir = i % 2;
>> +		ep_mask = BIT((16 * ep_dir) + ep_number);
>> +
>> +		if (!(ep_enabled_reg & ep_mask))
>> +			continue;
>> +
>> +		priv_ep = devm_kzalloc(&priv_dev->dev, sizeof(*priv_ep),
>> +				       GFP_KERNEL);
>> +		if (!priv_ep)
>> +			return -ENOMEM;
>> +
>> +		/* set parent of endpoint object */
>> +		priv_ep->cdns3_dev = priv_dev;
>> +		priv_dev->eps[found_endpoints++] = priv_ep;
>> +
>> +		snprintf(priv_ep->name, sizeof(priv_ep->name), "ep%d%s",
>> +			 ep_number, !!ep_dir ? "in" : "out");
>> +		priv_ep->endpoint.name = priv_ep->name;
>> +
>> +		usb_ep_set_maxpacket_limit(&priv_ep->endpoint,
>> +					   ENDPOINT_MAX_PACKET_LIMIT);
>
>CDNS3_ENDPOINT_MAX_PACKET_LIMIT
>
>> +		priv_ep->endpoint.max_streams = ENDPOINT_MAX_STREAMS;
>
>CDNS3_ENDPOINT_MAX_STREAMS
>
>> +		//TODO: Add implementation of cdns3_gadget_ep_ops
>> +		//priv_ep->endpoint.ops = &cdns3_gadget_ep_ops;
>> +		if (ep_dir)
>> +			priv_ep->endpoint.caps.dir_in = 1;
>> +		else
>> +			priv_ep->endpoint.caps.dir_out = 1;
>> +
>> +		if (iso_ep_reg & ep_mask)
>> +			priv_ep->endpoint.caps.type_iso = 1;
>> +
>> +		priv_ep->endpoint.caps.type_bulk = 1;
>> +		priv_ep->endpoint.caps.type_int = 1;
>> +		priv_ep->endpoint.maxburst = CDNS3_EP_BUF_SIZE - 1;
>> +
>> +		priv_ep->flags = 0;
>> +
>> +		dev_info(&priv_dev->dev, "Initialized  %s support: %s %s\n",
>> +			 priv_ep->name,
>> +			 priv_ep->endpoint.caps.type_bulk ? "BULK, INT" : "",
>> +			 priv_ep->endpoint.caps.type_iso ? "ISO" : "");
>> +
>> +		list_add_tail(&priv_ep->endpoint.ep_list,
>> +			      &priv_dev->gadget.ep_list);
>> +		INIT_LIST_HEAD(&priv_ep->request_list);
>> +		INIT_LIST_HEAD(&priv_ep->ep_match_pending_list);
>> +	}
>> +
>> +	priv_dev->ep_nums = found_endpoints;
>> +	return 0;
>> +}
>> +
>> +static void cdns3_gadget_release(struct device *dev)
>> +{
>> +	struct cdns3_device *priv_dev;
>> +
>> +	priv_dev = container_of(dev, struct cdns3_device, dev);
>> +	kfree(priv_dev);
>> +}
>> +
>> +static int __cdns3_gadget_init(struct cdns3 *cdns)
>> +{
>> +	struct cdns3_device *priv_dev;
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	priv_dev = kzalloc(sizeof(*priv_dev), GFP_KERNEL);
>> +	if (!priv_dev)
>> +		return -ENOMEM;
>> +
>> +	dev = &priv_dev->dev;
>> +	dev->release = cdns3_gadget_release;
>> +	dev->parent = cdns->dev;
>> +	dev_set_name(dev, "gadget-cdns3");
>> +	cdns->gadget_dev = dev;
>> +
>> +	priv_dev->sysdev = cdns->dev;
>> +	ret = device_register(dev);
>
>Why do you need to create this dummy device? you could just use cdns3->dev.

Peter do you have any suggestion ?

It's works as garbage collector during changing role.  In my opinion this is 
the only advantage.

>
>> +	if (ret)
>> +		goto err1;
>> +
>> +	priv_dev->regs = cdns->dev_regs;
>> +
>> +	/* fill gadget fields */
>> +	priv_dev->gadget.max_speed = USB_SPEED_SUPER;
>> +	priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
>> +	//TODO: Add implementation of cdns3_gadget_ops
>> +	//priv_dev->gadget.ops = &cdns3_gadget_ops;
>> +	priv_dev->gadget.name = "usb-ss-gadget";
>> +	priv_dev->gadget.sg_supported = 1;
>> +	priv_dev->is_connected = 0;
>> +
>> +	spin_lock_init(&priv_dev->lock);
>> +
>> +	priv_dev->in_standby_mode = 1;
>> +
>> +	/* initialize endpoint container */
>> +	INIT_LIST_HEAD(&priv_dev->gadget.ep_list);
>> +	INIT_LIST_HEAD(&priv_dev->ep_match_list);
>> +
>> +	ret = cdns3_init_ep0(priv_dev);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to create endpoint 0\n");
>> +		ret = -ENOMEM;
>
>why not just use the ret as is.
>
>> +		goto err2;
>> +	}
>> +
>> +	ret = cdns3_init_ep(priv_dev);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to create non zero endpoints\n");
>> +		ret = -ENOMEM;
>
>here too

Yes, sure.
>
>> +		goto err2;
>> +	}
>> +
>> +	/* allocate memory for default endpoint TRB */
>> +	priv_dev->trb_ep0 = dma_alloc_coherent(priv_dev->sysdev, 24,
>
>what is 24?
>In error path you are using TRB_SIZE * 2. Should we be using that here too?

Yes we need TRB_SIZE * 2. The second is for DMULT mode. DMA stop working on incorrect TRB.  
>
>> +					       &priv_dev->trb_ep0_dma, GFP_DMA);
>> +	if (!priv_dev->trb_ep0) {
>> +		dev_err(dev, "Failed to allocate memory for ep0 TRB\n");
>> +		ret = -ENOMEM;
>> +		goto err2;
>> +	}
>> +
>> +	/* allocate memory for setup packet buffer */
>> +	priv_dev->setup = dma_alloc_coherent(priv_dev->sysdev, 8,
>
>is 8 enough for all use cases? What about vendor specific setup requests?

It's only for SETUP packet. It always has 8 bytes.  For data stage for vendor request 
buffer will be delivered by vendor gadget drivers. 
>
>> +					     &priv_dev->setup_dma, GFP_DMA);
>> +	if (!priv_dev->setup) {
>> +		dev_err(dev, "Failed to allocate memory for SETUP buffer\n");
>> +		ret = -ENOMEM;
>> +		goto err3;
>> +	}
>> +
>> +	dev_dbg(dev, "Device Controller version: %08x\n",
>> +		readl(&priv_dev->regs->usb_cap6));
>> +	dev_dbg(dev, "USB Capabilities:: %08x\n",
>> +		readl(&priv_dev->regs->usb_cap1));
>> +	dev_dbg(dev, "On-Chip memory cnfiguration: %08x\n",
>> +		readl(&priv_dev->regs->usb_cap2));
>> +
>> +	/* add USB gadget device */
>> +	ret = usb_add_gadget_udc(&priv_dev->dev, &priv_dev->gadget);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to register USB device controller\n");
>> +		goto err4;
>> +	}
>> +
>> +	priv_dev->zlp_buf = kzalloc(ENDPOINT_ZLP_BUF_SIZE, GFP_KERNEL);
>> +	if (!priv_dev->zlp_buf) {
>> +		ret = -ENOMEM;
>> +		goto err4;
>> +	}
>
>how about allocating zlp_buf before usb_add_gadget_udc()?

Yes, sure. It will be safer. 
>
>> +
>> +	return 0;
>> +err4:
>> +	dma_free_coherent(priv_dev->sysdev, 8, priv_dev->setup,
>> +			  priv_dev->setup_dma);
>> +err3:
>> +	dma_free_coherent(priv_dev->sysdev, TRB_SIZE * 2, priv_dev->trb_ep0,
>> +			  priv_dev->trb_ep0_dma);
>> +err2:
>> +	device_del(dev);
>> +err1:
>> +	put_device(dev);
>> +	cdns->gadget_dev = NULL;
>> +	return ret;
>> +}
>> +
>> +/**
>> + * cdns3_gadget_remove: parent must call this to remove UDC
>> + *
>> + * cdns: cdns3 instance
>> + */
>> +void cdns3_gadget_remove(struct cdns3 *cdns)
>
>how about calling this cdns3_gadget_exit() to complememnt cdns3_gadget_init()

Function will be removed in next version. Code will be moved to cdns3_gadget_stop function

>
>> +{
>> +	struct cdns3_device *priv_dev;
>> +
>> +	if (!cdns->roles[CDNS3_ROLE_GADGET])
>> +		return;
>
>why this check? It will lead to an unbalanced exit() and future init().
>
Will be removed.
>> +
>> +	priv_dev = container_of(cdns->gadget_dev, struct cdns3_device, dev);
>> +	usb_del_gadget_udc(&priv_dev->gadget);
>> +	dma_free_coherent(priv_dev->sysdev, 8, priv_dev->setup,
>> +			  priv_dev->setup_dma);
>> +	dma_free_coherent(priv_dev->sysdev, TRB_SIZE * 2, priv_dev->trb_ep0,
>> +			  priv_dev->trb_ep0_dma);
>
>You need to free all memory allocation done in cdns3_init_ep0() and cdns3_init_ep()
>else we eat memory on every role switch if we plan on getting rid of the dummy gadget_dev.

Ok, 

>
>> +	device_unregister(cdns->gadget_dev);
>> +	cdns->gadget_dev = NULL;
>> +	kfree(priv_dev->zlp_buf);
>> +}
>> +
>> +static int cdns3_gadget_start(struct cdns3 *cdns)
>> +{
>> +	struct cdns3_device *priv_dev = container_of(cdns->gadget_dev,
>> +			struct cdns3_device, dev);
>> +	unsigned long flags;
>> +
>> +	pm_runtime_get_sync(cdns->dev);
>
>This is another reason why we shouldn't be creating a dummy gadget_dev.
>PM is tied to cdns->dev.
>
>> +	spin_lock_irqsave(&priv_dev->lock, flags);
>> +	priv_dev->start_gadget = 1;
>> +
>> +	if (!priv_dev->gadget_driver) {
>> +		spin_unlock_irqrestore(&priv_dev->lock, flags);
>> +		return 0;
>> +	}
>> +
>> +	cdns3_gadget_config(priv_dev);
>> +	priv_dev->in_standby_mode = 0;
>> +	spin_unlock_irqrestore(&priv_dev->lock, flags);
>> +	return 0;
>> +}
>> +
>> +static void __cdns3_gadget_stop(struct cdns3 *cdns)
>> +{
>> +	struct cdns3_device *priv_dev;
>> +	unsigned long flags;
>> +
>> +	priv_dev = container_of(cdns->gadget_dev, struct cdns3_device, dev);
>> +
>> +	if (priv_dev->gadget_driver)
>> +		priv_dev->gadget_driver->disconnect(&priv_dev->gadget);
>> +
>> +	usb_gadget_disconnect(&priv_dev->gadget);
>> +	spin_lock_irqsave(&priv_dev->lock, flags);
>> +	priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
>> +
>> +	/* disable interrupt for device */
>> +	writel(0, &priv_dev->regs->usb_ien);
>> +	writel(USB_CONF_DEVDS, &priv_dev->regs->usb_conf);
>> +	priv_dev->start_gadget = 0;
>> +	spin_unlock_irqrestore(&priv_dev->lock, flags);
>> +}
>> +
>> +static void cdns3_gadget_stop(struct cdns3 *cdns)
>> +{
>> +	if (cdns->role == CDNS3_ROLE_GADGET)
>> +		__cdns3_gadget_stop(cdns);
>
>Why worry about role here? That should be job of core/drd driver.

Yes, will be corrected.
>
>> +
>> +	pm_runtime_mark_last_busy(cdns->dev);
>> +	pm_runtime_put_autosuspend(cdns->dev);
>> +}
>> +
>> +static int cdns3_gadget_suspend(struct cdns3 *cdns, bool do_wakeup)
>> +{
>> +	__cdns3_gadget_stop(cdns);
>> +	return 0;
>> +}
>> +
>> +static int cdns3_gadget_resume(struct cdns3 *cdns, bool hibernated)
>> +{
>> +	struct cdns3_device *priv_dev;
>> +	unsigned long flags;
>> +
>> +	priv_dev = container_of(cdns->gadget_dev, struct cdns3_device, dev);
>> +	spin_lock_irqsave(&priv_dev->lock, flags);
>> +	priv_dev->start_gadget = 1;
>
>who is using this start_gadget flag?

It's look like guard protecting before access to register when device is disabled. 
It's no longer necessary in next version and will be removed.  When device role 
Is active then access to registers is enabled. 
>
>> +	if (!priv_dev->gadget_driver) {
>> +		spin_unlock_irqrestore(&priv_dev->lock, flags);
>> +		return 0;
>> +	}
>> +
>> +	cdns3_gadget_config(priv_dev);
>> +	priv_dev->in_standby_mode = 0;
>> +	spin_unlock_irqrestore(&priv_dev->lock, flags);
>> +	return 0;
>> +}
>> +
>> +/**
>> + * cdns3_gadget_init - initialize device structure
>> + *
>> + * cdns: cdns3 instance
>> + *
>> + * This function initializes the gadget.
>> + */
>> +int cdns3_gadget_init(struct cdns3 *cdns)
>> +{
>> +	struct cdns3_role_driver *rdrv;
>> +
>> +	rdrv = devm_kzalloc(cdns->dev, sizeof(*rdrv), GFP_KERNEL);
>> +	if (!rdrv)
>> +		return -ENOMEM;
>> +
>> +	rdrv->start	= cdns3_gadget_start;
>> +	rdrv->stop	= cdns3_gadget_stop;
>
>Why not use gadget_init()/gadget_exit() here? That sounds much cleaner
>as you won't have ISR's active after a role stop.

Without cdns3 prefix ? 
cdns3_gadget_exit has removed so we need cdns3_gadget_stop as non static function. 

So I can rename cdns3_gadget_stop  function to cdns3_gadget_exti, but cdn3_gadget_start is busy. 

cdn3_gadget_start is used only internally in this file so maybe she can be called  gadget_init or  __ cdn3_gadget_init

>
>> +	rdrv->suspend	= cdns3_gadget_suspend;
>> +	rdrv->resume	= cdns3_gadget_resume;
>> +	rdrv->irq	= cdns3_irq_handler_thread;
>> +	rdrv->name	= "gadget";
>> +	cdns->roles[CDNS3_ROLE_GADGET] = rdrv;
>> +	return __cdns3_gadget_init(cdns);
>> +}
>> diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h
>> index 75ca6214e79a..3b0d4d2e4831 100644
>> --- a/drivers/usb/cdns3/gadget.h
>> +++ b/drivers/usb/cdns3/gadget.h
>> @@ -1068,4 +1068,8 @@ struct cdns3_device {
>>  	struct usb_request		*pending_status_request;
>>  };
>>
>> +void cdns3_set_register_bit(void __iomem *ptr, u32 mask);
>> +int cdns3_init_ep0(struct cdns3_device *priv_dev);
>> +void cdns3_ep0_config(struct cdns3_device *priv_dev);
>> +void cdns3_select_ep(struct cdns3_device *priv_dev, u32 ep);
>>  #endif /* __LINUX_CDNS3_GADGET */
>>
>
>cheers,
>-roger
>
>--
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Powered by blists - more mailing lists