Re: [PATCH v9 1/9] qcom: scm: scm_set_warm_boot_addr() to set the warmboot address

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 10/25/2014 01:40 AM, Lina Iyer wrote:
> Set the warmboot address using an SCM call, only if the new address is
> different than the old one.

Please could you elaborate why a new address can be changed ?

> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>   drivers/soc/qcom/scm-boot.c | 22 ++++++++++++++++++++++
>   include/soc/qcom/scm-boot.h |  1 +
>   2 files changed, 23 insertions(+)
>
> diff --git a/drivers/soc/qcom/scm-boot.c b/drivers/soc/qcom/scm-boot.c
> index 60ff7b4..5710967 100644
> --- a/drivers/soc/qcom/scm-boot.c
> +++ b/drivers/soc/qcom/scm-boot.c
> @@ -37,3 +37,25 @@ int scm_set_boot_addr(phys_addr_t addr, int flags)
>   			&cmd, sizeof(cmd), NULL, 0);
>   }
>   EXPORT_SYMBOL(scm_set_boot_addr);
> +
> +

extra line.

> +int scm_set_warm_boot_addr(void *entry, int cpu)
> +{
> +	static int flags[NR_CPUS] = {
> +		SCM_FLAG_WARMBOOT_CPU0,
> +		SCM_FLAG_WARMBOOT_CPU1,
> +		SCM_FLAG_WARMBOOT_CPU2,
> +		SCM_FLAG_WARMBOOT_CPU3,
> +	};

Please do not do that, you don't know what NR_CPUS value could be in the 
future with the single kernel image and that could lead to a bug very 
hard to find. The kernel stack is 4096.

Move this out of the function:

static int scm_flags[] = {
	SCM_FLAG_WARMBOOT_CPU0,
	SCM_FLAG_WARMBOOT_CPU1,
	SCM_FLAG_WARMBOOT_CPU2,
	SCM_FLAG_WARMBOOT_CPU3,
};

> +	static DEFINE_PER_CPU(void *, last_known_entry);

It sounds odd to add those static declaration here even if I understand 
that is to encapsulate them.

> +	int ret;
> +
> +	if (entry == per_cpu(last_known_entry, cpu))
> +		return 0;

My question is: why scm_set_warm_boot_addr could be called with 
different addresses ?

If this is really needed, please replace the per_cpu variable by:

struct scm_boot_addr {
	int flag;
	phys_addr_t entry;
};

static struct scm_boot_addr scm_flags[] = {
	{ SCM_FLAG_WARMBOOT_CPU0, },
	{ SCM_FLAG_WARMBOOT_CPU1, },
	{ SCM_FLAG_WARMBOOT_CPU2, },
	{ SCM_FLAG_WARMBOOT_CPU3, },
};

> +	ret = scm_set_boot_addr(virt_to_phys(entry), flags[cpu]);
> +	if (!ret)
> +		per_cpu(last_known_entry, cpu) = entry;
> +
> +	return ret;
> +}
> diff --git a/include/soc/qcom/scm-boot.h b/include/soc/qcom/scm-boot.h
> index 02b445c..100938b 100644
> --- a/include/soc/qcom/scm-boot.h
> +++ b/include/soc/qcom/scm-boot.h
> @@ -22,5 +22,6 @@
>   #define SCM_FLAG_WARMBOOT_CPU3		0x40

By the way, if you look for encapsulation, perhaps these macros could be 
moved into scm-boot.c, no ?

>   int scm_set_boot_addr(phys_addr_t addr, int flags);
> +int scm_set_warm_boot_addr(void *entry, int cpu);
>
>   #endif
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog