PPC4xx ECC Configs, Defines and Source

PPC4xx ECC Configs, Defines and Source

[Grant Erickson]

Does anyone have any strong preferences on where configurations, definitions
and sources for a PPC4xx ECC monitoring and reporting driver should go?

Specifically, this concerns ECC handling code for the IBM DDR2 ECC
controller found in the 405EX[r], 440SP, 440SPe, 460EX and 460GT. However,
I'd like to do this in a way that other ECC contributors and maintainers can
build on.

Static Configs
--------------

I thought I might leverage the definitions Stefan Roese came up with for
u-boot for the base memory controller:

    CONFIG_PPC4xx_IBM_SDRAM:   Applicable to 405GP, 405CR, 405EP, AP1000,
                               and ML2
    CONFIG_PPC4xx_IBM_DDR:     Applicable to 440GP, 440GX, 440EP, and 440GR
    CONFIG_PPC4xx_DENALI_DDR2: Applicable to 440EPX and 440GRX
    CONFIG_PPC4xx_IBM_DDR2:    Applicable to 405EX[r], 440SP, 440SPe, 460EX
                               and 460GT.

Controlling whether ECC monitoring and reporting support should be compiled
in or a module:

    CONFIG_PPC_ECC or CONFIG_ECC

Dynamic Configs
---------------

It seems as though the DTS entries for SDRAM0: sdram { compatible = "..." }
would need to be expanded to include the above information as well were any
such ECC driver going to attempt to bind to the runtime DTS information.

Perhaps:

    compatible = "...", "ibm,sdram-4xx-sdram";
    compatible = "...", "ibm,sdram-4xx-ddr";
    compatible = "...", "ibm,sdram-4xx-ddr2";
    compatible = "...", "denali,sdram-4xx-ddr2";

Though, it appears that rainier.dts and sequioia.dts are already evolving in
this direction with:

    compatible = "ibm,sdram-440grx", "ibm,sdram-44x-ddr2denali";
    compatible = "ibm,sdram-440epx", "ibm,sdram-44x-ddr2denali";

In light of that, perhaps then:

    compatible = "...", "ibm,sdram-4xx";
    compatible = "...", "ibm,sdram-4xx-ddr";
    compatible = "...", "ibm,sdram-4xx-ddr2";
    compatible = "...", "ibm,sdram-4xx-ddr2denali";

Source
------

    arch/powerpc/sysdev/
        ppc4xx_ibm_sdram_ecc.c [future]
        ppc4xx_ibm_ddr_ecc.c [future]
        ppc4xx_ibm_ddr2_ecc.c
        ppc4xx_ibm_ddr2denali_ecc.c [future]

Headers and Defines
-------------------

    arch/powerpc/include/asm/ppc4xx_ibm_ddr2.h

    OR

    arch/powerpc/sysdev/ppc4xx_ibm_ddr2.h

Thoughts? Comments?

Regards,

Grant

Re: PPC4xx ECC Configs, Defines and Source

[Josh Boyer]

On Mon, Dec 08, 2008 at 11:28:15AM -0800, Grant Erickson wrote:
>Does anyone have any strong preferences on where configurations, definitions
>and sources for a PPC4xx ECC monitoring and reporting driver should go?
>
>Specifically, this concerns ECC handling code for the IBM DDR2 ECC
>controller found in the 405EX[r], 440SP, 440SPe, 460EX and 460GT. However,
>I'd like to do this in a way that other ECC contributors and maintainers can
>build on.
>
>Static Configs
>--------------
>
>I thought I might leverage the definitions Stefan Roese came up with for
>u-boot for the base memory controller:
>
>    CONFIG_PPC4xx_IBM_SDRAM:   Applicable to 405GP, 405CR, 405EP, AP1000,
>                               and ML2
>    CONFIG_PPC4xx_IBM_DDR:     Applicable to 440GP, 440GX, 440EP, and 440GR
>    CONFIG_PPC4xx_DENALI_DDR2: Applicable to 440EPX and 440GRX
>    CONFIG_PPC4xx_IBM_DDR2:    Applicable to 405EX[r], 440SP, 440SPe, 460EX
>                               and 460GT.

