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
