linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm/dt: Don't add disabled CPUs to system topology
@ 2013-06-06 17:11 James King
  2013-06-06 17:35 ` Nicolas Pitre
  2013-06-07 10:23 ` Lorenzo Pieralisi
  0 siblings, 2 replies; 8+ messages in thread
From: James King @ 2013-06-06 17:11 UTC (permalink / raw)
  To: grant.likely, rob.herring, rob, linux, lorenzo.pieralisi, nico,
	vincent.guittot, namhyung, a.p.zijlstra, devicetree-discuss,
	linux-doc, linux-kernel, linux-arm-kernel
  Cc: james.king, patches, linaro-kernel

If CPUs are marked as disabled in the devicetree, make sure they do
not exist in the system CPU information and CPU topology information.
In this case these CPUs will not be able to be added to the system later
using hot-plug. This allows a single chip with many CPUs to be easily
used in a variety of hardware devices where they may have different
actual processing requirements (eg for thermal/cost reasons).

- Change devicetree.c to ignore any cpu nodes marked as disabled,
  this effectively limits the number of active cpu cores so no need
  for the max_cpus=x in the chosen node.
- Change topology.c to ignore any cpu nodes marked as disabled, this
  is where the scheduler would learn about big/LITTLE cores so this
  effectively keeps the scheduler in sync.

Signed-off-by: James King <james.king@linaro.org>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 5 +++++
 arch/arm/kernel/devtree.c                      | 6 ++++++
 arch/arm/kernel/topology.c                     | 4 ++++
 3 files changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index f32494d..9fbcbc5 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -45,6 +45,10 @@ For the ARM architecture every CPU node must contain the following properties:
 		"marvell,xsc3"
 		"marvell,xscale"
 
+And optionally set the following properties:
+
+- status:	can be set to "disabled" to remove that CPU from the system CPU topology
+
 Example:
 
 	cpus {
@@ -73,5 +77,6 @@ Example:
 			device_type = "cpu";
 			compatible = "arm,cortex-a7";
 			reg = <0x101>;
+			status = "disabled";
 		};
 	};
diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index 5af04f6..f4ba8ee 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -112,6 +112,12 @@ void __init arm_dt_init_cpu_maps(void)
 			return;
 
 		/*
+		 * Check if the cpu is marked as "disabled", if so ignore.
+		 */
+		if (!of_device_is_available(cpu))
+			continue;
+
+		/*
 		 * Duplicate MPIDRs are a recipe for disaster.
 		 * Scan all initialized entries and check for
 		 * duplicates. If any is found just bail out.
diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index f10316b..90f8fb3 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -116,6 +116,10 @@ static void __init parse_dt_topology(void)
 		if (cpu_eff->compatible == NULL)
 			continue;
 
+		/* Check if the cpu is marked as "disabled", if so ignore. */
+		if (!of_device_is_available(cn))
+			continue;
+
 		rate = of_get_property(cn, "clock-frequency", &len);
 		if (!rate || len != 4) {
 			pr_err("%s missing clock-frequency property\n",
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm/dt: Don't add disabled CPUs to system topology
  2013-06-06 17:11 [PATCH] arm/dt: Don't add disabled CPUs to system topology James King
@ 2013-06-06 17:35 ` Nicolas Pitre
  2013-06-07 10:23 ` Lorenzo Pieralisi
  1 sibling, 0 replies; 8+ messages in thread
From: Nicolas Pitre @ 2013-06-06 17:35 UTC (permalink / raw)
  To: James King
  Cc: grant.likely, rob.herring, rob, Russell King - ARM Linux,
	lorenzo.pieralisi, vincent.guittot, namhyung, a.p.zijlstra,
	devicetree-discuss, linux-doc, linux-kernel, linux-arm-kernel,
	patches, linaro-kernel

On Thu, 6 Jun 2013, James King wrote:

> If CPUs are marked as disabled in the devicetree, make sure they do
> not exist in the system CPU information and CPU topology information.
> In this case these CPUs will not be able to be added to the system later
> using hot-plug. This allows a single chip with many CPUs to be easily
> used in a variety of hardware devices where they may have different
> actual processing requirements (eg for thermal/cost reasons).
> 
> - Change devicetree.c to ignore any cpu nodes marked as disabled,
>   this effectively limits the number of active cpu cores so no need
>   for the max_cpus=x in the chosen node.
> - Change topology.c to ignore any cpu nodes marked as disabled, this
>   is where the scheduler would learn about big/LITTLE cores so this
>   effectively keeps the scheduler in sync.
> 
> Signed-off-by: James King <james.king@linaro.org>

Acked-by: Nicolas Pitre <nico@linaro.org>


> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 5 +++++
>  arch/arm/kernel/devtree.c                      | 6 ++++++
>  arch/arm/kernel/topology.c                     | 4 ++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index f32494d..9fbcbc5 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -45,6 +45,10 @@ For the ARM architecture every CPU node must contain the following properties:
>  		"marvell,xsc3"
>  		"marvell,xscale"
>  
> +And optionally set the following properties:
> +
> +- status:	can be set to "disabled" to remove that CPU from the system CPU topology
> +
>  Example:
>  
>  	cpus {
> @@ -73,5 +77,6 @@ Example:
>  			device_type = "cpu";
>  			compatible = "arm,cortex-a7";
>  			reg = <0x101>;
> +			status = "disabled";
>  		};
>  	};
> diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
> index 5af04f6..f4ba8ee 100644
> --- a/arch/arm/kernel/devtree.c
> +++ b/arch/arm/kernel/devtree.c
> @@ -112,6 +112,12 @@ void __init arm_dt_init_cpu_maps(void)
>  			return;
>  
>  		/*
> +		 * Check if the cpu is marked as "disabled", if so ignore.
> +		 */
> +		if (!of_device_is_available(cpu))
> +			continue;
> +
> +		/*
>  		 * Duplicate MPIDRs are a recipe for disaster.
>  		 * Scan all initialized entries and check for
>  		 * duplicates. If any is found just bail out.
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index f10316b..90f8fb3 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -116,6 +116,10 @@ static void __init parse_dt_topology(void)
>  		if (cpu_eff->compatible == NULL)
>  			continue;
>  
> +		/* Check if the cpu is marked as "disabled", if so ignore. */
> +		if (!of_device_is_available(cn))
> +			continue;
> +
>  		rate = of_get_property(cn, "clock-frequency", &len);
>  		if (!rate || len != 4) {
>  			pr_err("%s missing clock-frequency property\n",
> -- 
> 1.8.1.2
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm/dt: Don't add disabled CPUs to system topology
  2013-06-06 17:11 [PATCH] arm/dt: Don't add disabled CPUs to system topology James King
  2013-06-06 17:35 ` Nicolas Pitre
@ 2013-06-07 10:23 ` Lorenzo Pieralisi
  2013-06-07 11:48   ` James King
  2013-06-07 14:20   ` Rob Herring
  1 sibling, 2 replies; 8+ messages in thread
From: Lorenzo Pieralisi @ 2013-06-07 10:23 UTC (permalink / raw)
  To: James King
  Cc: grant.likely, rob.herring, rob, linux, nico, vincent.guittot,
	namhyung, a.p.zijlstra, devicetree-discuss, linux-doc,
	linux-kernel, linux-arm-kernel, patches, linaro-kernel

Hi James,

On Thu, Jun 06, 2013 at 06:11:25PM +0100, James King wrote:
> If CPUs are marked as disabled in the devicetree, make sure they do
> not exist in the system CPU information and CPU topology information.
> In this case these CPUs will not be able to be added to the system later
> using hot-plug. This allows a single chip with many CPUs to be easily
> used in a variety of hardware devices where they may have different
> actual processing requirements (eg for thermal/cost reasons).
> 
> - Change devicetree.c to ignore any cpu nodes marked as disabled,
>   this effectively limits the number of active cpu cores so no need
>   for the max_cpus=x in the chosen node.
> - Change topology.c to ignore any cpu nodes marked as disabled, this
>   is where the scheduler would learn about big/LITTLE cores so this
>   effectively keeps the scheduler in sync.
> 

I have two questions:

1) Since with this approach the DT should change anyway if on different
   hardware devices based on the same chip you want to allow booting a
   different number of CPUs, why do not we remove the cpu nodes instead of
   disabling them ? Put it another way: cpu nodes define a cpu as
   possible (currently), we can simply remove the node if we do not want
   that cpu to be seen by the kernel.
2) If we go for the "status" property, why do not we use it to set present
   mask ? That way the cpu is possible but not present, you cannot
   hotplug it in. It is a bit of a stretch, granted, the cpu _is_ present,
   we just want to disable it, do not know how this is handled in x86
   and other archs though.

I am just asking, since it is something I thought about while writing
code that parses the DT cpu map, basically we do not have a way to
"disable" a cpu in the DT and that's what you are doing, I just would like
to understand the best way to put it into DT bindings.

Thanks,
Lorenzo


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm/dt: Don't add disabled CPUs to system topology
  2013-06-07 10:23 ` Lorenzo Pieralisi
@ 2013-06-07 11:48   ` James King
  2013-06-07 16:41     ` Lorenzo Pieralisi
  2013-06-07 14:20   ` Rob Herring
  1 sibling, 1 reply; 8+ messages in thread
From: James King @ 2013-06-07 11:48 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: grant.likely, rob.herring, rob, linux, nico, vincent.guittot,
	namhyung, a.p.zijlstra, devicetree-discuss, linux-doc,
	linux-kernel, linux-arm-kernel, patches, linaro-kernel

Hi Lorenzo,

On 7 June 2013 11:23, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> Hi James,
>
> On Thu, Jun 06, 2013 at 06:11:25PM +0100, James King wrote:
>> If CPUs are marked as disabled in the devicetree, make sure they do
>> not exist in the system CPU information and CPU topology information.
>> In this case these CPUs will not be able to be added to the system later
>> using hot-plug. This allows a single chip with many CPUs to be easily
>> used in a variety of hardware devices where they may have different
>> actual processing requirements (eg for thermal/cost reasons).
>>
>> - Change devicetree.c to ignore any cpu nodes marked as disabled,
>>   this effectively limits the number of active cpu cores so no need
>>   for the max_cpus=x in the chosen node.
>> - Change topology.c to ignore any cpu nodes marked as disabled, this
>>   is where the scheduler would learn about big/LITTLE cores so this
>>   effectively keeps the scheduler in sync.
>>
>
> I have two questions:
>
> 1) Since with this approach the DT should change anyway if on different
>    hardware devices based on the same chip you want to allow booting a
>    different number of CPUs, why do not we remove the cpu nodes instead of
>    disabling them ? Put it another way: cpu nodes define a cpu as
>    possible (currently), we can simply remove the node if we do not want
>    that cpu to be seen by the kernel.

