Re: [RFC PATCH 00/14] can: netlink: add CAN XL

[Oliver Hartkopp <socketcan@hartkopp.net>]

Addition ...

On 17.12.24 21:03, Oliver Hartkopp wrote:
> Hi Vincent,
> 
> On 17.12.24 19:17, Vincent Mailhol wrote:
>> On 17/12/2024 at 18:53, Oliver Hartkopp wrote:
>>
>> (...)
>>
>>>>> I assume it will follow the TDC pattern with
>>>>> CAN_CTRLMODE_XL_PWM_AUTO and CAN_CTRLMODE_XL_PWM_MANUAL
>>>>> or something similar?
>>>>
>>>> I am not sure why we would need a tristate here. The reason why I put
>>>> the tristate for the TDC is because some transceiver (the majority) 
>>>> will
>>>> automatically measure TDCV but some may give the option for the user to
>>>> manually set it.
>>>>
>>>>   From what I understand from the specification, PWMO is not something
>>>> which is dynamically measured. If I got it right, it is just the
>>>> remainder whenever the bit time is not a multiple of PWM. Something 
>>>> like
>>>> this:
>>>>
>>>>     pwmo = bit_time_in_minimum_time_quantum % pwm;
>>>>
>>>> Same for the PWMS and PWML. I am not yet sure how to calculate those
>>>> (more on this below) but these seem to be calculated once for all (like
>>>> any bittimming value other than the TDCs).
>>>>
>>>> Let me know if I missed something, the PWM is very new to me :)
>>>>
>>>>
>>>> So, for the implementation, I am thinking of:
>>>
>>> The PWM stuff is some very CAN XL specific therefore I would suggest to
>>> bring this into the naming too:
>>
>> It is specific to CAN XL for the moment. But if I do a parallel, before
>> CAN XL existed, the TDC was specific to CAN FD. But the structure was
>> named can_tdc, not can_fd_tdc, and now that the CAN XL reuses the TDC,
>> it comes in handy that the structure did not have fd in the name.
>>
>> The CAN XL still have a resXL flag for potential future extension, if
>> one day there is a CAN XXL, then the PWM would not be specific to CAN XL
>> anymore.
>>
>> My approach will be to:
>>
>>    - name structures and entries within a netlink nest in a generic
>>      manner
>>    - name struct fields (e.g. in can_priv) and the top level netlink
>>      entries with an XL prefix.
>>
>>>> 1. Add a CAN_CTRLMODE_PWM mode and a new nest for the PWM in netlink.h
>>>
>>> CAN_CTRLMODE_XL_PWM or CAN_CTRLMODE_XLPWM
>>
>> Ack. This flag is specific to CAN XL. I have a preference for
>> CAN_CTRLMODE_XL_PWM (with the underscore between XL and PWM).
> 
> ACK
> 
> 
>>>> 2. Add these to bittiming.c:
>>>>
>>>>     struct can_pwm {
>>> can_xl_pwm or can_xlpwm
>>
>> NACK. I keep can_pwm, but in can_priv, I will name the field:
> 
> Ok. Fine for me.
>>
>>    struct can_pwm xl_pwm;
>>
>>>>         u32 pwms; /* PWM short phase length */
>>>>         u32 pwml; /* PWM long phase length */
>>>>         u32 pwmo; /* PWM offset */
>>> that's fine
>>>
>>>>     };
>>>>
>>>>     struct can_pwm_const {
>>> dito
>>>
>>>>         u32 pwms_max;
>>>>         u32 pwml_max;
>>>>         u32 pwmo_max;
>>>>     };
>>>>
>>>>     static inline u32 can_get_pwm(const struct can_pwm *pwm)
>>>>     {
>>>>         return pwm->pwms + pwm->pwml;
>>>>     }
>>> ??
>>
>> ISO 11898-1:2024 tells me that (quote):
>>
>>    The PWM symbol length is the sum of PWMS and PWML.
>>
>> This inline function was to calculate this PWM symbol length. That said,
>> on second thought, I doubt that the drivers will need this information.
>> I will remove it for the moment. If someone needs it, we can reintroduce
>> this later.
>>
>>>> The minimum value of all those configurable ranges is already specified
>>>> to be one minimum time quantum (tq min), so I do not think we need a
>>>> field for the minimums.
>>>>
>>>>
>>>> At the moment, I am stuck on the PWM calculation. I do not know how to
>>>> derive good PWMS and PWML values out of the const ranges.
>>>>
>>>> The ISO 11898-1:2024 tells me that CiA 612-2 has recommendations on the
>>>> topic. But getting access to this document requires a subscription
>>>> (which I do not have):
>>>>
>>>>
>>>> https://www.can-cia.org/can-knowledge/cia-612-series-can-xl-
>>>> guidelines-and-application-notes
>>>>
>>>> Do any of you have access to this document? Or do any of you know a 
>>>> good
>>>> formula for the PWMS and PWML calculation?
>>>
>>> I have to check for the referenced document.
>>>
>>> Btw. this is some code that shows the idea of how to calculate the PWM
>>> values based on the CAN clock and the XL data bitrate:
>>>
>>>      if (CanClock_mhz == 100 && XlDatarate_mbps == 5) {
>>>          // @5Mbit/s>         pwmo = 0;
>>>          pwms = 6;
>>>          pwml = 14;
>>
>> one bit is 200 nano seconds.
>> one minimum time quantum is 10 ns.
>>
>> pwm = pwms + pwml
>>      = 6 + 14
>>      = 20 minimum time quantum
>>
>> So PWM duration is 20 * 10 = 200 ns. So there is only one PWM per bit.
>>
>>>      } else if (CanClock_mhz == 100) {
>>>          // @10Mbit/s>         pwmo = 0;
>>>          pwms = 3;
>>>          pwml = 7;
>>
>> 1 bit is 100 ns.
>> one minimum time quantum is 10 ns.
>>
>> pwm = pwms + pwml
>>      = 3 + 7
>>      = 10 minimum time quantum
>>
>> So PWM duration is 10 * 10 = 100 ns. So here also, there is only one PWM
>> per bit.
>>
>>>      } else if (CanClock_mhz == 160) {
>>>          // @10Mbit/s>         pwmo = 0;
>>>          pwms = 2;
>>>          pwml = 6;
>>
>> 1 bit is 100 ns.
>> one minimum time quantum is 6.25 ns.
>>
>> pwm = pwms + pwml
>>      = 2 + 6
>>      = 8 minimum time quantum
>>
>> So PWM duration is 8 * 6.25 = 50 ns. This time, there are two PWM per
>> bit. Interesting.
>>>      }
>>
>> So from those value, I derived:
>>
>>    pwms = round_down(30% * pwm)
>>    pwml = round_up(70% * pwm)
>>
>> PWMO was already established as:
>>
>>    pwmo = bit_time_in_minimum_time_quantum % pwm
>>
>> (in your example, the bit time is a multiple of PWM, so the PWMO is 
>> zero).
>>
>> This is some progress. What is not clear yet is how to find the number
>> of PWM per bit (i.e. why is it only one PWM per bit in your first two
>> examples, why is it two PWM per bit in your second example).
>>
>> The ISO has one more formula that I did not yet used. Maybe that's the
>> solution. But it is 3 a.m. here, let's say that this is a problem for
>> the tomorrow me.
>>
>>> For that reason I was thinking about some default calculation provided
>>> by the kernel (CAN_CTRLMODE_XL_PWM_AUTO) and some manual setting if
>>> people want to set the values analog to tseg1 and friends instead of
>>> setting a single bitrate.
>>
>> Don't worry, we have the same final result in mind. It is just that I
>> believe that we can use a trick so that we can do it with a single 
>> flag :)
>>
>> Let me illustrate in more details.
>>
>>    Case 1: PWM off
>>
>>      ip link set can0 (...) xl on pwm off
> 
> Ugh.
> 
> Did you take a look into all my patches from 2024-12-04, e.g.
> 
> https://lore.kernel.org/linux-can/20241204075741.3727-3- 
> socketcan@hartkopp.net/T/#u
> 
> which is introducing CAN_CTRLMODE_XL_TRX ??

