Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
References: <20210528113403.5374-1-peng.fan@oss.nxp.com>
 <162262192433.4130789.1017942859005253343@swboyd.mtv.corp.google.com>
 <a5833012-3e86-5be0-71f2-de4d9b32a152@pengutronix.de>
User-agent: mu4e 1.4.15; emacs 27.1
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Marc Kleine-Budde <mkl@pengutronix.de>,
        Stephen Boyd <sboyd@kernel.org>,
        "Peng Fan (OSS)" <peng.fan@oss.nxp.com>, mturquette@baylibre.com
Cc:     Peng Fan <peng.fan@nxp.com>, linux-kernel@vger.kernel.org,
        kernel@pengutronix.de, linux-clk@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 0/3] clk: support regmap
In-reply-to: <a5833012-3e86-5be0-71f2-de4d9b32a152@pengutronix.de>
Message-ID: <1j1r9kobln.fsf@starbuckisacylon.baylibre.com>
Date:   Wed, 02 Jun 2021 11:32:52 +0200
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


On Wed 02 Jun 2021 at 10:21, Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 6/2/21 10:18 AM, Stephen Boyd wrote:
>> Quoting Peng Fan (OSS) (2021-05-28 04:34:00)
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> To i.MX8ULP, a PCC register provides clk(mux, gate, divider) and peripheral
>>> reset functionality, so we need make sure the access to the PCC register
>>> be protected to avoid concurrent access from clk and reset subsystem.
>>>
>>> So let's use regmap here.
>>>
>>> The i.MX specific code will be posted if this patchset is ok for you.
>> 
>> We have a couple regmap clk drivers in the tree. Either combine the
>> different regmap clk drivers or duplicate it into the imx directory. I'd
>> prefer we combine them but last time I couldn't agree on the approach
>> when Jerome wanted to do it. Maybe now is the time to combine them all
>> into one common piece of code.
>
> IMHO for the basic drivers, such as gate, divider, mux, mux-div, etc... it makes
> no sense to have them in an arch specific subdir, like meson.

Indeed, those basic types were not meant to remain platform
specific. Some framework (ASoC for ex) make heavy use of regmap and
could welcome regmap based basic clock types.

At the time, Stephen (qcom) and I (meson) had slightly different
approaches. Before having those types spread through the kernel, I think
testing things out was a good thing ... this is why these are platform
specific ATM.

It's been 3 years now ... and it has not been a total disaster :)

In the end things are not so different. Let's compare:
a. Both have a generic "clk_regmap" type common to all regmap based
  types. This is very useful to easily fix the regmap pointer in static
  clocks (which are heavily used by both platform)

b. Meson uses a generic pointer to store the type specific info.
  Qcom embeds the generic clk_regmap into the specific type one.
  => In the end, I don't see any advantage to the meson
  approach. Switching to the qcom method would be quite a big bang
  for meson but it is doable (nothing difficult, just a huge line count)
  
c. Qcom basic regmap type deviates a bit from the regular basic ones
  when it comes to the actual data. The qcom "clk_regmap" also provides
  the gate, mux have the qcom "parent_map". In meson, I tried to keep as
  close as possible to regular basic types ... at least what they were 3
  years ago. Only the register poking part should be different actually.
  => I'd be in favor of keeping close to meson here.

A possible plan could be:
1. Rework meson as explained in [b] above.
2. reword types in qcom where necessary to avoid name clashes (add
   "_qcom" extension for ex)
3. Move the clk_regmap implementation out of meson to drivers/clk
4. Things are yours to play with ...

I can take care of 1. and 3. I would welcome help for 2. especially since
I won't be able to test it.

>
> regards,
> Marc