diff mbox

[v5,3/7] ARM: dts: Exynos542x/5800: add CPU OPP properties

Message ID 1449766729-435-4-git-send-email-b.zolnierkie@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bartlomiej Zolnierkiewicz Dec. 10, 2015, 4:58 p.m. UTC
From: Thomas Abraham <thomas.ab@samsung.com>

For Exynos542x/5800 platforms, add CPU operating points
for migrating from Exynos specific cpufreq driver to using
generic cpufreq driver.

Changes by Bartlomiej:
- split Exynos5420 support from the original patch
- merged Exynos5422 fixes from Ben

Changes by Ben Gamari:
- Port to operating-points-v2

Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Javier Martinez Canillas <javier@osg.samsung.com>
Cc: Andreas Faerber <afaerber@suse.de>
Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
Signed-off-by: Ben Gamari <ben@smart-cactus.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 arch/arm/boot/dts/exynos5420.dtsi      | 122 +++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/exynos5422-cpus.dtsi |  10 +++
 2 files changed, 132 insertions(+)

Comments

Krzysztof Kozlowski Dec. 11, 2015, 1:17 a.m. UTC | #1
On 11.12.2015 01:58, Bartlomiej Zolnierkiewicz wrote:
> From: Thomas Abraham <thomas.ab@samsung.com>
> 
> For Exynos542x/5800 platforms, add CPU operating points
> for migrating from Exynos specific cpufreq driver to using
> generic cpufreq driver.
> 
> Changes by Bartlomiej:
> - split Exynos5420 support from the original patch
> - merged Exynos5422 fixes from Ben
> 
> Changes by Ben Gamari:
> - Port to operating-points-v2
> 
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> Cc: Andreas Faerber <afaerber@suse.de>
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> Signed-off-by: Ben Gamari <ben@smart-cactus.org>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5420.dtsi      | 122 +++++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/exynos5422-cpus.dtsi |  10 +++
>  2 files changed, 132 insertions(+)
> 

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 11, 2015, 3:16 a.m. UTC | #2
On 10-12-15, 17:58, Bartlomiej Zolnierkiewicz wrote:
> 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>;
>  };

Why do you need to update this file? This file is included by
exynos5422-odroidxu3-common.dtsi, which already inherits cpus nodes
from exynos5800.dtsi (which inherits exynos5420.dtsi).

i.e. operating-points-v2 should already be set.
Javier Martinez Canillas Dec. 11, 2015, 3:25 a.m. UTC | #3
Hello Viresh,

On 12/11/2015 12:16 AM, Viresh Kumar wrote:
> On 10-12-15, 17:58, Bartlomiej Zolnierkiewicz wrote:
>> 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>;
>>  };
> 
> Why do you need to update this file? This file is included by
> exynos5422-odroidxu3-common.dtsi, which already inherits cpus nodes
> from exynos5800.dtsi (which inherits exynos5420.dtsi).
> 
> i.e. operating-points-v2 should already be set.
>

The problem is that the big and LITTLE cores have different ordering per SoCs:

- Exynos5420 and Exynos5800: cpu0-3 (Cortex-A15) and cpu4-7 (Coretx-A7)
- Exynos5422: cpu0-3 (Cortex-A7) and cpu4-7 (Cortex-A15)

So the OPP tables are set in this DTSI file, to prevent the OPP tables
in the Exynos5422 to be inverted for the cluster 0 and 1.

Best regards,
Viresh Kumar Dec. 11, 2015, 3:32 a.m. UTC | #4
On 11-12-15, 00:25, Javier Martinez Canillas wrote:
> The problem is that the big and LITTLE cores have different ordering per SoCs:
> 
> - Exynos5420 and Exynos5800: cpu0-3 (Cortex-A15) and cpu4-7 (Coretx-A7)
> - Exynos5422: cpu0-3 (Cortex-A7) and cpu4-7 (Cortex-A15)
> 
> So the OPP tables are set in this DTSI file, to prevent the OPP tables
> in the Exynos5422 to be inverted for the cluster 0 and 1.

Oh dude, that's really *ugly*. :)

