[v6,07/16] x86/pci: Add Hygon Dhyana support to PCI and north bridge

As Hygon registered its PCI Vendor ID as a new one 0x1d94, and there
are PCI Devices 0x1450/0x1463/0x1464 for Host bridge on Hygon Dhyana
platforms, so add Hygon Dhyana support to the PCI and north bridge
subsystem by using the code path of AMD family 17h.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>	# pci_ids.h
Signed-off-by: Pu Wen <puwen@hygon.cn>
---
 arch/x86/kernel/amd_nb.c | 47 +++++++++++++++++++++++++++++++++++++++++------
 arch/x86/pci/amd_bus.c   |  6 ++++--
 include/linux/pci_ids.h  |  2 ++
 3 files changed, 47 insertions(+), 8 deletions(-)

On Mon, Sep 10, 2018 at 09:17:11PM +0800, Pu Wen wrote:
> As Hygon registered its PCI Vendor ID as a new one 0x1d94, and there
> are PCI Devices 0x1450/0x1463/0x1464 for Host bridge on Hygon Dhyana
> platforms, so add Hygon Dhyana support to the PCI and north bridge
> subsystem by using the code path of AMD family 17h.
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>	# pci_ids.h
> Signed-off-by: Pu Wen <puwen@hygon.cn>
> ---
>  arch/x86/kernel/amd_nb.c | 47 +++++++++++++++++++++++++++++++++++++++++------
>  arch/x86/pci/amd_bus.c   |  6 ++++--
>  include/linux/pci_ids.h  |  2 ++
>  3 files changed, 47 insertions(+), 8 deletions(-)

Reviewed-by: Borislav Petkov <bp@suse.de>

On 09/10/2018 08:17 AM, Pu Wen wrote:
> As Hygon registered its PCI Vendor ID as a new one 0x1d94, and there
> are PCI Devices 0x1450/0x1463/0x1464 for Host bridge on Hygon Dhyana
> platforms, so add Hygon Dhyana support to the PCI and north bridge
> subsystem by using the code path of AMD family 17h.
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>	# pci_ids.h
> Signed-off-by: Pu Wen <puwen@hygon.cn>
> ---
>  arch/x86/kernel/amd_nb.c | 47 +++++++++++++++++++++++++++++++++++++++++------
>  arch/x86/pci/amd_bus.c   |  6 ++++--
>  include/linux/pci_ids.h  |  2 ++
>  3 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> index b481b95..3e2ea18 100644
> --- a/arch/x86/kernel/amd_nb.c
> +++ b/arch/x86/kernel/amd_nb.c
> @@ -61,6 +61,21 @@ static const struct pci_device_id amd_nb_link_ids[] = {
>  	{}
>  };
>  
> +static const struct pci_device_id hygon_root_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_HYGON, PCI_DEVICE_ID_AMD_17H_ROOT) },
> +	{}
> +};
> +
> +const struct pci_device_id hygon_nb_misc_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_HYGON, PCI_DEVICE_ID_AMD_17H_DF_F3) },
> +	{}
> +};
> +
> +static const struct pci_device_id hygon_nb_link_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_HYGON, PCI_DEVICE_ID_AMD_17H_DF_F4) },
> +	{}
> +};
> +
>  const struct amd_nb_bus_dev_range amd_nb_bus_dev_ranges[] __initconst = {
>  	{ 0x00, 0x18, 0x20 },
>  	{ 0xff, 0x00, 0x20 },
> @@ -197,12 +212,25 @@ int amd_cache_northbridges(void)
>  	u16 i = 0;
>  	struct amd_northbridge *nb;
>  	struct pci_dev *root, *misc, *link;
> +	const struct pci_device_id *root_ids = NULL;
> +	const struct pci_device_id *misc_ids = NULL;
> +	const struct pci_device_id *link_ids = NULL;
> +
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
> +		root_ids = amd_root_ids;
> +		misc_ids = amd_nb_misc_ids;
> +		link_ids = amd_nb_link_ids;
> +	} else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> +		root_ids = hygon_root_ids;
> +		misc_ids = hygon_nb_misc_ids;
> +		link_ids = hygon_nb_link_ids;
> +	}

To be compatible with "before this patch" you should probably do:

	if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
		root_ids = hygon_root_ids;
		misc_ids = hygon_nb_misc_ids;
		link_ids = hygon_nb_link_ids;
	} else {
		root_ids = amd_root_ids;
		misc_ids = amd_nb_misc_ids;
		link_ids = amd_nb_link_ids;
	}

That way they are always the AMD values if not your chip.

