[U-Boot] [PATCH 09/13] board: ti: am574x-idk: Add ddr data support

[Lukasz Majewski <lukma@denx.de>]

Hi Lokesh,

> Hi Lukas,
> 
> On Monday 18 December 2017 04:46 PM, Lukasz Majewski wrote:
> > Hi Lokesh,
> >   
> >> AM574x-idk has the following DDR parts attached:
> >> EMIF1: MT41K256M16HA (1GB with ECC)
> >> EMIF2: MT41K256M16HA (1GB without ECC)
> >>
> >> Enabling 2GB DDR without interleaving between EMIFs. And
> >> enabling ECC on EMIF1.
> >>
> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> >> Signed-off-by: Krunal Bhargav <k-bhargav@ti.com>
> >> ---
> >>  board/ti/am57xx/board.c | 47
> >> ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 44
> >> insertions(+), 3 deletions(-)
> >>
> >> diff --git a/board/ti/am57xx/board.c b/board/ti/am57xx/board.c
> >> index 2d14ae54fe..1377c7b1fe 100644
> >> --- a/board/ti/am57xx/board.c
> >> +++ b/board/ti/am57xx/board.c
> >> @@ -89,10 +89,18 @@ static const struct dmm_lisa_map_regs
> >> am571x_idk_lisa_regs = { .is_ma_present  = 0x1
> >>  };
> >>  
> >> +static const struct dmm_lisa_map_regs am574x_idk_lisa_regs = {
> >> +	.dmm_lisa_map_2 = 0xc0600200,
> >> +	.dmm_lisa_map_3 = 0x80600100,
> >> +	.is_ma_present  = 0x1
> >> +};
> >> +
> >>  void emif_get_dmm_regs(const struct dmm_lisa_map_regs
> >> **dmm_lisa_regs) {
> >>  	if (board_is_am571x_idk())
> >>  		*dmm_lisa_regs = &am571x_idk_lisa_regs;
> >> +	else if (board_is_am574x_idk())
> >> +		*dmm_lisa_regs = &am574x_idk_lisa_regs;
> >>  	else
> >>  		*dmm_lisa_regs = &beagle_x15_lisa_regs;
> >>  }
> >> @@ -231,8 +239,8 @@ static const struct emif_regs
> >> am571x_emif1_ddr3_666mhz_emif_regs =
> >> { .ref_ctrl			=
> >> 0x0000514d, .ref_ctrl_final			=
> >> 0x0000144a, .sdram_tim1			= 0xd333887c,
> >> -	.sdram_tim2			= 0x40b37fe3,
> >> -	.sdram_tim3			= 0x409f8ada,
> >> +	.sdram_tim2			= 0x30b37fe3,
> >> +	.sdram_tim3			= 0x409f8ad8,
> >>  	.read_idle_ctrl			= 0x00050000,
> >>  	.zq_config			= 0x5007190b,
> >>  	.temp_alert_config		= 0x00000000,
> >> @@ -249,17 +257,50 @@ static const struct emif_regs
> >> am571x_emif1_ddr3_666mhz_emif_regs =
> >> { .emif_rd_wr_exec_thresh		= 0x00000305 };
> >>  
> >> +static const struct emif_regs
> >> am574x_emif1_ddr3_666mhz_emif_ecc_regs = {
> >> +	.sdram_config_init		= 0x61863332,
> >> +	.sdram_config			= 0x61863332,
> >> +	.sdram_config2			= 0x08000000,
> >> +	.ref_ctrl			= 0x0000514d,
> >> +	.ref_ctrl_final			= 0x0000144a,
> >> +	.sdram_tim1			= 0xd333887c,
> >> +	.sdram_tim2			= 0x30b37fe3,
> >> +	.sdram_tim3			= 0x409f8ad8,
> >> +	.read_idle_ctrl			= 0x00050000,
> >> +	.zq_config			= 0x5007190b,
> >> +	.temp_alert_config		= 0x00000000,
> >> +	.emif_ddr_phy_ctlr_1_init	= 0x0024400f,
> >> +	.emif_ddr_phy_ctlr_1		= 0x0e24400f,
> >> +	.emif_ddr_ext_phy_ctrl_1	= 0x10040100,
> >> +	.emif_ddr_ext_phy_ctrl_2	= 0x00910091,
> >> +	.emif_ddr_ext_phy_ctrl_3	= 0x00950095,
> >> +	.emif_ddr_ext_phy_ctrl_4	= 0x009b009b,
> >> +	.emif_ddr_ext_phy_ctrl_5	= 0x009e009e,
> >> +	.emif_rd_wr_lvl_rmp_win		= 0x00000000,
> >> +	.emif_rd_wr_lvl_rmp_ctl		= 0x80000000,
> >> +	.emif_rd_wr_lvl_ctl		= 0x00000000,
> >> +	.emif_rd_wr_exec_thresh		= 0x00000305,
> >> +	.emif_ecc_ctrl_reg		= 0xD0000001,
> >> +	.emif_ecc_address_range_1	= 0x3FFF0000,
> >> +	.emif_ecc_address_range_2	= 0x00000000
> >> +};  
> > 
> > I'm wondering if it would be possible to:
> > 
> > Embed this memory setup (even as binary blob) to SPL FIT -> Those
> > values are generated from TI supplied excel sheet (when memory
> > details are provided).
> > 
> > 
> > Pros:
> > ----
> > 
> > - Since the same EMIF controller is used, one could only adjust the
> >   binary blob, when new memory (faster, slower, from other
> > manufacturer) is used in the product.
> > 
> > - There would be no need to add such code to the board file.  
> 
> yeah, ddr is not the only thing that comes in this bucket, PMIC data
> as well can be also made in similar way. I mean all the board related
> information can be moved out.

I think that the EMIF controller configuration is a bit special.

As you pointed out - for the whole AM57xx family one EMIF controller
type is used.

> But then the binary size will still
> remain the same. 

The goal here would be to not make the binary smaller, but reducing the
maintanence effort.

Example use case - company X has a product. They are using single u-boot
(with SPL and dts). The only thing, which they need to change is the
data needed for setting up proper memory configuration (DDR2/DDR3,
speed - 1500, 1333, ECC enabled/disabled, module size, etc). This all is
done in EMIF.

With separate EMIF blob configuration they don't need to rebuild u-boot
- they only change memory configuration binary data.

This data has a separate room in the non-volatile memory (e.g. SPI-NOR
flash). 

> Also, we will need a new driver to parse these new
> binary formats.

EMIF configuration data is IIRC 128 B at most. It is even possible to
copy 1:1 the binary data to EMIF (as it is now done in the u-boot code).

Hence, the "driver" would boil down to "memcpy".

> 
> As of now, the ddr excel sheet outputs the data in the $patch format,
> so still sticking to this format.

As stated before, those Ctrl+C and Ctrl+V data are then copied (in for
loop) to the EMIF registers.

Why not wrap EMIF binary data to FIT, store on non-volatile memory and
on each reset read it and "just" copy this data to EMIF? As fallback we
can use the "from excel" structure.

> 
> Yeah, i agree that it would be nice if we can come up with the
> separate binary for all board related info(i guess DT can be re used
> here).

Not all board related info - just EMIF.

DT is not an option here - it would require re-flashing u-boot binary.

> 
> Thanks and regards,
> Lokesh
> 
> >   
> >> +
> >>  void emif_get_reg_dump(u32 emif_nr, const struct emif_regs **regs)
> >>  {
> >>  	switch (emif_nr) {
> >>  	case 1:
> >>  		if (board_is_am571x_idk())
> >>  			*regs =
> >> &am571x_emif1_ddr3_666mhz_emif_regs;
> >> +		else if (board_is_am574x_idk())
> >> +			*regs =
> >> &am574x_emif1_ddr3_666mhz_emif_ecc_regs; else
> >>  			*regs =
> >> &beagle_x15_emif1_ddr3_532mhz_emif_regs; break;
> >>  	case 2:
> >> -		*regs = &beagle_x15_emif2_ddr3_532mhz_emif_regs;
> >> +		if (board_is_am574x_idk())
> >> +			*regs =
> >> &am571x_emif1_ddr3_666mhz_emif_regs;
> >> +		else
> >> +			*regs =
> >> &beagle_x15_emif2_ddr3_532mhz_emif_regs; break;
> >>  	}
> >>  }  
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
> > wd at denx.de 



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171219/50f8e859/attachment.sig>