Reusing files/definitions is fine to the point where things are
readable. But you have screwed it up so very badly.

Over that, why can't you keep cpu0-3 as A7 and 4-7 as a15 for all the
cases? The only worrying thing for you should be that CPU0 within the
kenrel should be controllable, right? i.e. you want a A15 to boot 5800
and A7 to boot 5422.

If yes, than you could have kept the CPUs in 5422 as:
0-3: A7
4-7: A15

and in 5420 as:
4-7: A15
0-3: A7

Wouldnt' that work ?
Krzysztof Kozlowski Dec. 11, 2015, 4 a.m. UTC | #5
On 11.12.2015 12:32, Viresh Kumar wrote:
> On 11-12-15, 00:25, Javier Martinez Canillas wrote:
>> The problem is that the big and LITTLE cores have different ordering per SoCs:
>>
>> - Exynos5420 and Exynos5800: cpu0-3 (Cortex-A15) and cpu4-7 (Coretx-A7)
>> - Exynos5422: cpu0-3 (Cortex-A7) and cpu4-7 (Cortex-A15)
>>
>> So the OPP tables are set in this DTSI file, to prevent the OPP tables
>> in the Exynos5422 to be inverted for the cluster 0 and 1.
> 
> Oh dude, that's really *ugly*. :)
> 
> Reusing files/definitions is fine to the point where things are
> readable. But you have screwed it up so very badly.
> 
> Over that, why can't you keep cpu0-3 as A7 and 4-7 as a15 for all the
> cases? The only worrying thing for you should be that CPU0 within the
> kenrel should be controllable, right? i.e. you want a A15 to boot 5800
> and A7 to boot 5422.
> 
> If yes, than you could have kept the CPUs in 5422 as:
> 0-3: A7
> 4-7: A15
> 
> and in 5420 as:
> 4-7: A15
> 0-3: A7
> 
> Wouldnt' that work ?

It wasn't working like this. The cpu0 got the index from booting cpu, so
on 5420 cpu0 was A15 and on 5422 it was A7.

Maybe I am not aware of some changes recently in the kernel but how do
you want to assign the booting CPU proper number (not CPU0)?

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 11, 2015, 4:13 a.m. UTC | #6
On 11-12-15, 13:00, Krzysztof Kozlowski wrote:
> It wasn't working like this. The cpu0 got the index from booting cpu, so
> on 5420 cpu0 was A15 and on 5422 it was A7.
> 
> Maybe I am not aware of some changes recently in the kernel but how do
> you want to assign the booting CPU proper number (not CPU0)?

Okay, this is how it works and I don't think you need to change the
order of CPUs based on machines.

The boot CPU isn't controlled by the DT file but your bootloader, so
it will be A15 on 5420 and A7 on 5422.

Now if you keep the order in DT as: 0-3 A7 and 4-7 A15 irrespective of
the SoCs, then this is how they will be assigned.

Linux CPU numbers               Actual CPU assigned to them
                                5420            5422

CPU0                            A15-0 (boot)    A7-0 (boot)
CPU1                            A7-0            A7-1
CPU2                            A7-1            A7-2
CPU3                            A7-2            A7-3
CPU4                            A7-3            A15-0
CPU5                            A15-1           A15-1
CPU6                            A15-2           A15-2
CPU7                            A15-3           A15-3

This happens because all non-boot CPUs will be added in the order they
are present in DT.

Now, there should be no *real* reason for you to want your CPUs to be
always contiguous, isn't it?

In the case of 5420, cpufreq will show related CPUs as:
Policy0: CPU0,5-7, Policy1: CPU1-4

and in the case of 5422, cpufreq will show related CPUs as:
Policy0: CPU0-3, Policy1: CPU4-7

And I think you should really fix this now..
Krzysztof Kozlowski Dec. 11, 2015, 4:18 a.m. UTC | #7
On 11.12.2015 13:13, Viresh Kumar wrote:
> On 11-12-15, 13:00, Krzysztof Kozlowski wrote:
>> It wasn't working like this. The cpu0 got the index from booting cpu, so
>> on 5420 cpu0 was A15 and on 5422 it was A7.
>>
>> Maybe I am not aware of some changes recently in the kernel but how do
>> you want to assign the booting CPU proper number (not CPU0)?
> 
> Okay, this is how it works and I don't think you need to change the
> order of CPUs based on machines.
> 
> The boot CPU isn't controlled by the DT file but your bootloader, so
> it will be A15 on 5420 and A7 on 5422.
> 
> Now if you keep the order in DT as: 0-3 A7 and 4-7 A15 irrespective of
> the SoCs, then this is how they will be assigned.
> 
> Linux CPU numbers               Actual CPU assigned to them
>                                 5420            5422
> 
> CPU0                            A15-0 (boot)    A7-0 (boot)
> CPU1                            A7-0            A7-1
> CPU2                            A7-1            A7-2
> CPU3                            A7-2            A7-3
> CPU4                            A7-3            A15-0
> CPU5                            A15-1           A15-1
> CPU6                            A15-2           A15-2
> CPU7                            A15-3           A15-3
> 
> This happens because all non-boot CPUs will be added in the order they
> are present in DT.
> 
> Now, there should be no *real* reason for you to want your CPUs to be
> always contiguous, isn't it?
> 
> In the case of 5420, cpufreq will show related CPUs as:
> Policy0: CPU0,5-7, Policy1: CPU1-4
> 
> and in the case of 5422, cpufreq will show related CPUs as:
> Policy0: CPU0-3, Policy1: CPU4-7
> 
> And I think you should really fix this now..

We had such configuration before (before df09df6f9ac3). I don't see any
benefit in what you described. Where is the "thing" to be fixed? It is
mixed up. The contiguous ordering is easier to read and more natural.

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 11, 2015, 4:38 a.m. UTC | #8
On 11-12-15, 13:18, Krzysztof Kozlowski wrote:
> We had such configuration before (before df09df6f9ac3). I don't see any
> benefit in what you described. Where is the "thing" to be fixed? It is
> mixed up. The contiguous ordering is easier to read and more natural.

This is what you are doing today (keeping on one CPU per cluster to
simplify it):

		cpu0: cpu@0 {
			device_type = "cpu";
			compatible = "arm,cortex-a15";
			reg = <0x0>;
			clock-frequency = <1800000000>;
			cci-control-port = <&cci_control1>;
		};

		cpu4: cpu@100 {
			device_type = "cpu";
			compatible = "arm,cortex-a7";
			reg = <0x100>;
			clock-frequency = <1000000000>;
			cci-control-port = <&cci_control0>;
		};


Then you overwrite it with:

                &cpu0 {
                	device_type = "cpu";
                	compatible = "arm,cortex-a7";
                	reg = <0x100>;
                	clock-frequency = <1000000000>;
                	cci-control-port = <&cci_control0>;
                };
                
                &cpu4 {
                	device_type = "cpu";
                	compatible = "arm,cortex-a15";
                	reg = <0x0>;
                	clock-frequency = <1800000000>;
                	cci-control-port = <&cci_control1>;
                };


Don't you think this isn't the right way of solving problems?

The DT overwrite feature isn't there to do such kind of stuff, though
it doesn't stop you from doing that.

Either you should keep separate paths for both the SoCs, or can solve
it the way I suggested earlier.

This came up because in the current series you are doing this:

		cpu0: cpu@0 {
			compatible = "arm,cortex-a15";
                        operating-points-v2 = <&cpu0_opp_table>;
		};

		cpu4: cpu@100 {
			device_type = "cpu";
			compatible = "arm,cortex-a7";
                        operating-points-v2 = <&cpu1_opp_table>;
		};


Then you overwrite it with:

                &cpu0 {
                	compatible = "arm,cortex-a7";
                        operating-points-v2 = <&cpu1_opp_table>;
                };
                
                &cpu4 {
                	compatible = "arm,cortex-a15";
                        operating-points-v2 = <&cpu0_opp_table>;
                };
