[v2] ARM: mvebu: x530: Add option for ECC

Some older x530 boards have layout issues that cause problems for DDR.
These are usually seen as training failures but can also cause problems
after training has completed. Add an option to enable ECC leaving the
default as N which will work with both old and new boards.

Signed-off-by: Chris Packham <judge.packham@gmail.com>
Reviewed-by: Stefan Roese <sr@denx.de>
---

Changes in v2:
- Define Kconfig symbol for SPL.

 arch/arm/mach-mvebu/Kconfig      |  1 +
 board/alliedtelesis/x530/Kconfig | 25 +++++++++++++++++++++++++
 board/alliedtelesis/x530/x530.c  |  8 +++++++-
 3 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100644 board/alliedtelesis/x530/Kconfig

Hi Chris,

On 1/11/22 02:49, Chris Packham wrote:
> Some older x530 boards have layout issues that cause problems for DDR.
> These are usually seen as training failures but can also cause problems
> after training has completed. Add an option to enable ECC leaving the
> default as N which will work with both old and new boards.
> 
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> Reviewed-by: Stefan Roese <sr@denx.de>
> ---
> 
> Changes in v2:
> - Define Kconfig symbol for SPL.
> 
>   arch/arm/mach-mvebu/Kconfig      |  1 +
>   board/alliedtelesis/x530/Kconfig | 25 +++++++++++++++++++++++++
>   board/alliedtelesis/x530/x530.c  |  8 +++++++-
>   3 files changed, 33 insertions(+), 1 deletion(-)
>   create mode 100644 board/alliedtelesis/x530/Kconfig
> 
> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> index d23cc0c760f1..7388ade98d52 100644
> --- a/arch/arm/mach-mvebu/Kconfig
> +++ b/arch/arm/mach-mvebu/Kconfig
> @@ -341,5 +341,6 @@ config SECURED_MODE_CSK_INDEX
>   
>   source "board/solidrun/clearfog/Kconfig"
>   source "board/kobol/helios4/Kconfig"
> +source "board/alliedtelesis/x530/Kconfig"
>   
>   endif
> diff --git a/board/alliedtelesis/x530/Kconfig b/board/alliedtelesis/x530/Kconfig
> new file mode 100644
> index 000000000000..9e676f17f39c
> --- /dev/null
> +++ b/board/alliedtelesis/x530/Kconfig
> @@ -0,0 +1,25 @@
> +menu "x530 configuration"
> +	depends on TARGET_X530
> +
> +config X530_ECC
> +	bool "Enable DDR3 ECC"
> +	help
> +	  Some of the older x530 board have layout issues which cause problems
> +	  for the DDR which usually exhibit as DDR training failures or
> +	  problems accessing DDR after training.
> +
> +	  The known affected boards are:
> +
> +	  * 844-001897-00 (x530-28GTXm, x530-28GPXm, GS980MX/28PSm)
> +	  * 844-001948-00 (GS980MX/28)
> +	  * 844-002008-00 (x530L-52GTX, x530L-52GPX)
> +	  * 844-001974-00 (x530-52GTXm, x530-52GPXm, GS980MX/52PSm)
> +
> +	  If you have a newer board you can set Y here, otherwise say N.
> +
> +config SPL_X530_ECC
> +	bool
> +	depends on X530_ECC
> +	default X530_ECC
> +

This seems a bit superfluous, as you don't really need to differentiate
between SPL and non-SPL, right?

Did you take a look at my comment about this in v1 of this patch? Here
again:

AFAIK, IS_ENABLED(CONFIG_X530_ECC) would be a solution here. There is
no need for a new SPL Kconfig option this way. Please give it a try.

Thanks,
Stefan

