[PATCH v5 0/6] genirq/gpio: Add driver for ThunderX and OCTEON-TX SoCs

[PATCH v5 0/6] genirq/gpio: Add driver for ThunderX and OCTEON-TX SoCs

[David Daney]

The ThunderX/OCTEON-TX GPIO hardware looks like a PCIe device, with
the interrupt signal from each GPIO line being routed to a dedicated
MSI-X.  This interrupt routing requires that we add some custom
processing to the beginning of the MSI-X irqdomain hierarchy.

Changes from v4:

 - Rebased to post-v4.10 to support .set_config() function.

 - Added .get_direction() support.

 - Added PIN_CONFIG_INPUT_DEBOUNCE support.

 - Removed some improper use of ENOSYS.

Changes from v3:

 - Add some "depends on" to the driver Kconfig to avoid build errors
   in some architectures when doing COMPILE_TEST builds.

Changes from v2:

 - in 4/6: Added Rob Harring's Acked-by

 - Added three patches to genirq/irqdomain to support interrupt code
   in the driver.

 - Rewrite irq code in driver to use irqdomain hierarchy.

 - Other naming and style changes as recommended by Linus Walleij.

Changes from v1:

 - in 1/3: Addressed Rob Harring's comments.

 - in 2/3: Trivial cleanups found in internal review + add some
   comments.

David Daney (6):
  genirq: Export more irq_chip_*_parent() functions.
  genirq: Add handle_fasteoi_{level,edge}_irq flow handlers.
  irqdomain: Add irq_domain_{push,pop}_irq() functions.
  dt-bindings: gpio: Add binding documentation for gpio-thunderx
  gpio: Add gpio driver support for ThunderX and OCTEON-TX
  MAINTAINERS: Add entry for THUNDERX GPIO Driver.

 .../devicetree/bindings/gpio/gpio-thunderx.txt     |  27 +
 MAINTAINERS                                        |   5 +
 drivers/gpio/Kconfig                               |   8 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-thunderx.c                       | 572 +++++++++++++++++++++
 include/linux/irq.h                                |   2 +
 include/linux/irqdomain.h                          |   3 +
 kernel/irq/chip.c                                  | 105 ++++
 kernel/irq/irqdomain.c                             | 137 +++++
 9 files changed, 860 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
 create mode 100644 drivers/gpio/gpio-thunderx.c

-- 
1.8.3.1

[PATCH v5 1/6] genirq: Export more irq_chip_*_parent() functions.

[David Daney]

Many of the family of functions including irq_chip_mask_parent(),
irq_chip_unmask_parent() are exported, but not all.

Add EXPORT_SYMBOL_GPL to irq_chip_enable_parent,
irq_chip_disable_parent and irq_chip_set_affinity_parent, so they
likewise are usable from modules.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 kernel/irq/chip.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index be3c34e..73ea90b 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -993,6 +993,7 @@ void irq_chip_enable_parent(struct irq_data *data)
 	else
 		data->chip->irq_unmask(data);
 }
+EXPORT_SYMBOL_GPL(irq_chip_enable_parent);
 
 /**
  * irq_chip_disable_parent - Disable the parent interrupt (defaults to mask if
@@ -1007,6 +1008,7 @@ void irq_chip_disable_parent(struct irq_data *data)
 	else
 		data->chip->irq_mask(data);
 }
+EXPORT_SYMBOL_GPL(irq_chip_disable_parent);
 
 /**
  * irq_chip_ack_parent - Acknowledge the parent interrupt
@@ -1069,6 +1071,7 @@ int irq_chip_set_affinity_parent(struct irq_data *data,
 
 	return -ENOSYS;
 }
+EXPORT_SYMBOL_GPL(irq_chip_set_affinity_parent);
 
 /**
  * irq_chip_set_type_parent - Set IRQ type on the parent interrupt
-- 
1.8.3.1

[PATCH v5 2/6] genirq: Add handle_fasteoi_{level,edge}_irq flow handlers.

[David Daney]

Follow-on patch for gpio-thunderx uses a irqdomain hierarchy which
requires slightly different flow handlers, add them to chip.c which
contains most of the other flow handlers.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 include/linux/irq.h |   2 ++
 kernel/irq/chip.c   | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index f887351..3db0eb8 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -518,6 +518,8 @@ static inline int irq_set_parent(int irq, int parent_irq)
 extern int irq_chip_pm_get(struct irq_data *data);
 extern int irq_chip_pm_put(struct irq_data *data);
 #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+extern void handle_fasteoi_edge_irq(struct irq_desc *desc);
+extern void handle_fasteoi_level_irq(struct irq_desc *desc);
 extern void irq_chip_enable_parent(struct irq_data *data);
 extern void irq_chip_disable_parent(struct irq_data *data);
 extern void irq_chip_ack_parent(struct irq_data *data);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 73ea90b..213105d 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -981,6 +981,108 @@ void irq_cpu_offline(void)
 
 #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
 /**
+ *	handle_fasteoi_edge_irq - irq handler for edge hierarchy
+ *	stacked on transparent controllers
+ *
+ *	@desc:	the interrupt description structure for this irq
+ *
+ *	Like handle_fasteoi_irq(), but for use with hierarchy where
+ *	the irq_chip also needs to have its ->irq_ack() function
+ *	called.
+ */
+void handle_fasteoi_edge_irq(struct irq_desc *desc)
+{
+	struct irq_chip *chip = desc->irq_data.chip;
+
+	raw_spin_lock(&desc->lock);
+
+	if (!irq_may_run(desc))
+		goto out;
+
+	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
+
+	/*
+	 * If its disabled or no action available
+	 * then mask it and get out of here:
+	 */
+	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
+		desc->istate |= IRQS_PENDING;
+		mask_irq(desc);
+		goto out;
+	}
+
+	kstat_incr_irqs_this_cpu(desc);
+	if (desc->istate & IRQS_ONESHOT)
+		mask_irq(desc);
+
+	/* Start handling the irq */
+	desc->irq_data.chip->irq_ack(&desc->irq_data);
+
+	preflow_handler(desc);
+	handle_irq_event(desc);
+
+	cond_unmask_eoi_irq(desc, chip);
+
+	raw_spin_unlock(&desc->lock);
+	return;
+out:
+	if (!(chip->flags & IRQCHIP_EOI_IF_HANDLED))
+		chip->irq_eoi(&desc->irq_data);
+	raw_spin_unlock(&desc->lock);
+}
+EXPORT_SYMBOL_GPL(handle_fasteoi_edge_irq);
+
+/**
+ *	handle_fasteoi_level_irq - irq handler for level hierarchy
+ *	stacked on transparent controllers
+ *
+ *	@desc:	the interrupt description structure for this irq
+ *
+ *	Like handle_fasteoi_irq(), but for use with hierarchy where
+ *	the irq_chip also needs to have its ->irq_mask_ack() function
+ *	called.
+ */
+void handle_fasteoi_level_irq(struct irq_desc *desc)
+{
+	struct irq_chip *chip = desc->irq_data.chip;
+
+	raw_spin_lock(&desc->lock);
+	mask_ack_irq(desc);
+
+	if (!irq_may_run(desc))
+		goto out;
+
+	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
+
+	/*
+	 * If its disabled or no action available
+	 * then mask it and get out of here:
+	 */
+	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
+		desc->istate |= IRQS_PENDING;
+		mask_irq(desc);
+		goto out;
+	}
+
+	kstat_incr_irqs_this_cpu(desc);
+	if (desc->istate & IRQS_ONESHOT)
+		mask_irq(desc);
+
+	preflow_handler(desc);
+	handle_irq_event(desc);
+
+	cond_unmask_eoi_irq(desc, chip);
+
+	raw_spin_unlock(&desc->lock);
+	return;
+out:
+	if (!(chip->flags & IRQCHIP_EOI_IF_HANDLED))
+		chip->irq_eoi(&desc->irq_data);
+	raw_spin_unlock(&desc->lock);
+}
+EXPORT_SYMBOL_GPL(handle_fasteoi_level_irq);
+
+/**
  * irq_chip_enable_parent - Enable the parent interrupt (defaults to unmask if
  * NULL)
  * @data:	Pointer to interrupt specific data
-- 
1.8.3.1

Re: [PATCH v5 2/6] genirq: Add handle_fasteoi_{level,edge}_irq flow handlers.

[Marc Zyngier]

On 01/03/17 01:48, David Daney wrote:
> Follow-on patch for gpio-thunderx uses a irqdomain hierarchy which
> requires slightly different flow handlers, add them to chip.c which
> contains most of the other flow handlers.
> 
> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
>  include/linux/irq.h |   2 ++
>  kernel/irq/chip.c   | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index f887351..3db0eb8 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -518,6 +518,8 @@ static inline int irq_set_parent(int irq, int parent_irq)
>  extern int irq_chip_pm_get(struct irq_data *data);
>  extern int irq_chip_pm_put(struct irq_data *data);
>  #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
> +extern void handle_fasteoi_edge_irq(struct irq_desc *desc);
> +extern void handle_fasteoi_level_irq(struct irq_desc *desc);
>  extern void irq_chip_enable_parent(struct irq_data *data);
>  extern void irq_chip_disable_parent(struct irq_data *data);
>  extern void irq_chip_ack_parent(struct irq_data *data);
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 73ea90b..213105d 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -981,6 +981,108 @@ void irq_cpu_offline(void)
>  
>  #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>  /**
> + *	handle_fasteoi_edge_irq - irq handler for edge hierarchy
> + *	stacked on transparent controllers
> + *
> + *	@desc:	the interrupt description structure for this irq
> + *
> + *	Like handle_fasteoi_irq(), but for use with hierarchy where
> + *	the irq_chip also needs to have its ->irq_ack() function
> + *	called.
> + */
> +void handle_fasteoi_edge_irq(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = desc->irq_data.chip;
> +
> +	raw_spin_lock(&desc->lock);
> +
> +	if (!irq_may_run(desc))
> +		goto out;
> +
> +	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
> +
> +	/*
> +	 * If its disabled or no action available
> +	 * then mask it and get out of here:
> +	 */
> +	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
> +		desc->istate |= IRQS_PENDING;
> +		mask_irq(desc);
> +		goto out;
> +	}
> +
> +	kstat_incr_irqs_this_cpu(desc);
> +	if (desc->istate & IRQS_ONESHOT)
> +		mask_irq(desc);
> +
> +	/* Start handling the irq */
> +	desc->irq_data.chip->irq_ack(&desc->irq_data);
> +
> +	preflow_handler(desc);
> +	handle_irq_event(desc);
> +
> +	cond_unmask_eoi_irq(desc, chip);
> +
> +	raw_spin_unlock(&desc->lock);
> +	return;
> +out:
> +	if (!(chip->flags & IRQCHIP_EOI_IF_HANDLED))
> +		chip->irq_eoi(&desc->irq_data);
> +	raw_spin_unlock(&desc->lock);
> +}
> +EXPORT_SYMBOL_GPL(handle_fasteoi_edge_irq);

So this is handle_fasteoi_irq with an irq_ack() added in the middle. In
the spirit of making this a bit more maintainable, how about making the
handle_fasteoi_irq code reusable (if necessary by forcing the compiler
to inline stuff)?

But the one thing that makes me uncomfortable here is that we're seem to
have this irq_ack() propagated all along the irqdata chain, which is not
what's happening. Only the EOI gets propagated.

Why can't you just put the irq_ack call in your top level irq_eoi
callback? That'd make it similar to what is happening on the mbigen side
(not exactly surprising, since they are doing very similar things).

Same remark about handle_fasteoi_level_irq.

Thoughts?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/6] genirq: Add handle_fasteoi_{level,edge}_irq flow handlers.

[Marc Zyngier]

