All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] RFT: pinctrl: samsung: separate wakeup irqdomain
@ 2015-04-09  7:40 ` Linus Walleij
  0 siblings, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2015-04-09  7:40 UTC (permalink / raw)
  To: linux-arm-kernel, linux-samsung-soc, Tomasz Figa, Kukjin Kim, Ben Dooks
  Cc: Linus Walleij

The Samsung pin control driver and subdrivers sometimes use both
an irqdomain for some IRQs and another irqdomain for wakeup irqs,
registering the first with a call to the per-SoC callback
.eint_gpio_init() and the second with a call to .eint_wkup_init()
however it seems both runpaths will assign the resulting irqdomain
to the per-bank bank.irq_domain member, making the second
(wakeup) irqdomain overwrite the first one.

I'm surprised this even works, and it seems that the S3C
per-domain "domain data" that seems to be used in part to work
around this bug by creating a local array with copies of the
irqdomain (!).

This patch mainly adds a new record to the GPIO/pin "bank"
for wakeups and use this in the .eint_wkup_init() callbacks
to pave the way for more cleanups.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Tomasz etc: I don't know if I'm just misunderstanding this,
can you look at it and tell me how badly I misunderstand
these Samsung wakeups, to me it is a complete mystery.
---
 drivers/pinctrl/samsung/pinctrl-exynos.c  | 6 +++---
 drivers/pinctrl/samsung/pinctrl-s3c24xx.c | 6 +++---
 drivers/pinctrl/samsung/pinctrl-s3c64xx.c | 6 +++---
 drivers/pinctrl/samsung/pinctrl-samsung.h | 1 +
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index c8f83f96546c..6adf34e3cce6 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -469,7 +469,7 @@ static void exynos_irq_demux_eint16_31(unsigned int irq, struct irq_desc *desc)
 				+ b->eint_offset);
 		mask = readl(d->virt_base + b->irq_chip->eint_mask
 				+ b->eint_offset);
-		exynos_irq_demux_eint(pend & ~mask, b->irq_domain);
+		exynos_irq_demux_eint(pend & ~mask, b->wkup_domain);
 	}
 
 	chained_irq_exit(chip, desc);
@@ -511,9 +511,9 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 		if (bank->eint_type != EINT_TYPE_WKUP)
 			continue;
 
-		bank->irq_domain = irq_domain_add_linear(bank->of_node,
+		bank->wkup_domain = irq_domain_add_linear(bank->of_node,
 				bank->nr_pins, &exynos_eint_irqd_ops, bank);
-		if (!bank->irq_domain) {
+		if (!bank->wkup_domain) {
 			dev_err(dev, "wkup irq domain add failed\n");
 			return -ENXIO;
 		}
diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
index f1993f42114c..1dd1e5a0ed58 100644
--- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
+++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
@@ -539,9 +539,9 @@ static int s3c24xx_eint_init(struct samsung_pinctrl_drv_data *d)
 		ops = (bank->eint_offset == 0) ? &s3c24xx_gpf_irq_ops
 					       : &s3c24xx_gpg_irq_ops;
 
-		bank->irq_domain = irq_domain_add_linear(bank->of_node,
+		bank->wkup_domain = irq_domain_add_linear(bank->of_node,
 				bank->nr_pins, ops, ddata);
-		if (!bank->irq_domain) {
+		if (!bank->wkup_domain) {
 			dev_err(dev, "wkup irq domain add failed\n");
 			return -ENXIO;
 		}
@@ -553,7 +553,7 @@ static int s3c24xx_eint_init(struct samsung_pinctrl_drv_data *d)
 				break;
 			if (!(mask & 1))
 				continue;
-			eint_data->domains[irq] = bank->irq_domain;
+			eint_data->domains[irq] = bank->wkup_domain;
 			++irq;
 		}
 	}
diff --git a/drivers/pinctrl/samsung/pinctrl-s3c64xx.c b/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
index 7756c1e9e763..9701424a8bea 100644
--- a/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
+++ b/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
@@ -757,9 +757,9 @@ static int s3c64xx_eint_eint0_init(struct samsung_pinctrl_drv_data *d)
 		}
 		ddata->bank = bank;
 
-		bank->irq_domain = irq_domain_add_linear(bank->of_node,
+		bank->wkup_domain = irq_domain_add_linear(bank->of_node,
 				nr_eints, &s3c64xx_eint0_irqd_ops, ddata);
-		if (!bank->irq_domain) {
+		if (!bank->wkup_domain) {
 			dev_err(dev, "wkup irq domain add failed\n");
 			return -ENXIO;
 		}
@@ -769,7 +769,7 @@ static int s3c64xx_eint_eint0_init(struct samsung_pinctrl_drv_data *d)
 		for (pin = 0; mask; ++pin, mask >>= 1) {
 			if (!(mask & 1))
 				continue;
-			data->domains[irq] = bank->irq_domain;
+			data->domains[irq] = bank->wkup_domain;
 			data->pins[irq] = pin;
 			ddata->eints[pin] = irq;
 			++irq;
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 1b8c0139d604..c97dd1e6ae2e 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -170,6 +170,7 @@ struct samsung_pin_bank {
 	struct device_node *of_node;
 	struct samsung_pinctrl_drv_data *drvdata;
 	struct irq_domain *irq_domain;
+	struct irq_domain *wkup_domain;
 	struct gpio_chip gpio_chip;
 	struct pinctrl_gpio_range grange;
 	struct exynos_irq_chip *irq_chip;
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH] RFT: pinctrl: samsung: separate wakeup irqdomain
@ 2015-04-09  7:40 ` Linus Walleij
  0 siblings, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2015-04-09  7:40 UTC (permalink / raw)
  To: linux-arm-kernel

