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

[Roger Quadros <rogerq@ti.com>]

Hi,

On 03/11/18 19:51, Pawel Laszczak wrote:
> Patch implements some function used for debugging driver.
> 
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  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@cadence.com>
> + */
> +
> +#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