Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Mason <slash.tmp@free.fr>]

On 30/06/2016 18:03, Sebastian Frias wrote:

> This adds support for a second-gen irq router/controller present
> on some Sigma Designs chips.

In the patch subject, do you mean SMP as in Symmetric Multi Processor?


> Signed-off-by: Sebastian Frias <sf84@laposte.net>

Is that the address you intend to submit with?


> This is a RFC because I have a few doubts:
> 1) I had to unroll irq_of_parse_and_map() in order to get the HW
> IRQ declared in the device tree so that I can associate it with
> a given domain.
> 
> 2) I'm not sure about the DT specification, in particular, the use
> of child nodes to declare different domains, but it works
> 
> 3) I'm calling this an irq router to somehow highlight the fact
> that it is not a simple interrupt controller. Indeed it does
> not latch the IRQ lines by itself, and does not supports edge
> detection.
> 
> 4) Do I have to do something more to handle the affinity stuff?
> 
> 5) checkpatch.pl reports warnings, etc. but I guess it's ok for
> now since it is a RFC :-)
> 
> Please feel free to comment and suggest improvements.

The "core" topic is over my head, so I'll just discuss the color
of the bike shed.

>  .../sigma,smp87xx-irqrouter.txt                    |  69 +++

In the *actual* submission, we can't use a wildcard like smp87xx
we'll have to use an actual part number.

>  drivers/irqchip/Makefile                           |   1 +
>  drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++

Likewise, I don't like the "_v2" suffix, it's too generic.
Actual submission should use something more specific.

> +++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt
> @@ -0,0 +1,69 @@
> +* Sigma Designs Interrupt Router
> +
> +This module can route N IRQ inputs into M IRQ outputs, with N>M.
> +For instance N=128, M=24.
> +
> +Note however that the HW does not latches the IRQ lines, so devices
                                     ^^^^^^^
"does not latch"

> +Required properties:
> +- compatible: Should be "sigma,smp87xx-irqrouter".

Same comment about wildcard.

> +- interrupt-controller: Identifies the node as an interrupt controller.
> +- inputs: The number of IRQ lines entering the router
> +- outputs: The number of IRQ lines exiting the router

As far as I can tell, if N > 256 then the register layout would
have to change. (Likewise, if M > 32)

Also, you hard-code the fact that N/32 = 4 with the status_i variables.

Would it make sense to use for loops?
(instead of unrolling the loops)

  e.g. for (i = 0; i < N/4; ++i) { ... }


> +Optional properties:
> +- interrupt-parent: pHandle of the parent interrupt controller, if not
> +  inherited from the parent node.

I'm not sure this is what "optional" means.


