This driver currently creates resources for use by a forthcoming ICH
chipset GPIO driver. It could be expanded to created the resources for
converting the esb2rom (mtd) and iTCO_wdt (wdt), and potentially more,
drivers to use the mfd model.
Signed-off-by: Aaron Sierra <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/mfd/Kconfig | 9 +
drivers/mfd/Makefile | 1 +
drivers/mfd/lpc_ich.c | 525 +++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/lpc_ich.h | 32 +++
4 files changed, 567 insertions(+), 0 deletions(-)
create mode 100644 drivers/mfd/lpc_ich.c
create mode 100644 include/linux/mfd/lpc_ich.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index cd13e9f..f581e59 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -724,6 +724,15 @@ config LPC_SCH
LPC bridge function of the Intel SCH provides support for
System Management Bus and General Purpose I/O.
+config LPC_ICH
+ tristate "Intel ICH LPC"
+ depends on PCI
+ select MFD_CORE
+ help
+ The LPC bridge function of the Intel ICH provides support for
+ many functional units. This driver provides needed support for
+ other drivers to control these functions, currently GPIO.
+
config MFD_RDC321X
tristate "Support for RDC-R321x southbridge"
select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b953bab..6636c5a 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -98,6 +98,7 @@ obj-$(CONFIG_MFD_DB5500_PRCMU) += db5500-prcmu.o
obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
obj-$(CONFIG_LPC_SCH) += lpc_sch.o
+obj-$(CONFIG_LPC_ICH) += lpc_ich.o
obj-$(CONFIG_MFD_RDC321X) += rdc321x-southbridge.o
obj-$(CONFIG_MFD_JANZ_CMODIO) += janz-cmodio.o
obj-$(CONFIG_MFD_JZ4740_ADC) += jz4740-adc.o
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
new file mode 100644
index 0000000..7cfcab4
--- /dev/null
+++ b/drivers/mfd/lpc_ich.c
@@ -0,0 +1,525 @@
+/*
+ * lpc_ich.c - LPC interface for Intel ICH
+ *
+ * LPC bridge function of the Intel ICH contains many other
+ * functional units, such as Interrupt controllers, Timers,
+ * Power Management, System Management, GPIO, RTC, and LPC
+ * Configuration Registers.
+ *
+ * This driver is derived from lpc_sch.
+
+ * Copyright (c) 2011 Extreme Engineering Solution, Inc.
+ * Author: Aaron Sierra <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License 2 as published
+ * by the Free Software Foundation.
+ *
+ * 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; see the file COPYING. If not, write to
+ * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ * This driver supports the following I/O Controller hubs:
+ * (See the intel documentation on http://developer.intel.com.)
+ * document number 290655-003, 290677-014: 82801AA (ICH), 82801AB (ICHO)
+ * document number 290687-002, 298242-027: 82801BA (ICH2)
+ * document number 290733-003, 290739-013: 82801CA (ICH3-S)
+ * document number 290716-001, 290718-007: 82801CAM (ICH3-M)
+ * document number 290744-001, 290745-025: 82801DB (ICH4)
+ * document number 252337-001, 252663-008: 82801DBM (ICH4-M)
+ * document number 273599-001, 273645-002: 82801E (C-ICH)
+ * document number 252516-001, 252517-028: 82801EB (ICH5), 82801ER (ICH5R)
+ * document number 300641-004, 300884-013: 6300ESB
+ * document number 301473-002, 301474-026: 82801F (ICH6)
+ * document number 313082-001, 313075-006: 631xESB, 632xESB
+ * document number 307013-003, 307014-024: 82801G (ICH7)
+ * document number 322896-001, 322897-001: NM10
+ * document number 313056-003, 313057-017: 82801H (ICH8)
+ * document number 316972-004, 316973-012: 82801I (ICH9)
+ * document number 319973-002, 319974-002: 82801J (ICH10)
+ * document number 322169-001, 322170-003: 5 Series, 3400 Series (PCH)
+ * document number 320066-003, 320257-008: EP80597 (IICH)
+ * document number 324645-001, 324646-001: Cougar Point (CPT)
+ * document number TBD : Patsburg (PBG)
+ * document number TBD : DH89xxCC
+ * document number TBD : Panther Point
+ * document number TBD : Lynx Point
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/acpi.h>
+#include <linux/pci.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/lpc_ich.h>
+
+#define ACPIBASE 0x40
+#define ACPIBASE_GPE_OFF 0x20
+#define ACPIBASE_GPE_END 0x2f
+#define ACPICTRL 0x44
+
+#define GPIOBASE 0x48
+#define GPIOCTRL 0x4C
+#define GPIOBASE_IO_SIZE 0x80
+
+static int lpc_ich_acpi_save = -1;
+static int lpc_ich_gpio_save = -1;
+
+static struct resource gpio_ich_res[] = {
+ /* BASE */
+ {
+ .flags = IORESOURCE_IO,
+ },
+ /* ACPI */
+ {
+ .flags = IORESOURCE_IO,
+ },
+};
+
+enum lpc_cells {
+ LPC_GPIO = 0,
+};
+
+static struct mfd_cell lpc_ich_cells[] = {
+ [LPC_GPIO] = {
+ .name = "gpio_ich",
+ .num_resources = ARRAY_SIZE(gpio_ich_res),
+ .resources = gpio_ich_res,
+ },
+};
+
+/* chipset related info */
+enum lpc_chipsets {
+ LPC_ICH = 0, /* ICH */
+ LPC_ICH0, /* ICH0 */
+ LPC_ICH2, /* ICH2 */
+ LPC_ICH2M, /* ICH2-M */
+ LPC_ICH3, /* ICH3-S */
+ LPC_ICH3M, /* ICH3-M */
+ LPC_ICH4, /* ICH4 */
+ LPC_ICH4M, /* ICH4-M */
+ LPC_CICH, /* C-ICH */
+ LPC_ICH5, /* ICH5 & ICH5R */
+ LPC_6300ESB, /* 6300ESB */
+ LPC_ICH6, /* ICH6 & ICH6R */
+ LPC_ICH6M, /* ICH6-M */
+ LPC_ICH6W, /* ICH6W & ICH6RW */
+ LPC_631XESB, /* 631xESB/632xESB */
+ LPC_ICH7, /* ICH7 & ICH7R */
+ LPC_ICH7DH, /* ICH7DH */
+ LPC_ICH7M, /* ICH7-M & ICH7-U */
+ LPC_ICH7MDH, /* ICH7-M DH */
+ LPC_NM10, /* NM10 */
+ LPC_ICH8, /* ICH8 & ICH8R */
+ LPC_ICH8DH, /* ICH8DH */
+ LPC_ICH8DO, /* ICH8DO */
+ LPC_ICH8M, /* ICH8M */
+ LPC_ICH8ME, /* ICH8M-E */
+ LPC_ICH9, /* ICH9 */
+ LPC_ICH9R, /* ICH9R */
+ LPC_ICH9DH, /* ICH9DH */
+ LPC_ICH9DO, /* ICH9DO */
+ LPC_ICH9M, /* ICH9M */
+ LPC_ICH9ME, /* ICH9M-E */
+ LPC_ICH10, /* ICH10 */
+ LPC_ICH10R, /* ICH10R */
+ LPC_ICH10D, /* ICH10D */
+ LPC_ICH10DO, /* ICH10DO */
+ LPC_PCH, /* PCH Desktop Full Featured */
+ LPC_PCHM, /* PCH Mobile Full Featured */
+ LPC_P55, /* P55 */
+ LPC_PM55, /* PM55 */
+ LPC_H55, /* H55 */
+ LPC_QM57, /* QM57 */
+ LPC_H57, /* H57 */
+ LPC_HM55, /* HM55 */
+ LPC_Q57, /* Q57 */
+ LPC_HM57, /* HM57 */
+ LPC_PCHMSFF, /* PCH Mobile SFF Full Featured */
+ LPC_QS57, /* QS57 */
+ LPC_3400, /* 3400 */
+ LPC_3420, /* 3420 */
+ LPC_3450, /* 3450 */
+ LPC_EP80579, /* EP80579 */
+ LPC_CPT, /* Cougar Point */
+ LPC_CPTD, /* Cougar Point Desktop */
+ LPC_CPTM, /* Cougar Point Mobile */
+ LPC_PBG, /* Patsburg */
+ LPC_DH89XXCC, /* DH89xxCC */
+ LPC_PPT, /* Panther Point */
+ LPC_LPT, /* Lynx Point */
+};
+
+struct lpc_ich_info lpc_chipset_info[] __devinitdata = {
+ [LPC_ICH] = {"ICH", 0},
+ [LPC_ICH0] = {"ICH0", 0},
+ [LPC_ICH2] = {"ICH2", 0},
+ [LPC_ICH2M] = {"ICH2-M", 0},
+ [LPC_ICH3] = {"ICH3-S", 0},
+ [LPC_ICH3M] = {"ICH3-M", 0},
+ [LPC_ICH4] = {"ICH4", 0},
+ [LPC_ICH4M] = {"ICH4-M", 0},
+ [LPC_CICH] = {"C-ICH", 0},
+ [LPC_ICH5] = {"ICH5 or ICH5R", 0},
+ [LPC_6300ESB] = {"6300ESB", 0},
+ [LPC_ICH6] = {"ICH6 or ICH6R", 0x0601},
+ [LPC_ICH6M] = {"ICH6-M", 0x0601},
+ [LPC_ICH6W] = {"ICH6W or ICH6RW", 0x0601},
+ [LPC_631XESB] = {"631xESB/632xESB", 0x0601},
+ [LPC_ICH7] = {"ICH7 or ICH7R", 0x0701},
+ [LPC_ICH7DH] = {"ICH7DH", 0x0701},
+ [LPC_ICH7M] = {"ICH7-M or ICH7-U", 0x0701},
+ [LPC_ICH7MDH] = {"ICH7-M DH", 0x0701},
+ [LPC_NM10] = {"NM10", 0},
+ [LPC_ICH8] = {"ICH8 or ICH8R", 0x0701},
+ [LPC_ICH8DH] = {"ICH8DH", 0x0701},
+ [LPC_ICH8DO] = {"ICH8DO", 0x0701},
+ [LPC_ICH8M] = {"ICH8M", 0x0701},
+ [LPC_ICH8ME] = {"ICH8M-E", 0x0701},
+ [LPC_ICH9] = {"ICH9", 0x0801},
+ [LPC_ICH9R] = {"ICH9R", 0x0801},
+ [LPC_ICH9DH] = {"ICH9DH", 0x0801},
+ [LPC_ICH9DO] = {"ICH9DO", 0x0801},
+ [LPC_ICH9M] = {"ICH9M", 0x0801},
+ [LPC_ICH9ME] = {"ICH9M-E", 0x0801},
+ [LPC_ICH10] = {"ICH10", 0x0a11},
+ [LPC_ICH10R] = {"ICH10R", 0x0a11},
+ [LPC_ICH10D] = {"ICH10D", 0x0a01},
+ [LPC_ICH10DO] = {"ICH10DO", 0x0a01},
+ [LPC_PCH] = {"PCH Desktop Full Featured", 0x0501},
+ [LPC_PCHM] = {"PCH Mobile Full Featured", 0x0501},
+ [LPC_P55] = {"P55", 0x0501},
+ [LPC_PM55] = {"PM55", 0x0501},
+ [LPC_H55] = {"H55", 0x0501},
+ [LPC_QM57] = {"QM57", 0x0501},
+ [LPC_H57] = {"H57", 0x0501},
+ [LPC_HM55] = {"HM55", 0x0501},
+ [LPC_Q57] = {"Q57", 0x0501},
+ [LPC_HM57] = {"HM57", 0x0501},
+ [LPC_PCHMSFF] = {"PCH Mobile SFF Full Featured",0x0501},
+ [LPC_QS57] = {"QS57", 0x0501},
+ [LPC_3400] = {"3400", 0x0501},
+ [LPC_3420] = {"3420", 0x0501},
+ [LPC_3450] = {"3450", 0x0501},
+ [LPC_EP80579] = {"EP80579", 0},
+ [LPC_CPT] = {"Cougar Point", 0x0501},
+ [LPC_CPTD] = {"Cougar Point Desktop", 0x0501},
+ [LPC_CPTM] = {"Cougar Point Mobile", 0x0501},
+ [LPC_PBG] = {"Patsburg", 0},
+ [LPC_DH89XXCC] = {"DH89xxCC", 0},
+ [LPC_PPT] = {"Panther Point", 0},
+ [LPC_LPT] = {"Lynx Point", 0},
+};
+
+/*
+ * This data only exists for exporting the supported PCI ids
+ * via MODULE_DEVICE_TABLE. We do not actually register a
+ * pci_driver, because the I/O Controller Hub has also other
+ * functions that probably will be registered by other drivers.
+ */
+static DEFINE_PCI_DEVICE_TABLE(lpc_ich_ids) = {
+ { PCI_VDEVICE(INTEL, 0x2410), LPC_ICH},
+ { PCI_VDEVICE(INTEL, 0x2420), LPC_ICH0},
+ { PCI_VDEVICE(INTEL, 0x2440), LPC_ICH2},
+ { PCI_VDEVICE(INTEL, 0x244c), LPC_ICH2M},
+ { PCI_VDEVICE(INTEL, 0x2480), LPC_ICH3},
+ { PCI_VDEVICE(INTEL, 0x248c), LPC_ICH3M},
+ { PCI_VDEVICE(INTEL, 0x24c0), LPC_ICH4},
+ { PCI_VDEVICE(INTEL, 0x24cc), LPC_ICH4M},
+ { PCI_VDEVICE(INTEL, 0x2450), LPC_CICH},
+ { PCI_VDEVICE(INTEL, 0x24d0), LPC_ICH5},
+ { PCI_VDEVICE(INTEL, 0x25a1), LPC_6300ESB},
+ { PCI_VDEVICE(INTEL, 0x2640), LPC_ICH6},
+ { PCI_VDEVICE(INTEL, 0x2641), LPC_ICH6M},
+ { PCI_VDEVICE(INTEL, 0x2642), LPC_ICH6W},
+ { PCI_VDEVICE(INTEL, 0x2670), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x2671), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x2672), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x2673), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x2674), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x2675), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x2676), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x2677), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x2678), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x2679), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x267a), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x267b), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x267c), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x267d), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x267e), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x267f), LPC_631XESB},
+ { PCI_VDEVICE(INTEL, 0x27b8), LPC_ICH7},
+ { PCI_VDEVICE(INTEL, 0x27b0), LPC_ICH7DH},
+ { PCI_VDEVICE(INTEL, 0x27b9), LPC_ICH7M},
+ { PCI_VDEVICE(INTEL, 0x27bd), LPC_ICH7MDH},
+ { PCI_VDEVICE(INTEL, 0x27bc), LPC_NM10},
+ { PCI_VDEVICE(INTEL, 0x2810), LPC_ICH8},
+ { PCI_VDEVICE(INTEL, 0x2812), LPC_ICH8DH},
+ { PCI_VDEVICE(INTEL, 0x2814), LPC_ICH8DO},
+ { PCI_VDEVICE(INTEL, 0x2815), LPC_ICH8M},
+ { PCI_VDEVICE(INTEL, 0x2811), LPC_ICH8ME},
+ { PCI_VDEVICE(INTEL, 0x2918), LPC_ICH9},
+ { PCI_VDEVICE(INTEL, 0x2916), LPC_ICH9R},
+ { PCI_VDEVICE(INTEL, 0x2912), LPC_ICH9DH},
+ { PCI_VDEVICE(INTEL, 0x2914), LPC_ICH9DO},
+ { PCI_VDEVICE(INTEL, 0x2919), LPC_ICH9M},
+ { PCI_VDEVICE(INTEL, 0x2917), LPC_ICH9ME},
+ { PCI_VDEVICE(INTEL, 0x3a18), LPC_ICH10},
+ { PCI_VDEVICE(INTEL, 0x3a16), LPC_ICH10R},
+ { PCI_VDEVICE(INTEL, 0x3a1a), LPC_ICH10D},
+ { PCI_VDEVICE(INTEL, 0x3a14), LPC_ICH10DO},
+ { PCI_VDEVICE(INTEL, 0x3b00), LPC_PCH},
+ { PCI_VDEVICE(INTEL, 0x3b01), LPC_PCHM},
+ { PCI_VDEVICE(INTEL, 0x3b02), LPC_P55},
+ { PCI_VDEVICE(INTEL, 0x3b03), LPC_PM55},
+ { PCI_VDEVICE(INTEL, 0x3b06), LPC_H55},
+ { PCI_VDEVICE(INTEL, 0x3b07), LPC_QM57},
+ { PCI_VDEVICE(INTEL, 0x3b08), LPC_H57},
+ { PCI_VDEVICE(INTEL, 0x3b09), LPC_HM55},
+ { PCI_VDEVICE(INTEL, 0x3b0a), LPC_Q57},
+ { PCI_VDEVICE(INTEL, 0x3b0b), LPC_HM57},
+ { PCI_VDEVICE(INTEL, 0x3b0d), LPC_PCHMSFF},
+ { PCI_VDEVICE(INTEL, 0x3b0f), LPC_QS57},
+ { PCI_VDEVICE(INTEL, 0x3b12), LPC_3400},
+ { PCI_VDEVICE(INTEL, 0x3b14), LPC_3420},
+ { PCI_VDEVICE(INTEL, 0x3b16), LPC_3450},
+ { PCI_VDEVICE(INTEL, 0x5031), LPC_EP80579},
+ { PCI_VDEVICE(INTEL, 0x1c41), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c42), LPC_CPTD},
+ { PCI_VDEVICE(INTEL, 0x1c43), LPC_CPTM},
+ { PCI_VDEVICE(INTEL, 0x1c44), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c45), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c46), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c47), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c48), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c49), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c4a), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c4b), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c4c), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c4d), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c4e), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c4f), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c50), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c51), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c52), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c53), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c54), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c55), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c56), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c57), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c58), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c59), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c5a), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c5b), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c5c), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c5d), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c5e), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1c5f), LPC_CPT},
+ { PCI_VDEVICE(INTEL, 0x1d40), LPC_PBG},
+ { PCI_VDEVICE(INTEL, 0x1d41), LPC_PBG},
+ { PCI_VDEVICE(INTEL, 0x2310), LPC_DH89XXCC},
+ { PCI_VDEVICE(INTEL, 0x1e40), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e41), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e42), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e43), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e44), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e45), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e46), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e47), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e48), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e49), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e4a), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e4b), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e4c), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e4d), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e4e), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e4f), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e50), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e51), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e52), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e53), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e54), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e55), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e56), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e57), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e58), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e59), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e5a), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e5b), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e5c), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e5d), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e5e), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x1e5f), LPC_PPT},
+ { PCI_VDEVICE(INTEL, 0x8c40), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c41), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c42), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c43), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c44), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c45), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c46), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c47), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c48), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c49), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c4a), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c4b), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c4c), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c4d), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c4e), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c4f), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c50), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c51), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c52), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c53), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c54), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c55), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c56), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c57), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c58), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c59), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c5a), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c5b), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c5c), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c5d), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c5e), LPC_LPT},
+ { PCI_VDEVICE(INTEL, 0x8c5f), LPC_LPT},
+ { 0, }, /* End of list */
+};
+MODULE_DEVICE_TABLE(pci, lpc_ich_ids);
+
+static void lpc_ich_restore_config_space(struct pci_dev *dev)
+{
+ if (lpc_ich_acpi_save >= 0)
+ pci_write_config_byte(dev, ACPICTRL, lpc_ich_acpi_save);
+ if (lpc_ich_gpio_save >= 0)
+ pci_write_config_byte(dev, GPIOCTRL, lpc_ich_gpio_save);
+
+ lpc_ich_acpi_save = -1;
+ lpc_ich_gpio_save = -1;
+}
+
+static void lpc_ich_finalize_cell(struct mfd_cell *cell,
+ const struct pci_device_id *id)
+{
+ cell->id = id->driver_data;
+ cell->platform_data = &lpc_chipset_info[id->driver_data];
+ cell->pdata_size = sizeof(struct lpc_ich_info);
+}
+
+static int __devinit lpc_ich_probe(struct pci_dev *dev,
+ const struct pci_device_id *id)
+{
+ u32 base_addr_cfg;
+ u32 base_addr;
+ u8 reg_save;
+ int ret;
+ bool cell_added = false;
+ bool acpi_conflict = false;
+
+ /* Setup power management base register */
+ pci_read_config_dword(dev, ACPIBASE, &base_addr_cfg);
+ base_addr = base_addr_cfg & 0x0000ff80;
+ if (!base_addr) {
+ dev_err(&dev->dev, "I/O space for ACPI uninitialized\n");
+ goto pm_done;
+ }
+
+ gpio_ich_res[ICH_RES_GPE0].start = base_addr + ACPIBASE_GPE_OFF;
+ gpio_ich_res[ICH_RES_GPE0].end = base_addr + ACPIBASE_GPE_END;
+ ret = acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPE0]);
+ if (ret) {
+ /* this isn't necessarily fatal for the GPIO */
+ gpio_ich_res[ICH_RES_GPE0].start = 0;
+ gpio_ich_res[ICH_RES_GPE0].end = 0;
+ acpi_conflict = true;
+ }
+
+ /* Enable LPC ACPI space */
+ pci_read_config_byte(dev, ACPICTRL, ®_save);
+ pci_write_config_byte(dev, ACPICTRL, reg_save | 0x10);
+ lpc_ich_acpi_save = reg_save;
+
+pm_done:
+ /* Setup GPIO base register */
+ pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
+ base_addr = base_addr_cfg & 0x0000ff80;
+ if (!base_addr) {
+ dev_err(&dev->dev, "I/O space for GPIO uninitialized\n");
+ /* GPIO in power-management space may still be available */
+ goto gpio_reg;
+ }
+
+ gpio_ich_res[ICH_RES_GPIO].start = base_addr;
+ gpio_ich_res[ICH_RES_GPIO].end = base_addr + GPIOBASE_IO_SIZE - 1;
+ ret = acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPIO]);
+ if (ret) {
+ /* this isn't necessarily fatal for the GPIO */
+ gpio_ich_res[ICH_RES_GPIO].start = 0;
+ gpio_ich_res[ICH_RES_GPIO].end = 0;
+ acpi_conflict = true;
+ goto gpio_reg;
+ }
+
+ /* Enable LPC GPIO space */
+ pci_read_config_byte(dev, GPIOCTRL, ®_save);
+ pci_write_config_byte(dev, GPIOCTRL, reg_save | 0x10);
+ lpc_ich_gpio_save = reg_save;
+
+gpio_reg:
+ lpc_ich_finalize_cell(&lpc_ich_cells[LPC_GPIO], id);
+ ret = mfd_add_devices(&dev->dev, 0, &lpc_ich_cells[LPC_GPIO],
+ 1, NULL, 0);
+ if (!ret)
+ cell_added = true;
+
+ if (acpi_conflict)
+ dev_info(&dev->dev, "ACPI resource conflicts found; "
+ "consider using acpi_enforce_resources=lax?\n");
+
+ /*
+ * We only care if at least one or none of the cells registered
+ * successfully.
+ */
+ if (!cell_added) {
+ lpc_ich_restore_config_space(dev);
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static void __devexit lpc_ich_remove(struct pci_dev *dev)
+{
+ mfd_remove_devices(&dev->dev);
+ lpc_ich_restore_config_space(dev);
+}
+
+static struct pci_driver lpc_ich_driver = {
+ .name = "lpc_ich",
+ .id_table = lpc_ich_ids,
+ .probe = lpc_ich_probe,
+ .remove = __devexit_p(lpc_ich_remove),
+};
+
+static int __init lpc_ich_init(void)
+{
+ return pci_register_driver(&lpc_ich_driver);
+}
+
+static void __exit lpc_ich_exit(void)
+{
+ pci_unregister_driver(&lpc_ich_driver);
+}
+
+module_init(lpc_ich_init);
+module_exit(lpc_ich_exit);
+
+MODULE_AUTHOR("Aaron Sierra <[email protected]>");
+MODULE_DESCRIPTION("LPC interface for Intel ICH");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
new file mode 100644
index 0000000..286c778
--- /dev/null
+++ b/include/linux/mfd/lpc_ich.h
@@ -0,0 +1,32 @@
+/*
+ * linux/drivers/mfd/lpc_ich.h
+ *
+ * Copyright (c) 2012 Extreme Engineering Solution, Inc.
+ * Author: Aaron Sierra <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License 2 as published
+ * by the Free Software Foundation.
+ *
+ * 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; see the file COPYING. If not, write to
+ * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#ifndef LPC_ICH_H
+#define LPC_ICH_H
+
+/* GPIO resources */
+#define ICH_RES_GPIO 0
+#define ICH_RES_GPE0 1
+
+struct lpc_ich_info {
+ char name[32];
+ unsigned int gpio_version;
+};
+
+#endif
--
1.7.0.4
Hi Aaron,
On Fri, 17 Feb 2012 17:28:23 -0600 (CST), Aaron Sierra wrote:
> This driver currently creates resources for use by a forthcoming ICH
> chipset GPIO driver. It could be expanded to created the resources for
> converting the esb2rom (mtd) and iTCO_wdt (wdt), and potentially more,
> drivers to use the mfd model.
>
> Signed-off-by: Aaron Sierra <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
Sorry I have some more comments. You resent the patch series yesterday
faster than I could review v3.
> ---
> drivers/mfd/Kconfig | 9 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/lpc_ich.c | 525 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/lpc_ich.h | 32 +++
> 4 files changed, 567 insertions(+), 0 deletions(-)
> create mode 100644 drivers/mfd/lpc_ich.c
> create mode 100644 include/linux/mfd/lpc_ich.h
>
> (...)
> +static struct mfd_cell lpc_ich_cells[] = {
> + [LPC_GPIO] = {
> + .name = "gpio_ich",
> + .num_resources = ARRAY_SIZE(gpio_ich_res),
> + .resources = gpio_ich_res,
I think you should set ignore_resource_conflicts here too. Your code is
already checking for ACPI resource conflicts, so there is no point in
having mfd-core check again. This is not only redundant, this also
makes the kernel log harder to read as the warnings are printed
multiple times.
> (...)
> +static void lpc_ich_restore_config_space(struct pci_dev *dev)
> +{
> + if (lpc_ich_acpi_save >= 0)
> + pci_write_config_byte(dev, ACPICTRL, lpc_ich_acpi_save);
> + if (lpc_ich_gpio_save >= 0)
> + pci_write_config_byte(dev, GPIOCTRL, lpc_ich_gpio_save);
> +
> + lpc_ich_acpi_save = -1;
> + lpc_ich_gpio_save = -1;
> +}
A minor optimization is possible here, by including the "save = -1"
statements inside their respective conditional.
> +
> +static void lpc_ich_finalize_cell(struct mfd_cell *cell,
> + const struct pci_device_id *id)
Called from a __devinit function so could be made __devinit too.
> +{
> + cell->id = id->driver_data;
> + cell->platform_data = &lpc_chipset_info[id->driver_data];
> + cell->pdata_size = sizeof(struct lpc_ich_info);
> +}
> +
> +static int __devinit lpc_ich_probe(struct pci_dev *dev,
> + const struct pci_device_id *id)
> +{
> + u32 base_addr_cfg;
> + u32 base_addr;
> + u8 reg_save;
> + int ret;
> + bool cell_added = false;
> + bool acpi_conflict = false;
> +
> + /* Setup power management base register */
> + pci_read_config_dword(dev, ACPIBASE, &base_addr_cfg);
> + base_addr = base_addr_cfg & 0x0000ff80;
> + if (!base_addr) {
> + dev_err(&dev->dev, "I/O space for ACPI uninitialized\n");
> + goto pm_done;
> + }
> +
> + gpio_ich_res[ICH_RES_GPE0].start = base_addr + ACPIBASE_GPE_OFF;
> + gpio_ich_res[ICH_RES_GPE0].end = base_addr + ACPIBASE_GPE_END;
> + ret = acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPE0]);
> + if (ret) {
> + /* this isn't necessarily fatal for the GPIO */
> + gpio_ich_res[ICH_RES_GPE0].start = 0;
> + gpio_ich_res[ICH_RES_GPE0].end = 0;
Is it really sufficient to disable the resource? I see that you handle
this case properly in the gpio-ich driver, however there's also the
platform subsystem which needs to be considered. The above will cause
platform_device_add_resources (called by mfd_add_device) to register an
I/O resource at address 0, size 1. I can see it in /proc/ioports:
0000-0cf7 : PCI Bus 0000:00
0000-001f : dma1
0000-0000 : gpio_ich.32 <-- HERE
0020-0021 : pic1
This is not clean and could cause a conflict on its own. So I don't
think this is the right approach. See below for a possible solution.
> + acpi_conflict = true;
Don't you want to jump to pm_done here? There's no point in enabling
the LPC ACPI space if you are never going to access it. Not that it
should really make a difference in practice, I presume that if ACPI is
using the resource, the LPC ACPI space is already enabled...
> + }
> +
> + /* Enable LPC ACPI space */
> + pci_read_config_byte(dev, ACPICTRL, ®_save);
> + pci_write_config_byte(dev, ACPICTRL, reg_save | 0x10);
> + lpc_ich_acpi_save = reg_save;
> +
> +pm_done:
> + /* Setup GPIO base register */
> + pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> + base_addr = base_addr_cfg & 0x0000ff80;
> + if (!base_addr) {
> + dev_err(&dev->dev, "I/O space for GPIO uninitialized\n");
> + /* GPIO in power-management space may still be available */
> + goto gpio_reg;
> + }
> +
> + gpio_ich_res[ICH_RES_GPIO].start = base_addr;
> + gpio_ich_res[ICH_RES_GPIO].end = base_addr + GPIOBASE_IO_SIZE - 1;
> + ret = acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPIO]);
> + if (ret) {
> + /* this isn't necessarily fatal for the GPIO */
> + gpio_ich_res[ICH_RES_GPIO].start = 0;
> + gpio_ich_res[ICH_RES_GPIO].end = 0;
I don't quite get how this can be non-fatal, given that the gpio-ich
driver's probe function will return -ENODEV in this case. So if this
resource is mandatory, let's make it exactly that. This means that
resource 0 is mandatory and resource 1 is optional. All you have to do
then is:
* Don't register the mfd device at all if GPIO resource is unavailable.
* If ACPI resource is unavailable, set num_resources to 1.
That should work, and this solves the ghost resource problem I
mentioned earlier.
Yet a completely different approach would be to delegate the ACPI
resource conflict checking to the gpio-ich subdriver. I suspect we may
end up doing that anyway, as requesting the whole I/O range when we
only need subsets thereof is likely to cause ACPI resource conflicts on
too many systems for the driver to be useful in practice. This is a
bigger change though and I would understand if you are reluctant to do
it as this point of the review cycle. This can be changed later and I
volunteer to take care of it (I need it for my Asus Z8NA-D6 board.)
> + acpi_conflict = true;
> + goto gpio_reg;
> + }
> +
> + /* Enable LPC GPIO space */
> + pci_read_config_byte(dev, GPIOCTRL, ®_save);
> + pci_write_config_byte(dev, GPIOCTRL, reg_save | 0x10);
> + lpc_ich_gpio_save = reg_save;
> +
> +gpio_reg:
Shouldn't this label be named gpio_done for consistency? Probably a
moot point given my remark above anyway.
> + lpc_ich_finalize_cell(&lpc_ich_cells[LPC_GPIO], id);
> + ret = mfd_add_devices(&dev->dev, 0, &lpc_ich_cells[LPC_GPIO],
> + 1, NULL, 0);
> + if (!ret)
> + cell_added = true;
> +
> + if (acpi_conflict)
> + dev_info(&dev->dev, "ACPI resource conflicts found; "
> + "consider using acpi_enforce_resources=lax?\n");
I'm not sure if it really makes sense to report this. ACPI resource
conflicts are already reported quite loudly by the acpi core. And
passing acpi_enforce_resources=lax blindly isn't quite recommended, so
I'm not sure if we really want to mention it here, it might do more
harm than help.
> +
> + /*
> + * We only care if at least one or none of the cells registered
> + * successfully.
> + */
> + if (!cell_added) {
> + lpc_ich_restore_config_space(dev);
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
--
Jean Delvare
Oh, BTW...
On Fri, 17 Feb 2012 17:28:23 -0600 (CST), Aaron Sierra wrote:
> This driver currently creates resources for use by a forthcoming ICH
> chipset GPIO driver. It could be expanded to created the resources for
> converting the esb2rom (mtd) and iTCO_wdt (wdt), and potentially more,
> drivers to use the mfd model.
>
> Signed-off-by: Aaron Sierra <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> (...)
> +static void lpc_ich_finalize_cell(struct mfd_cell *cell,
> + const struct pci_device_id *id)
> +{
> + cell->id = id->driver_data;
I don't think this makes any sense. By using global variables for
per-device resources and states, your driver pretty much assumes that
at most one supported device is present on every given system (which is
indeed always the case AFAIK.) So you should set id to 0 here...
> + cell->platform_data = &lpc_chipset_info[id->driver_data];
> + cell->pdata_size = sizeof(struct lpc_ich_info);
> +}
> (...)
> + ret = mfd_add_devices(&dev->dev, 0, &lpc_ich_cells[LPC_GPIO],
> + 1, NULL, 0);
... and to -1 here, so that the platform devices created don't receive a
number at all. Many mfd drivers do exactly this.
--
Jean Delvare
> > +{
> > + cell->id = id->driver_data;
> > + cell->platform_data = &lpc_chipset_info[id->driver_data];
> > + cell->pdata_size = sizeof(struct lpc_ich_info);
> > +}
> > +
> > +static int __devinit lpc_ich_probe(struct pci_dev *dev,
> > + const struct pci_device_id *id)
> > +{
> > + u32 base_addr_cfg;
> > + u32 base_addr;
> > + u8 reg_save;
> > + int ret;
> > + bool cell_added = false;
> > + bool acpi_conflict = false;
> > +
> > + /* Setup power management base register */
> > + pci_read_config_dword(dev, ACPIBASE, &base_addr_cfg);
> > + base_addr = base_addr_cfg & 0x0000ff80;
> > + if (!base_addr) {
> > + dev_err(&dev->dev, "I/O space for ACPI uninitialized\n");
> > + goto pm_done;
> > + }
> > +
> > + gpio_ich_res[ICH_RES_GPE0].start = base_addr + ACPIBASE_GPE_OFF;
> > + gpio_ich_res[ICH_RES_GPE0].end = base_addr + ACPIBASE_GPE_END;
> > + ret = acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPE0]);
> > + if (ret) {
> > + /* this isn't necessarily fatal for the GPIO */
> > + gpio_ich_res[ICH_RES_GPE0].start = 0;
> > + gpio_ich_res[ICH_RES_GPE0].end = 0;
>
> Is it really sufficient to disable the resource? I see that you handle
> this case properly in the gpio-ich driver, however there's also the
> platform subsystem which needs to be considered. The above will cause
> platform_device_add_resources (called by mfd_add_device) to register
> an I/O resource at address 0, size 1. I can see it in /proc/ioports:
>
> 0000-0cf7 : PCI Bus 0000:00
> 0000-001f : dma1
> 0000-0000 : gpio_ich.32 <-- HERE
> 0020-0021 : pic1
>
> This is not clean and could cause a conflict on its own. So I don't
> think this is the right approach. See below for a possible solution.
I will work on a solution for this.
> > + acpi_conflict = true;
>
> Don't you want to jump to pm_done here? There's no point in enabling
> the LPC ACPI space if you are never going to access it. Not that it
> should really make a difference in practice, I presume that if ACPI
> is using the resource, the LPC ACPI space is already enabled...
I think this is another case where the code structure is based on the
complete 3-patch set. In this patch it would makes sense to have a jump
here, but after the 3rd patch is applied it would be removed anyway
because I consider the GPE region to be not strictly mandatory (which I
get into more below) and the watchdog timer regions in ACPI space need
to be tested before jumping.
> > + }
> > +
> > + /* Enable LPC ACPI space */
> > + pci_read_config_byte(dev, ACPICTRL, ®_save);
> > + pci_write_config_byte(dev, ACPICTRL, reg_save | 0x10);
> > + lpc_ich_acpi_save = reg_save;
> > +
> > +pm_done:
> > + /* Setup GPIO base register */
> > + pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> > + base_addr = base_addr_cfg & 0x0000ff80;
> > + if (!base_addr) {
> > + dev_err(&dev->dev, "I/O space for GPIO uninitialized\n");
> > + /* GPIO in power-management space may still be available */
> > + goto gpio_reg;
> > + }
> > +
> > + gpio_ich_res[ICH_RES_GPIO].start = base_addr;
> > + gpio_ich_res[ICH_RES_GPIO].end = base_addr + GPIOBASE_IO_SIZE -
> > 1;
> > + ret = acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPIO]);
> > + if (ret) {
> > + /* this isn't necessarily fatal for the GPIO */
> > + gpio_ich_res[ICH_RES_GPIO].start = 0;
> > + gpio_ich_res[ICH_RES_GPIO].end = 0;
>
> I don't quite get how this can be non-fatal, given that the gpio-ich
> driver's probe function will return -ENODEV in this case. So if this
> resource is mandatory, let's make it exactly that.
The necessity for the ICH_RES_GPIO resource to exist is an issue I
thought better left to the gpio-ich driver. The way that driver is
currently written it is mandatory, but it doesn't *have* to be written
that way. For chipsets that have GPE0 and GPIO space, only one needs
to be present to have some usable GPIO.
> This means that resource 0 is mandatory and resource 1 is optional.
> All you have to do then is:
> * Don't register the mfd device at all if GPIO resource is
> unavailable.
> * If ACPI resource is unavailable, set num_resources to 1.
>
> That should work, and this solves the ghost resource problem I
> mentioned earlier.
>
> Yet a completely different approach would be to delegate the ACPI
> resource conflict checking to the gpio-ich subdriver. I suspect we
> may end up doing that anyway, as requesting the whole I/O range when
> we only need subsets thereof is likely to cause ACPI resource
> conflicts on too many systems for the driver to be useful in practice.
> This is a bigger change though and I would understand if you are
> reluctant to do it as this point of the review cycle. This can be
> changed later and I volunteer to take care of it (I need it for my
> Asus Z8NA-D6 board.)
You mean creating resources for individual bytes within 32-bit registers?
Otherwise, the only optimization (fix) I see is that ACPIBASE_GPE0_BASE
should be 0x28, not 0x20 and gpe0_sts_ofs in gpio-ich should be removed.
Currently, that portion of gpio-ich appears to be broken.
The whole issue of anything related to boot firmware having dominion
over hardware after booting to an OS is very frustrating.
> > + acpi_conflict = true;
> > + goto gpio_reg;
> > + }
> > +
> > + /* Enable LPC GPIO space */
> > + pci_read_config_byte(dev, GPIOCTRL, ®_save);
> > + pci_write_config_byte(dev, GPIOCTRL, reg_save | 0x10);
> > + lpc_ich_gpio_save = reg_save;
> > +
> > +gpio_reg:
>
> Shouldn't this label be named gpio_done for consistency? Probably a
> moot point given my remark above anyway.
Again, the label names come from lpc_ich after all three patches have
been applied. In that case one label jumps to code immediately following
WDT cell registration, "done with registration" and the other jumps
immediately before GPIO cell registration, "do registration". If I make
ICH_RES_GPIO mandatory, like you suggest, these labels could both follow
cell registration and be named consistently.
> > + lpc_ich_finalize_cell(&lpc_ich_cells[LPC_GPIO], id);
> > + ret = mfd_add_devices(&dev->dev, 0, &lpc_ich_cells[LPC_GPIO],
> > + 1, NULL, 0);
> > + if (!ret)
> > + cell_added = true;
> > +
> > + if (acpi_conflict)
> > + dev_info(&dev->dev, "ACPI resource conflicts found; "
> > + "consider using acpi_enforce_resources=lax?\n");
>
> I'm not sure if it really makes sense to report this. ACPI resource
> conflicts are already reported quite loudly by the acpi core. And
> passing acpi_enforce_resources=lax blindly isn't quite recommended,
> so I'm not sure if we really want to mention it here, it might do more
> harm than help.
So the question mark doesn't imply strongly enough that it's not an action
that should definitely be taken. Would you prefer a warning summarizing which
drivers are affected by the detected resource conflicts or no additional warning
at all?
-Aaron S.
> On Mon, 20 Feb 2012 14:36:27 -0600 (CST), Aaron Sierra wrote:
> > > > + }
> > > > +
> > > > + /* Enable LPC ACPI space */
> > > > + pci_read_config_byte(dev, ACPICTRL, ®_save);
> > > > + pci_write_config_byte(dev, ACPICTRL, reg_save | 0x10);
> > > > + lpc_ich_acpi_save = reg_save;
> > > > +
> > > > +pm_done:
> > > > + /* Setup GPIO base register */
> > > > + pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> > > > + base_addr = base_addr_cfg & 0x0000ff80;
> > > > + if (!base_addr) {
> > > > + dev_err(&dev->dev, "I/O space for GPIO uninitialized\n");
> > > > + /* GPIO in power-management space may still be available */
> > > > + goto gpio_reg;
> > > > + }
> > > > +
> > > > + gpio_ich_res[ICH_RES_GPIO].start = base_addr;
> > > > + gpio_ich_res[ICH_RES_GPIO].end = base_addr + GPIOBASE_IO_SIZE
> > > > -
> > > > 1;
> > > > + ret =
> > > > acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPIO]);
> > > > + if (ret) {
> > > > + /* this isn't necessarily fatal for the GPIO */
> > > > + gpio_ich_res[ICH_RES_GPIO].start = 0;
> > > > + gpio_ich_res[ICH_RES_GPIO].end = 0;
> > >
> > > I don't quite get how this can be non-fatal, given that the
> > > gpio-ich driver's probe function will return -ENODEV in this
> > > case. So if this resource is mandatory, let's make it exactly
> > > that.
> >
> > The necessity for the ICH_RES_GPIO resource to exist is an issue I
> > thought better left to the gpio-ich driver. The way that driver is
> > currently written it is mandatory, but it doesn't *have* to be
> > written that way. For chipsets that have GPE0 and GPIO space, only
> > one needs to be present to have some usable GPIO.
>
> Ah, OK, I had misunderstood this, I thought GPIO was always
> mandatory.
>
> We're only talking about the ICH6 and 3100, right? I find it
> questionable that you even attempt to request and enable the GPE0
> space on all other chips then. This could cause error messages
> that are not relevant at all.
You'll see in the V5 patch that I followed your suggestion and now
treat the GPIO space as mandatory. The local version I have
prepared for V6 only looks for GPE0 space for ICH6 and i3100
devices.
> > Otherwise, the only optimization (fix) I see is that
> > ACPIBASE_GPE0_BASE should be 0x28, not 0x20 and gpe0_sts_ofs
> > in gpio-ich should be removed. Currently, that portion of
> > gpio-ich appears to be broken.
>
> If you only need the I/O port at offset 0x28, then indeed it might
> make sense to only request that one to limit the risk of resource
> conflicts.
I addressed this with V5.
> > > > + lpc_ich_finalize_cell(&lpc_ich_cells[LPC_GPIO], id);
> > > > + ret = mfd_add_devices(&dev->dev, 0, &lpc_ich_cells[LPC_GPIO],
> > > > + 1, NULL, 0);
> > > > + if (!ret)
> > > > + cell_added = true;
> > > > +
> > > > + if (acpi_conflict)
> > > > + dev_info(&dev->dev, "ACPI resource conflicts found; "
> > > > + "consider using acpi_enforce_resources=lax?\n");
> > >
> > > I'm not sure if it really makes sense to report this. ACPI
> > > resource
> > > conflicts are already reported quite loudly by the acpi core. And
> > > passing acpi_enforce_resources=lax blindly isn't quite
> > > recommended,
> > > so I'm not sure if we really want to mention it here, it might do
> > > more
> > > harm than help.
> >
> > So the question mark doesn't imply strongly enough that it's not an
> > action that should definitely be taken. Would you prefer a warning
> > summarizing which drivers are affected by the detected resource
> > conflicts or no additional warning at all?
>
> I wouldn't put any additional warning at all. But maybe that's just
> me.
In V5, I added warnings indicating which drivers are affected by
conflicts, since it was trivial with the way that I compartmentalized
gpio and watchdog mfd cell creation in that version.
-Aaron S.
On Mon, 20 Feb 2012 14:36:27 -0600 (CST), Aaron Sierra wrote:
> > > + }
> > > +
> > > + /* Enable LPC ACPI space */
> > > + pci_read_config_byte(dev, ACPICTRL, ®_save);
> > > + pci_write_config_byte(dev, ACPICTRL, reg_save | 0x10);
> > > + lpc_ich_acpi_save = reg_save;
> > > +
> > > +pm_done:
> > > + /* Setup GPIO base register */
> > > + pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> > > + base_addr = base_addr_cfg & 0x0000ff80;
> > > + if (!base_addr) {
> > > + dev_err(&dev->dev, "I/O space for GPIO uninitialized\n");
> > > + /* GPIO in power-management space may still be available */
> > > + goto gpio_reg;
> > > + }
> > > +
> > > + gpio_ich_res[ICH_RES_GPIO].start = base_addr;
> > > + gpio_ich_res[ICH_RES_GPIO].end = base_addr + GPIOBASE_IO_SIZE -
> > > 1;
> > > + ret = acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPIO]);
> > > + if (ret) {
> > > + /* this isn't necessarily fatal for the GPIO */
> > > + gpio_ich_res[ICH_RES_GPIO].start = 0;
> > > + gpio_ich_res[ICH_RES_GPIO].end = 0;
> >
> > I don't quite get how this can be non-fatal, given that the gpio-ich
> > driver's probe function will return -ENODEV in this case. So if this
> > resource is mandatory, let's make it exactly that.
>
> The necessity for the ICH_RES_GPIO resource to exist is an issue I
> thought better left to the gpio-ich driver. The way that driver is
> currently written it is mandatory, but it doesn't *have* to be written
> that way. For chipsets that have GPE0 and GPIO space, only one needs
> to be present to have some usable GPIO.
Ah, OK, I had misunderstood this, I thought GPIO was always mandatory.
We're only talking about the ICH6 and 3100, right? I find it
questionable that you even attempt to request and enable the GPE0 space
on all other chips then. This could cause error messages that are not
relevant at all.
>
> > This means that resource 0 is mandatory and resource 1 is optional.
> > All you have to do then is:
> > * Don't register the mfd device at all if GPIO resource is
> > unavailable.
> > * If ACPI resource is unavailable, set num_resources to 1.
> >
> > That should work, and this solves the ghost resource problem I
> > mentioned earlier.
> >
> > Yet a completely different approach would be to delegate the ACPI
> > resource conflict checking to the gpio-ich subdriver. I suspect we
> > may end up doing that anyway, as requesting the whole I/O range when
> > we only need subsets thereof is likely to cause ACPI resource
> > conflicts on too many systems for the driver to be useful in practice.
> > This is a bigger change though and I would understand if you are
> > reluctant to do it as this point of the review cycle. This can be
> > changed later and I volunteer to take care of it (I need it for my
> > Asus Z8NA-D6 board.)
>
> You mean creating resources for individual bytes within 32-bit registers?
No, no, not to this level of granularity. My idea is to request 3
16-byte regions separately, namely starting at offsets 0x00, 0x30 and
0x40. This has two advantages :
* If the ACPI BIOS requests optional I/O ports as is the case on my
board (they request one port at offset 0x1A, presumably to blink a
LED) we can still request everything else. Of course we shouldn't
touch the one GPIO used by the ACPI BIOS, but you should never touch
a GPIO if you don't know what you're doing anyway.
* If the ACPI BIOS requests one block of GPIOs, this still gives us a
chance to get our hands on the other blocks. I have not yet see a
BIOS doing that but I imagine it could happen.
> Otherwise, the only optimization (fix) I see is that ACPIBASE_GPE0_BASE
> should be 0x28, not 0x20 and gpe0_sts_ofs in gpio-ich should be removed.
> Currently, that portion of gpio-ich appears to be broken.
If you only need the I/O port at offset 0x28, then indeed it might make
sense to only request that one to limit the risk of resource conflicts.
> The whole issue of anything related to boot firmware having dominion
> over hardware after booting to an OS is very frustrating.
Oh yes :( I really would like to see future versions of the ACPI
specification come up with a general solution to this problem.
> > > + acpi_conflict = true;
> > > + goto gpio_reg;
> > > + }
> > > +
> > > + /* Enable LPC GPIO space */
> > > + pci_read_config_byte(dev, GPIOCTRL, ®_save);
> > > + pci_write_config_byte(dev, GPIOCTRL, reg_save | 0x10);
> > > + lpc_ich_gpio_save = reg_save;
> > > +
> > > +gpio_reg:
> >
> > Shouldn't this label be named gpio_done for consistency? Probably a
> > moot point given my remark above anyway.
>
> Again, the label names come from lpc_ich after all three patches have
> been applied. In that case one label jumps to code immediately following
> WDT cell registration, "done with registration" and the other jumps
> immediately before GPIO cell registration, "do registration". If I make
> ICH_RES_GPIO mandatory, like you suggest, these labels could both follow
> cell registration and be named consistently.
Oh, OK. I have to admit I didn't look at the watchdog patch at all, as
I'm not using this feature. So feel free to ignore every comment of
mine which would find an answer in that patch ;)
> > > + lpc_ich_finalize_cell(&lpc_ich_cells[LPC_GPIO], id);
> > > + ret = mfd_add_devices(&dev->dev, 0, &lpc_ich_cells[LPC_GPIO],
> > > + 1, NULL, 0);
> > > + if (!ret)
> > > + cell_added = true;
> > > +
> > > + if (acpi_conflict)
> > > + dev_info(&dev->dev, "ACPI resource conflicts found; "
> > > + "consider using acpi_enforce_resources=lax?\n");
> >
> > I'm not sure if it really makes sense to report this. ACPI resource
> > conflicts are already reported quite loudly by the acpi core. And
> > passing acpi_enforce_resources=lax blindly isn't quite recommended,
> > so I'm not sure if we really want to mention it here, it might do more
> > harm than help.
>
> So the question mark doesn't imply strongly enough that it's not an action
> that should definitely be taken. Would you prefer a warning summarizing which
> drivers are affected by the detected resource conflicts or no additional warning
> at all?
I wouldn't put any additional warning at all. But maybe that's just me.
--
Jean Delvare