Config options for what?  Enabling certain function?  Not sure that's needed
if we can get away with it by just binding to the appropriate controller.

>Controlling whether ECC monitoring and reporting support should be compiled
>in or a module:
>
>    CONFIG_PPC_ECC or CONFIG_ECC

That's a bit too generic, unless you are trying to make a menu list under
that which would be used to allow people to enable things like:
CONFIG_4XX_ECC, CONFIG_FSL_ECC, etc.

>Dynamic Configs
>---------------
>Though, it appears that rainier.dts and sequioia.dts are already evolving in
>this direction with:
>
>    compatible = "ibm,sdram-440grx", "ibm,sdram-44x-ddr2denali";
>    compatible = "ibm,sdram-440epx", "ibm,sdram-44x-ddr2denali";
>
>In light of that, perhaps then:
>
>    compatible = "...", "ibm,sdram-4xx";
>    compatible = "...", "ibm,sdram-4xx-ddr";
>    compatible = "...", "ibm,sdram-4xx-ddr2";
>    compatible = "...", "ibm,sdram-4xx-ddr2denali";

These look better than the first list.  We'll probably have to redefine
some of the existing dts file mappings.

>Source
>------
>
>    arch/powerpc/sysdev/
>        ppc4xx_ibm_sdram_ecc.c [future]
>        ppc4xx_ibm_ddr_ecc.c [future]
>        ppc4xx_ibm_ddr2_ecc.c
>        ppc4xx_ibm_ddr2denali_ecc.c [future]

Why is there a need to have so many files?  I would think you could
have a single file with all the ECC monitoring implementations in it
called ppc4xx_ecc.c (or such).  Surely they would share some amount
of code?

>Headers and Defines
>-------------------
>
>    arch/powerpc/include/asm/ppc4xx_ibm_ddr2.h
>
>    OR
>
>    arch/powerpc/sysdev/ppc4xx_ibm_ddr2.h

That depends on the contents of the file.  If it's DCR defines, etc
it should be in the dcr-regs.h file.  Otherwise, I would think having
the header in sysdev should be sufficient unless there is some other
facility that would need whatever the contents there are.

josh

Re: PPC4xx ECC Configs, Defines and Source

[Grant Erickson]

On 12/8/08 12:21 PM, Josh Boyer wrote:
> On Mon, Dec 08, 2008 at 11:28:15AM -0800, Grant Erickson wrote:
>> Does anyone have any strong preferences on where configurations, definitions
>> and sources for a PPC4xx ECC monitoring and reporting driver should go?
>> 
>> Specifically, this concerns ECC handling code for the IBM DDR2 ECC
>> controller found in the 405EX[r], 440SP, 440SPe, 460EX and 460GT. However,
>> I'd like to do this in a way that other ECC contributors and maintainers can
>> build on.
>> 
>> Static Configs
>> --------------
>> 
>> I thought I might leverage the definitions Stefan Roese came up with for
>> u-boot for the base memory controller:
>> 
>>    CONFIG_PPC4xx_IBM_SDRAM:   Applicable to 405GP, 405CR, 405EP, AP1000,
>>                               and ML2
>>    CONFIG_PPC4xx_IBM_DDR:     Applicable to 440GP, 440GX, 440EP, and 440GR
>>    CONFIG_PPC4xx_DENALI_DDR2: Applicable to 440EPX and 440GRX
>>    CONFIG_PPC4xx_IBM_DDR2:    Applicable to 405EX[r], 440SP, 440SPe, 460EX
>>                               and 460GT.
> 
> Config options for what?  Enabling certain function?  Not sure that's needed
> if we can get away with it by just binding to the appropriate controller.

A good clarifying question. The configs would cover enabling compiled-in or
module support for the ECC monitoring and reporting code under discussion. I
suspect embedded platforms that do not have ECC would not want to compile
support for this.

