From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marek.vasut@gmail.com>
Date: Sat, 19 Sep 2020 15:04:38 +0200
Subject: [PATCH 2/2] arm: rmobile: Add HopeRun HiHope RZ/G2M board support
In-Reply-To: <TYBPR01MB53090A6ACDDA20526550F877863C0@TYBPR01MB5309.jpnprd01.prod.outlook.com>
References: <20200918160307.14323-1-biju.das.jz@bp.renesas.com>
 <20200918160307.14323-2-biju.das.jz@bp.renesas.com>
 <8f366d33-cc75-5902-2754-c5cabd0138a2@gmail.com>
 <TYBPR01MB53090A6ACDDA20526550F877863C0@TYBPR01MB5309.jpnprd01.prod.outlook.com>
Message-ID: <0bc06c2e-546d-0342-5ee1-066d08b35175@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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.