[v1,27/50] ARM: dts: exynos: align bus_wcore OPPs in Exynos5420
diff mbox series

Message ID 20190715124417.4787-28-l.luba@partner.samsung.com
State New, archived
Headers show
Series
  • Exynos5x clocks and buses changes
Related show

Commit Message

Lukasz Luba July 15, 2019, 12:43 p.m. UTC
This is the most important bus in the Exynos5x SoC. The whole communication
inside SoC does through that bus (apart from direct requests from CCI to
DRAM controller). It is also modeled as a master bus in devfreq framework.
It is also the only one OPP table throughout other buses which has voltage
values. The devfreq software controls the speed of that bus and other
buses. The other buses follows the rate of the master. There is only one
regulator. The old lowest OPP had pair 925mV, 84MHz which is enough for
this frequency. However, due to the fact that the other buses follows the
WCORE bus by taking the OPP from their table with the same id, e.g. opp02,
the children frequency should be stable with the set voltage.
It could cause random faults very hard to debug.
Thus, the patch removes the lowest OPP to make other buses' lowest OPPs
working. The new lowest OPP has voltage high enough for buses working up
to 333MHz. It also changes the frequencies of the OPPs to align them to
PLL value such that it is possible to set them using only a divider without
reprogramming OPP. Reprogramming the PLL was not set, so the real frequency
values were not the one from the OPP table, which could confuse the
governor algorithms which relay on OPP speed values making the system to
behave weird.

Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 arch/arm/boot/dts/exynos5420.dtsi | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Krzysztof Kozlowski July 17, 2019, 10:15 a.m. UTC | #1
On Mon, 15 Jul 2019 at 14:44, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>
> This is the most important bus in the Exynos5x SoC. The whole communication
> inside SoC does through that bus (apart from direct requests from CCI to
> DRAM controller). It is also modeled as a master bus in devfreq framework.
> It is also the only one OPP table throughout other buses which has voltage
> values. The devfreq software controls the speed of that bus and other
> buses. The other buses follows the rate of the master. There is only one
> regulator. The old lowest OPP had pair 925mV, 84MHz which is enough for

s/lowest/slowest/

> this frequency. However, due to the fact that the other buses follows the
> WCORE bus by taking the OPP from their table with the same id, e.g. opp02,
> the children frequency should be stable with the set voltage.
> It could cause random faults very hard to debug.
> Thus, the patch removes the lowest OPP to make other buses' lowest OPPs

s/lowest/slowest/

> working. The new lowest OPP has voltage high enough for buses working up
> to 333MHz. It also changes the frequencies of the OPPs to align them to
> PLL value such that it is possible to set them using only a divider without
> reprogramming OPP.

Reprogramming OPP? What is it?

> Reprogramming the PLL was not set, so the real frequency

I understood from the previous that reprogramming the OPP (PLL?) was
happening... Please rephrase entire sentence.

BR,
Krzysztof

> values were not the one from the OPP table, which could confuse the
> governor algorithms which relay on OPP speed values making the system to
> behave weird.
>
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  arch/arm/boot/dts/exynos5420.dtsi | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> index f8c36ff0d4c3..a355c76af5a5 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -1107,22 +1107,18 @@
>                         compatible = "operating-points-v2";
>
>                         opp00 {
> -                               opp-hz = /bits/ 64 <84000000>;
> -                               opp-microvolt = <925000>;
> +                               opp-hz = /bits/ 64 <150000000>;
> +                               opp-microvolt = <950000>;
>                         };
>                         opp01 {
> -                               opp-hz = /bits/ 64 <111000000>;
> +                               opp-hz = /bits/ 64 <200000000>;
>                                 opp-microvolt = <950000>;
>                         };
>                         opp02 {
> -                               opp-hz = /bits/ 64 <222000000>;
> +                               opp-hz = /bits/ 64 <300000000>;
>                                 opp-microvolt = <950000>;
>                         };
>                         opp03 {
> -                               opp-hz = /bits/ 64 <333000000>;
> -                               opp-microvolt = <950000>;
> -                       };
> -                       opp04 {
>                                 opp-hz = /bits/ 64 <400000000>;
>                                 opp-microvolt = <987500>;
>                         };
> --
> 2.17.1
>
Lukasz Luba July 17, 2019, 10:29 a.m. UTC | #2
On 7/17/19 12:15 PM, Krzysztof Kozlowski wrote:
> On Mon, 15 Jul 2019 at 14:44, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>>
>> This is the most important bus in the Exynos5x SoC. The whole communication
>> inside SoC does through that bus (apart from direct requests from CCI to
>> DRAM controller). It is also modeled as a master bus in devfreq framework.
>> It is also the only one OPP table throughout other buses which has voltage
>> values. The devfreq software controls the speed of that bus and other
>> buses. The other buses follows the rate of the master. There is only one
>> regulator. The old lowest OPP had pair 925mV, 84MHz which is enough for
> 
> s/lowest/slowest/
OK
> 
>> this frequency. However, due to the fact that the other buses follows the
>> WCORE bus by taking the OPP from their table with the same id, e.g. opp02,
>> the children frequency should be stable with the set voltage.
>> It could cause random faults very hard to debug.
>> Thus, the patch removes the lowest OPP to make other buses' lowest OPPs
> 
> s/lowest/slowest/
OK
> 
>> working. The new lowest OPP has voltage high enough for buses working up
>> to 333MHz. It also changes the frequencies of the OPPs to align them to
>> PLL value such that it is possible to set them using only a divider without
>> reprogramming OPP.
> 
> Reprogramming OPP? What is it?
Mistake, should be PLL. Thanks for that.
> 
>> Reprogramming the PLL was not set, so the real frequency
> 
> I understood from the previous that reprogramming the OPP (PLL?) was
> happening... Please rephrase entire sentence.
Yes, I will rewrite it when I will combine these patches into one.

