Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

2019-07-31 Thread Artur Świgoń 
On Wed, 2019-07-24 at 21:24 +0200, Krzysztof Kozlowski wrote:
> On Tue, Jul 23, 2019 at 02:20:13PM +0200, Artur Świgoń wrote:
> > This patch adds two fields tp the Exynos4412 DTS:
> 
> tp->to

Fixed. Thanks for catching the typo :)

> >   - parent: to declare connections between nodes that are not in a
> > parent-child relation in devfreq;
> 
> Is it a standard property?
> The explanation needs some improvement... why are you adding parent to a
> devices which are not child-parent?

This is not a standard property. I actually wanted to call it 'neighbor' at 
first. If you take a look at [1] and search for 'Exynos4x12', you will see that 
there are two power lines, and each has exactly one parent block (the rest are 
modelled in devfreq as its children). In [2], the parent of each child is 
indicated using the 'devfreq' property, e.g.,

_display {
devfreq = <_leftbus>;
status = "okay";
};

The single piece missing to make this topology a connected graph (for
interconnect QoS purposes) is the 'parent' property I proposed in this patch.
bus_leftbus is dependent on bus_dmc, therefore using the term 'parent' is
justified IMHO.

The explanation could be improved by adding what Chanwoo Choi <
cw00.c...@samsung.com> expressed in a parallel reply to this patch:
> - If 'devfreq' property is used between buses,
>   it indicates that there are requirement of sharing of power line.
> - If 'parent' property is used between buses,
>   it indicates that there are requirement of interconnect connection.

[1] Documentation/devicetree/bindings/devfreq/exynos-bus.txt
[2] arch/arm/boot/dts/exynos4412-odroid-common.dtsi (subject of this patch)

Best regards,
-- 
Artur Świgoń
Samsung R Institute Poland
Samsung Electronics

Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

2019-07-28 Thread Chanwoo Choi 
Hi,

