Re: [PATCH v1 08/11] clk: move meson clk-regmap implementation to common code

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6279 bytes --]

Stephen,

On Thu, Oct 03, 2024 at 12:33:40PM +0100, Conor Dooley wrote:
> On Wed, Oct 02, 2024 at 03:21:16PM +0200, Jerome Brunet wrote:
> > On Wed 02 Oct 2024 at 13:20, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> > 
> > > On 02/10/2024 12:48, Conor Dooley wrote:
> > >> From: Conor Dooley <conor.dooley@microchip.com>
> > >> I like this one better than qualcomms and wish to use it for the
> > >> PolarFire SoC clock drivers.
> > >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > >> ---
> > >>   drivers/clk/Kconfig                           |  4 ++
> > >>   drivers/clk/Makefile                          |  1 +
> > >>   drivers/clk/{meson => }/clk-regmap.c          |  2 +-
> > >>   drivers/clk/meson/Kconfig                     | 46 +++++++++----------
> > >>   drivers/clk/meson/Makefile                    |  1 -
> > >>   drivers/clk/meson/a1-peripherals.c            |  2 +-
> > >>   drivers/clk/meson/a1-pll.c                    |  2 +-
> > >>   drivers/clk/meson/axg-aoclk.c                 |  2 +-
> > >>   drivers/clk/meson/axg-audio.c                 |  2 +-
> > >>   drivers/clk/meson/axg.c                       |  2 +-
> > >>   drivers/clk/meson/c3-peripherals.c            |  2 +-
> > >>   drivers/clk/meson/c3-pll.c                    |  2 +-
> > >>   drivers/clk/meson/clk-cpu-dyndiv.c            |  2 +-
> > >>   drivers/clk/meson/clk-dualdiv.c               |  2 +-
> > >>   drivers/clk/meson/clk-mpll.c                  |  2 +-
> > >>   drivers/clk/meson/clk-phase.c                 |  2 +-
> > >>   drivers/clk/meson/clk-pll.c                   |  2 +-
> > >>   drivers/clk/meson/g12a-aoclk.c                |  2 +-
> > >>   drivers/clk/meson/g12a.c                      |  2 +-
> > >>   drivers/clk/meson/gxbb-aoclk.c                |  2 +-
> > >>   drivers/clk/meson/gxbb.c                      |  2 +-
> > >>   drivers/clk/meson/meson-aoclk.h               |  2 +-
> > >>   drivers/clk/meson/meson-eeclk.c               |  2 +-
> > >>   drivers/clk/meson/meson-eeclk.h               |  2 +-
> > >>   drivers/clk/meson/meson8-ddr.c                |  2 +-
> > >>   drivers/clk/meson/meson8b.c                   |  2 +-
> > >>   drivers/clk/meson/s4-peripherals.c            |  2 +-
> > >>   drivers/clk/meson/s4-pll.c                    |  2 +-
> > >>   drivers/clk/meson/sclk-div.c                  |  2 +-
> > >>   drivers/clk/meson/vclk.h                      |  2 +-
> > >>   drivers/clk/meson/vid-pll-div.c               |  2 +-
> > >>   .../meson => include/linux/clk}/clk-regmap.h  |  0
> > >>   32 files changed, 53 insertions(+), 53 deletions(-)
> > >>   rename drivers/clk/{meson => }/clk-regmap.c (99%)
> > >>   rename {drivers/clk/meson => include/linux/clk}/clk-regmap.h (100%)
> > >> 
> > > <snip>
> > >
> > > I don't have objections, but I think Stephen didn't like the idea
> > > a few years ago, but perhaps it has changed...
> > >
> > > Anyway, take my:
> > > Acked-by: Neil Armstrong <neil.armstrong@linaro.org>
> > 
> > We had a similar discussion 3y ago indeed:
> > https://lore.kernel.org/linux-clk/162734682512.2368309.12015873010777083014@swboyd.mtv.corp.google.com/
> > 
> > There are needs for a common regmap backed clocks indeed, but allowing
> > meson flavored regmap clocks to spread in the kernel was not really the
> > prefered way to do it. 
> 
> Cool, thanks for that link.
> 
> > 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. 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.

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?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]