> +endmenu
> diff --git a/board/alliedtelesis/x530/x530.c b/board/alliedtelesis/x530/x530.c
> index 866b6e68cc16..de20684f4353 100644
> --- a/board/alliedtelesis/x530/x530.c
> +++ b/board/alliedtelesis/x530/x530.c
> @@ -45,6 +45,12 @@ int hws_board_topology_load(struct serdes_map **serdes_map_array, u8 *count)
>   	return 0;
>   }
>   
> +#if CONFIG_IS_ENABLED(X530_ECC)
> +	#define BUS_MASK BUS_MASK_32BIT_ECC
> +#else
> +	#define BUS_MASK BUS_MASK_32BIT
> +#endif
> +
>   /*
>    * Define the DDR layout / topology here in the board file. This will
>    * be used by the DDR3 init code in the SPL U-Boot version to configure
> @@ -66,7 +72,7 @@ static struct mv_ddr_topology_map board_topology_map = {
>   	    0, 0,			/* cas_l cas_wl */
>   	    MV_DDR_TEMP_LOW,		/* temperature */
>   	    MV_DDR_TIM_2T} },		/* timing */
> -	BUS_MASK_32BIT_ECC,		/* subphys mask */
> +	BUS_MASK,			/* subphys mask */
>   	MV_DDR_CFG_DEFAULT,		/* ddr configuration data source */
>   	NOT_COMBINED,			/* ddr twin-die combined */
>   	{ {0} },			/* raw spd data */
> 

Viele Grüße,
Stefan Roese

On Tue, 18 Jan 2022, 8:11 PM Stefan Roese, <sr@denx.de> wrote:

> Hi Chris,
>
> On 1/11/22 02:49, Chris Packham wrote:
> > Some older x530 boards have layout issues that cause problems for DDR.
> > These are usually seen as training failures but can also cause problems
> > after training has completed. Add an option to enable ECC leaving the
> > default as N which will work with both old and new boards.
> >
> > Signed-off-by: Chris Packham <judge.packham@gmail.com>
> > Reviewed-by: Stefan Roese <sr@denx.de>
> > ---
> >
> > Changes in v2:
> > - Define Kconfig symbol for SPL.
> >
> >   arch/arm/mach-mvebu/Kconfig      |  1 +
> >   board/alliedtelesis/x530/Kconfig | 25 +++++++++++++++++++++++++
> >   board/alliedtelesis/x530/x530.c  |  8 +++++++-
> >   3 files changed, 33 insertions(+), 1 deletion(-)
> >   create mode 100644 board/alliedtelesis/x530/Kconfig
> >
> > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> > index d23cc0c760f1..7388ade98d52 100644
> > --- a/arch/arm/mach-mvebu/Kconfig
> > +++ b/arch/arm/mach-mvebu/Kconfig
> > @@ -341,5 +341,6 @@ config SECURED_MODE_CSK_INDEX
> >
> >   source "board/solidrun/clearfog/Kconfig"
> >   source "board/kobol/helios4/Kconfig"
> > +source "board/alliedtelesis/x530/Kconfig"
> >
> >   endif
> > diff --git a/board/alliedtelesis/x530/Kconfig
> b/board/alliedtelesis/x530/Kconfig
> > new file mode 100644
> > index 000000000000..9e676f17f39c
> > --- /dev/null
> > +++ b/board/alliedtelesis/x530/Kconfig
> > @@ -0,0 +1,25 @@
> > +menu "x530 configuration"
> > +     depends on TARGET_X530
> > +
> > +config X530_ECC
> > +     bool "Enable DDR3 ECC"
> > +     help
> > +       Some of the older x530 board have layout issues which cause
> problems
> > +       for the DDR which usually exhibit as DDR training failures or
> > +       problems accessing DDR after training.
> > +
> > +       The known affected boards are:
> > +
> > +       * 844-001897-00 (x530-28GTXm, x530-28GPXm, GS980MX/28PSm)
> > +       * 844-001948-00 (GS980MX/28)
> > +       * 844-002008-00 (x530L-52GTX, x530L-52GPX)
> > +       * 844-001974-00 (x530-52GTXm, x530-52GPXm, GS980MX/52PSm)
> > +
> > +       If you have a newer board you can set Y here, otherwise say N.
> > +
> > +config SPL_X530_ECC
> > +     bool
> > +     depends on X530_ECC
> > +     default X530_ECC
> > +
>
> This seems a bit superfluous, as you don't really need to differentiate
> between SPL and non-SPL, right?
>
> Did you take a look at my comment about this in v1 of this patch? Here
> again:
>

I think your comment and v2 crossed in cyberspace.


