[PATCH 0/9] can: rcar_canfd: Add support for Transceiver Delay Compensation

[PATCH 0/9] can: rcar_canfd: Add support for Transceiver Delay Compensation

[Geert Uytterhoeven]

	Hi all,

This patch series adds CAN-FD Transceiver Delay Compensation support to
the R-Car CAN-FD driver, after the customary cleanups and refactorings.

This has been tested on R-Car V4H (White Hawk), V4M (Gray Hawk Single),
and E3 (Ebisu-4D[1]), using various data bit rates.  Without proper TDC
configuration, transmitting at 8 Mbps makes the CAN-FD controller enter
BUS-OFF state.  The TDCV value as measured by the CAN-FD controller is 4
on all boards tested (base clock 40 MHz, i.e. 25 ns period), and ca. 90
ns as measured by a logic analyzer on Gray Hawk Single.

Note that the BSP (predating upstream TDC support), uses a much simpler
method: for transfer rates >= 5 Mbps on R-Car Gen4, it enables TDC with
a hardcoded (hardware) TDCO value of 2 (i.e. actual 3), which matches
the behavior of this series at 8 Mbps.

Thanks for your comments!

[1] r8a77990.dtsi configures the CANFD core clock to 40 MHz, limiting
    transfer rates to 4 Mbps.  Enable support for 8 Mbps by adding to
    ebisu.dtsi:

	&canfd {
		assigned-clock-rates = <80000000>;
	}

Geert Uytterhoeven (9):
  can: rcar_canfd: Consistently use ndev for net_device pointers
  can: rcar_canfd: Use ndev parameter in rcar_canfd_set_bittiming()
  can: rcar_canfd: Add helper variable ndev to rcar_canfd_rx_pkt()
  can: rcar_canfd: Add helper variable dev to
    rcar_canfd_reset_controller()
  can: rcar_canfd: Simplify data access in rcar_canfd_{ge,pu}t_data()
  can: rcar_canfd: Repurpose f_dcfg base for other registers
  can: rcar_canfd: Share config code in rcar_canfd_set_bittiming()
  can: rcar_canfd: Return early in rcar_canfd_set_bittiming() when not
    FD
  can: rcar_canfd: Add support for Transceiver Delay Compensation

 drivers/net/can/rcar/rcar_canfd.c | 193 +++++++++++++++++++-----------
 1 file changed, 126 insertions(+), 67 deletions(-)

-- 
2.43.0

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/9] can: rcar_canfd: Consistently use ndev for net_device pointers

[Geert Uytterhoeven]

Most net_device pointers are named "ndev", but some are called "dev".
Increase uniformity by always using "ndev".

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 7f10213738e5cee7..2174c9667cabce54 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1436,9 +1436,9 @@ static irqreturn_t rcar_canfd_channel_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void rcar_canfd_set_bittiming(struct net_device *dev)
+static void rcar_canfd_set_bittiming(struct net_device *ndev)
 {
-	struct rcar_canfd_channel *priv = netdev_priv(dev);
+	struct rcar_canfd_channel *priv = netdev_priv(ndev);
 	struct rcar_canfd_global *gpriv = priv->gpriv;
 	const struct can_bittiming *bt = &priv->can.bittiming;
 	const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
@@ -1818,10 +1818,10 @@ static int rcar_canfd_do_set_mode(struct net_device *ndev, enum can_mode mode)
 	}
 }
 
-static int rcar_canfd_get_berr_counter(const struct net_device *dev,
+static int rcar_canfd_get_berr_counter(const struct net_device *ndev,
 				       struct can_berr_counter *bec)
 {
-	struct rcar_canfd_channel *priv = netdev_priv(dev);
+	struct rcar_canfd_channel *priv = netdev_priv(ndev);
 	u32 val, ch = priv->channel;
 
 	/* Peripheral clock is already enabled in probe */
-- 
2.43.0

[PATCH 2/9] can: rcar_canfd: Use ndev parameter in rcar_canfd_set_bittiming()

[Geert Uytterhoeven]

There is no need to do a back-and-forth "priv = netdev_priv(ndev)" and
"priv->ndev" where the "ndev" parameter is available.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 2174c9667cabce54..dbf17b16c18aa5cc 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1458,7 +1458,7 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
 		       RCANFD_NCFG_NSJW(gpriv, sjw) | RCANFD_NCFG_NTSEG2(gpriv, tseg2));
 
 		rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);
-		netdev_dbg(priv->ndev, "nrate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
+		netdev_dbg(ndev, "nrate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
 			   brp, sjw, tseg1, tseg2);
 
 		/* Data bit timing settings */
@@ -1471,7 +1471,7 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
 		       RCANFD_DCFG_DSJW(gpriv, sjw) | RCANFD_DCFG_DTSEG2(gpriv, tseg2));
 
 		rcar_canfd_write(priv->base, RCANFD_F_DCFG(gpriv, ch), cfg);
-		netdev_dbg(priv->ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
+		netdev_dbg(ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
 			   brp, sjw, tseg1, tseg2);
 	} else {
 		/* Classical CAN only mode */
@@ -1488,8 +1488,7 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
 		}
 
 		rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);
-		netdev_dbg(priv->ndev,
-			   "rate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
+		netdev_dbg(ndev, "rate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
 			   brp, sjw, tseg1, tseg2);
 	}
 }
-- 
2.43.0

Re: [PATCH 2/9] can: rcar_canfd: Use ndev parameter in rcar_canfd_set_bittiming()

[Vincent Mailhol]

On 02/06/2025 at 20:54, Geert Uytterhoeven wrote:
> There is no need to do a back-and-forth "priv = netdev_priv(ndev)" and
> "priv->ndev" where the "ndev" parameter is available.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Are these still useful anyhow? You can get all the bittiming values through the
netlink interface.

Well, if you tell me these are still useful, then I trust you and OK to keep. If
not, consider removing.

> ---
>  drivers/net/can/rcar/rcar_canfd.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 2174c9667cabce54..dbf17b16c18aa5cc 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -1458,7 +1458,7 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
>  		       RCANFD_NCFG_NSJW(gpriv, sjw) | RCANFD_NCFG_NTSEG2(gpriv, tseg2));
>  
>  		rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);
> -		netdev_dbg(priv->ndev, "nrate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
> +		netdev_dbg(ndev, "nrate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
>  			   brp, sjw, tseg1, tseg2);
>  
>  		/* Data bit timing settings */
> @@ -1471,7 +1471,7 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
>  		       RCANFD_DCFG_DSJW(gpriv, sjw) | RCANFD_DCFG_DTSEG2(gpriv, tseg2));
>  
>  		rcar_canfd_write(priv->base, RCANFD_F_DCFG(gpriv, ch), cfg);
> -		netdev_dbg(priv->ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
> +		netdev_dbg(ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
>  			   brp, sjw, tseg1, tseg2);
>  	} else {
>  		/* Classical CAN only mode */
> @@ -1488,8 +1488,7 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
>  		}
>  
>  		rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);
> -		netdev_dbg(priv->ndev,
> -			   "rate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
> +		netdev_dbg(ndev, "rate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
>  			   brp, sjw, tseg1, tseg2);
>  	}
>  }

Yours sincerely,
Vincent Mailhol

Re: [PATCH 2/9] can: rcar_canfd: Use ndev parameter in rcar_canfd_set_bittiming()

[Geert Uytterhoeven]

Hi Vincent,

On Mon, 2 Jun 2025 at 15:01, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> On 02/06/2025 at 20:54, Geert Uytterhoeven wrote:
> > There is no need to do a back-and-forth "priv = netdev_priv(ndev)" and
> > "priv->ndev" where the "ndev" parameter is available.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Are these still useful anyhow? You can get all the bittiming values through the
> netlink interface.

My first thought was "They are useful, when you are stuck with an
old initrd that only has an old ifconfig", but then I realized you
need a fairly recent iproute2 package anyway.

> Well, if you tell me these are still useful, then I trust you and OK to keep. If
> not, consider removing.

No, I will remove them.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 3/9] can: rcar_canfd: Add helper variable ndev to rcar_canfd_rx_pkt()

[Geert Uytterhoeven]

rcar_canfd_rx_pkt() has many users of "priv->ndev".  Introduce a
shorthand to simplify the code.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index dbf17b16c18aa5cc..9251d8fd72472f22 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1690,7 +1690,8 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
 
 static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
 {
-	struct net_device_stats *stats = &priv->ndev->stats;
+	struct net_device *ndev = priv->ndev;
+	struct net_device_stats *stats = &ndev->stats;
 	struct rcar_canfd_global *gpriv = priv->gpriv;
 	struct canfd_frame *cf;
 	struct sk_buff *skb;
@@ -1706,14 +1707,13 @@ static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
 
 		if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
 		    sts & RCANFD_RFFDSTS_RFFDF)
