[PATCH] Detect mmconfig on nVidia MCP55

[PATCH] Detect mmconfig on nVidia MCP55

[Ed Swierk]

Detect and enable memory-mapped PCI configuration space on the nVidia
MCP55 southbridge even if the ACPI MCFG table is missing or wrong.

Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>

---
Index: linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c
===================================================================
--- linux-2.6.27.4.orig/arch/x86/pci/mmconfig-shared.c
+++ linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c
@@ -155,6 +155,26 @@ static const char __init *pci_mmcfg_amd_
 	return "AMD Family 10h NB";
 }
 
+static const char __init *pci_mmcfg_nvidia_mcp55(void)
+{
+	u32 extcfg;
+
+	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0x90, 4, &extcfg);
+
+	if (!(extcfg & 0x80000000))
+		return NULL;
+	pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]), GFP_KERNEL);
+	if (!pci_mmcfg_config)
+		return NULL;
+	pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;
+	pci_mmcfg_config[0].pci_segment = 0;
+	pci_mmcfg_config[0].start_bus_number = 0;
+	pci_mmcfg_config[0].end_bus_number = (1 << (8 - ((extcfg >> 28) & 3))) - 1;
+	pci_mmcfg_config_num = 1;
+
+	return "nVidia MCP55";
+}
+
 struct pci_mmcfg_hostbridge_probe {
 	u32 bus;
 	u32 devfn;
@@ -172,6 +192,8 @@ static struct pci_mmcfg_hostbridge_probe
 	  0x1200, pci_mmcfg_amd_fam10h },
 	{ 0xff, PCI_DEVFN(0, 0), PCI_VENDOR_ID_AMD,
 	  0x1200, pci_mmcfg_amd_fam10h },
+	{ 0, PCI_DEVFN(0, 0), PCI_VENDOR_ID_NVIDIA,
+	  0x0369, pci_mmcfg_nvidia_mcp55 },
 };
 
 static int __init pci_mmcfg_check_hostbridge(void)

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Ingo Molnar]

* Ed Swierk <eswierk@aristanetworks.com> wrote:

> +static const char __init *pci_mmcfg_nvidia_mcp55(void)
> +{
> +	u32 extcfg;
> +
> +	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0x90, 4, &extcfg);
> +
> +	if (!(extcfg & 0x80000000))
> +		return NULL;
> +	pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]), GFP_KERNEL);
> +	if (!pci_mmcfg_config)
> +		return NULL;
> +	pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;
> +	pci_mmcfg_config[0].pci_segment = 0;
> +	pci_mmcfg_config[0].start_bus_number = 0;
> +	pci_mmcfg_config[0].end_bus_number = (1 << (8 - ((extcfg >> 28) & 3))) - 1;
> +	pci_mmcfg_config_num = 1;
> +
> +	return "nVidia MCP55";

Looks good in principle, but here are a few code cleanliness observations:

1) Please introduce a helper variable to:

	struct acpi_mcfg_allocation *config;

   Which you can allocate, initialize - and then set pci_mmcfg_config to it. 
   Does not change the end result but makes the code structure cleaner and 
   shortens the lines.

2) Please use vertical spaces when initializing structure fields. Instead of 
   the messy looking (and over-long-line generating) construct of:

	pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;
	pci_mmcfg_config[0].pci_segment = 0;
	pci_mmcfg_config[0].start_bus_number = 0;
	pci_mmcfg_config[0].end_bus_number = (1 << (8 - ((extcfg >> 28) & 3))) - 1;
	pci_mmcfg_config_num = 1;

   You will get something like:

	config->address			= (extcfg & 0x00007fff) << 25;
	config->pci_segment		= 0;
	config->start_bus_number	= 0;
	config->end_bus_number		= (1 << (8 - ((extcfg >> 28) & 3)));

	pci_mmcfg_config = config;
	pci_mmcfg_config_num = 1;

   Which makes it more structured, more reviewable - and more pleasant to 
   look at as well.

3) Please use newlines in a more structured way, instead of:

	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0x90, 4, &extcfg);

	if (!(extcfg & 0x80000000))
		return NULL;
	pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]), GFP_KERNEL);
	if (!pci_mmcfg_config)
		return NULL;
	pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;

   Do something like:

	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0x90, 4, &extcfg);
	if (!(extcfg & 0x80000000))
		return NULL;

	config = kzalloc(sizeof(*config), GFP_KERNEL);
	if (!config)
		return NULL;

	config->address			= (extcfg & 0x00007fff) << 25;

   Note how the grouping of statements made the code flow really apparent 
   and straightforward, and made the returns and error conditions stand out.

4) Also, there's 6 magic constants in this patch:

      0x90, 4, 0x80000000, 0x00007fff, 28, 3

   Wherever you know already existing symbols for these in the PCI code 
   please use them - and where not, add a const u32 to introduce them.

   For example 0x80000000 means 'mmconf already enabled' - a suitable 
   symbol should be used. That way the code becomes self-explanatory at a 
   glance - even years down the line when everyone has forgotten what it's 
   all about.

   If a constant is very specific to the nVidia MCP55 chipset register 
   layout, you can define it straight before the pci_mmcfg_nvidia_mcp55() 
   function, no need to separate the definitions from the function by moving 
   it into some header file.

   Also, if possible and if it exists, add information (URLs if they exist) 
   about the chipset documentation that was the basis for this change.

5) Finally, some testing info and motivation-for-change info would be nice 
   as well. Which specific system/model encountered problems with 
   non-enabled mmconfig, and did it work well after you enabled it via this 
   chipset quirk?

Thanks!

	Ingo

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Loic Prylli]

On 02/04/2009 11:39 AM, Ed Swierk wrote:
> Detect and enable memory-mapped PCI configuration space on the nVidia
> MCP55 southbridge even if the ACPI MCFG table is missing or wrong.
>
>   


Minor question: there are motherboards with two MCP55 southbridges. 
Would the patch support mmconfig for both?


Loic

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Ed Swierk]

On Wed, Feb 4, 2009 at 9:07 AM, Loic Prylli <loic@myri.com> wrote:
> Minor question: there are motherboards with two MCP55 southbridges. Would
> the patch support mmconfig for both?

Can you point me to such a motherboard? I have only seen boards with
one MCP55 plus a C51 companion chip providing extra PCIe lanes.

--Ed

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Yinghai Lu]

On Wed, Feb 4, 2009 at 9:37 AM, Ed Swierk <eswierk@aristanetworks.com> wrote:
> On Wed, Feb 4, 2009 at 9:07 AM, Loic Prylli <loic@myri.com> wrote:
>> Minor question: there are motherboards with two MCP55 southbridges. Would
>> the patch support mmconfig for both?
>
> Can you point me to such a motherboard? I have only seen boards with
> one MCP55 plus a C51 companion chip providing extra PCIe lanes.
>
nvidia CRB

YH

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Yinghai Lu]

On Wed, Feb 4, 2009 at 8:39 AM, Ed Swierk <eswierk@aristanetworks.com> wrote:
> Detect and enable memory-mapped PCI configuration space on the nVidia
> MCP55 southbridge even if the ACPI MCFG table is missing or wrong.
>
> Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>
>
> ---
> Index: linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c
> ===================================================================
> --- linux-2.6.27.4.orig/arch/x86/pci/mmconfig-shared.c
> +++ linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c
> @@ -155,6 +155,26 @@ static const char __init *pci_mmcfg_amd_
>        return "AMD Family 10h NB";
>  }
>
> +static const char __init *pci_mmcfg_nvidia_mcp55(void)
> +{
> +       u32 extcfg;
> +
> +       raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0x90, 4, &extcfg);
> +
> +       if (!(extcfg & 0x80000000))
> +               return NULL;
> +       pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]), GFP_KERNEL);
> +       if (!pci_mmcfg_config)
> +               return NULL;
> +       pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;
> +       pci_mmcfg_config[0].pci_segment = 0;
> +       pci_mmcfg_config[0].start_bus_number = 0;
> +       pci_mmcfg_config[0].end_bus_number = (1 << (8 - ((extcfg >> 28) & 3))) - 1;
> +       pci_mmcfg_config_num = 1;
> +
> +       return "nVidia MCP55";
> +}
> +
>  struct pci_mmcfg_hostbridge_probe {
>        u32 bus;
>        u32 devfn;
> @@ -172,6 +192,8 @@ static struct pci_mmcfg_hostbridge_probe
>          0x1200, pci_mmcfg_amd_fam10h },
>        { 0xff, PCI_DEVFN(0, 0), PCI_VENDOR_ID_AMD,
>          0x1200, pci_mmcfg_amd_fam10h },
> +       { 0, PCI_DEVFN(0, 0), PCI_VENDOR_ID_NVIDIA,
> +         0x0369, pci_mmcfg_nvidia_mcp55 },

may break amd family 10h + mcp55 system. because some system have
setting in AMD cpu and mcp55
it will find setting in CPU nb, and your code will partially overwrite
those setting.