On 01/03/17 01:48, David Daney wrote:
> Follow-on patch for gpio-thunderx uses a irqdomain hierarchy which
> requires slightly different flow handlers, add them to chip.c which
> contains most of the other flow handlers.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  include/linux/irq.h |   2 ++
>  kernel/irq/chip.c   | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index f887351..3db0eb8 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -518,6 +518,8 @@ static inline int irq_set_parent(int irq, int parent_irq)
>  extern int irq_chip_pm_get(struct irq_data *data);
>  extern int irq_chip_pm_put(struct irq_data *data);
>  #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
> +extern void handle_fasteoi_edge_irq(struct irq_desc *desc);
> +extern void handle_fasteoi_level_irq(struct irq_desc *desc);
>  extern void irq_chip_enable_parent(struct irq_data *data);
>  extern void irq_chip_disable_parent(struct irq_data *data);
>  extern void irq_chip_ack_parent(struct irq_data *data);
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 73ea90b..213105d 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -981,6 +981,108 @@ void irq_cpu_offline(void)
>  
>  #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>  /**
> + *	handle_fasteoi_edge_irq - irq handler for edge hierarchy
> + *	stacked on transparent controllers
> + *
> + *	@desc:	the interrupt description structure for this irq
> + *
> + *	Like handle_fasteoi_irq(), but for use with hierarchy where
> + *	the irq_chip also needs to have its ->irq_ack() function
> + *	called.
> + */
> +void handle_fasteoi_edge_irq(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = desc->irq_data.chip;
> +
> +	raw_spin_lock(&desc->lock);
> +
> +	if (!irq_may_run(desc))
> +		goto out;
> +
> +	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
> +
> +	/*
> +	 * If its disabled or no action available
> +	 * then mask it and get out of here:
> +	 */
> +	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
> +		desc->istate |= IRQS_PENDING;
> +		mask_irq(desc);
> +		goto out;
> +	}
> +
> +	kstat_incr_irqs_this_cpu(desc);
> +	if (desc->istate & IRQS_ONESHOT)
> +		mask_irq(desc);
> +
> +	/* Start handling the irq */
> +	desc->irq_data.chip->irq_ack(&desc->irq_data);
> +
> +	preflow_handler(desc);
> +	handle_irq_event(desc);
> +
> +	cond_unmask_eoi_irq(desc, chip);
> +
> +	raw_spin_unlock(&desc->lock);
> +	return;
> +out:
> +	if (!(chip->flags & IRQCHIP_EOI_IF_HANDLED))
> +		chip->irq_eoi(&desc->irq_data);
> +	raw_spin_unlock(&desc->lock);
> +}
> +EXPORT_SYMBOL_GPL(handle_fasteoi_edge_irq);

So this is handle_fasteoi_irq with an irq_ack() added in the middle. In
the spirit of making this a bit more maintainable, how about making the
handle_fasteoi_irq code reusable (if necessary by forcing the compiler
to inline stuff)?

But the one thing that makes me uncomfortable here is that we're seem to
have this irq_ack() propagated all along the irqdata chain, which is not
what's happening. Only the EOI gets propagated.

Why can't you just put the irq_ack call in your top level irq_eoi
callback? That'd make it similar to what is happening on the mbigen side
(not exactly surprising, since they are doing very similar things).

Same remark about handle_fasteoi_level_irq.

Thoughts?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 2/6] genirq: Add handle_fasteoi_{level,edge}_irq flow handlers.

[David Daney]

I am finally getting back to this, sorry for the delay ...

On 03/14/2017 09:54 AM, Marc Zyngier wrote:
> On 01/03/17 01:48, David Daney wrote:
>> Follow-on patch for gpio-thunderx uses a irqdomain hierarchy which
>> requires slightly different flow handlers, add them to chip.c which
>> contains most of the other flow handlers.
>>
>> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>> ---
>>  include/linux/irq.h |   2 ++
>>  kernel/irq/chip.c   | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 104 insertions(+)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index f887351..3db0eb8 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -518,6 +518,8 @@ static inline int irq_set_parent(int irq, int parent_irq)
>>  extern int irq_chip_pm_get(struct irq_data *data);
>>  extern int irq_chip_pm_put(struct irq_data *data);
>>  #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>> +extern void handle_fasteoi_edge_irq(struct irq_desc *desc);
>> +extern void handle_fasteoi_level_irq(struct irq_desc *desc);
>>  extern void irq_chip_enable_parent(struct irq_data *data);
>>  extern void irq_chip_disable_parent(struct irq_data *data);
>>  extern void irq_chip_ack_parent(struct irq_data *data);
>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index 73ea90b..213105d 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -981,6 +981,108 @@ void irq_cpu_offline(void)
>>
>>  #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>>  /**
>> + *	handle_fasteoi_edge_irq - irq handler for edge hierarchy
>> + *	stacked on transparent controllers
>> + *
>> + *	@desc:	the interrupt description structure for this irq
>> + *
>> + *	Like handle_fasteoi_irq(), but for use with hierarchy where
>> + *	the irq_chip also needs to have its ->irq_ack() function
>> + *	called.
>> + */
>> +void handle_fasteoi_edge_irq(struct irq_desc *desc)
>> +{
>> +	struct irq_chip *chip = desc->irq_data.chip;
>> +
>> +	raw_spin_lock(&desc->lock);
>> +
>> +	if (!irq_may_run(desc))
>> +		goto out;
>> +
>> +	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
>> +
>> +	/*
>> +	 * If its disabled or no action available
>> +	 * then mask it and get out of here:
>> +	 */
>> +	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
>> +		desc->istate |= IRQS_PENDING;
>> +		mask_irq(desc);
>> +		goto out;
>> +	}
>> +
>> +	kstat_incr_irqs_this_cpu(desc);
>> +	if (desc->istate & IRQS_ONESHOT)
>> +		mask_irq(desc);
>> +
>> +	/* Start handling the irq */
>> +	desc->irq_data.chip->irq_ack(&desc->irq_data);
>> +
>> +	preflow_handler(desc);
>> +	handle_irq_event(desc);
>> +
>> +	cond_unmask_eoi_irq(desc, chip);
>> +
>> +	raw_spin_unlock(&desc->lock);
>> +	return;
>> +out:
>> +	if (!(chip->flags & IRQCHIP_EOI_IF_HANDLED))
>> +		chip->irq_eoi(&desc->irq_data);
>> +	raw_spin_unlock(&desc->lock);
>> +}
>> +EXPORT_SYMBOL_GPL(handle_fasteoi_edge_irq);
>
> So this is handle_fasteoi_irq with an irq_ack() added in the middle. In
> the spirit of making this a bit more maintainable, how about making the
> handle_fasteoi_irq code reusable (if necessary by forcing the compiler
> to inline stuff)?

You, yourself, said in response to an earlier version of the patch set:


     So you want to generalize CONFIG_IRQ_PREFLOW_FASTEOI so that it is
     on each level of a domain stack? Humm. I personally think that
     this is a massive bloat that is going to impact all the hot paths
     for no gain whatsoever, but I'll let tglx speak his mind on that.

Your current suggestion is, from the point of view of the "impact", 
almost identical to what I was proposing back then.

>
> But the one thing that makes me uncomfortable here is that we're seem to
> have this irq_ack() propagated all along the irqdata chain, which is not
> what's happening. Only the EOI gets propagated.

That is intentional.  The parent domain is handle_fasteoi_irq(), so we 
know that it has no irq_ack().


>
> Why can't you just put the irq_ack call in your top level irq_eoi
> callback? That'd make it similar to what is happening on the mbigen side
> (not exactly surprising, since they are doing very similar things).

irq_ack() must be called before handle_irq_event(), otherwise there is a 
race condition where edge interrupts could be lost.


>
> Same remark about handle_fasteoi_level_irq.
>
> Thoughts?

Several:

1) Adding the additional mask_ack_irq()/irq_ack() to 
handle_fasteoi_irq() is probably the cleanest solution, however ...

2) There is a risk of breaking systems that inadvertently supply the new 
chip methods, but the methods don't currently get called by 
handle_fasteoi_irq().

3) Checking for the presence of mask_ack_irq()/irq_ack() methods may 
introduce additional branch mispredictions and cache misses on all 
non-ThunderGPIO systems.


David Daney
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/6] genirq: Add handle_fasteoi_{level,edge}_irq flow handlers.

[David Daney]

I am finally getting back to this, sorry for the delay ...

On 03/14/2017 09:54 AM, Marc Zyngier wrote:
> On 01/03/17 01:48, David Daney wrote:
>> Follow-on patch for gpio-thunderx uses a irqdomain hierarchy which
>> requires slightly different flow handlers, add them to chip.c which
>> contains most of the other flow handlers.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>  include/linux/irq.h |   2 ++
>>  kernel/irq/chip.c   | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 104 insertions(+)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index f887351..3db0eb8 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -518,6 +518,8 @@ static inline int irq_set_parent(int irq, int parent_irq)
>>  extern int irq_chip_pm_get(struct irq_data *data);
>>  extern int irq_chip_pm_put(struct irq_data *data);
>>  #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>> +extern void handle_fasteoi_edge_irq(struct irq_desc *desc);
>> +extern void handle_fasteoi_level_irq(struct irq_desc *desc);
>>  extern void irq_chip_enable_parent(struct irq_data *data);
>>  extern void irq_chip_disable_parent(struct irq_data *data);
>>  extern void irq_chip_ack_parent(struct irq_data *data);
>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index 73ea90b..213105d 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -981,6 +981,108 @@ void irq_cpu_offline(void)
>>
>>  #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>>  /**
>> + *	handle_fasteoi_edge_irq - irq handler for edge hierarchy
>> + *	stacked on transparent controllers
>> + *
>> + *	@desc:	the interrupt description structure for this irq
>> + *
>> + *	Like handle_fasteoi_irq(), but for use with hierarchy where
>> + *	the irq_chip also needs to have its ->irq_ack() function
>> + *	called.
>> + */
>> +void handle_fasteoi_edge_irq(struct irq_desc *desc)
>> +{
>> +	struct irq_chip *chip = desc->irq_data.chip;
>> +
>> +	raw_spin_lock(&desc->lock);
>> +
>> +	if (!irq_may_run(desc))
>> +		goto out;
>> +
>> +	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
>> +
>> +	/*
>> +	 * If its disabled or no action available
>> +	 * then mask it and get out of here:
>> +	 */
>> +	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
>> +		desc->istate |= IRQS_PENDING;
>> +		mask_irq(desc);
>> +		goto out;
>> +	}
>> +
>> +	kstat_incr_irqs_this_cpu(desc);
>> +	if (desc->istate & IRQS_ONESHOT)
>> +		mask_irq(desc);
>> +
>> +	/* Start handling the irq */
>> +	desc->irq_data.chip->irq_ack(&desc->irq_data);
>> +
>> +	preflow_handler(desc);
>> +	handle_irq_event(desc);
>> +
>> +	cond_unmask_eoi_irq(desc, chip);
>> +
>> +	raw_spin_unlock(&desc->lock);
>> +	return;
>> +out:
>> +	if (!(chip->flags & IRQCHIP_EOI_IF_HANDLED))
>> +		chip->irq_eoi(&desc->irq_data);
>> +	raw_spin_unlock(&desc->lock);
>> +}
>> +EXPORT_SYMBOL_GPL(handle_fasteoi_edge_irq);
>
> So this is handle_fasteoi_irq with an irq_ack() added in the middle. In
> the spirit of making this a bit more maintainable, how about making the
> handle_fasteoi_irq code reusable (if necessary by forcing the compiler
> to inline stuff)?

You, yourself, said in response to an earlier version of the patch set:


     So you want to generalize CONFIG_IRQ_PREFLOW_FASTEOI so that it is
     on each level of a domain stack? Humm. I personally think that
     this is a massive bloat that is going to impact all the hot paths
     for no gain whatsoever, but I'll let tglx speak his mind on that.

Your current suggestion is, from the point of view of the "impact", 
almost identical to what I was proposing back then.

>
> But the one thing that makes me uncomfortable here is that we're seem to
> have this irq_ack() propagated all along the irqdata chain, which is not
> what's happening. Only the EOI gets propagated.

That is intentional.  The parent domain is handle_fasteoi_irq(), so we 
know that it has no irq_ack().


>
> Why can't you just put the irq_ack call in your top level irq_eoi
> callback? That'd make it similar to what is happening on the mbigen side
> (not exactly surprising, since they are doing very similar things).

irq_ack() must be called before handle_irq_event(), otherwise there is a 
race condition where edge interrupts could be lost.


>
> Same remark about handle_fasteoi_level_irq.
>
> Thoughts?

Several:

1) Adding the additional mask_ack_irq()/irq_ack() to 
handle_fasteoi_irq() is probably the cleanest solution, however ...

