Hi Wolfgang,
On 07/13/2014 02:58 AM, Wolfgang Denk wrote:
Dear Gabriel Huau,
In message 1405204264-10922-1-git-send-email-contact@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