diff mbox series

[v5,1/3] dt-bindings: soc: add mtk svs dt-bindings

Message ID 20190906100514.30803-2-roger.lu@mediatek.com (mailing list archive)
State New, archived
Headers show
Series PM / AVS: SVS: Introduce SVS engine | expand

Commit Message

Roger Lu Sept. 6, 2019, 10:05 a.m. UTC 
Document the binding for enabling mtk svs on MediaTek SoC.

Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 .../devicetree/bindings/power/mtk-svs.txt     | 88 +++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/mtk-svs.txt

Comments

Rob Herring (Arm) Sept. 30, 2019, 1:35 p.m. UTC | #1 
On Fri, Sep 06, 2019 at 06:05:13PM +0800, Roger Lu wrote:
> Document the binding for enabling mtk svs on MediaTek SoC.
> 
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> ---
>  .../devicetree/bindings/power/mtk-svs.txt     | 88 +++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/mtk-svs.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/mtk-svs.txt b/Documentation/devicetree/bindings/power/mtk-svs.txt
> new file mode 100644
> index 000000000000..6a71992ef162
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/mtk-svs.txt
> @@ -0,0 +1,88 @@
> +* Mediatek Smart Voltage Scaling (MTK SVS)
> +
> +This describes the device tree binding for the MTK SVS controller (bank)
> +which helps provide the optimized CPU/GPU/CCI voltages. This device also
> +needs thermal data to calculate thermal slope for accurately compensate
> +the voltages when temperature change.
> +
> +Required properties:
> +- compatible:
> +  - "mediatek,mt8183-svs" : For MT8183 family of SoCs
> +- reg: Address range of the MTK SVS controller.
> +- interrupts: IRQ for the MTK SVS controller.
> +- clocks, clock-names: Clocks needed for the svs controller. required
> +                       clocks are:
> +		       "main_clk": Main clock needed for register access

'_clk' is redundant.

