Message-ID: <577BA854.6090503@laposte.net>
Date:	Tue, 5 Jul 2016 14:30:12 +0200
From:	Sebastian Frias <sf84@...oste.net>
To:	Mason <slash.tmp@...e.fr>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>,
	Marc Zyngier <marc.zyngier@....com>
Subject: Re: [RFC PATCH v1] irqchip: add support for SMP irq router

On 07/04/2016 02:11 PM, Mason wrote:
> 
> In the patch subject, do you mean SMP as in Symmetric Multi Processor?

As in Sigma Multimedia Processor :-)
The prefix for Sigma's chips is SMP.

I can change that to "Tango" if it is confusing.

> 
> Is that the address you intend to submit with?

Yes.

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

:-)
Thanks for your comments, hopefully some knowledgeable people will discuss the core topic as well.

> 
>>  .../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.

Are you sure?
That would hinder genericity.
Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").

To me there's no need to link the compatible string of a given HW module with that of the chip name the module it is embedded into.
For example, the generic USB3 driver is "generic-xhci".
While this module is not generic to be embedded in chips from different manufacturers, it is supposed to be generic within Sigma, and multiple Sigma chips (with potentially different denominations) can use it.

> 
>>  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.

Well, the other driver is irq-tango.c that is generic as well.
I prefer versioning, as it is unrelated with the actual chip name.

Following the reasoning from my previous reply, I think the file name should be something like "smp-irqrouter.c" to be close to the "compatible" string.

However, IMHO there is a mix of various naming conventions on the kernel tree so I don't know which one is recommended.

> 
>> +++ 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"

Ok.

> 
> 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) { ... }
> 

Actually my current version uses this (also on "tango_irqdomain_handle_cascade_irq()"), but I'm not sure I'm happy with it because the person reading may think the code is generic while it is not.
I mean, there is no guarantee on how the HW guys will extend this in the future, and trying to make the code generic could end up as wasted premature optimization.

> 
>> +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.
> 

>From what I've seen in the documentation for other DT bindings, this is the meaning of "optional".
But I don't mind changing it if I get some hints about the meaning.

> 
>> +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.

I'm used to use parenthesis in macros to avoid operator priority side effects.

> 
>> +#define IRQ_ROUTER_ENABLE_MASK (BIT(31))
>> +#define IRQ_ROUTER_INVERT_MASK (BIT(16))
> 
> Parentheses already provided in BIT macro.

Same as above.

> 
>> +#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);
> 

I suppose you mean "status[i]".
There's a trade-off between explicit and clear code, and compact code.
I believe in this case the unrolled code not only is more clear, it may end up being faster.

As I was saying earlier, my current version uses a mixed approach with the loop unrolled for reading the registers and a for loop for the dispatch. 

If in the future we have to read much more registers, we should revisit this.

>> +#if 0
>> +#define SHORT_OR_FULL_NAME full_name
>> +#else
>> +#define SHORT_OR_FULL_NAME name
>> +#endif
> 
> Just pick one?

By making the choice explicit, the person reading/debugging is aware of the possibilities.
Picking "full_name" or "name" would likely prevent the person from knowing it has other debug options.

> 
>> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME : "<no-node>")
> 
> Is it possible for a node to not have a name?

The code is about checking the 'node' is not NULL before dereferencing it;
Furthermore, printk is supposed to handle NULL pointers for %s ("be lenient with your input and strict with your output")

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

>From my point of view, it makes a clear difference between macro parameters and rest of the code.

> 
>> +/*
>> +  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?

Good question, I had asked myself the same.
include/linux/irq.h:irq_set_chained_handler_and_data() declaration has a comment saying that IRQ_NOTHREAD is used, but since I had seen a lock being used by other drivers, I thought to give it a try on this RFC.

> 
> 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.

The key is "if it were...", since it is likely it will never be generic, I thought that dynamically allocating the arrays was not worth it.

I agree that there's a sort of hybrid approach, part "static", part "dynamic", in the sense that some things could be dynamically allocated (like the fields above), or dynamically handled (like the "status" registers), yet constant (and thus "static") macros or unrolled loops are used to define them because the driver is not really generic.

I can bet that if I had dynamically allocated the arrays, somebody could have commented that it was not necessary since the driver is not fully generic :-)

>> +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.
> 

You are right, these grew from just logging to actually doing something and did not get factored.

>> +
>> +	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?)

It would be nice to have a fully generic driver, but I doubt that is possible.
Small changes would always be necessary due to the fact that the HW description itself (register distribution and their meaning) is not on the DT.
In the end it is a trade-off between code (and DT) clarity and simplicity, and fully generic code that would require lots of glue to handle a complex DT capable of describing the generic HW. In the current form, most of the code is actually to handle the IRQs, if it were generic I think a significant part of the code would be there just to deal with genericity, obscuring what the driver does.

So I went for a warning :-)

Best regards,

Sebastian

Powered by blists - more mailing lists