Messages in this thread | ![/](/images/icornerl.gif) | | From | Thomas Gleixner <> | Subject | Re: [PATCH 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs | Date | Mon, 06 Nov 2023 12:30:51 +0100 |
| |
On Thu, Nov 02 2023 at 22:27, James Tai wrote: > Realtek DHC (Digital Home Center) SoCs share a common interrupt controller design. > This universal interrupt controller driver provides support for various variants > within the Realtek DHC SoC family. > > Each DHC SoC features two sets of extended interrupt controllers, each capable of > handling up to 32 interrupts. These expansion controllers are connected to the GIC > (Generic Interrupt Controller). > > Signed-off-by: James Tai <james.tai@realtek.com> > Change-Id: I1e711c70414c97c2c8497bd4ac9e8bbd964225c3
Please remove these internal change ids. They are not useful for anyone outside realtek.
> +config REALTEK_DHC_INTC > + tristate > + select IRQ_DOMAIN > +
> +static unsigned int realtek_intc_get_ints(struct realtek_intc_data *data)
static inline perhaps?
> +{ > + return readl(data->base + data->info->isr_offset); > +} > + > +static void realtek_intc_clear_ints_bit(struct realtek_intc_data *data, int bit) > +{ > + writel(BIT(bit) & ~1, data->base + data->info->isr_offset); > +} > + > +static unsigned int realtek_intc_get_inte(struct realtek_intc_data *data) > +{ > + unsigned int val; > + unsigned long flags;
unsigned long flags; unsigned int val;
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
Please fix the variable declarations all over the place.
> + spin_lock_irqsave(&data->lock, flags);
This needs to be a raw spinlock.
> + val = readl(data->base + data->info->scpu_int_en_offset); > + spin_unlock_irqrestore(&data->lock, flags); > + > + return val; > +}
> +static void realtek_intc_enable_irq(struct irq_data *data) > +{ > + struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data); > + unsigned long flags; > + u32 scpu_int_en, mask;
See above.
> + > + mask = intc_data->info->isr_to_scpu_int_en_mask[data->hwirq]; > + if (!mask) > + return; > + > + spin_lock_irqsave(&intc_data->lock, flags); > + > + scpu_int_en = readl(intc_data->base + intc_data->info->scpu_int_en_offset); > + scpu_int_en |= mask; > + writel(scpu_int_en, intc_data->base + intc_data->info->umsk_isr_offset); > + > + spin_unlock_irqrestore(&intc_data->lock, flags); > +} > + > +static void realtek_intc_disable_irq(struct irq_data *data) > +{ > + struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(data); > + unsigned long flags; > + u32 scpu_int_en, mask; > + > + mask = intc_data->info->isr_to_scpu_int_en_mask[data->hwirq]; > + if (!mask) > + return; > + > + spin_lock_irqsave(&intc_data->lock, flags); > + > + scpu_int_en = readl(intc_data->base + intc_data->info->scpu_int_en_offset); > + scpu_int_en &= ~mask; > + writel(scpu_int_en, intc_data->base + intc_data->info->umsk_isr_offset); > + > + spin_unlock_irqrestore(&intc_data->lock, flags); > +} > + > +static int realtek_intc_lookup_parent_irq(struct realtek_intc_data *intc_data, struct irq_data *d) > +{ > + unsigned int mask = BIT(d->hwirq); > + int i; > + > + for (i = 0; i < intc_data->subset_data_num; i++) > + if (intc_data->subset_data[i].cfg->ints_mask & mask) > + return intc_data->subset_data[i].parent_irq;
Lacks brackets around the for loop. See documentation.
> + > + return -EINVAL; > +} > + > +static int realtek_intc_set_affinity(struct irq_data *d, const struct cpumask *mask_val, bool force) > +{ > + struct realtek_intc_data *intc_data = irq_data_get_irq_chip_data(d); > + int irq; > + struct irq_chip *chip; > + struct irq_data *data; > + > + irq = realtek_intc_lookup_parent_irq(intc_data, d); > + if (irq < 0) > + return irq; > + > + chip = irq_get_chip(irq); > + data = irq_get_irq_data(irq);
So instead of two lookups you want to do:
data = irq_get_irq_data(irq); if (!data) return; chip = irq_data_get_irq_chip(data); ... > + > + irq_data_update_effective_affinity(d, cpu_online_mask);
So you update the effective affinity even if it cannot be set or if the parent irq returns an error code?
Aside of that setting it to cpu_online mask is just wrong. This is _NOT_ the effective affinity because the underlying GIC selects a single target CPU out of the caller provides cpu mask.
That said, this is also completely inconsistent vs. the other interrupts which share that GIC interrupt instance. I.e. /proc/irq/$N/affinity and effective_affinity become random number generators. That'll confuse existing userspace tools.
Having an affinity setter for demultiplexes interrupts is simply wrong.
> + if (chip && chip->irq_set_affinity) > + return chip->irq_set_affinity(data, mask_val, force); > + else > + return -EINVAL; > +} > + > +static struct irq_chip realtek_intc_chip = { > + .name = "realtek-intc", > + .irq_mask = realtek_intc_mask_irq, > + .irq_unmask = realtek_intc_unmask_irq, > + .irq_enable = realtek_intc_enable_irq, > + .irq_disable = realtek_intc_disable_irq, > + .irq_set_affinity = realtek_intc_set_affinity,
See docs vs. formatting of struct initializers.
> +}; > + > + > + data->subset_data_num = info->cfg_num; > + for (i = 0; i < info->cfg_num; i++) { > + ret = realtek_intc_subset(node, data, i); > + WARN(ret, "failed to init subset %d: %d", i, ret);
If this fails, then you still expose the affected interrupts as functional?
> +/** > + * realtek_intc_subset_cfg - subset interrupt mask > + * @ints_mask: inetrrupt mask > + */ > +struct realtek_intc_subset_cfg { > + unsigned int ints_mask; > +}; > + > +/** > + * realtek_intc_info - interrupt controller data. > + * @isr_offset: interrupt status register offset. > + * @umsk_isr_offset: unmask interrupt status register offset. > + * @scpu_int_en_offset: interrupt enable register offset. > + * @cfg: cfg of the subset. > + * @cfg_num: number of cfg. > + */ > +struct realtek_intc_info { > + unsigned int isr_offset; > + unsigned int umsk_isr_offset; > + unsigned int scpu_int_en_offset; > + const u32 *isr_to_scpu_int_en_mask; > + const struct realtek_intc_subset_cfg *cfg; > + int cfg_num;
See formatting doc.
> + > +#define IRQ_ALWAYS_ENABLED (-1)
U32_MAX ?
> +#define DISABLE_INTC (0) > +#define CLEAN_INTC_STATUS (0xfffffffe)
That's what GENMASK() is for.
Thanks,
tglx
| ![\](/images/icornerr.gif) |