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...?
Regards,
Simon