> +- nvmem-cells: Phandle to the calibration data provided by a nvmem device.
> +- nvmem-cell-names: Should be "svs-calibration-data" and "calibration-data"
> +
> +Subnodes:
> +- svs_cpu_little: SVS bank device node of little CPU
> +  compatible: "mediatek,mt8183-svs-cpu-little"
> +  operating-points-v2: OPP table hooked by SVS little CPU bank.
> +		       SVS will optimze this OPP table voltage part.
> +  vcpu-little-supply: PMIC buck of little CPU
> +- svs_cpu_big: SVS bank device node of big CPU
> +  compatible: "mediatek,mt8183-svs-cpu-big"
> +  operating-points-v2: OPP table hooked by SVS big CPU bank.
> +		       SVS will optimze this OPP table voltage part.
> +  vcpu-big-supply: PMIC buck of big CPU
> +- svs_cci: SVS bank device node of CCI
> +  compatible: "mediatek,mt8183-svs-cci"
> +  operating-points-v2: OPP table hooked by SVS CCI bank.
> +		       SVS will optimze this OPP table voltage part.
> +  vcci-supply: PMIC buck of CCI
> +- svs_gpu: SVS bank device node of GPU
> +  compatible: "mediatek,mt8183-svs-gpu"
> +  operating-points-v2: OPP table hooked by SVS GPU bank.
> +		       SVS will optimze this OPP table voltage part.
> +  vgpu-spply: PMIC buck of GPU
> +
> +Example:
> +
> +	svs: svs@1100b000 {
> +		compatible = "mediatek,mt8183-svs";
> +		reg = <0 0x1100b000 0 0x1000>;
> +		interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_LOW 0>;

GIC interrupts are 3 cells, you have 4.

> +		clocks = <&infracfg CLK_INFRA_THERM>;
> +		clock-names = "main_clk";
> +		nvmem-cells = <&svs_calibration>, <&thermal_calibration>;
> +		nvmem-cell-names = "svs-calibration-data", "calibration-data";
> +
> +		svs_cpu_little: svs_cpu_little {

Don't use '_' in node names.

> +			compatible = "mediatek,mt8183-svs-cpu-little";
> +			operating-points-v2 = <&cluster0_opp>;
> +		};
> +
> +		svs_cpu_big: svs_cpu_big {
> +			compatible = "mediatek,mt8183-svs-cpu-big";
> +			operating-points-v2 = <&cluster1_opp>;
> +		};
> +
> +		svs_cci: svs_cci {
> +			compatible = "mediatek,mt8183-svs-cci";
> +			operating-points-v2 = <&cci_opp>;
> +		};
> +
> +		svs_gpu: svs_gpu {
> +			compatible = "mediatek,mt8183-svs-gpu";
> +			power-domains = <&scpsys MT8183_POWER_DOMAIN_MFG_2D>;
> +			operating-points-v2 = <&gpu_opp_table>;
> +		};
> +	};
> +
> +	&svs_cpu_little {
> +		vcpu-little-supply = <&mt6358_vproc12_reg>;

It's already defined to have OPP and supply in the cpu nodes. Parse them
to get this information rather than duplicating it here.

The same should apply to the CCI and GPU.

Rob
Roger Lu Dec. 27, 2019, 6:50 a.m. UTC | #2 
Dear Rob,

Sorry for the late reply.

On Mon, 2019-09-30 at 08:35 -0500, Rob Herring wrote:
> On Fri, Sep 06, 2019 at 06:05:13PM +0800, Roger Lu wrote:
> > Document the binding for enabling mtk svs on MediaTek SoC.
> > 
> > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> > ---
> >  .../devicetree/bindings/power/mtk-svs.txt     | 88 +++++++++++++++++++
> >  1 file changed, 88 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/mtk-svs.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/power/mtk-svs.txt b/Documentation/devicetree/bindings/power/mtk-svs.txt
> > new file mode 100644
> > index 000000000000..6a71992ef162
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/mtk-svs.txt
> > @@ -0,0 +1,88 @@
> > +* Mediatek Smart Voltage Scaling (MTK SVS)
> > +
> > +This describes the device tree binding for the MTK SVS controller (bank)
> > +which helps provide the optimized CPU/GPU/CCI voltages. This device also
> > +needs thermal data to calculate thermal slope for accurately compensate
> > +the voltages when temperature change.
> > +
> > +Required properties:
> > +- compatible:
> > +  - "mediatek,mt8183-svs" : For MT8183 family of SoCs
> > +- reg: Address range of the MTK SVS controller.
> > +- interrupts: IRQ for the MTK SVS controller.
> > +- clocks, clock-names: Clocks needed for the svs controller. required
> > +                       clocks are:
> > +		       "main_clk": Main clock needed for register access
> 
> '_clk' is redundant.

Oh Okay. I'll remove _clk. Thanks.

> 
> > +- nvmem-cells: Phandle to the calibration data provided by a nvmem device.
> > +- nvmem-cell-names: Should be "svs-calibration-data" and "calibration-data"
> > +
> > +Subnodes:
> > +- svs_cpu_little: SVS bank device node of little CPU
> > +  compatible: "mediatek,mt8183-svs-cpu-little"
> > +  operating-points-v2: OPP table hooked by SVS little CPU bank.
> > +		       SVS will optimze this OPP table voltage part.
> > +  vcpu-little-supply: PMIC buck of little CPU
> > +- svs_cpu_big: SVS bank device node of big CPU
> > +  compatible: "mediatek,mt8183-svs-cpu-big"
> > +  operating-points-v2: OPP table hooked by SVS big CPU bank.
> > +		       SVS will optimze this OPP table voltage part.
> > +  vcpu-big-supply: PMIC buck of big CPU
> > +- svs_cci: SVS bank device node of CCI
> > +  compatible: "mediatek,mt8183-svs-cci"
> > +  operating-points-v2: OPP table hooked by SVS CCI bank.
> > +		       SVS will optimze this OPP table voltage part.
> > +  vcci-supply: PMIC buck of CCI
> > +- svs_gpu: SVS bank device node of GPU
> > +  compatible: "mediatek,mt8183-svs-gpu"
> > +  operating-points-v2: OPP table hooked by SVS GPU bank.
> > +		       SVS will optimze this OPP table voltage part.
> > +  vgpu-spply: PMIC buck of GPU
> > +
> > +Example:
> > +
> > +	svs: svs@1100b000 {
> > +		compatible = "mediatek,mt8183-svs";
> > +		reg = <0 0x1100b000 0 0x1000>;
> > +		interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_LOW 0>;
> 
> GIC interrupts are 3 cells, you have 4.

Oops, I'll remove the fourth parameter. Thanks a lot.

> 
> > +		clocks = <&infracfg CLK_INFRA_THERM>;
> > +		clock-names = "main_clk";
> > +		nvmem-cells = <&svs_calibration>, <&thermal_calibration>;
> > +		nvmem-cell-names = "svs-calibration-data", "calibration-data";
> > +
> > +		svs_cpu_little: svs_cpu_little {
> 
> Don't use '_' in node names.

Okay. I'll replace it with '-'. Thanks.

> 
> > +			compatible = "mediatek,mt8183-svs-cpu-little";
> > +			operating-points-v2 = <&cluster0_opp>;
> > +		};
> > +
> > +		svs_cpu_big: svs_cpu_big {
> > +			compatible = "mediatek,mt8183-svs-cpu-big";
> > +			operating-points-v2 = <&cluster1_opp>;
> > +		};
> > +
> > +		svs_cci: svs_cci {
> > +			compatible = "mediatek,mt8183-svs-cci";
> > +			operating-points-v2 = <&cci_opp>;
> > +		};
> > +
> > +		svs_gpu: svs_gpu {
> > +			compatible = "mediatek,mt8183-svs-gpu";
> > +			power-domains = <&scpsys MT8183_POWER_DOMAIN_MFG_2D>;
> > +			operating-points-v2 = <&gpu_opp_table>;
> > +		};
> > +	};
> > +
> > +	&svs_cpu_little {
> > +		vcpu-little-supply = <&mt6358_vproc12_reg>;
> 
> It's already defined to have OPP and supply in the cpu nodes. Parse them
> to get this information rather than duplicating it here.
> 
> The same should apply to the CCI and GPU.

Please let me explain the reason why I add SVS sub-nodes. I ever try to
parse other nodes to get desired power-domains/OPP table. However, it
makes SVS driver harder to develop and maintain.

1. When a SVS-controller-init wants GPU_CORE0's OPP table in one node
but it needs power-domains(GPU_MFG_2D) in another node, it becomes
complicated and confusing when SVS sub-node tries to parse many nodes.
Therefore, we want SVS sub-node to focus on what SVS bank requires by
how we do in this patch.

2. In hardware point of view, SVS controller depends on other hardware's
power only. All the SVS controller registers are in SVS hardware. So, we
think It's good that SVS sub-node describes what SVS controller requires
instead of linking other subsys nodes and parse the property that SVS
controller needs.

3. We want SVS driver to have a generic way to attain subsys device for
using "pm_runtime and OPP framework" API. If SVS driver tries to parse
CPU(little/big core) and other subsys device node(e.g cci/gpu), it means
SVS driver has to maintain different methodologies(cpu-specific?
devfreq? others?) in order to get CPU(little/big core) and other subsys
device(e.g cci/gpu) for using "pm_runtime and OPP framework" API.

> 
> Rob

Sincerely,
Roger Lu.
Nicolas Boichat Jan. 13, 2020, 7:02 a.m. UTC | #3 
On Fri, Dec 27, 2019 at 2:51 PM Roger Lu <roger.lu@mediatek.com> wrote:
>
> Dear Rob,
>
> Sorry for the late reply.
>
> On Mon, 2019-09-30 at 08:35 -0500, Rob Herring wrote:
> > On Fri, Sep 06, 2019 at 06:05:13PM +0800, Roger Lu wrote:
> > > Document the binding for enabling mtk svs on MediaTek SoC.
> > >
> > > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> > > ---
> > >  .../devicetree/bindings/power/mtk-svs.txt     | 88 +++++++++++++++++++
> > >  1 file changed, 88 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/power/mtk-svs.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/power/mtk-svs.txt b/Documentation/devicetree/bindings/power/mtk-svs.txt
> > > new file mode 100644
> > > index 000000000000..6a71992ef162
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/power/mtk-svs.txt
> > > @@ -0,0 +1,88 @@
> > > +* Mediatek Smart Voltage Scaling (MTK SVS)
> > > +
> > > +This describes the device tree binding for the MTK SVS controller (bank)
> > > +which helps provide the optimized CPU/GPU/CCI voltages. This device also
> > > +needs thermal data to calculate thermal slope for accurately compensate
> > > +the voltages when temperature change.
> > > +
> > > +Required properties:
> > > +- compatible:
> > > +  - "mediatek,mt8183-svs" : For MT8183 family of SoCs
> > > +- reg: Address range of the MTK SVS controller.
> > > +- interrupts: IRQ for the MTK SVS controller.
> > > +- clocks, clock-names: Clocks needed for the svs controller. required
> > > +                       clocks are:
> > > +                  "main_clk": Main clock needed for register access
> >
> > '_clk' is redundant.
>
> Oh Okay. I'll remove _clk. Thanks.
>
> >
> > > +- nvmem-cells: Phandle to the calibration data provided by a nvmem device.
> > > +- nvmem-cell-names: Should be "svs-calibration-data" and "calibration-data"
> > > +
> > > +Subnodes:
> > > +- svs_cpu_little: SVS bank device node of little CPU
> > > +  compatible: "mediatek,mt8183-svs-cpu-little"
> > > +  operating-points-v2: OPP table hooked by SVS little CPU bank.
> > > +                  SVS will optimze this OPP table voltage part.
> > > +  vcpu-little-supply: PMIC buck of little CPU
> > > +- svs_cpu_big: SVS bank device node of big CPU
> > > +  compatible: "mediatek,mt8183-svs-cpu-big"
> > > +  operating-points-v2: OPP table hooked by SVS big CPU bank.
> > > +                  SVS will optimze this OPP table voltage part.
> > > +  vcpu-big-supply: PMIC buck of big CPU
> > > +- svs_cci: SVS bank device node of CCI
> > > +  compatible: "mediatek,mt8183-svs-cci"
> > > +  operating-points-v2: OPP table hooked by SVS CCI bank.
> > > +                  SVS will optimze this OPP table voltage part.
> > > +  vcci-supply: PMIC buck of CCI
> > > +- svs_gpu: SVS bank device node of GPU
> > > +  compatible: "mediatek,mt8183-svs-gpu"
> > > +  operating-points-v2: OPP table hooked by SVS GPU bank.
> > > +                  SVS will optimze this OPP table voltage part.
> > > +  vgpu-spply: PMIC buck of GPU
> > > +
> > > +Example:
> > > +
> > > +   svs: svs@1100b000 {
> > > +           compatible = "mediatek,mt8183-svs";
> > > +           reg = <0 0x1100b000 0 0x1000>;
> > > +           interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_LOW 0>;
> >
> > GIC interrupts are 3 cells, you have 4.
>
> Oops, I'll remove the fourth parameter. Thanks a lot.
>
> >
> > > +           clocks = <&infracfg CLK_INFRA_THERM>;
> > > +           clock-names = "main_clk";
> > > +           nvmem-cells = <&svs_calibration>, <&thermal_calibration>;
> > > +           nvmem-cell-names = "svs-calibration-data", "calibration-data";
> > > +
> > > +           svs_cpu_little: svs_cpu_little {
> >
> > Don't use '_' in node names.
>
> Okay. I'll replace it with '-'. Thanks.
>
> >
> > > +                   compatible = "mediatek,mt8183-svs-cpu-little";
> > > +                   operating-points-v2 = <&cluster0_opp>;
> > > +           };
> > > +
> > > +           svs_cpu_big: svs_cpu_big {
> > > +                   compatible = "mediatek,mt8183-svs-cpu-big";
> > > +                   operating-points-v2 = <&cluster1_opp>;
> > > +           };
> > > +
> > > +           svs_cci: svs_cci {
> > > +                   compatible = "mediatek,mt8183-svs-cci";
> > > +                   operating-points-v2 = <&cci_opp>;
> > > +           };
> > > +
> > > +           svs_gpu: svs_gpu {
> > > +                   compatible = "mediatek,mt8183-svs-gpu";
> > > +                   power-domains = <&scpsys MT8183_POWER_DOMAIN_MFG_2D>;
> > > +                   operating-points-v2 = <&gpu_opp_table>;
> > > +           };
> > > +   };
> > > +
> > > +   &svs_cpu_little {
> > > +           vcpu-little-supply = <&mt6358_vproc12_reg>;
> >
> > It's already defined to have OPP and supply in the cpu nodes. Parse them
> > to get this information rather than duplicating it here.
> >
> > The same should apply to the CCI and GPU.
>
> Please let me explain the reason why I add SVS sub-nodes. I ever try to
> parse other nodes to get desired power-domains/OPP table. However, it
> makes SVS driver harder to develop and maintain.
>
> 1. When a SVS-controller-init wants GPU_CORE0's OPP table in one node
> but it needs power-domains(GPU_MFG_2D) in another node, it becomes
> complicated and confusing when SVS sub-node tries to parse many nodes.
> Therefore, we want SVS sub-node to focus on what SVS bank requires by
> how we do in this patch.
>
> 2. In hardware point of view, SVS controller depends on other hardware's
> power only. All the SVS controller registers are in SVS hardware. So, we
> think It's good that SVS sub-node describes what SVS controller requires
> instead of linking other subsys nodes and parse the property that SVS
> controller needs.
>
> 3. We want SVS driver to have a generic way to attain subsys device for
> using "pm_runtime and OPP framework" API. If SVS driver tries to parse
> CPU(little/big core) and other subsys device node(e.g cci/gpu), it means
> SVS driver has to maintain different methodologies(cpu-specific?
> devfreq? others?) in order to get CPU(little/big core) and other subsys
> device(e.g cci/gpu) for using "pm_runtime and OPP framework" API.

(Didn't see this more complete reply before replying to v6, I can see
your argument, but if we wanted to push further to have the sub-device
node in the DT)

From what I see, the SVS driver for node x (cpu/cci/gpu) requires only 3 things:
 - An OPP table => that should always be "operating-points-v2"
property of the node x.
 - A power domain => that should always be power-domains property of node x.
 - A regulator. That one is a bit tricky as the cpu/cci uses "proc",
but gpu uses "mali" (at least on 8183). But maybe you can add a
child-regulator-name property or something to the DT so that the SVS
driver can find the correct regulator?

Seems like the solution could be quite generic?

> >
> > Rob
>
> Sincerely,
> Roger Lu.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/power/mtk-svs.txt b/Documentation/devicetree/bindings/power/mtk-svs.txt
new file mode 100644
index 000000000000..6a71992ef162
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/mtk-svs.txt
@@ -0,0 +1,88 @@ 
+* Mediatek Smart Voltage Scaling (MTK SVS)
+
+This describes the device tree binding for the MTK SVS controller (bank)
+which helps provide the optimized CPU/GPU/CCI voltages. This device also
+needs thermal data to calculate thermal slope for accurately compensate
+the voltages when temperature change.
+
+Required properties:
+- compatible:
+  - "mediatek,mt8183-svs" : For MT8183 family of SoCs
+- reg: Address range of the MTK SVS controller.
+- interrupts: IRQ for the MTK SVS controller.
+- clocks, clock-names: Clocks needed for the svs controller. required
+                       clocks are:
+		       "main_clk": Main clock needed for register access
+- nvmem-cells: Phandle to the calibration data provided by a nvmem device.
+- nvmem-cell-names: Should be "svs-calibration-data" and "calibration-data"
+
+Subnodes:
+- svs_cpu_little: SVS bank device node of little CPU
+  compatible: "mediatek,mt8183-svs-cpu-little"
+  operating-points-v2: OPP table hooked by SVS little CPU bank.
+		       SVS will optimze this OPP table voltage part.
+  vcpu-little-supply: PMIC buck of little CPU
+- svs_cpu_big: SVS bank device node of big CPU
+  compatible: "mediatek,mt8183-svs-cpu-big"
+  operating-points-v2: OPP table hooked by SVS big CPU bank.
+		       SVS will optimze this OPP table voltage part.
+  vcpu-big-supply: PMIC buck of big CPU
+- svs_cci: SVS bank device node of CCI
+  compatible: "mediatek,mt8183-svs-cci"
+  operating-points-v2: OPP table hooked by SVS CCI bank.
+		       SVS will optimze this OPP table voltage part.
+  vcci-supply: PMIC buck of CCI
+- svs_gpu: SVS bank device node of GPU
+  compatible: "mediatek,mt8183-svs-gpu"
+  operating-points-v2: OPP table hooked by SVS GPU bank.
+		       SVS will optimze this OPP table voltage part.
+  vgpu-spply: PMIC buck of GPU
+
+Example:
+
+	svs: svs@1100b000 {
+		compatible = "mediatek,mt8183-svs";
+		reg = <0 0x1100b000 0 0x1000>;
+		interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_LOW 0>;
+		clocks = <&infracfg CLK_INFRA_THERM>;
+		clock-names = "main_clk";
+		nvmem-cells = <&svs_calibration>, <&thermal_calibration>;
+		nvmem-cell-names = "svs-calibration-data", "calibration-data";
+
+		svs_cpu_little: svs_cpu_little {
+			compatible = "mediatek,mt8183-svs-cpu-little";
+			operating-points-v2 = <&cluster0_opp>;
+		};
+
+		svs_cpu_big: svs_cpu_big {
+			compatible = "mediatek,mt8183-svs-cpu-big";
+			operating-points-v2 = <&cluster1_opp>;
+		};
+
+		svs_cci: svs_cci {
+			compatible = "mediatek,mt8183-svs-cci";
+			operating-points-v2 = <&cci_opp>;
+		};
+
+		svs_gpu: svs_gpu {
+			compatible = "mediatek,mt8183-svs-gpu";
+			power-domains = <&scpsys MT8183_POWER_DOMAIN_MFG_2D>;
+			operating-points-v2 = <&gpu_opp_table>;
+		};
+	};
+
+	&svs_cpu_little {
+		vcpu-little-supply = <&mt6358_vproc12_reg>;
+	};
+
+	&svs_cpu_big {
+		vcpu-big-supply = <&mt6358_vproc11_reg>;
+	};
+
+	&svs_cci {
+		vcci-supply = <&mt6358_vproc12_reg>;
+	};
+
+	&svs_gpu {
+		vgpu-spply = <&mt6358_vgpu_reg>;
+	};