Viresh Kumar Dec. 11, 2015, 4:39 a.m. UTC | #9
On 10-12-15, 17:58, Bartlomiej Zolnierkiewicz wrote:
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> index 48a0a55..616c2d0 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -50,6 +50,116 @@
>  		usbdrdphy1 = &usbdrd_phy1;
>  	};
>  
> +	cpu0_opp_table: opp_table0 {

Over and above what we are discussing in this thread, this should be
named cluster0_opp_table or cluster_a7_opp_table, not cpu0 as the
other one is never for cpu1 but cpu4.
Krzysztof Kozlowski Dec. 11, 2015, 4:53 a.m. UTC | #10
On 11.12.2015 13:38, Viresh Kumar wrote:
> On 11-12-15, 13:18, Krzysztof Kozlowski wrote:
>> We had such configuration before (before df09df6f9ac3). I don't see any
>> benefit in what you described. Where is the "thing" to be fixed? It is
>> mixed up. The contiguous ordering is easier to read and more natural.
> 
> This is what you are doing today (keeping on one CPU per cluster to
> simplify it):
> 
> 		cpu0: cpu@0 {
> 			device_type = "cpu";
> 			compatible = "arm,cortex-a15";
> 			reg = <0x0>;
> 			clock-frequency = <1800000000>;
> 			cci-control-port = <&cci_control1>;
> 		};
> 
> 		cpu4: cpu@100 {
> 			device_type = "cpu";
> 			compatible = "arm,cortex-a7";
> 			reg = <0x100>;
> 			clock-frequency = <1000000000>;
> 			cci-control-port = <&cci_control0>;
> 		};
> 
> 
> Then you overwrite it with:
> 
>                 &cpu0 {
>                 	device_type = "cpu";
>                 	compatible = "arm,cortex-a7";
>                 	reg = <0x100>;
>                 	clock-frequency = <1000000000>;
>                 	cci-control-port = <&cci_control0>;
>                 };
>                 
>                 &cpu4 {
>                 	device_type = "cpu";
>                 	compatible = "arm,cortex-a15";
>                 	reg = <0x0>;
>                 	clock-frequency = <1800000000>;
>                 	cci-control-port = <&cci_control1>;
>                 };
> 
> 
> Don't you think this isn't the right way of solving problems?
> 
> The DT overwrite feature isn't there to do such kind of stuff, though
> it doesn't stop you from doing that.

This is quite ugly, indeed, and it is getting uglier :)... but it does
not violate the idea of DT to describe the hardware. Both hardware
descriptions - the 5420 and overridden - are entirely correct... because
the CPU ordering comes from booting sequence (actually code in IROM
decides according to pulled up GPIO gpg2-1).


> Either you should keep separate paths for both the SoCs,

I like that idea. That makes it much more readable. Thanks for feedback!
I will send a patch for that.


> or can solve
> it the way I suggested earlier.
> 
> This came up because in the current series you are doing this:
> 
> 		cpu0: cpu@0 {
> 			compatible = "arm,cortex-a15";
>                         operating-points-v2 = <&cpu0_opp_table>;
> 		};
> 
> 		cpu4: cpu@100 {
> 			device_type = "cpu";
> 			compatible = "arm,cortex-a7";
>                         operating-points-v2 = <&cpu1_opp_table>;
> 		};
> 
> 
> Then you overwrite it with:
> 
>                 &cpu0 {
>                 	compatible = "arm,cortex-a7";
>                         operating-points-v2 = <&cpu1_opp_table>;
>                 };
>                 
>                 &cpu4 {
>                 	compatible = "arm,cortex-a15";
>                         operating-points-v2 = <&cpu0_opp_table>;
>                 };

Yes, it is getting uglier with each change...

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Javier Martinez Canillas Dec. 11, 2015, 4:53 a.m. UTC | #11
Hello Viresh,

