Message-ID: <1jmsg2adgu.fsf@starbuckisacylon.baylibre.com>
Date: Tue, 07 Jan 2025 15:46:41 +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 06 Jan 2025 at 13:09, Stephen Boyd <sboyd@...nel.org> wrote:

>> 
>> 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.
>
> The struct clk_init_data _can_ be thrown away or reused, but it isn't
> always done that way.

Yeah, I was actually thinking about using struct clk_regmap for a
start. It is much simpler

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-regmap.h#n23

>
>> * 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.
>> 
>> 
>> > 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.
>
> Alright.
>
>> 
>> > 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 ?

Just before discussing what seems to be a very generic solution, I'd
like to go ahead with a temporary solution to remove the clk_regmap
table in drivers/clk/meson, if you don't mind. Something simple.

As I have pointed in the cover letter, I have a significant number of
other clean-up on top of this. It's not necessarily complex but it is a
pain to rebase because of the amount of code involved ... and I have new
controller waiting. I'll circle back to the final solution afterward.

>> 
>
> Sure. I wonder if we should make it a 'const void *data' member of
> struct clk_init_data so it can be anything and then either take a flag
> day to pass that to the struct clk_ops::init() function or set the
> struct clk_hw::init member to NULL after the init function is called. If
> we're concerned about bloating clk_init_data then we could introduce
> another two registration APIs that take a data argument and then pass
> that to the init function.
>
>  int clk_hw_register_data(struct device *dev, struct clk_hw *hw, const void *data)
>  int of_clk_hw_register_data(struct device *dev, struct clk_hw *hw, const void *data)
>
> or we could wrap the init data in a container struct in the drivers and
> move the setting of struct clk_hw::init to NULL after calling the init
> function.
>
> 	struct clk_driver_init_data {
> 		void *data;
> 		int (*driver_init_function)(struct clk_hw *hw);
> 		int (*regmap_driver_init_function)(struct clk_regmap *rclk);
> 		etc...
>
> 		struct clk_init_data init;
> 	};
>
> Then the clk provider can use container_of(). If we did this we could
> even copy the contents of struct clk_hw::init into the driver specific
> wrapper that lives on the stack, repoint the struct clk_hw::init pointer
> to the stack copy, and then all the logic can live in the clk provider
> driver that registers the clk.
>
> This last option may be the best because it saves memory by not
> increasing the size of 'struct clk_init_data' and doesn't require a flag
> day to change the function signature of struct clk_ops::init(), even if
> there's only a handful of those right now. What do you think?

I think I see in which direction you want to go. The problem is that we
have been playing the 'container_of()' trick quite a lot. Embedding
something around init_data is not straight forward for me with the way
clocks are declared in drivers/clk/meson.

I'll have to separate the init_data out, which is desirable but it
brings another set of problems. One mess after the other :)

So, if it's OK, I'll resend this series with a temporary solution to
remove tables. Removing the table simplify the other clean-up I have
already line-up and avoid some unnecessary diffs. I'll circle back to
reworking the init_data afterward.

-- 
Jerome

Powered by blists - more mailing lists