On Mon 06 Jan 2025 at 13:09, Stephen Boyd [off-list ref] wrote:

quoted

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

quoted

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

quoted

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.

quoted

quoted

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.

quoted

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