diff mbox

[V3,1/3] x86/PCI: Fix PCI root numa_node info on AMD family15h

Message ID 1399489127-6961-2-git-send-email-suravee.suthikulpanit@amd.com
State Superseded
Headers show

Commit Message

Suravee Suthikulpanit May 7, 2014, 6:58 p.m. UTC 
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

This patch fixes the numa_node information in sysfs for PCI root
on AMD family15h platforms (currently showing -1) by adding
the hostbridge in the list of probed devices to be used for initializing
pci_root_info structue.

This mechanism is now deprecated in favor of info in ACPI.
Therefore, this patch also adds note stating the deprecation.

Reference(s):
  https://bugzilla.kernel.org/show_bug.cgi?id=72051
  Advanced Micro Devices (AMD), "BIOS and Kernel Developer's
  Guide (BKDG) for AMD Family 15h Models 00h-0fh Processors."  Section
  3.4 Device [1F:18]h Function 1 Configuration Registers; D18F1x[EC:E0]
  Configuration Map, 42301 Rev 3.12 - October 11, 2012.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
Tested-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Robert Richter <rric@kernel.org>
Cc: Daniel J Blueman <daniel@numascale.com>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
---
 arch/x86/pci/amd_bus.c | 75 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 29 deletions(-)

Comments

Robert Richter May 8, 2014, 8:59 a.m. UTC | #1 
On 07.05.14 13:58:45, suravee.suthikulpanit@amd.com wrote:
> @@ -113,10 +122,17 @@ static int __init early_fill_mp_bus_info(void)
>  		info = alloc_pci_root_info(min_bus, max_bus, node, link);
>  	}
>  
> +	/*
> +	 * The following code is only supported until Fam11h.
> +	 * Newer processors will depend on ACPI MCFG table instead.
> +	 */
> +	if (boot_cpu_data.x86 > 0x11)
> +		return 0;
> +
>  	/* get the default node and link for left over res */

As this is the only substantial change of your patch, I would better
drop ther rest or at least split it in two patches. Should this change
also be for stable?

-Robert
--
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
Robert Richter May 8, 2014, 9:01 a.m. UTC | #2 
On 08.05.14 10:59:05, Robert Richter wrote:
> On 07.05.14 13:58:45, suravee.suthikulpanit@amd.com wrote:
> > @@ -113,10 +122,17 @@ static int __init early_fill_mp_bus_info(void)
> >  		info = alloc_pci_root_info(min_bus, max_bus, node, link);
> >  	}
> >  
> > +	/*
> > +	 * The following code is only supported until Fam11h.
> > +	 * Newer processors will depend on ACPI MCFG table instead.
> > +	 */
> > +	if (boot_cpu_data.x86 > 0x11)
> > +		return 0;
> > +
> >  	/* get the default node and link for left over res */
> 
> As this is the only substantial change of your patch, I would better
> drop ther rest or at least split it in two patches. Should this change
> also be for stable?

Of course adding the hostbridge must be also part of the patch, didn't
note this due to the other noise. See why the split would be good?

> 
> -Robert
--
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
Suravee Suthikulpanit May 8, 2014, 2:39 p.m. UTC | #3 
On 5/8/2014 4:01 AM, Robert Richter wrote:
> On 08.05.14 10:59:05, Robert Richter wrote:
>> On 07.05.14 13:58:45, suravee.suthikulpanit@amd.com wrote:
>>> @@ -113,10 +122,17 @@ static int __init early_fill_mp_bus_info(void)
>>>   		info = alloc_pci_root_info(min_bus, max_bus, node, link);
>>>   	}
>>>
>>> +	/*
>>> +	 * The following code is only supported until Fam11h.
>>> +	 * Newer processors will depend on ACPI MCFG table instead.
>>> +	 */
>>> +	if (boot_cpu_data.x86 > 0x11)
>>> +		return 0;
>>> +
>>>   	/* get the default node and link for left over res */
>>
>> As this is the only substantial change of your patch, I would better
>> drop ther rest or at least split it in two patches. Should this change
>> also be for stable?
>
> Of course adding the hostbridge must be also part of the patch, didn't
> note this due to the other noise. See why the split would be good?
>
>>
>> -Robert


Robert,

I have already added the hostbridge for family15h in this patch.

+static struct amd_hostbridge hb_probes[] __initdata = {
+	{ 0, 0x18, 0x1100 }, /* K8 */
+	{ 0, 0x18, 0x1200 }, /* Family10h */
+	{ 0xff, 0, 0x1200 }, /* Family10h */
+	{ 0, 0x18, 0x1300 }, /* Family11h */
+	{ 0, 0x18, 0x1600 }, /* Family15h */ <--- HERE
  };