The Samsung pin control driver and subdrivers sometimes use both
an irqdomain for some IRQs and another irqdomain for wakeup irqs,
registering the first with a call to the per-SoC callback
.eint_gpio_init() and the second with a call to .eint_wkup_init()
however it seems both runpaths will assign the resulting irqdomain
to the per-bank bank.irq_domain member, making the second
(wakeup) irqdomain overwrite the first one.

I'm surprised this even works, and it seems that the S3C
per-domain "domain data" that seems to be used in part to work
around this bug by creating a local array with copies of the
irqdomain (!).

This patch mainly adds a new record to the GPIO/pin "bank"
for wakeups and use this in the .eint_wkup_init() callbacks
to pave the way for more cleanups.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Tomasz etc: I don't know if I'm just misunderstanding this,
can you look at it and tell me how badly I misunderstand
these Samsung wakeups, to me it is a complete mystery.
---
 drivers/pinctrl/samsung/pinctrl-exynos.c  | 6 +++---
 drivers/pinctrl/samsung/pinctrl-s3c24xx.c | 6 +++---
 drivers/pinctrl/samsung/pinctrl-s3c64xx.c | 6 +++---
 drivers/pinctrl/samsung/pinctrl-samsung.h | 1 +
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index c8f83f96546c..6adf34e3cce6 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -469,7 +469,7 @@ static void exynos_irq_demux_eint16_31(unsigned int irq, struct irq_desc *desc)
 				+ b->eint_offset);
 		mask = readl(d->virt_base + b->irq_chip->eint_mask
 				+ b->eint_offset);
-		exynos_irq_demux_eint(pend & ~mask, b->irq_domain);
+		exynos_irq_demux_eint(pend & ~mask, b->wkup_domain);
 	}
 
 	chained_irq_exit(chip, desc);
@@ -511,9 +511,9 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 		if (bank->eint_type != EINT_TYPE_WKUP)
 			continue;
 
