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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 26/09/2022 12:12, Ulf Hansson wrote:
> On Fri, 23 Sept 2022 at 20:00, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 22/09/2022 15:49, Ulf Hansson wrote:
>>> On Fri, 9 Sept 2022 at 16:22, Francesco Dolcini
>>> <francesco.dolcini@toradex.com> 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@toradex.com> 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@toradex.com> 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@gmail.com> 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]?

No, because it does not define the items (what are the power domains and
how many). This binding expects that any device has maxItems restricting it.

> 
> 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?

You mentioned it before and both me and Rob already responded - no,
because it does not restrict the number of items.

Best regards,
Krzysztof