RE: [PATCH 05/31] usb: usbssp: Added first part of initialization sequence.

From: Pawel Laszczak
Date: Mon Aug 06 2018 - 08:03:26 EST

Next message: Marc Kleine-Budde: "Re: [PATCH] can: sja1000: plx_pci: add support for ASEM CAN raw device"
Previous message: Dan Carpenter: "Re: [PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing."
In reply to: Roger Quadros: "Re: [PATCH 05/31] usb: usbssp: Added first part of initialization sequence."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,
> > Hi,
> >>
> >> Hi,
> >>
> >> On 19/07/18 20:57, Pawel Laszczak wrote:
> >>> Patch adds some initialization function. The initialization sequence
> >>> is quite complicated and this patch implements it only partially.
> >>> Initialization will be completed in next few patches.
> >>>
> >>> Patch introduce three new files:
> >>> 1. gadget-dbg.c - file contains functions used for debugging purpose.
> >>> 2. gadget-ext-caps.h - holds macro definition related to
> >>> Extended Capabilities
> >>> 3. gadget-if - file implements stuff related to upper layer
> >>> (e.g usb_ep_ops, usb_gadget_ops interface).
> >>>
> >>> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
> >>> ---
> >>> drivers/usb/usbssp/Makefile | 1 +
> >>> drivers/usb/usbssp/gadget-dbg.c | 30 ++++
> >>> drivers/usb/usbssp/gadget-ext-caps.h | 53 ++++++
> >>> drivers/usb/usbssp/gadget-if.c | 24 +++
> >>> drivers/usb/usbssp/gadget.c | 242 +++++++++++++++++++++++++++
> >>> drivers/usb/usbssp/gadget.h | 15 ++
> >>> 6 files changed, 365 insertions(+)
> >>> create mode 100644 drivers/usb/usbssp/gadget-dbg.c
> >>> create mode 100644 drivers/usb/usbssp/gadget-ext-caps.h
> >>> create mode 100644 drivers/usb/usbssp/gadget-if.c
> >>>
> >>> diff --git a/drivers/usb/usbssp/Makefile b/drivers/usb/usbssp/Makefile
> >>> index d85f15afb51c..0606f3c63cd0 100644
> >>> --- a/drivers/usb/usbssp/Makefile
> >>> +++ b/drivers/usb/usbssp/Makefile
> >>> @@ -5,6 +5,7 @@ CFLAGS_gadget-trace.o := -I$(src)
> >>> obj-$(CONFIG_USB_USBSSP_GADGET) += usbssp.o
> >>> usbssp-y := usbssp-plat.o gadget-ring.o \
> >>> gadget.o
> >>> + gadget-dbg.o
> >>>
> >>> ifneq ($(CONFIG_TRACING),)
> >>> usbssp-y += gadget-trace.o
> >>> diff --git a/drivers/usb/usbssp/gadget-dbg.c b/drivers/usb/usbssp/gadget-dbg.c
> >>> new file mode 100644
> >>> index 000000000000..277617d1a996
> >>> --- /dev/null
> >>> +++ b/drivers/usb/usbssp/gadget-dbg.c
> >>> @@ -0,0 +1,30 @@
> >>> +// 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 "gadget.h"
> >>> +
> >>> +void usbssp_dbg_trace(struct usbssp_udc *usbssp_data,
> >>> + void (*trace)(struct va_format *),
> >>> + const char *fmt, ...)
> >>> +{
> >>> + struct va_format vaf;
> >>> + va_list args;
> >>> +
> >>> + va_start(args, fmt);
> >>> + vaf.fmt = fmt;
> >>> + vaf.va = &args;
> >>> + dev_dbg(usbssp_data->dev, "%pV\n", &vaf);
> >>
> >> dev_dbg will mess up with timings to be useful.
> >> Why not just use one of the trace events you defined in gadget-trace.h?
> >
> > Function is equivalent of xhci_dbg_trace from XHCI driver.
> > It is a function of general use.
> > The problem with trace is that trace log is deleted after rebooting system.
> > Using trace log is problematic during developing and debugging driver.
>
> OK. But I think trace log is more useful for I/O related debugging as it
> keeps the timing impact minimal. For things that don't come in the I/O path
> you could use dev_dbg().
>
> > Do you know whether trace log is saved anywhere on disk ?
> >
> I think trace_cmd allows you to record on the disk or even send it over the network.
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lwn.net_Articles_410200_&d=DwICaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=e1OgxfvkL0qo9XO6fX1gscva-
> w03uSYC1nIyxl89-
> rI&m=6MvsISpCCpIMBnuBegpBOtoNlpzPXgvFMxqIROGhRVw&s=N3v03AhO0AP6HlVTyDvCRQqpJx4kcnqdw5appuOYG6U&e=

Thanks, I will try it.

>
> >>> + trace(&vaf);
> >>> + va_end(args);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(usbssp_dbg_trace);
> >>> +
>
> <snip>
>
> >>> int usbssp_gadget_init(struct usbssp_udc *usbssp_data)
> >>> {
> >>> int ret;
> >>> +
> >>> + /*
> >>> + * Check the compiler generated sizes of structures that must be laid
> >>> + * out in specific ways for hardware access.
> >>> + */
> >>> + BUILD_BUG_ON(sizeof(struct usbssp_doorbell_array) != 2*32/8);
> >>> + BUILD_BUG_ON(sizeof(struct usbssp_slot_ctx) != 8*32/8);
> >>> + BUILD_BUG_ON(sizeof(struct usbssp_ep_ctx) != 8*32/8);
> >>> + /* usbssp_device has eight fields, and also
> >>> + * embeds one usbssp_slot_ctx and 31 usbssp_ep_ctx
> >>> + */
> >>> + BUILD_BUG_ON(sizeof(struct usbssp_stream_ctx) != 4*32/8);
> >>> + BUILD_BUG_ON(sizeof(union usbssp_trb) != 4*32/8);
> >>> + BUILD_BUG_ON(sizeof(struct usbssp_erst_entry) != 4*32/8);
> >>> + BUILD_BUG_ON(sizeof(struct usbssp_cap_regs) != 8*32/8);
> >>> + BUILD_BUG_ON(sizeof(struct usbssp_intr_reg) != 8*32/8);
> >>> + /* usbssp_run_regs has eight fields and embeds 128 usbssp_intr_regs */
> >>> + BUILD_BUG_ON(sizeof(struct usbssp_run_regs) != (8+8*128)*32/8);
> >>> +
> >>> + /* fill gadget fields */
> >>> + /*TODO: implements usbssp_gadget_ops object*/
> >>> + //usbssp_data->gadget.ops = &usbssp_gadget_ops;
> >>> + usbssp_data->gadget.name = "usbssp-gadget";
> >>> + usbssp_data->gadget.max_speed = USB_SPEED_SUPER_PLUS;
> >>> + usbssp_data->gadget.speed = USB_SPEED_UNKNOWN;
> >>> + usbssp_data->gadget.sg_supported = true;
> >>> + usbssp_data->gadget.lpm_capable = 1;
> >>> +
> >>> + usbssp_data->setup_buf = kzalloc(USBSSP_EP0_SETUP_SIZE, GFP_KERNEL);
> >>> + if (!usbssp_data->setup_buf)
> >>> + return -ENOMEM;
> >>> +
> >>> + /*USBSSP support not aligned buffer but this option
> >>> + * improve performance of this controller.
> >>> + */
> >>
> >> Multi-line comment formatting. Checkpach should complain.
> > It doesn't complain about this case. I don't know why.
> >
>
> Can you try with --strict option to checkpatch?

With -options I see this warning and also many others related
to "spaces preferred"

I have to check all patches again.
>
> >>> + usbssp_data->gadget.quirk_ep_out_aligned_size = true;
> >>> + ret = usbssp_gen_setup(usbssp_data);
> >>> + if (ret < 0) {
> >>> + dev_err(usbssp_data->dev,
> >>> + "Generic initialization failed with error code%d\n",
> >>> + ret);
> >>> + goto err3;
> >>> + }
> >>> +
> >>> + ret = usbssp_gadget_init_endpoint(usbssp_data);
> >>> + if (ret < 0) {
> >>> + dev_err(usbssp_data->dev, "failed to initialize endpoints\n");
> >>> + goto err1;
> >>> + }
> >>> +
> >>> + ret = usb_add_gadget_udc(usbssp_data->dev, &usbssp_data->gadget);
> >>> +
> >>> + if (ret) {
> >>> + dev_err(usbssp_data->dev, "failed to register udc\n");
> >>> + goto err2;
> >>> + }
> >>> +
> >>> + return ret;
> >>> +err2:
> >>> + usbssp_gadget_free_endpoint(usbssp_data);
> >>> +err1:
> >>> + usbssp_halt(usbssp_data);
> >>> + /*TODO add implementation of usbssp_reset function*/
> >>> + //usbssp_reset(usbssp_data);
> >>> + /*TODO add implementation of freeing memory*/
> >>> + //usbssp_mem_cleanup(usbssp_data);
> >>> +err3:
> >>> return ret;
> >>> }
> >>>
> >>> @@ -60,5 +298,9 @@ int usbssp_gadget_exit(struct usbssp_udc *usbssp_data)
> >>> {
> >>> int ret = 0;
> >>>
> >>> + usb_del_gadget_udc(&usbssp_data->gadget);
> >>> + usbssp_gadget_free_endpoint(usbssp_data);
> >>> + /*TODO: add usbssp_stop implementation*/
> >>> + //usbssp_stop(usbssp_data);
> >>> return ret;
> >>> }
> >>> diff --git a/drivers/usb/usbssp/gadget.h b/drivers/usb/usbssp/gadget.h
> >>> index 55e20795d900..5d8918f8da84 100644
> >>> --- a/drivers/usb/usbssp/gadget.h
> >>> +++ b/drivers/usb/usbssp/gadget.h
> >>> @@ -12,8 +12,10 @@
> >>> #ifndef __LINUX_USBSSP_GADGET_H
> >>> #define __LINUX_USBSSP_GADGET_H
> >>>
> >>> +#include <linux/irq.h>
> >>> #include <linux/io-64-nonatomic-lo-hi.h>
> >>> #include <linux/usb/gadget.h>
> >>> +#include "gadget-ext-caps.h"
> >>>
> >>> /* Max number slots - only 1 is allowed */
> >>> #define DEV_MAX_SLOTS 1
> >>> @@ -1676,7 +1678,18 @@ static inline void usbssp_write_64(struct usbssp_udc *usbssp_data,
> >>> lo_hi_writeq(val, regs);
> >>> }
> >>>
> >>> +/* USBSSP memory management */
> >>> +void usbssp_dbg_trace(struct usbssp_udc *usbssp_data,
> >>> + void (*trace)(struct va_format *),
> >>> + const char *fmt, ...);
> >>
> >> what has this function to do with memory management?
> >>
> > Nothing,
> >
> >>> /* USBSSP Device controller glue */
> >>> +void usbssp_bottom_irq(struct work_struct *work);
> >>
> >> usbssp_bottom_irq() wasn't defined in this patch.
> > Removed from this patch
> >
> >>> +int usbssp_init(struct usbssp_udc *usbssp_data);
> >>> +void usbssp_stop(struct usbssp_udc *usbssp_data);
> >>> +int usbssp_handshake(void __iomem *ptr, u32 mask, u32 done, int usec);
> >>> +void usbssp_quiesce(struct usbssp_udc *usbssp_data);
> >>> +extern int usbssp_reset(struct usbssp_udc *usbssp_data);
> >>> +
> >>> int usbssp_suspend(struct usbssp_udc *usbssp_data, bool do_wakeup);
> >>> int usbssp_resume(struct usbssp_udc *usbssp_data, bool hibernated);
> >>>
> >>> @@ -1689,6 +1702,8 @@ dma_addr_t usbssp_trb_virt_to_dma(struct usbssp_segment *seg,
> >>> /* USBSSP gadget interface*/
> >>> int usbssp_gadget_init(struct usbssp_udc *usbssp_data);
> >>> int usbssp_gadget_exit(struct usbssp_udc *usbssp_data);
> >>> +void usbssp_gadget_free_endpoint(struct usbssp_udc *usbssp_data);
> >>> +int usbssp_gadget_init_endpoint(struct usbssp_udc *usbssp_data);
> >>>
> >>> static inline char *usbssp_slot_state_string(u32 state)
> >>> {
> >>>
> >>
>
> --
> cheers,
> -roger
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
cheers,
Pawell

Next message: Marc Kleine-Budde: "Re: [PATCH] can: sja1000: plx_pci: add support for ASEM CAN raw device"
Previous message: Dan Carpenter: "Re: [PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing."
In reply to: Roger Quadros: "Re: [PATCH 05/31] usb: usbssp: Added first part of initialization sequence."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]