The rest of the changes are mostly comments, some minor renaming of 
variables for clarity, and replace hardcode values with preprocessor 
macro.  If needed, I can split them.

Suravee
--
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
Robert Richter May 8, 2014, 3:14 p.m. UTC | #4 
On 08.05.14 09:39:55, Suravee Suthikulanit wrote:
> On 5/8/2014 4:01 AM, Robert Richter wrote:
> >On 08.05.14 10:59:05, Robert Richter wrote:
> >>On 07.05.14 13:58:45, suravee.suthikulpanit@amd.com wrote:
> >>>@@ -113,10 +122,17 @@ static int __init early_fill_mp_bus_info(void)
> >>>  		info = alloc_pci_root_info(min_bus, max_bus, node, link);
> >>>  	}
> >>>
> >>>+	/*
> >>>+	 * The following code is only supported until Fam11h.
> >>>+	 * Newer processors will depend on ACPI MCFG table instead.
> >>>+	 */
> >>>+	if (boot_cpu_data.x86 > 0x11)
> >>>+		return 0;
> >>>+
> >>>  	/* get the default node and link for left over res */
> >>
> >>As this is the only substantial change of your patch, I would better
> >>drop ther rest or at least split it in two patches. Should this change
> >>also be for stable?
> >
> >Of course adding the hostbridge must be also part of the patch, didn't
> >note this due to the other noise. See why the split would be good?
> >
> >>
> >>-Robert
> 
> 
> Robert,
> 
> I have already added the hostbridge for family15h in this patch.
> 
> +static struct amd_hostbridge hb_probes[] __initdata = {
> +	{ 0, 0x18, 0x1100 }, /* K8 */
> +	{ 0, 0x18, 0x1200 }, /* Family10h */
> +	{ 0xff, 0, 0x1200 }, /* Family10h */
> +	{ 0, 0x18, 0x1300 }, /* Family11h */
> +	{ 0, 0x18, 0x1600 }, /* Family15h */ <--- HERE

Yes, I noticed that, but later, thus my 2nd mail.

>  };
> 
> The rest of the changes are mostly comments, some minor renaming of
> variables for clarity, and replace hardcode values with preprocessor macro.
> If needed, I can split them.

I just would drop it, you just need the fam15h device and the cpu mode
check.

-Robert
--
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
Suravee Suthikulpanit May 8, 2014, 3:21 p.m. UTC | #5 
On 5/8/2014 10:14 AM, Robert Richter wrote:
> On 08.05.14 09:39:55, Suravee Suthikulanit wrote:
>> On 5/8/2014 4:01 AM, Robert Richter wrote:
>>> On 08.05.14 10:59:05, Robert Richter wrote:
>>>> On 07.05.14 13:58:45, suravee.suthikulpanit@amd.com wrote:
>>>>> @@ -113,10 +122,17 @@ static int __init early_fill_mp_bus_info(void)
>>>>>   		info = alloc_pci_root_info(min_bus, max_bus, node, link);
>>>>>   	}
>>>>>
>>>>> +	/*
>>>>> +	 * The following code is only supported until Fam11h.
>>>>> +	 * Newer processors will depend on ACPI MCFG table instead.
>>>>> +	 */
>>>>> +	if (boot_cpu_data.x86 > 0x11)
>>>>> +		return 0;
>>>>> +
>>>>>   	/* get the default node and link for left over res */
>>>>
>>>> As this is the only substantial change of your patch, I would better
>>>> drop ther rest or at least split it in two patches. Should this change
>>>> also be for stable?
>>>
>>> Of course adding the hostbridge must be also part of the patch, didn't
>>> note this due to the other noise. See why the split would be good?
>>>
>>>>
>>>> -Robert
>>
>>
>> Robert,
>>
>> I have already added the hostbridge for family15h in this patch.
>>
>> +static struct amd_hostbridge hb_probes[] __initdata = {
>> +	{ 0, 0x18, 0x1100 }, /* K8 */
>> +	{ 0, 0x18, 0x1200 }, /* Family10h */
>> +	{ 0xff, 0, 0x1200 }, /* Family10h */
>> +	{ 0, 0x18, 0x1300 }, /* Family11h */
>> +	{ 0, 0x18, 0x1600 }, /* Family15h */ <--- HERE
>
> Yes, I noticed that, but later, thus my 2nd mail.
>
>>   };
>>
>> The rest of the changes are mostly comments, some minor renaming of
>> variables for clarity, and replace hardcode values with preprocessor macro.
>> If needed, I can split them.
>
> I just would drop it, you just need the fam15h device and the cpu mode
> check.
>
> -Robert
>
The reason I put it all these comments here is because it took us a 
while to discuss what to do with this file going forward. There were 
some confusions.  Therefore, I just want to document it here.