YH

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> On Wed, Feb 4, 2009 at 8:39 AM, Ed Swierk <eswierk@aristanetworks.com> wrote:
> > Detect and enable memory-mapped PCI configuration space on the nVidia
> > MCP55 southbridge even if the ACPI MCFG table is missing or wrong.
> >
> > Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>
> >
> > ---
> > Index: linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c
> > ===================================================================
> > --- linux-2.6.27.4.orig/arch/x86/pci/mmconfig-shared.c
> > +++ linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c
> > @@ -155,6 +155,26 @@ static const char __init *pci_mmcfg_amd_
> >        return "AMD Family 10h NB";
> >  }
> >
> > +static const char __init *pci_mmcfg_nvidia_mcp55(void)
> > +{
> > +       u32 extcfg;
> > +
> > +       raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0x90, 4, &extcfg);
> > +
> > +       if (!(extcfg & 0x80000000))
> > +               return NULL;
> > +       pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]), GFP_KERNEL);
> > +       if (!pci_mmcfg_config)
> > +               return NULL;
> > +       pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;
> > +       pci_mmcfg_config[0].pci_segment = 0;
> > +       pci_mmcfg_config[0].start_bus_number = 0;
> > +       pci_mmcfg_config[0].end_bus_number = (1 << (8 - ((extcfg >> 28) & 3))) - 1;
> > +       pci_mmcfg_config_num = 1;
> > +
> > +       return "nVidia MCP55";
> > +}
> > +
> >  struct pci_mmcfg_hostbridge_probe {
> >        u32 bus;
> >        u32 devfn;
> > @@ -172,6 +192,8 @@ static struct pci_mmcfg_hostbridge_probe
> >          0x1200, pci_mmcfg_amd_fam10h },
> >        { 0xff, PCI_DEVFN(0, 0), PCI_VENDOR_ID_AMD,
> >          0x1200, pci_mmcfg_amd_fam10h },
> > +       { 0, PCI_DEVFN(0, 0), PCI_VENDOR_ID_NVIDIA,
> > +         0x0369, pci_mmcfg_nvidia_mcp55 },
> 
> may break amd family 10h + mcp55 system. because some system have setting 
> in AMD cpu and mcp55 it will find setting in CPU nb, and your code will 
> partially overwrite those setting.

That's a good point ...

We could do something like:

	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
		boot_cpu_data.x86 >= 0x10) {
		u64 mmconf_val;

		rdmsrl(MSR_FAM10H_MMIO_CONF_BASE, mmconf_val);
		if (mmconf_val & FAM10H_MMIO_CONF_ENABLE)
			return;
	}

To detect enabled on-CPU mmconf support. We could even put this into a 
helper function as other places might want to use it too.

	Ingo

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Yinghai Lu]

Ingo Molnar wrote:
> 
> To detect enabled on-CPU mmconf support. We could even put this into a 
> helper function as other places might want to use it too.
> 

sth like:

[PATCH] x86/pci: host mmconfig detect clean up

Impact: not assume one place for mmconfig in nb

prepare for following case: amd fam10h + mcp55

CPU MSR has some range, mcp55 pci config will have another one.
in some case: cpu MSR only include bus 0, and leave other to mcp55.

if it s mcp55 detect duties to execlude range that is used by CPU MSR
aka, if CPU state bus 0-255, range in mcp55 need to be dropped.
because HW in CPU will not route that mcp55 mmconfig to handle it.

Signed-off-by: Yinghai Lu <yinghai.lu@kernel.org>

---
 arch/x86/pci/mmconfig-shared.c |   90 ++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 40 deletions(-)

Index: linux-2.6/arch/x86/pci/mmconfig-shared.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/mmconfig-shared.c
+++ linux-2.6/arch/x86/pci/mmconfig-shared.c
@@ -24,23 +24,47 @@
 /* Indicate if the mmcfg resources have been placed into the resource table. */
 static int __initdata pci_mmcfg_resources_inserted;
 
+static __init int extend_mmcfg(int num)
+{
+	struct acpi_mcfg_allocation *new;
+	int new_num = pci_mmcfg_config_num + num;
+
+	new = kzalloc(sizeof(pci_mmcfg_config[0]) * new_num, GFP_KERNEL);
+	if (!new)
+		return -1;
+
+	if (pci_mmcfg_config) {
+		memcpy(new, pci_mmcfg_config,
+			 (sizeof(pci_mmcfg_config[0]) * new_num));
+		kfree(pci_mmcfg_config);
+	}
+	pci_mmcfg_config = new;
+
+	return 0;
+}
+
+static __init void fill_one_mmcfg(u64 addr, int segment, int start, int end)
+{
+	int i = pci_mmcfg_config_num;
+
+	pci_mmcfg_config_num++;
+	pci_mmcfg_config[i].address = addr;
+	pci_mmcfg_config[i].pci_segment = segment;
+	pci_mmcfg_config[i].start_bus_number = start;
+	pci_mmcfg_config[i].end_bus_number = end;
+}
+
 static const char __init *pci_mmcfg_e7520(void)
 {
 	u32 win;
 	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0xce, 2, &win);
 
 	win = win & 0xf000;
-	if(win == 0x0000 || win == 0xf000)
-		pci_mmcfg_config_num = 0;
-	else {
-		pci_mmcfg_config_num = 1;
-		pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]), GFP_KERNEL);
-		if (!pci_mmcfg_config)
+	if (win != 0x0000 && win != 0xf000) {
+		if (extend_mmcfg(1) == -1)
 			return NULL;
-		pci_mmcfg_config[0].address = win << 16;
-		pci_mmcfg_config[0].pci_segment = 0;
-		pci_mmcfg_config[0].start_bus_number = 0;
-		pci_mmcfg_config[0].end_bus_number = 255;
+
+		fill_one_mmcfg(win << 16, 0, 0, 255);
 	}
 
 	return "Intel Corporation E7520 Memory Controller Hub";
@@ -50,13 +74,11 @@ static const char __init *pci_mmcfg_inte
 {
 	u32 pciexbar, mask = 0, len = 0;
 
-	pci_mmcfg_config_num = 1;
-
 	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0x48, 4, &pciexbar);
 
 	/* Enable bit */
 	if (!(pciexbar & 1))
-		pci_mmcfg_config_num = 0;
+		return NULL;
 
 	/* Size bits */
 	switch ((pciexbar >> 1) & 3) {
@@ -73,28 +95,23 @@ static const char __init *pci_mmcfg_inte
 		len  = 0x04000000U;
 		break;
 	default:
-		pci_mmcfg_config_num = 0;
+		return NULL;
 	}
 
 	/* Errata #2, things break when not aligned on a 256Mb boundary */
 	/* Can only happen in 64M/128M mode */
 
 	if ((pciexbar & mask) & 0x0fffffffU)
-		pci_mmcfg_config_num = 0;
+		return NULL;
 
 	/* Don't hit the APIC registers and their friends */
 	if ((pciexbar & mask) >= 0xf0000000U)
-		pci_mmcfg_config_num = 0;
+		return NULL;
 
-	if (pci_mmcfg_config_num) {
-		pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]), GFP_KERNEL);
-		if (!pci_mmcfg_config)
-			return NULL;
-		pci_mmcfg_config[0].address = pciexbar & mask;
-		pci_mmcfg_config[0].pci_segment = 0;
-		pci_mmcfg_config[0].start_bus_number = 0;
-		pci_mmcfg_config[0].end_bus_number = (len >> 20) - 1;
-	}
+	if (extend_mmcfg(1) == -1)
+		return NULL;
+
+	fill_one_mmcfg(pciexbar & mask, 0, 0, (len >> 20) - 1);
 
 	return "Intel Corporation 945G/GZ/P/PL Express Memory Controller Hub";
 }
@@ -138,18 +155,12 @@ static const char __init *pci_mmcfg_amd_
 		busnbits = 8;
 	}
 
-	pci_mmcfg_config_num = (1 << segnbits);
-	pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]) *
-				   pci_mmcfg_config_num, GFP_KERNEL);
-	if (!pci_mmcfg_config)
+	if (extend_mmcfg(1 << segnbits) == -1)
 		return NULL;
 
-	for (i = 0; i < (1 << segnbits); i++) {
-		pci_mmcfg_config[i].address = base + (1<<28) * i;
-		pci_mmcfg_config[i].pci_segment = i;
-		pci_mmcfg_config[i].start_bus_number = 0;
-		pci_mmcfg_config[i].end_bus_number = (1 << busnbits) - 1;
-	}
+
+	for (i = 0; i < (1 << segnbits); i++)
+		fill_one_mmcfg(base + (1<<28) * i, i, 0, (1 << busnbits) - 1);
 
 	return "AMD Family 10h NB";
 }
@@ -198,14 +209,13 @@ static int __init pci_mmcfg_check_hostbr
 		if (pci_mmcfg_probes[i].vendor == vendor &&
 		    pci_mmcfg_probes[i].device == device)
 			name = pci_mmcfg_probes[i].probe();
-	}
 
-	if (name) {
-		printk(KERN_INFO "PCI: Found %s %s MMCONFIG support.\n",
-		       name, pci_mmcfg_config_num ? "with" : "without");
+		if (name)
+			printk(KERN_INFO "PCI: Found %s with MMCONFIG support.\n",
+			       name);
 	}
 
-	return name != NULL;
+	return pci_mmcfg_config_num != 0;
 }
 
 static void __init pci_mmcfg_insert_resources(void)

[PATCH] x86/pci: host mmconfig detect clean up v2

[Yinghai Lu]

Impact: not assume one place for mmconfig in nb

prepare for following case: amd fam10h + mcp55
CPU MSR has some range, mcp55 pci config will have another one.

also prepare for mcp55 + io55 system. every one will have one range.

if it s mcp55 detect duties to execlude range that is used by CPU MSR
aka, if CPU state bus 0-255, range in mcp55 need to be dropped.
because HW in CPU will not route that mcp55 mmconfig to handle it.

