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.
>
> 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.
> + 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].
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
