Message ID | 20220518192924.20948-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
---|---|
Headers | show |
Series | Renesas RZ/G2L IRQC support | expand |
On Wed, May 18, 2022 at 9:30 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Add a check to validate GPIO hwirq is always within the range of hwirq_max > set in the GPIO irq domain. ... > + if (WARN(hwirq >= domain->hwirq_max, > + "error: hwirq 0x%x is too large for %s\n", > + (int)hwirq, domain->name)) Using castings in the printf() often points to possible mistakes or missed custom specifiers. ... > + if (WARN(hwirq >= domain->hwirq_max, > + "error: hwirq 0x%x is too large for %s\n", (int)hwirq, domain->name)) Ditto.
On Wed, May 18, 2022 at 9:29 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Hi All, > > The RZ/G2L Interrupt Controller is a front-end for the GIC found on > Renesas RZ/G2L SoC's with below pins: > - IRQ sense select for 8 external interrupts, mapped to 8 GIC SPI > interrupts > - GPIO pins used as external interrupt input pins out of GPIOINT0-122 a > maximum of only 32 can be mapped to 32 GIC SPI interrupts, > - NMI edge select. > > _____________ > | GIC | > | ________ | > ____________ | | | | > NMI --------------------------------->| | SPI0-479 | | GIC-600| | > _______ | |------------>| | | > | | | | PPI16-31 | | | | > | | IRQ0-IRQ7 | IRQC |------------>| | | > P0_P48_4 --->| GPIO |---------------->| | | |________| | > | |GPIOINT0-122 | | | | > | |---------------->| TINT0-31 | | | > |______| |__________| |____________| > > The proposed patches add hierarchical IRQ domain, one in IRQC driver and > another in pinctrl driver. Upon interrupt requests map the interrupt to > GIC. Out of GPIOINT0-122 only 32 can be mapped to GIC SPI, this mapping is > handled by the pinctrl and IRQC driver. Where is the explanation on why valid_mask can't be used instead?
On Wed, May 18, 2022 at 10:10 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, May 18, 2022 at 9:29 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > Hi All, > > > > The RZ/G2L Interrupt Controller is a front-end for the GIC found on > > Renesas RZ/G2L SoC's with below pins: > > - IRQ sense select for 8 external interrupts, mapped to 8 GIC SPI > > interrupts > > - GPIO pins used as external interrupt input pins out of GPIOINT0-122 a > > maximum of only 32 can be mapped to 32 GIC SPI interrupts, > > - NMI edge select. > > > > _____________ > > | GIC | > > | ________ | > > ____________ | | | | > > NMI --------------------------------->| | SPI0-479 | | GIC-600| | > > _______ | |------------>| | | > > | | | | PPI16-31 | | | | > > | | IRQ0-IRQ7 | IRQC |------------>| | | > > P0_P48_4 --->| GPIO |---------------->| | | |________| | > > | |GPIOINT0-122 | | | | > > | |---------------->| TINT0-31 | | | > > |______| |__________| |____________| > > > > The proposed patches add hierarchical IRQ domain, one in IRQC driver and > > another in pinctrl driver. Upon interrupt requests map the interrupt to > > GIC. Out of GPIOINT0-122 only 32 can be mapped to GIC SPI, this mapping is > > handled by the pinctrl and IRQC driver. > > Where is the explanation on why valid_mask can't be used instead? > The .valid_mask option is one time setting but what I need is something dynamic i.e. out of 392 GPIO pins any 32 can be used as an interrupt pin. Also with this patch we also save on memory here [0]. [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/kernel/irq/irqdomain.c?h=next-20220518#n153 Cheers, Prabhakar > > -- > With Best Regards, > Andy Shevchenko
Hi Andy, Thank you for the review. On Wed, May 18, 2022 at 10:08 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, May 18, 2022 at 9:30 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > Add a check to validate GPIO hwirq is always within the range of hwirq_max > > set in the GPIO irq domain. > > ... > > > + if (WARN(hwirq >= domain->hwirq_max, > > + "error: hwirq 0x%x is too large for %s\n", > > + (int)hwirq, domain->name)) > > Using castings in the printf() often points to possible mistakes or > missed custom specifiers. > Right, I picked up the printf() just a few lines above where it did the same exact thing. I will update it in the next version. > ... > > > + if (WARN(hwirq >= domain->hwirq_max, > > + "error: hwirq 0x%x is too large for %s\n", (int)hwirq, domain->name)) > > Ditto. > Will drop castings. Cheers, Prabhakar
Hi Prabhakar, > Subject: Re: [PATCH v4 0/7] Renesas RZ/G2L IRQC support > > On Wed, May 18, 2022 at 10:10 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Wed, May 18, 2022 at 9:29 PM Lad Prabhakar > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > Hi All, > > > > > > The RZ/G2L Interrupt Controller is a front-end for the GIC found on > > > Renesas RZ/G2L SoC's with below pins: > > > - IRQ sense select for 8 external interrupts, mapped to 8 GIC SPI > > > interrupts > > > - GPIO pins used as external interrupt input pins out of GPIOINT0-122 a > > > maximum of only 32 can be mapped to 32 GIC SPI interrupts, > > > - NMI edge select. > > > > > > > _____________ > > > | GIC > | > > > | > ________ | > > > ____________ | | > | | > > > NMI --------------------------------->| | SPI0-479 | | GIC- > 600| | > > > _______ | |------------>| > | | > > > | | | | PPI16-31 | | > | | > > > | | IRQ0-IRQ7 | IRQC |------------>| > | | > > > P0_P48_4 --->| GPIO |---------------->| | | > |________| | > > > | |GPIOINT0-122 | | | > | > > > | |---------------->| TINT0-31 | | > | > > > |______| |__________| > |____________| > > > > > > The proposed patches add hierarchical IRQ domain, one in IRQC driver > > > and another in pinctrl driver. Upon interrupt requests map the > > > interrupt to GIC. Out of GPIOINT0-122 only 32 can be mapped to GIC > > > SPI, this mapping is handled by the pinctrl and IRQC driver. > > > > Where is the explanation on why valid_mask can't be used instead? > > > The .valid_mask option is one time setting One question, if it is one time setting, Is it possible to use .valid mask to invalidate invalid gpio lines?(ie, currently gpio range is 392, but there is only 123 GPIOs present in the SoC, not sure this call back can be used to invalidate the non-supported GPIOS??). Cheers, Biju but what I need is something > dynamic i.e. out of 392 GPIO pins any 32 can be used as an interrupt pin. > Also with this patch we also save on memory here [0]. , > > Andy Shevchenko
On Thu, May 19, 2022 at 6:07 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Wed, May 18, 2022 at 10:10 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, May 18, 2022 at 9:29 PM Lad Prabhakar > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: ... > > > GIC. Out of GPIOINT0-122 only 32 can be mapped to GIC SPI, this mapping is > > > handled by the pinctrl and IRQC driver. > > > > Where is the explanation on why valid_mask can't be used instead? > > > The .valid_mask option is one time setting but what I need is > something dynamic i.e. out of 392 GPIO pins any 32 can be used as an > interrupt pin. Also with this patch we also save on memory here [0]. Which internal APIs are bound to valid_mask not to be updated?
On Wed, May 18, 2022 at 9:30 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Supported GPIO IRQs by the chip is not always equal to the number of GPIO > pins. For example on Renesas RZ/G2L SoC where it has GPIO0-122 pins but at > a given point a maximum of only 32 GPIO pins can be used as IRQ lines in > the IRQC domain. > > This patch adds ngirq member to struct gpio_irq_chip and passes this as a > size to irq_domain_create_hierarchy()/irq_domain_create_simple() if it is > being set in the driver otherwise fallbacks to using ngpio. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> NAK As pointed out this is a property of the hardware and thus you should derive this property of the hardware from the compatible string. For example by passing per-variant .data in struct of_device_id. Unique hardware properties means unique hardware means it should have a unique compatible string. Otherwise something is wrong with the compatibles. Yours, Linus Walleij
On Wed, May 18, 2022 at 9:30 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > On Renesas RZ/G2L SoC not all the GPIO pins can be simultaneously used as > interrupts. The SoC allows 32 interrupts which is first come first serve > basis and is dynamic i.e. if there is a free slot (after rmmod) this can > be used by other GPIO pins being used as an interrupt. > > To handle such cases change child_offset_to_irq() callback to return error > codes in case of failure. All the users of child_offset_to_irq() callback > are also updated with this API change. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> This looks very useful! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi Linus, Thank you for the review. On Thu, May 19, 2022 at 2:26 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, May 18, 2022 at 9:30 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > Supported GPIO IRQs by the chip is not always equal to the number of GPIO > > pins. For example on Renesas RZ/G2L SoC where it has GPIO0-122 pins but at > > a given point a maximum of only 32 GPIO pins can be used as IRQ lines in > > the IRQC domain. > > > > This patch adds ngirq member to struct gpio_irq_chip and passes this as a > > size to irq_domain_create_hierarchy()/irq_domain_create_simple() if it is > > being set in the driver otherwise fallbacks to using ngpio. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > NAK > > As pointed out this is a property of the hardware and thus you should > derive this property of the hardware from the compatible string. > > For example by passing per-variant .data in struct of_device_id. > > Unique hardware properties means unique hardware means it should > have a unique compatible string. Otherwise something is wrong > with the compatibles. > Agreed, I will drop this. Cheers, Prabhakar
Hi Biju, On Thu, May 19, 2022 at 7:58 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Prabhakar, > > > Subject: Re: [PATCH v4 0/7] Renesas RZ/G2L IRQC support > > > > On Wed, May 18, 2022 at 10:10 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > On Wed, May 18, 2022 at 9:29 PM Lad Prabhakar > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > > > Hi All, > > > > > > > > The RZ/G2L Interrupt Controller is a front-end for the GIC found on > > > > Renesas RZ/G2L SoC's with below pins: > > > > - IRQ sense select for 8 external interrupts, mapped to 8 GIC SPI > > > > interrupts > > > > - GPIO pins used as external interrupt input pins out of GPIOINT0-122 a > > > > maximum of only 32 can be mapped to 32 GIC SPI interrupts, > > > > - NMI edge select. > > > > > > > > > > _____________ > > > > | GIC > > | > > > > | > > ________ | > > > > ____________ | | > > | | > > > > NMI --------------------------------->| | SPI0-479 | | GIC- > > 600| | > > > > _______ | |------------>| > > | | > > > > | | | | PPI16-31 | | > > | | > > > > | | IRQ0-IRQ7 | IRQC |------------>| > > | | > > > > P0_P48_4 --->| GPIO |---------------->| | | > > |________| | > > > > | |GPIOINT0-122 | | | > > | > > > > | |---------------->| TINT0-31 | | > > | > > > > |______| |__________| > > |____________| > > > > > > > > The proposed patches add hierarchical IRQ domain, one in IRQC driver > > > > and another in pinctrl driver. Upon interrupt requests map the > > > > interrupt to GIC. Out of GPIOINT0-122 only 32 can be mapped to GIC > > > > SPI, this mapping is handled by the pinctrl and IRQC driver. > > > > > > Where is the explanation on why valid_mask can't be used instead? > > > > > The .valid_mask option is one time setting > > One question, if it is one time setting, Is it possible to use .valid mask to invalidate > invalid gpio lines?(ie, currently gpio range is 392, but there is only 123 GPIOs > present in the SoC, not sure this call back can be used to invalidate the non-supported GPIOS??). > Yes can be added, I will include it in the next version. Cheers, Prabhakar