Message-ID: <520BEEC3.6060805@codeaurora.org>
Date:	Wed, 14 Aug 2013 13:55:31 -0700
From:	Rohit Vaswani <rvaswani@...eaurora.org>
To:	Mark Rutland <mark.rutland@....com>
CC:	David Brown <davidb@...eaurora.org>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	Pawel Moll <Pawel.Moll@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Ian Campbell <ian.campbell@...rix.com>,
	Rob Landley <rob@...dley.net>,
	Russell King <linux@....linux.org.uk>,
	Daniel Walker <dwalker@...o99.com>,
	Bryan Huntsman <bryanh@...eaurora.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
	Nicolas Pitre <nico@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
	Sudeep KarkadaNagesha <Sudeep.KarkadaNagesha@....com>
Subject: Re: [RESEND PATCH 2/4] ARM: msm: Re-organize platsmp to make it extensible

On 8/12/2013 8:50 AM, Mark Rutland wrote:
> Hi,
>
> Apologies for the long delay for review on this.
>
> I really like the direction this is going, but I have some qualms with
> the implementation.

Thanks for your review, but a few direct recommendations would help in 
making the implementation right.
>
> On Fri, Aug 02, 2013 at 03:15:23AM +0100, Rohit Vaswani wrote:
>> This makes it easy to add SMP support for new targets
>> by adding cpus property and the release sequence.
>> We add the enable-method property for the cpus property to
>> specify which release sequence to use.
>> While at it, add the 8660 cpus bindings to make SMP work.
>>
>> Signed-off-by: Rohit Vaswani <rvaswani@...eaurora.org>
>> ---
>>   Documentation/devicetree/bindings/arm/cpus.txt     |  6 ++
>>   Documentation/devicetree/bindings/arm/msm/scss.txt | 15 ++++
>>   arch/arm/boot/dts/msm8660-surf.dts                 | 23 +++++-
>>   arch/arm/mach-msm/platsmp.c                        | 94 +++++++++++++++++-----
>>   4 files changed, 115 insertions(+), 23 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/arm/msm/scss.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
>> index f32494d..327aad2 100644
>> --- a/Documentation/devicetree/bindings/arm/cpus.txt
>> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
>> @@ -44,6 +44,12 @@ For the ARM architecture every CPU node must contain the following properties:
>>   		"marvell,mohawk"
>>   		"marvell,xsc3"
>>   		"marvell,xscale"
>> +		"qcom,scorpion"
>> +- enable-method: Specifies the method used to enable or take the secondary cores
>> +		 out of reset. This allows different reset sequence for
>> +		 different types of cpus.
>> +		 This should be one of:
>> +		 "qcom,scss"
> While I'd certainly like to see a move to using enable-method for
> 32-bit, I think this needs a bit more thought:
>
> What does "qcom,scss" mean, precisely? It seems to be that we poke some
> registers in a "qcom,scss" device. I think we need to document
> enable-methods *very* carefully (and we need to do that for 64-bit as
> well with psci), as it's very likely there'll be subtle
> incompatibilities between platforms, especially if firmware is involved.
> We should try to leaves as little room for error as possible.
Yes qcom,scss implies poking registers in the qcom,scss device. How 
could I make that more clear in the documentation ?

