[PATCH RFC 0/3] can: calc_bittiming: add sample point calculation for PWM encoding

[PATCH RFC 0/3] can: calc_bittiming: add sample point calculation for PWM encoding

[Vincent Mailhol]

I wrote this series in response to Oliver's feedback in [1]. I am
sending this as an RFC based on top of [2] for the moment so that we
can discuss this separately. The plan is to merge this to the CAN XL
branch once we reach a consensus on this part.

Patch #1 and #2 are refactors, patch #3 introduces a dedicated
function for the PWM sample point calculation.

[1] https://lore.kernel.org/linux-can/743ba133-3735-48fd-994a-9727cfe8c114@hartkopp.net/

[2] [PATCH v2 00/10] can: netlink: add CAN XL
Link: https://lore.kernel.org/linux-can/20251021-canxl-netlink-v2-0-8b8f58257ab6@kernel.org/

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Vincent Mailhol (3):
      can: calc_bittiming: get rid of the incorrect "nominal" word
      can: calc_bittiming: add can_calc_sample_point_nrz()
      can: calc_bittiming: add can_calc_sample_point_pwm()

 drivers/net/can/dev/calc_bittiming.c | 68 ++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 23 deletions(-)
---
base-commit: ffee675aceb9f44b0502a8bec912abb0c4f4af62
change-id: 20251102-pwm_sample_point-8823cb3cd459
prerequisite-change-id: 20241229-canxl-netlink-bc640af10673:v2
prerequisite-patch-id: 6b3294205bd76b38257516c63b7001ab242c9b62
prerequisite-patch-id: 56431d12edcc0f325cf5204bb6868742c462c0ed
prerequisite-patch-id: 1547fd7ea8f1937f0491cfc0996b09890f850991
prerequisite-patch-id: 1dae270b0454352e46b927f71d1b47ff2bf7a49e
prerequisite-patch-id: e4d43de873dfdefc023a0b86e397b37ea2b9e9a3
prerequisite-patch-id: 4f3db477ff411effe70075c59ae6eac04fc65600
prerequisite-patch-id: 148dbfce9d3bb09537087ee93e60bb7819bdadee
prerequisite-patch-id: 7996539e26d449e8db260425c7287b4dce8cdf35
prerequisite-patch-id: 42215044df6a63fff07c7a7d771d7dc375cc8b0e
prerequisite-patch-id: 640ebf8ac8a1d114dcb91e6c05b9414bd09416fc
prerequisite-patch-id: 84ee5e4f937f8e4cd97833d601affea78fe55914

Best regards,
-- 
Vincent Mailhol <mailhol@kernel.org>

[PATCH RFC 1/3] can: calc_bittiming: get rid of the incorrect "nominal" word

[Vincent Mailhol]

The functions can_update_sample_point() and can_calc_bittiming() are
generic and meant to be used for both the nominal and the data
bittiming calculation.

However, those functions use terminologies such as "bitrate nominal"
or "sample point nominal". This is a leftover from when only Classical
CAN was supported and now became incorrect.

