Message-ID: <BYAPR07MB47097C3C0562BE7EA761EAF5DD2C0@BYAPR07MB4709.namprd07.prod.outlook.com>
Date:   Thu, 2 Aug 2018 06:25:22 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Roger Quadros <rogerq@...com>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        Felipe Balbi <balbi@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lukasz Tyrala <ltyrala@...ence.com>,
        Alan Douglas <adouglas@...ence.com>
Subject: RE: [PATCH 04/31] usb: usbssp: Added USBSSP platform driver

> > This patch adds platform driver that is entry point for loading and
> > unloading usbssp.ko modules.
> > It also adds information about this driver to drivers/usb/Kconfig and
> > drivers/usb/Makefile files and create Kconfig and Makefile files in
> > drivers/usb/usbssp directory.
> >
> > Patch also adds template for some function ivokked from
> 
> s/ivokked/invoked
> 
> > usbssp_plat.c file. These function will be implemented in next patches.
> >
> > This patch also introduce usbssp_trb_virt_to_dma that converts virtual
> > address of TRB's to DMA address. In this moment this function is used
> > only in gadget-trace.h.
> 
> s/"In this moment"/"At the moment"
> 
> >
> > From this moment the driver can be compiled.
> >
> > Signed-off-by: Pawel Laszczak <pawell@...ence.com>
> > ---
> >  drivers/usb/Kconfig              |   2 +
> >  drivers/usb/Makefile             |   2 +
> >  drivers/usb/usbssp/Kconfig       |  21 ++++
> >  drivers/usb/usbssp/Makefile      |  11 ++
> >  drivers/usb/usbssp/gadget-ring.c |  48 ++++++++
> >  drivers/usb/usbssp/gadget.c      |  64 +++++++++++
> >  drivers/usb/usbssp/gadget.h      |  16 ++-
> >  drivers/usb/usbssp/usbssp-plat.c | 186
> > +++++++++++++++++++++++++++++++
> >  8 files changed, 349 insertions(+), 1 deletion(-)  create mode 100644
> > drivers/usb/usbssp/Kconfig  create mode 100644
> > drivers/usb/usbssp/Makefile  create mode 100644
> > drivers/usb/usbssp/gadget-ring.c  create mode 100644
> > drivers/usb/usbssp/gadget.c  create mode 100644
> > drivers/usb/usbssp/usbssp-plat.c
> >
> 
> Build fails at this patch with error [1]. Building should never fail at any patch
> in the series.
> 
Yes it's true. There is a lack of #include <linux/irq.h> in drivers/usb/usbssp/gadget.h.

