[PATCH 0/3] mtk-pm-domains: Use 'syscon' phandle for SCPSYS regmap

[PATCH 0/3] mtk-pm-domains: Use 'syscon' phandle for SCPSYS regmap

[AngeloGioacchino Del Regno]

After an attempt by MediaTek to upstream MT8186, MT8192 and MT8195
devicetrees, Krzysztof brought up a discussion in which it emerged
that the usage of a 'simple-mfd' device is not an optimal choice for
a number of reasons [1].

Reading that made me just write this simple code to stop nesting the
SPM power-controller node inside of a "syscon", "simple-mfd" node,
also allowing to move it outside of the main soc bus.

[1]: https://patchwork.kernel.org/project/linux-mediatek/patch/20220704100028.19932-9-tinghan.shen@mediatek.com/

AngeloGioacchino Del Regno (3):
  dt-bindings: power: mediatek: Document phandle to SCPSYS syscon node
  dt-bindings: power: mediatek: Update example to use phandle to syscon
  soc: mediatek: pm-domains: Grab SCPSYS registers from phandle to
    syscon

 .../power/mediatek,power-controller.yaml      | 130 +++++++++---------
 drivers/soc/mediatek/mtk-pm-domains.c         |  16 ++-
 2 files changed, 77 insertions(+), 69 deletions(-)

-- 
2.35.1

[PATCH 1/3] dt-bindings: power: mediatek: Document phandle to SCPSYS syscon node

[AngeloGioacchino Del Regno]

Add a phandle to the syscon block providing access to SCPSYS registers:
this allows us to avoid using simple-mfd for the SCPSYS node and
nesting the System Power Manager node inside.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../devicetree/bindings/power/mediatek,power-controller.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
index 135c6f722091..848fdff7c9d8 100644
--- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
+++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
@@ -39,6 +39,11 @@ properties:
   '#size-cells':
     const: 0
 
+  syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the syscon block providing access to SCPSYS registers
+
 patternProperties:
   "^power-domain@[0-9a-f]+$":
     type: object
-- 
2.35.1

Re: [PATCH 1/3] dt-bindings: power: mediatek: Document phandle to SCPSYS syscon node

[Krzysztof Kozlowski]

On 11/07/2022 14:25, AngeloGioacchino Del Regno wrote:
> Add a phandle to the syscon block providing access to SCPSYS registers:
> this allows us to avoid using simple-mfd for the SCPSYS node and
> nesting the System Power Manager node inside.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../devicetree/bindings/power/mediatek,power-controller.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> index 135c6f722091..848fdff7c9d8 100644
> --- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> @@ -39,6 +39,11 @@ properties:
>    '#size-cells':
>      const: 0
>  
> +  syscon:

This is not a generic property. You need vendor prefix and property name
(can be followed by -syscon).

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the syscon block providing access to SCPSYS registers
> +
>  patternProperties:
>    "^power-domain@[0-9a-f]+$":
>      type: object


Best regards,
Krzysztof

[PATCH 2/3] dt-bindings: power: mediatek: Update example to use phandle to syscon

[AngeloGioacchino Del Regno]

The preferred way of declaring this node is by using a phandle to
syscon: update the example to reflect that.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../power/mediatek,power-controller.yaml      | 125 +++++++++---------
 1 file changed, 63 insertions(+), 62 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
index 848fdff7c9d8..bed059e4401d 100644
--- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
+++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
@@ -237,76 +237,77 @@ examples:
         scpsys: syscon@10006000 {
             compatible = "syscon", "simple-mfd";
             reg = <0 0x10006000 0 0x1000>;
+        };
+    };
 
