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 <[email protected]>
Cc: Aleix Roca Nonell <[email protected]>
Signed-off-by: James Tai <[email protected]>
Signed-off-by: Andreas Färber <[email protected]>
---
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);
+ }
+
+ 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);
--
2.16.4
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 <[email protected]>
> Cc: Aleix Roca Nonell <[email protected]>
> Signed-off-by: James Tai <[email protected]>
> Signed-off-by: Andreas Färber <[email protected]>
> ---
> 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...
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
--
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)
On Tue, 19 Nov 2019 21:56:48 +0100
Andreas Färber <[email protected]> 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.
--
Jazz is not dead. It just smells funny...
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.
--
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)
Am 19.11.19 um 23:29 schrieb Marc Zyngier:
> On Tue, 19 Nov 2019 21:56:48 +0100
> Andreas Färber <[email protected]> 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
--
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)
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.
--
Jazz is not dead. It just smells funny...
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 <[email protected]> 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.
--
Jazz is not dead. It just smells funny...
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.
--
Jazz is not dead. It just smells funny...
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
--
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)
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 <[email protected]> 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
--
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)
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 <[email protected]> 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.
--
Jazz is not dead. It just smells funny...