diff mbox series

[01/12] dt-bindings: power: Add bindings for the Mediatek SCPSYS power domains controller

Message ID 20200910172826.3074357-2-enric.balletbo@collabora.com
State Changes Requested
Headers show
Series soc: mediatek: pm-domains: Add new driver for SCPSYS power domains controller | expand

Checks

Context Check Description
robh/dt-meta-schema fail build log
robh/checkpatch success

Commit Message

Enric Balletbo i Serra Sept. 10, 2020, 5:28 p.m. UTC
The System Control Processor System (SCPSYS) has several power management
related tasks in the system. Add the bindings to define the power
domains for the SCPSYS power controller.

Co-developed-by: Matthias Brugger <mbrugger@suse.com>
Signed-off-by: Matthias Brugger <mbrugger@suse.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Dear Rob,

I am awasre that this binding is not ready, but I prefered to send because I'm
kind of blocked. Compiling this binding triggers the following error:

    mediatek,power-controller.example.dt.yaml: syscon@10006000: mfg_async@7:
    '#address-cells', '#size-cells', 'mfg_2d@8'
    do not match any of the regexes: 'pinctrl-[0-9]+'

This happens when a definition of a power-domain (parent) contains
another power-domain (child), like the example. I am not sure how to
specify this in the yaml and deal with this, so any clue is welcome.

Thanks,
  Enric

 .../power/mediatek,power-controller.yaml      | 171 ++++++++++++++++++
 1 file changed, 171 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/mediatek,power-controller.yaml

Comments

Rob Herring Sept. 11, 2020, 11:02 p.m. UTC | #1
On Thu, Sep 10, 2020 at 07:28:15PM +0200, Enric Balletbo i Serra wrote:
> The System Control Processor System (SCPSYS) has several power management
> related tasks in the system. Add the bindings to define the power
> domains for the SCPSYS power controller.
> 
> Co-developed-by: Matthias Brugger <mbrugger@suse.com>
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Dear Rob,
> 
> I am awasre that this binding is not ready, but I prefered to send because I'm
> kind of blocked. Compiling this binding triggers the following error:
> 
>     mediatek,power-controller.example.dt.yaml: syscon@10006000: mfg_async@7:
>     '#address-cells', '#size-cells', 'mfg_2d@8'
>     do not match any of the regexes: 'pinctrl-[0-9]+'
> 
> This happens when a definition of a power-domain (parent) contains
> another power-domain (child), like the example. I am not sure how to
> specify this in the yaml and deal with this, so any clue is welcome.

You just have to keep nesting schemas all the way down. Define a 
grandchild node under the child node and then all of its properties.

> 
> Thanks,
>   Enric
> 
>  .../power/mediatek,power-controller.yaml      | 171 ++++++++++++++++++
>  1 file changed, 171 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> new file mode 100644
> index 000000000000..8be9244ad160
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> @@ -0,0 +1,171 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/mediatek,power-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Power Domains Controller
> +
> +maintainers:
> +  - Weiyi Lu <weiyi.lu@mediatek.com>
> +  - Matthias Brugger <mbrugger@suse.com>
> +
> +description: |
> +  Mediatek processors include support for multiple power domains which can be
> +  powered up/down by software based on different application scenes to save power.
> +
> +  IP cores belonging to a power domain should contain a 'power-domains'
> +  property that is a phandle for SCPSYS node representing the domain.
> +
> +properties:
> +  $nodename:
> +    pattern: "^syscon@[0-9a-f]+$"
> +
> +  compatible:
> +    items:
> +      - enum:
> +        - mediatek,mt8173-power-controller
> +      - const: syscon
> +
> +  reg:
> +    maxItems: 1
> +
> +patternProperties:
> +  "^.*@[0-9]$":

Node names should be generic:

power-domain@

> +    type: object
> +    description: |
> +      Represents the power domains within the power controller node as documented
> +      in Documentation/devicetree/bindings/power/power-domain.yaml.
> +
> +    properties:
> +      reg:
> +        description: |
> +          Power domain index. Valid values are defined in:
> +              "include/dt-bindings/power/mt8173-power.h" - for MT8173 type power domain.
> +        maxItems: 1
> +
> +      '#power-domain-cells':
> +        description:
> +          Documented by the generic PM Domain bindings in
> +          Documentation/devicetree/bindings/power/power-domain.yaml.

No need to redefine a common property. This should define valid values 
for it.

> +
> +      clocks:
> +        description: |
> +          A number of phandles to clocks that need to be enabled during domain
> +          power-up sequencing.

No need to redefine 'clocks'. You need to define how many, what each one 
is, and the order.

> +
> +      clock-names:
> +        description: |
> +          List of names of clocks, in order to match the power-up sequencing
> +          for each power domain we need to group the clocks by name. BASIC
> +          clocks need to be enabled before enabling the corresponding power
> +          domain, and should not have a '-' in their name (i.e mm, mfg, venc).
> +          SUSBYS clocks need to be enabled before releasing the bus protection,
> +          and should contain a '-' in their name (i.e mm-0, isp-0, cam-0).
> +
> +          In order to follow properly the power-up sequencing, the clocks must
> +          be specified by order, adding first the BASIC clocks followed by the
> +          SUSBSYS clocks.

You need to define the names.

> +
> +      mediatek,infracfg:
> +        $ref: /schemas/types.yaml#definitions/phandle
> +        description: phandle to the device containing the INFRACFG register range.
> +
> +      mediatek,smi:
> +        $ref: /schemas/types.yaml#definitions/phandle
> +        description: phandle to the device containing the SMI register range.
> +
> +    required:
> +      - reg
> +      - '#power-domain-cells'
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mt8173-clk.h>
> +    #include <dt-bindings/power/mt8173-power.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        scpsys: syscon@10006000 {
> +            compatible = "mediatek,mt8173-power-controller", "syscon";
> +            reg = <0 0x10006000 0 0x1000>;
> +
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            /* power domains of the SoC */
> +            vdec@MT8173_POWER_DOMAIN_VDEC {
> +                reg = <MT8173_POWER_DOMAIN_VDEC>;
> +                clocks = <&topckgen CLK_TOP_MM_SEL>;
> +                clock-names = "mm";
> +                #power-domain-cells = <0>;
> +            };
> +
> +            venc@MT8173_POWER_DOMAIN_VENC {
> +                reg = <MT8173_POWER_DOMAIN_VENC>;
> +                clocks = <&topckgen CLK_TOP_MM_SEL>,
> +                         <&topckgen CLK_TOP_VENC_SEL>;
> +                clock-names = "mm", "venc";
> +                #power-domain-cells = <0>;
> +            };
> +            isp@MT8173_POWER_DOMAIN_ISP {
> +                reg = <MT8173_POWER_DOMAIN_ISP>;
> +                clocks = <&topckgen CLK_TOP_MM_SEL>;
> +                clock-names = "mm";
> +                #power-domain-cells = <0>;
> +            };
> +            mm@MT8173_POWER_DOMAIN_MM {
> +                reg = <MT8173_POWER_DOMAIN_MM>;
> +                clocks = <&topckgen CLK_TOP_MM_SEL>;
> +                clock-names = "mm";
> +                #power-domain-cells = <0>;
> +                mediatek,infracfg = <&infracfg>;
> +            };
> +            venc_lt@MT8173_POWER_DOMAIN_VENC_LT {
> +                reg = <MT8173_POWER_DOMAIN_VENC_LT>;
> +                clocks = <&topckgen CLK_TOP_MM_SEL>,
> +                         <&topckgen CLK_TOP_VENC_LT_SEL>;
> +                clock-names = "mm", "venclt";
> +                #power-domain-cells = <0>;
> +            };
> +            audio@MT8173_POWER_DOMAIN_AUDIO {
> +                reg = <MT8173_POWER_DOMAIN_AUDIO>;
> +                #power-domain-cells = <0>;
> +            };
> +            usb@MT8173_POWER_DOMAIN_USB {
> +                reg = <MT8173_POWER_DOMAIN_USB>;
> +                #power-domain-cells = <0>;
> +            };
> +            mfg_async@MT8173_POWER_DOMAIN_MFG_ASYNC {
> +                reg = <MT8173_POWER_DOMAIN_MFG_ASYNC>;
> +                clocks = <&clk26m>;
> +                clock-names = "mfg";
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                #power-domain-cells = <1>;
> +
> +                mfg_2d@MT8173_POWER_DOMAIN_MFG_2D {
> +                    reg = <MT8173_POWER_DOMAIN_MFG_2D>;
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +                    #power-domain-cells = <1>;
> +
> +                    mfg@MT8173_POWER_DOMAIN_MFG {
> +                        reg = <MT8173_POWER_DOMAIN_MFG>;
> +                        #power-domain-cells = <0>;
> +                        mediatek,infracfg = <&infracfg>;
> +                    };
> +                };
> +            };
> +        };
> +    };
> -- 
> 2.28.0
>
Matthias Brugger Sept. 14, 2020, 8:59 a.m. UTC | #2
On 12/09/2020 01:02, Rob Herring wrote:
> On Thu, Sep 10, 2020 at 07:28:15PM +0200, Enric Balletbo i Serra wrote:
>> The System Control Processor System (SCPSYS) has several power management
>> related tasks in the system. Add the bindings to define the power
>> domains for the SCPSYS power controller.
>>
>> Co-developed-by: Matthias Brugger <mbrugger@suse.com>
>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>> Dear Rob,
>>
>> I am awasre that this binding is not ready, but I prefered to send because I'm
>> kind of blocked. Compiling this binding triggers the following error:
>>
>>      mediatek,power-controller.example.dt.yaml: syscon@10006000: mfg_async@7:
>>      '#address-cells', '#size-cells', 'mfg_2d@8'
>>      do not match any of the regexes: 'pinctrl-[0-9]+'
>>
>> This happens when a definition of a power-domain (parent) contains
>> another power-domain (child), like the example. I am not sure how to
>> specify this in the yaml and deal with this, so any clue is welcome.
> 
> You just have to keep nesting schemas all the way down. Define a
> grandchild node under the child node and then all of its properties.
> 
>>
>> Thanks,
>>    Enric
>>
>>   .../power/mediatek,power-controller.yaml      | 171 ++++++++++++++++++
>>   1 file changed, 171 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>> new file mode 100644
>> index 000000000000..8be9244ad160
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>> @@ -0,0 +1,171 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/mediatek,power-controller.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Mediatek Power Domains Controller
>> +
>> +maintainers:
>> +  - Weiyi Lu <weiyi.lu@mediatek.com>
>> +  - Matthias Brugger <mbrugger@suse.com>
>> +
>> +description: |
>> +  Mediatek processors include support for multiple power domains which can be
>> +  powered up/down by software based on different application scenes to save power.
>> +
>> +  IP cores belonging to a power domain should contain a 'power-domains'
>> +  property that is a phandle for SCPSYS node representing the domain.
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^syscon@[0-9a-f]+$"
>> +
>> +  compatible:
>> +    items:
>> +      - enum:
>> +        - mediatek,mt8173-power-controller
>> +      - const: syscon
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +patternProperties:
>> +  "^.*@[0-9]$":
> 
> Node names should be generic:
> 
> power-domain@
> 

Enric correct me if I'm wrong, if we want to see the power domains in debugfs, 
they are listed by their name. If all are called power-domain then the listing 
is pretty much useless.

>> +    type: object
>> +    description: |
>> +      Represents the power domains within the power controller node as documented
>> +      in Documentation/devicetree/bindings/power/power-domain.yaml.
>> +
>> +    properties:
>> +      reg:
>> +        description: |
>> +          Power domain index. Valid values are defined in:
>> +              "include/dt-bindings/power/mt8173-power.h" - for MT8173 type power domain.
>> +        maxItems: 1
>> +
>> +      '#power-domain-cells':
>> +        description:
>> +          Documented by the generic PM Domain bindings in
>> +          Documentation/devicetree/bindings/power/power-domain.yaml.
> 
> No need to redefine a common property. This should define valid values
> for it.
> 
>> +
>> +      clocks:
>> +        description: |
>> +          A number of phandles to clocks that need to be enabled during domain
>> +          power-up sequencing.
> 
> No need to redefine 'clocks'. You need to define how many, what each one
> is, and the order.
> 

Do you mean we have to define each clock for each power domain of each SoC?

>> +
>> +      clock-names:
>> +        description: |
>> +          List of names of clocks, in order to match the power-up sequencing
>> +          for each power domain we need to group the clocks by name. BASIC
>> +          clocks need to be enabled before enabling the corresponding power
>> +          domain, and should not have a '-' in their name (i.e mm, mfg, venc).
>> +          SUSBYS clocks need to be enabled before releasing the bus protection,
>> +          and should contain a '-' in their name (i.e mm-0, isp-0, cam-0).
>> +
>> +          In order to follow properly the power-up sequencing, the clocks must
>> +          be specified by order, adding first the BASIC clocks followed by the
>> +          SUSBSYS clocks.
> 
> You need to define the names.
> 
>> +
>> +      mediatek,infracfg:
>> +        $ref: /schemas/types.yaml#definitions/phandle
>> +        description: phandle to the device containing the INFRACFG register range.
>> +
>> +      mediatek,smi:
>> +        $ref: /schemas/types.yaml#definitions/phandle
>> +        description: phandle to the device containing the SMI register range.
>> +
>> +    required:
>> +      - reg
>> +      - '#power-domain-cells'
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/mt8173-clk.h>
>> +    #include <dt-bindings/power/mt8173-power.h>
>> +
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        scpsys: syscon@10006000 {
>> +            compatible = "mediatek,mt8173-power-controller", "syscon";

The power domain controller is just one funcionality the SCPSYS block can 
provide. I think it should be child of the SCPSYS.

Regards,
Matthias
Enric Balletbo i Serra Sept. 14, 2020, 9:49 a.m. UTC | #3
On 14/9/20 10:59, Matthias Brugger wrote:
> 
> 
> On 12/09/2020 01:02, Rob Herring wrote:
>> On Thu, Sep 10, 2020 at 07:28:15PM +0200, Enric Balletbo i Serra wrote:
>>> The System Control Processor System (SCPSYS) has several power management
>>> related tasks in the system. Add the bindings to define the power
>>> domains for the SCPSYS power controller.
>>>
>>> Co-developed-by: Matthias Brugger <mbrugger@suse.com>
>>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> ---
>>> Dear Rob,
>>>
>>> I am awasre that this binding is not ready, but I prefered to send because I'm
>>> kind of blocked. Compiling this binding triggers the following error:
>>>
>>>      mediatek,power-controller.example.dt.yaml: syscon@10006000: mfg_async@7:
>>>      '#address-cells', '#size-cells', 'mfg_2d@8'
>>>      do not match any of the regexes: 'pinctrl-[0-9]+'
>>>
>>> This happens when a definition of a power-domain (parent) contains
>>> another power-domain (child), like the example. I am not sure how to
>>> specify this in the yaml and deal with this, so any clue is welcome.
>>
>> You just have to keep nesting schemas all the way down. Define a
>> grandchild node under the child node and then all of its properties.
>>
>>>
>>> Thanks,
>>>    Enric
>>>
>>>   .../power/mediatek,power-controller.yaml      | 171 ++++++++++++++++++
>>>   1 file changed, 171 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>>> b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>>> new file mode 100644
>>> index 000000000000..8be9244ad160
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>>> @@ -0,0 +1,171 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/power/mediatek,power-controller.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Mediatek Power Domains Controller
>>> +
>>> +maintainers:
>>> +  - Weiyi Lu <weiyi.lu@mediatek.com>
>>> +  - Matthias Brugger <mbrugger@suse.com>
>>> +
>>> +description: |
>>> +  Mediatek processors include support for multiple power domains which can be
>>> +  powered up/down by software based on different application scenes to save
>>> power.
>>> +
>>> +  IP cores belonging to a power domain should contain a 'power-domains'
>>> +  property that is a phandle for SCPSYS node representing the domain.
>>> +
>>> +properties:
>>> +  $nodename:
>>> +    pattern: "^syscon@[0-9a-f]+$"
>>> +
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +        - mediatek,mt8173-power-controller
>>> +      - const: syscon
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +patternProperties:
>>> +  "^.*@[0-9]$":
>>
>> Node names should be generic:
>>
>> power-domain@
>>
> 
> Enric correct me if I'm wrong, if we want to see the power domains in debugfs,
> they are listed by their name. If all are called power-domain then the listing
> is pretty much useless.
> 

cc'ing Dafna who might be interested in this discussion.

Yes, It'd be difficult to clearly identify which domain is without looking at
the DT. Now we have

# ls /sys/kernel/debug/pm_genpd
audio  mfg     mfg_async  pm_genpd_summary  vdec  venc_lt
isp    mfg_2d  mm         usb


Actually, I see two "problems" on using a generic name. The first one is that
debugfs uses that name and doesn't allow duplicate names, so we will get a bunch
of errors like this:

debugfs: Directory 'power-domain' with parent 'pm_gendpd' already present!
debugfs: Directory 'power-domain' with parent 'pm_gendpd' already present!
debugfs: Directory 'power-domain' with parent 'pm_gendpd' already present!
...

And we will lost the debug information. However, that's not probably a DT
problem as maybe debugfs should create different names in the form
power-domain@0, power-domain@1, etc.

The second one is what Matthias said, the name exported to the debugfs is
useless. Again, maybe is not a DT problem and the debugfs infra should handle
this cases in a better way, but that's not the case right now.


>>> +    type: object
>>> +    description: |
>>> +      Represents the power domains within the power controller node as
>>> documented
>>> +      in Documentation/devicetree/bindings/power/power-domain.yaml.
>>> +
>>> +    properties:
>>> +      reg:
>>> +        description: |
>>> +          Power domain index. Valid values are defined in:
>>> +              "include/dt-bindings/power/mt8173-power.h" - for MT8173 type
>>> power domain.
>>> +        maxItems: 1
>>> +
>>> +      '#power-domain-cells':
>>> +        description:
>>> +          Documented by the generic PM Domain bindings in
>>> +          Documentation/devicetree/bindings/power/power-domain.yaml.
>>
>> No need to redefine a common property. This should define valid values
>> for it.
>>
>>> +
>>> +      clocks:
>>> +        description: |
>>> +          A number of phandles to clocks that need to be enabled during domain
>>> +          power-up sequencing.
>>
>> No need to redefine 'clocks'. You need to define how many, what each one
>> is, and the order.
>>
> 
> Do you mean we have to define each clock for each power domain of each SoC?
> 
>>> +
>>> +      clock-names:
>>> +        description: |
>>> +          List of names of clocks, in order to match the power-up sequencing
>>> +          for each power domain we need to group the clocks by name. BASIC
>>> +          clocks need to be enabled before enabling the corresponding power
>>> +          domain, and should not have a '-' in their name (i.e mm, mfg, venc).
>>> +          SUSBYS clocks need to be enabled before releasing the bus protection,
>>> +          and should contain a '-' in their name (i.e mm-0, isp-0, cam-0).
>>> +
>>> +          In order to follow properly the power-up sequencing, the clocks must
>>> +          be specified by order, adding first the BASIC clocks followed by the
>>> +          SUSBSYS clocks.
>>
>> You need to define the names.
>>
>>> +
>>> +      mediatek,infracfg:
>>> +        $ref: /schemas/types.yaml#definitions/phandle
>>> +        description: phandle to the device containing the INFRACFG register
>>> range.
>>> +
>>> +      mediatek,smi:
>>> +        $ref: /schemas/types.yaml#definitions/phandle
>>> +        description: phandle to the device containing the SMI register range.
>>> +
>>> +    required:
>>> +      - reg
>>> +      - '#power-domain-cells'
>>> +
>>> +    additionalProperties: false
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/mt8173-clk.h>
>>> +    #include <dt-bindings/power/mt8173-power.h>
>>> +
>>> +    soc {
>>> +        #address-cells = <2>;
>>> +        #size-cells = <2>;
>>> +
>>> +        scpsys: syscon@10006000 {
>>> +            compatible = "mediatek,mt8173-power-controller", "syscon";
> 
> The power domain controller is just one funcionality the SCPSYS block can
> provide. I think it should be child of the SCPSYS.
> 
> Regards,
> Matthias
>
Rob Herring Sept. 22, 2020, 10:36 p.m. UTC | #4
On Mon, Sep 14, 2020 at 2:59 AM Matthias Brugger <matthias.bgg@gmail.com> wrote:
>
>
>
> On 12/09/2020 01:02, Rob Herring wrote:
> > On Thu, Sep 10, 2020 at 07:28:15PM +0200, Enric Balletbo i Serra wrote:
> >> The System Control Processor System (SCPSYS) has several power management
> >> related tasks in the system. Add the bindings to define the power
> >> domains for the SCPSYS power controller.
> >>
> >> Co-developed-by: Matthias Brugger <mbrugger@suse.com>
> >> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> ---
> >> Dear Rob,
> >>
> >> I am awasre that this binding is not ready, but I prefered to send because I'm
> >> kind of blocked. Compiling this binding triggers the following error:
> >>
> >>      mediatek,power-controller.example.dt.yaml: syscon@10006000: mfg_async@7:
> >>      '#address-cells', '#size-cells', 'mfg_2d@8'
> >>      do not match any of the regexes: 'pinctrl-[0-9]+'
> >>
> >> This happens when a definition of a power-domain (parent) contains
> >> another power-domain (child), like the example. I am not sure how to
> >> specify this in the yaml and deal with this, so any clue is welcome.
> >
> > You just have to keep nesting schemas all the way down. Define a
> > grandchild node under the child node and then all of its properties.
> >
> >>
> >> Thanks,
> >>    Enric
> >>
> >>   .../power/mediatek,power-controller.yaml      | 171 ++++++++++++++++++
> >>   1 file changed, 171 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> >> new file mode 100644
> >> index 000000000000..8be9244ad160
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> >> @@ -0,0 +1,171 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/power/mediatek,power-controller.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Mediatek Power Domains Controller
> >> +
> >> +maintainers:
> >> +  - Weiyi Lu <weiyi.lu@mediatek.com>
> >> +  - Matthias Brugger <mbrugger@suse.com>
> >> +
> >> +description: |
> >> +  Mediatek processors include support for multiple power domains which can be
> >> +  powered up/down by software based on different application scenes to save power.
> >> +
> >> +  IP cores belonging to a power domain should contain a 'power-domains'
> >> +  property that is a phandle for SCPSYS node representing the domain.
> >> +
> >> +properties:
> >> +  $nodename:
> >> +    pattern: "^syscon@[0-9a-f]+$"
> >> +
> >> +  compatible:
> >> +    items:
> >> +      - enum:
> >> +        - mediatek,mt8173-power-controller
> >> +      - const: syscon
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +patternProperties:
> >> +  "^.*@[0-9]$":
> >
> > Node names should be generic:
> >
> > power-domain@
> >
>
> Enric correct me if I'm wrong, if we want to see the power domains in debugfs,
> they are listed by their name. If all are called power-domain then the listing
> is pretty much useless.

Sorry, but not a binding problem.

Maybe if debugfs shows what devices are contained within a power
domain then it doesn't matter so much.

> >> +    type: object
> >> +    description: |
> >> +      Represents the power domains within the power controller node as documented
> >> +      in Documentation/devicetree/bindings/power/power-domain.yaml.
> >> +
> >> +    properties:
> >> +      reg:
> >> +        description: |
> >> +          Power domain index. Valid values are defined in:
> >> +              "include/dt-bindings/power/mt8173-power.h" - for MT8173 type power domain.
> >> +        maxItems: 1
> >> +
> >> +      '#power-domain-cells':
> >> +        description:
> >> +          Documented by the generic PM Domain bindings in
> >> +          Documentation/devicetree/bindings/power/power-domain.yaml.
> >
> > No need to redefine a common property. This should define valid values
> > for it.
> >
> >> +
> >> +      clocks:
> >> +        description: |
> >> +          A number of phandles to clocks that need to be enabled during domain
> >> +          power-up sequencing.
> >
> > No need to redefine 'clocks'. You need to define how many, what each one
> > is, and the order.
> >
>
> Do you mean we have to define each clock for each power domain of each SoC?

Yes.

Rob
Enric Balletbo i Serra Sept. 23, 2020, 4:12 p.m. UTC | #5
Hi Rob,

Thank you for your answer. I have some doubts, see below.


On 23/9/20 0:36, Rob Herring wrote:
> On Mon, Sep 14, 2020 at 2:59 AM Matthias Brugger <matthias.bgg@gmail.com> wrote:
>>
>>
>>
>> On 12/09/2020 01:02, Rob Herring wrote:
>>> On Thu, Sep 10, 2020 at 07:28:15PM +0200, Enric Balletbo i Serra wrote:
>>>> The System Control Processor System (SCPSYS) has several power management
>>>> related tasks in the system. Add the bindings to define the power
>>>> domains for the SCPSYS power controller.
>>>>
>>>> Co-developed-by: Matthias Brugger <mbrugger@suse.com>
>>>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>> ---
>>>> Dear Rob,
>>>>
>>>> I am awasre that this binding is not ready, but I prefered to send because I'm
>>>> kind of blocked. Compiling this binding triggers the following error:
>>>>
>>>>      mediatek,power-controller.example.dt.yaml: syscon@10006000: mfg_async@7:
>>>>      '#address-cells', '#size-cells', 'mfg_2d@8'
>>>>      do not match any of the regexes: 'pinctrl-[0-9]+'
>>>>
>>>> This happens when a definition of a power-domain (parent) contains
>>>> another power-domain (child), like the example. I am not sure how to
>>>> specify this in the yaml and deal with this, so any clue is welcome.
>>>
>>> You just have to keep nesting schemas all the way down. Define a
>>> grandchild node under the child node and then all of its properties.
>>>
>>>>
>>>> Thanks,
>>>>    Enric
>>>>
>>>>   .../power/mediatek,power-controller.yaml      | 171 ++++++++++++++++++
>>>>   1 file changed, 171 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>>>> new file mode 100644
>>>> index 000000000000..8be9244ad160
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>>>> @@ -0,0 +1,171 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/power/mediatek,power-controller.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Mediatek Power Domains Controller
>>>> +
>>>> +maintainers:
>>>> +  - Weiyi Lu <weiyi.lu@mediatek.com>
>>>> +  - Matthias Brugger <mbrugger@suse.com>
>>>> +
>>>> +description: |
>>>> +  Mediatek processors include support for multiple power domains which can be
>>>> +  powered up/down by software based on different application scenes to save power.
>>>> +
>>>> +  IP cores belonging to a power domain should contain a 'power-domains'
>>>> +  property that is a phandle for SCPSYS node representing the domain.
>>>> +
>>>> +properties:
>>>> +  $nodename:
>>>> +    pattern: "^syscon@[0-9a-f]+$"
>>>> +
>>>> +  compatible:
>>>> +    items:
>>>> +      - enum:
>>>> +        - mediatek,mt8173-power-controller
>>>> +      - const: syscon
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +patternProperties:
>>>> +  "^.*@[0-9]$":
>>>
>>> Node names should be generic:
>>>
>>> power-domain@
>>>
>>
>> Enric correct me if I'm wrong, if we want to see the power domains in debugfs,
>> they are listed by their name. If all are called power-domain then the listing
>> is pretty much useless.
> 
> Sorry, but not a binding problem.
> 
> Maybe if debugfs shows what devices are contained within a power
> domain then it doesn't matter so much.
> 
>>>> +    type: object
>>>> +    description: |
>>>> +      Represents the power domains within the power controller node as documented
>>>> +      in Documentation/devicetree/bindings/power/power-domain.yaml.
>>>> +
>>>> +    properties:
>>>> +      reg:
>>>> +        description: |
>>>> +          Power domain index. Valid values are defined in:
>>>> +              "include/dt-bindings/power/mt8173-power.h" - for MT8173 type power domain.
>>>> +        maxItems: 1
>>>> +
>>>> +      '#power-domain-cells':
>>>> +        description:
>>>> +          Documented by the generic PM Domain bindings in
>>>> +          Documentation/devicetree/bindings/power/power-domain.yaml.
>>>
>>> No need to redefine a common property. This should define valid values
>>> for it.
>>>
>>>> +
>>>> +      clocks:
>>>> +        description: |
>>>> +          A number of phandles to clocks that need to be enabled during domain
>>>> +          power-up sequencing.
>>>
>>> No need to redefine 'clocks'. You need to define how many, what each one
>>> is, and the order.
>>>
>>
>> Do you mean we have to define each clock for each power domain of each SoC?
> 
> Yes.
> 

But every power domain can use totally different clocks I.e in the scpsys
controller for MT8173 we can have:

clocks:
    items:
      - description: MMSYS reference clock
      - description: Video Encoder reference clock

That matches with:

+            power-domain@MT8173_POWER_DOMAIN_VENC {
+                reg = <MT8173_POWER_DOMAIN_VENC>;
+                clocks = <&topckgen CLK_TOP_MM_SEL>,
+                         <&topckgen CLK_TOP_VENC_SEL>;
+                clock-names = "mm", "venc";
+                #power-domain-cells = <0>;
+            };

But then we can have another power-domain (inside) with another clock.

+            power-domain@MT8173_POWER_DOMAIN_MFG_ASYNC {
+                reg = <MT8173_POWER_DOMAIN_MFG_ASYNC>;
+                clocks = <&clk26m>;
+                clock-names = "mfg";
+            };

Should we add a new item like this then?

      - description: 26MHz reference clock

But clocks will not match with the order for this power-domain (as MMSYS and
VENC clocks are missing here). AFAIK is not possible to specify different clocks
for different power-domains that are inside the SCPSYS power controller node.

For other SoCs we can have something more complex like this:

+	power-domain@MT8183_POWER_DOMAIN_CAM {
+		reg = <MT8183_POWER_DOMAIN_CAM>;
+		clocks = <&topckgen CLK_TOP_MUX_CAM>,
+			 <&camsys CLK_CAM_LARB6>,
+			 <&camsys CLK_CAM_LARB3>,
+			 <&camsys CLK_CAM_SENINF>,
+			 <&camsys CLK_CAM_CAMSV0>,
+			 <&camsys CLK_CAM_CAMSV1>,
+			 <&camsys CLK_CAM_CAMSV2>,
+			 <&camsys CLK_CAM_CCU>;
+		clock-names = "cam", "cam-0", "cam-1",
+			      "cam-2", "cam-3", "cam-4",
+			      "cam-5", "cam-6";
+		mediatek,infracfg = <&infracfg>;
+		mediatek,smi = <&smi_common>;
+		#power-domain-cells = <0>;
+	};

Thanks,
  Enric


> Rob
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
new file mode 100644
index 000000000000..8be9244ad160
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
@@ -0,0 +1,171 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/mediatek,power-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mediatek Power Domains Controller
+
+maintainers:
+  - Weiyi Lu <weiyi.lu@mediatek.com>
+  - Matthias Brugger <mbrugger@suse.com>
+
+description: |
+  Mediatek processors include support for multiple power domains which can be
+  powered up/down by software based on different application scenes to save power.
+
+  IP cores belonging to a power domain should contain a 'power-domains'
+  property that is a phandle for SCPSYS node representing the domain.
+
+properties:
+  $nodename:
+    pattern: "^syscon@[0-9a-f]+$"
+
+  compatible:
+    items:
+      - enum:
+        - mediatek,mt8173-power-controller
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+patternProperties:
+  "^.*@[0-9]$":
+    type: object
+    description: |
+      Represents the power domains within the power controller node as documented
+      in Documentation/devicetree/bindings/power/power-domain.yaml.
+
+    properties:
+      reg:
+        description: |
+          Power domain index. Valid values are defined in:
+              "include/dt-bindings/power/mt8173-power.h" - for MT8173 type power domain.
+        maxItems: 1
+
+      '#power-domain-cells':
+        description:
+          Documented by the generic PM Domain bindings in
+          Documentation/devicetree/bindings/power/power-domain.yaml.
+
+      clocks:
+        description: |
+          A number of phandles to clocks that need to be enabled during domain
+          power-up sequencing.
+
+      clock-names:
+        description: |
+          List of names of clocks, in order to match the power-up sequencing
+          for each power domain we need to group the clocks by name. BASIC
+          clocks need to be enabled before enabling the corresponding power
+          domain, and should not have a '-' in their name (i.e mm, mfg, venc).
+          SUSBYS clocks need to be enabled before releasing the bus protection,
+          and should contain a '-' in their name (i.e mm-0, isp-0, cam-0).
+
+          In order to follow properly the power-up sequencing, the clocks must
+          be specified by order, adding first the BASIC clocks followed by the
+          SUSBSYS clocks.
+
+      mediatek,infracfg:
+        $ref: /schemas/types.yaml#definitions/phandle
+        description: phandle to the device containing the INFRACFG register range.
+
+      mediatek,smi:
+        $ref: /schemas/types.yaml#definitions/phandle
+        description: phandle to the device containing the SMI register range.
+
+    required:
+      - reg
+      - '#power-domain-cells'
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/mt8173-clk.h>
+    #include <dt-bindings/power/mt8173-power.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        scpsys: syscon@10006000 {
+            compatible = "mediatek,mt8173-power-controller", "syscon";
+            reg = <0 0x10006000 0 0x1000>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            /* power domains of the SoC */
+            vdec@MT8173_POWER_DOMAIN_VDEC {
+                reg = <MT8173_POWER_DOMAIN_VDEC>;
+                clocks = <&topckgen CLK_TOP_MM_SEL>;
+                clock-names = "mm";
+                #power-domain-cells = <0>;
+            };
+
+            venc@MT8173_POWER_DOMAIN_VENC {
+                reg = <MT8173_POWER_DOMAIN_VENC>;
+                clocks = <&topckgen CLK_TOP_MM_SEL>,
+                         <&topckgen CLK_TOP_VENC_SEL>;
+                clock-names = "mm", "venc";
+                #power-domain-cells = <0>;
+            };
+            isp@MT8173_POWER_DOMAIN_ISP {
+                reg = <MT8173_POWER_DOMAIN_ISP>;
+                clocks = <&topckgen CLK_TOP_MM_SEL>;
+                clock-names = "mm";
+                #power-domain-cells = <0>;
+            };
+            mm@MT8173_POWER_DOMAIN_MM {
+                reg = <MT8173_POWER_DOMAIN_MM>;
+                clocks = <&topckgen CLK_TOP_MM_SEL>;
+                clock-names = "mm";
+                #power-domain-cells = <0>;
+                mediatek,infracfg = <&infracfg>;
+            };
+            venc_lt@MT8173_POWER_DOMAIN_VENC_LT {
+                reg = <MT8173_POWER_DOMAIN_VENC_LT>;
+                clocks = <&topckgen CLK_TOP_MM_SEL>,
+                         <&topckgen CLK_TOP_VENC_LT_SEL>;
+                clock-names = "mm", "venclt";
+                #power-domain-cells = <0>;
+            };
+            audio@MT8173_POWER_DOMAIN_AUDIO {
+                reg = <MT8173_POWER_DOMAIN_AUDIO>;
+                #power-domain-cells = <0>;
+            };
+            usb@MT8173_POWER_DOMAIN_USB {
+                reg = <MT8173_POWER_DOMAIN_USB>;
+                #power-domain-cells = <0>;
+            };
+            mfg_async@MT8173_POWER_DOMAIN_MFG_ASYNC {
+                reg = <MT8173_POWER_DOMAIN_MFG_ASYNC>;
+                clocks = <&clk26m>;
+                clock-names = "mfg";
+                #address-cells = <1>;
+                #size-cells = <0>;
+                #power-domain-cells = <1>;
+
+                mfg_2d@MT8173_POWER_DOMAIN_MFG_2D {
+                    reg = <MT8173_POWER_DOMAIN_MFG_2D>;
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+                    #power-domain-cells = <1>;
+
+                    mfg@MT8173_POWER_DOMAIN_MFG {
+                        reg = <MT8173_POWER_DOMAIN_MFG>;
+                        #power-domain-cells = <0>;
+                        mediatek,infracfg = <&infracfg>;
+                    };
+                };
+            };
+        };
+    };