v2: fix e7520 exit path

Signed-off-by: Yinghai Lu <yinghai.lu@kernel.org>

---
 arch/x86/pci/mmconfig-shared.c |   98 ++++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 44 deletions(-)

Index: linux-2.6/arch/x86/pci/mmconfig-shared.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/mmconfig-shared.c
+++ linux-2.6/arch/x86/pci/mmconfig-shared.c
@@ -24,24 +24,49 @@
 /* Indicate if the mmcfg resources have been placed into the resource table. */
 static int __initdata pci_mmcfg_resources_inserted;
 
+static __init int extend_mmcfg(int num)
+{
+	struct acpi_mcfg_allocation *new;
+	int new_num = pci_mmcfg_config_num + num;
+
+	new = kzalloc(sizeof(pci_mmcfg_config[0]) * new_num, GFP_KERNEL);
+	if (!new)
+		return -1;
+
+	if (pci_mmcfg_config) {
+		memcpy(new, pci_mmcfg_config,
+			 sizeof(pci_mmcfg_config[0]) * new_num);
+		kfree(pci_mmcfg_config);
+	}
+	pci_mmcfg_config = new;
+
+	return 0;
+}
+
+static __init void fill_one_mmcfg(u64 addr, int segment, int start, int end)
+{
+	int i = pci_mmcfg_config_num;
+
+	pci_mmcfg_config_num++;
+	pci_mmcfg_config[i].address = addr;
+	pci_mmcfg_config[i].pci_segment = segment;
+	pci_mmcfg_config[i].start_bus_number = start;
+	pci_mmcfg_config[i].end_bus_number = end;
+}
+
 static const char __init *pci_mmcfg_e7520(void)
 {
 	u32 win;
 	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0xce, 2, &win);
 
 	win = win & 0xf000;
-	if(win == 0x0000 || win == 0xf000)
-		pci_mmcfg_config_num = 0;
-	else {
-		pci_mmcfg_config_num = 1;
-		pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]), GFP_KERNEL);
-		if (!pci_mmcfg_config)
-			return NULL;
-		pci_mmcfg_config[0].address = win << 16;
-		pci_mmcfg_config[0].pci_segment = 0;
-		pci_mmcfg_config[0].start_bus_number = 0;
-		pci_mmcfg_config[0].end_bus_number = 255;
-	}
+	if (win == 0x0000 || win == 0xf000)
+		return NULL;
+
+	if (extend_mmcfg(1) == -1)
+		return NULL;
+
+	fill_one_mmcfg(win << 16, 0, 0, 255);
 
 	return "Intel Corporation E7520 Memory Controller Hub";
 }
@@ -50,13 +75,11 @@ static const char __init *pci_mmcfg_inte
 {
 	u32 pciexbar, mask = 0, len = 0;
 
-	pci_mmcfg_config_num = 1;
-
 	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0x48, 4, &pciexbar);
 
 	/* Enable bit */
 	if (!(pciexbar & 1))
-		pci_mmcfg_config_num = 0;
+		return NULL;
 
 	/* Size bits */
 	switch ((pciexbar >> 1) & 3) {
@@ -73,28 +96,23 @@ static const char __init *pci_mmcfg_inte
 		len  = 0x04000000U;
 		break;
 	default:
-		pci_mmcfg_config_num = 0;
+		return NULL;
 	}
 
 	/* Errata #2, things break when not aligned on a 256Mb boundary */
 	/* Can only happen in 64M/128M mode */
 
 	if ((pciexbar & mask) & 0x0fffffffU)
-		pci_mmcfg_config_num = 0;
+		return NULL;
 
 	/* Don't hit the APIC registers and their friends */
 	if ((pciexbar & mask) >= 0xf0000000U)
-		pci_mmcfg_config_num = 0;
+		return NULL;
 
-	if (pci_mmcfg_config_num) {
-		pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]), GFP_KERNEL);
-		if (!pci_mmcfg_config)
-			return NULL;
-		pci_mmcfg_config[0].address = pciexbar & mask;
-		pci_mmcfg_config[0].pci_segment = 0;
-		pci_mmcfg_config[0].start_bus_number = 0;
-		pci_mmcfg_config[0].end_bus_number = (len >> 20) - 1;
-	}
+	if (extend_mmcfg(1) == -1)
+		return NULL;
+
+	fill_one_mmcfg(pciexbar & mask, 0, 0, (len >> 20) - 1);
 
 	return "Intel Corporation 945G/GZ/P/PL Express Memory Controller Hub";
 }
@@ -138,18 +156,11 @@ static const char __init *pci_mmcfg_amd_
 		busnbits = 8;
 	}
 
-	pci_mmcfg_config_num = (1 << segnbits);
-	pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]) *
-				   pci_mmcfg_config_num, GFP_KERNEL);
-	if (!pci_mmcfg_config)
+	if (extend_mmcfg(1 << segnbits) == -1)
 		return NULL;
 
-	for (i = 0; i < (1 << segnbits); i++) {
-		pci_mmcfg_config[i].address = base + (1<<28) * i;
-		pci_mmcfg_config[i].pci_segment = i;
-		pci_mmcfg_config[i].start_bus_number = 0;
-		pci_mmcfg_config[i].end_bus_number = (1 << busnbits) - 1;
-	}
+	for (i = 0; i < (1 << segnbits); i++)
+		fill_one_mmcfg(base + (1<<28) * i, i, 0, (1 << busnbits) - 1);
 
 	return "AMD Family 10h NB";
 }
@@ -186,26 +197,25 @@ static int __init pci_mmcfg_check_hostbr
 
 	pci_mmcfg_config_num = 0;
 	pci_mmcfg_config = NULL;
-	name = NULL;
 
-	for (i = 0; !name && i < ARRAY_SIZE(pci_mmcfg_probes); i++) {
+	for (i = 0; i < ARRAY_SIZE(pci_mmcfg_probes); i++) {
 		bus =  pci_mmcfg_probes[i].bus;
 		devfn = pci_mmcfg_probes[i].devfn;
 		raw_pci_ops->read(0, bus, devfn, 0, 4, &l);
 		vendor = l & 0xffff;
 		device = (l >> 16) & 0xffff;
 
+		name = NULL;
 		if (pci_mmcfg_probes[i].vendor == vendor &&
 		    pci_mmcfg_probes[i].device == device)
 			name = pci_mmcfg_probes[i].probe();
-	}
 
-	if (name) {
-		printk(KERN_INFO "PCI: Found %s %s MMCONFIG support.\n",
-		       name, pci_mmcfg_config_num ? "with" : "without");
+		if (name)
+			printk(KERN_INFO "PCI: Found %s with MMCONFIG support.\n",
+			       name);
 	}
 
-	return name != NULL;
+	return pci_mmcfg_config_num != 0;
 }
 
 static void __init pci_mmcfg_insert_resources(void)

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Tvrtko Ursulin]

On Wednesday 04 February 2009 17:04:40 Ingo Molnar wrote:
> 2) Please use vertical spaces when initializing structure fields. Instead
> of the messy looking (and over-long-line generating) construct of:
>
>         pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;
>         pci_mmcfg_config[0].pci_segment = 0;
>         pci_mmcfg_config[0].start_bus_number = 0;
>         pci_mmcfg_config[0].end_bus_number = (1 << (8 - ((extcfg >> 28) &
> 3))) - 1; pci_mmcfg_config_num = 1;
>
>    You will get something like:
>
>         config->address                 = (extcfg & 0x00007fff) << 25;
>         config->pci_segment             = 0;
>         config->start_bus_number        = 0;
>         config->end_bus_number          = (1 << (8 - ((extcfg >> 28) &
> 3)));
>
>         pci_mmcfg_config = config;
>         pci_mmcfg_config_num = 1;
>
>    Which makes it more structured, more reviewable - and more pleasant to
>    look at as well.

Is this in CodingStyle now? Since I have noticed you are pushing for this 
quite a lot lately. And only for structure initialisation and not also for 
example for variable declarations?

I find it a matter of personal preference whether it is more pleasant to look 
at and whether it is more or less readable.

It is also worse from diff/patch point of view since it can happen in the 
future that what would be a one line change now becomes multiline. 

Regards,

Tvrtko

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Ingo Molnar]

* Tvrtko Ursulin <tvrtko.ursulin@sophos.com> wrote:

> On Wednesday 04 February 2009 17:04:40 Ingo Molnar wrote:
> > 2) Please use vertical spaces when initializing structure fields. Instead
> > of the messy looking (and over-long-line generating) construct of:
> >
> >         pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;
> >         pci_mmcfg_config[0].pci_segment = 0;
> >         pci_mmcfg_config[0].start_bus_number = 0;
> >         pci_mmcfg_config[0].end_bus_number = (1 << (8 - ((extcfg >> 28) &
> > 3))) - 1; pci_mmcfg_config_num = 1;
> >
> >    You will get something like:
> >
> >         config->address                 = (extcfg & 0x00007fff) << 25;
> >         config->pci_segment             = 0;
> >         config->start_bus_number        = 0;
> >         config->end_bus_number          = (1 << (8 - ((extcfg >> 28) &
> > 3)));
> >
> >         pci_mmcfg_config = config;
> >         pci_mmcfg_config_num = 1;
> >
> >    Which makes it more structured, more reviewable - and more pleasant to
> >    look at as well.

It is arch/x86/ and scheduler / etc. policy for new code - and we follow 
that principle when we clean up code as well.

