Date:   Wed, 18 Oct 2017 09:17:00 -0500
From:   Franklin S Cooper Jr <fcooper@...com>
To:     Sekhar Nori <nsekhar@...com>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        Mario Hüttel <mario.huettel@....net>,
        "Yang, Wenyou" <Wenyou.Yang@...rochip.com>, <wg@...ndegger.com>,
        <socketcan@...tkopp.net>, <quentin.schulz@...e-electrons.com>,
        <edumazet@...gle.com>, <linux-can@...r.kernel.org>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC:     Wenyou Yang <wenyou.yang@...el.com>,
        Dong Aisheng <b29396@...escale.com>,
        "Quadros, Roger" <rogerq@...com>
Subject: Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates



On 10/18/2017 08:24 AM, Sekhar Nori wrote:
> Hi Marc,
> 
> On Wednesday 18 October 2017 06:14 PM, Marc Kleine-Budde wrote:
>> On 09/21/2017 02:48 AM, Franklin S Cooper Jr wrote:
>>>
>>>
>>> On 09/20/2017 04:37 PM, Mario Hüttel wrote:
>>>>
>>>>
>>>> On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote:
>>>>> Hi Wenyou,
>>>>>
>>>>> On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
>>>>>>
>>>>>> On 2017/9/14 13:06, Sekhar Nori wrote:
>>>>>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>>>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>>>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>>>>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>>>>>>> bit
>>>>>>>>> was being transmitted and with a bit more investigation realized the
>>>>>>>>> actual
>>>>>>>>> MCAN IP would go back to initialization mode automatically.
>>>>>>>>>
>>>>>>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>>>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>>>>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>>>>>>> Compensation Offset register should be set. The document mentions
>>>>>>>>> that this
>>>>>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>>>>>>
>>>>>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>>>>>>> indicates
>>>>>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>>>>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>>>>>>> rate
>>>>>>>>> is above 2.5 Mbps.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@...com>
>>>>>>>>> ---
>>>>>>>>> I'm pretty surprised that this hasn't been implemented already since
>>>>>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>>>>>>> supports up to 10 Mbps.
>>>>>>>>>
>>>>>>>>> So it will be nice to get comments from users of this driver to
>>>>>>>>> understand
>>>>>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>>>>>>> patch.
>>>>>>>>> If they haven't what did they do to get around it if they needed higher
>>>>>>>>> speeds.
>>>>>>>>>
>>>>>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>>>>>>> insure
>>>>>>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>>>>>>> transceiver.
>>>>>>>> ping. Anyone has any thoughts on this?
>>>>>>> I added Dong who authored the m_can driver and Wenyou who added the only
>>>>>>> in-kernel user of the driver for any help.
>>>>>> I tested it on SAMA5D2 Xplained board both with and without this patch, 
>>>>>> both work with the 4M bps data bit rate.
>>>>> Thank you for testing this out. Its interesting that you have been able
>>>>> to use higher speeds without this patch. What is the CAN transceiver
>>>>> being used on the SAMA5D2 Xplained board? I tried looking at the
>>>>> schematic but it seems the CAN signals are used on an extension board
>>>>> which I can't find the schematic for. Also do you mind sharing your test
>>>>> setup? Were you doing a short point to point test?
>>>>>
>>>>> Thank You,
>>>>> Franklin
>>>> Hello Franklin,
>>>>
>>>> your patch definitely makes sense.
>>>>
>>>> I forgot the TDC in my patches because it was not present in the
>>>> previous driver versions and because I didn't encounter any
>>>> problems when testing it myself.
>>>>
>>>> The error is highly dependent on the hardware (transceiver) setup.
>>>> So it is definitely possible that some people don't encounter errors
>>>> without your patch.
>>>
>>> So the Transmission Delay Compensation feature Value register is suppose
>>> to take into consideration the transceiver delay automatically and add
>>> the value of TDCO on top of that. So why would TDCO be dependent on the
>>> transceiver? I've heard conflicting things regarding TDC so any
>>> clarification on what actually impacts it would be appreciated.
>>>
>>> Also part of the issue I'm having is how can we properly configure TDCO?
>>> Configuring TDCO is essentially figuring out what Secondary Sample Point
>>> to use. However, it is unclear what value to set SSP to and which use
>>> cases a given SSP will work or doesn't work. I've seen various
>>> recommendations from Bosch on choosing SSP but ultimately it seems they
>>> suggestion "real world testing" to come up with a proper value. Not
>>> setting TDCO causes problems for my device and improperly setting TDCO
>>> causes problems for my device. So its likely any value I use could end
>>> up breaking something for someone else.
>>>
>>> Currently I leaning to a DT property that can be used for setting SSP.
>>> Perhaps use a generic default value and allow individuals to override it
>>> via DT?
>>
>> Sounds reasonable. What's the status of this series?
> 
> I have had some offline discussions with Franklin on this, and I am not
> fully convinced that DT is the way to go here (although I don't have the
> agreement with Franklin there).

Probably the fundamental area where we disagree is what "default" SSP
value should be used. Based on a short (< 1 ft) point to point test
using a SSP of 50% worked fine. However, I'm not convinced that this
default value of 50% will work in a more "traditional" CAN bus at higher
speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
board in even the simplest test cases.

I believe that this default SSP should be a DT property that allows any
board to determine what default value works best in general.
> 
> There are two components in configuring the secondary sample point. It
> is the transceiver loopback delay and an offset (example half of the bit
> time in data phase).
> 
> While the transceiver loopback delay is pretty board dependent (and thus
> amenable to DT encoding), I am not quite sure the offset can be
> configured in DT because its not really board dependent.
> 
> Unfortunately, offset calculation does not seem to be an exact science.
> There are recommendations ranging from using 50% of bit time to making
> it same as the sample point configured. This means users who need to
> change the SSP due to offset variations need to change  their DT even
> without anything changing on their board.
> 
> Since we have a netlink socket interface to configure sample point, I
> wonder if that should be extended to configure SSP too (or at least the
> offset part of SSP)?

Sekhar is right that ideally the user should be able to set the SSP at
runtime. However, my issue is that based on my experience CAN users
expect the driver to just work the majority of times. For unique use
cases where the driver calculated values don't work then the user should
be able to override it. This should only be done for a very small
percentage of CAN users. Unless you allow DT to provide a default SSP
many users of MCAN may find that the default SSP doesn't work and must
always use runtime overrides to get anything to work. I don't think that
is a good user experience which is why I don't like the idea.


> 
> Thanks,
> Sekhar
> 

Powered by blists - more mailing lists