Also, the check for (boot_cpu_data.x86 > 0x11) was needed because it 
should not be done for family15h.

Suravee
--
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
Robert Richter May 8, 2014, 3:37 p.m. UTC | #6 
On 08.05.14 10:21:07, Suravee Suthikulanit wrote:
> The reason I put it all these comments here is because it took us a while to
> discuss what to do with this file going forward. There were some confusions.
> Therefore, I just want to document it here.
> 
> Also, the check for (boot_cpu_data.x86 > 0x11) was needed because it should
> not be done for family15h.

Yes, the only functional change of this patch is adding the bridge and
the family check, right? Basically:

+       { 0, 0x18, PCI_VENDOR_ID_AMD, 0x1600 },

and

+        /*
+         * The following code is only supported until Fam11h.
+         * Newer processors will depend on ACPI MCFG table instead.
+         */
+        if (boot_cpu_data.x86 > 0x11)
+                return 0;
+

This patch should stripped down to only those changes with a
split. And maybe this should be added to linux-stable?

All other rework is a different story... Can be done on top of this,
though I would drop it.

-Robert
--
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
Myron Stowe May 8, 2014, 4:22 p.m. UTC | #7 
On Thu, May 8, 2014 at 9:37 AM, Robert Richter <rric@kernel.org> wrote:
> On 08.05.14 10:21:07, Suravee Suthikulanit wrote:
>> The reason I put it all these comments here is because it took us a while to
>> discuss what to do with this file going forward. There were some confusions.
>> Therefore, I just want to document it here.

I agree.  Hopefully anyone getting into this in the future will be
able to find this thread in the archives as it has been enlightning

>>
>> Also, the check for (boot_cpu_data.x86 > 0x11) was needed because it should
>> not be done for family15h.
>
> Yes, the only functional change of this patch is adding the bridge and
> the family check, right? Basically:
>
> +       { 0, 0x18, PCI_VENDOR_ID_AMD, 0x1600 },
>
> and
>
> +        /*
> +         * The following code is only supported until Fam11h.
> +         * Newer processors will depend on ACPI MCFG table instead.
> +         */
> +        if (boot_cpu_data.x86 > 0x11)
> +                return 0;
> +
>
> This patch should stripped down to only those changes with a
> split. And maybe this should be added to linux-stable?
>
> All other rework is a different story... Can be done on top of this,
> though I would drop it.

I understand Robert's reasoning to split out the core changes (as
denoted above).  However, I *would* tend to follow on with an
additional subsequent patch that has the rest of the content as it
cleans this area up (as opposed to dropping it all together).  It
makes the code a little clearer and is basically little to no risk (no
functional change).

Myron
>
> -Robert
> --
> 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
--
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 mbox

Patch

diff --git a/arch/x86/pci/amd_bus.c b/arch/x86/pci/amd_bus.c
index e88f4c5..7c251c2 100644
--- a/arch/x86/pci/amd_bus.c
+++ b/arch/x86/pci/amd_bus.c
@@ -11,27 +11,33 @@ 
 
 #include "bus_numa.h"
 
-/*
- * This discovers the pcibus <-> node mapping on AMD K8.
- * also get peer root bus resource for io,mmio
- */
+#define AMD_NB_F0_NODE_ID			0x60
+#define AMD_NB_F0_UNIT_ID			0x64
+#define AMD_NB_F1_CONFIG_MAP_REG		0xe0
 
