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

[Marc Kleine-Budde]

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@xxxxxxxxxxx>
> > > 
> > > 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: signature.asc
Description: PGP signature