>> Controlling whether ECC monitoring and reporting support should be compiled
>> in or a module:
>> 
>>    CONFIG_PPC_ECC or CONFIG_ECC
> 
> That's a bit too generic, unless you are trying to make a menu list under
> that which would be used to allow people to enable things like:
> CONFIG_4XX_ECC, CONFIG_FSL_ECC, etc.

I'll have to think about this some more. More elaboration below.

Today, we can choose CONFIG_405EX. That nails down CONFIG_PPC4xx_IBM_DDR2
leaving the only open question at that point as to whether CONFIG_ECC is
'y', 'n' or 'm'.

That said, I am sure I am thinking too "statically" about compile-time
configuration, however.

>> Source
>> ------
>> 
>>    arch/powerpc/sysdev/
>>        ppc4xx_ibm_sdram_ecc.c [future]
>>        ppc4xx_ibm_ddr_ecc.c [future]
>>        ppc4xx_ibm_ddr2_ecc.c
>>        ppc4xx_ibm_ddr2denali_ecc.c [future]
> 
> Why is there a need to have so many files?  I would think you could
> have a single file with all the ECC monitoring implementations in it
> called ppc4xx_ecc.c (or such).  Surely they would share some amount
> of code?

Based on my foggy memory of the 405GP (ibm_sdram_ecc) roughly ten years ago,
a cursory look at today's Denali controller (from u-boot code), and the
405EX[r], the ECC handling code for these is all quite different.

There will likely be some common, shared code and that could go in something
like ppc4xx_ecc.c. My focus is only on the 405EX[r] and the associated DDR2
controller, so further abstraction will probably occur once another
controller is integrated.

>> Headers and Defines
>> -------------------
>> 
>>    arch/powerpc/include/asm/ppc4xx_ibm_ddr2.h
>> 
>>    OR
>> 
>>    arch/powerpc/sysdev/ppc4xx_ibm_ddr2.h
> 
> That depends on the contents of the file.  If it's DCR defines, etc
> it should be in the dcr-regs.h file.  Otherwise, I would think having
> the header in sysdev should be sufficient unless there is some other
> facility that would need whatever the contents there are.

