The first 3 patches add diagnostics to iomem resource management.
The x86 patch adds a diagnostic for bogus ACPI resource definitions.
Lastly, the issue addressed by the mfd driver patch emerged while
testing the other patches in this series.
Peter Hurley (5):
kernel: Print resource ranges when expanding overlaps
kernel: Print resource conflicts for failed requests
kernel: Print warning when inserting resource [mem 0x00000000]
x86: acpi: Print warning for malformed host bridge resources
drivers: mfd: Fix resource request for [mem 0x00000000]
arch/x86/pci/acpi.c | 4 ++++
drivers/mfd/lpc_ich.c | 3 +++
kernel/resource.c | 12 ++++++++++--
3 files changed, 17 insertions(+), 2 deletions(-)
--
1.7.12.3
Resource names can be too generic to distinguish which resources
overlap; eg.,
kernel: Expanded resource reserved due to conflict with PCI Bus 0000:00
kernel: Expanded resource reserved due to conflict with PCI Bus 0000:00
Rather, print decoded resource info as well; eg.,
kernel: resource: Expanding reserved [mem 0xcfe5ec00-0xcfffffff]; overlaps PCI Bus 0000:00 [mem 0xcff00000-0xdfffffff]
kernel: resource: Expanding reserved [mem 0xfe000000-0xfeffffff]; overlaps PCI Bus 0000:00 [mem 0xf0000000-0xfe000000]
Cc: Bjorn Helgaas <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
kernel/resource.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/resource.c b/kernel/resource.c
index 73f35d4..461c2e0 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -696,12 +696,13 @@ void insert_resource_expand_to_fit(struct resource *root, struct resource *new)
break;
/* Ok, expand resource to cover the conflict, then try again .. */
+ pr_warn("Expanding %s %pR; overlaps %s %pR\n",
+ new->name, new, conflict->name, conflict);
+
if (conflict->start < new->start)
new->start = conflict->start;
if (conflict->end > new->end)
new->end = conflict->end;
-
- printk("Expanded resource %s due to conflict with %s\n", new->name, conflict->name);
}
write_unlock(&resource_lock);
}
--
1.7.12.3
When resource requests fail, the conflicting resource is not
always apparent. Emit diagnostic print when resource conflicts prevent
resource requests. For example,
kernel: resource: Requested i5k_amb [mem 0xfe000000-0xfe01ffff flags 0x80000000] conflicts with PCI Bus 0000:00 [mem 0xf0000000-0xfe000000 flags 0x200]
Cc: Bjorn Helgaas <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
kernel/resource.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/resource.c b/kernel/resource.c
index 461c2e0..268b5fa 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -924,6 +924,10 @@ struct resource * __request_region(struct resource *parent,
write_lock(&resource_lock);
continue;
}
+
+ pr_debug("Requested %s %pr conflicts with %s %pr\n",
+ res->name, res, conflict->name, conflict);
+
/* Uhhuh, that didn't work out.. */
kfree(res);
res = NULL;
--
1.7.12.3
Inserting iomem resource [0x0-0x0] is most likely a bug; at least
print a warning message. Eg.,
resource: inserting NULL resource iTCO_wdt [mem 0x00000000]
Cc: Bjorn Helgaas <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
kernel/resource.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/resource.c b/kernel/resource.c
index 268b5fa..ab758bb 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -589,6 +589,9 @@ static struct resource * __insert_resource(struct resource *parent, struct resou
{
struct resource *first, *next;
+ if (new->flags & IORESOURCE_MEM && !new->start && !new->end)
+ pr_warn("inserting NULL resource %s %pR", new->name, new);
+
for (;; parent = first) {
first = __request_resource(parent, new);
if (!first)
--
1.7.12.3
An incorrectly specified host bridge window may prevent
other devices from claiming assigned resources. For example,
this flawed _CRS resource descriptor from a Dell T5400:
DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
0x00000000, // Granularity
0xF0000000, // Range Minimum
0xFE000000, // Range Maximum
0x00000000, // Translation Offset
0x0E000000, // Length
,, , AddressRangeMemory, TypeStatic)
prevents the adjacent device from claiming [mem 0xfe0000000-0xfe01ffff]
Sanity check that the resource at least conforms to a valid
PCI BAR; if not, emit a diagnostic warning.
Cc: Bjorn Helgaas <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: [email protected]
Signed-off-by: Peter Hurley <[email protected]>
---
arch/x86/pci/acpi.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 192397c..3468d16 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -298,6 +298,10 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
"host bridge window [%#llx-%#llx] "
"([%#llx-%#llx] ignored, not CPU addressable)\n",
start, orig_end, end + 1, orig_end);
+ } else if (flags & IORESOURCE_MEM && (start & 0x0f || ~end & 0x0f)) {
+ dev_warn(&info->bridge->dev,
+ "invalid host bridge window [%#llx-%#llx]\n",
+ start, end);
}
res = &info->res[info->res_num];
--
1.7.12.3
The older southbridges supported by the lpc_ich driver do not
provide memory-mapped space of the root complex. The driver
correctly avoids computing the iomem address in this case, yet
submits a zeroed resource request anyway (via mfd_add_devices()).
Remove the iomem resource from the resource array submitted to the
mfd core for the older southbridges.
Cc: Aaron Sierra <[email protected]>
Cc: Peter Tyser <[email protected]>
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/mfd/lpc_ich.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index a22544f..a46095b 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
res = wdt_mem_res(ICH_RES_MEM_GCS);
res->start = base_addr + ACPIBASE_GCS_OFF;
res->end = base_addr + ACPIBASE_GCS_END;
+ } else {
+ /* So don't register iomem for TCO ver 1 */
+ --lpc_ich_cells[LPC_WDT].num_resources;
}
lpc_ich_finalize_cell(&lpc_ich_cells[LPC_WDT], id);
--
1.7.12.3
> The older southbridges supported by the lpc_ich driver do not
> provide memory-mapped space of the root complex. The driver
> correctly avoids computing the iomem address in this case, yet
> submits a zeroed resource request anyway (via mfd_add_devices()).
>
> Remove the iomem resource from the resource array submitted to the
> mfd core for the older southbridges.
>
Peter, thanks for catching this.
> + } else {
> + /* So don't register iomem for TCO ver 1 */
> + --lpc_ich_cells[LPC_WDT].num_resources;
My only complaint is that pre-decrementing num_resources isn't
necessary and doesn't match the other cases where num_resources is
decremented (post-decremented), like here:
if (!base_addr) {
dev_err(&dev->dev, "I/O space for ACPI uninitialized\n");
lpc_ich_cells[LPC_GPIO].num_resources--;
goto gpe0_done;
}
-Aaron
On Thu, 2012-11-08 at 11:04 -0600, Aaron Sierra wrote:
> > The older southbridges supported by the lpc_ich driver do not
> > provide memory-mapped space of the root complex. The driver
> > correctly avoids computing the iomem address in this case, yet
> > submits a zeroed resource request anyway (via mfd_add_devices()).
> >
> > Remove the iomem resource from the resource array submitted to the
> > mfd core for the older southbridges.
> >
>
> Peter, thanks for catching this.
>
> > + } else {
> > + /* So don't register iomem for TCO ver 1 */
> > + --lpc_ich_cells[LPC_WDT].num_resources;
>
> My only complaint is that pre-decrementing num_resources isn't
> necessary and doesn't match the other cases where num_resources is
> decremented (post-decremented), like here:
>
> if (!base_addr) {
> dev_err(&dev->dev, "I/O space for ACPI uninitialized\n");
> lpc_ich_cells[LPC_GPIO].num_resources--;
> goto gpe0_done;
> }
Sorry, C++ force-of-habit. How's this instead? (please note, I also
retitled the patch to refer to lpc_ich specifically)
-- >8 --
Subject: [PATCH v2 5/5] mfd: lpc_ich: Fix resource request for [mem 0x00000000]
The older southbridges supported by the lpc_ich driver do not
provide memory-mapped space of the root complex. The driver
correctly avoids computing the iomem address in this case, yet
submits a zeroed resource request anyway (via mfd_add_devices()).
Remove the iomem resource from the resource array submitted to the
mfd core for the older southbridges.
Cc: Aaron Sierra <[email protected]>
Cc: Peter Tyser <[email protected]>
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
v2: post-decrement to match existing style
retitle patch subject
drivers/mfd/lpc_ich.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index a22544f..f507c09 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
res = wdt_mem_res(ICH_RES_MEM_GCS);
res->start = base_addr + ACPIBASE_GCS_OFF;
res->end = base_addr + ACPIBASE_GCS_END;
+ } else {
+ /* So don't register iomem for TCO ver 1 */
+ lpc_ich_cells[LPC_WDT].num_resources--;
}
lpc_ich_finalize_cell(&lpc_ich_cells[LPC_WDT], id);
--
1.8.0
> v2: post-decrement to match existing style
> retitle patch subject
>
> drivers/mfd/lpc_ich.c | 3 +++
> 1 file changed, 3 insertions(+)
Acked-by: Aaron Sierra <[email protected]>
You could make Samuel's job easier by sending a new e-mail
with the latest patch and the correct subject in the e-mail's
subject line. Since this patch doesn't really depend on any of
the other four you submitted, a new message/subject like this
seems appropriate to me:
[PATCH v3] mfd: lpc_ich: Fix resource request for [mem 0x00000000]
The older southbridges supported by the lpc_ich driver do not
provide memory-mapped space of the root complex. The driver
correctly avoids computing the iomem address in this case, yet
submits a zeroed resource request anyway (via mfd_add_devices()).
Remove the iomem resource from the resource array submitted to the
mfd core for the older southbridges.
Acked-by: Aaron Sierra <[email protected]>
Cc: Peter Tyser <[email protected]>
Cc: Samuel Ortiz <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
v2: post-decrement to match existing style
retitle patch subject
v3: respin as standalone patch
drivers/mfd/lpc_ich.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index a22544f..f507c09 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
res = wdt_mem_res(ICH_RES_MEM_GCS);
res->start = base_addr + ACPIBASE_GCS_OFF;
res->end = base_addr + ACPIBASE_GCS_END;
+ } else {
+ /* So don't register iomem for TCO ver 1 */
+ lpc_ich_cells[LPC_WDT].num_resources--;
}
lpc_ich_finalize_cell(&lpc_ich_cells[LPC_WDT], id);
--
1.8.0
On Wed, Nov 7, 2012 at 7:55 PM, Peter Hurley <[email protected]> wrote:
> An incorrectly specified host bridge window may prevent
> other devices from claiming assigned resources. For example,
> this flawed _CRS resource descriptor from a Dell T5400:
> DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
> 0x00000000, // Granularity
> 0xF0000000, // Range Minimum
> 0xFE000000, // Range Maximum
> 0x00000000, // Translation Offset
> 0x0E000000, // Length
> ,, , AddressRangeMemory, TypeStatic)
I think the problem here is that the Range Maximum should be
0xFDFFFFFF, not 0xFE000000, right?
> prevents the adjacent device from claiming [mem 0xfe0000000-0xfe01ffff]
>
> Sanity check that the resource at least conforms to a valid
> PCI BAR; if not, emit a diagnostic warning.
>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: H. Peter Anvin <[email protected]>
> Cc: [email protected]
> Signed-off-by: Peter Hurley <[email protected]>
> ---
> arch/x86/pci/acpi.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index 192397c..3468d16 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -298,6 +298,10 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
> "host bridge window [%#llx-%#llx] "
> "([%#llx-%#llx] ignored, not CPU addressable)\n",
> start, orig_end, end + 1, orig_end);
> + } else if (flags & IORESOURCE_MEM && (start & 0x0f || ~end & 0x0f)) {
> + dev_warn(&info->bridge->dev,
> + "invalid host bridge window [%#llx-%#llx]\n",
> + start, end);
We didn't actually *fix* anything here, so I guess we're just pointing
out the reason for a subsequent failure to claim the adjacent
resource.
As far as I know, the spec doesn't actually require resources of ACPI
devices to be non-overlapping. Windows accepts overlapping resources,
and I think Linux probably should, too, but right now we trip over
this.
In the meantime (until we figure out how to handle overlapping
resources better), can we do something to actually fix this? Maybe we
should truncate the end of the range to 0xFDFFFFFF like we do for
non-addressable parts of the range?
Is there a bugzilla or a complete dmesg log to look at?
Bjorn
> }
>
> res = &info->res[info->res_num];
> --
> 1.7.12.3
>
On Sat, 2012-11-10 at 14:52 -0700, Bjorn Helgaas wrote:
> On Wed, Nov 7, 2012 at 7:55 PM, Peter Hurley <[email protected]> wrote:
> > An incorrectly specified host bridge window may prevent
> > other devices from claiming assigned resources. For example,
> > this flawed _CRS resource descriptor from a Dell T5400:
> > DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
> > 0x00000000, // Granularity
> > 0xF0000000, // Range Minimum
> > 0xFE000000, // Range Maximum
> > 0x00000000, // Translation Offset
> > 0x0E000000, // Length
> > ,, , AddressRangeMemory, TypeStatic)
>
> I think the problem here is that the Range Maximum should be
> 0xFDFFFFFF, not 0xFE000000, right?
I presume so.
> > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> > index 192397c..3468d16 100644
> > --- a/arch/x86/pci/acpi.c
> > +++ b/arch/x86/pci/acpi.c
> > @@ -298,6 +298,10 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
> > "host bridge window [%#llx-%#llx] "
> > "([%#llx-%#llx] ignored, not CPU addressable)\n",
> > start, orig_end, end + 1, orig_end);
> > + } else if (flags & IORESOURCE_MEM && (start & 0x0f || ~end & 0x0f)) {
> > + dev_warn(&info->bridge->dev,
> > + "invalid host bridge window [%#llx-%#llx]\n",
> > + start, end);
>
> We didn't actually *fix* anything here, so I guess we're just pointing
> out the reason for a subsequent failure to claim the adjacent
> resource.
Correct. There is no fix; only a diagnostic warning.
The warning is also a 'red flag' that, on this machine, it might be
better to boot the kernel with the "pci=nocrs" option.
> As far as I know, the spec doesn't actually require resources of ACPI
> devices to be non-overlapping. Windows accepts overlapping resources,
> and I think Linux probably should, too, but right now we trip over
> this.
(note: I included a link below to the defect report which has
the /proc/iomem, dmesg & dmidecode)
The situation is this:
The adjacent resources (northbridge & southbridge) are not defined by
ACPI, but rather reserved with an e820 address descriptor from
[0xfe000000-0xfeffffff], so strictly speaking there is no overlapping
ACPI resource.
The e820 descriptor is bumped out to [0xf0000000-0xfeffffff] and the
malformed host bridge window is reparented to it.
At this point in the boot, there is no resource conflict.
Later in the boot, the i5k_amb driver tries to map
[0xfe000000-0xfe01ffff] which is the FB-DIMM AMB register window on the
Intel 5400 MCH and is rejected. The request is rejected because the
requested range does not map completely to a single parent and this is
not allowed. (The i5k_amb driver exposes the FB-DIMM temperature sensors
through sysfs).
There is no problem in Windows because no driver attempts to allocate
[0xfe000000-0xfe01ffff]. However, I doubt the PNP Manager would allow
another bus pdo to claim an overlapping resource with PCI bus 0. I
suspect the offending device would yellow bang. (That would be an
interesting experiment...)
> In the meantime (until we figure out how to handle overlapping
> resources better), can we do something to actually fix this? Maybe we
> should truncate the end of the range to 0xFDFFFFFF like we do for
> non-addressable parts of the range?
Auto-fixing this seems problematic because it's essentially impossible
to determine if the resource length or the resource end or both is
wrong.
> Is there a bugzilla or a complete dmesg log to look at?
https://bugzilla.kernel.org/show_bug.cgi?id=50161
Regards,
Peter Hurley
Hi Peter,
On Fri, Nov 09, 2012 at 06:55:29AM -0500, Peter Hurley wrote:
> The older southbridges supported by the lpc_ich driver do not
> provide memory-mapped space of the root complex. The driver
> correctly avoids computing the iomem address in this case, yet
> submits a zeroed resource request anyway (via mfd_add_devices()).
>
> Remove the iomem resource from the resource array submitted to the
> mfd core for the older southbridges.
>
> Acked-by: Aaron Sierra <[email protected]>
> Cc: Peter Tyser <[email protected]>
> Cc: Samuel Ortiz <[email protected]>
> Signed-off-by: Peter Hurley <[email protected]>
> ---
>
> v2: post-decrement to match existing style
> retitle patch subject
> v3: respin as standalone patch
>
> drivers/mfd/lpc_ich.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index a22544f..f507c09 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
> res = wdt_mem_res(ICH_RES_MEM_GCS);
> res->start = base_addr + ACPIBASE_GCS_OFF;
> res->end = base_addr + ACPIBASE_GCS_END;
> + } else {
So I suppose there is no v3 for the iTCO ? If we're expecting all versions
after 1 to have a memory mapped region, we should have something like:
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -830,7 +830,10 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev
*dev,
* we have to read RCBA from PCI Config space 0xf0 and use
* it as base. GCS = RCBA + ICH6_GCS(0x3410).
*/
- if (lpc_chipset_info[id->driver_data].iTCO_version == 2) {
+ if (lpc_chipset_info[id->driver_data].iTCO_version == 1) {
+ /* Don't register iomem for TCO ver 1 */
+ lpc_ich_cells[LPC_WDT].num_resources--;
+ } else {
pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
base_addr = base_addr_cfg & 0xffffc000;
if (!(base_addr_cfg & 1)) {
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
On Mon, 2012-11-19 at 18:46 +0100, Samuel Ortiz wrote:
> Hi Peter,
>
> On Fri, Nov 09, 2012 at 06:55:29AM -0500, Peter Hurley wrote:
> > The older southbridges supported by the lpc_ich driver do not
> > provide memory-mapped space of the root complex. The driver
> > correctly avoids computing the iomem address in this case, yet
> > submits a zeroed resource request anyway (via mfd_add_devices()).
> >
> > Remove the iomem resource from the resource array submitted to the
> > mfd core for the older southbridges.
> >
> > Acked-by: Aaron Sierra <[email protected]>
> > Cc: Peter Tyser <[email protected]>
> > Cc: Samuel Ortiz <[email protected]>
> > Signed-off-by: Peter Hurley <[email protected]>
> > ---
> >
> > v2: post-decrement to match existing style
> > retitle patch subject
> > v3: respin as standalone patch
> >
> > drivers/mfd/lpc_ich.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > index a22544f..f507c09 100644
> > --- a/drivers/mfd/lpc_ich.c
> > +++ b/drivers/mfd/lpc_ich.c
> > @@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
> > res = wdt_mem_res(ICH_RES_MEM_GCS);
> > res->start = base_addr + ACPIBASE_GCS_OFF;
> > res->end = base_addr + ACPIBASE_GCS_END;
> > + } else {
> So I suppose there is no v3 for the iTCO ? If we're expecting all versions
> after 1 to have a memory mapped region, we should have something like:
>
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -830,7 +830,10 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev
> *dev,
> * we have to read RCBA from PCI Config space 0xf0 and use
> * it as base. GCS = RCBA + ICH6_GCS(0x3410).
> */
> - if (lpc_chipset_info[id->driver_data].iTCO_version == 2) {
> + if (lpc_chipset_info[id->driver_data].iTCO_version == 1) {
> + /* Don't register iomem for TCO ver 1 */
> + lpc_ich_cells[LPC_WDT].num_resources--;
> + } else {
> pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
> base_addr = base_addr_cfg & 0xffffc000;
> if (!(base_addr_cfg & 1)) {
Hi Samuel,
I have no objection to your version.
FWIW, the iTCO_version field is exclusively a driver construct used to
differentiate southbridges that support memory-mapped I/O to the TCO
registers from those that only support port-based I/O. IOW, there's no
intrinsic meaning to the values and could be represented with a bool
type instead.
Regards,
Peter Hurley
Hi Peter,
On Mon, Nov 19, 2012 at 01:28:53PM -0500, Peter Hurley wrote:
> On Mon, 2012-11-19 at 18:46 +0100, Samuel Ortiz wrote:
> > Hi Peter,
> >
> > On Fri, Nov 09, 2012 at 06:55:29AM -0500, Peter Hurley wrote:
> > > The older southbridges supported by the lpc_ich driver do not
> > > provide memory-mapped space of the root complex. The driver
> > > correctly avoids computing the iomem address in this case, yet
> > > submits a zeroed resource request anyway (via mfd_add_devices()).
> > >
> > > Remove the iomem resource from the resource array submitted to the
> > > mfd core for the older southbridges.
> > >
> > > Acked-by: Aaron Sierra <[email protected]>
> > > Cc: Peter Tyser <[email protected]>
> > > Cc: Samuel Ortiz <[email protected]>
> > > Signed-off-by: Peter Hurley <[email protected]>
> > > ---
> > >
> > > v2: post-decrement to match existing style
> > > retitle patch subject
> > > v3: respin as standalone patch
> > >
> > > drivers/mfd/lpc_ich.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > > index a22544f..f507c09 100644
> > > --- a/drivers/mfd/lpc_ich.c
> > > +++ b/drivers/mfd/lpc_ich.c
> > > @@ -842,6 +842,9 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev *dev,
> > > res = wdt_mem_res(ICH_RES_MEM_GCS);
> > > res->start = base_addr + ACPIBASE_GCS_OFF;
> > > res->end = base_addr + ACPIBASE_GCS_END;
> > > + } else {
> > So I suppose there is no v3 for the iTCO ? If we're expecting all versions
> > after 1 to have a memory mapped region, we should have something like:
> >
> > --- a/drivers/mfd/lpc_ich.c
> > +++ b/drivers/mfd/lpc_ich.c
> > @@ -830,7 +830,10 @@ static int __devinit lpc_ich_init_wdt(struct pci_dev
> > *dev,
> > * we have to read RCBA from PCI Config space 0xf0 and use
> > * it as base. GCS = RCBA + ICH6_GCS(0x3410).
> > */
> > - if (lpc_chipset_info[id->driver_data].iTCO_version == 2) {
> > + if (lpc_chipset_info[id->driver_data].iTCO_version == 1) {
> > + /* Don't register iomem for TCO ver 1 */
> > + lpc_ich_cells[LPC_WDT].num_resources--;
> > + } else {
> > pci_read_config_dword(dev, RCBABASE, &base_addr_cfg);
> > base_addr = base_addr_cfg & 0xffffc000;
> > if (!(base_addr_cfg & 1)) {
>
> Hi Samuel,
>
> I have no objection to your version.
I applied and pushed it to my for-next branch.
> FWIW, the iTCO_version field is exclusively a driver construct used to
> differentiate southbridges that support memory-mapped I/O to the TCO
> registers from those that only support port-based I/O. IOW, there's no
> intrinsic meaning to the values and could be represented with a bool
> type instead.
Right, that sounds like follow up patch material.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/