diff mbox

[v4,7/8] ARM: dts: Exynos5422: fix OPP tables

Message ID 1449512300-17230-8-git-send-email-b.zolnierkie@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bartlomiej Zolnierkiewicz Dec. 7, 2015, 6:18 p.m. UTC
From: Ben Gamari <ben@smart-cactus.org>

The Exynos 5422 is identical to the 5800 except for the fact that it
boots from the A7 cores. Consequently, the core numbering is different:
cores 0-3 are A7s whereas 4-7 are A15s.

We can reuse the device tree of the 5800 for the 5422 but we must take
care to override the OPP tables and CPU clocks.  These are otherwise
inherited from the exynos5800 devicetree, which has the CPU clusters
reversed compared to the 5422. This results in the A15 cores only
reaching 1.4GHz, the maximum rate of the KFC clock.

Cc: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Ben Gamari <ben@smart-cactus.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 arch/arm/boot/dts/exynos5422-cpus.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Krzysztof Kozlowski Dec. 8, 2015, 8:13 a.m. UTC | #1
On 08.12.2015 03:18, Bartlomiej Zolnierkiewicz wrote:
> From: Ben Gamari <ben@smart-cactus.org>
> 
> The Exynos 5422 is identical to the 5800 except for the fact that it
> boots from the A7 cores. Consequently, the core numbering is different:
> cores 0-3 are A7s whereas 4-7 are A15s.
> 
> We can reuse the device tree of the 5800 for the 5422 but we must take
> care to override the OPP tables and CPU clocks.  These are otherwise
> inherited from the exynos5800 devicetree, which has the CPU clusters
> reversed compared to the 5422. This results in the A15 cores only
> reaching 1.4GHz, the maximum rate of the KFC clock.
> 
> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> Signed-off-by: Ben Gamari <ben@smart-cactus.org>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5422-cpus.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

This looks like a very-non-atomic way of handling a change. You added
opp tables to exynos5420 before so at that time they will be applied to
Odroid XU3 family which uses different CPU order. After that you are
fixing the tables to proper CPU order. Direct bisectability probably
won't be an issue because all of DTS would go to separate branch... but
the logic behind confuses.

I think this should be squashed into 3/8.

Best regards,
Krzysztof

> diff --git a/arch/arm/boot/dts/exynos5422-cpus.dtsi b/arch/arm/boot/dts/exynos5422-cpus.dtsi
> index b7f60c8..9a5131d 100644
> --- a/arch/arm/boot/dts/exynos5422-cpus.dtsi
> +++ b/arch/arm/boot/dts/exynos5422-cpus.dtsi
> @@ -20,8 +20,10 @@
>  	device_type = "cpu";
>  	compatible = "arm,cortex-a7";
>  	reg = <0x100>;
> +	clocks = <&clock CLK_KFC_CLK>;
>  	clock-frequency = <1000000000>;
>  	cci-control-port = <&cci_control0>;
> +	operating-points-v2 = <&cpu1_opp_table>;
>  };
>  
>  &cpu1 {
> @@ -30,6 +32,7 @@
>  	reg = <0x101>;
>  	clock-frequency = <1000000000>;
>  	cci-control-port = <&cci_control0>;
> +	operating-points-v2 = <&cpu1_opp_table>;
>  };
>  
>  &cpu2 {
> @@ -38,6 +41,7 @@
>  	reg = <0x102>;
>  	clock-frequency = <1000000000>;
>  	cci-control-port = <&cci_control0>;
> +	operating-points-v2 = <&cpu1_opp_table>;
>  };
>  
>  &cpu3 {
> @@ -46,14 +50,17 @@
>  	reg = <0x103>;
>  	clock-frequency = <1000000000>;
>  	cci-control-port = <&cci_control0>;
> +	operating-points-v2 = <&cpu1_opp_table>;
>  };
>  
>  &cpu4 {
>  	device_type = "cpu";
>  	compatible = "arm,cortex-a15";
>  	reg = <0x0>;
> +	clocks = <&clock CLK_ARM_CLK>;
>  	clock-frequency = <1800000000>;
>  	cci-control-port = <&cci_control1>;
> +	operating-points-v2 = <&cpu0_opp_table>;
>  };
>  
>  &cpu5 {
> @@ -62,6 +69,7 @@
>  	reg = <0x1>;
>  	clock-frequency = <1800000000>;
>  	cci-control-port = <&cci_control1>;
> +	operating-points-v2 = <&cpu0_opp_table>;
>  };
>  
>  &cpu6 {
> @@ -70,6 +78,7 @@
>  	reg = <0x2>;
>  	clock-frequency = <1800000000>;
>  	cci-control-port = <&cci_control1>;
> +	operating-points-v2 = <&cpu0_opp_table>;
>  };
>  
>  &cpu7 {
> @@ -78,4 +87,5 @@
>  	reg = <0x3>;
>  	clock-frequency = <1800000000>;
>  	cci-control-port = <&cci_control1>;
> +	operating-points-v2 = <&cpu0_opp_table>;
>  };
>
Javier Martinez Canillas Dec. 8, 2015, 1:41 p.m. UTC | #2
Hello Krzysztof,

