Re: [PATCH] can: flexcan: enable PER clock before obtaining its rate

Re: [PATCH] can: flexcan: enable PER clock before obtaining its rate

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2720 bytes --]

On 14.04.2025 10:36:46, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> The FlexCan driver assumes that the frequency of the 'per' clock can be
> obtained even on disabled clocks, which is not always true.
> 
> According to 'clk_get_rate' documentation, it is only valid once the clock
> source has been enabled.

In commit bde8870cd8c3 ("clk: Clarify clk_get_rate() expectations")
Maxime Ripard changed the documentation of the of the function in clk.c
to say it's allowed. However clk.h states "This is only valid once the
clock source has been enabled.".

I've added the common clock maintainers to Cc.

Which documentation is correct? Is the clk.h correct for archs not using
the common clock framework?

regards,
Marc

> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> ---
>  drivers/net/can/flexcan/flexcan-core.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index 6d80c341b26f..b142aa60620e 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -2056,6 +2056,26 @@ static int flexcan_setup_stop_mode(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static unsigned long get_per_clk_rate(struct clk *clk)
> +{
> +	unsigned long rate;
> +	int err;
> +
> +	rate = clk_get_rate(clk);
> +	if (rate)
> +		return rate;
> +
> +	/* Just in case this clock is disabled by default */
> +	err = clk_prepare_enable(clk);
> +	if (err)
> +		return 0;
> +
> +	rate = clk_get_rate(clk);
> +	clk_disable_unprepare(clk);
> +
> +	return rate;
> +}
> +
>  static const struct of_device_id flexcan_of_match[] = {
>  	{ .compatible = "fsl,imx8qm-flexcan", .data = &fsl_imx8qm_devtype_data, },
>  	{ .compatible = "fsl,imx8mp-flexcan", .data = &fsl_imx8mp_devtype_data, },
> @@ -2137,7 +2157,7 @@ static int flexcan_probe(struct platform_device *pdev)
>  			dev_err(&pdev->dev, "no per clock defined\n");
>  			return PTR_ERR(clk_per);
>  		}
> -		clock_freq = clk_get_rate(clk_per);
> +		clock_freq = get_per_clk_rate(clk_per);
>  	}
>  
>  	irq = platform_get_irq(pdev, 0);
> -- 
> 2.45.2
> 
> 

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: flexcan: enable PER clock before obtaining its rate

[Stephen Boyd]

Quoting Marc Kleine-Budde (2025-04-14 02:55:34)
> On 14.04.2025 10:36:46, Ciprian Costea wrote:
> > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > 
> > The FlexCan driver assumes that the frequency of the 'per' clock can be
> > obtained even on disabled clocks, which is not always true.
> > 
> > According to 'clk_get_rate' documentation, it is only valid once the clock
> > source has been enabled.
> 
> In commit bde8870cd8c3 ("clk: Clarify clk_get_rate() expectations")
> Maxime Ripard changed the documentation of the of the function in clk.c
> to say it's allowed. However clk.h states "This is only valid once the
> clock source has been enabled.".
> 
> I've added the common clock maintainers to Cc.
> 
> Which documentation is correct? Is the clk.h correct for archs not using
> the common clock framework?
> 

I don't know what arches not using the common clk framework (CCF) do so
I can't comment there. If you want something to work on an architecture
that doesn't use the CCF then follow the header file, but in all
practical cases _some_ rate will be returned from clk_get_rate() and
we're not going to BUG_ON() or crash the system in the CCF
implementation for this case. Enabling the clk is good hygiene though,
so is it really a problem to enable it here?

Re: [PATCH] can: flexcan: enable PER clock before obtaining its rate

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3581 bytes --]

Hello Stephen,

thanks for your input.

On 14.04.2025 17:27:10, Stephen Boyd wrote:
> Quoting Marc Kleine-Budde (2025-04-14 02:55:34)
> > On 14.04.2025 10:36:46, Ciprian Costea wrote:
> > > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > > 
> > > The FlexCan driver assumes that the frequency of the 'per' clock can be
> > > obtained even on disabled clocks, which is not always true.
> > > 
> > > According to 'clk_get_rate' documentation, it is only valid once the clock
> > > source has been enabled.
> > 
> > In commit bde8870cd8c3 ("clk: Clarify clk_get_rate() expectations")
> > Maxime Ripard changed the documentation of the of the function in clk.c
> > to say it's allowed. However clk.h states "This is only valid once the
> > clock source has been enabled.".
> > 
> > I've added the common clock maintainers to Cc.
> > 
> > Which documentation is correct? Is the clk.h correct for archs not using
> > the common clock framework?
> > 
> 
> I don't know what arches not using the common clk framework (CCF) do so
> I can't comment there.

The driver is used on Freescale/NXP SoCs: m68k (coldfire), arm, arm64.
Coldfire provides us directly with the fixed clock rate, so no clk_*()
functions are used there.

> If you want something to work on an architecture
> that doesn't use the CCF then follow the header file, but in all
> practical cases _some_ rate will be returned from clk_get_rate() and
> we're not going to BUG_ON() or crash the system in the CCF
> implementation for this case. Enabling the clk is good hygiene though,
> so is it really a problem to enable it here?

It's not a problem to enable the clock. If it would stay enabled, it
means a higher power consumption (I cannot quantify how much), as the
clock is only needed if the CAN interface is up. But we have more things
to cleanup:

For CAN controllers, information about the clock is more important than
for e.g. serial interfaces, so it's exported to user space. During probe
a CAN driver typically queries the clock rate with clk_get_rate()
(without enabling the clock) and stores the rate in a structure.

If the user space configures the bit rate of the CAN bus, the stored
clock rate is used to calculate the bit timing parameters. During ifup
the clocks are enabled and the previously calculated bit timing
parameters are programmed into the hardware.

In the early days of the stm32mp1 this has previously caused problems
because the frequency of the CAN clock changed between the initial
clk_get_rate() and the ifup.

For SoCs with CAN interfaces, the system designer usually configures the
CAN clock to a specific rate, so this problem does currently not occur.

What are our options?
- enable the clock before clk_get_rate(), disable it afterwards?
  -> This might be a bit more "hygienic", but doesn't solve the clock
     rate changes problem.

- clk_notifier_register() and update the saved rate
  -or-
- give our framework a pointer to the clock, so that it can do the
  query every time needed, instead of using the value from probe time

- delay the calculation of the bit timing parameter until ifup, do a
  clk_rate_exclusive_get(), re-calculate the bit timing parameters and
  program them into the HW

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]