Re: peak_pci: TX Frame Loss

[Stephane Grosjean <s.grosjean@peak-system.com>]

Hi,

The question has been forwarded to our hw engineers.
I will give you any feedback I'll have about that.

Regards,

Stéphane

Le 03/12/2015 12:23, Andri Yngvason a écrit :
> 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

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--