The reason we want disabled status rather than just remove the nodes
is to use a common soc.dtsi file which is included in many board.dts
files - eg:

file soc.dtsi contains:

                cpus {
                                cpu0: cpu@0 {
                                                device_type = "cpu";
                                                compatible = "arm,cortex-a7";
                                                reg = <0>;
                                                cluster = <&cluster0>;
                                                core = <&core0>;
                                };

                                cpu1: cpu@1 {
                                                device_type = "cpu";
                                                compatible = "arm,cortex-a7";
                                                reg = <1>;
                                                cluster = <&cluster0>;
                                                core = <&core1>;
                                };

                                cpu2: cpu@2 {
                                                device_type = "cpu";
                                                compatible = "arm,cortex-a15";
                                                reg = <2>;
                                                cluster = <&cluster0>;
                                                core = <&core2>;
                                };
                };

file board1.dts where we want the A15 disabled contains:

/include/ "soc.dtsi"

                cpus {
                                cpu2: cpu@2 {
                                                status = "disabled";
                                };
                };

> 2) If we go for the "status" property, why do not we use it to set present
>    mask ? That way the cpu is possible but not present, you cannot
>    hotplug it in. It is a bit of a stretch, granted, the cpu _is_ present,
>    we just want to disable it, do not know how this is handled in x86
>    and other archs though.

I have been struggling to find any equivalent example for another
arch, so just tried to solve our problem. I guess in the x86 world it
is less likely to want to disable processors in a SoC for heat/battery
issues so this has just never arisen. In this case the cpu is
physically present but not possible, but I am not sure it should be in
a present mask (giving the impression it can be used). Perhaps you
could elaborate with an example what you are thinking about here?