2) There is a risk of breaking systems that inadvertently supply the new 
chip methods, but the methods don't currently get called by 
handle_fasteoi_irq().

3) Checking for the presence of mask_ack_irq()/irq_ack() methods may 
introduce additional branch mispredictions and cache misses on all 
non-ThunderGPIO systems.


David Daney

[PATCH v5 3/6] irqdomain: Add irq_domain_{push,pop}_irq() functions.

[David Daney]

For an already existing irqdomain hierarchy, as might be obtained via
a call to pci_enable_msix(), a PCI driver wishing to add an additional
irqdomain to the hierarchy needs to be able to insert the irqdomain to
that already initialized hierarchy.  Calling
irq_domain_create_hierarchy() allows the new irqdomain to be created,
but no existing code allows for initializing the associated irq_data.

Add a couple of helper functions (irq_domain_push_irq()
irq_domain_pop_irq()) to initialize the irq_data for the new
irqdomain.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 include/linux/irqdomain.h |   3 +
 kernel/irq/irqdomain.c    | 137 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 140 insertions(+)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 188eced..a7a16b7 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -425,6 +425,9 @@ extern void irq_domain_free_irqs_common(struct irq_domain *domain,
 extern void irq_domain_free_irqs_top(struct irq_domain *domain,
 				     unsigned int virq, unsigned int nr_irqs);
 
+extern int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg);
+extern int irq_domain_pop_irq(struct irq_domain *domain, int virq);
+
 extern int irq_domain_alloc_irqs_parent(struct irq_domain *domain,
 					unsigned int irq_base,
 					unsigned int nr_irqs, void *arg);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 31805f2..d5d1c01 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1304,6 +1304,143 @@ int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
 	return ret;
 }
 
