Re: [PATCH v4 3/3] irqchip/realtek-rtl: use per-parent domains
From: Sander Vanheule
Date: Wed Feb 09 2022 - 16:22:36 EST
On Wed, 2022-02-09 at 14:10 +0000, Marc Zyngier wrote:
> On Sun, 06 Feb 2022 16:41:53 +0000,
> Sander Vanheule <sander@xxxxxxxxxxxxx> wrote:
> >
> > The interrupt router can connect each of its inputs to one of the parent
> > interrupts. These parent interrupts may be handled differently by their
> > interrupt controller. SoC interrupts should be treated per-parent, to
> > maintain this expected behaviour for routed child interrupts.
> >
> > For example, it is possible that both networking interrupts and the
> > system event timer interrupts are routed through this controller. Even
> > under high network load, event timer interrupts should take precedence,
> > which can be ensured by routing them to a higher priority parent.
> >
> > Rework the driver to use a separate domain for each output, using all
> > available parents interrupts (as specified in the devicetree). A
> > per-parent mask of child interrupts is used to keep track of which
> > domain should handle which interrupts.
>
> So you are encoding a particular priority scheme in the device tree
> even if this is really under SW control? That's pretty horrible.
>
What would be the alternative? I would really rather not hardcode priority tables in the
driver... For what it's worth, I see that at least "arm,nvic" and "atmel,aic" have
explicit priority specifier cells.
> >
> > Signed-off-by: Sander Vanheule <sander@xxxxxxxxxxxxx>
> > ---
> > drivers/irqchip/irq-realtek-rtl.c | 150 ++++++++++++++++++++++++------
> > 1 file changed, 124 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
> > index 388f4a7bfb80..868eb9b25e84 100644
> > --- a/drivers/irqchip/irq-realtek-rtl.c
> > +++ b/drivers/irqchip/irq-realtek-rtl.c
> > @@ -22,12 +22,22 @@
> > #define RTL_ICTL_IRR3 0x14
> >
> > #define RTL_ICTL_NUM_INPUTS 32
> > +#define RTL_ICTL_NUM_OUTPUTS 15
> >
> > #define REG(x) (realtek_ictl_base + x)
> >
> > static DEFINE_RAW_SPINLOCK(irq_lock);
> > static void __iomem *realtek_ictl_base;
> >
> > +struct realtek_ictl_output {
> > + /* IRQ controller data */
> > + struct fwnode_handle *fwnode;
> > + /* Output specific data */
> > + unsigned int output_index;
> > + struct irq_domain *domain;
> > + u32 child_mask;
> > +};
> > +
> > /*
> > * IRR0-IRR3 store 4 bits per interrupt, but Realtek uses inverted numbering,
> > * placing IRQ 31 in the first four bits. A routing value of '0' means the
> > @@ -37,6 +47,11 @@ static void __iomem *realtek_ictl_base;
> > #define IRR_OFFSET(idx) (4 * (3 - (idx * 4) / 32))
> > #define IRR_SHIFT(idx) ((idx * 4) % 32)
> >
> > +static inline u32 read_irr(void __iomem *irr0, int idx)
>
> You can drop the inline. The compiler is brave enough to try that itself.
>
> > +{
> > + return (readl(irr0 + IRR_OFFSET(idx)) >> IRR_SHIFT(idx)) & 0xf;
> > +}
> > +
> > static inline void write_irr(void __iomem *irr0, int idx, u32 value)
> > {
> > unsigned int offset = IRR_OFFSET(idx);
> > @@ -84,51 +99,128 @@ static struct irq_chip realtek_ictl_irq = {
> >
> > static int intc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> > {
> > + struct realtek_ictl_output *output = d->host_data;
> > unsigned long flags;
> > + u32 routing_old;
> > + int err = 0;
> > +
> > + raw_spin_lock_irqsave(&irq_lock, flags);
> > +
> > + /*
> > + * Inputs can only be routed to one output, so they shouldn't end up in
> > + * multiple domains. Perform this check in the same atomic context as
> > + * configuring the routing to prevent races.
> > + */
> > + routing_old = read_irr(REG(RTL_ICTL_IRR0), hw);
> > + if (routing_old && output->output_index != routing_old - 1) {
> > + pr_err("int %ld already routed to output %d\n",
> > + hw, routing_old - 1);
> > + err = -EINVAL;
> > + goto out;
> > + }
> > +
> > + output->child_mask |= BIT(hw);
> > + write_irr(REG(RTL_ICTL_IRR0), hw, output->output_index + 1);
> >
> > irq_set_chip_and_handler(irq, &realtek_ictl_irq, handle_level_irq);
> >
> > - raw_spin_lock_irqsave(&irq_lock, flags);
> > - write_irr(REG(RTL_ICTL_IRR0), hw, 1);
> > +out:
> > raw_spin_unlock_irqrestore(&irq_lock, flags);
> >
> > - return 0;
> > + return err;
> > +}
> > +
> > +static int intc_select(struct irq_domain *d, struct irq_fwspec *fwspec,
> > + enum irq_domain_bus_token bus_token)
> > +{
> > + struct realtek_ictl_output *output = d->host_data;
> > +
> > + if (fwspec->fwnode != output->fwnode)
> > + return false;
> > +
> > + /* Original specifiers only had one parameter */
> > + if (WARN_ON_ONCE(fwspec->param_count < 2))
> > + return true;
>
> You already warned when booting and finding the old binding. Doing it
> again probably is superfluous.
>
> > +
> > + return fwspec->param[1] == output->output_index;
> > }
> >
> > static const struct irq_domain_ops irq_domain_ops = {
> > .map = intc_map,
> > + .select = intc_select,
> > .xlate = irq_domain_xlate_onecell,
> > };
> >
> > static void realtek_irq_dispatch(struct irq_desc *desc)
> > {
> > + struct realtek_ictl_output *output = irq_desc_get_handler_data(desc);
> > struct irq_chip *chip = irq_desc_get_chip(desc);
> > - struct irq_domain *domain;
> > unsigned long pending;
> > unsigned int soc_int;
> >
> > chained_irq_enter(chip, desc);
> > - pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR));
> > + pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR))
> > + & output->child_mask;
> >
> > if (unlikely(!pending)) {
> > spurious_interrupt();
> > goto out;
> > }
> >
> > - domain = irq_desc_get_handler_data(desc);
> > - for_each_set_bit(soc_int, &pending, 32)
> > - generic_handle_domain_irq(domain, soc_int);
> > + for_each_set_bit(soc_int, &pending, RTL_ICTL_NUM_INPUTS)
> > + generic_handle_domain_irq(output->domain, soc_int);
> >
> > out:
> > chained_irq_exit(chip, desc);
> > }
> >
> > +static int __init setup_parent_interrupts(struct device_node *node, int *parents,
> > + unsigned int num_parents)
> > +{
> > + struct realtek_ictl_output *outputs;
> > + struct realtek_ictl_output *output;
> > + struct irq_domain *domain;
> > + unsigned int p;
> > +
> > + outputs = kcalloc(num_parents, sizeof(*outputs), GFP_KERNEL);
> > + if (!outputs)
> > + return -ENOMEM;
> > +
> > + for (p = 0; p < num_parents; p++) {
> > + output = outputs + p;
> > +
> > + domain = irq_domain_add_simple(node, RTL_ICTL_NUM_INPUTS, 0,
> > + &irq_domain_ops, output);
>
> Consider using irq_domain_add_linear() instead. add_simple really is
> legacy compatibility cruft.
I had changed this originally, but decided to stick to add_simple to avoid unrelated
changes. Unless you rather have that I roll into one of the current patches, I'll add a
small extra patch to implement this switch.
>
> > + if (!domain)
> > + goto domain_err;
> > +
> > + output->fwnode = of_node_to_fwnode(node);
> > + output->output_index = p;
> > + output->domain = domain;
> > +
> > + irq_set_chained_handler_and_data(parents[p], realtek_irq_dispatch,
> > output);
> > + }
> > +
> > + return 0;
> > +
> > +domain_err:
> > + while (p--) {
> > + irq_set_chained_handler_and_data(parents[p], NULL, NULL);
> > + irq_domain_remove(outputs[p].domain);
> > + }
> > +
> > + kfree(outputs);
> > +
> > + return -ENOMEM;
> > +}
> > +
> > static int __init realtek_rtl_of_init(struct device_node *node, struct device_node
> > *parent)
> > {
> > + int parent_irqs[RTL_ICTL_NUM_OUTPUTS];
> > struct of_phandle_args oirq;
> > - struct irq_domain *domain;
> > + unsigned int num_parents;
> > unsigned int soc_irq;
> > - int parent_irq;
> > + unsigned int p;
> >
> > realtek_ictl_base = of_iomap(node, 0);
> > if (!realtek_ictl_base)
> > @@ -139,37 +231,43 @@ static int __init realtek_rtl_of_init(struct device_node *node,
> > struct device_no
> > for (soc_irq = 0; soc_irq < RTL_ICTL_NUM_INPUTS; soc_irq++)
> > write_irr(REG(RTL_ICTL_IRR0), soc_irq, 0);
> >
> > - if (WARN_ON(!of_irq_count(node))) {
> > + num_parents = of_irq_count(node);
> > + if (num_parents > RTL_ICTL_NUM_OUTPUTS) {
> > + pr_err("too many parent interrupts\n");
> > + return -EINVAL;
> > + }
> > +
> > + for (p = 0; p < num_parents; p++)
> > + parent_irqs[p] = of_irq_get(node, p);
> > +
> > + if (WARN_ON(!num_parents)) {
> > /*
> > * If DT contains no parent interrupts, assume MIPS CPU IRQ 2
> > * (HW0) is connected to the first output. This is the case for
> > * all known hardware anyway. "interrupt-map" is deprecated, so
> > * don't bother trying to parse that.
> > + * Since this is to account for old devicetrees with one-cell
> > + * interrupt specifiers, only one output domain is needed.
> > */
> > oirq.np = of_find_compatible_node(NULL, NULL, "mti,cpu-interrupt-
> > controller");
> > oirq.args_count = 1;
> > oirq.args[0] = 2;
> >
> > - parent_irq = irq_create_of_mapping(&oirq);
> > + parent_irqs[0] = irq_create_of_mapping(&oirq);
> > + num_parents = 1;
> >
> > of_node_put(oirq.np);
> > - } else {
> > - parent_irq = of_irq_get(node, 0);
> > }
> >
> > - if (parent_irq < 0)
> > - return parent_irq;
> > - else if (!parent_irq)
> > - return -ENODEV;
> > -
> > - domain = irq_domain_add_simple(node, RTL_ICTL_NUM_INPUTS, 0,
> > - &irq_domain_ops, NULL);
> > - if (!domain)
> > - return -ENOMEM;
> > -
> > - irq_set_chained_handler_and_data(parent_irq, realtek_irq_dispatch, domain);
> > + /* Ensure we haven't collected any errors before proceeding */
> > + for (p = 0; p < num_parents; p++) {
> > + if (parent_irqs[p] < 0)
> > + return parent_irqs[p];
> > + if (!parent_irqs[p])
> > + return -ENODEV;
> > + }
> >
> > - return 0;
> > + return setup_parent_interrupts(node, &parent_irqs[0], num_parents);
>
> Directly use 'parent' instead of &parent[0].
Will update this, the WARN_ON_ONCE, and the inline in the next revision.
Best,
Sander