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

[Sebastian Frias]

Hi Thomas,

Thanks for your comments.
I appreciate the time you dedicated to the review.
I will take your comments into account, in the meanwhile, and since this was
a RFC, would it be possible to also get some feedback/comments from a
conceptual point of view?

I'm copy/pasting some of the questions I attached to the RFC email here:

----
2) In order to get a virq mapping for the domains associated with the outputs
(the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
See irq-tango_v4.c:1400

	/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
	hwirq      = index + irqrouter->input_count + irqrouter->swirq_count;

This feels a bit like a hack, so suggestions are welcome.

3) The file is called 'irq-tango_v4.c' but I think it should match the
compatible string, so I was thinking of renaming:
irq-tango_v4.c => irq-sigma_smp_irqrouter.c
irq-tango_v4.h => irq-sigma_smp_irqrouter.h

What do you think?

4) Do I have to do something more to handle the affinity stuff?

6) More of a theoretical question:
I have to #include <dt-bindings/interrupt-controller/irq-tango_v4.h> from
code that is not in the Linux tree. That's fine for now, but if in the future
there are other irq controllers and another header is required, how can the
code know which header to #include ?
Are we supposed to #include all of them, and then somehow detect which module
is actually active and act accordingly? If so, can we detect which module
matched and probed correctly to know which controller version we need to
talk to?
----

What the driver does is:
- creates a domain hierarchy attached to its parent domain (the GIC)
  - the callbacks for this domain hierarchy will be used to deal with IRQs
  "directly" routed to the GIC (i.e.: exclusive IRQ lines routed to the GIC
  and not shared).
  In this case the GIC dispatches the interrupts.
- create linear domains attached to this driver's node.
  - the callbacks for these linear domains will be used to deal with shared
  IRQs (they share the routing to one of the GIC inputs).
  In this case this driver dispatches the interrupts after reading the flags.

It is such concept, and then its implementation, that I would need some more
advise with, because, as stated in the questions above, it still does not
feels right, especially question 2.

I will reply to your comments below and will post a v3 later, hopefully after
dealing also with the answers to the doubts outlined above.


On 07/19/2016 06:49 PM, Thomas Gleixner wrote:
> On Tue, 19 Jul 2016, Sebastian Frias wrote:
>> +
>> +#define DBGERR(__format, ...)	panic("[%s:%d] %s(): " __format,	\
>> +				      __FILE__, __LINE__,		\
>> +				      __func__, ##__VA_ARGS__)
> 
> Please get rid of this macro mess. Use the functions (panic, ...)
> directly. There is no value for this file, line, func crappola. Think about
> proper texts which can be grepped for.

Ok.

> 
>> +
>> +#define DBGWARN(__format, ...)	pr_err("[%s:%d] %s(): " __format,	\
> 
> Very consistent. WARN == err ....
> 
>> +				       __FILE__, __LINE__,		\
>> +				       __func__, ##__VA_ARGS__)
>> +
>> +
>> +#if 0
> 
> Don't even think about posting code with "#if 0" or such in it. 
> 

The idea behind these macros was:
- to easily enable/disable them.
- to be more consistent with other code written at Sigma: this is a
Sigma-only driver.

>> +#define DBGLOG(__format, ...)   pr_info("[%s:%d] %s(): " __format,	\
>> +					__FILE__, __LINE__,		\
>> +					__func__, ##__VA_ARGS__)
>> +#else
>> +#define DBGLOG(__format, ...) do {} while (0)
>> +#endif
> 
> pr_debug() is there for a reason.

Yes, but doesn't pr_debug() requires changes to the Makefile?

> 
>> +#if 0
>> +#define SHORT_OR_FULL_NAME full_name
>> +#else
>> +#define SHORT_OR_FULL_NAME name
>> +#endif
> 
> Use one and be done with it. Really.

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.

But I can change this if upstreaming depends on it.

> 
>> +
>> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME :	\
>> +			     "<no-node>")
> 
> Sigh. pr_xxx("%s", NULL) prints <null>. That's really sufficient for your
> debug/error handling.

The code is checking for __node__ to be non-NULL before dereferencing it.

