Re: [PATCH 04/31] usb: usbssp: Added USBSSP platform driver

From: Roger Quadros
Date: Wed Aug 01 2018 - 08:24:25 EST

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@xxxxxxxxxxx>
> ---
> 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