+/* The irq_data was moved, fix the revmap to refer to the new location */
+static void irq_domain_fix_revmap(struct irq_data *d)
+{
+	void **slot;
+
+	if (d->hwirq < d->domain->revmap_size)
+		return; /* Not using radix tree. */
+
+	/* Fix up the revmap. */
+	mutex_lock(&revmap_trees_mutex);
+	slot = radix_tree_lookup_slot(&d->domain->revmap_tree, d->hwirq);
+	if (slot)
+		radix_tree_replace_slot(&d->domain->revmap_tree, slot, d);
+	mutex_unlock(&revmap_trees_mutex);
+}
+
+/**
+ * irq_domain_push_irq() - Push a domain in to the top of a hierarchy.
+ * @domain:	Domain to push.
+ * @virq:	Irq to push the domain in to.
+ * @arg:	Passed to the irq_domain_ops alloc() function.
+ *
+ * For an already existing irqdomain hierarchy, as might be obtained
+ * via a call to pci_enable_msix(), add an additional domain to the
+ * head of the processing chain.
+ */
+int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
+{
+	struct irq_data *child_irq_data;
+	struct irq_data *root_irq_data = irq_get_irq_data(virq);
+
+	if (domain == NULL)
+		return -EINVAL;
+
+	if (WARN_ON(!domain->ops->alloc))
+		return -EINVAL;
+
+	if (!root_irq_data)
+		return -EINVAL;
+
+	child_irq_data = kzalloc_node(sizeof(*child_irq_data), GFP_KERNEL,
+				      irq_data_get_node(root_irq_data));
+	if (!child_irq_data)
+		return -ENOMEM;
+
+	mutex_lock(&irq_domain_mutex);
+
+	/* Copy the original irq_data. */
+	*child_irq_data = *root_irq_data;
+
+	irq_domain_fix_revmap(child_irq_data);
+
+	/*
+	 * Overwrite the root_irq_data, which is embedded in struct
+	 * irq_desc, with values for this domain.
+	 */
+	root_irq_data->parent_data = child_irq_data;
+	root_irq_data->domain = domain;
+	root_irq_data->mask = 0;
+	root_irq_data->hwirq = 0;
+	root_irq_data->chip = NULL;
+	root_irq_data->chip_data = NULL;
+	domain->ops->alloc(domain, virq, 1, arg);
+
+	if (root_irq_data->hwirq < domain->revmap_size) {
+		domain->linear_revmap[root_irq_data->hwirq] = virq;
+	} else {
+		mutex_lock(&revmap_trees_mutex);
+		radix_tree_insert(&domain->revmap_tree,
+				  root_irq_data->hwirq, root_irq_data);
+		mutex_unlock(&revmap_trees_mutex);
+	}
+
+	mutex_unlock(&irq_domain_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(irq_domain_push_irq);
+
+/**
+ * irq_domain_pop_irq() - Remove a domain from the top of a hierarchy.
+ * @domain:	Domain to remove.
+ * @virq:	Irq to remove the domain from.
+ *
+ * Undo the effects of a call to irq_domain_push_irq().
+ */
+int irq_domain_pop_irq(struct irq_domain *domain, int virq)
+{
+	struct irq_data *root_irq_data = irq_get_irq_data(virq);
+	struct irq_data *child_irq_data;
+	struct irq_data *tmp_irq_data;
+
+	if (domain == NULL)
+		return -EINVAL;
+
+	if (!root_irq_data)
+		return -EINVAL;
+
+	tmp_irq_data = irq_domain_get_irq_data(domain, virq);
+
+	/* We can only "pop" if this domain is at the top of the list */
+	if (WARN_ON(root_irq_data != tmp_irq_data))
+		return -EINVAL;
+
+	if (WARN_ON(root_irq_data->domain != domain))
+		return -EINVAL;
+
+	child_irq_data = root_irq_data->parent_data;
+	if (WARN_ON(!child_irq_data))
+		return -EINVAL;
+
+	mutex_lock(&irq_domain_mutex);
+
+	root_irq_data->parent_data = NULL;
+
+	if (root_irq_data->hwirq >= domain->revmap_size) {
+		mutex_lock(&revmap_trees_mutex);
+		radix_tree_delete(&domain->revmap_tree, root_irq_data->hwirq);
+		mutex_unlock(&revmap_trees_mutex);
+	}
+
+	if (domain->ops->free)
+		domain->ops->free(domain, virq, 1);
+
+	/* Restore the original irq_data. */
+	*root_irq_data = *child_irq_data;
+
+	irq_domain_fix_revmap(root_irq_data);
+
+	mutex_unlock(&irq_domain_mutex);
+
+	kfree(child_irq_data);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(irq_domain_pop_irq);
+
 /**
  * irq_domain_free_irqs - Free IRQ number and associated data structures
  * @virq:	base IRQ number
-- 
1.8.3.1

Re: [PATCH v5 3/6] irqdomain: Add irq_domain_{push,pop}_irq() functions.

[Marc Zyngier]

Hi David,

On 01/03/17 01:48, David Daney wrote:
> For an already existing irqdomain hierarchy, as might be obtained via
> a call to pci_enable_msix(), a PCI driver wishing to add an additional
> irqdomain to the hierarchy needs to be able to insert the irqdomain to
> that already initialized hierarchy.  Calling
> irq_domain_create_hierarchy() allows the new irqdomain to be created,
> but no existing code allows for initializing the associated irq_data.

I must say that I like this idea a lot. Pretty elegant. Now, there is a
couple of things that do worry me. And instead of worrying, maybe I
should just ask the questions.

> Add a couple of helper functions (irq_domain_push_irq()
> irq_domain_pop_irq()) to initialize the irq_data for the new
> irqdomain.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  include/linux/irqdomain.h |   3 +
>  kernel/irq/irqdomain.c    | 137 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 140 insertions(+)
> 
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 188eced..a7a16b7 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -425,6 +425,9 @@ extern void irq_domain_free_irqs_common(struct irq_domain *domain,
>  extern void irq_domain_free_irqs_top(struct irq_domain *domain,
>  				     unsigned int virq, unsigned int nr_irqs);
>  
> +extern int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg);
> +extern int irq_domain_pop_irq(struct irq_domain *domain, int virq);
> +
>  extern int irq_domain_alloc_irqs_parent(struct irq_domain *domain,
>  					unsigned int irq_base,
>  					unsigned int nr_irqs, void *arg);
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 31805f2..d5d1c01 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -1304,6 +1304,143 @@ int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
>  	return ret;
>  }
>  
> +/* The irq_data was moved, fix the revmap to refer to the new location */
> +static void irq_domain_fix_revmap(struct irq_data *d)
> +{
> +	void **slot;
> +
> +	if (d->hwirq < d->domain->revmap_size)
> +		return; /* Not using radix tree. */
> +
> +	/* Fix up the revmap. */
> +	mutex_lock(&revmap_trees_mutex);
> +	slot = radix_tree_lookup_slot(&d->domain->revmap_tree, d->hwirq);
> +	if (slot)
> +		radix_tree_replace_slot(&d->domain->revmap_tree, slot, d);
> +	mutex_unlock(&revmap_trees_mutex);
> +}
> +
> +/**
> + * irq_domain_push_irq() - Push a domain in to the top of a hierarchy.
> + * @domain:	Domain to push.
> + * @virq:	Irq to push the domain in to.
> + * @arg:	Passed to the irq_domain_ops alloc() function.
> + *
> + * For an already existing irqdomain hierarchy, as might be obtained
> + * via a call to pci_enable_msix(), add an additional domain to the
> + * head of the processing chain.
> + */
> +int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
> +{
> +	struct irq_data *child_irq_data;
> +	struct irq_data *root_irq_data = irq_get_irq_data(virq);
> +
> +	if (domain == NULL)
> +		return -EINVAL;
> +
> +	if (WARN_ON(!domain->ops->alloc))
> +		return -EINVAL;
> +
> +	if (!root_irq_data)
> +		return -EINVAL;
> +
> +	child_irq_data = kzalloc_node(sizeof(*child_irq_data), GFP_KERNEL,
> +				      irq_data_get_node(root_irq_data));
> +	if (!child_irq_data)
> +		return -ENOMEM;
> +
> +	mutex_lock(&irq_domain_mutex);
> +
> +	/* Copy the original irq_data. */
> +	*child_irq_data = *root_irq_data;
> +
> +	irq_domain_fix_revmap(child_irq_data);
> +
> +	/*
> +	 * Overwrite the root_irq_data, which is embedded in struct
> +	 * irq_desc, with values for this domain.
> +	 */
> +	root_irq_data->parent_data = child_irq_data;
> +	root_irq_data->domain = domain;
> +	root_irq_data->mask = 0;
> +	root_irq_data->hwirq = 0;
> +	root_irq_data->chip = NULL;
> +	root_irq_data->chip_data = NULL;

What guarantees do we have that nobody is using this irqdesc at this
point? Is it a "don't do that because it will hurt" kind of thing? I'd
be more confident if we had some locking here, just to make sure that we
don't start processing an interrupt with all these NULL pointers.

Also, maybe moving the whole stuff to a helper in irqdesc.c if that
makes it easier/nicer? Your call.

> +	domain->ops->alloc(domain, virq, 1, arg);

Check return value? You may have to revert your previous fixup if it fails.

> +
> +	if (root_irq_data->hwirq < domain->revmap_size) {
> +		domain->linear_revmap[root_irq_data->hwirq] = virq;
> +	} else {
> +		mutex_lock(&revmap_trees_mutex);
> +		radix_tree_insert(&domain->revmap_tree,
> +				  root_irq_data->hwirq, root_irq_data);
> +		mutex_unlock(&revmap_trees_mutex);
> +	}
> +
> +	mutex_unlock(&irq_domain_mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(irq_domain_push_irq);
> +
> +/**
> + * irq_domain_pop_irq() - Remove a domain from the top of a hierarchy.
> + * @domain:	Domain to remove.
> + * @virq:	Irq to remove the domain from.
> + *
> + * Undo the effects of a call to irq_domain_push_irq().
> + */
> +int irq_domain_pop_irq(struct irq_domain *domain, int virq)
> +{
> +	struct irq_data *root_irq_data = irq_get_irq_data(virq);
> +	struct irq_data *child_irq_data;
> +	struct irq_data *tmp_irq_data;
> +
> +	if (domain == NULL)
> +		return -EINVAL;
> +
> +	if (!root_irq_data)
> +		return -EINVAL;
> +
> +	tmp_irq_data = irq_domain_get_irq_data(domain, virq);
> +
> +	/* We can only "pop" if this domain is at the top of the list */
> +	if (WARN_ON(root_irq_data != tmp_irq_data))
> +		return -EINVAL;
> +
> +	if (WARN_ON(root_irq_data->domain != domain))
> +		return -EINVAL;
> +
> +	child_irq_data = root_irq_data->parent_data;
> +	if (WARN_ON(!child_irq_data))
> +		return -EINVAL;
> +
> +	mutex_lock(&irq_domain_mutex);
> +
> +	root_irq_data->parent_data = NULL;
> +
> +	if (root_irq_data->hwirq >= domain->revmap_size) {
> +		mutex_lock(&revmap_trees_mutex);
> +		radix_tree_delete(&domain->revmap_tree, root_irq_data->hwirq);
> +		mutex_unlock(&revmap_trees_mutex);
> +	}
> +
> +	if (domain->ops->free)
> +		domain->ops->free(domain, virq, 1);
> +
> +	/* Restore the original irq_data. */
> +	*root_irq_data = *child_irq_data;

Similar concerns about locking here.

> +
> +	irq_domain_fix_revmap(root_irq_data);
> +
> +	mutex_unlock(&irq_domain_mutex);
> +
> +	kfree(child_irq_data);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(irq_domain_pop_irq);
> +
>  /**
>   * irq_domain_free_irqs - Free IRQ number and associated data structures
>   * @virq:	base IRQ number
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 3/6] irqdomain: Add irq_domain_{push,pop}_irq() functions.

[David Daney]

On 03/14/2017 09:11 AM, Marc Zyngier wrote:
> Hi David,
>
> On 01/03/17 01:48, David Daney wrote:
>> For an already existing irqdomain hierarchy, as might be obtained via
>> a call to pci_enable_msix(), a PCI driver wishing to add an additional
>> irqdomain to the hierarchy needs to be able to insert the irqdomain to
>> that already initialized hierarchy.  Calling
>> irq_domain_create_hierarchy() allows the new irqdomain to be created,
>> but no existing code allows for initializing the associated irq_data.
>
> I must say that I like this idea a lot. Pretty elegant. Now, there is a
> couple of things that do worry me. And instead of worrying, maybe I
> should just ask the questions.
>
>> Add a couple of helper functions (irq_domain_push_irq()
>> irq_domain_pop_irq()) to initialize the irq_data for the new
>> irqdomain.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>  include/linux/irqdomain.h |   3 +
>>  kernel/irq/irqdomain.c    | 137 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 140 insertions(+)
>>
>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>> index 188eced..a7a16b7 100644
>> --- a/include/linux/irqdomain.h
>> +++ b/include/linux/irqdomain.h
>> @@ -425,6 +425,9 @@ extern void irq_domain_free_irqs_common(struct irq_domain *domain,
>>  extern void irq_domain_free_irqs_top(struct irq_domain *domain,
>>  				     unsigned int virq, unsigned int nr_irqs);
>>
>> +extern int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg);
>> +extern int irq_domain_pop_irq(struct irq_domain *domain, int virq);
>> +
>>  extern int irq_domain_alloc_irqs_parent(struct irq_domain *domain,
>>  					unsigned int irq_base,
>>  					unsigned int nr_irqs, void *arg);
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index 31805f2..d5d1c01 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -1304,6 +1304,143 @@ int __irq_domain_alloc_irqs(struct irq_domain *domain, int irq_base,
>>  	return ret;
>>  }
>>
>> +/* The irq_data was moved, fix the revmap to refer to the new location */
>> +static void irq_domain_fix_revmap(struct irq_data *d)
>> +{
>> +	void **slot;
>> +
>> +	if (d->hwirq < d->domain->revmap_size)
>> +		return; /* Not using radix tree. */
>> +
>> +	/* Fix up the revmap. */
>> +	mutex_lock(&revmap_trees_mutex);
>> +	slot = radix_tree_lookup_slot(&d->domain->revmap_tree, d->hwirq);
>> +	if (slot)
>> +		radix_tree_replace_slot(&d->domain->revmap_tree, slot, d);
>> +	mutex_unlock(&revmap_trees_mutex);
>> +}
>> +
>> +/**
>> + * irq_domain_push_irq() - Push a domain in to the top of a hierarchy.
>> + * @domain:	Domain to push.
>> + * @virq:	Irq to push the domain in to.
>> + * @arg:	Passed to the irq_domain_ops alloc() function.
>> + *
>> + * For an already existing irqdomain hierarchy, as might be obtained
>> + * via a call to pci_enable_msix(), add an additional domain to the
>> + * head of the processing chain.
>> + */
>> +int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
>> +{
>> +	struct irq_data *child_irq_data;
>> +	struct irq_data *root_irq_data = irq_get_irq_data(virq);
>> +
>> +	if (domain == NULL)
>> +		return -EINVAL;
>> +
>> +	if (WARN_ON(!domain->ops->alloc))
>> +		return -EINVAL;
>> +
>> +	if (!root_irq_data)
>> +		return -EINVAL;
>> +
>> +	child_irq_data = kzalloc_node(sizeof(*child_irq_data), GFP_KERNEL,
>> +				      irq_data_get_node(root_irq_data));
>> +	if (!child_irq_data)
>> +		return -ENOMEM;
>> +
>> +	mutex_lock(&irq_domain_mutex);
>> +
>> +	/* Copy the original irq_data. */
>> +	*child_irq_data = *root_irq_data;
>> +
>> +	irq_domain_fix_revmap(child_irq_data);
>> +
>> +	/*
>> +	 * Overwrite the root_irq_data, which is embedded in struct
>> +	 * irq_desc, with values for this domain.
>> +	 */
>> +	root_irq_data->parent_data = child_irq_data;
>> +	root_irq_data->domain = domain;
>> +	root_irq_data->mask = 0;
>> +	root_irq_data->hwirq = 0;
>> +	root_irq_data->chip = NULL;
>> +	root_irq_data->chip_data = NULL;
>
> What guarantees do we have that nobody is using this irqdesc at this
> point? Is it a "don't do that because it will hurt" kind of thing?

Yes.

> I'd be more confident if we had some locking here, just to make sure that we
> don't start processing an interrupt with all these NULL pointers.
>

The only time it makes sense to push/pop is when no request_irq() are 
active.  Perhaps checking (with proper locking) that there are no 
actions registered is the proper thing to do.

> Also, maybe moving the whole stuff to a helper in irqdesc.c if that
> makes it easier/nicer? Your call.
>
>> +	domain->ops->alloc(domain, virq, 1, arg);
>
> Check return value? You may have to revert your previous fixup if it fails.

OK.

>
>> +
>> +	if (root_irq_data->hwirq < domain->revmap_size) {
>> +		domain->linear_revmap[root_irq_data->hwirq] = virq;
>> +	} else {
>> +		mutex_lock(&revmap_trees_mutex);
>> +		radix_tree_insert(&domain->revmap_tree,
>> +				  root_irq_data->hwirq, root_irq_data);
>> +		mutex_unlock(&revmap_trees_mutex);
>> +	}
>> +
>> +	mutex_unlock(&irq_domain_mutex);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(irq_domain_push_irq);
>> +
>> +/**
>> + * irq_domain_pop_irq() - Remove a domain from the top of a hierarchy.
>> + * @domain:	Domain to remove.
>> + * @virq:	Irq to remove the domain from.
>> + *
>> + * Undo the effects of a call to irq_domain_push_irq().
>> + */
>> +int irq_domain_pop_irq(struct irq_domain *domain, int virq)
>> +{
>> +	struct irq_data *root_irq_data = irq_get_irq_data(virq);
>> +	struct irq_data *child_irq_data;
>> +	struct irq_data *tmp_irq_data;
>> +
>> +	if (domain == NULL)
>> +		return -EINVAL;
>> +
>> +	if (!root_irq_data)
>> +		return -EINVAL;
>> +
>> +	tmp_irq_data = irq_domain_get_irq_data(domain, virq);
>> +
>> +	/* We can only "pop" if this domain is at the top of the list */
>> +	if (WARN_ON(root_irq_data != tmp_irq_data))
>> +		return -EINVAL;
>> +
>> +	if (WARN_ON(root_irq_data->domain != domain))
>> +		return -EINVAL;
>> +
>> +	child_irq_data = root_irq_data->parent_data;
>> +	if (WARN_ON(!child_irq_data))
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&irq_domain_mutex);
>> +
>> +	root_irq_data->parent_data = NULL;
>> +
>> +	if (root_irq_data->hwirq >= domain->revmap_size) {
>> +		mutex_lock(&revmap_trees_mutex);
>> +		radix_tree_delete(&domain->revmap_tree, root_irq_data->hwirq);
>> +		mutex_unlock(&revmap_trees_mutex);
>> +	}
>> +
>> +	if (domain->ops->free)
>> +		domain->ops->free(domain, virq, 1);
>> +
>> +	/* Restore the original irq_data. */
>> +	*root_irq_data = *child_irq_data;
>
> Similar concerns about locking here.
>
>> +
>> +	irq_domain_fix_revmap(root_irq_data);
>> +
>> +	mutex_unlock(&irq_domain_mutex);
>> +
>> +	kfree(child_irq_data);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(irq_domain_pop_irq);
>> +
>>  /**
>>   * irq_domain_free_irqs - Free IRQ number and associated data structures
>>   * @virq:	base IRQ number
>>
>
> Thanks,
>
> 	M.
>

[PATCH v5 4/6] dt-bindings: gpio: Add binding documentation for gpio-thunderx

[David Daney]

Signed-off-by: David Daney <david.daney@cavium.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/gpio/gpio-thunderx.txt     | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-thunderx.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt b/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
new file mode 100644
index 0000000..3f883ae
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
@@ -0,0 +1,27 @@
+Cavium ThunderX/OCTEON-TX GPIO controller bindings
+
+Required Properties:
+- reg: The controller bus address.
+- gpio-controller: Marks the device node as a GPIO controller.
+- #gpio-cells: Must be 2.
+  - First cell is the GPIO pin number relative to the controller.
+  - Second cell is a standard generic flag bitfield as described in gpio.txt.
+
+Optional Properties:
+- compatible: "cavium,thunder-8890-gpio", unused as PCI driver binding is used.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells: Must be present and have value of 2 if
+                    "interrupt-controller" is present.
+  - First cell is the GPIO pin number relative to the controller.
+  - Second cell is triggering flags as defined in interrupts.txt.
+
+Example:
+
+gpio_6_0: gpio@6,0 {
+	compatible = "cavium,thunder-8890-gpio";
+	reg = <0x3000 0 0 0 0>; /*  DEVFN = 0x30 (6:0) */
+	gpio-controller;
+	#gpio-cells = <2>;
+	interrupt-controller;
+	#interrupt-cells = <2>;
+};
-- 
1.8.3.1

Re: [PATCH v5 4/6] dt-bindings: gpio: Add binding documentation for gpio-thunderx

[Linus Walleij]

On Wed, Mar 1, 2017 at 2:48 AM, David Daney <david.daney@cavium.com> wrote:

> Signed-off-by: David Daney <david.daney@cavium.com>
> Acked-by: Rob Herring <robh@kernel.org>

This patch applied to the GPIO tree, really simplistic so why not
merge it.

Yours,
Linus Walleij

Re: [PATCH v5 4/6] dt-bindings: gpio: Add binding documentation for gpio-thunderx

[David Daney]

On 03/14/2017 07:53 AM, Linus Walleij wrote:
> On Wed, Mar 1, 2017 at 2:48 AM, David Daney <david.daney@cavium.com> wrote:
>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>
> This patch applied to the GPIO tree, really simplistic so why not
> merge it.

I think the idea is that with Rob's Acked-by, it could be merged via the 
GPIO tree when and if the other patches in the set are merged.  Since I 
don't maintain any trees pulled by Linus Torvalds, I am at the mercy of 
the various maintainers.

David Daney.

Re: [PATCH v5 4/6] dt-bindings: gpio: Add binding documentation for gpio-thunderx

[Linus Walleij]

On Tue, Mar 14, 2017 at 5:45 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 03/14/2017 07:53 AM, Linus Walleij wrote:
>>
>> On Wed, Mar 1, 2017 at 2:48 AM, David Daney <david.daney@cavium.com>
>> wrote:
>>
>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>
>>
>> This patch applied to the GPIO tree, really simplistic so why not
>> merge it.
>
>
> I think the idea is that with Rob's Acked-by, it could be merged via the
> GPIO tree when and if the other patches in the set are merged.  Since I
> don't maintain any trees pulled by Linus Torvalds, I am at the mercy of the
> various maintainers.

I agree that the code changes need to go in together.

But DT bindings are sort of decoupled from the kernel i general
(they are theoretically also used by other OSes such as *BSD)
so they can be merged in an orthogonal manner once they are
considered finished.

No biggie for me though, if you prefer, I can pull it out.

Yours,
Linus Walleij

[PATCH v5 5/6] gpio: Add gpio driver support for ThunderX and OCTEON-TX

[David Daney]

Cavium ThunderX and OCTEON-TX are arm64 based SoCs.  Add driver for
the on-chip GPIO pins.

Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
---
 drivers/gpio/Kconfig         |   8 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-thunderx.c | 572 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 581 insertions(+)
 create mode 100644 drivers/gpio/gpio-thunderx.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0504307..9291750 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -439,6 +439,14 @@ config GPIO_TS4800
 	help
 	  This driver support TS-4800 FPGA GPIO controllers.
 
+config GPIO_THUNDERX
+	tristate "Cavium ThunderX/OCTEON-TX GPIO"
+	depends on ARCH_THUNDER || (64BIT && COMPILE_TEST)
+	depends on PCI_MSI && IRQ_DOMAIN_HIERARCHY
+	help
+	  Say yes here to support the on-chip GPIO lines on the ThunderX
+	  and OCTEON-TX families of SoCs.
+
 config GPIO_TZ1090
 	bool "Toumaz Xenif TZ1090 GPIO support"
 	depends on SOC_TZ1090
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index becb96c..8d8eb15 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -111,6 +111,7 @@ obj-$(CONFIG_GPIO_SYSCON)	+= gpio-syscon.o
 obj-$(CONFIG_GPIO_TB10X)	+= gpio-tb10x.o
 obj-$(CONFIG_GPIO_TC3589X)	+= gpio-tc3589x.o
 obj-$(CONFIG_GPIO_TEGRA)	+= gpio-tegra.o
+obj-$(CONFIG_GPIO_THUNDERX)	+= gpio-thunderx.o
 obj-$(CONFIG_GPIO_TIMBERDALE)	+= gpio-timberdale.o
 obj-$(CONFIG_GPIO_PALMAS)	+= gpio-palmas.o
 obj-$(CONFIG_GPIO_TPIC2810)	+= gpio-tpic2810.o
diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
new file mode 100644
index 0000000..0973cac
--- /dev/null
+++ b/drivers/gpio/gpio-thunderx.c
@@ -0,0 +1,572 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2016, 2017 Cavium Inc.
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/spinlock.h>
+
+
+#define GPIO_RX_DAT	0x0
+#define GPIO_TX_SET	0x8
+#define GPIO_TX_CLR	0x10
+#define GPIO_CONST	0x90
+#define  GPIO_CONST_GPIOS_MASK 0xff
+#define GPIO_BIT_CFG	0x400
+#define  GPIO_BIT_CFG_TX_OE		BIT(0)
+#define  GPIO_BIT_CFG_PIN_XOR		BIT(1)
+#define  GPIO_BIT_CFG_INT_EN		BIT(2)
+#define  GPIO_BIT_CFG_INT_TYPE		BIT(3)
+#define  GPIO_BIT_CFG_FIL_MASK		GENMASK(11, 4)
+#define  GPIO_BIT_CFG_FIL_CNT_SHIFT	4
+#define  GPIO_BIT_CFG_FIL_SEL_SHIFT	8
+#define  GPIO_BIT_CFG_TX_OD		BIT(12)
+#define  GPIO_BIT_CFG_PIN_SEL_MASK	GENMASK(25, 16)
+#define GPIO_INTR	0x800
+#define  GPIO_INTR_INTR			BIT(0)
+#define  GPIO_INTR_INTR_W1S		BIT(1)
+#define  GPIO_INTR_ENA_W1C		BIT(2)
+#define  GPIO_INTR_ENA_W1S		BIT(3)
+#define GPIO_2ND_BANK	0x1400
+
+#define GLITCH_FILTER_400NS ((4u << GPIO_BIT_CFG_FIL_SEL_SHIFT) | \
+			     (9u << GPIO_BIT_CFG_FIL_CNT_SHIFT))
+
+struct thunderx_gpio;
+
+struct thunderx_line {
+	struct thunderx_gpio	*txgpio;
+	unsigned int		line;
+	unsigned int		fil_bits;
+};
+
+struct thunderx_gpio {
+	struct gpio_chip	chip;
+	u8 __iomem		*register_base;
+	struct irq_domain	*irqd;
+	struct msix_entry	*msix_entries;	/* per line MSI-X */
+	struct thunderx_line	*line_entries;	/* per line irq info */
+	raw_spinlock_t		lock;
+	unsigned long		invert_mask[2];
+	unsigned long		od_mask[2];
+	int			base_msi;
+};
+
+static unsigned int bit_cfg_reg(unsigned int line)
+{
+	return 8 * line + GPIO_BIT_CFG;
+}
+
+static unsigned int intr_reg(unsigned int line)
+{
+	return 8 * line + GPIO_INTR;
+}
+
+/*
+ * Check (and WARN) that the pin is available for GPIO.  We will not
+ * allow modification of the state of non-GPIO pins from this driver.
+ */
+static bool thunderx_gpio_is_gpio(struct thunderx_gpio *txgpio,
+				  unsigned int line)
+{
+	u64 bit_cfg = readq(txgpio->register_base + bit_cfg_reg(line));
+	bool rv = (bit_cfg & GPIO_BIT_CFG_PIN_SEL_MASK) == 0;
+
+	WARN_RATELIMIT(!rv, "Pin %d not available for GPIO\n", line);
+
+	return rv;
+}
+
+static int thunderx_gpio_dir_in(struct gpio_chip *chip, unsigned int line)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+
+	if (!thunderx_gpio_is_gpio(txgpio, line))
+		return -EIO;
+
+	raw_spin_lock(&txgpio->lock);
+	clear_bit(line, txgpio->invert_mask);
+	clear_bit(line, txgpio->od_mask);
+	writeq(txgpio->line_entries[line].fil_bits,
+	       txgpio->register_base + bit_cfg_reg(line));
+	raw_spin_unlock(&txgpio->lock);
+	return 0;
+}
+
+static void thunderx_gpio_set(struct gpio_chip *chip, unsigned int line,
+			      int value)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+	int bank = line / 64;
+	int bank_bit = line % 64;
+
+	void __iomem *reg = txgpio->register_base +
+		(bank * GPIO_2ND_BANK) + (value ? GPIO_TX_SET : GPIO_TX_CLR);
+
+	writeq(BIT_ULL(bank_bit), reg);
+}
+
+static int thunderx_gpio_dir_out(struct gpio_chip *chip, unsigned int line,
+				 int value)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+	u64 bit_cfg = txgpio->line_entries[line].fil_bits | GPIO_BIT_CFG_TX_OE;
+
+	if (!thunderx_gpio_is_gpio(txgpio, line))
+		return -EIO;
+
+	raw_spin_lock(&txgpio->lock);
+
+	thunderx_gpio_set(chip, line, value);
+
+	if (test_bit(line, txgpio->invert_mask))
+		bit_cfg |= GPIO_BIT_CFG_PIN_XOR;
+
+	if (test_bit(line, txgpio->od_mask))
+		bit_cfg |= GPIO_BIT_CFG_TX_OD;
+
+	writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(line));
+
+	raw_spin_unlock(&txgpio->lock);
+	return 0;
+}
+
+static int thunderx_gpio_get_direction(struct gpio_chip *chip, unsigned int line)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+	u64 bit_cfg;
+
+	if (!thunderx_gpio_is_gpio(txgpio, line))
+		return -EIO;
+
+	bit_cfg = readq(txgpio->register_base + bit_cfg_reg(line));
+
+	return !(bit_cfg & GPIO_BIT_CFG_TX_OE);
+}
+
+static int thunderx_gpio_set_config(struct gpio_chip *chip,
+				    unsigned int line,
+				    unsigned long cfg)
+{
+	bool orig_invert, orig_od, orig_dat, new_invert, new_od;
+	u32 arg, sel;
+	u64 bit_cfg;
+	int bank = line / 64;
+	int bank_bit = line % 64;
+	int ret = -ENOTSUPP;
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+	void __iomem *reg = txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_SET;
+
+	if (!thunderx_gpio_is_gpio(txgpio, line))
+		return -EIO;
+
+	raw_spin_lock(&txgpio->lock);
+	orig_invert = test_bit(line, txgpio->invert_mask);
+	new_invert  = orig_invert;
+	orig_od = test_bit(line, txgpio->od_mask);
+	new_od = orig_od;
+	orig_dat = ((readq(reg) >> bank_bit) & 1) ^ orig_invert;
+	bit_cfg = readq(txgpio->register_base + bit_cfg_reg(line));
+	switch (pinconf_to_config_param(cfg)) {
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		/*
+		 * Weird, setting open-drain mode causes signal
+		 * inversion.  Note this so we can compensate in the
+		 * dir_out function.
+		 */
+		set_bit(line, txgpio->invert_mask);
+		new_invert  = true;
+		set_bit(line, txgpio->od_mask);
+		new_od = true;
+		ret = 0;
+		break;
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		clear_bit(line, txgpio->invert_mask);
+		new_invert  = false;
+		clear_bit(line, txgpio->od_mask);
+		new_od  = false;
+		ret = 0;
+		break;
+	case PIN_CONFIG_INPUT_DEBOUNCE:
+		arg = pinconf_to_config_argument(cfg);
+		if (arg > 1228) { /* 15 * 2^15 * 2.5nS maximum */
+			ret = -EINVAL;
+			break;
+		}
+		arg *= 400; /* scale to 2.5nS clocks. */
+		sel = 0;
+		while (arg > 15) {
+			sel++;
+			arg++; /* always round up */
+			arg >>= 1;
+		}
+		txgpio->line_entries[line].fil_bits =
+			(sel << GPIO_BIT_CFG_FIL_SEL_SHIFT) |
+			(arg << GPIO_BIT_CFG_FIL_CNT_SHIFT);
+		bit_cfg &= ~GPIO_BIT_CFG_FIL_MASK;
+		bit_cfg |= txgpio->line_entries[line].fil_bits;
+		writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(line));
+		ret = 0;
+		break;
+	default:
+		break;
+	}
+	raw_spin_unlock(&txgpio->lock);
+
+	/*
+	 * If currently output and OPEN_DRAIN changed, install the new
+	 * settings
+	 */
+	if ((new_invert != orig_invert || new_od != orig_od) &&
+	    (bit_cfg & GPIO_BIT_CFG_TX_OE))
+		ret = thunderx_gpio_dir_out(chip, line, orig_dat ^ new_invert);
+
+	return ret;
+}
+
+static int thunderx_gpio_get(struct gpio_chip *chip, unsigned int line)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+	int bank = line / 64;
+	int bank_bit = line % 64;
+	u64 read_bits = readq(txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_RX_DAT);
+	u64 masked_bits = read_bits & BIT_ULL(bank_bit);
+
+	if (test_bit(line, txgpio->invert_mask))
+		return masked_bits == 0;
+	else
+		return masked_bits != 0;
+}
+
+static void thunderx_gpio_set_multiple(struct gpio_chip *chip,
+				       unsigned long *mask,
+				       unsigned long *bits)
+{
+	int bank;
+	u64 set_bits, clear_bits;
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+
+	for (bank = 0; bank <= chip->ngpio / 64; bank++) {
+		set_bits = bits[bank] & mask[bank];
+		clear_bits = ~bits[bank] & mask[bank];
+		writeq(set_bits, txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_SET);
+		writeq(clear_bits, txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_CLR);
+	}
+}
+
+static void thunderx_gpio_irq_ack(struct irq_data *data)
+{
+	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+
+	writeq(GPIO_INTR_INTR,
+	       txline->txgpio->register_base + intr_reg(txline->line));
+}
+
+static void thunderx_gpio_irq_mask(struct irq_data *data)
+{
+	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+
+	writeq(GPIO_INTR_ENA_W1C,
+	       txline->txgpio->register_base + intr_reg(txline->line));
+}
+
+static void thunderx_gpio_irq_mask_ack(struct irq_data *data)
+{
+	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+
+	writeq(GPIO_INTR_ENA_W1C | GPIO_INTR_INTR,
+	       txline->txgpio->register_base + intr_reg(txline->line));
+}
+
+static void thunderx_gpio_irq_unmask(struct irq_data *data)
+{
+	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+
+	writeq(GPIO_INTR_ENA_W1S,
+	       txline->txgpio->register_base + intr_reg(txline->line));
+}
+
+static int thunderx_gpio_irq_set_type(struct irq_data *data,
+				      unsigned int flow_type)
+{
+	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+	struct thunderx_gpio *txgpio = txline->txgpio;
+	u64 bit_cfg;
+
+	irqd_set_trigger_type(data, flow_type);
+
+	bit_cfg = txline->fil_bits | GPIO_BIT_CFG_INT_EN;
+
+	if (flow_type & IRQ_TYPE_EDGE_BOTH) {
+		irq_set_handler_locked(data, handle_fasteoi_edge_irq);
+		bit_cfg |= GPIO_BIT_CFG_INT_TYPE;
+	} else {
+		irq_set_handler_locked(data, handle_fasteoi_level_irq);
+	}
+
+	raw_spin_lock(&txgpio->lock);
+	if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW)) {
+		bit_cfg |= GPIO_BIT_CFG_PIN_XOR;
+		set_bit(txline->line, txgpio->invert_mask);
+	} else {
+		clear_bit(txline->line, txgpio->invert_mask);
+	}
+	clear_bit(txline->line, txgpio->od_mask);
+	writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(txline->line));
+	raw_spin_unlock(&txgpio->lock);
+
+	return IRQ_SET_MASK_OK;
+}
+
+static void thunderx_gpio_irq_enable(struct irq_data *data)
+{
+	irq_chip_enable_parent(data);
+	thunderx_gpio_irq_unmask(data);
+}
+
+static void thunderx_gpio_irq_disable(struct irq_data *data)
+{
+	thunderx_gpio_irq_mask(data);
+	irq_chip_disable_parent(data);
+}
+
+/*
+ * Interrupts are chained from underlying MSI-X vectors.  We have
+ * these irq_chip functions to be able to handle level triggering
+ * semantics and other acknowledgment tasks associated with the GPIO
+ * mechanism.
+ */
+static struct irq_chip thunderx_gpio_irq_chip = {
+	.name			= "GPIO",
+	.irq_enable		= thunderx_gpio_irq_enable,
+	.irq_disable		= thunderx_gpio_irq_disable,
+	.irq_ack		= thunderx_gpio_irq_ack,
+	.irq_mask		= thunderx_gpio_irq_mask,
+	.irq_mask_ack		= thunderx_gpio_irq_mask_ack,
+	.irq_unmask		= thunderx_gpio_irq_unmask,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_set_type		= thunderx_gpio_irq_set_type,
+
+	.flags			= IRQCHIP_SET_TYPE_MASKED
+};
+
+static int thunderx_gpio_irq_map(struct irq_domain *d, unsigned int irq,
+				 irq_hw_number_t hwirq)
+{
+	irq_set_irq_type(irq, IRQ_TYPE_LEVEL_LOW);
+	return 0;
+}
+
+static int thunderx_gpio_irq_translate(struct irq_domain *d,
+				       struct irq_fwspec *fwspec,
+				       irq_hw_number_t *hwirq,
+				       unsigned int *type)
+{
+	if (WARN_ON(fwspec->param_count < 2))
+		return -EINVAL;
+	*hwirq = fwspec->param[0];
+	*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+	return 0;
+}
+
+static int thunderx_gpio_irq_alloc(struct irq_domain *d, unsigned int virq,
+				   unsigned int nr_irqs, void *arg)
+{
+	struct thunderx_line *txline = arg;
+
+	return irq_domain_set_hwirq_and_chip(d, virq, txline->line,
+					     &thunderx_gpio_irq_chip, txline);
+}
+
+static const struct irq_domain_ops thunderx_gpio_irqd_ops = {
+	.map		= thunderx_gpio_irq_map,
+	.alloc		= thunderx_gpio_irq_alloc,
+	.translate	= thunderx_gpio_irq_translate
+};
+
+static int thunderx_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+
+	return irq_find_mapping(txgpio->irqd, offset);
+}
+
+static int thunderx_gpio_probe(struct pci_dev *pdev,
+			       const struct pci_device_id *id)
+{
+	void __iomem * const *tbl;
+	struct device *dev = &pdev->dev;
+	struct thunderx_gpio *txgpio;
+	struct gpio_chip *chip;
+	int ngpio, i;
+	int err = 0;
+
+	txgpio = devm_kzalloc(dev, sizeof(*txgpio), GFP_KERNEL);
+	if (!txgpio)
+		return -ENOMEM;
+
+	raw_spin_lock_init(&txgpio->lock);
+	chip = &txgpio->chip;
+
+	pci_set_drvdata(pdev, txgpio);
+
+	err = pcim_enable_device(pdev);
+	if (err) {
+		dev_err(dev, "Failed to enable PCI device: err %d\n", err);
+		goto out;
+	}
+
+	err = pcim_iomap_regions(pdev, 1 << 0, KBUILD_MODNAME);
+	if (err) {
+		dev_err(dev, "Failed to iomap PCI device: err %d\n", err);
+		goto out;
+	}
+
+	tbl = pcim_iomap_table(pdev);
+	txgpio->register_base = tbl[0];
+	if (!txgpio->register_base) {
+		dev_err(dev, "Cannot map PCI resource\n");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	if (pdev->subsystem_device == 0xa10a) {
+		/* CN88XX has no GPIO_CONST register*/
+		ngpio = 50;
+		txgpio->base_msi = 48;
+	} else {
+		u64 c = readq(txgpio->register_base + GPIO_CONST);
+
+		ngpio = c & GPIO_CONST_GPIOS_MASK;
+		txgpio->base_msi = (c >> 8) & 0xff;
+	}
+
+	txgpio->msix_entries = devm_kzalloc(dev,
+					  sizeof(struct msix_entry) * ngpio,
+					  GFP_KERNEL);
+	if (!txgpio->msix_entries) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	txgpio->line_entries = devm_kzalloc(dev,
+					    sizeof(struct thunderx_line) * ngpio,
+					    GFP_KERNEL);
+	if (!txgpio->line_entries) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < ngpio; i++) {
+		u64 bit_cfg = readq(txgpio->register_base + bit_cfg_reg(i));
+
+		txgpio->msix_entries[i].entry = txgpio->base_msi + (2 * i);
+		txgpio->line_entries[i].line = i;
+		txgpio->line_entries[i].txgpio = txgpio;
+		/*
+		 * If something has already programmed the pin, use
+		 * the existing glitch filter settings, otherwise go
+		 * to 400nS.
+		 */
+		txgpio->line_entries[i].fil_bits = bit_cfg ?
+			(bit_cfg & GPIO_BIT_CFG_FIL_MASK) : GLITCH_FILTER_400NS;
+
+		if ((bit_cfg & GPIO_BIT_CFG_TX_OE) && (bit_cfg & GPIO_BIT_CFG_TX_OD))
+			set_bit(i, txgpio->od_mask);
+		if (bit_cfg & GPIO_BIT_CFG_PIN_XOR)
+			set_bit(i, txgpio->invert_mask);
+	}
+
+
+	/* Enable all MSI-X for interrupts on all possible lines. */
+	err = pci_enable_msix(pdev, txgpio->msix_entries, ngpio);
+	if (err < 0)
+		goto out;
+
+	/*
+	 * Push GPIO specific irqdomain on hierarchy created as a side
+	 * effect of the pci_enable_msix()
+	 */
+	txgpio->irqd = irq_domain_create_hierarchy(irq_get_irq_data(txgpio->msix_entries[0].vector)->domain,
+						   0, 0, of_node_to_fwnode(dev->of_node),
+						   &thunderx_gpio_irqd_ops, txgpio);
+	if (!txgpio->irqd)
+		goto out;
+
+	/* Push on irq_data and the domain for each line. */
+	for (i = 0; i < ngpio; i++) {
+		err = irq_domain_push_irq(txgpio->irqd,
+					  txgpio->msix_entries[i].vector,
+					  &txgpio->line_entries[i]);
+		if (err < 0)
+			dev_err(dev, "irq_domain_push_irq: %d\n", err);
+	}
+
+	chip->label = KBUILD_MODNAME;
+	chip->parent = dev;
+	chip->owner = THIS_MODULE;
+	chip->base = -1; /* System allocated */
+	chip->can_sleep = false;
+	chip->ngpio = ngpio;
+	chip->get_direction = thunderx_gpio_get_direction;
+	chip->direction_input = thunderx_gpio_dir_in;
+	chip->get = thunderx_gpio_get;
+	chip->direction_output = thunderx_gpio_dir_out;
+	chip->set = thunderx_gpio_set;
+	chip->set_multiple = thunderx_gpio_set_multiple;
+	chip->set_config = thunderx_gpio_set_config;
+	chip->to_irq = thunderx_gpio_to_irq;
+	err = devm_gpiochip_add_data(dev, chip, txgpio);
+	if (err)
+		goto out;
+
+	dev_info(dev, "ThunderX GPIO: %d lines with base %d.\n",
+		 ngpio, chip->base);
+	return 0;
+out:
+	pci_set_drvdata(pdev, NULL);
+	return err;
+}
+
+static void thunderx_gpio_remove(struct pci_dev *pdev)
+{
+	int i;
+	struct thunderx_gpio *txgpio = pci_get_drvdata(pdev);
+
+	for (i = 0; i < txgpio->chip.ngpio; i++)
+		irq_domain_pop_irq(txgpio->irqd,
+				   txgpio->msix_entries[i].vector);
+
+	irq_domain_remove(txgpio->irqd);
+
+	pci_set_drvdata(pdev, NULL);
+}
+
+static const struct pci_device_id thunderx_gpio_id_table[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, 0xA00A) },
+	{ 0, }	/* end of table */
+};
+
+MODULE_DEVICE_TABLE(pci, thunderx_gpio_id_table);
+
+static struct pci_driver thunderx_gpio_driver = {
+	.name = KBUILD_MODNAME,
+	.id_table = thunderx_gpio_id_table,
+	.probe = thunderx_gpio_probe,
+	.remove = thunderx_gpio_remove,
+};
+
+module_pci_driver(thunderx_gpio_driver);
+
+MODULE_DESCRIPTION("Cavium Inc. ThunderX/OCTEON-TX GPIO Driver");
+MODULE_LICENSE("GPL");
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 5/6] gpio: Add gpio driver support for ThunderX and OCTEON-TX

