[PATCH v3] lpc_ich: fix gpio base and control offsets

[PATCH v3] lpc_ich: fix gpio base and control offsets

[Aaron Sierra]

In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found at
offsets 0x58 and 0x5C, respectively. This patch allows GPIO access to
properly be enabled (and disabled) for these chipsets.

Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/mfd/lpc_ich.c |  109 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 76 insertions(+), 33 deletions(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index d9d9303..a0cfdf9 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -75,8 +75,10 @@
 #define ACPIBASE_GCS_OFF	0x3410
 #define ACPIBASE_GCS_END	0x3414
 
-#define GPIOBASE		0x48
-#define GPIOCTRL		0x4C
+#define GPIOBASE_ICH0		0x58
+#define GPIOCTRL_ICH0		0x5C
+#define GPIOBASE_ICH6		0x48
+#define GPIOCTRL_ICH6		0x4C
 
 #define RCBABASE		0xf0
 
@@ -84,8 +86,17 @@
 #define wdt_mem_res(i) wdt_res(ICH_RES_MEM_OFF, i)
 #define wdt_res(b, i) (&wdt_ich_res[(b) + (i)])
 
-static int lpc_ich_acpi_save = -1;
-static int lpc_ich_gpio_save = -1;
+struct lpc_ich_cfg {
+	int base;
+	int ctrl;
+	int save;
+};
+
+struct lpc_ich_priv {
+	int chipset;
+	struct lpc_ich_cfg acpi;
+	struct lpc_ich_cfg gpio;
+};
 
 static struct resource wdt_ich_res[] = {
 	/* ACPI - TCO */
@@ -661,39 +672,44 @@ 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);
-		lpc_ich_acpi_save = -1;
+	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+
+	if (priv->acpi.save >= 0) {
+		pci_write_config_byte(dev, priv->acpi.ctrl, priv->acpi.save);
+		priv->acpi.save = -1;
 	}
 
-	if (lpc_ich_gpio_save >= 0) {
-		pci_write_config_byte(dev, GPIOCTRL, lpc_ich_gpio_save);
-		lpc_ich_gpio_save = -1;
+	if (priv->gpio.save >= 0) {
+		pci_write_config_byte(dev, priv->gpio.ctrl, priv->gpio.save);
+		priv->gpio.save = -1;
 	}
 }
 
 static void lpc_ich_enable_acpi_space(struct pci_dev *dev)
 {
+	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
 	u8 reg_save;
 
-	pci_read_config_byte(dev, ACPICTRL, &reg_save);
-	pci_write_config_byte(dev, ACPICTRL, reg_save | 0x10);
-	lpc_ich_acpi_save = reg_save;
+	pci_read_config_byte(dev, priv->acpi.ctrl, &reg_save);
+	pci_write_config_byte(dev, priv->acpi.ctrl, reg_save | 0x10);
+	priv->acpi.save = reg_save;
 }
 
 static void lpc_ich_enable_gpio_space(struct pci_dev *dev)
 {
+	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
 	u8 reg_save;
 
-	pci_read_config_byte(dev, GPIOCTRL, &reg_save);
-	pci_write_config_byte(dev, GPIOCTRL, reg_save | 0x10);
-	lpc_ich_gpio_save = reg_save;
+	pci_read_config_byte(dev, priv->gpio.ctrl, &reg_save);
+	pci_write_config_byte(dev, priv->gpio.ctrl, reg_save | 0x10);
+	priv->gpio.save = reg_save;
 }
 
-static void lpc_ich_finalize_cell(struct mfd_cell *cell,
-					const struct pci_device_id *id)
+static void lpc_ich_finalize_cell(struct pci_dev *dev, struct mfd_cell *cell)
 {
-	cell->platform_data = &lpc_chipset_info[id->driver_data];
+	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+
+	cell->platform_data = &lpc_chipset_info[priv->chipset];
 	cell->pdata_size = sizeof(struct lpc_ich_info);
 }
 
@@ -721,9 +737,9 @@ static int lpc_ich_check_conflict_gpio(struct resource *res)
 	return use_gpio ? use_gpio : ret;
 }
 
