| Message ID | 20200910172826.3074357-2-enric.balletbo@collabora.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | soc: mediatek: pm-domains: Add new driver for SCPSYS power domains controller | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
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 >
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
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 >
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
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 --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>; + }; + }; + }; + }; + };