Message-ID: <1jfrlwb69r.fsf@starbuckisacylon.baylibre.com>
Date: Mon, 06 Jan 2025 11:12:16 +0100
From: Jerome Brunet <jbrunet@...libre.com>
To: Stephen Boyd <sboyd@...nel.org>
Cc: Kevin Hilman <khilman@...libre.com>,  Martin Blumenstingl
 <martin.blumenstingl@...glemail.com>,  Michael Turquette
 <mturquette@...libre.com>,  Neil Armstrong <neil.armstrong@...aro.org>,
  linux-clk@...r.kernel.org,  linux-kernel@...r.kernel.org,
  linux-amlogic@...ts.infradead.org,  linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/3] clk: amlogic: drop clk_regmap tables

On Mon 30 Dec 2024 at 17:08, Stephen Boyd <sboyd@...nel.org> wrote:

> Quoting Jerome Brunet (2024-12-21 03:09:28)
>> On Fri 20 Dec 2024 at 16:12, Stephen Boyd <sboyd@...nel.org> wrote:
>> 
>> > Quoting Jerome Brunet (2024-12-20 09:17:43)
>> >> Remove the big clk_regmap tables that are used to populate the regmap
>> >> field of clk_regmap clocks at runtime. Instead of using tables, use devres
>> >> to allow the clocks to get the necessary regmap.
>> >> 
>> >> A simpler solution would have been to use dev_get_regmap() but this would
>> >> not work with syscon based controllers.
>> >
>> > Why not have two init functions, one that uses the syscon approach from
>> > the parent device?
>> 
>> That would duplicate all the ops and would not scale if anything else
>> comes along. It would also tie the controller quirks with
>> clock ops. I would like to keep to clock ops and controllers decoupled as
>> much as possible
>
> Hmm... Maybe the init function should be moved out of the clk_ops and
> into the clk_init_data structure. It isn't used beyond registration time
> anyway, so it may make sense to do that and decouple the clk_ops from
> the controllers completely. Or we can have two init routines, one for
> the software side and one for the hardware side, but that's probably
> confusing. If anything, a clk hardware init function can be exported and
> called from the clk software init function if needed.

The .init() is really for the clock itself, so it makes sense for it to
in the ops. Removing the init from the ops would just be another layer of
controller init, something we can deal with in the probe() function.

What I'm trying to do here is properly decouple what belongs in each.

>
>> 
>> > Is the typical path to not use a syscon anyway?
>> >
>> 
>> I sure hope there will be no new syscon based controller but, ATM, around
>> 50% are syscon based in drivers/clk/meson. Those are here to stay and I
>> doubt we can do anything about it.
>
> Ok.
>
>> 
>> >> 
>> >> This rework save a bit memory and the result is less of a maintenance
>> >> burden.
>> >> 
>> >> Unfortunately meson8b is left out for now since it is an early clock
>> >> driver that does not have proper device support for now.
>> >
>> > We should add a clk_hw_get_of_node() function that returns
>> > hw->core->of_node. Then there can be a similar function that looks at
>> > the of_node of a clk registered with of_clk_hw_register() and tries to
>> > find the regmap either with
>> > syscon_node_to_regmap(clk_hw_get_of_node(hw)) or on the parent of the
>> > node for the clk.
>> 
>> That's the thing. It means encoding the controller quirk of how to get
>> regmap in the clock ops. I would be prefer to avoid that.
>
> So if we moved the init function out of struct clk_ops it would work?

That's already what I'm doing actually. I have the controller part which
position regmap so the clock part may get it regardless of where it
comes from (syscon, io or something else)

> We could have helpers for the common paths, i.e. the device has the
> regmap, or the syscon has the regmap, etc.

I think this is what I'm doing actually.

>
>> 
>> With what you are suggesting I could make an ops that
>> * Try dev_get_regmap() first
>> * Try the syscon/of_node way next
>> 
>> I can make this "trial an error" approach work but I think it is pretty
>> nasty and encode controller stuff inside the clock driver.
>
> I get it. The difference in driver design while sharing the same clk
> hardware and clk_ops causes this tension.
>
>> 
>> >
>> > TL;DR: Don't use devres.
>> 
>> Using it makes thing nice and tidy. clk_regmap does not care were regmap
>> comes from. It just picks it up where it has been prepared
>
> It doesn't work for early clocks that don't have a device.

This is why I left the possibility for regmap to be "pre-populated" so it
continues to work the way it previously did.

I admit early clocks is a low priority for me since I only have one
controller like this and I do not expect more.

If cleaning up this particular case is important, then I could add
another level of init:
* A callback passed along the init data of the clock to get the regmap.
  That callback would be called by the .init() ops, if set.
  That can encode any quirks without polluting the ops.
* It will grow the init data so the change won't save memory anymore.
  This was more a bonus so I don't really mind. Maintainability is more
  important.
* If the callback is not set, then it goes through the default, as
  proposed here. This would avoid patching all the clk_regmap clock of
  every controller.

>
>> 
>> That approach could be extended to support controller with multiple
>> regmaps, with a name that does not depend on regmap_config and is local
>> to the clock controller. This will be useful when the name if defined
>> somewhere else (syscon, auxiliary device, etc ...)
>> 
>
> I think you're saying that clk_ops can be common things that aren't
> device/clk controller specific, while the regmap config is usually
> device/clk controller specific.

Agreed

> Furthermore, the name of the regmap is
> also usually device/clk controller specific.

The name registered in regmap_config itself is device specific, not
controller specific, since it can come from something else in the
platform (syscon or even aux devs), that why I think an independent
namespace is desirable -- Same goes the generic solution Conor is
working on I think.

> The regmap assignment
> doesn't really fit with the clk_ops because it's not operating on the
> clk hardware like the other clk_ops all do.

I see what you mean and I agree. It does not operate on the hardware but
it does collect the resources it needs to operate the HW, and ideally
it should do just that - without controller quirks popping up there.

Anyway a callback passed in init data takes care of 'io vs syscon'
controller too, same as devres. I can go that route if this is what you
prefer. I thought devres was a more elegant solution but it is indeed
restricted to 'device enabled' controllers. 

The change will be a bit ugly in the syscon ones but I don't mind.
Is that fine for v2 ?

-- 
Jerome

Powered by blists - more mailing lists