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

[Marc Kleine-Budde]

On 10/19/2017 03:54 PM, Franklin S Cooper Jr wrote:
> On 10/19/2017 06:32 AM, Marc Kleine-Budde wrote:
>> On 10/19/2017 01:09 PM, Sekhar Nori wrote:
>>> On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote:
>>>> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>>>>>> 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.
>>>>
>>>> If the default value is board specific and cannot be calculated in
>>>> general or from other values present in the DT, then it's from my point
>>>> of view describing the hardware.
>>>>
>>>>> 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.
>>>>
>>>> From my point of view I'd rather buy a board from a HW vendor where
>>>> CAN-FD works, rather than a board where I have to tweak the bit-timing
>>>> for a simple CAN-FD test setup.
>>>>
>>>> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
>>>> MBit/s -> 50%". Do we need an array of tuples in general?
> 
> Internally what I proposed was a binding that allowed you to pass in an
> array of a range of baud rates and then a SSP for that baud rate range.
> Therefore, if the baud rate being used impacted what SSP worked then it
> allows someone to provide a range of defaults. Of course a person also
> has the ability to use a single large range thus implementing a single
> default SSP value.

A single tuple is just a special case of a list of tuples :)

I was thinking of something like this:

First we need a struct defining the bitrate spp relationship.

The driver provides default values by a sorted array of these structs
together with array length.

During device registration we assign these default values to the actual
driver instance.

The netlink code can read and overwrite the current values. Maybe it can
read the default values.

The bitrate calculation code calculates the to be used spp value.

The driver sets the value during the open callback.

>> Do we need more than one tuple here?
>>
>>>> If good default values are transceiver and board specific, they can go
>>>> into the DT. We need a generic (this means driver agnostic) binding for
>>>> this. If this table needs to be tweaked for special purpose, then we can
>>>> add a netlink interface for this as well.
>>>>
>>>> Comments?
>>>
>>> I dont know how a good default (other than 50% as the starting point)
>>> can be arrived at without doing any actual measurements on the actual
>>> network. Since we do know that the value has to be tweaked, agree that
>>> netlink interface has to be provided.
> 
> Now I have seen in non public documentations that setting SP to SSP also
> works.

This can already by done now, without the need for new interfaces.

> This makes a bit more sense to me and I'm alot more comfortable going
> with this. However, since its based on non public information I can't
> justify it beyond that "it works for me". But I'm alot more 
> comfortable going with then saying "hey this default value works for 
> TI's dra76 evm. Therefore, every MCAN board will be stuck by default
> for a value that works for us". So if there is no push back with
> going with SSP = db SP with no documentation to back up why that is
> being used then I will try that out and send patches.

This means we postpone the whole add-new-interface-dance until the
SPP=SP approach doesn't work for some usecase?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Attachment: signature.asc
Description: OpenPGP digital signature