Re: [PATCH net-next 15/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 a third fCAN source (fck/pclk, used when
  `info->fcan_pclk` is set), but the clock comments and the boot banner…

--

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

This commit adds an fcan_pclk hardware info flag. With it set, the fCAN
clock falls back to the peripheral clock (fck) when the external can_clk
is unavailable, the rate is divided by info->postdiv and can_clk is set
to NULL. All existing rcar_canfd_hw_info instances now set fcan_pclk = 0
explicitly.

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 b7c4fe3477ac..f9173774387f 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;
> +		} else {

[Severity: Low]
Should the clock comments and the probe banner be updated for this new
fck source? Several places still describe only the external and canfd
clocks.

The comment just above this block in rcar_canfd_probe() still says:

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

With fcan_pclk set, the fallback is fck (gpriv->clkp), not the canfd
clock.

In struct rcar_canfd_global, the field comments also don't cover this
case. Here can_clk is NULL and the fCAN source is clkp:

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

The banner at the end of rcar_canfd_probe() wasn't changed either:

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

On this path extclk stays false. Won't the banner then say "canfd clk"
for a controller that has no canfd clock and runs from fck / postdiv?

The later "can: rcar_canfd: Add support for Renesas RZ/G3S" patch sets
.fcan_pclk = 1 in r9a08g045_hw_info. After that, an RZ/G3S board whose
can_clk is missing or has a rate of 0 would print this label.

Also, the new bitfield comment calls this clock pclk:

	unsigned fcan_pclk:1;		/* Has fcan sourced from pclk. */

The commit message and the code call it fck. Could the comment use the
same name?

[ ... ]

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