-static int lpc_ich_init_gpio(struct pci_dev *dev,
-				const struct pci_device_id *id)
+static int lpc_ich_init_gpio(struct pci_dev *dev)
 {
+	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
 	u32 base_addr_cfg;
 	u32 base_addr;
 	int ret;
@@ -731,7 +747,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev,
 	struct resource *res;
 
 	/* Setup power management base register */
-	pci_read_config_dword(dev, ACPIBASE, &base_addr_cfg);
+	pci_read_config_dword(dev, priv->acpi.base, &base_addr_cfg);
 	base_addr = base_addr_cfg & 0x0000ff80;
 	if (!base_addr) {
 		dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
@@ -757,7 +773,7 @@ static int lpc_ich_init_gpio(struct pci_dev *dev,
 
 gpe0_done:
 	/* Setup GPIO base register */
-	pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
+	pci_read_config_dword(dev, priv->gpio.base, &base_addr_cfg);
 	base_addr = base_addr_cfg & 0x0000ff80;
 	if (!base_addr) {
 		dev_notice(&dev->dev, "I/O space for GPIO uninitialized\n");
@@ -768,7 +784,7 @@ gpe0_done:
 	/* Older devices provide fewer GPIO and have a smaller resource size. */
 	res = &gpio_ich_res[ICH_RES_GPIO];
 	res->start = base_addr;
-	switch (lpc_chipset_info[id->driver_data].gpio_version) {
+	switch (lpc_chipset_info[priv->chipset].gpio_version) {
 	case ICH_V5_GPIO:
 	case ICH_V10CORP_GPIO:
 		res->end = res->start + 128 - 1;
@@ -784,10 +800,10 @@ gpe0_done:
 		acpi_conflict = true;
 		goto gpio_done;
 	}
-	lpc_chipset_info[id->driver_data].use_gpio = ret;
+	lpc_chipset_info[priv->chipset].use_gpio = ret;
 	lpc_ich_enable_gpio_space(dev);
 
-	lpc_ich_finalize_cell(&lpc_ich_cells[LPC_GPIO], id);
+	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_GPIO]);
 	ret = mfd_add_devices(&dev->dev, -1, &lpc_ich_cells[LPC_GPIO],
 			      1, NULL, 0, NULL);
 
@@ -798,16 +814,16 @@ gpio_done:
 	return ret;
 }
 
-static int lpc_ich_init_wdt(struct pci_dev *dev,
-				const struct pci_device_id *id)
+static int lpc_ich_init_wdt(struct pci_dev *dev)
 {
+	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
 	u32 base_addr_cfg;
 	u32 base_addr;
 	int ret;
 	struct resource *res;
 
 	/* Setup power management base register */
-	pci_read_config_dword(dev, ACPIBASE, &base_addr_cfg);
+	pci_read_config_dword(dev, priv->acpi.base, &base_addr_cfg);
 	base_addr = base_addr_cfg & 0x0000ff80;
 	if (!base_addr) {
 		dev_notice(&dev->dev, "I/O space for ACPI uninitialized\n");
@@ -830,7 +846,7 @@ static int 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 == 1) {
+	if (lpc_chipset_info[priv->chipset].iTCO_version == 1) {
 		/* Don't register iomem for TCO ver 1 */
 		lpc_ich_cells[LPC_WDT].num_resources--;
 	} else {
@@ -847,7 +863,7 @@ static int lpc_ich_init_wdt(struct pci_dev *dev,
 		res->end = base_addr + ACPIBASE_GCS_END;
 	}
 
-	lpc_ich_finalize_cell(&lpc_ich_cells[LPC_WDT], id);
+	lpc_ich_finalize_cell(dev, &lpc_ich_cells[LPC_WDT]);
 	ret = mfd_add_devices(&dev->dev, -1, &lpc_ich_cells[LPC_WDT],
 			      1, NULL, 0, NULL);
 
@@ -858,14 +874,35 @@ wdt_done:
 static int lpc_ich_probe(struct pci_dev *dev,
 				const struct pci_device_id *id)
 {
+	struct lpc_ich_priv *priv;
 	int ret;
 	bool cell_added = false;
 
-	ret = lpc_ich_init_wdt(dev, id);
+	priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
+	if (!priv)
+		return -ENOMEM;
+
+	priv->chipset = id->driver_data;
+	priv->acpi.save = -1;
+	priv->acpi.base = ACPIBASE;
+	priv->acpi.ctrl = ACPICTRL;
+
+	priv->gpio.save = -1;
+	if (priv->chipset <= LPC_ICH5) {
+		priv->gpio.base = GPIOBASE_ICH0;
+		priv->gpio.ctrl = GPIOCTRL_ICH0;
+	} else {
+		priv->gpio.base = GPIOBASE_ICH6;
+		priv->gpio.ctrl = GPIOCTRL_ICH6;
+	}
+
+	pci_set_drvdata(dev, priv);
+
+	ret = lpc_ich_init_wdt(dev);
 	if (!ret)
 		cell_added = true;
 
-	ret = lpc_ich_init_gpio(dev, id);
+	ret = lpc_ich_init_gpio(dev);
 	if (!ret)
 		cell_added = true;
 
@@ -876,6 +913,8 @@ static int lpc_ich_probe(struct pci_dev *dev,
 	if (!cell_added) {
 		dev_warn(&dev->dev, "No MFD cells added\n");
 		lpc_ich_restore_config_space(dev);
+		pci_set_drvdata(dev, NULL);
+		kfree(priv);
 		return -ENODEV;
 	}
 
@@ -884,8 +923,12 @@ static int lpc_ich_probe(struct pci_dev *dev,
 
 static void lpc_ich_remove(struct pci_dev *dev)
 {
+	void *priv = pci_get_drvdata(dev);
+
 	mfd_remove_devices(&dev->dev);
 	lpc_ich_restore_config_space(dev);
+	pci_set_drvdata(dev, NULL);
+	kfree(priv);
 }
 
 static struct pci_driver lpc_ich_driver = {
-- 
1.7.9.5

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Linus Walleij]

On Thu, Jan 24, 2013 at 9:52 PM, Aaron Sierra <asierra@xes-inc.com> wrote:

> In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found at
> offsets 0x58 and 0x5C, respectively. This patch allows GPIO access to
> properly be enabled (and disabled) for these chipsets.
>
> Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>

OK... Paul, can you test this on your setup?

> @@ -858,14 +874,35 @@ wdt_done:
>  static int lpc_ich_probe(struct pci_dev *dev,
>                                 const struct pci_device_id *id)
>  {
> +       struct lpc_ich_priv *priv;
>         int ret;
>         bool cell_added = false;
>
> -       ret = lpc_ich_init_wdt(dev, id);
> +       priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->chipset = id->driver_data;

So where is this id->driver_data which is just assigned to
priv->chipset coming from again? ACPI something?

Just trying to follow things...

Yours,
Linus Walleij

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Paul Bolle]

On Fri, 2013-01-25 at 10:47 +0100, Linus Walleij wrote:
> On Thu, Jan 24, 2013 at 9:52 PM, Aaron Sierra <asierra@xes-inc.com> wrote:
> > In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found at
> > offsets 0x58 and 0x5C, respectively. This patch allows GPIO access to
> > properly be enabled (and disabled) for these chipsets.
> >
> > Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> 
> OK... Paul, can you test this on your setup?

This seems to be the first time I'm added to this thread. I'm a bit lost
here: why should I test this patch? This doesn't seem to be related to
two older lpc_ich threads I was involved in (in November 2012). Not that
I mind testing stuff, that's not the problem...


Paul Bolle

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Linus Walleij]

On Fri, Jan 25, 2013 at 11:04 AM, Paul Bolle <pebolle@tiscali.nl> wrote:
> On Fri, 2013-01-25 at 10:47 +0100, Linus Walleij wrote:
>> On Thu, Jan 24, 2013 at 9:52 PM, Aaron Sierra <asierra@xes-inc.com> wrote:
>> > In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found at
>> > offsets 0x58 and 0x5C, respectively. This patch allows GPIO access to
>> > properly be enabled (and disabled) for these chipsets.
>> >
>> > Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
>> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
>>
>> OK... Paul, can you test this on your setup?
>
> This seems to be the first time I'm added to this thread. I'm a bit lost
> here: why should I test this patch? This doesn't seem to be related to
> two older lpc_ich threads I was involved in (in November 2012). Not that
> I mind testing stuff, that's not the problem...

Basically I do git log and when I see this I think:

commit 0c418844dce21fa7000b51190f393c7d6a7ee12d
Author: Paul Bolle <pebolle@tiscali.nl>
Date:   Mon Nov 19 21:04:11 2012 +0100

    mfd: lpc_ich: One uninitialized cell is no error

"Hm, there is another guy who's actually using this hardware."

"If this patch breaks it he will probably be upset."

Yours,
Linus Walleij

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Paul Bolle]

On Fri, 2013-01-25 at 12:06 +0100, Linus Walleij wrote:
> Basically I do git log and when I see this I think:
> 
> commit 0c418844dce21fa7000b51190f393c7d6a7ee12d
> Author: Paul Bolle <pebolle@tiscali.nl>
> Date:   Mon Nov 19 21:04:11 2012 +0100
> 
>     mfd: lpc_ich: One uninitialized cell is no error
> 
> "Hm, there is another guy who's actually using this hardware."

I see, thanks. (Note to self: I should work on my psychic abilities. If
all those quacks can use them it can't be too hard.)

> "If this patch breaks it he will probably be upset."

I backported this patch to 3.7.5 (which I'm still running now). That
basically meant applying e294bc91760e11d2f1ebbac1d0a979069edf7adb ("mfd:
lpc_ich: Fix resource request for [mem 0x00000000]") first and then
manually adding some harmless context changes (ie, __devinit and
__devexit stuff).

Would testing on 3.7.5 be helpful?


Paul Bolle

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Linus Walleij]

On Fri, Jan 25, 2013 at 1:00 PM, Paul Bolle <pebolle@tiscali.nl> wrote:

> I backported this patch to 3.7.5 (which I'm still running now). That
> basically meant applying e294bc91760e11d2f1ebbac1d0a979069edf7adb ("mfd:
> lpc_ich: Fix resource request for [mem 0x00000000]") first and then
> manually adding some harmless context changes (ie, __devinit and
> __devexit stuff).
>
> Would testing on 3.7.5 be helpful?

Sure it's as good an indicator of the quality of the patch as any.

If it works on that kernel, it makes probablility lower that it breaks
your machine later on atleast.

Thanks!
Linus Walleij

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Paul Bolle]

On Fri, 2013-01-25 at 13:43 +0100, Linus Walleij wrote:
> On Fri, Jan 25, 2013 at 1:00 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> > Would testing on 3.7.5 be helpful?
> 
> Sure it's as good an indicator of the quality of the patch as any.
> 
> If it works on that kernel, it makes probablility lower that it breaks
> your machine later on atleast.

0) insmod-ing an updated lpc_ich.ko generated quite a bit of noise in
dmesg:
    <6>[19913.247530] iTCO_wdt: Found a ICH8M-E TCO device (Version=2, TCOBASE=0x1060)
    <6>[19913.249310] iTCO_wdt: initialized. heartbeat=30 sec (nowayout=0)
    <4>[19913.249332] ACPI Warning: 0x0000000000001028-0x000000000000102f SystemIO conflicts with Region \_SB_.PCI0.LPC_.PMIO 1 (20120913/utaddress-251)
    <6>[19913.249338] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
    <4>[19913.249342] ACPI Warning: 0x00000000000011b0-0x00000000000011bf SystemIO conflicts with Region \_SB_.PCI0.LPC_.LPIO 1 (20120913/utaddress-251)
    <6>[19913.249347] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
    <4>[19913.249349] ACPI Warning: 0x0000000000001180-0x00000000000011af SystemIO conflicts with Region \_SB_.PCI0.LPC_.LPIO 1 (20120913/utaddress-251)
    <6>[19913.249353] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
    <4>[19913.249355] lpc_ich: Resource conflict(s) found affecting gpio_ich

I have no idea whatsoever what that could mean.

1) Should I try booting with the updated module too?


Paul Bolle

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Paul Bolle]

On Fri, 2013-01-25 at 14:25 +0100, Paul Bolle wrote:
> 0) insmod-ing an updated lpc_ich.ko generated quite a bit of noise in
> dmesg:
>     <6>[19913.247530] iTCO_wdt: Found a ICH8M-E TCO device (Version=2, TCOBASE=0x1060)
>     <6>[19913.249310] iTCO_wdt: initialized. heartbeat=30 sec (nowayout=0)
>     <4>[19913.249332] ACPI Warning: 0x0000000000001028-0x000000000000102f SystemIO conflicts with Region \_SB_.PCI0.LPC_.PMIO 1 (20120913/utaddress-251)
>     <6>[19913.249338] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
>     <4>[19913.249342] ACPI Warning: 0x00000000000011b0-0x00000000000011bf SystemIO conflicts with Region \_SB_.PCI0.LPC_.LPIO 1 (20120913/utaddress-251)
>     <6>[19913.249347] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
>     <4>[19913.249349] ACPI Warning: 0x0000000000001180-0x00000000000011af SystemIO conflicts with Region \_SB_.PCI0.LPC_.LPIO 1 (20120913/utaddress-251)
>     <6>[19913.249353] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
>     <4>[19913.249355] lpc_ich: Resource conflict(s) found affecting gpio_ich
> 
> I have no idea whatsoever what that could mean.

But please note that these messages are nothing new. I found identical
messages in all my logs (from v3.7.y kernels; I do not seem to have logs
of older releases anymore).


Paul Bolle

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Aaron Sierra]

> On Fri, 2013-01-25 at 14:25 +0100, Paul Bolle wrote:
> > 0) insmod-ing an updated lpc_ich.ko generated quite a bit of noise
> > in
> > dmesg:
> >     <6>[19913.247530] iTCO_wdt: Found a ICH8M-E TCO device
> >     (Version=2, TCOBASE=0x1060)
> >     <6>[19913.249310] iTCO_wdt: initialized. heartbeat=30 sec
> >     (nowayout=0)
> >     <4>[19913.249332] ACPI Warning:
> >     0x0000000000001028-0x000000000000102f SystemIO conflicts with
> >     Region \_SB_.PCI0.LPC_.PMIO 1 (20120913/utaddress-251)
> >     <6>[19913.249338] ACPI: If an ACPI driver is available for this
> >     device, you should use it instead of the native driver
> >     <4>[19913.249342] ACPI Warning:
> >     0x00000000000011b0-0x00000000000011bf SystemIO conflicts with
> >     Region \_SB_.PCI0.LPC_.LPIO 1 (20120913/utaddress-251)
> >     <6>[19913.249347] ACPI: If an ACPI driver is available for this
> >     device, you should use it instead of the native driver
> >     <4>[19913.249349] ACPI Warning:
> >     0x0000000000001180-0x00000000000011af SystemIO conflicts with
> >     Region \_SB_.PCI0.LPC_.LPIO 1 (20120913/utaddress-251)
> >     <6>[19913.249353] ACPI: If an ACPI driver is available for this
> >     device, you should use it instead of the native driver
> >     <4>[19913.249355] lpc_ich: Resource conflict(s) found affecting
> >     gpio_ich
> > 
> > I have no idea whatsoever what that could mean.
> 
> But please note that these messages are nothing new. I found
> identical
> messages in all my logs (from v3.7.y kernels; I do not seem to have
> logs
> of older releases anymore).
> 
> 
> Paul Bolle

Paul,
My understanding of these warnings is that your BIOS defines regions
in its ACPI tables that coincide with regions we want to pass to
gpio-ich. Because the regions are defined in the ACPI tables Linux
assumes that they are actively being used by the BIOS and that they
shouldn't be used, unless you tell the kernel to globally ignore
ACPI resource conflicts.

Similar warnings used to also appear for the TCO watchdog regions until
that was determined to be a regression and the ACPI resource conflict
checks were removed by this patch:

commit 092369efbd6ef6b4a215741ce9f65446bf45beff
Author: Feng Tang <feng.tang@intel.com>
Date:   Thu Aug 16 15:50:10 2012 +0800

    mfd: lpc_ich: Fix a 3.5 kernel regression for iTCO_wdt driver

    [trimmed]

    Actually the same check could be removed for the gpio-ich in lpc_ich.c,
    but I'm not sure if it will cause problems.

Like Feng, I also am not sure if removing the checks for GPIO would cause
problems, so the noise remains.

-Aaron S.

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Paul Bolle]

On Fri, 2013-01-25 at 14:25 +0100, Paul Bolle wrote:
> 1) Should I try booting with the updated module too?

Booting turned out to be a non-issue. I use lpc_ich as a module (Fedora
17 default) and lpc_ich isn't included by default in Fedora 17's
initramfs. So booting v3.7.5 with an updated module should hardly differ
from insmod'ing manually. And indeed, booting that way didn't lead to
any obvious issues.


Paul Bolle

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Peter Hurley]

