Re: [PATCH v3 16/17] driver/edac: enable Hygon support to AMD64 EDAC driver
On Mon, Aug 13, 2018 at 12:17 PM, Pu Wen wrote:
> On 2018/8/12 3:56, Michael Jin wrote:
>>
>> On Sat, Aug 11, 2018 at 9:30 AM, Pu Wen wrote:
>>>
>>> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
>>> - if (pvt->fam == 0x17) {
>>> + if (pvt->fam == 0x17 || pvt->vendor == X86_VENDOR_HYGON) {
>>> __f17h_set_scrubval(pvt, scrubval);
>>
>>
>> Separating the vendor check as an "else if (pvt->vendor ==
>> X86_VENDOR_HYGON)" block would make architectural changes (future
>> hygon models, i.e. 19h, 20h, etc) less confusing.
>
>
> Your suggestion is reasonable, but that might make the branch a little
> complicated.If we explicitly testing Hygon family in condition case,
> will that be ok?
> +if (pvt->fam == 0x17 ||
> + (pvt->vendor == X86_VENDOR_HYGON && pvt->fam == 0x18))
The condition case for family 18h and vendor Hygon looks better
because it is more clear.
Michael
Re: [PATCH v3 16/17] driver/edac: enable Hygon support to AMD64 EDAC driver
On 2018/8/12 4:10, Michael Jin wrote:
On Sat, Aug 11, 2018 at 3:56 PM, Michael Jin wrote:
On Sat, Aug 11, 2018 at 9:30 AM, Pu Wen wrote:
@@ -1051,6 +1065,16 @@ static void determine_memory_type(struct amd64_pvt *pvt)
else
pvt->dram_type = MEM_DDR4;
return;
+ case 0x18:
+ if (pvt->vendor == X86_VENDOR_HYGON) {
This vendor checking is not necessary as there are no other known
family 18h processors.
Actually, vendor checking works here. Although, AMD has not released a
family 18h yet.
Yes, that's the case.
Thanks,
Pu Wen
Re: [PATCH v3 16/17] driver/edac: enable Hygon support to AMD64 EDAC driver
On 2018/8/12 3:56, Michael Jin wrote:
On Sat, Aug 11, 2018 at 9:30 AM, Pu Wen wrote:
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 18aeabb..fb81354 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->vendor == X86_VENDOR_HYGON) {
__f17h_set_scrubval(pvt, scrubval);
Separating the vendor check as an "else if (pvt->vendor ==
X86_VENDOR_HYGON)" block would make architectural changes (future
hygon models, i.e. 19h, 20h, etc) less confusing.
Your suggestion is reasonable, but that might make the branch a little
complicated.If we explicitly testing Hygon family in condition case,
will that be ok?
+if (pvt->fam == 0x17 ||
+ (pvt->vendor == X86_VENDOR_HYGON && pvt->fam == 0x18))
+ amd64_read_pci_cfg(pvt->F6,
+ F17H_SCR_BASE_ADDR, &scrubval);
+ if (scrubval & BIT(0)) {
+ amd64_read_pci_cfg(pvt->F6,
The new lines after "amd64_read_pci_cfg(pvt->F6," can be removed.
To fix the warning "line over 80 characters" reported by running the
checking script checkpatch.pl, I added the new line.
But your sugesstion is reasonable, remove the new line will make the
codes much easier to read.
@@ -1051,6 +1065,16 @@ static void determine_memory_type(struct amd64_pvt *pvt)
else
pvt->dram_type = MEM_DDR4;
return;
+ case 0x18:
+ if (pvt->vendor == X86_VENDOR_HYGON) {
This vendor checking is not necessary as there are no other known
family 18h processors.
switch (pvt->fam) {
@@ -3192,6 +3227,13 @@ static struct amd64_family_type *per_family_init(struct
amd64_pvt *pvt)
pvt->ops= &family_types[F17_CPUS].ops;
break;
+ case 0x18:
+ if (pvt->vendor == X86_VENDOR_HYGON) {
+ fam_type= &family_types[HYGON_F18_CPUS];
+ pvt->ops= &family_types[HYGON_F18_CPUS].ops;
+ break;
+ }
There is a missing second 'break' statement after the "if (pvt->vendor
== X86_VENDOR_HYGON)" block for case 0x18, see case 0x15 and case 0x16
for comparison.
The missed second 'break' is on purpose. Thinking that if BIOS report
a vendor AMD and family 18h processor(which is not in case now),
the code will fall through and print out "Unsupported family".
diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index 2ab4d61..f7adc47 100644
+ case 0x18:
+ if (c->x86_vendor == X86_VENDOR_HYGON) {
+ xec_mask = 0x3f;
+ if (!boot_cpu_has(X86_FEATURE_SMCA)) {
+ pr_warn("Decoding supported only on Scalable MCA
processors.\n");
+ goto err_out;
+ }
+ break;
+ }
The 'break' statement could be moved outside of the "if (c->x86_vendor
== X86_VENDOR_HYGON)" block, this is to allow case 0x18 to reach the
'break' statement if the vendor is not X86_VENDOR_HYGON.
For the same reason as previous, if the vendor is not X86_VENDOR_HYGON,
it's not a valid vendor by now, and should fall through to the default
case and print out error message.
Thanks,
Pu Wen
Re: [PATCH v3 16/17] driver/edac: enable Hygon support to AMD64 EDAC driver
On 2018/8/12 3:56, Michael Jin wrote:
On Sat, Aug 11, 2018 at 9:30 AM, Pu Wen wrote:
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 18aeabb..fb81354 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->vendor == X86_VENDOR_HYGON) {
__f17h_set_scrubval(pvt, scrubval);
Separating the vendor check as an "else if (pvt->vendor ==
X86_VENDOR_HYGON)" block would make architectural changes (future
hygon models, i.e. 19h, 20h, etc) less confusing.
Your suggestion is reasonable, but that might make the branch a little
complicated.If we explicitly testing Hygon family in condition case,
will that be ok?
+if (pvt->fam == 0x17 ||
+ (pvt->vendor == X86_VENDOR_HYGON && pvt->fam == 0x18))
+ amd64_read_pci_cfg(pvt->F6,
+ F17H_SCR_BASE_ADDR, &scrubval);
+ if (scrubval & BIT(0)) {
+ amd64_read_pci_cfg(pvt->F6,
The new lines after "amd64_read_pci_cfg(pvt->F6," can be removed.
To fix the warning "line over 80 characters" reported by running the
checking script checkpatch.pl, I added the new line.
But your sugesstion is reasonable, remove the new line will make the
codes much easier to read.
@@ -1051,6 +1065,16 @@ static void determine_memory_type(struct amd64_pvt *pvt)
else
pvt->dram_type = MEM_DDR4;
return;
+ case 0x18:
+ if (pvt->vendor == X86_VENDOR_HYGON) {
This vendor checking is not necessary as there are no other known
family 18h processors.
switch (pvt->fam) {
@@ -3192,6 +3227,13 @@ static struct amd64_family_type *per_family_init(struct
amd64_pvt *pvt)
pvt->ops= &family_types[F17_CPUS].ops;
break;
+ case 0x18:
+ if (pvt->vendor == X86_VENDOR_HYGON) {
+ fam_type= &family_types[HYGON_F18_CPUS];
+ pvt->ops= &family_types[HYGON_F18_CPUS].ops;
+ break;
+ }
There is a missing second 'break' statement after the "if (pvt->vendor
== X86_VENDOR_HYGON)" block for case 0x18, see case 0x15 and case 0x16
for comparison.
The missed second 'break' is on purpose. Thinking that if BIOS report
a vendor AMD and family 18h processor(which is not in case now),
the code will fall through and print out "Unsupported family".
diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index 2ab4d61..f7adc47 100644
+ case 0x18:
+ if (c->x86_vendor == X86_VENDOR_HYGON) {
+ xec_mask = 0x3f;
+ if (!boot_cpu_has(X86_FEATURE_SMCA)) {
+ pr_warn("Decoding supported only on Scalable MCA
processors.\n");
+ goto err_out;
+ }
+ break;
+ }
The 'break' statement could be moved outside of the "if (c->x86_vendor
== X86_VENDOR_HYGON)" block, this is to allow case 0x18 to reach the
'break' statement if the vendor is not X86_VENDOR_HYGON.
For the same reason as previous, if the vendor is not X86_VENDOR_HYGON,
it's not a valid vendor by now, and should fall through to the default
case and print out error message.
Thanks,
Pu Wen
Re: [PATCH v3 16/17] driver/edac: enable Hygon support to AMD64 EDAC driver
On Sat, Aug 11, 2018 at 3:56 PM, Michael Jin wrote:
> On Sat, Aug 11, 2018 at 9:30 AM, Pu Wen wrote:
>> @@ -1051,6 +1065,16 @@ static void determine_memory_type(struct amd64_pvt
>> *pvt)
>> else
>> pvt->dram_type = MEM_DDR4;
>> return;
>> + case 0x18:
>> + if (pvt->vendor == X86_VENDOR_HYGON) {
>
> This vendor checking is not necessary as there are no other known
> family 18h processors.
Actually, vendor checking works here. Although, AMD has not released a
family 18h yet.
Re: [PATCH v3 16/17] driver/edac: enable Hygon support to AMD64 EDAC driver
On Sat, Aug 11, 2018 at 9:30 AM, Pu Wen wrote:
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 18aeabb..fb81354 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->vendor == X86_VENDOR_HYGON) {
> __f17h_set_scrubval(pvt, scrubval);
Separating the vendor check as an "else if (pvt->vendor ==
X86_VENDOR_HYGON)" block would make architectural changes (future
hygon models, i.e. 19h, 20h, etc) less confusing.
> + amd64_read_pci_cfg(pvt->F6,
> + F17H_SCR_BASE_ADDR, &scrubval);
> + if (scrubval & BIT(0)) {
> + amd64_read_pci_cfg(pvt->F6,
The new lines after "amd64_read_pci_cfg(pvt->F6," can be removed.
> @@ -1051,6 +1065,16 @@ static void determine_memory_type(struct amd64_pvt
> *pvt)
> else
> pvt->dram_type = MEM_DDR4;
> return;
> + case 0x18:
> + if (pvt->vendor == X86_VENDOR_HYGON) {
This vendor checking is not necessary as there are no other known
family 18h processors.
> switch (pvt->fam) {
> @@ -3192,6 +3227,13 @@ static struct amd64_family_type
> *per_family_init(struct amd64_pvt *pvt)
> pvt->ops= &family_types[F17_CPUS].ops;
> break;
>
> + case 0x18:
> + if (pvt->vendor == X86_VENDOR_HYGON) {
> + fam_type= &family_types[HYGON_F18_CPUS];
> + pvt->ops= &family_types[HYGON_F18_CPUS].ops;
> + break;
> + }
There is a missing second 'break' statement after the "if (pvt->vendor
== X86_VENDOR_HYGON)" block for case 0x18, see case 0x15 and case 0x16
for comparison.
> diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
> index 2ab4d61..f7adc47 100644
> + case 0x18:
> + if (c->x86_vendor == X86_VENDOR_HYGON) {
> + xec_mask = 0x3f;
> + if (!boot_cpu_has(X86_FEATURE_SMCA)) {
> + pr_warn("Decoding supported only on Scalable
> MCA processors.\n");
> + goto err_out;
> + }
> + break;
> + }
The 'break' statement could be moved outside of the "if (c->x86_vendor
== X86_VENDOR_HYGON)" block, this is to allow case 0x18 to reach the
'break' statement if the vendor is not X86_VENDOR_HYGON.
[PATCH v3 16/17] driver/edac: enable Hygon support to AMD64 EDAC driver
To make AMD64 EDAC and MCE drivers working on Hygon platforms, add
vendor checking for Hygon by using the code path of AMD 0x17. Add a
vendor field to struct amd64_pvt and initialize it in per_family_init
for vendor checking.
Also Hygon PCI Device ID DF_F0/DF_F6(0x1460/0x1466) of Host bridges
is needed for edac driver.
Signed-off-by: Pu Wen
---
drivers/edac/amd64_edac.c | 45 -
drivers/edac/amd64_edac.h | 5 +
drivers/edac/mce_amd.c| 12 +++-
3 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 18aeabb..fb81354 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->vendor == X86_VENDOR_HYGON) {
__f17h_set_scrubval(pvt, scrubval);
} else if (pvt->fam == 0x15 && pvt->model == 0x60) {
f15h_select_dct(pvt, 0);
@@ -273,6 +273,20 @@ static int get_scrub_rate(struct mem_ctl_info *mci)
scrubval = 0;
}
break;
+ case 0x18:
+ if (pvt->vendor == X86_VENDOR_HYGON) {
+ 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);
+ scrubval &= 0xF;
+ scrubval += 0x5;
+ } else {
+ scrubval = 0;
+ }
+ break;
+ }
default:
amd64_read_pci_cfg(pvt->F3, SCRCTRL, &scrubval);
@@ -1051,6 +1065,16 @@ static void determine_memory_type(struct amd64_pvt *pvt)
else
pvt->dram_type = MEM_DDR4;
return;
+ case 0x18:
+ if (pvt->vendor == X86_VENDOR_HYGON) {
+ 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))
+ pvt->dram_type = MEM_RDDR4;
+ else
+ pvt->dram_type = MEM_DDR4;
+ return;
+ }
default:
WARN(1, KERN_ERR "%s: Family??? 0x%x\n", __func__, pvt->fam);
@@ -2200,6 +2224,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,
+ }
+ },
};
/*
@@ -3149,6 +3183,7 @@ static struct amd64_family_type *per_family_init(struct
amd64_pvt *pvt)
pvt->ext_model = boot_cpu_data.x86_model >> 4;
pvt->stepping = boot_cpu_data.x86_stepping;
pvt->model = boot_cpu_data.x86_model;
+ pvt->vendor = boot_cpu_data.x86_vendor;
pvt->fam= boot_cpu_data.x86;
switch (pvt->fam) {
@@ -3192,6 +3227,13 @@ static struct amd64_family_type *per_family_init(struct
amd64_pvt *pvt)
pvt->ops= &family_types[F17_CPUS].ops;
break;
+ case 0x18:
+ if (pvt->vendor == X86_VENDOR_HYGON) {
+ 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 +3470,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..5176b51 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -116,6 +116,9 @@
#define PCI_DEVICE_ID_AMD_17H_DF_F00x1460
#define PCI_D