-			skb = alloc_canfd_skb(priv->ndev, &cf);
+			skb = alloc_canfd_skb(ndev, &cf);
 		else
-			skb = alloc_can_skb(priv->ndev,
-					    (struct can_frame **)&cf);
+			skb = alloc_can_skb(ndev, (struct can_frame **)&cf);
 	} else {
 		id = rcar_canfd_read(priv->base, RCANFD_C_RFID(ridx));
 		dlc = rcar_canfd_read(priv->base, RCANFD_C_RFPTR(ridx));
-		skb = alloc_can_skb(priv->ndev, (struct can_frame **)&cf);
+		skb = alloc_can_skb(ndev, (struct can_frame **)&cf);
 	}
 
 	if (!skb) {
@@ -1734,7 +1734,7 @@ static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
 
 		if (sts & RCANFD_RFFDSTS_RFESI) {
 			cf->flags |= CANFD_ESI;
-			netdev_dbg(priv->ndev, "ESI Error\n");
+			netdev_dbg(ndev, "ESI Error\n");
 		}
 
 		if (!(sts & RCANFD_RFFDSTS_RFFDF) && (id & RCANFD_RFID_RFRTR)) {
-- 
2.43.0

[PATCH 4/9] can: rcar_canfd: Add helper variable dev to rcar_canfd_reset_controller()

[Geert Uytterhoeven]

rcar_canfd_reset_controller() has many users of "pdev->dev".  Introduce
a shorthand to simplify the code.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 9251d8fd72472f22..94b3effd423acf77 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -841,6 +841,7 @@ static void rcar_canfd_set_mode(struct rcar_canfd_global *gpriv)
 
 static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
 {
+	struct device *dev = &gpriv->pdev->dev;
 	u32 sts, ch;
 	int err;
 
@@ -850,7 +851,7 @@ static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
 	err = readl_poll_timeout((gpriv->base + RCANFD_GSTS), sts,
 				 !(sts & RCANFD_GSTS_GRAMINIT), 2, 500000);
 	if (err) {
-		dev_dbg(&gpriv->pdev->dev, "global raminit failed\n");
+		dev_dbg(dev, "global raminit failed\n");
 		return err;
 	}
 
@@ -863,7 +864,7 @@ static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
 	err = readl_poll_timeout((gpriv->base + RCANFD_GSTS), sts,
 				 (sts & RCANFD_GSTS_GRSTSTS), 2, 500000);
 	if (err) {
-		dev_dbg(&gpriv->pdev->dev, "global reset failed\n");
+		dev_dbg(dev, "global reset failed\n");
 		return err;
 	}
 
@@ -887,8 +888,7 @@ static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
 					 (sts & RCANFD_CSTS_CRSTSTS),
 					 2, 500000);
 		if (err) {
-			dev_dbg(&gpriv->pdev->dev,
-				"channel %u reset failed\n", ch);
+			dev_dbg(dev, "channel %u reset failed\n", ch);
 			return err;
 		}
 	}
-- 
2.43.0

[PATCH 5/9] can: rcar_canfd: Simplify data access in rcar_canfd_{ge,pu}t_data()

[Geert Uytterhoeven]

Replace the repeated casts, pointer additions, and pointer dereferences
by array accesses to improve readability.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 94b3effd423acf77..0cad3c198e58e494 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -781,23 +781,23 @@ static void rcar_canfd_update_bit(void __iomem *base, u32 reg,
 static void rcar_canfd_get_data(struct rcar_canfd_channel *priv,
 				struct canfd_frame *cf, u32 off)
 {
+	u32 *data = (u32 *)cf->data;
 	u32 i, lwords;
 
 	lwords = DIV_ROUND_UP(cf->len, sizeof(u32));
 	for (i = 0; i < lwords; i++)
-		*((u32 *)cf->data + i) =
-			rcar_canfd_read(priv->base, off + i * sizeof(u32));
+		data[i] = rcar_canfd_read(priv->base, off + i * sizeof(u32));
 }
 
 static void rcar_canfd_put_data(struct rcar_canfd_channel *priv,
 				struct canfd_frame *cf, u32 off)
 {
+	const u32 *data = (u32 *)cf->data;
 	u32 i, lwords;
 
 	lwords = DIV_ROUND_UP(cf->len, sizeof(u32));
 	for (i = 0; i < lwords; i++)
-		rcar_canfd_write(priv->base, off + i * sizeof(u32),
-				 *((u32 *)cf->data + i));
+		rcar_canfd_write(priv->base, off + i * sizeof(u32), data[i]);
 }
 
 static void rcar_canfd_tx_failure_cleanup(struct net_device *ndev)
-- 
2.43.0

[PATCH 6/9] can: rcar_canfd: Repurpose f_dcfg base for other registers

[Geert Uytterhoeven]

Reuse the existing Channel Data Bitrate Configuration Register offset
member in the register configuration as the base offset for all related
channel-specific registers.
Rename the member and update the (incorrect) comment to reflect this.

This fixes the addresses of all other (currently unused)
channel-specific registers on R-Car Gen4 and RZ/G3E, and allows us to
replace RCANFD_GEN4_FDCFG() by the more generic RCANFD_F_CFDCFG().

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 0cad3c198e58e494..7a9a88fa5fb1a521 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -425,18 +425,16 @@
 #define RCANFD_C_RPGACC(r)		(0x1900 + (0x04 * (r)))
 
 /* R-Car Gen4 Classical and CAN FD mode specific register map */
-#define RCANFD_GEN4_FDCFG(m)		(0x1404 + (0x20 * (m)))
-
 #define RCANFD_GEN4_GAFL_OFFSET		(0x1800)
 
 /* CAN FD mode specific register map */
 
 /* RSCFDnCFDCmXXX -> RCANFD_F_XXX(m) */
-#define RCANFD_F_DCFG(gpriv, m)		((gpriv)->info->regs->f_dcfg + (0x20 * (m)))
-#define RCANFD_F_CFDCFG(m)		(0x0504 + (0x20 * (m)))
-#define RCANFD_F_CFDCTR(m)		(0x0508 + (0x20 * (m)))
-#define RCANFD_F_CFDSTS(m)		(0x050c + (0x20 * (m)))
-#define RCANFD_F_CFDCRC(m)		(0x0510 + (0x20 * (m)))
+#define RCANFD_F_DCFG(gpriv, m)		((gpriv)->info->regs->coffset + 0x00 + (0x20 * (m)))
+#define RCANFD_F_CFDCFG(gpriv, m)	((gpriv)->info->regs->coffset + 0x04 + (0x20 * (m)))
+#define RCANFD_F_CFDCTR(gpriv, m)	((gpriv)->info->regs->coffset + 0x08 + (0x20 * (m)))
+#define RCANFD_F_CFDSTS(gpriv, m)	((gpriv)->info->regs->coffset + 0x0c + (0x20 * (m)))
+#define RCANFD_F_CFDCRC(gpriv, m)	((gpriv)->info->regs->coffset + 0x10 + (0x20 * (m)))
 
 /* RSCFDnCFDGAFLXXXj offset */
 #define RCANFD_F_GAFL_OFFSET		(0x1000)
@@ -510,7 +508,7 @@ struct rcar_canfd_regs {
 	u16 cfcc;	/* Common FIFO Configuration/Control Register */
 	u16 cfsts;	/* Common FIFO Status Register */
 	u16 cfpctr;	/* Common FIFO Pointer Control Register */
-	u16 f_dcfg;	/* Global FD Configuration Register */
+	u16 coffset;	/* Channel Data Bitrate Configuration Register */
 	u16 rfoffset;	/* Receive FIFO buffer access ID register */
 	u16 cfoffset;	/* Transmit/receive FIFO buffer access ID register */
 };
@@ -641,7 +639,7 @@ static const struct rcar_canfd_regs rcar_gen3_regs = {
 	.cfcc = 0x0118,
 	.cfsts = 0x0178,
 	.cfpctr = 0x01d8,
-	.f_dcfg = 0x0500,
+	.coffset = 0x0500,
 	.rfoffset = 0x3000,
 	.cfoffset = 0x3400,
 };
@@ -651,7 +649,7 @@ static const struct rcar_canfd_regs rcar_gen4_regs = {
 	.cfcc = 0x0120,
 	.cfsts = 0x01e0,
 	.cfpctr = 0x0240,
-	.f_dcfg = 0x1400,
+	.coffset = 0x1400,
 	.rfoffset = 0x6000,
 	.cfoffset = 0x6400,
 };
@@ -827,8 +825,8 @@ static void rcar_canfd_set_mode(struct rcar_canfd_global *gpriv)
 
 		for_each_set_bit(ch, &gpriv->channels_mask,
 				 gpriv->info->max_channels)
-			rcar_canfd_set_bit(gpriv->base, RCANFD_GEN4_FDCFG(ch),
-					   val);
+			rcar_canfd_set_bit(gpriv->base,
+					   RCANFD_F_CFDCFG(gpriv, ch), val);
 	} else {
 		if (gpriv->fdmode)
 			rcar_canfd_set_bit(gpriv->base, RCANFD_GRMCFG,
-- 
2.43.0

Re: [PATCH 6/9] can: rcar_canfd: Repurpose f_dcfg base for other registers

[Vincent Mailhol]

On 02/06/2025 at 20:54, Geert Uytterhoeven wrote:
> Reuse the existing Channel Data Bitrate Configuration Register offset
> member in the register configuration as the base offset for all related
> channel-specific registers.
> Rename the member and update the (incorrect) comment to reflect this.
> 
> This fixes the addresses of all other (currently unused)
> channel-specific registers on R-Car Gen4 and RZ/G3E, and allows us to
> replace RCANFD_GEN4_FDCFG() by the more generic RCANFD_F_CFDCFG().
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/net/can/rcar/rcar_canfd.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 0cad3c198e58e494..7a9a88fa5fb1a521 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -425,18 +425,16 @@
>  #define RCANFD_C_RPGACC(r)		(0x1900 + (0x04 * (r)))
>  
>  /* R-Car Gen4 Classical and CAN FD mode specific register map */
> -#define RCANFD_GEN4_FDCFG(m)		(0x1404 + (0x20 * (m)))
> -
>  #define RCANFD_GEN4_GAFL_OFFSET		(0x1800)
>  
>  /* CAN FD mode specific register map */
>  
>  /* RSCFDnCFDCmXXX -> RCANFD_F_XXX(m) */
> -#define RCANFD_F_DCFG(gpriv, m)		((gpriv)->info->regs->f_dcfg + (0x20 * (m)))
> -#define RCANFD_F_CFDCFG(m)		(0x0504 + (0x20 * (m)))
> -#define RCANFD_F_CFDCTR(m)		(0x0508 + (0x20 * (m)))
> -#define RCANFD_F_CFDSTS(m)		(0x050c + (0x20 * (m)))
> -#define RCANFD_F_CFDCRC(m)		(0x0510 + (0x20 * (m)))
> +#define RCANFD_F_DCFG(gpriv, m)		((gpriv)->info->regs->coffset + 0x00 + (0x20 * (m)))
> +#define RCANFD_F_CFDCFG(gpriv, m)	((gpriv)->info->regs->coffset + 0x04 + (0x20 * (m)))
> +#define RCANFD_F_CFDCTR(gpriv, m)	((gpriv)->info->regs->coffset + 0x08 + (0x20 * (m)))
> +#define RCANFD_F_CFDSTS(gpriv, m)	((gpriv)->info->regs->coffset + 0x0c + (0x20 * (m)))
> +#define RCANFD_F_CFDCRC(gpriv, m)	((gpriv)->info->regs->coffset + 0x10 + (0x20 * (m)))

I really start to dislike all those function like macros in the rcar_canfd
driver. The only benefits of a function like macro is either to have type
polymorphism or to generate integer constant expression or to work with context
specific info (e.g. __func__ or __LINE__).

Can you just change these five function like macros to static functions?

And from now on, each time there is a need to modify one of the rcar_canfd, I
would like this to become an opportunity to little by little clean up that macro
madness.

>  /* RSCFDnCFDGAFLXXXj offset */
>  #define RCANFD_F_GAFL_OFFSET		(0x1000)
> @@ -510,7 +508,7 @@ struct rcar_canfd_regs {
>  	u16 cfcc;	/* Common FIFO Configuration/Control Register */
>  	u16 cfsts;	/* Common FIFO Status Register */
>  	u16 cfpctr;	/* Common FIFO Pointer Control Register */
> -	u16 f_dcfg;	/* Global FD Configuration Register */
> +	u16 coffset;	/* Channel Data Bitrate Configuration Register */
>  	u16 rfoffset;	/* Receive FIFO buffer access ID register */
>  	u16 cfoffset;	/* Transmit/receive FIFO buffer access ID register */
>  };
> @@ -641,7 +639,7 @@ static const struct rcar_canfd_regs rcar_gen3_regs = {
>  	.cfcc = 0x0118,
>  	.cfsts = 0x0178,
>  	.cfpctr = 0x01d8,
> -	.f_dcfg = 0x0500,
> +	.coffset = 0x0500,
>  	.rfoffset = 0x3000,
>  	.cfoffset = 0x3400,
>  };
> @@ -651,7 +649,7 @@ static const struct rcar_canfd_regs rcar_gen4_regs = {
>  	.cfcc = 0x0120,
>  	.cfsts = 0x01e0,
>  	.cfpctr = 0x0240,
> -	.f_dcfg = 0x1400,
> +	.coffset = 0x1400,
>  	.rfoffset = 0x6000,
>  	.cfoffset = 0x6400,
>  };
> @@ -827,8 +825,8 @@ static void rcar_canfd_set_mode(struct rcar_canfd_global *gpriv)
>  
>  		for_each_set_bit(ch, &gpriv->channels_mask,
>  				 gpriv->info->max_channels)
> -			rcar_canfd_set_bit(gpriv->base, RCANFD_GEN4_FDCFG(ch),
> -					   val);
> +			rcar_canfd_set_bit(gpriv->base,
> +					   RCANFD_F_CFDCFG(gpriv, ch), val);
>  	} else {
>  		if (gpriv->fdmode)
>  			rcar_canfd_set_bit(gpriv->base, RCANFD_GRMCFG,

Yours sincerely,
Vincent Mailhol

Re: [PATCH 6/9] can: rcar_canfd: Repurpose f_dcfg base for other registers

[Marc Kleine-Budde]

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

On 02.06.2025 22:16:48, Vincent Mailhol wrote:
> I really start to dislike all those function like macros in the rcar_canfd
> driver. The only benefits of a function like macro is either to have type
> polymorphism or to generate integer constant expression or to work with context
> specific info (e.g. __func__ or __LINE__).
> 
> Can you just change these five function like macros to static functions?
> 
> And from now on, each time there is a need to modify one of the rcar_canfd, I
> would like this to become an opportunity to little by little clean up that macro
> madness.

+1

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 --]

Re: [PATCH 6/9] can: rcar_canfd: Repurpose f_dcfg base for other registers

[Geert Uytterhoeven]

Hi Vincent,

On Mon, 2 Jun 2025 at 15:16, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> On 02/06/2025 at 20:54, Geert Uytterhoeven wrote:
> > Reuse the existing Channel Data Bitrate Configuration Register offset
> > member in the register configuration as the base offset for all related
> > channel-specific registers.
> > Rename the member and update the (incorrect) comment to reflect this.
> >
> > This fixes the addresses of all other (currently unused)
> > channel-specific registers on R-Car Gen4 and RZ/G3E, and allows us to
> > replace RCANFD_GEN4_FDCFG() by the more generic RCANFD_F_CFDCFG().
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  drivers/net/can/rcar/rcar_canfd.c | 22 ++++++++++------------
> >  1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> > index 0cad3c198e58e494..7a9a88fa5fb1a521 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -425,18 +425,16 @@
> >  #define RCANFD_C_RPGACC(r)           (0x1900 + (0x04 * (r)))
> >
> >  /* R-Car Gen4 Classical and CAN FD mode specific register map */
> > -#define RCANFD_GEN4_FDCFG(m)         (0x1404 + (0x20 * (m)))
> > -
> >  #define RCANFD_GEN4_GAFL_OFFSET              (0x1800)
> >
> >  /* CAN FD mode specific register map */
> >
> >  /* RSCFDnCFDCmXXX -> RCANFD_F_XXX(m) */
> > -#define RCANFD_F_DCFG(gpriv, m)              ((gpriv)->info->regs->f_dcfg + (0x20 * (m)))
> > -#define RCANFD_F_CFDCFG(m)           (0x0504 + (0x20 * (m)))
> > -#define RCANFD_F_CFDCTR(m)           (0x0508 + (0x20 * (m)))
> > -#define RCANFD_F_CFDSTS(m)           (0x050c + (0x20 * (m)))
> > -#define RCANFD_F_CFDCRC(m)           (0x0510 + (0x20 * (m)))
> > +#define RCANFD_F_DCFG(gpriv, m)              ((gpriv)->info->regs->coffset + 0x00 + (0x20 * (m)))
> > +#define RCANFD_F_CFDCFG(gpriv, m)    ((gpriv)->info->regs->coffset + 0x04 + (0x20 * (m)))
> > +#define RCANFD_F_CFDCTR(gpriv, m)    ((gpriv)->info->regs->coffset + 0x08 + (0x20 * (m)))
> > +#define RCANFD_F_CFDSTS(gpriv, m)    ((gpriv)->info->regs->coffset + 0x0c + (0x20 * (m)))
> > +#define RCANFD_F_CFDCRC(gpriv, m)    ((gpriv)->info->regs->coffset + 0x10 + (0x20 * (m)))
>
> I really start to dislike all those function like macros in the rcar_canfd
> driver. The only benefits of a function like macro is either to have type
> polymorphism or to generate integer constant expression or to work with context
> specific info (e.g. __func__ or __LINE__).

I agree much can be improved in the way this driver accesses registers.
Unfortunately a large part of it is due to the horrendous naming of the
registers in the documentation, and the two different register layouts.

> Can you just change these five function like macros to static functions?

I assume you want something like was done commit 6b9f8b53a1f3ad8e
("can: rcar_canfd: Add rcar_canfd_setrnc()")?

These five macro just calculate the offsets for specific registers
and for the specified channel indices.  Their return values are to
be passed to one of the five accessors that take register offsets
(rcar_canfd_{read,write,set_bit,cleat_bit, update}()).  Hence
converting the macros to accessor functions means there will be more
than five functions...

> And from now on, each time there is a need to modify one of the rcar_canfd, I
> would like this to become an opportunity to little by little clean up that macro
> madness.

That's exactly what Biju and I are doing, slowly ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 6/9] can: rcar_canfd: Repurpose f_dcfg base for other registers

[Vincent Mailhol]

On 02/06/2025 at 22:49, Geert Uytterhoeven wrote:
> Hi Vincent,
> 
> On Mon, 2 Jun 2025 at 15:16, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
>> On 02/06/2025 at 20:54, Geert Uytterhoeven wrote:
>>> Reuse the existing Channel Data Bitrate Configuration Register offset
>>> member in the register configuration as the base offset for all related
>>> channel-specific registers.
>>> Rename the member and update the (incorrect) comment to reflect this.
>>>
>>> This fixes the addresses of all other (currently unused)
>>> channel-specific registers on R-Car Gen4 and RZ/G3E, and allows us to
>>> replace RCANFD_GEN4_FDCFG() by the more generic RCANFD_F_CFDCFG().
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>>  drivers/net/can/rcar/rcar_canfd.c | 22 ++++++++++------------
>>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
>>> index 0cad3c198e58e494..7a9a88fa5fb1a521 100644
>>> --- a/drivers/net/can/rcar/rcar_canfd.c
>>> +++ b/drivers/net/can/rcar/rcar_canfd.c
>>> @@ -425,18 +425,16 @@
>>>  #define RCANFD_C_RPGACC(r)           (0x1900 + (0x04 * (r)))
>>>
>>>  /* R-Car Gen4 Classical and CAN FD mode specific register map */
>>> -#define RCANFD_GEN4_FDCFG(m)         (0x1404 + (0x20 * (m)))
>>> -
>>>  #define RCANFD_GEN4_GAFL_OFFSET              (0x1800)
>>>
>>>  /* CAN FD mode specific register map */
>>>
>>>  /* RSCFDnCFDCmXXX -> RCANFD_F_XXX(m) */
>>> -#define RCANFD_F_DCFG(gpriv, m)              ((gpriv)->info->regs->f_dcfg + (0x20 * (m)))
>>> -#define RCANFD_F_CFDCFG(m)           (0x0504 + (0x20 * (m)))
>>> -#define RCANFD_F_CFDCTR(m)           (0x0508 + (0x20 * (m)))
>>> -#define RCANFD_F_CFDSTS(m)           (0x050c + (0x20 * (m)))
>>> -#define RCANFD_F_CFDCRC(m)           (0x0510 + (0x20 * (m)))
>>> +#define RCANFD_F_DCFG(gpriv, m)              ((gpriv)->info->regs->coffset + 0x00 + (0x20 * (m)))
>>> +#define RCANFD_F_CFDCFG(gpriv, m)    ((gpriv)->info->regs->coffset + 0x04 + (0x20 * (m)))
>>> +#define RCANFD_F_CFDCTR(gpriv, m)    ((gpriv)->info->regs->coffset + 0x08 + (0x20 * (m)))
>>> +#define RCANFD_F_CFDSTS(gpriv, m)    ((gpriv)->info->regs->coffset + 0x0c + (0x20 * (m)))
>>> +#define RCANFD_F_CFDCRC(gpriv, m)    ((gpriv)->info->regs->coffset + 0x10 + (0x20 * (m)))
>>
>> I really start to dislike all those function like macros in the rcar_canfd
>> driver. The only benefits of a function like macro is either to have type
>> polymorphism or to generate integer constant expression or to work with context
>> specific info (e.g. __func__ or __LINE__).
> 
> I agree much can be improved in the way this driver accesses registers.
> Unfortunately a large part of it is due to the horrendous naming of the
> registers in the documentation, and the two different register layouts.
> 
>> Can you just change these five function like macros to static functions?
> 
> I assume you want something like was done commit 6b9f8b53a1f3ad8e
> ("can: rcar_canfd: Add rcar_canfd_setrnc()")?
> 
> These five macro just calculate the offsets for specific registers
> and for the specified channel indices.  Their return values are to
> be passed to one of the five accessors that take register offsets
> (rcar_canfd_{read,write,set_bit,cleat_bit, update}()).  Hence
> converting the macros to accessor functions means there will be more
> than five functions...

Not necessarily, you can just have:

  rcar_canfd_f_dcfg()
  rcar_canfd_f_cfdcfg()
  ...

and so on. Those would be used exactly the same way as the current function like
macro.

Or you can have:

  #define RCANFD_F_DCFG_BASE 0x00
  #define RCANFD_F_CFDCFS_BASE 0x04
  ...

and then a:

  rcar_canfd_get_c_reg(coffset, base, m)


I am keen on letting you decide what you think is best.

>> And from now on, each time there is a need to modify one of the rcar_canfd, I
>> would like this to become an opportunity to little by little clean up that macro
>> madness.
> 
> That's exactly what Biju and I are doing, slowly ;-)


Yours sincerely,
Vincent Mailhol

[PATCH 7/9] can: rcar_canfd: Share config code in rcar_canfd_set_bittiming()

[Geert Uytterhoeven]

The configuration register format for nominal bit timings in CAN-FD mode
and the format for bit timings in CAN mode on CAN-FD controllers with
shared Classical CAN registers are the same.

Restructure the code to make this clear, also reducing kernel size by 80
bytes.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 7a9a88fa5fb1a521..6e77e4844c3f63e9 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1450,15 +1450,19 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
 	tseg1 = bt->prop_seg + bt->phase_seg1 - 1;
 	tseg2 = bt->phase_seg2 - 1;
 
-	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
-		/* CAN FD only mode */
+	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) || gpriv->info->shared_can_regs) {
 		cfg = (RCANFD_NCFG_NTSEG1(gpriv, tseg1) | RCANFD_NCFG_NBRP(brp) |
 		       RCANFD_NCFG_NSJW(gpriv, sjw) | RCANFD_NCFG_NTSEG2(gpriv, tseg2));
+	} else {
+		cfg = (RCANFD_CFG_TSEG1(tseg1) | RCANFD_CFG_BRP(brp) |
+		       RCANFD_CFG_SJW(sjw) | RCANFD_CFG_TSEG2(tseg2));
+	}
 
-		rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);
-		netdev_dbg(ndev, "nrate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
-			   brp, sjw, tseg1, tseg2);
+	rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);
+	netdev_dbg(ndev, "nrate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n", brp,
+		   sjw, tseg1, tseg2);
 