-		bank->irq_domain = irq_domain_add_linear(bank->of_node,
+		bank->wkup_domain = irq_domain_add_linear(bank->of_node,
 				bank->nr_pins, &exynos_eint_irqd_ops, bank);
-		if (!bank->irq_domain) {
+		if (!bank->wkup_domain) {
 			dev_err(dev, "wkup irq domain add failed\n");
 			return -ENXIO;
 		}
diff --git a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
index f1993f42114c..1dd1e5a0ed58 100644
--- a/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
+++ b/drivers/pinctrl/samsung/pinctrl-s3c24xx.c
@@ -539,9 +539,9 @@ static int s3c24xx_eint_init(struct samsung_pinctrl_drv_data *d)
 		ops = (bank->eint_offset == 0) ? &s3c24xx_gpf_irq_ops
 					       : &s3c24xx_gpg_irq_ops;
 
-		bank->irq_domain = irq_domain_add_linear(bank->of_node,
+		bank->wkup_domain = irq_domain_add_linear(bank->of_node,
 				bank->nr_pins, ops, ddata);
-		if (!bank->irq_domain) {
+		if (!bank->wkup_domain) {
 			dev_err(dev, "wkup irq domain add failed\n");
 			return -ENXIO;
 		}
@@ -553,7 +553,7 @@ static int s3c24xx_eint_init(struct samsung_pinctrl_drv_data *d)
 				break;
 			if (!(mask & 1))
 				continue;
-			eint_data->domains[irq] = bank->irq_domain;
+			eint_data->domains[irq] = bank->wkup_domain;
 			++irq;
 		}
 	}
diff --git a/drivers/pinctrl/samsung/pinctrl-s3c64xx.c b/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
index 7756c1e9e763..9701424a8bea 100644
--- a/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
+++ b/drivers/pinctrl/samsung/pinctrl-s3c64xx.c
@@ -757,9 +757,9 @@ static int s3c64xx_eint_eint0_init(struct samsung_pinctrl_drv_data *d)
 		}
 		ddata->bank = bank;
 
-		bank->irq_domain = irq_domain_add_linear(bank->of_node,
+		bank->wkup_domain = irq_domain_add_linear(bank->of_node,
 				nr_eints, &s3c64xx_eint0_irqd_ops, ddata);
-		if (!bank->irq_domain) {
+		if (!bank->wkup_domain) {
 			dev_err(dev, "wkup irq domain add failed\n");
 			return -ENXIO;
 		}
@@ -769,7 +769,7 @@ static int s3c64xx_eint_eint0_init(struct samsung_pinctrl_drv_data *d)
 		for (pin = 0; mask; ++pin, mask >>= 1) {
 			if (!(mask & 1))
 				continue;
-			data->domains[irq] = bank->irq_domain;
+			data->domains[irq] = bank->wkup_domain;
 			data->pins[irq] = pin;
 			ddata->eints[pin] = irq;
 			++irq;
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 1b8c0139d604..c97dd1e6ae2e 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -170,6 +170,7 @@ struct samsung_pin_bank {
 	struct device_node *of_node;
 	struct samsung_pinctrl_drv_data *drvdata;
 	struct irq_domain *irq_domain;
+	struct irq_domain *wkup_domain;
 	struct gpio_chip gpio_chip;
 	struct pinctrl_gpio_range grange;
 	struct exynos_irq_chip *irq_chip;
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] RFT: pinctrl: samsung: separate wakeup irqdomain
  2015-04-09  7:40 ` Linus Walleij
@ 2015-04-09 12:00   ` Tomasz Figa
  -1 siblings, 0 replies; 4+ messages in thread
From: Tomasz Figa @ 2015-04-09 12:00 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-arm-kernel, linux-samsung-soc, Kukjin Kim, Ben Dooks

Hi Linus,

2015-04-09 9:40 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
> The Samsung pin control driver and subdrivers sometimes use both
> an irqdomain for some IRQs and another irqdomain for wakeup irqs,
> registering the first with a call to the per-SoC callback
> .eint_gpio_init() and the second with a call to .eint_wkup_init()
> however it seems both runpaths will assign the resulting irqdomain
> to the per-bank bank.irq_domain member, making the second
> (wakeup) irqdomain overwrite the first one.
>
> I'm surprised this even works, and it seems that the S3C
> per-domain "domain data" that seems to be used in part to work
> around this bug by creating a local array with copies of the
> irqdomain (!).
>
> This patch mainly adds a new record to the GPIO/pin "bank"
> for wakeups and use this in the .eint_wkup_init() callbacks
> to pave the way for more cleanups.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Tomasz etc: I don't know if I'm just misunderstanding this,
> can you look at it and tell me how badly I misunderstand
> these Samsung wakeups, to me it is a complete mystery.

On all Samsung SoCs known to me, there is no GPIO bank which have both
eint_wkup and eint_gpio functions at the same time, so there is no way
that bank->irq_domain could be overwritten.

On S3C64xx, the arrays in domain data are necessary to work around
crazy layout of pin banks which do not fully correspond to EINT banks
in the controller.

So, in general, I don't think there is really any problem with this
driver at the moment. If, in future, a SoC shows up with both IRQ
types in the same pin bank, then it will have to be reworked, but I
don't see why such thing could show up in a SoC, because it wouldn't
really add any value.

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCH] RFT: pinctrl: samsung: separate wakeup irqdomain
@ 2015-04-09 12:00   ` Tomasz Figa
  0 siblings, 0 replies; 4+ messages in thread
From: Tomasz Figa @ 2015-04-09 12:00 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Linus,

2015-04-09 9:40 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
> The Samsung pin control driver and subdrivers sometimes use both
> an irqdomain for some IRQs and another irqdomain for wakeup irqs,
> registering the first with a call to the per-SoC callback
> .eint_gpio_init() and the second with a call to .eint_wkup_init()
> however it seems both runpaths will assign the resulting irqdomain
> to the per-bank bank.irq_domain member, making the second
> (wakeup) irqdomain overwrite the first one.
>
> I'm surprised this even works, and it seems that the S3C
> per-domain "domain data" that seems to be used in part to work
> around this bug by creating a local array with copies of the
> irqdomain (!).
>
> This patch mainly adds a new record to the GPIO/pin "bank"
> for wakeups and use this in the .eint_wkup_init() callbacks
> to pave the way for more cleanups.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Tomasz etc: I don't know if I'm just misunderstanding this,
> can you look at it and tell me how badly I misunderstand
> these Samsung wakeups, to me it is a complete mystery.

On all Samsung SoCs known to me, there is no GPIO bank which have both
eint_wkup and eint_gpio functions at the same time, so there is no way
that bank->irq_domain could be overwritten.

On S3C64xx, the arrays in domain data are necessary to work around
crazy layout of pin banks which do not fully correspond to EINT banks
in the controller.

So, in general, I don't think there is really any problem with this
driver at the moment. If, in future, a SoC shows up with both IRQ
types in the same pin bank, then it will have to be reworked, but I
don't see why such thing could show up in a SoC, because it wouldn't
really add any value.

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-04-09 12:00 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-09  7:40 [PATCH] RFT: pinctrl: samsung: separate wakeup irqdomain Linus Walleij
2015-04-09  7:40 ` Linus Walleij
2015-04-09 12:00 ` Tomasz Figa
2015-04-09 12:00   ` Tomasz Figa

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.