All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
@ 2022-04-21 14:08 ` Marek Vasut
  0 siblings, 0 replies; 14+ messages in thread
From: Marek Vasut @ 2022-04-21 14:08 UTC (permalink / raw)
  To: linux-gpio
  Cc: Marek Vasut, Alexandre Torgue, Fabien Dessenne, Linus Walleij,
	Marc Zyngier, linux-stm32, linux-arm-kernel

The current EOI handler for LEVEL triggered interrupts calls clk_enable(),
register IO, clk_disable(). The clock manipulation requires locking which
happens with IRQs disabled in clk_enable_lock(). Instead of turning the
clock on and off all the time, enable the clock in case LEVEL interrupt is
requested and keep the clock enabled until all LEVEL interrupts are freed.
The LEVEL interrupts are an exception on this platform and seldom used, so
this does not affect the common case.

This simplifies the LEVEL interrupt handling considerably and also fixes
the following splat found when using preempt-rt:
 ------------[ cut here ]------------
 WARNING: CPU: 0 PID: 0 at kernel/locking/rtmutex.c:2040 __rt_mutex_trylock+0x37/0x62
 Modules linked in:
 CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.109-rt65-stable-standard-00068-g6a5afc4b1217 #85
 Hardware name: STM32 (Device Tree Support)
 [<c010a45d>] (unwind_backtrace) from [<c010766f>] (show_stack+0xb/0xc)
 [<c010766f>] (show_stack) from [<c06353ab>] (dump_stack+0x6f/0x84)
 [<c06353ab>] (dump_stack) from [<c01145e3>] (__warn+0x7f/0xa4)
 [<c01145e3>] (__warn) from [<c063386f>] (warn_slowpath_fmt+0x3b/0x74)
 [<c063386f>] (warn_slowpath_fmt) from [<c063b43d>] (__rt_mutex_trylock+0x37/0x62)
 [<c063b43d>] (__rt_mutex_trylock) from [<c063c053>] (rt_spin_trylock+0x7/0x16)
 [<c063c053>] (rt_spin_trylock) from [<c036a2f3>] (clk_enable_lock+0xb/0x80)
 [<c036a2f3>] (clk_enable_lock) from [<c036ba69>] (clk_core_enable_lock+0x9/0x18)
 [<c036ba69>] (clk_core_enable_lock) from [<c034e9f3>] (stm32_gpio_get+0x11/0x24)
 [<c034e9f3>] (stm32_gpio_get) from [<c034ef43>] (stm32_gpio_irq_trigger+0x1f/0x48)
 [<c034ef43>] (stm32_gpio_irq_trigger) from [<c014aa53>] (handle_fasteoi_irq+0x71/0xa8)
 [<c014aa53>] (handle_fasteoi_irq) from [<c0147111>] (generic_handle_irq+0x19/0x22)
 [<c0147111>] (generic_handle_irq) from [<c014752d>] (__handle_domain_irq+0x55/0x64)
 [<c014752d>] (__handle_domain_irq) from [<c0346f13>] (gic_handle_irq+0x53/0x64)
 [<c0346f13>] (gic_handle_irq) from [<c0100ba5>] (__irq_svc+0x65/0xc0)
 Exception stack(0xc0e01f18 to 0xc0e01f60)
 1f00:                                                       0000300c 00000000
 1f20: 0000300c c010ff01 00000000 00000000 c0e00000 c0e07714 00000001 c0e01f78
 1f40: c0e07758 00000000 ef7cd0ff c0e01f68 c010554b c0105542 40000033 ffffffff
 [<c0100ba5>] (__irq_svc) from [<c0105542>] (arch_cpu_idle+0xc/0x1e)
 [<c0105542>] (arch_cpu_idle) from [<c063be95>] (default_idle_call+0x21/0x3c)
 [<c063be95>] (default_idle_call) from [<c01324f7>] (do_idle+0xe3/0x1e4)
 [<c01324f7>] (do_idle) from [<c01327b3>] (cpu_startup_entry+0x13/0x14)
 [<c01327b3>] (cpu_startup_entry) from [<c0a00c13>] (start_kernel+0x397/0x3d4)
 [<c0a00c13>] (start_kernel) from [<00000000>] (0x0)
 ---[ end trace 0000000000000002 ]---

Power consumption measured on STM32MP157C DHCOM SoM is not increased or
is below noise threshold.

Fixes: 47beed513a85b ("pinctrl: stm32: Add level interrupt support to gpio irq chip")
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Fabien Dessenne <fabien.dessenne@foss.st.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
To: linux-gpio@vger.kernel.org
---
 drivers/pinctrl/stm32/pinctrl-stm32.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 242d1c37c6e4..7aecd0efde07 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -226,6 +226,13 @@ static void stm32_gpio_free(struct gpio_chip *chip, unsigned offset)
 	pinctrl_gpio_free(chip->base + offset);
 }
 
+static int stm32_gpio_get_noclk(struct gpio_chip *chip, unsigned int offset)
+{
+	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
+
+	return !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
+}
+
 static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
 	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
@@ -233,7 +240,7 @@ static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
 
 	clk_enable(bank->clk);
 
-	ret = !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
+	ret = stm32_gpio_get_noclk(chip, offset);
 
 	clk_disable(bank->clk);
 
@@ -317,7 +324,7 @@ static void stm32_gpio_irq_trigger(struct irq_data *d)
 		return;
 
 	/* If level interrupt type then retrig */
-	level = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
+	level = stm32_gpio_get_noclk(&bank->gpio_chip, d->hwirq);
 	if ((level == 0 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_LOW) ||
 	    (level == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
 		irq_chip_retrigger_hierarchy(d);
@@ -359,6 +366,7 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
 {
 	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
 	struct stm32_pinctrl *pctl = dev_get_drvdata(bank->gpio_chip.parent);
+	unsigned long flags;
 	int ret;
 
 	ret = stm32_gpio_direction_input(&bank->gpio_chip, irq_data->hwirq);
@@ -372,6 +380,10 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
 		return ret;
 	}
 
+	flags = irqd_get_trigger_type(irq_data);
+	if (flags & IRQ_TYPE_LEVEL_MASK)
+		clk_enable(bank->clk);
+
 	return 0;
 }
 
@@ -379,6 +391,9 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
 {
 	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
 
+	if (bank->irq_type[irq_data->hwirq] & IRQ_TYPE_LEVEL_MASK)
+		clk_disable(bank->clk);
+
 	gpiochip_unlock_as_irq(&bank->gpio_chip, irq_data->hwirq);
 }
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
@ 2022-04-21 14:08 ` Marek Vasut
  0 siblings, 0 replies; 14+ messages in thread
From: Marek Vasut @ 2022-04-21 14:08 UTC (permalink / raw)
  To: linux-gpio
  Cc: Marek Vasut, Alexandre Torgue, Fabien Dessenne, Linus Walleij,
	Marc Zyngier, linux-stm32, linux-arm-kernel

The current EOI handler for LEVEL triggered interrupts calls clk_enable(),
register IO, clk_disable(). The clock manipulation requires locking which
happens with IRQs disabled in clk_enable_lock(). Instead of turning the
clock on and off all the time, enable the clock in case LEVEL interrupt is
requested and keep the clock enabled until all LEVEL interrupts are freed.
The LEVEL interrupts are an exception on this platform and seldom used, so
this does not affect the common case.

This simplifies the LEVEL interrupt handling considerably and also fixes
the following splat found when using preempt-rt:
 ------------[ cut here ]------------
 WARNING: CPU: 0 PID: 0 at kernel/locking/rtmutex.c:2040 __rt_mutex_trylock+0x37/0x62
 Modules linked in:
 CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.109-rt65-stable-standard-00068-g6a5afc4b1217 #85
 Hardware name: STM32 (Device Tree Support)
 [<c010a45d>] (unwind_backtrace) from [<c010766f>] (show_stack+0xb/0xc)
 [<c010766f>] (show_stack) from [<c06353ab>] (dump_stack+0x6f/0x84)
 [<c06353ab>] (dump_stack) from [<c01145e3>] (__warn+0x7f/0xa4)
 [<c01145e3>] (__warn) from [<c063386f>] (warn_slowpath_fmt+0x3b/0x74)
 [<c063386f>] (warn_slowpath_fmt) from [<c063b43d>] (__rt_mutex_trylock+0x37/0x62)
 [<c063b43d>] (__rt_mutex_trylock) from [<c063c053>] (rt_spin_trylock+0x7/0x16)
 [<c063c053>] (rt_spin_trylock) from [<c036a2f3>] (clk_enable_lock+0xb/0x80)
 [<c036a2f3>] (clk_enable_lock) from [<c036ba69>] (clk_core_enable_lock+0x9/0x18)
 [<c036ba69>] (clk_core_enable_lock) from [<c034e9f3>] (stm32_gpio_get+0x11/0x24)
 [<c034e9f3>] (stm32_gpio_get) from [<c034ef43>] (stm32_gpio_irq_trigger+0x1f/0x48)
 [<c034ef43>] (stm32_gpio_irq_trigger) from [<c014aa53>] (handle_fasteoi_irq+0x71/0xa8)
 [<c014aa53>] (handle_fasteoi_irq) from [<c0147111>] (generic_handle_irq+0x19/0x22)
 [<c0147111>] (generic_handle_irq) from [<c014752d>] (__handle_domain_irq+0x55/0x64)
 [<c014752d>] (__handle_domain_irq) from [<c0346f13>] (gic_handle_irq+0x53/0x64)
 [<c0346f13>] (gic_handle_irq) from [<c0100ba5>] (__irq_svc+0x65/0xc0)
 Exception stack(0xc0e01f18 to 0xc0e01f60)
 1f00:                                                       0000300c 00000000
 1f20: 0000300c c010ff01 00000000 00000000 c0e00000 c0e07714 00000001 c0e01f78
 1f40: c0e07758 00000000 ef7cd0ff c0e01f68 c010554b c0105542 40000033 ffffffff
 [<c0100ba5>] (__irq_svc) from [<c0105542>] (arch_cpu_idle+0xc/0x1e)
 [<c0105542>] (arch_cpu_idle) from [<c063be95>] (default_idle_call+0x21/0x3c)
 [<c063be95>] (default_idle_call) from [<c01324f7>] (do_idle+0xe3/0x1e4)
 [<c01324f7>] (do_idle) from [<c01327b3>] (cpu_startup_entry+0x13/0x14)
 [<c01327b3>] (cpu_startup_entry) from [<c0a00c13>] (start_kernel+0x397/0x3d4)
 [<c0a00c13>] (start_kernel) from [<00000000>] (0x0)
 ---[ end trace 0000000000000002 ]---