Thanks,
James
>
> I am just asking, since it is something I thought about while writing
> code that parses the DT cpu map, basically we do not have a way to
> "disable" a cpu in the DT and that's what you are doing, I just would like
> to understand the best way to put it into DT bindings.
>
> Thanks,
> Lorenzo
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm/dt: Don't add disabled CPUs to system topology
  2013-06-07 10:23 ` Lorenzo Pieralisi
  2013-06-07 11:48   ` James King
@ 2013-06-07 14:20   ` Rob Herring
  2013-06-07 16:13     ` Lorenzo Pieralisi
  1 sibling, 1 reply; 8+ messages in thread
From: Rob Herring @ 2013-06-07 14:20 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: James King, linaro-kernel, a.p.zijlstra, nico,
	devicetree-discuss, linux-doc, linux-kernel, rob.herring,
	patches, namhyung, grant.likely, linux, linux-arm-kernel

On 06/07/2013 05:23 AM, Lorenzo Pieralisi wrote:
> Hi James,
> 
> On Thu, Jun 06, 2013 at 06:11:25PM +0100, James King wrote:
>> If CPUs are marked as disabled in the devicetree, make sure they do
>> not exist in the system CPU information and CPU topology information.
>> In this case these CPUs will not be able to be added to the system later
>> using hot-plug. This allows a single chip with many CPUs to be easily
>> used in a variety of hardware devices where they may have different
>> actual processing requirements (eg for thermal/cost reasons).
>>
>> - Change devicetree.c to ignore any cpu nodes marked as disabled,
>>   this effectively limits the number of active cpu cores so no need
>>   for the max_cpus=x in the chosen node.
>> - Change topology.c to ignore any cpu nodes marked as disabled, this
>>   is where the scheduler would learn about big/LITTLE cores so this
>>   effectively keeps the scheduler in sync.
>>
> 
> I have two questions:
> 
> 1) Since with this approach the DT should change anyway if on different
>    hardware devices based on the same chip you want to allow booting a
>    different number of CPUs, why do not we remove the cpu nodes instead of
>    disabling them ? Put it another way: cpu nodes define a cpu as
>    possible (currently), we can simply remove the node if we do not want
>    that cpu to be seen by the kernel.
> 2) If we go for the "status" property, why do not we use it to set present
>    mask ? That way the cpu is possible but not present, you cannot
>    hotplug it in. It is a bit of a stretch, granted, the cpu _is_ present,
>    we just want to disable it, do not know how this is handled in x86
>    and other archs though.

The meaning of disabled for cpus in ePAPR is that the cpu is offline
(i.e. in a spinloop or wfi), not that the cpu is unavailable. This is a
bit of a departure and inconsistency from how status for devices are
used. That would imply that we should be setting status to disabled for
all secondary cores and that possibly the status value should get
updated to reflect the state of the cpu.

Rob