-            spm: power-controller {
-                compatible = "mediatek,mt8173-power-controller";
+    spm: power-controller {
+        compatible = "mediatek,mt8173-power-controller";
+        #address-cells = <1>;
+        #size-cells = <0>;
+        #power-domain-cells = <1>;
+        syscon = <&scpsys>;
+
+        /* power domains of the SoC */
+        power-domain@MT8173_POWER_DOMAIN_VDEC {
+            reg = <MT8173_POWER_DOMAIN_VDEC>;
+            clocks = <&topckgen CLK_TOP_MM_SEL>;
+            clock-names = "mm";
+            #power-domain-cells = <0>;
+        };
+        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>;
+        };
+        power-domain@MT8173_POWER_DOMAIN_ISP {
+            reg = <MT8173_POWER_DOMAIN_ISP>;
+            clocks = <&topckgen CLK_TOP_MM_SEL>;
+            clock-names = "mm";
+            #power-domain-cells = <0>;
+        };
+        power-domain@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>;
+        };
+        power-domain@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>;
+        };
+        power-domain@MT8173_POWER_DOMAIN_AUDIO {
+            reg = <MT8173_POWER_DOMAIN_AUDIO>;
+            #power-domain-cells = <0>;
+        };
+        power-domain@MT8173_POWER_DOMAIN_USB {
+            reg = <MT8173_POWER_DOMAIN_USB>;
+            #power-domain-cells = <0>;
+        };
+        power-domain@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>;
+
+            power-domain@MT8173_POWER_DOMAIN_MFG_2D {
+                reg = <MT8173_POWER_DOMAIN_MFG_2D>;
                 #address-cells = <1>;
                 #size-cells = <0>;
                 #power-domain-cells = <1>;
 
-                /* power domains of the SoC */
-                power-domain@MT8173_POWER_DOMAIN_VDEC {
-                    reg = <MT8173_POWER_DOMAIN_VDEC>;
-                    clocks = <&topckgen CLK_TOP_MM_SEL>;
-                    clock-names = "mm";
-                    #power-domain-cells = <0>;
-                };
-                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>;
-                };
-                power-domain@MT8173_POWER_DOMAIN_ISP {
-                    reg = <MT8173_POWER_DOMAIN_ISP>;
-                    clocks = <&topckgen CLK_TOP_MM_SEL>;
-                    clock-names = "mm";
-                    #power-domain-cells = <0>;
-                };
-                power-domain@MT8173_POWER_DOMAIN_MM {
-                    reg = <MT8173_POWER_DOMAIN_MM>;
-                    clocks = <&topckgen CLK_TOP_MM_SEL>;
-                    clock-names = "mm";
+                power-domain@MT8173_POWER_DOMAIN_MFG {
+                    reg = <MT8173_POWER_DOMAIN_MFG>;
                     #power-domain-cells = <0>;
                     mediatek,infracfg = <&infracfg>;
                 };
-                power-domain@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>;
-                };
-                power-domain@MT8173_POWER_DOMAIN_AUDIO {
-                    reg = <MT8173_POWER_DOMAIN_AUDIO>;
-                    #power-domain-cells = <0>;
-                };
-                power-domain@MT8173_POWER_DOMAIN_USB {
-                    reg = <MT8173_POWER_DOMAIN_USB>;
-                    #power-domain-cells = <0>;
-                };
-                power-domain@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>;
-
-                    power-domain@MT8173_POWER_DOMAIN_MFG_2D {
-                        reg = <MT8173_POWER_DOMAIN_MFG_2D>;
-                        #address-cells = <1>;
-                        #size-cells = <0>;
-                        #power-domain-cells = <1>;
-
-                        power-domain@MT8173_POWER_DOMAIN_MFG {
-                            reg = <MT8173_POWER_DOMAIN_MFG>;
-                            #power-domain-cells = <0>;
-                            mediatek,infracfg = <&infracfg>;
-                        };
-                    };
-                };
             };
         };
     };
-- 
2.35.1

Re: [PATCH 2/3] dt-bindings: power: mediatek: Update example to use phandle to syscon

[Rob Herring]

On Mon, Jul 11, 2022 at 02:25:02PM +0200, AngeloGioacchino Del Regno wrote:
> The preferred way of declaring this node is by using a phandle to
> syscon: update the example to reflect that.

Preferred by who? Not me. 
 
What problem are you trying to solve? Better be a good reason for 
breaking compatibility.


> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../power/mediatek,power-controller.yaml      | 125 +++++++++---------
>  1 file changed, 63 insertions(+), 62 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> index 848fdff7c9d8..bed059e4401d 100644
> --- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> @@ -237,76 +237,77 @@ examples:
>          scpsys: syscon@10006000 {
>              compatible = "syscon", "simple-mfd";

Only generic compatibles is certainly not preferred.

>              reg = <0 0x10006000 0 0x1000>;
> +        };
> +    };
>  
> -            spm: power-controller {
> -                compatible = "mediatek,mt8173-power-controller";
> +    spm: power-controller {
> +        compatible = "mediatek,mt8173-power-controller";
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        #power-domain-cells = <1>;
> +        syscon = <&scpsys>;
> +
> +        /* power domains of the SoC */
> +        power-domain@MT8173_POWER_DOMAIN_VDEC {
> +            reg = <MT8173_POWER_DOMAIN_VDEC>;
> +            clocks = <&topckgen CLK_TOP_MM_SEL>;
> +            clock-names = "mm";
> +            #power-domain-cells = <0>;
> +        };
> +        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>;
> +        };
> +        power-domain@MT8173_POWER_DOMAIN_ISP {
> +            reg = <MT8173_POWER_DOMAIN_ISP>;
> +            clocks = <&topckgen CLK_TOP_MM_SEL>;
> +            clock-names = "mm";
> +            #power-domain-cells = <0>;
> +        };
> +        power-domain@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>;
> +        };
> +        power-domain@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>;
> +        };
> +        power-domain@MT8173_POWER_DOMAIN_AUDIO {
> +            reg = <MT8173_POWER_DOMAIN_AUDIO>;
> +            #power-domain-cells = <0>;
> +        };
> +        power-domain@MT8173_POWER_DOMAIN_USB {
> +            reg = <MT8173_POWER_DOMAIN_USB>;
> +            #power-domain-cells = <0>;
> +        };
> +        power-domain@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>;
> +
> +            power-domain@MT8173_POWER_DOMAIN_MFG_2D {
> +                reg = <MT8173_POWER_DOMAIN_MFG_2D>;
>                  #address-cells = <1>;
>                  #size-cells = <0>;
>                  #power-domain-cells = <1>;
>  
> -                /* power domains of the SoC */
> -                power-domain@MT8173_POWER_DOMAIN_VDEC {
> -                    reg = <MT8173_POWER_DOMAIN_VDEC>;
> -                    clocks = <&topckgen CLK_TOP_MM_SEL>;
> -                    clock-names = "mm";
> -                    #power-domain-cells = <0>;
> -                };
> -                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>;
> -                };
> -                power-domain@MT8173_POWER_DOMAIN_ISP {
> -                    reg = <MT8173_POWER_DOMAIN_ISP>;
> -                    clocks = <&topckgen CLK_TOP_MM_SEL>;
> -                    clock-names = "mm";
> -                    #power-domain-cells = <0>;
> -                };
> -                power-domain@MT8173_POWER_DOMAIN_MM {
> -                    reg = <MT8173_POWER_DOMAIN_MM>;
> -                    clocks = <&topckgen CLK_TOP_MM_SEL>;
> -                    clock-names = "mm";
> +                power-domain@MT8173_POWER_DOMAIN_MFG {
> +                    reg = <MT8173_POWER_DOMAIN_MFG>;
>                      #power-domain-cells = <0>;
>                      mediatek,infracfg = <&infracfg>;
>                  };
> -                power-domain@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>;
> -                };
> -                power-domain@MT8173_POWER_DOMAIN_AUDIO {
> -                    reg = <MT8173_POWER_DOMAIN_AUDIO>;
> -                    #power-domain-cells = <0>;
> -                };
> -                power-domain@MT8173_POWER_DOMAIN_USB {
> -                    reg = <MT8173_POWER_DOMAIN_USB>;
> -                    #power-domain-cells = <0>;
> -                };
> -                power-domain@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>;
> -
> -                    power-domain@MT8173_POWER_DOMAIN_MFG_2D {
> -                        reg = <MT8173_POWER_DOMAIN_MFG_2D>;
> -                        #address-cells = <1>;
> -                        #size-cells = <0>;
> -                        #power-domain-cells = <1>;
> -
> -                        power-domain@MT8173_POWER_DOMAIN_MFG {
> -                            reg = <MT8173_POWER_DOMAIN_MFG>;
> -                            #power-domain-cells = <0>;
> -                            mediatek,infracfg = <&infracfg>;
> -                        };
> -                    };
> -                };
>              };
>          };
>      };
> -- 
> 2.35.1
> 
> 

Re: [PATCH 2/3] dt-bindings: power: mediatek: Update example to use phandle to syscon

[AngeloGioacchino Del Regno]

Il 18/07/22 20:06, Rob Herring ha scritto:
> On Mon, Jul 11, 2022 at 02:25:02PM +0200, AngeloGioacchino Del Regno wrote:
>> The preferred way of declaring this node is by using a phandle to
>> syscon: update the example to reflect that.
> 
> Preferred by who? Not me.
>   
> What problem are you trying to solve? Better be a good reason for
> breaking compatibility.
> 
> 

