Re: peak_pci: TX Frame Loss

[Andri Yngvason <andri.yngvason@marel.com>]

On Thu, Dec 03, 2015 at 07:37:17AM +0100, Oliver Hartkopp wrote:
> Some more details:
> 
> On 12/02/2015 08:19 PM, Oliver Hartkopp wrote:
> > On 12/02/2015 07:09 PM, Andri Yngvason wrote:
> >> Quoting Andri Yngvason (2015-11-18 14:51:21)
> >> [...]
> >>> We've been experiencing frame loss on transmission in the peak_pci netdev
> >>> driver.
> >> [...]
> 
> 
> >>  I tried adding a delay to test my theory:
> >> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> >> index f968d1e..b5115c2 100644
> >> --- a/drivers/net/can/sja1000/sja1000.c
> >> +++ b/drivers/net/can/sja1000/sja1000.c
> >> @@ -93,6 +93,7 @@ static void sja1000_write_cmdreg(struct sja1000_priv *priv, u8 val)
> >>  	spin_lock_irqsave(&priv->cmdreg_lock, flags);
> >>  	priv->write_reg(priv, SJA1000_CMR, val);
> >>  	priv->read_reg(priv, SJA1000_SR);
> >> +	udelay(10);
> >>  	spin_unlock_irqrestore(&priv->cmdreg_lock, flags);
> >>  }
> > 
> > Hi Andri,
> > 
> > looking at the code above I wonder whether the
> > 
> > 	priv->read_reg(priv, SJA1000_SR);
> > 
> > is a bad hack anyway.
> > 
> > The read_reg() becomes readb(priv->reg_base + (port << 2)) in peak_pci.c and
> > the result is never used.
> > 
> > What if the entire operation gets 'optimized' at some point?

Hi Oliver,

According to ftrace it takes at least 1us to execute.

> > 
> > Can you check what happens if you just replace the priv->read_reg() with your
> > udelay(10) call?
> > 
> 
> In fact I contributed the first SJA1000 SMP fix myself.
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit?id=57c8a456640fa3ca777652f11f2db4179a3e66b6
> 
> As we only picked the code suggestion from Klaus the delay was introduced by
> priv->read_reg() - which is obviously not the proper way.
> 
> The data sheet says:
> 
> "Between two commands at least one internal clock cycle is needed in order to
> proceed. The internal clock is half of the external oscillator frequency."

The upper bound would have been more useful information here. ;)

> 
> The SJA1000 accepts external clocks from 0 to 24 MHz - all our drivers have an
> external clock of 16 MHz which leads to an internal clock of 8 MHz.
> 
> If we need an additional internal clock cycle we can calculate this with
> 4 MHz and 1/4.000.000 = 0.000,000,25 => 250ns = 0.25us
> 
> Even if someone would use an external clock of 8 MHz a udelay(1) should be enough.
> 
> Can you please make a test where you replace the priv->read_reg() just with
> udelay(1)?

I tried it and it's not enough. The sja1000 on the peak pci is an FPGA
implementation that claims to be sja1000 compatible. Is it possible that peak's
FPGA implementation is slower than the original sja1000?

Stéphane, what do you think?

> 
> If it works I would like to contribute a patch to fix my former attempt with a
> Reported-by: and Tested-by: tag from you.
> 
Sure, whatever gets this fixed is fine by me. ;)

Thanks,
Andri