Regards,
Lukasz
> 
> BR,
> Krzysztof
> 
>> values were not the one from the OPP table, which could confuse the
>> governor algorithms which relay on OPP speed values making the system to
>> behave weird.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   arch/arm/boot/dts/exynos5420.dtsi | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
>> index f8c36ff0d4c3..a355c76af5a5 100644
>> --- a/arch/arm/boot/dts/exynos5420.dtsi
>> +++ b/arch/arm/boot/dts/exynos5420.dtsi
>> @@ -1107,22 +1107,18 @@
>>                          compatible = "operating-points-v2";
>>
>>                          opp00 {
>> -                               opp-hz = /bits/ 64 <84000000>;
>> -                               opp-microvolt = <925000>;
>> +                               opp-hz = /bits/ 64 <150000000>;
>> +                               opp-microvolt = <950000>;
>>                          };
>>                          opp01 {
>> -                               opp-hz = /bits/ 64 <111000000>;
>> +                               opp-hz = /bits/ 64 <200000000>;
>>                                  opp-microvolt = <950000>;
>>                          };
>>                          opp02 {
>> -                               opp-hz = /bits/ 64 <222000000>;
>> +                               opp-hz = /bits/ 64 <300000000>;
>>                                  opp-microvolt = <950000>;
>>                          };
>>                          opp03 {
>> -                               opp-hz = /bits/ 64 <333000000>;
>> -                               opp-microvolt = <950000>;
>> -                       };
>> -                       opp04 {
>>                                  opp-hz = /bits/ 64 <400000000>;
>>                                  opp-microvolt = <987500>;
>>                          };
>> --
>> 2.17.1
>>
> 
>
Lukasz Luba July 17, 2019, 4:58 p.m. UTC | #3
Hi Krzysztof,

On 7/17/19 12:15 PM, Krzysztof Kozlowski wrote:
> On Mon, 15 Jul 2019 at 14:44, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>>
>> This is the most important bus in the Exynos5x SoC. The whole communication
>> inside SoC does through that bus (apart from direct requests from CCI to
>> DRAM controller). It is also modeled as a master bus in devfreq framework.
>> It is also the only one OPP table throughout other buses which has voltage
>> values. The devfreq software controls the speed of that bus and other
>> buses. The other buses follows the rate of the master. There is only one
>> regulator. The old lowest OPP had pair 925mV, 84MHz which is enough for
> 
> s/lowest/slowest/
please see below
> 
>> this frequency. However, due to the fact that the other buses follows the
>> WCORE bus by taking the OPP from their table with the same id, e.g. opp02,
>> the children frequency should be stable with the set voltage.
>> It could cause random faults very hard to debug.
>> Thus, the patch removes the lowest OPP to make other buses' lowest OPPs
> 
> s/lowest/slowest/
Actually, I have double checked that, because we always used this 
terminology: low OPP, high OPP, lower OPPs, higher OPPs. I can change
it here for you, but I think this is not something that people are used
to. Please check EAS pdf documentation or this file:
https://www.kernel.org/doc/Documentation/scheduler/sched-energy.txt
i.e. "running at a lower OPP" or "high OPPs", "lowest OPPs".

