Hi Jonas,
On Thu, 27 Jun 2024 at 10:00, Jonas Karlman <jo...@kwiboo.se> wrote:
>
> Hi Simon,
>
> On 2024-06-27 10:02, Simon Glass wrote:
> > Hi Jonas,
> >
> > On Wed, 26 Jun 2024 at 17:16, Jonas Karlman <jo...@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 <s...@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.
Thanks very much, I understand it now.
Regards,
Simon
