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/ |