On Wed, Jul 06, 2022 at 02:01:35PM +0200, Marco Felsch wrote:
> Hi Uwe,
>
> thanks for the patch.
>
> On 22-07-06, Uwe Kleine-König wrote:
>
> ...
>
> > @@ -96,11 +92,6 @@ extern struct dram_timing_info imx8mm_evk_dram_timing;
> >
> > static void start_atf(void)
> > {
> > - size_t bl31_size;
> > - const u8 *bl31;
> > - enum bootsource src;
> > - int instance;
> > -
> > /*
> > * If we are in EL3 we are running for the first time and need to
> > * initialize the DRAM and run TF-A (BL31). The TF-A will then jump
> > @@ -112,35 +103,7 @@ static void start_atf(void)
> > power_init_board();
> > imx8mm_ddr_init(&imx8mm_evk_dram_timing);
>
> I would move the imx8mm_ddr_init() into your helper
> imx8mm_load_and_start_image_via_tfa() and pass the timing as argument.
I thought about that, but didn't find a nice function name for that.
imx8mm_ddr_init_load_and_start_image_via_tfa_prepare_coffee() is a bit
long ... :-) Also while these things are "normal" in this order in the
PBL board code, mixing too many things in a single function isn't only
advantageous. I like it to have memory initialisation and image loading
in separate steps---my subjective view.
> So your helper doesn't depend on the 'hopefully' intialized DRAM. Else I
> would add a comment above your helper, that the DRAM must be initialized
> before calling this function.
Several functions depend on initialized DRAM.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
