lkml.org 
[lkml]   [2014]   [Oct]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/4] edac, amd64_edac: Add F15h M60h support
    On 10/1/2014 6:32 AM, Borislav Petkov wrote:
    > On Thu, Sep 18, 2014 at 02:57:10PM -0500, Aravind Gopalakrishnan wrote:
    >> This patch adds support for ECC error decoding for F15h M60h processor.
    >> Aside from the usual changes, the patch adds support for some new features
    >> in the processor:
    >> - DDR4(unbuffered, registered); LRDIMM DDR3 support
    >> - relevant debug messages have been modified/added to report these
    >> memory types
    >> - new dbam_to_cs mappers
    >> - if (F15h M60h && LRDIMM); we need a 'multiplier' value to find
    >> cs_size. This multiplier value is obtained from the per-dimm
    >> DCSM register. So, change the interface to accept a 'cs_mask_nr'
    >> value to facilitate this calculation
    >> Misc cleanup:
    >> - amd64_pci_table[] is condensed by using PCI_VDEVICE macro.
    >>
    >> Testing details:
    >> Tested the patch by injecting 'ECC' type errors using mce_amd_inj
    >> and error decoding works fine.
    >>
    >> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
    >> ---
    >> drivers/edac/amd64_edac.c | 226 ++++++++++++++++++++++++++++++++--------------
    >> drivers/edac/amd64_edac.h | 12 ++-
    >> 2 files changed, 169 insertions(+), 69 deletions(-)
    >>
    >> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
    >> index bbd6514..3e265e0 100644
    >> --- a/drivers/edac/amd64_edac.c
    >> +++ b/drivers/edac/amd64_edac.c
    >> @@ -690,11 +690,32 @@ static void debug_display_dimm_sizes(struct amd64_pvt *, u8);
    >>
    >> static void debug_dump_dramcfg_low(struct amd64_pvt *pvt, u32 dclr, int chan)
    >> {
    >> + int cs;
    >> +
    >> edac_dbg(1, "F2x%d90 (DRAM Cfg Low): 0x%08x\n", chan, dclr);
    >>
    >> - edac_dbg(1, " DIMM type: %sbuffered; all DIMMs support ECC: %s\n",
    >> - (dclr & BIT(16)) ? "un" : "",
    >> - (dclr & BIT(19)) ? "yes" : "no");
    >> + if (pvt->fam == 0x15 && pvt->model == 0x60) {
    >> + for_each_chip_select_mask(cs, chan, pvt) {
    >> + u32 dcsm = pvt->csels[chan].csmasks[cs];
    >> +
    >> + if (dcsm & 0x3) {
    >> + /* LRDIMMs */
    >> + edac_dbg(1, " DIMM type: LRDIMM %dx rank multiply;"
    >> + "CS = %d; all DIMMs support ECC: %s\n",
    >> + (dcsm & 0x3), cs,
    >> + (dclr & BIT(19)) ? "yes" : "no");
    > Why do we need to iterate over the DRAM CS sets? Just for the rank
    > multiplier, apparently. We dump those normally in read_dct_base_mask(),
    > though.

    It's not just for rank multiplier.. we find that it's LRDIMM only by
    examining dcsm. Hence the iteration here..

    Not sure if we should move it to read_dct_base_mask() as traditionally
    we report the DIMM type
    in this function.

    >> + } else {
    >> + edac_dbg(1, " DIMM type: %sbuffered; CS = %d;"
    >> + "all DIMMs support ECC: %s\n",
    >> + (dclr & BIT(16)) ? "un" : "", cs,
    >> + (dclr & BIT(19)) ? "yes" : "no");
    >> + }
    >> + }
    >> + } else {
    >> + edac_dbg(1, " DIMM type: %sbuffered; all DIMMs support ECC:"
    >> + "%s\n", (dclr & BIT(16)) ? "un" : "",
    >> + (dclr & BIT(19)) ? "yes" : "no");
    >> + }
    > Single if-else statements don't need {} braces.

    Ok, will fix this.

    >>
    >> edac_dbg(1, " PAR/ERR parity: %s\n",
    >> (dclr & BIT(8)) ? "enabled" : "disabled");
    >> @@ -756,7 +777,7 @@ static void prep_chip_selects(struct amd64_pvt *pvt)
    >> if (pvt->fam == 0xf && pvt->ext_model < K8_REV_F) {
    >> pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
    >> pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 8;
    >> - } else if (pvt->fam == 0x15 && pvt->model >= 0x30) {
    >> + } else if (pvt->fam == 0x15 && pvt->model == 0x30) {
    >> pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
    >> pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
    >> } else {
    >> @@ -817,10 +838,26 @@ static enum mem_type determine_memory_type(struct amd64_pvt *pvt, int cs)
    >> {
    >> enum mem_type type;
    >>
    >> - /* F15h supports only DDR3 */
    >> - if (pvt->fam >= 0x15)
    >> - type = (pvt->dclr0 & BIT(16)) ? MEM_DDR3 : MEM_RDDR3;
    >> - else if (pvt->fam == 0x10 || pvt->ext_model >= K8_REV_F) {
    >> + /* F15h, M60h supports DDR4 too*/
    >> + if (pvt->fam >= 0x15) {
    >> + if (pvt->model == 0x60) {
    >> + /*
    >> + * Since in init_csrow we iterate over just DCT0
    >> + * use '0' for dct values here when necessary.
    >> + */
    >> + u32 dram_ctrl;
    >> + u32 dcsm = pvt->csels[0].csmasks[cs];
    >> +
    >> + amd64_read_dct_pci_cfg(pvt, 0, DRAM_CONTROL,
    >> + &dram_ctrl);
    > We're reading DRAM_CONTROL at two locations, maybe we should cache it in
    > pvt->dram_ctl ?

    Makes sense. Will do this.

    >> + type = (((dram_ctrl >> 8) & 0x7) == 0x2) ? MEM_DDR4 :
    >> + (pvt->dclr0 & BIT(16)) ? MEM_DDR3 :
    >> + (dcsm & 0x3) ? MEM_LRDDR3 : MEM_RDDR3;
    > Doesn't D18F2x78_dct[3:0][DramType] define all possible types or do you
    > have to fallback to DCLR for DDR3 types?

    No, we need to fall back to DCLR to realize if it's Unbuffered DIMM or not.

    >> + } else {
    >> + type = (pvt->dclr0 & BIT(16)) ? MEM_DDR3 : MEM_RDDR3;
    >> + }
    > Single if-else statements don't need {} braces. Please read
    > Documentation/CodingStyle.

    Yeah. Sorry about that. Will fix.

    >> +
    >> + } else if (pvt->fam == 0x10 || pvt->ext_model >= K8_REV_F) {
    >> if (pvt->dchr0 & DDR3_MODE)
    >> type = (pvt->dclr0 & BIT(16)) ? MEM_DDR3 : MEM_RDDR3;
    >> else
    >> @@ -958,8 +995,12 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
    >> if (WARN_ON(!nb))
    >> return;
    >>
    >> - pci_func = (pvt->model == 0x30) ? PCI_DEVICE_ID_AMD_15H_M30H_NB_F1
    >> - : PCI_DEVICE_ID_AMD_15H_NB_F1;
    >> + if (pvt->model == 0x60)
    >> + pci_func = PCI_DEVICE_ID_AMD_15H_M60H_NB_F1;
    >> + else if (pvt->model == 0x30)
    >> + pci_func = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
    >> + else
    >> + pci_func = PCI_DEVICE_ID_AMD_15H_NB_F1;
    >>
    >> f1 = pci_get_related_function(nb->misc->vendor, pci_func, nb->misc);
    >> if (WARN_ON(!f1))
    >> @@ -1049,7 +1090,7 @@ static int ddr2_cs_size(unsigned i, bool dct_width)
    >> }
    >>
    >> static int k8_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
    >> - unsigned cs_mode)
    >> + unsigned cs_mode, int cs_mask_nr)
    >> {
    >> u32 dclr = dct ? pvt->dclr1 : pvt->dclr0;
    >>
    >> @@ -1167,8 +1208,43 @@ static int ddr3_cs_size(unsigned i, bool dct_width)
    >> return cs_size;
    >> }
    >>
    >> +static int ddr3_lrdimm_cs_size(unsigned i, unsigned rank_multiply)
    >> +{
    >> + unsigned shift = 0;
    >> + int cs_size = 0;
    >> +
    >> + if (i < 4 || i == 6)
    >> + cs_size = -1;
    >> + else if (i == 12)
    >> + shift = 7;
    >> + else if (!(i & 0x1))
    >> + shift = i >> 1;
    >> + else
    >> + shift = (i + 1) >> 1;
    >> +
    >> + if (cs_size != -1)
    >> + cs_size = rank_multiply * (128 << shift);
    >> +
    >> + return cs_size;
    >> +}
    >> +
    >> +static int ddr4_cs_size(unsigned i)
    >> +{
    >> + int cs_size = 0;
    >> +
    >> + if (i == 0)
    >> + cs_size = -1;
    >> + else if (i == 1)
    >> + cs_size = 1024;
    >> + else
    >> + /* Min cs_size = 1G */
    >> + cs_size = 1024 * (1 << (i >> 1));
    >> +
    >> + return cs_size;
    >> +}
    >> +
    >> static int f10_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
    >> - unsigned cs_mode)
    >> + unsigned cs_mode, int cs_mask_nr)
    >> {
    >> u32 dclr = dct ? pvt->dclr1 : pvt->dclr0;
    >>
    >> @@ -1184,18 +1260,56 @@ static int f10_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
    >> * F15h supports only 64bit DCT interfaces
    >> */
    >> static int f15_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
    >> - unsigned cs_mode)
    >> + unsigned cs_mode, int cs_mask_nr)
    >> {
    >> WARN_ON(cs_mode > 12);
    >>
    >> return ddr3_cs_size(cs_mode, false);
    >> }
    >>
    >> +/* F15h M60h supports DDR4 mapping as well.. */
    >> +static int f15_m60h_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
    >> + unsigned cs_mode, int cs_mask_nr)
    >> +{
    >> + int cs_size;
    >> + enum mem_type type;
    >> + u32 dram_ctrl;
    >> + u32 dcsm = pvt->csels[dct].csmasks[cs_mask_nr];
    >> +
    >> + WARN_ON(cs_mode > 12);
    >> +
    >> + amd64_read_dct_pci_cfg(pvt, dct, DRAM_CONTROL, &dram_ctrl);
    >> + type = (((dram_ctrl >> 8) & 0x7) == 0x2) ? MEM_DDR4 :
    >> + (pvt->dclr0 & BIT(16)) ? MEM_DDR3 :
    >> + (dcsm & 0x3) ? MEM_LRDDR3 : MEM_RDDR3;
    > This is the second time we're determining memory type, maybe we should
    > cache that too into pvt->dram_type...?

    Yes, Will do.

    >> +
    >> + if (type == MEM_DDR4) {
    >> + if (cs_mode > 9)
    >> + return -1;
    >> +
    >> + cs_size = ddr4_cs_size(cs_mode);
    >> + } else if (type == MEM_LRDDR3) {
    >> + unsigned rank_multiply = dcsm & 0xf;
    >> +
    >> + if (rank_multiply == 3)
    >> + rank_multiply = 4;
    >> + cs_size = ddr3_lrdimm_cs_size(cs_mode, rank_multiply);
    >> + } else {
    >> + /* Minimum cs size is 512mb for F15hM60h*/
    >> + if (cs_mode == 0x1)
    >> + return -1;
    >> +
    >> + cs_size = ddr3_cs_size(cs_mode, false);
    >> + }
    >> +
    >> + return cs_size;
    >> +}

    Shall resend as V2.

    Thanks,
    -Aravind.


    \
     
     \ /
      Last update: 2014-10-01 18:01    [W:2.213 / U:0.112 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site