Re: [PATCH net-next 11/37] can: rcar_canfd: Allow the CAN FD clock to be sourced from fck

[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] The patch adds an fCAN source taken from fck (`info->fcan_pclk`)
  in rcar_canfd_probe(), but the comments and the probe log still…

--

can: rcar_canfd: Allow the CAN FD clock to be sourced from fck

This patch adds an fcan_pclk hardware flag to struct rcar_canfd_hw_info.
When the flag is set and can_clk is missing or runs at 0 Hz,
rcar_canfd_probe() takes fCAN from the peripheral clock (fck) and divides
it by postdiv, rather than looking up the "canfd" clock. Every existing
hw_info instance sets fcan_pclk to 0.

Link: https://lore.kernel.org/all/1d9719e3-10ff-4cd8-b729-55fea93c37ce@wanadoo.fr
Link: https://patch.msgid.link/20260902141544.565763-4-claudiu.beznea+renesas@tuxon.dev

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index b7c4fe3477ac5..f9173774387fa 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c

[ ... ]

> @@ -2190,13 +2196,20 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  	 */
>  	gpriv->can_clk = devm_clk_get(dev, "can_clk");
>  	if (IS_ERR(gpriv->can_clk) || (clk_get_rate(gpriv->can_clk) == 0)) {
> -		gpriv->can_clk = devm_clk_get(dev, "canfd");
> -		if (IS_ERR(gpriv->can_clk))
> -			return dev_err_probe(dev, PTR_ERR(gpriv->can_clk),
> -					     "cannot get canfd clock\n");
> +		if (info->fcan_pclk) {
> +			fcan_freq = clk_get_rate(gpriv->clkp);
> +			gpriv->can_clk = NULL;

[Severity: Low]
This isn't a bug, but should the comments and the probe log message be
updated for this new fck path?

The comment just above this hunk in rcar_canfd_probe() still describes
only two choices:

	/* fCAN clock: Pick External clock. If not available fallback to
	 * CANFD clock
	 */

When fcan_pclk is set, the fallback is fck (gpriv->clkp) and "canfd" is
never looked up.

On this path gpriv->extclk stays false. The dev_info() at the end of
rcar_canfd_probe() will therefore still print "canfd clk":

	dev_info(dev, "global operational state (%s clk, %s mode)\n",
		 gpriv->extclk ? "ext" : "canfd",
		 ...

But no canfd clock was acquired, and fCAN actually comes from fck.

The field comments in struct rcar_canfd_global are also out of date:

	struct clk *can_clk;		/* fCAN clock */
	...
	bool extclk;			/* CANFD or Ext clock */

On this path can_clk is now NULL, and there are three possible fCAN
sources, not two.

Every hw_info sets fcan_pclk = 0 in this commit. Later in the series,
r9a08g045_hw_info sets .fcan_pclk = 1 and is used for both RZ/G3S and
RZ/G3L. None of these comments or the log string are changed elsewhere
in the series. On those SoCs, the boot log would report the wrong fCAN
clock source.

> +		} else {
> +			gpriv->can_clk = devm_clk_get(dev, "canfd");
> +			if (IS_ERR(gpriv->can_clk))
> +				return dev_err_probe(dev, PTR_ERR(gpriv->can_clk),
> +						     "cannot get canfd clock\n");
> +
> +			fcan_freq = clk_get_rate(gpriv->can_clk);
> +		}

[ ... ]

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