Thanks for the confirmation. That was my operating assumption. They would be
DCR sub-definitions, such as:

    /*
     * Macro for generating register field mnemonics
     */
    #define PPC_REG_BITS        32
    #define PPC_REG_VAL(bit, val) ((val) << ((PPC_REG_BITS - 1) - (bit)))
    
    /*
     * Memory controller registers
     */
    #define SDRAM_BESR     0x00    /* PLB bus error status (read/clear) */
    #define SDRAM_BESRT    0x01    /* PLB bus error status (test/set)   */
    #define SDRAM_BEARL    0x02    /* PLB bus error address low         */
    #define SDRAM_BEARH    0x03    /* PLB bus error address high        */
    #if !defined(CONFIG_405EX)
    #define SDRAM_MCSTAT   0x14    /* memory controller status          */
    #else
    #define SDRAM_MCSTAT   0x1F    /* memory controller status          */
    #endif
    #define SDRAM_MCOPT1   0x20    /* memory controller options 1       */
    #define SDRAM_MCOPT2   0x21    /* memory controller options 2       */
    #define SDRAM_ECCCR    0x98    /* ECC error status                  */
    #define SDRAM_ECCES    SDRAM_ECCCR
    
    /*
     * Memory Controller Bus Error Status
     */
    #define SDRAM_BESR_MASK        PPC_REG_VAL(7, 0xFF)
    #define SDRAM_BESR_M0ID_MASK   PPC_REG_VAL(3, 0xF)
    #define SDRAM_BESR_M0ID_ICU    PPC_REG_VAL(3, 0x0)
    #define SDRAM_BESR_M0ID_PCIE0  PPC_REG_VAL(3, 0x1)
    #define SDRAM_BESR_M0ID_PCIE1  PPC_REG_VAL(3, 0x2)
    #define SDRAM_BESR_M0ID_DMA    PPC_REG_VAL(3, 0x3)
    #define SDRAM_BESR_M0ID_DCU    PPC_REG_VAL(3, 0x4)
    #define SDRAM_BESR_M0ID_OPB    PPC_REG_VAL(3, 0x5)
    #define SDRAM_BESR_M0ID_MAL    PPC_REG_VAL(3, 0x6)
    #define SDRAM_BESR_M0ID_SEC    PPC_REG_VAL(3, 0x7)
    #define SDRAM_BESR_M0ET_MASK   PPC_REG_VAL(6, 0x7)
    #define SDRAM_BESR_M0ET_NONE   PPC_REG_VAL(6, 0x0)
    #define SDRAM_BESR_M0ET_ECC    PPC_REG_VAL(6, 0x1)
    #define SDRAM_BESR_M0RW_WRITE  PPC_REG_VAL(7, 0)
    #define SDRAM_BESR_M0RW_READ   PPC_REG_VAL(8, 1)
    
    /*
     * Memory Controller Options 1
     */
    #define SDRAM_MCOPT1_MCHK_MASK       0x30000000
    #define SDRAM_MCOPT1_MCHK_NON        0x00000000 /* No ECC gen        */
    #define SDRAM_MCOPT1_MCHK_GEN        0x20000000 /* ECC gen           */
    #define SDRAM_MCOPT1_MCHK_CHK        0x10000000 /* ECC gen & check   */
    #define SDRAM_MCOPT1_MCHK_CHK_REP    0x30000000 /* ECC gen, chk, rpt */
    #define SDRAM_MCOPT1_MCHK_CHK_DECODE(n) ((((u32)(n))>>28)&0x3)
    
    /*
     * ECC Error Status
     */
    #define SDRAM_ECCES_MASK                 PPC_REG_VAL(21, 0x3FFFFF)
    #define SDRAM_ECCES_BNCE_MASK            PPC_REG_VAL(15, 0xFFFF)
    #define SDRAM_ECCES_BNCE_ENCODE(lane)    PPC_REG_VAL(((lane) & 0xF), 1)
    #define SDRAM_ECCES_CKBER_MASK           PPC_REG_VAL(17, 0x3)
    #define SDRAM_ECCES_CKBER_NONE           PPC_REG_VAL(17, 0)
    #define SDRAM_ECCES_CKBER_16_ECC_0_3     PPC_REG_VAL(17, 2)
    #define SDRAM_ECCES_CKBER_32_ECC_0_3     PPC_REG_VAL(17, 1)
    #define SDRAM_ECCES_CKBER_32_ECC_4_8     PPC_REG_VAL(17, 2)
    #define SDRAM_ECCES_CKBER_32_ECC_0_8     PPC_REG_VAL(17, 3)
    #define SDRAM_ECCES_CE                   PPC_REG_VAL(18, 1)
    #define SDRAM_ECCES_UE                   PPC_REG_VAL(19, 1)
    #define SDRAM_ECCES_BKNER_MASK           PPC_REG_VAL(21, 0x3)
    #define SDRAM_ECCES_BK0ER                PPC_REG_VAL(20, 1)
    #define SDRAM_ECCES_BK1ER                PPC_REG_VAL(21, 1)

Regarding other items that might go into the DTS, I don't have enough
visibility into the other processors that this controller is used on (440SP,
440SPe, 460EX and 460GT) beyond the 405EX[r]; however, I am guessing the
interrupts are almost guaranteed to be different from processor to processor
suggesting an entry akin to:

        SDRAM0: memory-controller {
            compatible = "ibm,sdram-405ex", "ibm,sdram-4xx-ddr2";
            dcr-reg = <0x010 0x002>;

            ECC0: ecc {
                ...
                interrupt-parent = <&SDRAM0>;
                interrupts = <0x0 0x1>;
                interrupt-map = </* ECCDED Error */ 0x0 &UIC2 0x5 0x4
                                 /* ECCSEC Error */ 0x1 &UIC2 0x6 0x4>;
                ...
            };
        };

