Date:   Wed, 1 Aug 2018 15:24:14 +0300
From:   Roger Quadros <rogerq@...com>
To:     Pawel Laszczak <pawell@...ence.com>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <linux-usb@...r.kernel.org>, Felipe Balbi <balbi@...nel.org>,
        <linux-kernel@...r.kernel.org>, <ltyrala@...ence.com>,
        <adouglas@...ence.com>
Subject: Re: [PATCH 04/31] usb: usbssp: Added USBSSP platform driver

Hi,

On 19/07/18 20:57, Pawel Laszczak wrote:
> 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.


> 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

> +	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;

> +}
> +
> +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

> 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() ?

> +
> +	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");
> 


[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,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Powered by blists - more mailing lists