Re: [PATCH v3 36/65] clk: versatile: sp810: Add a determine_rate hook

Pawel Moll
2023-04-06T15:21:41Z

On 04/04/2023 11:11, Maxime Ripard wrote:
> The Versatile sp810 "timerclken" clock implements a mux with a
> set_parent hook, but doesn't provide a determine_rate implementation.
>
> This is a bit odd, since set_parent() is there to, as its name implies,
> change the parent of a clock.

Explanation of this mystery is pretty simple - the original patch:

commit 6e973d2c438502dcf956e76305258ba7d1c7d1d3
Author: Pawel Moll <pawel.moll@arm.com>
Date: Thu Apr 18 18:23:22 2013 +0100

clk: vexpress: Add separate SP810 driver

predates introduction of determine_rate to clk_ops...

commit 71472c0c06cf9a3d1540762ea205654c584e3bc4
Author: James Hogan <jhogan@kernel.org>
Date: Mon Jul 29 12:25:00 2013 +0100

clk: add support for clock reparent on set_rate

and clearly no one (the author included ;-) bothered to have another
look at this side of the driver.

> And if it was an oversight, then we are at least explicit about our
> behavior now and it can be further refined down the line.

It's been one hell of a memory lane trip, but my recollection suggest
that the main goal of the driver was simply initialisation of the mux
to select the 1MHz parent, because the other option - 32kHz - just
didn't make any sense whatsoever. And that would be the case on every
single platform using SP810 I know (or at least: knew), so it's seems
to me that making the state permanent, as you're suggesting (or I
think you're suggesting?) it's the right thing to do.

Thanks!

Paweł