> +The following example declares a irqrouter with 128 inputs and 24 outputs,
> +with registers @ 0x6F800 and connected to the GIC.
> +The two child nodes define two irqdomains, one connected to GIC input 2
> +(hwirq=2, level=high), and ther other connected to GIC input 3 (hwirq=3,
                              ^^^^

> +#define ROUTER_INPUTS  (128)
> +#define ROUTER_OUTPUTS (24)

Parentheses are unnecessary around constants.

> +#define IRQ_ROUTER_ENABLE_MASK (BIT(31))
> +#define IRQ_ROUTER_INVERT_MASK (BIT(16))

Parentheses already provided in BIT macro.

> +#define READ_SYS_IRQ_GROUP0 (0x420)
> +#define READ_SYS_IRQ_GROUP1 (0x424)
> +#define READ_SYS_IRQ_GROUP2 (0x428)
> +#define READ_SYS_IRQ_GROUP3 (0x42C)

If a for loop were used, we'd only need to
#define SYSTEM_IRQ	0x420

  for (i = 0; i < N/4; ++i) {
    status_i = readl(base + SYSTEM_IRQ + i*4);

> +#if 0
> +#define SHORT_OR_FULL_NAME full_name
> +#else
> +#define SHORT_OR_FULL_NAME name
> +#endif

Just pick one?

> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME : "<no-node>")

Is it possible for a node to not have a name?

I also think prefixing/postfixing macro parameters with underscores
is positively fugly.

> +/*
> +  context for the driver
> +*/
> +struct tango_irqrouter {
> +	raw_spinlock_t lock;

Is this lock really needed?

Is tango_irqdomain_handle_cascade_irq() expected to be called
concurrently on multiple cores?

Even then, is it necessary to lock, in order to read 4 MMIO regs?

> +	struct device_node *node;
> +	void __iomem *base;
> +	u32 input_count;
> +	u32 output_count;
> +	u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> +	u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> +	struct tango_irqrouter_output output[ROUTER_OUTPUTS];
> +};

Hmmm, if the driver were truly "parameterizable", I guess we should
dynamically allocate the arrays.

> +static void tango_irqchip_mask_irq(struct irq_data *data)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	u32 hwirq = data->hwirq;
> +	u32 offset = IRQ_TO_OFFSET(hwirq);
> +	u32 value = tango_readl(irqrouter, offset);
> +	u32 hwirq_reg_index = hwirq / 32;
> +	u32 hwirq_bit_index = hwirq % 32;
> +
> +	DBGLOG("%s: mask hwirq(in) %d : current regvalue 0x%x (routed to hwirq(out) %d)\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       hwirq, value, irqrouter_output->hwirq);
> +
> +	//mask cache
> +	irqrouter->irq_mask[hwirq_reg_index] &= ~(1 << hwirq_bit_index);
> +
> +	value &= ~(IRQ_ROUTER_ENABLE_MASK);
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +static void tango_irqchip_unmask_irq(struct irq_data *data)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	u32 hwirq = data->hwirq;
> +	u32 offset = IRQ_TO_OFFSET(hwirq);
> +	u32 value = tango_readl(irqrouter, offset);
> +	u32 hwirq_reg_index = hwirq / 32;
> +	u32 hwirq_bit_index = hwirq % 32;
> +
> +	DBGLOG("%s: unmask hwirq(in) %d : current regvalue 0x%x (routed to hwirq(out) %d)\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       hwirq, value, irqrouter_output->hwirq);
> +
> +	//unmask cache
> +	irqrouter->irq_mask[hwirq_reg_index] |= (1 << hwirq_bit_index);
> +
> +	value |= IRQ_ROUTER_ENABLE_MASK;
> +	tango_writel(irqrouter, offset, value);
> +}

There might be an opportunity to factorize the two functions,
and have mask/unmask call such "helper" with the proper args.


> +#define HANDLE_INVERTED_LINES(__irqstatus__, __x__) ((((~__irqstatus__) & irqrouter->irq_invert_mask[__x__]) & irqrouter->irq_mask[__x__]) | __irqstatus__)
> +#define HANDLE_ENABLE_AND_INVERSION_MASKS(__irqstatus__, __y__) (HANDLE_INVERTED_LINES(__irqstatus__, __y__) & irqrouter->irq_mask[__y__])

I'm pretty sure these macros make baby Jesus cry.


> +static int __init tango_of_irq_init(struct device_node *node,
> +				    struct device_node *parent)
> +{
> +	struct tango_irqrouter *irqrouter;
> +	struct device_node *child;
> +	void __iomem *base;
> +	u32 input_count, output_count, i;
> +
> +	base = of_iomap(node, 0);
> +	if (!base) {
> +		DBGERR("%s: failed to map combiner registers\n", node->name);
> +		return -ENXIO;
> +	}
> +
> +	of_property_read_u32(node, "inputs", &input_count);
> +	if (!input_count) {
> +		DBGERR("%s: missing 'inputs' property\n", node->name);
> +		return -EINVAL;
> +	}
> +
> +	of_property_read_u32(node, "outputs", &output_count);
> +	if (!output_count) {
> +		DBGERR("%s: missing 'outputs' property\n", node->name);
> +		return -EINVAL;
> +	}
> +
> +	if ((input_count != ROUTER_INPUTS)
> +	    || (output_count != ROUTER_OUTPUTS)) {
> +		DBGERR("%s: input/output count mismatch", node->name);
> +		return -EINVAL;
> +	}

So the driver is not intended to be parameterized?
(or perhaps only in a follow-up?)

Regards.