[PATCH 2/2] arm: rmobile: Add HopeRun HiHope RZ/G2M board support

[Marek Vasut <marek.vasut@gmail.com>]

On 9/19/20 2:18 PM, Biju Das wrote:

Hi,

[...]

>>> +++ b/board/hoperun/hihope-rzg2/hihope-rzg2.c
>>> @@ -0,0 +1,80 @@
>> [...]
>>> +#define RST_BASE0xE6160000
>>> +#define RST_CA57RESCNT(RST_BASE + 0x40)
>>> +#define RST_CODE0xA5A5000F
>>> +
>>> +/* If the firmware passed a device tree use it for U-Boot DRAM setup.
>>> +*/ extern u64 rcar_atf_boot_args[];
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +void s_init(void)
>>> +{
>>> +}
>>
>> Is this empty function needed ?
> 
> Yes Compilation error otherwise. This function is called from lowlevel_init_gen3.S. I believe it is place holder for doing some early initialisation such as watchdog
> That could be the reason  all rcar gen3 boards have this empty function for eg:-[1], please correct me if you think otherwise.
> [1] board/renesas/salvator-x/Salvator-x.c

Can you try fixing that with WEAK(s_init) in the lowlevel_init_gen3.S ?
I think that would be much better, if anyone needs to override the
function, then they can, otherwise empty WEAK function would be used.

>>> +#ifdef CONFIG_BOARD_EARLY_INIT_F
>>> +/* Kconfig forces this on, so just return 0 */
>>
>> I think BOARD_EARLY_INIT_F should really be disabled in Kconfig rather than
>> implementing empty function here.
>>
> 
> Ok, will fix in v2.
> 
> For eg:- file arch/arm/Kconfig
>  Select BOARD_EARLY_INIT_FUNCTION if !(RZA1 || TARGET_HIHOPE_RZG2)

Maybe it would be better to use imply BOARD_EARLY_INIT_F , and then
disable it on boards which don't need it (RZA1 and RZG2)

> I also noticed other boards in board/renesas directory with empty function(for eg:- ebisu,condor etc).
> For completeness, do you want me to fix that as well in  separate patch and removing empty functions.
> Select BOARD_EARLY_INIT_FUNCTION if !(RZA1 || TARGET_HIHOPE_RZG2|| TARGET_EBISU || TARGET_CONDOR)

Look at the 'imply' keyword, that might be even better.

[...]

>>> +int fdtdec_board_setup(const void *fdt_blob) {
>>> +void *atf_fdt_blob = (void *)(rcar_atf_boot_args[1]);
>>> +
>>> +if (fdt_magic(atf_fdt_blob) == FDT_MAGIC)
>>> +fdt_overlay_apply_node((void *)fdt_blob, 0, atf_fdt_blob,
>> 0);
>>> +
>>> +return 0;
>>> +}
>>
>> Please use board/renesas/rcar-common/common.c , no need to duplicate
>> the code.
> 
> OK will fix in V3.

Thanks

>>> +int dram_init(void)
>>> +{
>>> +return fdtdec_setup_mem_size_base(); }
>>> +
>>> +int dram_init_banksize(void)
>>> +{
>>> +return fdtdec_setup_memory_banksize(); }
>>> +
>>> +void reset_cpu(ulong addr)
>>> +{
>>> +writel(RST_CODE, RST_CA57RESCNT);
>>> +}
>>
>> Isn't there CA53 core in the RZG2 too ?
> 
> Yes, big little CPU 2xCA57 + 4 xCA53. Do you want me to add reset code for in case of CA53 boot mode???

I think if you can start U-Boot on either core, then the reset function
should handle both, yes.