| Message ID | 20191119021917.15917-3-afaerber@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: Realtek RTD1195/RTD1295/RTD1395 IRQ mux | expand |
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.
Am 19.11.19 um 13:01 schrieb Marc Zyngier: > On 2019-11-19 02:19, Andreas Färber wrote: >> +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. The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the same. My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) lookup, but are sparsely populated. So there are circumstances such as WDOG_NMI as well as reserved bits that we cannot enable. This check should be identical to v3; the equivalent mask check inside the interrupt handler was extended with "mask &&" to do the same in this v4. The other question I'll need to dig into, it's been two years since I wrote that code - first very simple guesswork, then more elaborate quirks like the above. Regards, Andreas
On Tue, 19 Nov 2019 21:56:48 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 19.11.19 um 13:01 schrieb Marc Zyngier: > > On 2019-11-19 02:19, Andreas Färber wrote: > >> +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. > > The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the same. > > My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) lookup, but > are sparsely populated. So there are circumstances such as WDOG_NMI as > well as reserved bits that we cannot enable. But the you should have failed the map. The moment you allow the mapping to occur, you have accepted the contract that this interrupt is usable. > This check should be > identical to v3; the equivalent mask check inside the interrupt handler > was extended with "mask &&" to do the same in this v4. Spurious interrupts are a different matter. What I'm objecting to here is a simple question of logic, whether or not you are allowed to fail enabling an interrupt that you've otherwise allowed to be populated. M.
Am 19.11.19 um 13:01 schrieb Marc Zyngier: > On 2019-11-19 02:19, Andreas Färber wrote: >> 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 [...] >> +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). This is supposed to be an ACK, i.e. clear-1-bits operation. The BSP had extended various drivers, such as 8250 UART, to do this ack in their interrupt handler through an additional DT reg region. I tried to clean that up by handling it centrally here in the irqchip driver. > > 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. There are three registers here: MIS_UMSK_ISR - MISC unmasked interrupt status register MIS_ISR - MISC masked interrupt status register MIS_SCPU_INT_EN - MISC SCPU interrupt enable register The latter is a regular R/W register; the former two have a write_data field as BIT(0), with 1 indicating a write vs. 0 indicating clear, RAZ. By enabling/disabling in _SCPU_INT_EN we mask/unmask them in _ISR but not in _UMSK_ISR. Does that shed any more light? So given that we're iterating over reg_isr above, we could probably drop the mask check here... In addition there are MIS_[UMSK_]ISR_SWC and MIS_SETTING_SWC registers for Secure World, and MIS_FAST_INT_EN_* and MIS_FAST_ISR as well as various GPIO interrupt registers. Regards, Andreas >> + } >> + >> + 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.
Am 19.11.19 um 23:29 schrieb Marc Zyngier: > On Tue, 19 Nov 2019 21:56:48 +0100 > Andreas Färber <afaerber@suse.de> wrote: >> Am 19.11.19 um 13:01 schrieb Marc Zyngier: >>> On 2019-11-19 02:19, Andreas Färber wrote: >>>> +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. >> >> The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the same. >> >> My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) lookup, but >> are sparsely populated. So there are circumstances such as WDOG_NMI as >> well as reserved bits that we cannot enable. > > But the you should have failed the map. The moment you allow the > mapping to occur, you have accepted the contract that this interrupt is > usable. > >> This check should be >> identical to v3; the equivalent mask check inside the interrupt handler >> was extended with "mask &&" to do the same in this v4. > > Spurious interrupts are a different matter. What I'm objecting to here > is a simple question of logic, whether or not you are allowed to fail > enabling an interrupt that you've otherwise allowed to be populated. Then what are you suggesting instead? I don't see how my array map lookup could fail other than returning a zero value, given its static initialization. Check for a zero mask in rtd1195_mux_irq_domain_map()? Then we wouldn't be able to use the mentioned WDOG_NMI. Add another per-mux info field for which interrupts are valid to map? Thanks, Andreas
On 2019-11-19 23:25, Andreas Färber wrote: > Am 19.11.19 um 13:01 schrieb Marc Zyngier: >> On 2019-11-19 02:19, Andreas Färber wrote: >>> 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 > [...] >>> +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). > > This is supposed to be an ACK, i.e. clear-1-bits operation. If it is an ACK, model it as such, and do not open-code it. > > The BSP had extended various drivers, such as 8250 UART, to do this > ack > in their interrupt handler through an additional DT reg region. I > tried > to clean that up by handling it centrally here in the irqchip driver. > >> >> 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. > > There are three registers here: > > MIS_UMSK_ISR - MISC unmasked interrupt status register > MIS_ISR - MISC masked interrupt status register > MIS_SCPU_INT_EN - MISC SCPU interrupt enable register > > The latter is a regular R/W register; the former two have a > write_data > field as BIT(0), with 1 indicating a write vs. 0 indicating clear, > RAZ. > > By enabling/disabling in _SCPU_INT_EN we mask/unmask them in _ISR but > not in _UMSK_ISR. > > Does that shed any more light? None whatsoever. Your mask callback doesn't make any sense, since it actually acks the interrupt. My gut feeling is that your enable/disable should really be mask/unmask. > > So given that we're iterating over reg_isr above, we could probably > drop > the mask check here... > > In addition there are MIS_[UMSK_]ISR_SWC and MIS_SETTING_SWC > registers > for Secure World, and MIS_FAST_INT_EN_* and MIS_FAST_ISR as well as > various GPIO interrupt registers. This doesn't seem relevant to the discussion here. M.
On 2019-11-19 23:33, Andreas Färber wrote: > Am 19.11.19 um 23:29 schrieb Marc Zyngier: >> On Tue, 19 Nov 2019 21:56:48 +0100 >> Andreas Färber <afaerber@suse.de> wrote: >>> Am 19.11.19 um 13:01 schrieb Marc Zyngier: >>>> On 2019-11-19 02:19, Andreas Färber wrote: >>>>> +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. >>> >>> The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the >>> same. >>> >>> My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) lookup, >>> but >>> are sparsely populated. So there are circumstances such as WDOG_NMI >>> as >>> well as reserved bits that we cannot enable. >> >> But the you should have failed the map. The moment you allow the >> mapping to occur, you have accepted the contract that this interrupt >> is >> usable. >> >>> This check should be >>> identical to v3; the equivalent mask check inside the interrupt >>> handler >>> was extended with "mask &&" to do the same in this v4. >> >> Spurious interrupts are a different matter. What I'm objecting to >> here >> is a simple question of logic, whether or not you are allowed to >> fail >> enabling an interrupt that you've otherwise allowed to be populated. > > Then what are you suggesting instead? I don't see how my array map > lookup could fail other than returning a zero value, given its static > initialization. Check for a zero mask in > rtd1195_mux_irq_domain_map()? > Then we wouldn't be able to use the mentioned WDOG_NMI. Add another > per-mux info field for which interrupts are valid to map? I'm suggesting that you fail the map if you're unable to allow the interrupt to be enabled. M.
Am 20.11.19 um 11:18 schrieb Marc Zyngier: > On 2019-11-19 23:25, Andreas Färber wrote: >> Am 19.11.19 um 13:01 schrieb Marc Zyngier: >>> On 2019-11-19 02:19, Andreas Färber wrote: >>>> 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 >> [...] >>>> +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). >> >> This is supposed to be an ACK, i.e. clear-1-bits operation. > > If it is an ACK, model it as such, and do not open-code it. I have found an .irq_ack callback - moving this there appears to work. Alternatively there is an irq_eoi callback and an IRQCHIP_EOI_IF_HANDLED flag. It would really help me if you spelled out explicitly where you think I should be moving code, as the documentation in irq.h is not all that helpful in terms of when are they called and what should be done there. In case not obvious, this is my first irqchip driver. >> >> The BSP had extended various drivers, such as 8250 UART, to do this ack >> in their interrupt handler through an additional DT reg region. I tried >> to clean that up by handling it centrally here in the irqchip driver. >> >>> >>> 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. >> >> There are three registers here: >> >> MIS_UMSK_ISR - MISC unmasked interrupt status register >> MIS_ISR - MISC masked interrupt status register >> MIS_SCPU_INT_EN - MISC SCPU interrupt enable register >> >> The latter is a regular R/W register; the former two have a write_data >> field as BIT(0), with 1 indicating a write vs. 0 indicating clear, RAZ. >> >> By enabling/disabling in _SCPU_INT_EN we mask/unmask them in _ISR but >> not in _UMSK_ISR. >> >> Does that shed any more light? > > None whatsoever. Your mask callback doesn't make any sense, since it > actually acks the interrupt. My gut feeling is that your enable/disable > should really be mask/unmask. Renaming enable -> unmask and disable -> mask works okay. Thanks, Andreas
On 2019-11-20 12:12, Andreas Färber wrote: > Am 20.11.19 um 11:18 schrieb Marc Zyngier: >> On 2019-11-19 23:25, Andreas Färber wrote: >>> Am 19.11.19 um 13:01 schrieb Marc Zyngier: >>>> On 2019-11-19 02:19, Andreas Färber wrote: >>>>> 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 >>> [...] >>>>> +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). >>> >>> This is supposed to be an ACK, i.e. clear-1-bits operation. >> >> If it is an ACK, model it as such, and do not open-code it. > > I have found an .irq_ack callback - moving this there appears to > work. > > Alternatively there is an irq_eoi callback and an > IRQCHIP_EOI_IF_HANDLED > flag. > > It would really help me if you spelled out explicitly where you think > I > should be moving code, as the documentation in irq.h is not all that > helpful in terms of when are they called and what should be done > there. > In case not obvious, this is my first irqchip driver. Implementing one callback or the other really depends on how the HW behaves. The irq framework gives you a wide range of flows that allow you to plug your driver in the stack, but the prerequisite is that you know *exactly* how the HW behaves. Ack and EOI have very different meanings, are called from different flows, and correspond to different states in the interrupt life cycle. Use the wrong one, and you will lose interrupts. If you don't know how the HW behaves, then the chances of something bad happening are pretty high (you'll end-up in deadlock land at some point). I'm afraid I cannot help you with that, short of being given access to some documentation that doesn't seem to exist. M.
Am 20.11.19 um 11:20 schrieb Marc Zyngier: > On 2019-11-19 23:33, Andreas Färber wrote: >> Am 19.11.19 um 23:29 schrieb Marc Zyngier: >>> On Tue, 19 Nov 2019 21:56:48 +0100 >>> Andreas Färber <afaerber@suse.de> wrote: >>>> Am 19.11.19 um 13:01 schrieb Marc Zyngier: >>>>> On 2019-11-19 02:19, Andreas Färber wrote: >>>>>> +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. >>>> >>>> The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the same. >>>> >>>> My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) lookup, but >>>> are sparsely populated. So there are circumstances such as WDOG_NMI as >>>> well as reserved bits that we cannot enable. >>> >>> But the you should have failed the map. The moment you allow the >>> mapping to occur, you have accepted the contract that this interrupt is >>> usable. >>> >>>> This check should be >>>> identical to v3; the equivalent mask check inside the interrupt handler >>>> was extended with "mask &&" to do the same in this v4. >>> >>> Spurious interrupts are a different matter. What I'm objecting to here >>> is a simple question of logic, whether or not you are allowed to fail >>> enabling an interrupt that you've otherwise allowed to be populated. >> >> Then what are you suggesting instead? I don't see how my array map >> lookup could fail other than returning a zero value, given its static >> initialization. Check for a zero mask in rtd1195_mux_irq_domain_map()? >> Then we wouldn't be able to use the mentioned WDOG_NMI. Add another >> per-mux info field for which interrupts are valid to map? > > I'm suggesting that you fail the map if you're unable to allow the > interrupt to be enabled. The NMI will always be enabled, it just can't be disabled. I have added a check to suppress a zero hwirq. Suppressing reserved IRQ bits will take some more effort to distinguish from NMIs. In particular if we flag this in the ..._isr_to_scpu_int_en array by some magic mask value like 0xffffffff then all users need to check for two rather than one value - but if we reduce the users, it shouldn't matter too much. With contract I assume you're referring to these callbacks having a void return type, unable to return an error to the caller, and there being no is_enabled/is_masked callbacks for anyone to discover this. Unfortunately NMI handling appears to be only used in GICv3 and is not very intuitive for me: Apparently I can only flag the whole irq_chip as being NMI but not individual IRQs? Would that mean that this driver would need to instantiate a second irq_chip for that one IRQ? How would that work for mapping from DT? Given that this mux relies on a maskable GICv2 IRQ, it's not a "true" NMI in the Linux sense anyway, other than the .irq_mask callback not being applicable. While I don't need that NMI immediately, I would prefer not to merge a driver that by design can't cope with it later. I'll try to post a v5 with rsv and nmi blocked in map for further discussion tonight. Regards, Andreas
On 2019-11-20 13:34, Andreas Färber wrote: > Am 20.11.19 um 11:20 schrieb Marc Zyngier: >> On 2019-11-19 23:33, Andreas Färber wrote: >>> Am 19.11.19 um 23:29 schrieb Marc Zyngier: >>>> On Tue, 19 Nov 2019 21:56:48 +0100 >>>> Andreas Färber <afaerber@suse.de> wrote: >>>>> Am 19.11.19 um 13:01 schrieb Marc Zyngier: >>>>>> On 2019-11-19 02:19, Andreas Färber wrote: >>>>>>> +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. >>>>> >>>>> The [UMSK_]ISR bits and the SCPU_INT_EN bits are not (all) the >>>>> same. >>>>> >>>>> My ..._isr_to_scpu_int_en[] arrays have 32 entries for O(1) >>>>> lookup, but >>>>> are sparsely populated. So there are circumstances such as >>>>> WDOG_NMI as >>>>> well as reserved bits that we cannot enable. >>>> >>>> But the you should have failed the map. The moment you allow the >>>> mapping to occur, you have accepted the contract that this >>>> interrupt is >>>> usable. >>>> >>>>> This check should be >>>>> identical to v3; the equivalent mask check inside the interrupt >>>>> handler >>>>> was extended with "mask &&" to do the same in this v4. >>>> >>>> Spurious interrupts are a different matter. What I'm objecting to >>>> here >>>> is a simple question of logic, whether or not you are allowed to >>>> fail >>>> enabling an interrupt that you've otherwise allowed to be >>>> populated. >>> >>> Then what are you suggesting instead? I don't see how my array map >>> lookup could fail other than returning a zero value, given its >>> static >>> initialization. Check for a zero mask in >>> rtd1195_mux_irq_domain_map()? >>> Then we wouldn't be able to use the mentioned WDOG_NMI. Add another >>> per-mux info field for which interrupts are valid to map? >> >> I'm suggesting that you fail the map if you're unable to allow the >> interrupt to be enabled. > > The NMI will always be enabled, it just can't be disabled. If I really cared, I'd cry. This HW is useless. > I have added a check to suppress a zero hwirq. Suppressing reserved > IRQ > bits will take some more effort to distinguish from NMIs. In > particular > if we flag this in the ..._isr_to_scpu_int_en array by some magic > mask > value like 0xffffffff then all users need to check for two rather > than > one value - but if we reduce the users, it shouldn't matter too much. 1) you can't suppress a level interrupt that cannot be disabled. It will fire back at you. 2) given that you have to demux things using MMIO accesses, performance is the least of anybody's worry. > > With contract I assume you're referring to these callbacks having a > void > return type, unable to return an error to the caller, and there being > no > is_enabled/is_masked callbacks for anyone to discover this. > > Unfortunately NMI handling appears to be only used in GICv3 and is > not > very intuitive for me: Apparently I can only flag the whole irq_chip > as > being NMI but not individual IRQs? Would that mean that this driver > would need to instantiate a second irq_chip for that one IRQ? How > would > that work for mapping from DT? Given that this mux relies on a > maskable > GICv2 IRQ, it's not a "true" NMI in the Linux sense anyway, other > than > the .irq_mask callback not being applicable. While I don't need that > NMI > immediately, I would prefer not to merge a driver that by design > can't > cope with it later. You are missing the point of the pseudo-NMI infrastructure. To be useful, it *must* be the root interrupt controller. Otherwise, you cannot distinguish it from the other interrupts it is muxed with. Your 'NMI' is absolutely unusable, and whoever designed this HW should be actively prevented from ever designing another interrupt controller again. > I'll try to post a v5 with rsv and nmi blocked in map for further > discussion tonight. I don't plan to review any of this until after the merge window, so please take as long as you want. M.
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); + } + + 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; + + 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, +}; + +static int rtd1195_mux_irq_domain_map(struct irq_domain *d, + unsigned int irq, irq_hw_number_t hw) +{ + struct rtd1195_irq_mux_data *data = d->host_data; + + irq_set_chip_and_handler(irq, &rtd1195_mux_irq_chip, handle_level_irq); + irq_set_chip_data(irq, data); + irq_set_probe(irq); + + return 0; +} + +static const struct irq_domain_ops rtd1195_mux_irq_domain_ops = { + .xlate = irq_domain_xlate_onecell, + .map = rtd1195_mux_irq_domain_map, +}; + +enum rtd1295_iso_isr_bits { + RTD1295_ISO_ISR_UR0_SHIFT = 2, + RTD1295_ISO_ISR_IRDA_SHIFT = 5, + RTD1295_ISO_ISR_I2C0_SHIFT = 8, + RTD1295_ISO_ISR_I2C1_SHIFT = 11, + RTD1295_ISO_ISR_RTC_HSEC_SHIFT = 12, + RTD1295_ISO_ISR_RTC_ALARM_SHIFT = 13, + RTD1295_ISO_ISR_GPIOA_SHIFT = 19, + RTD1295_ISO_ISR_GPIODA_SHIFT = 20, + RTD1295_ISO_ISR_GPHY_DV_SHIFT = 29, + RTD1295_ISO_ISR_GPHY_AV_SHIFT = 30, + RTD1295_ISO_ISR_I2C1_REQ_SHIFT = 31, +}; + +static const u32 rtd1295_iso_isr_to_scpu_int_en_mask[32] = { + [RTD1295_ISO_ISR_UR0_SHIFT] = BIT(2), + [RTD1295_ISO_ISR_IRDA_SHIFT] = BIT(5), + [RTD1295_ISO_ISR_I2C0_SHIFT] = BIT(8), + [RTD1295_ISO_ISR_I2C1_SHIFT] = BIT(11), + [RTD1295_ISO_ISR_RTC_HSEC_SHIFT] = BIT(12), + [RTD1295_ISO_ISR_RTC_ALARM_SHIFT] = BIT(13), + [RTD1295_ISO_ISR_GPIOA_SHIFT] = BIT(19), + [RTD1295_ISO_ISR_GPIODA_SHIFT] = BIT(20), + [RTD1295_ISO_ISR_GPHY_DV_SHIFT] = BIT(29), + [RTD1295_ISO_ISR_GPHY_AV_SHIFT] = BIT(30), + [RTD1295_ISO_ISR_I2C1_REQ_SHIFT] = BIT(31), +}; + +enum rtd1295_misc_isr_bits { + RTD1295_ISR_UR1_SHIFT = 3, + RTD1295_ISR_UR1_TO_SHIFT = 5, + RTD1295_ISR_UR2_SHIFT = 8, + RTD1295_ISR_RTC_MIN_SHIFT = 10, + RTD1295_ISR_RTC_HOUR_SHIFT = 11, + RTD1295_ISR_RTC_DATA_SHIFT = 12, + RTD1295_ISR_UR2_TO_SHIFT = 13, + RTD1295_ISR_I2C5_SHIFT = 14, + RTD1295_ISR_I2C4_SHIFT = 15, + RTD1295_ISR_GPIOA_SHIFT = 19, + RTD1295_ISR_GPIODA_SHIFT = 20, + RTD1295_ISR_LSADC0_SHIFT = 21, + RTD1295_ISR_LSADC1_SHIFT = 22, + RTD1295_ISR_I2C3_SHIFT = 23, + RTD1295_ISR_SC0_SHIFT = 24, + RTD1295_ISR_I2C2_SHIFT = 26, + RTD1295_ISR_GSPI_SHIFT = 27, + RTD1295_ISR_FAN_SHIFT = 29, +}; + +static const u32 rtd1295_misc_isr_to_scpu_int_en_mask[32] = { + [RTD1295_ISR_UR1_SHIFT] = BIT(3), + [RTD1295_ISR_UR1_TO_SHIFT] = BIT(5), + [RTD1295_ISR_UR2_TO_SHIFT] = BIT(6), + [RTD1295_ISR_UR2_SHIFT] = BIT(7), + [RTD1295_ISR_RTC_MIN_SHIFT] = BIT(10), + [RTD1295_ISR_RTC_HOUR_SHIFT] = BIT(11), + [RTD1295_ISR_RTC_DATA_SHIFT] = BIT(12), + [RTD1295_ISR_I2C5_SHIFT] = BIT(14), + [RTD1295_ISR_I2C4_SHIFT] = BIT(15), + [RTD1295_ISR_GPIOA_SHIFT] = BIT(19), + [RTD1295_ISR_GPIODA_SHIFT] = BIT(20), + [RTD1295_ISR_LSADC0_SHIFT] = BIT(21), + [RTD1295_ISR_LSADC1_SHIFT] = BIT(22), + [RTD1295_ISR_SC0_SHIFT] = BIT(24), + [RTD1295_ISR_I2C2_SHIFT] = BIT(26), + [RTD1295_ISR_GSPI_SHIFT] = BIT(27), + [RTD1295_ISR_I2C3_SHIFT] = BIT(28), + [RTD1295_ISR_FAN_SHIFT] = BIT(29), +}; + +static const struct rtd1195_irq_mux_info rtd1295_iso_irq_mux_info = { + .isr_offset = 0x0, + .umsk_isr_offset = 0x4, + .scpu_int_en_offset = 0x40, + .isr_to_int_en_mask = rtd1295_iso_isr_to_scpu_int_en_mask, +}; + +static const struct rtd1195_irq_mux_info rtd1295_misc_irq_mux_info = { + .umsk_isr_offset = 0x8, + .isr_offset = 0xc, + .scpu_int_en_offset = 0x80, + .isr_to_int_en_mask = rtd1295_misc_isr_to_scpu_int_en_mask, +}; + +static const struct of_device_id rtd1295_irq_mux_dt_matches[] = { + { + .compatible = "realtek,rtd1295-iso-irq-mux", + .data = &rtd1295_iso_irq_mux_info, + }, + { + .compatible = "realtek,rtd1295-misc-irq-mux", + .data = &rtd1295_misc_irq_mux_info, + }, + { + } +}; + +static int __init rtd1195_irq_mux_init(struct device_node *node, + struct device_node *parent) +{ + struct rtd1195_irq_mux_data *data; + const struct of_device_id *match; + const struct rtd1195_irq_mux_info *info; + void __iomem *base; + + match = of_match_node(rtd1295_irq_mux_dt_matches, node); + if (!match) + return -EINVAL; + + info = match->data; + if (!info) + return -EINVAL; + + base = of_iomap(node, 0); + if (!base) + return -EIO; + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->info = info; + data->reg_isr = base + info->isr_offset; + data->reg_umsk_isr = base + info->umsk_isr_offset; + data->reg_scpu_int_en = base + info->scpu_int_en_offset; + + data->irq = irq_of_parse_and_map(node, 0); + if (data->irq <= 0) { + kfree(data); + return -EINVAL; + } + + raw_spin_lock_init(&data->lock); + + writel_relaxed(data->scpu_int_en, data->reg_scpu_int_en); + + data->domain = irq_domain_add_linear(node, 32, + &rtd1195_mux_irq_domain_ops, data); + if (!data->domain) { + kfree(data); + return -ENOMEM; + } + + irq_set_chained_handler_and_data(data->irq, rtd1195_mux_irq_handle, data); + + return 0; +} +IRQCHIP_DECLARE(rtd1295_iso_mux, "realtek,rtd1295-iso-irq-mux", rtd1195_irq_mux_init); +IRQCHIP_DECLARE(rtd1295_misc_mux, "realtek,rtd1295-misc-irq-mux", rtd1195_irq_mux_init);