| Message ID | 20220523174238.28942-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Renesas RZ/G2L IRQC support | expand |
On Mon, May 23, 2022 at 7:43 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Allow free() callback to be overridden from irq_domain_ops for > hierarchical chips. > > This allows drivers to free up resources which are allocated during > child_to_parent_hwirq()/populate_parent_alloc_arg() callbacks. > > On Renesas RZ/G2L platform a bitmap is maintained for TINT slots, a slot > is allocated in child_to_parent_hwirq() callback which is freed up in free > callback hence this override. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> So that function today looks like this: static void gpiochip_hierarchy_setup_domain_ops(struct irq_domain_ops *ops) { ops->activate = gpiochip_irq_domain_activate; ops->deactivate = gpiochip_irq_domain_deactivate; ops->alloc = gpiochip_hierarchy_irq_domain_alloc; ops->free = irq_domain_free_irqs_common; /* * We only allow overriding the translate() function for * hierarchical chips, and this should only be done if the user * really need something other than 1:1 translation. */ if (!ops->translate) ops->translate = gpiochip_hierarchy_irq_domain_translate; } (...) - ops->free = irq_domain_free_irqs_common; (...) > + if (!ops->free) > + ops->free = irq_domain_free_irqs_common; Marc Z is working on cleaning up the way that gpiolib is (ab)using irqchips. We definitely need his ACK if we do things like this. This doesn't look like one of the big offenders to me, but I want to make sure we don't create new problems while Marc is trying to solve the old ones. Yours, Linus Walleij
On Mon, May 23, 2022 at 7:43 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Add IRQ domain to RZ/G2L pinctrl driver to handle GPIO interrupt. > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > used as IRQ lines at a given time. Selection of pins as IRQ lines > is handled by IA55 (which is the IRQC block) which sits in between the > GPIO and GIC. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> I don't know if I'm too tired or reading it wrong, but it seems you went through the trouble of making it possible to override .free() in the irqdomain in patch 3/5 and yet not using it in this patch 5/5? Yours, Linus Walleij
Hi Linus, Thank you for the review. On Tue, May 24, 2022 at 9:57 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, May 23, 2022 at 7:43 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > Add IRQ domain to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > > used as IRQ lines at a given time. Selection of pins as IRQ lines > > is handled by IA55 (which is the IRQC block) which sits in between the > > GPIO and GIC. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > I don't know if I'm too tired or reading it wrong, but it seems you > went through the trouble of making it possible to override .free() in > the irqdomain in patch 3/5 and yet not using it in this patch 5/5? > I think you missed it, free callback is overridden with rzg2l_gpio_irq_domain_free(). [0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220523174238.28942-6-prabhakar.mahadev-lad.rj@bp.renesas.com/ Cheers, Prabhakar
Hi Linus, Thank you for the feedback. On Tue, May 24, 2022 at 9:54 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, May 23, 2022 at 7:43 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > Allow free() callback to be overridden from irq_domain_ops for > > hierarchical chips. > > > > This allows drivers to free up resources which are allocated during > > child_to_parent_hwirq()/populate_parent_alloc_arg() callbacks. > > > > On Renesas RZ/G2L platform a bitmap is maintained for TINT slots, a slot > > is allocated in child_to_parent_hwirq() callback which is freed up in free > > callback hence this override. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > So that function today looks like this: > > static void gpiochip_hierarchy_setup_domain_ops(struct irq_domain_ops *ops) > { > ops->activate = gpiochip_irq_domain_activate; > ops->deactivate = gpiochip_irq_domain_deactivate; > ops->alloc = gpiochip_hierarchy_irq_domain_alloc; > ops->free = irq_domain_free_irqs_common; > > /* > * We only allow overriding the translate() function for > * hierarchical chips, and this should only be done if the user > * really need something other than 1:1 translation. > */ > if (!ops->translate) > ops->translate = gpiochip_hierarchy_irq_domain_translate; > } > > (...) > - ops->free = irq_domain_free_irqs_common; > (...) > > + if (!ops->free) > > + ops->free = irq_domain_free_irqs_common; > > Marc Z is working on cleaning up the way that gpiolib is (ab)using > irqchips. We definitely need his ACK if we do things like this. > This doesn't look like one of the big offenders to me, but I want > to make sure we don't create new problems while Marc is trying > to solve the old ones. > Agreed, I had a discussion with Marc on v3 series [0]. [0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220511183210.5248-4-prabhakar.mahadev-lad.rj@bp.renesas.com/ Cheers, Prabhakar
On Tue, May 24, 2022 at 11:01 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Tue, May 24, 2022 at 9:57 AM Linus Walleij <linus.walleij@linaro.org> wrote:> > > > On Mon, May 23, 2022 at 7:43 PM Lad Prabhakar > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > Add IRQ domain to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > > > used as IRQ lines at a given time. Selection of pins as IRQ lines > > > is handled by IA55 (which is the IRQC block) which sits in between the > > > GPIO and GIC. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > I don't know if I'm too tired or reading it wrong, but it seems you > > went through the trouble of making it possible to override .free() in > > the irqdomain in patch 3/5 and yet not using it in this patch 5/5? > > > I think you missed it, free callback is overridden with > rzg2l_gpio_irq_domain_free(). > > [0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220523174238.28942-6-prabhakar.mahadev-lad.rj@bp.renesas.com/ Yeah my bad, can't read properly today :/ Why is it necessary to do this stuff in the irqdomain rather than in the irqchip? Especially this: + bitmap_release_region(pctrl->tint_slot, i, get_order(1)); Since the idea with irq_domain is to translate physical (hardware) IRQs to Linux IRQ numbers, I don't see how this is related to that. To me it seems you have taken the usecase that is normally in irqchip and moved it to irqdomain. To me this seems much more like a job that needs to happen in the irqchip .irq_enable()/.irq_disable() pair, and which we have done before in Hans Verkuils patch series: 461c1a7d4733 gpiolib: override irq_enable/disable 4e9439ddacea gpiolib: add flag to indicate if the irq is disabled ca620f2de153 gliolib: set hooks in gpiochip_set_irq_hooks() This gets used by drivers such as: drivers/media/cec/platform/cec-gpio/cec-gpio.c Where you can see these dynamic calls: static bool cec_gpio_enable_irq(struct cec_adapter *adap) { struct cec_gpio *cec = cec_get_drvdata(adap); enable_irq(cec->cec_irq); return true; } static void cec_gpio_disable_irq(struct cec_adapter *adap) { struct cec_gpio *cec = cec_get_drvdata(adap); disable_irq(cec->cec_irq); } Which end up calling .irq_enable()/.irq_disable() on the irq_chip dynamically enabling/disabling the irq. If you prefer to have this done in process context up front when the irq is requested/released then irq_chip also have these callbacks: int (*irq_request_resources)(struct irq_data *data); void (*irq_release_resources)(struct irq_data *data); So I would think over the usecase here a bit. Why does this have to be in the irqdomain? Yours, Linus Walleij
On Tue, May 24, 2022 at 11:07 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Tue, May 24, 2022 at 9:54 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Mon, May 23, 2022 at 7:43 PM Lad Prabhakar > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > Allow free() callback to be overridden from irq_domain_ops for > > > hierarchical chips. > > > > > > This allows drivers to free up resources which are allocated during > > > child_to_parent_hwirq()/populate_parent_alloc_arg() callbacks. > > > > > > On Renesas RZ/G2L platform a bitmap is maintained for TINT slots, a slot > > > is allocated in child_to_parent_hwirq() callback which is freed up in free > > > callback hence this override. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > So that function today looks like this: > > > > static void gpiochip_hierarchy_setup_domain_ops(struct irq_domain_ops *ops) > > { > > ops->activate = gpiochip_irq_domain_activate; > > ops->deactivate = gpiochip_irq_domain_deactivate; > > ops->alloc = gpiochip_hierarchy_irq_domain_alloc; > > ops->free = irq_domain_free_irqs_common; > > > > /* > > * We only allow overriding the translate() function for > > * hierarchical chips, and this should only be done if the user > > * really need something other than 1:1 translation. > > */ > > if (!ops->translate) > > ops->translate = gpiochip_hierarchy_irq_domain_translate; > > } > > > > (...) > > - ops->free = irq_domain_free_irqs_common; > > (...) > > > + if (!ops->free) > > > + ops->free = irq_domain_free_irqs_common; > > > > Marc Z is working on cleaning up the way that gpiolib is (ab)using > > irqchips. We definitely need his ACK if we do things like this. > > This doesn't look like one of the big offenders to me, but I want > > to make sure we don't create new problems while Marc is trying > > to solve the old ones. > > > Agreed, I had a discussion with Marc on v3 series [0]. Hm yeah I guess I am just stepping on Marc's toes with all my mails :( I'll try to just wait for Marc's Reviewed-by instead and not add to the noise, I'm probably just wrong. Yours, Linus Walleij
On Tue, May 24, 2022 at 12:21 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > Since currently there is no other way to release the resources which are allocated in > child_to_parent_hwirq()/populate_parent_alloc_arg() callbacks the free callback is > overridden and the bitmap is released there. > > Let me know if there is any other better approach on handling the above and if it > can be moved into irqchip instead of irqdomain. I have no idea, as long as Marc Z ACKs the patch, it is good for me, the irqchip intrinsics has burnt me more than once. (Says the guy who constructed pin control, yeah I know my code isn't better!) Yours, Linus Walleij
Hi Marc On Mon, May 23, 2022 at 6:42 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. > > Cheers, > Prabhakar > > Changes for v4->v5: > * Updated commit message for patch 3/5 > * Dropped interrupt-parent from and included RB tag from Geert for patch 4/5 > * Implemented init_valid_mask() callback > * Dropped ngirq patch from previous series > * Dropped patches 4/7 and 5/7 from previous patch series will handle it separately. > > Changes for v3->v4: > * Updated description for interrupts-cells property in patch #1 > * Dropped the patch which overriding free callback in gpiolib > * Used devm helpers in patch#2 > * Patch #4, #5 and #6 are newly added > * In patch #7 dropped using gpio offset as hwirq > * Implemented immutable GPIO in patch #7 > * Implemented child_offset_to_irq() callback in patch #7 > > Changes for v2->v3: > * Updated description for interrupts-cells property in patch #1 > * Included RB tag from Geert for binding patch > * Fixed review comments pointed by Geert, Biju and Sergei. > > Changes for v1->v2: > * Included RB tag from Rob > * Fixed review comments pointed by Geert > * included GPIO driver changes > > Changes for RFCV4 -> V1: > * Used unevaluatedProperties. > * Altered the sequence of reg property > * Set the parent type > * Used raw_spin_lock() instead of raw_spin_lock_irqsave() > * Simplified parsing IRQ map. > * Will send the GPIO and pinctrl changes as part of separate series > > Changes for RFC v4: > * Used locking while RMW > * Now using interrupts property instead of interrupt-map > * Patch series depends on [0] > * Updated binding doc > * Fixed comments pointed by Andy > > [0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/ > 20220316200633.28974-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/ > > Changes for RFC v3: > -> Re-structured the driver as a hierarchical irq domain instead of chained > -> made use of IRQCHIP_* macros > -> dropped locking > -> Added support for IRQ0-7 interrupts > -> Introduced 2 new patches for GPIOLIB > -> Switched to using GPIOLIB for irqdomains in pinctrl > > RFC v2: https://patchwork.kernel.org/project/linux-renesas-soc/cover/ > 20210921193028.13099-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/ > > RFC v1: https://patchwork.kernel.org/project/linux-renesas-soc/cover/ > 20210803175109.1729-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/ > > Lad Prabhakar (5): > dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt > Controller > irqchip: Add RZ/G2L IA55 Interrupt Controller driver > gpio: gpiolib: Allow free() callback to be overridden > dt-bindings: pinctrl: renesas,rzg2l-pinctrl: Document the properties > to handle GPIO IRQ > pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO > interrupt > > .../renesas,rzg2l-irqc.yaml | 133 ++++++ > .../pinctrl/renesas,rzg2l-pinctrl.yaml | 15 + > drivers/gpio/gpiolib.c | 9 +- > drivers/irqchip/Kconfig | 8 + > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-renesas-rzg2l.c | 425 ++++++++++++++++++ > drivers/pinctrl/renesas/pinctrl-rzg2l.c | 236 ++++++++++ > 7 files changed, 824 insertions(+), 3 deletions(-) > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml > create mode 100644 drivers/irqchip/irq-renesas-rzg2l.c > Gentle ping. Are you happy with this series? Cheers, Prabhakar
On Mon, 23 May 2022 18:42:35 +0100, Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Add a driver for the Renesas RZ/G2L Interrupt Controller. > > This supports external pins being used as interrupts. It supports > one line for NMI, 8 external pins and 32 GPIO pins (out of 123) > to be used as IRQ lines. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/irqchip/Kconfig | 8 + > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-renesas-rzg2l.c | 425 ++++++++++++++++++++++++++++ > 3 files changed, 434 insertions(+) > create mode 100644 drivers/irqchip/irq-renesas-rzg2l.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index 15edb9a6fcae..f3d071422f3b 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -242,6 +242,14 @@ config RENESAS_RZA1_IRQC > Enable support for the Renesas RZ/A1 Interrupt Controller, to use up > to 8 external interrupts with configurable sense select. > > +config RENESAS_RZG2L_IRQC > + bool "Renesas RZ/G2L (and alike SoC) IRQC support" if COMPILE_TEST > + select GENERIC_IRQ_CHIP > + select IRQ_DOMAIN_HIERARCHY > + help > + Enable support for the Renesas RZ/G2L (and alike SoC) Interrupt Controller > + for external devices. > + > config SL28CPLD_INTC > bool "Kontron sl28cpld IRQ controller" > depends on MFD_SL28CPLD=y || COMPILE_TEST > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 160a1d8ceaa9..eaa56eec2b23 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -51,6 +51,7 @@ obj-$(CONFIG_RDA_INTC) += irq-rda-intc.o > obj-$(CONFIG_RENESAS_INTC_IRQPIN) += irq-renesas-intc-irqpin.o > obj-$(CONFIG_RENESAS_IRQC) += irq-renesas-irqc.o > obj-$(CONFIG_RENESAS_RZA1_IRQC) += irq-renesas-rza1.o > +obj-$(CONFIG_RENESAS_RZG2L_IRQC) += irq-renesas-rzg2l.o > obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o > obj-$(CONFIG_ARCH_NSPIRE) += irq-zevio.o > obj-$(CONFIG_ARCH_VT8500) += irq-vt8500.o > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c > new file mode 100644 > index 000000000000..a846c6ee11d7 > --- /dev/null > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > @@ -0,0 +1,425 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Renesas RZ/G2L IRQC Driver > + * > + * Copyright (C) 2022 Renesas Electronics Corporation. > + * > + * Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/irqchip.h> > +#include <linux/irqdomain.h> > +#include <linux/of_address.h> > +#include <linux/of_platform.h> > +#include <linux/pm_runtime.h> > +#include <linux/reset.h> > +#include <linux/spinlock.h> > + > +#define IRQC_IRQ_START 1 > +#define IRQC_IRQ_COUNT 8 > +#define IRQC_TINT_START (IRQC_IRQ_START + IRQC_IRQ_COUNT) > +#define IRQC_TINT_COUNT 32 > +#define IRQC_NUM_IRQ (IRQC_TINT_START + IRQC_TINT_COUNT) > + > +#define ISCR 0x10 > +#define IITSR 0x14 > +#define TSCR 0x20 > +#define TITSR0 0x24 > +#define TITSR1 0x28 > +#define TITSR0_MAX_INT 16 > +#define TITSEL_WIDTH 0x2 > +#define TSSR(n) (0x30 + ((n) * 4)) > +#define TIEN BIT(7) > +#define TSSEL_SHIFT(n) (8 * (n)) > +#define TSSEL_MASK GENMASK(7, 0) > +#define IRQ_MASK 0x3 > + > +#define TSSR_OFFSET(n) ((n) % 4) > +#define TSSR_INDEX(n) ((n) / 4) > + > +#define TITSR_TITSEL_EDGE_RISING 0 > +#define TITSR_TITSEL_EDGE_FALLING 1 > +#define TITSR_TITSEL_LEVEL_HIGH 2 > +#define TITSR_TITSEL_LEVEL_LOW 3 > + > +#define IITSR_IITSEL(n, sense) ((sense) << ((n) * 2)) > +#define IITSR_IITSEL_LEVEL_LOW 0 > +#define IITSR_IITSEL_EDGE_FALLING 1 > +#define IITSR_IITSEL_EDGE_RISING 2 > +#define IITSR_IITSEL_EDGE_BOTH 3 > +#define IITSR_IITSEL_MASK(n) IITSR_IITSEL((n), 3) > + > +#define TINT_EXTRACT_HWIRQ(x) FIELD_GET(GENMASK(15, 0), (x)) > +#define TINT_EXTRACT_GPIOINT(x) FIELD_GET(GENMASK(31, 16), (x)) > + > +struct rzg2l_irqc_priv { > + void __iomem *base; > + struct of_phandle_args map[IRQC_NUM_IRQ]; > + raw_spinlock_t lock; > +}; > + > +struct rzg2l_irqc_chip_data { > + int tint; > +}; > + > +static struct rzg2l_irqc_priv *irq_data_to_priv(struct irq_data *data) > +{ > + return data->domain->host_data; > +} > + > +static void rzg2l_irq_eoi(struct irq_data *d) > +{ > + unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_IRQ_START; > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > + u32 bit = BIT(hw_irq); > + u32 reg; > + > + reg = readl_relaxed(priv->base + ISCR); > + if (reg & bit) > + writel_relaxed(reg & ~bit, priv->base + ISCR); > +} > + > +static void rzg2l_tint_eoi(struct irq_data *d) > +{ > + unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_TINT_START; > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > + u32 bit = BIT(hw_irq); > + u32 reg; > + > + reg = readl_relaxed(priv->base + TSCR); > + if (reg & bit) > + writel_relaxed(reg & ~bit, priv->base + TSCR); > +} > + > +static void rzg2l_irqc_eoi(struct irq_data *d) > +{ > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > + unsigned int hw_irq = irqd_to_hwirq(d); > + > + raw_spin_lock(&priv->lock); > + if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT) > + rzg2l_irq_eoi(d); > + else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) > + rzg2l_tint_eoi(d); > + raw_spin_unlock(&priv->lock); > + irq_chip_eoi_parent(d); > +} > + > +static void rzg2l_irqc_irq_disable(struct irq_data *d) > +{ > + unsigned int hw_irq = irqd_to_hwirq(d); > + > + if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) { > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > + u32 offset = hw_irq - IRQC_TINT_START; > + u32 tssr_offset = TSSR_OFFSET(offset); > + u8 tssr_index = TSSR_INDEX(offset); > + u32 reg; > + > + raw_spin_lock(&priv->lock); > + reg = readl_relaxed(priv->base + TSSR(tssr_index)); > + reg &= ~(TSSEL_MASK << tssr_offset); > + writel_relaxed(reg, priv->base + TSSR(tssr_index)); > + raw_spin_unlock(&priv->lock); > + } > + irq_chip_disable_parent(d); > +} > + > +static void rzg2l_irqc_irq_enable(struct irq_data *d) > +{ > + unsigned int hw_irq = irqd_to_hwirq(d); > + > + if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) { > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > + unsigned long chip_data = *(unsigned long *)d->chip_data; > + u32 offset = hw_irq - IRQC_TINT_START; > + u32 tssr_offset = TSSR_OFFSET(offset); > + u8 tssr_index = TSSR_INDEX(offset); > + u32 reg; > + > + raw_spin_lock(&priv->lock); > + reg = readl_relaxed(priv->base + TSSR(tssr_index)); > + reg |= (TIEN | chip_data) << TSSEL_SHIFT(tssr_offset); > + writel_relaxed(reg, priv->base + TSSR(tssr_index)); > + raw_spin_unlock(&priv->lock); > + } > + irq_chip_enable_parent(d); > +} > + > +static int rzg2l_irq_set_type(struct irq_data *d, unsigned int type) > +{ > + unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_IRQ_START; > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > + u16 sense, tmp; > + > + switch (type & IRQ_TYPE_SENSE_MASK) { > + case IRQ_TYPE_LEVEL_LOW: > + sense = IITSR_IITSEL_LEVEL_LOW; > + break; > + > + case IRQ_TYPE_EDGE_FALLING: > + sense = IITSR_IITSEL_EDGE_FALLING; > + break; > + > + case IRQ_TYPE_EDGE_RISING: > + sense = IITSR_IITSEL_EDGE_RISING; > + break; > + > + case IRQ_TYPE_EDGE_BOTH: > + sense = IITSR_IITSEL_EDGE_BOTH; > + break; > + > + default: > + return -EINVAL; > + } > + > + raw_spin_lock(&priv->lock); > + tmp = readl_relaxed(priv->base + IITSR); > + tmp &= ~IITSR_IITSEL_MASK(hw_irq); > + tmp |= IITSR_IITSEL(hw_irq, sense); > + writel_relaxed(tmp, priv->base + IITSR); > + raw_spin_unlock(&priv->lock); > + > + return 0; > +} > + > +static int rzg2l_tint_set_edge(struct irq_data *d, unsigned int type) > +{ > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > + unsigned int hwirq = irqd_to_hwirq(d); > + u32 titseln = hwirq - IRQC_TINT_START; > + u32 offset; > + u8 sense; > + u32 reg; > + > + switch (type & IRQ_TYPE_SENSE_MASK) { > + case IRQ_TYPE_EDGE_RISING: > + sense = TITSR_TITSEL_EDGE_RISING; > + break; > + > + case IRQ_TYPE_EDGE_FALLING: > + sense = TITSR_TITSEL_EDGE_FALLING; > + break; > + > + default: > + return -EINVAL; > + } > + > + offset = TITSR0; > + if (titseln >= TITSR0_MAX_INT) { > + titseln -= TITSR0_MAX_INT; > + offset = TITSR1; > + } > + > + raw_spin_lock(&priv->lock); > + reg = readl_relaxed(priv->base + offset); > + reg &= ~(IRQ_MASK << (titseln * TITSEL_WIDTH)); > + reg |= sense << (titseln * TITSEL_WIDTH); > + writel_relaxed(reg, priv->base + offset); > + raw_spin_unlock(&priv->lock); > + > + return 0; > +} > + > +static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type) > +{ > + unsigned int hw_irq = irqd_to_hwirq(d); > + int ret = -EINVAL; > + > + if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT) > + ret = rzg2l_irq_set_type(d, type); > + else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) > + ret = rzg2l_tint_set_edge(d, type); > + if (ret) > + return ret; > + > + return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH); > +} > + > +static const struct irq_chip irqc_chip = { > + .name = "rzg2l-irqc", > + .irq_eoi = rzg2l_irqc_eoi, > + .irq_mask = irq_chip_mask_parent, > + .irq_unmask = irq_chip_unmask_parent, > + .irq_disable = rzg2l_irqc_irq_disable, > + .irq_enable = rzg2l_irqc_irq_enable, > + .irq_get_irqchip_state = irq_chip_get_parent_state, > + .irq_set_irqchip_state = irq_chip_set_parent_state, > + .irq_retrigger = irq_chip_retrigger_hierarchy, > + .irq_set_type = rzg2l_irqc_set_type, > + .flags = IRQCHIP_MASK_ON_SUSPEND | > + IRQCHIP_SET_TYPE_MASKED | > + IRQCHIP_SKIP_SET_WAKE, > +}; > + > +static int rzg2l_irqc_alloc(struct irq_domain *domain, unsigned int virq, > + unsigned int nr_irqs, void *arg) > +{ > + struct rzg2l_irqc_priv *priv = domain->host_data; > + unsigned long *chip_data = NULL; Why the init to NULL? > + struct irq_fwspec spec; > + irq_hw_number_t hwirq; > + int tint = -EINVAL; > + unsigned int type; > + unsigned int i; > + int ret; > + > + ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type); > + if (ret) > + return ret; > + > + /* > + * For TINT interrupts ie where pinctrl driver is child of irqc domain > + * the hwirq and TINT are encoded in fwspec->param[0]. > + * hwirq for TINT range from 9-40, hwirq is embedded 0-15 bits and TINT > + * from 16-31 bits. TINT from the pinctrl driver needs to be programmed > + * in IRQC registers to enable a given gpio pin as interrupt. > + */ > + if (hwirq > IRQC_IRQ_COUNT) { > + tint = TINT_EXTRACT_GPIOINT(hwirq); > + hwirq = TINT_EXTRACT_HWIRQ(hwirq); > + > + if (hwirq < IRQC_TINT_START) > + return -EINVAL; > + } > + > + if (hwirq > (IRQC_NUM_IRQ - 1)) > + return -EINVAL; > + > + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL); Are we really allocating an unsigned long for something that already fits in something that is pointer-sized? > + if (!chip_data) > + return -ENOMEM; > + *chip_data = tint; So here, *chip_data can be set to -EINVAL if hwirq <= IRQC_IRQ_COUNT? This can't be right. > + > + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &irqc_chip, > + chip_data); > + if (ret) { > + kfree(chip_data); > + return ret; > + } > + > + spec.fwnode = domain->parent->fwnode; > + spec.param_count = priv->map[hwirq].args_count; > + for (i = 0; i < spec.param_count; i++) > + spec.param[i] = priv->map[hwirq].args[i]; Why isn't that simply: spec = priv->map[hwirq]; as this really is the interrupt you want to map to? > + > + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &spec); or even better: ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &priv->map[hwirq]); > + if (ret) > + kfree(chip_data); > + > + return ret; > +} > + > +static void rzg2l_irqc_domain_free(struct irq_domain *domain, unsigned int virq, > + unsigned int nr_irqs) > +{ > + struct irq_data *d; > + > + d = irq_domain_get_irq_data(domain, virq); > + if (d) > + kfree(d->chip_data); > + > + irq_domain_free_irqs_common(domain, virq, nr_irqs); > +} > + > +static const struct irq_domain_ops rzg2l_irqc_domain_ops = { > + .alloc = rzg2l_irqc_alloc, > + .free = rzg2l_irqc_domain_free, > + .translate = irq_domain_translate_twocell, > +}; > + > +static int rzg2l_irqc_parse_map(struct rzg2l_irqc_priv *priv, > + struct device_node *np) > +{ > + unsigned int i; > + int ret; > + > + for (i = 0; i < IRQC_NUM_IRQ; i++) { > + ret = of_irq_parse_one(np, i, &priv->map[i]); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent) > +{ > + struct irq_domain *irq_domain, *parent_domain; > + struct platform_device *pdev; > + struct reset_control *resetn; > + struct rzg2l_irqc_priv *priv; > + int ret; > + > + pdev = of_find_device_by_node(node); > + if (!pdev) > + return -ENODEV; > + > + parent_domain = irq_find_host(parent); > + if (!parent_domain) { > + dev_err(&pdev->dev, "cannot find parent domain\n"); > + return -ENODEV; > + } > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL); > + if (IS_ERR(priv->base)) > + return PTR_ERR(priv->base); > + > + ret = rzg2l_irqc_parse_map(priv, node); > + if (ret) { > + dev_err(&pdev->dev, "cannot parse interrupts: %d\n", ret); > + return ret; > + } > + > + resetn = devm_reset_control_get_exclusive_by_index(&pdev->dev, 0); > + if (IS_ERR(resetn)) > + return IS_ERR(resetn); > + > + ret = reset_control_deassert(resetn); > + if (ret) { > + dev_err(&pdev->dev, "failed to deassert resetn pin, %d\n", ret); > + return ret; > + } > + > + pm_runtime_enable(&pdev->dev); > + ret = pm_runtime_resume_and_get(&pdev->dev); > + if (ret < 0) { > + dev_err(&pdev->dev, "pm_runtime_resume_and_get failed: %d\n", ret); > + goto pm_disable; > + } If using runtime PM, why isn't the core IRQ code made aware of this dependency by registering the device with irq_domain_set_pm_device() instead of leaving it enabled forever? > + > + raw_spin_lock_init(&priv->lock); > + > + irq_domain = irq_domain_add_hierarchy(parent_domain, 0, IRQC_NUM_IRQ, > + node, &rzg2l_irqc_domain_ops, > + priv); > + if (!irq_domain) { > + dev_err(&pdev->dev, "failed to add irq domain\n"); > + ret = -ENOMEM; > + goto pm_put; > + } > + > + return 0; > + > +pm_put: > + pm_runtime_put(&pdev->dev); > +pm_disable: > + pm_runtime_disable(&pdev->dev); > + reset_control_assert(resetn); > + return ret; > +} > + > +IRQCHIP_PLATFORM_DRIVER_BEGIN(rzg2l_irqc) > +IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_init) > +IRQCHIP_PLATFORM_DRIVER_END(rzg2l_irqc) > +MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>"); > +MODULE_DESCRIPTION("Renesas RZ/G2L IRQC Driver"); > +MODULE_LICENSE("GPL"); M.
Hi Marc, Thank you for the review. On Sat, Jun 25, 2022 at 10:30 AM Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 23 May 2022 18:42:35 +0100, > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > Add a driver for the Renesas RZ/G2L Interrupt Controller. > > > > This supports external pins being used as interrupts. It supports > > one line for NMI, 8 external pins and 32 GPIO pins (out of 123) > > to be used as IRQ lines. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/irqchip/Kconfig | 8 + > > drivers/irqchip/Makefile | 1 + > > drivers/irqchip/irq-renesas-rzg2l.c | 425 ++++++++++++++++++++++++++++ > > 3 files changed, 434 insertions(+) > > create mode 100644 drivers/irqchip/irq-renesas-rzg2l.c > > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > > index 15edb9a6fcae..f3d071422f3b 100644 > > --- a/drivers/irqchip/Kconfig > > +++ b/drivers/irqchip/Kconfig > > @@ -242,6 +242,14 @@ config RENESAS_RZA1_IRQC > > Enable support for the Renesas RZ/A1 Interrupt Controller, to use up > > to 8 external interrupts with configurable sense select. > > > > +config RENESAS_RZG2L_IRQC > > + bool "Renesas RZ/G2L (and alike SoC) IRQC support" if COMPILE_TEST > > + select GENERIC_IRQ_CHIP > > + select IRQ_DOMAIN_HIERARCHY > > + help > > + Enable support for the Renesas RZ/G2L (and alike SoC) Interrupt Controller > > + for external devices. > > + > > config SL28CPLD_INTC > > bool "Kontron sl28cpld IRQ controller" > > depends on MFD_SL28CPLD=y || COMPILE_TEST > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > > index 160a1d8ceaa9..eaa56eec2b23 100644 > > --- a/drivers/irqchip/Makefile > > +++ b/drivers/irqchip/Makefile > > @@ -51,6 +51,7 @@ obj-$(CONFIG_RDA_INTC) += irq-rda-intc.o > > obj-$(CONFIG_RENESAS_INTC_IRQPIN) += irq-renesas-intc-irqpin.o > > obj-$(CONFIG_RENESAS_IRQC) += irq-renesas-irqc.o > > obj-$(CONFIG_RENESAS_RZA1_IRQC) += irq-renesas-rza1.o > > +obj-$(CONFIG_RENESAS_RZG2L_IRQC) += irq-renesas-rzg2l.o > > obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o > > obj-$(CONFIG_ARCH_NSPIRE) += irq-zevio.o > > obj-$(CONFIG_ARCH_VT8500) += irq-vt8500.o > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c > > new file mode 100644 > > index 000000000000..a846c6ee11d7 > > --- /dev/null > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > > @@ -0,0 +1,425 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Renesas RZ/G2L IRQC Driver > > + * > > + * Copyright (C) 2022 Renesas Electronics Corporation. > > + * > > + * Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/clk.h> > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/irqchip.h> > > +#include <linux/irqdomain.h> > > +#include <linux/of_address.h> > > +#include <linux/of_platform.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/reset.h> > > +#include <linux/spinlock.h> > > + > > +#define IRQC_IRQ_START 1 > > +#define IRQC_IRQ_COUNT 8 > > +#define IRQC_TINT_START (IRQC_IRQ_START + IRQC_IRQ_COUNT) > > +#define IRQC_TINT_COUNT 32 > > +#define IRQC_NUM_IRQ (IRQC_TINT_START + IRQC_TINT_COUNT) > > + > > +#define ISCR 0x10 > > +#define IITSR 0x14 > > +#define TSCR 0x20 > > +#define TITSR0 0x24 > > +#define TITSR1 0x28 > > +#define TITSR0_MAX_INT 16 > > +#define TITSEL_WIDTH 0x2 > > +#define TSSR(n) (0x30 + ((n) * 4)) > > +#define TIEN BIT(7) > > +#define TSSEL_SHIFT(n) (8 * (n)) > > +#define TSSEL_MASK GENMASK(7, 0) > > +#define IRQ_MASK 0x3 > > + > > +#define TSSR_OFFSET(n) ((n) % 4) > > +#define TSSR_INDEX(n) ((n) / 4) > > + > > +#define TITSR_TITSEL_EDGE_RISING 0 > > +#define TITSR_TITSEL_EDGE_FALLING 1 > > +#define TITSR_TITSEL_LEVEL_HIGH 2 > > +#define TITSR_TITSEL_LEVEL_LOW 3 > > + > > +#define IITSR_IITSEL(n, sense) ((sense) << ((n) * 2)) > > +#define IITSR_IITSEL_LEVEL_LOW 0 > > +#define IITSR_IITSEL_EDGE_FALLING 1 > > +#define IITSR_IITSEL_EDGE_RISING 2 > > +#define IITSR_IITSEL_EDGE_BOTH 3 > > +#define IITSR_IITSEL_MASK(n) IITSR_IITSEL((n), 3) > > + > > +#define TINT_EXTRACT_HWIRQ(x) FIELD_GET(GENMASK(15, 0), (x)) > > +#define TINT_EXTRACT_GPIOINT(x) FIELD_GET(GENMASK(31, 16), (x)) > > + > > +struct rzg2l_irqc_priv { > > + void __iomem *base; > > + struct of_phandle_args map[IRQC_NUM_IRQ]; > > + raw_spinlock_t lock; > > +}; > > + > > +struct rzg2l_irqc_chip_data { > > + int tint; > > +}; > > + > > +static struct rzg2l_irqc_priv *irq_data_to_priv(struct irq_data *data) > > +{ > > + return data->domain->host_data; > > +} > > + > > +static void rzg2l_irq_eoi(struct irq_data *d) > > +{ > > + unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_IRQ_START; > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > + u32 bit = BIT(hw_irq); > > + u32 reg; > > + > > + reg = readl_relaxed(priv->base + ISCR); > > + if (reg & bit) > > + writel_relaxed(reg & ~bit, priv->base + ISCR); > > +} > > + > > +static void rzg2l_tint_eoi(struct irq_data *d) > > +{ > > + unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_TINT_START; > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > + u32 bit = BIT(hw_irq); > > + u32 reg; > > + > > + reg = readl_relaxed(priv->base + TSCR); > > + if (reg & bit) > > + writel_relaxed(reg & ~bit, priv->base + TSCR); > > +} > > + > > +static void rzg2l_irqc_eoi(struct irq_data *d) > > +{ > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > + unsigned int hw_irq = irqd_to_hwirq(d); > > + > > + raw_spin_lock(&priv->lock); > > + if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT) > > + rzg2l_irq_eoi(d); > > + else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) > > + rzg2l_tint_eoi(d); > > + raw_spin_unlock(&priv->lock); > > + irq_chip_eoi_parent(d); > > +} > > + > > +static void rzg2l_irqc_irq_disable(struct irq_data *d) > > +{ > > + unsigned int hw_irq = irqd_to_hwirq(d); > > + > > + if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) { > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > + u32 offset = hw_irq - IRQC_TINT_START; > > + u32 tssr_offset = TSSR_OFFSET(offset); > > + u8 tssr_index = TSSR_INDEX(offset); > > + u32 reg; > > + > > + raw_spin_lock(&priv->lock); > > + reg = readl_relaxed(priv->base + TSSR(tssr_index)); > > + reg &= ~(TSSEL_MASK << tssr_offset); > > + writel_relaxed(reg, priv->base + TSSR(tssr_index)); > > + raw_spin_unlock(&priv->lock); > > + } > > + irq_chip_disable_parent(d); > > +} > > + > > +static void rzg2l_irqc_irq_enable(struct irq_data *d) > > +{ > > + unsigned int hw_irq = irqd_to_hwirq(d); > > + > > + if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) { > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > + unsigned long chip_data = *(unsigned long *)d->chip_data; > > + u32 offset = hw_irq - IRQC_TINT_START; > > + u32 tssr_offset = TSSR_OFFSET(offset); > > + u8 tssr_index = TSSR_INDEX(offset); > > + u32 reg; > > + > > + raw_spin_lock(&priv->lock); > > + reg = readl_relaxed(priv->base + TSSR(tssr_index)); > > + reg |= (TIEN | chip_data) << TSSEL_SHIFT(tssr_offset); > > + writel_relaxed(reg, priv->base + TSSR(tssr_index)); > > + raw_spin_unlock(&priv->lock); > > + } > > + irq_chip_enable_parent(d); > > +} > > + > > +static int rzg2l_irq_set_type(struct irq_data *d, unsigned int type) > > +{ > > + unsigned int hw_irq = irqd_to_hwirq(d) - IRQC_IRQ_START; > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > + u16 sense, tmp; > > + > > + switch (type & IRQ_TYPE_SENSE_MASK) { > > + case IRQ_TYPE_LEVEL_LOW: > > + sense = IITSR_IITSEL_LEVEL_LOW; > > + break; > > + > > + case IRQ_TYPE_EDGE_FALLING: > > + sense = IITSR_IITSEL_EDGE_FALLING; > > + break; > > + > > + case IRQ_TYPE_EDGE_RISING: > > + sense = IITSR_IITSEL_EDGE_RISING; > > + break; > > + > > + case IRQ_TYPE_EDGE_BOTH: > > + sense = IITSR_IITSEL_EDGE_BOTH; > > + break; > > + > > + default: > > + return -EINVAL; > > + } > > + > > + raw_spin_lock(&priv->lock); > > + tmp = readl_relaxed(priv->base + IITSR); > > + tmp &= ~IITSR_IITSEL_MASK(hw_irq); > > + tmp |= IITSR_IITSEL(hw_irq, sense); > > + writel_relaxed(tmp, priv->base + IITSR); > > + raw_spin_unlock(&priv->lock); > > + > > + return 0; > > +} > > + > > +static int rzg2l_tint_set_edge(struct irq_data *d, unsigned int type) > > +{ > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > + unsigned int hwirq = irqd_to_hwirq(d); > > + u32 titseln = hwirq - IRQC_TINT_START; > > + u32 offset; > > + u8 sense; > > + u32 reg; > > + > > + switch (type & IRQ_TYPE_SENSE_MASK) { > > + case IRQ_TYPE_EDGE_RISING: > > + sense = TITSR_TITSEL_EDGE_RISING; > > + break; > > + > > + case IRQ_TYPE_EDGE_FALLING: > > + sense = TITSR_TITSEL_EDGE_FALLING; > > + break; > > + > > + default: > > + return -EINVAL; > > + } > > + > > + offset = TITSR0; > > + if (titseln >= TITSR0_MAX_INT) { > > + titseln -= TITSR0_MAX_INT; > > + offset = TITSR1; > > + } > > + > > + raw_spin_lock(&priv->lock); > > + reg = readl_relaxed(priv->base + offset); > > + reg &= ~(IRQ_MASK << (titseln * TITSEL_WIDTH)); > > + reg |= sense << (titseln * TITSEL_WIDTH); > > + writel_relaxed(reg, priv->base + offset); > > + raw_spin_unlock(&priv->lock); > > + > > + return 0; > > +} > > + > > +static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type) > > +{ > > + unsigned int hw_irq = irqd_to_hwirq(d); > > + int ret = -EINVAL; > > + > > + if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT) > > + ret = rzg2l_irq_set_type(d, type); > > + else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) > > + ret = rzg2l_tint_set_edge(d, type); > > + if (ret) > > + return ret; > > + > > + return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH); > > +} > > + > > +static const struct irq_chip irqc_chip = { > > + .name = "rzg2l-irqc", > > + .irq_eoi = rzg2l_irqc_eoi, > > + .irq_mask = irq_chip_mask_parent, > > + .irq_unmask = irq_chip_unmask_parent, > > + .irq_disable = rzg2l_irqc_irq_disable, > > + .irq_enable = rzg2l_irqc_irq_enable, > > + .irq_get_irqchip_state = irq_chip_get_parent_state, > > + .irq_set_irqchip_state = irq_chip_set_parent_state, > > + .irq_retrigger = irq_chip_retrigger_hierarchy, > > + .irq_set_type = rzg2l_irqc_set_type, > > + .flags = IRQCHIP_MASK_ON_SUSPEND | > > + IRQCHIP_SET_TYPE_MASKED | > > + IRQCHIP_SKIP_SET_WAKE, > > +}; > > + > > +static int rzg2l_irqc_alloc(struct irq_domain *domain, unsigned int virq, > > + unsigned int nr_irqs, void *arg) > > +{ > > + struct rzg2l_irqc_priv *priv = domain->host_data; > > + unsigned long *chip_data = NULL; > > Why the init to NULL? > Can be dropped. > > + struct irq_fwspec spec; > > + irq_hw_number_t hwirq; > > + int tint = -EINVAL; > > + unsigned int type; > > + unsigned int i; > > + int ret; > > + > > + ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type); > > + if (ret) > > + return ret; > > + > > + /* > > + * For TINT interrupts ie where pinctrl driver is child of irqc domain > > + * the hwirq and TINT are encoded in fwspec->param[0]. > > + * hwirq for TINT range from 9-40, hwirq is embedded 0-15 bits and TINT > > + * from 16-31 bits. TINT from the pinctrl driver needs to be programmed > > + * in IRQC registers to enable a given gpio pin as interrupt. > > + */ > > + if (hwirq > IRQC_IRQ_COUNT) { > > + tint = TINT_EXTRACT_GPIOINT(hwirq); > > + hwirq = TINT_EXTRACT_HWIRQ(hwirq); > > + > > + if (hwirq < IRQC_TINT_START) > > + return -EINVAL; > > + } > > + > > + if (hwirq > (IRQC_NUM_IRQ - 1)) > > + return -EINVAL; > > + > > + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL); > > Are we really allocating an unsigned long for something that already > fits in something that is pointer-sized? > I think I received some feedback to use unsigned long. Let me know what you want me to use here. > > + if (!chip_data) > > + return -ENOMEM; > > + *chip_data = tint; > > So here, *chip_data can be set to -EINVAL if hwirq <= IRQC_IRQ_COUNT? > This can't be right. > Yes *chip_data can be -EINVAL. IRQC block handles IRQ0-7 and GPIOINT0-122. So the -EINVAL here is for IRQ0-7 case were dont required the chip data in the call backs hence -EINVAL, Whereas for GPIOINT0-122 we need chip_data in the callbacks as this value needs to be programmed in the hardware registers. > > + > > + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &irqc_chip, > > + chip_data); > > + if (ret) { > > + kfree(chip_data); > > + return ret; > > + } > > + > > + spec.fwnode = domain->parent->fwnode; > > + spec.param_count = priv->map[hwirq].args_count; > > + for (i = 0; i < spec.param_count; i++) > > + spec.param[i] = priv->map[hwirq].args[i]; > > Why isn't that simply: > > spec = priv->map[hwirq]; > spec is of type ‘struct irq_fwspec’ and map is of type ‘struct of_phandle_args’. > as this really is the interrupt you want to map to? > Yes. > > + > > + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &spec); > > or even better: > > ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, > &priv->map[hwirq]); > Does not work as map is of type ‘struct of_phandle_args’. > > + if (ret) > > + kfree(chip_data); > > + > > + return ret; > > +} > > + > > +static void rzg2l_irqc_domain_free(struct irq_domain *domain, unsigned int virq, > > + unsigned int nr_irqs) > > +{ > > + struct irq_data *d; > > + > > + d = irq_domain_get_irq_data(domain, virq); > > + if (d) > > + kfree(d->chip_data); > > + > > + irq_domain_free_irqs_common(domain, virq, nr_irqs); > > +} > > + > > +static const struct irq_domain_ops rzg2l_irqc_domain_ops = { > > + .alloc = rzg2l_irqc_alloc, > > + .free = rzg2l_irqc_domain_free, > > + .translate = irq_domain_translate_twocell, > > +}; > > + > > +static int rzg2l_irqc_parse_map(struct rzg2l_irqc_priv *priv, > > + struct device_node *np) > > +{ > > + unsigned int i; > > + int ret; > > + > > + for (i = 0; i < IRQC_NUM_IRQ; i++) { > > + ret = of_irq_parse_one(np, i, &priv->map[i]); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent) > > +{ > > + struct irq_domain *irq_domain, *parent_domain; > > + struct platform_device *pdev; > > + struct reset_control *resetn; > > + struct rzg2l_irqc_priv *priv; > > + int ret; > > + > > + pdev = of_find_device_by_node(node); > > + if (!pdev) > > + return -ENODEV; > > + > > + parent_domain = irq_find_host(parent); > > + if (!parent_domain) { > > + dev_err(&pdev->dev, "cannot find parent domain\n"); > > + return -ENODEV; > > + } > > + > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL); > > + if (IS_ERR(priv->base)) > > + return PTR_ERR(priv->base); > > + > > + ret = rzg2l_irqc_parse_map(priv, node); > > + if (ret) { > > + dev_err(&pdev->dev, "cannot parse interrupts: %d\n", ret); > > + return ret; > > + } > > + > > + resetn = devm_reset_control_get_exclusive_by_index(&pdev->dev, 0); > > + if (IS_ERR(resetn)) > > + return IS_ERR(resetn); > > + > > + ret = reset_control_deassert(resetn); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to deassert resetn pin, %d\n", ret); > > + return ret; > > + } > > + > > + pm_runtime_enable(&pdev->dev); > > + ret = pm_runtime_resume_and_get(&pdev->dev); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "pm_runtime_resume_and_get failed: %d\n", ret); > > + goto pm_disable; > > + } > > If using runtime PM, why isn't the core IRQ code made aware of this > dependency by registering the device with irq_domain_set_pm_device() > instead of leaving it enabled forever? > Ouch will add irq_domain_set_pm_device() below. Cheers, Prabhakar
On Sat, 25 Jun 2022 11:54:44 +0100, "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > Hi Marc, > > Thank you for the review. > > On Sat, Jun 25, 2022 at 10:30 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Mon, 23 May 2022 18:42:35 +0100, > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > [...] > > > +static int rzg2l_irqc_alloc(struct irq_domain *domain, unsigned int virq, > > > + unsigned int nr_irqs, void *arg) > > > +{ > > > + struct rzg2l_irqc_priv *priv = domain->host_data; > > > + unsigned long *chip_data = NULL; > > > > Why the init to NULL? > > > Can be dropped. > > > > + struct irq_fwspec spec; > > > + irq_hw_number_t hwirq; > > > + int tint = -EINVAL; > > > + unsigned int type; > > > + unsigned int i; > > > + int ret; > > > + > > > + ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type); > > > + if (ret) > > > + return ret; > > > + > > > + /* > > > + * For TINT interrupts ie where pinctrl driver is child of irqc domain > > > + * the hwirq and TINT are encoded in fwspec->param[0]. > > > + * hwirq for TINT range from 9-40, hwirq is embedded 0-15 bits and TINT > > > + * from 16-31 bits. TINT from the pinctrl driver needs to be programmed > > > + * in IRQC registers to enable a given gpio pin as interrupt. > > > + */ > > > + if (hwirq > IRQC_IRQ_COUNT) { > > > + tint = TINT_EXTRACT_GPIOINT(hwirq); > > > + hwirq = TINT_EXTRACT_HWIRQ(hwirq); > > > + > > > + if (hwirq < IRQC_TINT_START) > > > + return -EINVAL; > > > + } > > > + > > > + if (hwirq > (IRQC_NUM_IRQ - 1)) > > > + return -EINVAL; > > > + > > > + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL); > > > > Are we really allocating an unsigned long for something that already > > fits in something that is pointer-sized? > > > I think I received some feedback to use unsigned long. Let me know > what you want me to use here. I think this is just a waste of memory, but I don't really care. > > > > + if (!chip_data) > > > + return -ENOMEM; > > > + *chip_data = tint; > > > > So here, *chip_data can be set to -EINVAL if hwirq <= IRQC_IRQ_COUNT? > > This can't be right. > > > Yes *chip_data can be -EINVAL. IRQC block handles IRQ0-7 and > GPIOINT0-122. So the -EINVAL here is for IRQ0-7 case were dont > required the chip data in the call backs hence -EINVAL, Whereas for > GPIOINT0-122 we need chip_data in the callbacks as this value needs to > be programmed in the hardware registers. I can't see anything that checks it (let alone the difference in types). And if it isn't checked, this means that the allocation is pointless. > > > > + > > > + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &irqc_chip, > > > + chip_data); > > > + if (ret) { > > > + kfree(chip_data); > > > + return ret; > > > + } > > > + > > > + spec.fwnode = domain->parent->fwnode; > > > + spec.param_count = priv->map[hwirq].args_count; > > > + for (i = 0; i < spec.param_count; i++) > > > + spec.param[i] = priv->map[hwirq].args[i]; > > > > Why isn't that simply: > > > > spec = priv->map[hwirq]; > > > spec is of type ‘struct irq_fwspec’ and map is of type ‘struct of_phandle_args’. > > > as this really is the interrupt you want to map to? > > > Yes. > > > > + > > > + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &spec); > > > > or even better: > > > > ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, > > &priv->map[hwirq]); > > > Does not work as map is of type ‘struct of_phandle_args’. Which begs the question: why don't you convert it to an irq_fwspec the first place and be done with it? > > > > + if (ret) > > > + kfree(chip_data); > > > + > > > + return ret; > > > +} > > > + > > > +static void rzg2l_irqc_domain_free(struct irq_domain *domain, unsigned int virq, > > > + unsigned int nr_irqs) > > > +{ > > > + struct irq_data *d; > > > + > > > + d = irq_domain_get_irq_data(domain, virq); > > > + if (d) > > > + kfree(d->chip_data); > > > + > > > + irq_domain_free_irqs_common(domain, virq, nr_irqs); > > > +} > > > + > > > +static const struct irq_domain_ops rzg2l_irqc_domain_ops = { > > > + .alloc = rzg2l_irqc_alloc, > > > + .free = rzg2l_irqc_domain_free, > > > + .translate = irq_domain_translate_twocell, > > > +}; > > > + > > > +static int rzg2l_irqc_parse_map(struct rzg2l_irqc_priv *priv, > > > + struct device_node *np) nit: this function could afford being renamed to something more correct. It really doesn't map anything, only retrieves the output interrupts. > > > +{ > > > + unsigned int i; > > > + int ret; > > > + > > > + for (i = 0; i < IRQC_NUM_IRQ; i++) { > > > + ret = of_irq_parse_one(np, i, &priv->map[i]); Make map an array of irq_fwspec, and use of_phandle_args_to_fwspec() for the conversion. > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent) > > > +{ > > > + struct irq_domain *irq_domain, *parent_domain; > > > + struct platform_device *pdev; > > > + struct reset_control *resetn; > > > + struct rzg2l_irqc_priv *priv; > > > + int ret; > > > + > > > + pdev = of_find_device_by_node(node); > > > + if (!pdev) > > > + return -ENODEV; > > > + > > > + parent_domain = irq_find_host(parent); > > > + if (!parent_domain) { > > > + dev_err(&pdev->dev, "cannot find parent domain\n"); > > > + return -ENODEV; > > > + } > > > + > > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > > + if (!priv) > > > + return -ENOMEM; > > > + > > > + priv->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL); > > > + if (IS_ERR(priv->base)) > > > + return PTR_ERR(priv->base); > > > + > > > + ret = rzg2l_irqc_parse_map(priv, node); > > > + if (ret) { > > > + dev_err(&pdev->dev, "cannot parse interrupts: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + resetn = devm_reset_control_get_exclusive_by_index(&pdev->dev, 0); > > > + if (IS_ERR(resetn)) > > > + return IS_ERR(resetn); > > > + > > > + ret = reset_control_deassert(resetn); > > > + if (ret) { > > > + dev_err(&pdev->dev, "failed to deassert resetn pin, %d\n", ret); > > > + return ret; > > > + } > > > + > > > + pm_runtime_enable(&pdev->dev); > > > + ret = pm_runtime_resume_and_get(&pdev->dev); > > > + if (ret < 0) { > > > + dev_err(&pdev->dev, "pm_runtime_resume_and_get failed: %d\n", ret); > > > + goto pm_disable; > > > + } > > > > If using runtime PM, why isn't the core IRQ code made aware of this > > dependency by registering the device with irq_domain_set_pm_device() > > instead of leaving it enabled forever? > > > Ouch will add irq_domain_set_pm_device() below. You'll need a bit more than that. You'll either need to take a PM reference on each alloc, or improve irq_chip_pm_{get,put}() to talk the hierarchy. That's probably a separate patch. M.
Hi Marc, On Sat, Jun 25, 2022 at 1:08 PM Marc Zyngier <maz@kernel.org> wrote: > > On Sat, 25 Jun 2022 11:54:44 +0100, > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > Hi Marc, > > > > Thank you for the review. > > > > On Sat, Jun 25, 2022 at 10:30 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Mon, 23 May 2022 18:42:35 +0100, > > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > [...] > > > > > +static int rzg2l_irqc_alloc(struct irq_domain *domain, unsigned int virq, > > > > + unsigned int nr_irqs, void *arg) > > > > +{ > > > > + struct rzg2l_irqc_priv *priv = domain->host_data; > > > > + unsigned long *chip_data = NULL; > > > > > > Why the init to NULL? > > > > > Can be dropped. > > > > > > + struct irq_fwspec spec; > > > > + irq_hw_number_t hwirq; > > > > + int tint = -EINVAL; > > > > + unsigned int type; > > > > + unsigned int i; > > > > + int ret; > > > > + > > > > + ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* > > > > + * For TINT interrupts ie where pinctrl driver is child of irqc domain > > > > + * the hwirq and TINT are encoded in fwspec->param[0]. > > > > + * hwirq for TINT range from 9-40, hwirq is embedded 0-15 bits and TINT > > > > + * from 16-31 bits. TINT from the pinctrl driver needs to be programmed > > > > + * in IRQC registers to enable a given gpio pin as interrupt. > > > > + */ > > > > + if (hwirq > IRQC_IRQ_COUNT) { > > > > + tint = TINT_EXTRACT_GPIOINT(hwirq); > > > > + hwirq = TINT_EXTRACT_HWIRQ(hwirq); > > > > + > > > > + if (hwirq < IRQC_TINT_START) > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (hwirq > (IRQC_NUM_IRQ - 1)) > > > > + return -EINVAL; > > > > + > > > > + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL); > > > > > > Are we really allocating an unsigned long for something that already > > > fits in something that is pointer-sized? > > > > > I think I received some feedback to use unsigned long. Let me know > > what you want me to use here. > > I think this is just a waste of memory, but I don't really care. > Is there any better way I can handle it? > > > > > > + if (!chip_data) > > > > + return -ENOMEM; > > > > + *chip_data = tint; > > > > > > So here, *chip_data can be set to -EINVAL if hwirq <= IRQC_IRQ_COUNT? > > > This can't be right. > > > > > Yes *chip_data can be -EINVAL. IRQC block handles IRQ0-7 and > > GPIOINT0-122. So the -EINVAL here is for IRQ0-7 case were dont > > required the chip data in the call backs hence -EINVAL, Whereas for > > GPIOINT0-122 we need chip_data in the callbacks as this value needs to > > be programmed in the hardware registers. > > I can't see anything that checks it (let alone the difference in > types). And if it isn't checked, this means that the allocation is > pointless. > There are checks for example below: static void rzg2l_irqc_irq_enable(struct irq_data *d) { unsigned int hw_irq = irqd_to_hwirq(d); if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) { struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); unsigned long chip_data = *(unsigned long *)d->chip_data; u32 offset = hw_irq - IRQC_TINT_START; u32 tssr_offset = TSSR_OFFSET(offset); u8 tssr_index = TSSR_INDEX(offset); u32 reg; raw_spin_lock(&priv->lock); reg = readl_relaxed(priv->base + TSSR(tssr_index)); reg |= (TIEN | chip_data) << TSSEL_SHIFT(tssr_offset); writel_relaxed(reg, priv->base + TSSR(tssr_index)); raw_spin_unlock(&priv->lock); } irq_chip_enable_parent(d); } This check hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ here would mean its GPIOINT0-122 and then the chip data will be used. > > > > > > + > > > > + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &irqc_chip, > > > > + chip_data); > > > > + if (ret) { > > > > + kfree(chip_data); > > > > + return ret; > > > > + } > > > > + > > > > + spec.fwnode = domain->parent->fwnode; > > > > + spec.param_count = priv->map[hwirq].args_count; > > > > + for (i = 0; i < spec.param_count; i++) > > > > + spec.param[i] = priv->map[hwirq].args[i]; > > > > > > Why isn't that simply: > > > > > > spec = priv->map[hwirq]; > > > > > spec is of type ‘struct irq_fwspec’ and map is of type ‘struct of_phandle_args’. > > > > > as this really is the interrupt you want to map to? > > > > > Yes. > > > > > > + > > > > + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &spec); > > > > > > or even better: > > > > > > ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, > > > &priv->map[hwirq]); > > > > > Does not work as map is of type ‘struct of_phandle_args’. > > Which begs the question: why don't you convert it to an irq_fwspec the > first place and be done with it? > Right.. > > > > > > + if (ret) > > > > + kfree(chip_data); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static void rzg2l_irqc_domain_free(struct irq_domain *domain, unsigned int virq, > > > > + unsigned int nr_irqs) > > > > +{ > > > > + struct irq_data *d; > > > > + > > > > + d = irq_domain_get_irq_data(domain, virq); > > > > + if (d) > > > > + kfree(d->chip_data); > > > > + > > > > + irq_domain_free_irqs_common(domain, virq, nr_irqs); > > > > +} > > > > + > > > > +static const struct irq_domain_ops rzg2l_irqc_domain_ops = { > > > > + .alloc = rzg2l_irqc_alloc, > > > > + .free = rzg2l_irqc_domain_free, > > > > + .translate = irq_domain_translate_twocell, > > > > +}; > > > > + > > > > +static int rzg2l_irqc_parse_map(struct rzg2l_irqc_priv *priv, > > > > + struct device_node *np) > > nit: this function could afford being renamed to something more > correct. It really doesn't map anything, only retrieves the output > interrupts. > Sure will do. > > > > +{ > > > > + unsigned int i; > > > > + int ret; > > > > + > > > > + for (i = 0; i < IRQC_NUM_IRQ; i++) { > > > > + ret = of_irq_parse_one(np, i, &priv->map[i]); > > Make map an array of irq_fwspec, and use of_phandle_args_to_fwspec() > for the conversion. > ... Good point, will do. > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent) > > > > +{ > > > > + struct irq_domain *irq_domain, *parent_domain; > > > > + struct platform_device *pdev; > > > > + struct reset_control *resetn; > > > > + struct rzg2l_irqc_priv *priv; > > > > + int ret; > > > > + > > > > + pdev = of_find_device_by_node(node); > > > > + if (!pdev) > > > > + return -ENODEV; > > > > + > > > > + parent_domain = irq_find_host(parent); > > > > + if (!parent_domain) { > > > > + dev_err(&pdev->dev, "cannot find parent domain\n"); > > > > + return -ENODEV; > > > > + } > > > > + > > > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > > > + if (!priv) > > > > + return -ENOMEM; > > > > + > > > > + priv->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL); > > > > + if (IS_ERR(priv->base)) > > > > + return PTR_ERR(priv->base); > > > > + > > > > + ret = rzg2l_irqc_parse_map(priv, node); > > > > + if (ret) { > > > > + dev_err(&pdev->dev, "cannot parse interrupts: %d\n", ret); > > > > + return ret; > > > > + } > > > > + > > > > + resetn = devm_reset_control_get_exclusive_by_index(&pdev->dev, 0); > > > > + if (IS_ERR(resetn)) > > > > + return IS_ERR(resetn); > > > > + > > > > + ret = reset_control_deassert(resetn); > > > > + if (ret) { > > > > + dev_err(&pdev->dev, "failed to deassert resetn pin, %d\n", ret); > > > > + return ret; > > > > + } > > > > + > > > > + pm_runtime_enable(&pdev->dev); > > > > + ret = pm_runtime_resume_and_get(&pdev->dev); > > > > + if (ret < 0) { > > > > + dev_err(&pdev->dev, "pm_runtime_resume_and_get failed: %d\n", ret); > > > > + goto pm_disable; > > > > + } > > > > > > If using runtime PM, why isn't the core IRQ code made aware of this > > > dependency by registering the device with irq_domain_set_pm_device() > > > instead of leaving it enabled forever? > > > > > Ouch will add irq_domain_set_pm_device() below. > > You'll need a bit more than that. You'll either need to take a PM > reference on each alloc, or improve irq_chip_pm_{get,put}() to talk > the hierarchy. > Aha I see. > That's probably a separate patch. > Agreed will make it a separate patch, once the driver gets in. Cheers, Prabhakar
On Sat, 25 Jun 2022 13:48:08 +0100, "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > Hi Marc, > > On Sat, Jun 25, 2022 at 1:08 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Sat, 25 Jun 2022 11:54:44 +0100, > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > Hi Marc, > > > > > > Thank you for the review. > > > > > > On Sat, Jun 25, 2022 at 10:30 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Mon, 23 May 2022 18:42:35 +0100, > > > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > > > > [...] > > > > > > > +static int rzg2l_irqc_alloc(struct irq_domain *domain, unsigned int virq, > > > > > + unsigned int nr_irqs, void *arg) > > > > > +{ > > > > > + struct rzg2l_irqc_priv *priv = domain->host_data; > > > > > + unsigned long *chip_data = NULL; > > > > > > > > Why the init to NULL? > > > > > > > Can be dropped. > > > > > > > > + struct irq_fwspec spec; > > > > > + irq_hw_number_t hwirq; > > > > > + int tint = -EINVAL; > > > > > + unsigned int type; > > > > > + unsigned int i; > > > > > + int ret; > > > > > + > > > > > + ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + /* > > > > > + * For TINT interrupts ie where pinctrl driver is child of irqc domain > > > > > + * the hwirq and TINT are encoded in fwspec->param[0]. > > > > > + * hwirq for TINT range from 9-40, hwirq is embedded 0-15 bits and TINT > > > > > + * from 16-31 bits. TINT from the pinctrl driver needs to be programmed > > > > > + * in IRQC registers to enable a given gpio pin as interrupt. > > > > > + */ > > > > > + if (hwirq > IRQC_IRQ_COUNT) { > > > > > + tint = TINT_EXTRACT_GPIOINT(hwirq); > > > > > + hwirq = TINT_EXTRACT_HWIRQ(hwirq); > > > > > + > > > > > + if (hwirq < IRQC_TINT_START) > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + if (hwirq > (IRQC_NUM_IRQ - 1)) > > > > > + return -EINVAL; > > > > > + > > > > > + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL); > > > > > > > > Are we really allocating an unsigned long for something that already > > > > fits in something that is pointer-sized? > > > > > > > I think I received some feedback to use unsigned long. Let me know > > > what you want me to use here. > > > > I think this is just a waste of memory, but I don't really care. > > > Is there any better way I can handle it? How about (shock, horror) a cast? > > > > > > > > > + if (!chip_data) > > > > > + return -ENOMEM; > > > > > + *chip_data = tint; > > > > > > > > So here, *chip_data can be set to -EINVAL if hwirq <= IRQC_IRQ_COUNT? > > > > This can't be right. > > > > > > > Yes *chip_data can be -EINVAL. IRQC block handles IRQ0-7 and > > > GPIOINT0-122. So the -EINVAL here is for IRQ0-7 case were dont > > > required the chip data in the call backs hence -EINVAL, Whereas for > > > GPIOINT0-122 we need chip_data in the callbacks as this value needs to > > > be programmed in the hardware registers. > > > > I can't see anything that checks it (let alone the difference in > > types). And if it isn't checked, this means that the allocation is > > pointless. > > > There are checks for example below: > > static void rzg2l_irqc_irq_enable(struct irq_data *d) > { > unsigned int hw_irq = irqd_to_hwirq(d); > > if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) { > struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > unsigned long chip_data = *(unsigned long *)d->chip_data; > u32 offset = hw_irq - IRQC_TINT_START; > u32 tssr_offset = TSSR_OFFSET(offset); > u8 tssr_index = TSSR_INDEX(offset); > u32 reg; > > raw_spin_lock(&priv->lock); > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > reg |= (TIEN | chip_data) << TSSEL_SHIFT(tssr_offset); > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > raw_spin_unlock(&priv->lock); > } > irq_chip_enable_parent(d); > } > > This check hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ here > would mean its GPIOINT0-122 and then the chip data will be used. That doesn't check the content of chip_data if outside of this condition. Nonetheless, you allocate an unsigned long to store -EINVAL. Not only this is a pointless allocation, but you use it to store something that you never retrieve the first place. Don't you see the problem? M.
Hi Marc, On Sat, Jun 25, 2022 at 5:09 PM Marc Zyngier <maz@kernel.org> wrote: > > On Sat, 25 Jun 2022 13:48:08 +0100, > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > Hi Marc, > > > > On Sat, Jun 25, 2022 at 1:08 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Sat, 25 Jun 2022 11:54:44 +0100, > > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > > > Hi Marc, > > > > > > > > Thank you for the review. > > > > > > > > On Sat, Jun 25, 2022 at 10:30 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Mon, 23 May 2022 18:42:35 +0100, > > > > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > > > > > > > [...] > > > > > > > > > +static int rzg2l_irqc_alloc(struct irq_domain *domain, unsigned int virq, > > > > > > + unsigned int nr_irqs, void *arg) > > > > > > +{ > > > > > > + struct rzg2l_irqc_priv *priv = domain->host_data; > > > > > > + unsigned long *chip_data = NULL; > > > > > > > > > > Why the init to NULL? > > > > > > > > > Can be dropped. > > > > > > > > > > + struct irq_fwspec spec; > > > > > > + irq_hw_number_t hwirq; > > > > > > + int tint = -EINVAL; > > > > > > + unsigned int type; > > > > > > + unsigned int i; > > > > > > + int ret; > > > > > > + > > > > > > + ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + /* > > > > > > + * For TINT interrupts ie where pinctrl driver is child of irqc domain > > > > > > + * the hwirq and TINT are encoded in fwspec->param[0]. > > > > > > + * hwirq for TINT range from 9-40, hwirq is embedded 0-15 bits and TINT > > > > > > + * from 16-31 bits. TINT from the pinctrl driver needs to be programmed > > > > > > + * in IRQC registers to enable a given gpio pin as interrupt. > > > > > > + */ > > > > > > + if (hwirq > IRQC_IRQ_COUNT) { > > > > > > + tint = TINT_EXTRACT_GPIOINT(hwirq); > > > > > > + hwirq = TINT_EXTRACT_HWIRQ(hwirq); > > > > > > + > > > > > > + if (hwirq < IRQC_TINT_START) > > > > > > + return -EINVAL; > > > > > > + } > > > > > > + > > > > > > + if (hwirq > (IRQC_NUM_IRQ - 1)) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL); > > > > > > > > > > Are we really allocating an unsigned long for something that already > > > > > fits in something that is pointer-sized? > > > > > > > > > I think I received some feedback to use unsigned long. Let me know > > > > what you want me to use here. > > > > > > I think this is just a waste of memory, but I don't really care. > > > > > Is there any better way I can handle it? > > How about (shock, horror) a cast? > Right I get you now.. > > > > > > > > > > > > + if (!chip_data) > > > > > > + return -ENOMEM; > > > > > > + *chip_data = tint; > > > > > > > > > > So here, *chip_data can be set to -EINVAL if hwirq <= IRQC_IRQ_COUNT? > > > > > This can't be right. > > > > > > > > > Yes *chip_data can be -EINVAL. IRQC block handles IRQ0-7 and > > > > GPIOINT0-122. So the -EINVAL here is for IRQ0-7 case were dont > > > > required the chip data in the call backs hence -EINVAL, Whereas for > > > > GPIOINT0-122 we need chip_data in the callbacks as this value needs to > > > > be programmed in the hardware registers. > > > > > > I can't see anything that checks it (let alone the difference in > > > types). And if it isn't checked, this means that the allocation is > > > pointless. > > > > > There are checks for example below: > > > > static void rzg2l_irqc_irq_enable(struct irq_data *d) > > { > > unsigned int hw_irq = irqd_to_hwirq(d); > > > > if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) { > > struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > unsigned long chip_data = *(unsigned long *)d->chip_data; > > u32 offset = hw_irq - IRQC_TINT_START; > > u32 tssr_offset = TSSR_OFFSET(offset); > > u8 tssr_index = TSSR_INDEX(offset); > > u32 reg; > > > > raw_spin_lock(&priv->lock); > > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > > reg |= (TIEN | chip_data) << TSSEL_SHIFT(tssr_offset); > > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > > raw_spin_unlock(&priv->lock); > > } > > irq_chip_enable_parent(d); > > } > > > > This check hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ here > > would mean its GPIOINT0-122 and then the chip data will be used. > > That doesn't check the content of chip_data if outside of this > condition. Nonetheless, you allocate an unsigned long to store > -EINVAL. Not only this is a pointless allocation, but you use it to > store something that you never retrieve the first place. Don't you see > the problem? > ... and when using cast I no longer need the allocation. Cheers, Prabhakar
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. Cheers, Prabhakar Changes for v4->v5: * Updated commit message for patch 3/5 * Dropped interrupt-parent from and included RB tag from Geert for patch 4/5 * Implemented init_valid_mask() callback * Dropped ngirq patch from previous series * Dropped patches 4/7 and 5/7 from previous patch series will handle it separately. Changes for v3->v4: * Updated description for interrupts-cells property in patch #1 * Dropped the patch which overriding free callback in gpiolib * Used devm helpers in patch#2 * Patch #4, #5 and #6 are newly added * In patch #7 dropped using gpio offset as hwirq * Implemented immutable GPIO in patch #7 * Implemented child_offset_to_irq() callback in patch #7 Changes for v2->v3: * Updated description for interrupts-cells property in patch #1 * Included RB tag from Geert for binding patch * Fixed review comments pointed by Geert, Biju and Sergei. Changes for v1->v2: * Included RB tag from Rob * Fixed review comments pointed by Geert * included GPIO driver changes Changes for RFCV4 -> V1: * Used unevaluatedProperties. * Altered the sequence of reg property * Set the parent type * Used raw_spin_lock() instead of raw_spin_lock_irqsave() * Simplified parsing IRQ map. * Will send the GPIO and pinctrl changes as part of separate series Changes for RFC v4: * Used locking while RMW * Now using interrupts property instead of interrupt-map * Patch series depends on [0] * Updated binding doc * Fixed comments pointed by Andy [0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/ 20220316200633.28974-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/ Changes for RFC v3: -> Re-structured the driver as a hierarchical irq domain instead of chained -> made use of IRQCHIP_* macros -> dropped locking -> Added support for IRQ0-7 interrupts -> Introduced 2 new patches for GPIOLIB -> Switched to using GPIOLIB for irqdomains in pinctrl RFC v2: https://patchwork.kernel.org/project/linux-renesas-soc/cover/ 20210921193028.13099-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/ RFC v1: https://patchwork.kernel.org/project/linux-renesas-soc/cover/ 20210803175109.1729-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/ Lad Prabhakar (5): dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt Controller irqchip: Add RZ/G2L IA55 Interrupt Controller driver gpio: gpiolib: Allow free() callback to be overridden dt-bindings: pinctrl: renesas,rzg2l-pinctrl: Document the properties to handle GPIO IRQ pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt .../renesas,rzg2l-irqc.yaml | 133 ++++++ .../pinctrl/renesas,rzg2l-pinctrl.yaml | 15 + drivers/gpio/gpiolib.c | 9 +- drivers/irqchip/Kconfig | 8 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-renesas-rzg2l.c | 425 ++++++++++++++++++ drivers/pinctrl/renesas/pinctrl-rzg2l.c | 236 ++++++++++ 7 files changed, 824 insertions(+), 3 deletions(-) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml create mode 100644 drivers/irqchip/irq-renesas-rzg2l.c