> AFAIK, IS_ENABLED(CONFIG_X530_ECC) would be a solution here. There is
> no need for a new SPL Kconfig option this way. Please give it a try.
>

I saw a few IS_ENABLED(CONFIG_FOO) and figured the were typos, but your
comment suggests otherwise. I'll give it a go at some point (I'm away right
now so it may miss this merge window).

In the meantime Marek's latest patch seems to work on the boards I've tried
with ECC enabled. I suspect we will still want to have this available as it
has been used on the earlier boards running our u-boot fork.


> Thanks,
> Stefan
>
> > +endmenu
> > diff --git a/board/alliedtelesis/x530/x530.c
> b/board/alliedtelesis/x530/x530.c
> > index 866b6e68cc16..de20684f4353 100644
> > --- a/board/alliedtelesis/x530/x530.c
> > +++ b/board/alliedtelesis/x530/x530.c
> > @@ -45,6 +45,12 @@ int hws_board_topology_load(struct serdes_map
> **serdes_map_array, u8 *count)
> >       return 0;   }
> >
> > +#if CONFIG_IS_ENABLED(X530_ECC)
> > +     #define BUS_MASK BUS_MASK_32BIT_ECC
> > +#else
> > +     #define BUS_MASK BUS_MASK_32BIT
> > +#endif
> > +
> >   /*
> >    * Define the DDR layout / topology here in the board file. This will
> >    * be used by the DDR3 init code in the SPL U-Boot version to configure
> > @@ -66,7 +72,7 @@ static struct mv_ddr_topology_map board_topology_map =
> {
> >           0, 0,                       /* cas_l cas_wl */
> >           MV_DDR_TEMP_LOW,            /* temperature */
> >           MV_DDR_TIM_2T} },           /* timing */
> > -     BUS_MASK_32BIT_ECC,             /* subphys mask */
> > +     BUS_MASK,                       /* subphys mask */
> >       MV_DDR_CFG_DEFAULT,             /* ddr configuration data source */
> >       NOT_COMBINED,                   /* ddr twin-die combined */
> >       { {0} },                        /* raw spd data */
> >
>
> Viele Grüße,
> Stefan Roese
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
>

On 1/18/22 09:57, Chris Packham wrote:
> 
> 
> On Tue, 18 Jan 2022, 8:11 PM Stefan Roese, <sr@denx.de 
> <mailto:sr@denx.de>> wrote:
> 
>     Hi Chris,
> 
>     On 1/11/22 02:49, Chris Packham wrote:
>      > Some older x530 boards have layout issues that cause problems for
>     DDR.
>      > These are usually seen as training failures but can also cause
>     problems
>      > after training has completed. Add an option to enable ECC leaving the
>      > default as N which will work with both old and new boards.
>      >
>      > Signed-off-by: Chris Packham <judge.packham@gmail.com
>     <mailto:judge.packham@gmail.com>>
>      > Reviewed-by: Stefan Roese <sr@denx.de <mailto:sr@denx.de>>
>      > ---
>      >
>      > Changes in v2:
>      > - Define Kconfig symbol for SPL.
>      >
>      >   arch/arm/mach-mvebu/Kconfig      |  1 +
>      >   board/alliedtelesis/x530/Kconfig | 25 +++++++++++++++++++++++++
>      >   board/alliedtelesis/x530/x530.c  |  8 +++++++-
>      >   3 files changed, 33 insertions(+), 1 deletion(-)
>      >   create mode 100644 board/alliedtelesis/x530/Kconfig
>      >
>      > diff --git a/arch/arm/mach-mvebu/Kconfig
>     b/arch/arm/mach-mvebu/Kconfig
>      > index d23cc0c760f1..7388ade98d52 100644
>      > --- a/arch/arm/mach-mvebu/Kconfig
>      > +++ b/arch/arm/mach-mvebu/Kconfig
>      > @@ -341,5 +341,6 @@ config SECURED_MODE_CSK_INDEX
>      >
>      >   source "board/solidrun/clearfog/Kconfig"
>      >   source "board/kobol/helios4/Kconfig"
>      > +source "board/alliedtelesis/x530/Kconfig"
>      >
>      >   endif
>      > diff --git a/board/alliedtelesis/x530/Kconfig
>     b/board/alliedtelesis/x530/Kconfig
>      > new file mode 100644
>      > index 000000000000..9e676f17f39c
>      > --- /dev/null
>      > +++ b/board/alliedtelesis/x530/Kconfig
>      > @@ -0,0 +1,25 @@
>      > +menu "x530 configuration"
>      > +     depends on TARGET_X530
>      > +
>      > +config X530_ECC
>      > +     bool "Enable DDR3 ECC"
>      > +     help
>      > +       Some of the older x530 board have layout issues which
>     cause problems
>      > +       for the DDR which usually exhibit as DDR training failures or
>      > +       problems accessing DDR after training.
>      > +
>      > +       The known affected boards are:
>      > +
>      > +       * 844-001897-00 (x530-28GTXm, x530-28GPXm, GS980MX/28PSm)
>      > +       * 844-001948-00 (GS980MX/28)
>      > +       * 844-002008-00 (x530L-52GTX, x530L-52GPX)
>      > +       * 844-001974-00 (x530-52GTXm, x530-52GPXm, GS980MX/52PSm)
>      > +
>      > +       If you have a newer board you can set Y here, otherwise
>     say N.
>      > +
>      > +config SPL_X530_ECC
>      > +     bool
>      > +     depends on X530_ECC
>      > +     default X530_ECC
>      > +
> 
>     This seems a bit superfluous, as you don't really need to differentiate
>     between SPL and non-SPL, right?
> 
>     Did you take a look at my comment about this in v1 of this patch? Here
>     again:
> 
> 
> I think your comment and v2 crossed in cyberspace.