Same goes for static structure initializers. We do:

 static const struct file_operations perf_fops = {
	.release		= perf_release,
	.read			= perf_read,
	.poll			= terf_poll,
	.unlocked_ioctl		= perf_ioctl,
	.compat_ioctl		= perf_ioctl,
 };

And not:

 static const struct file_operations perf_fops = {
	.release = perf_release,
	.read = perf_read,
	.poll = perf_poll,
	.unlocked_ioctl	= perf_ioctl,
	.compat_ioctl = pert_ioctl,
 };

Beyond the prettiness and fun to look at factor, there are other advantages 
of vertical spaces as well:

Trivia: you play the role of the code reviewer. I have hidden two typos in 
the initializers above, one in each initializer block. The typos are of the 
same type but are in difference places so both need to be found 
individually.

Try to find the typos, and record the number of seconds it took for you to 
find them. Which typo took less time to find?

I'd suspect that for most people block#1 is the winner.

[ Note, you have a look at it as real source code with fixed width fonts and 
  you'll see the difference. You seem to be using KMail or so: there's weird 
  line wraps and other corruption of the code in your quote above - have you 
  really looked at the real code how it looks like to developers? ]

> I find it a matter of personal preference whether it is more pleasant to 
> look at and whether it is more or less readable.

Is your argument that to you it is less pleasant to look at?

Differences in taste is OK in borderline issues, but i think this one is far 
from being borderline, it's a basic typographic and aesthetic principle.

I'm not sure vertical spaces can be properly described via more rigid 
CodingStyle rules - for example vertical spaces look ugly if there's just 
two field initializations for example - but in the above case they are 
clearly the right thing to do.

	Ingo

Re: [PATCH] x86/pci: host mmconfig detect clean up v2

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> Impact: not assume one place for mmconfig in nb
> 
> prepare for following case: amd fam10h + mcp55
> CPU MSR has some range, mcp55 pci config will have another one.
> 
> also prepare for mcp55 + io55 system. every one will have one range.
> 
> if it s mcp55 detect duties to execlude range that is used by CPU MSR
> aka, if CPU state bus 0-255, range in mcp55 need to be dropped.
> because HW in CPU will not route that mcp55 mmconfig to handle it.
> 
> v2: fix e7520 exit path
> 
> Signed-off-by: Yinghai Lu <yinghai.lu@kernel.org>
> 
> ---
>  arch/x86/pci/mmconfig-shared.c |   98 ++++++++++++++++++++++-------------------
>  1 file changed, 54 insertions(+), 44 deletions(-)

looks good to me. One for the PCI tree?

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [PATCH] x86/pci: host mmconfig detect clean up v2

[Yinghai Lu]

On Thu, Feb 5, 2009 at 10:15 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Yinghai Lu <yinghai@kernel.org> wrote:
>
>> Impact: not assume one place for mmconfig in nb
>>
>> prepare for following case: amd fam10h + mcp55
>> CPU MSR has some range, mcp55 pci config will have another one.
>>
>> also prepare for mcp55 + io55 system. every one will have one range.
>>
>> if it s mcp55 detect duties to execlude range that is used by CPU MSR
>> aka, if CPU state bus 0-255, range in mcp55 need to be dropped.
>> because HW in CPU will not route that mcp55 mmconfig to handle it.
>>
>> v2: fix e7520 exit path
>>
>> Signed-off-by: Yinghai Lu <yinghai.lu@kernel.org>
>>
>> ---
>>  arch/x86/pci/mmconfig-shared.c |   98 ++++++++++++++++++++++-------------------
>>  1 file changed, 54 insertions(+), 44 deletions(-)
>
> looks good to me. One for the PCI tree?
>
> Acked-by: Ingo Molnar <mingo@elte.hu>
>
should go with Ed's updated patch before.

to tip or pci tree.

YH

Re: [PATCH] x86/pci: host mmconfig detect clean up v2

[Ed Swierk]

On Thu, Feb 5, 2009 at 10:31 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> should go with Ed's updated patch before.
>
> to tip or pci tree.

This looks good to me, too. Thanks.

Do you want me to submit a revised patch for detecting MCP55 mmconfig?

--Ed

Re: [PATCH] x86/pci: host mmconfig detect clean up v2

[Yinghai Lu]

Ed Swierk wrote:
> On Thu, Feb 5, 2009 at 10:31 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> should go with Ed's updated patch before.
>>
>> to tip or pci tree.
> 
> This looks good to me, too. Thanks.
> 
> Do you want me to submit a revised patch for detecting MCP55 mmconfig?
> 

sure. please make sure the new one support multi mcp55. some system have mcp55 + io55 + io55.

YH

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Tvrtko Ursulin]

On Thursday 05 February 2009 18:00:19 Ingo Molnar wrote:
> * Tvrtko Ursulin <tvrtko.ursulin@sophos.com> wrote:
> > On Wednesday 04 February 2009 17:04:40 Ingo Molnar wrote:
> > > 2) Please use vertical spaces when initializing structure fields.
> > > Instead of the messy looking (and over-long-line generating) construct
> > > of:
> > >
> > >         pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;
> > >         pci_mmcfg_config[0].pci_segment = 0;
> > >         pci_mmcfg_config[0].start_bus_number = 0;
> > >         pci_mmcfg_config[0].end_bus_number = (1 << (8 - ((extcfg >> 28)
> > > & 3))) - 1; pci_mmcfg_config_num = 1;
> > >
> > >    You will get something like:
> > >
> > >         config->address                 = (extcfg & 0x00007fff) << 25;
> > >         config->pci_segment             = 0;
> > >         config->start_bus_number        = 0;
> > >         config->end_bus_number          = (1 << (8 - ((extcfg >> 28) &
> > > 3)));
> > >
> > >         pci_mmcfg_config = config;
> > >         pci_mmcfg_config_num = 1;
> > >
> > >    Which makes it more structured, more reviewable - and more pleasant
> > > to look at as well.
>
> It is arch/x86/ and scheduler / etc. policy for new code - and we follow
> that principle when we clean up code as well.

You also didn't say anything about variable declarations I asked about? And I can add structure definition to 
that question as well. 

> Beyond the prettiness and fun to look at factor, there are other advantages
> of vertical spaces as well:
>
> Trivia: you play the role of the code reviewer. I have hidden two typos in
> the initializers above, one in each initializer block. The typos are of the
> same type but are in difference places so both need to be found
> individually.
>
> Try to find the typos, and record the number of seconds it took for you to
> find them. Which typo took less time to find?

Actually test was invalidated by me looking for two typos in each block for a long long time. Therefore by 
providing interesting visual things to look at you made me skip the important bit with instructions. :)

But I personally think it is not such a big difference. And please remember that the part I originally 
commented on was different in a way that it had expressions on the right hand side. There with a huge gap 
between left and right you then break to logical association and make brain evaluate it as two separate groups 
which perhaps doesn't always makes sense.

And how could you say that not vertically aligning increases maximum line length is beyond me.

> I'd suspect that for most people block#1 is the winner.

_You_ think it's fun to look at and you suspect it is the winner, which is basically my point. I think you are 
going to far. Either you specify the rules in CodingStyle or you don't have it as a reason to send patches for 
rework. Until then you can't say it is a policy, it can at least depend on circumstances.

> [ Note, you have a look at it as real source code with fixed width fonts
> and you'll see the difference. You seem to be using KMail or so: there's
> weird line wraps and other corruption of the code in your quote above -
> have you really looked at the real code how it looks like to developers? ]

Yes I have, it looks like ordinary word wrap when replying. I don't see any other corruptions though - have 
you added that just to make it sound more dramatic together with suggesting I am not a developer?

> > I find it a matter of personal preference whether it is more pleasant to
> > look at and whether it is more or less readable.
>
> Is your argument that to you it is less pleasant to look at?

No, my argument is that in this case it is a personal preference what is more pleasant to look at.

> Differences in taste is OK in borderline issues, but i think this one is
> far from being borderline, it's a basic typographic and aesthetic
> principle.

While for some cases I also like vertical alignment, for the original code I think it  was borderline _at 
most_. 

> I'm not sure vertical spaces can be properly described via more rigid
> CodingStyle rules - for example vertical spaces look ugly if there's just
> two field initializations for example - but in the above case they are
> clearly the right thing to do.

Well I don't wish to argue on this subject very much. 

I just thought it is unfair to criticise on an issue which cannot be a hard rule and in a particular example 
was not an improvement. 

Since you also agree it is not possible to describe it as a hard rule, why bother people with it?

Tvrtko

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Ingo Molnar]

* Tvrtko Ursulin <tvrtko.ursulin@sophos.com> wrote:

> On Thursday 05 February 2009 18:00:19 Ingo Molnar wrote:
> > * Tvrtko Ursulin <tvrtko.ursulin@sophos.com> wrote:
> > > On Wednesday 04 February 2009 17:04:40 Ingo Molnar wrote:
> > > > 2) Please use vertical spaces when initializing structure fields.
> > > > Instead of the messy looking (and over-long-line generating) construct
> > > > of:
> > > >
> > > >         pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;
> > > >         pci_mmcfg_config[0].pci_segment = 0;
> > > >         pci_mmcfg_config[0].start_bus_number = 0;
> > > >         pci_mmcfg_config[0].end_bus_number = (1 << (8 - ((extcfg >> 28)
> > > > & 3))) - 1; pci_mmcfg_config_num = 1;
> > > >
> > > >    You will get something like:
> > > >
> > > >         config->address                 = (extcfg & 0x00007fff) << 25;
> > > >         config->pci_segment             = 0;
> > > >         config->start_bus_number        = 0;
> > > >         config->end_bus_number          = (1 << (8 - ((extcfg >> 28) &
> > > > 3)));
> > > >
> > > >         pci_mmcfg_config = config;
> > > >         pci_mmcfg_config_num = 1;
> > > >
> > > >    Which makes it more structured, more reviewable - and more pleasant
> > > > to look at as well.
> >
> > It is arch/x86/ and scheduler / etc. policy for new code - and we follow
> > that principle when we clean up code as well.
> 
> You also didn't say anything about variable declarations I asked about? 
> And I can add structure definition to that question as well.

