| Message ID | 1532352037-7151-17-git-send-email-puwen@hygon.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Hygon Dhyana Family 18h processor | expand |
On 23/07/2018 15:20, Pu Wen wrote: > @@ -211,7 +211,7 @@ static int __set_scrub_rate(struct amd64_pvt *pvt, u32 new_bw, u32 min_rate) > > scrubval = scrubrates[i].scrubval; > > - if (pvt->fam == 0x17) { > + if (pvt->fam == 0x17 || pvt->fam == 0x18) { > __f17h_set_scrubval(pvt, scrubval); > } else if (pvt->fam == 0x15 && pvt->model == 0x60) { > f15h_select_dct(pvt, 0); This, and many other occurrences in this file, should in my opinion avoid testing family 18h without also checking for Hygon as a vendor. You probably need to add a vendor field to struct amd64_pvt and initialize it in per_family_init. Paolo
On 2018-07-29 07:42, Paolo Bonzini wrote: >On 23/07/2018 15:20, Pu Wen wrote: >> scrubval = scrubrates[i].scrubval; >> >> - if (pvt->fam == 0x17) { >> + if (pvt->fam == 0x17 || pvt->fam == 0x18) { >> __f17h_set_scrubval(pvt, scrubval); >> } else if (pvt->fam == 0x15 && pvt->model == 0x60) { >> f15h_select_dct(pvt, 0); > >This, and many other occurrences in this file, should in my opinion >avoid testing family 18h without also checking for Hygon as a vendor. >You probably need to add a vendor field to struct amd64_pvt and >initialize it in per_family_init. Thanks for the suggestion. As AMD and Hygon will negotiate the usage of CPU family number to make sure the unique of family numbers in both company's processors. As Hygon will use family 18h for Dhyana, AMD will not use family 18h and jump to family 19h for new product. So if family number if 18h, processor should be Hygon Dhyana. Based on this assumption, we created this patch set. To keep consistency, surely can modify the patchset as suggested, but will introduce some additional vendor checking codes. If the vendor field is added to amd64_pvt, and check the vendor in 0x18 codes, then the codes may like: - if (pvt->fam == 0x17) { + if (pvt->fam == 0x17 || pvt->vendor == X86_VENDOR_HYGON) { switch cases will be modified similar to: + case 0x18: + if(pvt->vendor == X86_VENDOR_HYGON) { + fam_type = &family_types[HYGON_F18_CPUS]; + pvt->ops = &family_types[HYGON_F18_CPUS].ops; + break; + } + and: case 0x17: + case 0x18: + /* Family 18h is only for Hygon now */ + if(pvt->fam == 0x18 && pvt->vendor == X86_VENDOR_AMD) + break; + Do something here both for AMD 17h and Hygon 18h... Any suggestions for that? Thanks, Pu Wen
On 30/07/2018 18:43, Pu Wen wrote: > On 2018-07-29 07:42, Paolo Bonzini wrote: >> On 23/07/2018 15:20, Pu Wen wrote: >>> scrubval = scrubrates[i].scrubval; >>> >>> - if (pvt->fam == 0x17) { >>> + if (pvt->fam == 0x17 || pvt->fam == 0x18) { >>> __f17h_set_scrubval(pvt, scrubval); >>> } else if (pvt->fam == 0x15 && pvt->model == 0x60) { >>> f15h_select_dct(pvt, 0); >> >> This, and many other occurrences in this file, should in my opinion >> avoid testing family 18h without also checking for Hygon as a vendor. >> You probably need to add a vendor field to struct amd64_pvt and >> initialize it in per_family_init. > > Thanks for the suggestion. > > As AMD and Hygon will negotiate the usage of CPU family number > to make sure the unique of family numbers in both company's processors. > As Hygon will use family 18h for Dhyana, AMD will not use family 18h > and jump to family 19h for new product. So if family number if 18h, > processor should be Hygon Dhyana. Based on this assumption, we created > this patch set. But if that's the case, it doesn't make sense to have a new vendor! If AMD's 17h and Hygon's 18h ever diverge, you could always choose the right behavior based on the family, without checking the vendor. However, if the x86 maintainers prefer to have a new X86_VENDOR_* constant, I'd just ignore the fact that AMD will skip family 18h, and introduce vendor checks along the lines below. This has the advantage that it's not an issue if AMD ends up _not_ skipping family 18h. > If the vendor field is added to amd64_pvt, and check the vendor in 0x18 > codes, then the codes may like: > - if (pvt->fam == 0x17) { > + if (pvt->fam == 0x17 || pvt->vendor == X86_VENDOR_HYGON) { > > switch cases will be modified similar to: > + case 0x18: > + if(pvt->vendor == X86_VENDOR_HYGON) { > + fam_type = &family_types[HYGON_F18_CPUS]; > + pvt->ops = &family_types[HYGON_F18_CPUS].ops; > + break; > + } > + Either that, or if (pvt->vendor == X86_VENDOR_AMD) { ... } else { fam_type = &family_types[HYGON_F18_CPUS]; pvt->ops = &family_types[HYGON_F18_CPUS].ops; break; } Paolo
On 2018/7/31 15:38, Paolo Bonzini wrote: > But if that's the case, it doesn't make sense to have a new vendor! If > AMD's 17h and Hygon's 18h ever diverge, you could always choose the > right behavior based on the family, without checking the vendor. > > However, if the x86 maintainers prefer to have a new X86_VENDOR_* > constant, I'd just ignore the fact that AMD will skip family 18h, and > introduce vendor checks along the lines below. This has the advantage > that it's not an issue if AMD ends up _not_ skipping family 18h. Thanks for the suggestion. It's reasonable for the fully consideration. For code consistency, will check explicitly for Hygon before testing family 18h in next patch set. Thanks, Pu Wen
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index 18aeabb..d8b4b0e 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -211,7 +211,7 @@ static int __set_scrub_rate(struct amd64_pvt *pvt, u32 new_bw, u32 min_rate) scrubval = scrubrates[i].scrubval; - if (pvt->fam == 0x17) { + if (pvt->fam == 0x17 || pvt->fam == 0x18) { __f17h_set_scrubval(pvt, scrubval); } else if (pvt->fam == 0x15 && pvt->model == 0x60) { f15h_select_dct(pvt, 0); @@ -264,6 +264,7 @@ static int get_scrub_rate(struct mem_ctl_info *mci) break; case 0x17: + case 0x18: amd64_read_pci_cfg(pvt->F6, F17H_SCR_BASE_ADDR, &scrubval); if (scrubval & BIT(0)) { amd64_read_pci_cfg(pvt->F6, F17H_SCR_LIMIT_ADDR, &scrubval); @@ -1044,6 +1045,7 @@ static void determine_memory_type(struct amd64_pvt *pvt) goto ddr3; case 0x17: + case 0x18: if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(5)) pvt->dram_type = MEM_LRDDR4; else if ((pvt->umc[0].dimm_cfg | pvt->umc[1].dimm_cfg) & BIT(4)) @@ -2200,6 +2202,16 @@ static struct amd64_family_type family_types[] = { .dbam_to_cs = f17_base_addr_to_cs_size, } }, + [HYGON_F18_CPUS] = { + /* Hygon F18h uses the same AMD F17h support */ + .ctl_name = "Hygon_F18h", + .f0_id = PCI_DEVICE_ID_HYGON_18H_DF_F0, + .f6_id = PCI_DEVICE_ID_HYGON_18H_DF_F6, + .ops = { + .early_channel_count = f17_early_channel_count, + .dbam_to_cs = f17_base_addr_to_cs_size, + } + }, }; /* @@ -3192,6 +3204,11 @@ static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt) pvt->ops = &family_types[F17_CPUS].ops; break; + case 0x18: + fam_type = &family_types[HYGON_F18_CPUS]; + pvt->ops = &family_types[HYGON_F18_CPUS].ops; + break; + default: amd64_err("Unsupported family!\n"); return NULL; @@ -3428,6 +3445,7 @@ static const struct x86_cpu_id amd64_cpuids[] = { { X86_VENDOR_AMD, 0x15, X86_MODEL_ANY, X86_FEATURE_ANY, 0 }, { X86_VENDOR_AMD, 0x16, X86_MODEL_ANY, X86_FEATURE_ANY, 0 }, { X86_VENDOR_AMD, 0x17, X86_MODEL_ANY, X86_FEATURE_ANY, 0 }, + { X86_VENDOR_HYGON, 0x18, X86_MODEL_ANY, X86_FEATURE_ANY, 0 }, { } }; MODULE_DEVICE_TABLE(x86cpu, amd64_cpuids); diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h index 1d4b74e..6e5f609 100644 --- a/drivers/edac/amd64_edac.h +++ b/drivers/edac/amd64_edac.h @@ -116,6 +116,9 @@ #define PCI_DEVICE_ID_AMD_17H_DF_F0 0x1460 #define PCI_DEVICE_ID_AMD_17H_DF_F6 0x1466 +#define PCI_DEVICE_ID_HYGON_18H_DF_F0 0x1460 +#define PCI_DEVICE_ID_HYGON_18H_DF_F6 0x1466 + /* * Function 1 - Address Map */ @@ -281,6 +284,7 @@ enum amd_families { F16_CPUS, F16_M30H_CPUS, F17_CPUS, + HYGON_F18_CPUS, NUM_FAMILIES, }; diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c index 2ab4d61..c605089 100644 --- a/drivers/edac/mce_amd.c +++ b/drivers/edac/mce_amd.c @@ -1059,7 +1059,8 @@ static int __init mce_amd_init(void) { struct cpuinfo_x86 *c = &boot_cpu_data; - if (c->x86_vendor != X86_VENDOR_AMD) + if (c->x86_vendor != X86_VENDOR_AMD && + c->x86_vendor != X86_VENDOR_HYGON) return -ENODEV; fam_ops = kzalloc(sizeof(struct amd_decoder_ops), GFP_KERNEL); @@ -1113,6 +1114,7 @@ static int __init mce_amd_init(void) break; case 0x17: + case 0x18: xec_mask = 0x3f; if (!boot_cpu_has(X86_FEATURE_SMCA)) { printk(KERN_WARNING "Decoding supported only on Scalable MCA processors.\n");
To make AMD64 MCE and EDAC drivers working on Hygon platforms, add Hygon vendor check for them. Also Hygon PCI Device ID DF_F0/DF_F6(0x1460/0x1466) of Host bridge is needed for these drivers. And support Dhyana processors by using AMD 0x17 codes. Signed-off-by: Pu Wen <puwen@hygon.cn> --- drivers/edac/amd64_edac.c | 20 +++++++++++++++++++- drivers/edac/amd64_edac.h | 4 ++++ drivers/edac/mce_amd.c | 4 +++- 3 files changed, 26 insertions(+), 2 deletions(-)