On 12/08/2015 05:13 AM, Krzysztof Kozlowski wrote:
> On 08.12.2015 03:18, Bartlomiej Zolnierkiewicz wrote:
>> From: Ben Gamari <ben@smart-cactus.org>
>>
>> The Exynos 5422 is identical to the 5800 except for the fact that it
>> boots from the A7 cores. Consequently, the core numbering is different:
>> cores 0-3 are A7s whereas 4-7 are A15s.
>>
>> We can reuse the device tree of the 5800 for the 5422 but we must take
>> care to override the OPP tables and CPU clocks.  These are otherwise
>> inherited from the exynos5800 devicetree, which has the CPU clusters
>> reversed compared to the 5422. This results in the A15 cores only
>> reaching 1.4GHz, the maximum rate of the KFC clock.
>>
>> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
>> Signed-off-by: Ben Gamari <ben@smart-cactus.org>
>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>> ---
>>  arch/arm/boot/dts/exynos5422-cpus.dtsi | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
> 
> This looks like a very-non-atomic way of handling a change. You added
> opp tables to exynos5420 before so at that time they will be applied to
> Odroid XU3 family which uses different CPU order. After that you are
> fixing the tables to proper CPU order. Direct bisectability probably
> won't be an issue because all of DTS would go to separate branch... but
> the logic behind confuses.
> 

Agreed.

> I think this should be squashed into 3/8.
> 

I think the patch should be split in two changes, the CPUs device nodes
having the wrong clock for clusters is a bug and has to be fixed in a
patch before adding the OPP tables and the OPP tables changes should be
separated and merged with patch 3/8 as you suggest.

> Best regards,
> Krzysztof
> 

Best regards,
Krzysztof Kozlowski Dec. 9, 2015, 12:34 a.m. UTC | #3
On 08.12.2015 22:41, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 12/08/2015 05:13 AM, Krzysztof Kozlowski wrote:
>> On 08.12.2015 03:18, Bartlomiej Zolnierkiewicz wrote:
>>> From: Ben Gamari <ben@smart-cactus.org>
>>>
>>> The Exynos 5422 is identical to the 5800 except for the fact that it
>>> boots from the A7 cores. Consequently, the core numbering is different:
>>> cores 0-3 are A7s whereas 4-7 are A15s.
>>>
>>> We can reuse the device tree of the 5800 for the 5422 but we must take
>>> care to override the OPP tables and CPU clocks.  These are otherwise
>>> inherited from the exynos5800 devicetree, which has the CPU clusters
>>> reversed compared to the 5422. This results in the A15 cores only
>>> reaching 1.4GHz, the maximum rate of the KFC clock.
>>>
>>> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
>>> Signed-off-by: Ben Gamari <ben@smart-cactus.org>
>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>> ---
>>>  arch/arm/boot/dts/exynos5422-cpus.dtsi | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>
>> This looks like a very-non-atomic way of handling a change. You added
>> opp tables to exynos5420 before so at that time they will be applied to
>> Odroid XU3 family which uses different CPU order. After that you are
>> fixing the tables to proper CPU order. Direct bisectability probably
>> won't be an issue because all of DTS would go to separate branch... but
>> the logic behind confuses.
>>
> 
> Agreed.
> 
>> I think this should be squashed into 3/8.
>>
> 
> I think the patch should be split in two changes, the CPUs device nodes
> having the wrong clock for clusters is a bug and has to be fixed in a
> patch before adding the OPP tables and the OPP tables changes should be
> separated and merged with patch 3/8 as you suggest.

I don't get the point about wrong clock (bug). Where is the bug? Beside
of course what was introduced in 3/8 and it is not valid for reversed
cluster order.

Best regards,
Krzysztof
Javier Martinez Canillas Dec. 9, 2015, 2:19 a.m. UTC | #4
Hello Krzysztof,

On 12/08/2015 09:34 PM, Krzysztof Kozlowski wrote:
> On 08.12.2015 22:41, Javier Martinez Canillas wrote:
>> On 12/08/2015 05:13 AM, Krzysztof Kozlowski wrote:
>>>
>>> This looks like a very-non-atomic way of handling a change. You added
>>> opp tables to exynos5420 before so at that time they will be applied to
>>> Odroid XU3 family which uses different CPU order. After that you are
>>> fixing the tables to proper CPU order. Direct bisectability probably
>>> won't be an issue because all of DTS would go to separate branch... but
>>> the logic behind confuses.
>>>
>>
>> Agreed.
>>
>>> I think this should be squashed into 3/8.
>>>
>>
>> I think the patch should be split in two changes, the CPUs device nodes
>> having the wrong clock for clusters is a bug and has to be fixed in a
>> patch before adding the OPP tables and the OPP tables changes should be
>> separated and merged with patch 3/8 as you suggest.
> 
> I don't get the point about wrong clock (bug). Where is the bug? Beside
> of course what was introduced in 3/8 and it is not valid for reversed
> cluster order.
>

You are absolutely correct, for some reason I thought that the CLK_ARM_CLK
and CLK_KFC_CLK clocks were already defined in the cpu0 and cpu4 nodes from
exynos5420.dtsi and commit df09df6f9ac3 ("ARM: dts: add exynos5422-cpus.dtsi
to correct cpu order") missed that when reversing the cores for Exynos5422.

But on a second look to patch 3/8, I see that the clocks are defined in that
patch so I agree that $SUBJECT should just be squashed with 3/8 without doing
any split. Sorry for the noise.

> Best regards,
> Krzysztof
> 

Best regards,
Bartlomiej Zolnierkiewicz Dec. 10, 2015, 3:57 p.m. UTC | #5
Hi,

On Tuesday, December 08, 2015 05:13:26 PM Krzysztof Kozlowski wrote:
> On 08.12.2015 03:18, Bartlomiej Zolnierkiewicz wrote:
> > From: Ben Gamari <ben@smart-cactus.org>
> > 
> > The Exynos 5422 is identical to the 5800 except for the fact that it
> > boots from the A7 cores. Consequently, the core numbering is different:
> > cores 0-3 are A7s whereas 4-7 are A15s.
> > 
> > We can reuse the device tree of the 5800 for the 5422 but we must take
> > care to override the OPP tables and CPU clocks.  These are otherwise
> > inherited from the exynos5800 devicetree, which has the CPU clusters
> > reversed compared to the 5422. This results in the A15 cores only
> > reaching 1.4GHz, the maximum rate of the KFC clock.
> > 
> > Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> > Signed-off-by: Ben Gamari <ben@smart-cactus.org>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > ---
> >  arch/arm/boot/dts/exynos5422-cpus.dtsi | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> 
> This looks like a very-non-atomic way of handling a change. You added
> opp tables to exynos5420 before so at that time they will be applied to
> Odroid XU3 family which uses different CPU order. After that you are
> fixing the tables to proper CPU order. Direct bisectability probably
> won't be an issue because all of DTS would go to separate branch... but
> the logic behind confuses.

It was done this way to preserve original authorship of changes and
bisectability should not be affected unless patch #8 is merged before
patch #7.

> I think this should be squashed into 3/8.

OK.

> Best regards,
> Krzysztof

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
diff mbox

Patch

diff --git a/arch/arm/boot/dts/exynos5422-cpus.dtsi b/arch/arm/boot/dts/exynos5422-cpus.dtsi
index b7f60c8..9a5131d 100644
--- a/arch/arm/boot/dts/exynos5422-cpus.dtsi
+++ b/arch/arm/boot/dts/exynos5422-cpus.dtsi
@@ -20,8 +20,10 @@ 
 	device_type = "cpu";
 	compatible = "arm,cortex-a7";
 	reg = <0x100>;
+	clocks = <&clock CLK_KFC_CLK>;
 	clock-frequency = <1000000000>;
 	cci-control-port = <&cci_control0>;
+	operating-points-v2 = <&cpu1_opp_table>;
 };
 
 &cpu1 {
@@ -30,6 +32,7 @@ 
 	reg = <0x101>;
 	clock-frequency = <1000000000>;
 	cci-control-port = <&cci_control0>;
+	operating-points-v2 = <&cpu1_opp_table>;
 };
 
 &cpu2 {
@@ -38,6 +41,7 @@ 
 	reg = <0x102>;
 	clock-frequency = <1000000000>;
 	cci-control-port = <&cci_control0>;
+	operating-points-v2 = <&cpu1_opp_table>;
 };
 
 &cpu3 {
@@ -46,14 +50,17 @@ 
 	reg = <0x103>;
 	clock-frequency = <1000000000>;
 	cci-control-port = <&cci_control0>;
+	operating-points-v2 = <&cpu1_opp_table>;
 };
 
 &cpu4 {
 	device_type = "cpu";
 	compatible = "arm,cortex-a15";
 	reg = <0x0>;
+	clocks = <&clock CLK_ARM_CLK>;
 	clock-frequency = <1800000000>;
 	cci-control-port = <&cci_control1>;
+	operating-points-v2 = <&cpu0_opp_table>;
 };
 
 &cpu5 {
@@ -62,6 +69,7 @@ 
 	reg = <0x1>;
 	clock-frequency = <1800000000>;
 	cci-control-port = <&cci_control1>;
+	operating-points-v2 = <&cpu0_opp_table>;
 };
 
 &cpu6 {
@@ -70,6 +78,7 @@ 
 	reg = <0x2>;
 	clock-frequency = <1800000000>;
 	cci-control-port = <&cci_control1>;
+	operating-points-v2 = <&cpu0_opp_table>;
 };
 
 &cpu7 {
@@ -78,4 +87,5 @@ 
 	reg = <0x3>;
 	clock-frequency = <1800000000>;
 	cci-control-port = <&cci_control1>;
+	operating-points-v2 = <&cpu0_opp_table>;
 };