lkml.org 
[lkml]   [2018]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 04/31] usb: usbssp: Added USBSSP platform driver
    From
    Date
    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@cadence.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

    \
     
     \ /
      Last update: 2018-08-01 14:25    [W:4.542 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site