diff mbox series

pinctrl: renesas: rzg2l: Always call rzg2l_gpio_request() for interrupt pins

Message ID 20241003131642.472298-1-prabhakar.mahadev-lad.rj@bp.renesas.com
State New
Headers show
Series pinctrl: renesas: rzg2l: Always call rzg2l_gpio_request() for interrupt pins | expand

Commit Message

Prabhakar Oct. 3, 2024, 1:16 p.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Ensure that rzg2l_gpio_request() is called for GPIO pins configured as
interrupts, regardless of whether they are muxed in u-boot. This
guarantees that the pinctrl core is aware of the GPIO pin usage via
pinctrl_gpio_request(), which is invoked through rzg2l_gpio_request().

Fixes: 2fd4fe19d0150 ("pinctrl: renesas: rzg2l: Configure interrupt input mode")
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Output before this patch on G2L/SMARC:
root@smarc-rzg2l:~# cat /sys/kernel/debug/pinctrl/11030000.pinctrl-pinctrl-rzg2l/pinmux-pins | grep P2_1
pin 17 (P2_1): UNCLAIMED

Output after this patch G2L/SMARC:
root@smarc-rzg2l:~# cat /sys/kernel/debug/pinctrl/11030000.pinctrl-pinctrl-rzg2l/pinmux-pins | grep P2_1
pin 17 (P2_1): GPIO 11030000.pinctrl:529
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

Comments

Claudiu Beznea Oct. 9, 2024, 8:11 a.m. UTC | #1 
Hi, Prabhakar,

