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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250220-iciness-mobilize-94a027ff52ce@spud>
Date: Thu, 20 Feb 2025 15:29:10 +0000
From: Conor Dooley <conor@...nel.org>
To: Stephen Boyd <sboyd@...nel.org>
Cc: Conor Dooley <conor.dooley@...rochip.com>,
	Jerome Brunet <jbrunet@...libre.com>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	linux-kernel@...r.kernel.org,
	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

On Tue, Jan 21, 2025 at 05:38:53PM +0000, Conor Dooley wrote:
> Hey Stephen,
> 
> Any thoughts on the example I gave below?

I'll give you a few more days to comment, and then I'll just send a
fresh revision, implemented as below. The links seem to have expired in
a rebase along the way, so I've provided some fresh links.

Cheers,
Conor.

> 
> On Fri, Dec 06, 2024 at 01:56:08PM +0000, Conor Dooley wrote:
> > On Tue, Dec 03, 2024 at 02:50:31PM -0800, Stephen Boyd wrote:
> > > Quoting Conor Dooley (2024-11-28 02:36:16)
> > > > On Thu, Nov 14, 2024 at 05:29:54PM -0800, Stephen Boyd wrote:
> > > > > Quoting Conor Dooley (2024-11-06 04:56:25)
> > > > > > 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;
> > > > >   };
> > > > 
> > > > What's the point of this? I don't understand why you want to do this over
> > > > what clk_divider et al already do, where clk_hw and the iomem pointer
> > > > are in the struct itself.
> > > 
> > > Can you give an example? I don't understand what you're suggesting. I
> > > prefer a struct clk_regmap_hw like above so that the existing struct
> > > clk_hw in the kernel aren't increased by a pointer. SoC drivers can use
> > > the same struct as a replacement for their struct clk_hw member today.
> > 
> > Best example I guess is to link what I did? This one is the core
> > changes:
> > https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=syscon-rework-2&id=35904222355e971c24b3eb9b9fad3dd0c38d1393

https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=syscon-rework-2&id=435c8eb223ee804297a0491fae2b00d3d5a9c773

> > clk-gate has my original hack that I did while trying to figure out
> > what you wanted, clk-divider-regmap is a 99% copy of clk-divider with
> > the types, function names and readl()/writel() implementations modified.
> > Before your last set of comments I was doing something identical to the
> > clk-gate change for clk-divider also.
> > Here's the changes required to my driver to make it work with the
> > updated:
> > https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=syscon-rework-2&id=ea40211fe20f8bc6ef0320b93e1baa5b3f244601

https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=syscon-rework-2&id=f55e907e93c55c943725dd62c2fc7dc76cdbd8d5

> > It's pretty much a drop in replacement, other than the additional
> > complexity in probe.
> > 
> > Hopefully that either gets my point across or lets you spot why I don't
> > understand the benefit of a wrapper around clk_hw.
> > 
> > Cheers,
> > Conor.
> 
> 



Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)