On 19. 7. 26. 오후 9:02, Marek Szyprowski wrote:
> Hi
> 
> On 2019-07-25 15:13, Chanwoo Choi wrote:
>> 2019년 7월 24일 (수) 오전 8:07, Artur Świgoń 님이 작성:
>>> This patch adds two fields tp the Exynos4412 DTS:
>>>- parent: to declare connections between nodes that are not in a
>>>  parent-child relation in devfreq;
>>>- #interconnect-cells: required by the interconnect framework.
>>>
>>> Please note that #interconnect-cells is always zero and node IDs are not
>>> hardcoded anywhere.
>>>
>>> Signed-off-by: Artur Świgoń 
>>> ---
>>>   arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
>>>   arch/arm/boot/dts/exynos4412.dtsi   | 9 +
>>>   2 files changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi 
>>> b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>>> index ea55f377d17c..bdd61ae86103 100644
>>> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>>> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>>> @@ -106,6 +106,7 @@
>>>   _leftbus {
>>>  devfreq-events = <_leftbus_3>, <_rightbus_3>;
>>>  vdd-supply = <_reg>;
>>> +   parent = <_dmc>;
>> It is wrong. 'bus_leftbus' has not any h/w dependency of 'bus_dmc'
>> and 'bus_leftbus' is not child of 'bus_dmc'.
>>
>> Even it there are some PMQoS requirement between them,
>> it it not proper to tie both 'bus_leftbus' and 'bus_dmc'.
> 
> There is strict dependency between them. DMC bus running at frequency 
> lower than left (or right) bus really doesn't make much sense, because 
> it will limit the left bus performance. This dependency should be 
> modeled somehow.

I misunderstood new 'parent' prototype as the existing 'devfreq' property.
I didn't understand why use the 'devfreq' property because 'bus_dmc' and
'bus_leftbus' don't share the power line. Please ignore my previous comment.

Basically, I agree that it is necessary to support the QoS requirement
between buses or if possible, between bus and gpu.

To support the interconnect between bus_dmc, bus_leftbus and bus_display,
it used the either 'devfreq' or 'parent' properties to connect them.

In order to catch the meaning of 'devfreq' and 'parent' properties,
If possible, it would be separate the usage role of between 'devfreq'
or 'parent' properties. Because it is possible to connect the 'buses'
with only using 'parent' property if all buses in the path uses
the devfreq governors except for 'passive' governor.

- If 'devfreq' property is used between buses,
  it indicates that there are requirement of shading of power line.
- If 'parent' property is used between buses,
  it indicates that there are requirement of interconnect connection.

> 
>>>  status = "okay";
>>>   };
>>>
>>> diff --git a/arch/arm/boot/dts/exynos4412.dtsi 
>>> b/arch/arm/boot/dts/exynos4412.dtsi
>>> index d20db2dfe8e2..a70a671acacd 100644
>>> --- a/arch/arm/boot/dts/exynos4412.dtsi
>>> +++ b/arch/arm/boot/dts/exynos4412.dtsi
>>> @@ -390,6 +390,7 @@
>>>  clocks = < CLK_DIV_DMC>;
>>>  clock-names = "bus";
>>>  operating-points-v2 = <_dmc_opp_table>;
>>> +   #interconnect-cells = <0>;
>>>  status = "disabled";
>>>  };
>>>
>>> @@ -398,6 +399,7 @@
>>>  clocks = < CLK_DIV_ACP>;
>>>  clock-names = "bus";
>>>  operating-points-v2 = <_acp_opp_table>;
>>> +   #interconnect-cells = <0>;
>>>  status = "disabled";
>>>  };
>>>
>>> @@ -406,6 +408,7 @@
>>>  clocks = < CLK_DIV_C2C>;
>>>  clock-names = "bus";
>>>  operating-points-v2 = <_dmc_opp_table>;
>>> +   #interconnect-cells = <0>;
>>>  status = "disabled";
>>>  };
>>>
>>> @@ -459,6 +462,7 @@
>>>  clocks = < CLK_DIV_GDL>;
>>>  clock-names = "bus";
>>>  operating-points-v2 = <_leftbus_opp_table>;
>>> +   #interconnect-cells = <0>;
>>>  status = "disabled";
>>>  };
>>>
>>> @@ -467,6 +471,7 @@
>>>  clocks = < CLK_DIV_GDR>;
>>>  clock-names = "bus";
>>>  operating-points-v2 = <_leftbus_opp_table>;
>>> +   #interconnect-cells = <0>;
>>>  status = "disabled";
>>>  };
>>>
>>> @@ -475,6 +480,7 @@
>>>  clocks = < CLK_ACLK160>;
>>>  clock-names = "bus";
>>>  operating-points-v2 = <_display_opp_table>;
>>> +   #interconnect-cells = <0>;
>>>  status = "disabled";
>>>  };
>>>
>>> @@ -483,6 +489,7 @@
>>>

Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

2019-07-26 Thread Marek Szyprowski 
Hi