> I am just asking, since it is something I thought about while writing
> code that parses the DT cpu map, basically we do not have a way to
> "disable" a cpu in the DT and that's what you are doing, I just would like
> to understand the best way to put it into DT bindings.
> 
> Thanks,
> Lorenzo
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm/dt: Don't add disabled CPUs to system topology
  2013-06-07 14:20   ` Rob Herring
@ 2013-06-07 16:13     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 8+ messages in thread
From: Lorenzo Pieralisi @ 2013-06-07 16:13 UTC (permalink / raw)
  To: Rob Herring
  Cc: James King, linaro-kernel, a.p.zijlstra, nico,
	devicetree-discuss, linux-doc, linux-kernel, rob.herring,
	patches, namhyung, grant.likely, linux, linux-arm-kernel

On Fri, Jun 07, 2013 at 03:20:20PM +0100, Rob Herring wrote:
> On 06/07/2013 05:23 AM, Lorenzo Pieralisi wrote:
> > Hi James,
> > 
> > On Thu, Jun 06, 2013 at 06:11:25PM +0100, James King wrote:
> >> If CPUs are marked as disabled in the devicetree, make sure they do
> >> not exist in the system CPU information and CPU topology information.
> >> In this case these CPUs will not be able to be added to the system later
> >> using hot-plug. This allows a single chip with many CPUs to be easily
> >> used in a variety of hardware devices where they may have different
> >> actual processing requirements (eg for thermal/cost reasons).
> >>
> >> - Change devicetree.c to ignore any cpu nodes marked as disabled,
> >>   this effectively limits the number of active cpu cores so no need
> >>   for the max_cpus=x in the chosen node.
> >> - Change topology.c to ignore any cpu nodes marked as disabled, this
> >>   is where the scheduler would learn about big/LITTLE cores so this
> >>   effectively keeps the scheduler in sync.
> >>
> > 
> > I have two questions:
> > 
> > 1) Since with this approach the DT should change anyway if on different
> >    hardware devices based on the same chip you want to allow booting a
> >    different number of CPUs, why do not we remove the cpu nodes instead of
> >    disabling them ? Put it another way: cpu nodes define a cpu as
> >    possible (currently), we can simply remove the node if we do not want
> >    that cpu to be seen by the kernel.
> > 2) If we go for the "status" property, why do not we use it to set present
> >    mask ? That way the cpu is possible but not present, you cannot
> >    hotplug it in. It is a bit of a stretch, granted, the cpu _is_ present,
> >    we just want to disable it, do not know how this is handled in x86
> >    and other archs though.
> 
> The meaning of disabled for cpus in ePAPR is that the cpu is offline
> (i.e. in a spinloop or wfi), not that the cpu is unavailable. This is a
> bit of a departure and inconsistency from how status for devices are
> used. That would imply that we should be setting status to disabled for
> all secondary cores and that possibly the status value should get
> updated to reflect the state of the cpu.

Yes, that's what I understood from the ePAPR as well. According to
the ePAPR, as you say, a cpu with its status property == "disabled" is a
possible CPU, since it can be enabled (through a specific enable-method).

I am not sure "status" can be reused for the purpose this patch was developed
for without changing the bindings in the ePAPR (ie if DT parsing skips
cpu nodes with status == "disabled", this is a significant departure
from what ePAPR defines, and it would force us to define an enable-method
to enable/online those CPUs which is not what this patch was developed for).

How was PowerPC tackling the problem James set about solving ?

Lorenzo


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm/dt: Don't add disabled CPUs to system topology
  2013-06-07 11:48   ` James King
@ 2013-06-07 16:41     ` Lorenzo Pieralisi
  2013-06-10 10:48       ` James King
  0 siblings, 1 reply; 8+ messages in thread
From: Lorenzo Pieralisi @ 2013-06-07 16:41 UTC (permalink / raw)
  To: James King
  Cc: grant.likely, rob.herring, rob, linux, nico, vincent.guittot,
	namhyung, a.p.zijlstra, devicetree-discuss, linux-doc,
	linux-kernel, linux-arm-kernel, patches, linaro-kernel

