[PATCH v5 14/16] rockchip: Avoid #ifdefs in RK3399 SPL

[Jonas Karlman]

Hi Simon,

On 2024-06-27 10:02, Simon Glass wrote:
> Hi Jonas,
> 
> On Wed, 26 Jun 2024 at 17:16, Jonas Karlman <jonas at kwiboo.se> wrote:
>>
>> Hi Simon,
>>
>> On 2024-06-26 17:59, Simon Glass wrote:
>>> The code here is confusing due to large blocks which are #ifdefed out.
>>> Add a function phase_sdram_init() which returns whether SDRAM init
>>> should happen in the current phase, using that as needed to control the
>>> code flow.
>>>
>>> This increases code size by about 500 bytes in SPL when the cache is on,
>>> since it must call the rather large rockchip_sdram_size() function.
>>>
>>> - Drop the non-dcache optimisation, since the cache should normally be on
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> ---
>>>
>>> Changes in v5:
>>> - Move setting of pmugrf into the probe() function
>>>
>>> Changes in v3:
>>> - Split out the refactoring into a separate patch
>>>
>>>  drivers/ram/rockchip/sdram_rk3399.c | 33 ++++++++++++++---------------
>>>  1 file changed, 16 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
>>> index 3c4e20f4e80..b259e8e3dd6 100644
>>> --- a/drivers/ram/rockchip/sdram_rk3399.c
>>> +++ b/drivers/ram/rockchip/sdram_rk3399.c
>>
>> [snip]
>>
>>> +/**
>>> + * phase_sdram_init() - Check if this is the phase where SDRAM init happens
>>> + *
>>> + * Returns: true to do SDRAM init in this phase, false to not
>>> + */
>>> +static bool phase_sdram_init(void)
>>> +{
>>> +     return spl_phase() == PHASE_TPL ||
>>> +         (!IS_ENABLED(CONFIG_TPL) && !spl_in_proper());
>>
>> This still need to check for !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL)
>> to be correct, else you must build with both CONFIG_TPL and
>> CONFIG_ROCKCHIP_EXTERNAL_TPL or the generated SPL will incorrectly try
>> to initialize DRAM.
>>
>> DRAM should only be initialized in TPL or ROCKCHIP_EXTERNAL_TPL,
>> or if none of them are defined in SPL. All other phases should
>> just read and report dram size.
> 
> Thanks for the info.
> 
> The reason I am finding this hard to understand is that
> ROCKCHIP_EXTERNAL_TPL does not appear in the code as it is. So I
> cannot figure out why it needs to be added. Is there a bug in the
> current code, or does my patch introduce a bug, or...?

The current code does not handle ROCKCHIP_EXTERNAL_TPL and TPL must be
built to later be replaced with the external TPL blob.

This adds a new helper that states "Check if this is the phase where
SDRAM init happens", and for this statement to be true it should also
consider ROCKCHIP_EXTERNAL_TPL or a bugfix patch is later needed for
this newly added helper.

Adding !IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) should make this
statement be valid for the foreseeable combinations.

	return spl_phase() == PHASE_TPL ||
	       (!IS_ENABLED(CONFIG_TPL) &&
		!IS_ENABLED(CONFIG_ROCKCHIP_EXTERNAL_TPL) &&
		!spl_in_proper());

With that it should be possible to have TPL=n and ROCKCHIP_EXTERNAL_TPL=y
and produce a working SPL.

Regards,
Jonas

> 
> Regards,
> Simon