-struct pci_hostbridge_probe {
+#define RANGE_NUM				16
+#define AMD_NB_F1_CONFIG_MAP_RANGES		4
+
+struct amd_hostbridge {
 	u32 bus;
 	u32 slot;
-	u32 vendor;
 	u32 device;
 };
 
-static struct pci_hostbridge_probe pci_probes[] __initdata = {
-	{ 0, 0x18, PCI_VENDOR_ID_AMD, 0x1100 },
-	{ 0, 0x18, PCI_VENDOR_ID_AMD, 0x1200 },
-	{ 0xff, 0, PCI_VENDOR_ID_AMD, 0x1200 },
-	{ 0, 0x18, PCI_VENDOR_ID_AMD, 0x1300 },
+/*
+ * IMPORTANT NOTE:
+ * hb_probes[] and early_root_info_init() is in maintenance mode.
+ * It only supports K8, Fam10h, Fam11h, and Fam15h_00h-0fh .
+ * Future processor will rely on information in ACPI.
+ */
+static struct amd_hostbridge hb_probes[] __initdata = {
+	{ 0, 0x18, 0x1100 }, /* K8 */
+	{ 0, 0x18, 0x1200 }, /* Family10h */
+	{ 0xff, 0, 0x1200 }, /* Family10h */
+	{ 0, 0x18, 0x1300 }, /* Family11h */
+	{ 0, 0x18, 0x1600 }, /* Family15h */
 };
 
-#define RANGE_NUM 16
-
 static struct pci_root_info __init *find_pci_root_info(int node, int link)
 {
 	struct pci_root_info *info;
@@ -45,12 +51,12 @@  static struct pci_root_info __init *find_pci_root_info(int node, int link)
 }
 
 /**
- * early_fill_mp_bus_to_node()
+ * early_root_info_init()
  * called before pcibios_scan_root and pci_scan_bus
- * fills the mp_bus_to_cpumask array based according to the LDT Bus Number
- * Registers found in the K8 northbridge
+ * fills the mp_bus_to_cpumask array based according
+ * to the LDT Bus Number Registers found in the northbridge.
  */
-static int __init early_fill_mp_bus_info(void)
+static int __init early_root_info_init(void)
 {
 	int i;
 	unsigned bus;
@@ -75,19 +81,21 @@  static int __init early_fill_mp_bus_info(void)
 		return -1;
 
 	found = false;
-	for (i = 0; i < ARRAY_SIZE(pci_probes); i++) {
+	for (i = 0; i < ARRAY_SIZE(hb_probes); i++) {
 		u32 id;
 		u16 device;
 		u16 vendor;
 
-		bus = pci_probes[i].bus;
-		slot = pci_probes[i].slot;
+		bus = hb_probes[i].bus;
+		slot = hb_probes[i].slot;
 		id = read_pci_config(bus, slot, 0, PCI_VENDOR_ID);
-
 		vendor = id & 0xffff;
 		device = (id>>16) & 0xffff;
-		if (pci_probes[i].vendor == vendor &&
-		    pci_probes[i].device == device) {
+
+		if (vendor != PCI_VENDOR_ID_AMD)
+			continue;
+
+		if (hb_probes[i].device == device) {
 			found = true;
 			break;
 		}
@@ -96,10 +104,11 @@  static int __init early_fill_mp_bus_info(void)
 	if (!found)
 		return 0;
 
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < AMD_NB_F1_CONFIG_MAP_RANGES; i++) {
 		int min_bus;
 		int max_bus;
-		reg = read_pci_config(bus, slot, 1, 0xe0 + (i << 2));
+		reg = read_pci_config(bus, slot, 1,
+				AMD_NB_F1_CONFIG_MAP_REG + (i << 2));
 
 		/* Check if that register is enabled for bus range */
 		if ((reg & 7) != 3)
@@ -113,10 +122,17 @@  static int __init early_fill_mp_bus_info(void)
 		info = alloc_pci_root_info(min_bus, max_bus, node, link);
 	}
 
+	/*
+	 * The following code is only supported until Fam11h.
+	 * Newer processors will depend on ACPI MCFG table instead.
+	 */
+	if (boot_cpu_data.x86 > 0x11)
+		return 0;
+
 	/* get the default node and link for left over res */
-	reg = read_pci_config(bus, slot, 0, 0x60);
+	reg = read_pci_config(bus, slot, 0, AMD_NB_F0_NODE_ID);
 	def_node = (reg >> 8) & 0x07;
-	reg = read_pci_config(bus, slot, 0, 0x64);
+	reg = read_pci_config(bus, slot, 0, AMD_NB_F0_UNIT_ID);
 	def_link = (reg >> 8) & 0x03;
 
 	memset(range, 0, sizeof(range));
@@ -363,7 +379,7 @@  static int __init pci_io_ecs_init(void)
 	int cpu;
 
 	/* assume all cpus from fam10h have IO ECS */
-        if (boot_cpu_data.x86 < 0x10)
+	if (boot_cpu_data.x86 < 0x10)
 		return 0;
 
 	/* Try the PCI method first. */
@@ -387,7 +403,8 @@  static int __init amd_postcore_init(void)
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
 		return 0;
 
-	early_fill_mp_bus_info();
+	early_root_info_init();
+
 	pci_io_ecs_init();
 
 	return 0;