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

[Biju Das]

Hi Marek,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] arm: rmobile: Add HopeRun HiHope RZ/G2M board
> support
>
> 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.

OK. Will add weak function in lowlevel_init_gen3.S.


 >>> +#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)

OK Will fix this in V3.

> > 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.

Will send separate patch for this.

> [...]
>
> >>> +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.

OK. Will fix this in V3.

Cheers,
Biju


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647