Hello Rob,

I've produced this series after a misunderstanding with Krzysztof (entirely my
bad!), then the discussion [1] went on and I'm handing it to MediaTek to resolve.

Please ignore this series.


[1]: 
https://patchwork.kernel.org/project/linux-mediatek/patch/20220704100028.19932-9-tinghan.shen@mediatek.com/

Regards,
Angelo

[PATCH 3/3] soc: mediatek: pm-domains: Grab SCPSYS registers from phandle to syscon

[AngeloGioacchino Del Regno]

Instead of requiring nesting of the power-controller inside of a
"syscon", "simple-mfd" node, look for a phandle to SCPSYS in the
"syscon" property of the power controller node.

Compatibility with older devicetrees is retained by falling back
to looking for a parent node if no syscon phandle is found.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-pm-domains.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
index a3dae391a38a..c5a1c766cd50 100644
--- a/drivers/soc/mediatek/mtk-pm-domains.c
+++ b/drivers/soc/mediatek/mtk-pm-domains.c
@@ -590,8 +590,7 @@ static int scpsys_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	const struct scpsys_soc_data *soc;
-	struct device_node *node;
-	struct device *parent;
+	struct device_node *node, *syscon;
 	struct scpsys *scpsys;
 	int ret;
 
@@ -611,13 +610,16 @@ static int scpsys_probe(struct platform_device *pdev)
 	scpsys->pd_data.domains = scpsys->domains;
 	scpsys->pd_data.num_domains = soc->num_domains;
 
-	parent = dev->parent;
-	if (!parent) {
-		dev_err(dev, "no parent for syscon devices\n");
-		return -ENODEV;
+	syscon = of_parse_phandle(dev->of_node, "syscon", 0);
+	if (!syscon) {
+		if (!dev->parent)
+			return -ENODEV;
+		scpsys->base = syscon_node_to_regmap(dev->parent->of_node);
+	} else {
+		scpsys->base = syscon_node_to_regmap(syscon);
+		of_node_put(syscon);
 	}
 
-	scpsys->base = syscon_node_to_regmap(parent->of_node);
 	if (IS_ERR(scpsys->base)) {
 		dev_err(dev, "no regmap available\n");
 		return PTR_ERR(scpsys->base);
-- 
2.35.1

Re: [PATCH 3/3] soc: mediatek: pm-domains: Grab SCPSYS registers from phandle to syscon

[Krzysztof Kozlowski]

On 11/07/2022 14:25, AngeloGioacchino Del Regno wrote:
> Instead of requiring nesting of the power-controller inside of a
> "syscon", "simple-mfd" node, look for a phandle to SCPSYS in the
> "syscon" property of the power controller node.
> 
> Compatibility with older devicetrees is retained by falling back
> to looking for a parent node if no syscon phandle is found.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/soc/mediatek/mtk-pm-domains.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
> index a3dae391a38a..c5a1c766cd50 100644
> --- a/drivers/soc/mediatek/mtk-pm-domains.c
> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
> @@ -590,8 +590,7 @@ static int scpsys_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	const struct scpsys_soc_data *soc;
> -	struct device_node *node;
> -	struct device *parent;
> +	struct device_node *node, *syscon;
>  	struct scpsys *scpsys;
>  	int ret;
>  
> @@ -611,13 +610,16 @@ static int scpsys_probe(struct platform_device *pdev)
>  	scpsys->pd_data.domains = scpsys->domains;
>  	scpsys->pd_data.num_domains = soc->num_domains;
>  
> -	parent = dev->parent;
> -	if (!parent) {
> -		dev_err(dev, "no parent for syscon devices\n");
> -		return -ENODEV;
> +	syscon = of_parse_phandle(dev->of_node, "syscon", 0);
> +	if (!syscon) {
> +		if (!dev->parent)
> +			return -ENODEV;
> +		scpsys->base = syscon_node_to_regmap(dev->parent->of_node);
> +	} else {
> +		scpsys->base = syscon_node_to_regmap(syscon);
> +		of_node_put(syscon);
>  	}

I responded in previous DTS thread. Technically code looks ok, but it
does not make really sense - either this is a child or a sibling. Not
both for the same hardware!

Best regards,
Krzysztof