[David Daney]

Cavium ThunderX and OCTEON-TX are arm64 based SoCs.  Add driver for
the on-chip GPIO pins.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/gpio/Kconfig         |   8 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-thunderx.c | 572 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 581 insertions(+)
 create mode 100644 drivers/gpio/gpio-thunderx.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0504307..9291750 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -439,6 +439,14 @@ config GPIO_TS4800
 	help
 	  This driver support TS-4800 FPGA GPIO controllers.
 
+config GPIO_THUNDERX
+	tristate "Cavium ThunderX/OCTEON-TX GPIO"
+	depends on ARCH_THUNDER || (64BIT && COMPILE_TEST)
+	depends on PCI_MSI && IRQ_DOMAIN_HIERARCHY
+	help
+	  Say yes here to support the on-chip GPIO lines on the ThunderX
+	  and OCTEON-TX families of SoCs.
+
 config GPIO_TZ1090
 	bool "Toumaz Xenif TZ1090 GPIO support"
 	depends on SOC_TZ1090
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index becb96c..8d8eb15 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -111,6 +111,7 @@ obj-$(CONFIG_GPIO_SYSCON)	+= gpio-syscon.o
 obj-$(CONFIG_GPIO_TB10X)	+= gpio-tb10x.o
 obj-$(CONFIG_GPIO_TC3589X)	+= gpio-tc3589x.o
 obj-$(CONFIG_GPIO_TEGRA)	+= gpio-tegra.o