On 03.10.2024 16:16, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Ensure that rzg2l_gpio_request() is called for GPIO pins configured as
> interrupts, regardless of whether they are muxed in u-boot. This
> guarantees that the pinctrl core is aware of the GPIO pin usage via
> pinctrl_gpio_request(), which is invoked through rzg2l_gpio_request().
> 
> Fixes: 2fd4fe19d0150 ("pinctrl: renesas: rzg2l: Configure interrupt input mode")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Output before this patch on G2L/SMARC:
> root@smarc-rzg2l:~# cat /sys/kernel/debug/pinctrl/11030000.pinctrl-pinctrl-rzg2l/pinmux-pins | grep P2_1
> pin 17 (P2_1): UNCLAIMED
> 
> Output after this patch G2L/SMARC:
> root@smarc-rzg2l:~# cat /sys/kernel/debug/pinctrl/11030000.pinctrl-pinctrl-rzg2l/pinmux-pins | grep P2_1
> pin 17 (P2_1): GPIO 11030000.pinctrl:529
> ---
>  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index 60ef20ca3ccf..1dceaf8290ea 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -2368,20 +2368,11 @@ static const struct irq_chip rzg2l_gpio_irqchip = {
>  
>  static int rzg2l_gpio_interrupt_input_mode(struct gpio_chip *chip, unsigned int offset)
>  {
> -	struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip);
> -	const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[offset];
> -	u64 *pin_data = pin_desc->drv_data;
> -	u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data);
> -	u8 bit = RZG2L_PIN_ID_TO_PIN(offset);
> -	u8 reg8;
>  	int ret;
>  
> -	reg8 = readb(pctrl->base + PMC(off));
> -	if (reg8 & BIT(bit)) {
> -		ret = rzg2l_gpio_request(chip, offset);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = rzg2l_gpio_request(chip, offset);
> +	if (ret)
> +		return ret;
>  

With this approach I'm getting the following on RZ/G3S SMARC Carrier II board:

[    0.368129] pinctrl-rzg2l 11030000.pinctrl: pinctrl-rzg2l support registered
[    0.390426] 1004b800.serial: ttySC0 at MMIO 0x1004b800 (irq = 42,
base_baud = 0) is a scif
[    0.390558] printk: legacy console [ttySC0] enabled
[    1.601991] pinctrl-rzg2l 11030000.pinctrl: pin P12_0 already requested
by 11030000.pinctrl:608; cannot claim for 11030000.pinctrl:608
[    1.614208] pinctrl-rzg2l 11030000.pinctrl: error -EINVAL: pin-96
(11030000.pinctrl:608)
[    1.622313] gpio gpiochip0: (11030000.pinctrl): can't look up hwirq 96
[    1.631801] ravb 11c30000.ethernet eth0: Base address at 0x11c30000,
d2:7b:7f:8f:d8:52, IRQ 46.
[    1.645752] pinctrl-rzg2l 11030000.pinctrl: pin P12_1 already requested
by 11030000.pinctrl:609; cannot claim for 11030000.pinctrl:609
[    1.657923] pinctrl-rzg2l 11030000.pinctrl: error -EINVAL: pin-97
(11030000.pinctrl:609)
[    1.666035] gpio gpiochip0: (11030000.pinctrl): can't look up hwirq 97
[    1.675573] ravb 11c40000.ethernet eth1: Base address at 0x11c40000,
d2:7b:7f:8f:d8:52, IRQ 47.
[    1.700907] pinctrl-rzg2l 11030000.pinctrl: pin P18_0 already requested
by 11030000.pinctrl:656; cannot claim for 11030000.pinctrl:656
[    1.713272] pinctrl-rzg2l 11030000.pinctrl: error -EINVAL: pin-144
(11030000.pinctrl:656)
[    1.721496] gpio gpiochip0: (11030000.pinctrl): can't look up hwirq 144
[    1.729209] pinctrl-rzg2l 11030000.pinctrl: pin P0_1 already requested
by 11030000.pinctrl:513; cannot claim for 11030000.pinctrl:513
[    1.741345] pinctrl-rzg2l 11030000.pinctrl: error -EINVAL: pin-1
(11030000.pinctrl:513)
[    1.749432] gpio gpiochip0: (11030000.pinctrl): can't look up hwirq 1
[    1.756285] pinctrl-rzg2l 11030000.pinctrl: pin P0_3 already requested
by 11030000.pinctrl:515; cannot claim for 11030000.pinctrl:515
[    1.768475] pinctrl-rzg2l 11030000.pinctrl: error -EINVAL: pin-3
(11030000.pinctrl:515)
[    1.776524] gpio gpiochip0: (11030000.pinctrl): can't look up hwirq 3
[    1.783124] gpio-keys keys: Found button without gpio or irq
[    1.788851] renesas_sdhi_internal_dmac 11c00000.mmc: mmc0 base at
0x0000000011c00000, max clock rate 125 MHz
[    1.798791] gpio-keys keys: probe with driver gpio-keys failed with
error -22


All these ports are hogs to configure them as input. Removing the hog
property make this patch work but I'm not sure this is the right approach
(see below diff).

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
index 71424e69939e..6e95933cd7ef 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
@@ -196,15 +196,6 @@ &sdhi2 {
 #endif

 &pinctrl {
-#if SW_CONFIG3 == SW_ON
-       eth0-phy-irq-hog {
-               gpio-hog;
-               gpios = <RZG2L_GPIO(12, 0) GPIO_ACTIVE_LOW>;
-               input;
-               line-name = "eth0-phy-irq";
-       };
-#endif
-
        eth0_pins: eth0 {
                txc {
                        pinmux = <RZG2L_PORT_PINMUX(1, 0, 1)>;  /* ET0_TXC */
@@ -239,15 +230,6 @@ mux {
                };
        };

-#if SW_CONFIG3 == SW_ON
-       eth1-phy-irq-hog {
-               gpio-hog;
-               gpios = <RZG2L_GPIO(12, 1) GPIO_ACTIVE_LOW>;
-               input;
-               line-name = "eth1-phy-irq";
-       };
-#endif
-
        eth1_pins: eth1 {
                txc {
                        pinmux = <RZG2L_PORT_PINMUX(7, 0, 1)>;  /* ET1_TXC */
diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
index 4509151344c4..9dd313f6f8c2 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
@@ -71,26 +71,6 @@ &i2c0 {
 };

 &pinctrl {
-       key-1-gpio-hog {
-               gpio-hog;
-               gpios = <RZG2L_GPIO(18, 0) GPIO_ACTIVE_LOW>;
-               input;
-               line-name = "key-1-gpio-irq";
-       };
-
-       key-2-gpio-hog {
-               gpio-hog;
-               gpios = <RZG2L_GPIO(0, 1) GPIO_ACTIVE_LOW>;
-               input;
-               line-name = "key-2-gpio-irq";
-       };
-
-       key-3-gpio-hog {
-               gpio-hog;
-               gpios = <RZG2L_GPIO(0, 3) GPIO_ACTIVE_LOW>;
-               input;
-               line-name = "key-3-gpio-irq";
-       };

        scif0_pins: scif0 {
                pinmux = <RZG2L_PORT_PINMUX(6, 3, 1)>, /* RXD */


>  	return rzg2l_gpio_direction_input(chip, offset);
>  }
Linus Walleij Oct. 10, 2024, 7:47 p.m. UTC | #2 
On Wed, Oct 9, 2024 at 10:27 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Wed, Oct 9, 2024 at 9:11 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote:

> > All these ports are hogs to configure them as input. Removing the hog
> > property make this patch work but I'm not sure this is the right approach
> > (see below diff).
> >
> I have dropped a query [0] to GPIO maintainers to check on the correct approach.
>
> https://lore.kernel.org/all/CA+V-a8vxUjTWccV-wLgy5CJiFYfEMsx-f+8weCJDP6uD_dh4AA@mail.gmail.com/

Yeah I replied, the callbacks in struct irq_chip rzg2l_gpio_irqchip
should be calling the following callbacks:

/* lock/unlock as IRQ */
int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset);
void gpiochip_unlock_as_irq(struct gpio_chip *gc, unsigned int offset);

In its
.irq_request_resources and .irq_release_resources callbacks.

And it currently doesn't even define these callbacks.

If the driver was using the GPIOLIB_IRQCHIP and adding the
irqchip in the standard way along with the gpiochip, this would
not be a problem.

Can you look into simply using GPIOLIB_IRQCHIP like most
other drivers as well?

Yours,
Linus Walleij
Prabhakar Oct. 10, 2024, 7:52 p.m. UTC | #3 
Hi Linus,

On Thu, Oct 10, 2024 at 8:47 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Oct 9, 2024 at 10:27 AM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Wed, Oct 9, 2024 at 9:11 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
>
> > > All these ports are hogs to configure them as input. Removing the hog
> > > property make this patch work but I'm not sure this is the right approach
> > > (see below diff).
> > >
> > I have dropped a query [0] to GPIO maintainers to check on the correct approach.
> >
> > https://lore.kernel.org/all/CA+V-a8vxUjTWccV-wLgy5CJiFYfEMsx-f+8weCJDP6uD_dh4AA@mail.gmail.com/
>
> Yeah I replied, the callbacks in struct irq_chip rzg2l_gpio_irqchip
> should be calling the following callbacks:
>
Sorry I wanted to do some poc before I responded to your email.

> /* lock/unlock as IRQ */
> int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset);
> void gpiochip_unlock_as_irq(struct gpio_chip *gc, unsigned int offset);
>
> In its
> .irq_request_resources and .irq_release_resources callbacks.
>
> And it currently doesn't even define these callbacks.
>
> If the driver was using the GPIOLIB_IRQCHIP and adding the
> irqchip in the standard way along with the gpiochip, this would
> not be a problem.
>
> Can you look into simply using GPIOLIB_IRQCHIP like most
> other drivers as well?
>
Thanks for the pointers, I'll investigate and add support for it.

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 60ef20ca3ccf..1dceaf8290ea 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -2368,20 +2368,11 @@  static const struct irq_chip rzg2l_gpio_irqchip = {
 
 static int rzg2l_gpio_interrupt_input_mode(struct gpio_chip *chip, unsigned int offset)
 {
-	struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip);
-	const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[offset];
-	u64 *pin_data = pin_desc->drv_data;
-	u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data);
-	u8 bit = RZG2L_PIN_ID_TO_PIN(offset);
-	u8 reg8;
 	int ret;
 
-	reg8 = readb(pctrl->base + PMC(off));
-	if (reg8 & BIT(bit)) {
-		ret = rzg2l_gpio_request(chip, offset);
-		if (ret)
-			return ret;
-	}
+	ret = rzg2l_gpio_request(chip, offset);
+	if (ret)
+		return ret;
 
 	return rzg2l_gpio_direction_input(chip, offset);
 }