or:

        SDRAM0: memory-controller {
            compatible = "ibm,sdram-405ex", "ibm,sdram-4xx-ddr2";
            dcr-reg = <0x010 0x002>;
            ...
            interrupt-parent = <&SDRAM0>;
            interrupts = <0x0 0x1>;
            interrupt-map = </* ECCDED Error */ 0x0 &UIC2 0x5 0x4
                             /* ECCSEC Error */ 0x1 &UIC2 0x6 0x4>;
            ...
        };

or some such. My DTS expertise is near zero and growing.

Regards,

Grant

Re: PPC4xx ECC Configs, Defines and Source

[Josh Boyer]

On Mon, 08 Dec 2008 14:08:01 -0800
Grant Erickson <gerickson@nuovations.com> wrote:

> On 12/8/08 12:21 PM, Josh Boyer wrote:
> > On Mon, Dec 08, 2008 at 11:28:15AM -0800, Grant Erickson wrote:
> >> Does anyone have any strong preferences on where configurations, definitions
> >> and sources for a PPC4xx ECC monitoring and reporting driver should go?
> >> 
> >> Specifically, this concerns ECC handling code for the IBM DDR2 ECC
> >> controller found in the 405EX[r], 440SP, 440SPe, 460EX and 460GT. However,
> >> I'd like to do this in a way that other ECC contributors and maintainers can
> >> build on.
> >> 
> >> Static Configs
> >> --------------
> >> 
> >> I thought I might leverage the definitions Stefan Roese came up with for
> >> u-boot for the base memory controller:
> >> 
> >>    CONFIG_PPC4xx_IBM_SDRAM:   Applicable to 405GP, 405CR, 405EP, AP1000,
> >>                               and ML2
> >>    CONFIG_PPC4xx_IBM_DDR:     Applicable to 440GP, 440GX, 440EP, and 440GR
> >>    CONFIG_PPC4xx_DENALI_DDR2: Applicable to 440EPX and 440GRX
> >>    CONFIG_PPC4xx_IBM_DDR2:    Applicable to 405EX[r], 440SP, 440SPe, 460EX
> >>                               and 460GT.
> > 
> > Config options for what?  Enabling certain function?  Not sure that's needed
> > if we can get away with it by just binding to the appropriate controller.
> 
> A good clarifying question. The configs would cover enabling compiled-in or
> module support for the ECC monitoring and reporting code under discussion. I
> suspect embedded platforms that do not have ECC would not want to compile
> support for this.

Hm.  Still not sure about that.  We have a multi-board config that
still needs to work, so we'd theoretically want to be able to enable
all, some, or none of these and not have the kernel freak out.  Hence
the DTS bindings are important.

> >> Controlling whether ECC monitoring and reporting support should be compiled
> >> in or a module:
> >> 
> >>    CONFIG_PPC_ECC or CONFIG_ECC
> > 
> > That's a bit too generic, unless you are trying to make a menu list under
> > that which would be used to allow people to enable things like:
> > CONFIG_4XX_ECC, CONFIG_FSL_ECC, etc.
> 
> I'll have to think about this some more. More elaboration below.
> 
> Today, we can choose CONFIG_405EX. That nails down CONFIG_PPC4xx_IBM_DDR2
> leaving the only open question at that point as to whether CONFIG_ECC is
> 'y', 'n' or 'm'.

Well, at the moment CONFIG_PPC4xx_IBM_DDR2 doesn't exist, and has no
meaning.  Adding a Kconfig option for it just so you can select an ECC
option seems superfluous.  Because if you select no for ECC, then you
have a Kconfig option that does nothing.  You could simply have:

CONFIG_4xx_IBM_DDR2_ECC

or something like that.

