Quoting Jerome Brunet (2024-12-21 03:09:28)

On Fri 20 Dec 2024 at 16:12, Stephen Boyd [off-list ref] wrote:

quoted

Quoting Jerome Brunet (2024-12-20 09:17:43)

quoted

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.

quoted

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.

quoted

quoted

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?
We could have helpers for the common paths, i.e. the device has the
regmap, or the syscon has the regmap, etc.

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.

quoted

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.

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. Furthermore, the name of the regmap is
also usually device/clk controller specific. 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.