The compilation works correct starting from "0007-usb-usbssp-Initialization-added-usbssp_mem_init.patch"
Between 0004 and 0007 there is a problem in drivers/usb/usbssp/Makefile  (lack of "\").

Should I prepare a new series or I should wait for other comments ? 

> 
> > diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig index
> > f699abab1787..dc05f384c34c 100644
> > --- a/drivers/usb/Kconfig
> > +++ b/drivers/usb/Kconfig
> > @@ -110,6 +110,8 @@ source "drivers/usb/mtu3/Kconfig"
> >
> >  source "drivers/usb/musb/Kconfig"
> >
> > +source "drivers/usb/usbssp/Kconfig"
> > +
> >  source "drivers/usb/dwc3/Kconfig"
> >
> >  source "drivers/usb/dwc2/Kconfig"
> > diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile index
> > 060643a1b5c8..b1cd5f83d440 100644
> > --- a/drivers/usb/Makefile
> > +++ b/drivers/usb/Makefile
> > @@ -8,6 +8,8 @@
> >  obj-$(CONFIG_USB)		+= core/
> >  obj-$(CONFIG_USB_SUPPORT)	+= phy/
> >
> > +obj-$(CONFIG_USB_USBSSP)	+= usbssp/
> > +
> >  obj-$(CONFIG_USB_DWC3)		+= dwc3/
> >  obj-$(CONFIG_USB_DWC2)		+= dwc2/
> >  obj-$(CONFIG_USB_ISP1760)	+= isp1760/
> > diff --git a/drivers/usb/usbssp/Kconfig b/drivers/usb/usbssp/Kconfig
> > new file mode 100644 index 000000000000..ee20b01753dc
> > --- /dev/null
> > +++ b/drivers/usb/usbssp/Kconfig
> > @@ -0,0 +1,21 @@
> > +config USB_USBSSP
> 
> Do you want to choose a better Kconfig symbol name? USB is repeated twice
> in USB_USBSSP.
> 
> I'd recommend to add something signifying Cadence in the symbol
> 
> some examples
> 
> USB_CADSSP, USB_CSSP

Ok, I will change this to USB_CSSP

> > +	tristate "Cadence USBSSP DRD Controller"
> > +	depends on (USB || USB_GADGET) && HAS_DMA
> > +	select USB_USBSSP_GADGET
> 
> Not good to select a symbol that has dependencies.
> 
> > +	help
> > +	  Say Y here if your system has a cadence USBSSP dual-role
> controller.
> > +	  It supports: dual-role switch Host-only, and Peripheral-only.
> > +
> > +	  If you choose to build this driver is a dynamically linked
> > +	  module, the module will be called usbssp.ko.
> > +
> > +if USB_USBSSP
> > +
> > +config USB_USBSSP_GADGET
> > +	tristate "Gadget only mode"
> > +	default USB_USBSSP
> > +	depends on USB_GADGET=y || USB_GADGET=USB_USBSSP
> > +	help
> > +	  Select this when you want to use USBSSP in gadget mode only,
> 
> s/,/.
> 
> > +endif
> > +
> > diff --git a/drivers/usb/usbssp/Makefile b/drivers/usb/usbssp/Makefile
> > new file mode 100644 index 000000000000..d85f15afb51c
> > --- /dev/null
> > +++ b/drivers/usb/usbssp/Makefile
> > @@ -0,0 +1,11 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +# define_trace.h needs to know how to find our header
> > +CFLAGS_gadget-trace.o := -I$(src)
> > +
> > +obj-$(CONFIG_USB_USBSSP_GADGET) += usbssp.o
> > +usbssp-y 			:= usbssp-plat.o gadget-ring.o \
> > +				   gadget.o
> > +
> > +ifneq ($(CONFIG_TRACING),)
> > +	usbssp-y		+= gadget-trace.o
> > +endif
> > diff --git a/drivers/usb/usbssp/gadget-ring.c
> > b/drivers/usb/usbssp/gadget-ring.c
> > new file mode 100644
> > index 000000000000..d1da59306d02
> > --- /dev/null
> > +++ b/drivers/usb/usbssp/gadget-ring.c
> > @@ -0,0 +1,48 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * USBSSP device controller driver
> > + *
> > + * Copyright (C) 2018 Cadence.
> > + *
> > + * Author: Pawel Laszczak
> > + *
> > + * A lot of code based on Linux XHCI driver.
> > + * Origin: Copyright (C) 2008 Intel Corp  */
> > +
> > +#include <linux/scatterlist.h>
> > +#include <linux/slab.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/irq.h>
> > +#include "gadget-trace.h"
> > +#include "gadget.h"
> > +
> > +/*
> > + * Returns zero if the TRB isn't in this segment, otherwise it
> > +returns the DMA
> > + * address of the TRB.
> > + */
> > +dma_addr_t usbssp_trb_virt_to_dma(struct usbssp_segment *seg,
> > +				  union usbssp_trb *trb)
> > +{
> > +	unsigned long segment_offset;
> > +
> > +	if (!seg || !trb || trb < seg->trbs)
> > +		return 0;
> > +	/* offset in TRBs */
> > +	segment_offset = trb - seg->trbs;
> > +	if (segment_offset >= TRBS_PER_SEGMENT)
> > +		return 0;
> > +	return seg->dma + (segment_offset * sizeof(*trb)); }
> > +
> > +irqreturn_t usbssp_irq(int irq, void *priv) {
> > +	struct usbssp_udc *usbssp_data = (struct usbssp_udc *)priv;
> > +	irqreturn_t ret = IRQ_NONE;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&usbssp_data->lock, flags);
> > +
> > +	spin_unlock_irqrestore(&usbssp_data->lock, flags);
> > +	return ret;
> > +}
> > diff --git a/drivers/usb/usbssp/gadget.c b/drivers/usb/usbssp/gadget.c
> > new file mode 100644 index 000000000000..2f60d7dd1fe4
> > --- /dev/null
> > +++ b/drivers/usb/usbssp/gadget.c
> > @@ -0,0 +1,64 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * USBSSP device controller driver
> > + *
> > + * Copyright (C) 2018 Cadence.
> > + *
> > + * Author: Pawel Laszczak
> > + *
> > + * A lot of code based on Linux XHCI driver.
> > + * Origin: Copyright (C) 2008 Intel Corp  */
> > +
> > +#include <linux/pci.h>
> > +#include <linux/irq.h>
> > +#include <linux/log2.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/slab.h>
> > +#include <linux/dmi.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/delay.h>
> > +
> > +#include "gadget-trace.h"
> > +#include "gadget.h"
> > +
> > +#ifdef CONFIG_PM
> > +/*
> > + * Stop DC (not bus-specific)
> > + *
> > + * This is called when the machine transition into S3/S4 mode.
> > + *
> > + */
> > +int usbssp_suspend(struct usbssp_udc *usbssp_data, bool do_wakeup) {
> > +	/*TODO*/
> > +	return -ENOSYS;
> > +}
> > +
> > +/*
> > + * start DC (not bus-specific)
> > + *
> > + * This is called when the machine transition from S3/S4 mode.
> > + *
> > + */
> > +int usbssp_resume(struct usbssp_udc *usbssp_data, bool hibernated) {
> > +	/*TODO*/
> > +	return -ENOSYS;
> > +}
> > +
> > +#endif	/* CONFIG_PM */
> > +
> > +int usbssp_gadget_init(struct usbssp_udc *usbssp_data) {
> > +	int ret;
> > +	return ret;
> ret is not initialized before returning.
> Maybe just
> return 0;
I left this compiler warning because the next patches will add implementation of this function and then warning disappear. 
I trayed to prepare this series of patches in a way to avoid deletion line in next patches in series. 

> > +}
> > +
> > +int usbssp_gadget_exit(struct usbssp_udc *usbssp_data) {
> > +	int ret = 0;
> > +
> > +	return ret;
> 
> return 0;
> 
> > +}
> > diff --git a/drivers/usb/usbssp/gadget.h b/drivers/usb/usbssp/gadget.h
> > index b5c17603af78..55e20795d900 100644
> > --- a/drivers/usb/usbssp/gadget.h
> > +++ b/drivers/usb/usbssp/gadget.h
> > @@ -9,7 +9,6 @@
> >   * A lot of code based on Linux XHCI driver.
> >   * Origin: Copyright (C) 2008 Intel Corp.
> >   */
> > -
> 
> unnecessary blank line removal
> 
> >  #ifndef __LINUX_USBSSP_GADGET_H
> >  #define __LINUX_USBSSP_GADGET_H
> >
> > @@ -1676,6 +1675,21 @@ static inline void usbssp_write_64(struct
> > usbssp_udc *usbssp_data,  {
> >  	lo_hi_writeq(val, regs);
> >  }
> > +
> > +/* USBSSP Device controller glue */
> > +int usbssp_suspend(struct usbssp_udc *usbssp_data, bool do_wakeup);
> > +int usbssp_resume(struct usbssp_udc *usbssp_data, bool hibernated);
> > +
> > +irqreturn_t usbssp_irq(int irq, void *priv);
> > +
> > +/* USBSSP ring, segment, TRB, and TD functions */ dma_addr_t
> > +usbssp_trb_virt_to_dma(struct usbssp_segment *seg,
> > +				union usbssp_trb *trb);
> > +
> > +/* USBSSP gadget interface*/
> > +int usbssp_gadget_init(struct usbssp_udc *usbssp_data); int
> > +usbssp_gadget_exit(struct usbssp_udc *usbssp_data);
> > +
> >  static inline char *usbssp_slot_state_string(u32 state)  {
> >  	switch (state) {
> > diff --git a/drivers/usb/usbssp/usbssp-plat.c
> > b/drivers/usb/usbssp/usbssp-plat.c
> 
> Is this file meant only for gadget controller or later even for host controller?
> If only for gadget then this could be just called gadget-plat.c

I'm not sure at this moment. I have not wondered  at now how this driver will 
be integrated with XHCI driver and DRD driver.
 

> > new file mode 100644
> > index 000000000000..c048044148aa
> > --- /dev/null
> > +++ b/drivers/usb/usbssp/usbssp-plat.c
> > @@ -0,0 +1,186 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * USBSSP device controller driver
> > + *
> > + * Copyright (C) 2018 Cadence.
> > + *
> > + * Author: Pawel Laszczak
> > + *
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/usb/phy.h>
> > +#include <linux/slab.h>
> > +#include <linux/acpi.h>
> > +
> > +#include "gadget.h"
> > +
> > +#define DRIVER_AUTHOR "Pawel Laszczak"
> > +#define DRIVER_DESC "USBSSP Device Controller (USBSSP) Driver"
> > +
> > +#ifdef CONFIG_OF
> > +
> > +static const struct of_device_id usbssp_dev_of_match[] = {
> > +	{
> > +		.compatible = "Cadence, usbssp-dev",
> 
> Avoid upper-case in compatible strings.
> 
> > +	},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, usbssp_dev_of_match); #endif
> > +
> > +int usbssp_is_platform(void)
> > +{
> > +	return 1;
> > +}
> > +
> > +static int usbssp_plat_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct resource *res;
> > +	struct usbssp_udc *usbssp_data;
> > +	int ret = 0;
> > +	int irq;
> > +	struct device *sysdev;
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		dev_err(&pdev->dev, "Incorrect IRQ number\n");
> 
> IRQ number might be correct but might be some other issue.
> You could just say "couldn't get IRQ"
> 
> > +		return -ENODEV;
> 
> Also, we don't want to print any error message if we got a -EPROBE_DEFER.
> And we need to return that instead of -ENODEV for deferred probing to
> work.
> 
> > +	}
> 
> So how about
> 	if (irq < 0) {
> 		if (irq != -EPROBE_DEFER)
> 			dev_err(&pdev->dev, "couldn't get IRQ\n")
> 
> 		return irq;
> 	}
> 
> > +
> > +	usbssp_data = devm_kzalloc(dev, sizeof(*usbssp_data),
> GFP_KERNEL);
> > +	if (!usbssp_data)
> > +		return -ENOMEM;
> > +
> > +	for (sysdev = &pdev->dev; sysdev; sysdev = sysdev->parent) {
> > +		if (is_of_node(sysdev->fwnode) ||
> > +		    is_acpi_device_node(sysdev->fwnode))
> > +			break;
> > +#ifdef CONFIG_PCI
> > +		else if (sysdev->bus == &pci_bus_type)
> > +			break;
> > +#endif
> > +	}
> 
> It is hard to understand what is this for loop doing exactly.
> 
> xhci-plat.c seems to have this comment. You should add it above as well.
>         /*
>          * sysdev must point to a device that is known to the system firmware
>          * or PCI hardware. We handle these three cases here:
>          * 1. xhci_plat comes from firmware
>          * 2. xhci_plat is child of a device from firmware (dwc3-plat)
>          * 3. xhci_plat is grandchild of a pci device (dwc3-pci)
>          */
> 
> 
> > +
> > +	if (!sysdev)
> > +		sysdev = &pdev->dev;
> > +
> > +	/* Try to set 64-bit DMA first */
> > +	if (WARN_ON(!dev->dma_mask))
> > +		/* Platform did not initialize dma_mask */
> > +		ret = dma_coerce_mask_and_coherent(dev,
> > +				DMA_BIT_MASK(64));
> > +	else
> > +		ret = dma_set_mask_and_coherent(dev,
> DMA_BIT_MASK(64));
> > +
> > +	/* If seting 64-bit DMA mask fails, fall back to 32-bit DMA mask */
> > +	if (ret) {
> > +		ret = dma_set_mask_and_coherent(dev,
> DMA_BIT_MASK(32));
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	usbssp_data->regs = devm_ioremap_resource(dev, res);
> > +
> > +	if (IS_ERR(usbssp_data->regs)) {
> dev_err() ?
> 
> > +		ret = PTR_ERR(usbssp_data->regs);
> > +		return ret;
> > +	}
> > +
> > +	usbssp_data->rsrc_start = res->start;
> > +	usbssp_data->rsrc_len = resource_size(res);
> > +
> > +	ret = devm_request_irq(dev, irq, usbssp_irq, IRQF_SHARED,
> > +			dev_name(dev), usbssp_data);
> 
> devm_request_threaded_irq() ?
Currently for deferred interrupts I use workqueue thread. Some time ago I tried use this function
but I have some problem with it. I have plan to replace devm_request_irq with devm_request_threaded_irq. 

In USBSSP controller for device side I have big problem with handling commands.  Handling command works like 
in XHCI controller. After invoking command driver has to wait for completion so it should sleep for some time. 
During waiting (e.g wait_for_completion) we need to enable the interrupts because we need to detect the completion event. Additionally some API  function from USBSSP gadget driver are invoked by upper layer with disabled interrupts. 
In this situation we can't enable interrupt before invoking command and driver has to detect completion in other way.
The concept taken from the host is not a perfect for device driver where most driver should works in interrupt context, 

> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	usbssp_data->irq = irq;
> > +	usbssp_data->dev = dev;
> > +	platform_set_drvdata(pdev, usbssp_data);
> > +	ret = usbssp_gadget_init(usbssp_data);
> > +
> > +	return ret;
> > +}
> > +
> > +static int usbssp_plat_remove(struct platform_device *pdev) {
> > +	int ret = 0;
> > +	struct usbssp_udc *usbssp_data;
> > +
> > +	usbssp_data = (struct usbssp_udc *)platform_get_drvdata(pdev);
> > +	ret = usbssp_gadget_exit(usbssp_data);
> > +	return ret;
> > +
> 
> move this blank line above return ret;
> > +}
> > +
> > +static int __maybe_unused usbssp_plat_suspend(struct device *dev) {
> > +	struct usbssp_udc *usbssp_data = dev_get_drvdata(dev);
> > +
> > +	return usbssp_suspend(usbssp_data, device_may_wakeup(dev)); }
> > +
> > +static int __maybe_unused usbssp_plat_resume(struct device *dev) {
> > +	struct usbssp_udc *usbssp_data = dev_get_drvdata(dev);
> > +
> > +	return usbssp_resume(usbssp_data, 0); }
> > +
> > +static int __maybe_unused usbssp_plat_runtime_suspend(struct device
> > +*dev) {
> > +	struct usbssp_udc *usbssp_data = dev_get_drvdata(dev);
> > +
> > +	return usbssp_suspend(usbssp_data, true); }
> > +
> > +static int __maybe_unused usbssp_plat_runtime_resume(struct device
> > +*dev) {
> > +	struct usbssp_udc *usbssp_data = dev_get_drvdata(dev);
> > +
> > +	return usbssp_resume(usbssp_data, 0); }
> > +
> > +static const struct dev_pm_ops usbssp_plat_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(usbssp_plat_suspend,
> usbssp_plat_resume)
> > +
> > +	SET_RUNTIME_PM_OPS(usbssp_plat_runtime_suspend,
> > +			usbssp_plat_runtime_resume,
> > +			NULL)
> > +};
> > +
> > +static struct platform_driver usbssp_driver = {
> > +	.probe	= usbssp_plat_probe,
> > +	.remove	= usbssp_plat_remove,
> > +	.driver	= {
> > +		.name = "usbssp-dev",
> > +		.pm = &usbssp_plat_pm_ops,
> > +		.of_match_table = of_match_ptr(usbssp_dev_of_match),
> > +	},
> > +};
> > +
> > +static int __init usbssp_plat_init(void) {
> > +	return platform_driver_register(&usbssp_driver);
> > +}
> > +module_init(usbssp_plat_init);
> > +
> > +static void __exit usbssp_plat_exit(void) {
> > +	platform_driver_unregister(&usbssp_driver);
> > +}
> > +module_exit(usbssp_plat_exit);
> > +
> > +MODULE_ALIAS("platform:usbss-gadget");
> usbssp-gadget?
> 
> Why did you choose a different name for compatible? "usbssp-dev"
> Would be nice to have it consistent.
> 
> > +MODULE_DESCRIPTION("USBSSP' Device Controller (USBSSP) Driver");
> 
> USBSSP, 2 times?
> 
> > +MODULE_LICENSE("GPL v2");
> >
I will correct this. 
> 
> [1] build error
> 
>   CC [M]  drivers/usb/usbssp/gadget-trace.o In file included from
> drivers/usb/usbssp/gadget-trace.h:27:0,
>                  from drivers/usb/usbssp/gadget-trace.c:13:
> drivers/usb/usbssp/gadget.h:1683:1: error: unknown type name
> ‘irqreturn_t’
>  irqreturn_t usbssp_irq(int irq, void *priv);  ^~~~~~~~~~~ In file included from
> ./include/trace/trace_events.h:394:0,
>                  from ./include/trace/define_trace.h:96,
>                  from drivers/usb/usbssp/gadget-trace.h:482,
>                  from drivers/usb/usbssp/gadget-trace.c:13:
> drivers/usb/usbssp/./gadget-trace.h: In function
> ‘trace_raw_output_usbssp_log_request’:
> drivers/usb/usbssp/./gadget-trace.h:201:477: warning: format ‘%llx’ expects
> argument of type ‘long long unsigned int’, but argument 6 has type
> ‘dma_addr_t {aka unsigned int}’ [-Wformat=]
> DECLARE_EVENT_CLASS(usbssp_log_request,
> 
> ^
> scripts/Makefile.build:317: recipe for target 'drivers/usb/usbssp/gadget-
> trace.o' failed
> make[3]: *** [drivers/usb/usbssp/gadget-trace.o] Error 1
> scripts/Makefile.build:558: recipe for target 'drivers/usb/usbssp' failed
> make[2]: *** [drivers/usb/usbssp] Error 2
> scripts/Makefile.build:558: recipe for target 'drivers/usb' failed
> make[1]: *** [drivers/usb] Error 2
> Makefile:1029: recipe for target 'drivers' failed
> make: *** [drivers] Error 2
> 
> --
cheers,
Pawel

Powered by blists - more mailing lists