[U-Boot] [PATCH 1/9] ddr: altera: Configuring SDRAM extra cycles timing parameters

[Chin Liang See]

On Mon, 2016-09-19 at 20:54 +0200, Marek Vasut wrote:
> On 09/19/2016 12:11 PM, Chin Liang See wrote:
> > On Mon, 2016-09-19 at 16:22 +0200, Marek Vasut wrote:
> > > On 09/15/2016 09:26 AM, Chin Liang See wrote:
> > > > To enable configuration of sdr.ctrlcfg.extratime1 register
> > > > which
> > > > enable
> > > > extra clocks for read to write command timing. This is critical
> > > > to
> > > > ensure successful LPDDR2 interface
> > > > 
> > > > Signed-off-by: Chin Liang See <clsee at altera.com>
> > > > ---
> > > >  arch/arm/mach-socfpga/include/mach/sdram.h | 8 +++++++-
> > > >  arch/arm/mach-socfpga/qts-filter.sh        | 2 +-
> > > >  arch/arm/mach-socfpga/wrap_sdram_config.c  | 9 +++++++++
> > > >  drivers/ddr/altera/sdram.c                 | 3 +++
> > > >  4 files changed, 20 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/mach-socfpga/include/mach/sdram.h
> > > > b/arch/arm/mach-socfpga/include/mach/sdram.h
> > > > index f12bb84..b11228f 100644
> > > > --- a/arch/arm/mach-socfpga/include/mach/sdram.h
> > > > +++ b/arch/arm/mach-socfpga/include/mach/sdram.h
> > > > @@ -30,7 +30,8 @@ struct socfpga_sdr_ctrl {
> > > >  	u32	dram_timing4;	/* 0x10 */
> > > >  	u32	lowpwr_timing;
> > > >  	u32	dram_odt;
> > > > -	u32	__padding0[4];
> > > > +	u32	extratime1;
> > > > +	u32	__padding0[3];
> > > >  	u32	dram_addrw;	/* 0x2c */
> > > >  	u32	dram_if_width;	/* 0x30 */
> > > >  	u32	dram_dev_width;
> > > > @@ -88,6 +89,7 @@ struct socfpga_sdram_config {
> > > >  	u32	dram_timing4;
> > > >  	u32	lowpwr_timing;
> > > >  	u32	dram_odt;
> > > > +	u32	extratime1;
> > > >  	u32	dram_addrw;
> > > >  	u32	dram_if_width;
> > > >  	u32	dram_dev_width;
> > > 
> > > This seems to be changing the DRAM register layout, is this
> > > really
> > > correct and was this really tested on AV SoCDK ?
> > 
> > Previously its treated unused register as default value is good
> > enough.
> > But this not true anymore for LPDDR2 and we are exposing extratime1
> > register.
> 
> I mean the later one , which adds an entry and moves the other
> registers
> by 4 bytes.

Oh you referring to socfpga_sdram_config. That structure is used to
store the handoff value. The address of register is actually pointed by
structure socfpga_sdr_ctrl.

> 
> > While for testing, I tested both CV and AV SoCDK few times as I
> > also
> > worried even they are using DDR3 instead LPDDR2.
> 
> Yes, they do, it's in the documentation ;-)

Oh actually I meant that this patch should not impact existing boards
which are using DDR3.

Thanks
Chin Liang

> 
> > > 
> > > [...]
> > > 
> > > > diff --git a/arch/arm/mach-socfpga/wrap_sdram_config.c
> > > > b/arch/arm/mach-socfpga/wrap_sdram_config.c
> > > > index 31cc7de..d72f5e1 100644
> > > > --- a/arch/arm/mach-socfpga/wrap_sdram_config.c
> > > > +++ b/arch/arm/mach-socfpga/wrap_sdram_config.c
> > > > @@ -81,6 +81,15 @@ static const struct socfpga_sdram_config
> > > > sdram_config = {
> > > >  			SDR_CTRLGRP_DRAMODT_READ_LSB)		
> > > > 	|
> > > >  		(CONFIG_HPS_SDR_CTRLCFG_DRAMODT_WRITE <<
> > > >  			SDR_CTRLGRP_DRAMODT_WRITE_LSB),
> > > > +#ifdef
> > > > CONFIG_HPS_SDR_CTRLCFG_EXTRATIME1_CFG_EXTRA_CTL_CLK_RD_TO_WR
> > > 
> > > How come this is not always defined for all boards ?
> > 
> > This is to ensure it still works if users are using older SOCEDS
> > instead of SOCEDS 16.1. Besides that, this is only applicable for
> > LPDDR2. With that, patches #2 to #9 are not needed.
> > 
> > Thanks
> > Chin Liang
> > 
> > > 
> > > > +	.extratime1 =
> > > > +	(CONFIG_HPS_SDR_CTRLCFG_EXTRATIME1_CFG_EXTRA_CTL_CLK_R
> > > > D_TO
> > > > _WR <<
> > > > +			SDR_CTRLGRP_EXTRATIME1_RD_TO_WR_LSB)	
> > > > 	|
> > > > +	(CONFIG_HPS_SDR_CTRLCFG_EXTRATIME1_CFG_EXTRA_CTL_CLK_R
> > > > D_TO
> > > > _WR_BC <<
> > > > +			SDR_CTRLGRP_EXTRATIME1_RD_TO_WR_BC_LSB
> > > > )	
> > > > 	|
> > > > +(CONFIG_HPS_SDR_CTRLCFG_EXTRATIME1_CFG_EXTRA_CTL_CLK_RD_TO_WR_
> > > > DIFF
> > > > _CHIP <<
> > > > +			SDR_CTRLGRP_EXTRATIME1_RD_TO_WR_DIFF_L
> > > > SB),
> > > > +#endif
> > > >  	.dram_addrw =
> > > >  		(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_COLBITS <<
> > > >  			SDR_CTRLGRP_DRAMADDRW_COLBITS_LSB)	
> > > > 	
> > > > > 
> > > > diff --git a/drivers/ddr/altera/sdram.c
> > > > b/drivers/ddr/altera/sdram.c
> > > > index 7e4606d..e74c5b0 100644
> > > > --- a/drivers/ddr/altera/sdram.c
> > > > +++ b/drivers/ddr/altera/sdram.c
> > > > @@ -418,6 +418,9 @@ static void sdr_load_regs(const struct
> > > > socfpga_sdram_config *cfg)
> > > >  
> > > >  	debug("Configuring DRAMODT\n");
> > > >  	writel(cfg->dram_odt, &sdr_ctrl->dram_odt);
> > > > +
> > > > +	debug("Configuring EXTRATIME1\n");
> > > > +	writel(cfg->extratime1, &sdr_ctrl->extratime1);
> > > >  }
> > > >  
> > > >  /**
> > > > 
> > > 
> > > 
> 
>