Re: [PATCH net-next 05/12] can: rcar_canfd: Add support for Renesas RZ/G3S

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern: r9a08g045_hw_info sets .fixed_canfd = 1, but
  rcar_canfd_probe() still reads "renesas,no-can-fd" (fdmode = false,…

--

can: rcar_canfd: Add support for Renesas RZ/G3S

This patch adds an r9a08g045_hw_info entry for the RZ/G3S CAN-FD
controller and binds it to the "renesas,r9a08g045-canfd" compatible. The
variant is gen4-like, with two channels, 16 AFL pages, an fcan clock
sourced from pclk, and the operating mode fixed to CAN FD.

Link: https://patch.msgid.link/20260902141544.565763-6-claudiu.beznea+renesas@tuxon.dev

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index ac5d5959d8e4c..74c3c3f20991f 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -661,6 +661,26 @@ static const struct rcar_canfd_hw_info rzg2l_hw_info = {
>  	.fixed_canfd = 0,
>  };
>  
> +static const struct rcar_canfd_hw_info r9a08g045_hw_info = {
> +	.nom_bittiming = &rcar_canfd_gen4_nom_bittiming_const,
> +	.data_bittiming = &rcar_canfd_gen4_data_bittiming_const,

[ ... ]

> +	.ch_interface_mode = 1,
> +	.shared_can_regs = 1,
> +	.external_clk = 1,
> +	.fcan_pclk = 1,
> +	.fixed_canfd = 1,
> +};

[Severity: Low]
This isn't a bug as long as the DT follows the binding, but should
rcar_canfd_probe() ignore or reject "renesas,no-can-fd" and
"renesas,fd-only" when info->fixed_canfd is set?

This is the first hw_info with fixed_canfd = 1. Both properties are still
read without checking that flag:

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_probe() {
    ...
	if (of_property_read_bool(dev->of_node, "renesas,no-can-fd"))
		fdmode = false;			/* Classical CAN only mode */
    ...
	if (of_property_read_bool(dev->of_node, "renesas,fd-only"))
		gpriv->fd_only_mode = true; /* FD-Only mode for CAN-FD */
    ...
}

Meanwhile rcar_canfd_reset_controller() skips the CLOE/FDOE writes, so
the controller stays in CAN FD mode:

	/* Set the controller into appropriate mode */
	if (gpriv->info->fixed_canfd)
		continue;

With "renesas,no-can-fd", fdmode == false makes the rest of the driver
set up classical CAN while the controller is still in CAN FD mode:

  - 8 byte RFPLS/CFPLS in rcar_canfd_configure_rx() and
    rcar_canfd_configure_tx()
  - no RCANFD_GCFG_CMPOC in rcar_canfd_configure_controller()
  - no RCANFD_GCTR_CFMPOFIE in rcar_canfd_enable_global_interrupts()
  - only nominal bit timing in rcar_canfd_channel_probe()

FD frames on the bus would then be dropped silently instead of being
reported as errors.

With "renesas,fd-only", the probe message reports "fd-only" mode, but the
hardware cannot enter that mode.

The binding change for this compatible sets "renesas,no-can-fd: false"
and "renesas,fd-only: false". Only a DT that fails schema validation can
hit this, and the compatible is new, so no existing DTs carry these
properties.

As hardening, could probe force fdmode = true and clear fd_only_mode when
fixed_canfd is set, or fail probe instead?

> +
>  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,

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110115112.3807247-2-mkl%40pengutronix.de