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

[Roger Lu]

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@xxxxxxxxxxxx>
> > ---
> >  .../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.