[U-Boot] [PATCH v3 2/2] arm: add initial support for Amlogic Meson and ODROID-C2

[Marek Vasut <marex@denx.de>]

On 04/11/2016 11:02 PM, Beniamino Galvani wrote:
> On Mon, Apr 11, 2016 at 02:08:56AM +0200, Marek Vasut wrote:
>>
>>> +#define GXBB_GPIO_0_EN		GXBB_PERIPHS_ADDR(0x0c)
>>> +#define GXBB_GPIO_0_OUT		GXBB_PERIPHS_ADDR(0x0d)
>>> +#define GXBB_GPIO_0_IN		GXBB_PERIPHS_ADDR(0x0e)
>>
>> You can also define this as
>> GXBB_GPIO_EN(n)  (0xc + 3 * (n) + 0)
>> GXBB_GPIO_OUT(n) (0xc + 3 * (n) + 1)
>> GXBB_GPIO_IN(n)  (0xc + 3 * (n) + 2)
> 
> This would work well if GPIO_6 didn't exist :)

Hrm, good point. You can probably special-case the gpio 6 with ternary
operator then .

>>> +#define GXBB_GPIO_1_EN		GXBB_PERIPHS_ADDR(0x0f)
>>> +#define GXBB_GPIO_1_OUT		GXBB_PERIPHS_ADDR(0x10)
>>> +#define GXBB_GPIO_1_IN		GXBB_PERIPHS_ADDR(0x11)
>>> +#define GXBB_GPIO_2_EN		GXBB_PERIPHS_ADDR(0x12)
>>> +#define GXBB_GPIO_2_OUT		GXBB_PERIPHS_ADDR(0x13)
>>> +#define GXBB_GPIO_2_IN		GXBB_PERIPHS_ADDR(0x14)
>>> +#define GXBB_GPIO_3_EN		GXBB_PERIPHS_ADDR(0x15)
>>> +#define GXBB_GPIO_3_OUT		GXBB_PERIPHS_ADDR(0x16)
>>> +#define GXBB_GPIO_3_IN		GXBB_PERIPHS_ADDR(0x17)
>>> +#define GXBB_GPIO_4_EN		GXBB_PERIPHS_ADDR(0x18)
>>> +#define GXBB_GPIO_4_OUT		GXBB_PERIPHS_ADDR(0x19)
>>> +#define GXBB_GPIO_4_IN		GXBB_PERIPHS_ADDR(0x1a)
>>> +#define GXBB_GPIO_5_EN		GXBB_PERIPHS_ADDR(0x1b)
>>> +#define GXBB_GPIO_5_OUT		GXBB_PERIPHS_ADDR(0x1c)
>>> +#define GXBB_GPIO_5_IN		GXBB_PERIPHS_ADDR(0x1d)
>>> +#define GXBB_GPIO_6_EN		GXBB_PERIPHS_ADDR(0x08)
>>> +#define GXBB_GPIO_6_OUT		GXBB_PERIPHS_ADDR(0x09)
>>> +#define GXBB_GPIO_6_IN		GXBB_PERIPHS_ADDR(0x0a)
> 
>> It'd be nice to have base addresses somewhere at the beginning instead
>> of having them mixed with the bit macros, but that's a matter of taste.
> 
> I agree, I'll change this.
> 
>>> +	val = fdt_getprop(gd->fdt_blob, offset, "reg", &len);
>>> +	if (len < sizeof(*val) * 4)
>>> +		return -EINVAL;
>>> +
>>> +	/* Don't use fdt64_t to avoid unaligned access */
>>
>> This looks iffy, can you elaborate on this issue ?
> 
> I was getting a "Synchronous Abort handler, esr 0x96000021" which
> seemed to indicate a alignment fault, but thinking again about it I'm
> not sure anymore of the real cause. fdt64_t and fdt64_to_cpu() don't
> work here, I will try to investigate better why. Suggestions are
> welcome :)

Toolchain issues ? Stack alignment issue ?

>>> +	/* Reserve first 16 MiB of RAM */
>>
>> Why ?
> 
> I'll add a comment, first 16 MiB seems to be reserved for firmware.

Thanks.

>>> +void reset_cpu(ulong addr)
>>> +{
>>
>> How does the system reboot then ?
> 
> The system reboots through a call to secure monitor, which is not
> implemented in this submission.

Ha, either implement the call or add comment please.

>>> +struct mm_region *mem_map = gxbb_mem_map;
>>
>> This looks super-iffy, I wouldn't be surprised if this started being
>> optimized away at some point.
> 
> I don't understand, why that should happen?

If you link the file alone, nothing references the symbols, so they can
be optimized away. But this is luckily not the case here.

>>> +	/* Reset PHY on GPIOZ_14 */
>>> +	clrbits_le32(GXBB_GPIO_3_EN, BIT(14));
>>> +	clrbits_le32(GXBB_GPIO_3_OUT, BIT(14));
>>> +	udelay(100000);
>>
>> mdelay(100); , though that is quite some wait.
> 
> Will change and decrease the timeout.
> 
>>> diff --git a/drivers/serial/serial_meson.c b/drivers/serial/serial_meson.c
>>
>> This should go in a separate patch.
> 
> I originally submitted the driver as separate patch, then Tom
> suggested that the initial platform patch should contain everything
> needed to perform a boot, so UART, DDR, board file and DTS.

Meh, I politely disagree with Tom ;-)

> Thanks for the review,
> 
> Beniamino
> 


-- 
Best regards,
Marek Vasut