;)

>     AFAIK, IS_ENABLED(CONFIG_X530_ECC) would be a solution here. There is
>     no need for a new SPL Kconfig option this way. Please give it a try.
> 
> 
> I saw a few IS_ENABLED(CONFIG_FOO) and figured the were typos, but your 
> comment suggests otherwise. I'll give it a go at some point (I'm away 
> right now so it may miss this merge window).

No hurry here. It does not seem to be super urgent. I'll just defer this
patch for a few weeks.

Thanks,
Stefan

> In the meantime Marek's latest patch seems to work on the boards I've 
> tried with ECC enabled. I suspect we will still want to have this 
> available as it has been used on the earlier boards running our u-boot fork.
> 
> 
>     Thanks,
>     Stefan
> 
>      > +endmenu
>      > diff --git a/board/alliedtelesis/x530/x530.c
>     b/board/alliedtelesis/x530/x530.c
>      > index 866b6e68cc16..de20684f4353 100644
>      > --- a/board/alliedtelesis/x530/x530.c
>      > +++ b/board/alliedtelesis/x530/x530.c
>      > @@ -45,6 +45,12 @@ int hws_board_topology_load(struct serdes_map
>     **serdes_map_array, u8 *count)
>      >       return 0;   }
>      >
>      > +#if CONFIG_IS_ENABLED(X530_ECC)
>      > +     #define BUS_MASK BUS_MASK_32BIT_ECC
>      > +#else
>      > +     #define BUS_MASK BUS_MASK_32BIT
>      > +#endif
>      > +
>      >   /*
>      >    * Define the DDR layout / topology here in the board file.
>     This will
>      >    * be used by the DDR3 init code in the SPL U-Boot version to
>     configure
>      > @@ -66,7 +72,7 @@ static struct mv_ddr_topology_map
>     board_topology_map = {
>      >           0, 0,                       /* cas_l cas_wl */
>      >           MV_DDR_TEMP_LOW,            /* temperature */
>      >           MV_DDR_TIM_2T} },           /* timing */
>      > -     BUS_MASK_32BIT_ECC,             /* subphys mask */
>      > +     BUS_MASK,                       /* subphys mask */
>      >       MV_DDR_CFG_DEFAULT,             /* ddr configuration data
>     source */
>      >       NOT_COMBINED,                   /* ddr twin-die combined */
>      >       { {0} },                        /* raw spd data */
>      >
> 
>     Viele Grüße,
>     Stefan Roese
> 
>     -- 
>     DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>     HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>     Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email:
>     sr@denx.de <mailto:sr@denx.de>
> 

Viele Grüße,
Stefan Roese