Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

On Wednesday 18 October 2017 07:47 PM, Franklin S Cooper Jr wrote:
> 
> 
> 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@xxxxxx>
>>>>>>>>>> ---
>>>>>>>>>> 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.

With that, I think, we are taking DT from describing board/hardware
characteristics to providing default values that software should use.

In any case, if Marc and/or Wolfgang are okay with it, binding
documentation for such a property should be sent to DT maintainers for
review.

>>
>> 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.

Fair enough. But not quite sure if CAN users expect CAN-FD to "just
work" without doing any bittiming related setup.

Thanks,
Sekhar