> That said, I am sure I am thinking too "statically" about compile-time
> configuration, however.
> 
> >> Source
> >> ------
> >> 
> >>    arch/powerpc/sysdev/
> >>        ppc4xx_ibm_sdram_ecc.c [future]
> >>        ppc4xx_ibm_ddr_ecc.c [future]
> >>        ppc4xx_ibm_ddr2_ecc.c
> >>        ppc4xx_ibm_ddr2denali_ecc.c [future]
> > 
> > Why is there a need to have so many files?  I would think you could
> > have a single file with all the ECC monitoring implementations in it
> > called ppc4xx_ecc.c (or such).  Surely they would share some amount
> > of code?
> 
> Based on my foggy memory of the 405GP (ibm_sdram_ecc) roughly ten years ago,
> a cursory look at today's Denali controller (from u-boot code), and the
> 405EX[r], the ECC handling code for these is all quite different.
> 
> There will likely be some common, shared code and that could go in something
> like ppc4xx_ecc.c. My focus is only on the 405EX[r] and the associated DDR2
> controller, so further abstraction will probably occur once another
> controller is integrated.

I'll buy that.  Just start with ppc4xx_ecc.c with your initial support
and then we can refactor as other implementations get added (if they
do).

> >> Headers and Defines
> >> -------------------
> >> 
> >>    arch/powerpc/include/asm/ppc4xx_ibm_ddr2.h
> >> 
> >>    OR
> >> 
> >>    arch/powerpc/sysdev/ppc4xx_ibm_ddr2.h
> > 
> > That depends on the contents of the file.  If it's DCR defines, etc
> > it should be in the dcr-regs.h file.  Otherwise, I would think having
> > the header in sysdev should be sufficient unless there is some other
> > facility that would need whatever the contents there are.
> 
> Thanks for the confirmation. That was my operating assumption. They would be
> DCR sub-definitions, such as:

Those you can probably leave in the local .h file.

> Regarding other items that might go into the DTS, I don't have enough
> visibility into the other processors that this controller is used on (440SP,
> 440SPe, 460EX and 460GT) beyond the 405EX[r]; however, I am guessing the
> interrupts are almost guaranteed to be different from processor to processor
> suggesting an entry akin to:

<snip>

>         SDRAM0: memory-controller {
>             compatible = "ibm,sdram-405ex", "ibm,sdram-4xx-ddr2";
>             dcr-reg = <0x010 0x002>;
>             ...
>             interrupt-parent = <&SDRAM0>;
>             interrupts = <0x0 0x1>;
>             interrupt-map = </* ECCDED Error */ 0x0 &UIC2 0x5 0x4
>                              /* ECCSEC Error */ 0x1 &UIC2 0x6 0x4>;
>             ...
>         };

More like this one.

> or some such. My DTS expertise is near zero and growing.

That's pretty darn good so far.

Just to make sure, are you planning on just implementing a driver to
deal with whatever settings the bootloader configured?  E.g., if ECC is
enabled deal with correctable/uncorrectable errors and if not, do
nothing?  Basically you are looking to implement a scrub driver, yes?

I ask because since ECC is memory module specific and memory controller
setup is pretty tricky, I think it's best to leave whatever
configuration the bootloader set and work with that.  Having to redo
memory controller setups in Linux to enable ECC isn't something I'd
look forward to.

josh

Re: PPC4xx ECC Configs, Defines and Source

[Grant Erickson]

On 12/8/08 3:10 PM, Josh Boyer wrote:
> On Mon, 08 Dec 2008 14:08:01 -0800
> Grant Erickson <gerickson@nuovations.com> wrote:
> 
> Well, at the moment CONFIG_PPC4xx_IBM_DDR2 doesn't exist, and has no
> meaning.  Adding a Kconfig option for it just so you can select an ECC
> option seems superfluous.  Because if you select no for ECC, then you
> have a Kconfig option that does nothing.  You could simply have:
> 
> CONFIG_4xx_IBM_DDR2_ECC
> 
> or something like that.

Reasonable enough.

> I'll buy that.  Just start with ppc4xx_ecc.c with your initial support
> and then we can refactor as other implementations get added (if they
> do).

Sounds good.

>>         SDRAM0: memory-controller {
>>             compatible = "ibm,sdram-405ex", "ibm,sdram-4xx-ddr2";
>>             dcr-reg = <0x010 0x002>;
>>             ...
>>             interrupt-parent = <&SDRAM0>;
>>             interrupts = <0x0 0x1>;
>>             interrupt-map = </* ECCDED Error */ 0x0 &UIC2 0x5 0x4
>>                              /* ECCSEC Error */ 0x1 &UIC2 0x6 0x4>;
>>             ...
>>         };
> 
> More like this one.
> 
>> or some such. My DTS expertise is near zero and growing.
> 
> That's pretty darn good so far.

