Re: [PATCH v4 3/7] can: rcar_canfd: Allow the CAN FD clock to be sourced from fck

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

On 10/16/26 16:39, Marc Kleine-Budde wrote:
> On 02.09.2026 17:15:40, Claudiu Beznea wrote:
>> 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
> 
> [...]
> 
>>
>>   /* Helper functions */
>> @@ -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)) {
>              ^^^^^^^^^^^^^^^^^^^^^
> 
> As sashiko.dev [1] pointed out here's no check for -EPROBE_DEFER.

This looks to me a pre-existing issue.

The newly added support does:

fcan_freq = clk_get_rate(gpriv->clkp);

At the time this is executed the clkp was already checked against any errors 
(above) including -EPROBE_DEFER.

Please let me know what is the procedure now. Should I re-send the series and 
address the fixes? Should I send follow up patches?

> 
> [1] https://lore.kernel.org/all/sashiko-outbox-173569@kernel.org/
> 
> The netdev bot found more issues.
> pw-bot: cr
> 
> regards,
> Marc
> 
> --- Patch [15]: [PATCH net-next 15/37] can: rcar_canfd: Allow the CAN FD clock to be sourced from fck ---
> commit 4ef0042ccf34457229cdde398a008bebaab54e69
> Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> 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 {
> 
> Discovered by: opus-5-5 · Missed by: gpt-6-astra, sashiko-gemini · Confirmed by: gpt-6-astra
> 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.

We can update the comment as follows to comply with the clock name in the driver:

/* fCAN clock: Pick External clock. If not available fallback to
  * CANFD clock or peripheral clock in case fcan_pclk = 1
  */

or, to comply with the clock name in the bindings:

/* fCAN clock: Pick External clock. If not available fallback to
  * CANFD clock or fck in case fcan_pclk = 1
  */

Please let me know what is prefered.


> 
> 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 */

There is already a pointer covering the clkp:

         struct clk *clkp;		/* Peripheral 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?

That could be updated with "clkp" to comply with the driver name or "fck" to 
comply with the bindings or with "peripheral". Please let me know what is 
preferred.

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

The commit message mentions: ...the CAN FD clock can be sourced either from fck
(the peripheral clock) or from an external clock (can_clk).

The driver uses clkp (peripheral clock) to refer to the fck.

fck is the clock name used in the bindings. In bindings I kept the same clock 
name and provided the proper phandle in the device tree. I don't consider we 
should change the driver and the bindings for this thing. Please let me know 
otherwise.

The clock diagram is something like:

                         +----+
pclk (fck) -> 1/2 div ->|    |
                         | mux|---> fcan
ext  (can_clk)--------->|    |
                         ++++++

b/w () are the clock names in bindings.

The full HW block with the clocks involved can be found at 
https://github.com/claudiubeznea/logs/blob/master/canfd-clk.png

Please let me know what is the procedure for this. Should I re-spin the series? 
Should I send follow up patches?

Thank you,
Claudiu