On 12/11/2015 01:38 AM, Viresh Kumar wrote:
> On 11-12-15, 13:18, Krzysztof Kozlowski wrote:
>> We had such configuration before (before df09df6f9ac3). I don't see any
>> benefit in what you described. Where is the "thing" to be fixed? It is
>> mixed up. The contiguous ordering is easier to read and more natural.
> 
> This is what you are doing today (keeping on one CPU per cluster to
> simplify it):
> 
> 		cpu0: cpu@0 {
> 			device_type = "cpu";
> 			compatible = "arm,cortex-a15";
> 			reg = <0x0>;
> 			clock-frequency = <1800000000>;
> 			cci-control-port = <&cci_control1>;
> 		};
> 
> 		cpu4: cpu@100 {
> 			device_type = "cpu";
> 			compatible = "arm,cortex-a7";
> 			reg = <0x100>;
> 			clock-frequency = <1000000000>;
> 			cci-control-port = <&cci_control0>;
> 		};
> 
> 
> Then you overwrite it with:
> 
>                 &cpu0 {
>                 	device_type = "cpu";
>                 	compatible = "arm,cortex-a7";
>                 	reg = <0x100>;
>                 	clock-frequency = <1000000000>;
>                 	cci-control-port = <&cci_control0>;
>                 };
>                 
>                 &cpu4 {
>                 	device_type = "cpu";
>                 	compatible = "arm,cortex-a15";
>                 	reg = <0x0>;
>                 	clock-frequency = <1800000000>;
>                 	cci-control-port = <&cci_control1>;
>                 };
> 
> 
> Don't you think this isn't the right way of solving problems?
> 
> The DT overwrite feature isn't there to do such kind of stuff, though
> it doesn't stop you from doing that.
>

I still fail to understand why the override is not a good way to solve
the issue.
 
> Either you should keep separate paths for both the SoCs, or can solve

There's no point IMHO to duplicate the HW description since is the only
difference between the Exynos5422 and Exynos5800 SoCs AFAIK.

> it the way I suggested earlier.
>

As Krzysztof said, contiguous ordering is more natural and easier to
read and I agree with him.

