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

[Stefan Roese]

On 1/18/22 09:57, Chris Packham wrote:
> 
> 
> On Tue, 18 Jan 2022, 8:11 PM Stefan Roese, <sr at denx.de 
> <mailto:sr at 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 at gmail.com
>     <mailto:judge.packham at gmail.com>>
>      > Reviewed-by: Stefan Roese <sr at denx.de <mailto:sr at 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 at denx.de <mailto:sr at denx.de>
> 

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 at denx.de