Re: [PATCH 2/2] ARM: mvebu: x530: Add option for ECC

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3892 bytes --]

On Tue, Jan 11, 2022 at 02:37:53PM +1300, Chris Packham wrote:
> On Mon, Jan 10, 2022 at 9:10 PM Chris Packham <judge.packham@gmail.com> wrote:
> >
> >
> >
> > On Mon, 10 Jan 2022, 8:37 PM Stefan Roese, <sr@denx.de> wrote:
> >>
> >> On 1/6/22 04:08, 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>
> >>
> >> Just checking: Most likely you don't have the possibility to detect the
> >> board version at runtime?
> >
> >
> > Not in a way that can be done without working RAM. For our internal fork we maintain 2 defconfigs one with ECC one without. We can manage via the BOM which boards get which bootloader.
> >
> >>
> >> If this is not possible:
> >>
> >> Reviewed-by: Stefan Roese <sr@denx.de>
> >>
> >> Thanks,
> >> Stefan
> >>
> >> > ---
> >> >
> >> >   arch/arm/mach-mvebu/Kconfig      |  1 +
> >> >   board/alliedtelesis/x530/Kconfig | 20 ++++++++++++++++++++
> >> >   board/alliedtelesis/x530/x530.c  |  8 +++++++-
> >> >   3 files changed, 28 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..5c1ae36aebaa
> >> > --- /dev/null
> >> > +++ b/board/alliedtelesis/x530/Kconfig
> >> > @@ -0,0 +1,20 @@
> >> > +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.
> >> > +
> >> > +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
> >> > +
> 
> Hmm this doesn't actually appear to be working as intended. When the
> SPL is built I end up with CONFIG_IS_ENABLED(X530_ECC) == 0 even when
> I set it to y. Do I need to define a SPL version of this Kconfig
> symbol?

If you're using CONFIG_IS_ENABLED, then yes.  It doesn't always make
sense to use CONFIG_IS_ENABLED or other macros rather than just #ifdef
checks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]