[PATCH 4/4] edac, amd64_edac: Add F15h M60h support

[Aravind Gopalakrishnan]

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 <***@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");
+ } 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");
+ }

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);
+ type = (((dram_ctrl >> 8) & 0x7) == 0x2) ? MEM_DDR4 :
+ (pvt->dclr0 & BIT(16)) ? MEM_DDR3 :
+ (dcsm & 0x3) ? MEM_LRDDR3 : MEM_RDDR3;
+ } else {
+ type = (pvt->dclr0 & BIT(16)) ? MEM_DDR3 : MEM_RDDR3;
+ }
+
+ } 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;
+
+ 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;
+}
+
/*
* F16h and F15h model 30h have only limited cs_modes.
*/
static int f16_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);

@@ -1757,13 +1871,20 @@ static void debug_display_dimm_sizes(struct amd64_pvt *pvt, u8 ctrl)

size0 = 0;
if (dcsb[dimm*2] & DCSB_CS_ENABLE)
+ /* For f15m60h, need multiplier for LRDIMM cs_size
+ * calculation. We pass 'dimm' value to the dbam_to_cs
+ * mapper so we can find the multiplier from the
+ * corresponding DCSM.
+ */
size0 = pvt->ops->dbam_to_cs(pvt, ctrl,
- DBAM_DIMM(dimm, dbam));
+ DBAM_DIMM(dimm, dbam),
+ dimm);

size1 = 0;
if (dcsb[dimm*2 + 1] & DCSB_CS_ENABLE)
size1 = pvt->ops->dbam_to_cs(pvt, ctrl,
- DBAM_DIMM(dimm, dbam));
+ DBAM_DIMM(dimm, dbam),
+ dimm);

amd64_info(EDAC_MC ": %d: %5dMB %d: %5dMB\n",
dimm * 2, size0,
@@ -1812,6 +1933,16 @@ static struct amd64_family_type family_types[] = {
.dbam_to_cs = f16_dbam_to_chip_select,
}
},
+ [F15_M60H_CPUS] = {
+ .ctl_name = "F15h_M60h",
+ .f1_id = PCI_DEVICE_ID_AMD_15H_M60H_NB_F1,
+ .f3_id = PCI_DEVICE_ID_AMD_15H_M60H_NB_F3,
+ .ops = {
+ .early_channel_count = f1x_early_channel_count,
+ .map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow,
+ .dbam_to_cs = f15_m60h_dbam_to_chip_select,
+ }
+ },
[F16_CPUS] = {
.ctl_name = "F16h",
.f1_id = PCI_DEVICE_ID_AMD_16H_NB_F1,
@@ -2238,7 +2369,8 @@ static u32 get_csrow_nr_pages(struct amd64_pvt *pvt, u8 dct, int csrow_nr)
*/
cs_mode = DBAM_DIMM(csrow_nr / 2, dbam);

- nr_pages = pvt->ops->dbam_to_cs(pvt, dct, cs_mode) << (20 - PAGE_SHIFT);
+ nr_pages = pvt->ops->dbam_to_cs(pvt, dct, cs_mode, (csrow_nr / 2))
+ << (20 - PAGE_SHIFT);

edac_dbg(0, "csrow: %d, channel: %d, DBAM idx: %d\n",
csrow_nr, dct, cs_mode);
@@ -2604,6 +2736,10 @@ static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt)
fam_type = &family_types[F15_M30H_CPUS];
pvt->ops = &family_types[F15_M30H_CPUS].ops;
break;
+ } else if (pvt->model == 0x60) {
+ fam_type = &family_types[F15_M60H_CPUS];
+ pvt->ops = &family_types[F15_M60H_CPUS].ops;
+ break;
}

