Re: [PATCH 1/4] dt-bindings: opp: Introduce opp-bw-MBs bindings

[Viresh Kumar <viresh.kumar@linaro.org>]

On 09-04-19, 17:36, Georgi Djakov wrote:
> Hi Viresh,
> 
> On 3/14/19 08:23, Viresh Kumar wrote:
> > On 13-03-19, 11:00, Georgi Djakov wrote:
> >> In addition to frequency and voltage, some devices may have bandwidth
> >> requirements for their interconnect throughput - for example a CPU
> >> or GPU may also need to increase or decrease their bandwidth to DDR
> >> memory based on the current operating performance point.
> >>
> >> Extend the OPP tables with additional property to describe the bandwidth
> >> needs of a device. The average and peak bandwidth values depend on the
> >> hardware and its properties.
> >>
> >> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> >> ---
> >>  Documentation/devicetree/bindings/opp/opp.txt | 45 +++++++++++++++++++
> >>  1 file changed, 45 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> >> index 76b6c79604a5..fa598264615f 100644
> >> --- a/Documentation/devicetree/bindings/opp/opp.txt
> >> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> >> @@ -129,6 +129,9 @@ Optional properties:
> >>  - opp-microamp-<name>: Named opp-microamp property. Similar to
> >>    opp-microvolt-<name> property, but for microamp instead.
> >>  
> >> +- opp-bw-MBs: The interconnect bandwidth is specified with an array containing
> >> +  the two integer values for average and peak bandwidth in megabytes per second.
> >> +
> >>  - opp-level: A value representing the performance level of the device,
> >>    expressed as a 32-bit integer.
> >>  
> >> @@ -546,3 +549,45 @@ Example 6: opp-microvolt-<name>, opp-microamp-<name>:
> >>  		};
> >>  	};
> >>  };
> >> +
> >> +Example 7: opp-bw-MBs:
> >> +(example: average and peak bandwidth values are defined for each OPP and the
> >> +interconnect between CPU and DDR memory is scaled together with CPU frequency)
> >> +
> >> +/ {
> >> +	cpus {
> >> +		CPU0: cpu@0 {
> >> +			compatible = "arm,cortex-a53", "arm,armv8";
> >> +			...
> >> +			operating-points-v2 = <&cpu_opp_table>;
> >> +			/* path between the CPU and DDR memory */
> >> +			interconnects = <&rpm_bimc MASTER_AMPSS_M0
> >> +					&rpm_bimc SLAVE_EBI_CH0>;
> > 
> > Can we have multiple paths for a device ?
> 
> I suppose that this is also a possible scenario. Will propose something
> to handle multiple paths too.
> 
> >> +		};
> >> +	};
> >> +
> >> +	cpu_opp_table: cpu_opp_table {
> >> +		compatible = "operating-points-v2";
> >> +		opp-shared;
> >> +
> >> +		opp-200000000 {
> >> +			opp-hz = /bits/ 64 <200000000>;
> >> +			/* 457 MB/s average and 1525 MB/s peak bandwidth */
> >> +			opp-bw-MBs = <457 1525>;
> > 
> > In that case fixing this to just 2 entries in the array is incorrect
> > and we should take care of that in the bindings here.
> 
> We can encode the path name into the property (when there are multiple
> paths). We already have opp-microamp-<name> and opp-microamp-<name>, so
> we can follow the same practice.
> 
> For example:
> 
> CPU0: cpu@0 {
> 	compatible = "arm,cortex-a53", "arm,armv8";
> 	...
> 	operating-points-v2 = <&cpu_opp_table>;
> 	/* path between the CPU and DDR and path between CPU and L3 */
> 	interconnects = <&bimc MASTER_AMPSS_M0 &bimc SLAVE_EBI_CH0>,
> 			<&bimc MASTER_AMPSS_M0 &bimc SLAVE_L3>;
> 	interconnect-names "cpu-mem", "cpu-l3";
> };
> 
> cpu_opp_table: cpu_opp_table {
> 	compatible = "operating-points-v2";
> 	opp-shared;
> 
> 	opp-200000000 {
> 		opp-hz = /bits/ 64 <200000000>;
> 		/* 457 MB/s average, 1525 MB/s peak bandwidth to DDR */
> 		opp-bw-MBps-cpu-mem = <457 1525>;
> 		/* 914 MB/s average, 3050 MB/s peak bandwidth to L3 */
> 		opp-bw-MBps-cpu-l3 = <914 3050>;
> 	};
> };

The -<name> property is different as only one of the value is ever used, i.e. we
can have opp-microvolt-speed0/1/2/3 (4 different values/properties) and only
opp-microvolt-speed1 will be used eventually and all others are discarded.

Also I am not sure if this will be actually required. We already have a list of
interconnects above and the order of that can be taken as reference here. i.e.

CPU0: cpu@0 {
	compatible = "arm,cortex-a53", "arm,armv8";
	...
	operating-points-v2 = <&cpu_opp_table>;
	/* path between the CPU and DDR and path between CPU and L3 */
	interconnects = <&bimc MASTER_AMPSS_M0 &bimc SLAVE_EBI_CH0>,
			<&bimc MASTER_AMPSS_M0 &bimc SLAVE_L3>;
};

cpu_opp_table: cpu_opp_table {
	compatible = "operating-points-v2";
	opp-shared;

	opp-200000000 {
		opp-hz = /bits/ 64 <200000000>;
		/* 457 MB/s average, 1525 MB/s peak bandwidth to DDR */
		/* 914 MB/s average, 3050 MB/s peak bandwidth to L3 */
		opp-bw-MBps = <457 1525>, <914 3050>;
	};
};

I also strongly believe that "opp-bw-MBps" should be renamed in a way to make it
independent of the OPPs. For example, we may have devices which also need to add
their vote for the bandwidth but don't have an OPP table as they don't do DVFS.
And the same property should be used by them directly as what we will have in
the individual OPPs in the above example case.

So maybe something like bw-MBps or something else.

-- 
viresh