Firstly, when posting on lkml please use proper line length breaks. Your 
email was almost unreadable in my mailer, so i had to stop reading it.

Also, i'm surprised you see the need to try to influence things here - i 
dont see a single upstream contribution from you in the past ~4 years of git 
log so how can you have any knowledge and experience about such details?

Both of those issues pretty materially weaken your standing to be taken 
seriously when it comes to fine details of Linux kernel coding style.

	Ingo

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Tvrtko Ursulin]

On Friday 06 February 2009 15:42:31 Ingo Molnar wrote:
> * Tvrtko Ursulin <tvrtko.ursulin@sophos.com> wrote:
> > On Thursday 05 February 2009 18:00:19 Ingo Molnar wrote:
> > > * Tvrtko Ursulin <tvrtko.ursulin@sophos.com> wrote:
> > > > On Wednesday 04 February 2009 17:04:40 Ingo Molnar wrote:
> > > > > 2) Please use vertical spaces when initializing structure fields.
> > > > > Instead of the messy looking (and over-long-line generating)
> > > > > construct of:
> > > > >
> > > > >         pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;
> > > > >         pci_mmcfg_config[0].pci_segment = 0;
> > > > >         pci_mmcfg_config[0].start_bus_number = 0;
> > > > >         pci_mmcfg_config[0].end_bus_number = (1 << (8 - ((extcfg >>
> > > > > 28) & 3))) - 1; pci_mmcfg_config_num = 1;
> > > > >
> > > > >    You will get something like:
> > > > >
> > > > >         config->address                 = (extcfg & 0x00007fff) <<
> > > > > 25; config->pci_segment             = 0;
> > > > >         config->start_bus_number        = 0;
> > > > >         config->end_bus_number          = (1 << (8 - ((extcfg >>
> > > > > 28) & 3)));
> > > > >
> > > > >         pci_mmcfg_config = config;
> > > > >         pci_mmcfg_config_num = 1;
> > > > >
> > > > >    Which makes it more structured, more reviewable - and more
> > > > > pleasant to look at as well.
> > >
> > > It is arch/x86/ and scheduler / etc. policy for new code - and we
> > > follow that principle when we clean up code as well.
> >
> > You also didn't say anything about variable declarations I asked about?
> > And I can add structure definition to that question as well.
>
> Firstly, when posting on lkml please use proper line length breaks. Your
> email was almost unreadable in my mailer, so i had to stop reading it.
>
> Also, i'm surprised you see the need to try to influence things here - i
> dont see a single upstream contribution from you in the past ~4 years of
> git log so how can you have any knowledge and experience about such
> details?
>
> Both of those issues pretty materially weaken your standing to be taken
> seriously when it comes to fine details of Linux kernel coding style.

Sorry about the line length, I was trying not to word wrap code this time.

Apart from that, frankly, I find your reply a bit childish. I haven't
realised I need special talking points to express my opinion on a public 
mailing list. Especially since in my previous reply I said that I don't
want to argue about this very much and that I am just expressing my opinion.

Tvrtko

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Ed Swierk]

Detect and enable memory-mapped PCI configuration space on the nVidia
MCP55 southbridge.  Tested against 2.6.27.4 on an Arista Networks
development board with one MCP55, Coreboot firmware, no ACPI.

Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>

---

I've tried to incorporate the code style feedback from Ingo.  I'm not
sure whether this correctly handles boards with more than one MCP55, or
with an AMD 10h--Yinghai?

Index: linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c
===================================================================
--- linux-2.6.27.4.orig/arch/x86/pci/mmconfig-shared.c
+++ linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c
@@ -166,6 +166,36 @@ static const char __init *pci_mmcfg_amd_
 	return "AMD Family 10h NB";
 }
 
+static const char __init *pci_mmcfg_nvidia_mcp55(void)
+{
+	u32 extcfg;
+	u64 base;
+	int end;
+	static const u32 extcfg_regnum =      0x90;
+	static const u32 extcfg_regsize =     4;
+	static const u32 extcfg_enable_mask = 0x80000000;
+	static const u32 extcfg_end_mask =    0x30000000;
+	static const int extcfg_end_shift =   28;
+	static const int extcfg_endbus[] =    { 255, 127, 63, 31 };
+	static const u32 extcfg_base_mask =   0x00007fff;
+	static const int extcfg_base_lshift = 25;
+
+	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), extcfg_regnum, extcfg_regsize,
+			  &extcfg);
+
+	if (!(extcfg & extcfg_enable_mask))
+		return NULL;
+
+	if (extend_mmcfg(1) == -1)
+		return NULL;
+
+	base = (extcfg & extcfg_base_mask) << extcfg_base_lshift;
+	end = (extcfg & extcfg_end_mask) >> extcfg_end_shift;
+	fill_one_mmcfg(base, 0, 0, extcfg_endbus[end]);
+
+	return "nVidia MCP55";
+}
+
 struct pci_mmcfg_hostbridge_probe {
 	u32 bus;
 	u32 devfn;
@@ -183,6 +213,8 @@ static struct pci_mmcfg_hostbridge_probe
 	  0x1200, pci_mmcfg_amd_fam10h },
 	{ 0xff, PCI_DEVFN(0, 0), PCI_VENDOR_ID_AMD,
 	  0x1200, pci_mmcfg_amd_fam10h },
+	{ 0, PCI_DEVFN(0, 0), PCI_VENDOR_ID_NVIDIA,
+	  0x0369, pci_mmcfg_nvidia_mcp55 },
 };
 
 static int __init pci_mmcfg_check_hostbridge(void)

Re: [PATCH] Detect mmconfig on nVidia MCP55

[Yinghai Lu]

Ed Swierk wrote:
> Detect and enable memory-mapped PCI configuration space on the nVidia
> MCP55 southbridge.  Tested against 2.6.27.4 on an Arista Networks
> development board with one MCP55, Coreboot firmware, no ACPI.
> 
> Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>
> 
> ---
> 
> I've tried to incorporate the code style feedback from Ingo.  I'm not
> sure whether this correctly handles boards with more than one MCP55, or
> with an AMD 10h--Yinghai?
> 
> Index: linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c
> ===================================================================
> --- linux-2.6.27.4.orig/arch/x86/pci/mmconfig-shared.c
> +++ linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c
> @@ -166,6 +166,36 @@ static const char __init *pci_mmcfg_amd_
>  	return "AMD Family 10h NB";
>  }
>  
> +static const char __init *pci_mmcfg_nvidia_mcp55(void)
> +{
> +	u32 extcfg;
> +	u64 base;
> +	int end;
> +	static const u32 extcfg_regnum =      0x90;
> +	static const u32 extcfg_regsize =     4;
> +	static const u32 extcfg_enable_mask = 0x80000000;
> +	static const u32 extcfg_end_mask =    0x30000000;
> +	static const int extcfg_end_shift =   28;
> +	static const int extcfg_endbus[] =    { 255, 127, 63, 31 };
> +	static const u32 extcfg_base_mask =   0x00007fff;
> +	static const int extcfg_base_lshift = 25;
> +
> +	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), extcfg_regnum, extcfg_regsize,
> +			  &extcfg);
1. mcp55 could one bus1
2. io55 could be on 0x40, 0x80, 0xc0

so it seems we could loop all 0-255 to find those HT in one function and add them one by one.

YH
> +
> +	if (!(extcfg & extcfg_enable_mask))
> +		return NULL;
> +
> +	if (extend_mmcfg(1) == -1)
> +		return NULL;
> +
> +	base = (extcfg & extcfg_base_mask) << extcfg_base_lshift;
> +	end = (extcfg & extcfg_end_mask) >> extcfg_end_shift;
> +	fill_one_mmcfg(base, 0, 0, extcfg_endbus[end]);
> +
> +	return "nVidia MCP55";
> +}
> +
>  struct pci_mmcfg_hostbridge_probe {
>  	u32 bus;
>  	u32 devfn;
> @@ -183,6 +213,8 @@ static struct pci_mmcfg_hostbridge_probe
>  	  0x1200, pci_mmcfg_amd_fam10h },
>  	{ 0xff, PCI_DEVFN(0, 0), PCI_VENDOR_ID_AMD,
>  	  0x1200, pci_mmcfg_amd_fam10h },
> +	{ 0, PCI_DEVFN(0, 0), PCI_VENDOR_ID_NVIDIA,
> +	  0x0369, pci_mmcfg_nvidia_mcp55 },
>  };
>  
>  static int __init pci_mmcfg_check_hostbridge(void)
> 

[PATCH] x86/pci: host mmconfig detect clean up v3

[Yinghai Lu]

Impact: not assume one place for mmconfig in nb

prepare for following case: amd fam10h + mcp55
CPU MSR has some range, mcp55 pci config will have another one.

also prepare for mcp55 + io55 system. every one will have one range.

