Changes from V1:
- Splitting up the patch
- Remove unnecessary helper functions
- Add family/model to amd64_pvt
- Cleanup indent issues
Aravind Gopalakrishnan (3):
EDAC, AMD64_EDAC: Add PCI Device ID Functions 3 and 4 for newer F15h
models.
EDAC, AMD64_EDAC: Add relevant condition checks as F15h M30h does not
support GART or L3.
EDAC, AMD64_EDAC: Add ECC decoding support for newer F15h models.
arch/x86/kernel/amd_nb.c | 14 ++-
drivers/edac/amd64_edac.c | 276 +++++++++++++++++++++++++++++++++++++++++----
drivers/edac/amd64_edac.h | 65 ++++++++++-
include/linux/pci_ids.h | 2 +
4 files changed, 330 insertions(+), 27 deletions(-)
--
1.7.10.4
Adding code to check for specific model (F15h, M30h) and if yes,
do not add flag AMD_NB_GART. Also check cpuid_edx(0x80000006) for
prescence of L3. If no L3, do not add any L3 flags.
Signed-off-by: Aravind Gopalakrishnan <[email protected]>
diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
index 3048ded..3ee7a4d 100644
--- a/arch/x86/kernel/amd_nb.c
+++ b/arch/x86/kernel/amd_nb.c
@@ -20,6 +20,7 @@ const struct pci_device_id amd_nb_misc_ids[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F3) },
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M10H_F3) },
+ { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F3) },
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F3) },
{}
};
@@ -27,6 +28,7 @@ EXPORT_SYMBOL(amd_nb_misc_ids);
static const struct pci_device_id amd_nb_link_ids[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
+ { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
{}
};
@@ -81,13 +83,21 @@ int amd_cache_northbridges(void)
next_northbridge(misc, amd_nb_misc_ids);
node_to_amd_nb(i)->link = link =
next_northbridge(link, amd_nb_link_ids);
- }
+ }
+ /* GART present only on Fam15h upto model 0fh */
if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
- boot_cpu_data.x86 == 0x15)
+ (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
amd_northbridges.flags |= AMD_NB_GART;
/*
+ * Check CPUID Fn8000_0006_EDX: L3 Cache Identifiers.
+ * If == 0, then no need to proceed as there is no L3.
+ */
+ if (cpuid_edx(0x80000006) == 0)
+ return 0;
+
+ /*
* Some CPU families support L3 Cache Index Disable. There are some
* limitations because of E382 and E388 on family 0x10.
*/
--
1.7.10.4
Adding support for handling ECC error decoding for new F15 models.
On newer models, support has been included for upto 4 DCT's,
however, only DCT0 and DCT3 are currently configured. (Refer BKDG Section 2.10)
There is also a new "Routing DRAM Requests" algorithm for this model.
Tested on Fam15h M30h with ECC turned on using mce_amd_inj facility and
verified to be functionally correct.
Signed-off-by: Aravind Gopalakrishnan <[email protected]>
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 8b6a034..42dab12 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -123,7 +123,11 @@ static void f15h_select_dct(struct amd64_pvt *pvt, u8 dct)
u32 reg = 0;
amd64_read_pci_cfg(pvt->F1, DCT_CFG_SEL, ®);
- reg &= 0xfffffffe;
+ /*
+ * If Fam15h M30h, mask out last two bits for programming dct.
+ * Else, only mask out the last bit.
+ */
+ reg &= (F15_M30H_CPUS) ? 0xfffffffc : 0xfffffff8;
reg |= dct;
amd64_write_pci_cfg(pvt->F1, DCT_CFG_SEL, reg);
}
@@ -133,8 +137,12 @@ static int f15_read_dct_pci_cfg(struct amd64_pvt *pvt, int addr, u32 *val,
{
u8 dct = 0;
+ /*
+ * For F15 M30h, the second dct is DCT 3.
+ * Refer BKDG Section 2.10
+ */
if (addr >= 0x140 && addr <= 0x1a0) {
- dct = 1;
+ dct = F15H_M30H_CPU ? 3 : 1;
addr -= 0x100;
}
@@ -205,8 +213,9 @@ static int amd64_set_scrub_rate(struct mem_ctl_info *mci, u32 bw)
if (boot_cpu_data.x86 == 0xf)
min_scrubrate = 0x0;
- /* F15h Erratum #505 */
- if (boot_cpu_data.x86 == 0x15)
+ /* F15h Models 0x00 - 0x0f Erratum #505 */
+ if (boot_cpu_data.x86 == 0x15 &&
+ boot_cpu_data.x86_model != 0x30)
f15h_select_dct(pvt, 0);
return __amd64_set_scrub_rate(pvt->F3, bw, min_scrubrate);
@@ -218,8 +227,9 @@ static int amd64_get_scrub_rate(struct mem_ctl_info *mci)
u32 scrubval = 0;
int i, retval = -EINVAL;
- /* F15h Erratum #505 */
- if (boot_cpu_data.x86 == 0x15)
+ /* F15h Models 0x00 - 0x0f Erratum #505 */
+ if (boot_cpu_data.x86 == 0x15 &&
+ boot_cpu_data.x86_model != 0x30)
f15h_select_dct(pvt, 0);
amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
@@ -343,10 +353,10 @@ static void get_cs_base_and_mask(struct amd64_pvt *pvt, int csrow, u8 dct,
addr_shift = 4;
/*
- * F16h needs two addr_shift values: 8 for high and 6 for low
- * (cf. F16h BKDG).
+ * F16h and F15h(model 30h) needs two addr_shift values:
+ * 8 for high and 6 for low (cf. F16h BKDG).
*/
- } else if (boot_cpu_data.x86 == 0x16) {
+ } else if (boot_cpu_data.x86 == 0x16 || F15H_M30H_CPU) {
csbase = pvt->csels[dct].csbases[csrow];
csmask = pvt->csels[dct].csmasks[csrow >> 1];
@@ -743,6 +753,9 @@ static void prep_chip_selects(struct amd64_pvt *pvt)
if (boot_cpu_data.x86 == 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 (F15H_M30H_CPU) {
+ pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
+ pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
} else {
pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
@@ -850,9 +863,9 @@ static u64 get_error_address(struct mce *m)
addr = m->addr & GENMASK(start_bit, end_bit);
/*
- * Erratum 637 workaround
+ * Erratum 637 workaround for F15h Models 0x00 - 0x0f
*/
- if (c->x86 == 0x15) {
+ if (c->x86 == 0x15 && c->x86_model != 0x30) {
struct amd64_pvt *pvt;
u64 cc6_base, tmp_addr;
u32 tmp;
@@ -918,6 +931,7 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
struct amd_northbridge *nb;
struct pci_dev *misc, *f1 = NULL;
struct cpuinfo_x86 *c = &boot_cpu_data;
+ unsigned int pci_func;
int off = range << 3;
u32 llim;
@@ -942,7 +956,12 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
return;
misc = nb->misc;
- f1 = pci_get_related_function(misc->vendor, PCI_DEVICE_ID_AMD_15H_NB_F1, misc);
+
+ pci_func = (c->x86_model == 0x30) ? PCI_DEVICE_ID_AMD_15H_M30H_NB_F1
+ : PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
+
+ f1 = pci_get_related_function(misc->vendor, pci_func, misc);
+
if (WARN_ON(!f1))
return;
@@ -1173,7 +1192,8 @@ static int f15_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
}
/*
- * F16h has only limited cs_modes
+ * F16h has only limited cs_modes.
+ * F15 M30h follows the same pattern.
*/
static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
unsigned cs_mode)
@@ -1218,6 +1238,51 @@ static void read_dram_ctl_register(struct amd64_pvt *pvt)
}
/*
+ * Determine channel (DCT) based on the interleaving mode:
+ * F15h M30h BKDG, 2.10.12 Memory Interleaving Modes.
+ */
+static u8 f15_m30h_determine_channel(struct amd64_pvt *pvt, u64 sys_addr,
+ u8 intlv_en, int num_dcts_intlv, u32 dct_sel)
+{
+ u8 channel = 0;
+ u8 select;
+ u8 intlv_addr = dct_sel_interleave_addr(pvt);
+
+ if (!(intlv_en))
+ return (u8)(dct_sel);
+ /*
+ * If num_dcts_intlv == 2, them consider addr bit 8 or addr bit 9
+ * depending on intlv_addr, then return either channel = 0/1/2/3.
+ * If num_dcts_intlv == 4, consider either bits[8:9] or bits[9:10]
+ * depending on intlv_addr, then return the value obtained.
+ */
+
+ if (num_dcts_intlv == 2) {
+ if (intlv_addr == 0x4)
+ select = (sys_addr >> 8) & BIT(0);
+ else if (intlv_addr == 0x5)
+ select = (sys_addr >> 9) & BIT(0);
+ else
+ return -EINVAL;
+
+ /* If select !=0, upper DCT selected, else lower DCT */
+ channel = select ? 0x3 : 0;
+
+ } else if (num_dcts_intlv == 4) {
+ if (intlv_addr == 0x4)
+ select = (sys_addr >> 8) & 0x3;
+ else if (intlv_addr == 0x5)
+ select = (sys_addr >> 9) & 0x3;
+ else
+ return -EINVAL;
+
+ channel = select;
+ }
+
+ return channel;
+}
+
+/*
* Determine channel (DCT) based on the interleaving mode: F10h BKDG, 2.8.9 Memory
* Interleaving Modes.
*/
@@ -1366,6 +1431,10 @@ static int f1x_lookup_addr_in_dct(u64 in_addr, u8 nid, u8 dct)
(in_addr & cs_mask), (cs_base & cs_mask));
if ((in_addr & cs_mask) == (cs_base & cs_mask)) {
+ if (F15H_M30H_CPU) {
+ cs_found = csrow;
+ break;
+ }
cs_found = f10_process_possible_spare(pvt, dct, csrow);
edac_dbg(1, " MATCH csrow=%d\n", cs_found);
@@ -1492,20 +1561,151 @@ static int f1x_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
return cs_found;
}
-static int f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt, u64 sys_addr,
- int *chan_sel)
+/*
+ * Routing DRAM Requests algorithm is different for F15h M30h.
+ * It is cleaner to use a brand new function rather than
+ * adding quirks in the more generic 'f1x_match_to_this_node'.
+ * Refer "2.10.5 DRAM Routing Requests" in BKDG for info.
+ */
+static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
+ u64 sys_addr, int *chan_sel)
+{
+ int cs_found = -EINVAL;
+ int num_dcts_intlv = 0;
+ u64 chan_addr, chan_offset, high_addr_offset;
+ u64 dct_base, dct_limit;
+ u32 dct_cont_base_reg, dct_cont_limit_reg, tmp;
+ u8 channel, alias_channel, leg_mmio_hole;
+
+
+ /* Read DRAM Control registers specific to F15 M30h. */
+ amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &dct_cont_base_reg);
+ amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &dct_cont_limit_reg);
+
+ /* Extract F15h M30h specific config */
+ u64 dhar_offset = f10_dhar_offset(pvt);
+ u8 dct_offset_en = (u8) ((dct_cont_base_reg >> 3) & BIT(0));
+ u8 dct_sel = (u8) ((dct_cont_base_reg >> 4) & 0x7);
+ u8 intlv_addr = dct_sel_interleave_addr(pvt);
+ u8 node_id = dram_dst_node(pvt, range);
+ u8 intlv_en = dram_intlv_en(pvt, range);
+
+ edac_dbg(1, "(range %d) SystemAddr= 0x%llx Limit=0x%llx\n",
+ range, sys_addr, get_dram_limit(pvt, range));
+
+ if (!(get_dram_base(pvt, range) <= sys_addr) &&
+ !(get_dram_limit(pvt, range) >= sys_addr))
+ return -EINVAL;
+
+ if (dhar_valid(pvt) &&
+ dhar_base(pvt) <= sys_addr &&
+ sys_addr < BIT_64(32)) {
+ amd64_warn("Huh? Address is in the MMIO hole: 0x%016llx\n",
+ sys_addr);
+ return -EINVAL;
+ }
+
+ /* Verify sys_addr is within DCT Range. */
+ dct_base = (dct_sel_baseaddr(pvt) << 27);
+ dct_limit = (((dct_cont_limit_reg >> 11) & 0x1FFF) << 27) | 0x7FFFFFF;
+ if (!(dct_cont_base_reg & BIT(0)) &&
+ !(dct_base <= sys_addr && dct_limit >= sys_addr)) {
+ return -EINVAL;
+ }
+
+ /* Verify number of dct's that participate in channel interleaving. */
+ num_dcts_intlv = (int) hweight8(intlv_en);
+
+ if (!(num_dcts_intlv % 2 == 0) || (num_dcts_intlv > 4))
+ return -EINVAL;
+
+ channel = f15_m30h_determine_channel(pvt, sys_addr, intlv_en,
+ num_dcts_intlv, dct_sel);
+
+ /* Verify if we stay within the MAX number of channels allowed */
+ if (channel > 4 || channel < 0)
+ return -EINVAL;
+
+ leg_mmio_hole = (u8) (dct_cont_base_reg >> 1 & BIT(0));
+
+ /* Get normalized DCT addr */
+ if (leg_mmio_hole && (sys_addr >= BIT_64(32)))
+ chan_offset = dhar_offset;
+ else
+ chan_offset = dct_base;
+
+ chan_addr = sys_addr - chan_offset;
+
+ /* remove channel interleave */
+ if (num_dcts_intlv == 2) {
+ if (intlv_addr == 0x4)
+ chan_addr = ((chan_addr >> 9) << 8) |
+ (chan_addr & 0xff);
+ else if (intlv_addr == 0x5)
+ chan_addr = ((chan_addr >> 10) << 9) |
+ (chan_addr & 0x1ff);
+ else
+ return -EINVAL;
+
+ } else if (num_dcts_intlv == 4) {
+ if (intlv_addr == 0x4)
+ chan_addr = ((chan_addr >> 10) << 8) |
+ (chan_addr & 0xff);
+ else if (intlv_addr == 0x5)
+ chan_addr = ((chan_addr >> 11) << 9) |
+ (chan_addr & 0x1ff);
+ else
+ return -EINVAL;
+ }
+
+ if (dct_offset_en) {
+ amd64_read_pci_cfg(pvt->F1,
+ DRAM_CONT_HIGH_OFF + (int) channel * 4,
+ &tmp);
+ chan_addr += ((tmp >> 11) & 0xfff) << 27;
+ }
+
+ /* Set DCTCFGSEL = ChannelSelect */
+ f15h_select_dct(pvt, channel);
+
+ edac_dbg(1, " Normalized DCT addr: 0x%llx\n", chan_addr);
+ /* Find the Chip select.. */
+ /*
+ * NOTE: if channel = 3, then alias it to 1.
+ * This is because, in F15 M30h, there is support for
+ * 4 DCT's, but only 2 are currently included in the architecture.
+ * They are DCT0 and DCT3. But we have read all registers of DCT3
+ * into pvt->csels[1]. So we need to use '1' here to get correct
+ * info. Refer F15 M30h BKDG Section 2.10 and 2.10.3 for clarifications
+ */
+
+ alias_channel = (channel == 3) ? 1 : channel;
+
+ cs_found = f1x_lookup_addr_in_dct(chan_addr, node_id, alias_channel);
+
+ if (cs_found >= 0)
+ *chan_sel = alias_channel;
+
+ return cs_found;
+}
+
+static int
+f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt,
+ u64 sys_addr,
+ int *chan_sel)
{
int cs_found = -EINVAL;
unsigned range;
for (range = 0; range < DRAM_RANGES; range++) {
-
if (!dram_rw(pvt, range))
continue;
-
- if ((get_dram_base(pvt, range) <= sys_addr) &&
- (get_dram_limit(pvt, range) >= sys_addr)) {
-
+ if (F15H_M30H_CPU)
+ cs_found = f15_m30h_match_to_this_node(pvt, range,
+ sys_addr,
+ chan_sel);
+ else if ((get_dram_base(pvt, range) <= sys_addr) &&
+ (get_dram_limit(pvt, range) >= sys_addr)) {
cs_found = f1x_match_to_this_node(pvt, range,
sys_addr, chan_sel);
if (cs_found >= 0)
@@ -1624,6 +1824,17 @@ static struct amd64_family_type amd64_family_types[] = {
.read_dct_pci_cfg = f15_read_dct_pci_cfg,
}
},
+ [F15_M30H_CPUS] = {
+ .ctl_name = "F15h_M30h",
+ .f1_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1,
+ .f3_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F3,
+ .ops = {
+ .early_channel_count = f1x_early_channel_count,
+ .map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow,
+ .dbam_to_cs = f16_dbam_to_chip_select,
+ .read_dct_pci_cfg = f15_read_dct_pci_cfg,
+ }
+ },
[F16_CPUS] = {
.ctl_name = "F16h",
.f1_id = PCI_DEVICE_ID_AMD_16H_NB_F1,
@@ -2388,27 +2599,44 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci,
static struct amd64_family_type *amd64_per_family_init(struct amd64_pvt *pvt)
{
u8 fam = boot_cpu_data.x86;
+ u8 model = boot_cpu_data.x86_model;
struct amd64_family_type *fam_type = NULL;
switch (fam) {
case 0xf:
fam_type = &amd64_family_types[K8_CPUS];
pvt->ops = &amd64_family_types[K8_CPUS].ops;
+ pvt->family = fam;
+ pvt->model = model;
break;
case 0x10:
fam_type = &amd64_family_types[F10_CPUS];
pvt->ops = &amd64_family_types[F10_CPUS].ops;
+ pvt->family = fam;
+ pvt->model = model;
break;
case 0x15:
+ if (model == 0x30) {
+ fam_type = &amd64_family_types[F15_M30H_CPUS];
+ pvt->ops = &amd64_family_types[F15_M30H_CPUS].ops;
+ pvt->family = fam;
+ pvt->model = model;
+ break;
+ }
+
fam_type = &amd64_family_types[F15_CPUS];
pvt->ops = &amd64_family_types[F15_CPUS].ops;
+ pvt->family = fam;
+ pvt->model = model;
break;
case 0x16:
fam_type = &amd64_family_types[F16_CPUS];
pvt->ops = &amd64_family_types[F16_CPUS].ops;
+ pvt->family = fam;
+ pvt->model = model;
break;
default:
@@ -2638,6 +2866,14 @@ static DEFINE_PCI_DEVICE_TABLE(amd64_pci_table) = {
},
{
.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,
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 2c6f113..ff15f14 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -170,6 +170,8 @@
/*
* 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_16H_NB_F1 0x1531
@@ -181,13 +183,24 @@
#define DRAM_BASE_LO 0x40
#define DRAM_LIMIT_LO 0x44
-#define dram_intlv_en(pvt, i) ((u8)((pvt->ranges[i].base.lo >> 8) & 0x7))
+/*
+ * F15 M30h DRAM Controller Base/Limit
+ * D18F1x2[1C:00]
+ */
+#define DRAM_CONT_BASE 0x200
+#define DRAM_CONT_LIMIT 0x204
+
+/*
+ * F15 M30h DRAM Controller High Addre Offset
+ * D18F1x2[4C:40]
+ */
+#define DRAM_CONT_HIGH_OFF 0x240
+
#define dram_rw(pvt, i) ((u8)(pvt->ranges[i].base.lo & 0x3))
#define dram_intlv_sel(pvt, i) ((u8)((pvt->ranges[i].lim.lo >> 8) & 0x7))
#define dram_dst_node(pvt, i) ((u8)(pvt->ranges[i].lim.lo & 0x7))
#define DHAR 0xf0
-#define dhar_valid(pvt) ((pvt)->dhar & BIT(0))
#define dhar_mem_hoist_valid(pvt) ((pvt)->dhar & BIT(1))
#define dhar_base(pvt) ((pvt)->dhar & 0xff000000)
#define k8_dhar_offset(pvt) (((pvt)->dhar & 0x0000ff00) << 16)
@@ -234,8 +247,6 @@
#define DDR3_MODE BIT(8)
#define DCT_SEL_LO 0x110
-#define dct_sel_baseaddr(pvt) ((pvt)->dct_sel_lo & 0xFFFFF800)
-#define dct_sel_interleave_addr(pvt) (((pvt)->dct_sel_lo >> 6) & 0x3)
#define dct_high_range_enabled(pvt) ((pvt)->dct_sel_lo & BIT(0))
#define dct_interleave_enabled(pvt) ((pvt)->dct_sel_lo & BIT(2))
@@ -293,10 +304,14 @@
/* MSRs */
#define MSR_MCGCTL_NBE BIT(4)
+/* Helper Macro for F15h M30h */
+#define F15H_M30H_CPU ((pvt->family == 0x15) && \
+ (pvt->model == 0x30))
enum amd_families {
K8_CPUS = 0,
F10_CPUS,
F15_CPUS,
+ F15_M30H_CPUS,
F16_CPUS,
NUM_FAMILIES,
};
@@ -337,6 +352,8 @@ struct amd64_pvt {
struct pci_dev *F1, *F2, *F3;
u16 mc_node_id; /* MC index of this MC node */
+ u8 family; /* CPU family */
+ u8 model; /* CPU model */
int ext_model; /* extended model value of this node */
int channel_count;
@@ -414,6 +431,14 @@ static inline u16 extract_syndrome(u64 status)
return ((status >> 47) & 0xff) | ((status >> 16) & 0xff00);
}
+static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt)
+{
+ if (F15H_M30H_CPU)
+ return (((pvt->dct_sel_hi >> 9) & 0x1) << 2) |
+ ((pvt->dct_sel_lo >> 6) & 0x3);
+
+ return ((pvt)->dct_sel_lo >> 6) & 0x3;
+}
/*
* per-node ECC settings descriptor
*/
@@ -504,3 +529,33 @@ static inline void enable_caches(void *dummy)
{
write_cr0(read_cr0() & ~X86_CR0_CD);
}
+
+static inline u8 dram_intlv_en(struct amd64_pvt *pvt, unsigned int i)
+{
+ u32 tmp;
+ if (F15H_M30H_CPU) {
+ amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &tmp);
+ return (u8) tmp & 0xF;
+ }
+ return (u8) (pvt->ranges[i].base.lo >> 8) & 0x7;
+}
+
+static inline u8 dhar_valid(struct amd64_pvt *pvt)
+{
+ u32 tmp;
+ if (F15H_M30H_CPU) {
+ amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
+ return (tmp >> 1) & BIT(0);
+ }
+ return (pvt)->dhar & BIT(0);
+}
+
+static inline u32 dct_sel_baseaddr(struct amd64_pvt *pvt)
+{
+ u32 tmp;
+ if (F15H_M30H_CPU) {
+ amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
+ return (tmp >> 11) & 0x1FFF;
+ }
+ return (pvt)->dct_sel_lo & 0xFFFFF800;
+}
--
1.7.10.4
Adding PCI_DEVICE_ID_AMD_15H_NB_M30H_F3 and PCI_DEVICE_ID_AMD_15H_NB_M30H_F4
for F15h model 30h. This is required for the file amd_nb.c
Signed-off-by: Aravind Gopalakrishnan <[email protected]>
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 3bed2e8..d1fe5d0 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -518,6 +518,8 @@
#define PCI_DEVICE_ID_AMD_11H_NB_MISC 0x1303
#define PCI_DEVICE_ID_AMD_11H_NB_LINK 0x1304
#define PCI_DEVICE_ID_AMD_15H_M10H_F3 0x1403
+#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F3 0x141d
+#define PCI_DEVICE_ID_AMD_15H_M30H_NB_F4 0x141e
#define PCI_DEVICE_ID_AMD_15H_NB_F0 0x1600
#define PCI_DEVICE_ID_AMD_15H_NB_F1 0x1601
#define PCI_DEVICE_ID_AMD_15H_NB_F2 0x1602
--
1.7.10.4
On Fri, Aug 02, 2013 at 05:43:02PM -0500, Aravind Gopalakrishnan wrote:
> Adding PCI_DEVICE_ID_AMD_15H_NB_M30H_F3 and PCI_DEVICE_ID_AMD_15H_NB_M30H_F4
> for F15h model 30h. This is required for the file amd_nb.c
... and amd64_edac.c
Also, remember to read the comment at the beginning of
include/linux/pci_ids.h!
I fixed up the commit message when applying.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Fri, Aug 02, 2013 at 05:43:03PM -0500, Aravind Gopalakrishnan wrote:
> Adding code to check for specific model (F15h, M30h) and if yes,
> do not add flag AMD_NB_GART. Also check cpuid_edx(0x80000006) for
> prescence of L3. If no L3, do not add any L3 flags.
>
> Signed-off-by: Aravind Gopalakrishnan <[email protected]>
>
> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> index 3048ded..3ee7a4d 100644
> --- a/arch/x86/kernel/amd_nb.c
> +++ b/arch/x86/kernel/amd_nb.c
> @@ -20,6 +20,7 @@ const struct pci_device_id amd_nb_misc_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F3) },
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M10H_F3) },
> + { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F3) },
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F3) },
> {}
> };
> @@ -27,6 +28,7 @@ EXPORT_SYMBOL(amd_nb_misc_ids);
>
> static const struct pci_device_id amd_nb_link_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
> + { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
> {}
> };
> @@ -81,13 +83,21 @@ int amd_cache_northbridges(void)
> next_northbridge(misc, amd_nb_misc_ids);
> node_to_amd_nb(i)->link = link =
> next_northbridge(link, amd_nb_link_ids);
> - }
> + }
>
> + /* GART present only on Fam15h upto model 0fh */
> if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
> - boot_cpu_data.x86 == 0x15)
> + (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
> amd_northbridges.flags |= AMD_NB_GART;
>
> /*
> + * Check CPUID Fn8000_0006_EDX: L3 Cache Identifiers.
> + * If == 0, then no need to proceed as there is no L3.
This comment explains the code. Just sit down and think about it: does
it make any sense to have that in a comment?
> + */
> + if (cpuid_edx(0x80000006) == 0)
> + return 0;
> +
> + /*
> * Some CPU families support L3 Cache Index Disable. There are some
> * limitations because of E382 and E388 on family 0x10.
> */
> --
I fixed it up like this:
commit a0cb1bab68823648def0fda19bf307ad08eb25d2
Author: Aravind Gopalakrishnan <[email protected]>
Date: Fri Aug 2 17:43:03 2013 -0500
x86, amd_nb: Clarify F15h, model 30h GART and L3 support
F15h, models 0x30 and later don't have a GART. Note that. Also check
cpuid_edx(0x80000006) for prescence of L3 because there are models which
don't sport an L3.
Signed-off-by: Aravind Gopalakrishnan <[email protected]>
[ Boris: rewrite commit message, cleanup comments. ]
Signed-off-by: Borislav Petkov <[email protected]>
diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
index 3048ded1b598..59554dca96ec 100644
--- a/arch/x86/kernel/amd_nb.c
+++ b/arch/x86/kernel/amd_nb.c
@@ -20,6 +20,7 @@ const struct pci_device_id amd_nb_misc_ids[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F3) },
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M10H_F3) },
+ { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F3) },
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F3) },
{}
};
@@ -27,6 +28,7 @@ EXPORT_SYMBOL(amd_nb_misc_ids);
static const struct pci_device_id amd_nb_link_ids[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
+ { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
{}
};
@@ -81,13 +83,20 @@ int amd_cache_northbridges(void)
next_northbridge(misc, amd_nb_misc_ids);
node_to_amd_nb(i)->link = link =
next_northbridge(link, amd_nb_link_ids);
- }
+ }
+ /* GART present only on Fam15h upto model 0fh */
if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
- boot_cpu_data.x86 == 0x15)
+ (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
amd_northbridges.flags |= AMD_NB_GART;
/*
+ * Check for L3 cache presence.
+ */
+ if (!cpuid_edx(0x80000006))
+ return 0;
+
+ /*
* Some CPU families support L3 Cache Index Disable. There are some
* limitations because of E382 and E388 on family 0x10.
*/
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Fri, Aug 02, 2013 at 05:43:04PM -0500, Aravind Gopalakrishnan wrote:
> Adding support for handling ECC error decoding for new F15 models.
> On newer models, support has been included for upto 4 DCT's,
> however, only DCT0 and DCT3 are currently configured. (Refer BKDG Section 2.10)
> There is also a new "Routing DRAM Requests" algorithm for this model.
>
> Tested on Fam15h M30h with ECC turned on using mce_amd_inj facility and
> verified to be functionally correct.
>
> Signed-off-by: Aravind Gopalakrishnan <[email protected]>
>
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 8b6a034..42dab12 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -123,7 +123,11 @@ static void f15h_select_dct(struct amd64_pvt *pvt, u8 dct)
> u32 reg = 0;
>
> amd64_read_pci_cfg(pvt->F1, DCT_CFG_SEL, ®);
> - reg &= 0xfffffffe;
> + /*
> + * If Fam15h M30h, mask out last two bits for programming dct.
> + * Else, only mask out the last bit.
> + */
No need for that comment.
> + reg &= (F15_M30H_CPUS) ? 0xfffffffc : 0xfffffff8;
Let's see, I had 0xfffffffe and now you have 0xfffffff8. See the
difference?
Also, F15H_M30H_CPUS is an enum, AFAICT, and it will always be true as
long as it is != 0. What you meant here is "F15H_M30H_CPU". So you see
how those defines are misleading and made you use them wrong.
So let's correct it all and make it more clear:
reg &= (pvt->model >= 0x30) ? ~3 : ~1;
we don't need to look at the family because this function -
f15h_select_dct - is called on F15h only anyway.
> reg |= dct;
> amd64_write_pci_cfg(pvt->F1, DCT_CFG_SEL, reg);
> }
> @@ -133,8 +137,12 @@ static int f15_read_dct_pci_cfg(struct amd64_pvt *pvt, int addr, u32 *val,
> {
> u8 dct = 0;
>
> + /*
> + * For F15 M30h, the second dct is DCT 3.
> + * Refer BKDG Section 2.10
> + */
> if (addr >= 0x140 && addr <= 0x1a0) {
> - dct = 1;
> + dct = F15H_M30H_CPU ? 3 : 1;
ditto here: use pvt->model like above.
> addr -= 0x100;
> }
>
> @@ -205,8 +213,9 @@ static int amd64_set_scrub_rate(struct mem_ctl_info *mci, u32 bw)
> if (boot_cpu_data.x86 == 0xf)
> min_scrubrate = 0x0;
>
> - /* F15h Erratum #505 */
> - if (boot_cpu_data.x86 == 0x15)
> + /* F15h Models 0x00 - 0x0f Erratum #505 */
Hmm, are you sure this erratum is fixed in SR? Talk to Eric about it
first, please. RG says "No fix planned".
> + if (boot_cpu_data.x86 == 0x15 &&
> + boot_cpu_data.x86_model != 0x30)
> f15h_select_dct(pvt, 0);
>
> return __amd64_set_scrub_rate(pvt->F3, bw, min_scrubrate);
> @@ -218,8 +227,9 @@ static int amd64_get_scrub_rate(struct mem_ctl_info *mci)
> u32 scrubval = 0;
> int i, retval = -EINVAL;
>
> - /* F15h Erratum #505 */
> - if (boot_cpu_data.x86 == 0x15)
> + /* F15h Models 0x00 - 0x0f Erratum #505 */
ditto.
> + if (boot_cpu_data.x86 == 0x15 &&
> + boot_cpu_data.x86_model != 0x30)
> f15h_select_dct(pvt, 0);
>
> amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
> @@ -343,10 +353,10 @@ static void get_cs_base_and_mask(struct amd64_pvt *pvt, int csrow, u8 dct,
> addr_shift = 4;
>
> /*
> - * F16h needs two addr_shift values: 8 for high and 6 for low
> - * (cf. F16h BKDG).
> + * F16h and F15h(model 30h) needs two addr_shift values:
> + * 8 for high and 6 for low (cf. F16h BKDG).
> */
> - } else if (boot_cpu_data.x86 == 0x16) {
> + } else if (boot_cpu_data.x86 == 0x16 || F15H_M30H_CPU) {
pvt->family and ->model please. This macro is ugly and confusing.
> csbase = pvt->csels[dct].csbases[csrow];
> csmask = pvt->csels[dct].csmasks[csrow >> 1];
>
> @@ -743,6 +753,9 @@ static void prep_chip_selects(struct amd64_pvt *pvt)
> if (boot_cpu_data.x86 == 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 (F15H_M30H_CPU) {
> + pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
> + pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
> } else {
> pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
> pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
> @@ -850,9 +863,9 @@ static u64 get_error_address(struct mce *m)
> addr = m->addr & GENMASK(start_bit, end_bit);
>
> /*
> - * Erratum 637 workaround
> + * Erratum 637 workaround for F15h Models 0x00 - 0x0f
Same here - RG says this erratum is, like 505, "No fix planned"
> */
> - if (c->x86 == 0x15) {
> + if (c->x86 == 0x15 && c->x86_model != 0x30) {
> struct amd64_pvt *pvt;
> u64 cc6_base, tmp_addr;
> u32 tmp;
> @@ -918,6 +931,7 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
> struct amd_northbridge *nb;
> struct pci_dev *misc, *f1 = NULL;
> struct cpuinfo_x86 *c = &boot_cpu_data;
> + unsigned int pci_func;
> int off = range << 3;
> u32 llim;
>
> @@ -942,7 +956,12 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
> return;
>
> misc = nb->misc;
> - f1 = pci_get_related_function(misc->vendor, PCI_DEVICE_ID_AMD_15H_NB_F1, misc);
> +
> + pci_func = (c->x86_model == 0x30) ? PCI_DEVICE_ID_AMD_15H_M30H_NB_F1
> + : PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
Something's wrong with this line :-)
> +
> + f1 = pci_get_related_function(misc->vendor, pci_func, misc);
> +
> if (WARN_ON(!f1))
> return;
>
> @@ -1173,7 +1192,8 @@ static int f15_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
> }
>
> /*
> - * F16h has only limited cs_modes
> + * F16h has only limited cs_modes.
> + * F15 M30h follows the same pattern.
> */
> static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
> unsigned cs_mode)
> @@ -1218,6 +1238,51 @@ static void read_dram_ctl_register(struct amd64_pvt *pvt)
> }
>
> /*
> + * Determine channel (DCT) based on the interleaving mode:
> + * F15h M30h BKDG, 2.10.12 Memory Interleaving Modes.
> + */
> +static u8 f15_m30h_determine_channel(struct amd64_pvt *pvt, u64 sys_addr,
> + u8 intlv_en, int num_dcts_intlv, u32 dct_sel)
Arg indentation.
> +{
> + u8 channel = 0;
> + u8 select;
> + u8 intlv_addr = dct_sel_interleave_addr(pvt);
> +
> + if (!(intlv_en))
> + return (u8)(dct_sel);
> + /*
> + * If num_dcts_intlv == 2, them consider addr bit 8 or addr bit 9
> + * depending on intlv_addr, then return either channel = 0/1/2/3.
> + * If num_dcts_intlv == 4, consider either bits[8:9] or bits[9:10]
> + * depending on intlv_addr, then return the value obtained.
No need for that comment.
> + */
> +
> + if (num_dcts_intlv == 2) {
> + if (intlv_addr == 0x4)
> + select = (sys_addr >> 8) & BIT(0);
> + else if (intlv_addr == 0x5)
> + select = (sys_addr >> 9) & BIT(0);
So, basically you can take care of the two cases together by doing:
select = (sys_addr >> 8) & 0x3;
?
> + else
> + return -EINVAL;
> +
> + /* If select !=0, upper DCT selected, else lower DCT */
I think we can see that, no need for the comment.
> + channel = select ? 0x3 : 0;
> +
> + } else if (num_dcts_intlv == 4) {
> + if (intlv_addr == 0x4)
> + select = (sys_addr >> 8) & 0x3;
> + else if (intlv_addr == 0x5)
> + select = (sys_addr >> 9) & 0x3;
Ditto here, as above.
> + else
> + return -EINVAL;
> +
> + channel = select;
> + }
> +
> + return channel;
> +}
> +
> +/*
> * Determine channel (DCT) based on the interleaving mode: F10h BKDG, 2.8.9 Memory
> * Interleaving Modes.
> */
> @@ -1366,6 +1431,10 @@ static int f1x_lookup_addr_in_dct(u64 in_addr, u8 nid, u8 dct)
> (in_addr & cs_mask), (cs_base & cs_mask));
>
> if ((in_addr & cs_mask) == (cs_base & cs_mask)) {
> + if (F15H_M30H_CPU) {
> + cs_found = csrow;
> + break;
> + }
> cs_found = f10_process_possible_spare(pvt, dct, csrow);
>
> edac_dbg(1, " MATCH csrow=%d\n", cs_found);
> @@ -1492,20 +1561,151 @@ static int f1x_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
> return cs_found;
> }
>
> -static int f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt, u64 sys_addr,
> - int *chan_sel)
> +/*
> + * Routing DRAM Requests algorithm is different for F15h M30h.
> + * It is cleaner to use a brand new function rather than
> + * adding quirks in the more generic 'f1x_match_to_this_node'.
> + * Refer "2.10.5 DRAM Routing Requests" in BKDG for info.
> + */
This comment belongs in the commit message of this patch.
> +static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
> + u64 sys_addr, int *chan_sel)
Arg indentation
> +{
> + int cs_found = -EINVAL;
> + int num_dcts_intlv = 0;
> + u64 chan_addr, chan_offset, high_addr_offset;
> + u64 dct_base, dct_limit;
> + u32 dct_cont_base_reg, dct_cont_limit_reg, tmp;
> + u8 channel, alias_channel, leg_mmio_hole;
> +
> +
> + /* Read DRAM Control registers specific to F15 M30h. */
No need for the comment.
> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &dct_cont_base_reg);
> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &dct_cont_limit_reg);
> +
> + /* Extract F15h M30h specific config */
No need for the comment.
> + u64 dhar_offset = f10_dhar_offset(pvt);
> + u8 dct_offset_en = (u8) ((dct_cont_base_reg >> 3) & BIT(0));
> + u8 dct_sel = (u8) ((dct_cont_base_reg >> 4) & 0x7);
> + u8 intlv_addr = dct_sel_interleave_addr(pvt);
> + u8 node_id = dram_dst_node(pvt, range);
> + u8 intlv_en = dram_intlv_en(pvt, range);
> +
> + edac_dbg(1, "(range %d) SystemAddr= 0x%llx Limit=0x%llx\n",
> + range, sys_addr, get_dram_limit(pvt, range));
> +
> + if (!(get_dram_base(pvt, range) <= sys_addr) &&
> + !(get_dram_limit(pvt, range) >= sys_addr))
> + return -EINVAL;
> +
> + if (dhar_valid(pvt) &&
> + dhar_base(pvt) <= sys_addr &&
> + sys_addr < BIT_64(32)) {
> + amd64_warn("Huh? Address is in the MMIO hole: 0x%016llx\n",
> + sys_addr);
> + return -EINVAL;
> + }
> +
> + /* Verify sys_addr is within DCT Range. */
> + dct_base = (dct_sel_baseaddr(pvt) << 27);
> + dct_limit = (((dct_cont_limit_reg >> 11) & 0x1FFF) << 27) | 0x7FFFFFF;
> + if (!(dct_cont_base_reg & BIT(0)) &&
> + !(dct_base <= sys_addr && dct_limit >= sys_addr)) {
> + return -EINVAL;
> + }
> +
> + /* Verify number of dct's that participate in channel interleaving. */
> + num_dcts_intlv = (int) hweight8(intlv_en);
> +
> + if (!(num_dcts_intlv % 2 == 0) || (num_dcts_intlv > 4))
> + return -EINVAL;
> +
> + channel = f15_m30h_determine_channel(pvt, sys_addr, intlv_en,
> + num_dcts_intlv, dct_sel);
> +
> + /* Verify if we stay within the MAX number of channels allowed */
s/if//
> + if (channel > 4 || channel < 0)
> + return -EINVAL;
> +
> + leg_mmio_hole = (u8) (dct_cont_base_reg >> 1 & BIT(0));
> +
> + /* Get normalized DCT addr */
> + if (leg_mmio_hole && (sys_addr >= BIT_64(32)))
> + chan_offset = dhar_offset;
> + else
> + chan_offset = dct_base;
> +
> + chan_addr = sys_addr - chan_offset;
> +
> + /* remove channel interleave */
> + if (num_dcts_intlv == 2) {
> + if (intlv_addr == 0x4)
> + chan_addr = ((chan_addr >> 9) << 8) |
> + (chan_addr & 0xff);
> + else if (intlv_addr == 0x5)
> + chan_addr = ((chan_addr >> 10) << 9) |
> + (chan_addr & 0x1ff);
> + else
> + return -EINVAL;
> +
> + } else if (num_dcts_intlv == 4) {
> + if (intlv_addr == 0x4)
> + chan_addr = ((chan_addr >> 10) << 8) |
> + (chan_addr & 0xff);
> + else if (intlv_addr == 0x5)
> + chan_addr = ((chan_addr >> 11) << 9) |
> + (chan_addr & 0x1ff);
> + else
> + return -EINVAL;
> + }
> +
> + if (dct_offset_en) {
> + amd64_read_pci_cfg(pvt->F1,
> + DRAM_CONT_HIGH_OFF + (int) channel * 4,
> + &tmp);
> + chan_addr += ((tmp >> 11) & 0xfff) << 27;
> + }
> +
> + /* Set DCTCFGSEL = ChannelSelect */
No need for the comment.
> + f15h_select_dct(pvt, channel);
> +
> + edac_dbg(1, " Normalized DCT addr: 0x%llx\n", chan_addr);
> + /* Find the Chip select.. */
> + /*
> + * NOTE: if channel = 3, then alias it to 1.
> + * This is because, in F15 M30h, there is support for
> + * 4 DCT's, but only 2 are currently included in the architecture.
> + * They are DCT0 and DCT3. But we have read all registers of DCT3
> + * into pvt->csels[1]. So we need to use '1' here to get correct
> + * info. Refer F15 M30h BKDG Section 2.10 and 2.10.3 for clarifications
> + */
Comment needs block formatting.
> +
> + alias_channel = (channel == 3) ? 1 : channel;
> +
> + cs_found = f1x_lookup_addr_in_dct(chan_addr, node_id, alias_channel);
> +
> + if (cs_found >= 0)
> + *chan_sel = alias_channel;
> +
> + return cs_found;
> +}
> +
> +static int
> +f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt,
> + u64 sys_addr,
> + int *chan_sel)
> {
Arg indentation.
> int cs_found = -EINVAL;
> unsigned range;
>
> for (range = 0; range < DRAM_RANGES; range++) {
> -
> if (!dram_rw(pvt, range))
> continue;
> -
> - if ((get_dram_base(pvt, range) <= sys_addr) &&
> - (get_dram_limit(pvt, range) >= sys_addr)) {
> -
> + if (F15H_M30H_CPU)
> + cs_found = f15_m30h_match_to_this_node(pvt, range,
> + sys_addr,
> + chan_sel);
> + else if ((get_dram_base(pvt, range) <= sys_addr) &&
> + (get_dram_limit(pvt, range) >= sys_addr)) {
> cs_found = f1x_match_to_this_node(pvt, range,
> sys_addr, chan_sel);
> if (cs_found >= 0)
> @@ -1624,6 +1824,17 @@ static struct amd64_family_type amd64_family_types[] = {
> .read_dct_pci_cfg = f15_read_dct_pci_cfg,
> }
> },
> + [F15_M30H_CPUS] = {
> + .ctl_name = "F15h_M30h",
> + .f1_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1,
> + .f3_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F3,
> + .ops = {
> + .early_channel_count = f1x_early_channel_count,
> + .map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow,
> + .dbam_to_cs = f16_dbam_to_chip_select,
> + .read_dct_pci_cfg = f15_read_dct_pci_cfg,
> + }
> + },
> [F16_CPUS] = {
> .ctl_name = "F16h",
> .f1_id = PCI_DEVICE_ID_AMD_16H_NB_F1,
> @@ -2388,27 +2599,44 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci,
> static struct amd64_family_type *amd64_per_family_init(struct amd64_pvt *pvt)
> {
> u8 fam = boot_cpu_data.x86;
> + u8 model = boot_cpu_data.x86_model;
> struct amd64_family_type *fam_type = NULL;
>
> switch (fam) {
> case 0xf:
> fam_type = &amd64_family_types[K8_CPUS];
> pvt->ops = &amd64_family_types[K8_CPUS].ops;
> + pvt->family = fam;
You could call it pvt->fam for less typing if you like.
> + pvt->model = model;
> break;
>
> case 0x10:
> fam_type = &amd64_family_types[F10_CPUS];
> pvt->ops = &amd64_family_types[F10_CPUS].ops;
> + pvt->family = fam;
> + pvt->model = model;
> break;
>
> case 0x15:
> + if (model == 0x30) {
> + fam_type = &amd64_family_types[F15_M30H_CPUS];
> + pvt->ops = &amd64_family_types[F15_M30H_CPUS].ops;
> + pvt->family = fam;
> + pvt->model = model;
> + break;
> + }
> +
> fam_type = &amd64_family_types[F15_CPUS];
> pvt->ops = &amd64_family_types[F15_CPUS].ops;
> + pvt->family = fam;
> + pvt->model = model;
> break;
>
> case 0x16:
> fam_type = &amd64_family_types[F16_CPUS];
> pvt->ops = &amd64_family_types[F16_CPUS].ops;
> + pvt->family = fam;
> + pvt->model = model;
This ->family and ->model assignment is repeated in every case. What
do you think, could it probably be done only once, outside the switch
statement?
> break;
>
> default:
> @@ -2638,6 +2866,14 @@ static DEFINE_PCI_DEVICE_TABLE(amd64_pci_table) = {
> },
> {
> .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,
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 2c6f113..ff15f14 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -170,6 +170,8 @@
> /*
> * 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_16H_NB_F1 0x1531
> @@ -181,13 +183,24 @@
> #define DRAM_BASE_LO 0x40
> #define DRAM_LIMIT_LO 0x44
>
> -#define dram_intlv_en(pvt, i) ((u8)((pvt->ranges[i].base.lo >> 8) & 0x7))
> +/*
> + * F15 M30h DRAM Controller Base/Limit
> + * D18F1x2[1C:00]
> + */
> +#define DRAM_CONT_BASE 0x200
> +#define DRAM_CONT_LIMIT 0x204
> +
> +/*
> + * F15 M30h DRAM Controller High Addre Offset
> + * D18F1x2[4C:40]
> + */
> +#define DRAM_CONT_HIGH_OFF 0x240
> +
> #define dram_rw(pvt, i) ((u8)(pvt->ranges[i].base.lo & 0x3))
> #define dram_intlv_sel(pvt, i) ((u8)((pvt->ranges[i].lim.lo >> 8) & 0x7))
> #define dram_dst_node(pvt, i) ((u8)(pvt->ranges[i].lim.lo & 0x7))
>
> #define DHAR 0xf0
> -#define dhar_valid(pvt) ((pvt)->dhar & BIT(0))
> #define dhar_mem_hoist_valid(pvt) ((pvt)->dhar & BIT(1))
> #define dhar_base(pvt) ((pvt)->dhar & 0xff000000)
> #define k8_dhar_offset(pvt) (((pvt)->dhar & 0x0000ff00) << 16)
> @@ -234,8 +247,6 @@
> #define DDR3_MODE BIT(8)
>
> #define DCT_SEL_LO 0x110
> -#define dct_sel_baseaddr(pvt) ((pvt)->dct_sel_lo & 0xFFFFF800)
> -#define dct_sel_interleave_addr(pvt) (((pvt)->dct_sel_lo >> 6) & 0x3)
> #define dct_high_range_enabled(pvt) ((pvt)->dct_sel_lo & BIT(0))
> #define dct_interleave_enabled(pvt) ((pvt)->dct_sel_lo & BIT(2))
>
> @@ -293,10 +304,14 @@
> /* MSRs */
> #define MSR_MCGCTL_NBE BIT(4)
>
> +/* Helper Macro for F15h M30h */
> +#define F15H_M30H_CPU ((pvt->family == 0x15) && \
> + (pvt->model == 0x30))
Please drop this macro - the condition is not that complicated to write
out and not error prone.
> enum amd_families {
> K8_CPUS = 0,
> F10_CPUS,
> F15_CPUS,
> + F15_M30H_CPUS,
> F16_CPUS,
> NUM_FAMILIES,
> };
> @@ -337,6 +352,8 @@ struct amd64_pvt {
> struct pci_dev *F1, *F2, *F3;
>
> u16 mc_node_id; /* MC index of this MC node */
> + u8 family; /* CPU family */
> + u8 model; /* CPU model */
> int ext_model; /* extended model value of this node */
> int channel_count;
>
> @@ -414,6 +431,14 @@ static inline u16 extract_syndrome(u64 status)
> return ((status >> 47) & 0xff) | ((status >> 16) & 0xff00);
> }
>
> +static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt)
> +{
> + if (F15H_M30H_CPU)
> + return (((pvt->dct_sel_hi >> 9) & 0x1) << 2) |
> + ((pvt->dct_sel_lo >> 6) & 0x3);
> +
> + return ((pvt)->dct_sel_lo >> 6) & 0x3;
> +}
> /*
> * per-node ECC settings descriptor
> */
> @@ -504,3 +529,33 @@ static inline void enable_caches(void *dummy)
> {
> write_cr0(read_cr0() & ~X86_CR0_CD);
> }
> +
> +static inline u8 dram_intlv_en(struct amd64_pvt *pvt, unsigned int i)
> +{
> + u32 tmp;
> + if (F15H_M30H_CPU) {
u32 tmp inside the if-statement.
> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &tmp);
> + return (u8) tmp & 0xF;
> + }
> + return (u8) (pvt->ranges[i].base.lo >> 8) & 0x7;
> +}
> +
> +static inline u8 dhar_valid(struct amd64_pvt *pvt)
> +{
> + u32 tmp;
> + if (F15H_M30H_CPU) {
ditto.
> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
> + return (tmp >> 1) & BIT(0);
> + }
> + return (pvt)->dhar & BIT(0);
> +}
> +
> +static inline u32 dct_sel_baseaddr(struct amd64_pvt *pvt)
> +{
> + u32 tmp;
> + if (F15H_M30H_CPU) {
ditto.
> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
> + return (tmp >> 11) & 0x1FFF;
> + }
> + return (pvt)->dct_sel_lo & 0xFFFFF800;
> +}
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 8/6/2013 3:23 PM, Borislav Petkov wrote:
> On Fri, Aug 02, 2013 at 05:43:04PM -0500, Aravind Gopalakrishnan wrote:
>> Adding support for handling ECC error decoding for new F15 models.
>> On newer models, support has been included for upto 4 DCT's,
>> however, only DCT0 and DCT3 are currently configured. (Refer BKDG Section 2.10)
>> There is also a new "Routing DRAM Requests" algorithm for this model.
>>
>> Tested on Fam15h M30h with ECC turned on using mce_amd_inj facility and
>> verified to be functionally correct.
>>
>> Signed-off-by: Aravind Gopalakrishnan <[email protected]>
>>
>> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
>> index 8b6a034..42dab12 100644
>> --- a/drivers/edac/amd64_edac.c
>> +++ b/drivers/edac/amd64_edac.c
>> @@ -123,7 +123,11 @@ static void f15h_select_dct(struct amd64_pvt *pvt, u8 dct)
>> u32 reg = 0;
>>
>> amd64_read_pci_cfg(pvt->F1, DCT_CFG_SEL, ®);
>> - reg &= 0xfffffffe;
>> + /*
>> + * If Fam15h M30h, mask out last two bits for programming dct.
>> + * Else, only mask out the last bit.
>> + */
> No need for that comment.
>
>> + reg &= (F15_M30H_CPUS) ? 0xfffffffc : 0xfffffff8;
> Let's see, I had 0xfffffffe and now you have 0xfffffff8. See the
> difference?
>
> Also, F15H_M30H_CPUS is an enum, AFAICT, and it will always be true as
> long as it is != 0. What you meant here is "F15H_M30H_CPU". So you see
> how those defines are misleading and made you use them wrong.
>
> So let's correct it all and make it more clear:
>
> reg &= (pvt->model >= 0x30) ? ~3 : ~1;
>
> we don't need to look at the family because this function -
> f15h_select_dct - is called on F15h only anyway.
Oops.. My bad. Will fix this.
>> reg |= dct;
>> amd64_write_pci_cfg(pvt->F1, DCT_CFG_SEL, reg);
>> }
>> @@ -133,8 +137,12 @@ static int f15_read_dct_pci_cfg(struct amd64_pvt *pvt, int addr, u32 *val,
>> {
>> u8 dct = 0;
>>
>> + /*
>> + * For F15 M30h, the second dct is DCT 3.
>> + * Refer BKDG Section 2.10
>> + */
>> if (addr >= 0x140 && addr <= 0x1a0) {
>> - dct = 1;
>> + dct = F15H_M30H_CPU ? 3 : 1;
> ditto here: use pvt->model like above.
Okay.
>> addr -= 0x100;
>> }
>>
>> @@ -205,8 +213,9 @@ static int amd64_set_scrub_rate(struct mem_ctl_info *mci, u32 bw)
>> if (boot_cpu_data.x86 == 0xf)
>> min_scrubrate = 0x0;
>>
>> - /* F15h Erratum #505 */
>> - if (boot_cpu_data.x86 == 0x15)
>> + /* F15h Models 0x00 - 0x0f Erratum #505 */
> Hmm, are you sure this erratum is fixed in SR? Talk to Eric about it
> first, please. RG says "No fix planned".
>
>> + if (boot_cpu_data.x86 == 0x15 &&
>> + boot_cpu_data.x86_model != 0x30)
>> f15h_select_dct(pvt, 0);
>>
>> return __amd64_set_scrub_rate(pvt->F3, bw, min_scrubrate);
>> @@ -218,8 +227,9 @@ static int amd64_get_scrub_rate(struct mem_ctl_info *mci)
>> u32 scrubval = 0;
>> int i, retval = -EINVAL;
>>
>> - /* F15h Erratum #505 */
>> - if (boot_cpu_data.x86 == 0x15)
>> + /* F15h Models 0x00 - 0x0f Erratum #505 */
> ditto.
I have pinged some people about it, will let you know..
>> + if (boot_cpu_data.x86 == 0x15 &&
>> + boot_cpu_data.x86_model != 0x30)
>> f15h_select_dct(pvt, 0);
>>
>> amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
>> @@ -343,10 +353,10 @@ static void get_cs_base_and_mask(struct amd64_pvt *pvt, int csrow, u8 dct,
>> addr_shift = 4;
>>
>> /*
>> - * F16h needs two addr_shift values: 8 for high and 6 for low
>> - * (cf. F16h BKDG).
>> + * F16h and F15h(model 30h) needs two addr_shift values:
>> + * 8 for high and 6 for low (cf. F16h BKDG).
>> */
>> - } else if (boot_cpu_data.x86 == 0x16) {
>> + } else if (boot_cpu_data.x86 == 0x16 || F15H_M30H_CPU) {
> pvt->family and ->model please. This macro is ugly and confusing.
>
>> csbase = pvt->csels[dct].csbases[csrow];
>> csmask = pvt->csels[dct].csmasks[csrow >> 1];
>>
>> @@ -743,6 +753,9 @@ static void prep_chip_selects(struct amd64_pvt *pvt)
>> if (boot_cpu_data.x86 == 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 (F15H_M30H_CPU) {
>> + pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
>> + pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
>> } else {
>> pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
>> pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
>> @@ -850,9 +863,9 @@ static u64 get_error_address(struct mce *m)
>> addr = m->addr & GENMASK(start_bit, end_bit);
>>
>> /*
>> - * Erratum 637 workaround
>> + * Erratum 637 workaround for F15h Models 0x00 - 0x0f
> Same here - RG says this erratum is, like 505, "No fix planned"
>
>> */
>> - if (c->x86 == 0x15) {
>> + if (c->x86 == 0x15 && c->x86_model != 0x30) {
>> struct amd64_pvt *pvt;
>> u64 cc6_base, tmp_addr;
>> u32 tmp;
>> @@ -918,6 +931,7 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
>> struct amd_northbridge *nb;
>> struct pci_dev *misc, *f1 = NULL;
>> struct cpuinfo_x86 *c = &boot_cpu_data;
>> + unsigned int pci_func;
>> int off = range << 3;
>> u32 llim;
>>
>> @@ -942,7 +956,12 @@ static void read_dram_base_limit_regs(struct amd64_pvt *pvt, unsigned range)
>> return;
>>
>> misc = nb->misc;
>> - f1 = pci_get_related_function(misc->vendor, PCI_DEVICE_ID_AMD_15H_NB_F1, misc);
>> +
>> + pci_func = (c->x86_model == 0x30) ? PCI_DEVICE_ID_AMD_15H_M30H_NB_F1
>> + : PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
> Something's wrong with this line :-)
Oh no! (vim auto-complete! :( )
Will fix this.
>> +
>> + f1 = pci_get_related_function(misc->vendor, pci_func, misc);
>> +
>> if (WARN_ON(!f1))
>> return;
>>
>> @@ -1173,7 +1192,8 @@ static int f15_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
>> }
>>
>> /*
>> - * F16h has only limited cs_modes
>> + * F16h has only limited cs_modes.
>> + * F15 M30h follows the same pattern.
>> */
>> static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
>> unsigned cs_mode)
>> @@ -1218,6 +1238,51 @@ static void read_dram_ctl_register(struct amd64_pvt *pvt)
>> }
>>
>> /*
>> + * Determine channel (DCT) based on the interleaving mode:
>> + * F15h M30h BKDG, 2.10.12 Memory Interleaving Modes.
>> + */
>> +static u8 f15_m30h_determine_channel(struct amd64_pvt *pvt, u64 sys_addr,
>> + u8 intlv_en, int num_dcts_intlv, u32 dct_sel)
> Arg indentation.
Sorry, will fix.
>> +{
>> + u8 channel = 0;
>> + u8 select;
>> + u8 intlv_addr = dct_sel_interleave_addr(pvt);
>> +
>> + if (!(intlv_en))
>> + return (u8)(dct_sel);
>> + /*
>> + * If num_dcts_intlv == 2, them consider addr bit 8 or addr bit 9
>> + * depending on intlv_addr, then return either channel = 0/1/2/3.
>> + * If num_dcts_intlv == 4, consider either bits[8:9] or bits[9:10]
>> + * depending on intlv_addr, then return the value obtained.
> No need for that comment.
>
>> + */
>> +
>> + if (num_dcts_intlv == 2) {
>> + if (intlv_addr == 0x4)
>> + select = (sys_addr >> 8) & BIT(0);
>> + else if (intlv_addr == 0x5)
>> + select = (sys_addr >> 9) & BIT(0);
> So, basically you can take care of the two cases together by doing:
>
> select = (sys_addr >> 8) & 0x3;
>
> ?
>
>> + else
>> + return -EINVAL;
>> +
>> + /* If select !=0, upper DCT selected, else lower DCT */
> I think we can see that, no need for the comment.
>
>> + channel = select ? 0x3 : 0;
>> +
>> + } else if (num_dcts_intlv == 4) {
>> + if (intlv_addr == 0x4)
>> + select = (sys_addr >> 8) & 0x3;
>> + else if (intlv_addr == 0x5)
>> + select = (sys_addr >> 9) & 0x3;
> Ditto here, as above.
>
>> + else
>> + return -EINVAL;
>> +
>> + channel = select;
>> + }
>> +
>> + return channel;
>> +}
>> +
>> +/*
>> * Determine channel (DCT) based on the interleaving mode: F10h BKDG, 2.8.9 Memory
>> * Interleaving Modes.
>> */
>> @@ -1366,6 +1431,10 @@ static int f1x_lookup_addr_in_dct(u64 in_addr, u8 nid, u8 dct)
>> (in_addr & cs_mask), (cs_base & cs_mask));
>>
>> if ((in_addr & cs_mask) == (cs_base & cs_mask)) {
>> + if (F15H_M30H_CPU) {
>> + cs_found = csrow;
>> + break;
>> + }
>> cs_found = f10_process_possible_spare(pvt, dct, csrow);
>>
>> edac_dbg(1, " MATCH csrow=%d\n", cs_found);
>> @@ -1492,20 +1561,151 @@ static int f1x_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
>> return cs_found;
>> }
>>
>> -static int f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt, u64 sys_addr,
>> - int *chan_sel)
>> +/*
>> + * Routing DRAM Requests algorithm is different for F15h M30h.
>> + * It is cleaner to use a brand new function rather than
>> + * adding quirks in the more generic 'f1x_match_to_this_node'.
>> + * Refer "2.10.5 DRAM Routing Requests" in BKDG for info.
>> + */
> This comment belongs in the commit message of this patch.
Okay.
>> +static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt, unsigned range,
>> + u64 sys_addr, int *chan_sel)
> Arg indentation
Sorry, will fix..
>> +{
>> + int cs_found = -EINVAL;
>> + int num_dcts_intlv = 0;
>> + u64 chan_addr, chan_offset, high_addr_offset;
>> + u64 dct_base, dct_limit;
>> + u32 dct_cont_base_reg, dct_cont_limit_reg, tmp;
>> + u8 channel, alias_channel, leg_mmio_hole;
>> +
>> +
>> + /* Read DRAM Control registers specific to F15 M30h. */
> No need for the comment.
>
>> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &dct_cont_base_reg);
>> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &dct_cont_limit_reg);
>> +
>> + /* Extract F15h M30h specific config */
> No need for the comment.
>
>> + u64 dhar_offset = f10_dhar_offset(pvt);
>> + u8 dct_offset_en = (u8) ((dct_cont_base_reg >> 3) & BIT(0));
>> + u8 dct_sel = (u8) ((dct_cont_base_reg >> 4) & 0x7);
>> + u8 intlv_addr = dct_sel_interleave_addr(pvt);
>> + u8 node_id = dram_dst_node(pvt, range);
>> + u8 intlv_en = dram_intlv_en(pvt, range);
>> +
>> + edac_dbg(1, "(range %d) SystemAddr= 0x%llx Limit=0x%llx\n",
>> + range, sys_addr, get_dram_limit(pvt, range));
>> +
>> + if (!(get_dram_base(pvt, range) <= sys_addr) &&
>> + !(get_dram_limit(pvt, range) >= sys_addr))
>> + return -EINVAL;
>> +
>> + if (dhar_valid(pvt) &&
>> + dhar_base(pvt) <= sys_addr &&
>> + sys_addr < BIT_64(32)) {
>> + amd64_warn("Huh? Address is in the MMIO hole: 0x%016llx\n",
>> + sys_addr);
>> + return -EINVAL;
>> + }
>> +
>> + /* Verify sys_addr is within DCT Range. */
>> + dct_base = (dct_sel_baseaddr(pvt) << 27);
>> + dct_limit = (((dct_cont_limit_reg >> 11) & 0x1FFF) << 27) | 0x7FFFFFF;
>> + if (!(dct_cont_base_reg & BIT(0)) &&
>> + !(dct_base <= sys_addr && dct_limit >= sys_addr)) {
>> + return -EINVAL;
>> + }
>> +
>> + /* Verify number of dct's that participate in channel interleaving. */
>> + num_dcts_intlv = (int) hweight8(intlv_en);
>> +
>> + if (!(num_dcts_intlv % 2 == 0) || (num_dcts_intlv > 4))
>> + return -EINVAL;
>> +
>> + channel = f15_m30h_determine_channel(pvt, sys_addr, intlv_en,
>> + num_dcts_intlv, dct_sel);
>> +
>> + /* Verify if we stay within the MAX number of channels allowed */
> s/if//
>
>> + if (channel > 4 || channel < 0)
>> + return -EINVAL;
>> +
>> + leg_mmio_hole = (u8) (dct_cont_base_reg >> 1 & BIT(0));
>> +
>> + /* Get normalized DCT addr */
>> + if (leg_mmio_hole && (sys_addr >= BIT_64(32)))
>> + chan_offset = dhar_offset;
>> + else
>> + chan_offset = dct_base;
>> +
>> + chan_addr = sys_addr - chan_offset;
>> +
>> + /* remove channel interleave */
>> + if (num_dcts_intlv == 2) {
>> + if (intlv_addr == 0x4)
>> + chan_addr = ((chan_addr >> 9) << 8) |
>> + (chan_addr & 0xff);
>> + else if (intlv_addr == 0x5)
>> + chan_addr = ((chan_addr >> 10) << 9) |
>> + (chan_addr & 0x1ff);
>> + else
>> + return -EINVAL;
>> +
>> + } else if (num_dcts_intlv == 4) {
>> + if (intlv_addr == 0x4)
>> + chan_addr = ((chan_addr >> 10) << 8) |
>> + (chan_addr & 0xff);
>> + else if (intlv_addr == 0x5)
>> + chan_addr = ((chan_addr >> 11) << 9) |
>> + (chan_addr & 0x1ff);
>> + else
>> + return -EINVAL;
>> + }
>> +
>> + if (dct_offset_en) {
>> + amd64_read_pci_cfg(pvt->F1,
>> + DRAM_CONT_HIGH_OFF + (int) channel * 4,
>> + &tmp);
>> + chan_addr += ((tmp >> 11) & 0xfff) << 27;
>> + }
>> +
>> + /* Set DCTCFGSEL = ChannelSelect */
> No need for the comment.
>
>> + f15h_select_dct(pvt, channel);
>> +
>> + edac_dbg(1, " Normalized DCT addr: 0x%llx\n", chan_addr);
>> + /* Find the Chip select.. */
>> + /*
>> + * NOTE: if channel = 3, then alias it to 1.
>> + * This is because, in F15 M30h, there is support for
>> + * 4 DCT's, but only 2 are currently included in the architecture.
>> + * They are DCT0 and DCT3. But we have read all registers of DCT3
>> + * into pvt->csels[1]. So we need to use '1' here to get correct
>> + * info. Refer F15 M30h BKDG Section 2.10 and 2.10.3 for clarifications
>> + */
> Comment needs block formatting.
Okay.
>> +
>> + alias_channel = (channel == 3) ? 1 : channel;
>> +
>> + cs_found = f1x_lookup_addr_in_dct(chan_addr, node_id, alias_channel);
>> +
>> + if (cs_found >= 0)
>> + *chan_sel = alias_channel;
>> +
>> + return cs_found;
>> +}
>> +
>> +static int
>> +f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt,
>> + u64 sys_addr,
>> + int *chan_sel)
>> {
> Arg indentation.
>
>> int cs_found = -EINVAL;
>> unsigned range;
>>
>> for (range = 0; range < DRAM_RANGES; range++) {
>> -
>> if (!dram_rw(pvt, range))
>> continue;
>> -
>> - if ((get_dram_base(pvt, range) <= sys_addr) &&
>> - (get_dram_limit(pvt, range) >= sys_addr)) {
>> -
>> + if (F15H_M30H_CPU)
>> + cs_found = f15_m30h_match_to_this_node(pvt, range,
>> + sys_addr,
>> + chan_sel);
>> + else if ((get_dram_base(pvt, range) <= sys_addr) &&
>> + (get_dram_limit(pvt, range) >= sys_addr)) {
>> cs_found = f1x_match_to_this_node(pvt, range,
>> sys_addr, chan_sel);
>> if (cs_found >= 0)
>> @@ -1624,6 +1824,17 @@ static struct amd64_family_type amd64_family_types[] = {
>> .read_dct_pci_cfg = f15_read_dct_pci_cfg,
>> }
>> },
>> + [F15_M30H_CPUS] = {
>> + .ctl_name = "F15h_M30h",
>> + .f1_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1,
>> + .f3_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F3,
>> + .ops = {
>> + .early_channel_count = f1x_early_channel_count,
>> + .map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow,
>> + .dbam_to_cs = f16_dbam_to_chip_select,
>> + .read_dct_pci_cfg = f15_read_dct_pci_cfg,
>> + }
>> + },
>> [F16_CPUS] = {
>> .ctl_name = "F16h",
>> .f1_id = PCI_DEVICE_ID_AMD_16H_NB_F1,
>> @@ -2388,27 +2599,44 @@ static void setup_mci_misc_attrs(struct mem_ctl_info *mci,
>> static struct amd64_family_type *amd64_per_family_init(struct amd64_pvt *pvt)
>> {
>> u8 fam = boot_cpu_data.x86;
>> + u8 model = boot_cpu_data.x86_model;
>> struct amd64_family_type *fam_type = NULL;
>>
>> switch (fam) {
>> case 0xf:
>> fam_type = &amd64_family_types[K8_CPUS];
>> pvt->ops = &amd64_family_types[K8_CPUS].ops;
>> + pvt->family = fam;
> You could call it pvt->fam for less typing if you like.
Sure!, and I will take it outside the switch as suggested below.
>> + pvt->model = model;
>> break;
>>
>> case 0x10:
>> fam_type = &amd64_family_types[F10_CPUS];
>> pvt->ops = &amd64_family_types[F10_CPUS].ops;
>> + pvt->family = fam;
>> + pvt->model = model;
>> break;
>>
>> case 0x15:
>> + if (model == 0x30) {
>> + fam_type = &amd64_family_types[F15_M30H_CPUS];
>> + pvt->ops = &amd64_family_types[F15_M30H_CPUS].ops;
>> + pvt->family = fam;
>> + pvt->model = model;
>> + break;
>> + }
>> +
>> fam_type = &amd64_family_types[F15_CPUS];
>> pvt->ops = &amd64_family_types[F15_CPUS].ops;
>> + pvt->family = fam;
>> + pvt->model = model;
>> break;
>>
>> case 0x16:
>> fam_type = &amd64_family_types[F16_CPUS];
>> pvt->ops = &amd64_family_types[F16_CPUS].ops;
>> + pvt->family = fam;
>> + pvt->model = model;
> This ->family and ->model assignment is repeated in every case. What
> do you think, could it probably be done only once, outside the switch
> statement?
>
>> break;
>>
>> default:
>> @@ -2638,6 +2866,14 @@ static DEFINE_PCI_DEVICE_TABLE(amd64_pci_table) = {
>> },
>> {
>> .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,
>> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
>> index 2c6f113..ff15f14 100644
>> --- a/drivers/edac/amd64_edac.h
>> +++ b/drivers/edac/amd64_edac.h
>> @@ -170,6 +170,8 @@
>> /*
>> * 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_16H_NB_F1 0x1531
>> @@ -181,13 +183,24 @@
>> #define DRAM_BASE_LO 0x40
>> #define DRAM_LIMIT_LO 0x44
>>
>> -#define dram_intlv_en(pvt, i) ((u8)((pvt->ranges[i].base.lo >> 8) & 0x7))
>> +/*
>> + * F15 M30h DRAM Controller Base/Limit
>> + * D18F1x2[1C:00]
>> + */
>> +#define DRAM_CONT_BASE 0x200
>> +#define DRAM_CONT_LIMIT 0x204
>> +
>> +/*
>> + * F15 M30h DRAM Controller High Addre Offset
>> + * D18F1x2[4C:40]
>> + */
>> +#define DRAM_CONT_HIGH_OFF 0x240
>> +
>> #define dram_rw(pvt, i) ((u8)(pvt->ranges[i].base.lo & 0x3))
>> #define dram_intlv_sel(pvt, i) ((u8)((pvt->ranges[i].lim.lo >> 8) & 0x7))
>> #define dram_dst_node(pvt, i) ((u8)(pvt->ranges[i].lim.lo & 0x7))
>>
>> #define DHAR 0xf0
>> -#define dhar_valid(pvt) ((pvt)->dhar & BIT(0))
>> #define dhar_mem_hoist_valid(pvt) ((pvt)->dhar & BIT(1))
>> #define dhar_base(pvt) ((pvt)->dhar & 0xff000000)
>> #define k8_dhar_offset(pvt) (((pvt)->dhar & 0x0000ff00) << 16)
>> @@ -234,8 +247,6 @@
>> #define DDR3_MODE BIT(8)
>>
>> #define DCT_SEL_LO 0x110
>> -#define dct_sel_baseaddr(pvt) ((pvt)->dct_sel_lo & 0xFFFFF800)
>> -#define dct_sel_interleave_addr(pvt) (((pvt)->dct_sel_lo >> 6) & 0x3)
>> #define dct_high_range_enabled(pvt) ((pvt)->dct_sel_lo & BIT(0))
>> #define dct_interleave_enabled(pvt) ((pvt)->dct_sel_lo & BIT(2))
>>
>> @@ -293,10 +304,14 @@
>> /* MSRs */
>> #define MSR_MCGCTL_NBE BIT(4)
>>
>> +/* Helper Macro for F15h M30h */
>> +#define F15H_M30H_CPU ((pvt->family == 0x15) && \
>> + (pvt->model == 0x30))
> Please drop this macro - the condition is not that complicated to write
> out and not error prone.
Okay.
>> enum amd_families {
>> K8_CPUS = 0,
>> F10_CPUS,
>> F15_CPUS,
>> + F15_M30H_CPUS,
>> F16_CPUS,
>> NUM_FAMILIES,
>> };
>> @@ -337,6 +352,8 @@ struct amd64_pvt {
>> struct pci_dev *F1, *F2, *F3;
>>
>> u16 mc_node_id; /* MC index of this MC node */
>> + u8 family; /* CPU family */
>> + u8 model; /* CPU model */
>> int ext_model; /* extended model value of this node */
>> int channel_count;
>>
>> @@ -414,6 +431,14 @@ static inline u16 extract_syndrome(u64 status)
>> return ((status >> 47) & 0xff) | ((status >> 16) & 0xff00);
>> }
>>
>> +static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt)
>> +{
>> + if (F15H_M30H_CPU)
>> + return (((pvt->dct_sel_hi >> 9) & 0x1) << 2) |
>> + ((pvt->dct_sel_lo >> 6) & 0x3);
>> +
>> + return ((pvt)->dct_sel_lo >> 6) & 0x3;
>> +}
>> /*
>> * per-node ECC settings descriptor
>> */
>> @@ -504,3 +529,33 @@ static inline void enable_caches(void *dummy)
>> {
>> write_cr0(read_cr0() & ~X86_CR0_CD);
>> }
>> +
>> +static inline u8 dram_intlv_en(struct amd64_pvt *pvt, unsigned int i)
>> +{
>> + u32 tmp;
>> + if (F15H_M30H_CPU) {
> u32 tmp inside the if-statement.
>
>> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &tmp);
>> + return (u8) tmp & 0xF;
>> + }
>> + return (u8) (pvt->ranges[i].base.lo >> 8) & 0x7;
>> +}
>> +
>> +static inline u8 dhar_valid(struct amd64_pvt *pvt)
>> +{
>> + u32 tmp;
>> + if (F15H_M30H_CPU) {
> ditto.
>
>> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
>> + return (tmp >> 1) & BIT(0);
>> + }
>> + return (pvt)->dhar & BIT(0);
>> +}
>> +
>> +static inline u32 dct_sel_baseaddr(struct amd64_pvt *pvt)
>> +{
>> + u32 tmp;
>> + if (F15H_M30H_CPU) {
> ditto.
>
>> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
>> + return (tmp >> 11) & 0x1FFF;
>> + }
>> + return (pvt)->dct_sel_lo & 0xFFFFF800;
>> +}
will send out changes in V3;
Thanks,
Aravind.
On 8/6/2013 3:19 PM, Borislav Petkov wrote:
> On Fri, Aug 02, 2013 at 05:43:03PM -0500, Aravind Gopalakrishnan wrote:
>> Adding code to check for specific model (F15h, M30h) and if yes,
>> do not add flag AMD_NB_GART. Also check cpuid_edx(0x80000006) for
>> prescence of L3. If no L3, do not add any L3 flags.
>>
>> Signed-off-by: Aravind Gopalakrishnan <[email protected]>
>>
>> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
>> index 3048ded..3ee7a4d 100644
>> --- a/arch/x86/kernel/amd_nb.c
>> +++ b/arch/x86/kernel/amd_nb.c
>> @@ -20,6 +20,7 @@ const struct pci_device_id amd_nb_misc_ids[] = {
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F3) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M10H_F3) },
>> + { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F3) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F3) },
>> {}
>> };
>> @@ -27,6 +28,7 @@ EXPORT_SYMBOL(amd_nb_misc_ids);
>>
>> static const struct pci_device_id amd_nb_link_ids[] = {
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
>> + { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
>> {}
>> };
>> @@ -81,13 +83,21 @@ int amd_cache_northbridges(void)
>> next_northbridge(misc, amd_nb_misc_ids);
>> node_to_amd_nb(i)->link = link =
>> next_northbridge(link, amd_nb_link_ids);
>> - }
>> + }
>>
>> + /* GART present only on Fam15h upto model 0fh */
>> if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
>> - boot_cpu_data.x86 == 0x15)
>> + (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
>> amd_northbridges.flags |= AMD_NB_GART;
>>
>> /*
>> + * Check CPUID Fn8000_0006_EDX: L3 Cache Identifiers.
>> + * If == 0, then no need to proceed as there is no L3.
> This comment explains the code. Just sit down and think about it: does
> it make any sense to have that in a comment?
>> + */
>> + if (cpuid_edx(0x80000006) == 0)
>> + return 0;
>> +
>> + /*
>> * Some CPU families support L3 Cache Index Disable. There are some
>> * limitations because of E382 and E388 on family 0x10.
>> */
>> --
> I fixed it up like this:
>
> commit a0cb1bab68823648def0fda19bf307ad08eb25d2
> Author: Aravind Gopalakrishnan <[email protected]>
> Date: Fri Aug 2 17:43:03 2013 -0500
>
> x86, amd_nb: Clarify F15h, model 30h GART and L3 support
>
> F15h, models 0x30 and later don't have a GART. Note that. Also check
> cpuid_edx(0x80000006) for prescence of L3 because there are models which
> don't sport an L3.
>
> Signed-off-by: Aravind Gopalakrishnan <[email protected]>
> [ Boris: rewrite commit message, cleanup comments. ]
> Signed-off-by: Borislav Petkov <[email protected]>
Okay, Thanks!
> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> index 3048ded1b598..59554dca96ec 100644
> --- a/arch/x86/kernel/amd_nb.c
> +++ b/arch/x86/kernel/amd_nb.c
> @@ -20,6 +20,7 @@ const struct pci_device_id amd_nb_misc_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC) },
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F3) },
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M10H_F3) },
> + { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F3) },
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F3) },
> {}
> };
> @@ -27,6 +28,7 @@ EXPORT_SYMBOL(amd_nb_misc_ids);
>
> static const struct pci_device_id amd_nb_link_ids[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F4) },
> + { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_M30H_NB_F4) },
> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F4) },
> {}
> };
> @@ -81,13 +83,20 @@ int amd_cache_northbridges(void)
> next_northbridge(misc, amd_nb_misc_ids);
> node_to_amd_nb(i)->link = link =
> next_northbridge(link, amd_nb_link_ids);
> - }
> + }
>
> + /* GART present only on Fam15h upto model 0fh */
> if (boot_cpu_data.x86 == 0xf || boot_cpu_data.x86 == 0x10 ||
> - boot_cpu_data.x86 == 0x15)
> + (boot_cpu_data.x86 == 0x15 && boot_cpu_data.x86_model < 0x10))
> amd_northbridges.flags |= AMD_NB_GART;
>
> /*
> + * Check for L3 cache presence.
> + */
> + if (!cpuid_edx(0x80000006))
> + return 0;
> +
> + /*
> * Some CPU families support L3 Cache Index Disable. There are some
> * limitations because of E382 and E388 on family 0x10.
> */
>
>
>
On 8/6/2013 3:17 PM, Borislav Petkov wrote:
> On Fri, Aug 02, 2013 at 05:43:02PM -0500, Aravind Gopalakrishnan wrote:
>> Adding PCI_DEVICE_ID_AMD_15H_NB_M30H_F3 and PCI_DEVICE_ID_AMD_15H_NB_M30H_F4
>> for F15h model 30h. This is required for the file amd_nb.c
> ... and amd64_edac.c
>
> Also, remember to read the comment at the beginning of
> include/linux/pci_ids.h!
>
> I fixed up the commit message when applying.
>
Okay, Thanks!
On 8/6/2013 3:55 PM, Aravind Gopalakrishnan wrote:
> On 8/6/2013 3:23 PM, Borislav Petkov wrote:
>> On Fri, Aug 02, 2013 at 05:43:04PM -0500, Aravind Gopalakrishnan wrote:
>>> Adding support for handling ECC error decoding for new F15 models.
>>> On newer models, support has been included for upto 4 DCT's,
>>> however, only DCT0 and DCT3 are currently configured. (Refer BKDG
>>> Section 2.10)
>>> There is also a new "Routing DRAM Requests" algorithm for this model.
>>>
>>> Tested on Fam15h M30h with ECC turned on using mce_amd_inj facility and
>>> verified to be functionally correct.
>>>
>>> Signed-off-by: Aravind Gopalakrishnan <[email protected]>
>>>
>>> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
>>> index 8b6a034..42dab12 100644
>>> --- a/drivers/edac/amd64_edac.c
>>> +++ b/drivers/edac/amd64_edac.c
>>> @@ -123,7 +123,11 @@ static void f15h_select_dct(struct amd64_pvt
>>> *pvt, u8 dct)
>>> u32 reg = 0;
>>> amd64_read_pci_cfg(pvt->F1, DCT_CFG_SEL, ®);
>>> - reg &= 0xfffffffe;
>>> + /*
>>> + * If Fam15h M30h, mask out last two bits for programming dct.
>>> + * Else, only mask out the last bit.
>>> + */
>> No need for that comment.
>>
>>> + reg &= (F15_M30H_CPUS) ? 0xfffffffc : 0xfffffff8;
>> Let's see, I had 0xfffffffe and now you have 0xfffffff8. See the
>> difference?
>>
>> Also, F15H_M30H_CPUS is an enum, AFAICT, and it will always be true as
>> long as it is != 0. What you meant here is "F15H_M30H_CPU". So you see
>> how those defines are misleading and made you use them wrong.
>>
>> So let's correct it all and make it more clear:
>>
>> reg &= (pvt->model >= 0x30) ? ~3 : ~1;
>>
>> we don't need to look at the family because this function -
>> f15h_select_dct - is called on F15h only anyway.
> Oops.. My bad. Will fix this.
>
>>> reg |= dct;
>>> amd64_write_pci_cfg(pvt->F1, DCT_CFG_SEL, reg);
>>> }
>>> @@ -133,8 +137,12 @@ static int f15_read_dct_pci_cfg(struct
>>> amd64_pvt *pvt, int addr, u32 *val,
>>> {
>>> u8 dct = 0;
>>> + /*
>>> + * For F15 M30h, the second dct is DCT 3.
>>> + * Refer BKDG Section 2.10
>>> + */
>>> if (addr >= 0x140 && addr <= 0x1a0) {
>>> - dct = 1;
>>> + dct = F15H_M30H_CPU ? 3 : 1;
>> ditto here: use pvt->model like above.
> Okay.
>
>>> addr -= 0x100;
>>> }
>>> @@ -205,8 +213,9 @@ static int amd64_set_scrub_rate(struct
>>> mem_ctl_info *mci, u32 bw)
>>> if (boot_cpu_data.x86 == 0xf)
>>> min_scrubrate = 0x0;
>>> - /* F15h Erratum #505 */
>>> - if (boot_cpu_data.x86 == 0x15)
>>> + /* F15h Models 0x00 - 0x0f Erratum #505 */
>> Hmm, are you sure this erratum is fixed in SR? Talk to Eric about it
>> first, please. RG says "No fix planned".
>>
>>> + if (boot_cpu_data.x86 == 0x15 &&
>>> + boot_cpu_data.x86_model != 0x30)
>>> f15h_select_dct(pvt, 0);
>>> return __amd64_set_scrub_rate(pvt->F3, bw, min_scrubrate);
>>> @@ -218,8 +227,9 @@ static int amd64_get_scrub_rate(struct
>>> mem_ctl_info *mci)
>>> u32 scrubval = 0;
>>> int i, retval = -EINVAL;
>>> - /* F15h Erratum #505 */
>>> - if (boot_cpu_data.x86 == 0x15)
>>> + /* F15h Models 0x00 - 0x0f Erratum #505 */
>> ditto.
>
> I have pinged some people about it, will let you know..
>
>>> + if (boot_cpu_data.x86 == 0x15 &&
>>> + boot_cpu_data.x86_model != 0x30)
>>> f15h_select_dct(pvt, 0);
>>> amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
>>> @@ -343,10 +353,10 @@ static void get_cs_base_and_mask(struct
>>> amd64_pvt *pvt, int csrow, u8 dct,
>>> addr_shift = 4;
>>> /*
>>> - * F16h needs two addr_shift values: 8 for high and 6 for low
>>> - * (cf. F16h BKDG).
>>> + * F16h and F15h(model 30h) needs two addr_shift values:
>>> + * 8 for high and 6 for low (cf. F16h BKDG).
>>> */
>>> - } else if (boot_cpu_data.x86 == 0x16) {
>>> + } else if (boot_cpu_data.x86 == 0x16 || F15H_M30H_CPU) {
>> pvt->family and ->model please. This macro is ugly and confusing.
>>
Quick question:
Shall I change all instances of boot_cpu_data.[x86|x86_model] to use
pvt->fam and pvt->model
wherever applicable as part of this patch or have it go in as a separate
patch?
>>> csbase = pvt->csels[dct].csbases[csrow];
>>> csmask = pvt->csels[dct].csmasks[csrow >> 1];
>>> @@ -743,6 +753,9 @@ static void prep_chip_selects(struct amd64_pvt
>>> *pvt)
>>> if (boot_cpu_data.x86 == 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 (F15H_M30H_CPU) {
>>> + pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 4;
>>> + pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 2;
>>> } else {
>>> pvt->csels[0].b_cnt = pvt->csels[1].b_cnt = 8;
>>> pvt->csels[0].m_cnt = pvt->csels[1].m_cnt = 4;
>>> @@ -850,9 +863,9 @@ static u64 get_error_address(struct mce *m)
>>> addr = m->addr & GENMASK(start_bit, end_bit);
>>> /*
>>> - * Erratum 637 workaround
>>> + * Erratum 637 workaround for F15h Models 0x00 - 0x0f
>> Same here - RG says this erratum is, like 505, "No fix planned"
>>
>>> */
>>> - if (c->x86 == 0x15) {
>>> + if (c->x86 == 0x15 && c->x86_model != 0x30) {
>>> struct amd64_pvt *pvt;
>>> u64 cc6_base, tmp_addr;
>>> u32 tmp;
>>> @@ -918,6 +931,7 @@ static void read_dram_base_limit_regs(struct
>>> amd64_pvt *pvt, unsigned range)
>>> struct amd_northbridge *nb;
>>> struct pci_dev *misc, *f1 = NULL;
>>> struct cpuinfo_x86 *c = &boot_cpu_data;
>>> + unsigned int pci_func;
>>> int off = range << 3;
>>> u32 llim;
>>> @@ -942,7 +956,12 @@ static void read_dram_base_limit_regs(struct
>>> amd64_pvt *pvt, unsigned range)
>>> return;
>>> misc = nb->misc;
>>> - f1 = pci_get_related_function(misc->vendor,
>>> PCI_DEVICE_ID_AMD_15H_NB_F1, misc);
>>> +
>>> + pci_func = (c->x86_model == 0x30) ?
>>> PCI_DEVICE_ID_AMD_15H_M30H_NB_F1
>>> + : PCI_DEVICE_ID_AMD_15H_M30H_NB_F1;
>> Something's wrong with this line :-)
>
> Oh no! (vim auto-complete! :( )
> Will fix this.
>
>>> +
>>> + f1 = pci_get_related_function(misc->vendor, pci_func, misc);
>>> +
>>> if (WARN_ON(!f1))
>>> return;
>>> @@ -1173,7 +1192,8 @@ static int f15_dbam_to_chip_select(struct
>>> amd64_pvt *pvt, u8 dct,
>>> }
>>> /*
>>> - * F16h has only limited cs_modes
>>> + * F16h has only limited cs_modes.
>>> + * F15 M30h follows the same pattern.
>>> */
>>> static int f16_dbam_to_chip_select(struct amd64_pvt *pvt, u8 dct,
>>> unsigned cs_mode)
>>> @@ -1218,6 +1238,51 @@ static void read_dram_ctl_register(struct
>>> amd64_pvt *pvt)
>>> }
>>> /*
>>> + * Determine channel (DCT) based on the interleaving mode:
>>> + * F15h M30h BKDG, 2.10.12 Memory Interleaving Modes.
>>> + */
>>> +static u8 f15_m30h_determine_channel(struct amd64_pvt *pvt, u64
>>> sys_addr,
>>> + u8 intlv_en, int num_dcts_intlv, u32 dct_sel)
>> Arg indentation.
> Sorry, will fix.
>>> +{
>>> + u8 channel = 0;
>>> + u8 select;
>>> + u8 intlv_addr = dct_sel_interleave_addr(pvt);
>>> +
>>> + if (!(intlv_en))
>>> + return (u8)(dct_sel);
>>> + /*
>>> + * If num_dcts_intlv == 2, them consider addr bit 8 or addr bit 9
>>> + * depending on intlv_addr, then return either channel = 0/1/2/3.
>>> + * If num_dcts_intlv == 4, consider either bits[8:9] or bits[9:10]
>>> + * depending on intlv_addr, then return the value obtained.
>> No need for that comment.
>>
>>> + */
>>> +
>>> + if (num_dcts_intlv == 2) {
>>> + if (intlv_addr == 0x4)
>>> + select = (sys_addr >> 8) & BIT(0);
>>> + else if (intlv_addr == 0x5)
>>> + select = (sys_addr >> 9) & BIT(0);
>> So, basically you can take care of the two cases together by doing:
>>
>> select = (sys_addr >> 8) & 0x3;
>>
>> ?
>>
>>> + else
>>> + return -EINVAL;
>>> +
>>> + /* If select !=0, upper DCT selected, else lower DCT */
>> I think we can see that, no need for the comment.
>>
>>> + channel = select ? 0x3 : 0;
>>> +
>>> + } else if (num_dcts_intlv == 4) {
>>> + if (intlv_addr == 0x4)
>>> + select = (sys_addr >> 8) & 0x3;
>>> + else if (intlv_addr == 0x5)
>>> + select = (sys_addr >> 9) & 0x3;
>> Ditto here, as above.
>>
>>> + else
>>> + return -EINVAL;
>>> +
>>> + channel = select;
>>> + }
>>> +
>>> + return channel;
>>> +}
>>> +
>>> +/*
>>> * Determine channel (DCT) based on the interleaving mode: F10h
>>> BKDG, 2.8.9 Memory
>>> * Interleaving Modes.
>>> */
>>> @@ -1366,6 +1431,10 @@ static int f1x_lookup_addr_in_dct(u64
>>> in_addr, u8 nid, u8 dct)
>>> (in_addr & cs_mask), (cs_base & cs_mask));
>>> if ((in_addr & cs_mask) == (cs_base & cs_mask)) {
>>> + if (F15H_M30H_CPU) {
>>> + cs_found = csrow;
>>> + break;
>>> + }
>>> cs_found = f10_process_possible_spare(pvt, dct, csrow);
>>> edac_dbg(1, " MATCH csrow=%d\n", cs_found);
>>> @@ -1492,20 +1561,151 @@ static int f1x_match_to_this_node(struct
>>> amd64_pvt *pvt, unsigned range,
>>> return cs_found;
>>> }
>>> -static int f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt, u64
>>> sys_addr,
>>> - int *chan_sel)
>>> +/*
>>> + * Routing DRAM Requests algorithm is different for F15h M30h.
>>> + * It is cleaner to use a brand new function rather than
>>> + * adding quirks in the more generic 'f1x_match_to_this_node'.
>>> + * Refer "2.10.5 DRAM Routing Requests" in BKDG for info.
>>> + */
>> This comment belongs in the commit message of this patch.
> Okay.
>
>>> +static int f15_m30h_match_to_this_node(struct amd64_pvt *pvt,
>>> unsigned range,
>>> + u64 sys_addr, int *chan_sel)
>> Arg indentation
> Sorry, will fix..
>>> +{
>>> + int cs_found = -EINVAL;
>>> + int num_dcts_intlv = 0;
>>> + u64 chan_addr, chan_offset, high_addr_offset;
>>> + u64 dct_base, dct_limit;
>>> + u32 dct_cont_base_reg, dct_cont_limit_reg, tmp;
>>> + u8 channel, alias_channel, leg_mmio_hole;
>>> +
>>> +
>>> + /* Read DRAM Control registers specific to F15 M30h. */
>> No need for the comment.
>>
>>> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &dct_cont_base_reg);
>>> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &dct_cont_limit_reg);
>>> +
>>> + /* Extract F15h M30h specific config */
>> No need for the comment.
>>
>>> + u64 dhar_offset = f10_dhar_offset(pvt);
>>> + u8 dct_offset_en = (u8) ((dct_cont_base_reg >> 3) & BIT(0));
>>> + u8 dct_sel = (u8) ((dct_cont_base_reg >> 4) & 0x7);
>>> + u8 intlv_addr = dct_sel_interleave_addr(pvt);
>>> + u8 node_id = dram_dst_node(pvt, range);
>>> + u8 intlv_en = dram_intlv_en(pvt, range);
>>> +
>>> + edac_dbg(1, "(range %d) SystemAddr= 0x%llx Limit=0x%llx\n",
>>> + range, sys_addr, get_dram_limit(pvt, range));
>>> +
>>> + if (!(get_dram_base(pvt, range) <= sys_addr) &&
>>> + !(get_dram_limit(pvt, range) >= sys_addr))
>>> + return -EINVAL;
>>> +
>>> + if (dhar_valid(pvt) &&
>>> + dhar_base(pvt) <= sys_addr &&
>>> + sys_addr < BIT_64(32)) {
>>> + amd64_warn("Huh? Address is in the MMIO hole: 0x%016llx\n",
>>> + sys_addr);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Verify sys_addr is within DCT Range. */
>>> + dct_base = (dct_sel_baseaddr(pvt) << 27);
>>> + dct_limit = (((dct_cont_limit_reg >> 11) & 0x1FFF) << 27) |
>>> 0x7FFFFFF;
>>> + if (!(dct_cont_base_reg & BIT(0)) &&
>>> + !(dct_base <= sys_addr && dct_limit >= sys_addr)) {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Verify number of dct's that participate in channel
>>> interleaving. */
>>> + num_dcts_intlv = (int) hweight8(intlv_en);
>>> +
>>> + if (!(num_dcts_intlv % 2 == 0) || (num_dcts_intlv > 4))
>>> + return -EINVAL;
>>> +
>>> + channel = f15_m30h_determine_channel(pvt, sys_addr, intlv_en,
>>> + num_dcts_intlv, dct_sel);
>>> +
>>> + /* Verify if we stay within the MAX number of channels allowed */
>> s/if//
>>
>>> + if (channel > 4 || channel < 0)
>>> + return -EINVAL;
>>> +
>>> + leg_mmio_hole = (u8) (dct_cont_base_reg >> 1 & BIT(0));
>>> +
>>> + /* Get normalized DCT addr */
>>> + if (leg_mmio_hole && (sys_addr >= BIT_64(32)))
>>> + chan_offset = dhar_offset;
>>> + else
>>> + chan_offset = dct_base;
>>> +
>>> + chan_addr = sys_addr - chan_offset;
>>> +
>>> + /* remove channel interleave */
>>> + if (num_dcts_intlv == 2) {
>>> + if (intlv_addr == 0x4)
>>> + chan_addr = ((chan_addr >> 9) << 8) |
>>> + (chan_addr & 0xff);
>>> + else if (intlv_addr == 0x5)
>>> + chan_addr = ((chan_addr >> 10) << 9) |
>>> + (chan_addr & 0x1ff);
>>> + else
>>> + return -EINVAL;
>>> +
>>> + } else if (num_dcts_intlv == 4) {
>>> + if (intlv_addr == 0x4)
>>> + chan_addr = ((chan_addr >> 10) << 8) |
>>> + (chan_addr & 0xff);
>>> + else if (intlv_addr == 0x5)
>>> + chan_addr = ((chan_addr >> 11) << 9) |
>>> + (chan_addr & 0x1ff);
>>> + else
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (dct_offset_en) {
>>> + amd64_read_pci_cfg(pvt->F1,
>>> + DRAM_CONT_HIGH_OFF + (int) channel * 4,
>>> + &tmp);
>>> + chan_addr += ((tmp >> 11) & 0xfff) << 27;
>>> + }
>>> +
>>> + /* Set DCTCFGSEL = ChannelSelect */
>> No need for the comment.
>>
>>> + f15h_select_dct(pvt, channel);
>>> +
>>> + edac_dbg(1, " Normalized DCT addr: 0x%llx\n", chan_addr);
>>> + /* Find the Chip select.. */
>>> + /*
>>> + * NOTE: if channel = 3, then alias it to 1.
>>> + * This is because, in F15 M30h, there is support for
>>> + * 4 DCT's, but only 2 are currently included in the architecture.
>>> + * They are DCT0 and DCT3. But we have read all registers of DCT3
>>> + * into pvt->csels[1]. So we need to use '1' here to get correct
>>> + * info. Refer F15 M30h BKDG Section 2.10 and 2.10.3 for
>>> clarifications
>>> + */
>> Comment needs block formatting.
> Okay.
>
>>> +
>>> + alias_channel = (channel == 3) ? 1 : channel;
>>> +
>>> + cs_found = f1x_lookup_addr_in_dct(chan_addr, node_id,
>>> alias_channel);
>>> +
>>> + if (cs_found >= 0)
>>> + *chan_sel = alias_channel;
>>> +
>>> + return cs_found;
>>> +}
>>> +
>>> +static int
>>> +f1x_translate_sysaddr_to_cs(struct amd64_pvt *pvt,
>>> + u64 sys_addr,
>>> + int *chan_sel)
>>> {
>> Arg indentation.
>>
>>> int cs_found = -EINVAL;
>>> unsigned range;
>>> for (range = 0; range < DRAM_RANGES; range++) {
>>> -
>>> if (!dram_rw(pvt, range))
>>> continue;
>>> -
>>> - if ((get_dram_base(pvt, range) <= sys_addr) &&
>>> - (get_dram_limit(pvt, range) >= sys_addr)) {
>>> -
>>> + if (F15H_M30H_CPU)
>>> + cs_found = f15_m30h_match_to_this_node(pvt, range,
>>> + sys_addr,
>>> + chan_sel);
>>> + else if ((get_dram_base(pvt, range) <= sys_addr) &&
>>> + (get_dram_limit(pvt, range) >= sys_addr)) {
>>> cs_found = f1x_match_to_this_node(pvt, range,
>>> sys_addr, chan_sel);
>>> if (cs_found >= 0)
>>> @@ -1624,6 +1824,17 @@ static struct amd64_family_type
>>> amd64_family_types[] = {
>>> .read_dct_pci_cfg = f15_read_dct_pci_cfg,
>>> }
>>> },
>>> + [F15_M30H_CPUS] = {
>>> + .ctl_name = "F15h_M30h",
>>> + .f1_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F1,
>>> + .f3_id = PCI_DEVICE_ID_AMD_15H_M30H_NB_F3,
>>> + .ops = {
>>> + .early_channel_count = f1x_early_channel_count,
>>> + .map_sysaddr_to_csrow = f1x_map_sysaddr_to_csrow,
>>> + .dbam_to_cs = f16_dbam_to_chip_select,
>>> + .read_dct_pci_cfg = f15_read_dct_pci_cfg,
>>> + }
>>> + },
>>> [F16_CPUS] = {
>>> .ctl_name = "F16h",
>>> .f1_id = PCI_DEVICE_ID_AMD_16H_NB_F1,
>>> @@ -2388,27 +2599,44 @@ static void setup_mci_misc_attrs(struct
>>> mem_ctl_info *mci,
>>> static struct amd64_family_type *amd64_per_family_init(struct
>>> amd64_pvt *pvt)
>>> {
>>> u8 fam = boot_cpu_data.x86;
>>> + u8 model = boot_cpu_data.x86_model;
>>> struct amd64_family_type *fam_type = NULL;
>>> switch (fam) {
>>> case 0xf:
>>> fam_type = &amd64_family_types[K8_CPUS];
>>> pvt->ops = &amd64_family_types[K8_CPUS].ops;
>>> + pvt->family = fam;
>> You could call it pvt->fam for less typing if you like.
> Sure!, and I will take it outside the switch as suggested below.
>>> + pvt->model = model;
>>> break;
>>> case 0x10:
>>> fam_type = &amd64_family_types[F10_CPUS];
>>> pvt->ops = &amd64_family_types[F10_CPUS].ops;
>>> + pvt->family = fam;
>>> + pvt->model = model;
>>> break;
>>> case 0x15:
>>> + if (model == 0x30) {
>>> + fam_type = &amd64_family_types[F15_M30H_CPUS];
>>> + pvt->ops = &amd64_family_types[F15_M30H_CPUS].ops;
>>> + pvt->family = fam;
>>> + pvt->model = model;
>>> + break;
>>> + }
>>> +
>>> fam_type = &amd64_family_types[F15_CPUS];
>>> pvt->ops = &amd64_family_types[F15_CPUS].ops;
>>> + pvt->family = fam;
>>> + pvt->model = model;
>>> break;
>>> case 0x16:
>>> fam_type = &amd64_family_types[F16_CPUS];
>>> pvt->ops = &amd64_family_types[F16_CPUS].ops;
>>> + pvt->family = fam;
>>> + pvt->model = model;
>> This ->family and ->model assignment is repeated in every case. What
>> do you think, could it probably be done only once, outside the switch
>> statement?
>>
>>> break;
>>> default:
>>> @@ -2638,6 +2866,14 @@ static
>>> DEFINE_PCI_DEVICE_TABLE(amd64_pci_table) = {
>>> },
>>> {
>>> .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,
>>> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
>>> index 2c6f113..ff15f14 100644
>>> --- a/drivers/edac/amd64_edac.h
>>> +++ b/drivers/edac/amd64_edac.h
>>> @@ -170,6 +170,8 @@
>>> /*
>>> * 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_16H_NB_F1 0x1531
>>> @@ -181,13 +183,24 @@
>>> #define DRAM_BASE_LO 0x40
>>> #define DRAM_LIMIT_LO 0x44
>>> -#define dram_intlv_en(pvt, i) ((u8)((pvt->ranges[i].base.lo >> 8)
>>> & 0x7))
>>> +/*
>>> + * F15 M30h DRAM Controller Base/Limit
>>> + * D18F1x2[1C:00]
>>> + */
>>> +#define DRAM_CONT_BASE 0x200
>>> +#define DRAM_CONT_LIMIT 0x204
>>> +
>>> +/*
>>> + * F15 M30h DRAM Controller High Addre Offset
>>> + * D18F1x2[4C:40]
>>> + */
>>> +#define DRAM_CONT_HIGH_OFF 0x240
>>> +
>>> #define dram_rw(pvt, i) ((u8)(pvt->ranges[i].base.lo & 0x3))
>>> #define dram_intlv_sel(pvt, i) ((u8)((pvt->ranges[i].lim.lo >> 8)
>>> & 0x7))
>>> #define dram_dst_node(pvt, i) ((u8)(pvt->ranges[i].lim.lo & 0x7))
>>> #define DHAR 0xf0
>>> -#define dhar_valid(pvt) ((pvt)->dhar & BIT(0))
>>> #define dhar_mem_hoist_valid(pvt) ((pvt)->dhar & BIT(1))
>>> #define dhar_base(pvt) ((pvt)->dhar & 0xff000000)
>>> #define k8_dhar_offset(pvt) (((pvt)->dhar & 0x0000ff00) << 16)
>>> @@ -234,8 +247,6 @@
>>> #define DDR3_MODE BIT(8)
>>> #define DCT_SEL_LO 0x110
>>> -#define dct_sel_baseaddr(pvt) ((pvt)->dct_sel_lo & 0xFFFFF800)
>>> -#define dct_sel_interleave_addr(pvt) (((pvt)->dct_sel_lo >> 6) & 0x3)
>>> #define dct_high_range_enabled(pvt) ((pvt)->dct_sel_lo & BIT(0))
>>> #define dct_interleave_enabled(pvt) ((pvt)->dct_sel_lo & BIT(2))
>>> @@ -293,10 +304,14 @@
>>> /* MSRs */
>>> #define MSR_MCGCTL_NBE BIT(4)
>>> +/* Helper Macro for F15h M30h */
>>> +#define F15H_M30H_CPU ((pvt->family == 0x15) && \
>>> + (pvt->model == 0x30))
>> Please drop this macro - the condition is not that complicated to write
>> out and not error prone.
> Okay.
>>> enum amd_families {
>>> K8_CPUS = 0,
>>> F10_CPUS,
>>> F15_CPUS,
>>> + F15_M30H_CPUS,
>>> F16_CPUS,
>>> NUM_FAMILIES,
>>> };
>>> @@ -337,6 +352,8 @@ struct amd64_pvt {
>>> struct pci_dev *F1, *F2, *F3;
>>> u16 mc_node_id; /* MC index of this MC node */
>>> + u8 family; /* CPU family */
>>> + u8 model; /* CPU model */
>>> int ext_model; /* extended model value of this node */
>>> int channel_count;
>>> @@ -414,6 +431,14 @@ static inline u16 extract_syndrome(u64 status)
>>> return ((status >> 47) & 0xff) | ((status >> 16) & 0xff00);
>>> }
>>> +static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt)
>>> +{
>>> + if (F15H_M30H_CPU)
>>> + return (((pvt->dct_sel_hi >> 9) & 0x1) << 2) |
>>> + ((pvt->dct_sel_lo >> 6) & 0x3);
>>> +
>>> + return ((pvt)->dct_sel_lo >> 6) & 0x3;
>>> +}
>>> /*
>>> * per-node ECC settings descriptor
>>> */
>>> @@ -504,3 +529,33 @@ static inline void enable_caches(void *dummy)
>>> {
>>> write_cr0(read_cr0() & ~X86_CR0_CD);
>>> }
>>> +
>>> +static inline u8 dram_intlv_en(struct amd64_pvt *pvt, unsigned int i)
>>> +{
>>> + u32 tmp;
>>> + if (F15H_M30H_CPU) {
>> u32 tmp inside the if-statement.
>>
>>> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_LIMIT, &tmp);
>>> + return (u8) tmp & 0xF;
>>> + }
>>> + return (u8) (pvt->ranges[i].base.lo >> 8) & 0x7;
>>> +}
>>> +
>>> +static inline u8 dhar_valid(struct amd64_pvt *pvt)
>>> +{
>>> + u32 tmp;
>>> + if (F15H_M30H_CPU) {
>> ditto.
>>
>>> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
>>> + return (tmp >> 1) & BIT(0);
>>> + }
>>> + return (pvt)->dhar & BIT(0);
>>> +}
>>> +
>>> +static inline u32 dct_sel_baseaddr(struct amd64_pvt *pvt)
>>> +{
>>> + u32 tmp;
>>> + if (F15H_M30H_CPU) {
>> ditto.
>>
>>> + amd64_read_pci_cfg(pvt->F1, DRAM_CONT_BASE, &tmp);
>>> + return (tmp >> 11) & 0x1FFF;
>>> + }
>>> + return (pvt)->dct_sel_lo & 0xFFFFF800;
>>> +}
>
> will send out changes in V3;
>
> Thanks,
> Aravind.
On Tue, Aug 06, 2013 at 05:00:51PM -0500, Aravind Gopalakrishnan wrote:
> Quick question: Shall I change all instances of
> boot_cpu_data.[x86|x86_model] to use pvt->fam and pvt->model wherever
> applicable as part of this patch or have it go in as a separate patch?
Yes, a separate pre-patch would be lovely.
Also, please trim your reply by quoting only the relevant part because
with a huge mail, almost completely quoted like this one, I'm choking in
">" lines trying to find the text you wrote.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
>
>>> addr -= 0x100;
>>> }
>>> @@ -205,8 +213,9 @@ static int amd64_set_scrub_rate(struct
>>> mem_ctl_info *mci, u32 bw)
>>> if (boot_cpu_data.x86 == 0xf)
>>> min_scrubrate = 0x0;
>>> - /* F15h Erratum #505 */
>>> - if (boot_cpu_data.x86 == 0x15)
>>> + /* F15h Models 0x00 - 0x0f Erratum #505 */
>> Hmm, are you sure this erratum is fixed in SR? Talk to Eric about it
>> first, please. RG says "No fix planned".
>>
>>> + if (boot_cpu_data.x86 == 0x15 &&
>>> + boot_cpu_data.x86_model != 0x30)
>>> f15h_select_dct(pvt, 0);
>>> return __amd64_set_scrub_rate(pvt->F3, bw, min_scrubrate);
>>> @@ -218,8 +227,9 @@ static int amd64_get_scrub_rate(struct
>>> mem_ctl_info *mci)
>>> u32 scrubval = 0;
>>> int i, retval = -EINVAL;
>>> - /* F15h Erratum #505 */
>>> - if (boot_cpu_data.x86 == 0x15)
>>> + /* F15h Models 0x00 - 0x0f Erratum #505 */
>> ditto.
>
> I have pinged some people about it, will let you know..
>
From email conversations internally, here are the current status of the
errata:
* Erratum 505: Should not be carried over to newer Fam15h models.
* Erratum 637: Not fixed.
I have taken this into account and changed the code to workaround only E637.
> will send out changes in V3;
>
> Thanks,
> Aravind.
Corrected the indentations; Removed unnecessary comments;
Sending out changes in [PATCH 3/3 V3].
Thanks,
-Aravind.