[U-Boot] [PATCH 1/3] armv7/fsl-ls102xa: Workaround for DDR erratum A008514

[Yao Yuan]

Hi Sinan,

Thanks for your review.
Please see my comments.

Best Regards,
Yuan Yao

> -----Original Message-----
> From: Sinan Akman [mailto:sinan at writeme.com]
> Sent: Saturday, August 15, 2015 12:28 AM
> To: Yuan Yao-B46683; Sun York-R58495; Wang Huan-B18965
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH 1/3] armv7/fsl-ls102xa: Workaround for DDR
> erratum A008514
> 
> 
>    Hi Yuan
> 
> On 14/08/15 02:54 AM, Yuan Yao wrote:
> > Affects: DDR
> > Description: Memory controller performance is not optimal with default
> > internal target queue register values.
> > Impact: Memory controller performance is not optimal.
> > Workaround: Write a value of 63b2_0002h to address: 157_020Ch.
> >
> > Signed-off-by: Yuan Yao <yao.yuan at freescale.com>
> > ---
> >   arch/arm/include/asm/arch-ls102xa/immap_ls102xa.h |  2 +-
> >   board/freescale/ls1021aqds/ls1021aqds.c           | 10 ++++++++++
> >   board/freescale/ls1021atwr/ls1021atwr.c           | 10 ++++++++++
> >   3 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/include/asm/arch-ls102xa/immap_ls102xa.h
> > b/arch/arm/include/asm/arch-ls102xa/immap_ls102xa.h
> > index d34044a..21bd65b 100644
> > --- a/arch/arm/include/asm/arch-ls102xa/immap_ls102xa.h
> > +++ b/arch/arm/include/asm/arch-ls102xa/immap_ls102xa.h
> > @@ -221,7 +221,7 @@ struct ccsr_scfg {
> >   	u32 scfgrevcr;
> >   	u32 coresrencr;
> >   	u32 pex2pmrdsr;
> > -	u32 ddrc1cr;
> > +	u32 eddrtqcfg;
> 
>    Is this a simple name change on the register, that ddrc1cr is not needed/used
> anywhere else ?

[Yuan Yao] 
Yes, there is no ddrc1cr in ls102xa, the register on 0x157020c is named eddrtqcfg.

> >   	u32 ddrc2cr;
> >   	u32 ddrc3cr;
> >   	u32 ddrc4cr;
> > diff --git a/board/freescale/ls1021aqds/ls1021aqds.c
> > b/board/freescale/ls1021aqds/ls1021aqds.c
> > index d6ef6ba..52bffc8 100644
> > --- a/board/freescale/ls1021aqds/ls1021aqds.c
> > +++ b/board/freescale/ls1021aqds/ls1021aqds.c
> > @@ -299,6 +299,16 @@ int board_early_init_f(void)
> >   	out_be32(&scfg->endiancr, SCFG_ENDIANCR_LE);
> >
> >   	/*
> > +	 * Memory controller require a register write before being enabled.
> > +	 * Affects: DDR
> > +	 * Register: EDDRTQCFG
> > +	 * Description: Memory controller performance is not optimal with
> > +	 *		default internal target queue register values.
> > +	 * Workaround: Write a value of 63b2_0002h to address: 157_020Ch.
> > +	 */
> > +	out_be32(&scfg->eddrtqcfg, 0x63b20002);
> 
>    Can you perhaps bring the comment from your 2/3 patch that explains
> EDDRTQCFG Registers are so that the reader sees it before its first usage.
> 
>    Also, is it possible to explain here what that magic value 0x63b20002
> corresponds to and perhaps use a #define that explains its function ?

[Yuan Yao] 
Sorry for here, I also don't sure what's the means of the magic value about 0x63b20002.
Also I'm not suggest to use the #define. Because the 0x63b20002 is just a silicon erratum workaround.
I just get the information for the document for silicon erratum. There is no more information for software.
The register eddrtqcfg is a reserve register for software. It should not open for developer as silicon team said.

> > +
> > +	/*
> >   	 * Enable snoop requests and DVM message requests for
> >   	 * Slave insterface S4 (A7 core cluster)
> >   	 */
> > diff --git a/board/freescale/ls1021atwr/ls1021atwr.c
> > b/board/freescale/ls1021atwr/ls1021atwr.c
> > index b7458a9..c565e91 100644
> > --- a/board/freescale/ls1021atwr/ls1021atwr.c
> > +++ b/board/freescale/ls1021atwr/ls1021atwr.c
> > @@ -501,6 +501,16 @@ int board_early_init_f(void)
> >   	out_be32(&scfg->endiancr, SCFG_ENDIANCR_LE);
> >
> >   	/*
> > +	 * Memory controller require a register write before being enabled.
> > +	 * Affects: DDR
> > +	 * Register: EDDRTQCFG
> > +	 * Description: Memory controller performance is not optimal with
> > +	 *		default internal target queue register values.
> > +	 * Workaround: Write a value of 63b2_0002h to address: 157_020Ch.
> > +	 */
> > +	out_be32(&scfg->eddrtqcfg, 0x63b20002);
> 
>    Same here.

[Yuan Yao] 
The same here.

> > +
> > +	/*
> >   	 * Enable snoop requests and DVM message requests for
> >   	 * Slave insterface S4 (A7 core cluster)
> >   	 */
> >
> 
>    Regards
>    Sinan Akman