Remove or replace any occurrences of the word "nominal" with something
more accurate.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/calc_bittiming.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 268ec6fa7c49..222117596704 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -24,7 +24,7 @@
  */
 static int
 can_update_sample_point(const struct can_bittiming_const *btc,
-			const unsigned int sample_point_nominal, const unsigned int tseg,
+			unsigned int sp_origin, unsigned int tseg,
 			unsigned int *tseg1_ptr, unsigned int *tseg2_ptr,
 			unsigned int *sample_point_error_ptr)
 {
@@ -35,8 +35,7 @@ can_update_sample_point(const struct can_bittiming_const *btc,
 
 	for (i = 0; i <= 1; i++) {
 		tseg2 = tseg + CAN_SYNC_SEG -
-			(sample_point_nominal * (tseg + CAN_SYNC_SEG)) /
-			1000 - i;
+			(sp_origin * (tseg + CAN_SYNC_SEG)) / 1000 - i;
 		tseg2 = clamp(tseg2, btc->tseg2_min, btc->tseg2_max);
 		tseg1 = tseg - tseg2;
 		if (tseg1 > btc->tseg1_max) {
@@ -46,9 +45,9 @@ can_update_sample_point(const struct can_bittiming_const *btc,
 
 		sample_point = 1000 * (tseg + CAN_SYNC_SEG - tseg2) /
 			(tseg + CAN_SYNC_SEG);
-		sample_point_error = abs(sample_point_nominal - sample_point);
+		sample_point_error = abs(sp_origin - sample_point);
 
-		if (sample_point <= sample_point_nominal &&
+		if (sample_point <= sp_origin &&
 		    sample_point_error < best_sample_point_error) {
 			best_sample_point = sample_point;
 			best_sample_point_error = sample_point_error;
@@ -68,11 +67,11 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 {
 	struct can_priv *priv = netdev_priv(dev);
 	unsigned int bitrate;			/* current bitrate */
-	unsigned int bitrate_error;		/* difference between current and nominal value */
+	unsigned int bitrate_error;		/* difference between current and calculated value */
 	unsigned int best_bitrate_error = UINT_MAX;
-	unsigned int sample_point_error;	/* difference between current and nominal value */
+	unsigned int sample_point_error;	/* difference between current and calculated value */
 	unsigned int best_sample_point_error = UINT_MAX;
-	unsigned int sample_point_nominal;	/* nominal sample point */
+	unsigned int sample_point;
 	unsigned int best_tseg = 0;		/* current best value for tseg */
 	unsigned int best_brp = 0;		/* current best value for brp */
 	unsigned int brp, tsegall, tseg, tseg1 = 0, tseg2 = 0;
@@ -81,14 +80,14 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 
 	/* Use CiA recommended sample points */
 	if (bt->sample_point) {
-		sample_point_nominal = bt->sample_point;
+		sample_point = bt->sample_point;
 	} else {
 		if (bt->bitrate > 800 * KILO /* BPS */)
-			sample_point_nominal = 750;
+			sample_point = 750;
 		else if (bt->bitrate > 500 * KILO /* BPS */)
-			sample_point_nominal = 800;
+			sample_point = 800;
 		else
-			sample_point_nominal = 875;
+			sample_point = 875;
 	}
 
 	/* tseg even = round down, odd = round up */
@@ -115,7 +114,7 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 		if (bitrate_error < best_bitrate_error)
 			best_sample_point_error = UINT_MAX;
 
-		can_update_sample_point(btc, sample_point_nominal, tseg / 2,
+		can_update_sample_point(btc, sample_point, tseg / 2,
 					&tseg1, &tseg2, &sample_point_error);
 		if (sample_point_error >= best_sample_point_error)
 			continue;
@@ -146,9 +145,8 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 	}
 
 	/* real sample point */
-	bt->sample_point = can_update_sample_point(btc, sample_point_nominal,
-						   best_tseg, &tseg1, &tseg2,
-						   NULL);
+	bt->sample_point = can_update_sample_point(btc, sample_point, best_tseg,
+						   &tseg1, &tseg2, NULL);
 
 	v64 = (u64)best_brp * 1000 * 1000 * 1000;
 	do_div(v64, priv->clock.freq);

-- 
2.51.0

Re: [PATCH RFC 1/3] can: calc_bittiming: get rid of the incorrect "nominal" word

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 5717 bytes --]

On 02.11.2025 23:01:22, Vincent Mailhol wrote:
> The functions can_update_sample_point() and can_calc_bittiming() are
> generic and meant to be used for both the nominal and the data
> bittiming calculation.

""There are 2 hard problems in computer science: cache invalidation,
  naming things, and off-by-1 errors.""

Here it's naming things. Back in the days, in commit 7da29f97d6c8 ("can:
dev: can-calc-bit-timing(): better sample point calculation"), I wanted
to distinguish between the sample point the user requested and the
current sample point.

I was thinking about the signal that goes into a control loops, but at
university the lecture was in German, so I picked the wrong term. I
think "set point" or "reference value" are better terms.

> However, those functions use terminologies such as "bitrate nominal"
> or "sample point nominal". This is a leftover from when only Classical
> CAN was supported and now became incorrect.
>
> Remove or replace any occurrences of the word "nominal" with something
> more accurate.

What about replacing "nominal" with "reference"

> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
> ---
>  drivers/net/can/dev/calc_bittiming.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
> index 268ec6fa7c49..222117596704 100644
> --- a/drivers/net/can/dev/calc_bittiming.c
> +++ b/drivers/net/can/dev/calc_bittiming.c
> @@ -24,7 +24,7 @@
>   */
>  static int
>  can_update_sample_point(const struct can_bittiming_const *btc,
> -			const unsigned int sample_point_nominal, const unsigned int tseg,
> +			unsigned int sp_origin, unsigned int tseg,

Please don't remove the "const".

>  			unsigned int *tseg1_ptr, unsigned int *tseg2_ptr,
>  			unsigned int *sample_point_error_ptr)
>  {
> @@ -35,8 +35,7 @@ can_update_sample_point(const struct can_bittiming_const *btc,
>
>  	for (i = 0; i <= 1; i++) {
>  		tseg2 = tseg + CAN_SYNC_SEG -
> -			(sample_point_nominal * (tseg + CAN_SYNC_SEG)) /
> -			1000 - i;
> +			(sp_origin * (tseg + CAN_SYNC_SEG)) / 1000 - i;
>  		tseg2 = clamp(tseg2, btc->tseg2_min, btc->tseg2_max);
>  		tseg1 = tseg - tseg2;
>  		if (tseg1 > btc->tseg1_max) {
> @@ -46,9 +45,9 @@ can_update_sample_point(const struct can_bittiming_const *btc,
>
>  		sample_point = 1000 * (tseg + CAN_SYNC_SEG - tseg2) /
>  			(tseg + CAN_SYNC_SEG);
> -		sample_point_error = abs(sample_point_nominal - sample_point);
> +		sample_point_error = abs(sp_origin - sample_point);
>
> -		if (sample_point <= sample_point_nominal &&
> +		if (sample_point <= sp_origin &&
>  		    sample_point_error < best_sample_point_error) {
>  			best_sample_point = sample_point;
>  			best_sample_point_error = sample_point_error;
> @@ -68,11 +67,11 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
>  {
>  	struct can_priv *priv = netdev_priv(dev);
>  	unsigned int bitrate;			/* current bitrate */
> -	unsigned int bitrate_error;		/* difference between current and nominal value */
> +	unsigned int bitrate_error;		/* difference between current and calculated value */

What about: "difference between reference and calculated value"

>  	unsigned int best_bitrate_error = UINT_MAX;
> -	unsigned int sample_point_error;	/* difference between current and nominal value */
> +	unsigned int sample_point_error;	/* difference between current and calculated value */
>  	unsigned int best_sample_point_error = UINT_MAX;
> -	unsigned int sample_point_nominal;	/* nominal sample point */
> +	unsigned int sample_point;
>  	unsigned int best_tseg = 0;		/* current best value for tseg */
>  	unsigned int best_brp = 0;		/* current best value for brp */
>  	unsigned int brp, tsegall, tseg, tseg1 = 0, tseg2 = 0;
> @@ -81,14 +80,14 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
>
>  	/* Use CiA recommended sample points */
>  	if (bt->sample_point) {
> -		sample_point_nominal = bt->sample_point;
> +		sample_point = bt->sample_point;
>  	} else {
>  		if (bt->bitrate > 800 * KILO /* BPS */)
> -			sample_point_nominal = 750;
> +			sample_point = 750;
>  		else if (bt->bitrate > 500 * KILO /* BPS */)
> -			sample_point_nominal = 800;
> +			sample_point = 800;
>  		else
> -			sample_point_nominal = 875;
> +			sample_point = 875;
>  	}
>
>  	/* tseg even = round down, odd = round up */
> @@ -115,7 +114,7 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
>  		if (bitrate_error < best_bitrate_error)
>  			best_sample_point_error = UINT_MAX;
>
> -		can_update_sample_point(btc, sample_point_nominal, tseg / 2,
> +		can_update_sample_point(btc, sample_point, tseg / 2,
>  					&tseg1, &tseg2, &sample_point_error);
>  		if (sample_point_error >= best_sample_point_error)
>  			continue;
> @@ -146,9 +145,8 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
>  	}
>
>  	/* real sample point */
> -	bt->sample_point = can_update_sample_point(btc, sample_point_nominal,
> -						   best_tseg, &tseg1, &tseg2,
> -						   NULL);
> +	bt->sample_point = can_update_sample_point(btc, sample_point, best_tseg,
> +						   &tseg1, &tseg2, NULL);
>
>  	v64 = (u64)best_brp * 1000 * 1000 * 1000;
>  	do_div(v64, priv->clock.freq);

Marc

--
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH RFC 2/3] can: calc_bittiming: add can_calc_sample_point_nrz()

[Vincent Mailhol]

CAN XL optimal sample point for PWM encoding (when TMS is on) differs
from the NRZ optimal one. There is thus a need to calculate a
different sample point depending whether TMS is on or off.

This is a preparation change: move the sample point calculation from
can_calc_bittiming() into the new can_calc_sample_point_nrz()
function.

In an upcoming change, a function will be added to calculate the
sample point for PWM encoding.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/calc_bittiming.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 222117596704..9b2d0e458518 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -10,6 +10,18 @@
 
 #define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
 
+/* CiA recommended sample points for Non Return to Zero encoding. */
+static int can_calc_sample_point_nrz(const struct can_bittiming *bt)
+{
+	if (bt->bitrate > 800 * KILO /* BPS */)
+		return 750;
+
+	if (bt->bitrate > 500 * KILO /* BPS */)
+		return 800;
+
+	return 875;
+}
+
 /* Bit-timing calculation derived from:
  *
  * Code based on LinCAN sources and H8S2638 project
@@ -78,17 +90,10 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 	u64 v64;
 	int err;
 
-	/* Use CiA recommended sample points */
-	if (bt->sample_point) {
+	if (bt->sample_point)
 		sample_point = bt->sample_point;
-	} else {
-		if (bt->bitrate > 800 * KILO /* BPS */)
-			sample_point = 750;
-		else if (bt->bitrate > 500 * KILO /* BPS */)
-			sample_point = 800;
-		else
-			sample_point = 875;
-	}
+	else
+		sample_point = can_calc_sample_point_nrz(bt);
 
 	/* tseg even = round down, odd = round up */
 	for (tseg = (btc->tseg1_max + btc->tseg2_max) * 2 + 1;

-- 
2.51.0

[PATCH RFC 3/3] can: calc_bittiming: add can_calc_sample_point_pwm()

[Vincent Mailhol]

The optimum sample point value depends on the bit symmetry. The more
asymmetric the bit is, the more the sample point would be located
towards the end of the bit. On the contrary, if the transceiver only
has a small asymmetry, the optimal sample point would be slightly
after the centre of the bit.

For NRZ encoding (used by Classical CAN, CAN FD and CAN XL with TMS
off), the optimum sample points values are above 70% as implemented in
can_calc_sample_point_nrz().

When TMS is on, CAN XL optimum sample points are near to 50% or
60% [1]. Add can_calc_sample_point_pwm() which returns a sample point
which is suitable for PWM encoding. We crafted the formula to make it
return the same values as below table (source: table 3 of [1]).

       Bit rate (Mbits/s)	Sample point
       -------------------------------------
         2.0			 51.3%
         5.0			 53.1%
         8.0			 55.0%
        10.0			 56.3%
        12.3			 53.8%
        13.3			 58.3%
        14.5			 54.5%
        16.0			 60.0%
        17.7			 55.6%
        20.0			 62.5%

The calculation simply consists of setting a slightly too high sample
point and then letting can_update_sample_point() correct the values.

For now, it is just a formula up our sleeves which matches the
empirical observations of [1]. Once CiA recommendations become
available, can_calc_sample_point_pwm() should be updated accordingly.

[1] CAN XL system design: Clock tolerances and edge deviations edge
    deviations
Link: https://www.can-cia.org/fileadmin/cia/documents/publications/cnlm/december_2024/cnlm_24-4_p18_can_xl_system_design_clock_tolerances_and_edge_deviations_dr_arthur_mutter_bosch.pdf

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/calc_bittiming.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 9b2d0e458518..be6726dcd7e7 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -22,6 +22,21 @@ static int can_calc_sample_point_nrz(const struct can_bittiming *bt)
 	return 875;
 }
 
+/* Sample points for Pulse-Width Modulation encoding. */
+static int can_calc_sample_point_pwm(const struct can_bittiming *bt)
+{
+	if (bt->bitrate > 15 * MEGA /* BPS */)
+		return 625;
+
+	if (bt->bitrate > 9 * MEGA /* BPS */)
+		return 600;
+
+	if (bt->bitrate > 4 * MEGA /* BPS */)
+		return 560;
+
+	return 520;
+}
+
 /* Bit-timing calculation derived from:
  *
  * Code based on LinCAN sources and H8S2638 project
@@ -92,6 +107,10 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 
 	if (bt->sample_point)
 		sample_point = bt->sample_point;
+
+	else if (btc == priv->xl.data_bittiming_const &&
+		 (priv->ctrlmode & CAN_CTRLMODE_XL_TMS))
+		sample_point = can_calc_sample_point_pwm(bt);
 	else
 		sample_point = can_calc_sample_point_nrz(bt);
 

-- 
2.51.0

Re: [PATCH RFC 0/3] can: calc_bittiming: add sample point calculation for PWM encoding

[Oliver Hartkopp]

On 02.11.25 23:01, Vincent Mailhol wrote:
> I wrote this series in response to Oliver's feedback in [1]. I am
> sending this as an RFC based on top of [2] for the moment so that we
> can discuss this separately. The plan is to merge this to the CAN XL
> branch once we reach a consensus on this part.
> 
> Patch #1 and #2 are refactors, patch #3 introduces a dedicated
> function for the PWM sample point calculation.
> 
> [1] https://lore.kernel.org/linux-can/743ba133-3735-48fd-994a-9727cfe8c114@hartkopp.net/
> 
> [2] [PATCH v2 00/10] can: netlink: add CAN XL
> Link: https://lore.kernel.org/linux-can/20251021-canxl-netlink-v2-0-8b8f58257ab6@kernel.org/
> 
> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>

Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>

> ---
> Vincent Mailhol (3):
>        can: calc_bittiming: get rid of the incorrect "nominal" word
>        can: calc_bittiming: add can_calc_sample_point_nrz()
>        can: calc_bittiming: add can_calc_sample_point_pwm()
> 
>   drivers/net/can/dev/calc_bittiming.c | 68 ++++++++++++++++++++++++------------
>   1 file changed, 45 insertions(+), 23 deletions(-)
> ---
> base-commit: ffee675aceb9f44b0502a8bec912abb0c4f4af62
> change-id: 20251102-pwm_sample_point-8823cb3cd459
> prerequisite-change-id: 20241229-canxl-netlink-bc640af10673:v2
> prerequisite-patch-id: 6b3294205bd76b38257516c63b7001ab242c9b62
> prerequisite-patch-id: 56431d12edcc0f325cf5204bb6868742c462c0ed
> prerequisite-patch-id: 1547fd7ea8f1937f0491cfc0996b09890f850991
> prerequisite-patch-id: 1dae270b0454352e46b927f71d1b47ff2bf7a49e
> prerequisite-patch-id: e4d43de873dfdefc023a0b86e397b37ea2b9e9a3
> prerequisite-patch-id: 4f3db477ff411effe70075c59ae6eac04fc65600
> prerequisite-patch-id: 148dbfce9d3bb09537087ee93e60bb7819bdadee
> prerequisite-patch-id: 7996539e26d449e8db260425c7287b4dce8cdf35
> prerequisite-patch-id: 42215044df6a63fff07c7a7d771d7dc375cc8b0e
> prerequisite-patch-id: 640ebf8ac8a1d114dcb91e6c05b9414bd09416fc
> prerequisite-patch-id: 84ee5e4f937f8e4cd97833d601affea78fe55914
> 
> Best regards,