+	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
 		/* Data bit timing settings */
 		brp = dbt->brp - 1;
 		sjw = dbt->sjw - 1;
@@ -1471,23 +1475,6 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
 		rcar_canfd_write(priv->base, RCANFD_F_DCFG(gpriv, ch), cfg);
 		netdev_dbg(ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
 			   brp, sjw, tseg1, tseg2);
-	} else {
-		/* Classical CAN only mode */
-		if (gpriv->info->shared_can_regs) {
-			cfg = (RCANFD_NCFG_NTSEG1(gpriv, tseg1) |
-			       RCANFD_NCFG_NBRP(brp) |
-			       RCANFD_NCFG_NSJW(gpriv, sjw) |
-			       RCANFD_NCFG_NTSEG2(gpriv, tseg2));
-		} else {
-			cfg = (RCANFD_CFG_TSEG1(tseg1) |
-			       RCANFD_CFG_BRP(brp) |
-			       RCANFD_CFG_SJW(sjw) |
-			       RCANFD_CFG_TSEG2(tseg2));
-		}
-
-		rcar_canfd_write(priv->base, RCANFD_CCFG(ch), cfg);
-		netdev_dbg(ndev, "rate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
-			   brp, sjw, tseg1, tseg2);
 	}
 }
 
-- 
2.43.0