> 
>> +
>> +#define BITMASK_VECTOR_SIZE(__count__) (__count__ / 32)
>> +#define IRQ_TO_OFFSET(__hwirq__) (__hwirq__ * 4)
>> +
>> +struct tango_irqrouter;
>> +
>> +/*
>> + * Maintains the mapping between a Linux virq and a hwirq
>> + * on the parent controller.
>> + * It is used by tango_irqdomain_map() or tango_irqdomain_hierarchy_alloc()
>> + * to setup the route between input IRQ and output IRQ
>> + */
>> +struct tango_irqrouter_output {
>> +	struct tango_irqrouter *context;
>> +
>> +	u32 domain_id;
>> +
>> +	u32 hwirq;
>> +	u32 hwirq_level;
>> +	u32 virq;
>> +
>> +	int shared_count;
>> +	int *shared_irqs;
>> +};
>> +
>> +/*
>> + * Context for the driver
>> + */
>> +struct tango_irqrouter {
>> +	raw_spinlock_t lock;
>> +	struct device_node *node;
>> +	void __iomem *base;
> 
> Plase align struct members proper
> 
> 	raw_spinlock_t		lock;
> 	struct device_node	*node;
> 	void __iomem		*base;

Ok.

> 
>> +
>> +	int input_count;
>> +	u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
>> +	u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> 
> You only ever use BITMASK_VECTOR_SIZE(ROUTER_INPUTS). Why can't you simply
> do:
> 
> #define ROUTER_INPUTs_MASK_SIZE	(ROUTER__INPUTS / 32) ????

I guess I can do that.

> 
>> +static inline u32 tango_readl(struct tango_irqrouter *irqrouter,
>> +			      int reg)
>> +{
>> +	u32 val = readl_relaxed(irqrouter->base + reg);
>> +	/*DBGLOG("r[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
>> +	  irqrouter->base, reg, irqrouter->base + reg, val);*/
>> +	return val;
> 
> Please get rid of this debug nonsense.

Ok, although I can guarantee it was really useful to determine and prove
that the HW documentation of the module was wrong...

> 
>> +}
>> +
>> +
>> +static inline void tango_writel(struct tango_irqrouter *irqrouter,
>> +				int reg,
>> +				u32 val)
>> +{
>> +	/*DBGLOG("w[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
>> +	  irqrouter->base, reg, irqrouter->base + reg, val);*/
>> +	writel_relaxed(val, irqrouter->base + reg);
>> +}
>> +
>> +
>> +static inline void tango_set_swirq_enable(struct tango_irqrouter *irqrouter,
>> +					  int swirq,
>> +					  bool enable)
>> +{
>> +	u32 offset = SWIRQ_ENABLE;
> 
> If you name that varible 'reg' then it becomes obvious what it does.

Well, it was to add a sort of "consistency" between the code for SW IRQs and HW IRQs.

> 
>> +	u32 value = tango_readl(irqrouter, offset);
>> +	u32 swirq_bit_index = swirq % SWIRQ_COUNT;
>> +
>> +#if 1
>> +	DBGLOG("%smask swirq(in) %d : current regvalue 0x%x\n",
>> +	       enable ? "un":"",
>> +	       swirq, value);
>> +#endif
>> +
>> +	if (enable) {
>> +		/* unmask swirq */
> 
> The whole code lacks comments, but here you document the obvious ....

I will remove the comments.

> 
>> +		irqrouter->swirq_mask |= (1 << swirq_bit_index);
>> +		value |= (1 << swirq_bit_index);
>> +	} else {
>> +		/* mask swirq */
>> +		irqrouter->swirq_mask &= ~(1 << swirq_bit_index);
>> +		value &= ~(1 << swirq_bit_index);
>> +	}
>> +
>> +	tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_hwirq_enable(struct tango_irqrouter *irqrouter,
>> +					  int hwirq,
>> +					  bool enable)
>> +{
>> +	u32 offset = IRQ_TO_OFFSET(hwirq);
>> +	u32 value = tango_readl(irqrouter, offset);
>> +	u32 hwirq_reg_index = hwirq / 32;
>> +	u32 hwirq_bit_index = hwirq % 32;
>> +	u32 *enable_mask = &(irqrouter->irq_mask[hwirq_reg_index]);
>> +
>> +#if 1
>> +	DBGLOG("%smask hwirq(in) %d : current regvalue 0x%x\n",
>> +	       enable ? "un":"",
>> +	       hwirq, value);
>> +#endif
>> +
>> +	if (enable) {
>> +		/* unmask irq */
>> +		*enable_mask |= (1 << hwirq_bit_index);
>> +		value |= IRQ_ROUTER_ENABLE_MASK;
>> +	} else {
>> +		/* mask irq */
>> +		*enable_mask &= ~(1 << hwirq_bit_index);
>> +		value &= ~(IRQ_ROUTER_ENABLE_MASK);
>> +	}
>> +
>> +	tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_swirq_inversion(struct tango_irqrouter *irqrouter,
>> +					     int swirq,
>> +					     bool invert)
>> +{
>> +
>> +	DBGLOG("swirq(in) %d %s inverted\n", swirq, invert ? "":"not");
>> +
>> +	if (invert)
>> +		DBGERR("SW IRQs cannot be inverted!\n");
> 
> Groan.

I rather keep this to catch typos in code requesting SW IRQs.

> 
>> +}
>> +
>> +
>> +static inline void tango_set_hwirq_inversion(struct tango_irqrouter *irqrouter,
>> +					     int hwirq,
>> +					     bool invert)
>> +{
>> +	u32 offset = IRQ_TO_OFFSET(hwirq);
>> +	u32 value = tango_readl(irqrouter, offset);
>> +	u32 hwirq_reg_index = hwirq / 32;
>> +	u32 hwirq_bit_index = hwirq % 32;
>> +	u32 *invert_mask = &(irqrouter->irq_invert_mask[hwirq_reg_index]);
>> +
>> +	if (invert) {
>> +		*invert_mask |= (1 << hwirq_bit_index);
>> +		value |= IRQ_ROUTER_INVERT_MASK;
>> +	} else {
>> +		*invert_mask &= ~(1 << hwirq_bit_index);
>> +		value &= ~(IRQ_ROUTER_INVERT_MASK);
>> +	}
>> +
>> +	DBGLOG("hwirq(in) %d %s inverted\n", hwirq, invert ? "":"not");
>> +
>> +	tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_swirq_route(struct tango_irqrouter *irqrouter,
>> +					 int swirq_in,
>> +					 int irq_out)
>> +{
>> +	u32 swirq_reg_index = swirq_in / 4;
>> +	u32 swirq_bit_index = (swirq_in % 4) * 8;
>> +	u32 mask = ~(0x1f << swirq_bit_index);
>> +	u32 offset = SWIRQ_MAP_GROUP0 + (swirq_reg_index * 4);
>> +	u32 value = tango_readl(irqrouter, offset);
>> +
>> +	DBGLOG("ri %d, bi %d, mask 0x%x, offset 0x%x, val 0x%x\n",
>> +	       swirq_reg_index,
>> +	       swirq_bit_index,
>> +	       mask,
>> +	       offset,
>> +	       value);
>> +
>> +	DBGLOG("route swirq %d => hwirq(out) %d\n", swirq_in, irq_out);
>> +
>> +	value &= mask;
>> +
>> +	if (irq_out < 0) {
> 
> This logic is utterly confusing, really. What's a negative interrupt number?

It is a sort of "magic value" to disable the IRQ and the routing.

> 
>> +		tango_set_irq_enable(irqrouter,
>> +				     swirq_in + irqrouter->input_count,
>> +				     0);
> 
> Now you write back 'value' with the bit for this irq masked. Is that correct?

Yes, tango_set_irq_enable() is used to setup the mask (in this case disable the IRQ
by masking it) to one register, and later we write 'value' (which deals with the
routing) to a different register.
Since up to 4 SW IRQ routes can be programmed in one of multiple routing registers,
to "disable" the routing for one of them, we need to mask the other 3 and leave them
untouched.
'value' is ANDed with 'mask' which is keeps the 3 other routing intact and resetting
the routing for the given IRQ.

> 
>> +	} else
> 
>   } else {
> 
>   please

I think I'm confused, we need to use {} for single statements on "else" blocks,
but not on "for" blocks?

> 
>> +		value |= ((irq_out & 0x1f) << swirq_bit_index);
>> +
>> +	tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_hwirq_route(struct tango_irqrouter *irqrouter,
>> +					 int irq_in,
>> +					 int irq_out)
>> +{
>> +	u32 offset = IRQ_TO_OFFSET(irq_in);
>> +	u32 value;
>> +
>> +	DBGLOG("route hwirq(in) %d => hwirq(out) %d\n", irq_in, irq_out);
>> +
>> +	if (irq_out < 0) {
>> +		tango_set_irq_enable(irqrouter,
>> +				     irq_in,
>> +				     0);
> 
> This fits in a single line.

Ok.
Actually, I was wondering if for consistency it wouldn't be better if all
function calls had one argument per line?

> 
>> +		value = 0;
> 
> Please document why you are setting the value to 0 here.

Ok.
(it is for the same reason than for SW IRQs, to "disable" the routing, or more
precisely, to set disabled IRQs to a known value, in this case 0)

> 
>> +	} else
>> +		value = (irq_out & 0x1f);
>> +
>> +	tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline int tango_set_irq_enable(struct tango_irqrouter *irqrouter,
>> +				       int irq,
>> +				       bool enable)
>> +{
>> +	if (irq >= (irqrouter->input_count + irqrouter->swirq_count))
>> +		return -EINVAL;
> 
> How can that happen? Paranoia programming?

No, actually it can happen, that's the reason for question 2 of my RFC:

----
2) In order to get a virq mapping for the domains associated with the outputs
(the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
See irq-tango_v4.c:1400

/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
hwirq = index + irqrouter->input_count + irqrouter->swirq_count;

This feels a bit like a hack, so suggestions are welcome.
----


> 
>> +	else if (irq >= irqrouter->input_count)
> 
> That "else" is pointless. You return above.

Ok.

> 
>> +		tango_set_swirq_enable(irqrouter,
>> +				       irq - irqrouter->input_count,
>> +				       enable);
>> +	else
>> +		tango_set_hwirq_enable(irqrouter,
>> +				       irq,
>> +				       enable);
> 
> This can be written way simpler.
> 
>      	if (irq >= irqrouter->input_count)
> 	   	irq -= irqrouter->input_count;
> 	tango_set_hwirq_enable(irqrouter, irq, enable);
>     	  
> Hmm?

Ok, but is it ok to modify function parameters?
I thought it was not recommended.
Indeed, some people may assume they are constants.

> 
>> +static int tango_set_irq_type(struct tango_irqrouter *irqrouter,
>> +			      int hwirq_in,
>> +			      u32 type,
>> +			      u32 parent_type)
>> +{
>> +	int err;
>> +
>> +	if (parent_type & (type & IRQ_TYPE_SENSE_MASK))
>> +		/* same polarity */
>> +		err = tango_set_irq_inversion(irqrouter, hwirq_in, 0);
>> +	else
>> +		/* invert polarity */
>> +		err = tango_set_irq_inversion(irqrouter, hwirq_in, 1);
> 
>   	bool invert = parent_type & (type & IRQ_TYPE_SENSE_MASK);
> 
>   	err = tango_set_irq_inversion(irqrouter, hwirq_in, invert);	
> 

Ok.

>> +
>> +	if (err < 0) {
>> +		DBGWARN("Failed to setup IRQ %d polarity\n", hwirq_in);
>> +		return err;
>> +	}
>> +
>> +	switch (type & IRQ_TYPE_SENSE_MASK) {
>> +	case IRQ_TYPE_EDGE_RISING:
>> +	case IRQ_TYPE_EDGE_FALLING:
>> +		DBGERR("Does not support edge triggers\n");
> 
> This really sucks. In fact you panic here while the code suggests that you
> print a DEBUG error and continue .....
> 

If I use panic() here, do I still have to use 'break' as well?

>> +		break;
>> +	case IRQ_TYPE_LEVEL_HIGH:
>> +		break;
> 
> You can spare that break.

Ok.
(before there were logs so the 'break' was necessary)

> 
>> +	case IRQ_TYPE_LEVEL_LOW:
>> +		break;
>> +	default:
>> +		DBGWARN("Invalid trigger mode 0x%x for hwirq(in) %d\n",
>> +			type, hwirq_in);
> 
> So here you continue with an error code, but above for EDGE you panic. I
> really don't get that logic.

Ok.
Do you suggest I just make the code panic for any error? I'm fine with that.

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +static int tango_get_output_for_hwirq(struct tango_irqrouter *irqrouter,
>> +				      int hwirq_in,
>> +				      struct tango_irqrouter_output **out_val)
>> +{
>> +	struct tango_irqrouter_output *irqrouter_output;
>> +	int i;
>> +
>> +	if (!out_val)
>> +		return -EINVAL;
> 
> This is utter crap. Look at the call site:

I think I'm confused, so it is not recommended to check for NULL pointers before
dereferencing them? Regardless of how the current code calls the function?

> 
>> +	struct tango_irqrouter_output *irqrouter_output = NULL;
>> +
>> +	tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
>> +	if (!irqrouter_output)
>> +		goto handle_parent;
> 
> So out_val CANNOT be NULL. 

You are right, with current code it can't.
But I thought that it was a good practice to:
"be lenient with your input and strict with your output"

>What's the purpose of your return values if you
> ignore them?

I can make the function 'void'.

> 
>> +
>> +	/* Get the irqrouter_output for the hwirq */
> 
> This is another really helpful comment.

It is true that now that this is a function, the name of the function
speaks for itself, so I can remove it.

> 
>> +	for (i = 0; i < irqrouter->output_count; i++) {
>> +		int j;
>> +
>> +		irqrouter_output = &(irqrouter->output[i]);
>> +
>> +		for (j = 0; j < irqrouter_output->shared_count; j++) {
>> +			if (hwirq_in == irqrouter_output->shared_irqs[j])
>> +				goto found_router_output;
>> +		}
>> +	}
>> +	if (i == irqrouter->output_count) {
> 
> What other reason would be to get here than this?

You are right, relic of a refactoring.

> 
>> +		DBGWARN("Couldn't find hwirq mapping\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +found_router_output:
>> +
>> +	*out_val = irqrouter_output;
>> +	return 0;
>> +}
> 
> This whole thing can be condensed to:
> 
> static struct tango_irqrouter_output *
> tango_get_output_for_hwirq(struct tango_irqrouter *irqr, int hwirq_in)
> {
> 	struct tango_irqrouter_output *rout;
> 	int i, j;
> 
> 	/* Scan the outputs for a matching hwirq number */
> 	for (i = 0, rout = irqr->output; i < irqr->output_count; i++, rout++) {
> 		for (j = 0; j < rout->shared_count; j++) {
> 			if (rout->shared_irqs[j] == hwirq_in)
> 			   	return rout;
> 		}
> 	}
> 	return NULL;
> }
> 
> And the call site would become:
> 
>     	struct tango_irqrouter_output *rout;
> 
> 	rout = tango_get_output_for_hwirq(irqrouter, hwirq_in);
> 	if (!rout)
> 		goto handle_parent;
> 
> Hmm?

Ok.

> 
>> +/* 'irqchip' handling callbacks
>> + * Used for 'shared' IRQs, i.e.: IRQs that share a GIC input
>> + * This driver performs the IRQ dispatch based on the flags
>> + */
>> +
> 
> Multiline comments have this format:
> 	 
> /*
>  * First line
>  * ....
>  * Last line
>  */

Ok.

> 
>> +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;
>> +	int hwirq_in = (int)data->hwirq;
> 
> Huch? What's that silly typecast for? Why can't you use u32 for hwirq_in?

data->hwirq is of type irq_hw_number_t (unsigned long), I don't know if that 
can change in the future.

So do you think it is safe to use u32 for data->hwirq (and all other "irq"
occurrences)?

> 
>> +	tango_set_irq_enable(irqrouter, hwirq_in, 0);
>> +}
> 
>> +#ifdef CONFIG_SMP
>> +static int tango_irqchip_set_irq_affinity(struct irq_data *data,
>> +					  const struct cpumask *mask_val,
>> +					  bool force)
>> +{
>> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct irq_chip *parent_chip = irq_get_chip(irqrouter_output->virq);
>> +	struct irq_data *parent_data = irq_get_irq_data(irqrouter_output->virq);
>> +
>> +	DBGLOG("%s:%s(0x%p)\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain);
>> +
>> +	if (parent_chip && parent_chip->irq_set_affinity)
>> +		return parent_chip->irq_set_affinity(parent_data,
>> +						     mask_val,
>> +						     force);
>> +	else
>> +		return -EINVAL;
> 
> You really can spare identation levels by writing it a bit more clever:
> 
>     	if (!parent || !parent->irq_set_affinity)
> 	   	 return -EINVAL;
> 
> 	return parent->irq_set_affinity(parent, mask, force);
> 
> You might notice that I shortened the variable names and it still is entirely
> clear what the code does.

Ok.

> 
> Aside of that mask_val is a clear misnomer.

Actually, this code comes from exynos-combiner.c and is part of the reason for
question 4 of the RFC:

----
4) Do I have to do something more to handle the affinity stuff?
----

> 
>> +}
>> +#endif
>> +
>> +
>> +static inline u32 tango_dispatch_irqs(struct irq_domain *domain,
>> +				      struct irq_desc *desc,
>> +				      u32 status,
>> +				      int base)
>> +{
>> +	u32 hwirq;
>> +	u32 virq;
> 
> Please put same types onto a single line

Ok.

> 
>> +
>> +	while (status) {
>> +		hwirq = __ffs(status);
>> +		virq = irq_find_mapping(domain, base + hwirq);
>> +		if (unlikely(!virq))
>> +			handle_bad_irq(desc);
>> +		else
>> +			generic_handle_irq(virq);
>> +
>> +		status &= ~BIT(hwirq);
>> +	}
>> +
>> +	return status;
> 
> Huch? status is guaranteed to be 0 here. So what's the point of this return
> value.

You are right, relic of a refactoring.
I wanted to do:

+		if (unlikely(!virq))
+			handle_bad_irq(desc);
+		else {
+			generic_handle_irq(virq);
+                       status_out &= ~BIT(hwirq);
+               }
+		status &= ~BIT(hwirq);

with status_out=status at the start of the function.

> 
>> +}
>> +
>> +static void tango_irqdomain_handle_cascade_irq(struct irq_desc *desc)
>> +{
>> +	struct irq_domain *domain = irq_desc_get_handler_data(desc);
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> +	struct irq_chip *host_chip = irq_desc_get_chip(desc);
>> +	u32 i, status;
>> +	u32 swirq_status, irq_status[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
>> +
>> +#if 0
>> +	DBGLOG("%s:%s(0x%p): irqrouter_output 0x%p, hwirq(out) %d\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
>> +	       irqrouter_output, irqrouter_output->hwirq);
>> +#endif
>> +
>> +	chained_irq_enter(host_chip, desc);
>> +
>> +	raw_spin_lock(&(irqrouter->lock));
>> +	swirq_status = tango_readl(irqrouter, READ_SWIRQ_STATUS);
>> +	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++)
>> +		irq_status[i] = tango_readl(irqrouter,
>> +					    READ_SYS_IRQ_GROUP0 + i*4);
> 
>   i * 4		please

Ok.

> 
>> +	raw_spin_unlock(&(irqrouter->lock));
>> +
>> +	/* HW irqs */
> 
> And that comment tells us what?
> 
>> +	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++) {
>> +#if 0
>> +		DBGLOG("%d: 0x%08x (en 0x%08x, inv 0x%08x)\n",
>> +		       i,
>> +		       irq_status[i],
>> +		       irqrouter->irq_mask[0],
>> +		       irqrouter->irq_invert_mask[0]);
>> +#endif
>> +
>> +#define HANDLE_INVERTED_LINES(__irqstatus__, __x__) ((((~__irqstatus__) & irqrouter->irq_invert_mask[__x__]) & irqrouter->irq_mask[__x__]) | __irqstatus__)
>> +#define HANDLE_EN_AND_INV_MASKS(__irqstatus__, __y__) (HANDLE_INVERTED_LINES(__irqstatus__, __y__) & irqrouter->irq_mask[__y__])
> 
> What the hell does this unparseable macro mess? What's so wrong with a proper
> inline function?

Ok, I'll inline it.

> 
>> +
>> +		irq_status[i] = HANDLE_EN_AND_INV_MASKS(irq_status[i], i);
>> +		status = tango_dispatch_irqs(domain, desc, irq_status[i], i*32);
>> +		if (status & irq_status[i])
> 
> Can you pretty please explain how this can happen? It can't. Because you clear
> every single bit of status in tango_dispatch_irqs(). See above.

You are right that with the current code of tango_dispatch_irqs() it cannot happen,
but that's more of a typo. I wanted to have a way to print something from this
driver in the event of unhandled IRQs.

Just to understand something, is leaving some debug or checks somehow not recommended?

> 
> Please clean up that unholy mess of nonsensical checks and debug code.
> 

Ok.

>> +			DBGERR("%s: %d unhandled IRQs (as a mask) 0x%x\n",
>> +			       NODE_NAME(irq_domain_get_of_node(domain)),
>> +			       i,
>> +			       status & irq_status[i]);
>> +	}
>> +
>> +	/* SW irqs */
>> +	swirq_status &= irqrouter->swirq_mask;
>> +	status = tango_dispatch_irqs(domain, desc, swirq_status, 128);
> 
> 128 is the magic number pulled out of thin air or what?

I will replace that with irqrouter->input_count.

> 
>> +	if (status & swirq_status)
>> +		DBGERR("%s: Unhandled IRQs (as a mask) 0x%x\n",
>> +		       NODE_NAME(irq_domain_get_of_node(domain)),
>> +		       status & swirq_status);
> 
> See above.
> 
>> +
>> +	chained_irq_exit(host_chip, desc);
>> +}
>> +
>> +
>> +/**
>> + * tango_irqdomain_map - route a hwirq(in) to a hwirq(out).
>> + * NOTE: The hwirq(out) must have been already allocated and enabled on
>> + * the parent controller.
> 
> Please put notes AFTER the argument descriptions

The note refers to the title:
"tango_irqdomain_map - route a hwirq(in) to a hwirq(out)."

> 
>> + * @hwirq: HW IRQ of the device requesting an IRQ
>> + * if hwirq > inputs it is a SW IRQ
>> + * @virq: Linux IRQ (associated to the domain) to be given to the device
>> + * @domain: IRQ domain (from the domain, we get the irqrouter_output
>> + * in order to know to which output we need to route hwirq to)
> 
> And if you spend a few seconds to align this proper, then it becomes suddenly
> readable.
> 
> * @hwirq:	HW IRQ of the device requesting an IRQ
> *		if hwirq > inputs it is a SW IRQ
> * @virq:	Linux IRQ (associated to the domain) to be given to the device
> * @domain:	IRQ domain (from the domain, we get the irqrouter_output
> *		in order to know to which output we need to route hwirq to)
> 
> Hmm?

Ok.

> 
>> + */
>> +static int tango_irqdomain_map(struct irq_domain *domain,
>> +			       unsigned int virq,
>> +			       irq_hw_number_t hwirq)
>> +{
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> +
>> +	DBGLOG("%s:%s(0x%p): hwirq(in) %d := virq %d, and route "
>> +	       "hwirq(in) %d => hwirq(out) %d (virq %d)\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)),
>> +	       domain->name,
>> +	       domain,
>> +	       (u32)hwirq,
>> +	       virq,
>> +	       (u32)hwirq,
>> +	       irqrouter_output->hwirq,
>> +	       irqrouter_output->virq);
>> +
>> +	if (hwirq >= (irqrouter->input_count + irqrouter->swirq_count))
>> +		DBGERR("%s: Invalid hwirq(in) %d >= %d + %d\n",
>> +		       NODE_NAME(irq_domain_get_of_node(domain)),
>> +		       (u32)hwirq,
>> +		       irqrouter->input_count,
>> +		       irqrouter->swirq_count);
>> +	else if (hwirq >= irqrouter->input_count)
>> +		DBGLOG("Map swirq %ld\n", hwirq - irqrouter->input_count);
>> +
>> +	irq_set_chip_and_handler(virq,
>> +				 &tango_irq_chip_shared_ops,
>> +				 handle_level_irq);
>> +	irq_set_chip_data(virq, domain);
>> +	irq_set_probe(virq);
>> +
>> +	tango_set_irq_route(irqrouter, hwirq, irqrouter_output->hwirq);
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +/**
>> + * tango_irqdomain_translate - used to select the domain for a given
>> + * irq_fwspec
> 
> It hardly selects the domain. @domain is handed in as a argument. And please
> get rid of that silly 'used to'. That just adds characters and no information.
> 
>     - Select the router section for a firmware spec
> 
> Hmm?

Ok, maybe I did not express it correctly.
The callback is called from irqdomain.c:irq_find_matching_fwspec() and each
domain is tested, if the callback returns non-zero, the current domain is considered
a "match" for the fwspec and returned by irq_find_matching_fwspec().

> 
>> + * @domain: a registered domain
>> + * @fwspec: an IRQ specification. This callback is used to translate the
>> + * parameters given as irq_fwspec into a HW IRQ and Type values.
> 
> fwspec is a callback?

No, the sentence refers to the callback below (tango_irqdomain_translate) that is
being described by this comment.
I can reformulate or remove the comment.

> 
>> + * @out_hwirq: 
>> + * @out_type:
> 
> Missing docs.....

Ok.

> 
>> + */
>> +static int tango_irqdomain_translate(struct irq_domain *domain,
>> +				     struct irq_fwspec *fwspec,
>> +				     unsigned long *out_hwirq,
>> +				     unsigned int *out_type)
>> +{
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> +	irq_hw_number_t irq;
>> +	u32 domain_id, type;
>> +	int err;
>> +
>> +	DBGLOG("%s:%s(0x%p): argc %d for hwirq(out) %d\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)),
>> +	       domain->name,
>> +	       domain,
>> +	       fwspec->param_count,
>> +	       irqrouter_output->hwirq);
>> +
>> +	err = tango_parse_fwspec(domain,
>> +				 fwspec,
>> +				 &domain_id,
>> +				 &irq,
>> +				 &type);
> 
> Please use the full 80 characters. Your patch does not become more valuable
> when you inflate the line count. It's more valuable when it is easy to read
> and understand.

Ok.

As a side note, does the 80 characters limit still makes sense? I mean, I would
imagine that anybody now is using wide screens capable of at least double that
amount.

> 
>> +	if (err < 0) {
>> +		DBGWARN("Failed to parse fwspec\n");
>> +		return err;
>> +	}
>> +
>> +	switch (domain_id) {
>> +	case SIGMA_HWIRQ:
>> +		DBGLOG("Request is for SIGMA_HWIRQ\n");
>> +		break;
>> +	case SIGMA_SWIRQ:
>> +		DBGLOG("Request is for SIGMA_SWIRQ\n");
>> +		irq += irqrouter->input_count;
>> +		break;
>> +	default:
>> +		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
>> +		       domain_id,
>> +		       irqrouter_output->domain_id);
> 
> Huch what? You only ever handle HW and SW irqs and now you happily proceed if
> the id is something else. Consistent error handling is optional and can be
> replaced by random debug prints, right?

You are right that this case should never happen as the 'select' callback must
have replied correctly.
This is more of a relic when I did not had access to the 'select' callback.
(I was working on 4.4)

> 
>> +		break;
>> +	};
>> +
>> +	*out_hwirq = irq;
>> +	*out_type  = type & IRQ_TYPE_SENSE_MASK;
>> +
>> +	DBGLOG("hwirq %d type 0x%x\n", (u32)*out_hwirq, (u32)*out_type);
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +/**
>> + * tango_irqdomain_select - used to select the domain for a given irq_fwspec
>> + * @domain: a registered domain
>> + * @fwspec: an IRQ specification. This callback should return zero if the
>> + * irq_fwspec does not belong to the given domain. If it does, it should
>> + * return non-zero.
> 
> Callback????

Ok.

> 
>> + * In practice it will return non-zero if the irq_fwspec matches one of the
>> + * IRQs shared within the given domain.
>> + * @bus_token: a bus token
>> + */
>> +static int tango_irqdomain_select(struct irq_domain *domain,
>> +				  struct irq_fwspec *fwspec,
>> +				  enum irq_domain_bus_token bus_token)
>> +{
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> +	irq_hw_number_t irq;
>> +	u32 domain_id, type;
>> +	int err;
>> +
>> +	DBGLOG("%s:%s(0x%p): argc %d, 0x%p, bus 0x%x\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
>> +	       fwspec->param_count, fwspec->fwnode, bus_token);
>> +
>> +	DBGLOG("router 0x%p, output 0x%p\n", irqrouter, irqrouter_output);
>> +
>> +	err = tango_parse_fwspec(domain, fwspec, &domain_id, &irq, &type);
>> +	if (err < 0)
>> +		return 0;
>> +
>> +	switch (domain_id) {
>> +	case SIGMA_HWIRQ:
>> +		DBGLOG("Request is for SIGMA_HWIRQ\n");
>> +		break;
>> +	case SIGMA_SWIRQ:
>> +		DBGLOG("Request is for SIGMA_SWIRQ\n");
>> +		break;
>> +	default:
>> +		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
>> +		       domain_id,
>> +		       irqrouter_output->domain_id);
>> +		break;
>> +	};
>> +
>> +	if (!irqrouter->implicit_groups) {
>> +		int i;
>> +
>> +		/* Check if the requested IRQ belongs to those listed
>> +		 * to be sharing the output assigned to this domain
> 
> Your using of 'domain' is utterly confusing. Please use something like
> hwdomain or whatever which makes it clear that this is something else than the
> irq domain. I tripped over this several times now.

Ok.

> 
>> +		 */
>> +		if (irqrouter_output->shared_count <= 0) {
>> +			DBGLOG("Not shared IRQ line?\n");
>> +			return 0;
>> +		}
>> +
>> +		for (i = 0; i < irqrouter_output->shared_count; i++) {
>> +			if (irq == irqrouter_output->shared_irqs[i]) {
>> +				DBGLOG("Match: IRQ %lu\n", irq);
>> +				return 1;
>> +			}
>> +		}
>> +	} else {
>> +		/* Otherwise, check if the domain_id given matches
>> +		 * the one assigned to this output
>> +		 */
>> +		if (domain_id == irqrouter_output->domain_id) {
>> +			DBGLOG("Match: Domain ID %d\n", domain_id);
>> +			return 1;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +/* 'irqrouter' handling callbacks
>> + * Used for 'direct' IRQs, i.e.: IRQs that are directly routed to the GIC
>> + * This driver does not dispatch the IRQs, the GIC does.
>> + */
>> +
>> +
>> +static void tango_irqrouter_mask_irq(struct irq_data *data)
>> +{
>> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> +	struct tango_irqrouter *irqrouter = domain->host_data;
>> +	int hwirq_in = (int)data->hwirq;
>> +
>> +	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)),
>> +	       domain->name,
>> +	       domain,
>> +	       hwirq_in);
>> +
>> +	tango_set_irq_enable(irqrouter, hwirq_in, 0);
>> +
>> +	irq_chip_mask_parent(data);
>> +}
>> +
>> +
>> +static void tango_irqrouter_unmask_irq(struct irq_data *data)
>> +{
>> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> +	struct tango_irqrouter *irqrouter = domain->host_data;
>> +	int hwirq_in = (int)data->hwirq;
>> +
>> +	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)),
>> +	       domain->name,
>> +	       domain,
>> +	       hwirq_in);
>> +
>> +	tango_set_irq_enable(irqrouter, hwirq_in, 1);
>> +
>> +	irq_chip_unmask_parent(data);
>> +}
>> +
>> +
>> +static int tango_irqrouter_set_irq_type(struct irq_data *data,
>> +					unsigned int type)
>> +{
>> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> +	struct tango_irqrouter_output *irqrouter_output = NULL;
>> +	struct tango_irqrouter *irqrouter = domain->host_data;
>> +	int hwirq_in = (int)data->hwirq;
>> +	u32 parent_type;
>> +
>> +	DBGLOG("%s:%s(0x%p) type 0x%x for hwirq(in) %d\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)),
>> +	       domain->name,
>> +	       domain,
>> +	       type,
>> +	       hwirq_in);
>> +
>> +	tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
>> +	if (!irqrouter_output)
>> +		goto handle_parent;
> 
> So if there is no output, then you unconditionally set the type on the
> parent. If that's correct, this needs a big fat comment at least.

Ok.
This is related to question 2 of my RFC:

----
2) In order to get a virq mapping for the domains associated with the outputs
(the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
See irq-tango_v4.c:1400

/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
hwirq = index + irqrouter->input_count + irqrouter->swirq_count;

This feels a bit like a hack, so suggestions are welcome.
----

Indeed, this callback will be called once when requesting a "fake HW IRQ" so
that alloc() callback is called and we get a free output (a GIC input) so 
that we can later route IRQs to it.
And another time when an actual HW IRQ is requested, in which case the
tango_set_irq_type() needs to be called so that we can see if inversion needs
to be implemented.

Actually, if I don't deal with inversion and consider there will never be
mismatches between the level setup on the GIC input and that one of the
device requesting the IRQ to be routed to said GIC input, then I could just
use irq_chip_set_type_parent() as callback directly.

> 
>> +	parent_type = (irqrouter_output->hwirq_level & IRQ_TYPE_SENSE_MASK);
>> +	tango_set_irq_type(irqrouter, hwirq_in, type, parent_type);
>> +
>> +handle_parent:
>> +	return irq_chip_set_type_parent(data, type);
>> +}
> 
> I'm stopping here. You can apply the above to the rest of the code as well.
> 
> 

Thanks again for your review.
Best regards,

Sebastian