+obj-$(CONFIG_GPIO_THUNDERX)	+= gpio-thunderx.o
 obj-$(CONFIG_GPIO_TIMBERDALE)	+= gpio-timberdale.o
 obj-$(CONFIG_GPIO_PALMAS)	+= gpio-palmas.o
 obj-$(CONFIG_GPIO_TPIC2810)	+= gpio-tpic2810.o
diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
new file mode 100644
index 0000000..0973cac
--- /dev/null
+++ b/drivers/gpio/gpio-thunderx.c
@@ -0,0 +1,572 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2016, 2017 Cavium Inc.
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/spinlock.h>
+
+
+#define GPIO_RX_DAT	0x0
+#define GPIO_TX_SET	0x8
+#define GPIO_TX_CLR	0x10
+#define GPIO_CONST	0x90
+#define  GPIO_CONST_GPIOS_MASK 0xff
+#define GPIO_BIT_CFG	0x400
+#define  GPIO_BIT_CFG_TX_OE		BIT(0)
+#define  GPIO_BIT_CFG_PIN_XOR		BIT(1)
+#define  GPIO_BIT_CFG_INT_EN		BIT(2)
+#define  GPIO_BIT_CFG_INT_TYPE		BIT(3)
+#define  GPIO_BIT_CFG_FIL_MASK		GENMASK(11, 4)
+#define  GPIO_BIT_CFG_FIL_CNT_SHIFT	4
+#define  GPIO_BIT_CFG_FIL_SEL_SHIFT	8
+#define  GPIO_BIT_CFG_TX_OD		BIT(12)
+#define  GPIO_BIT_CFG_PIN_SEL_MASK	GENMASK(25, 16)
+#define GPIO_INTR	0x800
+#define  GPIO_INTR_INTR			BIT(0)
+#define  GPIO_INTR_INTR_W1S		BIT(1)
+#define  GPIO_INTR_ENA_W1C		BIT(2)
+#define  GPIO_INTR_ENA_W1S		BIT(3)
+#define GPIO_2ND_BANK	0x1400
+
+#define GLITCH_FILTER_400NS ((4u << GPIO_BIT_CFG_FIL_SEL_SHIFT) | \
+			     (9u << GPIO_BIT_CFG_FIL_CNT_SHIFT))
+
+struct thunderx_gpio;
+
+struct thunderx_line {
+	struct thunderx_gpio	*txgpio;
+	unsigned int		line;
+	unsigned int		fil_bits;
+};
+
+struct thunderx_gpio {
+	struct gpio_chip	chip;
+	u8 __iomem		*register_base;
+	struct irq_domain	*irqd;
+	struct msix_entry	*msix_entries;	/* per line MSI-X */
+	struct thunderx_line	*line_entries;	/* per line irq info */
+	raw_spinlock_t		lock;
+	unsigned long		invert_mask[2];
+	unsigned long		od_mask[2];
+	int			base_msi;
+};
+
+static unsigned int bit_cfg_reg(unsigned int line)
+{
+	return 8 * line + GPIO_BIT_CFG;
+}
+
+static unsigned int intr_reg(unsigned int line)
+{
+	return 8 * line + GPIO_INTR;
+}
+
+/*
+ * Check (and WARN) that the pin is available for GPIO.  We will not
+ * allow modification of the state of non-GPIO pins from this driver.
+ */
+static bool thunderx_gpio_is_gpio(struct thunderx_gpio *txgpio,
+				  unsigned int line)
+{
+	u64 bit_cfg = readq(txgpio->register_base + bit_cfg_reg(line));
+	bool rv = (bit_cfg & GPIO_BIT_CFG_PIN_SEL_MASK) == 0;
+
+	WARN_RATELIMIT(!rv, "Pin %d not available for GPIO\n", line);
+
+	return rv;
+}
+
+static int thunderx_gpio_dir_in(struct gpio_chip *chip, unsigned int line)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+
+	if (!thunderx_gpio_is_gpio(txgpio, line))
+		return -EIO;
+
+	raw_spin_lock(&txgpio->lock);
+	clear_bit(line, txgpio->invert_mask);
+	clear_bit(line, txgpio->od_mask);
+	writeq(txgpio->line_entries[line].fil_bits,
+	       txgpio->register_base + bit_cfg_reg(line));
+	raw_spin_unlock(&txgpio->lock);
+	return 0;
+}
+
+static void thunderx_gpio_set(struct gpio_chip *chip, unsigned int line,
+			      int value)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+	int bank = line / 64;
+	int bank_bit = line % 64;
+
+	void __iomem *reg = txgpio->register_base +
+		(bank * GPIO_2ND_BANK) + (value ? GPIO_TX_SET : GPIO_TX_CLR);
+
+	writeq(BIT_ULL(bank_bit), reg);
+}
+
+static int thunderx_gpio_dir_out(struct gpio_chip *chip, unsigned int line,
+				 int value)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+	u64 bit_cfg = txgpio->line_entries[line].fil_bits | GPIO_BIT_CFG_TX_OE;
+
+	if (!thunderx_gpio_is_gpio(txgpio, line))
+		return -EIO;
+
+	raw_spin_lock(&txgpio->lock);
+
+	thunderx_gpio_set(chip, line, value);
+
+	if (test_bit(line, txgpio->invert_mask))
+		bit_cfg |= GPIO_BIT_CFG_PIN_XOR;
+
+	if (test_bit(line, txgpio->od_mask))
+		bit_cfg |= GPIO_BIT_CFG_TX_OD;
+
+	writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(line));
+
+	raw_spin_unlock(&txgpio->lock);
+	return 0;
+}
+
+static int thunderx_gpio_get_direction(struct gpio_chip *chip, unsigned int line)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+	u64 bit_cfg;
+
+	if (!thunderx_gpio_is_gpio(txgpio, line))
+		return -EIO;
+
+	bit_cfg = readq(txgpio->register_base + bit_cfg_reg(line));
+
+	return !(bit_cfg & GPIO_BIT_CFG_TX_OE);
+}
+
+static int thunderx_gpio_set_config(struct gpio_chip *chip,
+				    unsigned int line,
+				    unsigned long cfg)
+{
+	bool orig_invert, orig_od, orig_dat, new_invert, new_od;
+	u32 arg, sel;
+	u64 bit_cfg;
+	int bank = line / 64;
+	int bank_bit = line % 64;
+	int ret = -ENOTSUPP;
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+	void __iomem *reg = txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_SET;
+
+	if (!thunderx_gpio_is_gpio(txgpio, line))
+		return -EIO;
+
+	raw_spin_lock(&txgpio->lock);
+	orig_invert = test_bit(line, txgpio->invert_mask);
+	new_invert  = orig_invert;
+	orig_od = test_bit(line, txgpio->od_mask);
+	new_od = orig_od;
+	orig_dat = ((readq(reg) >> bank_bit) & 1) ^ orig_invert;
+	bit_cfg = readq(txgpio->register_base + bit_cfg_reg(line));
+	switch (pinconf_to_config_param(cfg)) {
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		/*
+		 * Weird, setting open-drain mode causes signal
+		 * inversion.  Note this so we can compensate in the
+		 * dir_out function.
+		 */
+		set_bit(line, txgpio->invert_mask);
+		new_invert  = true;
+		set_bit(line, txgpio->od_mask);
+		new_od = true;
+		ret = 0;
+		break;
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		clear_bit(line, txgpio->invert_mask);
+		new_invert  = false;
+		clear_bit(line, txgpio->od_mask);
+		new_od  = false;
+		ret = 0;
+		break;
+	case PIN_CONFIG_INPUT_DEBOUNCE:
+		arg = pinconf_to_config_argument(cfg);
+		if (arg > 1228) { /* 15 * 2^15 * 2.5nS maximum */
+			ret = -EINVAL;
+			break;
+		}
+		arg *= 400; /* scale to 2.5nS clocks. */
+		sel = 0;
+		while (arg > 15) {
+			sel++;
+			arg++; /* always round up */
+			arg >>= 1;
+		}
+		txgpio->line_entries[line].fil_bits =
+			(sel << GPIO_BIT_CFG_FIL_SEL_SHIFT) |
+			(arg << GPIO_BIT_CFG_FIL_CNT_SHIFT);
+		bit_cfg &= ~GPIO_BIT_CFG_FIL_MASK;
+		bit_cfg |= txgpio->line_entries[line].fil_bits;
+		writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(line));
+		ret = 0;
+		break;
+	default:
+		break;
+	}
+	raw_spin_unlock(&txgpio->lock);
+
+	/*
+	 * If currently output and OPEN_DRAIN changed, install the new
+	 * settings
+	 */
+	if ((new_invert != orig_invert || new_od != orig_od) &&
+	    (bit_cfg & GPIO_BIT_CFG_TX_OE))
+		ret = thunderx_gpio_dir_out(chip, line, orig_dat ^ new_invert);
+
+	return ret;
+}
+
+static int thunderx_gpio_get(struct gpio_chip *chip, unsigned int line)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+	int bank = line / 64;
+	int bank_bit = line % 64;
+	u64 read_bits = readq(txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_RX_DAT);
+	u64 masked_bits = read_bits & BIT_ULL(bank_bit);
+
+	if (test_bit(line, txgpio->invert_mask))
+		return masked_bits == 0;
+	else
+		return masked_bits != 0;
+}
+
+static void thunderx_gpio_set_multiple(struct gpio_chip *chip,
+				       unsigned long *mask,
+				       unsigned long *bits)
+{
+	int bank;
+	u64 set_bits, clear_bits;
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+
+	for (bank = 0; bank <= chip->ngpio / 64; bank++) {
+		set_bits = bits[bank] & mask[bank];
+		clear_bits = ~bits[bank] & mask[bank];
+		writeq(set_bits, txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_SET);
+		writeq(clear_bits, txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_CLR);
+	}
+}
+
+static void thunderx_gpio_irq_ack(struct irq_data *data)
+{
+	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+
+	writeq(GPIO_INTR_INTR,
+	       txline->txgpio->register_base + intr_reg(txline->line));
+}
+
+static void thunderx_gpio_irq_mask(struct irq_data *data)
+{
+	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+
+	writeq(GPIO_INTR_ENA_W1C,
+	       txline->txgpio->register_base + intr_reg(txline->line));
+}
+
+static void thunderx_gpio_irq_mask_ack(struct irq_data *data)
+{
+	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+
+	writeq(GPIO_INTR_ENA_W1C | GPIO_INTR_INTR,
+	       txline->txgpio->register_base + intr_reg(txline->line));
+}
+
+static void thunderx_gpio_irq_unmask(struct irq_data *data)
+{
+	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+
+	writeq(GPIO_INTR_ENA_W1S,
+	       txline->txgpio->register_base + intr_reg(txline->line));
+}
+
+static int thunderx_gpio_irq_set_type(struct irq_data *data,
+				      unsigned int flow_type)
+{
+	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+	struct thunderx_gpio *txgpio = txline->txgpio;
+	u64 bit_cfg;
+
+	irqd_set_trigger_type(data, flow_type);
+
+	bit_cfg = txline->fil_bits | GPIO_BIT_CFG_INT_EN;
+
+	if (flow_type & IRQ_TYPE_EDGE_BOTH) {
+		irq_set_handler_locked(data, handle_fasteoi_edge_irq);
+		bit_cfg |= GPIO_BIT_CFG_INT_TYPE;
+	} else {
+		irq_set_handler_locked(data, handle_fasteoi_level_irq);
+	}
+
+	raw_spin_lock(&txgpio->lock);
+	if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW)) {
+		bit_cfg |= GPIO_BIT_CFG_PIN_XOR;
+		set_bit(txline->line, txgpio->invert_mask);
+	} else {
+		clear_bit(txline->line, txgpio->invert_mask);
+	}
+	clear_bit(txline->line, txgpio->od_mask);
+	writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(txline->line));
+	raw_spin_unlock(&txgpio->lock);
+
+	return IRQ_SET_MASK_OK;
+}
+
+static void thunderx_gpio_irq_enable(struct irq_data *data)
+{
+	irq_chip_enable_parent(data);
+	thunderx_gpio_irq_unmask(data);
+}
+
+static void thunderx_gpio_irq_disable(struct irq_data *data)
+{
+	thunderx_gpio_irq_mask(data);
+	irq_chip_disable_parent(data);
+}
+
+/*
+ * Interrupts are chained from underlying MSI-X vectors.  We have
+ * these irq_chip functions to be able to handle level triggering
+ * semantics and other acknowledgment tasks associated with the GPIO
+ * mechanism.
+ */
+static struct irq_chip thunderx_gpio_irq_chip = {
+	.name			= "GPIO",
+	.irq_enable		= thunderx_gpio_irq_enable,
+	.irq_disable		= thunderx_gpio_irq_disable,
+	.irq_ack		= thunderx_gpio_irq_ack,
+	.irq_mask		= thunderx_gpio_irq_mask,
+	.irq_mask_ack		= thunderx_gpio_irq_mask_ack,
+	.irq_unmask		= thunderx_gpio_irq_unmask,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_set_type		= thunderx_gpio_irq_set_type,
+
+	.flags			= IRQCHIP_SET_TYPE_MASKED
+};
+
+static int thunderx_gpio_irq_map(struct irq_domain *d, unsigned int irq,
+				 irq_hw_number_t hwirq)
+{
+	irq_set_irq_type(irq, IRQ_TYPE_LEVEL_LOW);
+	return 0;
+}
+
+static int thunderx_gpio_irq_translate(struct irq_domain *d,
+				       struct irq_fwspec *fwspec,
+				       irq_hw_number_t *hwirq,
+				       unsigned int *type)
+{
+	if (WARN_ON(fwspec->param_count < 2))
+		return -EINVAL;
+	*hwirq = fwspec->param[0];
+	*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+	return 0;
+}
+
+static int thunderx_gpio_irq_alloc(struct irq_domain *d, unsigned int virq,
+				   unsigned int nr_irqs, void *arg)
+{
+	struct thunderx_line *txline = arg;
+
+	return irq_domain_set_hwirq_and_chip(d, virq, txline->line,
+					     &thunderx_gpio_irq_chip, txline);
+}
+
+static const struct irq_domain_ops thunderx_gpio_irqd_ops = {
+	.map		= thunderx_gpio_irq_map,
+	.alloc		= thunderx_gpio_irq_alloc,
+	.translate	= thunderx_gpio_irq_translate
+};
+
+static int thunderx_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+
+	return irq_find_mapping(txgpio->irqd, offset);
+}
+
+static int thunderx_gpio_probe(struct pci_dev *pdev,
+			       const struct pci_device_id *id)
+{
+	void __iomem * const *tbl;
+	struct device *dev = &pdev->dev;
+	struct thunderx_gpio *txgpio;
+	struct gpio_chip *chip;
+	int ngpio, i;
+	int err = 0;
+
+	txgpio = devm_kzalloc(dev, sizeof(*txgpio), GFP_KERNEL);
+	if (!txgpio)
+		return -ENOMEM;
+
+	raw_spin_lock_init(&txgpio->lock);
+	chip = &txgpio->chip;
+
+	pci_set_drvdata(pdev, txgpio);
+
+	err = pcim_enable_device(pdev);
+	if (err) {
+		dev_err(dev, "Failed to enable PCI device: err %d\n", err);
+		goto out;
+	}
+
+	err = pcim_iomap_regions(pdev, 1 << 0, KBUILD_MODNAME);
+	if (err) {
+		dev_err(dev, "Failed to iomap PCI device: err %d\n", err);
+		goto out;
+	}
+
+	tbl = pcim_iomap_table(pdev);
+	txgpio->register_base = tbl[0];
+	if (!txgpio->register_base) {
+		dev_err(dev, "Cannot map PCI resource\n");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	if (pdev->subsystem_device == 0xa10a) {
+		/* CN88XX has no GPIO_CONST register*/
+		ngpio = 50;
+		txgpio->base_msi = 48;
+	} else {
+		u64 c = readq(txgpio->register_base + GPIO_CONST);
+
+		ngpio = c & GPIO_CONST_GPIOS_MASK;
+		txgpio->base_msi = (c >> 8) & 0xff;
+	}
+
+	txgpio->msix_entries = devm_kzalloc(dev,
+					  sizeof(struct msix_entry) * ngpio,
+					  GFP_KERNEL);
+	if (!txgpio->msix_entries) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	txgpio->line_entries = devm_kzalloc(dev,
+					    sizeof(struct thunderx_line) * ngpio,
+					    GFP_KERNEL);
+	if (!txgpio->line_entries) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < ngpio; i++) {
+		u64 bit_cfg = readq(txgpio->register_base + bit_cfg_reg(i));
+
+		txgpio->msix_entries[i].entry = txgpio->base_msi + (2 * i);
+		txgpio->line_entries[i].line = i;
+		txgpio->line_entries[i].txgpio = txgpio;
+		/*
+		 * If something has already programmed the pin, use
+		 * the existing glitch filter settings, otherwise go
+		 * to 400nS.
+		 */
+		txgpio->line_entries[i].fil_bits = bit_cfg ?
+			(bit_cfg & GPIO_BIT_CFG_FIL_MASK) : GLITCH_FILTER_400NS;
+
+		if ((bit_cfg & GPIO_BIT_CFG_TX_OE) && (bit_cfg & GPIO_BIT_CFG_TX_OD))
+			set_bit(i, txgpio->od_mask);
+		if (bit_cfg & GPIO_BIT_CFG_PIN_XOR)
+			set_bit(i, txgpio->invert_mask);
+	}
+
+
+	/* Enable all MSI-X for interrupts on all possible lines. */
+	err = pci_enable_msix(pdev, txgpio->msix_entries, ngpio);
+	if (err < 0)
+		goto out;
+
+	/*
+	 * Push GPIO specific irqdomain on hierarchy created as a side
+	 * effect of the pci_enable_msix()
+	 */
+	txgpio->irqd = irq_domain_create_hierarchy(irq_get_irq_data(txgpio->msix_entries[0].vector)->domain,
+						   0, 0, of_node_to_fwnode(dev->of_node),
+						   &thunderx_gpio_irqd_ops, txgpio);
+	if (!txgpio->irqd)
+		goto out;
+
+	/* Push on irq_data and the domain for each line. */
+	for (i = 0; i < ngpio; i++) {
+		err = irq_domain_push_irq(txgpio->irqd,
+					  txgpio->msix_entries[i].vector,
+					  &txgpio->line_entries[i]);
+		if (err < 0)
+			dev_err(dev, "irq_domain_push_irq: %d\n", err);
+	}
+
+	chip->label = KBUILD_MODNAME;
+	chip->parent = dev;
+	chip->owner = THIS_MODULE;
+	chip->base = -1; /* System allocated */
+	chip->can_sleep = false;
+	chip->ngpio = ngpio;
+	chip->get_direction = thunderx_gpio_get_direction;
+	chip->direction_input = thunderx_gpio_dir_in;
+	chip->get = thunderx_gpio_get;
+	chip->direction_output = thunderx_gpio_dir_out;
+	chip->set = thunderx_gpio_set;
+	chip->set_multiple = thunderx_gpio_set_multiple;
+	chip->set_config = thunderx_gpio_set_config;
+	chip->to_irq = thunderx_gpio_to_irq;
+	err = devm_gpiochip_add_data(dev, chip, txgpio);
+	if (err)
+		goto out;
+
+	dev_info(dev, "ThunderX GPIO: %d lines with base %d.\n",
+		 ngpio, chip->base);
+	return 0;
+out:
+	pci_set_drvdata(pdev, NULL);
+	return err;
+}
+
+static void thunderx_gpio_remove(struct pci_dev *pdev)
+{
+	int i;
+	struct thunderx_gpio *txgpio = pci_get_drvdata(pdev);
+
+	for (i = 0; i < txgpio->chip.ngpio; i++)
+		irq_domain_pop_irq(txgpio->irqd,
+				   txgpio->msix_entries[i].vector);
+
+	irq_domain_remove(txgpio->irqd);
+
+	pci_set_drvdata(pdev, NULL);
+}
+
+static const struct pci_device_id thunderx_gpio_id_table[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, 0xA00A) },
+	{ 0, }	/* end of table */
+};
+
+MODULE_DEVICE_TABLE(pci, thunderx_gpio_id_table);
+
+static struct pci_driver thunderx_gpio_driver = {
+	.name = KBUILD_MODNAME,
+	.id_table = thunderx_gpio_id_table,
+	.probe = thunderx_gpio_probe,
+	.remove = thunderx_gpio_remove,
+};
+
+module_pci_driver(thunderx_gpio_driver);
+
+MODULE_DESCRIPTION("Cavium Inc. ThunderX/OCTEON-TX GPIO Driver");
+MODULE_LICENSE("GPL");
-- 
1.8.3.1

