Re: [PATCH v3] clk: set initial best mux parent to current parent with CLK_MUX_ROUND_CLOSEST

[Yang Xiwen]

On 3/11/2024 5:34 PM, Maxime Ripard wrote:
> On Thu, Mar 07, 2024 at 07:18:05PM +0800, Yang Xiwen wrote:
> > On 3/7/2024 4:48 PM, Maxime Ripard wrote:
> > > Hi,
> > >
> > > On Thu, Mar 07, 2024 at 10:03:50AM +0800, Yang Xiwen via B4 Relay wrote:
> > > > From: Yang Xiwen <forbidden405@xxxxxxxxxxx>
> > > >
> > > > Originally, the initial clock rate is hardcoded to 0, this can lead to
> > > > some problem when setting a very small rate with CLK_MUX_ROUND_CLOSEST.
> > > >
> > > > For example, if the lowest possible rate provided by the mux is 1000Hz,
> > > > setting a rate below 500Hz will fail, because no clock can provide a
> > > > better rate than the non-existant 0Hz. But it should succeed with 1000Hz
> > > > being set.
> > > >
> > > > Setting the initial best parent to current parent could solve this bug.
> > > >
> > > > Signed-off-by: Yang Xiwen <forbidden405@xxxxxxxxxxx>
> > > I don't think it would be the way to go. The biggest issue to me is that
> > > it's inconsistent, and only changing the behaviour for a given flag
> > > doesn't solve that.
> >
> > I think the current behavior is odd but conforms to the document if
> > CLK_MUX_ROUND_CLOSEST is not specified.
> clk_mux_determine_rate_flags isn't documented, and the determine_rate
> clk_ops documentation doesn't mention it can return an error.
>
> > If i understand correctly, the default behavior of mux clocks is to
> > select the closest rate lower than requested rate, and
> > CLK_MUX_ROUND_CLOSEST removes the "lower than" limitation, which is
> > what this version tries to accomplish.
> The situation is not as clear-cut as you make it to be, unfortunately.
> The determine_rate clk_ops implementation states:
>
>    Given a target rate as input, returns the closest rate actually
>    supported by the clock, and optionally the parent clock that should be
>    used to provide the clock rate.
>
> So CLK_MUX_ROUND_CLOSEST shouldn't exist, because that's what
> determine_rate expects so it should always be there.
>
> Now, the "actually supported by the clock" can be interpreted in
> multiple ways, and most importantly, doesn't state what the behaviour is
> if we can't find a rate actually supported by the clock.
>
> But now, this situation has been ambiguous for a while and thus drivers
> kind of relied on that ambiguity.
>
> So the way to fix it up is:
>
>    - Assess what drivers are relying on
>    - Document the current behaviour in clk_ops determine_rate


From my investigation, it's totally a mess, especially for platform clk 
drivers (PLL). Some drivers always round down, the others round to 
nearest, with or without a specific flag to switch between them, depend 
on the division functions they choose. Fixing all of them seems needs 
quite a lot of time and would probably introduce some regressions.

We'd probably only have to say both rounding to nearest and rounding 
down are acceptable, though either one is preferred.


>    - Make clk_mux_determine_rate_flags consistent with that


I think we must keep existing flags and document the current behavior 
correctly because of the massive existing users of clk_mux.


That's why i'm going to only fix CLK_MUX_ROUND_CLOSEST users. Hopefully 
it won't cause too many regressions.


>    - Run that through kernelci to make sure we don't have any regression


We don't. I run 'tools/testing/kunit/kunit.py run --kunitconfig 
drivers/clk/.kunitconfig' each time before i send patches.


Over all, it seems quite a lot of work here.


> Maxime


The situation here becomes even more complex when it comes to U-Boot clk 
framework. They chose slightly different prototypes and stated 
clk_set_rate() can fail with -ve. It's a great burden for clk driver 
authors and maintainers when they try to port their drivers to U-Boot. 
Let's Cc U-Boot clk maintainers as well, and see how we can resolve the 
mess here.

--
Regards,
Yang Xiwen