Hi Viresh,
On 4/10/19 07:05, Viresh Kumar wrote:
> 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@xxxxxxxxxx>
>>>> ---
>>>> 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>;
> };
> };
This works too.
>
> 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.
Ok, will make it bandwidth-MBps.
Thanks,
Georgi