fam_type = &family_types[F15_CPUS];
@@ -2828,55 +2964,13 @@ static void remove_one_instance(struct pci_dev *pdev)
* inquiry this table to see if this driver is for a given device found.
*/
static const struct pci_device_id amd64_pci_table[] = {
- {
- .vendor = PCI_VENDOR_ID_AMD,
- .device = PCI_DEVICE_ID_AMD_K8_NB_MEMCTL,
- .subvendor = PCI_ANY_ID,
- .subdevice = PCI_ANY_ID,
- .class = 0,
- .class_mask = 0,
- },
- {
- .vendor = PCI_VENDOR_ID_AMD,
- .device = PCI_DEVICE_ID_AMD_10H_NB_DRAM,
- .subvendor = PCI_ANY_ID,
- .subdevice = PCI_ANY_ID,
- .class = 0,
- .class_mask = 0,
- },
- {
- .vendor = PCI_VENDOR_ID_AMD,
- .device = PCI_DEVICE_ID_AMD_15H_NB_F2,
- .subvendor = PCI_ANY_ID,
- .subdevice = PCI_ANY_ID,
- .class = 0,
- .class_mask = 0,
- },
- {
- .vendor = PCI_VENDOR_ID_AMD,
- .device = PCI_DEVICE_ID_AMD_15H_M30H_NB_F2,
- .subvendor = PCI_ANY_ID,
- .subdevice = PCI_ANY_ID,
- .class = 0,
- .class_mask = 0,
- },
- {
- .vendor = PCI_VENDOR_ID_AMD,
- .device = PCI_DEVICE_ID_AMD_16H_NB_F2,
- .subvendor = PCI_ANY_ID,
- .subdevice = PCI_ANY_ID,
- .class = 0,
- .class_mask = 0,
- },
- {
- .vendor = PCI_VENDOR_ID_AMD,
- .device = PCI_DEVICE_ID_AMD_16H_M30H_NB_F2,
- .subvendor = PCI_ANY_ID,
- .subdevice = PCI_ANY_ID,
- .class = 0,
- .class_mask = 0,
- },
-
+ { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_K8_NB_MEMCTL) },
+ { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_10H_NB_DRAM) },
+ { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_NB_F2) },
+ { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F2) },
+ { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_15H_M60H_NB_F2) },
+ { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_NB_F2) },
+ { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_16H_M30H_NB_F2) },
{0, }
};
MODULE_DEVICE_TABLE(pci, amd64_pci_table);
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 55fb594..dbbae84 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -162,10 +162,12 @@
/*
* PCI-defined configuration space registers
*/
-#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F1 0x141b
-#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F2 0x141c
#define PCI_DEVICE_ID_AMD_15H_NB_F1 0x1601
#define PCI_DEVICE_ID_AMD_15H_NB_F2 0x1602
+#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F1 0x141b
+#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F2 0x141c
+#define PCI_DEVICE_ID_AMD_15H_M60H_NB_F1 0x1571
+#define PCI_DEVICE_ID_AMD_15H_M60H_NB_F2 0x1572
#define PCI_DEVICE_ID_AMD_16H_NB_F1 0x1531
#define PCI_DEVICE_ID_AMD_16H_NB_F2 0x1532
#define PCI_DEVICE_ID_AMD_16H_M30H_NB_F1 0x1581
@@ -221,6 +223,8 @@

#define csrow_enabled(i, dct, pvt) ((pvt)->csels[(dct)].csbases[(i)] & DCSB_CS_ENABLE)

+#define DRAM_CONTROL 0x78
+
#define DBAM0 0x80
#define DBAM1 0x180

@@ -301,6 +305,7 @@ enum amd_families {
F10_CPUS,
F15_CPUS,
F15_M30H_CPUS,
+ F15_M60H_CPUS,
F16_CPUS,
F16_M30H_CPUS,
NUM_FAMILIES,
@@ -480,7 +485,8 @@ struct low_ops {
int (*early_channel_count) (struct amd64_pvt *pvt);
void (*map_sysaddr_to_csrow) (struct mem_ctl_info *mci, u64 sys_addr,
struct err_info *);
- int (*dbam_to_cs) (struct amd64_pvt *pvt, u8 dct, unsigned cs_mode);
+ int (*dbam_to_cs) (struct amd64_pvt *pvt, u8 dct,
+ unsigned cs_mode, int cs_mask_nr);
};

struct amd64_family_type {

[Borislav Petkov]

Post by Aravind Gopalakrishnan
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
- 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
- amd64_pci_table[] is condensed by using PCI_VDEVICE macro.
Tested the patch by injecting 'ECC' type errors using mce_amd_inj
and error decoding works fine.
---
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.

Post by Aravind Gopalakrishnan
+ } 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.

Post by Aravind Gopalakrishnan
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 ?

Post by Aravind Gopalakrishnan
+ (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?

Post by Aravind Gopalakrishnan
+ } else {
+ type = (pvt->dclr0 & BIT(16)) ? MEM_DDR3 : MEM_RDDR3;
+ }

Single if-else statements don't need {} braces. Please read
Documentation/CodingStyle.

Post by Aravind Gopalakrishnan
+
+ } 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);
+ (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...?

Post by Aravind Gopalakrishnan
+
+ 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;
+}