Great, thanks.

> Just to make sure, are you planning on just implementing a driver to
> deal with whatever settings the bootloader configured?  E.g., if ECC is
> enabled deal with correctable/uncorrectable errors and if not, do
> nothing?  Basically you are looking to implement a scrub driver, yes?
> 
> I ask because since ECC is memory module specific and memory controller
> setup is pretty tricky, I think it's best to leave whatever
> configuration the bootloader set and work with that.  Having to redo
> memory controller setups in Linux to enable ECC isn't something I'd
> look forward to.

Precisely. The driver will basically check if ECC is enabled (as was set/not
set by u-boot) and, if so, will take ECC SEC/DED interrupts, log SEC errors
to some data structure fetchable by a proc entry or some device node. For
DED errors, execute on some policy, at its simplest, generating a panic.

At no point will the driver/code attempt to change the controller
configuration beyond reading/clearing ECC event/interrupt status.

Regards,

Grant

Re: PPC4xx ECC Configs, Defines and Source

[Stefan Roese]

Hi Grant,

On Tuesday 09 December 2008, Grant Erickson wrote:
> > Just to make sure, are you planning on just implementing a driver to
> > deal with whatever settings the bootloader configured?  E.g., if ECC is
> > enabled deal with correctable/uncorrectable errors and if not, do
> > nothing?  Basically you are looking to implement a scrub driver, yes?
> >
> > I ask because since ECC is memory module specific and memory controller
> > setup is pretty tricky, I think it's best to leave whatever
> > configuration the bootloader set and work with that.  Having to redo
> > memory controller setups in Linux to enable ECC isn't something I'd
> > look forward to.
>
> Precisely. The driver will basically check if ECC is enabled (as was
> set/not set by u-boot) and, if so, will take ECC SEC/DED interrupts, log
> SEC errors to some data structure fetchable by a proc entry or some device
> node. For DED errors, execute on some policy, at its simplest, generating a
> panic.
>
> At no point will the driver/code attempt to change the controller
> configuration beyond reading/clearing ECC event/interrupt status.

Seems that such a driver should be implemented in the Linux EDAC subsystem 
(see drivers/edac and Documentation/edac.txt) to me. Did you take a look at 
this?

Best regards,
Stefan

Re: PPC4xx ECC Configs, Defines and Source

[Grant Erickson]

On 12/8/08 9:57 PM, Stefan Roese wrote:
> On Tuesday 09 December 2008, Grant Erickson wrote:
>>> Just to make sure, are you planning on just implementing a driver to
>>> deal with whatever settings the bootloader configured?  E.g., if ECC is
>>> enabled deal with correctable/uncorrectable errors and if not, do
>>> nothing?  Basically you are looking to implement a scrub driver, yes?
>>> 
>>> I ask because since ECC is memory module specific and memory controller
>>> setup is pretty tricky, I think it's best to leave whatever
>>> configuration the bootloader set and work with that.  Having to redo
>>> memory controller setups in Linux to enable ECC isn't something I'd
>>> look forward to.
>> 
>> Precisely. The driver will basically check if ECC is enabled (as was
>> set/not set by u-boot) and, if so, will take ECC SEC/DED interrupts, log
>> SEC errors to some data structure fetchable by a proc entry or some device
>> node. For DED errors, execute on some policy, at its simplest, generating a
>> panic.
>> 
>> At no point will the driver/code attempt to change the controller
>> configuration beyond reading/clearing ECC event/interrupt status.
> 
> Seems that such a driver should be implemented in the Linux EDAC subsystem
> (see drivers/edac and Documentation/edac.txt) to me. Did you take a look at
> this?

Stefan:

Thanks for the reference. I will give this a closer examination. At first
glance, this looks promising. Somehow, this managed to escape my Google
search dragnet. Their project team may need to work on their page keywords.

