Messages in this thread | | | Subject | Re: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver | Date | Tue, 19 Nov 2019 12:01:33 +0000 | From | Marc Zyngier <> |
| |
On 2019-11-19 02:19, Andreas Färber wrote: > This irq mux driver implements the RTD1295 SoC's non-linear mapping > between status and enable bits. > > Based in part on QNAP's arch/arm/mach-rtk119x/rtk_irq_mux.c and > Synology's drivers/irqchip/irq-rtk.c code. > > Signed-off-by: Andreas Färber <afaerber@suse.de> > Cc: Aleix Roca Nonell <kernelrocks@gmail.com> > Signed-off-by: James Tai <james.tai@realtek.com> > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > v3 -> v4: > * Drop no-op .irq_set_affinity callback (Thomas) > * Clear all interrupts (James) > * Updated SPDX-License-identifier > * Use tabular formatting (Thomas) > * Adopt different braces style (Thomas) > * Use raw_spinlock_t (Thomas) > * Shortened callback from isr_to_scpu_int_en_mask to > isr_to_int_en_mask (Thomas) > * Fixed of_iomap() error handling to not use IS_ERR() > * Don't mask unmapped NMIs by checking for a non-zero mask > * Cache SCPU_INT_EN to avoid superfluous reads (Thomas) > * Renamed functions and variables from rtd119x to rtd1195 > > v2 -> v3: > * Adopted spin_lock_irq{save,restore}() (Marc) > * Adopted single-write masking (Marc) > * Adopted misc compatible string > * Introduced explicit bit mapping > * Adopted looped processing of pending interrupts (Marc) > * Replaced unmask implementation with UMSK_ISR write > * Introduced enable/disable ops and dropped no longer needed UART0 > quirk > > v1 -> v2: > * Renamed struct fields to avoid ambiguity (Marc) > * Refactored offset lookup to avoid per-compatible init functions > * Inserted white lines to clarify balanced locking (Marc) > * Dropped forwarding of set_affinity to GIC (Marc) > * Added spinlocks for consistency (Marc) > * Limited initialization quirk to iso mux > * Fixed spinlock initialization (Andrew) > > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-rtd1195-mux.c | 283 > ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 284 insertions(+) > create mode 100644 drivers/irqchip/irq-rtd1195-mux.c > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index e806dda690ea..d678881eebc8 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -104,3 +104,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-rtd1195-mux.o > diff --git a/drivers/irqchip/irq-rtd1195-mux.c > b/drivers/irqchip/irq-rtd1195-mux.c > new file mode 100644 > index 000000000000..e6b08438b23c > --- /dev/null > +++ b/drivers/irqchip/irq-rtd1195-mux.c > @@ -0,0 +1,283 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Realtek RTD1295 IRQ mux > + * > + * Copyright (c) 2017-2019 Andreas Färber > + */ > + > +#include <linux/io.h> > +#include <linux/irqchip.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/irqdomain.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/slab.h> > + > +struct rtd1195_irq_mux_info { > + unsigned int isr_offset; > + unsigned int umsk_isr_offset; > + unsigned int scpu_int_en_offset; > + const u32 *isr_to_int_en_mask; > +}; > + > +struct rtd1195_irq_mux_data { > + void __iomem *reg_isr; > + void __iomem *reg_umsk_isr; > + void __iomem *reg_scpu_int_en; > + const struct rtd1195_irq_mux_info *info; > + int irq; > + u32 scpu_int_en; > + struct irq_domain *domain; > + raw_spinlock_t lock; > +}; > + > +static void rtd1195_mux_irq_handle(struct irq_desc *desc) > +{ > + struct rtd1195_irq_mux_data *data = > irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + u32 isr, mask; > + int i; > + > + chained_irq_enter(chip, desc); > + > + isr = readl_relaxed(data->reg_isr); > + > + while (isr) { > + i = __ffs(isr); > + isr &= ~BIT(i); > + > + mask = data->info->isr_to_int_en_mask[i]; > + if (mask && !(data->scpu_int_en & mask)) > + continue; > + > + if (!generic_handle_irq(irq_find_mapping(data->domain, i))) > + writel_relaxed(BIT(i), data->reg_isr);
What does this write do exactly? It is the same thing as a 'mask', which is pretty odd. So either:
- this is not doing anything and your 'mask' callback is bogus (otherwise you'd never have more than a single interrupt)
- or this is an ACK operation, and this should be described as such (and then fix the mask/unmask/enable/disable mess that results from it).
as I can't see how the same register can be used for both purposes. You should be able to verify this experimentally, even without documentation.
> + } > + > + chained_irq_exit(chip, desc); > +} > + > +static void rtd1195_mux_mask_irq(struct irq_data *data) > +{ > + struct rtd1195_irq_mux_data *mux_data = > irq_data_get_irq_chip_data(data); > + > + writel_relaxed(BIT(data->hwirq), mux_data->reg_isr); > +} > + > +static void rtd1195_mux_unmask_irq(struct irq_data *data) > +{ > + struct rtd1195_irq_mux_data *mux_data = > irq_data_get_irq_chip_data(data); > + > + writel_relaxed(BIT(data->hwirq), mux_data->reg_umsk_isr); > +} > + > +static void rtd1195_mux_enable_irq(struct irq_data *data) > +{ > + struct rtd1195_irq_mux_data *mux_data = > irq_data_get_irq_chip_data(data); > + unsigned long flags; > + u32 mask; > + > + mask = mux_data->info->isr_to_int_en_mask[data->hwirq]; > + if (!mask) > + return;
How can this happen? You've mapped the interrupt, so it exists. I can't see how you can decide to fail such enable.
> + > + raw_spin_lock_irqsave(&mux_data->lock, flags); > + > + mux_data->scpu_int_en |= mask; > + writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en); > + > + raw_spin_unlock_irqrestore(&mux_data->lock, flags); > +} > + > +static void rtd1195_mux_disable_irq(struct irq_data *data) > +{ > + struct rtd1195_irq_mux_data *mux_data = > irq_data_get_irq_chip_data(data); > + unsigned long flags; > + u32 mask; > + > + mask = mux_data->info->isr_to_int_en_mask[data->hwirq]; > + if (!mask) > + return; > + > + raw_spin_lock_irqsave(&mux_data->lock, flags); > + > + mux_data->scpu_int_en &= ~mask; > + writel_relaxed(mux_data->scpu_int_en, mux_data->reg_scpu_int_en); > + > + raw_spin_unlock_irqrestore(&mux_data->lock, flags); > +} > + > +static struct irq_chip rtd1195_mux_irq_chip = { > + .name = "rtd1195-mux", > + .irq_mask = rtd1195_mux_mask_irq, > + .irq_unmask = rtd1195_mux_unmask_irq, > + .irq_enable = rtd1195_mux_enable_irq, > + .irq_disable = rtd1195_mux_disable_irq, > +};
[...]
Although the code is pretty clean, the way you drive the HW looks suspicious, and requires clarification.
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |