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

From: Matthias Brugger
Date: Mon Sep 14 2020 - 04:59:45 EST




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@xxxxxxxx>
Signed-off-by: Matthias Brugger <mbrugger@xxxxxxxx>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
---
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@xxxxxxxxxxxx>
+ - Matthias Brugger <mbrugger@xxxxxxxx>
+
+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