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

[gabriel huau]

Hi Wolfgang,

On 07/13/2014 02:58 AM, Wolfgang Denk wrote:
> 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?
>
> ...

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