if it s mcp55 detect duties to execlude range that is used by CPU MSR
aka, if CPU state bus 0-255, range in mcp55 need to be dropped.
because HW in CPU will not route that mcp55 mmconfig to handle it.

v2: fix e7520 exit path
v3: make it could support
   PCI MMCONFIG 0 [00-3f]
   PCI MMCONFIG 0 [40-7f]
   PCI MMCONFIG 0 [80-ff]

Signed-off-by: Yinghai Lu <yinghai.lu@kernel.org>

---
 arch/x86/pci/mmconfig-shared.c |  161 +++++++++++++++++++++++++++--------------
 arch/x86/pci/mmconfig_64.c     |   15 ++-
 2 files changed, 118 insertions(+), 58 deletions(-)

Index: linux-2.6/arch/x86/pci/mmconfig-shared.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/mmconfig-shared.c
+++ linux-2.6/arch/x86/pci/mmconfig-shared.c
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/acpi.h>
 #include <linux/bitmap.h>
+#include <linux/sort.h>
 #include <asm/e820.h>
 #include <asm/pci_x86.h>
 
@@ -24,24 +25,49 @@
 /* Indicate if the mmcfg resources have been placed into the resource table. */
 static int __initdata pci_mmcfg_resources_inserted;
 
+static __init int extend_mmcfg(int num)
+{
+	struct acpi_mcfg_allocation *new;
+	int new_num = pci_mmcfg_config_num + num;
+
+	new = kzalloc(sizeof(pci_mmcfg_config[0]) * new_num, GFP_KERNEL);
+	if (!new)
+		return -1;
+
+	if (pci_mmcfg_config) {
+		memcpy(new, pci_mmcfg_config,
+			 sizeof(pci_mmcfg_config[0]) * new_num);
+		kfree(pci_mmcfg_config);
+	}
+	pci_mmcfg_config = new;
+
+	return 0;
+}
+
+static __init void fill_one_mmcfg(u64 addr, int segment, int start, int end)
+{
+	int i = pci_mmcfg_config_num;
+
+	pci_mmcfg_config_num++;
+	pci_mmcfg_config[i].address = addr;
+	pci_mmcfg_config[i].pci_segment = segment;
+	pci_mmcfg_config[i].start_bus_number = start;
+	pci_mmcfg_config[i].end_bus_number = end;
+}
+
 static const char __init *pci_mmcfg_e7520(void)
 {
 	u32 win;
 	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0xce, 2, &win);
 
 	win = win & 0xf000;
-	if(win == 0x0000 || win == 0xf000)
-		pci_mmcfg_config_num = 0;
-	else {
-		pci_mmcfg_config_num = 1;
-		pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]), GFP_KERNEL);
-		if (!pci_mmcfg_config)
-			return NULL;
-		pci_mmcfg_config[0].address = win << 16;
-		pci_mmcfg_config[0].pci_segment = 0;
-		pci_mmcfg_config[0].start_bus_number = 0;
-		pci_mmcfg_config[0].end_bus_number = 255;
-	}
+	if (win == 0x0000 || win == 0xf000)
+		return NULL;
+
+	if (extend_mmcfg(1) == -1)
+		return NULL;
+
+	fill_one_mmcfg(win << 16, 0, 0, 255);
 
 	return "Intel Corporation E7520 Memory Controller Hub";
 }
@@ -50,13 +76,11 @@ static const char __init *pci_mmcfg_inte
 {
 	u32 pciexbar, mask = 0, len = 0;
 
-	pci_mmcfg_config_num = 1;
-
 	raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0x48, 4, &pciexbar);
 
 	/* Enable bit */
 	if (!(pciexbar & 1))
-		pci_mmcfg_config_num = 0;
+		return NULL;
 
 	/* Size bits */
 	switch ((pciexbar >> 1) & 3) {
@@ -73,28 +97,23 @@ static const char __init *pci_mmcfg_inte
 		len  = 0x04000000U;
 		break;
 	default:
-		pci_mmcfg_config_num = 0;
+		return NULL;
 	}
 
 	/* Errata #2, things break when not aligned on a 256Mb boundary */
 	/* Can only happen in 64M/128M mode */
 
 	if ((pciexbar & mask) & 0x0fffffffU)
-		pci_mmcfg_config_num = 0;
+		return NULL;
 
 	/* Don't hit the APIC registers and their friends */
 	if ((pciexbar & mask) >= 0xf0000000U)
-		pci_mmcfg_config_num = 0;
+		return NULL;
 
-	if (pci_mmcfg_config_num) {
-		pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]), GFP_KERNEL);
-		if (!pci_mmcfg_config)
-			return NULL;
-		pci_mmcfg_config[0].address = pciexbar & mask;
-		pci_mmcfg_config[0].pci_segment = 0;
-		pci_mmcfg_config[0].start_bus_number = 0;
-		pci_mmcfg_config[0].end_bus_number = (len >> 20) - 1;
-	}
+	if (extend_mmcfg(1) == -1)
+		return NULL;
+
+	fill_one_mmcfg(pciexbar & mask, 0, 0, (len >> 20) - 1);
 
 	return "Intel Corporation 945G/GZ/P/PL Express Memory Controller Hub";
 }
@@ -138,18 +157,11 @@ static const char __init *pci_mmcfg_amd_
 		busnbits = 8;
 	}
 
-	pci_mmcfg_config_num = (1 << segnbits);
-	pci_mmcfg_config = kzalloc(sizeof(pci_mmcfg_config[0]) *
-				   pci_mmcfg_config_num, GFP_KERNEL);
-	if (!pci_mmcfg_config)
+	if (extend_mmcfg(1 << segnbits) == -1)
 		return NULL;
 
-	for (i = 0; i < (1 << segnbits); i++) {
-		pci_mmcfg_config[i].address = base + (1<<28) * i;
-		pci_mmcfg_config[i].pci_segment = i;
-		pci_mmcfg_config[i].start_bus_number = 0;
-		pci_mmcfg_config[i].end_bus_number = (1 << busnbits) - 1;
-	}
+	for (i = 0; i < (1 << segnbits); i++)
+		fill_one_mmcfg(base + (1<<28) * i, i, 0, (1 << busnbits) - 1);
 
 	return "AMD Family 10h NB";
 }
@@ -173,6 +185,48 @@ static struct pci_mmcfg_hostbridge_probe
 	  0x1200, pci_mmcfg_amd_fam10h },
 };
 
+static int __init cmp_mmcfg(const void *x1, const void *x2)
+{
+	const typeof(pci_mmcfg_config[0]) *m1 = x1;
+	const typeof(pci_mmcfg_config[0]) *m2 = x2;
+	int start1, start2;
+
+	start1 = m1->start_bus_number;
+	start2 = m2->start_bus_number;
+
+	return start1 - start2;
+}
+
+static void __init pci_mmcfg_check_end_bus_number(void)
+{
+	int i;
+	typeof(pci_mmcfg_config[0]) *cfg, *cfgx;
+
+	/* sort them at first */
+	sort(pci_mmcfg_config, pci_mmcfg_config_num,
+		 sizeof(pci_mmcfg_config[0]), cmp_mmcfg, NULL);
+
+	/* last one*/
+	if (pci_mmcfg_config_num > 0) {
+		i = pci_mmcfg_config_num - 1;
+		cfg = &pci_mmcfg_config[i];
+		if (cfg->end_bus_number < cfg->start_bus_number)
+			cfg->end_bus_number = 255;
+	}
+
+	/* don't overlap please */
+	for (i = 0; i < pci_mmcfg_config_num - 1; i++) {
+		cfg = &pci_mmcfg_config[i];
+		cfgx = &pci_mmcfg_config[i+1];
+
+		if (cfg->end_bus_number < cfg->start_bus_number)
+			cfg->end_bus_number = 255;
+
+		if (cfg->end_bus_number >= cfgx->start_bus_number)
+			cfg->end_bus_number = cfgx->start_bus_number - 1;
+	}
+}
+
 static int __init pci_mmcfg_check_hostbridge(void)
 {
 	u32 l;
@@ -186,31 +240,33 @@ static int __init pci_mmcfg_check_hostbr
 
 	pci_mmcfg_config_num = 0;
 	pci_mmcfg_config = NULL;
-	name = NULL;
 
-	for (i = 0; !name && i < ARRAY_SIZE(pci_mmcfg_probes); i++) {
+	for (i = 0; i < ARRAY_SIZE(pci_mmcfg_probes); i++) {
 		bus =  pci_mmcfg_probes[i].bus;
 		devfn = pci_mmcfg_probes[i].devfn;
 		raw_pci_ops->read(0, bus, devfn, 0, 4, &l);
 		vendor = l & 0xffff;
 		device = (l >> 16) & 0xffff;
 
+		name = NULL;
 		if (pci_mmcfg_probes[i].vendor == vendor &&
 		    pci_mmcfg_probes[i].device == device)
 			name = pci_mmcfg_probes[i].probe();
-	}
 
-	if (name) {
-		printk(KERN_INFO "PCI: Found %s %s MMCONFIG support.\n",
-		       name, pci_mmcfg_config_num ? "with" : "without");
+		if (name)
+			printk(KERN_INFO "PCI: Found %s with MMCONFIG support.\n",
+			       name);
 	}
 
-	return name != NULL;
+	/* some end_bus_number is crazy, fix it */
+	pci_mmcfg_check_end_bus_number();
+
+	return pci_mmcfg_config_num != 0;
 }
 
 static void __init pci_mmcfg_insert_resources(void)
 {
-#define PCI_MMCFG_RESOURCE_NAME_LEN 19
+#define PCI_MMCFG_RESOURCE_NAME_LEN 24
 	int i;
 	struct resource *res;
 	char *names;
@@ -228,9 +284,10 @@ static void __init pci_mmcfg_insert_reso
 		struct acpi_mcfg_allocation *cfg = &pci_mmcfg_config[i];
 		num_buses = cfg->end_bus_number - cfg->start_bus_number + 1;
 		res->name = names;
-		snprintf(names, PCI_MMCFG_RESOURCE_NAME_LEN, "PCI MMCONFIG %u",
-			 cfg->pci_segment);
-		res->start = cfg->address;
+		snprintf(names, PCI_MMCFG_RESOURCE_NAME_LEN,
+			 "PCI MMCONFIG %u [%02x-%02x]", cfg->pci_segment,
+			 cfg->start_bus_number, cfg->end_bus_number);
+		res->start = cfg->address + (cfg->start_bus_number << 20);
 		res->end = res->start + (num_buses << 20) - 1;
 		res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
 		insert_resource(&iomem_resource, res);
@@ -354,8 +411,6 @@ static void __init pci_mmcfg_reject_brok
 	    (pci_mmcfg_config[0].address == 0))
 		return;
 
-	cfg = &pci_mmcfg_config[0];
-
 	for (i = 0; i < pci_mmcfg_config_num; i++) {
 		int valid = 0;
 		u64 addr, size;
@@ -423,10 +478,10 @@ static void __init __pci_mmcfg_init(int
 			known_bridge = 1;
 	}
 
-	if (!known_bridge) {
+	if (!known_bridge)
 		acpi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);
-		pci_mmcfg_reject_broken(early);
-	}
+
+	pci_mmcfg_reject_broken(early);
 
 	if ((pci_mmcfg_config_num == 0) ||
 	    (pci_mmcfg_config == NULL) ||
Index: linux-2.6/arch/x86/pci/mmconfig_64.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/mmconfig_64.c
+++ linux-2.6/arch/x86/pci/mmconfig_64.c
@@ -112,13 +112,18 @@ static struct pci_raw_ops pci_mmcfg = {
 static void __iomem * __init mcfg_ioremap(struct acpi_mcfg_allocation *cfg)
 {
 	void __iomem *addr;
-	u32 size;
+	u64 start, size;
 
-	size = (cfg->end_bus_number + 1) << 20;
-	addr = ioremap_nocache(cfg->address, size);
+	start = cfg->start_bus_number;
+	start <<= 20;
+	start += cfg->address;
+	size = cfg->end_bus_number + 1 - cfg->start_bus_number;
+	size <<= 20;
+	addr = ioremap_nocache(start, size);
 	if (addr) {
 		printk(KERN_INFO "PCI: Using MMCONFIG at %Lx - %Lx\n",
-		       cfg->address, cfg->address + size - 1);
+		       start, start + size - 1);
+		addr -= cfg->start_bus_number << 20;
 	}
 	return addr;
 }
@@ -157,7 +162,7 @@ void __init pci_mmcfg_arch_free(void)
 
 	for (i = 0; i < pci_mmcfg_config_num; ++i) {
 		if (pci_mmcfg_virt[i].virt) {
-			iounmap(pci_mmcfg_virt[i].virt);
+			iounmap(pci_mmcfg_virt[i].virt + (pci_mmcfg_virt[i].cfg->start_bus_number << 20));
 			pci_mmcfg_virt[i].virt = NULL;
 			pci_mmcfg_virt[i].cfg = NULL;
 		}

Subject: [PATCH] x86/pci: Detect mmconfig on nVidia MCP55 -v2

[Yinghai Lu]

From: Ed Swierk <eswierk@aristanetworks.com>

Detect and enable memory-mapped PCI configuration space on the nVidia
MCP55 southbridge.  Tested against 2.6.27.4 on an Arista Networks
development board with one MCP55, Coreboot firmware, no ACPI.

v2: add multi mcp55 support, and conexist with amd quad core system by Yinghai
    also only enable it only when acpi=off or acpi is not built-in

Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/pci/mmconfig-shared.c |   64 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

Index: linux-2.6/arch/x86/pci/mmconfig-shared.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/mmconfig-shared.c
+++ linux-2.6/arch/x86/pci/mmconfig-shared.c
@@ -166,6 +166,68 @@ static const char __init *pci_mmcfg_amd_
 	return "AMD Family 10h NB";
 }
 
+static bool __initdata mcp55_checked;
+static const char __init *pci_mmcfg_nvidia_mcp55(void)
+{
+	int bus;
+	int mcp55_mmconf_found = 0;
+
+	static const u32 extcfg_regnum		= 0x90;
+	static const u32 extcfg_regsize		= 4;
+	static const u32 extcfg_enable_mask	= 1<<31;
+	static const u32 extcfg_start_mask	= 0xff<<16;
+	static const int extcfg_start_shift	= 16;
+	static const u32 extcfg_size_mask	= 0x3<<28;
+	static const int extcfg_size_shift	= 28;
+	static const int extcfg_sizebus[]	= {0x100, 0x80, 0x40, 0x20};
+	static const u32 extcfg_base_mask[]	= {0x7ff8, 0x7ffc, 0x7ffe, 0x7fff};
+	static const int extcfg_base_lshift	= 25;
+
+	/*
+	 * do check if amd fam10h already took over
+	 */
+	if (!acpi_disabled || pci_mmcfg_config_num || mcp55_checked)
+		return NULL;
+
+	mcp55_checked = true;
+	for (bus = 0; bus < 256; bus++) {
+		u64 base;
+		u32 l, extcfg;
+		u16 vendor, device;
+		int start, size_index, end;
+
+		raw_pci_ops->read(0, bus, PCI_DEVFN(0, 0), 0, 4, &l);
+		vendor = l & 0xffff;
+		device = (l >> 16) & 0xffff;
+
+		if (PCI_VENDOR_ID_NVIDIA != vendor || 0x0369 != device)
+			continue;
+
+		raw_pci_ops->read(0, bus, PCI_DEVFN(0, 0), extcfg_regnum,
+				  extcfg_regsize, &extcfg);
+
+		if (!(extcfg & extcfg_enable_mask))
+			continue;
+
+		if (extend_mmcfg(1) == -1)
+			continue;
+
+		size_index = (extcfg & extcfg_size_mask) >> extcfg_size_shift;
+		base = extcfg & extcfg_base_mask[size_index];
+		/* base could > 4G */
+		base <<= extcfg_base_lshift;
+		start = (extcfg & extcfg_start_mask) >> extcfg_start_shift;
+		end = start + extcfg_sizebus[size_index] - 1;
+		fill_one_mmcfg(base, 0, start, end);
+		mcp55_mmconf_found++;
+	}
+
+	if (!mcp55_mmconf_found)
+		return NULL;
+
+	return "nVidia MCP55";
+}
+
 struct pci_mmcfg_hostbridge_probe {
 	u32 bus;
 	u32 devfn;
@@ -183,6 +245,8 @@ static struct pci_mmcfg_hostbridge_probe
 	  0x1200, pci_mmcfg_amd_fam10h },
 	{ 0xff, PCI_DEVFN(0, 0), PCI_VENDOR_ID_AMD,
 	  0x1200, pci_mmcfg_amd_fam10h },
+	{ 0, PCI_DEVFN(0, 0), PCI_VENDOR_ID_NVIDIA,
+	  0x0369, pci_mmcfg_nvidia_mcp55 },
 };
 
 static int __init cmp_mmcfg(const void *x1, const void *x2)

Re: Subject: [PATCH] x86/pci: Detect mmconfig on nVidia MCP55 -v2

[Ed Swierk]

On Mon, Feb 9, 2009 at 6:00 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> From: Ed Swierk <eswierk@aristanetworks.com>
>
> Detect and enable memory-mapped PCI configuration space on the nVidia
> MCP55 southbridge.  Tested against 2.6.27.4 on an Arista Networks
> development board with one MCP55, Coreboot firmware, no ACPI.
>
> v2: add multi mcp55 support, and conexist with amd quad core system by Yinghai
>    also only enable it only when acpi=off or acpi is not built-in
>
> Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>

A couple of comments below. Let me test this version before signing off.

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>
> ---
>  arch/x86/pci/mmconfig-shared.c |   64 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> Index: linux-2.6/arch/x86/pci/mmconfig-shared.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/pci/mmconfig-shared.c
> +++ linux-2.6/arch/x86/pci/mmconfig-shared.c
> @@ -166,6 +166,68 @@ static const char __init *pci_mmcfg_amd_
>        return "AMD Family 10h NB";
>  }
>
> +static bool __initdata mcp55_checked;
> +static const char __init *pci_mmcfg_nvidia_mcp55(void)
> +{
> +       int bus;
> +       int mcp55_mmconf_found = 0;
> +
> +       static const u32 extcfg_regnum          = 0x90;
> +       static const u32 extcfg_regsize         = 4;
> +       static const u32 extcfg_enable_mask     = 1<<31;
> +       static const u32 extcfg_start_mask      = 0xff<<16;
> +       static const int extcfg_start_shift     = 16;
> +       static const u32 extcfg_size_mask       = 0x3<<28;
> +       static const int extcfg_size_shift      = 28;
> +       static const int extcfg_sizebus[]       = {0x100, 0x80, 0x40, 0x20};
> +       static const u32 extcfg_base_mask[]     = {0x7ff8, 0x7ffc, 0x7ffe, 0x7fff};
> +       static const int extcfg_base_lshift     = 25;
> +
> +       /*
> +        * do check if amd fam10h already took over
> +        */
> +       if (!acpi_disabled || pci_mmcfg_config_num || mcp55_checked)
> +               return NULL;

Why skip this if !acpi_disabled? I thought we want to have the host
bridge mmconfig settings override the ACPI MCFG. At least that appears
to be the logic for other host bridges.

> +       mcp55_checked = true;
> +       for (bus = 0; bus < 256; bus++) {
> +               u64 base;
> +               u32 l, extcfg;
> +               u16 vendor, device;
> +               int start, size_index, end;
> +
> +               raw_pci_ops->read(0, bus, PCI_DEVFN(0, 0), 0, 4, &l);

Don't we need to check devices 8, 16 and 24 as well as 0?

> +               vendor = l & 0xffff;
> +               device = (l >> 16) & 0xffff;
> +
> +               if (PCI_VENDOR_ID_NVIDIA != vendor || 0x0369 != device)
> +                       continue;
> +
> +               raw_pci_ops->read(0, bus, PCI_DEVFN(0, 0), extcfg_regnum,
> +                                 extcfg_regsize, &extcfg);
> +
> +               if (!(extcfg & extcfg_enable_mask))
> +                       continue;
> +
> +               if (extend_mmcfg(1) == -1)
> +                       continue;
> +
> +               size_index = (extcfg & extcfg_size_mask) >> extcfg_size_shift;
> +               base = extcfg & extcfg_base_mask[size_index];
> +               /* base could > 4G */
> +               base <<= extcfg_base_lshift;
> +               start = (extcfg & extcfg_start_mask) >> extcfg_start_shift;
> +               end = start + extcfg_sizebus[size_index] - 1;
> +               fill_one_mmcfg(base, 0, start, end);
> +               mcp55_mmconf_found++;
> +       }
> +
> +       if (!mcp55_mmconf_found)
> +               return NULL;
> +
> +       return "nVidia MCP55";
> +}
> +
>  struct pci_mmcfg_hostbridge_probe {
>        u32 bus;
>        u32 devfn;
> @@ -183,6 +245,8 @@ static struct pci_mmcfg_hostbridge_probe
>          0x1200, pci_mmcfg_amd_fam10h },
>        { 0xff, PCI_DEVFN(0, 0), PCI_VENDOR_ID_AMD,
>          0x1200, pci_mmcfg_amd_fam10h },
> +       { 0, PCI_DEVFN(0, 0), PCI_VENDOR_ID_NVIDIA,
> +         0x0369, pci_mmcfg_nvidia_mcp55 },
>  };
>
>  static int __init cmp_mmcfg(const void *x1, const void *x2)
>