On 2019-07-25 15:13, Chanwoo Choi wrote:
> 2019년 7월 24일 (수) 오전 8:07, Artur Świgoń 님이 작성:
>> This patch adds two fields tp the Exynos4412 DTS:
>>- parent: to declare connections between nodes that are not in a
>>  parent-child relation in devfreq;
>>- #interconnect-cells: required by the interconnect framework.
>>
>> Please note that #interconnect-cells is always zero and node IDs are not
>> hardcoded anywhere.
>>
>> Signed-off-by: Artur Świgoń 
>> ---
>>   arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
>>   arch/arm/boot/dts/exynos4412.dtsi   | 9 +
>>   2 files changed, 10 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi 
>> b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>> index ea55f377d17c..bdd61ae86103 100644
>> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>> @@ -106,6 +106,7 @@
>>   _leftbus {
>>  devfreq-events = <_leftbus_3>, <_rightbus_3>;
>>  vdd-supply = <_reg>;
>> +   parent = <_dmc>;
> It is wrong. 'bus_leftbus' has not any h/w dependency of 'bus_dmc'
> and 'bus_leftbus' is not child of 'bus_dmc'.
>
> Even it there are some PMQoS requirement between them,
> it it not proper to tie both 'bus_leftbus' and 'bus_dmc'.

There is strict dependency between them. DMC bus running at frequency 
lower than left (or right) bus really doesn't make much sense, because 
it will limit the left bus performance. This dependency should be 
modeled somehow.

>>  status = "okay";
>>   };
>>
>> diff --git a/arch/arm/boot/dts/exynos4412.dtsi 
>> b/arch/arm/boot/dts/exynos4412.dtsi
>> index d20db2dfe8e2..a70a671acacd 100644
>> --- a/arch/arm/boot/dts/exynos4412.dtsi
>> +++ b/arch/arm/boot/dts/exynos4412.dtsi
>> @@ -390,6 +390,7 @@
>>  clocks = < CLK_DIV_DMC>;
>>  clock-names = "bus";
>>  operating-points-v2 = <_dmc_opp_table>;
>> +   #interconnect-cells = <0>;
>>  status = "disabled";
>>  };
>>
>> @@ -398,6 +399,7 @@
>>  clocks = < CLK_DIV_ACP>;
>>  clock-names = "bus";
>>  operating-points-v2 = <_acp_opp_table>;
>> +   #interconnect-cells = <0>;
>>  status = "disabled";
>>  };
>>
>> @@ -406,6 +408,7 @@
>>  clocks = < CLK_DIV_C2C>;
>>  clock-names = "bus";
>>  operating-points-v2 = <_dmc_opp_table>;
>> +   #interconnect-cells = <0>;
>>  status = "disabled";
>>  };
>>
>> @@ -459,6 +462,7 @@
>>  clocks = < CLK_DIV_GDL>;
>>  clock-names = "bus";
>>  operating-points-v2 = <_leftbus_opp_table>;
>> +   #interconnect-cells = <0>;
>>  status = "disabled";
>>  };
>>
>> @@ -467,6 +471,7 @@
>>  clocks = < CLK_DIV_GDR>;
>>  clock-names = "bus";
>>  operating-points-v2 = <_leftbus_opp_table>;
>> +   #interconnect-cells = <0>;
>>  status = "disabled";
>>  };
>>
>> @@ -475,6 +480,7 @@
>>  clocks = < CLK_ACLK160>;
>>  clock-names = "bus";
>>  operating-points-v2 = <_display_opp_table>;
>> +   #interconnect-cells = <0>;
>>  status = "disabled";
>>  };
>>
>> @@ -483,6 +489,7 @@
>>  clocks = < CLK_ACLK133>;
>>  clock-names = "bus";
>>  operating-points-v2 = <_fsys_opp_table>;
>> +   #interconnect-cells = <0>;
>>  status = "disabled";
>>  };
>>
>> @@ -491,6 +498,7 @@
>>  clocks = < CLK_ACLK100>;
>>  clock-names = "bus";
>>  operating-points-v2 = <_peri_opp_table>;
>> +   #interconnect-cells = <0>;
>>  status = "disabled";
>>  };
>>
>> @@ -499,6 +507,7 @@
>>  clocks = < CLK_SCLK_MFC>;
>>  clock-names = "bus";
>>  operating-points-v2 = <_leftbus_opp_table>;
>> +   #interconnect-cells = <0>;
>>  status = "disabled";
>>  };
>>
>> --
>> 2.17.1
>>
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R Institute Poland

Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

