| Message ID | 20240403203503.634465-3-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add IAX45 support for RZ/Five SoC | expand |
Hi Prabhakar, Thanks for the patch. > -----Original Message----- > From: Prabhakar <prabhakar.csengg@gmail.com> > Sent: Wednesday, April 3, 2024 9:35 PM > Subject: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared to the RZ/G2L (family) > SoC. > > Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC controller driver. Two new > registers, IMSK and TMSK, are defined to handle masking on RZ/Five SoC. The implementation utilizes > a new data structure, `struct rzg2l_irqc_data`, to determine mask support for a specific controller > instance. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > v1->v2 > - Added IRQCHIP_MATCH() for RZ/Five > - Retaining a copy of OF data in priv > - Rebased the changes > --- > drivers/irqchip/irq-renesas-rzg2l.c | 137 +++++++++++++++++++++++++++- > 1 file changed, 132 insertions(+), 5 deletions(-) > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c > index f6484bf15e0b..6fa8d65605dc 100644 > --- a/drivers/irqchip/irq-renesas-rzg2l.c > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > @@ -37,6 +37,8 @@ > #define TSSEL_SHIFT(n) (8 * (n)) > #define TSSEL_MASK GENMASK(7, 0) > #define IRQ_MASK 0x3 > +#define IMSK 0x10010 > +#define TMSK 0x10020 > > #define TSSR_OFFSET(n) ((n) % 4) > #define TSSR_INDEX(n) ((n) / 4) > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache { > u32 titsr[2]; > }; > > +/** > + * struct rzg2l_irqc_of_data - OF data structure > + * @mask_supported: Indicates if mask registers are available */ > +struct rzg2l_irqc_of_data { > + bool mask_supported; > +}; > + > /** > * struct rzg2l_irqc_priv - IRQ controller private data structure > * @base: Controller's base address > + * @data: OF data pointer > * @fwspec: IRQ firmware specific data > * @lock: Lock to serialize access to hardware registers > * @cache: Registers cache for suspend/resume > */ > static struct rzg2l_irqc_priv { > void __iomem *base; > + const struct rzg2l_irqc_of_data *data; > struct irq_fwspec fwspec[IRQC_NUM_IRQ]; > raw_spinlock_t lock; > struct rzg2l_irqc_reg_cache cache; > @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d) > irq_chip_eoi_parent(d); > } > > +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv, > + unsigned int hwirq) > +{ > + u32 imsk = readl_relaxed(priv->base + IMSK); > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > + > + writel_relaxed(imsk | bit, priv->base + IMSK); } > + > +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv, > + unsigned int hwirq) > +{ > + u32 imsk = readl_relaxed(priv->base + IMSK); > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > + > + writel_relaxed(imsk & ~bit, priv->base + IMSK); } > + > +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv, > + unsigned int hwirq) > +{ > + u32 tmsk = readl_relaxed(priv->base + TMSK); > + u32 bit = BIT(hwirq - IRQC_TINT_START); > + > + writel_relaxed(tmsk | bit, priv->base + TMSK); } > + > +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv, > + unsigned int hwirq) > +{ > + u32 tmsk = readl_relaxed(priv->base + TMSK); > + u32 bit = BIT(hwirq - IRQC_TINT_START); > + > + writel_relaxed(tmsk & ~bit, priv->base + TMSK); } > + > +/* Must be called while priv->lock is held */ static void > +rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq) > +{ > + if (!priv->data->mask_supported) > + return; > + > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > + rzg2l_irqc_mask_irq_interrupt(priv, hwirq); > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); } > + > +static void rzg2l_irqc_mask(struct irq_data *d) { > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > + > + raw_spin_lock(&priv->lock); > + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d)); > + raw_spin_unlock(&priv->lock); > + irq_chip_mask_parent(d); > +} > + > +/* Must be called while priv->lock is held */ static void > +rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int > +hwirq) { > + if (!priv->data->mask_supported) > + return; > + > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq); > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); } > + > +static void rzg2l_irqc_unmask(struct irq_data *d) { > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > + > + raw_spin_lock(&priv->lock); > + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d)); > + raw_spin_unlock(&priv->lock); > + irq_chip_unmask_parent(d); > +} > + > static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) { > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(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); > + if (enable) > + rzg2l_irqc_unmask_once(priv, hw_irq); > + else > + rzg2l_irqc_mask_once(priv, hw_irq); > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > if (enable) > reg |= TIEN << TSSEL_SHIFT(tssr_offset); @@ -157,6 +253,13 @@ static void > rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) > reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset)); > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > raw_spin_unlock(&priv->lock); > + } else { > + raw_spin_lock(&priv->lock); > + if (enable) > + rzg2l_irqc_unmask_once(priv, hw_irq); > + else > + rzg2l_irqc_mask_once(priv, hw_irq); > + raw_spin_unlock(&priv->lock); > } > } > > @@ -324,8 +427,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops = { 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_mask = rzg2l_irqc_mask, > + .irq_unmask = rzg2l_irqc_unmask, I feel this will be clean, if we have static const struct irq_chip rzg2l_irqc_chip = { .name = "rzg2l-irqc", ... .irq_mask = irq_chip_mask_parent, .irq_unmask = irq_chip_unmask_parent, .... }; static const struct irq_chip rzfive_irqc_chip = { .name = "rzfive-irqc", ... .irq_mask = rzfive_irqc_mask, .irq_unmask = rzfive_irqc_unmask, .... }; And passing this in rzg2l_irqc_init() and rzfive_irqc_init(), see below return rzg2l_irqc_init_helper(node, parent, & rzg2l_irqc_chip); return rzg2l_irqc_init_helper(node, parent, & rzfive_irqc_chip); > .irq_disable = rzg2l_irqc_irq_disable, > .irq_enable = rzg2l_irqc_irq_enable, > .irq_get_irqchip_state = irq_chip_get_parent_state, > @@ -401,7 +504,16 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv, > return 0; > } > > -static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent) > +static const struct rzg2l_irqc_of_data rzg2l_irqc_mask_supported_data = { > + .mask_supported = true, > +}; > + > +static const struct rzg2l_irqc_of_data rzg2l_irqc_default_data = { > + .mask_supported = false, > +}; > + > +static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent, > + const struct rzg2l_irqc_of_data *of_data) Maybe rename this as rzg2l_irqc_init_helper() > { > struct irq_domain *irq_domain, *parent_domain; > struct platform_device *pdev; > @@ -422,6 +534,8 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node > *parent) > if (!rzg2l_irqc_data) > return -ENOMEM; > > + rzg2l_irqc_data->data = of_data; > + > rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL); > if (IS_ERR(rzg2l_irqc_data->base)) > return PTR_ERR(rzg2l_irqc_data->base); @@ -472,8 +586,21 @@ static int > rzg2l_irqc_init(struct device_node *node, struct device_node *parent) > return ret; > } > > +static int __init rzg2l_irqc_default_init(struct device_node *node, > + struct device_node *parent) > +{ > + return rzg2l_irqc_init(node, parent, &rzg2l_irqc_default_data); } > + > +static int __init rzg2l_irqc_mask_supported_init(struct device_node *node, > + struct device_node *parent) > +{ > + return rzg2l_irqc_init(node, parent, &rzg2l_irqc_mask_supported_data); > +} > + > IRQCHIP_PLATFORM_DRIVER_BEGIN(rzg2l_irqc) > -IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_init) Retain this name > +IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_default_init) > +IRQCHIP_MATCH("renesas,r9a07g043f-irqc", > +rzg2l_irqc_mask_supported_init) Maybe rename this as rzfive_irqc_init ?? Cheers, Biju > IRQCHIP_PLATFORM_DRIVER_END(rzg2l_irqc) > MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>"); > MODULE_DESCRIPTION("Renesas RZ/G2L IRQC Driver"); > -- > 2.34.1 >
Hi Biju, Thank you for the review. On Thu, Apr 4, 2024 at 8:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Prabhakar, > > Thanks for the patch. > > > -----Original Message----- > > From: Prabhakar <prabhakar.csengg@gmail.com> > > Sent: Wednesday, April 3, 2024 9:35 PM > > Subject: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared to the RZ/G2L (family) > > SoC. > > > > Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC controller driver. Two new > > registers, IMSK and TMSK, are defined to handle masking on RZ/Five SoC. The implementation utilizes > > a new data structure, `struct rzg2l_irqc_data`, to determine mask support for a specific controller > > instance. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > v1->v2 > > - Added IRQCHIP_MATCH() for RZ/Five > > - Retaining a copy of OF data in priv > > - Rebased the changes > > --- > > drivers/irqchip/irq-renesas-rzg2l.c | 137 +++++++++++++++++++++++++++- > > 1 file changed, 132 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c > > index f6484bf15e0b..6fa8d65605dc 100644 > > --- a/drivers/irqchip/irq-renesas-rzg2l.c > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > > @@ -37,6 +37,8 @@ > > #define TSSEL_SHIFT(n) (8 * (n)) > > #define TSSEL_MASK GENMASK(7, 0) > > #define IRQ_MASK 0x3 > > +#define IMSK 0x10010 > > +#define TMSK 0x10020 > > > > #define TSSR_OFFSET(n) ((n) % 4) > > #define TSSR_INDEX(n) ((n) / 4) > > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache { > > u32 titsr[2]; > > }; > > > > +/** > > + * struct rzg2l_irqc_of_data - OF data structure > > + * @mask_supported: Indicates if mask registers are available */ > > +struct rzg2l_irqc_of_data { > > + bool mask_supported; > > +}; > > + > > /** > > * struct rzg2l_irqc_priv - IRQ controller private data structure > > * @base: Controller's base address > > + * @data: OF data pointer > > * @fwspec: IRQ firmware specific data > > * @lock: Lock to serialize access to hardware registers > > * @cache: Registers cache for suspend/resume > > */ > > static struct rzg2l_irqc_priv { > > void __iomem *base; > > + const struct rzg2l_irqc_of_data *data; > > struct irq_fwspec fwspec[IRQC_NUM_IRQ]; > > raw_spinlock_t lock; > > struct rzg2l_irqc_reg_cache cache; > > @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d) > > irq_chip_eoi_parent(d); > > } > > > > +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > + unsigned int hwirq) > > +{ > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > + > > + writel_relaxed(imsk | bit, priv->base + IMSK); } > > + > > +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > + unsigned int hwirq) > > +{ > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > + > > + writel_relaxed(imsk & ~bit, priv->base + IMSK); } > > + > > +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > + unsigned int hwirq) > > +{ > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > + > > + writel_relaxed(tmsk | bit, priv->base + TMSK); } > > + > > +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > + unsigned int hwirq) > > +{ > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > + > > + writel_relaxed(tmsk & ~bit, priv->base + TMSK); } > > + > > +/* Must be called while priv->lock is held */ static void > > +rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq) > > +{ > > + if (!priv->data->mask_supported) > > + return; > > + > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > + rzg2l_irqc_mask_irq_interrupt(priv, hwirq); > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); } > > + > > +static void rzg2l_irqc_mask(struct irq_data *d) { > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > + > > + raw_spin_lock(&priv->lock); > > + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d)); > > + raw_spin_unlock(&priv->lock); > > + irq_chip_mask_parent(d); > > +} > > + > > +/* Must be called while priv->lock is held */ static void > > +rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int > > +hwirq) { > > + if (!priv->data->mask_supported) > > + return; > > + > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq); > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); } > > + > > +static void rzg2l_irqc_unmask(struct irq_data *d) { > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > + > > + raw_spin_lock(&priv->lock); > > + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d)); > > + raw_spin_unlock(&priv->lock); > > + irq_chip_unmask_parent(d); > > +} > > + > > static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) { > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(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); > > + if (enable) > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > + else > > + rzg2l_irqc_mask_once(priv, hw_irq); > > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > > if (enable) > > reg |= TIEN << TSSEL_SHIFT(tssr_offset); @@ -157,6 +253,13 @@ static void > > rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) > > reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset)); > > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > > raw_spin_unlock(&priv->lock); > > + } else { > > + raw_spin_lock(&priv->lock); > > + if (enable) > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > + else > > + rzg2l_irqc_mask_once(priv, hw_irq); > > + raw_spin_unlock(&priv->lock); > > } > > } > > > > @@ -324,8 +427,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops = { 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_mask = rzg2l_irqc_mask, > > + .irq_unmask = rzg2l_irqc_unmask, > > I feel this will be clean, if we have > > static const struct irq_chip rzg2l_irqc_chip = { > .name = "rzg2l-irqc", > ... > .irq_mask = irq_chip_mask_parent, > .irq_unmask = irq_chip_unmask_parent, > .... > }; > > static const struct irq_chip rzfive_irqc_chip = { > .name = "rzfive-irqc", > ... > .irq_mask = rzfive_irqc_mask, > .irq_unmask = rzfive_irqc_unmask, > .... > }; > > And passing this in rzg2l_irqc_init() and rzfive_irqc_init(), see below > > return rzg2l_irqc_init_helper(node, parent, & rzg2l_irqc_chip); > return rzg2l_irqc_init_helper(node, parent, & rzfive_irqc_chip); > If we do the above we are stuck with "struct irq_chip" as data, for further upcoming SoCs (for example RZ/V2H) which have more features we need to pass custom data to handle these features. > > > .irq_disable = rzg2l_irqc_irq_disable, > > .irq_enable = rzg2l_irqc_irq_enable, > > .irq_get_irqchip_state = irq_chip_get_parent_state, > > @@ -401,7 +504,16 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv, > > return 0; > > } > > > > -static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent) > > +static const struct rzg2l_irqc_of_data rzg2l_irqc_mask_supported_data = { > > + .mask_supported = true, > > +}; > > + > > +static const struct rzg2l_irqc_of_data rzg2l_irqc_default_data = { > > + .mask_supported = false, > > +}; > > + > > +static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent, > > + const struct rzg2l_irqc_of_data *of_data) > > Maybe rename this as rzg2l_irqc_init_helper() OK. > > { > > struct irq_domain *irq_domain, *parent_domain; > > struct platform_device *pdev; > > @@ -422,6 +534,8 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node > > *parent) > > if (!rzg2l_irqc_data) > > return -ENOMEM; > > > > + rzg2l_irqc_data->data = of_data; > > + > > rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL); > > if (IS_ERR(rzg2l_irqc_data->base)) > > return PTR_ERR(rzg2l_irqc_data->base); @@ -472,8 +586,21 @@ static int > > rzg2l_irqc_init(struct device_node *node, struct device_node *parent) > > return ret; > > } > > > > +static int __init rzg2l_irqc_default_init(struct device_node *node, > > + struct device_node *parent) > > +{ > > + return rzg2l_irqc_init(node, parent, &rzg2l_irqc_default_data); } > > + > > +static int __init rzg2l_irqc_mask_supported_init(struct device_node *node, > > + struct device_node *parent) > > +{ > > + return rzg2l_irqc_init(node, parent, &rzg2l_irqc_mask_supported_data); > > +} > > + > > IRQCHIP_PLATFORM_DRIVER_BEGIN(rzg2l_irqc) > > -IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_init) > Retain this name > OK. > > +IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_default_init) > > +IRQCHIP_MATCH("renesas,r9a07g043f-irqc", > > +rzg2l_irqc_mask_supported_init) > Maybe rename this as rzfive_irqc_init ?? > OK. Cheers, Prabhakar
Hi Lad, Prabhakar, > -----Original Message----- > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Sent: Thursday, April 4, 2024 2:27 PM > Subject: Re: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC > > Hi Biju, > > Thank you for the review. > > On Thu, Apr 4, 2024 at 8:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > Hi Prabhakar, > > > > Thanks for the patch. > > > > > -----Original Message----- > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > Sent: Wednesday, April 3, 2024 9:35 PM > > > Subject: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for > > > RZ/Five SoC > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as > > > compared to the RZ/G2L (family) SoC. > > > > > > Introduce masking/unmasking support for IRQ and TINT interrupts in > > > IRQC controller driver. Two new registers, IMSK and TMSK, are > > > defined to handle masking on RZ/Five SoC. The implementation > > > utilizes a new data structure, `struct rzg2l_irqc_data`, to determine mask support for a > specific controller instance. > > > > > > Signed-off-by: Lad Prabhakar > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > v1->v2 > > > - Added IRQCHIP_MATCH() for RZ/Five > > > - Retaining a copy of OF data in priv > > > - Rebased the changes > > > --- > > > drivers/irqchip/irq-renesas-rzg2l.c | 137 > > > +++++++++++++++++++++++++++- > > > 1 file changed, 132 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c > > > b/drivers/irqchip/irq-renesas-rzg2l.c > > > index f6484bf15e0b..6fa8d65605dc 100644 > > > --- a/drivers/irqchip/irq-renesas-rzg2l.c > > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > > > @@ -37,6 +37,8 @@ > > > #define TSSEL_SHIFT(n) (8 * (n)) > > > #define TSSEL_MASK GENMASK(7, 0) > > > #define IRQ_MASK 0x3 > > > +#define IMSK 0x10010 > > > +#define TMSK 0x10020 > > > > > > #define TSSR_OFFSET(n) ((n) % 4) > > > #define TSSR_INDEX(n) ((n) / 4) > > > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache { > > > u32 titsr[2]; > > > }; > > > > > > +/** > > > + * struct rzg2l_irqc_of_data - OF data structure > > > + * @mask_supported: Indicates if mask registers are available */ > > > +struct rzg2l_irqc_of_data { > > > + bool mask_supported; > > > +}; > > > + > > > /** > > > * struct rzg2l_irqc_priv - IRQ controller private data structure > > > * @base: Controller's base address > > > + * @data: OF data pointer > > > * @fwspec: IRQ firmware specific data > > > * @lock: Lock to serialize access to hardware registers > > > * @cache: Registers cache for suspend/resume > > > */ > > > static struct rzg2l_irqc_priv { > > > void __iomem *base; > > > + const struct rzg2l_irqc_of_data *data; > > > struct irq_fwspec fwspec[IRQC_NUM_IRQ]; > > > raw_spinlock_t lock; > > > struct rzg2l_irqc_reg_cache cache; > > > @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d) > > > irq_chip_eoi_parent(d); > > > } > > > > > > +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > > + unsigned int hwirq) { > > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > > + > > > + writel_relaxed(imsk | bit, priv->base + IMSK); } > > > + > > > +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > > + unsigned int hwirq) { > > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > > + > > > + writel_relaxed(imsk & ~bit, priv->base + IMSK); } > > > + > > > +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > > + unsigned int hwirq) { > > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > > + > > > + writel_relaxed(tmsk | bit, priv->base + TMSK); } > > > + > > > +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > > + unsigned int hwirq) { > > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > > + > > > + writel_relaxed(tmsk & ~bit, priv->base + TMSK); } > > > + > > > +/* Must be called while priv->lock is held */ static void > > > +rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int > > > +hwirq) { > > > + if (!priv->data->mask_supported) > > > + return; > > > + > > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > > + rzg2l_irqc_mask_irq_interrupt(priv, hwirq); > > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > > + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); } > > > + > > > +static void rzg2l_irqc_mask(struct irq_data *d) { > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > > + > > > + raw_spin_lock(&priv->lock); > > > + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d)); > > > + raw_spin_unlock(&priv->lock); > > > + irq_chip_mask_parent(d); > > > +} > > > + > > > +/* Must be called while priv->lock is held */ static void > > > +rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int > > > +hwirq) { > > > + if (!priv->data->mask_supported) > > > + return; > > > + > > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > > + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq); > > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > > + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); } > > > + > > > +static void rzg2l_irqc_unmask(struct irq_data *d) { > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > > + > > > + raw_spin_lock(&priv->lock); > > > + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d)); > > > + raw_spin_unlock(&priv->lock); > > > + irq_chip_unmask_parent(d); > > > +} > > > + > > > static void rzg2l_tint_irq_endisable(struct irq_data *d, bool > > > enable) { > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(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); > > > + if (enable) > > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > > + else > > > + rzg2l_irqc_mask_once(priv, hw_irq); > > > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > > > if (enable) > > > reg |= TIEN << TSSEL_SHIFT(tssr_offset); @@ > > > -157,6 +253,13 @@ static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) > > > reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset)); > > > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > > > raw_spin_unlock(&priv->lock); > > > + } else { > > > + raw_spin_lock(&priv->lock); > > > + if (enable) > > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > > + else > > > + rzg2l_irqc_mask_once(priv, hw_irq); > > > + raw_spin_unlock(&priv->lock); > > > } > > > } > > > > > > @@ -324,8 +427,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops > > > = { 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_mask = rzg2l_irqc_mask, > > > + .irq_unmask = rzg2l_irqc_unmask, > > > > I feel this will be clean, if we have > > > > static const struct irq_chip rzg2l_irqc_chip = { > > .name = "rzg2l-irqc", > > ... > > .irq_mask = irq_chip_mask_parent, > > .irq_unmask = irq_chip_unmask_parent, > > .... > > }; > > > > static const struct irq_chip rzfive_irqc_chip = { > > .name = "rzfive-irqc", > > ... > > .irq_mask = rzfive_irqc_mask, > > .irq_unmask = rzfive_irqc_unmask, > > .... > > }; > > > > And passing this in rzg2l_irqc_init() and rzfive_irqc_init(), see > > below > > > > return rzg2l_irqc_init_helper(node, parent, & rzg2l_irqc_chip); return > > rzg2l_irqc_init_helper(node, parent, & rzfive_irqc_chip); > > > If we do the above we are stuck with "struct irq_chip" as data, for further upcoming SoCs (for > example RZ/V2H) which have more features we need to pass custom data to handle these features. That time device data can be extended like below struct rz_g2l_irq_chip { struct irq_chip; void *data; /* custom data */ } Cheers, Biju
On Thu, Apr 4, 2024 at 2:31 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Lad, Prabhakar, > > > -----Original Message----- > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > Sent: Thursday, April 4, 2024 2:27 PM > > Subject: Re: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC > > > > Hi Biju, > > > > Thank you for the review. > > > > On Thu, Apr 4, 2024 at 8:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > Hi Prabhakar, > > > > > > Thanks for the patch. > > > > > > > -----Original Message----- > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > Sent: Wednesday, April 3, 2024 9:35 PM > > > > Subject: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for > > > > RZ/Five SoC > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as > > > > compared to the RZ/G2L (family) SoC. > > > > > > > > Introduce masking/unmasking support for IRQ and TINT interrupts in > > > > IRQC controller driver. Two new registers, IMSK and TMSK, are > > > > defined to handle masking on RZ/Five SoC. The implementation > > > > utilizes a new data structure, `struct rzg2l_irqc_data`, to determine mask support for a > > specific controller instance. > > > > > > > > Signed-off-by: Lad Prabhakar > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > --- > > > > v1->v2 > > > > - Added IRQCHIP_MATCH() for RZ/Five > > > > - Retaining a copy of OF data in priv > > > > - Rebased the changes > > > > --- > > > > drivers/irqchip/irq-renesas-rzg2l.c | 137 > > > > +++++++++++++++++++++++++++- > > > > 1 file changed, 132 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c > > > > b/drivers/irqchip/irq-renesas-rzg2l.c > > > > index f6484bf15e0b..6fa8d65605dc 100644 > > > > --- a/drivers/irqchip/irq-renesas-rzg2l.c > > > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > > > > @@ -37,6 +37,8 @@ > > > > #define TSSEL_SHIFT(n) (8 * (n)) > > > > #define TSSEL_MASK GENMASK(7, 0) > > > > #define IRQ_MASK 0x3 > > > > +#define IMSK 0x10010 > > > > +#define TMSK 0x10020 > > > > > > > > #define TSSR_OFFSET(n) ((n) % 4) > > > > #define TSSR_INDEX(n) ((n) / 4) > > > > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache { > > > > u32 titsr[2]; > > > > }; > > > > > > > > +/** > > > > + * struct rzg2l_irqc_of_data - OF data structure > > > > + * @mask_supported: Indicates if mask registers are available */ > > > > +struct rzg2l_irqc_of_data { > > > > + bool mask_supported; > > > > +}; > > > > + > > > > /** > > > > * struct rzg2l_irqc_priv - IRQ controller private data structure > > > > * @base: Controller's base address > > > > + * @data: OF data pointer > > > > * @fwspec: IRQ firmware specific data > > > > * @lock: Lock to serialize access to hardware registers > > > > * @cache: Registers cache for suspend/resume > > > > */ > > > > static struct rzg2l_irqc_priv { > > > > void __iomem *base; > > > > + const struct rzg2l_irqc_of_data *data; > > > > struct irq_fwspec fwspec[IRQC_NUM_IRQ]; > > > > raw_spinlock_t lock; > > > > struct rzg2l_irqc_reg_cache cache; > > > > @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d) > > > > irq_chip_eoi_parent(d); > > > > } > > > > > > > > +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > > > + unsigned int hwirq) { > > > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > > > + > > > > + writel_relaxed(imsk | bit, priv->base + IMSK); } > > > > + > > > > +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > > > + unsigned int hwirq) { > > > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > > > + > > > > + writel_relaxed(imsk & ~bit, priv->base + IMSK); } > > > > + > > > > +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > > > + unsigned int hwirq) { > > > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > > > + > > > > + writel_relaxed(tmsk | bit, priv->base + TMSK); } > > > > + > > > > +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > > > + unsigned int hwirq) { > > > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > > > + > > > > + writel_relaxed(tmsk & ~bit, priv->base + TMSK); } > > > > + > > > > +/* Must be called while priv->lock is held */ static void > > > > +rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int > > > > +hwirq) { > > > > + if (!priv->data->mask_supported) > > > > + return; > > > > + > > > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > > > + rzg2l_irqc_mask_irq_interrupt(priv, hwirq); > > > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > > > + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); } > > > > + > > > > +static void rzg2l_irqc_mask(struct irq_data *d) { > > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > > > + > > > > + raw_spin_lock(&priv->lock); > > > > + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d)); > > > > + raw_spin_unlock(&priv->lock); > > > > + irq_chip_mask_parent(d); > > > > +} > > > > + > > > > +/* Must be called while priv->lock is held */ static void > > > > +rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int > > > > +hwirq) { > > > > + if (!priv->data->mask_supported) > > > > + return; > > > > + > > > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > > > + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq); > > > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > > > + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); } > > > > + > > > > +static void rzg2l_irqc_unmask(struct irq_data *d) { > > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > > > + > > > > + raw_spin_lock(&priv->lock); > > > > + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d)); > > > > + raw_spin_unlock(&priv->lock); > > > > + irq_chip_unmask_parent(d); > > > > +} > > > > + > > > > static void rzg2l_tint_irq_endisable(struct irq_data *d, bool > > > > enable) { > > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(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); > > > > + if (enable) > > > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > > > + else > > > > + rzg2l_irqc_mask_once(priv, hw_irq); > > > > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > > > > if (enable) > > > > reg |= TIEN << TSSEL_SHIFT(tssr_offset); @@ > > > > -157,6 +253,13 @@ static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) > > > > reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset)); > > > > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > > > > raw_spin_unlock(&priv->lock); > > > > + } else { > > > > + raw_spin_lock(&priv->lock); > > > > + if (enable) > > > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > > > + else > > > > + rzg2l_irqc_mask_once(priv, hw_irq); > > > > + raw_spin_unlock(&priv->lock); > > > > } > > > > } > > > > > > > > @@ -324,8 +427,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops > > > > = { 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_mask = rzg2l_irqc_mask, > > > > + .irq_unmask = rzg2l_irqc_unmask, > > > > > > I feel this will be clean, if we have > > > > > > static const struct irq_chip rzg2l_irqc_chip = { > > > .name = "rzg2l-irqc", > > > ... > > > .irq_mask = irq_chip_mask_parent, > > > .irq_unmask = irq_chip_unmask_parent, > > > .... > > > }; > > > > > > static const struct irq_chip rzfive_irqc_chip = { > > > .name = "rzfive-irqc", > > > ... > > > .irq_mask = rzfive_irqc_mask, > > > .irq_unmask = rzfive_irqc_unmask, > > > .... > > > }; > > > > > > And passing this in rzg2l_irqc_init() and rzfive_irqc_init(), see > > > below > > > > > > return rzg2l_irqc_init_helper(node, parent, & rzg2l_irqc_chip); return > > > rzg2l_irqc_init_helper(node, parent, & rzfive_irqc_chip); > > > > > If we do the above we are stuck with "struct irq_chip" as data, for further upcoming SoCs (for > > example RZ/V2H) which have more features we need to pass custom data to handle these features. > > That time device data can be extended like below > > struct rz_g2l_irq_chip { > struct irq_chip; > void *data; /* custom data */ > } > Ok, but i'll wait for Geert to come back on this as Geert suggested to me to do it this way. Cheers, Prabhakar
Hi Prabhakar, On Wed, Apr 3, 2024 at 10:36 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared > to the RZ/G2L (family) SoC. > > Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC > controller driver. Two new registers, IMSK and TMSK, are defined to > handle masking on RZ/Five SoC. The implementation utilizes a new data > structure, `struct rzg2l_irqc_data`, to determine mask support for a > specific controller instance. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > v1->v2 > - Added IRQCHIP_MATCH() for RZ/Five > - Retaining a copy of OF data in priv > - Rebased the changes Thanks for the update! > --- a/drivers/irqchip/irq-renesas-rzg2l.c > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > @@ -37,6 +37,8 @@ > #define TSSEL_SHIFT(n) (8 * (n)) > #define TSSEL_MASK GENMASK(7, 0) > #define IRQ_MASK 0x3 > +#define IMSK 0x10010 > +#define TMSK 0x10020 > > #define TSSR_OFFSET(n) ((n) % 4) > #define TSSR_INDEX(n) ((n) / 4) > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache { > u32 titsr[2]; > }; > > +/** > + * struct rzg2l_irqc_of_data - OF data structure > + * @mask_supported: Indicates if mask registers are available > + */ > +struct rzg2l_irqc_of_data { > + bool mask_supported; > +}; > + > /** > * struct rzg2l_irqc_priv - IRQ controller private data structure > * @base: Controller's base address > + * @data: OF data pointer > * @fwspec: IRQ firmware specific data > * @lock: Lock to serialize access to hardware registers > * @cache: Registers cache for suspend/resume > */ > static struct rzg2l_irqc_priv { > void __iomem *base; > + const struct rzg2l_irqc_of_data *data; That's not a copy, but a pointer. > struct irq_fwspec fwspec[IRQC_NUM_IRQ]; > raw_spinlock_t lock; > struct rzg2l_irqc_reg_cache cache; > @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d) > irq_chip_eoi_parent(d); > } > > +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv, > + unsigned int hwirq) > +{ > + u32 imsk = readl_relaxed(priv->base + IMSK); > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > + > + writel_relaxed(imsk | bit, priv->base + IMSK); > +} > + > +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv, > + unsigned int hwirq) > +{ > + u32 imsk = readl_relaxed(priv->base + IMSK); > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > + > + writel_relaxed(imsk & ~bit, priv->base + IMSK); > +} > + > +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv, > + unsigned int hwirq) > +{ > + u32 tmsk = readl_relaxed(priv->base + TMSK); > + u32 bit = BIT(hwirq - IRQC_TINT_START); > + > + writel_relaxed(tmsk | bit, priv->base + TMSK); > +} > + > +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv, > + unsigned int hwirq) > +{ > + u32 tmsk = readl_relaxed(priv->base + TMSK); > + u32 bit = BIT(hwirq - IRQC_TINT_START); > + > + writel_relaxed(tmsk & ~bit, priv->base + TMSK); > +} > + > +/* Must be called while priv->lock is held */ > +static void rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq) > +{ > + if (!priv->data->mask_supported) > + return; > + > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > + rzg2l_irqc_mask_irq_interrupt(priv, hwirq); > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); > +} > + > +static void rzg2l_irqc_mask(struct irq_data *d) > +{ > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > + > + raw_spin_lock(&priv->lock); > + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d)); > + raw_spin_unlock(&priv->lock); > + irq_chip_mask_parent(d); > +} > + > +/* Must be called while priv->lock is held */ > +static void rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq) > +{ > + if (!priv->data->mask_supported) > + return; > + > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq); > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); > +} > + > +static void rzg2l_irqc_unmask(struct irq_data *d) > +{ > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > + > + raw_spin_lock(&priv->lock); > + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d)); > + raw_spin_unlock(&priv->lock); > + irq_chip_unmask_parent(d); > +} > + > static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) > { > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(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); > + if (enable) > + rzg2l_irqc_unmask_once(priv, hw_irq); > + else > + rzg2l_irqc_mask_once(priv, hw_irq); You already know this is a TINT interrupt, so you could call rzg2l_irqc_(un)mask_irq_interrupt() directly. > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > if (enable) > reg |= TIEN << TSSEL_SHIFT(tssr_offset); > @@ -157,6 +253,13 @@ static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) > reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset)); > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > raw_spin_unlock(&priv->lock); > + } else { > + raw_spin_lock(&priv->lock); > + if (enable) > + rzg2l_irqc_unmask_once(priv, hw_irq); > + else > + rzg2l_irqc_mask_once(priv, hw_irq); Likewise. > + raw_spin_unlock(&priv->lock); > } > } Gr{oetje,eeting}s, Geert
Hi Prabhakar, On Thu, Apr 4, 2024 at 3:35 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Thu, Apr 4, 2024 at 2:31 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > -----Original Message----- > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > On Thu, Apr 4, 2024 at 8:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > -----Original Message----- > > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as > > > > > compared to the RZ/G2L (family) SoC. > > > > > > > > > > Introduce masking/unmasking support for IRQ and TINT interrupts in > > > > > IRQC controller driver. Two new registers, IMSK and TMSK, are > > > > > defined to handle masking on RZ/Five SoC. The implementation > > > > > utilizes a new data structure, `struct rzg2l_irqc_data`, to determine mask support for a > > > specific controller instance. > > > > > > > > > > Signed-off-by: Lad Prabhakar > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > --- > > > > > v1->v2 > > > > > - Added IRQCHIP_MATCH() for RZ/Five > > > > > - Retaining a copy of OF data in priv > > > > > - Rebased the changes > > > > > --- > > > > > drivers/irqchip/irq-renesas-rzg2l.c | 137 > > > > > +++++++++++++++++++++++++++- > > > > > 1 file changed, 132 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c > > > > > b/drivers/irqchip/irq-renesas-rzg2l.c > > > > > index f6484bf15e0b..6fa8d65605dc 100644 > > > > > --- a/drivers/irqchip/irq-renesas-rzg2l.c > > > > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > > > > > @@ -37,6 +37,8 @@ > > > > > #define TSSEL_SHIFT(n) (8 * (n)) > > > > > #define TSSEL_MASK GENMASK(7, 0) > > > > > #define IRQ_MASK 0x3 > > > > > +#define IMSK 0x10010 > > > > > +#define TMSK 0x10020 > > > > > > > > > > #define TSSR_OFFSET(n) ((n) % 4) > > > > > #define TSSR_INDEX(n) ((n) / 4) > > > > > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache { > > > > > u32 titsr[2]; > > > > > }; > > > > > > > > > > +/** > > > > > + * struct rzg2l_irqc_of_data - OF data structure > > > > > + * @mask_supported: Indicates if mask registers are available */ > > > > > +struct rzg2l_irqc_of_data { > > > > > + bool mask_supported; > > > > > +}; > > > > > + > > > > > /** > > > > > * struct rzg2l_irqc_priv - IRQ controller private data structure > > > > > * @base: Controller's base address > > > > > + * @data: OF data pointer > > > > > * @fwspec: IRQ firmware specific data > > > > > * @lock: Lock to serialize access to hardware registers > > > > > * @cache: Registers cache for suspend/resume > > > > > */ > > > > > static struct rzg2l_irqc_priv { > > > > > void __iomem *base; > > > > > + const struct rzg2l_irqc_of_data *data; > > > > > struct irq_fwspec fwspec[IRQC_NUM_IRQ]; > > > > > raw_spinlock_t lock; > > > > > struct rzg2l_irqc_reg_cache cache; > > > > > @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d) > > > > > irq_chip_eoi_parent(d); > > > > > } > > > > > > > > > > +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > > > > + unsigned int hwirq) { > > > > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > > > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > > > > + > > > > > + writel_relaxed(imsk | bit, priv->base + IMSK); } > > > > > + > > > > > +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > > > > + unsigned int hwirq) { > > > > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > > > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > > > > + > > > > > + writel_relaxed(imsk & ~bit, priv->base + IMSK); } > > > > > + > > > > > +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > > > > + unsigned int hwirq) { > > > > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > > > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > > > > + > > > > > + writel_relaxed(tmsk | bit, priv->base + TMSK); } > > > > > + > > > > > +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > > > > + unsigned int hwirq) { > > > > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > > > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > > > > + > > > > > + writel_relaxed(tmsk & ~bit, priv->base + TMSK); } > > > > > + > > > > > +/* Must be called while priv->lock is held */ static void > > > > > +rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int > > > > > +hwirq) { > > > > > + if (!priv->data->mask_supported) > > > > > + return; > > > > > + > > > > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > > > > + rzg2l_irqc_mask_irq_interrupt(priv, hwirq); > > > > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > > > > + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); } > > > > > + > > > > > +static void rzg2l_irqc_mask(struct irq_data *d) { > > > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > > > > + > > > > > + raw_spin_lock(&priv->lock); > > > > > + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d)); > > > > > + raw_spin_unlock(&priv->lock); > > > > > + irq_chip_mask_parent(d); > > > > > +} > > > > > + > > > > > +/* Must be called while priv->lock is held */ static void > > > > > +rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int > > > > > +hwirq) { > > > > > + if (!priv->data->mask_supported) > > > > > + return; > > > > > + > > > > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > > > > + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq); > > > > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > > > > + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); } > > > > > + > > > > > +static void rzg2l_irqc_unmask(struct irq_data *d) { > > > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > > > > + > > > > > + raw_spin_lock(&priv->lock); > > > > > + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d)); > > > > > + raw_spin_unlock(&priv->lock); > > > > > + irq_chip_unmask_parent(d); > > > > > +} > > > > > + > > > > > static void rzg2l_tint_irq_endisable(struct irq_data *d, bool > > > > > enable) { > > > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(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); > > > > > + if (enable) > > > > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > > > > + else > > > > > + rzg2l_irqc_mask_once(priv, hw_irq); > > > > > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > > > > > if (enable) > > > > > reg |= TIEN << TSSEL_SHIFT(tssr_offset); @@ > > > > > -157,6 +253,13 @@ static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) > > > > > reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset)); > > > > > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > > > > > raw_spin_unlock(&priv->lock); > > > > > + } else { > > > > > + raw_spin_lock(&priv->lock); > > > > > + if (enable) > > > > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > > > > + else > > > > > + rzg2l_irqc_mask_once(priv, hw_irq); > > > > > + raw_spin_unlock(&priv->lock); > > > > > } > > > > > } > > > > > > > > > > @@ -324,8 +427,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops > > > > > = { 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_mask = rzg2l_irqc_mask, > > > > > + .irq_unmask = rzg2l_irqc_unmask, > > > > > > > > I feel this will be clean, if we have > > > > > > > > static const struct irq_chip rzg2l_irqc_chip = { > > > > .name = "rzg2l-irqc", > > > > ... > > > > .irq_mask = irq_chip_mask_parent, > > > > .irq_unmask = irq_chip_unmask_parent, > > > > .... > > > > }; > > > > > > > > static const struct irq_chip rzfive_irqc_chip = { > > > > .name = "rzfive-irqc", > > > > ... > > > > .irq_mask = rzfive_irqc_mask, > > > > .irq_unmask = rzfive_irqc_unmask, > > > > .... > > > > }; > > > > > > > > And passing this in rzg2l_irqc_init() and rzfive_irqc_init(), see > > > > below > > > > > > > > return rzg2l_irqc_init_helper(node, parent, & rzg2l_irqc_chip); return > > > > rzg2l_irqc_init_helper(node, parent, & rzfive_irqc_chip); > > > > > > > If we do the above we are stuck with "struct irq_chip" as data, for further upcoming SoCs (for > > > example RZ/V2H) which have more features we need to pass custom data to handle these features. > > > > That time device data can be extended like below > > > > struct rz_g2l_irq_chip { > > struct irq_chip; > > void *data; /* custom data */ > > } > > > Ok, but i'll wait for Geert to come back on this as Geert suggested to > me to do it this way. I agree with Biju. Having separate irq_chips lets us avoid taking the spinlock on RZ/G2L. Gr{oetje,eeting}s, Geert
Hi Geert, Thank you for the review. On Thu, Apr 18, 2024 at 4:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Wed, Apr 3, 2024 at 10:36 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared > > to the RZ/G2L (family) SoC. > > > > Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC > > controller driver. Two new registers, IMSK and TMSK, are defined to > > handle masking on RZ/Five SoC. The implementation utilizes a new data > > structure, `struct rzg2l_irqc_data`, to determine mask support for a > > specific controller instance. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > v1->v2 > > - Added IRQCHIP_MATCH() for RZ/Five > > - Retaining a copy of OF data in priv > > - Rebased the changes > > Thanks for the update! > > > --- a/drivers/irqchip/irq-renesas-rzg2l.c > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > > @@ -37,6 +37,8 @@ > > #define TSSEL_SHIFT(n) (8 * (n)) > > #define TSSEL_MASK GENMASK(7, 0) > > #define IRQ_MASK 0x3 > > +#define IMSK 0x10010 > > +#define TMSK 0x10020 > > > > #define TSSR_OFFSET(n) ((n) % 4) > > #define TSSR_INDEX(n) ((n) / 4) > > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache { > > u32 titsr[2]; > > }; > > > > +/** > > + * struct rzg2l_irqc_of_data - OF data structure > > + * @mask_supported: Indicates if mask registers are available > > + */ > > +struct rzg2l_irqc_of_data { > > + bool mask_supported; > > +}; > > + > > /** > > * struct rzg2l_irqc_priv - IRQ controller private data structure > > * @base: Controller's base address > > + * @data: OF data pointer > > * @fwspec: IRQ firmware specific data > > * @lock: Lock to serialize access to hardware registers > > * @cache: Registers cache for suspend/resume > > */ > > static struct rzg2l_irqc_priv { > > void __iomem *base; > > + const struct rzg2l_irqc_of_data *data; > > That's not a copy, but a pointer. > Oops, should that be OK or shall I create a copy instead? > > struct irq_fwspec fwspec[IRQC_NUM_IRQ]; > > raw_spinlock_t lock; > > struct rzg2l_irqc_reg_cache cache; > > @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d) > > irq_chip_eoi_parent(d); > > } > > > > +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > + unsigned int hwirq) > > +{ > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > + > > + writel_relaxed(imsk | bit, priv->base + IMSK); > > +} > > + > > +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > + unsigned int hwirq) > > +{ > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > + > > + writel_relaxed(imsk & ~bit, priv->base + IMSK); > > +} > > + > > +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > + unsigned int hwirq) > > +{ > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > + > > + writel_relaxed(tmsk | bit, priv->base + TMSK); > > +} > > + > > +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > + unsigned int hwirq) > > +{ > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > + > > + writel_relaxed(tmsk & ~bit, priv->base + TMSK); > > +} > > + > > +/* Must be called while priv->lock is held */ > > +static void rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq) > > +{ > > + if (!priv->data->mask_supported) > > + return; > > + > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > + rzg2l_irqc_mask_irq_interrupt(priv, hwirq); > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); > > +} > > + > > +static void rzg2l_irqc_mask(struct irq_data *d) > > +{ > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > + > > + raw_spin_lock(&priv->lock); > > + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d)); > > + raw_spin_unlock(&priv->lock); > > + irq_chip_mask_parent(d); > > +} > > + > > +/* Must be called while priv->lock is held */ > > +static void rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq) > > +{ > > + if (!priv->data->mask_supported) > > + return; > > + > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq); > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); > > +} > > + > > +static void rzg2l_irqc_unmask(struct irq_data *d) > > +{ > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > + > > + raw_spin_lock(&priv->lock); > > + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d)); > > + raw_spin_unlock(&priv->lock); > > + irq_chip_unmask_parent(d); > > +} > > + > > static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) > > { > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(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); > > + if (enable) > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > + else > > + rzg2l_irqc_mask_once(priv, hw_irq); > > You already know this is a TINT interrupt, so you could call > rzg2l_irqc_(un)mask_irq_interrupt() directly. > Agreed. > > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > > if (enable) > > reg |= TIEN << TSSEL_SHIFT(tssr_offset); > > @@ -157,6 +253,13 @@ static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) > > reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset)); > > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > > raw_spin_unlock(&priv->lock); > > + } else { > > + raw_spin_lock(&priv->lock); > > + if (enable) > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > + else > > + rzg2l_irqc_mask_once(priv, hw_irq); > > Likewise. > OK. Cheers, Prabhakar
Hi Geert, On Thu, Apr 18, 2024 at 4:13 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Thu, Apr 4, 2024 at 3:35 PM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > On Thu, Apr 4, 2024 at 2:31 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > -----Original Message----- > > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > > On Thu, Apr 4, 2024 at 8:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > -----Original Message----- > > > > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > > > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as > > > > > > compared to the RZ/G2L (family) SoC. > > > > > > > > > > > > Introduce masking/unmasking support for IRQ and TINT interrupts in > > > > > > IRQC controller driver. Two new registers, IMSK and TMSK, are > > > > > > defined to handle masking on RZ/Five SoC. The implementation > > > > > > utilizes a new data structure, `struct rzg2l_irqc_data`, to determine mask support for a > > > > specific controller instance. > > > > > > > > > > > > Signed-off-by: Lad Prabhakar > > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > --- > > > > > > v1->v2 > > > > > > - Added IRQCHIP_MATCH() for RZ/Five > > > > > > - Retaining a copy of OF data in priv > > > > > > - Rebased the changes > > > > > > --- > > > > > > drivers/irqchip/irq-renesas-rzg2l.c | 137 > > > > > > +++++++++++++++++++++++++++- > > > > > > 1 file changed, 132 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c > > > > > > b/drivers/irqchip/irq-renesas-rzg2l.c > > > > > > index f6484bf15e0b..6fa8d65605dc 100644 > > > > > > --- a/drivers/irqchip/irq-renesas-rzg2l.c > > > > > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > > > > > > @@ -37,6 +37,8 @@ > > > > > > #define TSSEL_SHIFT(n) (8 * (n)) > > > > > > #define TSSEL_MASK GENMASK(7, 0) > > > > > > #define IRQ_MASK 0x3 > > > > > > +#define IMSK 0x10010 > > > > > > +#define TMSK 0x10020 > > > > > > > > > > > > #define TSSR_OFFSET(n) ((n) % 4) > > > > > > #define TSSR_INDEX(n) ((n) / 4) > > > > > > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache { > > > > > > u32 titsr[2]; > > > > > > }; > > > > > > > > > > > > +/** > > > > > > + * struct rzg2l_irqc_of_data - OF data structure > > > > > > + * @mask_supported: Indicates if mask registers are available */ > > > > > > +struct rzg2l_irqc_of_data { > > > > > > + bool mask_supported; > > > > > > +}; > > > > > > + > > > > > > /** > > > > > > * struct rzg2l_irqc_priv - IRQ controller private data structure > > > > > > * @base: Controller's base address > > > > > > + * @data: OF data pointer > > > > > > * @fwspec: IRQ firmware specific data > > > > > > * @lock: Lock to serialize access to hardware registers > > > > > > * @cache: Registers cache for suspend/resume > > > > > > */ > > > > > > static struct rzg2l_irqc_priv { > > > > > > void __iomem *base; > > > > > > + const struct rzg2l_irqc_of_data *data; > > > > > > struct irq_fwspec fwspec[IRQC_NUM_IRQ]; > > > > > > raw_spinlock_t lock; > > > > > > struct rzg2l_irqc_reg_cache cache; > > > > > > @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d) > > > > > > irq_chip_eoi_parent(d); > > > > > > } > > > > > > > > > > > > +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > > > > > + unsigned int hwirq) { > > > > > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > > > > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > > > > > + > > > > > > + writel_relaxed(imsk | bit, priv->base + IMSK); } > > > > > > + > > > > > > +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv, > > > > > > + unsigned int hwirq) { > > > > > > + u32 imsk = readl_relaxed(priv->base + IMSK); > > > > > > + u32 bit = BIT(hwirq - IRQC_IRQ_START); > > > > > > + > > > > > > + writel_relaxed(imsk & ~bit, priv->base + IMSK); } > > > > > > + > > > > > > +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > > > > > + unsigned int hwirq) { > > > > > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > > > > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > > > > > + > > > > > > + writel_relaxed(tmsk | bit, priv->base + TMSK); } > > > > > > + > > > > > > +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv, > > > > > > + unsigned int hwirq) { > > > > > > + u32 tmsk = readl_relaxed(priv->base + TMSK); > > > > > > + u32 bit = BIT(hwirq - IRQC_TINT_START); > > > > > > + > > > > > > + writel_relaxed(tmsk & ~bit, priv->base + TMSK); } > > > > > > + > > > > > > +/* Must be called while priv->lock is held */ static void > > > > > > +rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int > > > > > > +hwirq) { > > > > > > + if (!priv->data->mask_supported) > > > > > > + return; > > > > > > + > > > > > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > > > > > + rzg2l_irqc_mask_irq_interrupt(priv, hwirq); > > > > > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > > > > > + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); } > > > > > > + > > > > > > +static void rzg2l_irqc_mask(struct irq_data *d) { > > > > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > > > > > + > > > > > > + raw_spin_lock(&priv->lock); > > > > > > + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d)); > > > > > > + raw_spin_unlock(&priv->lock); > > > > > > + irq_chip_mask_parent(d); > > > > > > +} > > > > > > + > > > > > > +/* Must be called while priv->lock is held */ static void > > > > > > +rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int > > > > > > +hwirq) { > > > > > > + if (!priv->data->mask_supported) > > > > > > + return; > > > > > > + > > > > > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) > > > > > > + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq); > > > > > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) > > > > > > + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); } > > > > > > + > > > > > > +static void rzg2l_irqc_unmask(struct irq_data *d) { > > > > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); > > > > > > + > > > > > > + raw_spin_lock(&priv->lock); > > > > > > + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d)); > > > > > > + raw_spin_unlock(&priv->lock); > > > > > > + irq_chip_unmask_parent(d); > > > > > > +} > > > > > > + > > > > > > static void rzg2l_tint_irq_endisable(struct irq_data *d, bool > > > > > > enable) { > > > > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(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); > > > > > > + if (enable) > > > > > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > > > > > + else > > > > > > + rzg2l_irqc_mask_once(priv, hw_irq); > > > > > > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > > > > > > if (enable) > > > > > > reg |= TIEN << TSSEL_SHIFT(tssr_offset); @@ > > > > > > -157,6 +253,13 @@ static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) > > > > > > reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset)); > > > > > > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > > > > > > raw_spin_unlock(&priv->lock); > > > > > > + } else { > > > > > > + raw_spin_lock(&priv->lock); > > > > > > + if (enable) > > > > > > + rzg2l_irqc_unmask_once(priv, hw_irq); > > > > > > + else > > > > > > + rzg2l_irqc_mask_once(priv, hw_irq); > > > > > > + raw_spin_unlock(&priv->lock); > > > > > > } > > > > > > } > > > > > > > > > > > > @@ -324,8 +427,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops > > > > > > = { 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_mask = rzg2l_irqc_mask, > > > > > > + .irq_unmask = rzg2l_irqc_unmask, > > > > > > > > > > I feel this will be clean, if we have > > > > > > > > > > static const struct irq_chip rzg2l_irqc_chip = { > > > > > .name = "rzg2l-irqc", > > > > > ... > > > > > .irq_mask = irq_chip_mask_parent, > > > > > .irq_unmask = irq_chip_unmask_parent, > > > > > .... > > > > > }; > > > > > > > > > > static const struct irq_chip rzfive_irqc_chip = { > > > > > .name = "rzfive-irqc", > > > > > ... > > > > > .irq_mask = rzfive_irqc_mask, > > > > > .irq_unmask = rzfive_irqc_unmask, > > > > > .... > > > > > }; > > > > > > > > > > And passing this in rzg2l_irqc_init() and rzfive_irqc_init(), see > > > > > below > > > > > > > > > > return rzg2l_irqc_init_helper(node, parent, & rzg2l_irqc_chip); return > > > > > rzg2l_irqc_init_helper(node, parent, & rzfive_irqc_chip); > > > > > > > > > If we do the above we are stuck with "struct irq_chip" as data, for further upcoming SoCs (for > > > > example RZ/V2H) which have more features we need to pass custom data to handle these features. > > > > > > That time device data can be extended like below > > > > > > struct rz_g2l_irq_chip { > > > struct irq_chip; > > > void *data; /* custom data */ > > > } > > > > > Ok, but i'll wait for Geert to come back on this as Geert suggested to > > me to do it this way. > > I agree with Biju. > > Having separate irq_chips lets us avoid taking the spinlock on RZ/G2L. > Agreed, I will add separate irq_chips. Cheers, Prabhakar
Hi Prabhakar, On Fri, Apr 19, 2024 at 9:15 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Thu, Apr 18, 2024 at 4:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Wed, Apr 3, 2024 at 10:36 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared > > > to the RZ/G2L (family) SoC. > > > > > > Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC > > > controller driver. Two new registers, IMSK and TMSK, are defined to > > > handle masking on RZ/Five SoC. The implementation utilizes a new data > > > structure, `struct rzg2l_irqc_data`, to determine mask support for a > > > specific controller instance. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > v1->v2 > > > - Added IRQCHIP_MATCH() for RZ/Five > > > - Retaining a copy of OF data in priv > > > - Rebased the changes > > > > Thanks for the update! > > > > > --- a/drivers/irqchip/irq-renesas-rzg2l.c > > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > > > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache { > > > u32 titsr[2]; > > > }; > > > > > > +/** > > > + * struct rzg2l_irqc_of_data - OF data structure > > > + * @mask_supported: Indicates if mask registers are available > > > + */ > > > +struct rzg2l_irqc_of_data { > > > + bool mask_supported; > > > +}; > > > + > > > /** > > > * struct rzg2l_irqc_priv - IRQ controller private data structure > > > * @base: Controller's base address > > > + * @data: OF data pointer > > > * @fwspec: IRQ firmware specific data > > > * @lock: Lock to serialize access to hardware registers > > > * @cache: Registers cache for suspend/resume > > > */ > > > static struct rzg2l_irqc_priv { > > > void __iomem *base; > > > + const struct rzg2l_irqc_of_data *data; > > > > That's not a copy, but a pointer. > > > Oops, should that be OK or shall I create a copy instead? If you would use a copy, all SoC-specific rzg2l_irqc_of_data structures could become __initconst. However, depending on how far you want to go with the irq_chip separation, you may no longer need this field anyway. Gr{oetje,eeting}s, Geert
diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c index f6484bf15e0b..6fa8d65605dc 100644 --- a/drivers/irqchip/irq-renesas-rzg2l.c +++ b/drivers/irqchip/irq-renesas-rzg2l.c @@ -37,6 +37,8 @@ #define TSSEL_SHIFT(n) (8 * (n)) #define TSSEL_MASK GENMASK(7, 0) #define IRQ_MASK 0x3 +#define IMSK 0x10010 +#define TMSK 0x10020 #define TSSR_OFFSET(n) ((n) % 4) #define TSSR_INDEX(n) ((n) / 4) @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache { u32 titsr[2]; }; +/** + * struct rzg2l_irqc_of_data - OF data structure + * @mask_supported: Indicates if mask registers are available + */ +struct rzg2l_irqc_of_data { + bool mask_supported; +}; + /** * struct rzg2l_irqc_priv - IRQ controller private data structure * @base: Controller's base address + * @data: OF data pointer * @fwspec: IRQ firmware specific data * @lock: Lock to serialize access to hardware registers * @cache: Registers cache for suspend/resume */ static struct rzg2l_irqc_priv { void __iomem *base; + const struct rzg2l_irqc_of_data *data; struct irq_fwspec fwspec[IRQC_NUM_IRQ]; raw_spinlock_t lock; struct rzg2l_irqc_reg_cache cache; @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d) irq_chip_eoi_parent(d); } +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv, + unsigned int hwirq) +{ + u32 imsk = readl_relaxed(priv->base + IMSK); + u32 bit = BIT(hwirq - IRQC_IRQ_START); + + writel_relaxed(imsk | bit, priv->base + IMSK); +} + +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv, + unsigned int hwirq) +{ + u32 imsk = readl_relaxed(priv->base + IMSK); + u32 bit = BIT(hwirq - IRQC_IRQ_START); + + writel_relaxed(imsk & ~bit, priv->base + IMSK); +} + +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv, + unsigned int hwirq) +{ + u32 tmsk = readl_relaxed(priv->base + TMSK); + u32 bit = BIT(hwirq - IRQC_TINT_START); + + writel_relaxed(tmsk | bit, priv->base + TMSK); +} + +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv, + unsigned int hwirq) +{ + u32 tmsk = readl_relaxed(priv->base + TMSK); + u32 bit = BIT(hwirq - IRQC_TINT_START); + + writel_relaxed(tmsk & ~bit, priv->base + TMSK); +} + +/* Must be called while priv->lock is held */ +static void rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq) +{ + if (!priv->data->mask_supported) + return; + + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) + rzg2l_irqc_mask_irq_interrupt(priv, hwirq); + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); +} + +static void rzg2l_irqc_mask(struct irq_data *d) +{ + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); + + raw_spin_lock(&priv->lock); + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d)); + raw_spin_unlock(&priv->lock); + irq_chip_mask_parent(d); +} + +/* Must be called while priv->lock is held */ +static void rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq) +{ + if (!priv->data->mask_supported) + return; + + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT) + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq); + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ) + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); +} + +static void rzg2l_irqc_unmask(struct irq_data *d) +{ + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d); + + raw_spin_lock(&priv->lock); + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d)); + raw_spin_unlock(&priv->lock); + irq_chip_unmask_parent(d); +} + static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) { + struct rzg2l_irqc_priv *priv = irq_data_to_priv(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); + if (enable) + rzg2l_irqc_unmask_once(priv, hw_irq); + else + rzg2l_irqc_mask_once(priv, hw_irq); reg = readl_relaxed(priv->base + TSSR(tssr_index)); if (enable) reg |= TIEN << TSSEL_SHIFT(tssr_offset); @@ -157,6 +253,13 @@ static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset)); writel_relaxed(reg, priv->base + TSSR(tssr_index)); raw_spin_unlock(&priv->lock); + } else { + raw_spin_lock(&priv->lock); + if (enable) + rzg2l_irqc_unmask_once(priv, hw_irq); + else + rzg2l_irqc_mask_once(priv, hw_irq); + raw_spin_unlock(&priv->lock); } } @@ -324,8 +427,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops = { 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_mask = rzg2l_irqc_mask, + .irq_unmask = rzg2l_irqc_unmask, .irq_disable = rzg2l_irqc_irq_disable, .irq_enable = rzg2l_irqc_irq_enable, .irq_get_irqchip_state = irq_chip_get_parent_state, @@ -401,7 +504,16 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv, return 0; } -static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent) +static const struct rzg2l_irqc_of_data rzg2l_irqc_mask_supported_data = { + .mask_supported = true, +}; + +static const struct rzg2l_irqc_of_data rzg2l_irqc_default_data = { + .mask_supported = false, +}; + +static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent, + const struct rzg2l_irqc_of_data *of_data) { struct irq_domain *irq_domain, *parent_domain; struct platform_device *pdev; @@ -422,6 +534,8 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent) if (!rzg2l_irqc_data) return -ENOMEM; + rzg2l_irqc_data->data = of_data; + rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL); if (IS_ERR(rzg2l_irqc_data->base)) return PTR_ERR(rzg2l_irqc_data->base); @@ -472,8 +586,21 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent) return ret; } +static int __init rzg2l_irqc_default_init(struct device_node *node, + struct device_node *parent) +{ + return rzg2l_irqc_init(node, parent, &rzg2l_irqc_default_data); +} + +static int __init rzg2l_irqc_mask_supported_init(struct device_node *node, + struct device_node *parent) +{ + return rzg2l_irqc_init(node, parent, &rzg2l_irqc_mask_supported_data); +} + IRQCHIP_PLATFORM_DRIVER_BEGIN(rzg2l_irqc) -IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_init) +IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_default_init) +IRQCHIP_MATCH("renesas,r9a07g043f-irqc", rzg2l_irqc_mask_supported_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");