Message-ID: <430bde3b35382e640843e32a9f351326.sboyd@kernel.org>
Date: Thu, 14 Nov 2024 17:29:54 -0800
From: Stephen Boyd <sboyd@...nel.org>
To: Conor Dooley <conor@...nel.org>, Jerome Brunet <jbrunet@...libre.com>
Cc: Neil Armstrong <neil.armstrong@...aro.org>, linux-kernel@...r.kernel.org, Conor Dooley <conor.dooley@...rochip.com>, Daire McNamara <daire.mcnamara@...rochip.com>, pierre-henry.moussay@...rochip.com, valentina.fernandezalanis@...rochip.com, Michael Turquette <mturquette@...libre.com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Jassi Brar <jassisinghbrar@...il.com>, Lee Jones <lee@...nel.org>, Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, Kevin Hilman <khilman@...libre.com>, Martin Blumenstingl <martin.blumenstingl@...glemail.com>, Philipp Zabel <p.zabel@...gutronix.de>, linux-riscv@...ts.infradead.org, linux-clk@...r.kernel.org, devicetree@...r.kernel.org, linux-amlogic@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v1 08/11] clk: move meson clk-regmap implementation to common code

Quoting Conor Dooley (2024-11-06 04:56:25)
> On Thu, Oct 03, 2024 at 12:33:40PM +0100, Conor Dooley wrote:
> > > IIRC, Stephen's idea was more the bring regmap support in clk-gate.c,
> > > clk-mux, etc ... I'm not quite sure how make iomem and regmap co-exist
> > > in a manageable/maintainable way within those drivers (without adding yet
> > > another level of abstraction I mean) ? Silently creating a regmap maybe
> > > ? but that's probably a bit heavy. I did not really had time to dig more
> > > on this, I guess no one did.
> > 
> > I guess I have some motivation to looking into it at the moment. I had
> > my reservations about the Meson approach too, liking it more than
> > Qualcomm's didn't mean I completely liked it.
> > It was already my intention to implement point b of your mail, had the
> > general idea here been acceptable, cos that's a divergence from how the
> > generic clock types (that the driver in question currently uses) work.
> > And on that note, I just noticed I left the mild-annoyance variable name
> > "sigh" in the submitted driver changes, which I had used for the
> > clk_regmap struct that your point b in the link relates to.
> > 
> > > I don't really have a preference one way or the other but if it is going
> > > to be exposed in 'include/linux', we need to be sure that's how we want
> > > to do it. With clocks poping in many driver subsystems, it will
> > > difficult to change afterward. 
> > 
> > Yeah, I agree. I didn't expect this to go in right away, and I also
> > didn't want to surge ahead on some rework of the clock types, were
> > people to hate even the reuse.
> 
> Hmm, so how (in-)complete of a regmap implementation can I get away
> with? I only need clk-gate and clk-divider for this patchset...
> 
> Shoving the regmap into the clk structs makes things pretty trivial as I
> don't need to do anything special in any function other than
> clk_*_readl()/clk_*_writel() and the registration code. A flag isn't
> even needed to determine if a clock is a regmap one I don't think, since
> you can just check if the regmap pointer is non-NULL.

For the basic clk types I think it would be good to leave the old stuff
alone. We have already split the logic out into helpers, so I wonder if
we can do this better by making kernel modules for the different basic
regmap clk types and exposing registration APIs. If we force drivers
that use the basic regmap types to 'select' the module then we'll make
it so that we don't include code that isn't used anywhere. That's one of
the problems with the basic clk types, it's always built. It also lets
us avoid making regmap a dependency for the clk framework at large.

Doing that would also let us avoid the flag because it will be explicit
in any registration API, clk_register_divider() vs.
clk_register_regmap_divider(). Yes we duplicate some boiler plate logic
around read-only and registration paths, but this is alright as long as
we can share most of the code and gain the advantage of removing the
code entirely when it isn't used.

I wonder if we can even make a regmap on the fly for the iomem pointers
so that clk_divider_readl() can always use the regmap API to access the
hardware. Sounds wasteful but maybe it would work.

> My use case doesn't
> actually need the registration code changes either as, currently, only reg
> gets set at runtime, but leaving that out is a level of incomplete I'd not
> let myself away with.
> Obviously shoving the extra members into the clk structs has the downside
> of taking up a pointer and a offset worth of memory for each clock of
> that type registered, but it is substantially easier to support devices
> with multiple regmaps that way. Probably moot though since the approach you
> suggested in the thread linked above that implements a clk_hw_get_regmap()
> has to store a pointer to the regmap's identifier which would take up an
> identical amount of memory.

We don't need to store the regmap identifier in the struct clk. We can
store it in the 'struct clk_init_data' with some new field, and only do
that when/if we actually need to. We would need to pass the init data to
the clk_ops::init() callback though. We currently knock that out during
registration so that clk_hw->init is NULL. Probably we can just set that
to NULL after the init routine runs in __clk_core_init().

Long story short, don't add something to 'struct clk_core', 'struct
clk', or 'struct clk_hw' for these details. We can have a 'struct
clk_regmap_hw' that everyone else can build upon:

  struct clk_regmap_hw {
        struct regmap *regmap;
        struct clk_hw hw;
  };

and then set the regmap pointer during registration in
clk_hw_init_regmap().

int clk_hw_init_regmap(struct clk_hw *hw)
{
	struct device *dev;
	struct regmap *regmap;
	struct clk_regmap_hw *rhw;

	rhw = clk_hw_to_clk_regmap_hw(hw);

	dev = clk_hw_get_dev(hw);
	if (!dev)
		return -EINVAL;

	regmap = dev_get_regmap(dev, hw->init->regmap_name);
	if (!regmap)
		return -EINVAL; // Print helpful message
	rhw->regmap = regmap;

	return 0;
}

or we can even make it so that there's clk_hw_init_regmap() and
clk_hw_init_regmap_name() so that drivers can have multiple functions if
the clks need different regmaps.

int my_init_regmap(struct clk_hw *hw)
{
	int ret;

	ret = clk_hw_init_regmap_name(hw, "my_name");
	...
}

If you don't need the multiple regmap support then it's fine to punt
here until later.

> 
> I don't really care which way you want it done, both are pretty easy to
> implement if I can get away with just doing so for the two standard
> clock types that I am using - is it okay to just do those two?
> 

Of course, doing only what is minimally required is better than changing
everything if you're not sure the approach is going to land.

Powered by blists - more mailing lists