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

[Maxime Ripard]

On Tue, Mar 12, 2024 at 04:52:29PM +0800, Yang Xiwen wrote:
> 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.

I agree it's a mess, but that's a mess you wanted to clean up in the
first place :)

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

Which, without a documentation, makes it more of a mess.

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

That's kunit, not kernelci (https://linux.kernelci.org/job/)

> 
> Over all, it seems quite a lot of work here.
>
> 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.

I mean, eventually, that's on them. If U-Boot chose to have a
somewhat-similar-but-not-really clock framework, there's nothing Linux
can solve here, even though I definitely can see the frustration for the
developpers that have to work on both.

Maxime

Attachment: signature.asc
Description: PGP signature