On Fri, Jun 07, 2013 at 12:48:58PM +0100, James King wrote:
> Hi Lorenzo,
> 
> On 7 June 2013 11:23, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > Hi James,
> >
> > On Thu, Jun 06, 2013 at 06:11:25PM +0100, James King wrote:
> >> If CPUs are marked as disabled in the devicetree, make sure they do
> >> not exist in the system CPU information and CPU topology information.
> >> In this case these CPUs will not be able to be added to the system later
> >> using hot-plug. This allows a single chip with many CPUs to be easily
> >> used in a variety of hardware devices where they may have different
> >> actual processing requirements (eg for thermal/cost reasons).
> >>
> >> - Change devicetree.c to ignore any cpu nodes marked as disabled,
> >>   this effectively limits the number of active cpu cores so no need
> >>   for the max_cpus=x in the chosen node.
> >> - Change topology.c to ignore any cpu nodes marked as disabled, this
> >>   is where the scheduler would learn about big/LITTLE cores so this
> >>   effectively keeps the scheduler in sync.
> >>
> >
> > I have two questions:
> >
> > 1) Since with this approach the DT should change anyway if on different
> >    hardware devices based on the same chip you want to allow booting a
> >    different number of CPUs, why do not we remove the cpu nodes instead of
> >    disabling them ? Put it another way: cpu nodes define a cpu as
> >    possible (currently), we can simply remove the node if we do not want
> >    that cpu to be seen by the kernel.
> 
> The reason we want disabled status rather than just remove the nodes
> is to use a common soc.dtsi file which is included in many board.dts
> files - eg:
> 
> file soc.dtsi contains:
> 
>                 cpus {
>                                 cpu0: cpu@0 {
>                                                 device_type = "cpu";
>                                                 compatible = "arm,cortex-a7";
>                                                 reg = <0>;
>                                                 cluster = <&cluster0>;
>                                                 core = <&core0>;

Minor nit, "cluster" and "core" phandles are not part of cpu the bindings
that will be merged this cycle, I know it is just an example.

>                                 };
> 
>                                 cpu1: cpu@1 {
>                                                 device_type = "cpu";
>                                                 compatible = "arm,cortex-a7";
>                                                 reg = <1>;
>                                                 cluster = <&cluster0>;
>                                                 core = <&core1>;
>                                 };
> 
>                                 cpu2: cpu@2 {
>                                                 device_type = "cpu";
>                                                 compatible = "arm,cortex-a15";
>                                                 reg = <2>;
>                                                 cluster = <&cluster0>;
>                                                 core = <&core2>;
>                                 };
>                 };
> 
> file board1.dts where we want the A15 disabled contains:
> 
> /include/ "soc.dtsi"
> 
>                 cpus {
>                                 cpu2: cpu@2 {
>                                                 status = "disabled";
>                                 };
>                 };

Understood, see the other reply as far as the status property is concerned.

> > 2) If we go for the "status" property, why do not we use it to set present
> >    mask ? That way the cpu is possible but not present, you cannot
> >    hotplug it in. It is a bit of a stretch, granted, the cpu _is_ present,
> >    we just want to disable it, do not know how this is handled in x86
> >    and other archs though.
> 
> I have been struggling to find any equivalent example for another
> arch, so just tried to solve our problem. I guess in the x86 world it
> is less likely to want to disable processors in a SoC for heat/battery
> issues so this has just never arisen. In this case the cpu is
> physically present but not possible, but I am not sure it should be in
> a present mask (giving the impression it can be used). Perhaps you
> could elaborate with an example what you are thinking about here?

I was thinking about using status == "disabled" to mark a cpu as
possible but not present; that is a bad idea for the reason you mentioned
and also for the point Rob raised related to the ePAPR.

I am not sure how we can solve this issue, as I mentioned the easiest
solution consists in not defining cpu nodes in the DT or probably add
an additional property != status with proper bindings attached to it.

Lorenzo


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] arm/dt: Don't add disabled CPUs to system topology
  2013-06-07 16:41     ` Lorenzo Pieralisi
@ 2013-06-10 10:48       ` James King
  0 siblings, 0 replies; 8+ messages in thread
From: James King @ 2013-06-10 10:48 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: grant.likely, rob.herring, rob, linux, nico, vincent.guittot,
	namhyung, a.p.zijlstra, devicetree-discuss, linux-doc,
	linux-kernel, linux-arm-kernel, patches, linaro-kernel