> This came up because in the current series you are doing this:
> 
> 		cpu0: cpu@0 {
> 			compatible = "arm,cortex-a15";
>                         operating-points-v2 = <&cpu0_opp_table>;
> 		};
> 
> 		cpu4: cpu@100 {
> 			device_type = "cpu";
> 			compatible = "arm,cortex-a7";
>                         operating-points-v2 = <&cpu1_opp_table>;
> 		};
> 
> 
> Then you overwrite it with:
> 
>                 &cpu0 {
>                 	compatible = "arm,cortex-a7";
>                         operating-points-v2 = <&cpu1_opp_table>;

But yes, I agree with you that the OPP tables names are misleading at the
very least for not saying wrong. So yes, either {cluster0,a7,kfc}_opp_table
as you suggest are much better possible names and the same for the a15 one.

Best regards,
Krzysztof Kozlowski Dec. 11, 2015, 5:28 a.m. UTC | #12
On 11.12.2015 13:53, Javier Martinez Canillas wrote:
> Hello Viresh,
> 
> On 12/11/2015 01:38 AM, Viresh Kumar wrote:
>> On 11-12-15, 13:18, Krzysztof Kozlowski wrote:
>>> We had such configuration before (before df09df6f9ac3). I don't see any
>>> benefit in what you described. Where is the "thing" to be fixed? It is
>>> mixed up. The contiguous ordering is easier to read and more natural.
>>
>> This is what you are doing today (keeping on one CPU per cluster to
>> simplify it):
>>
>> 		cpu0: cpu@0 {
>> 			device_type = "cpu";
>> 			compatible = "arm,cortex-a15";
>> 			reg = <0x0>;
>> 			clock-frequency = <1800000000>;
>> 			cci-control-port = <&cci_control1>;
>> 		};
>>
>> 		cpu4: cpu@100 {
>> 			device_type = "cpu";
>> 			compatible = "arm,cortex-a7";
>> 			reg = <0x100>;
>> 			clock-frequency = <1000000000>;
>> 			cci-control-port = <&cci_control0>;
>> 		};
>>
>>
>> Then you overwrite it with:
>>
>>                 &cpu0 {
>>                 	device_type = "cpu";
>>                 	compatible = "arm,cortex-a7";
>>                 	reg = <0x100>;
>>                 	clock-frequency = <1000000000>;
>>                 	cci-control-port = <&cci_control0>;
>>                 };
>>                 
>>                 &cpu4 {
>>                 	device_type = "cpu";
>>                 	compatible = "arm,cortex-a15";
>>                 	reg = <0x0>;
>>                 	clock-frequency = <1800000000>;
>>                 	cci-control-port = <&cci_control1>;
>>                 };
>>
>>
>> Don't you think this isn't the right way of solving problems?
>>
>> The DT overwrite feature isn't there to do such kind of stuff, though
>> it doesn't stop you from doing that.
>>
> 
> I still fail to understand why the override is not a good way to solve
> the issue.
>  
>> Either you should keep separate paths for both the SoCs, or can solve
> 
> There's no point IMHO to duplicate the HW description since is the only
> difference between the Exynos5422 and Exynos5800 SoCs AFAIK.

Actually I think there is no nice way of making this as separate paths.
As Javier's mentioned, there aren't many differences. Currently the CPU
ordering is the only difference in DT.

Making it as separate path would create hierarchy like:
 - exynos5420-based-board.dts
   \- include: exynos5420.dtsi
      \- include: exynos5.dtsi
   \- include: exynos5420-cpu.dtsi (the cpus are not in exynos5420.dtsi)

 - exynos5422-based-board.dts
   \- include: exynos5420.dtsi
      \- include: exynos5.dtsi
   \- include: exynos5422-cpu.dtsi (the cpus are not in exynos5420.dtsi)

which of course is okay... except we keep the definition of CPUs
completely outside of main Exynos5420 DTSI. Then we have to include both
DTSI for each new DTS.

Other idea is to create artificial "exynos5420-common":
 - exynos5420-based-board.dts
   \- include: exynos5420.dtsi
       \- include: exynos5420-common.dtsi
          \- include: exynos5.dtsi
       \- include: exynos5420-cpu.dtsi

 - exynos5422-based-board.dts
   \- include: exynos5422.dtsi
      \- include: exynos5420-common.dtsi
         \- include: exynos5.dtsi
      \- include: exynos5422-cpu.dtsi

This is also confusing...

Any third idea?

Best regards,
KRzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 11, 2015, 5:41 a.m. UTC | #13
On 11-12-15, 14:28, Krzysztof Kozlowski wrote:
> Actually I think there is no nice way of making this as separate paths.
> As Javier's mentioned, there aren't many differences. Currently the CPU
> ordering is the only difference in DT.
> 
> Making it as separate path would create hierarchy like:
>  - exynos5420-based-board.dts
>    \- include: exynos5420.dtsi
>       \- include: exynos5.dtsi
>    \- include: exynos5420-cpu.dtsi (the cpus are not in exynos5420.dtsi)
> 
>  - exynos5422-based-board.dts
>    \- include: exynos5420.dtsi
>       \- include: exynos5.dtsi
>    \- include: exynos5422-cpu.dtsi (the cpus are not in exynos5420.dtsi)
> 
> which of course is okay... except we keep the definition of CPUs
> completely outside of main Exynos5420 DTSI. Then we have to include both
> DTSI for each new DTS.

So what? There isn't anything wrong in this case and is just the right
thing to do, IMHO. We have just kept the CPU devices separately,
simple.
diff mbox

Patch

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 48a0a55..616c2d0 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -50,6 +50,116 @@ 
 		usbdrdphy1 = &usbdrd_phy1;
 	};
 
