On 17/12/2024 at 18:53, Oliver Hartkopp wrote:
> Hi Vincent,
(...)
>>> 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).
>> 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:
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
- 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.
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). 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).
Yours sincerely,
Vincent Mailhol
