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 <[email protected]> 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...
