From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753621AbcGDMLh (ORCPT ); Mon, 4 Jul 2016 08:11:37 -0400 Received: from smtp2-g21.free.fr ([212.27.42.2]:53401 "EHLO smtp2-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753400AbcGDMLb (ORCPT ); Mon, 4 Jul 2016 08:11:31 -0400 Subject: Re: [RFC PATCH v1] irqchip: add support for SMP irq router To: Sebastian Frias , LKML Cc: Thomas Gleixner , Jason Cooper , Marc Zyngier References: <577542D1.4070307@laposte.net> From: Mason Message-ID: <577A5260.3070001@free.fr> Date: Mon, 4 Jul 2016 14:11:12 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0 SeaMonkey/2.40 MIME-Version: 1.0 In-Reply-To: <577542D1.4070307@laposte.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 : "") 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.