Re: [PATCH v1 0/5] power: domain: Add driver for a PM domain provider which controls

[Ulf Hansson]

On Fri, 9 Sept 2022 at 16:22, Francesco Dolcini
<francesco.dolcini@xxxxxxxxxxx> wrote:
>
> Hello Ulf,
>
> On Fri, Aug 26, 2022 at 03:50:46PM +0200, Ulf Hansson wrote:
> > On Thu, 28 Jul 2022 at 13:21, Francesco Dolcini
> > <francesco.dolcini@xxxxxxxxxxx> wrote:
> > >
> > > On Thu, Jul 28, 2022 at 11:37:07AM +0200, Ulf Hansson wrote:
> > > > On Tue, 26 Jul 2022 at 18:03, Francesco Dolcini
> > > > <francesco.dolcini@xxxxxxxxxxx> wrote:
> > > > >
> > > > > Hello Ulf and everybody,
> > > > >
> > > > > On Wed, Jul 13, 2022 at 01:43:28PM +0200, Ulf Hansson wrote:
> > > > > > On Thu, 23 Jun 2022 at 18:14, Max Krummenacher <max.oss.09@xxxxxxxxx> wrote:
> > > > > > > So our plan is to explicitly handle a (shared) regulator in every
> > > > > > > driver involved, adding that regulator capability for drivers not
> > > > > > > already having one.
> > > > > >
> > > > > > Please don't! I have recently rejected a similar approach for Tegra
> > > > > > platforms, which now have been converted into using the power domain
> > > > > > approach.
> > > > >
> > > > > Just to quickly re-iterate how our hardware design looks like, we do
> > > > > have a single gpio that control the power of a whole board area that is
> > > > > supposed to be powered-off in suspend mode, this area could contains
> > > > > devices that have a proper Linux driver and some passive driver-less
> > > > > components (e.g. level shifter) - the exact mix varies.
> > > > >
> > > > > Our proposal in this series was to model this as a power domain that
> > > > > could be controlled with a regulator. Krzysztof, Robin and others
> > > > > clearly argued against this idea.
> > > >
> > > > Well, historically we haven't modelled these kinds of power-rails
> > > > other than through power-domains. And this is exactly what genpd and
> > > > PM domains in Linux are there to help us with.
> > > >
> > > > Moreover, on another SoC/platform, maybe the power-rails are deployed
> > > > differently and maybe those have the ability to scale performance too.
> > > > Then it doesn't really fit well with the regulator model anymore.
> > > >
> > > > If we want to continue to keep drivers portable, I don't see any
> > > > better option than continuing to model these power-rails as
> > > > power-domains.
> > > >
> > > > >
> > > > > The other approach would be to have a single regulator shared with the
> > > > > multiple devices we have there (still not clear how that would work in
> > > > > case we have only driver-less passive components). This is just a
> > > > > device-tree matter, maybe we would need to add support for a supply to
> > > > > some device drivers.
> > > > >
> > > > > Honestly my conclusion from this discussion is that the only viable
> > > > > option is this second one, do I miss something?
> > > >
> > > > No thanks!
> > > >
> > > > Well, unless you can convince me there are benefits to this approach
> > > > over the power-domain approach.
> > >
> > > I'm fine with our current power-domain proposal here, I do not need to
> > > convince you, I have the other problem to convince someone to merge
> > > it :-)
> > >
> > > Maybe Krzysztof, Robin or Mark can comment again after you explained
> > > your view on this topic.
> >
> > To move things forward, I suggest you re-start with the power domain approach.
> >
> > Moreover, to avoid any churns, just implement it as another new SoC
> > specific genpd provider and let the provider deal with the regulator.
> I'm sorry, but I was not able to understand what you mean, can you
> provide some additional hint on the topic? Some reference driver we can
> look at?

Typically, "git grep pm_genpd_init" will find genpd providers.

There are a couple of examples where a regulator (among other things)
is being controlled from the genpd's ->power_on|off() callbacks, such
as:

drivers/soc/mediatek/mtk-pm-domains.c
drivers/soc/imx/gpc.c

>
> The driver we implemented and proposed with this patch is just
> connecting a power-domain to a regulator, it's something at the board
> level, not at the SoC one.
> We do not have a (existing) SoC driver were we could add the power
> domain provider as an additional functionality.

Right, so you need to add a new SoC/platform driver for this.

>
> > In this way, you don't need to invent any new types of DT bindings,
> > but can re-use existing ones.
> The only new binding would be a new "compatible" to have a place to
> tie the regulator instance used in the device tree, but I do not think
> that this is an issue at all.

Yes, I agree.

>
> The main concern that was raised on this topic was that we have to
> somehow link the power-domain to the specific peripherals (the power
> domain consumer) in the device tree.

Yes, that is needed. Although, I don't see how that is a concern?

We already have the valid bindings to use for this, see more below.

>
> Adding the power-domain property there will trigger validation errors
> unless we do explicitly add the power-domains to the schema for each
> peripheral we need this. To me this does not really work, but maybe I'm
> not understanding something.
>
> This is what Rob wrote on the topic [1]:
>   > No. For 'power-domains' bindings have to define how many there are and
>   > what each one is.
>
> Just as an example from patch [2]:
>
>   can1: can@0 {
>     compatible = "microchip,mcp251xfd";
>     power-domains = <&pd_sleep_moci>;
>   };
>
> leads to:
>
>   imx8mm-verdin-nonwifi-dahlia.dtb: can@0: 'power-domains' does not match any of the regexes: 'pinctrl-[0-9]+'
>           From schema: .../bindings/net/can/microchip,mcp251xfd.yaml

I think it should be fine to just add the below line to the DT
bindings, for each peripheral device to fix the above problem.

power-domains: true

That should be okay, right?

>
> > If you post a new version, please keep me cced, then I will help to review it.
> Thanks!
>
> Francesco
>
> [1] https://lore.kernel.org/all/20220613191549.GA4092455-robh@xxxxxxxxxx/
> [2] https://lore.kernel.org/all/20220609150851.23084-6-max.oss.09@xxxxxxxxx/
>

Kind regards
Uffe