[RFC PATCH] ARM: mvebu: Let the device-tree determine smp_ops

[RFC PATCH] ARM: mvebu: Let the device-tree determine smp_ops

[Chris Packham]

The machine specific SMP operations can be configured either via
setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
both of these are called and setup_arch wins because it is called last.
This means that it is not possible to substitute a different set of SMP
operations via the device-tree.

All of the ARMADA_370_XP_DT compatible devices are either single core or
declare an enable-method in the dts (via one of armada-xp-mv78230.dtsi,
armada-xp-mv78260.dtsi or armada-xp-mv78460.dtsi). Remove the smp
assignment from board-v7.c so that the SMP operations set via the
device-tree aren't discarded.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Hi,

(This is the first patch I've sent to the arm-lkml so beware of my newbie-ness)

I'm working on a new board that uses a integrated CPU that according to the
vendor is "based on the ARMADA-XP", more on that to come in the future
hopefully. For the most part things just work if I use a .dts based on the
mv78260.

One difference I've encountered is in the way the secondary CPU is initialised,
which requires me to supply a slightly different set of machine specific SMP
operations so I can bring up the 2nd core. It looks like I should be able to
supply a different /cpus/enable-method in the .dts and once I define the
corresponding set of operations it should be picked up. 

Which leads me to what I think could be a bug (or just a lack of clear
specification). The smp ops are set by both arm_dt_init_cpu_maps (from the
enable-method) and setup_arch (from mdesc). The latter always wins because it
is called last and doesn't check to see if smp_ops has already been set.

This is my attempt at fixing the problem by not setting mdesc.smp for the
ARMADA_370_XP_DT machines thus preventing setup_arch from overriding what has
been setup by arm_dt_init_cpu_maps.

Thanks,
Chris

 arch/arm/mach-mvebu/board-v7.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index 6478626..894974b 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -206,7 +206,6 @@ static const char * const armada_370_xp_dt_compat[] = {
 DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
 	.l2c_aux_val	= 0,
 	.l2c_aux_mask	= ~0,
-	.smp		= smp_ops(armada_xp_smp_ops),
 	.init_machine	= mvebu_dt_init,
 	.init_irq       = mvebu_init_irq,
 	.restart	= mvebu_restart,
-- 
2.2.0.rc0

[RFC PATCH] ARM: mvebu: Let the device-tree determine smp_ops

[Andrew Lunn]

On Thu, Nov 06, 2014 at 05:49:56PM +1300, Chris Packham wrote:
> The machine specific SMP operations can be configured either via
> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
> both of these are called and setup_arch wins because it is called last.
> This means that it is not possible to substitute a different set of SMP
> operations via the device-tree.
> 
> All of the ARMADA_370_XP_DT compatible devices are either single core or
> declare an enable-method in the dts (via one of armada-xp-mv78230.dtsi,
> armada-xp-mv78260.dtsi or armada-xp-mv78460.dtsi). Remove the smp
> assignment from board-v7.c so that the SMP operations set via the
> device-tree aren't discarded.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> Hi,
> 
> (This is the first patch I've sent to the arm-lkml so beware of my newbie-ness)

Hi Chris

Congratulations on a good looking patch. Nice changelog, has a
signed-off-by, comments are bellow the ---. All good.

It is however a good idea to Cc: the mvebu maintainers. I added a few
people to Cc:.

> I'm working on a new board that uses a integrated CPU that according to the
> vendor is "based on the ARMADA-XP", more on that to come in the future
> hopefully. 

A packet processor chip with an embedded CPU? Next generation
Prestera?  No need to answer that if you don't want to.

> For the most part things just work if I use a .dts based on the
> mv78260.
 
> This is my attempt at fixing the problem by not setting mdesc.smp for the
> ARMADA_370_XP_DT machines thus preventing setup_arch from overriding what has
> been setup by arm_dt_init_cpu_maps.

You really need Thomas or Gregory to Ack this patch, but to me it
looks correct.  Interestingly, this machine descriptor is used by 370,
which is a single processor. The smp operations are useless, so your
patch is also partly a cleanup.

     Andrew

[RFC PATCH] ARM: mvebu: Let the device-tree determine smp_ops

[Thomas Petazzoni]

Dear Chris Packham,

On Thu,  6 Nov 2014 17:49:56 +1300, Chris Packham wrote:
> The machine specific SMP operations can be configured either via
> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
> both of these are called and setup_arch wins because it is called last.
> This means that it is not possible to substitute a different set of SMP
> operations via the device-tree.
> 
> All of the ARMADA_370_XP_DT compatible devices are either single core or
> declare an enable-method in the dts (via one of armada-xp-mv78230.dtsi,
> armada-xp-mv78260.dtsi or armada-xp-mv78460.dtsi). Remove the smp
> assignment from board-v7.c so that the SMP operations set via the
> device-tree aren't discarded.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> Hi,
> 
> (This is the first patch I've sent to the arm-lkml so beware of my newbie-ness)
> 
> I'm working on a new board that uses a integrated CPU that according to the
> vendor is "based on the ARMADA-XP", more on that to come in the future
> hopefully. For the most part things just work if I use a .dts based on the
> mv78260.
> 
> One difference I've encountered is in the way the secondary CPU is initialised,
> which requires me to supply a slightly different set of machine specific SMP
> operations so I can bring up the 2nd core. It looks like I should be able to
> supply a different /cpus/enable-method in the .dts and once I define the
> corresponding set of operations it should be picked up. 
> 
> Which leads me to what I think could be a bug (or just a lack of clear
> specification). The smp ops are set by both arm_dt_init_cpu_maps (from the
> enable-method) and setup_arch (from mdesc). The latter always wins because it
> is called last and doesn't check to see if smp_ops has already been set.
> 
> This is my attempt at fixing the problem by not setting mdesc.smp for the
> ARMADA_370_XP_DT machines thus preventing setup_arch from overriding what has
> been setup by arm_dt_init_cpu_maps.
> 
> Thanks,
> Chris
> 
>  arch/arm/mach-mvebu/board-v7.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
> index 6478626..894974b 100644
> --- a/arch/arm/mach-mvebu/board-v7.c
> +++ b/arch/arm/mach-mvebu/board-v7.c
> @@ -206,7 +206,6 @@ static const char * const armada_370_xp_dt_compat[] = {
>  DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
>  	.l2c_aux_val	= 0,
>  	.l2c_aux_mask	= ~0,
> -	.smp		= smp_ops(armada_xp_smp_ops),
>  	.init_machine	= mvebu_dt_init,
>  	.init_irq       = mvebu_init_irq,
>  	.restart	= mvebu_restart,

There is a very good reason to keep this .smp initialization. The
Device Tree for Armada XP used to *not* have the enable-method
property, since the SMP enable-method binding did not exist at the
time we introduced the Armada XP SMP support. Therefore, if we want to
keep backward compatibility with the old Armada XP DTs and continue to
have SMP support working with those, we need to keep this .smp
initialization essentially forever.

Yes, backward compatibility sometimes sucks :-/

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[RFC PATCH] ARM: mvebu: Let the device-tree determine smp_ops

[Andrew Lunn]

> > diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
> > index 6478626..894974b 100644
> > --- a/arch/arm/mach-mvebu/board-v7.c
> > +++ b/arch/arm/mach-mvebu/board-v7.c
> > @@ -206,7 +206,6 @@ static const char * const armada_370_xp_dt_compat[] = {
> >  DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
> >  	.l2c_aux_val	= 0,
> >  	.l2c_aux_mask	= ~0,
> > -	.smp		= smp_ops(armada_xp_smp_ops),
> >  	.init_machine	= mvebu_dt_init,
> >  	.init_irq       = mvebu_init_irq,
> >  	.restart	= mvebu_restart,
> 
> There is a very good reason to keep this .smp initialization. The
> Device Tree for Armada XP used to *not* have the enable-method
> property, since the SMP enable-method binding did not exist at the
> time we introduced the Armada XP SMP support. Therefore, if we want to
> keep backward compatibility with the old Armada XP DTs and continue to
> have SMP support working with those, we need to keep this .smp
> initialization essentially forever.

Hi Thomas

Any idea what order things are done?

Would it be possible to remove the .smp entry, and then in
mvebu_dt_init() check if there are smp operations set. If not, then
set them to armada_xp_smp_ops?

    Andrew

[RFC PATCH] ARM: mvebu: Let the device-tree determine smp_ops

[Thomas Petazzoni]

Dear Andrew Lunn,

On Thu, 6 Nov 2014 16:21:35 +0100, Andrew Lunn wrote:

> Any idea what order things are done?
> 
> Would it be possible to remove the .smp entry, and then in
> mvebu_dt_init() check if there are smp operations set. If not, then
> set them to armada_xp_smp_ops?

->init_machine() is *way* too late for SMP. ->init_early() is also too
late as far as I remember. ->dt_fixup() would work, though I believe.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[RFC PATCH] ARM: mvebu: Let the device-tree determine smp_ops

[Chris Packham]

On 11/07/2014 03:49 AM, Andrew Lunn wrote:
> On Thu, Nov 06, 2014 at 05:49:56PM +1300, Chris Packham wrote:
>> The machine specific SMP operations can be configured either via
>> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
>> both of these are called and setup_arch wins because it is called last.
>> This means that it is not possible to substitute a different set of SMP
>> operations via the device-tree.
>>
>> All of the ARMADA_370_XP_DT compatible devices are either single core or
>> declare an enable-method in the dts (via one of armada-xp-mv78230.dtsi,
>> armada-xp-mv78260.dtsi or armada-xp-mv78460.dtsi). Remove the smp
>> assignment from board-v7.c so that the SMP operations set via the
>> device-tree aren't discarded.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>> Hi,
>>
>> (This is the first patch I've sent to the arm-lkml so beware of my newbie-ness)
> Hi Chris
>
> Congratulations on a good looking patch. Nice changelog, has a
> signed-off-by, comments are bellow the ---. All good.
>
> It is however a good idea to Cc: the mvebu maintainers. I added a few
> people to Cc:.
Thanks for the warm welcome. I actually thought the same thing after I 
hit send, thanks for filling in the Cc list for me.
>> I'm working on a new board that uses a integrated CPU that according to the
>> vendor is "based on the ARMADA-XP", more on that to come in the future
>> hopefully.
> A packet processor chip with an embedded CPU? Next generation
> Prestera?  No need to answer that if you don't want to.
You guess correctly. Not sure how much I can say without falling foul of 
our NDA but they know I intend on trying to get this stuff upstream 
rather than being locked into their particular SDK.
>> For the most part things just work if I use a .dts based on the
>> mv78260.
>   
>> This is my attempt at fixing the problem by not setting mdesc.smp for the
>> ARMADA_370_XP_DT machines thus preventing setup_arch from overriding what has
>> been setup by arm_dt_init_cpu_maps.
> You really need Thomas or Gregory to Ack this patch, but to me it
> looks correct.  Interestingly, this machine descriptor is used by 370,
> which is a single processor. The smp operations are useless, so your
> patch is also partly a cleanup.
>
>       Andrew

[RFC PATCH] ARM: mvebu: Let the device-tree determine smp_ops

[Chris Packham]

Hi Thomas,

On 11/07/2014 03:58 AM, Thomas Petazzoni wrote:
> Dear Chris Packham,
>
> On Thu,  6 Nov 2014 17:49:56 +1300, Chris Packham wrote:
>> The machine specific SMP operations can be configured either via
>> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
>> both of these are called and setup_arch wins because it is called last.
>> This means that it is not possible to substitute a different set of SMP
>> operations via the device-tree.
>>
>> All of the ARMADA_370_XP_DT compatible devices are either single core or
>> declare an enable-method in the dts (via one of armada-xp-mv78230.dtsi,
>> armada-xp-mv78260.dtsi or armada-xp-mv78460.dtsi). Remove the smp
>> assignment from board-v7.c so that the SMP operations set via the
>> device-tree aren't discarded.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>> Hi,
>>
>> (This is the first patch I've sent to the arm-lkml so beware of my newbie-ness)
>>
>> I'm working on a new board that uses a integrated CPU that according to the
>> vendor is "based on the ARMADA-XP", more on that to come in the future
>> hopefully. For the most part things just work if I use a .dts based on the
>> mv78260.
>>
>> One difference I've encountered is in the way the secondary CPU is initialised,
>> which requires me to supply a slightly different set of machine specific SMP
>> operations so I can bring up the 2nd core. It looks like I should be able to
>> supply a different /cpus/enable-method in the .dts and once I define the
>> corresponding set of operations it should be picked up.
>>
>> Which leads me to what I think could be a bug (or just a lack of clear
>> specification). The smp ops are set by both arm_dt_init_cpu_maps (from the
>> enable-method) and setup_arch (from mdesc). The latter always wins because it
>> is called last and doesn't check to see if smp_ops has already been set.
>>
>> This is my attempt at fixing the problem by not setting mdesc.smp for the
>> ARMADA_370_XP_DT machines thus preventing setup_arch from overriding what has
>> been setup by arm_dt_init_cpu_maps.
>>
>> Thanks,
>> Chris
>>
>>   arch/arm/mach-mvebu/board-v7.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
>> index 6478626..894974b 100644
>> --- a/arch/arm/mach-mvebu/board-v7.c
>> +++ b/arch/arm/mach-mvebu/board-v7.c
>> @@ -206,7 +206,6 @@ static const char * const armada_370_xp_dt_compat[] = {
>>   DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
>>   	.l2c_aux_val	= 0,
>>   	.l2c_aux_mask	= ~0,
>> -	.smp		= smp_ops(armada_xp_smp_ops),
>>   	.init_machine	= mvebu_dt_init,
>>   	.init_irq       = mvebu_init_irq,
>>   	.restart	= mvebu_restart,
> There is a very good reason to keep this .smp initialization. The
> Device Tree for Armada XP used to *not* have the enable-method
> property, since the SMP enable-method binding did not exist at the
> time we introduced the Armada XP SMP support. Therefore, if we want to
> keep backward compatibility with the old Armada XP DTs and continue to
> have SMP support working with those, we need to keep this .smp
> initialization essentially forever.
>
> Yes, backward compatibility sometimes sucks :-/
Darn. I thought that might be the case but I wanted to keep the scope of 
my change small. How about making setup_arch check if smp_ops is already 
set? I didn't do that initially because I thought that might affect too 
many platforms. Alternatively if we gave ARMADA_370_XP_DT a smp_init 
function that returned non-zero if smp_ops is already set then I think 
it'd be backwards compatible and keep the scope of the change restricted 
to the 370-XP platforms.

[RFC PATCH] ARM: mvebu: Let the device-tree determine smp_ops

[Andrew Lunn]

> >> I'm working on a new board that uses a integrated CPU that according to the
> >> vendor is "based on the ARMADA-XP", more on that to come in the future
> >> hopefully.
> > A packet processor chip with an embedded CPU? Next generation
> > Prestera?  No need to answer that if you don't want to.

> You guess correctly. Not sure how much I can say without falling foul of 
> our NDA but they know I intend on trying to get this stuff upstream 
> rather than being locked into their particular SDK.

Hi Chris

Nice to hear you want to upstream it. There are some interesting
chips, even in consumer level devices like the TPE-1620WS, which i
would love to play with. But the Switching part of Marvell don't seem
interested in the community, unlike the processors part of Marvell
which is engaging with the community.

      Andrew

[RFC PATCH] ARM: mvebu: Let the device-tree determine smp_ops

[Andrew Lunn]

> Darn. I thought that might be the case but I wanted to keep the scope of 
> my change small. How about making setup_arch check if smp_ops is already 
> set?

I think that would be last resort solution. Try not to touch core code
unless you really have to.

> Alternatively if we gave ARMADA_370_XP_DT a smp_init function that
> returned non-zero if smp_ops is already set then I think it'd be
> backwards compatible and keep the scope of the change restricted to
> the 370-XP platforms.

This sounds more reasonable.

     Andrew

[RFC PATCHv2] ARM: mvebu: Let the device-tree determine smp_ops

[Chris Packham]

The machine specific SMP operations can be configured either via
setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
both of these are called and setup_arch wins because it is called last.
This means that it is not possible to substitute a different set of SMP
operations via the device-tree.

For the ARMADA_370_XP_DT compatible devices add a smp_init function that
detects if the device tree has an enable-method defined. If it does
return true to indicate that the smp_ops have already been set which
will prevent setup_arch from overriding them.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Hi,

On 11/07/2014 09:16 AM, Andrew Lunn wrote:
>> Darn. I thought that might be the case but I wanted to keep the scope of
>> my change small. How about making setup_arch check if smp_ops is already
>> set?
> 
> I think that would be last resort solution. Try not to touch core code
> unless you really have to.
> 
>> Alternatively if we gave ARMADA_370_XP_DT a smp_init function that
>> returned non-zero if smp_ops is already set then I think it'd be
>> backwards compatible and keep the scope of the change restricted to
>> the 370-XP platforms.
> 
> This sounds more reasonable.
> 
>       Andrew
> 

So here's my next attempt at a patch that is backwards compatible but allows
the device tree to determine smp_ops. The device-tree searching was borrowed
from arm_dt_init_cpu_maps which is probably an indication that I'm duplicating
something that perhaps I shouldn't.

I have reservations about the fact that my code is active upon the device-tree
having and enable-method rather than the combination of the device tree having
the method and the kernel having that method declared. 

I'm also not sure that this is the intended use of the smp_init callback (a
quick grep through Documentation/ doesn't yield any hits and asm/mach/arch.h
doesn't say anything either) but nobody seemed to object when I proposed this
usage.

For v3 I wonder if I should just add a getter for smp_ops and have
armada_smp_init just use that or perhaps have a flag that is set by
set_smp_ops_by_method when it knows that the enable-method is something we can
use. Or perhaps I'm over-thinking things :).

Thanks,
Chris

 arch/arm/mach-mvebu/board-v7.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index 6478626..7e79eb1 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -198,6 +198,31 @@ static void __init mvebu_dt_init(void)
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
+static bool __init armada_smp_init(void)
+{
+	struct device_node *cpu, *cpus;
+	int len;
+	bool found_method = false;
+
+	cpus = of_find_node_by_path("/cpus");
+	if (!cpus)
+		return false;
+
+	for_each_child_of_node(cpus, cpu) {
+		if (of_node_cmp(cpu->type, "cpu"))
+			continue;
+		if (of_find_property(cpu, "enable-method", &len))
+			found_method = true;
+	}
+
+	/* Fallback to an enable-method in the cpus node */
+	if (!found_method)
+		if (of_find_property(cpus, "enable-method", &len))
+			found_method = true;
+
+	return found_method;
+}
+
 static const char * const armada_370_xp_dt_compat[] = {
 	"marvell,armada-370-xp",
 	NULL,
@@ -206,6 +231,7 @@ static const char * const armada_370_xp_dt_compat[] = {
 DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
 	.l2c_aux_val	= 0,
 	.l2c_aux_mask	= ~0,
+	.smp_init	= armada_smp_init,
 	.smp		= smp_ops(armada_xp_smp_ops),
 	.init_machine	= mvebu_dt_init,
 	.init_irq       = mvebu_init_irq,
-- 
2.2.0.rc0

[RFC PATCHv3] ARM: mvebu: Let the device-tree determine smp_ops

[Chris Packham]

The machine specific SMP operations can be configured either via
setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
both of these are called and setup_arch wins because it is called last.
This means that it is not possible to substitute a different set of SMP
operations via the device-tree.

Add a new smp_ops_is_set API that tells us if something else has already
set smp_ops. Use this new API in a smp_init function for the
ARMADA_370_XP_DT compatible devices to prevent setup_arch from
overriding the operations configured earlier.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Hi,

I didn't see any comment's on v2[1]. I'm not sure if I should take this as
disapproval with that approach or just a sign that people are busy.

This is an alternative solution that addresses one issue I had with the v2
implementation namely we now know whether smp_ops have been set or not and can
use this information to avoid setting them again. The downside is I'm now
modifying core code.

Thanks,
Chris
--
[1] - http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/300480.html

 arch/arm/include/asm/smp.h     |  1 +
 arch/arm/kernel/smp.c          | 10 +++++++++-
 arch/arm/mach-mvebu/board-v7.c |  6 ++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
index 18f5a55..aa55889 100644
--- a/arch/arm/include/asm/smp.h
+++ b/arch/arm/include/asm/smp.h
@@ -122,5 +122,6 @@ struct of_cpu_method {
  * set platform specific SMP operations
  */
 extern void smp_set_ops(struct smp_operations *);
+extern bool smp_ops_is_set(void);
 
 #endif /* ifndef __ASM_ARM_SMP_H */
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 13396d3..f303655 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -77,13 +77,21 @@ enum ipi_msg_type {
 static DECLARE_COMPLETION(cpu_running);
 
 static struct smp_operations smp_ops;
+static bool smp_ops_set = false;
 
 void __init smp_set_ops(struct smp_operations *ops)
 {
-	if (ops)
+	if (ops) {
 		smp_ops = *ops;
+		smp_ops_set = true;
+	}
 };
 
+bool smp_ops_is_set(void)
+{
+	return smp_ops_set;
+}
+
 static unsigned long get_arch_pgd(pgd_t *pgd)
 {
 	phys_addr_t pgdir = virt_to_idmap(pgd);
diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index 6478626..86b4b5d 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -198,6 +198,11 @@ static void __init mvebu_dt_init(void)
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
+static bool __init armada_smp_init(void)
+{
+	return smp_ops_is_set();
+}
+
 static const char * const armada_370_xp_dt_compat[] = {
 	"marvell,armada-370-xp",
 	NULL,
@@ -206,6 +211,7 @@ static const char * const armada_370_xp_dt_compat[] = {
 DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
 	.l2c_aux_val	= 0,
 	.l2c_aux_mask	= ~0,
+	.smp_init	= armada_smp_init,
 	.smp		= smp_ops(armada_xp_smp_ops),
 	.init_machine	= mvebu_dt_init,
 	.init_irq       = mvebu_init_irq,
-- 
2.2.0.rc0

[RFC PATCHv2] ARM: mvebu: Let the device-tree determine smp_ops

[Thomas Petazzoni]

Dear Chris Packham,

On Fri,  7 Nov 2014 15:33:46 +1300, Chris Packham wrote:

> +static bool __init armada_smp_init(void)
> +{
> +	struct device_node *cpu, *cpus;
> +	int len;
> +	bool found_method = false;

I don't think this boolean variable is needed.

> +
> +	cpus = of_find_node_by_path("/cpus");
> +	if (!cpus)
> +		return false;
> +
> +	for_each_child_of_node(cpus, cpu) {
> +		if (of_node_cmp(cpu->type, "cpu"))
> +			continue;
> +		if (of_find_property(cpu, "enable-method", &len))
> +			found_method = true;

Replace with:
		if (of_find_property(cpu, "enable-method", &len)) {
			of_node_put(cpus);
			return true;
		}

> +	}
> +
> +	/* Fallback to an enable-method in the cpus node */
> +	if (!found_method)
> +		if (of_find_property(cpus, "enable-method", &len))
> +			found_method = true;

Replace with:

	if (of_find_property(cpus, "enable-method", &len)) {
		of_node_put(cpus);
		return true;
	}

	of_node_put(cpus);
	return false;

Also, please add a comment above the function to clarify what it is
doing and why we're doing it. The commit log should also probably
mention why we're keeping the .smp = smp_ops(...) field (i.e, backward
compatibility with old DT).

>  static const char * const armada_370_xp_dt_compat[] = {
>  	"marvell,armada-370-xp",
>  	NULL,
> @@ -206,6 +231,7 @@ static const char * const armada_370_xp_dt_compat[] = {
>  DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
>  	.l2c_aux_val	= 0,
>  	.l2c_aux_mask	= ~0,
> +	.smp_init	= armada_smp_init,
>  	.smp		= smp_ops(armada_xp_smp_ops),
>  	.init_machine	= mvebu_dt_init,
>  	.init_irq       = mvebu_init_irq,

Best regards,

Thomas Petazzoni
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[RFC PATCHv2] ARM: mvebu: Let the device-tree determine smp_ops

[Thomas Petazzoni]

Dear Chris Packham,

On Fri,  7 Nov 2014 15:33:46 +1300, Chris Packham wrote:
> The machine specific SMP operations can be configured either via
> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
> both of these are called and setup_arch wins because it is called last.
> This means that it is not possible to substitute a different set of SMP
> operations via the device-tree.
> 
> For the ARMADA_370_XP_DT compatible devices add a smp_init function that
> detects if the device tree has an enable-method defined. If it does
> return true to indicate that the smp_ops have already been set which
> will prevent setup_arch from overriding them.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

My colleague Maxime Ripard (in Cc) rightfully suggests exploring a
different option: what about getting rid completely of the .smp field
of the DT_MACHINE structure, and instead have some code run early
enough that looks if an enable-method is defined, and if not, defines
it to the default value. This way, we continue to be backward
compatible in terms of DT, but we always use the enable-method from the
DT, and not sometimes from DT, sometimes from the DT_MACHINE structure.

Unfortunately, it will have to be done on the flattened DT, because the
DT is unflattened right before the enable-method properties are looked
up:

        unflatten_device_tree();

        arm_dt_init_cpu_maps();

And manipulating the DT in its flattened format, while possible in
->dt_fixup(), is a pain, and probably doesn't allow adding new
properties anyway.

So, in the end, maybe this idea doesn't work, I haven't checked
completely.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[RFC PATCHv2] ARM: mvebu: Let the device-tree determine smp_ops

[Chris Packham]

Hi Thomas,

On 11/17/2014 09:56 PM, Thomas Petazzoni wrote:
> Dear Chris Packham,
>
> On Fri,  7 Nov 2014 15:33:46 +1300, Chris Packham wrote:
>> The machine specific SMP operations can be configured either via
>> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
>> both of these are called and setup_arch wins because it is called last.
>> This means that it is not possible to substitute a different set of SMP
>> operations via the device-tree.
>>
>> For the ARMADA_370_XP_DT compatible devices add a smp_init function that
>> detects if the device tree has an enable-method defined. If it does
>> return true to indicate that the smp_ops have already been set which
>> will prevent setup_arch from overriding them.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>
> My colleague Maxime Ripard (in Cc) rightfully suggests exploring a
> different option: what about getting rid completely of the .smp field
> of the DT_MACHINE structure, and instead have some code run early
> enough that looks if an enable-method is defined, and if not, defines
> it to the default value. This way, we continue to be backward
> compatible in terms of DT, but we always use the enable-method from the
> DT, and not sometimes from DT, sometimes from the DT_MACHINE structure.
>
> Unfortunately, it will have to be done on the flattened DT, because the
> DT is unflattened right before the enable-method properties are looked
> up:
>
>          unflatten_device_tree();
>
>          arm_dt_init_cpu_maps();

That probably wouldn't be too hard because set_smp_ops_by_method() tells 
us if it's actually set something. We'd just need to propagate that up a 
few levels. We could either propagate this result through to setup_arch 
or move the smp_set_ops() call from setup_arch to something that gets 
invoked based on the return from set_smp_ops_by_method() or 
arm_dt_init_cpu_maps().

I'm a little hesitant because this is core code and I don't have access 
to a large set of devices to test (plus the whole newbie thing). But I 
could give it a go and see how far I get.

> And manipulating the DT in its flattened format, while possible in
> ->dt_fixup(), is a pain, and probably doesn't allow adding new
> properties anyway.
>
> So, in the end, maybe this idea doesn't work, I haven't checked
> completely.
>
> Best regards,
>
> Thomas
>

[RFC PATCHv2] ARM: mvebu: Let the device-tree determine smp_ops

[Chris Packham]

Hi Thomas, Maxime

On Mon, 17 Nov 2014, Thomas Petazzoni wrote:

> Dear Chris Packham,
>
> On Fri,  7 Nov 2014 15:33:46 +1300, Chris Packham wrote:
>> The machine specific SMP operations can be configured either via
>> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
>> both of these are called and setup_arch wins because it is called last.
>> This means that it is not possible to substitute a different set of SMP
>> operations via the device-tree.
>>
>> For the ARMADA_370_XP_DT compatible devices add a smp_init function that
>> detects if the device tree has an enable-method defined. If it does
>> return true to indicate that the smp_ops have already been set which
>> will prevent setup_arch from overriding them.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>
> My colleague Maxime Ripard (in Cc) rightfully suggests exploring a
> different option: what about getting rid completely of the .smp field
> of the DT_MACHINE structure, and instead have some code run early
> enough that looks if an enable-method is defined, and if not, defines
> it to the default value. This way, we continue to be backward
> compatible in terms of DT, but we always use the enable-method from the
> DT, and not sometimes from DT, sometimes from the DT_MACHINE structure.
>
> Unfortunately, it will have to be done on the flattened DT, because the
> DT is unflattened right before the enable-method properties are looked
> up:
>
>        unflatten_device_tree();
>
>        arm_dt_init_cpu_maps();
>
> And manipulating the DT in its flattened format, while possible in
> ->dt_fixup(), is a pain, and probably doesn't allow adding new
> properties anyway.
>
> So, in the end, maybe this idea doesn't work, I haven't checked
> completely.
>

So yeah it's tricky to work with the flattened dt. Not impossible the 
powerpc arch code does quite a lot with it. Arm does less but still uses 
it in atags_to_fdt.c. Below is a rough attempt at an implementation that 
seems to work. Because of the flattened dt it's harder to iterate through 
the cpu nodes so I haven't implemented anything to look for an 
enable-method attached to them.

---- 8< ----
Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for
  enable-method

When the device tree doesn't define an enable-method insert a property
into the flattened device tree. arm_dt_init_cpu_maps() will then parse
this an set smp_ops appropriately. Now that we have this fallback it is
no longer necessary to set .smp in the DT_MACHINE definition.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
  arch/arm/mach-mvebu/Makefile   |  2 ++
  arch/arm/mach-mvebu/board-v7.c | 23 ++++++++++++++++++++++-
  2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 1636cdb..f818eaf 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -14,3 +14,5 @@ endif

  obj-$(CONFIG_MACH_DOVE)		 += dove.o
  obj-$(CONFIG_MACH_KIRKWOOD)	 += kirkwood.o kirkwood-pm.o
+
+CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
\ No newline at end of file
diff --git a/arch/arm/mach-mvebu/board-v7.c 
b/arch/arm/mach-mvebu/board-v7.c
index b2524d6..45851a2 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -17,6 +17,8 @@
  #include <linux/clk-provider.h>
  #include <linux/of_address.h>
  #include <linux/of_platform.h>
+#include <linux/of_fdt.h>
+#include <linux/libfdt.h>
  #include <linux/io.h>
  #include <linux/clocksource.h>
  #include <linux/dma-mapping.h>
@@ -184,6 +186,25 @@ static void __init mvebu_dt_init(void)
  	of_platform_populate(NULL, of_default_bus_match_table, NULL, 
NULL);
  }

+static void __init armada_370_xp_dt_fixup(void)
+{
+	void *prop;
+	int offset;
+	int len;
+
+	offset = fdt_path_offset(initial_boot_params, "/cpus");
+	prop = fdt_getprop(initial_boot_params, offset, "enable-method", 
&len);
+
+	if (!prop) {
+		pr_info("No enable-method defined. "
+			"Falling back to \"marvell,armada-xp-smp\"\n");
+
+		fdt_appendprop(initial_boot_params, offset, 
"enable-method",
+			       "marvell,armada-xp-smp",
+			       sizeof("marvell,armada-xp-smp"));
+	}
+}
+
  static const char * const armada_370_xp_dt_compat[] = {
  	"marvell,armada-370-xp",
  	NULL,
@@ -192,11 +213,11 @@ static const char * const armada_370_xp_dt_compat[] 
= {
  DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
  	.l2c_aux_val	= 0,
  	.l2c_aux_mask	= ~0,
-	.smp		= smp_ops(armada_xp_smp_ops),
  	.init_machine	= mvebu_dt_init,
  	.init_irq       = mvebu_init_irq,
  	.restart	= mvebu_restart,
  	.dt_compat	= armada_370_xp_dt_compat,
+	.dt_fixup	= armada_370_xp_dt_fixup,
  MACHINE_END

  static const char * const armada_375_dt_compat[] = {
-- 
2.2.0.rc0

[RFC PATCHv2] ARM: mvebu: Let the device-tree determine smp_ops

[Chris Packham]

On 11/18/2014 12:34 PM, Chris Packham wrote:
> Hi Thomas, Maxime
>
> On Mon, 17 Nov 2014, Thomas Petazzoni wrote:
>
>> Dear Chris Packham,
>>
>> On Fri,  7 Nov 2014 15:33:46 +1300, Chris Packham wrote:
>>> The machine specific SMP operations can be configured either via
>>> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
>>> both of these are called and setup_arch wins because it is called last.
>>> This means that it is not possible to substitute a different set of SMP
>>> operations via the device-tree.
>>>
>>> For the ARMADA_370_XP_DT compatible devices add a smp_init function that
>>> detects if the device tree has an enable-method defined. If it does
>>> return true to indicate that the smp_ops have already been set which
>>> will prevent setup_arch from overriding them.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>
>> My colleague Maxime Ripard (in Cc) rightfully suggests exploring a
>> different option: what about getting rid completely of the .smp field
>> of the DT_MACHINE structure, and instead have some code run early
>> enough that looks if an enable-method is defined, and if not, defines
>> it to the default value. This way, we continue to be backward
>> compatible in terms of DT, but we always use the enable-method from the
>> DT, and not sometimes from DT, sometimes from the DT_MACHINE structure.
>>
>> Unfortunately, it will have to be done on the flattened DT, because the
>> DT is unflattened right before the enable-method properties are looked
>> up:
>>
>>        unflatten_device_tree();
>>
>>        arm_dt_init_cpu_maps();
>>
>> And manipulating the DT in its flattened format, while possible in
>> ->dt_fixup(), is a pain, and probably doesn't allow adding new
>> properties anyway.
>>
>> So, in the end, maybe this idea doesn't work, I haven't checked
>> completely.
>>
>
> So yeah it's tricky to work with the flattened dt. Not impossible the
> powerpc arch code does quite a lot with it. Arm does less but still uses
> it in atags_to_fdt.c. Below is a rough attempt at an implementation that
> seems to work. Because of the flattened dt it's harder to iterate
> through the cpu nodes so I haven't implemented anything to look for an
> enable-method attached to them.

(I really need to figure out how to tell Thunderbird how to wrap some 
parts but not others)

>
> ---- 8< ----
> Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for
>   enable-method
>
> When the device tree doesn't define an enable-method insert a property
> into the flattened device tree. arm_dt_init_cpu_maps() will then parse
> this an set smp_ops appropriately. Now that we have this fallback it is
> no longer necessary to set .smp in the DT_MACHINE definition.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>   arch/arm/mach-mvebu/Makefile   |  2 ++
>   arch/arm/mach-mvebu/board-v7.c | 23 ++++++++++++++++++++++-
>   2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> index 1636cdb..f818eaf 100644
> --- a/arch/arm/mach-mvebu/Makefile
> +++ b/arch/arm/mach-mvebu/Makefile
> @@ -14,3 +14,5 @@ endif
>
>   obj-$(CONFIG_MACH_DOVE)         += dove.o
>   obj-$(CONFIG_MACH_KIRKWOOD)     += kirkwood.o kirkwood-pm.o
> +
> +CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
> \ No newline at end of file
> diff --git a/arch/arm/mach-mvebu/board-v7.c
> b/arch/arm/mach-mvebu/board-v7.c
> index b2524d6..45851a2 100644
> --- a/arch/arm/mach-mvebu/board-v7.c
> +++ b/arch/arm/mach-mvebu/board-v7.c
> @@ -17,6 +17,8 @@
>   #include <linux/clk-provider.h>
>   #include <linux/of_address.h>
>   #include <linux/of_platform.h>
> +#include <linux/of_fdt.h>
> +#include <linux/libfdt.h>
>   #include <linux/io.h>
>   #include <linux/clocksource.h>
>   #include <linux/dma-mapping.h>
> @@ -184,6 +186,25 @@ static void __init mvebu_dt_init(void)
>       of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>   }
>
> +static void __init armada_370_xp_dt_fixup(void)
> +{
> +    void *prop;
> +    int offset;
> +    int len;
> +
> +    offset = fdt_path_offset(initial_boot_params, "/cpus");
> +    prop = fdt_getprop(initial_boot_params, offset, "enable-method",
> &len);
> +
> +    if (!prop) {
> +        pr_info("No enable-method defined. "
> +            "Falling back to \"marvell,armada-xp-smp\"\n");
> +
> +        fdt_appendprop(initial_boot_params, offset, "enable-method",
> +                   "marvell,armada-xp-smp",
> +                   sizeof("marvell,armada-xp-smp"));

Instead of inserting something into the device tree I could just call 
set_smp_ops here. That might be safer than trying to insert something 
into the device tree.

> +    }
> +}
> +
>   static const char * const armada_370_xp_dt_compat[] = {
>       "marvell,armada-370-xp",
>       NULL,
> @@ -192,11 +213,11 @@ static const char * const
> armada_370_xp_dt_compat[] = {
>   DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
>       .l2c_aux_val    = 0,
>       .l2c_aux_mask    = ~0,
> -    .smp        = smp_ops(armada_xp_smp_ops),
>       .init_machine    = mvebu_dt_init,
>       .init_irq       = mvebu_init_irq,
>       .restart    = mvebu_restart,
>       .dt_compat    = armada_370_xp_dt_compat,
> +    .dt_fixup    = armada_370_xp_dt_fixup,
>   MACHINE_END
>
>   static const char * const armada_375_dt_compat[] = {

[RFC PATCHv2] ARM: mvebu: Let the device-tree determine smp_ops

[Maxime Ripard]

Hi,

On Tue, Nov 18, 2014 at 12:34:41PM +1300, Chris Packham wrote:
> Hi Thomas, Maxime
> 
> On Mon, 17 Nov 2014, Thomas Petazzoni wrote:
> 
> >Dear Chris Packham,
> >
> >On Fri,  7 Nov 2014 15:33:46 +1300, Chris Packham wrote:
> >>The machine specific SMP operations can be configured either via
> >>setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
> >>both of these are called and setup_arch wins because it is called last.
> >>This means that it is not possible to substitute a different set of SMP
> >>operations via the device-tree.
> >>
> >>For the ARMADA_370_XP_DT compatible devices add a smp_init function that
> >>detects if the device tree has an enable-method defined. If it does
> >>return true to indicate that the smp_ops have already been set which
> >>will prevent setup_arch from overriding them.
> >>
> >>Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >
> >My colleague Maxime Ripard (in Cc) rightfully suggests exploring a
> >different option: what about getting rid completely of the .smp field
> >of the DT_MACHINE structure, and instead have some code run early
> >enough that looks if an enable-method is defined, and if not, defines
> >it to the default value. This way, we continue to be backward
> >compatible in terms of DT, but we always use the enable-method from the
> >DT, and not sometimes from DT, sometimes from the DT_MACHINE structure.
> >
> >Unfortunately, it will have to be done on the flattened DT, because the
> >DT is unflattened right before the enable-method properties are looked
> >up:
> >
> >       unflatten_device_tree();
> >
> >       arm_dt_init_cpu_maps();
> >
> >And manipulating the DT in its flattened format, while possible in
> >->dt_fixup(), is a pain, and probably doesn't allow adding new
> >properties anyway.
> >
> >So, in the end, maybe this idea doesn't work, I haven't checked
> >completely.
> >
> 
> So yeah it's tricky to work with the flattened dt. Not impossible the
> powerpc arch code does quite a lot with it. Arm does less but still uses it
> in atags_to_fdt.c. Below is a rough attempt at an implementation that seems
> to work. Because of the flattened dt it's harder to iterate through the cpu
> nodes so I haven't implemented anything to look for an enable-method
> attached to them.
> 
> ---- 8< ----
> Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for
>  enable-method
> 
> When the device tree doesn't define an enable-method insert a property
> into the flattened device tree. arm_dt_init_cpu_maps() will then parse
> this an set smp_ops appropriately. Now that we have this fallback it is
> no longer necessary to set .smp in the DT_MACHINE definition.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  arch/arm/mach-mvebu/Makefile   |  2 ++
>  arch/arm/mach-mvebu/board-v7.c | 23 ++++++++++++++++++++++-
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> index 1636cdb..f818eaf 100644
> --- a/arch/arm/mach-mvebu/Makefile
> +++ b/arch/arm/mach-mvebu/Makefile
> @@ -14,3 +14,5 @@ endif
> 
>  obj-$(CONFIG_MACH_DOVE)		 += dove.o
>  obj-$(CONFIG_MACH_KIRKWOOD)	 += kirkwood.o kirkwood-pm.o
> +
> +CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
> \ No newline at end of file
> diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
> index b2524d6..45851a2 100644
> --- a/arch/arm/mach-mvebu/board-v7.c
> +++ b/arch/arm/mach-mvebu/board-v7.c
> @@ -17,6 +17,8 @@
>  #include <linux/clk-provider.h>
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
> +#include <linux/of_fdt.h>
> +#include <linux/libfdt.h>
>  #include <linux/io.h>
>  #include <linux/clocksource.h>
>  #include <linux/dma-mapping.h>
> @@ -184,6 +186,25 @@ static void __init mvebu_dt_init(void)
>  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>  }
> 
> +static void __init armada_370_xp_dt_fixup(void)
> +{
> +	void *prop;
> +	int offset;
> +	int len;
> +
> +	offset = fdt_path_offset(initial_boot_params, "/cpus");
> +	prop = fdt_getprop(initial_boot_params, offset, "enable-method", &len);
> +
> +	if (!prop) {
> +		pr_info("No enable-method defined. "
> +			"Falling back to \"marvell,armada-xp-smp\"\n");
> +
> +		fdt_appendprop(initial_boot_params, offset, "enable-method",
> +			       "marvell,armada-xp-smp",
> +			       sizeof("marvell,armada-xp-smp"));
> +	}
> +}
> +

It's what I had in mind yes :)

I don't think you need to use fdt_appendprop though, fdt_setprop would
be enough.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141118/ecb01da2/attachment.sig>

[RFC PATCHv2] ARM: mvebu: Let the device-tree determine smp_ops

[Maxime Ripard]

On Tue, Nov 18, 2014 at 12:31:52AM +0000, Chris Packham wrote:
> 
> 
> On 11/18/2014 12:34 PM, Chris Packham wrote:
> > Hi Thomas, Maxime
> >
> > On Mon, 17 Nov 2014, Thomas Petazzoni wrote:
> >
> >> Dear Chris Packham,
> >>
> >> On Fri,  7 Nov 2014 15:33:46 +1300, Chris Packham wrote:
> >>> The machine specific SMP operations can be configured either via
> >>> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
> >>> both of these are called and setup_arch wins because it is called last.
> >>> This means that it is not possible to substitute a different set of SMP
> >>> operations via the device-tree.
> >>>
> >>> For the ARMADA_370_XP_DT compatible devices add a smp_init function that
> >>> detects if the device tree has an enable-method defined. If it does
> >>> return true to indicate that the smp_ops have already been set which
> >>> will prevent setup_arch from overriding them.
> >>>
> >>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >>
> >> My colleague Maxime Ripard (in Cc) rightfully suggests exploring a
> >> different option: what about getting rid completely of the .smp field
> >> of the DT_MACHINE structure, and instead have some code run early
> >> enough that looks if an enable-method is defined, and if not, defines
> >> it to the default value. This way, we continue to be backward
> >> compatible in terms of DT, but we always use the enable-method from the
> >> DT, and not sometimes from DT, sometimes from the DT_MACHINE structure.
> >>
> >> Unfortunately, it will have to be done on the flattened DT, because the
> >> DT is unflattened right before the enable-method properties are looked
> >> up:
> >>
> >>        unflatten_device_tree();
> >>
> >>        arm_dt_init_cpu_maps();
> >>
> >> And manipulating the DT in its flattened format, while possible in
> >> ->dt_fixup(), is a pain, and probably doesn't allow adding new
> >> properties anyway.
> >>
> >> So, in the end, maybe this idea doesn't work, I haven't checked
> >> completely.
> >>
> >
> > So yeah it's tricky to work with the flattened dt. Not impossible the
> > powerpc arch code does quite a lot with it. Arm does less but still uses
> > it in atags_to_fdt.c. Below is a rough attempt at an implementation that
> > seems to work. Because of the flattened dt it's harder to iterate
> > through the cpu nodes so I haven't implemented anything to look for an
> > enable-method attached to them.
> 
> (I really need to figure out how to tell Thunderbird how to wrap some 
> parts but not others)
> 
> >
> > ---- 8< ----
> > Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for
> >   enable-method
> >
> > When the device tree doesn't define an enable-method insert a property
> > into the flattened device tree. arm_dt_init_cpu_maps() will then parse
> > this an set smp_ops appropriately. Now that we have this fallback it is
> > no longer necessary to set .smp in the DT_MACHINE definition.
> >
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > ---
> >   arch/arm/mach-mvebu/Makefile   |  2 ++
> >   arch/arm/mach-mvebu/board-v7.c | 23 ++++++++++++++++++++++-
> >   2 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> > index 1636cdb..f818eaf 100644
> > --- a/arch/arm/mach-mvebu/Makefile
> > +++ b/arch/arm/mach-mvebu/Makefile
> > @@ -14,3 +14,5 @@ endif
> >
> >   obj-$(CONFIG_MACH_DOVE)         += dove.o
> >   obj-$(CONFIG_MACH_KIRKWOOD)     += kirkwood.o kirkwood-pm.o
> > +
> > +CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
> > \ No newline at end of file
> > diff --git a/arch/arm/mach-mvebu/board-v7.c
> > b/arch/arm/mach-mvebu/board-v7.c
> > index b2524d6..45851a2 100644
> > --- a/arch/arm/mach-mvebu/board-v7.c
> > +++ b/arch/arm/mach-mvebu/board-v7.c
> > @@ -17,6 +17,8 @@
> >   #include <linux/clk-provider.h>
> >   #include <linux/of_address.h>
> >   #include <linux/of_platform.h>
> > +#include <linux/of_fdt.h>
> > +#include <linux/libfdt.h>
> >   #include <linux/io.h>
> >   #include <linux/clocksource.h>
> >   #include <linux/dma-mapping.h>
> > @@ -184,6 +186,25 @@ static void __init mvebu_dt_init(void)
> >       of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> >   }
> >
> > +static void __init armada_370_xp_dt_fixup(void)
> > +{
> > +    void *prop;
> > +    int offset;
> > +    int len;
> > +
> > +    offset = fdt_path_offset(initial_boot_params, "/cpus");
> > +    prop = fdt_getprop(initial_boot_params, offset, "enable-method",
> > &len);
> > +
> > +    if (!prop) {
> > +        pr_info("No enable-method defined. "
> > +            "Falling back to \"marvell,armada-xp-smp\"\n");
> > +
> > +        fdt_appendprop(initial_boot_params, offset, "enable-method",
> > +                   "marvell,armada-xp-smp",
> > +                   sizeof("marvell,armada-xp-smp"));
> 
> Instead of inserting something into the device tree I could just call 
> set_smp_ops here. That might be safer than trying to insert something 
> into the device tree.

I don't think this is necessary. Injecting something in the DT is
safe, u-boot does that at every boot :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141118/4161b5d3/attachment.sig>

[RFC PATCHv2] ARM: mvebu: Let the device-tree determine smp_ops

[Chris Packham]

Hi Maxime,

On 11/18/2014 09:21 PM, Maxime Ripard wrote:
> On Tue, Nov 18, 2014 at 12:31:52AM +0000, Chris Packham wrote:
>>
>>
>> On 11/18/2014 12:34 PM, Chris Packham wrote:
>>> Hi Thomas, Maxime
>>>
>>> On Mon, 17 Nov 2014, Thomas Petazzoni wrote:
>>>
>>>> Dear Chris Packham,
>>>>
>>>> On Fri,  7 Nov 2014 15:33:46 +1300, Chris Packham wrote:
>>>>> The machine specific SMP operations can be configured either via
>>>>> setup_arch or via arm_dt_init_cpu_maps. For the ARMADA_370_XP_DT devices
>>>>> both of these are called and setup_arch wins because it is called last.
>>>>> This means that it is not possible to substitute a different set of SMP
>>>>> operations via the device-tree.
>>>>>
>>>>> For the ARMADA_370_XP_DT compatible devices add a smp_init function that
>>>>> detects if the device tree has an enable-method defined. If it does
>>>>> return true to indicate that the smp_ops have already been set which
>>>>> will prevent setup_arch from overriding them.
>>>>>
>>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>
>>>> My colleague Maxime Ripard (in Cc) rightfully suggests exploring a
>>>> different option: what about getting rid completely of the .smp field
>>>> of the DT_MACHINE structure, and instead have some code run early
>>>> enough that looks if an enable-method is defined, and if not, defines
>>>> it to the default value. This way, we continue to be backward
>>>> compatible in terms of DT, but we always use the enable-method from the
>>>> DT, and not sometimes from DT, sometimes from the DT_MACHINE structure.
>>>>
>>>> Unfortunately, it will have to be done on the flattened DT, because the
>>>> DT is unflattened right before the enable-method properties are looked
>>>> up:
>>>>
>>>>         unflatten_device_tree();
>>>>
>>>>         arm_dt_init_cpu_maps();
>>>>
>>>> And manipulating the DT in its flattened format, while possible in
>>>> ->dt_fixup(), is a pain, and probably doesn't allow adding new
>>>> properties anyway.
>>>>
>>>> So, in the end, maybe this idea doesn't work, I haven't checked
>>>> completely.
>>>>
>>>
>>> So yeah it's tricky to work with the flattened dt. Not impossible the
>>> powerpc arch code does quite a lot with it. Arm does less but still uses
>>> it in atags_to_fdt.c. Below is a rough attempt at an implementation that
>>> seems to work. Because of the flattened dt it's harder to iterate
>>> through the cpu nodes so I haven't implemented anything to look for an
>>> enable-method attached to them.
>>
>> (I really need to figure out how to tell Thunderbird how to wrap some
>> parts but not others)
>>
>>>
>>> ---- 8< ----
>>> Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for
>>>    enable-method
>>>
>>> When the device tree doesn't define an enable-method insert a property
>>> into the flattened device tree. arm_dt_init_cpu_maps() will then parse
>>> this an set smp_ops appropriately. Now that we have this fallback it is
>>> no longer necessary to set .smp in the DT_MACHINE definition.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> ---
>>>    arch/arm/mach-mvebu/Makefile   |  2 ++
>>>    arch/arm/mach-mvebu/board-v7.c | 23 ++++++++++++++++++++++-
>>>    2 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
>>> index 1636cdb..f818eaf 100644
>>> --- a/arch/arm/mach-mvebu/Makefile
>>> +++ b/arch/arm/mach-mvebu/Makefile
>>> @@ -14,3 +14,5 @@ endif
>>>
>>>    obj-$(CONFIG_MACH_DOVE)         += dove.o
>>>    obj-$(CONFIG_MACH_KIRKWOOD)     += kirkwood.o kirkwood-pm.o
>>> +
>>> +CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
>>> \ No newline at end of file
>>> diff --git a/arch/arm/mach-mvebu/board-v7.c
>>> b/arch/arm/mach-mvebu/board-v7.c
>>> index b2524d6..45851a2 100644
>>> --- a/arch/arm/mach-mvebu/board-v7.c
>>> +++ b/arch/arm/mach-mvebu/board-v7.c
>>> @@ -17,6 +17,8 @@
>>>    #include <linux/clk-provider.h>
>>>    #include <linux/of_address.h>
>>>    #include <linux/of_platform.h>
>>> +#include <linux/of_fdt.h>
>>> +#include <linux/libfdt.h>
>>>    #include <linux/io.h>
>>>    #include <linux/clocksource.h>
>>>    #include <linux/dma-mapping.h>
>>> @@ -184,6 +186,25 @@ static void __init mvebu_dt_init(void)
>>>        of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>>>    }
>>>
>>> +static void __init armada_370_xp_dt_fixup(void)
>>> +{
>>> +    void *prop;
>>> +    int offset;
>>> +    int len;
>>> +
>>> +    offset = fdt_path_offset(initial_boot_params, "/cpus");
>>> +    prop = fdt_getprop(initial_boot_params, offset, "enable-method",
>>> &len);
>>> +
>>> +    if (!prop) {
>>> +        pr_info("No enable-method defined. "
>>> +            "Falling back to \"marvell,armada-xp-smp\"\n");
>>> +
>>> +        fdt_appendprop(initial_boot_params, offset, "enable-method",
>>> +                   "marvell,armada-xp-smp",
>>> +                   sizeof("marvell,armada-xp-smp"));
>>
>> Instead of inserting something into the device tree I could just call
>> set_smp_ops here. That might be safer than trying to insert something
>> into the device tree.
>
> I don't think this is necessary. Injecting something in the DT is
> safe, u-boot does that at every boot :)
>

Actually I thought a bit more about this option this morning. What I'm 
trying to do is provide a fallback that defines smp_ops when there isn't 
an enable-method in the device tree. I don't actually need do do 
anything to the incoming device tree, I don't even need to look at it. I 
can unconditionally call set_smp_ops() and if the device tree has an 
enable-method it will override whatever has been configured. If the 
device tree doesn't define an enable-method it will use the default that 
I've configured here. That's actually very little code and can all be 
contained in board-v7.c.

[RFC PATCHv4] ARM: mvebu: use dt_fixup to provide fallback for enable-method

[Chris Packham]

We need to maintain backwards compatibility with device trees that don't
define an enable method. At the same time we want the device tree to be
able to specify an enable-method and have it stick.

Previously by having smp assigned in the DT_MACHINE definition this
would be picked up by setup_arch() and override whatever
arm_dt_init_cpu_maps() had configured. Now we move the initial
assignment of default smp_ops to a dt_fixup and let
arm_dt_init_cpu_maps() override that if the device tree defines an
enable-method.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Hi,

For those loosing track

v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/300182.html
v2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/300480.html
v3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/302945.html

(snip)
>>> Instead of inserting something into the device tree I could just call
>>> set_smp_ops here. That might be safer than trying to insert something
>>> into the device tree.
>>
>> I don't think this is necessary. Injecting something in the DT is
>> safe, u-boot does that at every boot :)
>>
>
> Actually I thought a bit more about this option this morning. What I'm
> trying to do is provide a fallback that defines smp_ops when there isn't
> an enable-method in the device tree. I don't actually need do do
> anything to the incoming device tree, I don't even need to look at it. I
> can unconditionally call set_smp_ops() and if the device tree has an
> enable-method it will override whatever has been configured. If the
> device tree doesn't define an enable-method it will use the default that
> I've configured here. That's actually very little code and can all be
> contained in board-v7.c.

I'm pretty happy with this incarnation. It doesn't touch core code. It provides
a fallback for old device trees and it achieves my original goal of allowing
the device tree to configure the smp_ops via the enable-method property. 

 arch/arm/mach-mvebu/board-v7.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index b2524d6..a4ece42 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -184,6 +184,11 @@ static void __init mvebu_dt_init(void)
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
+static void __init armada_370_xp_dt_fixup(void)
+{
+	smp_set_ops(smp_ops(armada_xp_smp_ops));
+}
+
 static const char * const armada_370_xp_dt_compat[] = {
 	"marvell,armada-370-xp",
 	NULL,
@@ -192,11 +197,11 @@ static const char * const armada_370_xp_dt_compat[] = {
 DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
 	.l2c_aux_val	= 0,
 	.l2c_aux_mask	= ~0,
-	.smp		= smp_ops(armada_xp_smp_ops),
 	.init_machine	= mvebu_dt_init,
 	.init_irq       = mvebu_init_irq,
 	.restart	= mvebu_restart,
 	.dt_compat	= armada_370_xp_dt_compat,
+	.dt_fixup	= armada_370_xp_dt_fixup,
 MACHINE_END
 
 static const char * const armada_375_dt_compat[] = {
-- 
2.2.0.rc0