Re: [PATCH V13 3/5] i2c: tegra: Add DMA support

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2552 bytes --]

On Wed, Feb 06, 2019 at 04:38:55PM +0000, Sowjanya Komatineni wrote:
> > >> Looking into timestamps and transactions, DMA timeouts after start of DMA for I2C1 to touch during this transaction.
> > >> While it is waiting for I2C1 DMA transfer, lots of DVC transactions 
> > >> happened thru DMA which are successful
> > >>
> > >> What is the I2C1 speed?
> > > 
> > > 400KHz
> > > 
> > >> Also incase if device is running slow for some reason, probably timeout was not enough as this patch series changes timeout with base 100mS + msg transfer time based on transfer size.
> > >> Can you give quick try with increased timeout incase if device is running slow?
> > >>
> > > 
> > > Tried to increase the timeout to 1 second, doesn't help.
> > > 
> > > What helped again is the I2C HW resetting after each transfer. Likely that means that HW isn't programmed correctly, please carefully check every bit.
> > > 
> > > DMA-only + I2C HW reset: http://dpaste.com/26AQXFM.txt
> >
> > Seems I found what's the problem. Here is the fix, please include it to v14 if it is correct.
> >
> > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index a9391c3646b6..5ad54da70304 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -912,7 +912,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)  static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev,
> >                                        size_t len)  {
> > -       u32 val, reg;
> > +       u32 val = 0, reg;
> >         u8 dma_burst = 0;
> >         struct dma_slave_config slv_config = {0};
> >         struct dma_chan *chan;
> > @@ -922,7 +922,6 @@ static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev,
> >                 reg = I2C_MST_FIFO_CONTROL;
> >         else
> >                 reg = I2C_FIFO_CONTROL;
> > -       val = i2c_readl(i2c_dev, reg);
>  >
>  >        if (i2c_dev->is_curr_dma_xfer) {
>  >                if (len & 0xF)
> 
> Thanks dmitry. Good catch. Didn't caught to my eyes. Yes FIFO_CONTROL is set based on read value without masking and oring causing back-back DMA transfers resulting in incorrect trig levels
> We can directly set trig levels.

It might be safer to explicitly clear the trigger levels before we
overwrite them. Though, admittedly, this register hasn't changed in
years, so it's unlikely we'll ever see it change in a way that might
result in breakage if we construct the register value from scratch.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]