Re: [PATCH v5 5/6] gpio: Add gpio driver support for ThunderX and OCTEON-TX

[Linus Walleij]

On Wed, Mar 1, 2017 at 2:48 AM, David Daney <david.daney@cavium.com> wrote:

> Cavium ThunderX and OCTEON-TX are arm64 based SoCs.  Add driver for
> the on-chip GPIO pins.
>
> Signed-off-by: David Daney <david.daney@cavium.com>

I guess this can't be merged until the corresponding IRQ changes
are merged. It can be applied in the irqchip tree or next merge
window or they can give me an immutable branch to pull to
the GPIO subsystem or whatever works.

Some pretty minor things remain.

> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/spinlock.h>

I think you should explicitly include <linux/bitops.h>

Other than that it is all fine.

> +/*
> + * Interrupts are chained from underlying MSI-X vectors.  We have
> + * these irq_chip functions to be able to handle level triggering
> + * semantics and other acknowledgment tasks associated with the GPIO
> + * mechanism.
> + */
> +static struct irq_chip thunderx_gpio_irq_chip = {
> +       .name                   = "GPIO",
> +       .irq_enable             = thunderx_gpio_irq_enable,
> +       .irq_disable            = thunderx_gpio_irq_disable,
> +       .irq_ack                = thunderx_gpio_irq_ack,
> +       .irq_mask               = thunderx_gpio_irq_mask,
> +       .irq_mask_ack           = thunderx_gpio_irq_mask_ack,
> +       .irq_unmask             = thunderx_gpio_irq_unmask,
> +       .irq_eoi                = irq_chip_eoi_parent,
> +       .irq_set_affinity       = irq_chip_set_affinity_parent,
> +       .irq_set_type           = thunderx_gpio_irq_set_type,
> +
> +       .flags                  = IRQCHIP_SET_TYPE_MASKED
> +};