2019-07-25 Thread Chanwoo Choi 
2019년 7월 24일 (수) 오전 8:07, Artur Świgoń 님이 작성:
>
> This patch adds two fields tp the Exynos4412 DTS:
>   - parent: to declare connections between nodes that are not in a
> parent-child relation in devfreq;
>   - #interconnect-cells: required by the interconnect framework.
>
> Please note that #interconnect-cells is always zero and node IDs are not
> hardcoded anywhere.
>
> Signed-off-by: Artur Świgoń 
> ---
>  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
>  arch/arm/boot/dts/exynos4412.dtsi   | 9 +
>  2 files changed, 10 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi 
> b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index ea55f377d17c..bdd61ae86103 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -106,6 +106,7 @@
>  _leftbus {
> devfreq-events = <_leftbus_3>, <_rightbus_3>;
> vdd-supply = <_reg>;
> +   parent = <_dmc>;

It is wrong. 'bus_leftbus' has not any h/w dependency of 'bus_dmc'
and 'bus_leftbus' is not child of 'bus_dmc'.

Even it there are some PMQoS requirement between them,
it it not proper to tie both 'bus_leftbus' and 'bus_dmc'.

> status = "okay";
>  };
>
> diff --git a/arch/arm/boot/dts/exynos4412.dtsi 
> b/arch/arm/boot/dts/exynos4412.dtsi
> index d20db2dfe8e2..a70a671acacd 100644
> --- a/arch/arm/boot/dts/exynos4412.dtsi
> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> @@ -390,6 +390,7 @@
> clocks = < CLK_DIV_DMC>;
> clock-names = "bus";
> operating-points-v2 = <_dmc_opp_table>;
> +   #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -398,6 +399,7 @@
> clocks = < CLK_DIV_ACP>;
> clock-names = "bus";
> operating-points-v2 = <_acp_opp_table>;
> +   #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -406,6 +408,7 @@
> clocks = < CLK_DIV_C2C>;
> clock-names = "bus";
> operating-points-v2 = <_dmc_opp_table>;
> +   #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -459,6 +462,7 @@
> clocks = < CLK_DIV_GDL>;
> clock-names = "bus";
> operating-points-v2 = <_leftbus_opp_table>;
> +   #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -467,6 +471,7 @@
> clocks = < CLK_DIV_GDR>;
> clock-names = "bus";
> operating-points-v2 = <_leftbus_opp_table>;
> +   #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -475,6 +480,7 @@
> clocks = < CLK_ACLK160>;
> clock-names = "bus";
> operating-points-v2 = <_display_opp_table>;
> +   #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -483,6 +489,7 @@
> clocks = < CLK_ACLK133>;
> clock-names = "bus";
> operating-points-v2 = <_fsys_opp_table>;
> +   #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -491,6 +498,7 @@
> clocks = < CLK_ACLK100>;
> clock-names = "bus";
> operating-points-v2 = <_peri_opp_table>;
> +   #interconnect-cells = <0>;
> status = "disabled";
> };
>
> @@ -499,6 +507,7 @@
> clocks = < CLK_SCLK_MFC>;
> clock-names = "bus";
> operating-points-v2 = <_leftbus_opp_table>;
> +   #interconnect-cells = <0>;
> status = "disabled";
> };
>
> --
> 2.17.1
>


-- 
Best Regards,
Chanwoo Choi

Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

2019-07-24 Thread Krzysztof Kozlowski 
On Tue, Jul 23, 2019 at 02:20:13PM +0200, Artur Świgoń wrote:
> This patch adds two fields tp the Exynos4412 DTS:

tp->to

>   - parent: to declare connections between nodes that are not in a
> parent-child relation in devfreq;

Is it a standard property?
The explanation needs some improvement... why are you adding parent to a
devices which are not child-parent?

Best regards,
Krzysztof

>   - #interconnect-cells: required by the interconnect framework.
> 
> Please note that #interconnect-cells is always zero and node IDs are not
> hardcoded anywhere.
> 
> Signed-off-by: Artur Świgoń 
> ---
>  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
>  arch/arm/boot/dts/exynos4412.dtsi   | 9 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi 
> b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index ea55f377d17c..bdd61ae86103 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -106,6 +106,7 @@
>  _leftbus {
>   devfreq-events = <_leftbus_3>, <_rightbus_3>;
>   vdd-supply = <_reg>;
> + parent = <_dmc>;
>   status = "okay";
>  };
>  
> diff --git a/arch/arm/boot/dts/exynos4412.dtsi 
> b/arch/arm/boot/dts/exynos4412.dtsi
> index d20db2dfe8e2..a70a671acacd 100644
> --- a/arch/arm/boot/dts/exynos4412.dtsi
> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> @@ -390,6 +390,7 @@
>   clocks = < CLK_DIV_DMC>;
>   clock-names = "bus";
>   operating-points-v2 = <_dmc_opp_table>;
> + #interconnect-cells = <0>;
>   status = "disabled";
>   };
>  
> @@ -398,6 +399,7 @@
>   clocks = < CLK_DIV_ACP>;
>   clock-names = "bus";
>   operating-points-v2 = <_acp_opp_table>;
> + #interconnect-cells = <0>;
>   status = "disabled";
>   };
>  
> @@ -406,6 +408,7 @@
>   clocks = < CLK_DIV_C2C>;
>   clock-names = "bus";
>   operating-points-v2 = <_dmc_opp_table>;
> + #interconnect-cells = <0>;
>   status = "disabled";
>   };
>  
> @@ -459,6 +462,7 @@
>   clocks = < CLK_DIV_GDL>;
>   clock-names = "bus";
>   operating-points-v2 = <_leftbus_opp_table>;
> + #interconnect-cells = <0>;
>   status = "disabled";
>   };
>  
> @@ -467,6 +471,7 @@
>   clocks = < CLK_DIV_GDR>;
>   clock-names = "bus";
>   operating-points-v2 = <_leftbus_opp_table>;
> + #interconnect-cells = <0>;
>   status = "disabled";
>   };
>  
> @@ -475,6 +480,7 @@
>   clocks = < CLK_ACLK160>;
>   clock-names = "bus";
>   operating-points-v2 = <_display_opp_table>;
> + #interconnect-cells = <0>;
>   status = "disabled";
>   };
>  
> @@ -483,6 +489,7 @@
>   clocks = < CLK_ACLK133>;
>   clock-names = "bus";
>   operating-points-v2 = <_fsys_opp_table>;
> + #interconnect-cells = <0>;
>   status = "disabled";
>   };
>  
> @@ -491,6 +498,7 @@
>   clocks = < CLK_ACLK100>;
>   clock-names = "bus";
>   operating-points-v2 = <_peri_opp_table>;
> + #interconnect-cells = <0>;
>   status = "disabled";
>   };
>  
> @@ -499,6 +507,7 @@
>   clocks = < CLK_SCLK_MFC>;
>   clock-names = "bus";
>   operating-points-v2 = <_leftbus_opp_table>;
> + #interconnect-cells = <0>;
>   status = "disabled";
>   };
>  
> -- 
> 2.17.1
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