Thanks again,

Grant

Re: PPC4xx ECC Configs, Defines and Source

[Benjamin Herrenschmidt]

> >I thought I might leverage the definitions Stefan Roese came up with for
> >u-boot for the base memory controller:
> >
> >    CONFIG_PPC4xx_IBM_SDRAM:   Applicable to 405GP, 405CR, 405EP, AP1000,
> >                               and ML2
> >    CONFIG_PPC4xx_IBM_DDR:     Applicable to 440GP, 440GX, 440EP, and 440GR
> >    CONFIG_PPC4xx_DENALI_DDR2: Applicable to 440EPX and 440GRX
> >    CONFIG_PPC4xx_IBM_DDR2:    Applicable to 405EX[r], 440SP, 440SPe, 460EX
> >                               and 460GT.
> 
> Config options for what?  Enabling certain function?  Not sure that's needed
> if we can get away with it by just binding to the appropriate controller.

You still want to have ways to enable compilation of only selected ones,
so that if I build a kernel for only a single board, I don't carry the
code for everybody else...

The way I see things is that there would be one master config option
that is user visible "Support for ECC monitoring on 4xx SoCs".

The various SoCs themselves are already select'ed by the various
platforms you enabled, so the SoC Kconfig entries could silently then
select which type of memory controllers are enabled

The combination of those types and the master option defines which
sub-drivers are actually built.

> >Controlling whether ECC monitoring and reporting support should be compiled
> >in or a module:
> >
> >    CONFIG_PPC_ECC or CONFIG_ECC
> 
> That's a bit too generic, unless you are trying to make a menu list under
> that which would be used to allow people to enable things like:
> CONFIG_4XX_ECC, CONFIG_FSL_ECC, etc.

Agreed.

> Why is there a need to have so many files?  I would think you could
> have a single file with all the ECC monitoring implementations in it
> called ppc4xx_ecc.c (or such).  Surely they would share some amount
> of code?

Well, it depends how much they share, but I'd rather have separate files
with helpers in ppc4xx_soc.c if it's small, that way, it's easier to
only build selected files based on what SoC support is enabled.

Cheers,
Ben.

Re: PPC4xx ECC Configs, Defines and Source

[Stefan Roese]

On Wednesday 10 December 2008, Benjamin Herrenschmidt wrote:
> > Why is there a need to have so many files?  I would think you could
> > have a single file with all the ECC monitoring implementations in it
> > called ppc4xx_ecc.c (or such).  Surely they would share some amount
> > of code?
>
> Well, it depends how much they share, but I'd rather have separate files
> with helpers in ppc4xx_soc.c if it's small, that way, it's easier to
> only build selected files based on what SoC support is enabled.

ACK. The Denali core for example from 440EPx/GRx is completely different. 
Trying to fit this Denali ECC handling into the IBM-DDR(2) code would result 
in an ugly mess.

Best regards,
Stefan

Re: PPC4xx ECC Configs, Defines and Source

[Josh Boyer]

On Wed, 10 Dec 2008 10:07:16 +0100
Stefan Roese <sr@denx.de> wrote:

> On Wednesday 10 December 2008, Benjamin Herrenschmidt wrote:
> > > Why is there a need to have so many files?  I would think you could
> > > have a single file with all the ECC monitoring implementations in it
> > > called ppc4xx_ecc.c (or such).  Surely they would share some amount
> > > of code?
> >
> > Well, it depends how much they share, but I'd rather have separate files
> > with helpers in ppc4xx_soc.c if it's small, that way, it's easier to
> > only build selected files based on what SoC support is enabled.
> 
> ACK. The Denali core for example from 440EPx/GRx is completely different. 
> Trying to fit this Denali ECC handling into the IBM-DDR(2) code would result 
> in an ugly mess.

That's fine.  I think you are all jumping the gun a bit there.  Grant
is talking about providing a single scrub implementation for 405EXr,
not all of them.  Whether the other implementations ever even
materialize is something entirely different.

Anyway, separate files is fine.  For now, we'll have one. :)

josh