* [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver @ 2019-07-07 13:22 Aleix Roca Nonell 2019-07-07 13:27 ` Andreas Färber 2019-07-08 9:36 ` Marc Zyngier 0 siblings, 2 replies; 6+ messages in thread From: Aleix Roca Nonell @ 2019-07-07 13:22 UTC (permalink / raw) To: Andreas Färber, Rob Herring, Mark Rutland, Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Matthias Brugger, devicetree, linux-kernel, linux-arm-kernel This driver adds support for the RTD1296 and RTD1295 interrupt controller (intc). It is based on both the BPI-SINOVOIP project and Andreas Färber's previous attempt to submit a similar driver. There is currently no publicly available datasheet on this SoC and the exact behaviour of the registers controlling the intc remain uncertain. This driver controls two intcs: "iso" and "misc". Each intc has its own Interrupt Enable Register (IER) and Interrupt Status Resgister (ISR). However, not all "misc" intc irqs have the same offsets for both ISR and IER. For this reason an ISR to IER offsets table is defined. The driver catches the IER value to reduce accesses to the table inside the interrupt handler. Actually, the driver stores the ISR offsets of currently enabled interrupts in a variable. Signed-off-by: Aleix Roca Nonell <kernelrocks@gmail.com> --- drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-rtd129x.c | 371 ++++++++++++++++++++++++++++++++++ 2 files changed, 372 insertions(+) create mode 100644 drivers/irqchip/irq-rtd129x.c diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 606a003a0000..0689c3956250 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -100,3 +100,4 @@ obj-$(CONFIG_MADERA_IRQ) += irq-madera.o obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o obj-$(CONFIG_TI_SCI_INTA_IRQCHIP) += irq-ti-sci-inta.o +obj-$(CONFIG_ARCH_REALTEK) += irq-rtd129x.o diff --git a/drivers/irqchip/irq-rtd129x.c b/drivers/irqchip/irq-rtd129x.c new file mode 100644 index 000000000000..76358ca50f10 --- /dev/null +++ b/drivers/irqchip/irq-rtd129x.c @@ -0,0 +1,371 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/irqchip.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/irqdomain.h> +#include <linux/io.h> +#include <linux/spinlock.h> +#include <linux/irqchip.h> +#include <linux/bits.h> +#include <linux/irqchip/chained_irq.h> + +#define RTD129X_INTC_NR_IRQS 32 +#define DEV_NAME "RTD1296_INTC" + +/* + * This interrupt controller (hereinafter intc) driver controls two intcs: "iso" + * and "misc". Each intc has its own Interrupt Enable Register (IER) and + * Interrupt Status Resgister (ISR). However, not all "misc" intc irqs have the + * same offsets for both ISR and IER. For this reason an ISR to IER offsets + * table is defined. Also, to reduce accesses to this table in the interrupt + * handler, the driver stores the ISR offsets of currently enabled interrupts in + * a variable. + */ + +enum misc_int_en { + MISC_INT_FAIL = 0xFF, + MISC_INT_RVD = 0xFE, + MISC_INT_EN_FAN = 29, + MISC_INT_EN_I2C3 = 28, + MISC_INT_EN_GSPI = 27, + MISC_INT_EN_I2C2 = 26, + MISC_INT_EN_SC0 = 24, + MISC_INT_EN_LSADC1 = 22, + MISC_INT_EN_LSADC0 = 21, + MISC_INT_EN_GPIODA = 20, + MISC_INT_EN_GPIOA = 19, + MISC_INT_EN_I2C4 = 15, + MISC_INT_EN_I2C5 = 14, + MISC_INT_EN_RTC_DATA = 12, + MISC_INT_EN_RTC_HOUR = 11, + MISC_INT_EN_RTC_MIN = 10, + MISC_INT_EN_UR2 = 7, + MISC_INT_EN_UR2_TO = 6, + MISC_INT_EN_UR1_TO = 5, + MISC_INT_EN_UR1 = 3, +}; + +enum iso_int_en { + ISO_INT_FAIL = 0xFF, + ISO_INT_RVD = 0xFE, + ISO_INT_EN_I2C1_REQ = 31, + ISO_INT_EN_GPHY_AV = 30, + ISO_INT_EN_GPHY_DV = 29, + ISO_INT_EN_GPIODA = 20, + ISO_INT_EN_GPIOA = 19, + ISO_INT_EN_RTC_ALARM = 13, + ISO_INT_EN_RTC_HSEC = 12, + ISO_INT_EN_I2C1 = 11, + ISO_INT_EN_I2C0 = 8, + ISO_INT_EN_IRDA = 5, + ISO_INT_EN_UR0 = 2, +}; + +unsigned char rtd129x_intc_enable_map_misc[RTD129X_INTC_NR_IRQS] = { + MISC_INT_FAIL, /* Bit0 */ + MISC_INT_FAIL, /* Bit1 */ + MISC_INT_RVD, /* Bit2 */ + MISC_INT_EN_UR1, /* Bit3 */ + MISC_INT_FAIL, /* Bit4 */ + MISC_INT_EN_UR1_TO, /* Bit5 */ + MISC_INT_RVD, /* Bit6 */ + MISC_INT_RVD, /* Bit7 */ + MISC_INT_EN_UR2, /* Bit8 */ + MISC_INT_RVD, /* Bit9 */ + MISC_INT_EN_RTC_MIN, /* Bit10 */ + MISC_INT_EN_RTC_HOUR, /* Bit11 */ + MISC_INT_EN_RTC_DATA, /* Bit12 */ + MISC_INT_EN_UR2_TO, /* Bit13 */ + MISC_INT_EN_I2C5, /* Bit14 */ + MISC_INT_EN_I2C4, /* Bit15 */ + MISC_INT_FAIL, /* Bit16 */ + MISC_INT_FAIL, /* Bit17 */ + MISC_INT_FAIL, /* Bit18 */ + MISC_INT_EN_GPIOA, /* Bit19 */ + MISC_INT_EN_GPIODA, /* Bit20 */ + MISC_INT_EN_LSADC0, /* Bit21 */ + MISC_INT_EN_LSADC1, /* Bit22 */ + MISC_INT_EN_I2C3, /* Bit23 */ + MISC_INT_EN_SC0, /* Bit24 */ + MISC_INT_FAIL, /* Bit25 */ + MISC_INT_EN_I2C2, /* Bit26 */ + MISC_INT_EN_GSPI, /* Bit27 */ + MISC_INT_FAIL, /* Bit28 */ + MISC_INT_EN_FAN, /* Bit29 */ + MISC_INT_FAIL, /* Bit30 */ + MISC_INT_FAIL /* Bit31 */ +}; + +unsigned char rtd129x_intc_enable_map_iso[RTD129X_INTC_NR_IRQS] = { + ISO_INT_FAIL, /* Bit0 */ + ISO_INT_RVD, /* Bit1 */ + ISO_INT_EN_UR0, /* Bit2 */ + ISO_INT_FAIL, /* Bit3 */ + ISO_INT_FAIL, /* Bit4 */ + ISO_INT_EN_IRDA, /* Bit5 */ + ISO_INT_FAIL, /* Bit6 */ + ISO_INT_RVD, /* Bit7 */ + ISO_INT_EN_I2C0, /* Bit8 */ + ISO_INT_RVD, /* Bit9 */ + ISO_INT_FAIL, /* Bit10 */ + ISO_INT_EN_I2C1, /* Bit11 */ + ISO_INT_EN_RTC_HSEC, /* Bit12 */ + ISO_INT_EN_RTC_ALARM, /* Bit13 */ + ISO_INT_FAIL, /* Bit14 */ + ISO_INT_FAIL, /* Bit15 */ + ISO_INT_FAIL, /* Bit16 */ + ISO_INT_FAIL, /* Bit17 */ + ISO_INT_FAIL, /* Bit18 */ + ISO_INT_EN_GPIOA, /* Bit19 */ + ISO_INT_EN_GPIODA, /* Bit20 */ + ISO_INT_RVD, /* Bit21 */ + ISO_INT_RVD, /* Bit22 */ + ISO_INT_RVD, /* Bit23 */ + ISO_INT_RVD, /* Bit24 */ + ISO_INT_FAIL, /* Bit25 */ + ISO_INT_FAIL, /* Bit26 */ + ISO_INT_FAIL, /* Bit27 */ + ISO_INT_FAIL, /* Bit28 */ + ISO_INT_EN_GPHY_DV, /* Bit29 */ + ISO_INT_EN_GPHY_AV, /* Bit30 */ + ISO_INT_EN_I2C1_REQ /* Bit31 */ +}; + +struct rtd129x_intc_data { + void __iomem *unmask; + void __iomem *isr; + void __iomem *ier; + u32 ier_cached; + u32 isr_en; + raw_spinlock_t lock; + unsigned int parent_irq; + const unsigned char *en_map; +}; + +static struct irq_domain *rtd129x_intc_domain; + +static void rtd129x_intc_irq_handle(struct irq_desc *desc) +{ + struct rtd129x_intc_data *priv = irq_desc_get_handler_data(desc); + struct irq_chip *chip = irq_desc_get_chip(desc); + unsigned int local_irq; + u32 status; + int i; + + chained_irq_enter(chip, desc); + + raw_spin_lock(&priv->lock); + status = readl_relaxed(priv->isr); + status &= priv->isr_en; + raw_spin_unlock(&priv->lock); + + while (status) { + i = __ffs(status); + status &= ~BIT(i); + + local_irq = irq_find_mapping(rtd129x_intc_domain, i); + if (likely(local_irq)) { + if (!generic_handle_irq(local_irq)) + writel_relaxed(BIT(i), priv->isr); + } else { + handle_bad_irq(desc); + } + } + + chained_irq_exit(chip, desc); +} + +static void rtd129x_intc_mask(struct irq_data *data) +{ + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); + + writel_relaxed(BIT(data->hwirq), priv->isr); +} + +static void rtd129x_intc_unmask(struct irq_data *data) +{ + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); + + writel_relaxed(BIT(data->hwirq), priv->unmask); +} + +static void rtd129x_intc_enable(struct irq_data *data) +{ + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); + unsigned long flags; + u8 en_offset; + + en_offset = priv->en_map[data->hwirq]; + + if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) { + raw_spin_lock_irqsave(&priv->lock, flags); + + priv->isr_en |= BIT(data->hwirq); + priv->ier_cached |= BIT(en_offset); + writel_relaxed(priv->ier_cached, priv->ier); + + raw_spin_unlock_irqrestore(&priv->lock, flags); + } else if (en_offset == MISC_INT_FAIL) { + pr_err("[%s] Enable irq(%lu) failed\n", DEV_NAME, data->hwirq); + } +} + +static void rtd129x_intc_disable(struct irq_data *data) +{ + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); + unsigned long flags; + u8 en_offset; + + en_offset = priv->en_map[data->hwirq]; + + if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) { + raw_spin_lock_irqsave(&priv->lock, flags); + + priv->isr_en &= ~BIT(data->hwirq); + priv->ier_cached &= ~BIT(en_offset); + writel_relaxed(priv->ier_cached, priv->ier); + + raw_spin_unlock_irqrestore(&priv->lock, flags); + } else if (en_offset == MISC_INT_FAIL) { + pr_err("[%s] Disable irq(%lu) failed\n", DEV_NAME, data->hwirq); + } +} + +static struct irq_chip rtd129x_intc_chip = { + .name = DEV_NAME, + .irq_mask = rtd129x_intc_mask, + .irq_unmask = rtd129x_intc_unmask, + .irq_enable = rtd129x_intc_enable, + .irq_disable = rtd129x_intc_disable, +}; + +static int rtd129x_intc_map(struct irq_domain *d, unsigned int virq, + irq_hw_number_t hw_irq) +{ + struct rtd129x_intc_data *priv = d->host_data; + + irq_set_chip_and_handler(virq, &rtd129x_intc_chip, handle_level_irq); + irq_set_chip_data(virq, priv); + irq_set_probe(virq); + + return 0; +} + +static const struct irq_domain_ops rtd129x_intc_domain_ops = { + .xlate = irq_domain_xlate_onecell, + .map = rtd129x_intc_map, +}; + +static const struct of_device_id rtd129x_intc_matches[] = { + { .compatible = "realtek,rtd129x-intc-misc", + .data = rtd129x_intc_enable_map_misc + }, + { .compatible = "realtek,rtd129x-intc-iso", + .data = rtd129x_intc_enable_map_iso + }, + { } +}; + +static int rtd129x_intc_of_init(struct device_node *node, + struct device_node *parent) +{ + struct rtd129x_intc_data *priv; + const struct of_device_id *match; + u32 isr_tmp, ier_tmp, ier_bit; + int ret, i; + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) { + ret = -ENOMEM; + goto err; + } + + raw_spin_lock_init(&priv->lock); + + priv->isr = of_iomap(node, 0); + if (!priv->isr) { + pr_err("unable to obtain status reg iomap address\n"); + ret = -ENOMEM; + goto free_priv; + } + + priv->ier = of_iomap(node, 1); + if (!priv->ier) { + pr_err("unable to obtain enable reg iomap address\n"); + ret = -ENOMEM; + goto iounmap_status; + } + + priv->unmask = of_iomap(node, 2); + if (!priv->unmask) { + pr_err("unable to obtain unmask reg iomap address\n"); + ret = -ENOMEM; + goto iounmap_enable; + } + + priv->parent_irq = irq_of_parse_and_map(node, 0); + if (!priv->parent_irq) { + pr_err("failed to map parent interrupt %d\n", priv->parent_irq); + ret = -EINVAL; + goto iounmap_all; + } + + match = of_match_node(rtd129x_intc_matches, node); + if (!match) { + pr_err("failed to find matching node\n"); + ret = -ENODEV; + goto iounmap_all; + } + priv->en_map = match->data; + + // initialize enabled irq's map to its matching status bit in isr by + // inverse walking the enable to status offsets map. Only needed for + // misc + priv->ier_cached = readl_relaxed(priv->ier); + if (priv->en_map == rtd129x_intc_enable_map_misc) { + ier_tmp = priv->ier_cached; + isr_tmp = 0; + while (ier_tmp) { + ier_bit = __ffs(ier_tmp); + ier_tmp &= ~BIT(ier_bit); + for (i = 0; i < RTD129X_INTC_NR_IRQS; i++) + if (priv->en_map[i] == ier_bit) + isr_tmp |= BIT(i); + } + priv->isr_en = isr_tmp; + } else { + priv->isr_en = priv->ier_cached; + } + + rtd129x_intc_domain = irq_domain_add_linear(node, RTD129X_INTC_NR_IRQS, + &rtd129x_intc_domain_ops, + priv); + if (!rtd129x_intc_domain) { + pr_err("failed to create irq domain\n"); + ret = -ENOMEM; + goto iounmap_all; + } + + irq_set_chained_handler_and_data(priv->parent_irq, + rtd129x_intc_irq_handle, priv); + + return 0; + +iounmap_all: + iounmap(priv->unmask); +iounmap_enable: + iounmap(priv->ier); +iounmap_status: + iounmap(priv->isr); +free_priv: + kfree(priv); +err: + return ret; +} + +IRQCHIP_DECLARE(rtd129x_intc_misc, "realtek,rtd129x-intc-misc", + rtd129x_intc_of_init); +IRQCHIP_DECLARE(rtd129x_intc_iso, "realtek,rtd129x-intc-iso", + rtd129x_intc_of_init); -- 2.21.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver 2019-07-07 13:22 [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver Aleix Roca Nonell @ 2019-07-07 13:27 ` Andreas Färber 2019-07-07 15:46 ` Aleix Roca Nonell 2019-07-08 9:36 ` Marc Zyngier 1 sibling, 1 reply; 6+ messages in thread From: Andreas Färber @ 2019-07-07 13:27 UTC (permalink / raw) To: Aleix Roca Nonell Cc: Mark Rutland, devicetree, Jason Cooper, Marc Zyngier, linux-kernel, Rob Herring, Matthias Brugger, Thomas Gleixner, linux-arm-kernel Am 07.07.19 um 15:22 schrieb Aleix Roca Nonell: > This driver adds support for the RTD1296 and RTD1295 interrupt > controller (intc). It is based on both the BPI-SINOVOIP project and > Andreas Färber's previous attempt to submit a similar driver. Doing that without my Signed-off-by and Copyright is certainly not okay. It is also lacking a clear description of what you changed from my last submission or the post-submission GitHub version adressing review comments, which broke. Regards, Andreas -- SUSE Linux GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver 2019-07-07 13:27 ` Andreas Färber @ 2019-07-07 15:46 ` Aleix Roca Nonell 0 siblings, 0 replies; 6+ messages in thread From: Aleix Roca Nonell @ 2019-07-07 15:46 UTC (permalink / raw) To: Andreas Färber Cc: Mark Rutland, devicetree, Jason Cooper, Marc Zyngier, linux-kernel, Rob Herring, Matthias Brugger, Thomas Gleixner, linux-arm-kernel On Sun, Jul 07, 2019 at 03:27:16PM +0200, Andreas Färber wrote: > Am 07.07.19 um 15:22 schrieb Aleix Roca Nonell: > > This driver adds support for the RTD1296 and RTD1295 interrupt > > controller (intc). It is based on both the BPI-SINOVOIP project and > > Andreas Färber's previous attempt to submit a similar driver. > > Doing that without my Signed-off-by and Copyright is certainly not okay. > It is also lacking a clear description of what you changed from my last > submission or the post-submission GitHub version adressing review > comments, which broke. I'm really sorry about that, because I rewrote the code (almost) from scratch (given that I wasn't aware of this previous attempt when I started working with it) I was not sure if it was necessary. I will address this in the next version of the patch series. Thank you and sorry for the inconvenience. > > Regards, > Andreas > > -- > SUSE Linux GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah > HRB 21284 (AG Nürnberg) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver 2019-07-07 13:22 [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver Aleix Roca Nonell 2019-07-07 13:27 ` Andreas Färber @ 2019-07-08 9:36 ` Marc Zyngier 2019-08-12 8:26 ` Aleix Roca Nonell 1 sibling, 1 reply; 6+ messages in thread From: Marc Zyngier @ 2019-07-08 9:36 UTC (permalink / raw) To: Aleix Roca Nonell, Andreas Färber, Rob Herring, Mark Rutland, Thomas Gleixner, Jason Cooper Cc: Matthias Brugger, devicetree, linux-kernel, linux-arm-kernel On 07/07/2019 14:22, Aleix Roca Nonell wrote: > This driver adds support for the RTD1296 and RTD1295 interrupt > controller (intc). It is based on both the BPI-SINOVOIP project and > Andreas Färber's previous attempt to submit a similar driver. > > There is currently no publicly available datasheet on this SoC and the > exact behaviour of the registers controlling the intc remain uncertain. > > This driver controls two intcs: "iso" and "misc". Each intc has its own > Interrupt Enable Register (IER) and Interrupt Status Resgister (ISR). Register > However, not all "misc" intc irqs have the same offsets for both ISR and > IER. For this reason an ISR to IER offsets table is defined. > > The driver catches the IER value to reduce accesses to the table inside the > interrupt handler. Actually, the driver stores the ISR offsets of currently > enabled interrupts in a variable. > > Signed-off-by: Aleix Roca Nonell <kernelrocks@gmail.com> I expect Andreas and you to sort the attribution issue. I'm certainly not going to take this in if things are unclear. > --- > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-rtd129x.c | 371 ++++++++++++++++++++++++++++++++++ > 2 files changed, 372 insertions(+) > create mode 100644 drivers/irqchip/irq-rtd129x.c > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 606a003a0000..0689c3956250 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -100,3 +100,4 @@ obj-$(CONFIG_MADERA_IRQ) += irq-madera.o > obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o > obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o > obj-$(CONFIG_TI_SCI_INTA_IRQCHIP) += irq-ti-sci-inta.o > +obj-$(CONFIG_ARCH_REALTEK) += irq-rtd129x.o > diff --git a/drivers/irqchip/irq-rtd129x.c b/drivers/irqchip/irq-rtd129x.c > new file mode 100644 > index 000000000000..76358ca50f10 > --- /dev/null > +++ b/drivers/irqchip/irq-rtd129x.c > @@ -0,0 +1,371 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include <linux/irqchip.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/irqdomain.h> > +#include <linux/io.h> > +#include <linux/spinlock.h> > +#include <linux/irqchip.h> > +#include <linux/bits.h> > +#include <linux/irqchip/chained_irq.h> > + > +#define RTD129X_INTC_NR_IRQS 32 > +#define DEV_NAME "RTD1296_INTC" > + > +/* > + * This interrupt controller (hereinafter intc) driver controls two intcs: "iso" > + * and "misc". Each intc has its own Interrupt Enable Register (IER) and > + * Interrupt Status Resgister (ISR). However, not all "misc" intc irqs have the > + * same offsets for both ISR and IER. For this reason an ISR to IER offsets > + * table is defined. Also, to reduce accesses to this table in the interrupt > + * handler, the driver stores the ISR offsets of currently enabled interrupts in > + * a variable. > + */ > + > +enum misc_int_en { > + MISC_INT_FAIL = 0xFF, > + MISC_INT_RVD = 0xFE, > + MISC_INT_EN_FAN = 29, > + MISC_INT_EN_I2C3 = 28, > + MISC_INT_EN_GSPI = 27, > + MISC_INT_EN_I2C2 = 26, > + MISC_INT_EN_SC0 = 24, > + MISC_INT_EN_LSADC1 = 22, > + MISC_INT_EN_LSADC0 = 21, > + MISC_INT_EN_GPIODA = 20, > + MISC_INT_EN_GPIOA = 19, > + MISC_INT_EN_I2C4 = 15, > + MISC_INT_EN_I2C5 = 14, > + MISC_INT_EN_RTC_DATA = 12, > + MISC_INT_EN_RTC_HOUR = 11, > + MISC_INT_EN_RTC_MIN = 10, > + MISC_INT_EN_UR2 = 7, > + MISC_INT_EN_UR2_TO = 6, > + MISC_INT_EN_UR1_TO = 5, > + MISC_INT_EN_UR1 = 3, > +}; > + > +enum iso_int_en { > + ISO_INT_FAIL = 0xFF, > + ISO_INT_RVD = 0xFE, > + ISO_INT_EN_I2C1_REQ = 31, > + ISO_INT_EN_GPHY_AV = 30, > + ISO_INT_EN_GPHY_DV = 29, > + ISO_INT_EN_GPIODA = 20, > + ISO_INT_EN_GPIOA = 19, > + ISO_INT_EN_RTC_ALARM = 13, > + ISO_INT_EN_RTC_HSEC = 12, > + ISO_INT_EN_I2C1 = 11, > + ISO_INT_EN_I2C0 = 8, > + ISO_INT_EN_IRDA = 5, > + ISO_INT_EN_UR0 = 2, > +}; > + > +unsigned char rtd129x_intc_enable_map_misc[RTD129X_INTC_NR_IRQS] = { > + MISC_INT_FAIL, /* Bit0 */ > + MISC_INT_FAIL, /* Bit1 */ > + MISC_INT_RVD, /* Bit2 */ > + MISC_INT_EN_UR1, /* Bit3 */ > + MISC_INT_FAIL, /* Bit4 */ > + MISC_INT_EN_UR1_TO, /* Bit5 */ > + MISC_INT_RVD, /* Bit6 */ > + MISC_INT_RVD, /* Bit7 */ > + MISC_INT_EN_UR2, /* Bit8 */ > + MISC_INT_RVD, /* Bit9 */ > + MISC_INT_EN_RTC_MIN, /* Bit10 */ > + MISC_INT_EN_RTC_HOUR, /* Bit11 */ > + MISC_INT_EN_RTC_DATA, /* Bit12 */ > + MISC_INT_EN_UR2_TO, /* Bit13 */ > + MISC_INT_EN_I2C5, /* Bit14 */ > + MISC_INT_EN_I2C4, /* Bit15 */ > + MISC_INT_FAIL, /* Bit16 */ > + MISC_INT_FAIL, /* Bit17 */ > + MISC_INT_FAIL, /* Bit18 */ > + MISC_INT_EN_GPIOA, /* Bit19 */ > + MISC_INT_EN_GPIODA, /* Bit20 */ > + MISC_INT_EN_LSADC0, /* Bit21 */ > + MISC_INT_EN_LSADC1, /* Bit22 */ > + MISC_INT_EN_I2C3, /* Bit23 */ > + MISC_INT_EN_SC0, /* Bit24 */ > + MISC_INT_FAIL, /* Bit25 */ > + MISC_INT_EN_I2C2, /* Bit26 */ > + MISC_INT_EN_GSPI, /* Bit27 */ > + MISC_INT_FAIL, /* Bit28 */ > + MISC_INT_EN_FAN, /* Bit29 */ > + MISC_INT_FAIL, /* Bit30 */ > + MISC_INT_FAIL /* Bit31 */ > +}; > + > +unsigned char rtd129x_intc_enable_map_iso[RTD129X_INTC_NR_IRQS] = { > + ISO_INT_FAIL, /* Bit0 */ > + ISO_INT_RVD, /* Bit1 */ > + ISO_INT_EN_UR0, /* Bit2 */ > + ISO_INT_FAIL, /* Bit3 */ > + ISO_INT_FAIL, /* Bit4 */ > + ISO_INT_EN_IRDA, /* Bit5 */ > + ISO_INT_FAIL, /* Bit6 */ > + ISO_INT_RVD, /* Bit7 */ > + ISO_INT_EN_I2C0, /* Bit8 */ > + ISO_INT_RVD, /* Bit9 */ > + ISO_INT_FAIL, /* Bit10 */ > + ISO_INT_EN_I2C1, /* Bit11 */ > + ISO_INT_EN_RTC_HSEC, /* Bit12 */ > + ISO_INT_EN_RTC_ALARM, /* Bit13 */ > + ISO_INT_FAIL, /* Bit14 */ > + ISO_INT_FAIL, /* Bit15 */ > + ISO_INT_FAIL, /* Bit16 */ > + ISO_INT_FAIL, /* Bit17 */ > + ISO_INT_FAIL, /* Bit18 */ > + ISO_INT_EN_GPIOA, /* Bit19 */ > + ISO_INT_EN_GPIODA, /* Bit20 */ > + ISO_INT_RVD, /* Bit21 */ > + ISO_INT_RVD, /* Bit22 */ > + ISO_INT_RVD, /* Bit23 */ > + ISO_INT_RVD, /* Bit24 */ > + ISO_INT_FAIL, /* Bit25 */ > + ISO_INT_FAIL, /* Bit26 */ > + ISO_INT_FAIL, /* Bit27 */ > + ISO_INT_FAIL, /* Bit28 */ > + ISO_INT_EN_GPHY_DV, /* Bit29 */ > + ISO_INT_EN_GPHY_AV, /* Bit30 */ > + ISO_INT_EN_I2C1_REQ /* Bit31 */ > +}; > + > +struct rtd129x_intc_data { > + void __iomem *unmask; > + void __iomem *isr; > + void __iomem *ier; > + u32 ier_cached; > + u32 isr_en; > + raw_spinlock_t lock; > + unsigned int parent_irq; > + const unsigned char *en_map; > +}; > + > +static struct irq_domain *rtd129x_intc_domain; > + > +static void rtd129x_intc_irq_handle(struct irq_desc *desc) > +{ > + struct rtd129x_intc_data *priv = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + unsigned int local_irq; > + u32 status; > + int i; > + > + chained_irq_enter(chip, desc); > + > + raw_spin_lock(&priv->lock); > + status = readl_relaxed(priv->isr); > + status &= priv->isr_en; > + raw_spin_unlock(&priv->lock); What is this lock protecting? isr_en? > + > + while (status) { > + i = __ffs(status); > + status &= ~BIT(i); > + > + local_irq = irq_find_mapping(rtd129x_intc_domain, i); > + if (likely(local_irq)) { > + if (!generic_handle_irq(local_irq)) > + writel_relaxed(BIT(i), priv->isr); What are the write semantics of the ISR register? Hot bit clear? How does it work since mask() does the same thing? Clearly, something is wrong here. > + } else { > + handle_bad_irq(desc); > + } > + } > + > + chained_irq_exit(chip, desc); > +} > + > +static void rtd129x_intc_mask(struct irq_data *data) > +{ > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > + > + writel_relaxed(BIT(data->hwirq), priv->isr); > +} > + > +static void rtd129x_intc_unmask(struct irq_data *data) > +{ > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > + > + writel_relaxed(BIT(data->hwirq), priv->unmask); What effect does this have on the isr register? The whole mask/unmask thing seems to be pretty dodgy... > +} > + > +static void rtd129x_intc_enable(struct irq_data *data) > +{ > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > + unsigned long flags; > + u8 en_offset; > + > + en_offset = priv->en_map[data->hwirq]; > + > + if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) { > + raw_spin_lock_irqsave(&priv->lock, flags); > + > + priv->isr_en |= BIT(data->hwirq); > + priv->ier_cached |= BIT(en_offset); > + writel_relaxed(priv->ier_cached, priv->ier); > + > + raw_spin_unlock_irqrestore(&priv->lock, flags); > + } else if (en_offset == MISC_INT_FAIL) { > + pr_err("[%s] Enable irq(%lu) failed\n", DEV_NAME, data->hwirq); > + } > +} > + > +static void rtd129x_intc_disable(struct irq_data *data) > +{ > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > + unsigned long flags; > + u8 en_offset; > + > + en_offset = priv->en_map[data->hwirq]; > + > + if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) { > + raw_spin_lock_irqsave(&priv->lock, flags); > + > + priv->isr_en &= ~BIT(data->hwirq); > + priv->ier_cached &= ~BIT(en_offset); > + writel_relaxed(priv->ier_cached, priv->ier); > + > + raw_spin_unlock_irqrestore(&priv->lock, flags); > + } else if (en_offset == MISC_INT_FAIL) { > + pr_err("[%s] Disable irq(%lu) failed\n", DEV_NAME, data->hwirq); > + } > +} So here's a thought: Why do we need all of this? If mask/unmask do their job correctly, we could just enable all interrupts in one go (just a 32bit write) at probe time, and leave all interrupts masked until they are in use. You could then drop all these silly tables that don't bring much... > + > +static struct irq_chip rtd129x_intc_chip = { > + .name = DEV_NAME, > + .irq_mask = rtd129x_intc_mask, > + .irq_unmask = rtd129x_intc_unmask, > + .irq_enable = rtd129x_intc_enable, > + .irq_disable = rtd129x_intc_disable, > +}; > + > +static int rtd129x_intc_map(struct irq_domain *d, unsigned int virq, > + irq_hw_number_t hw_irq) > +{ > + struct rtd129x_intc_data *priv = d->host_data; > + > + irq_set_chip_and_handler(virq, &rtd129x_intc_chip, handle_level_irq); > + irq_set_chip_data(virq, priv); > + irq_set_probe(virq); > + > + return 0; > +} > + > +static const struct irq_domain_ops rtd129x_intc_domain_ops = { > + .xlate = irq_domain_xlate_onecell, > + .map = rtd129x_intc_map, > +}; > + > +static const struct of_device_id rtd129x_intc_matches[] = { > + { .compatible = "realtek,rtd129x-intc-misc", > + .data = rtd129x_intc_enable_map_misc > + }, > + { .compatible = "realtek,rtd129x-intc-iso", > + .data = rtd129x_intc_enable_map_iso > + }, > + { } > +}; > + > +static int rtd129x_intc_of_init(struct device_node *node, > + struct device_node *parent) > +{ > + struct rtd129x_intc_data *priv; > + const struct of_device_id *match; > + u32 isr_tmp, ier_tmp, ier_bit; > + int ret, i; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) { > + ret = -ENOMEM; > + goto err; > + } > + > + raw_spin_lock_init(&priv->lock); > + > + priv->isr = of_iomap(node, 0); > + if (!priv->isr) { > + pr_err("unable to obtain status reg iomap address\n"); > + ret = -ENOMEM; > + goto free_priv; > + } > + > + priv->ier = of_iomap(node, 1); > + if (!priv->ier) { > + pr_err("unable to obtain enable reg iomap address\n"); > + ret = -ENOMEM; > + goto iounmap_status; > + } > + > + priv->unmask = of_iomap(node, 2); > + if (!priv->unmask) { > + pr_err("unable to obtain unmask reg iomap address\n"); > + ret = -ENOMEM; > + goto iounmap_enable; > + } > + > + priv->parent_irq = irq_of_parse_and_map(node, 0); > + if (!priv->parent_irq) { > + pr_err("failed to map parent interrupt %d\n", priv->parent_irq); > + ret = -EINVAL; > + goto iounmap_all; > + } > + > + match = of_match_node(rtd129x_intc_matches, node); > + if (!match) { > + pr_err("failed to find matching node\n"); > + ret = -ENODEV; > + goto iounmap_all; > + } > + priv->en_map = match->data; > + > + // initialize enabled irq's map to its matching status bit in isr by > + // inverse walking the enable to status offsets map. Only needed for > + // misc Why do we need any of this? The kernel is supposed to start from a clean slate, not to inherit whatever has been set before, unless there is a very compelling reason. > + priv->ier_cached = readl_relaxed(priv->ier); > + if (priv->en_map == rtd129x_intc_enable_map_misc) { > + ier_tmp = priv->ier_cached; > + isr_tmp = 0; > + while (ier_tmp) { > + ier_bit = __ffs(ier_tmp); > + ier_tmp &= ~BIT(ier_bit); > + for (i = 0; i < RTD129X_INTC_NR_IRQS; i++) > + if (priv->en_map[i] == ier_bit) > + isr_tmp |= BIT(i); > + } > + priv->isr_en = isr_tmp; > + } else { > + priv->isr_en = priv->ier_cached; > + } > + > + rtd129x_intc_domain = irq_domain_add_linear(node, RTD129X_INTC_NR_IRQS, > + &rtd129x_intc_domain_ops, > + priv); > + if (!rtd129x_intc_domain) { > + pr_err("failed to create irq domain\n"); > + ret = -ENOMEM; > + goto iounmap_all; > + } > + > + irq_set_chained_handler_and_data(priv->parent_irq, > + rtd129x_intc_irq_handle, priv); > + > + return 0; > + > +iounmap_all: > + iounmap(priv->unmask); > +iounmap_enable: > + iounmap(priv->ier); > +iounmap_status: > + iounmap(priv->isr); > +free_priv: > + kfree(priv); > +err: > + return ret; > +} > + > +IRQCHIP_DECLARE(rtd129x_intc_misc, "realtek,rtd129x-intc-misc", > + rtd129x_intc_of_init); > +IRQCHIP_DECLARE(rtd129x_intc_iso, "realtek,rtd129x-intc-iso", > + rtd129x_intc_of_init); > Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver 2019-07-08 9:36 ` Marc Zyngier @ 2019-08-12 8:26 ` Aleix Roca Nonell 2019-08-13 15:15 ` Marc Zyngier 0 siblings, 1 reply; 6+ messages in thread From: Aleix Roca Nonell @ 2019-08-12 8:26 UTC (permalink / raw) To: Marc Zyngier Cc: Mark Rutland, devicetree, Jason Cooper, linux-kernel, Rob Herring, Matthias Brugger, Thomas Gleixner, Andreas Färber, linux-arm-kernel Hi Mark and everyone! Sorry for the large delay, I'm doing this in my free time, which is not that abundant. In this mail, I'm focusing only on the largest change mentioned by Mark. I will answer the rest later. On Mon, Jul 08, 2019 at 10:36:14AM +0100, Marc Zyngier wrote: > On 07/07/2019 14:22, Aleix Roca Nonell wrote: > > This driver adds support for the RTD1296 and RTD1295 interrupt > > controller (intc). It is based on both the BPI-SINOVOIP project and > > Andreas Färber's previous attempt to submit a similar driver. > > > > There is currently no publicly available datasheet on this SoC and the > > exact behaviour of the registers controlling the intc remain uncertain. > > > > This driver controls two intcs: "iso" and "misc". Each intc has its own > > Interrupt Enable Register (IER) and Interrupt Status Resgister (ISR). > > Register > > > However, not all "misc" intc irqs have the same offsets for both ISR and > > IER. For this reason an ISR to IER offsets table is defined. > > > > The driver catches the IER value to reduce accesses to the table inside the > > interrupt handler. Actually, the driver stores the ISR offsets of currently > > enabled interrupts in a variable. > > > > Signed-off-by: Aleix Roca Nonell <kernelrocks@gmail.com> > > I expect Andreas and you to sort the attribution issue. I'm certainly > not going to take this in if things are unclear. > > > --- > > drivers/irqchip/Makefile | 1 + > > drivers/irqchip/irq-rtd129x.c | 371 ++++++++++++++++++++++++++++++++++ > > 2 files changed, 372 insertions(+) > > create mode 100644 drivers/irqchip/irq-rtd129x.c > > > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > > index 606a003a0000..0689c3956250 100644 > > --- a/drivers/irqchip/Makefile > > +++ b/drivers/irqchip/Makefile > > @@ -100,3 +100,4 @@ obj-$(CONFIG_MADERA_IRQ) += irq-madera.o > > obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o > > obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o > > obj-$(CONFIG_TI_SCI_INTA_IRQCHIP) += irq-ti-sci-inta.o > > +obj-$(CONFIG_ARCH_REALTEK) += irq-rtd129x.o > > diff --git a/drivers/irqchip/irq-rtd129x.c b/drivers/irqchip/irq-rtd129x.c > > new file mode 100644 > > index 000000000000..76358ca50f10 > > --- /dev/null > > +++ b/drivers/irqchip/irq-rtd129x.c > > @@ -0,0 +1,371 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +#include <linux/irqchip.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/of_irq.h> > > +#include <linux/irqdomain.h> > > +#include <linux/io.h> > > +#include <linux/spinlock.h> > > +#include <linux/irqchip.h> > > +#include <linux/bits.h> > > +#include <linux/irqchip/chained_irq.h> > > + > > +#define RTD129X_INTC_NR_IRQS 32 > > +#define DEV_NAME "RTD1296_INTC" > > + > > +/* > > + * This interrupt controller (hereinafter intc) driver controls two intcs: "iso" > > + * and "misc". Each intc has its own Interrupt Enable Register (IER) and > > + * Interrupt Status Resgister (ISR). However, not all "misc" intc irqs have the > > + * same offsets for both ISR and IER. For this reason an ISR to IER offsets > > + * table is defined. Also, to reduce accesses to this table in the interrupt > > + * handler, the driver stores the ISR offsets of currently enabled interrupts in > > + * a variable. > > + */ > > + > > +enum misc_int_en { > > + MISC_INT_FAIL = 0xFF, > > + MISC_INT_RVD = 0xFE, > > + MISC_INT_EN_FAN = 29, > > + MISC_INT_EN_I2C3 = 28, > > + MISC_INT_EN_GSPI = 27, > > + MISC_INT_EN_I2C2 = 26, > > + MISC_INT_EN_SC0 = 24, > > + MISC_INT_EN_LSADC1 = 22, > > + MISC_INT_EN_LSADC0 = 21, > > + MISC_INT_EN_GPIODA = 20, > > + MISC_INT_EN_GPIOA = 19, > > + MISC_INT_EN_I2C4 = 15, > > + MISC_INT_EN_I2C5 = 14, > > + MISC_INT_EN_RTC_DATA = 12, > > + MISC_INT_EN_RTC_HOUR = 11, > > + MISC_INT_EN_RTC_MIN = 10, > > + MISC_INT_EN_UR2 = 7, > > + MISC_INT_EN_UR2_TO = 6, > > + MISC_INT_EN_UR1_TO = 5, > > + MISC_INT_EN_UR1 = 3, > > +}; > > + > > +enum iso_int_en { > > + ISO_INT_FAIL = 0xFF, > > + ISO_INT_RVD = 0xFE, > > + ISO_INT_EN_I2C1_REQ = 31, > > + ISO_INT_EN_GPHY_AV = 30, > > + ISO_INT_EN_GPHY_DV = 29, > > + ISO_INT_EN_GPIODA = 20, > > + ISO_INT_EN_GPIOA = 19, > > + ISO_INT_EN_RTC_ALARM = 13, > > + ISO_INT_EN_RTC_HSEC = 12, > > + ISO_INT_EN_I2C1 = 11, > > + ISO_INT_EN_I2C0 = 8, > > + ISO_INT_EN_IRDA = 5, > > + ISO_INT_EN_UR0 = 2, > > +}; > > + > > +unsigned char rtd129x_intc_enable_map_misc[RTD129X_INTC_NR_IRQS] = { > > + MISC_INT_FAIL, /* Bit0 */ > > + MISC_INT_FAIL, /* Bit1 */ > > + MISC_INT_RVD, /* Bit2 */ > > + MISC_INT_EN_UR1, /* Bit3 */ > > + MISC_INT_FAIL, /* Bit4 */ > > + MISC_INT_EN_UR1_TO, /* Bit5 */ > > + MISC_INT_RVD, /* Bit6 */ > > + MISC_INT_RVD, /* Bit7 */ > > + MISC_INT_EN_UR2, /* Bit8 */ > > + MISC_INT_RVD, /* Bit9 */ > > + MISC_INT_EN_RTC_MIN, /* Bit10 */ > > + MISC_INT_EN_RTC_HOUR, /* Bit11 */ > > + MISC_INT_EN_RTC_DATA, /* Bit12 */ > > + MISC_INT_EN_UR2_TO, /* Bit13 */ > > + MISC_INT_EN_I2C5, /* Bit14 */ > > + MISC_INT_EN_I2C4, /* Bit15 */ > > + MISC_INT_FAIL, /* Bit16 */ > > + MISC_INT_FAIL, /* Bit17 */ > > + MISC_INT_FAIL, /* Bit18 */ > > + MISC_INT_EN_GPIOA, /* Bit19 */ > > + MISC_INT_EN_GPIODA, /* Bit20 */ > > + MISC_INT_EN_LSADC0, /* Bit21 */ > > + MISC_INT_EN_LSADC1, /* Bit22 */ > > + MISC_INT_EN_I2C3, /* Bit23 */ > > + MISC_INT_EN_SC0, /* Bit24 */ > > + MISC_INT_FAIL, /* Bit25 */ > > + MISC_INT_EN_I2C2, /* Bit26 */ > > + MISC_INT_EN_GSPI, /* Bit27 */ > > + MISC_INT_FAIL, /* Bit28 */ > > + MISC_INT_EN_FAN, /* Bit29 */ > > + MISC_INT_FAIL, /* Bit30 */ > > + MISC_INT_FAIL /* Bit31 */ > > +}; > > + > > +unsigned char rtd129x_intc_enable_map_iso[RTD129X_INTC_NR_IRQS] = { > > + ISO_INT_FAIL, /* Bit0 */ > > + ISO_INT_RVD, /* Bit1 */ > > + ISO_INT_EN_UR0, /* Bit2 */ > > + ISO_INT_FAIL, /* Bit3 */ > > + ISO_INT_FAIL, /* Bit4 */ > > + ISO_INT_EN_IRDA, /* Bit5 */ > > + ISO_INT_FAIL, /* Bit6 */ > > + ISO_INT_RVD, /* Bit7 */ > > + ISO_INT_EN_I2C0, /* Bit8 */ > > + ISO_INT_RVD, /* Bit9 */ > > + ISO_INT_FAIL, /* Bit10 */ > > + ISO_INT_EN_I2C1, /* Bit11 */ > > + ISO_INT_EN_RTC_HSEC, /* Bit12 */ > > + ISO_INT_EN_RTC_ALARM, /* Bit13 */ > > + ISO_INT_FAIL, /* Bit14 */ > > + ISO_INT_FAIL, /* Bit15 */ > > + ISO_INT_FAIL, /* Bit16 */ > > + ISO_INT_FAIL, /* Bit17 */ > > + ISO_INT_FAIL, /* Bit18 */ > > + ISO_INT_EN_GPIOA, /* Bit19 */ > > + ISO_INT_EN_GPIODA, /* Bit20 */ > > + ISO_INT_RVD, /* Bit21 */ > > + ISO_INT_RVD, /* Bit22 */ > > + ISO_INT_RVD, /* Bit23 */ > > + ISO_INT_RVD, /* Bit24 */ > > + ISO_INT_FAIL, /* Bit25 */ > > + ISO_INT_FAIL, /* Bit26 */ > > + ISO_INT_FAIL, /* Bit27 */ > > + ISO_INT_FAIL, /* Bit28 */ > > + ISO_INT_EN_GPHY_DV, /* Bit29 */ > > + ISO_INT_EN_GPHY_AV, /* Bit30 */ > > + ISO_INT_EN_I2C1_REQ /* Bit31 */ > > +}; > > + > > +struct rtd129x_intc_data { > > + void __iomem *unmask; > > + void __iomem *isr; > > + void __iomem *ier; > > + u32 ier_cached; > > + u32 isr_en; > > + raw_spinlock_t lock; > > + unsigned int parent_irq; > > + const unsigned char *en_map; > > +}; > > + > > +static struct irq_domain *rtd129x_intc_domain; > > + > > +static void rtd129x_intc_irq_handle(struct irq_desc *desc) > > +{ > > + struct rtd129x_intc_data *priv = irq_desc_get_handler_data(desc); > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > + unsigned int local_irq; > > + u32 status; > > + int i; > > + > > + chained_irq_enter(chip, desc); > > + > > + raw_spin_lock(&priv->lock); > > + status = readl_relaxed(priv->isr); > > + status &= priv->isr_en; > > + raw_spin_unlock(&priv->lock); > > What is this lock protecting? isr_en? > > > + > > + while (status) { > > + i = __ffs(status); > > + status &= ~BIT(i); > > + > > + local_irq = irq_find_mapping(rtd129x_intc_domain, i); > > + if (likely(local_irq)) { > > + if (!generic_handle_irq(local_irq)) > > + writel_relaxed(BIT(i), priv->isr); > > What are the write semantics of the ISR register? Hot bit clear? How > does it work since mask() does the same thing? Clearly, something is > wrong here. Sorry but I have not been able to found the definition of "hot bit clear", could you explain it? Anyways, you were right, apparently the mask/unmask code were doing nothing useful. More on this below. > > > + } else { > > + handle_bad_irq(desc); > > + } > > + } > > + > > + chained_irq_exit(chip, desc); > > +} > > + > > +static void rtd129x_intc_mask(struct irq_data *data) > > +{ > > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > > + > > + writel_relaxed(BIT(data->hwirq), priv->isr); > > +} > > + > > +static void rtd129x_intc_unmask(struct irq_data *data) > > +{ > > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > > + > > + writel_relaxed(BIT(data->hwirq), priv->unmask); > > What effect does this have on the isr register? The whole mask/unmask > thing seems to be pretty dodgy... > > > +} > > + > > +static void rtd129x_intc_enable(struct irq_data *data) > > +{ > > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > > + unsigned long flags; > > + u8 en_offset; > > + > > + en_offset = priv->en_map[data->hwirq]; > > + > > + if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) { > > + raw_spin_lock_irqsave(&priv->lock, flags); > > + > > + priv->isr_en |= BIT(data->hwirq); > > + priv->ier_cached |= BIT(en_offset); > > + writel_relaxed(priv->ier_cached, priv->ier); > > + > > + raw_spin_unlock_irqrestore(&priv->lock, flags); > > + } else if (en_offset == MISC_INT_FAIL) { > > + pr_err("[%s] Enable irq(%lu) failed\n", DEV_NAME, data->hwirq); > > + } > > +} > > + > > +static void rtd129x_intc_disable(struct irq_data *data) > > +{ > > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > > + unsigned long flags; > > + u8 en_offset; > > + > > + en_offset = priv->en_map[data->hwirq]; > > + > > + if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) { > > + raw_spin_lock_irqsave(&priv->lock, flags); > > + > > + priv->isr_en &= ~BIT(data->hwirq); > > + priv->ier_cached &= ~BIT(en_offset); > > + writel_relaxed(priv->ier_cached, priv->ier); > > + > > + raw_spin_unlock_irqrestore(&priv->lock, flags); > > + } else if (en_offset == MISC_INT_FAIL) { > > + pr_err("[%s] Disable irq(%lu) failed\n", DEV_NAME, data->hwirq); > > + } > > +} > > So here's a thought: Why do we need all of this? If mask/unmask do their > job correctly, we could just enable all interrupts in one go (just a > 32bit write) at probe time, and leave all interrupts masked until they > are in use. You could then drop all these silly tables that don't bring > much... The idea of dropping all those tables look really good to me, that would greatly simplify the code! I have been trying to mask all interrupts on the probe function using the ISR register but while doing so, I realized that it does not work. Writing to ISR does not mask interrupts, apparently it only acknowledges them once they have been triggered. On the scarse available documentation of this Soc I cannot find a mask-like register. It seems interrupts are managed with an ISR and an IER register. So it should be posible to use the enable register to maks/unmask instead. These do work. However, that would mean that we have to keep those ugly tables. Nonetheless we might still be able to do something else. Please, correct me if I'm wrong, but do we really need to mask/unamsk in this scenario? This is the devised board layout: +------+ +----------+ +---------+ | | | | | | | UART |-------|2 INTC |-------|c GIC | | | +----|1 | +----|b | +------+ | +--|0 | | +--|a | | | | | | | | | | | +----------+ | | +---------+ | | Once the UART generates an interrupt it passes through the line 2 of the custom realtek interrupt contoller before reaching the GIC's line "c". On the INTC interrupt handler, we call chained_irq_enter/exit to mask/unmask the GIC's "c" line. Because all of this realtek INTC interrupt lines (2,1,0,...) are muxed on the GIC's line "c", this means that while on the INTC interrupt handler it is not possible to send further interrupts on the CPU. Given that interrupts are masked on the GIC, it seems safe to just remove INTC's mask/unmask functions. Therefore, the only work that this INTC handler would needs to do is to acknowledge the interrupt by writing to the ISR, which it could be done in the respective irq_ack callback of struct irq_chip instead of in the handler body. I have implemented this solution and it seems to work. What do you think? I'm missing something crucial? > > > + > > +static struct irq_chip rtd129x_intc_chip = { > > + .name = DEV_NAME, > > + .irq_mask = rtd129x_intc_mask, > > + .irq_unmask = rtd129x_intc_unmask, > > + .irq_enable = rtd129x_intc_enable, > > + .irq_disable = rtd129x_intc_disable, > > +}; > > + > > +static int rtd129x_intc_map(struct irq_domain *d, unsigned int virq, > > + irq_hw_number_t hw_irq) > > +{ > > + struct rtd129x_intc_data *priv = d->host_data; > > + > > + irq_set_chip_and_handler(virq, &rtd129x_intc_chip, handle_level_irq); > > + irq_set_chip_data(virq, priv); > > + irq_set_probe(virq); > > + > > + return 0; > > +} > > + > > +static const struct irq_domain_ops rtd129x_intc_domain_ops = { > > + .xlate = irq_domain_xlate_onecell, > > + .map = rtd129x_intc_map, > > +}; > > + > > +static const struct of_device_id rtd129x_intc_matches[] = { > > + { .compatible = "realtek,rtd129x-intc-misc", > > + .data = rtd129x_intc_enable_map_misc > > + }, > > + { .compatible = "realtek,rtd129x-intc-iso", > > + .data = rtd129x_intc_enable_map_iso > > + }, > > + { } > > +}; > > + > > +static int rtd129x_intc_of_init(struct device_node *node, > > + struct device_node *parent) > > +{ > > + struct rtd129x_intc_data *priv; > > + const struct of_device_id *match; > > + u32 isr_tmp, ier_tmp, ier_bit; > > + int ret, i; > > + > > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > + if (!priv) { > > + ret = -ENOMEM; > > + goto err; > > + } > > + > > + raw_spin_lock_init(&priv->lock); > > + > > + priv->isr = of_iomap(node, 0); > > + if (!priv->isr) { > > + pr_err("unable to obtain status reg iomap address\n"); > > + ret = -ENOMEM; > > + goto free_priv; > > + } > > + > > + priv->ier = of_iomap(node, 1); > > + if (!priv->ier) { > > + pr_err("unable to obtain enable reg iomap address\n"); > > + ret = -ENOMEM; > > + goto iounmap_status; > > + } > > + > > + priv->unmask = of_iomap(node, 2); > > + if (!priv->unmask) { > > + pr_err("unable to obtain unmask reg iomap address\n"); > > + ret = -ENOMEM; > > + goto iounmap_enable; > > + } > > + > > + priv->parent_irq = irq_of_parse_and_map(node, 0); > > + if (!priv->parent_irq) { > > + pr_err("failed to map parent interrupt %d\n", priv->parent_irq); > > + ret = -EINVAL; > > + goto iounmap_all; > > + } > > + > > + match = of_match_node(rtd129x_intc_matches, node); > > + if (!match) { > > + pr_err("failed to find matching node\n"); > > + ret = -ENODEV; > > + goto iounmap_all; > > + } > > + priv->en_map = match->data; > > + > > + // initialize enabled irq's map to its matching status bit in isr by > > + // inverse walking the enable to status offsets map. Only needed for > > + // misc > > Why do we need any of this? The kernel is supposed to start from a clean > slate, not to inherit whatever has been set before, unless there is a > very compelling reason. > > > + priv->ier_cached = readl_relaxed(priv->ier); > > + if (priv->en_map == rtd129x_intc_enable_map_misc) { > > + ier_tmp = priv->ier_cached; > > + isr_tmp = 0; > > + while (ier_tmp) { > > + ier_bit = __ffs(ier_tmp); > > + ier_tmp &= ~BIT(ier_bit); > > + for (i = 0; i < RTD129X_INTC_NR_IRQS; i++) > > + if (priv->en_map[i] == ier_bit) > > + isr_tmp |= BIT(i); > > + } > > + priv->isr_en = isr_tmp; > > + } else { > > + priv->isr_en = priv->ier_cached; > > + } > > + > > + rtd129x_intc_domain = irq_domain_add_linear(node, RTD129X_INTC_NR_IRQS, > > + &rtd129x_intc_domain_ops, > > + priv); > > + if (!rtd129x_intc_domain) { > > + pr_err("failed to create irq domain\n"); > > + ret = -ENOMEM; > > + goto iounmap_all; > > + } > > + > > + irq_set_chained_handler_and_data(priv->parent_irq, > > + rtd129x_intc_irq_handle, priv); > > + > > + return 0; > > + > > +iounmap_all: > > + iounmap(priv->unmask); > > +iounmap_enable: > > + iounmap(priv->ier); > > +iounmap_status: > > + iounmap(priv->isr); > > +free_priv: > > + kfree(priv); > > +err: > > + return ret; > > +} > > + > > +IRQCHIP_DECLARE(rtd129x_intc_misc, "realtek,rtd129x-intc-misc", > > + rtd129x_intc_of_init); > > +IRQCHIP_DECLARE(rtd129x_intc_iso, "realtek,rtd129x-intc-iso", > > + rtd129x_intc_of_init); > > > > Thanks, Thank you very much for your time! > > M. > -- > Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver 2019-08-12 8:26 ` Aleix Roca Nonell @ 2019-08-13 15:15 ` Marc Zyngier 0 siblings, 0 replies; 6+ messages in thread From: Marc Zyngier @ 2019-08-13 15:15 UTC (permalink / raw) To: Aleix Roca Nonell Cc: Mark Rutland, devicetree, Jason Cooper, linux-kernel, Rob Herring, Matthias Brugger, Thomas Gleixner, Andreas Färber, linux-arm-kernel On Mon, 12 Aug 2019 09:26:48 +0100, Aleix Roca Nonell <kernelrocks@gmail.com> wrote: > > Hi Mark and everyone! Sorry for the large delay, I'm doing this in my > free time, which is not that abundant. In this mail, I'm focusing only > on the largest change mentioned by Mark. I will answer the rest later. > > On Mon, Jul 08, 2019 at 10:36:14AM +0100, Marc Zyngier wrote: > > On 07/07/2019 14:22, Aleix Roca Nonell wrote: > > > This driver adds support for the RTD1296 and RTD1295 interrupt > > > controller (intc). It is based on both the BPI-SINOVOIP project and > > > Andreas Färber's previous attempt to submit a similar driver. > > > > > > There is currently no publicly available datasheet on this SoC and the > > > exact behaviour of the registers controlling the intc remain uncertain. > > > > > > This driver controls two intcs: "iso" and "misc". Each intc has its own > > > Interrupt Enable Register (IER) and Interrupt Status Resgister (ISR). > > > > Register > > > > > However, not all "misc" intc irqs have the same offsets for both ISR and > > > IER. For this reason an ISR to IER offsets table is defined. > > > > > > The driver catches the IER value to reduce accesses to the table inside the > > > interrupt handler. Actually, the driver stores the ISR offsets of currently > > > enabled interrupts in a variable. > > > > > > Signed-off-by: Aleix Roca Nonell <kernelrocks@gmail.com> > > > > I expect Andreas and you to sort the attribution issue. I'm certainly > > not going to take this in if things are unclear. > > > > > --- > > > drivers/irqchip/Makefile | 1 + > > > drivers/irqchip/irq-rtd129x.c | 371 ++++++++++++++++++++++++++++++++++ > > > 2 files changed, 372 insertions(+) > > > create mode 100644 drivers/irqchip/irq-rtd129x.c > > > > > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > > > index 606a003a0000..0689c3956250 100644 > > > --- a/drivers/irqchip/Makefile > > > +++ b/drivers/irqchip/Makefile > > > @@ -100,3 +100,4 @@ obj-$(CONFIG_MADERA_IRQ) += irq-madera.o > > > obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o > > > obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o > > > obj-$(CONFIG_TI_SCI_INTA_IRQCHIP) += irq-ti-sci-inta.o > > > +obj-$(CONFIG_ARCH_REALTEK) += irq-rtd129x.o > > > diff --git a/drivers/irqchip/irq-rtd129x.c b/drivers/irqchip/irq-rtd129x.c > > > new file mode 100644 > > > index 000000000000..76358ca50f10 > > > --- /dev/null > > > +++ b/drivers/irqchip/irq-rtd129x.c > > > @@ -0,0 +1,371 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > + > > > +#include <linux/irqchip.h> > > > +#include <linux/of.h> > > > +#include <linux/of_address.h> > > > +#include <linux/of_irq.h> > > > +#include <linux/irqdomain.h> > > > +#include <linux/io.h> > > > +#include <linux/spinlock.h> > > > +#include <linux/irqchip.h> > > > +#include <linux/bits.h> > > > +#include <linux/irqchip/chained_irq.h> > > > + > > > +#define RTD129X_INTC_NR_IRQS 32 > > > +#define DEV_NAME "RTD1296_INTC" > > > + > > > +/* > > > + * This interrupt controller (hereinafter intc) driver controls two intcs: "iso" > > > + * and "misc". Each intc has its own Interrupt Enable Register (IER) and > > > + * Interrupt Status Resgister (ISR). However, not all "misc" intc irqs have the > > > + * same offsets for both ISR and IER. For this reason an ISR to IER offsets > > > + * table is defined. Also, to reduce accesses to this table in the interrupt > > > + * handler, the driver stores the ISR offsets of currently enabled interrupts in > > > + * a variable. > > > + */ > > > + > > > +enum misc_int_en { > > > + MISC_INT_FAIL = 0xFF, > > > + MISC_INT_RVD = 0xFE, > > > + MISC_INT_EN_FAN = 29, > > > + MISC_INT_EN_I2C3 = 28, > > > + MISC_INT_EN_GSPI = 27, > > > + MISC_INT_EN_I2C2 = 26, > > > + MISC_INT_EN_SC0 = 24, > > > + MISC_INT_EN_LSADC1 = 22, > > > + MISC_INT_EN_LSADC0 = 21, > > > + MISC_INT_EN_GPIODA = 20, > > > + MISC_INT_EN_GPIOA = 19, > > > + MISC_INT_EN_I2C4 = 15, > > > + MISC_INT_EN_I2C5 = 14, > > > + MISC_INT_EN_RTC_DATA = 12, > > > + MISC_INT_EN_RTC_HOUR = 11, > > > + MISC_INT_EN_RTC_MIN = 10, > > > + MISC_INT_EN_UR2 = 7, > > > + MISC_INT_EN_UR2_TO = 6, > > > + MISC_INT_EN_UR1_TO = 5, > > > + MISC_INT_EN_UR1 = 3, > > > +}; > > > + > > > +enum iso_int_en { > > > + ISO_INT_FAIL = 0xFF, > > > + ISO_INT_RVD = 0xFE, > > > + ISO_INT_EN_I2C1_REQ = 31, > > > + ISO_INT_EN_GPHY_AV = 30, > > > + ISO_INT_EN_GPHY_DV = 29, > > > + ISO_INT_EN_GPIODA = 20, > > > + ISO_INT_EN_GPIOA = 19, > > > + ISO_INT_EN_RTC_ALARM = 13, > > > + ISO_INT_EN_RTC_HSEC = 12, > > > + ISO_INT_EN_I2C1 = 11, > > > + ISO_INT_EN_I2C0 = 8, > > > + ISO_INT_EN_IRDA = 5, > > > + ISO_INT_EN_UR0 = 2, > > > +}; > > > + > > > +unsigned char rtd129x_intc_enable_map_misc[RTD129X_INTC_NR_IRQS] = { > > > + MISC_INT_FAIL, /* Bit0 */ > > > + MISC_INT_FAIL, /* Bit1 */ > > > + MISC_INT_RVD, /* Bit2 */ > > > + MISC_INT_EN_UR1, /* Bit3 */ > > > + MISC_INT_FAIL, /* Bit4 */ > > > + MISC_INT_EN_UR1_TO, /* Bit5 */ > > > + MISC_INT_RVD, /* Bit6 */ > > > + MISC_INT_RVD, /* Bit7 */ > > > + MISC_INT_EN_UR2, /* Bit8 */ > > > + MISC_INT_RVD, /* Bit9 */ > > > + MISC_INT_EN_RTC_MIN, /* Bit10 */ > > > + MISC_INT_EN_RTC_HOUR, /* Bit11 */ > > > + MISC_INT_EN_RTC_DATA, /* Bit12 */ > > > + MISC_INT_EN_UR2_TO, /* Bit13 */ > > > + MISC_INT_EN_I2C5, /* Bit14 */ > > > + MISC_INT_EN_I2C4, /* Bit15 */ > > > + MISC_INT_FAIL, /* Bit16 */ > > > + MISC_INT_FAIL, /* Bit17 */ > > > + MISC_INT_FAIL, /* Bit18 */ > > > + MISC_INT_EN_GPIOA, /* Bit19 */ > > > + MISC_INT_EN_GPIODA, /* Bit20 */ > > > + MISC_INT_EN_LSADC0, /* Bit21 */ > > > + MISC_INT_EN_LSADC1, /* Bit22 */ > > > + MISC_INT_EN_I2C3, /* Bit23 */ > > > + MISC_INT_EN_SC0, /* Bit24 */ > > > + MISC_INT_FAIL, /* Bit25 */ > > > + MISC_INT_EN_I2C2, /* Bit26 */ > > > + MISC_INT_EN_GSPI, /* Bit27 */ > > > + MISC_INT_FAIL, /* Bit28 */ > > > + MISC_INT_EN_FAN, /* Bit29 */ > > > + MISC_INT_FAIL, /* Bit30 */ > > > + MISC_INT_FAIL /* Bit31 */ > > > +}; > > > + > > > +unsigned char rtd129x_intc_enable_map_iso[RTD129X_INTC_NR_IRQS] = { > > > + ISO_INT_FAIL, /* Bit0 */ > > > + ISO_INT_RVD, /* Bit1 */ > > > + ISO_INT_EN_UR0, /* Bit2 */ > > > + ISO_INT_FAIL, /* Bit3 */ > > > + ISO_INT_FAIL, /* Bit4 */ > > > + ISO_INT_EN_IRDA, /* Bit5 */ > > > + ISO_INT_FAIL, /* Bit6 */ > > > + ISO_INT_RVD, /* Bit7 */ > > > + ISO_INT_EN_I2C0, /* Bit8 */ > > > + ISO_INT_RVD, /* Bit9 */ > > > + ISO_INT_FAIL, /* Bit10 */ > > > + ISO_INT_EN_I2C1, /* Bit11 */ > > > + ISO_INT_EN_RTC_HSEC, /* Bit12 */ > > > + ISO_INT_EN_RTC_ALARM, /* Bit13 */ > > > + ISO_INT_FAIL, /* Bit14 */ > > > + ISO_INT_FAIL, /* Bit15 */ > > > + ISO_INT_FAIL, /* Bit16 */ > > > + ISO_INT_FAIL, /* Bit17 */ > > > + ISO_INT_FAIL, /* Bit18 */ > > > + ISO_INT_EN_GPIOA, /* Bit19 */ > > > + ISO_INT_EN_GPIODA, /* Bit20 */ > > > + ISO_INT_RVD, /* Bit21 */ > > > + ISO_INT_RVD, /* Bit22 */ > > > + ISO_INT_RVD, /* Bit23 */ > > > + ISO_INT_RVD, /* Bit24 */ > > > + ISO_INT_FAIL, /* Bit25 */ > > > + ISO_INT_FAIL, /* Bit26 */ > > > + ISO_INT_FAIL, /* Bit27 */ > > > + ISO_INT_FAIL, /* Bit28 */ > > > + ISO_INT_EN_GPHY_DV, /* Bit29 */ > > > + ISO_INT_EN_GPHY_AV, /* Bit30 */ > > > + ISO_INT_EN_I2C1_REQ /* Bit31 */ > > > +}; > > > + > > > +struct rtd129x_intc_data { > > > + void __iomem *unmask; > > > + void __iomem *isr; > > > + void __iomem *ier; > > > + u32 ier_cached; > > > + u32 isr_en; > > > + raw_spinlock_t lock; > > > + unsigned int parent_irq; > > > + const unsigned char *en_map; > > > +}; > > > + > > > +static struct irq_domain *rtd129x_intc_domain; > > > + > > > +static void rtd129x_intc_irq_handle(struct irq_desc *desc) > > > +{ > > > + struct rtd129x_intc_data *priv = irq_desc_get_handler_data(desc); > > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > > + unsigned int local_irq; > > > + u32 status; > > > + int i; > > > + > > > + chained_irq_enter(chip, desc); > > > + > > > + raw_spin_lock(&priv->lock); > > > + status = readl_relaxed(priv->isr); > > > + status &= priv->isr_en; > > > + raw_spin_unlock(&priv->lock); > > > > What is this lock protecting? isr_en? > > > > > + > > > + while (status) { > > > + i = __ffs(status); > > > + status &= ~BIT(i); > > > + > > > + local_irq = irq_find_mapping(rtd129x_intc_domain, i); > > > + if (likely(local_irq)) { > > > + if (!generic_handle_irq(local_irq)) > > > + writel_relaxed(BIT(i), priv->isr); > > > > What are the write semantics of the ISR register? Hot bit clear? How > > does it work since mask() does the same thing? Clearly, something is > > wrong here. > > Sorry but I have not been able to found the definition of "hot bit > clear", could you explain it? Anyways, you were right, apparently the > mask/unmask code were doing nothing useful. More on this below. A hot-bit clear (or set) is a register where to write the bits that you want to clear (or set), leaving alone the bits that are written as zero. For example: REG = 0xFFFF clear_reg(0x1001) REG = 0x7FFE set_reg(0x1000) REG = 0xFFFE It is extremely useful for registers that need to be accessed concurrently (the GIC uses that a lot, for example). > > > > > > + } else { > > > + handle_bad_irq(desc); > > > + } > > > + } > > > + > > > + chained_irq_exit(chip, desc); > > > +} > > > + > > > +static void rtd129x_intc_mask(struct irq_data *data) > > > +{ > > > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > > > + > > > + writel_relaxed(BIT(data->hwirq), priv->isr); > > > +} > > > + > > > +static void rtd129x_intc_unmask(struct irq_data *data) > > > +{ > > > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > > > + > > > + writel_relaxed(BIT(data->hwirq), priv->unmask); > > > > What effect does this have on the isr register? The whole mask/unmask > > thing seems to be pretty dodgy... > > > > > +} > > > + > > > +static void rtd129x_intc_enable(struct irq_data *data) > > > +{ > > > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > > > + unsigned long flags; > > > + u8 en_offset; > > > + > > > + en_offset = priv->en_map[data->hwirq]; > > > + > > > + if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) { > > > + raw_spin_lock_irqsave(&priv->lock, flags); > > > + > > > + priv->isr_en |= BIT(data->hwirq); > > > + priv->ier_cached |= BIT(en_offset); > > > + writel_relaxed(priv->ier_cached, priv->ier); > > > + > > > + raw_spin_unlock_irqrestore(&priv->lock, flags); > > > + } else if (en_offset == MISC_INT_FAIL) { > > > + pr_err("[%s] Enable irq(%lu) failed\n", DEV_NAME, data->hwirq); > > > + } > > > +} > > > + > > > +static void rtd129x_intc_disable(struct irq_data *data) > > > +{ > > > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data); > > > + unsigned long flags; > > > + u8 en_offset; > > > + > > > + en_offset = priv->en_map[data->hwirq]; > > > + > > > + if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) { > > > + raw_spin_lock_irqsave(&priv->lock, flags); > > > + > > > + priv->isr_en &= ~BIT(data->hwirq); > > > + priv->ier_cached &= ~BIT(en_offset); > > > + writel_relaxed(priv->ier_cached, priv->ier); > > > + > > > + raw_spin_unlock_irqrestore(&priv->lock, flags); > > > + } else if (en_offset == MISC_INT_FAIL) { > > > + pr_err("[%s] Disable irq(%lu) failed\n", DEV_NAME, data->hwirq); > > > + } > > > +} > > > > So here's a thought: Why do we need all of this? If mask/unmask do their > > job correctly, we could just enable all interrupts in one go (just a > > 32bit write) at probe time, and leave all interrupts masked until they > > are in use. You could then drop all these silly tables that don't bring > > much... > > The idea of dropping all those tables look really good to me, that > would greatly simplify the code! I have been trying to mask all > interrupts on the probe function using the ISR register but while > doing so, I realized that it does not work. Writing to ISR does not > mask interrupts, apparently it only acknowledges them once they have > been triggered. On the scarse available documentation of this Soc I > cannot find a mask-like register. It seems interrupts are managed with > an ISR and an IER register. So it should be posible to use the enable > register to maks/unmask instead. These do work. However, that would > mean that we have to keep those ugly tables. > > Nonetheless we might still be able to do something else. Please, > correct me if I'm wrong, but do we really need to mask/unamsk in this > scenario? This is the devised board layout: > > +------+ +----------+ +---------+ > | | | | | | > | UART |-------|2 INTC |-------|c GIC | > | | +----|1 | +----|b | > +------+ | +--|0 | | +--|a | > | | | | | | | | > | | +----------+ | | +---------+ > | | > > Once the UART generates an interrupt it passes through the line 2 of > the custom realtek interrupt contoller before reaching the GIC's line > "c". On the INTC interrupt handler, we call chained_irq_enter/exit > to mask/unmask the GIC's "c" line. Because all of this realtek INTC > interrupt lines (2,1,0,...) are muxed on the GIC's line "c", this > means that while on the INTC interrupt handler it is not possible to > send further interrupts on the CPU. Given that interrupts are masked > on the GIC, it seems safe to just remove INTC's mask/unmask functions. No, that's not true. If you cannot mask an individual interrupt at the INTC level, it means that the only way to stop a screaming interrupt (because the endpoint has crashed or that the kernel doesn't have a driver for it) is to disable the interrupt at the GIC level, killing all users of the INTC. Also, because the core code doesn't really know that the INTC is behind the GIC, it cannot do that automatically. So if you get into that situation, your system is dead. Believe it or not, that's not something I want to see. An irqchip driver without a mask callback is a lose grenade, and the pin is in your pocket. > Therefore, the only work that this INTC handler would needs to do is > to acknowledge the interrupt by writing to the ISR, which it could be > done in the respective irq_ack callback of struct irq_chip instead of > in the handler body. > > I have implemented this solution and it seems to work. What do you > think? I'm missing something crucial? See above. Your system is terribly unsafe. Now, I'm pretty sure the Realtek folks could help you there. Or you could start trying to reverse engineer the thing, which shouldn't really hard (try poking at the registers next to the ones you already have). Thanks, M. -- Jazz is not dead, it just smells funny. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-13 15:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-07-07 13:22 [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver Aleix Roca Nonell 2019-07-07 13:27 ` Andreas Färber 2019-07-07 15:46 ` Aleix Roca Nonell 2019-07-08 9:36 ` Marc Zyngier 2019-08-12 8:26 ` Aleix Roca Nonell 2019-08-13 15:15 ` Marc Zyngier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).