Re: PPC4xx ECC Configs, Defines and Source

[Grant Erickson <gerickson@nuovations.com>]

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