>  
>  	if (amd_northbridges.num)
>  		return 0;
>  
>  	misc = NULL;
> -	while ((misc = next_northbridge(misc, amd_nb_misc_ids)) != NULL)
> +	while ((misc = next_northbridge(misc, misc_ids)) != NULL)
>  		i++;
>  
>  	if (!i)
> @@ -218,11 +246,11 @@ int amd_cache_northbridges(void)
>  	link = misc = root = NULL;
>  	for (i = 0; i != amd_northbridges.num; i++) {
>  		node_to_amd_nb(i)->root = root =
> -			next_northbridge(root, amd_root_ids);
> +			next_northbridge(root, root_ids);
>  		node_to_amd_nb(i)->misc = misc =
> -			next_northbridge(misc, amd_nb_misc_ids);
> +			next_northbridge(misc, misc_ids);
>  		node_to_amd_nb(i)->link = link =
> -			next_northbridge(link, amd_nb_link_ids);
> +			next_northbridge(link, link_ids);
>  	}
>  
>  	if (amd_gart_present())
> @@ -263,9 +291,15 @@ bool __init early_is_amd_nb(u32 device)
>  {
>  	const struct pci_device_id *id;
>  	u32 vendor = device & 0xffff;
> +	const struct pci_device_id *misc_ids = NULL;
> +
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> +		misc_ids = amd_nb_misc_ids;
> +	else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
> +		misc_ids = hygon_nb_misc_ids;

Same comment as above.  This will probably eliminate the PANIC that
that was reported by LKP.

Thanks,
Tom

>  
>  	device >>= 16;
> -	for (id = amd_nb_misc_ids; id->vendor; id++)
> +	for (id = misc_ids; id->vendor; id++)
>  		if (vendor == id->vendor && device == id->device)
>  			return true;
>  	return false;
> @@ -277,7 +311,8 @@ struct resource *amd_get_mmconfig_range(struct resource *res)
>  	u64 base, msr;
>  	unsigned int segn_busn_bits;
>  
> -	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD &&
> +	    boot_cpu_data.x86_vendor != X86_VENDOR_HYGON)
>  		return NULL;
>  
>  	/* assume all cpus from fam10h have mmconfig */
> diff --git a/arch/x86/pci/amd_bus.c b/arch/x86/pci/amd_bus.c
> index 649bdde..bfa50e6 100644
> --- a/arch/x86/pci/amd_bus.c
> +++ b/arch/x86/pci/amd_bus.c
> @@ -93,7 +93,8 @@ static int __init early_root_info_init(void)
>  		vendor = id & 0xffff;
>  		device = (id>>16) & 0xffff;
>  
> -		if (vendor != PCI_VENDOR_ID_AMD)
> +		if (vendor != PCI_VENDOR_ID_AMD &&
> +		    vendor != PCI_VENDOR_ID_HYGON)
>  			continue;
>  
>  		if (hb_probes[i].device == device) {
> @@ -390,7 +391,8 @@ static int __init pci_io_ecs_init(void)
>  
>  static int __init amd_postcore_init(void)
>  {
> -	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD &&
> +	    boot_cpu_data.x86_vendor != X86_VENDOR_HYGON)
>  		return 0;
>  
>  	early_root_info_init();
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index d157983..8a0841c 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2561,6 +2561,8 @@
>  
>  #define PCI_VENDOR_ID_AMAZON		0x1d0f
>  
> +#define PCI_VENDOR_ID_HYGON		0x1d94
> +
>  #define PCI_VENDOR_ID_TEKRAM		0x1de1
>  #define PCI_DEVICE_ID_TEKRAM_DC290	0xdc29
>  
>

On Wed, 19 Sep 2018, Lendacky, Thomas wrote:
> 
> To be compatible with "before this patch" you should probably do:
> 
> 	if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> 		root_ids = hygon_root_ids;
> 		misc_ids = hygon_nb_misc_ids;
> 		link_ids = hygon_nb_link_ids;
> 	} else {
> 		root_ids = amd_root_ids;
> 		misc_ids = amd_nb_misc_ids;
> 		link_ids = amd_nb_link_ids;
> 	}
> 
> That way they are always the AMD values if not your chip.

And please can we have a comment why this is called on anything which is
not AMD or HYGON. It's all but obvious and clearly unexpected.

Thanks,

	tglx

On 2018/9/20 1:20, Lendacky, Thomas wrote:
>> @@ -197,12 +212,25 @@ int amd_cache_northbridges(void)
>>   	u16 i = 0;
>>   	struct amd_northbridge *nb;
>>   	struct pci_dev *root, *misc, *link;
>> +	const struct pci_device_id *root_ids = NULL;
>> +	const struct pci_device_id *misc_ids = NULL;
>> +	const struct pci_device_id *link_ids = NULL;
>> +
>> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
>> +		root_ids = amd_root_ids;
>> +		misc_ids = amd_nb_misc_ids;
>> +		link_ids = amd_nb_link_ids;
>> +	} else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
>> +		root_ids = hygon_root_ids;
>> +		misc_ids = hygon_nb_misc_ids;
>> +		link_ids = hygon_nb_link_ids;
>> +	}
>
> To be compatible with "before this patch" you should probably do:
>
> 	if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> 		root_ids = hygon_root_ids;
> 		misc_ids = hygon_nb_misc_ids;
> 		link_ids = hygon_nb_link_ids;
> 	} else {
> 		root_ids = amd_root_ids;
> 		misc_ids = amd_nb_misc_ids;
> 		link_ids = amd_nb_link_ids;
> 	}
>
> That way they are always the AMD values if not your chip.
>
...
>> @@ -263,9 +291,15 @@ bool __init early_is_amd_nb(u32 device)
>>   {
>>   	const struct pci_device_id *id;
>>   	u32 vendor = device & 0xffff;
>> +	const struct pci_device_id *misc_ids = NULL;
>> +
>> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
>> +		misc_ids = amd_nb_misc_ids;
>> +	else if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
>> +		misc_ids = hygon_nb_misc_ids;
>
> Same comment as above.  This will probably eliminate the PANIC that
> that was reported by LKP.

Yes, it's this modification that caused the PANIC reported by LKP.
Because the misc_ids will be NULL if running on Intel chip.

If change the vendor checking like this:

+	if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
+		misc_ids = hygon_nb_misc_ids;
+	else
+		misc_ids = amd_nb_misc_ids;

The PANIC will be eliminated.

The PANIC will also be eliminated by adding AMD vendor checking in
amd_gart_present:

+	if(boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+		return false;

To prevent the function return true on non AMD platform.

Thanks,
Pu Wen