On Fri, 2013-01-25 at 10:47 +0100, Linus Walleij wrote:
> On Thu, Jan 24, 2013 at 9:52 PM, Aaron Sierra <asierra@xes-inc.com> wrote:
> 
> > In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found at
> > offsets 0x58 and 0x5C, respectively. This patch allows GPIO access to
> > properly be enabled (and disabled) for these chipsets.
> >
> > Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> 
> OK... Paul, can you test this on your setup?
> 
> > @@ -858,14 +874,35 @@ wdt_done:
> >  static int lpc_ich_probe(struct pci_dev *dev,
> >                                 const struct pci_device_id *id)
> >  {
> > +       struct lpc_ich_priv *priv;
> >         int ret;
> >         bool cell_added = false;
> >
> > -       ret = lpc_ich_init_wdt(dev, id);
> > +       priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       priv->chipset = id->driver_data;
> 
> So where is this id->driver_data which is just assigned to
> priv->chipset coming from again? ACPI something?

That's how pci device probing works.

The driver defines a struct pci_device_id[] table with
DEFINE_PCI_DEVICE_TABLE(), initializing the .driver_data fields with an
index into a static array of device types (in this case, struct
lpc_ich_info lpc_chipset_info[]), and the pci subsystem passes the
actual matching pci_device_id* to the driver's probe() function.

There's more information in Documentation/pci.txt

Regards,
Peter Hurley

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Peter Hurley]

