| Message ID | 1375483384-2302-4-git-send-email-Aravind.Gopalakrishnan@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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 <Aravind.Gopalakrishnan@amd.com> > > 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; > +}
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 <Aravind.Gopalakrishnan@amd.com> >> >> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <Aravind.Gopalakrishnan@amd.com> >>> >>> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
> >>> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; +}
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 <Aravind.Gopalakrishnan@amd.com>