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

[Marc Kleine-Budde]

On 16.10.2026 21:07:07, Claudiu Beznea wrote:
>
>
> 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.

It's not about "clkp".

What's missing is, that "gpriv->can_clk" is checked against IS_ERR():

> > >   	gpriv->can_clk = devm_clk_get(dev, "can_clk");
> > >   	if (IS_ERR(gpriv->can_clk) || (clk_get_rate(gpriv->can_clk) == 0)) {

but if it's a -EPROBE_DEFER, IS_ERR() returns true. I think in this case
you want to return with dev_err_probe().

There's also a difference between "can_clk" not found (e.g. not
configured in DT), and there was a problem with "can_clk". I don't know
if this is important for this driver, but if you want to distinguish
between those, you can use devm_clk_get_optional(). I will return NULL
if the clock is not found, -EPROBE_DEFER if not available yet, the clock
or, in case of an error -errno.

Thinking about it: if "can_clk" is not available, I would consider it
intentional, so the driver should use the other clock. If the clock has
an error, the driver should not fallback to a probably untested clock
configuration.

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

netdev's sashiko's classified this as low. So the patch should go in as
is, if it comes to the same conclusion I in my next PR. Please send a
follow up series.

> > --- 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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxx
> > Link: https://patch.msgid.link/20260902141544.565763-4-claudiu.beznea+renesas@xxxxxxxxx
> >
> > > 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.

Use the clock names from the binding. (Even in the linked clock tree the
clock have more than one name :))

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

IMHO: Use the clock names from the binding. The binding name is ABI, the
driver's internal name can change.

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

Fine :) The binding cannot be changed, it ABI.

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

Follow up patches is fine.

regards,
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: signature.asc
Description: PGP signature