[U-Boot] [PATCH] fsl-lsch3: soc: Enable AHB support flexspi controller

[Ashish Kumar]

Hello York,

> -----Original Message-----
> From: York Sun
> Sent: Tuesday, September 25, 2018 10:20 PM
> To: Ashish Kumar <ashish.kumar at nxp.com>; u-boot at lists.denx.de
> Cc: Rajat Srivastava <rajat.srivastava at nxp.com>; Yogesh Narayan Gaur
> <yogeshnarayan.gaur at nxp.com>
> Subject: Re: [PATCH] fsl-lsch3: soc: Enable AHB support flexspi controller
> 
> On 09/25/2018 01:44 AM, Ashish Kumar wrote:
> > Enable AHB support for flexspi controller interface meaning memory can
> > be accessed via md command using absolute addresses
> >
> > Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur at nxp.com>
> > Signed-off-by: Rajat Srivastava <rajat.srivastava at nxp.com>
> > Signed-off-by: Ashish Kumar <Ashish.Kumar at nxp.com>
> > ---
> >  arch/arm/cpu/armv8/fsl-layerscape/Kconfig | 10 ++++++
> >  arch/arm/cpu/armv8/fsl-layerscape/soc.c   | 42 +++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> >
> > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
> > b/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
> > index 5280d33ec8..8d66783b7c 100644
> > --- a/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
> > +++ b/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
> > @@ -327,6 +327,16 @@ config SYS_FSPI_AHB_INIT
> >  	  performed. Default LUT programmed in AHB mode is Fast Read
> command
> >  	  with 4-byte addressing enabled.
> >
> > +config FSPI_AHB_EN_4BYTE
> > +	bool "Enable 4-byte Fast Read cmd for AHB mode"
> 
> s/cmd/command
While take care in v2
> 
> > +	depends on NXP_FSPI
> > +	default n
> > +	help
> > +	  The default setting for FSPI AHB bus just supports 3-byte addressing.
> > +	  But some FSPI flash sizes are up to 64MBytes.
> > +	  This flag enables fast read cmd for AHB mode and modifies required
> > +	  LUT to support full FSPI flash.
> > +
> 
> It is FlexSPI, not FSPI, nor flexspi. Please use consistent term for FlexSPI in
> subject, commit message, document and comment. If you have to use FSPI in
> macro names, make sure it it has clear meaning.
In description and comment it will be replaced by FlexSPI, but in macro/config/variables names can I use FSPI and fspi ?

> 
> >  config SYS_CCI400_OFFSET
> >  	hex "Offset for CCI400 base"
> >  	depends on SYS_FSL_HAS_CCI400
> > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > index 3f15cb08ff..60beb0dc96 100644
> > --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > @@ -640,6 +640,45 @@ void fsl_lsch2_early_init_f(void)  }  #endif
> >
> > +#ifdef CONFIG_FSPI_AHB_EN_4BYTE
> > +int fspi_ahb_init(void)
> > +{
> > +	/* Enable 4bytes address support and fast read */
> > +	u32 *fspi_lut, lut_key, *fspi_key;
> > +
> > +	fspi_key = (void *)SYS_FSL_FSPI_ADDR + 0x18;
> > +	fspi_lut = (void *)SYS_FSL_FSPI_ADDR + 0x200;
> > +
> > +	lut_key = in_be32(fspi_key);
> > +
> > +	if (lut_key == 0x5af05af0) {
> > +		/* That means the register is BE */
> > +		out_be32(fspi_key, 0x5af05af0);
> > +		/* Unlock the lut table */
> > +		out_be32(fspi_key + 1, 0x00000002);
> > +		out_be32(fspi_lut, 0x0820040c);
> > +		out_be32(fspi_lut + 1, 0x24003008);
> > +		out_be32(fspi_lut + 2, 0x00000000);
> > +		/* Lock the lut table */
> > +		out_be32(fspi_key, 0x5af05af0);
> > +		out_be32(fspi_key + 1, 0x00000001);
> > +	} else {
> > +		/* That means the register is LE */
> > +		out_le32(fspi_key, 0x5af05af0);
> > +		/* Unlock the lut table */
> > +		out_le32(fspi_key + 1, 0x00000002);
> > +		out_le32(fspi_lut, 0x0820040c);
> > +		out_le32(fspi_lut + 1, 0x24003008);
> > +		out_le32(fspi_lut + 2, 0x00000000);
> > +		/* Lock the lut table */
> > +		out_le32(fspi_key, 0x5af05af0);
> > +		out_le32(fspi_key + 1, 0x00000001);
> 
> Is there anyway you can make the magic numbers more meaningful?
How about adding comments above each magic number, explaining meaning?

Regards
Ashish
> 
> York