>
> I don't want to see this devolve into meaning "whatever the Linux driver
> for this happens to do at the current point in time", as that just leads
> to breakage as we have no clear distinction between actual requirements
> and implementation details.
>
> Given we already have platforms without an enable-method, we can't make
> it a required property either -- those platforms are already booting by
> figuring out an enable method from the platform's compatible string.
So, you recommend we continue to using the platform compatible string as 
well ?
We currently don't have a perfect method to use enable-method in generic 
code. More on this below...
>
> With PSCI, enable-method also implies a method for disabling a
> particular CPU, so it would be nice for the description to cover this.
>
>>   
>>   Example:
>>   
>> diff --git a/Documentation/devicetree/bindings/arm/msm/scss.txt b/Documentation/devicetree/bindings/arm/msm/scss.txt
>> new file mode 100644
>> index 0000000..21c3e26
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/msm/scss.txt
>> @@ -0,0 +1,15 @@
>> +* SCSS - Scorpion Sub-system
>> +
>> +Properties
>> +
>> +- compatible : Should contain "qcom,scss".
>> +
>> +- reg: Specifies the base address for the SCSS registers used for
>> +       booting up secondary cores.
>> +
>> +Example:
>> +
>> +	scss@...000 {
>> +		compatible = "qcom,scss";
>> +		reg = <0x00902000 0x2000>;
>> +	};
>> diff --git a/arch/arm/boot/dts/msm8660-surf.dts b/arch/arm/boot/dts/msm8660-surf.dts
>> index cdc010e..203e51a 100644
>> --- a/arch/arm/boot/dts/msm8660-surf.dts
>> +++ b/arch/arm/boot/dts/msm8660-surf.dts
>> @@ -7,6 +7,22 @@
>>   	compatible = "qcom,msm8660-surf", "qcom,msm8660";
>>   	interrupt-parent = <&intc>;
>>   
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		compatible = "qcom,scorpion";
>> +		device_type = "cpu";
>> +		enable-method = "qcom,scss";
>> +
>> +		cpu@0 {
>> +			reg = <0>;
>> +		};
>> +
>> +		cpu@1 {
>> +			reg = <1>;
>> +		};
>> +	};
>> +
> I *really* like moving the common properties out into the /cpus node --
> ePAPR suggests it, it's less error prone, and it takes up less space.
> However, I'm not sure our generic/arch code handles it properly, and I
> think we need to audit that before we can start writing dts that depend
> on it. I'd be happy to be wrong here if anyone can correct me. :)
>
> We probably need to factor out the way we read properties for cpu nodes,
> falling back to ones present in /cpus if not present. There's already a
> lot of pain getting the node for a logical (rather than physical) cpu
> id.
>
> Sudeep recently factored out finding the cpu node for a logical cpu id
> [1,2] in generic code with a per-arch callback, it shouldn't be too hard
> to have shims around that to read (optionally) common properties.
>
> [...]
>
>> -static void prepare_cold_cpu(unsigned int cpu)
>> +static int scorpion_release_secondary(void)
>>   {
>> -	int ret;
>> -	ret = scm_set_boot_addr(virt_to_phys(secondary_startup),
>> -				SCM_FLAG_COLDBOOT_CPU1);
>> -	if (ret == 0) {
>> -		void __iomem *sc1_base_ptr;
>> -		sc1_base_ptr = ioremap_nocache(0x00902000, SZ_4K*2);
>> -		if (sc1_base_ptr) {
>> -			writel(0, sc1_base_ptr + VDD_SC1_ARRAY_CLAMP_GFS_CTL);
>> -			writel(0, sc1_base_ptr + SCSS_CPU1CORE_RESET);
>> -			writel(3, sc1_base_ptr + SCSS_DBG_STATUS_CORE_PWRDUP);
>> -			iounmap(sc1_base_ptr);
>> -		}
>> -	} else
>> -		printk(KERN_DEBUG "Failed to set secondary core boot "
>> -				  "address\n");
>> +	void __iomem *sc1_base_ptr;
>> +	struct device_node *dn = NULL;
>> +
>> +	dn = of_find_compatible_node(dn, NULL, "qcom,scss");
>> +	if (!dn) {
>> +		pr_err("%s: Missing scss node in device tree\n", __func__);
>> +		return -ENXIO;
>> +	}
>> +
>> +	sc1_base_ptr = of_iomap(dn, 0);
>> +	if (sc1_base_ptr) {
>> +		writel_relaxed(0, sc1_base_ptr + VDD_SC1_ARRAY_CLAMP_GFS_CTL);
>> +		writel_relaxed(0, sc1_base_ptr + SCSS_CPU1CORE_RESET);
>> +		writel_relaxed(3, sc1_base_ptr + SCSS_DBG_STATUS_CORE_PWRDUP);
>> +		mb();
>> +		iounmap(sc1_base_ptr);
> Does this boot *all* secondary CPUs directly into the kernel? Is that
> safe (e.g. if the kernel supports fewer CPUs than are physically
> present)?
Im not sure I understand the concern with safety here. The CPU for which 
the release_secondary will be called will be taken out of reset with 
this sequence.

>
> Is this a one-time thing, or are we able to dynamically hotplug CPUs? If
> the latter, the map/unmap looks odd to me.

This is a one-time thing done for each CPU that's specified in the 
device tree (or based on the command line over-ride).
>> +	} else {
>> +		return -ENOMEM;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>> -static int msm_boot_secondary(unsigned int cpu, struct task_struct *idle)
>> +static DEFINE_PER_CPU(int, cold_boot_done);
>> +
>> +static void boot_cold_cpu(unsigned int cpu)
>>   {
>> -	static int cold_boot_done;
>> +	const char *enable_method;
>> +	struct device_node *dn = NULL;
>>   
>> -	/* Only need to bring cpu out of reset this way once */
>> -	if (cold_boot_done == false) {
>> -		prepare_cold_cpu(cpu);
>> -		cold_boot_done = true;
>> +	dn = of_find_node_by_name(dn, "cpus");
>> +	if (!dn) {
>> +		pr_err("%s: Missing node cpus in device tree\n", __func__);
>> +		return;
>>   	}
>>   
>> +	enable_method = of_get_property(dn, "enable-method", NULL);
> Please factor this out from the platform code.
>
> If we're going to use enable-method, it should be decoupled from
> platform code -- common code should parse it to find the appropriate
> handler. Also, we *must* support having an enable-method per-cpu as KVM
> does for PSCI (though I definitely would like support for shared
> properties as mentioned above).
Currently with most platforms, smp.c calls into the boot_secondary 
function which decides which cpu it is
and then calls the appropriate function. This is because smp ops allow 
only 1 callback function to be registered...
Hence, using the enable-method in platform specific code works.

If we create a generic property should it mandate having generic code 
handle that ?
I currently don't have a good way of incorporating enable-method in 
generic code as it would mean to establish a mechanism to
associate the enable-method string with cpu specific boot_secondary 
functions.
You have an approach in mind that I can try ?

>> +	if (!enable_method) {
>> +			pr_err("%s: cpus node is missing enable-method property\n",
>> +					__func__);
>> +	} else if (!strcmp(enable_method, "qcom,scss")) {
>> +		if (per_cpu(cold_boot_done, cpu) == false) {
>> +			scorpion_release_secondary();
>> +			per_cpu(cold_boot_done, cpu) = true;
>> +		}
>> +	} else {
>> +		pr_err("%s: Invalid enable-method property: %s\n",
>> +				__func__, enable_method);
>> +	}
>> +}
>> +
>> +static int msm_boot_secondary(unsigned int cpu, struct task_struct *idle)
>> +{
>> +	boot_cold_cpu(cpu);
>> +
>>   	/*
>>   	 * set synchronisation state between this boot processor
>>   	 * and the secondary one
>> @@ -118,8 +148,28 @@ static void __init msm_smp_init_cpus(void)
>>   		set_cpu_possible(i, true);
>>   }
>>   
>> +static const int cold_boot_flags[] __initconst = {
>> +	0,
>> +	SCM_FLAG_COLDBOOT_CPU1,
>> +};
> So we only ever support booting two CPUs?
The next patch which adds support for a quadcore chip, adds more flags.
>
> Is the mapping of MPIDR to register bits arbitrary? Or do we know what
> they would be for four CPUs, four clusters, and beyond?
Four cpus.

>
>> +
>>   static void __init msm_smp_prepare_cpus(unsigned int max_cpus)
>>   {
>> +	int cpu, map;
>> +	unsigned int flags = 0;
>> +
>> +	for_each_present_cpu(cpu) {
>> +		map = cpu_logical_map(cpu);
>> +		if (map > ARRAY_SIZE(cold_boot_flags)) {
>> +			set_cpu_present(cpu, false);
>> +			__WARN();
>> +			continue;
>> +		}
>> +		flags |= cold_boot_flags[map];
> It's a shame that this leaves a window where some CPUs seem bootable,
> but aren't (though I can't offer a better way of handling this given we
> have dts without enable-method properties).
Any CPU that seems bootable and is defined in DT, should be bootable.
The cold_boot_flags are just a mechanism to set appropriate values for 
the scm call and
they aren't used as a method to disallow CPUs from being bootable and 
hence the __WARN() if this is done incorrectly.

>
>> +	}
>> +
>> +	if (scm_set_boot_addr(virt_to_phys(secondary_startup), flags))
>> +		pr_warn("Failed to set CPU boot address\n");
>>   }
>>   
>>   struct smp_operations msm_smp_ops __initdata = {
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> hosted by The Linux Foundation
>>
> Thanks,
> Mark
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/184150.html
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/189619.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Thanks,
Rohit Vaswani

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists