Message ID | 20220111014942.3476139-1-judge.packham@gmail.com |
---|---|
State | Deferred |
Delegated to: | Tom Rini |
Headers | show |
Series | [v2] ARM: mvebu: x530: Add option for ECC | expand |
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
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 + +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 */