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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 14 2014 at 01:30 -0700, Daniel Lezcano wrote:
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 ?

Hotplug could have a different warmboot address.


Signed-off-by: Lina Iyer <lina.iyer@xxxxxxxxxx>
---
 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,
};
Sure.

+	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.

Sure.
+	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, },
};

Hmm.. OK.

+	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 ?

Dont think anybody outside the file needs it.

 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

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux