[0/5] remove ACPI motherboard driver, use PNP system driver instead

[0/5] remove ACPI motherboard driver, use PNP system driver instead

[Bjorn Helgaas]

The ACPI motherboard driver is mostly redundant with the PNP system
driver.  This series moves the little bit of ACPI-specific stuff out
of the motherboard driver into acpi/osl.c, adds a little bit to the
PNP system driver so it covers all the remaining functionality of
the ACPI driver, and removes the ACPI driver.

Feedback welcome.

Bjorn

[patch 1/5] ACPI: move FADT resource reservations from motherboard driver to osl

[Bjorn Helgaas]

Resources described by the FADT aren't really a good fit for the
ACPI motherboard driver.

The motherboard driver cares about PNP0C01 and PNP0C02 devices and
their resources.

The FADT describes some resources used by the ACPI core.  Often, they
are also described by by the _CRS of a motherboard device, but I think
it's better to reserve them specifically in the ACPI osl.c because
(a) the motherboard driver is optional and ACPI uses the resources even
if the driver is absent, and (b) I want to remove the ACPI motherboard
driver because it's mostly redundant with the PNP system.c driver.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: mm-work13/drivers/acpi/motherboard.c
===================================================================
--- mm-work13.orig/drivers/acpi/motherboard.c	2007-01-11 12:34:57.000000000 -0700
+++ mm-work13/drivers/acpi/motherboard.c	2007-01-11 12:35:28.000000000 -0700
@@ -118,58 +118,9 @@
 		},
 };
 
-static void __init acpi_request_region (struct acpi_generic_address *addr,
-	unsigned int length, char *desc)
-{
-	if (!addr->address || !length)
-		return;
-
-	if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_IO)
-		request_region(addr->address, length, desc);
-	else if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
-		request_mem_region(addr->address, length, desc);
-}
-
-static void __init acpi_reserve_resources(void)
-{
-	acpi_request_region(&acpi_gbl_FADT->xpm1a_evt_blk,
-			       acpi_gbl_FADT->pm1_evt_len, "ACPI PM1a_EVT_BLK");
-
-	acpi_request_region(&acpi_gbl_FADT->xpm1b_evt_blk,
-			       acpi_gbl_FADT->pm1_evt_len, "ACPI PM1b_EVT_BLK");
-
-	acpi_request_region(&acpi_gbl_FADT->xpm1a_cnt_blk,
-			       acpi_gbl_FADT->pm1_cnt_len, "ACPI PM1a_CNT_BLK");
-
-	acpi_request_region(&acpi_gbl_FADT->xpm1b_cnt_blk,
-			       acpi_gbl_FADT->pm1_cnt_len, "ACPI PM1b_CNT_BLK");
-
-	if (acpi_gbl_FADT->pm_tm_len == 4)
-		acpi_request_region(&acpi_gbl_FADT->xpm_tmr_blk, 4, "ACPI PM_TMR");
-
-	acpi_request_region(&acpi_gbl_FADT->xpm2_cnt_blk,
-			       acpi_gbl_FADT->pm2_cnt_len, "ACPI PM2_CNT_BLK");
-
-	/* Length of GPE blocks must be a non-negative multiple of 2 */
-
-	if (!(acpi_gbl_FADT->gpe0_blk_len & 0x1))
-		acpi_request_region(&acpi_gbl_FADT->xgpe0_blk,
-			       acpi_gbl_FADT->gpe0_blk_len, "ACPI GPE0_BLK");
-
-	if (!(acpi_gbl_FADT->gpe1_blk_len & 0x1))
-		acpi_request_region(&acpi_gbl_FADT->xgpe1_blk,
-			       acpi_gbl_FADT->gpe1_blk_len, "ACPI GPE1_BLK");
-}
-
 static int __init acpi_motherboard_init(void)
 {
 	acpi_bus_register_driver(&acpi_motherboard_driver);
-	/*
-	 * Guarantee motherboard IO reservation first
-	 * This module must run after scan.c
-	 */
-	if (!acpi_disabled)
-		acpi_reserve_resources();
 	return 0;
 }
 
Index: mm-work13/drivers/acpi/osl.c
===================================================================
--- mm-work13.orig/drivers/acpi/osl.c	2007-01-11 12:34:57.000000000 -0700
+++ mm-work13/drivers/acpi/osl.c	2007-01-11 12:37:10.000000000 -0700
@@ -75,6 +75,51 @@
 static void *acpi_irq_context;
 static struct workqueue_struct *kacpid_wq;
 
+static void __init acpi_request_region (struct acpi_generic_address *addr,
+	unsigned int length, char *desc)
+{
+	struct resource *res;
+
+	if (!addr->address || !length)
+		return;
+
+	if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_IO)
+		res = request_region(addr->address, length, desc);
+	else if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
+		res = request_mem_region(addr->address, length, desc);
+}
+
+static void __init acpi_reserve_resources(void)
+{
+	acpi_request_region(&acpi_fadt.xpm1a_evt_blk, acpi_fadt.pm1_evt_len,
+		"ACPI PM1a_EVT_BLK");
+
+	acpi_request_region(&acpi_fadt.xpm1b_evt_blk, acpi_fadt.pm1_evt_len,
+		"ACPI PM1b_EVT_BLK");
+
+	acpi_request_region(&acpi_fadt.xpm1a_cnt_blk, acpi_fadt.pm1_cnt_len,
+		"ACPI PM1a_CNT_BLK");
+
+	acpi_request_region(&acpi_fadt.xpm1b_cnt_blk, acpi_fadt.pm1_cnt_len,
+		"ACPI PM1b_CNT_BLK");
+
+	if (acpi_fadt.pm_tm_len == 4)
+		acpi_request_region(&acpi_fadt.xpm_tmr_blk, 4, "ACPI PM_TMR");
+
+	acpi_request_region(&acpi_fadt.xpm2_cnt_blk, acpi_fadt.pm2_cnt_len,
+		"ACPI PM2_CNT_BLK");
+
+	/* Length of GPE blocks must be a non-negative multiple of 2 */
+
+	if (!(acpi_fadt.gpe0_blk_len & 0x1))
+		acpi_request_region(&acpi_fadt.xgpe0_blk,
+			       acpi_fadt.gpe0_blk_len, "ACPI GPE0_BLK");
+
+	if (!(acpi_fadt.gpe1_blk_len & 0x1))
+		acpi_request_region(&acpi_fadt.xgpe1_blk,
+			       acpi_fadt.gpe1_blk_len, "ACPI GPE1_BLK");
+}
+
 acpi_status acpi_os_initialize(void)
 {
 	return AE_OK;
@@ -82,6 +127,8 @@
 
 acpi_status acpi_os_initialize1(void)
 {
+	acpi_reserve_resources();
+
 	/*
 	 * Initialize PCI configuration space access, as we'll need to access
 	 * it while walking the namespace (bus 0 and root bridges w/ _BBNs).

[patch 2/5] PNP: reserve system board iomem resources as well as ioport resources

[Bjorn Helgaas]

Most x86 boxes have no iomem system board resources, but some ia64
boxes do.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: mm-work3/drivers/pnp/system.c
===================================================================
--- mm-work3.orig/drivers/pnp/system.c	2007-01-11 15:56:57.000000000 -0700
+++ mm-work3/drivers/pnp/system.c	2007-01-12 12:14:53.000000000 -0700
@@ -3,7 +3,8 @@
  *
  * Some code is based on pnpbios_core.c
  * Copyright 2002 Adam Belay <ambx1@neo.rr.com>
- *
+ * (c) Copyright 2007 Hewlett-Packard Development Company, L.P.
+ *	Bjorn Helgaas <bjorn.helgaas@hp.com>
  */
 
 #include <linux/pnp.h>
@@ -21,7 +22,7 @@
 	{	"",			0	}
 };
 
-static void reserve_ioport_range(char *pnpid, int start, int end)
+static void reserve_range(char *pnpid, int start, int end, int port)
 {
 	struct resource *res;
 	char *regionid;
@@ -30,7 +31,10 @@
 	if ( regionid == NULL )
 		return;
 	snprintf(regionid, 16, "pnp %s", pnpid);
-	res = request_region(start,end-start+1,regionid);
+	if (port)
+		res = request_region(start,end-start+1,regionid);
+	else
+		res = request_mem_region(start,end-start+1,regionid);
 	if ( res == NULL )
 		kfree( regionid );
 	else
@@ -41,8 +45,8 @@
 	 * have double reservations.
 	 */
 	printk(KERN_INFO
-		"pnp: %s: ioport range 0x%x-0x%x %s reserved\n",
-		pnpid, start, end,
+		"pnp: %s: %s range 0x%x-0x%x %s reserved\n",
+		pnpid, port ? "ioport" : "iomem", start, end,
 		NULL != res ? "has been" : "could not be"
 	);
 
@@ -75,13 +79,21 @@
 			/* invalid endpoint */
 			/* Do nothing */
 			continue;
-		reserve_ioport_range(
+		reserve_range(
 			dev->dev.bus_id,
 			pnp_port_start(dev, i),
-			pnp_port_end(dev, i)
+			pnp_port_end(dev, i), 1
 		);
 	}
 
+	for (i = 0; i < PNP_MAX_MEM; i++) {
+		if (!pnp_mem_valid(dev, i))
+			continue;
+
+		reserve_range(dev->dev.bus_id, pnp_mem_start(dev, i),
+			pnp_mem_end(dev, i), 0);
+	}
+
 	return;
 }
 

[patch 3/5] PNP: system.c whitespace cleanup

[Bjorn Helgaas]

No functional change.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: mm-work3/drivers/pnp/system.c
===================================================================
--- mm-work3.orig/drivers/pnp/system.c	2007-01-12 12:14:53.000000000 -0700
+++ mm-work3/drivers/pnp/system.c	2007-01-12 12:16:59.000000000 -0700
@@ -28,15 +28,15 @@
 	char *regionid;
 
 	regionid = kmalloc(16, GFP_KERNEL);
-	if ( regionid == NULL )
+	if (regionid == NULL)
 		return;
 	snprintf(regionid, 16, "pnp %s", pnpid);
 	if (port)
 		res = request_region(start,end-start+1,regionid);
 	else
 		res = request_mem_region(start,end-start+1,regionid);
-	if ( res == NULL )
-		kfree( regionid );
+	if (res == NULL)
+		kfree(regionid);
 	else
 		res->flags &= ~IORESOURCE_BUSY;
 	/*
@@ -47,24 +47,18 @@
 	printk(KERN_INFO
 		"pnp: %s: %s range 0x%x-0x%x %s reserved\n",
 		pnpid, port ? "ioport" : "iomem", start, end,
-		NULL != res ? "has been" : "could not be"
-	);
-
-	return;
+		NULL != res ? "has been" : "could not be");
 }
 
-static void reserve_resources_of_dev( struct pnp_dev *dev )
+static void reserve_resources_of_dev(struct pnp_dev *dev)
 {
 	int i;
 
-	for (i=0;i<PNP_MAX_PORT;i++) {
+	for (i = 0; i < PNP_MAX_PORT; i++) {
 		if (!pnp_port_valid(dev, i))
-			/* end of resources */
 			continue;
 		if (pnp_port_start(dev, i) == 0)
-			/* disabled */
-			/* Do nothing */
-			continue;
+			continue;	/* disabled */
 		if (pnp_port_start(dev, i) < 0x100)
 			/*
 			 * Below 0x100 is only standard PC hardware
@@ -76,14 +70,10 @@
 			 */
 			continue;
 		if (pnp_port_end(dev, i) < pnp_port_start(dev, i))
-			/* invalid endpoint */
-			/* Do nothing */
-			continue;
-		reserve_range(
-			dev->dev.bus_id,
-			pnp_port_start(dev, i),
-			pnp_port_end(dev, i), 1
-		);
+			continue;	/* invalid */
+
+		reserve_range(dev->dev.bus_id, pnp_port_start(dev, i),
+			pnp_port_end(dev, i), 1);
 	}
 
 	for (i = 0; i < PNP_MAX_MEM; i++) {

[patch 4/5] i386: turn on CONFIG_PNP in defconfig

[Bjorn Helgaas]

I'm trying to remove drivers/acpi/motherboard.c, which is mostly
redundant with drivers/pnp/system.c.  So make sure that we include the
PNP driver in the default config.  Most distros enable this already.

Turning on CONFIG_PNP also causes the following options to be enabled:

    CONFIG_PNPACPI
    CONFIG_SERIAL_8250_PNP

CONFIG_SERIAL_8250_PNP causes legacy serial ports to be discovered
twice, which is ugly but harmless:

    serial8250: ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
    00:07: ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: mm-work13/arch/i386/defconfig
===================================================================
--- mm-work13.orig/arch/i386/defconfig	2007-01-11 13:56:18.000000000 -0700
+++ mm-work13/arch/i386/defconfig	2007-01-11 13:57:23.000000000 -0700
@@ -466,7 +466,7 @@
 #
 # Plug and Play support
 #
-# CONFIG_PNP is not set
+CONFIG_PNP=y
 
 #
 # Block devices

[patch 5/5] ACPI: remove motherboard driver (redundant with PNP system driver)

[Bjorn Helgaas]

The PNP system board driver (drivers/pnp/system.c) contains all the
same functionality, so we don't need the ACPI version.

Previously, a motherboard device would be claimed by *both* the ACPI and
PNP drivers, resulting in stuff like this in /proc/ioports:

    1200-121f : motherboard	<-- from drivers/acpi/motherboard.c
      1200-121f : pnp 00:0d	<-- from drivers/pnp/system.c

Make sure to enable CONFIG_PNP (and CONFIG_PNPACPI) to include the
PNP system board driver.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: mm-work13/drivers/acpi/Makefile
===================================================================
--- mm-work13.orig/drivers/acpi/Makefile	2007-01-08 12:03:18.000000000 -0700
+++ mm-work13/drivers/acpi/Makefile	2007-01-11 13:17:02.000000000 -0700
@@ -37,7 +37,7 @@
 
 obj-y				+= sleep/
 obj-y				+= bus.o glue.o
-obj-y				+= scan.o motherboard.o
+obj-y				+= scan.o
 obj-$(CONFIG_ACPI_AC) 		+= ac.o
 obj-$(CONFIG_ACPI_BATTERY)	+= battery.o
 obj-$(CONFIG_ACPI_BUTTON)	+= button.o
Index: mm-work13/drivers/acpi/motherboard.c
===================================================================
--- mm-work13.orig/drivers/acpi/motherboard.c	2007-01-11 12:35:28.000000000 -0700
+++ /dev/null	1970-01-01 00:00:00.000000000 +0000
@@ -1,131 +0,0 @@
-/* 
- * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or (at
- *  your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful, but
- *  WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- *  General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License along
- *  with this program; if not, write to the Free Software Foundation, Inc.,
- *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
- *
- * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- */
-
-/* Purpose: Prevent PCMCIA cards from using motherboard resources. */
-
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/types.h>
-#include <linux/pci.h>
-#include <linux/ioport.h>
-#include <asm/io.h>
-
-#include <acpi/acpi_bus.h>
-#include <acpi/acpi_drivers.h>
-
-#define _COMPONENT		ACPI_SYSTEM_COMPONENT
-ACPI_MODULE_NAME("acpi_motherboard")
-
-/* Dell use PNP0C01 instead of PNP0C02 */
-#define ACPI_MB_HID			"PNP0C01,PNP0C02"
-/**
- * Doesn't care about legacy IO ports, only IO ports beyond 0x1000 are reserved
- * Doesn't care about the failure of 'request_region', since other may reserve
- * the io ports as well
- */
-#define IS_RESERVED_ADDR(base, len) \
-	(((len) > 0) && ((base) > 0) && ((base) + (len) < IO_SPACE_LIMIT) \
-	&& ((base) + (len) > PCIBIOS_MIN_IO))
-/*
- * Clearing the flag (IORESOURCE_BUSY) allows drivers to use
- * the io ports if they really know they can use it, while
- * still preventing hotplug PCI devices from using it.
- */
-
-/*
- * When CONFIG_PNP is enabled, pnp/system.c binds to PNP0C01
- * and PNP0C02, redundant with acpi_reserve_io_ranges().
- * But acpi_reserve_io_ranges() is necessary for !CONFIG_PNP.
- */
-static acpi_status acpi_reserve_io_ranges(struct acpi_resource *res, void *data)
-{
-	struct resource *requested_res = NULL;
-
-
-	if (res->type == ACPI_RESOURCE_TYPE_IO) {
-		struct acpi_resource_io *io_res = &res->data.io;
-
-		if (io_res->minimum != io_res->maximum)
-			return AE_OK;
-		if (IS_RESERVED_ADDR
-		    (io_res->minimum, io_res->address_length)) {
-			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-					  "Motherboard resources 0x%08x - 0x%08x\n",
-					  io_res->minimum,
-					  io_res->minimum +
-					  io_res->address_length));
-			requested_res =
-			    request_region(io_res->minimum,
-					   io_res->address_length, "motherboard");
-		}
-	} else if (res->type == ACPI_RESOURCE_TYPE_FIXED_IO) {
-		struct acpi_resource_fixed_io *fixed_io_res =
-		    &res->data.fixed_io;
-
-		if (IS_RESERVED_ADDR
-		    (fixed_io_res->address, fixed_io_res->address_length)) {
-			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-					  "Motherboard resources 0x%08x - 0x%08x\n",
-					  fixed_io_res->address,
-					  fixed_io_res->address +
-					  fixed_io_res->address_length));
-			requested_res =
-			    request_region(fixed_io_res->address,
-					   fixed_io_res->address_length,
-					   "motherboard");
-		}
-	} else {
-		/* Memory mapped IO? */
-	}
-
-	if (requested_res)
-		requested_res->flags &= ~IORESOURCE_BUSY;
-	return AE_OK;
-}
-
-static int acpi_motherboard_add(struct acpi_device *device)
-{
-	if (!device)
-		return -EINVAL;
-	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
-			    acpi_reserve_io_ranges, NULL);
-
-	return 0;
-}
-
-static struct acpi_driver acpi_motherboard_driver = {
-	.name = "motherboard",
-	.class = "",
-	.ids = ACPI_MB_HID,
-	.ops = {
-		.add = acpi_motherboard_add,
-		},
-};
-
-static int __init acpi_motherboard_init(void)
-{
-	acpi_bus_register_driver(&acpi_motherboard_driver);
-	return 0;
-}
-
-/**
- * Reserve motherboard resources after PCI claim BARs,
- * but before PCI assign resources for uninitialized PCI devices
- */
-fs_initcall(acpi_motherboard_init);

Re: [patch 1/5] ACPI: move FADT resource reservations from motherboard driver to osl

[Shaohua Li]

On Tue, 2007-01-16 at 10:33 -0700, Bjorn Helgaas wrote:
> Resources described by the FADT aren't really a good fit for the
> ACPI motherboard driver.
> 
> The motherboard driver cares about PNP0C01 and PNP0C02 devices and
> their resources.
> 
> The FADT describes some resources used by the ACPI core.  Often, they
> are also described by by the _CRS of a motherboard device, but I think
> it's better to reserve them specifically in the ACPI osl.c because
> (a) the motherboard driver is optional and ACPI uses the resources even
> if the driver is absent, and (b) I want to remove the ACPI motherboard
> driver because it's mostly redundant with the PNP system.c driver.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
Thanks for the patch set, this is in the wish list for a long time.

> +
>  acpi_status acpi_os_initialize(void)
>  {
>  	return AE_OK;
> @@ -82,6 +127,8 @@
>  
>  acpi_status acpi_os_initialize1(void)
>  {
> +	acpi_reserve_resources();
This sounds a little earlier. motherboard device usually returns a super
set of the acpi io ports. reserving acpi io ports first might make some
motherboard io port isn't reserved as there might be conflict.

Thanks,
Shaohua

Re: [patch 4/5] i386: turn on CONFIG_PNP in defconfig

[Len Brown]

On Tuesday 16 January 2007 12:34, Bjorn Helgaas wrote:
> I'm trying to remove drivers/acpi/motherboard.c, which is mostly
> redundant with drivers/pnp/system.c.  So make sure that we include the
> PNP driver in the default config.  Most distros enable this already.
> 
> Turning on CONFIG_PNP also causes the following options to be enabled:
> 
>     CONFIG_PNPACPI
>     CONFIG_SERIAL_8250_PNP
> 
> CONFIG_SERIAL_8250_PNP causes legacy serial ports to be discovered
> twice, which is ugly but harmless:
> 
>     serial8250: ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
>     00:07: ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> Index: mm-work13/arch/i386/defconfig
> ===================================================================
> --- mm-work13.orig/arch/i386/defconfig	2007-01-11 13:56:18.000000000 -0700
> +++ mm-work13/arch/i386/defconfig	2007-01-11 13:57:23.000000000 -0700
> @@ -466,7 +466,7 @@
>  #
>  # Plug and Play support
>  #
> -# CONFIG_PNP is not set
> +CONFIG_PNP=y
>  
>  #
>  # Block devices

shouldn't CONFIG_PNPACPI=y also appear in defconfig with this change?
If it doesn't, then somebody who runs make oldconfig will get prompted for it.

Also, do we need to be concerned about the case where
CONFIG_PNP=n
CONFIG_ACPI=y
This is the case that motherboard.c was intended to cover.

CONFIG_PNP depends on ACPI (or ISA)
But nothing mandates that somebody must select it.

what do you think?

-Len


-Len

Re: [patch 1/5] ACPI: move FADT resource reservations from motherboard driver to osl

[Bjorn Helgaas]

On Tuesday 16 January 2007 18:59, Shaohua Li wrote:
> >  acpi_status acpi_os_initialize1(void)
> >  {
> > +	acpi_reserve_resources();
> This sounds a little earlier. motherboard device usually returns a super
> set of the acpi io ports. reserving acpi io ports first might make some
> motherboard io port isn't reserved as there might be conflict.

That's true; this does reserve the FADT resources earlier, and there
are often conflicts.

I'll try to figure out a way to reserve the FADT resources later.

Re: [patch 4/5] i386: turn on CONFIG_PNP in defconfig

[Bjorn Helgaas]

On Wednesday 17 January 2007 20:08, Len Brown wrote:
> > --- mm-work13.orig/arch/i386/defconfig	2007-01-11 13:56:18.000000000 -0700
> > +++ mm-work13/arch/i386/defconfig	2007-01-11 13:57:23.000000000 -0700
> > @@ -466,7 +466,7 @@
> >  #
> >  # Plug and Play support
> >  #
> > -# CONFIG_PNP is not set
> > +CONFIG_PNP=y
> >  
> >  #
> >  # Block devices
> 
> shouldn't CONFIG_PNPACPI=y also appear in defconfig with this change?
> If it doesn't, then somebody who runs make oldconfig will get prompted for it.

Yes, you're right.  I'll add PNPACPI=y in the next round.

> Also, do we need to be concerned about the case where
> CONFIG_PNP=n
> CONFIG_ACPI=y
> This is the case that motherboard.c was intended to cover.
> 
> CONFIG_PNP depends on ACPI (or ISA)
> But nothing mandates that somebody must select it.

Hmmm... true.  Could we get away with having ACPI select PNP?

Two other, longer term thoughts:
  1) I think it would be good if PNP reserved resources for all
     active devices, as PCI already does (though PCI might do it
     even for disabled devices).  Then we really wouldn't need
     system.c or motherboard.c at all.

  2) I think we should deprecate ACPI driver registration and do
     everything through PNP, with the ACPI stuff as an optional
     capability of PNP devices.  Then PNP=n, ACPI=y really wouldn't
     make much sense.

Bjorn

Re: [patch 4/5] i386: turn on CONFIG_PNP in defconfig

[Len Brown]

On Thursday 18 January 2007 12:49, Bjorn Helgaas wrote:
> On Wednesday 17 January 2007 20:08, Len Brown wrote:
> > > --- mm-work13.orig/arch/i386/defconfig	2007-01-11 13:56:18.000000000 -0700
> > > +++ mm-work13/arch/i386/defconfig	2007-01-11 13:57:23.000000000 -0700
> > > @@ -466,7 +466,7 @@
> > >  #
> > >  # Plug and Play support
> > >  #
> > > -# CONFIG_PNP is not set
> > > +CONFIG_PNP=y
> > >  
> > >  #
> > >  # Block devices
> > 
> > shouldn't CONFIG_PNPACPI=y also appear in defconfig with this change?
> > If it doesn't, then somebody who runs make oldconfig will get prompted for it.
> 
> Yes, you're right.  I'll add PNPACPI=y in the next round.

Hmm, I guess if we're going to include it by default,
it is time to delete its dependency on EXPERIMENTAL:
Though so many things depend on EXPERIMENTAL
these days that maybe it doesn't mean so much.

config PNPACPI
        bool "Plug and Play ACPI support (EXPERIMENTAL)"
        depends on PNP && ACPI && EXPERIMENTAL

> > Also, do we need to be concerned about the case where
> > CONFIG_PNP=n
> > CONFIG_ACPI=y
> > This is the case that motherboard.c was intended to cover.
> > 
> > CONFIG_PNP depends on ACPI (or ISA)
> > But nothing mandates that somebody must select it.
> 
> Hmmm... true.  Could we get away with having ACPI select PNP?

Every time I've tried to use select in recent memory it seems
something goes bad.  I think there is a rule, such as the
thing selected can not depend on anything else, which if violated
breaks people's ranconfigs and adrian get grumpy about it.

So I think if we want to tie them together, then ACPI
should actually depend on PNP -- which is consistent,
with how ACPI depends on PM today.

 
> Two other, longer term thoughts:
>   1) I think it would be good if PNP reserved resources for all
>      active devices, as PCI already does (though PCI might do it
>      even for disabled devices).  Then we really wouldn't need
>      system.c or motherboard.c at all.
> 
>   2) I think we should deprecate ACPI driver registration and do
>      everything through PNP, with the ACPI stuff as an optional
>      capability of PNP devices.  Then PNP=n, ACPI=y really wouldn't
>      make much sense.

Whelp, when we're guaranteed that PNP exists when ACPI exists
then we can get smarter about PNP.

thanks,
-Len

Re: [patch 1/5] ACPI: move FADT resource reservations from motherboard driver to osl

[Bjorn Helgaas]

On Sunday 21 January 2007 18:27, Shaohua Li wrote:
> On Fri, 2007-01-19 at 12:46 -0700, Bjorn Helgaas wrote:
> > On Thursday 18 January 2007 18:12, Shaohua Li wrote:
> > > > +device_initcall(acpi_reserve_resources);
> > > Will this be called before PCI assigned resources to PCI devices? We use
> > > fs_initcall in motherboard to avoid PCI devices use motherboard's
> > > resources before.
> > 
> > I think we're OK.  Here's the current order:
> > 
> >   pnp_system_init            fs_initcall       drivers/pnp/system.c
> >   pcibios_assign_resources   fs_initcall       arch/i386/pci/i386.c
> >   acpi_reserve_resources     device_initcall   drivers/acpi/osl.c
> > 
> > So pnp_system_init() will reserve all the motherboard resources before
> > the PCI resources are assigned.  What do you think?
>
> In this way, we could just remove acpi_reserve_resources as pci already
> assigns resources to pci devices. The concern is motherboard might not
> list all acpi resources in its _CRS, but I don't know if there are such
> BIOS. Microsoft seems to reserve motherboard resources in the same way.
> Maybe we could just remove acpi_reserve_resources and give it a spin in
> -mm to see if anybody complains.

I agree that in theory we should be able to remove acpi_reserve_resources()
and just depend on pnp_system_init() reserving all the motherboard
resources.  But your concern is very valid: it looks like both HP DL360
(x86) and rx2600 (ia64) firmware list things in the FADT that aren't
included in _CRS for motherboard devices.  I think these are BIOS bugs,
but for now, it seems safest to keep acpi_reserve_resources() to work
around the bugs.

Bjorn

Re: [patch 1/5] ACPI: move FADT resource reservations from motherboard driver to osl

[Shaohua Li]

On Fri, 2007-01-19 at 12:46 -0700, Bjorn Helgaas wrote:
> On Thursday 18 January 2007 18:12, Shaohua Li wrote:
> > > +device_initcall(acpi_reserve_resources);
> > Will this be called before PCI assigned resources to PCI devices? We use
> > fs_initcall in motherboard to avoid PCI devices use motherboard's
> > resources before.
> 
> I think we're OK.  Here's the current order:
> 
>   pnp_system_init            fs_initcall       drivers/pnp/system.c
>   pcibios_assign_resources   fs_initcall       arch/i386/pci/i386.c
>   acpi_reserve_resources     device_initcall   drivers/acpi/osl.c
> 
> So pnp_system_init() will reserve all the motherboard resources before
> the PCI resources are assigned.  What do you think?
In this way, we could just remove acpi_reserve_resources as pci already
assigns resources to pci devices. The concern is motherboard might not
list all acpi resources in its _CRS, but I don't know if there are such
BIOS. Microsoft seems to reserve motherboard resources in the same way.
Maybe we could just remove acpi_reserve_resources and give it a spin in
-mm to see if anybody complains.

Thanks,
Shaohua

Re: [patch 1/5] ACPI: move FADT resource reservations from motherboard driver to osl

[Bjorn Helgaas]

On Thursday 18 January 2007 18:12, Shaohua Li wrote:
> > +device_initcall(acpi_reserve_resources);
> Will this be called before PCI assigned resources to PCI devices? We use
> fs_initcall in motherboard to avoid PCI devices use motherboard's
> resources before.

I think we're OK.  Here's the current order:

  pnp_system_init            fs_initcall       drivers/pnp/system.c
  pcibios_assign_resources   fs_initcall       arch/i386/pci/i386.c
  acpi_reserve_resources     device_initcall   drivers/acpi/osl.c

So pnp_system_init() will reserve all the motherboard resources before
the PCI resources are assigned.  What do you think?

Bjorn

Re: [patch 1/5] ACPI: move FADT resource reservations from motherboard driver to osl

[Shaohua Li]

On Thu, 2007-01-18 at 16:42 -0700, Bjorn Helgaas wrote:
> Resources described by the FADT aren't really a good fit for the
> ACPI motherboard driver.
> 
> The motherboard driver cares about PNP0C01 and PNP0C02 devices and
> their resources.
> 
> The FADT describes some resources used by the ACPI core.  Often, they
> are also described by by the _CRS of a motherboard device, but I think
> it's better to reserve them specifically in the ACPI osl.c because
> (a) the motherboard driver is optional and ACPI uses the resources even
> if the driver is absent, and (b) I want to remove the ACPI motherboard
> driver because it's mostly redundant with the PNP system.c driver.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> Index: mm-work3/drivers/acpi/motherboard.c
> ===================================================================
> --- mm-work3.orig/drivers/acpi/motherboard.c	2007-01-14 14:52:30.000000000 -0700
> +++ mm-work3/drivers/acpi/motherboard.c	2007-01-18 10:21:13.000000000 -0700
> @@ -118,58 +118,9 @@
>  		},
>  };
>  
> -static void __init acpi_request_region (struct acpi_generic_address *addr,
> -	unsigned int length, char *desc)
> -{
> -	if (!addr->address || !length)
> -		return;
> -
> -	if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_IO)
> -		request_region(addr->address, length, desc);
> -	else if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> -		request_mem_region(addr->address, length, desc);
> -}
> -
> -static void __init acpi_reserve_resources(void)
> -{
> -	acpi_request_region(&acpi_gbl_FADT->xpm1a_evt_blk,
> -			       acpi_gbl_FADT->pm1_evt_len, "ACPI PM1a_EVT_BLK");
> -
> -	acpi_request_region(&acpi_gbl_FADT->xpm1b_evt_blk,
> -			       acpi_gbl_FADT->pm1_evt_len, "ACPI PM1b_EVT_BLK");
> -
> -	acpi_request_region(&acpi_gbl_FADT->xpm1a_cnt_blk,
> -			       acpi_gbl_FADT->pm1_cnt_len, "ACPI PM1a_CNT_BLK");
> -
> -	acpi_request_region(&acpi_gbl_FADT->xpm1b_cnt_blk,
> -			       acpi_gbl_FADT->pm1_cnt_len, "ACPI PM1b_CNT_BLK");
> -
> -	if (acpi_gbl_FADT->pm_tm_len == 4)
> -		acpi_request_region(&acpi_gbl_FADT->xpm_tmr_blk, 4, "ACPI PM_TMR");
> -
> -	acpi_request_region(&acpi_gbl_FADT->xpm2_cnt_blk,
> -			       acpi_gbl_FADT->pm2_cnt_len, "ACPI PM2_CNT_BLK");
> -
> -	/* Length of GPE blocks must be a non-negative multiple of 2 */
> -
> -	if (!(acpi_gbl_FADT->gpe0_blk_len & 0x1))
> -		acpi_request_region(&acpi_gbl_FADT->xgpe0_blk,
> -			       acpi_gbl_FADT->gpe0_blk_len, "ACPI GPE0_BLK");
> -
> -	if (!(acpi_gbl_FADT->gpe1_blk_len & 0x1))
> -		acpi_request_region(&acpi_gbl_FADT->xgpe1_blk,
> -			       acpi_gbl_FADT->gpe1_blk_len, "ACPI GPE1_BLK");
> -}
> -
>  static int __init acpi_motherboard_init(void)
>  {
>  	acpi_bus_register_driver(&acpi_motherboard_driver);
> -	/*
> -	 * Guarantee motherboard IO reservation first
> -	 * This module must run after scan.c
> -	 */
> -	if (!acpi_disabled)
> -		acpi_reserve_resources();
>  	return 0;
>  }
>  
> Index: mm-work3/drivers/acpi/osl.c
> ===================================================================
> --- mm-work3.orig/drivers/acpi/osl.c	2007-01-14 14:52:30.000000000 -0700
> +++ mm-work3/drivers/acpi/osl.c	2007-01-18 14:39:47.000000000 -0700
> @@ -75,6 +75,54 @@
>  static void *acpi_irq_context;
>  static struct workqueue_struct *kacpid_wq;
>  
> +static void __init acpi_request_region (struct acpi_generic_address *addr,
> +	unsigned int length, char *desc)
> +{
> +	struct resource *res;
> +
> +	if (!addr->address || !length)
> +		return;
> +
> +	if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_IO)
> +		res = request_region(addr->address, length, desc);
> +	else if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> +		res = request_mem_region(addr->address, length, desc);
> +}
> +
> +static int __init acpi_reserve_resources(void)
> +{
> +	acpi_request_region(&acpi_fadt.xpm1a_evt_blk, acpi_fadt.pm1_evt_len,
> +		"ACPI PM1a_EVT_BLK");
> +
> +	acpi_request_region(&acpi_fadt.xpm1b_evt_blk, acpi_fadt.pm1_evt_len,
> +		"ACPI PM1b_EVT_BLK");
> +
> +	acpi_request_region(&acpi_fadt.xpm1a_cnt_blk, acpi_fadt.pm1_cnt_len,
> +		"ACPI PM1a_CNT_BLK");
> +
> +	acpi_request_region(&acpi_fadt.xpm1b_cnt_blk, acpi_fadt.pm1_cnt_len,
> +		"ACPI PM1b_CNT_BLK");
> +
> +	if (acpi_fadt.pm_tm_len == 4)
> +		acpi_request_region(&acpi_fadt.xpm_tmr_blk, 4, "ACPI PM_TMR");
> +
> +	acpi_request_region(&acpi_fadt.xpm2_cnt_blk, acpi_fadt.pm2_cnt_len,
> +		"ACPI PM2_CNT_BLK");
> +
> +	/* Length of GPE blocks must be a non-negative multiple of 2 */
> +
> +	if (!(acpi_fadt.gpe0_blk_len & 0x1))
> +		acpi_request_region(&acpi_fadt.xgpe0_blk,
> +			       acpi_fadt.gpe0_blk_len, "ACPI GPE0_BLK");
> +
> +	if (!(acpi_fadt.gpe1_blk_len & 0x1))
> +		acpi_request_region(&acpi_fadt.xgpe1_blk,
> +			       acpi_fadt.gpe1_blk_len, "ACPI GPE1_BLK");
> +
> +	return 0;
> +}
> +device_initcall(acpi_reserve_resources);
Will this be called before PCI assigned resources to PCI devices? We use
fs_initcall in motherboard to avoid PCI devices use motherboard's
resources before.

Thanks,
Shaohua

[patch 1/5] ACPI: move FADT resource reservations from motherboard driver to osl

[Bjorn Helgaas]

Resources described by the FADT aren't really a good fit for the
ACPI motherboard driver.

The motherboard driver cares about PNP0C01 and PNP0C02 devices and
their resources.

The FADT describes some resources used by the ACPI core.  Often, they
are also described by by the _CRS of a motherboard device, but I think
it's better to reserve them specifically in the ACPI osl.c because
(a) the motherboard driver is optional and ACPI uses the resources even
if the driver is absent, and (b) I want to remove the ACPI motherboard
driver because it's mostly redundant with the PNP system.c driver.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: mm-work3/drivers/acpi/motherboard.c
===================================================================
--- mm-work3.orig/drivers/acpi/motherboard.c	2007-01-14 14:52:30.000000000 -0700
+++ mm-work3/drivers/acpi/motherboard.c	2007-01-18 10:21:13.000000000 -0700
@@ -118,58 +118,9 @@
 		},
 };
 
-static void __init acpi_request_region (struct acpi_generic_address *addr,
-	unsigned int length, char *desc)
-{
-	if (!addr->address || !length)
-		return;
-
-	if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_IO)
-		request_region(addr->address, length, desc);
-	else if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
-		request_mem_region(addr->address, length, desc);
-}
-
-static void __init acpi_reserve_resources(void)
-{
-	acpi_request_region(&acpi_gbl_FADT->xpm1a_evt_blk,
-			       acpi_gbl_FADT->pm1_evt_len, "ACPI PM1a_EVT_BLK");
-
-	acpi_request_region(&acpi_gbl_FADT->xpm1b_evt_blk,
-			       acpi_gbl_FADT->pm1_evt_len, "ACPI PM1b_EVT_BLK");
-
-	acpi_request_region(&acpi_gbl_FADT->xpm1a_cnt_blk,
-			       acpi_gbl_FADT->pm1_cnt_len, "ACPI PM1a_CNT_BLK");
-
-	acpi_request_region(&acpi_gbl_FADT->xpm1b_cnt_blk,
-			       acpi_gbl_FADT->pm1_cnt_len, "ACPI PM1b_CNT_BLK");
-
-	if (acpi_gbl_FADT->pm_tm_len == 4)
-		acpi_request_region(&acpi_gbl_FADT->xpm_tmr_blk, 4, "ACPI PM_TMR");
-
-	acpi_request_region(&acpi_gbl_FADT->xpm2_cnt_blk,
-			       acpi_gbl_FADT->pm2_cnt_len, "ACPI PM2_CNT_BLK");
-
-	/* Length of GPE blocks must be a non-negative multiple of 2 */
-
-	if (!(acpi_gbl_FADT->gpe0_blk_len & 0x1))
-		acpi_request_region(&acpi_gbl_FADT->xgpe0_blk,
-			       acpi_gbl_FADT->gpe0_blk_len, "ACPI GPE0_BLK");
-
-	if (!(acpi_gbl_FADT->gpe1_blk_len & 0x1))
-		acpi_request_region(&acpi_gbl_FADT->xgpe1_blk,
-			       acpi_gbl_FADT->gpe1_blk_len, "ACPI GPE1_BLK");
-}
-
 static int __init acpi_motherboard_init(void)
 {
 	acpi_bus_register_driver(&acpi_motherboard_driver);
-	/*
-	 * Guarantee motherboard IO reservation first
-	 * This module must run after scan.c
-	 */
-	if (!acpi_disabled)
-		acpi_reserve_resources();
 	return 0;
 }
 
Index: mm-work3/drivers/acpi/osl.c
===================================================================
--- mm-work3.orig/drivers/acpi/osl.c	2007-01-14 14:52:30.000000000 -0700
+++ mm-work3/drivers/acpi/osl.c	2007-01-18 14:39:47.000000000 -0700
@@ -75,6 +75,54 @@
 static void *acpi_irq_context;
 static struct workqueue_struct *kacpid_wq;
 
+static void __init acpi_request_region (struct acpi_generic_address *addr,
+	unsigned int length, char *desc)
+{
+	struct resource *res;
+
+	if (!addr->address || !length)
+		return;
+
+	if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_IO)
+		res = request_region(addr->address, length, desc);
+	else if (addr->address_space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
+		res = request_mem_region(addr->address, length, desc);
+}
+
+static int __init acpi_reserve_resources(void)
+{
+	acpi_request_region(&acpi_fadt.xpm1a_evt_blk, acpi_fadt.pm1_evt_len,
+		"ACPI PM1a_EVT_BLK");
+
+	acpi_request_region(&acpi_fadt.xpm1b_evt_blk, acpi_fadt.pm1_evt_len,
+		"ACPI PM1b_EVT_BLK");
+
+	acpi_request_region(&acpi_fadt.xpm1a_cnt_blk, acpi_fadt.pm1_cnt_len,
+		"ACPI PM1a_CNT_BLK");
+
+	acpi_request_region(&acpi_fadt.xpm1b_cnt_blk, acpi_fadt.pm1_cnt_len,
+		"ACPI PM1b_CNT_BLK");
+
+	if (acpi_fadt.pm_tm_len == 4)
+		acpi_request_region(&acpi_fadt.xpm_tmr_blk, 4, "ACPI PM_TMR");
+
+	acpi_request_region(&acpi_fadt.xpm2_cnt_blk, acpi_fadt.pm2_cnt_len,
+		"ACPI PM2_CNT_BLK");
+
+	/* Length of GPE blocks must be a non-negative multiple of 2 */
+
+	if (!(acpi_fadt.gpe0_blk_len & 0x1))
+		acpi_request_region(&acpi_fadt.xgpe0_blk,
+			       acpi_fadt.gpe0_blk_len, "ACPI GPE0_BLK");
+
+	if (!(acpi_fadt.gpe1_blk_len & 0x1))
+		acpi_request_region(&acpi_fadt.xgpe1_blk,
+			       acpi_fadt.gpe1_blk_len, "ACPI GPE1_BLK");
+
+	return 0;
+}
+device_initcall(acpi_reserve_resources);
+
 acpi_status acpi_os_initialize(void)
 {
 	return AE_OK;