Regards,
Lukasz
> 
>> working. The new lowest OPP has voltage high enough for buses working up
>> to 333MHz. It also changes the frequencies of the OPPs to align them to
>> PLL value such that it is possible to set them using only a divider without
>> reprogramming OPP.
> 
> Reprogramming OPP? What is it?
> 
>> Reprogramming the PLL was not set, so the real frequency
> 
> I understood from the previous that reprogramming the OPP (PLL?) was
> happening... Please rephrase entire sentence.
> 
> BR,
> Krzysztof
> 
>> values were not the one from the OPP table, which could confuse the
>> governor algorithms which relay on OPP speed values making the system to
>> behave weird.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   arch/arm/boot/dts/exynos5420.dtsi | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
>> index f8c36ff0d4c3..a355c76af5a5 100644
>> --- a/arch/arm/boot/dts/exynos5420.dtsi
>> +++ b/arch/arm/boot/dts/exynos5420.dtsi
>> @@ -1107,22 +1107,18 @@
>>                          compatible = "operating-points-v2";
>>
>>                          opp00 {
>> -                               opp-hz = /bits/ 64 <84000000>;
>> -                               opp-microvolt = <925000>;
>> +                               opp-hz = /bits/ 64 <150000000>;
>> +                               opp-microvolt = <950000>;
>>                          };
>>                          opp01 {
>> -                               opp-hz = /bits/ 64 <111000000>;
>> +                               opp-hz = /bits/ 64 <200000000>;
>>                                  opp-microvolt = <950000>;
>>                          };
>>                          opp02 {
>> -                               opp-hz = /bits/ 64 <222000000>;
>> +                               opp-hz = /bits/ 64 <300000000>;
>>                                  opp-microvolt = <950000>;
>>                          };
>>                          opp03 {
>> -                               opp-hz = /bits/ 64 <333000000>;
>> -                               opp-microvolt = <950000>;
>> -                       };
>> -                       opp04 {
>>                                  opp-hz = /bits/ 64 <400000000>;
>>                                  opp-microvolt = <987500>;
>>                          };
>> --
>> 2.17.1
>>
> 
>
Krzysztof Kozlowski July 23, 2019, 12:08 p.m. UTC | #4
On Wed, 17 Jul 2019 at 18:58, Lukasz Luba <l.luba@partner.samsung.com> wrote:
>
> Hi Krzysztof,
>
> On 7/17/19 12:15 PM, Krzysztof Kozlowski wrote:
> > On Mon, 15 Jul 2019 at 14:44, Lukasz Luba <l.luba@partner.samsung.com> wrote:
> >>
> >> This is the most important bus in the Exynos5x SoC. The whole communication
> >> inside SoC does through that bus (apart from direct requests from CCI to
> >> DRAM controller). It is also modeled as a master bus in devfreq framework.
> >> It is also the only one OPP table throughout other buses which has voltage
> >> values. The devfreq software controls the speed of that bus and other
> >> buses. The other buses follows the rate of the master. There is only one
> >> regulator. The old lowest OPP had pair 925mV, 84MHz which is enough for
> >
> > s/lowest/slowest/
> please see below
> >
> >> this frequency. However, due to the fact that the other buses follows the
> >> WCORE bus by taking the OPP from their table with the same id, e.g. opp02,
> >> the children frequency should be stable with the set voltage.
> >> It could cause random faults very hard to debug.
> >> Thus, the patch removes the lowest OPP to make other buses' lowest OPPs
> >
> > s/lowest/slowest/
> Actually, I have double checked that, because we always used this
> terminology: low OPP, high OPP, lower OPPs, higher OPPs. I can change
> it here for you, but I think this is not something that people are used
> to. Please check EAS pdf documentation or this file:
> https://www.kernel.org/doc/Documentation/scheduler/sched-energy.txt
> i.e. "running at a lower OPP" or "high OPPs", "lowest OPPs".

Hmm, indeed, you're right. Don't change it then.

Best regards,
Krzysztof

Patch
diff mbox series

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index f8c36ff0d4c3..a355c76af5a5 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -1107,22 +1107,18 @@ 
 			compatible = "operating-points-v2";
 
 			opp00 {
-				opp-hz = /bits/ 64 <84000000>;
-				opp-microvolt = <925000>;
+				opp-hz = /bits/ 64 <150000000>;
+				opp-microvolt = <950000>;
 			};
 			opp01 {
-				opp-hz = /bits/ 64 <111000000>;
+				opp-hz = /bits/ 64 <200000000>;
 				opp-microvolt = <950000>;
 			};
 			opp02 {
-				opp-hz = /bits/ 64 <222000000>;
+				opp-hz = /bits/ 64 <300000000>;
 				opp-microvolt = <950000>;
 			};
 			opp03 {
-				opp-hz = /bits/ 64 <333000000>;
-				opp-microvolt = <950000>;
-			};
-			opp04 {
 				opp-hz = /bits/ 64 <400000000>;
 				opp-microvolt = <987500>;
 			};