Power consumption measured on STM32MP157C DHCOM SoM is not increased or
is below noise threshold.

Fixes: 47beed513a85b ("pinctrl: stm32: Add level interrupt support to gpio irq chip")
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Fabien Dessenne <fabien.dessenne@foss.st.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
To: linux-gpio@vger.kernel.org
---
 drivers/pinctrl/stm32/pinctrl-stm32.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 242d1c37c6e4..7aecd0efde07 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -226,6 +226,13 @@ static void stm32_gpio_free(struct gpio_chip *chip, unsigned offset)
 	pinctrl_gpio_free(chip->base + offset);
 }
 
+static int stm32_gpio_get_noclk(struct gpio_chip *chip, unsigned int offset)
+{
+	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
+
+	return !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
+}
+
 static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
 	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
@@ -233,7 +240,7 @@ static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
 
 	clk_enable(bank->clk);
 
-	ret = !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
+	ret = stm32_gpio_get_noclk(chip, offset);
 
 	clk_disable(bank->clk);
 
@@ -317,7 +324,7 @@ static void stm32_gpio_irq_trigger(struct irq_data *d)
 		return;
 
 	/* If level interrupt type then retrig */
-	level = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
+	level = stm32_gpio_get_noclk(&bank->gpio_chip, d->hwirq);
 	if ((level == 0 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_LOW) ||
 	    (level == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
 		irq_chip_retrigger_hierarchy(d);
@@ -359,6 +366,7 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
 {
 	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
 	struct stm32_pinctrl *pctl = dev_get_drvdata(bank->gpio_chip.parent);
+	unsigned long flags;
 	int ret;
 
 	ret = stm32_gpio_direction_input(&bank->gpio_chip, irq_data->hwirq);
@@ -372,6 +380,10 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
 		return ret;
 	}
 
+	flags = irqd_get_trigger_type(irq_data);
+	if (flags & IRQ_TYPE_LEVEL_MASK)
+		clk_enable(bank->clk);
+
 	return 0;
 }
 
@@ -379,6 +391,9 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
 {
 	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
 
+	if (bank->irq_type[irq_data->hwirq] & IRQ_TYPE_LEVEL_MASK)
+		clk_disable(bank->clk);
+
 	gpiochip_unlock_as_irq(&bank->gpio_chip, irq_data->hwirq);
 }
 
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
  2022-04-21 14:08 ` Marek Vasut
@ 2022-04-22  9:20   ` Fabien DESSENNE
  -1 siblings, 0 replies; 14+ messages in thread
From: Fabien DESSENNE @ 2022-04-22  9:20 UTC (permalink / raw)
  To: Marek Vasut, linux-gpio
  Cc: Alexandre Torgue, Linus Walleij, Marc Zyngier, linux-stm32,
	linux-arm-kernel

Hi Marek,

I agree there is something wrong with the clock management in IRQ 
context here and your patch goes in the right way.
There are also some other problems regarding performance (enabling / 
disabling clock each time we want to change the IO value, ...).
For these both issues I have a patch, which basically keeps the GPIO 
clocks enabled from probe.
I did not have time to submit it, but, considering your concerns, I will 
do it in the coming days.
For the time being I suggest that we do not apply your patch.

BR
Fabien

On 21/04/2022 16:08, Marek Vasut wrote:
> The current EOI handler for LEVEL triggered interrupts calls clk_enable(),
> register IO, clk_disable(). The clock manipulation requires locking which
> happens with IRQs disabled in clk_enable_lock(). Instead of turning the
> clock on and off all the time, enable the clock in case LEVEL interrupt is
> requested and keep the clock enabled until all LEVEL interrupts are freed.
> The LEVEL interrupts are an exception on this platform and seldom used, so
> this does not affect the common case.
> 
> This simplifies the LEVEL interrupt handling considerably and also fixes
> the following splat found when using preempt-rt:
>   ------------[ cut here ]------------
>   WARNING: CPU: 0 PID: 0 at kernel/locking/rtmutex.c:2040 __rt_mutex_trylock+0x37/0x62
>   Modules linked in:
>   CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.109-rt65-stable-standard-00068-g6a5afc4b1217 #85
>   Hardware name: STM32 (Device Tree Support)
>   [<c010a45d>] (unwind_backtrace) from [<c010766f>] (show_stack+0xb/0xc)
>   [<c010766f>] (show_stack) from [<c06353ab>] (dump_stack+0x6f/0x84)
>   [<c06353ab>] (dump_stack) from [<c01145e3>] (__warn+0x7f/0xa4)
>   [<c01145e3>] (__warn) from [<c063386f>] (warn_slowpath_fmt+0x3b/0x74)
>   [<c063386f>] (warn_slowpath_fmt) from [<c063b43d>] (__rt_mutex_trylock+0x37/0x62)
>   [<c063b43d>] (__rt_mutex_trylock) from [<c063c053>] (rt_spin_trylock+0x7/0x16)
>   [<c063c053>] (rt_spin_trylock) from [<c036a2f3>] (clk_enable_lock+0xb/0x80)
>   [<c036a2f3>] (clk_enable_lock) from [<c036ba69>] (clk_core_enable_lock+0x9/0x18)
>   [<c036ba69>] (clk_core_enable_lock) from [<c034e9f3>] (stm32_gpio_get+0x11/0x24)
>   [<c034e9f3>] (stm32_gpio_get) from [<c034ef43>] (stm32_gpio_irq_trigger+0x1f/0x48)
>   [<c034ef43>] (stm32_gpio_irq_trigger) from [<c014aa53>] (handle_fasteoi_irq+0x71/0xa8)
>   [<c014aa53>] (handle_fasteoi_irq) from [<c0147111>] (generic_handle_irq+0x19/0x22)
>   [<c0147111>] (generic_handle_irq) from [<c014752d>] (__handle_domain_irq+0x55/0x64)
>   [<c014752d>] (__handle_domain_irq) from [<c0346f13>] (gic_handle_irq+0x53/0x64)
>   [<c0346f13>] (gic_handle_irq) from [<c0100ba5>] (__irq_svc+0x65/0xc0)
>   Exception stack(0xc0e01f18 to 0xc0e01f60)
>   1f00:                                                       0000300c 00000000
>   1f20: 0000300c c010ff01 00000000 00000000 c0e00000 c0e07714 00000001 c0e01f78
>   1f40: c0e07758 00000000 ef7cd0ff c0e01f68 c010554b c0105542 40000033 ffffffff
>   [<c0100ba5>] (__irq_svc) from [<c0105542>] (arch_cpu_idle+0xc/0x1e)
>   [<c0105542>] (arch_cpu_idle) from [<c063be95>] (default_idle_call+0x21/0x3c)
>   [<c063be95>] (default_idle_call) from [<c01324f7>] (do_idle+0xe3/0x1e4)
>   [<c01324f7>] (do_idle) from [<c01327b3>] (cpu_startup_entry+0x13/0x14)
>   [<c01327b3>] (cpu_startup_entry) from [<c0a00c13>] (start_kernel+0x397/0x3d4)
>   [<c0a00c13>] (start_kernel) from [<00000000>] (0x0)
>   ---[ end trace 0000000000000002 ]---
> 
> Power consumption measured on STM32MP157C DHCOM SoM is not increased or
> is below noise threshold.
> 
> Fixes: 47beed513a85b ("pinctrl: stm32: Add level interrupt support to gpio irq chip")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Fabien Dessenne <fabien.dessenne@foss.st.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-arm-kernel@lists.infradead.org
> To: linux-gpio@vger.kernel.org
> ---
>   drivers/pinctrl/stm32/pinctrl-stm32.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 242d1c37c6e4..7aecd0efde07 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -226,6 +226,13 @@ static void stm32_gpio_free(struct gpio_chip *chip, unsigned offset)
>   	pinctrl_gpio_free(chip->base + offset);
>   }
>   
> +static int stm32_gpio_get_noclk(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
> +
> +	return !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
> +}
> +
>   static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
>   {
>   	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
> @@ -233,7 +240,7 @@ static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
>   
>   	clk_enable(bank->clk);
>   
> -	ret = !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
> +	ret = stm32_gpio_get_noclk(chip, offset);
>   
>   	clk_disable(bank->clk);
>   
> @@ -317,7 +324,7 @@ static void stm32_gpio_irq_trigger(struct irq_data *d)
>   		return;
>   
>   	/* If level interrupt type then retrig */
> -	level = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
> +	level = stm32_gpio_get_noclk(&bank->gpio_chip, d->hwirq);
>   	if ((level == 0 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_LOW) ||
>   	    (level == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
>   		irq_chip_retrigger_hierarchy(d);
> @@ -359,6 +366,7 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>   {
>   	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>   	struct stm32_pinctrl *pctl = dev_get_drvdata(bank->gpio_chip.parent);
> +	unsigned long flags;
>   	int ret;
>   
>   	ret = stm32_gpio_direction_input(&bank->gpio_chip, irq_data->hwirq);
> @@ -372,6 +380,10 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>   		return ret;
>   	}
>   
> +	flags = irqd_get_trigger_type(irq_data);
> +	if (flags & IRQ_TYPE_LEVEL_MASK)
> +		clk_enable(bank->clk);
> +
>   	return 0;
>   }
>   
> @@ -379,6 +391,9 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>   {
>   	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>   
> +	if (bank->irq_type[irq_data->hwirq] & IRQ_TYPE_LEVEL_MASK)
> +		clk_disable(bank->clk);
> +
>   	gpiochip_unlock_as_irq(&bank->gpio_chip, irq_data->hwirq);
>   }
>   

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
@ 2022-04-22  9:20   ` Fabien DESSENNE
  0 siblings, 0 replies; 14+ messages in thread
From: Fabien DESSENNE @ 2022-04-22  9:20 UTC (permalink / raw)
  To: Marek Vasut, linux-gpio
  Cc: Alexandre Torgue, Linus Walleij, Marc Zyngier, linux-stm32,
	linux-arm-kernel

Hi Marek,

I agree there is something wrong with the clock management in IRQ 
context here and your patch goes in the right way.
There are also some other problems regarding performance (enabling / 
disabling clock each time we want to change the IO value, ...).
For these both issues I have a patch, which basically keeps the GPIO 
clocks enabled from probe.
I did not have time to submit it, but, considering your concerns, I will 
do it in the coming days.
For the time being I suggest that we do not apply your patch.

BR
Fabien

On 21/04/2022 16:08, Marek Vasut wrote:
> The current EOI handler for LEVEL triggered interrupts calls clk_enable(),
> register IO, clk_disable(). The clock manipulation requires locking which
> happens with IRQs disabled in clk_enable_lock(). Instead of turning the
> clock on and off all the time, enable the clock in case LEVEL interrupt is
> requested and keep the clock enabled until all LEVEL interrupts are freed.
> The LEVEL interrupts are an exception on this platform and seldom used, so
> this does not affect the common case.
> 
> This simplifies the LEVEL interrupt handling considerably and also fixes
> the following splat found when using preempt-rt:
>   ------------[ cut here ]------------
>   WARNING: CPU: 0 PID: 0 at kernel/locking/rtmutex.c:2040 __rt_mutex_trylock+0x37/0x62
>   Modules linked in:
>   CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.109-rt65-stable-standard-00068-g6a5afc4b1217 #85
>   Hardware name: STM32 (Device Tree Support)
>   [<c010a45d>] (unwind_backtrace) from [<c010766f>] (show_stack+0xb/0xc)
>   [<c010766f>] (show_stack) from [<c06353ab>] (dump_stack+0x6f/0x84)
>   [<c06353ab>] (dump_stack) from [<c01145e3>] (__warn+0x7f/0xa4)
>   [<c01145e3>] (__warn) from [<c063386f>] (warn_slowpath_fmt+0x3b/0x74)
>   [<c063386f>] (warn_slowpath_fmt) from [<c063b43d>] (__rt_mutex_trylock+0x37/0x62)
>   [<c063b43d>] (__rt_mutex_trylock) from [<c063c053>] (rt_spin_trylock+0x7/0x16)
>   [<c063c053>] (rt_spin_trylock) from [<c036a2f3>] (clk_enable_lock+0xb/0x80)
>   [<c036a2f3>] (clk_enable_lock) from [<c036ba69>] (clk_core_enable_lock+0x9/0x18)
>   [<c036ba69>] (clk_core_enable_lock) from [<c034e9f3>] (stm32_gpio_get+0x11/0x24)
>   [<c034e9f3>] (stm32_gpio_get) from [<c034ef43>] (stm32_gpio_irq_trigger+0x1f/0x48)
>   [<c034ef43>] (stm32_gpio_irq_trigger) from [<c014aa53>] (handle_fasteoi_irq+0x71/0xa8)
>   [<c014aa53>] (handle_fasteoi_irq) from [<c0147111>] (generic_handle_irq+0x19/0x22)
>   [<c0147111>] (generic_handle_irq) from [<c014752d>] (__handle_domain_irq+0x55/0x64)
>   [<c014752d>] (__handle_domain_irq) from [<c0346f13>] (gic_handle_irq+0x53/0x64)
>   [<c0346f13>] (gic_handle_irq) from [<c0100ba5>] (__irq_svc+0x65/0xc0)
>   Exception stack(0xc0e01f18 to 0xc0e01f60)
>   1f00:                                                       0000300c 00000000
>   1f20: 0000300c c010ff01 00000000 00000000 c0e00000 c0e07714 00000001 c0e01f78
>   1f40: c0e07758 00000000 ef7cd0ff c0e01f68 c010554b c0105542 40000033 ffffffff
>   [<c0100ba5>] (__irq_svc) from [<c0105542>] (arch_cpu_idle+0xc/0x1e)
>   [<c0105542>] (arch_cpu_idle) from [<c063be95>] (default_idle_call+0x21/0x3c)
>   [<c063be95>] (default_idle_call) from [<c01324f7>] (do_idle+0xe3/0x1e4)
>   [<c01324f7>] (do_idle) from [<c01327b3>] (cpu_startup_entry+0x13/0x14)
>   [<c01327b3>] (cpu_startup_entry) from [<c0a00c13>] (start_kernel+0x397/0x3d4)
>   [<c0a00c13>] (start_kernel) from [<00000000>] (0x0)
>   ---[ end trace 0000000000000002 ]---
> 
> Power consumption measured on STM32MP157C DHCOM SoM is not increased or
> is below noise threshold.
> 
> Fixes: 47beed513a85b ("pinctrl: stm32: Add level interrupt support to gpio irq chip")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Fabien Dessenne <fabien.dessenne@foss.st.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-arm-kernel@lists.infradead.org
> To: linux-gpio@vger.kernel.org
> ---
>   drivers/pinctrl/stm32/pinctrl-stm32.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 242d1c37c6e4..7aecd0efde07 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -226,6 +226,13 @@ static void stm32_gpio_free(struct gpio_chip *chip, unsigned offset)
>   	pinctrl_gpio_free(chip->base + offset);
>   }
>   
> +static int stm32_gpio_get_noclk(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
> +
> +	return !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
> +}
> +
>   static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
>   {
>   	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
> @@ -233,7 +240,7 @@ static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
>   
>   	clk_enable(bank->clk);
>   
> -	ret = !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
> +	ret = stm32_gpio_get_noclk(chip, offset);
>   
>   	clk_disable(bank->clk);
>   
> @@ -317,7 +324,7 @@ static void stm32_gpio_irq_trigger(struct irq_data *d)
>   		return;
>   
>   	/* If level interrupt type then retrig */
> -	level = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
> +	level = stm32_gpio_get_noclk(&bank->gpio_chip, d->hwirq);
>   	if ((level == 0 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_LOW) ||
>   	    (level == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
>   		irq_chip_retrigger_hierarchy(d);
> @@ -359,6 +366,7 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>   {
>   	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>   	struct stm32_pinctrl *pctl = dev_get_drvdata(bank->gpio_chip.parent);
> +	unsigned long flags;
>   	int ret;
>   
>   	ret = stm32_gpio_direction_input(&bank->gpio_chip, irq_data->hwirq);
> @@ -372,6 +380,10 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>   		return ret;
>   	}
>   
> +	flags = irqd_get_trigger_type(irq_data);
> +	if (flags & IRQ_TYPE_LEVEL_MASK)
> +		clk_enable(bank->clk);
> +
>   	return 0;
>   }
>   
> @@ -379,6 +391,9 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>   {
>   	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>   
> +	if (bank->irq_type[irq_data->hwirq] & IRQ_TYPE_LEVEL_MASK)
> +		clk_disable(bank->clk);
> +
>   	gpiochip_unlock_as_irq(&bank->gpio_chip, irq_data->hwirq);
>   }
>   

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
  2022-04-22  9:20   ` Fabien DESSENNE
@ 2022-04-22  9:32     ` Marek Vasut
  -1 siblings, 0 replies; 14+ messages in thread
From: Marek Vasut @ 2022-04-22  9:32 UTC (permalink / raw)
  To: Fabien DESSENNE, linux-gpio
  Cc: Alexandre Torgue, Linus Walleij, Marc Zyngier, linux-stm32,
	linux-arm-kernel

On 4/22/22 11:20, Fabien DESSENNE wrote:
> Hi Marek,

Hi,

> I agree there is something wrong with the clock management in IRQ 
> context here and your patch goes in the right way.
> There are also some other problems regarding performance (enabling / 
> disabling clock each time we want to change the IO value, ...).

These are already fixed by separate patch:
[PATCH] irqchip/stm32: Do not call stm32_gpio_get() for edge triggered 
IRQs in EOI

> For these both issues I have a patch, which basically keeps the GPIO 
> clocks enabled from probe.
> I did not have time to submit it, but, considering your concerns, I will 
> do it in the coming days.
> For the time being I suggest that we do not apply your patch.

Is there anything specifically wrong with this patch ?

I think you should be easily able to apply your patch which keeps the 
clock enabled all the time on top of this one. The added bonus is that 
the commit message of this patch - which explains what the problem is 
with the LEVEL IRQs - would be retained, and so would bisectability in 
case your patch causes some sort of problem.

> BR
> Fabien
> 
> On 21/04/2022 16:08, Marek Vasut wrote:
>> The current EOI handler for LEVEL triggered interrupts calls 
>> clk_enable(),
>> register IO, clk_disable(). The clock manipulation requires locking which
>> happens with IRQs disabled in clk_enable_lock(). Instead of turning the
>> clock on and off all the time, enable the clock in case LEVEL 
>> interrupt is
>> requested and keep the clock enabled until all LEVEL interrupts are 
>> freed.
>> The LEVEL interrupts are an exception on this platform and seldom 
>> used, so
>> this does not affect the common case.
>>
>> This simplifies the LEVEL interrupt handling considerably and also fixes
>> the following splat found when using preempt-rt:
>>   ------------[ cut here ]------------
>>   WARNING: CPU: 0 PID: 0 at kernel/locking/rtmutex.c:2040 
>> __rt_mutex_trylock+0x37/0x62
>>   Modules linked in:
>>   CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
>> 5.10.109-rt65-stable-standard-00068-g6a5afc4b1217 #85
>>   Hardware name: STM32 (Device Tree Support)
>>   [<c010a45d>] (unwind_backtrace) from [<c010766f>] (show_stack+0xb/0xc)
>>   [<c010766f>] (show_stack) from [<c06353ab>] (dump_stack+0x6f/0x84)
>>   [<c06353ab>] (dump_stack) from [<c01145e3>] (__warn+0x7f/0xa4)
>>   [<c01145e3>] (__warn) from [<c063386f>] (warn_slowpath_fmt+0x3b/0x74)
>>   [<c063386f>] (warn_slowpath_fmt) from [<c063b43d>] 
>> (__rt_mutex_trylock+0x37/0x62)
>>   [<c063b43d>] (__rt_mutex_trylock) from [<c063c053>] 
>> (rt_spin_trylock+0x7/0x16)
>>   [<c063c053>] (rt_spin_trylock) from [<c036a2f3>] 
>> (clk_enable_lock+0xb/0x80)
>>   [<c036a2f3>] (clk_enable_lock) from [<c036ba69>] 
>> (clk_core_enable_lock+0x9/0x18)
>>   [<c036ba69>] (clk_core_enable_lock) from [<c034e9f3>] 
>> (stm32_gpio_get+0x11/0x24)
>>   [<c034e9f3>] (stm32_gpio_get) from [<c034ef43>] 
>> (stm32_gpio_irq_trigger+0x1f/0x48)
>>   [<c034ef43>] (stm32_gpio_irq_trigger) from [<c014aa53>] 
>> (handle_fasteoi_irq+0x71/0xa8)
>>   [<c014aa53>] (handle_fasteoi_irq) from [<c0147111>] 
>> (generic_handle_irq+0x19/0x22)
>>   [<c0147111>] (generic_handle_irq) from [<c014752d>] 
>> (__handle_domain_irq+0x55/0x64)
>>   [<c014752d>] (__handle_domain_irq) from [<c0346f13>] 
>> (gic_handle_irq+0x53/0x64)
>>   [<c0346f13>] (gic_handle_irq) from [<c0100ba5>] (__irq_svc+0x65/0xc0)
>>   Exception stack(0xc0e01f18 to 0xc0e01f60)
>>   1f00:                                                       0000300c 
>> 00000000
>>   1f20: 0000300c c010ff01 00000000 00000000 c0e00000 c0e07714 00000001 
>> c0e01f78
>>   1f40: c0e07758 00000000 ef7cd0ff c0e01f68 c010554b c0105542 40000033 
>> ffffffff
>>   [<c0100ba5>] (__irq_svc) from [<c0105542>] (arch_cpu_idle+0xc/0x1e)
>>   [<c0105542>] (arch_cpu_idle) from [<c063be95>] 
>> (default_idle_call+0x21/0x3c)
>>   [<c063be95>] (default_idle_call) from [<c01324f7>] (do_idle+0xe3/0x1e4)
>>   [<c01324f7>] (do_idle) from [<c01327b3>] (cpu_startup_entry+0x13/0x14)
>>   [<c01327b3>] (cpu_startup_entry) from [<c0a00c13>] 
>> (start_kernel+0x397/0x3d4)
>>   [<c0a00c13>] (start_kernel) from [<00000000>] (0x0)
>>   ---[ end trace 0000000000000002 ]---
>>
>> Power consumption measured on STM32MP157C DHCOM SoM is not increased or
>> is below noise threshold.
>>
>> Fixes: 47beed513a85b ("pinctrl: stm32: Add level interrupt support to 
>> gpio irq chip")
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>> Cc: Fabien Dessenne <fabien.dessenne@foss.st.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: linux-stm32@st-md-mailman.stormreply.com
>> Cc: linux-arm-kernel@lists.infradead.org
>> To: linux-gpio@vger.kernel.org
>> ---
>>   drivers/pinctrl/stm32/pinctrl-stm32.c | 19 +++++++++++++++++--
>>   1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c 
>> b/drivers/pinctrl/stm32/pinctrl-stm32.c
>> index 242d1c37c6e4..7aecd0efde07 100644
>> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
>> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
>> @@ -226,6 +226,13 @@ static void stm32_gpio_free(struct gpio_chip 
>> *chip, unsigned offset)
>>       pinctrl_gpio_free(chip->base + offset);
>>   }
>> +static int stm32_gpio_get_noclk(struct gpio_chip *chip, unsigned int 
>> offset)
>> +{
>> +    struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
>> +
>> +    return !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
>> +}
>> +
>>   static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
>>   {
>>       struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
>> @@ -233,7 +240,7 @@ static int stm32_gpio_get(struct gpio_chip *chip, 
>> unsigned offset)
>>       clk_enable(bank->clk);
>> -    ret = !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
>> +    ret = stm32_gpio_get_noclk(chip, offset);
>>       clk_disable(bank->clk);
>> @@ -317,7 +324,7 @@ static void stm32_gpio_irq_trigger(struct irq_data 
>> *d)
>>           return;
>>       /* If level interrupt type then retrig */
>> -    level = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
>> +    level = stm32_gpio_get_noclk(&bank->gpio_chip, d->hwirq);
>>       if ((level == 0 && bank->irq_type[d->hwirq] == 
>> IRQ_TYPE_LEVEL_LOW) ||
>>           (level == 1 && bank->irq_type[d->hwirq] == 
>> IRQ_TYPE_LEVEL_HIGH))
>>           irq_chip_retrigger_hierarchy(d);
>> @@ -359,6 +366,7 @@ static int stm32_gpio_irq_request_resources(struct 
>> irq_data *irq_data)
>>   {
>>       struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>>       struct stm32_pinctrl *pctl = 
>> dev_get_drvdata(bank->gpio_chip.parent);
>> +    unsigned long flags;
>>       int ret;
>>       ret = stm32_gpio_direction_input(&bank->gpio_chip, 
>> irq_data->hwirq);
>> @@ -372,6 +380,10 @@ static int 
>> stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>>           return ret;
>>       }
>> +    flags = irqd_get_trigger_type(irq_data);
>> +    if (flags & IRQ_TYPE_LEVEL_MASK)
>> +        clk_enable(bank->clk);
>> +
>>       return 0;
>>   }
>> @@ -379,6 +391,9 @@ static void 
>> stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>>   {
>>       struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>> +    if (bank->irq_type[irq_data->hwirq] & IRQ_TYPE_LEVEL_MASK)
>> +        clk_disable(bank->clk);
>> +
>>       gpiochip_unlock_as_irq(&bank->gpio_chip, irq_data->hwirq);
>>   }

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
@ 2022-04-22  9:32     ` Marek Vasut
  0 siblings, 0 replies; 14+ messages in thread
From: Marek Vasut @ 2022-04-22  9:32 UTC (permalink / raw)
  To: Fabien DESSENNE, linux-gpio
  Cc: Alexandre Torgue, Linus Walleij, Marc Zyngier, linux-stm32,
	linux-arm-kernel

On 4/22/22 11:20, Fabien DESSENNE wrote:
> Hi Marek,

Hi,

> I agree there is something wrong with the clock management in IRQ 
> context here and your patch goes in the right way.
> There are also some other problems regarding performance (enabling / 
> disabling clock each time we want to change the IO value, ...).

These are already fixed by separate patch:
[PATCH] irqchip/stm32: Do not call stm32_gpio_get() for edge triggered 
IRQs in EOI

> For these both issues I have a patch, which basically keeps the GPIO 
> clocks enabled from probe.
> I did not have time to submit it, but, considering your concerns, I will 
> do it in the coming days.
> For the time being I suggest that we do not apply your patch.

Is there anything specifically wrong with this patch ?

I think you should be easily able to apply your patch which keeps the 
clock enabled all the time on top of this one. The added bonus is that 
the commit message of this patch - which explains what the problem is 
with the LEVEL IRQs - would be retained, and so would bisectability in 
case your patch causes some sort of problem.

> BR
> Fabien
> 
> On 21/04/2022 16:08, Marek Vasut wrote:
>> The current EOI handler for LEVEL triggered interrupts calls 
>> clk_enable(),
>> register IO, clk_disable(). The clock manipulation requires locking which
>> happens with IRQs disabled in clk_enable_lock(). Instead of turning the
>> clock on and off all the time, enable the clock in case LEVEL 
>> interrupt is
>> requested and keep the clock enabled until all LEVEL interrupts are 
>> freed.
>> The LEVEL interrupts are an exception on this platform and seldom 
>> used, so
>> this does not affect the common case.
>>
>> This simplifies the LEVEL interrupt handling considerably and also fixes
>> the following splat found when using preempt-rt:
>>   ------------[ cut here ]------------
>>   WARNING: CPU: 0 PID: 0 at kernel/locking/rtmutex.c:2040 
>> __rt_mutex_trylock+0x37/0x62
>>   Modules linked in:
>>   CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
>> 5.10.109-rt65-stable-standard-00068-g6a5afc4b1217 #85
>>   Hardware name: STM32 (Device Tree Support)
>>   [<c010a45d>] (unwind_backtrace) from [<c010766f>] (show_stack+0xb/0xc)
>>   [<c010766f>] (show_stack) from [<c06353ab>] (dump_stack+0x6f/0x84)
>>   [<c06353ab>] (dump_stack) from [<c01145e3>] (__warn+0x7f/0xa4)
>>   [<c01145e3>] (__warn) from [<c063386f>] (warn_slowpath_fmt+0x3b/0x74)
>>   [<c063386f>] (warn_slowpath_fmt) from [<c063b43d>] 
>> (__rt_mutex_trylock+0x37/0x62)
>>   [<c063b43d>] (__rt_mutex_trylock) from [<c063c053>] 
>> (rt_spin_trylock+0x7/0x16)
>>   [<c063c053>] (rt_spin_trylock) from [<c036a2f3>] 
>> (clk_enable_lock+0xb/0x80)
>>   [<c036a2f3>] (clk_enable_lock) from [<c036ba69>] 
>> (clk_core_enable_lock+0x9/0x18)
>>   [<c036ba69>] (clk_core_enable_lock) from [<c034e9f3>] 
>> (stm32_gpio_get+0x11/0x24)
>>   [<c034e9f3>] (stm32_gpio_get) from [<c034ef43>] 
>> (stm32_gpio_irq_trigger+0x1f/0x48)
>>   [<c034ef43>] (stm32_gpio_irq_trigger) from [<c014aa53>] 
>> (handle_fasteoi_irq+0x71/0xa8)
>>   [<c014aa53>] (handle_fasteoi_irq) from [<c0147111>] 
>> (generic_handle_irq+0x19/0x22)
>>   [<c0147111>] (generic_handle_irq) from [<c014752d>] 
>> (__handle_domain_irq+0x55/0x64)
>>   [<c014752d>] (__handle_domain_irq) from [<c0346f13>] 
>> (gic_handle_irq+0x53/0x64)
>>   [<c0346f13>] (gic_handle_irq) from [<c0100ba5>] (__irq_svc+0x65/0xc0)
>>   Exception stack(0xc0e01f18 to 0xc0e01f60)
>>   1f00:                                                       0000300c 
>> 00000000
>>   1f20: 0000300c c010ff01 00000000 00000000 c0e00000 c0e07714 00000001 
>> c0e01f78
>>   1f40: c0e07758 00000000 ef7cd0ff c0e01f68 c010554b c0105542 40000033 
>> ffffffff
>>   [<c0100ba5>] (__irq_svc) from [<c0105542>] (arch_cpu_idle+0xc/0x1e)
>>   [<c0105542>] (arch_cpu_idle) from [<c063be95>] 
>> (default_idle_call+0x21/0x3c)
>>   [<c063be95>] (default_idle_call) from [<c01324f7>] (do_idle+0xe3/0x1e4)
>>   [<c01324f7>] (do_idle) from [<c01327b3>] (cpu_startup_entry+0x13/0x14)
>>   [<c01327b3>] (cpu_startup_entry) from [<c0a00c13>] 
>> (start_kernel+0x397/0x3d4)
>>   [<c0a00c13>] (start_kernel) from [<00000000>] (0x0)
>>   ---[ end trace 0000000000000002 ]---
>>
>> Power consumption measured on STM32MP157C DHCOM SoM is not increased or
>> is below noise threshold.
>>
>> Fixes: 47beed513a85b ("pinctrl: stm32: Add level interrupt support to 
>> gpio irq chip")
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>> Cc: Fabien Dessenne <fabien.dessenne@foss.st.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: linux-stm32@st-md-mailman.stormreply.com
>> Cc: linux-arm-kernel@lists.infradead.org
>> To: linux-gpio@vger.kernel.org
>> ---
>>   drivers/pinctrl/stm32/pinctrl-stm32.c | 19 +++++++++++++++++--
>>   1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c 
>> b/drivers/pinctrl/stm32/pinctrl-stm32.c
>> index 242d1c37c6e4..7aecd0efde07 100644
>> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
>> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
>> @@ -226,6 +226,13 @@ static void stm32_gpio_free(struct gpio_chip 
>> *chip, unsigned offset)
>>       pinctrl_gpio_free(chip->base + offset);
>>   }
>> +static int stm32_gpio_get_noclk(struct gpio_chip *chip, unsigned int 
>> offset)
>> +{
>> +    struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
>> +
>> +    return !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
>> +}
>> +
>>   static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
>>   {
>>       struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
>> @@ -233,7 +240,7 @@ static int stm32_gpio_get(struct gpio_chip *chip, 
>> unsigned offset)
>>       clk_enable(bank->clk);
>> -    ret = !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
>> +    ret = stm32_gpio_get_noclk(chip, offset);
>>       clk_disable(bank->clk);
>> @@ -317,7 +324,7 @@ static void stm32_gpio_irq_trigger(struct irq_data 
>> *d)
>>           return;
>>       /* If level interrupt type then retrig */
>> -    level = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
>> +    level = stm32_gpio_get_noclk(&bank->gpio_chip, d->hwirq);
>>       if ((level == 0 && bank->irq_type[d->hwirq] == 
>> IRQ_TYPE_LEVEL_LOW) ||
>>           (level == 1 && bank->irq_type[d->hwirq] == 
>> IRQ_TYPE_LEVEL_HIGH))
>>           irq_chip_retrigger_hierarchy(d);
>> @@ -359,6 +366,7 @@ static int stm32_gpio_irq_request_resources(struct 
>> irq_data *irq_data)
>>   {
>>       struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>>       struct stm32_pinctrl *pctl = 
>> dev_get_drvdata(bank->gpio_chip.parent);
>> +    unsigned long flags;
>>       int ret;
>>       ret = stm32_gpio_direction_input(&bank->gpio_chip, 
>> irq_data->hwirq);
>> @@ -372,6 +380,10 @@ static int 
>> stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>>           return ret;
>>       }
>> +    flags = irqd_get_trigger_type(irq_data);
>> +    if (flags & IRQ_TYPE_LEVEL_MASK)
>> +        clk_enable(bank->clk);
>> +
>>       return 0;
>>   }
>> @@ -379,6 +391,9 @@ static void 
>> stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>>   {
>>       struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>> +    if (bank->irq_type[irq_data->hwirq] & IRQ_TYPE_LEVEL_MASK)
>> +        clk_disable(bank->clk);
>> +
>>       gpiochip_unlock_as_irq(&bank->gpio_chip, irq_data->hwirq);
>>   }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
  2022-04-22  9:20   ` Fabien DESSENNE
@ 2022-04-22 10:08     ` Marc Zyngier
  -1 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2022-04-22 10:08 UTC (permalink / raw)
  To: Fabien DESSENNE
  Cc: Marek Vasut, linux-gpio, Alexandre Torgue, Linus Walleij,
	linux-stm32, linux-arm-kernel

On Fri, 22 Apr 2022 10:20:36 +0100,
Fabien DESSENNE <fabien.dessenne@foss.st.com> wrote:
> 
> Hi Marek,
> 
> I agree there is something wrong with the clock management in IRQ
> context here and your patch goes in the right way.
> There are also some other problems regarding performance (enabling /
> disabling clock each time we want to change the IO value, ...).
> For these both issues I have a patch, which basically keeps the GPIO
> clocks enabled from probe.
> I did not have time to submit it, but, considering your concerns, I
> will do it in the coming days.
> For the time being I suggest that we do not apply your patch.

Why? This fixes a glaring issue, and there are no alternatives at the
moment. So if there is something to improve on, please base your patch
on top of Marek's.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
@ 2022-04-22 10:08     ` Marc Zyngier
  0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2022-04-22 10:08 UTC (permalink / raw)
  To: Fabien DESSENNE
  Cc: Marek Vasut, linux-gpio, Alexandre Torgue, Linus Walleij,
	linux-stm32, linux-arm-kernel

On Fri, 22 Apr 2022 10:20:36 +0100,
Fabien DESSENNE <fabien.dessenne@foss.st.com> wrote:
> 
> Hi Marek,
> 
> I agree there is something wrong with the clock management in IRQ
> context here and your patch goes in the right way.
> There are also some other problems regarding performance (enabling /
> disabling clock each time we want to change the IO value, ...).
> For these both issues I have a patch, which basically keeps the GPIO
> clocks enabled from probe.
> I did not have time to submit it, but, considering your concerns, I
> will do it in the coming days.
> For the time being I suggest that we do not apply your patch.

Why? This fixes a glaring issue, and there are no alternatives at the
moment. So if there is something to improve on, please base your patch
on top of Marek's.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
  2022-04-22 10:08     ` Marc Zyngier
@ 2022-04-22 10:25       ` Fabien DESSENNE
  -1 siblings, 0 replies; 14+ messages in thread
From: Fabien DESSENNE @ 2022-04-22 10:25 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Marek Vasut, linux-gpio, Alexandre Torgue, Linus Walleij,
	linux-stm32, linux-arm-kernel

Hi Mark and Marek,

My intention was to have a single commit, instead of two commits with 
the second one making a part of the previous one obsolete.

Nevertheless I understand your points which are valid, so I will review 
Marek's patch first before submitting another patch.

BR
Fabien


On 22/04/2022 12:08, Marc Zyngier wrote:
> On Fri, 22 Apr 2022 10:20:36 +0100,
> Fabien DESSENNE <fabien.dessenne@foss.st.com> wrote:
>>
>> Hi Marek,
>>
>> I agree there is something wrong with the clock management in IRQ
>> context here and your patch goes in the right way.
>> There are also some other problems regarding performance (enabling /
>> disabling clock each time we want to change the IO value, ...).
>> For these both issues I have a patch, which basically keeps the GPIO
>> clocks enabled from probe.
>> I did not have time to submit it, but, considering your concerns, I
>> will do it in the coming days.
>> For the time being I suggest that we do not apply your patch.
> 
> Why? This fixes a glaring issue, and there are no alternatives at the
> moment. So if there is something to improve on, please base your patch
> on top of Marek's.
> 
> Thanks,
> 
> 	M.
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
@ 2022-04-22 10:25       ` Fabien DESSENNE
  0 siblings, 0 replies; 14+ messages in thread
From: Fabien DESSENNE @ 2022-04-22 10:25 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Marek Vasut, linux-gpio, Alexandre Torgue, Linus Walleij,
	linux-stm32, linux-arm-kernel

Hi Mark and Marek,

My intention was to have a single commit, instead of two commits with 
the second one making a part of the previous one obsolete.

Nevertheless I understand your points which are valid, so I will review 
Marek's patch first before submitting another patch.

BR
Fabien


On 22/04/2022 12:08, Marc Zyngier wrote:
> On Fri, 22 Apr 2022 10:20:36 +0100,
> Fabien DESSENNE <fabien.dessenne@foss.st.com> wrote:
>>
>> Hi Marek,
>>
>> I agree there is something wrong with the clock management in IRQ
>> context here and your patch goes in the right way.
>> There are also some other problems regarding performance (enabling /
>> disabling clock each time we want to change the IO value, ...).
>> For these both issues I have a patch, which basically keeps the GPIO
>> clocks enabled from probe.
>> I did not have time to submit it, but, considering your concerns, I
>> will do it in the coming days.
>> For the time being I suggest that we do not apply your patch.
> 
> Why? This fixes a glaring issue, and there are no alternatives at the
> moment. So if there is something to improve on, please base your patch
> on top of Marek's.
> 
> Thanks,
> 
> 	M.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
  2022-04-21 14:08 ` Marek Vasut
@ 2022-04-22 14:28   ` Fabien DESSENNE
  -1 siblings, 0 replies; 14+ messages in thread
From: Fabien DESSENNE @ 2022-04-22 14:28 UTC (permalink / raw)
  To: Marek Vasut, linux-gpio
  Cc: Alexandre Torgue, Linus Walleij, Marc Zyngier, linux-stm32,
	linux-arm-kernel

Hi Marek

Thank you for the patch.

Like Linus did for your other patch, I would prefer the subject prefix 
to be "pinctrl: stm32:"

Apart from that I added my RB below

BR
Fabien



On 21/04/2022 16:08, Marek Vasut wrote:
> The current EOI handler for LEVEL triggered interrupts calls clk_enable(),
> register IO, clk_disable(). The clock manipulation requires locking which
> happens with IRQs disabled in clk_enable_lock(). Instead of turning the
> clock on and off all the time, enable the clock in case LEVEL interrupt is
> requested and keep the clock enabled until all LEVEL interrupts are freed.
> The LEVEL interrupts are an exception on this platform and seldom used, so
> this does not affect the common case.
> 
> This simplifies the LEVEL interrupt handling considerably and also fixes
> the following splat found when using preempt-rt:
>   ------------[ cut here ]------------
>   WARNING: CPU: 0 PID: 0 at kernel/locking/rtmutex.c:2040 __rt_mutex_trylock+0x37/0x62
>   Modules linked in:
>   CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.109-rt65-stable-standard-00068-g6a5afc4b1217 #85
>   Hardware name: STM32 (Device Tree Support)
>   [<c010a45d>] (unwind_backtrace) from [<c010766f>] (show_stack+0xb/0xc)
>   [<c010766f>] (show_stack) from [<c06353ab>] (dump_stack+0x6f/0x84)
>   [<c06353ab>] (dump_stack) from [<c01145e3>] (__warn+0x7f/0xa4)
>   [<c01145e3>] (__warn) from [<c063386f>] (warn_slowpath_fmt+0x3b/0x74)
>   [<c063386f>] (warn_slowpath_fmt) from [<c063b43d>] (__rt_mutex_trylock+0x37/0x62)
>   [<c063b43d>] (__rt_mutex_trylock) from [<c063c053>] (rt_spin_trylock+0x7/0x16)
>   [<c063c053>] (rt_spin_trylock) from [<c036a2f3>] (clk_enable_lock+0xb/0x80)
>   [<c036a2f3>] (clk_enable_lock) from [<c036ba69>] (clk_core_enable_lock+0x9/0x18)
>   [<c036ba69>] (clk_core_enable_lock) from [<c034e9f3>] (stm32_gpio_get+0x11/0x24)
>   [<c034e9f3>] (stm32_gpio_get) from [<c034ef43>] (stm32_gpio_irq_trigger+0x1f/0x48)
>   [<c034ef43>] (stm32_gpio_irq_trigger) from [<c014aa53>] (handle_fasteoi_irq+0x71/0xa8)
>   [<c014aa53>] (handle_fasteoi_irq) from [<c0147111>] (generic_handle_irq+0x19/0x22)
>   [<c0147111>] (generic_handle_irq) from [<c014752d>] (__handle_domain_irq+0x55/0x64)
>   [<c014752d>] (__handle_domain_irq) from [<c0346f13>] (gic_handle_irq+0x53/0x64)
>   [<c0346f13>] (gic_handle_irq) from [<c0100ba5>] (__irq_svc+0x65/0xc0)
>   Exception stack(0xc0e01f18 to 0xc0e01f60)
>   1f00:                                                       0000300c 00000000
>   1f20: 0000300c c010ff01 00000000 00000000 c0e00000 c0e07714 00000001 c0e01f78
>   1f40: c0e07758 00000000 ef7cd0ff c0e01f68 c010554b c0105542 40000033 ffffffff
>   [<c0100ba5>] (__irq_svc) from [<c0105542>] (arch_cpu_idle+0xc/0x1e)
>   [<c0105542>] (arch_cpu_idle) from [<c063be95>] (default_idle_call+0x21/0x3c)
>   [<c063be95>] (default_idle_call) from [<c01324f7>] (do_idle+0xe3/0x1e4)
>   [<c01324f7>] (do_idle) from [<c01327b3>] (cpu_startup_entry+0x13/0x14)
>   [<c01327b3>] (cpu_startup_entry) from [<c0a00c13>] (start_kernel+0x397/0x3d4)
>   [<c0a00c13>] (start_kernel) from [<00000000>] (0x0)
>   ---[ end trace 0000000000000002 ]---
> 
> Power consumption measured on STM32MP157C DHCOM SoM is not increased or
> is below noise threshold.
> 
> Fixes: 47beed513a85b ("pinctrl: stm32: Add level interrupt support to gpio irq chip")
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Fabien Dessenne <fabien.dessenne@foss.st.com>

> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Fabien Dessenne <fabien.dessenne@foss.st.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-arm-kernel@lists.infradead.org
> To: linux-gpio@vger.kernel.org
> ---
>   drivers/pinctrl/stm32/pinctrl-stm32.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 242d1c37c6e4..7aecd0efde07 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -226,6 +226,13 @@ static void stm32_gpio_free(struct gpio_chip *chip, unsigned offset)
>   	pinctrl_gpio_free(chip->base + offset);
>   }
>   
> +static int stm32_gpio_get_noclk(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
> +
> +	return !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
> +}
> +
>   static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
>   {
>   	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
> @@ -233,7 +240,7 @@ static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
>   
>   	clk_enable(bank->clk);
>   
> -	ret = !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
> +	ret = stm32_gpio_get_noclk(chip, offset);
>   
>   	clk_disable(bank->clk);
>   
> @@ -317,7 +324,7 @@ static void stm32_gpio_irq_trigger(struct irq_data *d)
>   		return;
>   
>   	/* If level interrupt type then retrig */
> -	level = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
> +	level = stm32_gpio_get_noclk(&bank->gpio_chip, d->hwirq);
>   	if ((level == 0 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_LOW) ||
>   	    (level == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
>   		irq_chip_retrigger_hierarchy(d);
> @@ -359,6 +366,7 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>   {
>   	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>   	struct stm32_pinctrl *pctl = dev_get_drvdata(bank->gpio_chip.parent);
> +	unsigned long flags;
>   	int ret;
>   
>   	ret = stm32_gpio_direction_input(&bank->gpio_chip, irq_data->hwirq);
> @@ -372,6 +380,10 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>   		return ret;
>   	}
>   
> +	flags = irqd_get_trigger_type(irq_data);
> +	if (flags & IRQ_TYPE_LEVEL_MASK)
> +		clk_enable(bank->clk);
> +
>   	return 0;
>   }
>   
> @@ -379,6 +391,9 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>   {
>   	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>   
> +	if (bank->irq_type[irq_data->hwirq] & IRQ_TYPE_LEVEL_MASK)
> +		clk_disable(bank->clk);
> +
>   	gpiochip_unlock_as_irq(&bank->gpio_chip, irq_data->hwirq);
>   }
>   

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
@ 2022-04-22 14:28   ` Fabien DESSENNE
  0 siblings, 0 replies; 14+ messages in thread
From: Fabien DESSENNE @ 2022-04-22 14:28 UTC (permalink / raw)
  To: Marek Vasut, linux-gpio
  Cc: Alexandre Torgue, Linus Walleij, Marc Zyngier, linux-stm32,
	linux-arm-kernel

Hi Marek

Thank you for the patch.

Like Linus did for your other patch, I would prefer the subject prefix 
to be "pinctrl: stm32:"

Apart from that I added my RB below

BR
Fabien



On 21/04/2022 16:08, Marek Vasut wrote:
> The current EOI handler for LEVEL triggered interrupts calls clk_enable(),
> register IO, clk_disable(). The clock manipulation requires locking which
> happens with IRQs disabled in clk_enable_lock(). Instead of turning the
> clock on and off all the time, enable the clock in case LEVEL interrupt is
> requested and keep the clock enabled until all LEVEL interrupts are freed.
> The LEVEL interrupts are an exception on this platform and seldom used, so
> this does not affect the common case.
> 
> This simplifies the LEVEL interrupt handling considerably and also fixes
> the following splat found when using preempt-rt:
>   ------------[ cut here ]------------
>   WARNING: CPU: 0 PID: 0 at kernel/locking/rtmutex.c:2040 __rt_mutex_trylock+0x37/0x62
>   Modules linked in:
>   CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.109-rt65-stable-standard-00068-g6a5afc4b1217 #85
>   Hardware name: STM32 (Device Tree Support)
>   [<c010a45d>] (unwind_backtrace) from [<c010766f>] (show_stack+0xb/0xc)
>   [<c010766f>] (show_stack) from [<c06353ab>] (dump_stack+0x6f/0x84)
>   [<c06353ab>] (dump_stack) from [<c01145e3>] (__warn+0x7f/0xa4)
>   [<c01145e3>] (__warn) from [<c063386f>] (warn_slowpath_fmt+0x3b/0x74)
>   [<c063386f>] (warn_slowpath_fmt) from [<c063b43d>] (__rt_mutex_trylock+0x37/0x62)
>   [<c063b43d>] (__rt_mutex_trylock) from [<c063c053>] (rt_spin_trylock+0x7/0x16)
>   [<c063c053>] (rt_spin_trylock) from [<c036a2f3>] (clk_enable_lock+0xb/0x80)
>   [<c036a2f3>] (clk_enable_lock) from [<c036ba69>] (clk_core_enable_lock+0x9/0x18)
>   [<c036ba69>] (clk_core_enable_lock) from [<c034e9f3>] (stm32_gpio_get+0x11/0x24)
>   [<c034e9f3>] (stm32_gpio_get) from [<c034ef43>] (stm32_gpio_irq_trigger+0x1f/0x48)
>   [<c034ef43>] (stm32_gpio_irq_trigger) from [<c014aa53>] (handle_fasteoi_irq+0x71/0xa8)
>   [<c014aa53>] (handle_fasteoi_irq) from [<c0147111>] (generic_handle_irq+0x19/0x22)
>   [<c0147111>] (generic_handle_irq) from [<c014752d>] (__handle_domain_irq+0x55/0x64)
>   [<c014752d>] (__handle_domain_irq) from [<c0346f13>] (gic_handle_irq+0x53/0x64)
>   [<c0346f13>] (gic_handle_irq) from [<c0100ba5>] (__irq_svc+0x65/0xc0)
>   Exception stack(0xc0e01f18 to 0xc0e01f60)
>   1f00:                                                       0000300c 00000000
>   1f20: 0000300c c010ff01 00000000 00000000 c0e00000 c0e07714 00000001 c0e01f78
>   1f40: c0e07758 00000000 ef7cd0ff c0e01f68 c010554b c0105542 40000033 ffffffff
>   [<c0100ba5>] (__irq_svc) from [<c0105542>] (arch_cpu_idle+0xc/0x1e)
>   [<c0105542>] (arch_cpu_idle) from [<c063be95>] (default_idle_call+0x21/0x3c)
>   [<c063be95>] (default_idle_call) from [<c01324f7>] (do_idle+0xe3/0x1e4)
>   [<c01324f7>] (do_idle) from [<c01327b3>] (cpu_startup_entry+0x13/0x14)
>   [<c01327b3>] (cpu_startup_entry) from [<c0a00c13>] (start_kernel+0x397/0x3d4)
>   [<c0a00c13>] (start_kernel) from [<00000000>] (0x0)
>   ---[ end trace 0000000000000002 ]---
> 
> Power consumption measured on STM32MP157C DHCOM SoM is not increased or
> is below noise threshold.
> 
> Fixes: 47beed513a85b ("pinctrl: stm32: Add level interrupt support to gpio irq chip")
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Fabien Dessenne <fabien.dessenne@foss.st.com>

> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Fabien Dessenne <fabien.dessenne@foss.st.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-arm-kernel@lists.infradead.org
> To: linux-gpio@vger.kernel.org
> ---
>   drivers/pinctrl/stm32/pinctrl-stm32.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 242d1c37c6e4..7aecd0efde07 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -226,6 +226,13 @@ static void stm32_gpio_free(struct gpio_chip *chip, unsigned offset)
>   	pinctrl_gpio_free(chip->base + offset);
>   }
>   
> +static int stm32_gpio_get_noclk(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
> +
> +	return !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
> +}
> +
>   static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
>   {
>   	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
> @@ -233,7 +240,7 @@ static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
>   
>   	clk_enable(bank->clk);
>   
> -	ret = !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
> +	ret = stm32_gpio_get_noclk(chip, offset);
>   
>   	clk_disable(bank->clk);
>   
> @@ -317,7 +324,7 @@ static void stm32_gpio_irq_trigger(struct irq_data *d)
>   		return;
>   
>   	/* If level interrupt type then retrig */
> -	level = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
> +	level = stm32_gpio_get_noclk(&bank->gpio_chip, d->hwirq);
>   	if ((level == 0 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_LOW) ||
>   	    (level == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
>   		irq_chip_retrigger_hierarchy(d);
> @@ -359,6 +366,7 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>   {
>   	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>   	struct stm32_pinctrl *pctl = dev_get_drvdata(bank->gpio_chip.parent);
> +	unsigned long flags;
>   	int ret;
>   
>   	ret = stm32_gpio_direction_input(&bank->gpio_chip, irq_data->hwirq);
> @@ -372,6 +380,10 @@ static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>   		return ret;
>   	}
>   
> +	flags = irqd_get_trigger_type(irq_data);
> +	if (flags & IRQ_TYPE_LEVEL_MASK)
> +		clk_enable(bank->clk);
> +
>   	return 0;
>   }
>   
> @@ -379,6 +391,9 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>   {
>   	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>   
> +	if (bank->irq_type[irq_data->hwirq] & IRQ_TYPE_LEVEL_MASK)
> +		clk_disable(bank->clk);
> +
>   	gpiochip_unlock_as_irq(&bank->gpio_chip, irq_data->hwirq);
>   }
>   

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
  2022-04-21 14:08 ` Marek Vasut
@ 2022-04-22 22:11   ` Linus Walleij
  -1 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2022-04-22 22:11 UTC (permalink / raw)
  To: Marek Vasut
  Cc: linux-gpio, Alexandre Torgue, Fabien Dessenne, Marc Zyngier,
	linux-stm32, linux-arm-kernel

On Thu, Apr 21, 2022 at 4:08 PM Marek Vasut <marex@denx.de> wrote:

> The current EOI handler for LEVEL triggered interrupts calls clk_enable(),
> register IO, clk_disable(). The clock manipulation requires locking which
> happens with IRQs disabled in clk_enable_lock(). Instead of turning the
> clock on and off all the time, enable the clock in case LEVEL interrupt is
> requested and keep the clock enabled until all LEVEL interrupts are freed.
> The LEVEL interrupts are an exception on this platform and seldom used, so
> this does not affect the common case.
>
> This simplifies the LEVEL interrupt handling considerably and also fixes
> the following splat found when using preempt-rt:
>  ------------[ cut here ]------------
>  WARNING: CPU: 0 PID: 0 at kernel/locking/rtmutex.c:2040 __rt_mutex_trylock+0x37/0x62
>  Modules linked in:
>  CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.109-rt65-stable-standard-00068-g6a5afc4b1217 #85
>  Hardware name: STM32 (Device Tree Support)
>  [<c010a45d>] (unwind_backtrace) from [<c010766f>] (show_stack+0xb/0xc)
>  [<c010766f>] (show_stack) from [<c06353ab>] (dump_stack+0x6f/0x84)
>  [<c06353ab>] (dump_stack) from [<c01145e3>] (__warn+0x7f/0xa4)
>  [<c01145e3>] (__warn) from [<c063386f>] (warn_slowpath_fmt+0x3b/0x74)
>  [<c063386f>] (warn_slowpath_fmt) from [<c063b43d>] (__rt_mutex_trylock+0x37/0x62)
>  [<c063b43d>] (__rt_mutex_trylock) from [<c063c053>] (rt_spin_trylock+0x7/0x16)
>  [<c063c053>] (rt_spin_trylock) from [<c036a2f3>] (clk_enable_lock+0xb/0x80)
>  [<c036a2f3>] (clk_enable_lock) from [<c036ba69>] (clk_core_enable_lock+0x9/0x18)
>  [<c036ba69>] (clk_core_enable_lock) from [<c034e9f3>] (stm32_gpio_get+0x11/0x24)
>  [<c034e9f3>] (stm32_gpio_get) from [<c034ef43>] (stm32_gpio_irq_trigger+0x1f/0x48)
>  [<c034ef43>] (stm32_gpio_irq_trigger) from [<c014aa53>] (handle_fasteoi_irq+0x71/0xa8)
>  [<c014aa53>] (handle_fasteoi_irq) from [<c0147111>] (generic_handle_irq+0x19/0x22)
>  [<c0147111>] (generic_handle_irq) from [<c014752d>] (__handle_domain_irq+0x55/0x64)
>  [<c014752d>] (__handle_domain_irq) from [<c0346f13>] (gic_handle_irq+0x53/0x64)
>  [<c0346f13>] (gic_handle_irq) from [<c0100ba5>] (__irq_svc+0x65/0xc0)
>  Exception stack(0xc0e01f18 to 0xc0e01f60)
>  1f00:                                                       0000300c 00000000
>  1f20: 0000300c c010ff01 00000000 00000000 c0e00000 c0e07714 00000001 c0e01f78
>  1f40: c0e07758 00000000 ef7cd0ff c0e01f68 c010554b c0105542 40000033 ffffffff
>  [<c0100ba5>] (__irq_svc) from [<c0105542>] (arch_cpu_idle+0xc/0x1e)
>  [<c0105542>] (arch_cpu_idle) from [<c063be95>] (default_idle_call+0x21/0x3c)
>  [<c063be95>] (default_idle_call) from [<c01324f7>] (do_idle+0xe3/0x1e4)
>  [<c01324f7>] (do_idle) from [<c01327b3>] (cpu_startup_entry+0x13/0x14)
>  [<c01327b3>] (cpu_startup_entry) from [<c0a00c13>] (start_kernel+0x397/0x3d4)
>  [<c0a00c13>] (start_kernel) from [<00000000>] (0x0)
>  ---[ end trace 0000000000000002 ]---
>
> Power consumption measured on STM32MP157C DHCOM SoM is not increased or
> is below noise threshold.
>
> Fixes: 47beed513a85b ("pinctrl: stm32: Add level interrupt support to gpio irq chip")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Fabien Dessenne <fabien.dessenne@foss.st.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-arm-kernel@lists.infradead.org
> To: linux-gpio@vger.kernel.org

Patch applied to fixes, tweaked subject to pinctrl: stm32:

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested
@ 2022-04-22 22:11   ` Linus Walleij
  0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2022-04-22 22:11 UTC (permalink / raw)
  To: Marek Vasut
  Cc: linux-gpio, Alexandre Torgue, Fabien Dessenne, Marc Zyngier,
	linux-stm32, linux-arm-kernel

On Thu, Apr 21, 2022 at 4:08 PM Marek Vasut <marex@denx.de> wrote:

> The current EOI handler for LEVEL triggered interrupts calls clk_enable(),
> register IO, clk_disable(). The clock manipulation requires locking which
> happens with IRQs disabled in clk_enable_lock(). Instead of turning the
> clock on and off all the time, enable the clock in case LEVEL interrupt is
> requested and keep the clock enabled until all LEVEL interrupts are freed.
> The LEVEL interrupts are an exception on this platform and seldom used, so
> this does not affect the common case.
>
> This simplifies the LEVEL interrupt handling considerably and also fixes
> the following splat found when using preempt-rt:
>  ------------[ cut here ]------------
>  WARNING: CPU: 0 PID: 0 at kernel/locking/rtmutex.c:2040 __rt_mutex_trylock+0x37/0x62
>  Modules linked in:
>  CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.109-rt65-stable-standard-00068-g6a5afc4b1217 #85
>  Hardware name: STM32 (Device Tree Support)
>  [<c010a45d>] (unwind_backtrace) from [<c010766f>] (show_stack+0xb/0xc)
>  [<c010766f>] (show_stack) from [<c06353ab>] (dump_stack+0x6f/0x84)
>  [<c06353ab>] (dump_stack) from [<c01145e3>] (__warn+0x7f/0xa4)
>  [<c01145e3>] (__warn) from [<c063386f>] (warn_slowpath_fmt+0x3b/0x74)
>  [<c063386f>] (warn_slowpath_fmt) from [<c063b43d>] (__rt_mutex_trylock+0x37/0x62)
>  [<c063b43d>] (__rt_mutex_trylock) from [<c063c053>] (rt_spin_trylock+0x7/0x16)
>  [<c063c053>] (rt_spin_trylock) from [<c036a2f3>] (clk_enable_lock+0xb/0x80)
>  [<c036a2f3>] (clk_enable_lock) from [<c036ba69>] (clk_core_enable_lock+0x9/0x18)
>  [<c036ba69>] (clk_core_enable_lock) from [<c034e9f3>] (stm32_gpio_get+0x11/0x24)
>  [<c034e9f3>] (stm32_gpio_get) from [<c034ef43>] (stm32_gpio_irq_trigger+0x1f/0x48)
>  [<c034ef43>] (stm32_gpio_irq_trigger) from [<c014aa53>] (handle_fasteoi_irq+0x71/0xa8)
>  [<c014aa53>] (handle_fasteoi_irq) from [<c0147111>] (generic_handle_irq+0x19/0x22)
>  [<c0147111>] (generic_handle_irq) from [<c014752d>] (__handle_domain_irq+0x55/0x64)
>  [<c014752d>] (__handle_domain_irq) from [<c0346f13>] (gic_handle_irq+0x53/0x64)
>  [<c0346f13>] (gic_handle_irq) from [<c0100ba5>] (__irq_svc+0x65/0xc0)
>  Exception stack(0xc0e01f18 to 0xc0e01f60)
>  1f00:                                                       0000300c 00000000
>  1f20: 0000300c c010ff01 00000000 00000000 c0e00000 c0e07714 00000001 c0e01f78
>  1f40: c0e07758 00000000 ef7cd0ff c0e01f68 c010554b c0105542 40000033 ffffffff
>  [<c0100ba5>] (__irq_svc) from [<c0105542>] (arch_cpu_idle+0xc/0x1e)
>  [<c0105542>] (arch_cpu_idle) from [<c063be95>] (default_idle_call+0x21/0x3c)
>  [<c063be95>] (default_idle_call) from [<c01324f7>] (do_idle+0xe3/0x1e4)
>  [<c01324f7>] (do_idle) from [<c01327b3>] (cpu_startup_entry+0x13/0x14)
>  [<c01327b3>] (cpu_startup_entry) from [<c0a00c13>] (start_kernel+0x397/0x3d4)
>  [<c0a00c13>] (start_kernel) from [<00000000>] (0x0)
>  ---[ end trace 0000000000000002 ]---
>
> Power consumption measured on STM32MP157C DHCOM SoM is not increased or
> is below noise threshold.
>
> Fixes: 47beed513a85b ("pinctrl: stm32: Add level interrupt support to gpio irq chip")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Fabien Dessenne <fabien.dessenne@foss.st.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-arm-kernel@lists.infradead.org
> To: linux-gpio@vger.kernel.org

Patch applied to fixes, tweaked subject to pinctrl: stm32:

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-04-22 22:48 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-21 14:08 [PATCH] irqchip/stm32: Keep pinctrl block clock enabled when LEVEL IRQ requested Marek Vasut
2022-04-21 14:08 ` Marek Vasut
2022-04-22  9:20 ` Fabien DESSENNE
2022-04-22  9:20   ` Fabien DESSENNE
2022-04-22  9:32   ` Marek Vasut
2022-04-22  9:32     ` Marek Vasut
2022-04-22 10:08   ` Marc Zyngier
2022-04-22 10:08     ` Marc Zyngier
2022-04-22 10:25     ` Fabien DESSENNE
2022-04-22 10:25       ` Fabien DESSENNE
2022-04-22 14:28 ` Fabien DESSENNE
2022-04-22 14:28   ` Fabien DESSENNE
2022-04-22 22:11 ` Linus Walleij
2022-04-22 22:11   ` Linus Walleij

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.