On Fri, 2013-01-25 at 08:44 -0500, Peter Hurley wrote:
> On Fri, 2013-01-25 at 10:47 +0100, Linus Walleij wrote:
> > On Thu, Jan 24, 2013 at 9:52 PM, Aaron Sierra <asierra@xes-inc.com> wrote:
> > 
> > > In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found at
> > > offsets 0x58 and 0x5C, respectively. This patch allows GPIO access to
> > > properly be enabled (and disabled) for these chipsets.
> > >
> > > Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
> > > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > 
> > OK... Paul, can you test this on your setup?
> > 
> > > @@ -858,14 +874,35 @@ wdt_done:
> > >  static int lpc_ich_probe(struct pci_dev *dev,
> > >                                 const struct pci_device_id *id)
> > >  {
> > > +       struct lpc_ich_priv *priv;
> > >         int ret;
> > >         bool cell_added = false;
> > >
> > > -       ret = lpc_ich_init_wdt(dev, id);
> > > +       priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
> > > +       if (!priv)
> > > +               return -ENOMEM;
> > > +
> > > +       priv->chipset = id->driver_data;
> > 
> > So where is this id->driver_data which is just assigned to
> > priv->chipset coming from again? ACPI something?
> 
> That's how pci device probing works.
> 
> The driver defines a struct pci_device_id[] table with
> DEFINE_PCI_DEVICE_TABLE(), initializing the .driver_data fields with an
> index into a static array of device types (in this case, struct
> lpc_ich_info lpc_chipset_info[]), and the pci subsystem passes the
> actual matching pci_device_id* to the driver's probe() function.
> 
> There's more information in Documentation/pci.txt

Correction. Documentation/PCI/pci.txt

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Samuel Ortiz]

Hi Linus,

On Fri, Jan 25, 2013 at 10:47:45AM +0100, Linus Walleij wrote:
> On Thu, Jan 24, 2013 at 9:52 PM, Aaron Sierra <asierra@xes-inc.com> wrote:
> 
> > In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found at
> > offsets 0x58 and 0x5C, respectively. This patch allows GPIO access to
> > properly be enabled (and disabled) for these chipsets.
> >
> > Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> 
> OK... Paul, can you test this on your setup?
> 
> > @@ -858,14 +874,35 @@ wdt_done:
> >  static int lpc_ich_probe(struct pci_dev *dev,
> >                                 const struct pci_device_id *id)
> >  {
> > +       struct lpc_ich_priv *priv;
> >         int ret;
> >         bool cell_added = false;
> >
> > -       ret = lpc_ich_init_wdt(dev, id);
> > +       priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       priv->chipset = id->driver_data;
> 
> So where is this id->driver_data which is just assigned to
> priv->chipset coming from again? ACPI something?
It comes from the static PCI table the driver defines itself. It allows you to
pass meta data associated to a specific PCI ID.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Linus Walleij]

On Fri, Jan 25, 2013 at 5:48 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> On Fri, Jan 25, 2013 at 10:47:45AM +0100, Linus Walleij wrote:

>> So where is this id->driver_data which is just assigned to
>> priv->chipset coming from again? ACPI something?
>
> It comes from the static PCI table the driver defines itself. It allows you to
> pass meta data associated to a specific PCI ID.

OK cool, I think we can conclude that this patch is fine after the
boot tests and all:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Paul Bolle]

On Mon, 2013-01-28 at 11:11 +0100, Linus Walleij wrote:
> OK cool, I think we can conclude that this patch is fine after the
> boot tests and all:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
 
In case this ACK requires my feedback: I just reported that booting
v3.7.5 with an updated lpc_ich module didn't lead to any obvious issues.


Paul Bolle

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Samuel Ortiz]

Hi Arron,

On Thu, Jan 24, 2013 at 02:52:39PM -0600, Aaron Sierra wrote:
> In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found at
> offsets 0x58 and 0x5C, respectively. This patch allows GPIO access to
> properly be enabled (and disabled) for these chipsets.
> 
> Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/mfd/lpc_ich.c |  109 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 76 insertions(+), 33 deletions(-)
I applied this one to my for-next branch, but:


> @@ -858,14 +874,35 @@ wdt_done:
>  static int lpc_ich_probe(struct pci_dev *dev,
>  				const struct pci_device_id *id)
>  {
> +	struct lpc_ich_priv *priv;
>  	int ret;
>  	bool cell_added = false;
> 
> -	ret = lpc_ich_init_wdt(dev, id);
> +	priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
> +	if (!priv)
> +		return -ENOMEM;
Could you please come back to me with a follow up patch that would convert
this to the devm_kzalloc() API ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Aaron Sierra]

> On Thu, Jan 24, 2013 at 02:52:39PM -0600, Aaron Sierra wrote:
> > In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found
> > at
> > offsets 0x58 and 0x5C, respectively. This patch allows GPIO access
> > to
> > properly be enabled (and disabled) for these chipsets.
> > 
> > Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > ---
> >  drivers/mfd/lpc_ich.c |  109
> >  ++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 76 insertions(+), 33 deletions(-)
> I applied this one to my for-next branch, but:
> 
> 
> > @@ -858,14 +874,35 @@ wdt_done:
> >  static int lpc_ich_probe(struct pci_dev *dev,
> >  				const struct pci_device_id *id)
> >  {
> > +	struct lpc_ich_priv *priv;
> >  	int ret;
> >  	bool cell_added = false;
> > 
> > -	ret = lpc_ich_init_wdt(dev, id);
> > +	priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
> > +	if (!priv)
> > +		return -ENOMEM;
> Could you please come back to me with a follow up patch that would
> convert this to the devm_kzalloc() API ?

Yeah, no problem.

-Aaron S.

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Anatol Pomozov]

Hi, Aaron

On Sun, Feb 3, 2013 at 9:02 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> Hi Arron,
>
> On Thu, Jan 24, 2013 at 02:52:39PM -0600, Aaron Sierra wrote:
>> In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found at
>> offsets 0x58 and 0x5C, respectively. This patch allows GPIO access to
>> properly be enabled (and disabled) for these chipsets.
>>
>> Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
>> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
>> ---
>>  drivers/mfd/lpc_ich.c |  109 ++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 76 insertions(+), 33 deletions(-)
> I applied this one to my for-next branch, but:
>
>
>> @@ -858,14 +874,35 @@ wdt_done:
>>  static int lpc_ich_probe(struct pci_dev *dev,
>>                               const struct pci_device_id *id)
>>  {
>> +     struct lpc_ich_priv *priv;
>>       int ret;
>>       bool cell_added = false;
>>
>> -     ret = lpc_ich_init_wdt(dev, id);
>> +     priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
>> +     if (!priv)
>> +             return -ENOMEM;
> Could you please come back to me with a follow up patch that would convert
> this to the devm_kzalloc() API ?

I compiled linux-next (sha == 019f4bc86a462), tried to boot it on my
machine and it fails at the very early stage with following oops:


    0.000000] NUMA: nodes only cover 0MB of your 98292MB e820 RAM. Not used.