[Aravind Gopalakrishnan]

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
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
- 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
- amd64_pci_table[] is condensed by using PCI_VDEVICE macro.
Tested the patch by injecting 'ECC' type errors using mce_amd_inj
and error decoding works fine.
---
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.

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
+ } 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.

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
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.

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
+ (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.

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
+ } 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.

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
+
+ } 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);
+ (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.

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
+
+ 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.

[Borislav Petkov]

Post by Aravind Gopalakrishnan

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
+ 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..

So we can look only at the first DCSM, no? Or are there systems with
different types of LRDIMMs on one DCT?

[Aravind Gopalakrishnan]

Post by Borislav Petkov

Post by Aravind Gopalakrishnan

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
+ 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..

So we can look only at the first DCSM, no? Or are there systems with
different types of LRDIMMs on one DCT?

Not AFAIK, But I'll ask to make sure.
If different LRDIMMs on one DCT don't exist, then sure, I'll modify the
code to use first DCSM.

Regarding resend-
I'm not sure how you'll want the patches.. I just noticed a
'edac-for-3.19' branch.
Would you prefer I based changes on top of this for merging purposes?
Or just resend the original 4 patches based off tip as usual?

Thanks,
-Aravind.

[Borislav Petkov]

Post by Aravind Gopalakrishnan
Not AFAIK, But I'll ask to make sure.

Yes please.

Post by Aravind Gopalakrishnan
If different LRDIMMs on one DCT don't exist, then sure, I'll modify the code
to use first DCSM.

Right.

Post by Aravind Gopalakrishnan
Regarding resend-
I'm not sure how you'll want the patches.. I just noticed a 'edac-for-3.19'
branch.

I uploaded it for the 0day bot so that the patches can be build-tested a
little with different configs. Don't pay attention to any other branches
except to "for-next" which is what will go to Linus come next merge
window. The other branches might get rebased and you shouldn't base any
work ontop.

Post by Aravind Gopalakrishnan
Would you prefer I based changes on top of this for merging purposes?
Or just resend the original 4 patches based off tip as usual?

Nah, just resend the last one - 4/4 - the other three are fine.

Thanks.

[Aravind Gopalakrishnan]

Post by Borislav Petkov

Post by Aravind Gopalakrishnan

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
+ 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..

So we can look only at the first DCSM, no? Or are there systems with
different types of LRDIMMs on one DCT?

So it seems we can theoretically have systems in this manner, but that
is not a common case..

So, shall I just use first DCSM to keep it simple for now?
And find rank multiplier iteratively as and when need arises?

Thanks,
-Aravind.

[Borislav Petkov]

So it seems we can theoretically have systems in this manner, but that is
not a common case..
So, shall I just use first DCSM to keep it simple for now?
And find rank multiplier iteratively as and when need arises?

Yep, sounds nicely conservative.

Thanks.

[Aravind Gopalakrishnan]

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
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
- 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
- amd64_pci_table[] is condensed by using PCI_VDEVICE macro.
Tested the patch by injecting 'ECC' type errors using mce_amd_inj
and error decoding works fine.
---
drivers/edac/amd64_edac.c | 226 ++++++++++++++++++++++++++++++++--------------
drivers/edac/amd64_edac.h | 12 ++-
2 files changed, 169 insertions(+), 69 deletions(-)
@@ -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 ?

Post by Aravind Gopalakrishnan
+ (dcsm & 0x3) ? MEM_LRDDR3 : MEM_RDDR3;

@@ -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);
+ (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...?

The more I think about this, I'm finding it's hard to do this cleanly.
I initially thought I'd just cache this in pvt->dram_type the first time
I'm doing this.
But, the pvt->ops->dbam_to_cs() mappers get called first before
determine_memory_type().

So, If I look for dram_type in f15_m60h_dbam_to_chip_select() it's ugly
as that's really the point of
having a determine_memory_type().

Also, there's just a lot of if-else statements in
determine_memory_type() now.
This could benefit from having a per-family low_ops function.
And, we can call this early... somewhere in read_mc_regs() so that we
have information ready to use in
f15_m60h_dbam_to_chip_select() and in init_csrows() which needs
dram_type too.

Oh, btw- We can do away with a pvt->dram_ctrl as
f15_m60h_dbam_to_chip_select() really just needs the dram_type.

Thoughts?

-Aravind.

[Borislav Petkov]

Post by Aravind Gopalakrishnan
The more I think about this, I'm finding it's hard to do this cleanly.
I initially thought I'd just cache this in pvt->dram_type the first time I'm
doing this.
But, the pvt->ops->dbam_to_cs() mappers get called first before
determine_memory_type().
So, If I look for dram_type in f15_m60h_dbam_to_chip_select() it's ugly as
that's really the point of
having a determine_memory_type().
Also, there's just a lot of if-else statements in determine_memory_type()
now.
This could benefit from having a per-family low_ops function.
And, we can call this early... somewhere in read_mc_regs() so that we have
information ready to use in
f15_m60h_dbam_to_chip_select() and in init_csrows() which needs dram_type
too.

Right, this is what I was thinking too: somewhere in read_mc_regs(),
after having collected ->dclr0, you call determine_memory_type() and
store it into pvt->dram_type.

Post by Aravind Gopalakrishnan
Oh, btw- We can do away with a pvt->dram_ctrl as
f15_m60h_dbam_to_chip_select() really just needs the dram_type.

Yes, you make the read of DRAM_CONTROL inside determine_memory_type() as
we don't need it anywhere else.

If we do, all of a sudden, we'll move it up to read_mc_regs(). IOW, I'm
trying to centralize all reg reads in read_mc_regs() and use locally
cached info later so I don't have to access the hardware each time
needlessly, if it can be helped.

Thanks.

[Aravind Gopalakrishnan]

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
The more I think about this, I'm finding it's hard to do this cleanly.
I initially thought I'd just cache this in pvt->dram_type the first time I'm
doing this.
But, the pvt->ops->dbam_to_cs() mappers get called first before
determine_memory_type().
So, If I look for dram_type in f15_m60h_dbam_to_chip_select() it's ugly as
that's really the point of
having a determine_memory_type().
Also, there's just a lot of if-else statements in determine_memory_type()
now.
This could benefit from having a per-family low_ops function.
And, we can call this early... somewhere in read_mc_regs() so that we have
information ready to use in
f15_m60h_dbam_to_chip_select() and in init_csrows() which needs dram_type
too.

Right, this is what I was thinking too: somewhere in read_mc_regs(),
after having collected ->dclr0, you call determine_memory_type() and
store it into pvt->dram_type.

Yep.
So, let me go ahead and make these changes.
Shall send out a V2 once I have an answer on the LRDIMM question..

Thanks,
-Aravind.

Post by Borislav Petkov

Post by Aravind Gopalakrishnan
Oh, btw- We can do away with a pvt->dram_ctrl as
f15_m60h_dbam_to_chip_select() really just needs the dram_type.

Yes, you make the read of DRAM_CONTROL inside determine_memory_type() as
we don't need it anywhere else.
If we do, all of a sudden, we'll move it up to read_mc_regs(). IOW, I'm
trying to centralize all reg reads in read_mc_regs() and use locally
cached info later so I don't have to access the hardware each time
needlessly, if it can be helped.