Re: [PATCH v2 16/17] driver/edac: enable Hygon support to AMD64 EDAC driver

[Paolo Bonzini]

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