[U-Boot] [PATCH v5] mx6: add support of multi-processor command

[Wolfgang Denk]

Dear Gabriel Huau,

In message <1405204264-10922-1-git-send-email-contact at 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: wd at denx.de
"A little knowledge is a dangerous thing."                - Doug Gwyn