Re: Subject: [PATCH] x86/pci: Detect mmconfig on nVidia MCP55 -v2

[Yinghai Lu]

On Tue, Feb 10, 2009 at 2:57 PM, Ed Swierk <eswierk@aristanetworks.com> wrote:
> On Mon, Feb 9, 2009 at 6:00 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> From: Ed Swierk <eswierk@aristanetworks.com>
>>
>> Detect and enable memory-mapped PCI configuration space on the nVidia
>> MCP55 southbridge.  Tested against 2.6.27.4 on an Arista Networks
>> development board with one MCP55, Coreboot firmware, no ACPI.
>>
>> v2: add multi mcp55 support, and conexist with amd quad core system by Yinghai
>>    also only enable it only when acpi=off or acpi is not built-in
>>
>> Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>
>
> A couple of comments below. Let me test this version before signing off.
>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  arch/x86/pci/mmconfig-shared.c |   64 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 64 insertions(+)
>>
>> Index: linux-2.6/arch/x86/pci/mmconfig-shared.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/pci/mmconfig-shared.c
>> +++ linux-2.6/arch/x86/pci/mmconfig-shared.c
>> @@ -166,6 +166,68 @@ static const char __init *pci_mmcfg_amd_
>>        return "AMD Family 10h NB";
>>  }
>>
>> +static bool __initdata mcp55_checked;
>> +static const char __init *pci_mmcfg_nvidia_mcp55(void)
>> +{
>> +       int bus;
>> +       int mcp55_mmconf_found = 0;
>> +
>> +       static const u32 extcfg_regnum          = 0x90;
>> +       static const u32 extcfg_regsize         = 4;
>> +       static const u32 extcfg_enable_mask     = 1<<31;
>> +       static const u32 extcfg_start_mask      = 0xff<<16;
>> +       static const int extcfg_start_shift     = 16;
>> +       static const u32 extcfg_size_mask       = 0x3<<28;
>> +       static const int extcfg_size_shift      = 28;
>> +       static const int extcfg_sizebus[]       = {0x100, 0x80, 0x40, 0x20};
>> +       static const u32 extcfg_base_mask[]     = {0x7ff8, 0x7ffc, 0x7ffe, 0x7fff};
>> +       static const int extcfg_base_lshift     = 25;
>> +
>> +       /*
>> +        * do check if amd fam10h already took over
>> +        */
>> +       if (!acpi_disabled || pci_mmcfg_config_num || mcp55_checked)
>> +               return NULL;
>
> Why skip this if !acpi_disabled? I thought we want to have the host
> bridge mmconfig settings override the ACPI MCFG. At least that appears
> to be the logic for other host bridges.

try to make thing simple.
for system: with AMD Fam10h, and MCP55 + 2 IO55
amd fam10h cpu: said it needs [0,0]
mcp55: said it need [0,255]
io55 #1: said it need [0x40, 0x40+255], then u8 end_bus_number = 0x3f
io55 #2: said it need [0x80, 0x80+255], then u8 end_bus_number = 0x7f
MMIO route in NBs is right.

but MCFG is right... so just use that.

when acpi=off, if check_and_enable_amd_mconf, it will forcely to make
amd fam10h cpu: said it needs [0,255] and pci_mmcfg_config_num = 1
so mcp55 mconf is not used.


>
>> +       mcp55_checked = true;
>> +       for (bus = 0; bus < 256; bus++) {
>> +               u64 base;
>> +               u32 l, extcfg;
>> +               u16 vendor, device;
>> +               int start, size_index, end;
>> +
>> +               raw_pci_ops->read(0, bus, PCI_DEVFN(0, 0), 0, 4, &l);
>
> Don't we need to check devices 8, 16 and 24 as well as 0?

you didn't put mcp55 on device 0? why?

YH

Re: Subject: [PATCH] x86/pci: Detect mmconfig on nVidia MCP55 -v2

[Ed Swierk]

On Tue, Feb 10, 2009 at 9:05 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> try to make thing simple.
> for system: with AMD Fam10h, and MCP55 + 2 IO55
> amd fam10h cpu: said it needs [0,0]
> mcp55: said it need [0,255]
> io55 #1: said it need [0x40, 0x40+255], then u8 end_bus_number = 0x3f
> io55 #2: said it need [0x80, 0x80+255], then u8 end_bus_number = 0x7f
> MMIO route in NBs is right.

Oh, I misinterpreted your earlier comment--I thought you were
referring to 0x40, 0x80, 0xc0 as devfns, not bus numbers. The MCP55 is
on device 0 on our board as well.

> but MCFG is right... so just use that.
>
> when acpi=off, if check_and_enable_amd_mconf, it will forcely to make
> amd fam10h cpu: said it needs [0,255] and pci_mmcfg_config_num = 1
> so mcp55 mconf is not used.

OK, I'm just pointing out that the existing mmconfig hostbridge
detection code overrides ACPI MCFG. Past commit comments suggest that
the hostbridge registers are more trustworthy than ACPI on some
boards.

--Ed

Re: [PATCH] x86/pci: host mmconfig detect clean up v3

[Ed Swierk]

On Mon, Feb 9, 2009 at 5:59 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> Impact: not assume one place for mmconfig in nb
>
> prepare for following case: amd fam10h + mcp55
> CPU MSR has some range, mcp55 pci config will have another one.
>
> also prepare for mcp55 + io55 system. every one will have one range.
>
> if it s mcp55 detect duties to execlude range that is used by CPU MSR
> aka, if CPU state bus 0-255, range in mcp55 need to be dropped.
> because HW in CPU will not route that mcp55 mmconfig to handle it.
>
> v2: fix e7520 exit path
> v3: make it could support
>   PCI MMCONFIG 0 [00-3f]
>   PCI MMCONFIG 0 [40-7f]
>   PCI MMCONFIG 0 [80-ff]
>
> Signed-off-by: Yinghai Lu <yinghai.lu@kernel.org>

Looks okay to me. I tested the patch on a board with an AMD 0Fh CPU
and one MCP55, no ACPI.

--Ed

Re: Subject: [PATCH] x86/pci: Detect mmconfig on nVidia MCP55 -v2

[Ed Swierk]

On Mon, Feb 9, 2009 at 6:00 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> From: Ed Swierk <eswierk@aristanetworks.com>
>
> Detect and enable memory-mapped PCI configuration space on the nVidia
> MCP55 southbridge.  Tested against 2.6.27.4 on an Arista Networks
> development board with one MCP55, Coreboot firmware, no ACPI.
>
> v2: add multi mcp55 support, and conexist with amd quad core system by Yinghai
>    also only enable it only when acpi=off or acpi is not built-in
>
> Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

I tested patch v2 and it works OK on our board.

--Ed