diff mbox

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

Message ID 1415327626-16079-1-git-send-email-chris.packham@alliedtelesis.co.nz (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Packham Nov. 7, 2014, 2:33 a.m. UTC 
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(+)

Comments

Thomas Petazzoni Nov. 17, 2014, 8:45 a.m. UTC | #1 
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 Nov. 17, 2014, 8:56 a.m. UTC | #2 
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
Chris Packham Nov. 17, 2014, 8:46 p.m. UTC | #3 
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
>
diff mbox

Patch

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,