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

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi Vincent,

On 15.12.24 10:21, Vincent Mailhol wrote:

> I am done reading (and understanding) ISO 11898-1:2024, so let's resume
> the work!

Good! ;-)

> On 04/12/2024 at 16:56, Oliver Hartkopp wrote:
>> Hi Vincent,
>>
>> On 03.12.24 10:45, Vincent Mailhol wrote:
>>
>>>> https://lore.kernel.org/linux-can/20241201112333.6950-1-
>>>> socketcan@hartkopp.net/T/#u
>>>> https://lore.kernel.org/linux-can/20241201112230.6917-1-
>>>> socketcan@hartkopp.net/T/#t
>>
>>> Thanks for all the testing and the fixes. Because of the lack of
>>> testing of this RFC on my side, I was expecting such issues. But I
>>> really appreciate that you took time to investigate and debug, really
>>> helpful! I will make sure to incorporate these fixes in the next
>>> version.
>>
>> I'll send out an extended RFC V2 which is my current test base for you
>> in some minutes.
>>
>> Either for the kernel and iproute2 there are two new patches that add
>> new controlmode flags.
>>
>>> The next series I send will add the pwm and drop the RFC patch.
>>
>> Excellent!
>>
>> 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:

> 
> 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
> 
> 2. Add these to bittiming.c:
> 
>    struct can_pwm {
can_xl_pwm or can_xlpwm

>    	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;
>    }
??

> 
> 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;
     } else if (CanClock_mhz == 100) {
         // @10Mbit/s
         pwmo = 0;
         pwms = 3;
         pwml = 7;
     } else if (CanClock_mhz == 160) {
         // @10Mbit/s
         pwmo = 0;
         pwms = 2;
         pwml = 6;
     }

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.

Best regards,
Oliver