+	cpu0_opp_table: opp_table0 {
+		compatible = "operating-points-v2";
+		opp-shared;
+		opp@1800000000 {
+			opp-hz = /bits/ 64 <1800000000>;
+			opp-microvolt = <1250000>;
+			clock-latency-ns = <140000>;
+		};
+		opp@1700000000 {
+			opp-hz = /bits/ 64 <1700000000>;
+			opp-microvolt = <1212500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@1600000000 {
+			opp-hz = /bits/ 64 <1600000000>;
+			opp-microvolt = <1175000>;
+			clock-latency-ns = <140000>;
+		};
+		opp@1500000000 {
+			opp-hz = /bits/ 64 <1500000000>;
+			opp-microvolt = <1137500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@1400000000 {
+			opp-hz = /bits/ 64 <1400000000>;
+			opp-microvolt = <1112500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@1300000000 {
+			opp-hz = /bits/ 64 <1300000000>;
+			opp-microvolt = <1062500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@1200000000 {
+			opp-hz = /bits/ 64 <1200000000>;
+			opp-microvolt = <1037500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@1100000000 {
+			opp-hz = /bits/ 64 <1100000000>;
+			opp-microvolt = <1012500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@1000000000 {
+			opp-hz = /bits/ 64 <1000000000>;
+			opp-microvolt = < 987500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@900000000 {
+			opp-hz = /bits/ 64 <900000000>;
+			opp-microvolt = < 962500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@800000000 {
+			opp-hz = /bits/ 64 <800000000>;
+			opp-microvolt = < 937500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@700000000 {
+			opp-hz = /bits/ 64 <700000000>;
+			opp-microvolt = < 912500>;
+			clock-latency-ns = <140000>;
+		};
+	};
+
+	cpu1_opp_table: opp_table1 {
+		compatible = "operating-points-v2";
+		opp-shared;
+		opp@1300000000 {
+			opp-hz = /bits/ 64 <1300000000>;
+			opp-microvolt = <1275000>;
+			clock-latency-ns = <140000>;
+		};
+		opp@1200000000 {
+			opp-hz = /bits/ 64 <1200000000>;
+			opp-microvolt = <1212500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@1100000000 {
+			opp-hz = /bits/ 64 <1100000000>;
+			opp-microvolt = <1162500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@1000000000 {
+			opp-hz = /bits/ 64 <1000000000>;
+			opp-microvolt = <1112500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@900000000 {
+			opp-hz = /bits/ 64 <900000000>;
+			opp-microvolt = <1062500>;
+			clock-latency-ns = <140000>;
+		};
+		opp@800000000 {
+			opp-hz = /bits/ 64 <800000000>;
+			opp-microvolt = <1025000>;
+			clock-latency-ns = <140000>;
+		};
+		opp@700000000 {
+			opp-hz = /bits/ 64 <700000000>;
+			opp-microvolt = <975000>;
+			clock-latency-ns = <140000>;
+		};
+		opp@600000000 {
+			opp-hz = /bits/ 64 <600000000>;
+			opp-microvolt = <937500>;
+			clock-latency-ns = <140000>;
+		};
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
@@ -58,8 +168,11 @@ 
 			device_type = "cpu";
 			compatible = "arm,cortex-a15";
 			reg = <0x0>;
+			clocks = <&clock CLK_ARM_CLK>;
+			clock-names = "cpu-cluster.0";
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			operating-points-v2 = <&cpu0_opp_table>;
 		};
 
 		cpu1: cpu@1 {
@@ -68,6 +181,7 @@ 
 			reg = <0x1>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			operating-points-v2 = <&cpu0_opp_table>;
 		};
 
 		cpu2: cpu@2 {
@@ -76,6 +190,7 @@ 
 			reg = <0x2>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			operating-points-v2 = <&cpu0_opp_table>;
 		};
 
 		cpu3: cpu@3 {
@@ -84,14 +199,18 @@ 
 			reg = <0x3>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+			operating-points-v2 = <&cpu0_opp_table>;
 		};
 
 		cpu4: cpu@100 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a7";
 			reg = <0x100>;
+			clocks = <&clock CLK_KFC_CLK>;
+			clock-names = "cpu-cluster.1";
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			operating-points-v2 = <&cpu1_opp_table>;
 		};
 
 		cpu5: cpu@101 {
@@ -100,6 +219,7 @@ 
 			reg = <0x101>;
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			operating-points-v2 = <&cpu1_opp_table>;
 		};
 
 		cpu6: cpu@102 {
@@ -108,6 +228,7 @@ 
 			reg = <0x102>;
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			operating-points-v2 = <&cpu1_opp_table>;
 		};
 
 		cpu7: cpu@103 {
@@ -116,6 +237,7 @@ 
 			reg = <0x103>;
 			clock-frequency = <1000000000>;
 			cci-control-port = <&cci_control0>;
+			operating-points-v2 = <&cpu1_opp_table>;
 		};
 	};
 
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>;
 };