On 7 June 2013 17:41, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Fri, Jun 07, 2013 at 12:48:58PM +0100, James King wrote:
>> Hi Lorenzo,
>>
>> On 7 June 2013 11:23, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> > Hi James,
>> >
>> > On Thu, Jun 06, 2013 at 06:11:25PM +0100, James King wrote:
>> >> If CPUs are marked as disabled in the devicetree, make sure they do
>> >> not exist in the system CPU information and CPU topology information.
>> >> In this case these CPUs will not be able to be added to the system later
>> >> using hot-plug. This allows a single chip with many CPUs to be easily
>> >> used in a variety of hardware devices where they may have different
>> >> actual processing requirements (eg for thermal/cost reasons).
>> >>
>> >> - Change devicetree.c to ignore any cpu nodes marked as disabled,
>> >>   this effectively limits the number of active cpu cores so no need
>> >>   for the max_cpus=x in the chosen node.
>> >> - Change topology.c to ignore any cpu nodes marked as disabled, this
>> >>   is where the scheduler would learn about big/LITTLE cores so this
>> >>   effectively keeps the scheduler in sync.
>> >>
>> >
>> > I have two questions:
>> >
>> > 1) Since with this approach the DT should change anyway if on different
>> >    hardware devices based on the same chip you want to allow booting a
>> >    different number of CPUs, why do not we remove the cpu nodes instead of
>> >    disabling them ? Put it another way: cpu nodes define a cpu as
>> >    possible (currently), we can simply remove the node if we do not want
>> >    that cpu to be seen by the kernel.
>>
>> The reason we want disabled status rather than just remove the nodes
>> is to use a common soc.dtsi file which is included in many board.dts
>> files - eg:
>>
>> file soc.dtsi contains:
>>
>>                 cpus {
>>                                 cpu0: cpu@0 {
>>                                                 device_type = "cpu";
>>                                                 compatible = "arm,cortex-a7";
>>                                                 reg = <0>;
>>                                                 cluster = <&cluster0>;
>>                                                 core = <&core0>;
>
> Minor nit, "cluster" and "core" phandles are not part of cpu the bindings
> that will be merged this cycle, I know it is just an example.
>
>>                                 };
>>
>>                                 cpu1: cpu@1 {
>>                                                 device_type = "cpu";
>>                                                 compatible = "arm,cortex-a7";
>>                                                 reg = <1>;
>>                                                 cluster = <&cluster0>;
>>                                                 core = <&core1>;
>>                                 };
>>
>>                                 cpu2: cpu@2 {
>>                                                 device_type = "cpu";
>>                                                 compatible = "arm,cortex-a15";
>>                                                 reg = <2>;
>>                                                 cluster = <&cluster0>;
>>                                                 core = <&core2>;
>>                                 };
>>                 };
>>
>> file board1.dts where we want the A15 disabled contains:
>>
>> /include/ "soc.dtsi"
>>
>>                 cpus {
>>                                 cpu2: cpu@2 {
>>                                                 status = "disabled";
>>                                 };
>>                 };
>
> Understood, see the other reply as far as the status property is concerned.
>
>> > 2) If we go for the "status" property, why do not we use it to set present
>> >    mask ? That way the cpu is possible but not present, you cannot
>> >    hotplug it in. It is a bit of a stretch, granted, the cpu _is_ present,
>> >    we just want to disable it, do not know how this is handled in x86
>> >    and other archs though.
>>
>> I have been struggling to find any equivalent example for another
>> arch, so just tried to solve our problem. I guess in the x86 world it
>> is less likely to want to disable processors in a SoC for heat/battery
>> issues so this has just never arisen. In this case the cpu is
>> physically present but not possible, but I am not sure it should be in
>> a present mask (giving the impression it can be used). Perhaps you
>> could elaborate with an example what you are thinking about here?
>
> I was thinking about using status == "disabled" to mark a cpu as
> possible but not present; that is a bad idea for the reason you mentioned
> and also for the point Rob raised related to the ePAPR.
>
> I am not sure how we can solve this issue, as I mentioned the easiest
> solution consists in not defining cpu nodes in the DT or probably add
> an additional property != status with proper bindings attached to it.
>

So what the ePAPR says is:
"disabled" == Indicates that the device is not presently operational,
but it might
become operational in the future (for example, something is not
plugged in, or switched off).
Refer to the device binding for details on what disabled means for
a given device.

So, would "disabled" be ok to use if those cores 'could' be re-added
to the system using hot-plug?

This would probably be fine for our usage as the user would have to
have the right permissions to hot-plug in a cpu anyway (so is just as
strong a protection as the DT anyway - if you root your device you can
burn it). The main thing is the scheduler will not try and use it by
default.

If that sounds ok - I need to go back and look at the changes to see
what will still allow a core to be hot-plugged in.

Thanks,
James

> Lorenzo
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2013-06-10 10:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-06 17:11 [PATCH] arm/dt: Don't add disabled CPUs to system topology James King
2013-06-06 17:35 ` Nicolas Pitre
2013-06-07 10:23 ` Lorenzo Pieralisi
2013-06-07 11:48   ` James King
2013-06-07 16:41     ` Lorenzo Pieralisi
2013-06-10 10:48       ` James King
2013-06-07 14:20   ` Rob Herring
2013-06-07 16:13     ` Lorenzo Pieralisi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).