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

[Ulf Hansson]

On Fri, 23 Sept 2022 at 20:00, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 22/09/2022 15:49, Ulf Hansson wrote:
> > 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
>
> Again, as Rob said, no, because it must be strictly defined. So for
> example: "maxItems: 1" for simple cases. But what if device is then part
> of two power domains?
>
> >
> > That should be okay, right?
>
> Adding it to each peripheral scales poorly. Especially that literally
> any device can be part of such power domain.

Right.

>
> If we are going with power domain approach, then it should be applicable
> basically to every device or to every device of some class (e.g. I2C,
> SPI). This means it should be added to respective core schema in
> dtschema repo, in a way it does not interfere with other power-domains
> properties (existing ones).

Isn't that already taken care of [1]?

If there is more than one power domain per device, perhaps we may need
to extend it with a more strict binding? But, that doesn't seem to be
the case here - and if it turns out to be needed later on, we can
always extend the bindings, no?

Note also that we already have DT bindings specifying "power-domains:
true" to deal with the above. Isn't that what we want?

>
>
> Best regards,
> Krzysztof
>

Kind regards
Uffe

[1]
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/power-domain/power-domain-consumer.yaml