[    1.008406] kvm: already loaded the other module
[    1.206776] ------------[ cut here ]------------
[    1.211395] kernel BUG at mm/slab.c:2772!
[    1.215400] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
[    1.220846] Modules linked in:
[    1.223917] CPU 15
[    1.225842] Pid: 1, comm: swapper/0 Not tainted 3.8.0-next-20130214-dbg-DEV
[    1.235763] RIP: 0010:[<ffffffff8118d740>]  [<ffffffff8118d740>]
cache_grow+0x460/0x470
[    1.243784] RSP: 0000:ffff8817ed6f7b68  EFLAGS: 00010002
[    1.249093] RAX: ffff8817ed6f7fd8 RBX: ffff881811c22fc0 RCX: 0000000000000000
[    1.256224] RDX: 0000000000000000 RSI: 000000000004121c RDI: ffff881811c22fc0
[    1.263351] RBP: ffff8817ed6f7bb8 R08: 0000000000000001 R09: 0000000000000000
[    1.270483] R10: 0000000000000003 R11: 0000000000000000 R12: ffff8817ec92ce00
[    1.277610] R13: 0000000000000000 R14: 0000000000000000 R15: 000000000000000c
[    1.284738] FS:  0000000000000000(0000) GS:ffff8818733e0000(0000)
knlGS:0000000000000000
[    1.292821] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[    1.298563] CR2: 0000000000000000 CR3: 0000000001a0b000 CR4: 00000000000007e0
[    1.305691] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    1.312820] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[    1.319946] Process swapper/0 (pid: 1, threadinfo ffff8817ed6f6000,
task ffff8817ed6f4040)
[    1.328203] Stack:
[    1.330215]  ffff8817ec92ce00 ffff881811c27100 ffff8817ec92ce00
0000000000000000
[    1.337673]  ffff881811c27158 ffff881811c22fc0 ffff8817ec92ce00
0000000000000000
[    1.345132]  ffff881811c27158 000000000000000c ffff8817ed6f7c48
ffffffff8118db02
[    1.352589] Call Trace:
[    1.355035]  [<ffffffff8118db02>] cache_alloc_refill+0x3b2/0x430
[    1.361036]  [<ffffffff811860b5>] ? interleave_nodes+0x85/0xa0
[    1.366864]  [<ffffffff8118e737>] kmem_cache_alloc_trace+0x167/0x2e0
[    1.373214]  [<ffffffff810daa2d>] ? trace_hardirqs_on+0xd/0x10
[    1.379050]  [<ffffffff813eb9a2>] ? lpc_ich_probe+0x32/0x510
[    1.384714]  [<ffffffff813eb9a2>] lpc_ich_probe+0x32/0x510
[    1.390196]  [<ffffffff815d86fb>] ? _raw_spin_unlock+0x2b/0x50
[    1.396025]  [<ffffffff8133493c>] ? pci_match_device+0xdc/0xf0
[    1.401853]  [<ffffffff81334abf>] pci_device_probe+0x12f/0x140
[    1.407683]  [<ffffffff813e2249>] driver_probe_device+0x89/0x390
[    1.413689]  [<ffffffff813e25fb>] __driver_attach+0xab/0xb0
[    1.419260]  [<ffffffff813e2550>] ? driver_probe_device+0x390/0x390
[    1.425529]  [<ffffffff813e2550>] ? driver_probe_device+0x390/0x390
[    1.431789]  [<ffffffff813e03e4>] bus_for_each_dev+0x64/0xa0
[    1.437445]  [<ffffffff813e1cce>] driver_attach+0x1e/0x20
[    1.442840]  [<ffffffff813e17a1>] bus_add_driver+0x101/0x290
[    1.448501]  [<ffffffff81b1de59>] ? lkdtm_module_init+0x1ce/0x1ce
[    1.454588]  [<ffffffff813e291a>] driver_register+0x7a/0x160
[    1.460245]  [<ffffffff81b1de59>] ? lkdtm_module_init+0x1ce/0x1ce
[    1.466332]  [<ffffffff81334574>] __pci_register_driver+0x64/0x70
[    1.472421]  [<ffffffff81b1de72>] lpc_ich_init+0x19/0x1b
[    1.477738]  [<ffffffff8100020f>] do_one_initcall+0x3f/0x170
[    1.483400]  [<ffffffff81aeef7d>] kernel_init_freeable+0x137/0x1c6
[    1.489574]  [<ffffffff81aee837>] ? do_early_param+0x87/0x87
[    1.495229]  [<ffffffff815ca890>] ? rest_init+0xe0/0xe0
[    1.500451]  [<ffffffff815ca89e>] kernel_init+0xe/0xf0
[    1.505587]  [<ffffffff815e0f5c>] ret_from_fork+0x7c/0xb0
[    1.510987]  [<ffffffff815ca890>] ? rest_init+0xe0/0xe0
[    1.516207] Code: 74 9c fa 89 45 b0 e8 a0 81 f4 ff 8b 45 b0 eb 8e
8b 75 c8 44 89 ea 48 89 df e8 3d d8 ff ff 48 85 c0 49 89 c6 74 d1 e9
39 fc ff ff <0f> 0b b8 01 00 00 00 e9 b7 fc ff ff 0f 1f 40 00 66 66 66
66 90
[    1.536170] RIP  [<ffffffff8118d740>] cache_grow+0x460/0x470
[    1.541842]  RSP <ffff8817ed6f7b68>
[    1.545336] ---[ end trace 0ff547b29d70eb9b ]---
[    1.549953] Kernel panic - not syncing: Fatal exception
[    1.555187] Rebooting in 10 seconds..