I think you should implement .irq_request_resources() and
.irq_release_resources() calling
gpiochip_lock_as_irq(chip, d->hwirq)
and gpiochip_unlock_as_irq(chip, d->hwirq) respectively,
much like gpiochip_irq_reqres() and gpiochip_irq_relres()
in the gpiolib.

This helps the kernel knowing that the lines are taken for
interrupts and must be inputs.

> +static int thunderx_gpio_irq_map(struct irq_domain *d, unsigned int irq,
> +                                irq_hw_number_t hwirq)
> +{
> +       irq_set_irq_type(irq, IRQ_TYPE_LEVEL_LOW);

I would prefer that you just write the right state into the hardware
if that is what you want to do. Normally the consumer specifies
trigger type.

But I'm uncertain, maybe this is allright with the IRQ maintainers.

Apart from these nitpicks it is a beautiful driver.

Yours,
Linus Walleij

[PATCH v5 6/6] MAINTAINERS: Add entry for THUNDERX GPIO Driver.

[David Daney]

Signed-off-by: David Daney <david.daney@cavium.com>
---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 76dfb51..c281607 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11033,6 +11033,11 @@ M:	Andreas Noever <andreas.noever@gmail.com>
 S:	Maintained
 F:	drivers/thunderbolt/
 
+THUNDERX GPIO DRIVER
+M:	David Daney <david.daney@cavium.com>
+S:	Maintained
+F:	drivers/gpio/gpio-thunderx.c
+
 TI BQ27XXX POWER SUPPLY DRIVER
 R:	Andrew F. Davis <afd@ti.com>
 F:	include/linux/power/bq27xxx_battery.h
-- 
1.8.3.1