Re: [RFC PATCH v1 13/14] usb:cdns3: Adds debugging function.

From: Roger Quadros
Date: Thu Nov 08 2018 - 04:34:55 EST

Hi,

On 03/11/18 19:51, Pawel Laszczak wrote:
> Patch implements some function used for debugging driver.
>
> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
> ---
> drivers/usb/cdns3/Makefile | 2 +-
> drivers/usb/cdns3/debug.c | 128 +++++++++++++++++++++++++++++++++++++
> drivers/usb/cdns3/ep0.c | 3 +
> drivers/usb/cdns3/gadget.c | 12 ++++
> drivers/usb/cdns3/gadget.h | 13 +++-
> 5 files changed, 156 insertions(+), 2 deletions(-)
> create mode 100644 drivers/usb/cdns3/debug.c
>
> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> index f4cfc978626f..34e60d03c4ec 100644
> --- a/drivers/usb/cdns3/Makefile
> +++ b/drivers/usb/cdns3/Makefile
> @@ -2,6 +2,6 @@ obj-$(CONFIG_USB_CDNS3) += cdns3.o
> obj-$(CONFIG_USB_CDNS3_PCI_WRAP) += cdns3-pci.o
>
> cdns3-y := core.o drd.o
> -cdns3-$(CONFIG_USB_CDNS3_GADGET) += gadget.o ep0.o
> +cdns3-$(CONFIG_USB_CDNS3_GADGET) += gadget.o ep0.o debug.o
> cdns3-$(CONFIG_USB_CDNS3_HOST) += host.o
> cdns3-pci-y := cdns3-pci-wrap.o
> diff --git a/drivers/usb/cdns3/debug.c b/drivers/usb/cdns3/debug.c
> new file mode 100644
> index 000000000000..bebf22c4d18e
> --- /dev/null
> +++ b/drivers/usb/cdns3/debug.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Cadence USBSS DRD Driver.
> + *
> + * Copyright (C) 2018 Cadence.
> + *
> + * Author: Pawel Laszczak <pawell@xxxxxxxxxxx>
> + */
> +
> +#include "gadget.h"
> +
> +static inline char *cdns3_decode_ep_irq(u32 ep_sts, const char *ep_name)
> +{
> + static char str[256];
> + int ret;
> +
> + ret = sprintf(str, "IRQ for %s: %08x ", ep_name, ep_sts);
> +
> + if (ep_sts & EP_STS_SETUP)
> + ret += sprintf(str + ret, "SETUP ");
> + if (ep_sts & EP_STS_IOC)
> + ret += sprintf(str + ret, "IOC ");
> + if (ep_sts & EP_STS_ISP)
> + ret += sprintf(str + ret, "ISP ");
> + if (ep_sts & EP_STS_DESCMIS)
> + ret += sprintf(str + ret, "DESCMIS ");
> + if (ep_sts & EP_STS_STREAMR)
> + ret += sprintf(str + ret, "STREAMR ");
> + if (ep_sts & EP_STS_MD_EXIT)
> + ret += sprintf(str + ret, "MD_EXIT ");
> + if (ep_sts & EP_STS_TRBERR)
> + ret += sprintf(str + ret, "TRBERR ");
> + if (ep_sts & EP_STS_NRDY)
> + ret += sprintf(str + ret, "NRDY ");
> + if (ep_sts & EP_STS_PRIME)
> + ret += sprintf(str + ret, "PRIME ");
> + if (ep_sts & EP_STS_SIDERR)
> + ret += sprintf(str + ret, "SIDERRT ");
> + if (ep_sts & EP_STS_OUTSMM)
> + ret += sprintf(str + ret, "OUTSMM ");
> + if (ep_sts & EP_STS_ISOERR)
> + ret += sprintf(str + ret, "ISOERR ");
> + if (ep_sts & EP_STS_IOT)
> + ret += sprintf(str + ret, "IOT ");
> +
> + return str;
> +}
> +
> +char *cdns3_decode_epx_irq(struct cdns3_endpoint *priv_ep)
> +{
> + struct cdns3_device *priv_dev = priv_ep->cdns3_dev;
> +
> + return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
> + priv_ep->name);
> +}
> +
> +char *cdns3_decode_ep0_irq(struct cdns3_device *priv_dev, int dir)
> +{
> + if (dir)
> + return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
> + "ep0IN");
> + else
> + return cdns3_decode_ep_irq(readl(&priv_dev->regs->ep_sts),
> + "ep0OUT");
> +}
> +
> +void cdns3_dbg_setup(struct cdns3_device *priv_dev)
> +{
> + struct usb_ctrlrequest *setup = priv_dev->setup;
> +
> + dev_dbg(&priv_dev->dev,
> + "SETUP BRT: %02x BR: %02x V: %04x I: %04x L: %04x\n",
> + setup->bRequestType,
> + setup->bRequest,
> + le16_to_cpu(setup->wValue),
> + le16_to_cpu(setup->wIndex),
> + le16_to_cpu(setup->wLength));
> +}
> +
> +/**
> + * Debug a transfer ring.
> + *
> + * Prints out all TRBs in the endpoint ring, even those after the Link TRB.
> + *.
> + */
> +void cdns3_dbg_ring(struct cdns3_device *priv_dev,
> + struct cdns3_endpoint *priv_ep)
> +{
> + u64 addr = priv_ep->trb_pool_dma;
> + struct cdns3_trb *trb;
> + int i;
> +
> + for (i = 0; i < TRBS_PER_SEGMENT; ++i) {
> + trb = &priv_ep->trb_pool[i];
> + dev_dbg(&priv_dev->dev, "@%016llx %08x %08x %08x\n", addr,
> + le32_to_cpu(trb->buffer),
> + le32_to_cpu(trb->length),
> + le32_to_cpu(trb->control));
> + addr += sizeof(*trb);
> + }
> +}
> +
> +void cdns3_dbg_ring_ptrs(struct cdns3_device *priv_dev,
> + struct cdns3_endpoint *priv_ep)
> +{
> + struct cdns3_trb *trb;
> +
> + trb = &priv_ep->trb_pool[priv_ep->dequeue];
> + dev_dbg(&priv_dev->dev,
> + "Ring deq index: %d, trb: %p (virt), 0x%llx (dma)\n",
> + priv_ep->dequeue, trb,
> + cdns3_trb_virt_to_dma(priv_ep, trb));
> +
> + trb = &priv_ep->trb_pool[priv_ep->enqueue];
> + dev_dbg(&priv_dev->dev,
> + "Ring enq index: %d, trb: %p (virt), 0x%llx (dma)\n",
> + priv_ep->enqueue, trb,
> + cdns3_trb_virt_to_dma(priv_ep, trb));
> +}
> +
> +void cdns3_dbg_ep_rings(struct cdns3_device *priv_dev,
> + struct cdns3_endpoint *priv_ep)
> +{
> + dev_dbg(&priv_dev->dev, "Endpoint ring %s:\n", priv_ep->name);
> +
> + cdns3_dbg_ring_ptrs(priv_dev, priv_ep);
> + cdns3_dbg_ring(priv_dev, priv_ep);
> +}
> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
> index 555ac7f6842e..46c943a74f77 100644
> --- a/drivers/usb/cdns3/ep0.c
> +++ b/drivers/usb/cdns3/ep0.c
> @@ -604,12 +604,15 @@ void cdns3_check_ep0_interrupt_proceed(struct cdns3_device *priv_dev, int dir)
> cdns3_select_ep(priv_dev, 0 | (dir ? USB_DIR_IN : USB_DIR_OUT));
> ep_sts_reg = readl(&regs->ep_sts);
>
> + dev_dbg(&priv_dev->dev, "%s\n", cdns3_decode_ep0_irq(priv_dev, dir));
> +
> __pending_setup_status_handler(priv_dev);
>
> if ((ep_sts_reg & EP_STS_SETUP) && dir == 0) {
> struct usb_ctrlrequest *setup = priv_dev->setup;
>
> writel(EP_STS_SETUP | EP_STS_IOC | EP_STS_ISP, &regs->ep_sts);
> + cdns3_dbg_setup(priv_dev);
>
> priv_dev->ep0_data_dir = setup->bRequestType & USB_DIR_IN;
> cdns3_ep0_setup_phase(priv_dev);
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index 905cad1a8229..ba4a36ce6e9a 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -124,6 +124,14 @@ void cdns3_select_ep(struct cdns3_device *priv_dev, u32 ep)
> wmb();
> }
>
> +dma_addr_t cdns3_trb_virt_to_dma(struct cdns3_endpoint *priv_ep,
> + struct cdns3_trb *trb)
> +{
> + u32 offset = (char *)trb - (char *)priv_ep->trb_pool;
> +
> + return priv_ep->trb_pool_dma + offset;
> +}
> +
> /**
> * cdns3_allocate_trb_pool - Allocates TRB's pool for selected endpoint
> * @priv_ep: endpoint object
> @@ -428,6 +436,8 @@ int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
> trb->control |= first_pcs;
>
> priv_req->on_ring = 1;
> +
> + cdns3_dbg_ep_rings(priv_dev, priv_ep);
> arm:
> /* arm transfer on selected endpoint */
> cdns3_select_ep(priv_ep->cdns3_dev, address);
> @@ -537,6 +547,8 @@ static int cdns3_check_ep_interrupt_proceed(struct cdns3_endpoint *priv_ep)
> cdns3_select_ep(priv_dev, priv_ep->endpoint.address);
> ep_sts_reg = readl(&regs->ep_sts);
>
> + dev_dbg(&priv_dev->dev, "%s\n", cdns3_decode_epx_irq(priv_ep));
> +
> if (ep_sts_reg & EP_STS_TRBERR)
> writel(EP_STS_TRBERR, &regs->ep_sts);
>
> diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h
> index 3f67ef1e5cda..3b2c01ff655b 100644
> --- a/drivers/usb/cdns3/gadget.h
> +++ b/drivers/usb/cdns3/gadget.h
> @@ -1085,5 +1085,16 @@ struct usb_request *cdns3_gadget_ep_alloc_request(struct usb_ep *ep,
> void cdns3_gadget_ep_free_request(struct usb_ep *ep,
> struct usb_request *request);
> int cdns3_gadget_ep_dequeue(struct usb_ep *ep, struct usb_request *request);
> -
> +dma_addr_t cdns3_trb_virt_to_dma(struct cdns3_endpoint *priv_ep,
> + struct cdns3_trb *trb);
> +
> +char *cdns3_decode_epx_irq(struct cdns3_endpoint *priv_ep);
> +char *cdns3_decode_ep0_irq(struct cdns3_device *priv_dev, int dir);
> +void cdns3_dbg_ring(struct cdns3_device *priv_dev,
> + struct cdns3_endpoint *priv_ep);
> +void cdns3_dbg_ring_ptrs(struct cdns3_device *priv_dev,
> + struct cdns3_endpoint *priv_ep);
> +void cdns3_dbg_ep_rings(struct cdns3_device *priv_dev,
> + struct cdns3_endpoint *priv_ep);
> +void cdns3_dbg_setup(struct cdns3_device *priv_dev);
> #endif /* __LINUX_CDNS3_GADGET */
>

How often are these invoked?

For I/O intensive cases dev_dbg() will not be useful as it will affect timing adversely
because of which it might prevent the issue from happening when debug is enabled.

How about using tracepoints instead?

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