lkml.org 
[lkml]   [2019]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4 2/8] irqchip: Add Realtek RTD1295 mux driver
    Date
    From
    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...

    \
     
     \ /
      Last update: 2019-11-19 13:02    [W:3.456 / U:0.108 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site