Aaron, could it be related your change somehow?

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Samuel Ortiz]

Hi Anatol,

On Thu, Feb 14, 2013 at 02:48:05PM -0800, Anatol Pomozov wrote:
> Hi, Aaron
> 
> On Sun, Feb 3, 2013 at 9:02 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> > Hi Arron,
> >
> > On Thu, Jan 24, 2013 at 02:52:39PM -0600, Aaron Sierra wrote:
> >> In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found at
> >> offsets 0x58 and 0x5C, respectively. This patch allows GPIO access to
> >> properly be enabled (and disabled) for these chipsets.
> >>
> >> Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
> >> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> >> ---
> >>  drivers/mfd/lpc_ich.c |  109 ++++++++++++++++++++++++++++++++++---------------
> >>  1 file changed, 76 insertions(+), 33 deletions(-)
> > I applied this one to my for-next branch, but:
> >
> >
> >> @@ -858,14 +874,35 @@ wdt_done:
> >>  static int lpc_ich_probe(struct pci_dev *dev,
> >>                               const struct pci_device_id *id)
> >>  {
> >> +     struct lpc_ich_priv *priv;
> >>       int ret;
> >>       bool cell_added = false;
> >>
> >> -     ret = lpc_ich_init_wdt(dev, id);
> >> +     priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
> >> +     if (!priv)
> >> +             return -ENOMEM;
> > Could you please come back to me with a follow up patch that would convert
> > this to the devm_kzalloc() API ?
> 
> I compiled linux-next (sha == 019f4bc86a462), tried to boot it on my
> machine and it fails at the very early stage with following oops:
Would you mind trying the following fix:

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 5c2ef41..fddc8e2 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -916,7 +916,7 @@ static int lpc_ich_probe(struct pci_dev *dev,
        int ret;
        bool cell_added = false;
 
-       priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
+       priv = kzalloc(sizeof(struct lpc_ich_priv), GFP_KERNEL);
        if (!priv)
                return -ENOMEM;

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH v3] lpc_ich: fix gpio base and control offsets

[Anatol Pomozov]

Hi, Samuel.


-- replying my message to all
On Fri, Feb 15, 2013 at 2:16 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> Hi Anatol,
>
> On Thu, Feb 14, 2013 at 02:48:05PM -0800, Anatol Pomozov wrote:
>> Hi, Aaron
>>
>> On Sun, Feb 3, 2013 at 9:02 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
>> > Hi Arron,
>> >
>> > On Thu, Jan 24, 2013 at 02:52:39PM -0600, Aaron Sierra wrote:
>> >> In ICH5 and earlier the GPIOBASE and GPIOCTRL registers are found at
>> >> offsets 0x58 and 0x5C, respectively. This patch allows GPIO access to
>> >> properly be enabled (and disabled) for these chipsets.
>> >>
>> >> Signed-off-by: Agócs Pál <agocs.pal.86@gmail.com>
>> >> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
>> >> ---
>> >>  drivers/mfd/lpc_ich.c |  109 ++++++++++++++++++++++++++++++++++---------------
>> >>  1 file changed, 76 insertions(+), 33 deletions(-)
>> > I applied this one to my for-next branch, but:
>> >
>> >
>> >> @@ -858,14 +874,35 @@ wdt_done:
>> >>  static int lpc_ich_probe(struct pci_dev *dev,
>> >>                               const struct pci_device_id *id)
>> >>  {
>> >> +     struct lpc_ich_priv *priv;
>> >>       int ret;
>> >>       bool cell_added = false;
>> >>
>> >> -     ret = lpc_ich_init_wdt(dev, id);
>> >> +     priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
>> >> +     if (!priv)
>> >> +             return -ENOMEM;
>> > Could you please come back to me with a follow up patch that would convert
>> > this to the devm_kzalloc() API ?
>>
>> I compiled linux-next (sha == 019f4bc86a462), tried to boot it on my
>> machine and it fails at the very early stage with following oops:
> Would you mind trying the following fix:
>
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 5c2ef41..fddc8e2 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -916,7 +916,7 @@ static int lpc_ich_probe(struct pci_dev *dev,
>         int ret;
>         bool cell_added = false;
>
> -       priv = kmalloc(GFP_KERNEL, sizeof(struct lpc_ich_priv));
> +       priv = kzalloc(sizeof(struct lpc_ich_priv), GFP_KERNEL);
>         if (!priv)
>                 return -ENOMEM;

Yep, it helped. Now I can boot my machine with linux-next (the patch
is tested on Feb 15 tag).

Thanks for the quick fix.