Re: [PATCH 5/5] dma: tegra: add tracepoints to driver

[Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>]

On 22/11/2018 08:55, Jon Hunter wrote:
> On 21/11/2018 16:13, Ben Dooks wrote:
> > Add some trace-points to the driver to allow for debuging via the
> > trace pipe.
> >
> > Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>
> > ---
> > Fixes since v1:
> > - take copy of dmachan name instead of pointer to device
> >
> > Fixes since v2:
> > - finish off Steven's review comments
> > - fix saving the dma channel name
> > - use txstate as argument to trace_tegra_dma_tx_status()
> > - fix type of dma_cookie to dma_cookie_t
> >
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx> (maintainer:TRACING)
> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx> (maintainer:TRACING)
> > ---
> >   drivers/dma/tegra20-apb-dma.c        |  7 ++++
> >   include/trace/events/tegra_apb_dma.h | 61 ++++++++++++++++++++++++++++
> >   2 files changed, 68 insertions(+)
> >   create mode 100644 include/trace/events/tegra_apb_dma.h
> >
> > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> > index c60c3f7cad5f..659b49bf6fdb 100644
> > --- a/drivers/dma/tegra20-apb-dma.c
> > +++ b/drivers/dma/tegra20-apb-dma.c
> > @@ -38,6 +38,9 @@
> >   
> >   #include "dmaengine.h"
> >   
> > +#define CREATE_TRACE_POINTS
> > +#include <trace/events/tegra_apb_dma.h>
> > +
> >   #define TEGRA_APBDMA_GENERAL			0x0
> >   #define TEGRA_APBDMA_GENERAL_ENABLE		BIT(31)
> >   
> > @@ -672,6 +675,8 @@ static void tegra_dma_tasklet(unsigned long data)
> >   		dmaengine_desc_get_callback(&dma_desc->txd, &cb);
> >   		cb_count = dma_desc->cb_count;
> >   		dma_desc->cb_count = 0;
> > +		trace_tegra_dma_complete_cb(&tdc->dma_chan, cb_count,
> > +					    cb.callback);
> >   		spin_unlock_irqrestore(&tdc->lock, flags);
> >   		while (cb_count--)
> >   			dmaengine_desc_callback_invoke(&cb, NULL);
> > @@ -688,6 +693,7 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
> >   
> >   	spin_lock_irqsave(&tdc->lock, flags);
> >   
> > +	trace_tegra_dma_isr(&tdc->dma_chan, irq);
> >   	status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
> >   	if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
> >   		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
> > @@ -846,6 +852,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
> >   		dma_set_residue(txstate, residual);
> >   	}
> >   
> > +	trace_tegra_dma_tx_status(&tdc->dma_chan, cookie, txstate);
> >   	spin_unlock_irqrestore(&tdc->lock, flags);
> >   	return ret;
> >   }
>
> My only comment here is that apart from the ISR trace, the others could
> be added to the dmaengine driver core and then these could be used for
> other DMAs as well.

Hmm, I will go have a look at how difficult that would be.

--
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html