Message ID | 20241003131642.472298-1-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
---|---|
State | Under Review |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | pinctrl: renesas: rzg2l: Always call rzg2l_gpio_request() for interrupt pins | expand |
Hi Prabhakar, On Thu, Oct 3, 2024 at 3:16 PM Prabhakar <prabhakar.csengg@gmail.com> 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> Thanks for your patch! > 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 Just wondering: is this restored to UNCLAIMED after releasing the interrupt (i.e. after unbinding the ADV7535 driver)? Gr{oetje,eeting}s, Geert
Hi Geert, On Thu, Oct 3, 2024 at 2:46 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Thu, Oct 3, 2024 at 3:16 PM Prabhakar <prabhakar.csengg@gmail.com> 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> > > Thanks for your patch! > > > 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 > > Just wondering: is this restored to UNCLAIMED after releasing the > interrupt (i.e. after unbinding the ADV7535 driver)? > Actually it doesn't report `UNCLAIMED` after `modprobe -r adv7511`, pinmux-pins reports P2_1 is claimed 11030000.pinctrl:529. `modprobe adv7511` later succeeds though (maybe because it's the same device). rzg2l_gpio_free() is called from the rzg2l_gpio_irq_domain_free() path, either this path is not being called when IRQ is freed or the adv7511 isn't releasing the IRQ. Cheers, Prabhakar
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); > }
Hi Claudiu, On Wed, Oct 9, 2024 at 9:11 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote: > > 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 > Thanks for the test, I noticed the same. > > 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/ Cheers, Prabhakar
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
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 --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); }