Re: [U-Boot] [PATCH v5] mx6: add support of multi-processor command
Hi Gabriel,
On 13/07/2014 00:31, Gabriel Huau wrote:
This allows u-boot to load different OS or Bare Metal application on the
different cores of the i.MX6DQ.
For example: we can run Android on cpu0 and a RT OS like QNX/FreeRTOS on cpu1.
Signed-off-by: Gabriel Huau cont...@huau-gabriel.fr
---
Changes for v2:
- Add a commit log message to explain the purpose of this patch
Changes for v3:
- Remove unnecessary check for unsigned values when they are negative
Changes for v4:
- Add CONFIG_MP to the common mx6 configuration
- Get the number of CPUs dynamically instead of using a macro
Changes for v5:
- Rebase on the last update of the tree (conflicts solved)
However, I get several warnings applying your patch:
arch/arm/include/asm/arch/sys_proto.h:17:0: warning: is_soc_rev
redefined [enabled by default]
arch/arm/include/asm/arch/sys_proto.h:15:0: note: this is the location
of the previous definition
and:
arch/arm/cpu/armv7/mx6/mp.c:101:2: warning: implicit declaration of
function 'get_nr_cpus' [-Wimplicit-function-declaration]
In file included from arch/arm/imx-common/cpu.c:15:0:
You muxt fix them.
- Add a dummy header to solve build issue regarding the common/board_f.c
I do not think this is the best solution. An empty file is a file that
is not needed.
arch/arm/cpu/armv7/mx6/Makefile | 1 +
arch/arm/cpu/armv7/mx6/mp.c | 134
++
arch/arm/cpu/armv7/mx6/soc.c | 6 ++
arch/arm/include/asm/arch-mx6/imx-regs.h | 13 +++
arch/arm/include/asm/arch-mx6/sys_proto.h | 1 +
arch/arm/include/asm/mp.h | 11 +++
include/configs/mx6_common.h | 2 +
7 files changed, 168 insertions(+)
create mode 100644 arch/arm/cpu/armv7/mx6/mp.c
create mode 100644 arch/arm/include/asm/mp.h
I have just investigate a bit. The file is included by common/board_f.c
but it is, frankly, quite not used. There are several prototype inside it:
void setup_mp(void);
void cpu_mp_lmb_reserve(struct lmb *lmb);
int is_core_disabled(int nr);
They are not used at all.
u32 determine_mp_bootpg(unsigned int *pagesize);
This is the only one that is used.
Then it makes more sense to drop mp.h from board_f.c and add a prototype
for determine_mp_bootpg(). This function is already protected by:
#if defined(CONFIG_MP) (defined(CONFIG_MPC86xx) || defined(CONFIG_E500))
diff --git a/arch/arm/cpu/armv7/mx6/Makefile b/arch/arm/cpu/armv7/mx6/Makefile
index 6dc9f8e..bf6effc 100644
--- a/arch/arm/cpu/armv7/mx6/Makefile
+++ b/arch/arm/cpu/armv7/mx6/Makefile
@@ -10,3 +10,4 @@
obj-y:= soc.o clock.o
obj-$(CONFIG_SPL_BUILD) += ddr.o
obj-$(CONFIG_SECURE_BOOT)+= hab.o
+obj-$(CONFIG_MP) += mp.o
diff --git a/arch/arm/cpu/armv7/mx6/mp.c b/arch/arm/cpu/armv7/mx6/mp.c
new file mode 100644
index 000..85003d3
--- /dev/null
+++ b/arch/arm/cpu/armv7/mx6/mp.c
@@ -0,0 +1,134 @@
+/*
+ * (C) Copyright 2014
+ * Gabriel Huau cont...@huau-gabriel.fr
+ *
+ * (C) Copyright 2009 Freescale Semiconductor, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include common.h
+#include asm/io.h
+#include asm/errno.h
+#include asm/arch/sys_proto.h
+#include asm/arch/imx-regs.h
+
+int cpu_reset(int nr)
+{
+ uint32_t reg;
+ struct src *src = (struct src *)SRC_BASE_ADDR;
+
+ reg = __raw_readl(src-scr);
+
+ switch (nr) {
+ case 1:
+ reg |= SRC_SCR_CORE_1_RESET_MASK;
+ break;
+
+ case 2:
+ reg |= SRC_SCR_CORE_2_RESET_MASK;
+ break;
+
+ case 3:
+ reg |= SRC_SCR_CORE_3_RESET_MASK;
+ break;
+ }
+
+ /* Software reset of the CPU N */
+ __raw_writel(reg, src-scr);
+
+ return 0;
+}
+
+int cpu_status(int nr)
+{
+ uint32_t reg;
+ struct src *src = (struct src *)SRC_BASE_ADDR;
+
+ reg = __raw_readl(src-scr);
+
+ switch (nr) {
+ case 1:
+ printf(core 1: %d\n, !!(reg SRC_SCR_CORE_1_ENABLE_MASK));
+ break;
+
+ case 2:
+ printf(core 2: %d\n, !!(reg SRC_SCR_CORE_2_ENABLE_MASK));
+ break;
+
+ case 3:
+ printf(core 3: %d\n, !!(reg SRC_SCR_CORE_3_ENABLE_MASK));
+ break;
+ }
+
+ return 0;
+}
+
+int cpu_release(int nr, int argc, char *const argv[])
+{
+ uint32_t reg;
+ struct src *src = (struct src *)SRC_BASE_ADDR;
+ uint32_t boot_addr;
+
+ boot_addr = simple_strtoul(argv[0], NULL, 16);
+ reg = __raw_readl(src-scr);
+
+ switch (nr) {
+ case 1:
+ __raw_writel(boot_addr, src-gpr3);
+ reg |= SRC_SCR_CORE_1_ENABLE_MASK;
+ break;
+
+ case 2:
+ __raw_writel(boot_addr, src-gpr5);
+ reg |= SRC_SCR_CORE_2_ENABLE_MASK;
+ break;
+
+ case 3:
+
Re: [U-Boot] [PATCH v5] mx6: add support of multi-processor command
On 07/15/2014 12:49 AM, Stefano Babic wrote: Hi Gabriel, On 13/07/2014 00:31, Gabriel Huau wrote: This allows u-boot to load different OS or Bare Metal application on the different cores of the i.MX6DQ. For example: we can run Android on cpu0 and a RT OS like QNX/FreeRTOS on cpu1. Signed-off-by: Gabriel Huau cont...@huau-gabriel.fr --- Changes for v2: - Add a commit log message to explain the purpose of this patch Changes for v3: - Remove unnecessary check for unsigned values when they are negative Changes for v4: - Add CONFIG_MP to the common mx6 configuration - Get the number of CPUs dynamically instead of using a macro Changes for v5: - Rebase on the last update of the tree (conflicts solved) However, I get several warnings applying your patch: arch/arm/include/asm/arch/sys_proto.h:17:0: warning: is_soc_rev redefined [enabled by default] arch/arm/include/asm/arch/sys_proto.h:15:0: note: this is the location of the previous definition and: arch/arm/cpu/armv7/mx6/mp.c:101:2: warning: implicit declaration of function 'get_nr_cpus' [-Wimplicit-function-declaration] In file included from arch/arm/imx-common/cpu.c:15:0: You muxt fix them. My bad during the merge conflict, I'll fix that for the next version of the patch. - Add a dummy header to solve build issue regarding the common/board_f.c I do not think this is the best solution. An empty file is a file that is not needed. arch/arm/cpu/armv7/mx6/Makefile | 1 + arch/arm/cpu/armv7/mx6/mp.c | 134 ++ arch/arm/cpu/armv7/mx6/soc.c | 6 ++ arch/arm/include/asm/arch-mx6/imx-regs.h | 13 +++ arch/arm/include/asm/arch-mx6/sys_proto.h | 1 + arch/arm/include/asm/mp.h | 11 +++ include/configs/mx6_common.h | 2 + 7 files changed, 168 insertions(+) create mode 100644 arch/arm/cpu/armv7/mx6/mp.c create mode 100644 arch/arm/include/asm/mp.h I have just investigate a bit. The file is included by common/board_f.c but it is, frankly, quite not used. There are several prototype inside it: void setup_mp(void); void cpu_mp_lmb_reserve(struct lmb *lmb); int is_core_disabled(int nr); They are not used at all. u32 determine_mp_bootpg(unsigned int *pagesize); This is the only one that is used. Then it makes more sense to drop mp.h from board_f.c and add a prototype for determine_mp_bootpg(). This function is already protected by: #if defined(CONFIG_MP) (defined(CONFIG_MPC86xx) || defined(CONFIG_E500)) I agree adding an empty is not necessary the best solution, but I'd rather not to add any cpu/board specific defines in the common/ folder. That's why I think we should keep CONFIG_PM as this is a generic define. If necessary, I can propose another patch to fix it. Best regards, Stefano Babic ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v5] mx6: add support of multi-processor command
Hi Gabriel, On 15/07/2014 16:13, gabriel huau wrote: I have just investigate a bit. The file is included by common/board_f.c but it is, frankly, quite not used. There are several prototype inside it: void setup_mp(void); void cpu_mp_lmb_reserve(struct lmb *lmb); int is_core_disabled(int nr); They are not used at all. u32 determine_mp_bootpg(unsigned int *pagesize); This is the only one that is used. Then it makes more sense to drop mp.h from board_f.c and add a prototype for determine_mp_bootpg(). This function is already protected by: #if defined(CONFIG_MP) (defined(CONFIG_MPC86xx) || defined(CONFIG_E500)) I agree adding an empty is not necessary the best solution, but I'd rather not to add any cpu/board specific defines in the common/ folder. That's why I think we should keep CONFIG_PM as this is a generic define. If necessary, I can propose another patch to fix it. I think we are saying the same thing. I agree letting CONFIG_MP, this is not the point. But prototype for determine_mp_bootg() can be moved in a powerpc include file and board_f does not need to include any mp.h Best regards, Stefano Babic -- = DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de = ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v5] mx6: add support of multi-processor command
Agreed, I misunderstood sorry. I'll do the modification for the next version of the patch. Thanks! Regards, Gabriel On 07/15/2014 12:35 PM, Stefano Babic wrote: Hi Gabriel, On 15/07/2014 16:13, gabriel huau wrote: I have just investigate a bit. The file is included by common/board_f.c but it is, frankly, quite not used. There are several prototype inside it: void setup_mp(void); void cpu_mp_lmb_reserve(struct lmb *lmb); int is_core_disabled(int nr); They are not used at all. u32 determine_mp_bootpg(unsigned int *pagesize); This is the only one that is used. Then it makes more sense to drop mp.h from board_f.c and add a prototype for determine_mp_bootpg(). This function is already protected by: #if defined(CONFIG_MP) (defined(CONFIG_MPC86xx) || defined(CONFIG_E500)) I agree adding an empty is not necessary the best solution, but I'd rather not to add any cpu/board specific defines in the common/ folder. That's why I think we should keep CONFIG_PM as this is a generic define. If necessary, I can propose another patch to fix it. I think we are saying the same thing. I agree letting CONFIG_MP, this is not the point. But prototype for determine_mp_bootg() can be moved in a powerpc include file and board_f does not need to include any mp.h Best regards, Stefano Babic ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v5] mx6: add support of multi-processor command
Dear Gabriel Huau,
In message 1405204264-10922-1-git-send-email-cont...@huau-gabriel.fr you
wrote:
This allows u-boot to load different OS or Bare Metal application on the
different cores of the i.MX6DQ.
For example: we can run Android on cpu0 and a RT OS like QNX/FreeRTOS on cpu1.
Has this patch really to be specific for the quad core version? Can
we not also support the dual core version in the same way?
...
+int cpu_reset(int nr)
+{
+ uint32_t reg;
+ struct src *src = (struct src *)SRC_BASE_ADDR;
+
+ reg = __raw_readl(src-scr);
+
+ switch (nr) {
+ case 1:
+ reg |= SRC_SCR_CORE_1_RESET_MASK;
+ break;
+
+ case 2:
+ reg |= SRC_SCR_CORE_2_RESET_MASK;
+ break;
+
+ case 3:
+ reg |= SRC_SCR_CORE_3_RESET_MASK;
+ break;
+ }
I feel this should not be hardwired for 4 cores, and I also think we
should avoid using such a switch statement here. All you need is an
index into an array.
+int cpu_status(int nr)
+{
+ uint32_t reg;
+ struct src *src = (struct src *)SRC_BASE_ADDR;
+
+ reg = __raw_readl(src-scr);
+
+ switch (nr) {
+ case 1:
+ printf(core 1: %d\n, !!(reg SRC_SCR_CORE_1_ENABLE_MASK));
+ break;
+
+ case 2:
+ printf(core 2: %d\n, !!(reg SRC_SCR_CORE_2_ENABLE_MASK));
+ break;
+
+ case 3:
+ printf(core 3: %d\n, !!(reg SRC_SCR_CORE_3_ENABLE_MASK));
+ break;
+ }
Ditto. Such code duplication does not scale. Please rework to avoid
the switch.
+ switch (nr) {
+ case 1:
+ __raw_writel(boot_addr, src-gpr3);
+ reg |= SRC_SCR_CORE_1_ENABLE_MASK;
+ break;
+
+ case 2:
+ __raw_writel(boot_addr, src-gpr5);
+ reg |= SRC_SCR_CORE_2_ENABLE_MASK;
+ break;
+
+ case 3:
+ __raw_writel(boot_addr, src-gpr7);
+ reg |= SRC_SCR_CORE_3_ENABLE_MASK;
+ break;
+ }
+
+ /* CPU N is ready to start */
+ __raw_writel(reg, src-scr);
Ditto here.
And can you please explain why you are using __raw_writel() here?
+ reg = __raw_readl(src-scr);
+
+ switch (nr) {
+ case 1:
+ reg = ~SRC_SCR_CORE_1_ENABLE_MASK;
+ break;
+
+ case 2:
+ reg = ~SRC_SCR_CORE_2_ENABLE_MASK;
+ break;
+
+ case 3:
+ reg = ~SRC_SCR_CORE_3_ENABLE_MASK;
+ break;
+ }
+
+ /* Disable the CPU N */
+ __raw_writel(reg, src-scr);
Again, please avoid the switch.
We have read-modify-write macros which you could use, unless you
really have to use the __raw_*() accessors. Why is this needed?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
A little knowledge is a dangerous thing.- Doug Gwyn
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v5] mx6: add support of multi-processor command
Hi Wolfgang,
On 07/13/2014 02:58 AM, Wolfgang Denk wrote:
Dear Gabriel Huau,
In message 1405204264-10922-1-git-send-email-cont...@huau-gabriel.fr you
wrote:
This allows u-boot to load different OS or Bare Metal application on the
different cores of the i.MX6DQ.
For example: we can run Android on cpu0 and a RT OS like QNX/FreeRTOS on cpu1.
Has this patch really to be specific for the quad core version? Can
we not also support the dual core version in the same way?
...
Nope, but it makes sense for only Dual and Quad version. I'll update the
commit message to be more specific.
+int cpu_reset(int nr)
+{
+ uint32_t reg;
+ struct src *src = (struct src *)SRC_BASE_ADDR;
+
+ reg = __raw_readl(src-scr);
+
+ switch (nr) {
+ case 1:
+ reg |= SRC_SCR_CORE_1_RESET_MASK;
+ break;
+
+ case 2:
+ reg |= SRC_SCR_CORE_2_RESET_MASK;
+ break;
+
+ case 3:
+ reg |= SRC_SCR_CORE_3_RESET_MASK;
+ break;
+ }
I feel this should not be hardwired for 4 cores, and I also think we
should avoid using such a switch statement here. All you need is an
index into an array.
Agreed.
+int cpu_status(int nr)
+{
+ uint32_t reg;
+ struct src *src = (struct src *)SRC_BASE_ADDR;
+
+ reg = __raw_readl(src-scr);
+
+ switch (nr) {
+ case 1:
+ printf(core 1: %d\n, !!(reg SRC_SCR_CORE_1_ENABLE_MASK));
+ break;
+
+ case 2:
+ printf(core 2: %d\n, !!(reg SRC_SCR_CORE_2_ENABLE_MASK));
+ break;
+
+ case 3:
+ printf(core 3: %d\n, !!(reg SRC_SCR_CORE_3_ENABLE_MASK));
+ break;
+ }
Ditto. Such code duplication does not scale. Please rework to avoid
the switch.
+ switch (nr) {
+ case 1:
+ __raw_writel(boot_addr, src-gpr3);
+ reg |= SRC_SCR_CORE_1_ENABLE_MASK;
+ break;
+
+ case 2:
+ __raw_writel(boot_addr, src-gpr5);
+ reg |= SRC_SCR_CORE_2_ENABLE_MASK;
+ break;
+
+ case 3:
+ __raw_writel(boot_addr, src-gpr7);
+ reg |= SRC_SCR_CORE_3_ENABLE_MASK;
+ break;
+ }
+
+ /* CPU N is ready to start */
+ __raw_writel(reg, src-scr);
Ditto here.
And can you please explain why you are using __raw_writel() here?
No particular reason, I'll update with the generic macro.
+ reg = __raw_readl(src-scr);
+
+ switch (nr) {
+ case 1:
+ reg = ~SRC_SCR_CORE_1_ENABLE_MASK;
+ break;
+
+ case 2:
+ reg = ~SRC_SCR_CORE_2_ENABLE_MASK;
+ break;
+
+ case 3:
+ reg = ~SRC_SCR_CORE_3_ENABLE_MASK;
+ break;
+ }
+
+ /* Disable the CPU N */
+ __raw_writel(reg, src-scr);
Again, please avoid the switch.
We have read-modify-write macros which you could use, unless you
really have to use the __raw_*() accessors. Why is this needed?
Best regards,
Wolfgang Denk
I'll send a version 6 with your correction.
Regards,
Gabriel
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