[PATCH 8/9] can: rcar_canfd: Return early in rcar_canfd_set_bittiming() when not FD

[Geert Uytterhoeven]

Return early after completing all setup for non-FD mode in
rcar_canfd_set_bittiming(), to prepare for the advent of more FD-only
setup.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 6e77e4844c3f63e9..7b6bb67ed6f76f14 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1462,20 +1462,21 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
 	netdev_dbg(ndev, "nrate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n", brp,
 		   sjw, tseg1, tseg2);
 
-	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
-		/* Data bit timing settings */
-		brp = dbt->brp - 1;
-		sjw = dbt->sjw - 1;
-		tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
-		tseg2 = dbt->phase_seg2 - 1;
-
-		cfg = (RCANFD_DCFG_DTSEG1(gpriv, tseg1) | RCANFD_DCFG_DBRP(brp) |
-		       RCANFD_DCFG_DSJW(gpriv, sjw) | RCANFD_DCFG_DTSEG2(gpriv, tseg2));
-
-		rcar_canfd_write(priv->base, RCANFD_F_DCFG(gpriv, ch), cfg);
-		netdev_dbg(ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
-			   brp, sjw, tseg1, tseg2);
-	}
+	if (!(priv->can.ctrlmode & CAN_CTRLMODE_FD))
+		return;
+
+	/* Data bit timing settings */
+	brp = dbt->brp - 1;
+	sjw = dbt->sjw - 1;
+	tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
+	tseg2 = dbt->phase_seg2 - 1;
+
+	cfg = (RCANFD_DCFG_DTSEG1(gpriv, tseg1) | RCANFD_DCFG_DBRP(brp) |
+	       RCANFD_DCFG_DSJW(gpriv, sjw) | RCANFD_DCFG_DTSEG2(gpriv, tseg2));
+
+	rcar_canfd_write(priv->base, RCANFD_F_DCFG(gpriv, ch), cfg);
+	netdev_dbg(ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
+		   brp, sjw, tseg1, tseg2);
 }
 
 static int rcar_canfd_start(struct net_device *ndev)
-- 
2.43.0

[PATCH 9/9] can: rcar_canfd: Add support for Transceiver Delay Compensation

[Geert Uytterhoeven]

The Renesas CAN-FD hardware block supports configuring Transceiver Delay
Compensation, and reading back the Transceiver Delay Compensation
Result, which is needed to support high transfer rates like 8 Mbps.
The Secondary Sample Point is either the measured delay plus the
configured offset, or just the configured offset.

Fix the existing RCANFD_FDCFG_TDCO() macro for the intended use case
(writing instead of reading the field).  Add register definition bits
for the Channel n CAN-FD Status Register.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/net/can/rcar/rcar_canfd.c | 80 +++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 7b6bb67ed6f76f14..a9c5b4ac040bdc0a 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -191,9 +191,19 @@
 /* RSCFDnCFDCmFDCFG */
 #define RCANFD_GEN4_FDCFG_CLOE		BIT(30)
 #define RCANFD_GEN4_FDCFG_FDOE		BIT(28)
+#define RCANFD_FDCFG_TDCO		GENMASK(23, 16)
 #define RCANFD_FDCFG_TDCE		BIT(9)
 #define RCANFD_FDCFG_TDCOC		BIT(8)
-#define RCANFD_FDCFG_TDCO(x)		(((x) & 0x7f) >> 16)
+
+/* RSCFDnCFDCmFDSTS */
+#define RCANFD_FDSTS_SOC		GENMASK(31, 24)
+#define RCANFD_FDSTS_EOC		GENMASK(23, 16)
+#define RCANFD_GEN4_FDSTS_PNSTS		GENMASK(13, 12)
+#define RCANFD_FDSTS_SOCO		BIT(9)
+#define RCANFD_FDSTS_EOCO		BIT(8)
+#define RCANFD_FDSTS_TDCR(gpriv, x)	((x) & ((gpriv)->info->tdc_const->tdcv_max - 1))
+#define RCANFD_FDSTS_TDCVF(gpriv) \
+	((gpriv)->info->tdc_const->tdcv_max > 128 ? BIT(15) : BIT(7))
 
 /* RSCFDnCFDRFCCx */
 #define RCANFD_RFCC_RFIM		BIT(12)
@@ -527,6 +537,7 @@ struct rcar_canfd_shift_data {
 struct rcar_canfd_hw_info {
 	const struct can_bittiming_const *nom_bittiming;
 	const struct can_bittiming_const *data_bittiming;
+	const struct can_tdc_const *tdc_const;
 	const struct rcar_canfd_regs *regs;
 	const struct rcar_canfd_shift_data *sh;
 	u8 rnc_field_width;
@@ -634,6 +645,25 @@ static const struct can_bittiming_const rcar_canfd_bittiming_const = {
 	.brp_inc = 1,
 };
 
+/* CAN FD Transmission Delay Compensation constants */
+static const struct can_tdc_const rcar_canfd_gen3_tdc_const = {
+	.tdcv_min = 1,
+	.tdcv_max = 128,
+	.tdco_min = 1,
+	.tdco_max = 128,
+	.tdcf_min = 0,	/* Filter window not supported */
+	.tdcf_max = 0,
+};
+
+static const struct can_tdc_const rcar_canfd_gen4_tdc_const = {
+	.tdcv_min = 1,
+	.tdcv_max = 256,
+	.tdco_min = 1,
+	.tdco_max = 256,
+	.tdcf_min = 0,	/* Filter window not supported */
+	.tdcf_max = 0,
+};
+
 static const struct rcar_canfd_regs rcar_gen3_regs = {
 	.rfcc = 0x00b8,
 	.cfcc = 0x0118,
@@ -679,6 +709,7 @@ static const struct rcar_canfd_shift_data rcar_gen4_shift_data = {
 static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
 	.nom_bittiming = &rcar_canfd_gen3_nom_bittiming_const,
 	.data_bittiming = &rcar_canfd_gen3_data_bittiming_const,
+	.tdc_const = &rcar_canfd_gen3_tdc_const,
 	.regs = &rcar_gen3_regs,
 	.sh = &rcar_gen3_shift_data,
 	.rnc_field_width = 8,
@@ -695,6 +726,7 @@ static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
 static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
 	.nom_bittiming = &rcar_canfd_gen4_nom_bittiming_const,
 	.data_bittiming = &rcar_canfd_gen4_data_bittiming_const,
+	.tdc_const = &rcar_canfd_gen4_tdc_const,
 	.regs = &rcar_gen4_regs,
 	.sh = &rcar_gen4_shift_data,
 	.rnc_field_width = 16,
@@ -711,6 +743,7 @@ static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
 static const struct rcar_canfd_hw_info rzg2l_hw_info = {
 	.nom_bittiming = &rcar_canfd_gen3_nom_bittiming_const,
 	.data_bittiming = &rcar_canfd_gen3_data_bittiming_const,
+	.tdc_const = &rcar_canfd_gen3_tdc_const,
 	.regs = &rcar_gen3_regs,
 	.sh = &rcar_gen3_shift_data,
 	.rnc_field_width = 8,
@@ -727,6 +760,7 @@ static const struct rcar_canfd_hw_info rzg2l_hw_info = {
 static const struct rcar_canfd_hw_info r9a09g047_hw_info = {
 	.nom_bittiming = &rcar_canfd_gen4_nom_bittiming_const,
 	.data_bittiming = &rcar_canfd_gen4_data_bittiming_const,
+	.tdc_const = &rcar_canfd_gen4_tdc_const,
 	.regs = &rcar_gen4_regs,
 	.sh = &rcar_gen4_shift_data,
 	.rnc_field_width = 16,
@@ -1436,12 +1470,15 @@ static irqreturn_t rcar_canfd_channel_interrupt(int irq, void *dev_id)
 
 static void rcar_canfd_set_bittiming(struct net_device *ndev)
 {
+	u32 mask = RCANFD_FDCFG_TDCO | RCANFD_FDCFG_TDCE | RCANFD_FDCFG_TDCOC;
 	struct rcar_canfd_channel *priv = netdev_priv(ndev);
 	struct rcar_canfd_global *gpriv = priv->gpriv;
 	const struct can_bittiming *bt = &priv->can.bittiming;
 	const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
+	const struct can_tdc_const *tdc_const = priv->can.fd.tdc_const;
+	const struct can_tdc *tdc = &priv->can.fd.tdc;
+	u32 cfg, tdcmode = 0, tdco = 0;
 	u16 brp, sjw, tseg1, tseg2;
-	u32 cfg;
 	u32 ch = priv->channel;
 
 	/* Nominal bit timing settings */
@@ -1477,6 +1514,22 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
 	rcar_canfd_write(priv->base, RCANFD_F_DCFG(gpriv, ch), cfg);
 	netdev_dbg(ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
 		   brp, sjw, tseg1, tseg2);
+
+	/* Transceiver Delay Compensation */
+	if (priv->can.ctrlmode & CAN_CTRLMODE_TDC_AUTO) {
+		/* TDC enabled, measured + offset */
+		tdcmode = RCANFD_FDCFG_TDCE;
+		tdco = tdc->tdco - 1;
+		netdev_dbg(ndev, "tdc: auto %u\n", tdco);
+	} else if (priv->can.ctrlmode & CAN_CTRLMODE_TDC_MANUAL) {
+		/* TDC enabled, offset only */
+		tdcmode = RCANFD_FDCFG_TDCE | RCANFD_FDCFG_TDCOC;
+		tdco = min(tdc->tdcv + tdc->tdco, tdc_const->tdco_max) - 1;
+		netdev_dbg(ndev, "tdc: manual %u\n", tdco);
+	}
+
+	rcar_canfd_update_bit(gpriv->base, RCANFD_F_CFDCFG(gpriv, ch), mask,
+			      tdcmode | FIELD_PREP(RCANFD_FDCFG_TDCO, tdco));
 }
 
 static int rcar_canfd_start(struct net_device *ndev)
@@ -1787,6 +1840,22 @@ static int rcar_canfd_rx_poll(struct napi_struct *napi, int quota)
 	return num_pkts;
 }
 
+static int rcar_canfd_get_auto_tdcv(const struct net_device *ndev, u32 *tdcv)
+{
+	struct rcar_canfd_channel *priv = netdev_priv(ndev);
+	struct rcar_canfd_global *gpriv = priv->gpriv;
+	u32 sts = rcar_canfd_read(priv->base, RCANFD_F_CFDSTS(gpriv, priv->channel));
+	u32 tdco = priv->can.fd.tdc.tdco;
+	u32 tdcr;
+
+	/* Transceiver Delay Compensation Result */
+	tdcr = RCANFD_FDSTS_TDCR(gpriv, sts) + 1;
+
+	*tdcv = tdcr < tdco ? 0 : tdcr - tdco;
+
+	return 0;
+}
+
 static int rcar_canfd_do_set_mode(struct net_device *ndev, enum can_mode mode)
 {
 	int err;
@@ -1909,12 +1978,17 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
 	if (gpriv->fdmode) {
 		priv->can.bittiming_const = gpriv->info->nom_bittiming;
 		priv->can.fd.data_bittiming_const = gpriv->info->data_bittiming;
+		priv->can.fd.tdc_const = gpriv->info->tdc_const;
 
 		/* Controller starts in CAN FD only mode */
 		err = can_set_static_ctrlmode(ndev, CAN_CTRLMODE_FD);
 		if (err)
 			goto fail;
-		priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
+
+		priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING |
+					       CAN_CTRLMODE_TDC_AUTO |
+					       CAN_CTRLMODE_TDC_MANUAL;
+		priv->can.fd.do_get_auto_tdcv = rcar_canfd_get_auto_tdcv;
 	} else {
 		/* Controller starts in Classical CAN only mode */
 		priv->can.bittiming_const = &rcar_canfd_bittiming_const;
-- 
2.43.0

Re: [PATCH 9/9] can: rcar_canfd: Add support for Transceiver Delay Compensation

[Vincent Mailhol]

On 02/06/2025 at 20:54, Geert Uytterhoeven wrote:
> The Renesas CAN-FD hardware block supports configuring Transceiver Delay
> Compensation, and reading back the Transceiver Delay Compensation
> Result, which is needed to support high transfer rates like 8 Mbps.
> The Secondary Sample Point is either the measured delay plus the
> configured offset, or just the configured offset.
> 
> Fix the existing RCANFD_FDCFG_TDCO() macro for the intended use case
> (writing instead of reading the field).  Add register definition bits
> for the Channel n CAN-FD Status Register.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/net/can/rcar/rcar_canfd.c | 80 +++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 7b6bb67ed6f76f14..a9c5b4ac040bdc0a 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -191,9 +191,19 @@
>  /* RSCFDnCFDCmFDCFG */
>  #define RCANFD_GEN4_FDCFG_CLOE		BIT(30)
>  #define RCANFD_GEN4_FDCFG_FDOE		BIT(28)
> +#define RCANFD_FDCFG_TDCO		GENMASK(23, 16)
>  #define RCANFD_FDCFG_TDCE		BIT(9)
>  #define RCANFD_FDCFG_TDCOC		BIT(8)
> -#define RCANFD_FDCFG_TDCO(x)		(((x) & 0x7f) >> 16)
> +
> +/* RSCFDnCFDCmFDSTS */
> +#define RCANFD_FDSTS_SOC		GENMASK(31, 24)
> +#define RCANFD_FDSTS_EOC		GENMASK(23, 16)
> +#define RCANFD_GEN4_FDSTS_PNSTS		GENMASK(13, 12)
> +#define RCANFD_FDSTS_SOCO		BIT(9)
> +#define RCANFD_FDSTS_EOCO		BIT(8)
> +#define RCANFD_FDSTS_TDCR(gpriv, x)	((x) & ((gpriv)->info->tdc_const->tdcv_max - 1))
> +#define RCANFD_FDSTS_TDCVF(gpriv) \
> +	((gpriv)->info->tdc_const->tdcv_max > 128 ? BIT(15) : BIT(7))

See my previous comment: no more function like macro please.

>  /* RSCFDnCFDRFCCx */
>  #define RCANFD_RFCC_RFIM		BIT(12)
> @@ -527,6 +537,7 @@ struct rcar_canfd_shift_data {
>  struct rcar_canfd_hw_info {
>  	const struct can_bittiming_const *nom_bittiming;
>  	const struct can_bittiming_const *data_bittiming;
> +	const struct can_tdc_const *tdc_const;
>  	const struct rcar_canfd_regs *regs;
>  	const struct rcar_canfd_shift_data *sh;
>  	u8 rnc_field_width;
> @@ -634,6 +645,25 @@ static const struct can_bittiming_const rcar_canfd_bittiming_const = {
>  	.brp_inc = 1,
>  };
>  
> +/* CAN FD Transmission Delay Compensation constants */
> +static const struct can_tdc_const rcar_canfd_gen3_tdc_const = {
> +	.tdcv_min = 1,

Interesting. This is the first time I see a driver with the tdcv_min and the
tdco_min different than 0. At one point in time, I wanted those to be implicit
values. Guess it was finally a good idea to include those minimums to the framework.

> +	.tdcv_max = 128,
> +	.tdco_min = 1,
> +	.tdco_max = 128,
> +	.tdcf_min = 0,	/* Filter window not supported */
> +	.tdcf_max = 0,
> +};
> +
> +static const struct can_tdc_const rcar_canfd_gen4_tdc_const = {
> +	.tdcv_min = 1,
> +	.tdcv_max = 256,
> +	.tdco_min = 1,
> +	.tdco_max = 256,
> +	.tdcf_min = 0,	/* Filter window not supported */
> +	.tdcf_max = 0,
> +};
> +
>  static const struct rcar_canfd_regs rcar_gen3_regs = {
>  	.rfcc = 0x00b8,
>  	.cfcc = 0x0118,
> @@ -679,6 +709,7 @@ static const struct rcar_canfd_shift_data rcar_gen4_shift_data = {
>  static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
>  	.nom_bittiming = &rcar_canfd_gen3_nom_bittiming_const,
>  	.data_bittiming = &rcar_canfd_gen3_data_bittiming_const,
> +	.tdc_const = &rcar_canfd_gen3_tdc_const,
>  	.regs = &rcar_gen3_regs,
>  	.sh = &rcar_gen3_shift_data,
>  	.rnc_field_width = 8,
> @@ -695,6 +726,7 @@ static const struct rcar_canfd_hw_info rcar_gen3_hw_info = {
>  static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
>  	.nom_bittiming = &rcar_canfd_gen4_nom_bittiming_const,
>  	.data_bittiming = &rcar_canfd_gen4_data_bittiming_const,
> +	.tdc_const = &rcar_canfd_gen4_tdc_const,
>  	.regs = &rcar_gen4_regs,
>  	.sh = &rcar_gen4_shift_data,
>  	.rnc_field_width = 16,
> @@ -711,6 +743,7 @@ static const struct rcar_canfd_hw_info rcar_gen4_hw_info = {
>  static const struct rcar_canfd_hw_info rzg2l_hw_info = {
>  	.nom_bittiming = &rcar_canfd_gen3_nom_bittiming_const,
>  	.data_bittiming = &rcar_canfd_gen3_data_bittiming_const,
> +	.tdc_const = &rcar_canfd_gen3_tdc_const,
>  	.regs = &rcar_gen3_regs,
>  	.sh = &rcar_gen3_shift_data,
>  	.rnc_field_width = 8,
> @@ -727,6 +760,7 @@ static const struct rcar_canfd_hw_info rzg2l_hw_info = {
>  static const struct rcar_canfd_hw_info r9a09g047_hw_info = {
>  	.nom_bittiming = &rcar_canfd_gen4_nom_bittiming_const,
>  	.data_bittiming = &rcar_canfd_gen4_data_bittiming_const,
> +	.tdc_const = &rcar_canfd_gen4_tdc_const,
>  	.regs = &rcar_gen4_regs,
>  	.sh = &rcar_gen4_shift_data,
>  	.rnc_field_width = 16,
> @@ -1436,12 +1470,15 @@ static irqreturn_t rcar_canfd_channel_interrupt(int irq, void *dev_id)
>  
>  static void rcar_canfd_set_bittiming(struct net_device *ndev)
>  {
> +	u32 mask = RCANFD_FDCFG_TDCO | RCANFD_FDCFG_TDCE | RCANFD_FDCFG_TDCOC;
>  	struct rcar_canfd_channel *priv = netdev_priv(ndev);
>  	struct rcar_canfd_global *gpriv = priv->gpriv;
>  	const struct can_bittiming *bt = &priv->can.bittiming;
>  	const struct can_bittiming *dbt = &priv->can.fd.data_bittiming;
> +	const struct can_tdc_const *tdc_const = priv->can.fd.tdc_const;
> +	const struct can_tdc *tdc = &priv->can.fd.tdc;
> +	u32 cfg, tdcmode = 0, tdco = 0;
>  	u16 brp, sjw, tseg1, tseg2;
> -	u32 cfg;
>  	u32 ch = priv->channel;
>  
>  	/* Nominal bit timing settings */
> @@ -1477,6 +1514,22 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
>  	rcar_canfd_write(priv->base, RCANFD_F_DCFG(gpriv, ch), cfg);
>  	netdev_dbg(ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
>  		   brp, sjw, tseg1, tseg2);
> +
> +	/* Transceiver Delay Compensation */
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_TDC_AUTO) {
> +		/* TDC enabled, measured + offset */
> +		tdcmode = RCANFD_FDCFG_TDCE;
> +		tdco = tdc->tdco - 1;
> +		netdev_dbg(ndev, "tdc: auto %u\n", tdco);

Same as previously. Are those debugs really useful? You can get the value
through the netlink interface (OK, you still have to do the minus one by hand,
but don't tell me that's the reason).

> +	} else if (priv->can.ctrlmode & CAN_CTRLMODE_TDC_MANUAL) {
> +		/* TDC enabled, offset only */
> +		tdcmode = RCANFD_FDCFG_TDCE | RCANFD_FDCFG_TDCOC;
> +		tdco = min(tdc->tdcv + tdc->tdco, tdc_const->tdco_max) - 1;

That's an edge case I did not think of and that is thus not handled by the
framework. This min() is a bit hacky, but I do not see a better workaround.
Also, I guess that this edge case will rarely occur.

> +		netdev_dbg(ndev, "tdc: manual %u\n", tdco);
> +	}
> +
> +	rcar_canfd_update_bit(gpriv->base, RCANFD_F_CFDCFG(gpriv, ch), mask,
> +			      tdcmode | FIELD_PREP(RCANFD_FDCFG_TDCO, tdco));
>  }
>  
>  static int rcar_canfd_start(struct net_device *ndev)
> @@ -1787,6 +1840,22 @@ static int rcar_canfd_rx_poll(struct napi_struct *napi, int quota)
>  	return num_pkts;
>  }
>  
> +static int rcar_canfd_get_auto_tdcv(const struct net_device *ndev, u32 *tdcv)
> +{
> +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
> +	struct rcar_canfd_global *gpriv = priv->gpriv;
> +	u32 sts = rcar_canfd_read(priv->base, RCANFD_F_CFDSTS(gpriv, priv->channel));
> +	u32 tdco = priv->can.fd.tdc.tdco;
> +	u32 tdcr;
> +
> +	/* Transceiver Delay Compensation Result */
> +	tdcr = RCANFD_FDSTS_TDCR(gpriv, sts) + 1;
> +
> +	*tdcv = tdcr < tdco ? 0 : tdcr - tdco;
> +
> +	return 0;
> +}
> +
>  static int rcar_canfd_do_set_mode(struct net_device *ndev, enum can_mode mode)
>  {
>  	int err;
> @@ -1909,12 +1978,17 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
>  	if (gpriv->fdmode) {
>  		priv->can.bittiming_const = gpriv->info->nom_bittiming;
>  		priv->can.fd.data_bittiming_const = gpriv->info->data_bittiming;
> +		priv->can.fd.tdc_const = gpriv->info->tdc_const;
>  
>  		/* Controller starts in CAN FD only mode */
>  		err = can_set_static_ctrlmode(ndev, CAN_CTRLMODE_FD);
>  		if (err)
>  			goto fail;
> -		priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
> +
> +		priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING |
> +					       CAN_CTRLMODE_TDC_AUTO |
> +					       CAN_CTRLMODE_TDC_MANUAL;
> +		priv->can.fd.do_get_auto_tdcv = rcar_canfd_get_auto_tdcv;
>  	} else {
>  		/* Controller starts in Classical CAN only mode */
>  		priv->can.bittiming_const = &rcar_canfd_bittiming_const;

Yours sincerely,
Vincent Mailhol

Re: [PATCH 9/9] can: rcar_canfd: Add support for Transceiver Delay Compensation

[Geert Uytterhoeven]

Hi Vincent,

On Mon, 2 Jun 2025 at 15:41, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> On 02/06/2025 at 20:54, Geert Uytterhoeven wrote:
> > The Renesas CAN-FD hardware block supports configuring Transceiver Delay
> > Compensation, and reading back the Transceiver Delay Compensation
> > Result, which is needed to support high transfer rates like 8 Mbps.
> > The Secondary Sample Point is either the measured delay plus the
> > configured offset, or just the configured offset.
> >
> > Fix the existing RCANFD_FDCFG_TDCO() macro for the intended use case
> > (writing instead of reading the field).  Add register definition bits
> > for the Channel n CAN-FD Status Register.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -191,9 +191,19 @@
> >  /* RSCFDnCFDCmFDCFG */
> >  #define RCANFD_GEN4_FDCFG_CLOE               BIT(30)
> >  #define RCANFD_GEN4_FDCFG_FDOE               BIT(28)
> > +#define RCANFD_FDCFG_TDCO            GENMASK(23, 16)
> >  #define RCANFD_FDCFG_TDCE            BIT(9)
> >  #define RCANFD_FDCFG_TDCOC           BIT(8)
> > -#define RCANFD_FDCFG_TDCO(x)         (((x) & 0x7f) >> 16)
> > +
> > +/* RSCFDnCFDCmFDSTS */
> > +#define RCANFD_FDSTS_SOC             GENMASK(31, 24)
> > +#define RCANFD_FDSTS_EOC             GENMASK(23, 16)
> > +#define RCANFD_GEN4_FDSTS_PNSTS              GENMASK(13, 12)
> > +#define RCANFD_FDSTS_SOCO            BIT(9)
> > +#define RCANFD_FDSTS_EOCO            BIT(8)
> > +#define RCANFD_FDSTS_TDCR(gpriv, x)  ((x) & ((gpriv)->info->tdc_const->tdcv_max - 1))
> > +#define RCANFD_FDSTS_TDCVF(gpriv) \
> > +     ((gpriv)->info->tdc_const->tdcv_max > 128 ? BIT(15) : BIT(7))
>
> See my previous comment: no more function like macro please.

OK, "int rcar_canfd_get_fdsts_tdcr(gpriv, sts)".

RCANFD_FDSTS_TDCVF() is unused, so I'll drop it.

> > @@ -634,6 +645,25 @@ static const struct can_bittiming_const rcar_canfd_bittiming_const = {
> >       .brp_inc = 1,
> >  };
> >
> > +/* CAN FD Transmission Delay Compensation constants */
> > +static const struct can_tdc_const rcar_canfd_gen3_tdc_const = {
> > +     .tdcv_min = 1,
>
> Interesting. This is the first time I see a driver with the tdcv_min and the
> tdco_min different than 0. At one point in time, I wanted those to be implicit
> values. Guess it was finally a good idea to include those minimums to the framework.

Really? As most other timing values need subtracting 1 when programming
the hardware, I would expect it to be rather common.

> > @@ -1477,6 +1514,22 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
> >       rcar_canfd_write(priv->base, RCANFD_F_DCFG(gpriv, ch), cfg);
> >       netdev_dbg(ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
> >                  brp, sjw, tseg1, tseg2);
> > +
> > +     /* Transceiver Delay Compensation */
> > +     if (priv->can.ctrlmode & CAN_CTRLMODE_TDC_AUTO) {
> > +             /* TDC enabled, measured + offset */
> > +             tdcmode = RCANFD_FDCFG_TDCE;
> > +             tdco = tdc->tdco - 1;
> > +             netdev_dbg(ndev, "tdc: auto %u\n", tdco);
>
> Same as previously. Are those debugs really useful? You can get the value
> through the netlink interface (OK, you still have to do the minus one by hand,
> but don't tell me that's the reason).

No, I just mimicked the existing debug prints, which we already agreed
upon to remove.

> > +     } else if (priv->can.ctrlmode & CAN_CTRLMODE_TDC_MANUAL) {
> > +             /* TDC enabled, offset only */
> > +             tdcmode = RCANFD_FDCFG_TDCE | RCANFD_FDCFG_TDCOC;
> > +             tdco = min(tdc->tdcv + tdc->tdco, tdc_const->tdco_max) - 1;
>
> That's an edge case I did not think of and that is thus not handled by the
> framework. This min() is a bit hacky, but I do not see a better workaround.
> Also, I guess that this edge case will rarely occur.

can_calc_tdco() also does a silent min(..., tdc_const->tdco_max).

>
> > +             netdev_dbg(ndev, "tdc: manual %u\n", tdco);
> > +     }
> > +
> > +     rcar_canfd_update_bit(gpriv->base, RCANFD_F_CFDCFG(gpriv, ch), mask,
> > +                           tdcmode | FIELD_PREP(RCANFD_FDCFG_TDCO, tdco));
> >  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 9/9] can: rcar_canfd: Add support for Transceiver Delay Compensation

[Vincent Mailhol]

On 02/06/2025 at 23:08, Geert Uytterhoeven wrote:
> Hi Vincent,
> 
> On Mon, 2 Jun 2025 at 15:41, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
>> On 02/06/2025 at 20:54, Geert Uytterhoeven wrote:
>>> The Renesas CAN-FD hardware block supports configuring Transceiver Delay
>>> Compensation, and reading back the Transceiver Delay Compensation
>>> Result, which is needed to support high transfer rates like 8 Mbps.
>>> The Secondary Sample Point is either the measured delay plus the
>>> configured offset, or just the configured offset.
>>>
>>> Fix the existing RCANFD_FDCFG_TDCO() macro for the intended use case
>>> (writing instead of reading the field).  Add register definition bits
>>> for the Channel n CAN-FD Status Register.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
>>> --- a/drivers/net/can/rcar/rcar_canfd.c
>>> +++ b/drivers/net/can/rcar/rcar_canfd.c
>>> @@ -191,9 +191,19 @@
>>>  /* RSCFDnCFDCmFDCFG */
>>>  #define RCANFD_GEN4_FDCFG_CLOE               BIT(30)
>>>  #define RCANFD_GEN4_FDCFG_FDOE               BIT(28)
>>> +#define RCANFD_FDCFG_TDCO            GENMASK(23, 16)
>>>  #define RCANFD_FDCFG_TDCE            BIT(9)
>>>  #define RCANFD_FDCFG_TDCOC           BIT(8)
>>> -#define RCANFD_FDCFG_TDCO(x)         (((x) & 0x7f) >> 16)
>>> +
>>> +/* RSCFDnCFDCmFDSTS */
>>> +#define RCANFD_FDSTS_SOC             GENMASK(31, 24)
>>> +#define RCANFD_FDSTS_EOC             GENMASK(23, 16)
>>> +#define RCANFD_GEN4_FDSTS_PNSTS              GENMASK(13, 12)
>>> +#define RCANFD_FDSTS_SOCO            BIT(9)
>>> +#define RCANFD_FDSTS_EOCO            BIT(8)
>>> +#define RCANFD_FDSTS_TDCR(gpriv, x)  ((x) & ((gpriv)->info->tdc_const->tdcv_max - 1))
>>> +#define RCANFD_FDSTS_TDCVF(gpriv) \
>>> +     ((gpriv)->info->tdc_const->tdcv_max > 128 ? BIT(15) : BIT(7))
>>
>> See my previous comment: no more function like macro please.
> 
> OK, "int rcar_canfd_get_fdsts_tdcr(gpriv, sts)".
> 
> RCANFD_FDSTS_TDCVF() is unused, so I'll drop it.
> 
>>> @@ -634,6 +645,25 @@ static const struct can_bittiming_const rcar_canfd_bittiming_const = {
>>>       .brp_inc = 1,
>>>  };
>>>
>>> +/* CAN FD Transmission Delay Compensation constants */
>>> +static const struct can_tdc_const rcar_canfd_gen3_tdc_const = {
>>> +     .tdcv_min = 1,
>>
>> Interesting. This is the first time I see a driver with the tdcv_min and the
>> tdco_min different than 0. At one point in time, I wanted those to be implicit
>> values. Guess it was finally a good idea to include those minimums to the framework.
> 
> Really? As most other timing values need subtracting 1 when programming
> the hardware, I would expect it to be rather common.

Do a

  git grep "tdc[ov]_min ="

and see by yourself :)

I also expected that this might occur one day, so glad I anticipated.

>>> @@ -1477,6 +1514,22 @@ static void rcar_canfd_set_bittiming(struct net_device *ndev)
>>>       rcar_canfd_write(priv->base, RCANFD_F_DCFG(gpriv, ch), cfg);
>>>       netdev_dbg(ndev, "drate: brp %u, sjw %u, tseg1 %u, tseg2 %u\n",
>>>                  brp, sjw, tseg1, tseg2);
>>> +
>>> +     /* Transceiver Delay Compensation */
>>> +     if (priv->can.ctrlmode & CAN_CTRLMODE_TDC_AUTO) {
>>> +             /* TDC enabled, measured + offset */
>>> +             tdcmode = RCANFD_FDCFG_TDCE;
>>> +             tdco = tdc->tdco - 1;
>>> +             netdev_dbg(ndev, "tdc: auto %u\n", tdco);
>>
>> Same as previously. Are those debugs really useful? You can get the value
>> through the netlink interface (OK, you still have to do the minus one by hand,
>> but don't tell me that's the reason).
> 
> No, I just mimicked the existing debug prints, which we already agreed
> upon to remove.
> 
>>> +     } else if (priv->can.ctrlmode & CAN_CTRLMODE_TDC_MANUAL) {
>>> +             /* TDC enabled, offset only */
>>> +             tdcmode = RCANFD_FDCFG_TDCE | RCANFD_FDCFG_TDCOC;
>>> +             tdco = min(tdc->tdcv + tdc->tdco, tdc_const->tdco_max) - 1;
>>
>> That's an edge case I did not think of and that is thus not handled by the
>> framework. This min() is a bit hacky, but I do not see a better workaround.
>> Also, I guess that this edge case will rarely occur.
> 
> can_calc_tdco() also does a silent min(..., tdc_const->tdco_max).

Good catch!

>>> +             netdev_dbg(ndev, "tdc: manual %u\n", tdco);
>>> +     }
>>> +
>>> +     rcar_canfd_update_bit(gpriv->base, RCANFD_F_CFDCFG(gpriv, ch), mask,
>>> +                           tdcmode | FIELD_PREP(RCANFD_FDCFG_TDCO, tdco));
>>>  }

Yours sincerely,
Vincent Mailhol

Re: [PATCH 0/9] can: rcar_canfd: Add support for Transceiver Delay Compensation

[Vincent Mailhol]

On 02/06/2025 at 20:54, Geert Uytterhoeven wrote:
> 	Hi all,
> 
> This patch series adds CAN-FD Transceiver Delay Compensation support to
> the R-Car CAN-FD driver, after the customary cleanups and refactorings.
> 
> This has been tested on R-Car V4H (White Hawk), V4M (Gray Hawk Single),
> and E3 (Ebisu-4D[1]), using various data bit rates.  Without proper TDC
> configuration, transmitting at 8 Mbps makes the CAN-FD controller enter
> BUS-OFF state.  The TDCV value as measured by the CAN-FD controller is 4
> on all boards tested (base clock 40 MHz, i.e. 25 ns period), and ca. 90
> ns as measured by a logic analyzer on Gray Hawk Single.
> 
> Note that the BSP (predating upstream TDC support), uses a much simpler
> method: for transfer rates >= 5 Mbps on R-Car Gen4, it enables TDC with
> a hardcoded (hardware) TDCO value of 2 (i.e. actual 3), which matches
> the behavior of this series at 8 Mbps.
> 
> Thanks for your comments!

Aside from my request to change the function like macros into actual functions,
the series looks good.

Under the condition that you rework the macros into functions:

Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>


Yours sincerely,
Vincent Mailhol