* [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend()
@ 2011-07-21 0:19 Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 2/6 v2] gpio-pch: add spinlock in suspend/resume processing Tomoya MORINAGA
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Tomoya MORINAGA @ 2011-07-21 0:19 UTC (permalink / raw)
To: Grant Likely, linux-kernel, alexander.stein
Cc: qi.wang, yong.y.wang, joel.clark, kok.howg.ewe, toshiharu-linux,
Tomoya MORINAGA
Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
---
drivers/gpio/gpio-pch.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
index 36919e7..ca9c7b0 100644
--- a/drivers/gpio/gpio-pch.c
+++ b/drivers/gpio/gpio-pch.c
@@ -241,7 +241,6 @@ static int pch_gpio_suspend(struct pci_dev *pdev, pm_message_t state)
struct pch_gpio *chip = pci_get_drvdata(pdev);
pch_gpio_save_reg_conf(chip);
- pch_gpio_restore_reg_conf(chip);
ret = pci_save_state(pdev);
if (ret) {
--
1.7.4.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/6 v2] gpio-pch: add spinlock in suspend/resume processing
2011-07-21 0:19 [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend() Tomoya MORINAGA
@ 2011-07-21 0:19 ` Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 3/6 v2] gpio-pch: support ML7223 IOH n-Bus Tomoya MORINAGA
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Tomoya MORINAGA @ 2011-07-21 0:19 UTC (permalink / raw)
To: Grant Likely, linux-kernel, alexander.stein
Cc: qi.wang, yong.y.wang, joel.clark, kok.howg.ewe, toshiharu-linux,
Tomoya MORINAGA
Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
---
drivers/gpio/gpio-pch.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
index ca9c7b0..252bddb 100644
--- a/drivers/gpio/gpio-pch.c
+++ b/drivers/gpio/gpio-pch.c
@@ -55,6 +55,9 @@ struct pch_gpio_reg_data {
* @gpio: Data for GPIO infrastructure.
* @pch_gpio_reg: Memory mapped Register data is saved here
* when suspend.
+ * @spinlock: Used for register access protection in
+ * interrupt context pch_irq_mask,
+ * pch_irq_unmask and pch_irq_type;
*/
struct pch_gpio {
void __iomem *base;
@@ -63,6 +66,7 @@ struct pch_gpio {
struct gpio_chip gpio;
struct pch_gpio_reg_data pch_gpio_reg;
struct mutex lock;
+ spinlock_t spinlock;
};
static void pch_gpio_set(struct gpio_chip *gpio, unsigned nr, int val)
@@ -239,8 +243,11 @@ static int pch_gpio_suspend(struct pci_dev *pdev, pm_message_t state)
{
s32 ret;
struct pch_gpio *chip = pci_get_drvdata(pdev);
+ unsigned long flags;
+ spin_lock_irqsave(&chip->spinlock, flags);
pch_gpio_save_reg_conf(chip);
+ spin_unlock_irqrestore(&chip->spinlock, flags);
ret = pci_save_state(pdev);
if (ret) {
@@ -260,6 +267,7 @@ static int pch_gpio_resume(struct pci_dev *pdev)
{
s32 ret;
struct pch_gpio *chip = pci_get_drvdata(pdev);
+ unsigned long flags;
ret = pci_enable_wake(pdev, PCI_D0, 0);
@@ -271,9 +279,11 @@ static int pch_gpio_resume(struct pci_dev *pdev)
}
pci_restore_state(pdev);
+ spin_lock_irqsave(&chip->spinlock, flags);
iowrite32(0x01, &chip->reg->reset);
iowrite32(0x00, &chip->reg->reset);
pch_gpio_restore_reg_conf(chip);
+ spin_unlock_irqrestore(&chip->spinlock, flags);
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/6 v2] gpio-pch: support ML7223 IOH n-Bus
2011-07-21 0:19 [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend() Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 2/6 v2] gpio-pch: add spinlock in suspend/resume processing Tomoya MORINAGA
@ 2011-07-21 0:19 ` Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 4/6 v2] gpio-pch: modify gpio_nums and mask Tomoya MORINAGA
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Tomoya MORINAGA @ 2011-07-21 0:19 UTC (permalink / raw)
To: Grant Likely, linux-kernel, alexander.stein
Cc: qi.wang, yong.y.wang, joel.clark, kok.howg.ewe, toshiharu-linux,
Tomoya MORINAGA
Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
---
drivers/gpio/gpio-pch.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
index 252bddb..d548069 100644
--- a/drivers/gpio/gpio-pch.c
+++ b/drivers/gpio/gpio-pch.c
@@ -296,6 +296,7 @@ static int pch_gpio_resume(struct pci_dev *pdev)
static DEFINE_PCI_DEVICE_TABLE(pch_gpio_pcidev_id) = {
{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x8803) },
{ PCI_DEVICE(PCI_VENDOR_ID_ROHM, 0x8014) },
+ { PCI_DEVICE(PCI_VENDOR_ID_ROHM, 0x8043) },
{ 0, }
};
MODULE_DEVICE_TABLE(pci, pch_gpio_pcidev_id);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/6 v2] gpio-pch: modify gpio_nums and mask
2011-07-21 0:19 [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend() Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 2/6 v2] gpio-pch: add spinlock in suspend/resume processing Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 3/6 v2] gpio-pch: support ML7223 IOH n-Bus Tomoya MORINAGA
@ 2011-07-21 0:19 ` Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 5/6 v2] gpio-pch: Save register value in suspend() Tomoya MORINAGA
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Tomoya MORINAGA @ 2011-07-21 0:19 UTC (permalink / raw)
To: Grant Likely, linux-kernel, alexander.stein
Cc: qi.wang, yong.y.wang, joel.clark, kok.howg.ewe, toshiharu-linux,
Tomoya MORINAGA
Currently, the number of GPIO pins is set fixed value(=12).
Also PIN MASK is set as '0xfff'.
However the pins differs by IOH.
This patch sets the value correctly.
Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
---
drivers/gpio/gpio-pch.c | 31 +++++++++++++++++++++++++------
1 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
index d548069..4ac69bd 100644
--- a/drivers/gpio/gpio-pch.c
+++ b/drivers/gpio/gpio-pch.c
@@ -18,9 +18,6 @@
#include <linux/pci.h>
#include <linux/gpio.h>
-#define PCH_GPIO_ALL_PINS 0xfff /* Mask for GPIO pins 0 to 11 */
-#define GPIO_NUM_PINS 12 /* Specifies number of GPIO PINS GPIO0-GPIO11 */
-
struct pch_regs {
u32 ien;
u32 istatus;
@@ -37,6 +34,19 @@ struct pch_regs {
u32 reset;
};
+enum pch_type_t {
+ INTEL_EG20T_PCH,
+ OKISEMI_ML7223m_IOH, /* OKISEMI ML7223 IOH PCIe Bus-m */
+ OKISEMI_ML7223n_IOH /* OKISEMI ML7223 IOH PCIe Bus-n */
+};
+
+/* Specifies number of GPIO PINS */
+static int gpio_pins[] = {
+ [INTEL_EG20T_PCH] = 12,
+ [OKISEMI_ML7223m_IOH] = 8,
+ [OKISEMI_ML7223n_IOH] = 8,
+};
+
/**
* struct pch_gpio_reg_data - The register store data.
* @po_reg: To store contents of PO register.
@@ -55,6 +65,7 @@ struct pch_gpio_reg_data {
* @gpio: Data for GPIO infrastructure.
* @pch_gpio_reg: Memory mapped Register data is saved here
* when suspend.
+ * @ioh: IOH ID
* @spinlock: Used for register access protection in
* interrupt context pch_irq_mask,
* pch_irq_unmask and pch_irq_type;
@@ -66,6 +77,7 @@ struct pch_gpio {
struct gpio_chip gpio;
struct pch_gpio_reg_data pch_gpio_reg;
struct mutex lock;
+ enum pch_type_t ioh;
spinlock_t spinlock;
};
@@ -100,7 +112,7 @@ static int pch_gpio_direction_output(struct gpio_chip *gpio, unsigned nr,
u32 reg_val;
mutex_lock(&chip->lock);
- pm = ioread32(&chip->reg->pm) & PCH_GPIO_ALL_PINS;
+ pm = ioread32(&chip->reg->pm) & ((1 << gpio_pins[chip->ioh]) - 1);
pm |= (1 << nr);
iowrite32(pm, &chip->reg->pm);
@@ -122,7 +134,7 @@ static int pch_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
u32 pm;
mutex_lock(&chip->lock);
- pm = ioread32(&chip->reg->pm) & PCH_GPIO_ALL_PINS; /*bits 0-11*/
+ pm = ioread32(&chip->reg->pm) & ((1 << gpio_pins[chip->ioh]) - 1);
pm &= ~(1 << nr);
iowrite32(pm, &chip->reg->pm);
mutex_unlock(&chip->lock);
@@ -162,7 +174,7 @@ static void pch_gpio_setup(struct pch_gpio *chip)
gpio->set = pch_gpio_set;
gpio->dbg_show = NULL;
gpio->base = -1;
- gpio->ngpio = GPIO_NUM_PINS;
+ gpio->ngpio = gpio_pins[chip->ioh];
gpio->can_sleep = 0;
}
@@ -196,6 +208,13 @@ static int __devinit pch_gpio_probe(struct pci_dev *pdev,
goto err_iomap;
}
+ if (pdev->device == 0x8803)
+ chip->ioh = INTEL_EG20T_PCH;
+ else if (pdev->device == 0x8014)
+ chip->ioh = OKISEMI_ML7223m_IOH;
+ else if (pdev->device == 0x8043)
+ chip->ioh = OKISEMI_ML7223n_IOH;
+
chip->reg = chip->base;
pci_set_drvdata(pdev, chip);
mutex_init(&chip->lock);
--
1.7.4.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/6 v2] gpio-pch: Save register value in suspend()
2011-07-21 0:19 [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend() Tomoya MORINAGA
` (2 preceding siblings ...)
2011-07-21 0:19 ` [PATCH 4/6 v2] gpio-pch: modify gpio_nums and mask Tomoya MORINAGA
@ 2011-07-21 0:19 ` Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 6/6 v6] gpio-pch: Support interrupt function Tomoya MORINAGA
2011-10-05 18:00 ` [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend() Grant Likely
5 siblings, 0 replies; 13+ messages in thread
From: Tomoya MORINAGA @ 2011-07-21 0:19 UTC (permalink / raw)
To: Grant Likely, linux-kernel, alexander.stein
Cc: qi.wang, yong.y.wang, joel.clark, kok.howg.ewe, toshiharu-linux,
Tomoya MORINAGA
Currently, when suspend is occurred, register im0/1 and gpio_use_sel are not
saved.
This patch modifies so that register im0/1 and gpio_use_sel are saved.
Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
---
drivers/gpio/gpio-pch.c | 22 +++++++++++++++++++++-
1 files changed, 21 insertions(+), 1 deletions(-)
diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
index 4ac69bd..7f773af 100644
--- a/drivers/gpio/gpio-pch.c
+++ b/drivers/gpio/gpio-pch.c
@@ -30,7 +30,8 @@ struct pch_regs {
u32 pm;
u32 im0;
u32 im1;
- u32 reserved[4];
+ u32 reserved[3];
+ u32 gpio_use_sel;
u32 reset;
};
@@ -51,10 +52,17 @@ static int gpio_pins[] = {
* struct pch_gpio_reg_data - The register store data.
* @po_reg: To store contents of PO register.
* @pm_reg: To store contents of PM register.
+ * @im0_reg: To store contents of IM0 register.
+ * @im1_reg: To store contents of IM1 register.
+ * @gpio_use_sel_reg : To store contents of GPIO_USE_SEL register.
+ * (Only ML7223 Bus-n)
*/
struct pch_gpio_reg_data {
u32 po_reg;
u32 pm_reg;
+ u32 im0_reg;
+ u32 im1_reg;
+ u32 gpio_use_sel_reg;
};
/**
@@ -149,6 +157,12 @@ static void pch_gpio_save_reg_conf(struct pch_gpio *chip)
{
chip->pch_gpio_reg.po_reg = ioread32(&chip->reg->po);
chip->pch_gpio_reg.pm_reg = ioread32(&chip->reg->pm);
+ chip->pch_gpio_reg.im0_reg = ioread32(&chip->reg->im0);
+ if (chip->ioh == INTEL_EG20T_PCH)
+ chip->pch_gpio_reg.im1_reg = ioread32(&chip->reg->im1);
+ if (chip->ioh == OKISEMI_ML7223n_IOH)
+ chip->pch_gpio_reg.gpio_use_sel_reg =\
+ ioread32(&chip->reg->gpio_use_sel);
}
/*
@@ -160,6 +174,12 @@ static void pch_gpio_restore_reg_conf(struct pch_gpio *chip)
iowrite32(chip->pch_gpio_reg.po_reg, &chip->reg->po);
/* to store contents of PM register */
iowrite32(chip->pch_gpio_reg.pm_reg, &chip->reg->pm);
+ iowrite32(chip->pch_gpio_reg.im0_reg, &chip->reg->im0);
+ if (chip->ioh == INTEL_EG20T_PCH)
+ iowrite32(chip->pch_gpio_reg.im1_reg, &chip->reg->im1);
+ if (chip->ioh == OKISEMI_ML7223n_IOH)
+ iowrite32(chip->pch_gpio_reg.gpio_use_sel_reg,
+ &chip->reg->gpio_use_sel);
}
static void pch_gpio_setup(struct pch_gpio *chip)
--
1.7.4.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 6/6 v6] gpio-pch: Support interrupt function
2011-07-21 0:19 [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend() Tomoya MORINAGA
` (3 preceding siblings ...)
2011-07-21 0:19 ` [PATCH 5/6 v2] gpio-pch: Save register value in suspend() Tomoya MORINAGA
@ 2011-07-21 0:19 ` Tomoya MORINAGA
2011-12-08 19:37 ` gpio-pch: does not honour IRQF_ONESHOT? Jean-Francois Dagenais
2011-10-05 18:00 ` [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend() Grant Likely
5 siblings, 1 reply; 13+ messages in thread
From: Tomoya MORINAGA @ 2011-07-21 0:19 UTC (permalink / raw)
To: Grant Likely, linux-kernel, alexander.stein
Cc: qi.wang, yong.y.wang, joel.clark, kok.howg.ewe, toshiharu-linux,
Tomoya MORINAGA
Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
---
drivers/gpio/Kconfig | 1 +
drivers/gpio/gpio-pch.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 188 insertions(+), 0 deletions(-)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 2967002..bd64a55 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -352,6 +352,7 @@ config GPIO_LANGWELL
config GPIO_PCH
tristate "Intel EG20T PCH / OKI SEMICONDUCTOR ML7223 IOH GPIO"
depends on PCI && X86
+ select GENERIC_IRQ_CHIP
help
This driver is for PCH(Platform controller Hub) GPIO of Intel Topcliff
which is an IOH(Input/Output Hub) for x86 embedded processor.
diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
index 7f773af..46b5209 100644
--- a/drivers/gpio/gpio-pch.c
+++ b/drivers/gpio/gpio-pch.c
@@ -17,6 +17,17 @@
#include <linux/kernel.h>
#include <linux/pci.h>
#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+
+#define PCH_EDGE_FALLING 0
+#define PCH_EDGE_RISING BIT(0)
+#define PCH_LEVEL_L BIT(1)
+#define PCH_LEVEL_H (BIT(0) | BIT(1))
+#define PCH_EDGE_BOTH BIT(2)
+#define PCH_IM_MASK (BIT(0) | BIT(1) | BIT(2))
+
+#define PCH_IRQ_BASE 24
struct pch_regs {
u32 ien;
@@ -50,6 +61,8 @@ static int gpio_pins[] = {
/**
* struct pch_gpio_reg_data - The register store data.
+ * @ien_reg: To store contents of IEN register.
+ * @imask_reg: To store contents of IMASK register.
* @po_reg: To store contents of PO register.
* @pm_reg: To store contents of PM register.
* @im0_reg: To store contents of IM0 register.
@@ -58,6 +71,8 @@ static int gpio_pins[] = {
* (Only ML7223 Bus-n)
*/
struct pch_gpio_reg_data {
+ u32 ien_reg;
+ u32 imask_reg;
u32 po_reg;
u32 pm_reg;
u32 im0_reg;
@@ -73,6 +88,8 @@ struct pch_gpio_reg_data {
* @gpio: Data for GPIO infrastructure.
* @pch_gpio_reg: Memory mapped Register data is saved here
* when suspend.
+ * @lock: Used for register access protection
+ * @irq_base: Save base of IRQ number for interrupt
* @ioh: IOH ID
* @spinlock: Used for register access protection in
* interrupt context pch_irq_mask,
@@ -85,6 +102,7 @@ struct pch_gpio {
struct gpio_chip gpio;
struct pch_gpio_reg_data pch_gpio_reg;
struct mutex lock;
+ int irq_base;
enum pch_type_t ioh;
spinlock_t spinlock;
};
@@ -155,6 +173,8 @@ static int pch_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
*/
static void pch_gpio_save_reg_conf(struct pch_gpio *chip)
{
+ chip->pch_gpio_reg.ien_reg = ioread32(&chip->reg->ien);
+ chip->pch_gpio_reg.imask_reg = ioread32(&chip->reg->imask);
chip->pch_gpio_reg.po_reg = ioread32(&chip->reg->po);
chip->pch_gpio_reg.pm_reg = ioread32(&chip->reg->pm);
chip->pch_gpio_reg.im0_reg = ioread32(&chip->reg->im0);
@@ -170,6 +190,8 @@ static void pch_gpio_save_reg_conf(struct pch_gpio *chip)
*/
static void pch_gpio_restore_reg_conf(struct pch_gpio *chip)
{
+ iowrite32(chip->pch_gpio_reg.ien_reg, &chip->reg->ien);
+ iowrite32(chip->pch_gpio_reg.imask_reg, &chip->reg->imask);
/* to store contents of PO register */
iowrite32(chip->pch_gpio_reg.po_reg, &chip->reg->po);
/* to store contents of PM register */
@@ -182,6 +204,12 @@ static void pch_gpio_restore_reg_conf(struct pch_gpio *chip)
&chip->reg->gpio_use_sel);
}
+static int pch_gpio_to_irq(struct gpio_chip *gpio, unsigned offset)
+{
+ struct pch_gpio *chip = container_of(gpio, struct pch_gpio, gpio);
+ return chip->irq_base + offset;
+}
+
static void pch_gpio_setup(struct pch_gpio *chip)
{
struct gpio_chip *gpio = &chip->gpio;
@@ -196,6 +224,130 @@ static void pch_gpio_setup(struct pch_gpio *chip)
gpio->base = -1;
gpio->ngpio = gpio_pins[chip->ioh];
gpio->can_sleep = 0;
+ gpio->to_irq = pch_gpio_to_irq;
+}
+
+static int pch_irq_type(struct irq_data *d, unsigned int type)
+{
+ u32 im;
+ u32 *im_reg;
+ u32 ien;
+ u32 im_pos;
+ int ch;
+ unsigned long flags;
+ u32 val;
+ int irq = d->irq;
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ struct pch_gpio *chip = gc->private;
+
+ ch = irq - chip->irq_base;
+ if (irq <= chip->irq_base + 7) {
+ im_reg = &chip->reg->im0;
+ im_pos = ch;
+ } else {
+ im_reg = &chip->reg->im1;
+ im_pos = ch - 8;
+ }
+ dev_dbg(chip->dev, "%s:irq=%d type=%d ch=%d pos=%d\n",
+ __func__, irq, type, ch, im_pos);
+
+ spin_lock_irqsave(&chip->spinlock, flags);
+
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ val = PCH_EDGE_RISING;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ val = PCH_EDGE_FALLING;
+ break;
+ case IRQ_TYPE_EDGE_BOTH:
+ val = PCH_EDGE_BOTH;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ val = PCH_LEVEL_H;
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ val = PCH_LEVEL_L;
+ break;
+ case IRQ_TYPE_PROBE:
+ goto end;
+ default:
+ dev_warn(chip->dev, "%s: unknown type(%dd)",
+ __func__, type);
+ goto end;
+ }
+
+ /* Set interrupt mode */
+ im = ioread32(im_reg) & ~(PCH_IM_MASK << (im_pos * 4));
+ iowrite32(im | (val << (im_pos * 4)), im_reg);
+
+ /* iclr */
+ iowrite32(BIT(ch), &chip->reg->iclr);
+
+ /* IMASKCLR */
+ iowrite32(BIT(ch), &chip->reg->imaskclr);
+
+ /* Enable interrupt */
+ ien = ioread32(&chip->reg->ien);
+ iowrite32(ien | BIT(ch), &chip->reg->ien);
+end:
+ spin_unlock_irqrestore(&chip->spinlock, flags);
+
+ return 0;
+}
+
+static void pch_irq_unmask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ struct pch_gpio *chip = gc->private;
+
+ iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->imaskclr);
+}
+
+static void pch_irq_mask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ struct pch_gpio *chip = gc->private;
+
+ iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->imask);
+}
+
+static irqreturn_t pch_gpio_handler(int irq, void *dev_id)
+{
+ struct pch_gpio *chip = dev_id;
+ u32 reg_val = ioread32(&chip->reg->istatus);
+ int i;
+ int ret = IRQ_NONE;
+
+ for (i = 0; i < gpio_pins[chip->ioh]; i++) {
+ if (reg_val & BIT(i)) {
+ dev_dbg(chip->dev, "%s:[%d]:irq=%d status=0x%x\n",
+ __func__, i, irq, reg_val);
+ iowrite32(BIT(i), &chip->reg->iclr);
+ generic_handle_irq(chip->irq_base + i);
+ ret = IRQ_HANDLED;
+ }
+ }
+ return ret;
+}
+
+static __devinit void pch_gpio_alloc_generic_chip(struct pch_gpio *chip,
+ unsigned int irq_start, unsigned int num)
+{
+ struct irq_chip_generic *gc;
+ struct irq_chip_type *ct;
+
+ gc = irq_alloc_generic_chip("pch_gpio", 1, irq_start, chip->base,
+ handle_simple_irq);
+ gc->private = chip;
+ ct = gc->chip_types;
+
+ ct->chip.irq_mask = pch_irq_mask;
+ ct->chip.irq_unmask = pch_irq_unmask;
+ ct->chip.irq_set_type = pch_irq_type;
+
+ irq_setup_generic_chip(gc, IRQ_MSK(num), IRQ_GC_INIT_MASK_CACHE,
+ IRQ_NOREQUEST | IRQ_NOPROBE, 0);
}
static int __devinit pch_gpio_probe(struct pci_dev *pdev,
@@ -203,6 +355,7 @@ static int __devinit pch_gpio_probe(struct pci_dev *pdev,
{
s32 ret;
struct pch_gpio *chip;
+ int irq_base;
chip = kzalloc(sizeof(*chip), GFP_KERNEL);
if (chip == NULL)
@@ -245,8 +398,36 @@ static int __devinit pch_gpio_probe(struct pci_dev *pdev,
goto err_gpiochip_add;
}
+ irq_base = irq_alloc_descs(-1, 0, gpio_pins[chip->ioh], GFP_KERNEL);
+ if (irq_base < 0) {
+ dev_warn(&pdev->dev, "PCH gpio: Failed to get IRQ base num\n");
+ chip->irq_base = -1;
+ goto end;
+ }
+ chip->irq_base = irq_base;
+
+ ret = request_irq(pdev->irq, pch_gpio_handler,
+ IRQF_SHARED, KBUILD_MODNAME, chip);
+ if (ret != 0) {
+ dev_err(&pdev->dev,
+ "%s request_irq failed\n", __func__);
+ goto err_request_irq;
+ }
+
+ pch_gpio_alloc_generic_chip(chip, irq_base, gpio_pins[chip->ioh]);
+
+ /* Initialize interrupt ien register */
+ iowrite32(0, &chip->reg->ien);
+end:
return 0;
+err_request_irq:
+ irq_free_descs(irq_base, gpio_pins[chip->ioh]);
+
+ ret = gpiochip_remove(&chip->gpio);
+ if (ret)
+ dev_err(&pdev->dev, "%s gpiochip_remove failed\n", __func__);
+
err_gpiochip_add:
pci_iounmap(pdev, chip->base);
@@ -267,6 +448,12 @@ static void __devexit pch_gpio_remove(struct pci_dev *pdev)
int err;
struct pch_gpio *chip = pci_get_drvdata(pdev);
+ if (chip->irq_base != -1) {
+ free_irq(pdev->irq, chip);
+
+ irq_free_descs(chip->irq_base, gpio_pins[chip->ioh]);
+ }
+
err = gpiochip_remove(&chip->gpio);
if (err)
dev_err(&pdev->dev, "Failed gpiochip_remove\n");
--
1.7.4.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend()
2011-07-21 0:19 [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend() Tomoya MORINAGA
` (4 preceding siblings ...)
2011-07-21 0:19 ` [PATCH 6/6 v6] gpio-pch: Support interrupt function Tomoya MORINAGA
@ 2011-10-05 18:00 ` Grant Likely
5 siblings, 0 replies; 13+ messages in thread
From: Grant Likely @ 2011-10-05 18:00 UTC (permalink / raw)
To: Tomoya MORINAGA
Cc: linux-kernel, alexander.stein, qi.wang, yong.y.wang, joel.clark,
kok.howg.ewe, toshiharu-linux
Applied all 6, thanks.
g.
On Thu, Jul 21, 2011 at 09:19:54AM +0900, Tomoya MORINAGA wrote:
>
> Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
> ---
> drivers/gpio/gpio-pch.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
> index 36919e7..ca9c7b0 100644
> --- a/drivers/gpio/gpio-pch.c
> +++ b/drivers/gpio/gpio-pch.c
> @@ -241,7 +241,6 @@ static int pch_gpio_suspend(struct pci_dev *pdev, pm_message_t state)
> struct pch_gpio *chip = pci_get_drvdata(pdev);
>
> pch_gpio_save_reg_conf(chip);
> - pch_gpio_restore_reg_conf(chip);
>
> ret = pci_save_state(pdev);
> if (ret) {
> --
> 1.7.4.4
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* gpio-pch: does not honour IRQF_ONESHOT?
2011-07-21 0:19 ` [PATCH 6/6 v6] gpio-pch: Support interrupt function Tomoya MORINAGA
@ 2011-12-08 19:37 ` Jean-Francois Dagenais
2012-04-25 20:09 ` gpio-pch: BUG - driver does not honour IRQF_ONESHOT Jean-Francois Dagenais
0 siblings, 1 reply; 13+ messages in thread
From: Jean-Francois Dagenais @ 2011-12-08 19:37 UTC (permalink / raw)
To: Tomoya MORINAGA
Cc: Grant Likely, linux-kernel, alexander.stein, qi.wang,
yong.y.wang, joel.clark, kok.howg.ewe, toshiharu-linux
Hello all,
I am using the interrupt function of gpio-pch.c. I have an adp5588 who's interrupt line goes into one of the
GPIO lines of a EG20T.
I have a patch (not yet submitted to lkml) which lets the platform control the IRQ flags for the chip. I had to
do this to allow the IRQ line to be shared. I used IRQF_SHARED | IRQF_ONESHOT | IRQF_TRIGGER_LOW.
Unfortunately, since all of the handling of the adp5588 is done in a thread function, the interrupt stays low
between the moment the hard handler is run and the 5588 function is run. Since pch_gpio_handler clears
the interrupt status right away, I get an interrupt storm for the pch_gpio_handler function.
the line that does "iowrite32(BIT(i), &chip->reg->iclr);" right before calling generic_handle_irq should be
executed only after the corresponding nested ISR has run it's thread function.
I am open to patching this and submitting, but I would like some pointers before I dive in.
Thanks for the help!
(and thanks Tomoya for the interrupt support ;)
On Jul 20, 2011, at 20:19, Tomoya MORINAGA wrote:
>
> Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
> ---
> drivers/gpio/Kconfig | 1 +
> drivers/gpio/gpio-pch.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 188 insertions(+), 0 deletions(-)
> ...
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpio-pch: BUG - driver does not honour IRQF_ONESHOT
2011-12-08 19:37 ` gpio-pch: does not honour IRQF_ONESHOT? Jean-Francois Dagenais
@ 2012-04-25 20:09 ` Jean-Francois Dagenais
2012-04-25 23:03 ` gpio-pch: BUG - driver does not honour IRQF_ONESHOTn Thomas Gleixner
0 siblings, 1 reply; 13+ messages in thread
From: Jean-Francois Dagenais @ 2012-04-25 20:09 UTC (permalink / raw)
To: Tomoya MORINAGA, Grant Likely
Cc: open list, alexander.stein, qi.wang, yong.y.wang, joel.clark,
kok.howg.ewe, toshiharu-linux
Isn't anyone concerned with this? I am replying to myself to provoke a response.
Configuring a gpio pin with the gpio-pch driver with
"IRQF_TRIGGER_LOW | IRQF_ONESHOT" generates an interrupt
storm for threaded ISR until the ISR thread actually gets to physically clear
the interrupt on the triggering chip!! The immediate observable symptom
is the high CPU usage for my ISR thread task and the interrupt count in
/proc/interrupts incrementing radically.
See below for more details.
On Dec 8, 2011, at 14:37, Jean-Francois Dagenais wrote:
> Hello all,
>
> I am using the interrupt function of gpio-pch.c. I have an adp5588 who's interrupt line goes into one of the
> GPIO lines of a EG20T.
>
> I have a patch (not yet submitted to lkml) which lets the platform control the IRQ flags for the chip. I had to
> do this to allow the IRQ line to be shared. I used IRQF_SHARED | IRQF_ONESHOT | IRQF_TRIGGER_LOW.
>
> Unfortunately, since all of the handling of the adp5588 is done in a thread function, the interrupt stays low
> between the moment the hard handler is run and the 5588 function is run. Since pch_gpio_handler clears
> the interrupt status right away, I get an interrupt storm for the pch_gpio_handler function.
>
> the line that does "iowrite32(BIT(i), &chip->reg->iclr);" right before calling generic_handle_irq should be
> executed only after the corresponding nested ISR has run it's thread function.
>
> I am open to patching this and submitting, but I would like some pointers before I dive in.
>
> Thanks for the help!
> (and thanks Tomoya for the interrupt support ;)
>
> On Jul 20, 2011, at 20:19, Tomoya MORINAGA wrote:
>
>>
>> Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
>> ---
>> drivers/gpio/Kconfig | 1 +
>> drivers/gpio/gpio-pch.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 188 insertions(+), 0 deletions(-)
>> ...
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: gpio-pch: BUG - driver does not honour IRQF_ONESHOTn
2012-04-25 20:09 ` gpio-pch: BUG - driver does not honour IRQF_ONESHOT Jean-Francois Dagenais
@ 2012-04-25 23:03 ` Thomas Gleixner
2012-04-27 20:14 ` Jean-Francois Dagenais
0 siblings, 1 reply; 13+ messages in thread
From: Thomas Gleixner @ 2012-04-25 23:03 UTC (permalink / raw)
To: Jean-Francois Dagenais
Cc: Tomoya MORINAGA, Grant Likely, open list, alexander.stein,
qi.wang, yong.y.wang, joel.clark, kok.howg.ewe, toshiharu-linux
On Wed, 25 Apr 2012, Jean-Francois Dagenais wrote:
> Isn't anyone concerned with this? I am replying to myself to provoke a response.
Here you go.
> Configuring a gpio pin with the gpio-pch driver with
> "IRQF_TRIGGER_LOW | IRQF_ONESHOT" generates an interrupt
> storm for threaded ISR until the ISR thread actually gets to physically clear
> the interrupt on the triggering chip!! The immediate observable symptom
> is the high CPU usage for my ISR thread task and the interrupt count in
> /proc/interrupts incrementing radically.
> See below for more details.
>
> > Unfortunately, since all of the handling of the adp5588 is done in
> > a thread function, the interrupt stays low between the moment the
> > hard handler is run and the 5588 function is run. Since
> > pch_gpio_handler clears the interrupt status right away, I get an
> > interrupt storm for the pch_gpio_handler function.
> > the line that does "iowrite32(BIT(i), &chip->reg->iclr);" right
> > before calling generic_handle_irq should be executed only after
> > the corresponding nested ISR has run it's thread function.
Sigh, that driver is broken in several ways.
Does the following untested patch solve your problem?
It's not complete and I neither have access to that hardware nor did I
bother to look for docs.
Fact is, that using handle_simple_irq is preventing the usage of
oneshot threaded handlers for the demultiplexed interrupts, because
the simple handler does not deal with masking/unmasking. So yes you
run into an irq storm.
The "ack" of the sub interrupt before calling the handler is crap as
well. Though it kinda works as long as you are not using threaded
interrupts...
The other nonsense is that the set_type function unconditionally
enables the interrupt. It's supposed to set the type and nothing
else. The unmasking is done by the core code. If that hardware
requires to do the type changes in masked state, then it should
indicate it to the core by adding the IRQCHIP_SET_TYPE_MASKED flag to
the irq chip. But I can't see it masking it. I just see the
unconditional unmask and enable. Looks like one of those, it works but
we don't know why thingies....
That said, the patch is just done from my core code POV and the
limited information I was able to pull out of the driver code.
Thanks,
tglx
diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
index e8729cc..8b92fec 100644
--- a/drivers/gpio/gpio-pch.c
+++ b/drivers/gpio/gpio-pch.c
@@ -230,16 +230,12 @@ static void pch_gpio_setup(struct pch_gpio *chip)
static int pch_irq_type(struct irq_data *d, unsigned int type)
{
- u32 im;
- u32 __iomem *im_reg;
- u32 ien;
- u32 im_pos;
- int ch;
- unsigned long flags;
- u32 val;
- int irq = d->irq;
struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
struct pch_gpio *chip = gc->private;
+ u32 im, im_pos, val;
+ u32 __iomem *im_reg;
+ unsigned long flags;
+ int ch, irq = d->irq;
ch = irq - chip->irq_base;
if (irq <= chip->irq_base + 7) {
@@ -270,30 +266,22 @@ static int pch_irq_type(struct irq_data *d, unsigned int type)
case IRQ_TYPE_LEVEL_LOW:
val = PCH_LEVEL_L;
break;
- case IRQ_TYPE_PROBE:
- goto end;
default:
- dev_warn(chip->dev, "%s: unknown type(%dd)",
- __func__, type);
- goto end;
+ goto unlock;
}
/* Set interrupt mode */
im = ioread32(im_reg) & ~(PCH_IM_MASK << (im_pos * 4));
iowrite32(im | (val << (im_pos * 4)), im_reg);
- /* iclr */
- iowrite32(BIT(ch), &chip->reg->iclr);
+ /* And the handler */
+ if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
+ __irq_set_handler_locked(d->irq, handle_level_irq);
+ else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
+ __irq_set_handler_locked(d->irq, handle_edge_irq);
- /* IMASKCLR */
- iowrite32(BIT(ch), &chip->reg->imaskclr);
-
- /* Enable interrupt */
- ien = ioread32(&chip->reg->ien);
- iowrite32(ien | BIT(ch), &chip->reg->ien);
-end:
+unlock:
spin_unlock_irqrestore(&chip->spinlock, flags);
-
return 0;
}
@@ -313,18 +301,24 @@ static void pch_irq_mask(struct irq_data *d)
iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->imask);
}
+static void pch_irq_ack(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ struct pch_gpio *chip = gc->private;
+
+ iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->iclr);
+}
+
static irqreturn_t pch_gpio_handler(int irq, void *dev_id)
{
struct pch_gpio *chip = dev_id;
u32 reg_val = ioread32(&chip->reg->istatus);
- int i;
- int ret = IRQ_NONE;
+ int i, ret = IRQ_NONE;
for (i = 0; i < gpio_pins[chip->ioh]; i++) {
if (reg_val & BIT(i)) {
dev_dbg(chip->dev, "%s:[%d]:irq=%d status=0x%x\n",
__func__, i, irq, reg_val);
- iowrite32(BIT(i), &chip->reg->iclr);
generic_handle_irq(chip->irq_base + i);
ret = IRQ_HANDLED;
}
@@ -343,6 +337,7 @@ static __devinit void pch_gpio_alloc_generic_chip(struct pch_gpio *chip,
gc->private = chip;
ct = gc->chip_types;
+ ct->chip.irq_ack = pch_irq_ack;
ct->chip.irq_mask = pch_irq_mask;
ct->chip.irq_unmask = pch_irq_unmask;
ct->chip.irq_set_type = pch_irq_type;
@@ -357,6 +352,7 @@ static int __devinit pch_gpio_probe(struct pci_dev *pdev,
s32 ret;
struct pch_gpio *chip;
int irq_base;
+ u32 msk;
chip = kzalloc(sizeof(*chip), GFP_KERNEL);
if (chip == NULL)
@@ -418,8 +414,10 @@ static int __devinit pch_gpio_probe(struct pci_dev *pdev,
pch_gpio_alloc_generic_chip(chip, irq_base, gpio_pins[chip->ioh]);
- /* Initialize interrupt ien register */
- iowrite32(0, &chip->reg->ien);
+ /* Mask all interrupts, but enable them */
+ msk = (1 << gpio_pins[chip->ioh]) - 1;
+ iowrite32(msk, &chip->reg->imask);
+ iowrite32(msk, &chip->reg->ien);
end:
return 0;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: gpio-pch: BUG - driver does not honour IRQF_ONESHOTn
2012-04-25 23:03 ` gpio-pch: BUG - driver does not honour IRQF_ONESHOTn Thomas Gleixner
@ 2012-04-27 20:14 ` Jean-Francois Dagenais
2012-04-28 8:13 ` [PATCH] gpio: pch9: Use proper flow type handlers Thomas Gleixner
0 siblings, 1 reply; 13+ messages in thread
From: Jean-Francois Dagenais @ 2012-04-27 20:14 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Grant Likely, open list, alexander.stein, qi.wang, yong.y.wang,
joel.clark, kok.howg.ewe, tomoya.rohm
On Apr 25, 2012, at 19:03, Thomas Gleixner wrote:
> On Wed, 25 Apr 2012, Jean-Francois Dagenais wrote:
>
>> Isn't anyone concerned with this? I am replying to myself to provoke a response.
>
> Here you go.
>
>> Configuring a gpio pin with the gpio-pch driver with
>> "IRQF_TRIGGER_LOW | IRQF_ONESHOT" generates an interrupt
>> storm for threaded ISR until the ISR thread actually gets to physically clear
>> the interrupt on the triggering chip!! The immediate observable symptom
>> is the high CPU usage for my ISR thread task and the interrupt count in
>> /proc/interrupts incrementing radically.
>> See below for more details.
>>
>
>>> Unfortunately, since all of the handling of the adp5588 is done in
>>> a thread function, the interrupt stays low between the moment the
>>> hard handler is run and the 5588 function is run. Since
>>> pch_gpio_handler clears the interrupt status right away, I get an
>>> interrupt storm for the pch_gpio_handler function.
>
>>> the line that does "iowrite32(BIT(i), &chip->reg->iclr);" right
>>> before calling generic_handle_irq should be executed only after
>>> the corresponding nested ISR has run it's thread function.
>
> Sigh, that driver is broken in several ways.
>
> Does the following untested patch solve your problem?
it seems to be working ok, I have an ad714x on it and it requested
a threaded ISR with "IRQF_SHARED | IRQF_TRIGGER_LOW | IRQF_ONESHOT".
I will continue to use it and update you if I find anything wrong. If you format a
proper commit patch, I can ack it.
>
> It's not complete and I neither have access to that hardware nor did I
> bother to look for docs.
>
> Fact is, that using handle_simple_irq is preventing the usage of
> oneshot threaded handlers for the demultiplexed interrupts, because
> the simple handler does not deal with masking/unmasking. So yes you
> run into an irq storm.
>
> The "ack" of the sub interrupt before calling the handler is crap as
> well. Though it kinda works as long as you are not using threaded
> interrupts...
>
> The other nonsense is that the set_type function unconditionally
> enables the interrupt. It's supposed to set the type and nothing
> else. The unmasking is done by the core code. If that hardware
> requires to do the type changes in masked state, then it should
> indicate it to the core by adding the IRQCHIP_SET_TYPE_MASKED flag to
> the irq chip. But I can't see it masking it. I just see the
> unconditional unmask and enable. Looks like one of those, it works but
> we don't know why thingies....
>
> That said, the patch is just done from my core code POV and the
> limited information I was able to pull out of the driver code.
>
> Thanks,
Good job, Thank YOU!
>
> tglx
>
> diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
> index e8729cc..8b92fec 100644
> --- a/drivers/gpio/gpio-pch.c
> +++ b/drivers/gpio/gpio-pch.c
> @@ -230,16 +230,12 @@ static void pch_gpio_setup(struct pch_gpio *chip)
>
> static int pch_irq_type(struct irq_data *d, unsigned int type)
> {
> - u32 im;
> - u32 __iomem *im_reg;
> - u32 ien;
> - u32 im_pos;
> - int ch;
> - unsigned long flags;
> - u32 val;
> - int irq = d->irq;
> struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> struct pch_gpio *chip = gc->private;
> + u32 im, im_pos, val;
> + u32 __iomem *im_reg;
> + unsigned long flags;
> + int ch, irq = d->irq;
>
> ch = irq - chip->irq_base;
> if (irq <= chip->irq_base + 7) {
> @@ -270,30 +266,22 @@ static int pch_irq_type(struct irq_data *d, unsigned int type)
> case IRQ_TYPE_LEVEL_LOW:
> val = PCH_LEVEL_L;
> break;
> - case IRQ_TYPE_PROBE:
> - goto end;
> default:
> - dev_warn(chip->dev, "%s: unknown type(%dd)",
> - __func__, type);
> - goto end;
> + goto unlock;
> }
>
> /* Set interrupt mode */
> im = ioread32(im_reg) & ~(PCH_IM_MASK << (im_pos * 4));
> iowrite32(im | (val << (im_pos * 4)), im_reg);
>
> - /* iclr */
> - iowrite32(BIT(ch), &chip->reg->iclr);
> + /* And the handler */
> + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> + __irq_set_handler_locked(d->irq, handle_level_irq);
> + else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> + __irq_set_handler_locked(d->irq, handle_edge_irq);
>
> - /* IMASKCLR */
> - iowrite32(BIT(ch), &chip->reg->imaskclr);
> -
> - /* Enable interrupt */
> - ien = ioread32(&chip->reg->ien);
> - iowrite32(ien | BIT(ch), &chip->reg->ien);
> -end:
> +unlock:
> spin_unlock_irqrestore(&chip->spinlock, flags);
> -
> return 0;
> }
>
> @@ -313,18 +301,24 @@ static void pch_irq_mask(struct irq_data *d)
> iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->imask);
> }
>
> +static void pch_irq_ack(struct irq_data *d)
> +{
> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> + struct pch_gpio *chip = gc->private;
> +
> + iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->iclr);
> +}
> +
> static irqreturn_t pch_gpio_handler(int irq, void *dev_id)
> {
> struct pch_gpio *chip = dev_id;
> u32 reg_val = ioread32(&chip->reg->istatus);
> - int i;
> - int ret = IRQ_NONE;
> + int i, ret = IRQ_NONE;
>
> for (i = 0; i < gpio_pins[chip->ioh]; i++) {
> if (reg_val & BIT(i)) {
> dev_dbg(chip->dev, "%s:[%d]:irq=%d status=0x%x\n",
> __func__, i, irq, reg_val);
> - iowrite32(BIT(i), &chip->reg->iclr);
> generic_handle_irq(chip->irq_base + i);
> ret = IRQ_HANDLED;
> }
> @@ -343,6 +337,7 @@ static __devinit void pch_gpio_alloc_generic_chip(struct pch_gpio *chip,
> gc->private = chip;
> ct = gc->chip_types;
>
> + ct->chip.irq_ack = pch_irq_ack;
> ct->chip.irq_mask = pch_irq_mask;
> ct->chip.irq_unmask = pch_irq_unmask;
> ct->chip.irq_set_type = pch_irq_type;
> @@ -357,6 +352,7 @@ static int __devinit pch_gpio_probe(struct pci_dev *pdev,
> s32 ret;
> struct pch_gpio *chip;
> int irq_base;
> + u32 msk;
>
> chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> if (chip == NULL)
> @@ -418,8 +414,10 @@ static int __devinit pch_gpio_probe(struct pci_dev *pdev,
>
> pch_gpio_alloc_generic_chip(chip, irq_base, gpio_pins[chip->ioh]);
>
> - /* Initialize interrupt ien register */
> - iowrite32(0, &chip->reg->ien);
> + /* Mask all interrupts, but enable them */
> + msk = (1 << gpio_pins[chip->ioh]) - 1;
> + iowrite32(msk, &chip->reg->imask);
> + iowrite32(msk, &chip->reg->ien);
> end:
> return 0;
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] gpio: pch9: Use proper flow type handlers
2012-04-27 20:14 ` Jean-Francois Dagenais
@ 2012-04-28 8:13 ` Thomas Gleixner
2012-05-12 0:20 ` Grant Likely
0 siblings, 1 reply; 13+ messages in thread
From: Thomas Gleixner @ 2012-04-28 8:13 UTC (permalink / raw)
To: Jean-Francois Dagenais
Cc: Grant Likely, open list, alexander.stein, qi.wang, yong.y.wang,
joel.clark, kok.howg.ewe, tomoya.rohm
Jean-Francois Dagenais reported:
Configuring a gpio pin with the gpio-pch driver with
"IRQF_TRIGGER_LOW | IRQF_ONESHOT" generates an interrupt storm for
threaded ISR until the ISR thread actually gets to physically clear
the interrupt on the triggering chip!! The immediate observable
symptom is the high CPU usage for my ISR thread task and the
interrupt count in /proc/interrupts incrementing radically.
The driver is wrong in several ways:
1) Using handle_simple_irq() does not provide proper flow control
handling. In the case of oneshot threaded handlers for the
demultiplexed interrupts this results in an interrupt storm because
the simple handler does not deal with masking/unmasking. Even
without threaded oneshot handlers an interrupt storm for level type
interrupts can easily be triggered when the interrupt is disabled
and the interrupt line is activated from the device.
2) Acknowlegding the demultiplexed interrupt before calling the
handler is wrong for level type interrupts.
3) The set_type function unconditionally enables the interrupt. It's
supposed to set the type and nothing else. The unmasking is done by
the core code.
Move the acknowledge code into a separate function and add it to the
demux irqchip callbacks.
Remove the unconditional enabling from the set_type() callback and set
the proper flow handlers depending on the selected type (level/edge).
Reported-and-tested-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: alexander.stein@systec-electronic.com
Cc: qi.wang@intel.com
Cc: yong.y.wang@intel.com
Cc: joel.clark@intel.com
Cc: kok.howg.ewe@intel.com
Cc: toshiharu-linux@dsn.okisemi.com
Link: http://lkml.kernel.org/r/alpine.LFD.2.02.1204252332070.2542@ionos
---
drivers/gpio/gpio-pch.c | 57 +++++++++++++++++++++++-------------------------
1 file changed, 28 insertions(+), 29 deletions(-)
Index: linux-2.6/drivers/gpio/gpio-pch.c
===================================================================
--- linux-2.6.orig/drivers/gpio/gpio-pch.c
+++ linux-2.6/drivers/gpio/gpio-pch.c
@@ -230,16 +230,12 @@ static void pch_gpio_setup(struct pch_gp
static int pch_irq_type(struct irq_data *d, unsigned int type)
{
- u32 im;
- u32 __iomem *im_reg;
- u32 ien;
- u32 im_pos;
- int ch;
- unsigned long flags;
- u32 val;
- int irq = d->irq;
struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
struct pch_gpio *chip = gc->private;
+ u32 im, im_pos, val;
+ u32 __iomem *im_reg;
+ unsigned long flags;
+ int ch, irq = d->irq;
ch = irq - chip->irq_base;
if (irq <= chip->irq_base + 7) {
@@ -270,30 +266,22 @@ static int pch_irq_type(struct irq_data
case IRQ_TYPE_LEVEL_LOW:
val = PCH_LEVEL_L;
break;
- case IRQ_TYPE_PROBE:
- goto end;
default:
- dev_warn(chip->dev, "%s: unknown type(%dd)",
- __func__, type);
- goto end;
+ goto unlock;
}
/* Set interrupt mode */
im = ioread32(im_reg) & ~(PCH_IM_MASK << (im_pos * 4));
iowrite32(im | (val << (im_pos * 4)), im_reg);
- /* iclr */
- iowrite32(BIT(ch), &chip->reg->iclr);
+ /* And the handler */
+ if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
+ __irq_set_handler_locked(d->irq, handle_level_irq);
+ else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
+ __irq_set_handler_locked(d->irq, handle_edge_irq);
- /* IMASKCLR */
- iowrite32(BIT(ch), &chip->reg->imaskclr);
-
- /* Enable interrupt */
- ien = ioread32(&chip->reg->ien);
- iowrite32(ien | BIT(ch), &chip->reg->ien);
-end:
+unlock:
spin_unlock_irqrestore(&chip->spinlock, flags);
-
return 0;
}
@@ -313,18 +301,24 @@ static void pch_irq_mask(struct irq_data
iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->imask);
}
+static void pch_irq_ack(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ struct pch_gpio *chip = gc->private;
+
+ iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->iclr);
+}
+
static irqreturn_t pch_gpio_handler(int irq, void *dev_id)
{
struct pch_gpio *chip = dev_id;
u32 reg_val = ioread32(&chip->reg->istatus);
- int i;
- int ret = IRQ_NONE;
+ int i, ret = IRQ_NONE;
for (i = 0; i < gpio_pins[chip->ioh]; i++) {
if (reg_val & BIT(i)) {
dev_dbg(chip->dev, "%s:[%d]:irq=%d status=0x%x\n",
__func__, i, irq, reg_val);
- iowrite32(BIT(i), &chip->reg->iclr);
generic_handle_irq(chip->irq_base + i);
ret = IRQ_HANDLED;
}
@@ -343,6 +337,7 @@ static __devinit void pch_gpio_alloc_gen
gc->private = chip;
ct = gc->chip_types;
+ ct->chip.irq_ack = pch_irq_ack;
ct->chip.irq_mask = pch_irq_mask;
ct->chip.irq_unmask = pch_irq_unmask;
ct->chip.irq_set_type = pch_irq_type;
@@ -357,6 +352,7 @@ static int __devinit pch_gpio_probe(stru
s32 ret;
struct pch_gpio *chip;
int irq_base;
+ u32 msk;
chip = kzalloc(sizeof(*chip), GFP_KERNEL);
if (chip == NULL)
@@ -408,8 +404,13 @@ static int __devinit pch_gpio_probe(stru
}
chip->irq_base = irq_base;
+ /* Mask all interrupts, but enable them */
+ msk = (1 << gpio_pins[chip->ioh]) - 1;
+ iowrite32(msk, &chip->reg->imask);
+ iowrite32(msk, &chip->reg->ien);
+
ret = request_irq(pdev->irq, pch_gpio_handler,
- IRQF_SHARED, KBUILD_MODNAME, chip);
+ IRQF_SHARED, KBUILD_MODNAME, chip);
if (ret != 0) {
dev_err(&pdev->dev,
"%s request_irq failed\n", __func__);
@@ -418,8 +419,6 @@ static int __devinit pch_gpio_probe(stru
pch_gpio_alloc_generic_chip(chip, irq_base, gpio_pins[chip->ioh]);
- /* Initialize interrupt ien register */
- iowrite32(0, &chip->reg->ien);
end:
return 0;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio: pch9: Use proper flow type handlers
2012-04-28 8:13 ` [PATCH] gpio: pch9: Use proper flow type handlers Thomas Gleixner
@ 2012-05-12 0:20 ` Grant Likely
0 siblings, 0 replies; 13+ messages in thread
From: Grant Likely @ 2012-05-12 0:20 UTC (permalink / raw)
To: Thomas Gleixner, Jean-Francois Dagenais
Cc: open list, alexander.stein, qi.wang, yong.y.wang, joel.clark,
kok.howg.ewe, tomoya.rohm
On Sat, 28 Apr 2012 10:13:45 +0200 (CEST), Thomas Gleixner <tglx@linutronix.de> wrote:
> Jean-Francois Dagenais reported:
>
> Configuring a gpio pin with the gpio-pch driver with
> "IRQF_TRIGGER_LOW | IRQF_ONESHOT" generates an interrupt storm for
> threaded ISR until the ISR thread actually gets to physically clear
> the interrupt on the triggering chip!! The immediate observable
> symptom is the high CPU usage for my ISR thread task and the
> interrupt count in /proc/interrupts incrementing radically.
>
> The driver is wrong in several ways:
>
> 1) Using handle_simple_irq() does not provide proper flow control
> handling. In the case of oneshot threaded handlers for the
> demultiplexed interrupts this results in an interrupt storm because
> the simple handler does not deal with masking/unmasking. Even
> without threaded oneshot handlers an interrupt storm for level type
> interrupts can easily be triggered when the interrupt is disabled
> and the interrupt line is activated from the device.
>
> 2) Acknowlegding the demultiplexed interrupt before calling the
> handler is wrong for level type interrupts.
>
> 3) The set_type function unconditionally enables the interrupt. It's
> supposed to set the type and nothing else. The unmasking is done by
> the core code.
>
> Move the acknowledge code into a separate function and add it to the
> demux irqchip callbacks.
>
> Remove the unconditional enabling from the set_type() callback and set
> the proper flow handlers depending on the selected type (level/edge).
>
> Reported-and-tested-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: alexander.stein@systec-electronic.com
> Cc: qi.wang@intel.com
> Cc: yong.y.wang@intel.com
> Cc: joel.clark@intel.com
> Cc: kok.howg.ewe@intel.com
> Cc: toshiharu-linux@dsn.okisemi.com
> Link: http://lkml.kernel.org/r/alpine.LFD.2.02.1204252332070.2542@ionos
Applied, thanks. I'll push to Linus right away.
g.
> ---
> drivers/gpio/gpio-pch.c | 57 +++++++++++++++++++++++-------------------------
> 1 file changed, 28 insertions(+), 29 deletions(-)
>
> Index: linux-2.6/drivers/gpio/gpio-pch.c
> ===================================================================
> --- linux-2.6.orig/drivers/gpio/gpio-pch.c
> +++ linux-2.6/drivers/gpio/gpio-pch.c
> @@ -230,16 +230,12 @@ static void pch_gpio_setup(struct pch_gp
>
> static int pch_irq_type(struct irq_data *d, unsigned int type)
> {
> - u32 im;
> - u32 __iomem *im_reg;
> - u32 ien;
> - u32 im_pos;
> - int ch;
> - unsigned long flags;
> - u32 val;
> - int irq = d->irq;
> struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> struct pch_gpio *chip = gc->private;
> + u32 im, im_pos, val;
> + u32 __iomem *im_reg;
> + unsigned long flags;
> + int ch, irq = d->irq;
>
> ch = irq - chip->irq_base;
> if (irq <= chip->irq_base + 7) {
> @@ -270,30 +266,22 @@ static int pch_irq_type(struct irq_data
> case IRQ_TYPE_LEVEL_LOW:
> val = PCH_LEVEL_L;
> break;
> - case IRQ_TYPE_PROBE:
> - goto end;
> default:
> - dev_warn(chip->dev, "%s: unknown type(%dd)",
> - __func__, type);
> - goto end;
> + goto unlock;
> }
>
> /* Set interrupt mode */
> im = ioread32(im_reg) & ~(PCH_IM_MASK << (im_pos * 4));
> iowrite32(im | (val << (im_pos * 4)), im_reg);
>
> - /* iclr */
> - iowrite32(BIT(ch), &chip->reg->iclr);
> + /* And the handler */
> + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> + __irq_set_handler_locked(d->irq, handle_level_irq);
> + else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> + __irq_set_handler_locked(d->irq, handle_edge_irq);
>
> - /* IMASKCLR */
> - iowrite32(BIT(ch), &chip->reg->imaskclr);
> -
> - /* Enable interrupt */
> - ien = ioread32(&chip->reg->ien);
> - iowrite32(ien | BIT(ch), &chip->reg->ien);
> -end:
> +unlock:
> spin_unlock_irqrestore(&chip->spinlock, flags);
> -
> return 0;
> }
>
> @@ -313,18 +301,24 @@ static void pch_irq_mask(struct irq_data
> iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->imask);
> }
>
> +static void pch_irq_ack(struct irq_data *d)
> +{
> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> + struct pch_gpio *chip = gc->private;
> +
> + iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->iclr);
> +}
> +
> static irqreturn_t pch_gpio_handler(int irq, void *dev_id)
> {
> struct pch_gpio *chip = dev_id;
> u32 reg_val = ioread32(&chip->reg->istatus);
> - int i;
> - int ret = IRQ_NONE;
> + int i, ret = IRQ_NONE;
>
> for (i = 0; i < gpio_pins[chip->ioh]; i++) {
> if (reg_val & BIT(i)) {
> dev_dbg(chip->dev, "%s:[%d]:irq=%d status=0x%x\n",
> __func__, i, irq, reg_val);
> - iowrite32(BIT(i), &chip->reg->iclr);
> generic_handle_irq(chip->irq_base + i);
> ret = IRQ_HANDLED;
> }
> @@ -343,6 +337,7 @@ static __devinit void pch_gpio_alloc_gen
> gc->private = chip;
> ct = gc->chip_types;
>
> + ct->chip.irq_ack = pch_irq_ack;
> ct->chip.irq_mask = pch_irq_mask;
> ct->chip.irq_unmask = pch_irq_unmask;
> ct->chip.irq_set_type = pch_irq_type;
> @@ -357,6 +352,7 @@ static int __devinit pch_gpio_probe(stru
> s32 ret;
> struct pch_gpio *chip;
> int irq_base;
> + u32 msk;
>
> chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> if (chip == NULL)
> @@ -408,8 +404,13 @@ static int __devinit pch_gpio_probe(stru
> }
> chip->irq_base = irq_base;
>
> + /* Mask all interrupts, but enable them */
> + msk = (1 << gpio_pins[chip->ioh]) - 1;
> + iowrite32(msk, &chip->reg->imask);
> + iowrite32(msk, &chip->reg->ien);
> +
> ret = request_irq(pdev->irq, pch_gpio_handler,
> - IRQF_SHARED, KBUILD_MODNAME, chip);
> + IRQF_SHARED, KBUILD_MODNAME, chip);
> if (ret != 0) {
> dev_err(&pdev->dev,
> "%s request_irq failed\n", __func__);
> @@ -418,8 +419,6 @@ static int __devinit pch_gpio_probe(stru
>
> pch_gpio_alloc_generic_chip(chip, irq_base, gpio_pins[chip->ioh]);
>
> - /* Initialize interrupt ien register */
> - iowrite32(0, &chip->reg->ien);
> end:
> return 0;
>
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2012-05-12 0:20 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-21 0:19 [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend() Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 2/6 v2] gpio-pch: add spinlock in suspend/resume processing Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 3/6 v2] gpio-pch: support ML7223 IOH n-Bus Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 4/6 v2] gpio-pch: modify gpio_nums and mask Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 5/6 v2] gpio-pch: Save register value in suspend() Tomoya MORINAGA
2011-07-21 0:19 ` [PATCH 6/6 v6] gpio-pch: Support interrupt function Tomoya MORINAGA
2011-12-08 19:37 ` gpio-pch: does not honour IRQF_ONESHOT? Jean-Francois Dagenais
2012-04-25 20:09 ` gpio-pch: BUG - driver does not honour IRQF_ONESHOT Jean-Francois Dagenais
2012-04-25 23:03 ` gpio-pch: BUG - driver does not honour IRQF_ONESHOTn Thomas Gleixner
2012-04-27 20:14 ` Jean-Francois Dagenais
2012-04-28 8:13 ` [PATCH] gpio: pch9: Use proper flow type handlers Thomas Gleixner
2012-05-12 0:20 ` Grant Likely
2011-10-05 18:00 ` [PATCH 1/6 v2] gpio-pch: Delete invalid "restore" code in suspend() Grant Likely
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.