2019-07-23 Thread Artur Świgoń 
This patch adds two fields tp the Exynos4412 DTS:
  - parent: to declare connections between nodes that are not in a
parent-child relation in devfreq;
  - #interconnect-cells: required by the interconnect framework.

Please note that #interconnect-cells is always zero and node IDs are not
hardcoded anywhere.

Signed-off-by: Artur Świgoń 
---
 arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
 arch/arm/boot/dts/exynos4412.dtsi   | 9 +
 2 files changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi 
b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
index ea55f377d17c..bdd61ae86103 100644
--- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
+++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
@@ -106,6 +106,7 @@
 _leftbus {
devfreq-events = <_leftbus_3>, <_rightbus_3>;
vdd-supply = <_reg>;
+   parent = <_dmc>;
status = "okay";
 };
 
diff --git a/arch/arm/boot/dts/exynos4412.dtsi 
b/arch/arm/boot/dts/exynos4412.dtsi
index d20db2dfe8e2..a70a671acacd 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -390,6 +390,7 @@
clocks = < CLK_DIV_DMC>;
clock-names = "bus";
operating-points-v2 = <_dmc_opp_table>;
+   #interconnect-cells = <0>;
status = "disabled";
};
 
@@ -398,6 +399,7 @@
clocks = < CLK_DIV_ACP>;
clock-names = "bus";
operating-points-v2 = <_acp_opp_table>;
+   #interconnect-cells = <0>;
status = "disabled";
};
 
@@ -406,6 +408,7 @@
clocks = < CLK_DIV_C2C>;
clock-names = "bus";
operating-points-v2 = <_dmc_opp_table>;
+   #interconnect-cells = <0>;
status = "disabled";
};
 
@@ -459,6 +462,7 @@
clocks = < CLK_DIV_GDL>;
clock-names = "bus";
operating-points-v2 = <_leftbus_opp_table>;
+   #interconnect-cells = <0>;
status = "disabled";
};
 
@@ -467,6 +471,7 @@
clocks = < CLK_DIV_GDR>;
clock-names = "bus";
operating-points-v2 = <_leftbus_opp_table>;
+   #interconnect-cells = <0>;
status = "disabled";
};
 
@@ -475,6 +480,7 @@
clocks = < CLK_ACLK160>;
clock-names = "bus";
operating-points-v2 = <_display_opp_table>;
+   #interconnect-cells = <0>;
status = "disabled";
};
 
@@ -483,6 +489,7 @@
clocks = < CLK_ACLK133>;
clock-names = "bus";
operating-points-v2 = <_fsys_opp_table>;
+   #interconnect-cells = <0>;
status = "disabled";
};
 
@@ -491,6 +498,7 @@
clocks = < CLK_ACLK100>;
clock-names = "bus";
operating-points-v2 = <_peri_opp_table>;
+   #interconnect-cells = <0>;
status = "disabled";
};
 
@@ -499,6 +507,7 @@
clocks = < CLK_SCLK_MFC>;
clock-names = "bus";
operating-points-v2 = <_leftbus_opp_table>;
+   #interconnect-cells = <0>;
status = "disabled";
};
 
-- 
2.17.1