It would be nice if you implement the PWM stuff based on these patch 
sets from 2024-12-04. I introduced CAN_CTRLMODE_XL_TRX and 
CAN_CTRLMODE_XL_RRS either in the kernel and in the iproute2 which is 
already operational in my setup.

Thanks,
Oliver

> 
> We are talking about having a CAN XL capable/switchable transceiver 
> attached to the CAN (XL) controller or not. IMO we should also name it 
> accordingly with
> 
> ip link set can0 (...) xl on xltrx off
> 
> That the transceiver switching is done with PWM under the hood is 
> nothing we should disclose to the user in this way IMHO.
> 
>>
>>      - CAN_CTRLMODE_XL_PWM bit is false
>>      - IFLA_CAN_XL_PWM is not provided
>>
>>    Case 2: PWM on auto
>>
>>      ip link set can0 (...) xl on pwm on
>>
>>      - CAN_CTRLMODE_XL_PWM bit is true
>>      - IFLA_CAN_XL_PWM is not provided
>>
>>    Case 3: PWM on manual
>>
>>      ip link set can0 (...) xl on pwm on pwms xxxx pwml yyyy
>>
>>      - CAN_CTRLMODE_XL_PWM bit is false
>>      - IFLA_CAN_XL_PWM is provided (and contains a nest with both
>>        IFLA_CAN_PWMS and IFLA_CAN_PWML)
>>
>> So, here, I use the presence or the absence of the IFLA_CAN_XL_PWM as an
>> hidden flag to know whether the calculation should be done or not.
> 
> Yes, that's a good idea. Excellent default behavior.
> 
> At least when reading IFLA_CAN_XL_PWM the provided or automatically 
> calculated values would become visible.
> 
>> Note that there is one more case:
>>
>>    Case 4: full auto??? (let the kernel decide between PWM on or off)
>>
>>      ip link set can0 (...) xl on
>>
>> Here, the user does not tell if PWM should be on or off. I think that
>> this case should be banned (return error).
> 
> Yes! Good idea!
> 
>> For what I understand, if one
>> node uses PWM and the other doesn't, it just doesn't work. So, contrary
>> to TDC, there is no way to calculate if this should be on or off.
>>
>> (side note: I will be busy for personal reasons until this Sunday, do
>> not be surprised if my next answer is delayed).
> 
> No problem.
> 
> Best regards,
> Oliver
> 
>