[PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO

[PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO

[Masahiro Yamada]

This series adds a GPIO controller for UniPhier SoC family.
It also works as an irqchip in hierarchy domain manner.

My problem is mapping of IRQ from this controller to the parent
irqchip is not contiguous.

  IRQ line of GPIO  --->  Parent interrupt
        0           --->     48
        1           --->     49
                ...
        15          --->     63
        16          --->    154
        17          --->    155
                ...
        20          --->    158
        21          --->    217
        22          --->    218
                ...

So, I need to have an array of parent hwirqs somehow.

Probably, most of people will try to use "interrupts" DT property,
but I noticed a potential problem for hierarchy IRQ domain.
If "interrupts" property exists in the device node, IRQ resource
may be statically allocated when platform devices are populated
from DT.  I asked this question some time ago:
https://lkml.org/lkml/2017/7/6/758

After I tackled this, I decided to put the array in the driver,
but I could not get a positive response for this.
The discussion mostly happened in v1 thread:
http://patchwork.ozlabs.org/patch/797145/

Recently, the new API irq_domain_push_irq() was merged in the
mainline.  I thought this might be useful to solve the hierarchy
domain issue.  Hence, here is a trial.

I found patch 2 is needed to avoid "type mismatch" error.

One more thing, I am worried about a race condition.

I think there is a possibility where a device tries to get IRQ
after irq_domain_create_hierarchy(), but before irq_domain_push_irq().

	priv->domain = irq_domain_create_hierarchy(...)
	if (!priv->domain)
		return -ENOMEM;

        [  *** What if a irq consumer device request the irq here? *** ]

	for (i = 0; i < nirqs; i++) {
		virq = platform_get_irq(pdev, i);
		if (virq < 0)
			continue;

		ret = irq_domain_push_irq(priv->domain, virq, (void *)(long)i);
		if (ret)
			return ret;
	}

By the time irq_domain_create_hierarchy() finished,
the irqdomain will be added to the "irq_domain_list".

If a device calls platform_get_irq(),
the domain is registered, but virq is not allocated yet.

So, irq_create_fwspec_mapping() will call irq_domain_alloc_irqs(),
then irqchip's .alloc() hook is invoked with fwspec passed as arg.

I tried to fix this by patch 3 and 4.

This topic is related to both irqdomain and GPIO,
so includes them in a series.

Comments are appreciated.

I am not sure if my approach is correct.
If I am doing wrong, I will go back to the previous adhoc solution.



Masahiro Yamada (6):
  irqdomain: rename variables in irq_domain_{push,pop}_irq()
  irqdomain: clear trigger type in irq_domain_push_irq()
  irqdomain: move IRQ_DOMAIN_NAME_ALLOCATED define to the original
    position
  irqdomain: set irq domain flags before the irq domain becomes visible
  irqdomain: add IRQ_DOMAIN_FLAG_NO_CREATE flag
  gpio: uniphier: add UniPhier GPIO controller driver

 .../devicetree/bindings/gpio/gpio-uniphier.txt     |  43 ++
 MAINTAINERS                                        |   1 +
 drivers/gpio/Kconfig                               |   8 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-uniphier.c                       | 486 +++++++++++++++++++++
 include/dt-bindings/gpio/uniphier-gpio.h           |  18 +
 include/linux/irqdomain.h                          |  22 +-
 kernel/irq/irqdomain.c                             |  93 ++--
 8 files changed, 619 insertions(+), 53 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-uniphier.txt
 create mode 100644 drivers/gpio/gpio-uniphier.c
 create mode 100644 include/dt-bindings/gpio/uniphier-gpio.h

-- 
2.7.4

[PATCH v4 1/6] irqdomain: rename variables in irq_domain_{push,pop}_irq()

[Masahiro Yamada]

The meaning of "root" in irq_domain_{push,pop} is opposite to the
documentation.  Documentation/IRQ-domain.txt depicts the hierarchy
IRQ domain as follows:

    CPU Vector irq_domain (root irq_domain to manage CPU vectors)
            ^
            |
    Interrupt Remapping irq_domain (manage irq_remapping entries)
            ^
            |
    IOAPIC irq_domain (manage IOAPIC delivery entries/pins)

>From above, the inner-most domain (nearest to the CPU) is "root".

The document also says, "When building irq_domain hierarchy, the
irq_domain near to the device is child and the irq_domain near to
CPU is parent."  This is how irq_data->parent_data works.  In
contrast, these function use a variable "child_irq_data" for that.

To avoid confusion, flip the direction as follows:

  root_irq_data  -> tail_irq_data
  child_irq_data -> parent_irq_data

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v4:
 - Newly added


 kernel/irq/irqdomain.c | 68 +++++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index e84b705..da3e0b6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1467,20 +1467,20 @@ static void irq_domain_fix_revmap(struct irq_data *d)
 }
 
 /**
- * irq_domain_push_irq() - Push a domain in to the top of a hierarchy.
+ * irq_domain_push_irq() - Push a domain in to the tail 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.  Must be called before request_irq()
+ * tail of the processing chain.  Must be called before request_irq()
  * has been called.
  */
 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);
+	struct irq_data *parent_irq_data;
+	struct irq_data *tail_irq_data = irq_get_irq_data(virq);
 	struct irq_desc *desc;
 	int rv = 0;
 
@@ -1505,43 +1505,43 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 	if (WARN_ON(!irq_domain_is_hierarchy(domain)))
 		return -EINVAL;
 
-	if (!root_irq_data)
+	if (!tail_irq_data)
 		return -EINVAL;
 
-	if (domain->parent != root_irq_data->domain)
+	if (domain->parent != tail_irq_data->domain)
 		return -EINVAL;
 
-	child_irq_data = kzalloc_node(sizeof(*child_irq_data), GFP_KERNEL,
-				      irq_data_get_node(root_irq_data));
-	if (!child_irq_data)
+	parent_irq_data = kzalloc_node(sizeof(*parent_irq_data), GFP_KERNEL,
+				      irq_data_get_node(tail_irq_data));
+	if (!parent_irq_data)
 		return -ENOMEM;
 
 	mutex_lock(&irq_domain_mutex);
 
 	/* Copy the original irq_data. */
-	*child_irq_data = *root_irq_data;
+	*parent_irq_data = *tail_irq_data;
 
 	/*
-	 * Overwrite the root_irq_data, which is embedded in struct
+	 * Overwrite the tail_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;
+	tail_irq_data->parent_data = parent_irq_data;
+	tail_irq_data->domain = domain;
+	tail_irq_data->mask = 0;
+	tail_irq_data->hwirq = 0;
+	tail_irq_data->chip = NULL;
+	tail_irq_data->chip_data = NULL;
 
 	/* May (probably does) set hwirq, chip, etc. */
 	rv = irq_domain_alloc_irqs_hierarchy(domain, virq, 1, arg);
 	if (rv) {
 		/* Restore the original irq_data. */
-		*root_irq_data = *child_irq_data;
+		*tail_irq_data = *parent_irq_data;
 		goto error;
 	}
 
-	irq_domain_fix_revmap(child_irq_data);
-	irq_domain_set_mapping(domain, root_irq_data->hwirq, root_irq_data);
+	irq_domain_fix_revmap(parent_irq_data);
+	irq_domain_set_mapping(domain, tail_irq_data->hwirq, tail_irq_data);
 
 error:
 	mutex_unlock(&irq_domain_mutex);
@@ -1551,7 +1551,7 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 EXPORT_SYMBOL_GPL(irq_domain_push_irq);
 
 /**
- * irq_domain_pop_irq() - Remove a domain from the top of a hierarchy.
+ * irq_domain_pop_irq() - Remove a domain from the tail of a hierarchy.
  * @domain:	Domain to remove.
  * @virq:	Irq to remove the domain from.
  *
@@ -1560,8 +1560,8 @@ EXPORT_SYMBOL_GPL(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 *tail_irq_data = irq_get_irq_data(virq);
+	struct irq_data *parent_irq_data;
 	struct irq_data *tmp_irq_data;
 	struct irq_desc *desc;
 
@@ -1583,37 +1583,37 @@ int irq_domain_pop_irq(struct irq_domain *domain, int virq)
 	if (domain == NULL)
 		return -EINVAL;
 
-	if (!root_irq_data)
+	if (!tail_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))
+	/* We can only "pop" if this domain is at the tail of the list */
+	if (WARN_ON(tail_irq_data != tmp_irq_data))
 		return -EINVAL;
 
-	if (WARN_ON(root_irq_data->domain != domain))
+	if (WARN_ON(tail_irq_data->domain != domain))
 		return -EINVAL;
 
-	child_irq_data = root_irq_data->parent_data;
-	if (WARN_ON(!child_irq_data))
+	parent_irq_data = tail_irq_data->parent_data;
+	if (WARN_ON(!parent_irq_data))
 		return -EINVAL;
 
 	mutex_lock(&irq_domain_mutex);
 
-	root_irq_data->parent_data = NULL;
+	tail_irq_data->parent_data = NULL;
 
-	irq_domain_clear_mapping(domain, root_irq_data->hwirq);
+	irq_domain_clear_mapping(domain, tail_irq_data->hwirq);
 	irq_domain_free_irqs_hierarchy(domain, virq, 1);
 
 	/* Restore the original irq_data. */
-	*root_irq_data = *child_irq_data;
+	*tail_irq_data = *parent_irq_data;
 
-	irq_domain_fix_revmap(root_irq_data);
+	irq_domain_fix_revmap(tail_irq_data);
 
 	mutex_unlock(&irq_domain_mutex);
 
-	kfree(child_irq_data);
+	kfree(parent_irq_data);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH v4 1/6] irqdomain: rename variables in irq_domain_{push,pop}_irq()

[Marc Zyngier]

On 07/09/17 12:41, Masahiro Yamada wrote:
> The meaning of "root" in irq_domain_{push,pop} is opposite to the
> documentation.  Documentation/IRQ-domain.txt depicts the hierarchy
> IRQ domain as follows:
> 
>     CPU Vector irq_domain (root irq_domain to manage CPU vectors)
>             ^
>             |
>     Interrupt Remapping irq_domain (manage irq_remapping entries)
>             ^
>             |
>     IOAPIC irq_domain (manage IOAPIC delivery entries/pins)
> 
> From above, the inner-most domain (nearest to the CPU) is "root".
> 
> The document also says, "When building irq_domain hierarchy, the
> irq_domain near to the device is child and the irq_domain near to
> CPU is parent."  This is how irq_data->parent_data works.  In
> contrast, these function use a variable "child_irq_data" for that.
The exact opposite argument could be used for the data structure. The
irq_desc is the root of the list ordered with parent_data.

Yes, this is confusing, but because we're using the same English words
to describe two different things, we're bound to make one thing more
difficult. I'm unconvinced that this change helps anything (it certainly
confuses me more than anything else).

Thanks,

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

Re: [PATCH v4 1/6] irqdomain: rename variables in irq_domain_{push,pop}_irq()

[David Daney]

On 09/07/2017 05:47 AM, Marc Zyngier wrote:
> On 07/09/17 12:41, Masahiro Yamada wrote:
>> The meaning of "root" in irq_domain_{push,pop} is opposite to the
>> documentation.  Documentation/IRQ-domain.txt depicts the hierarchy
>> IRQ domain as follows:
>>
>>      CPU Vector irq_domain (root irq_domain to manage CPU vectors)
>>              ^
>>              |
>>      Interrupt Remapping irq_domain (manage irq_remapping entries)
>>              ^
>>              |
>>      IOAPIC irq_domain (manage IOAPIC delivery entries/pins)
>>
>>  From above, the inner-most domain (nearest to the CPU) is "root".
>>
>> The document also says, "When building irq_domain hierarchy, the
>> irq_domain near to the device is child and the irq_domain near to
>> CPU is parent."  This is how irq_data->parent_data works.  In
>> contrast, these function use a variable "child_irq_data" for that.
> The exact opposite argument could be used for the data structure. The
> irq_desc is the root of the list ordered with parent_data.
> 
> Yes, this is confusing, but because we're using the same English words
> to describe two different things, we're bound to make one thing more
> difficult. I'm unconvinced that this change helps anything (it certainly
> confuses me more than anything else).
> 

There may be room for improvement here.

Here is my recollection of how I choose the names:

"root" is the thing embedded in the struct irq_desc, if you think about 
a typical linked list structure like this, we can refer to the starting 
point as the "root".  Sometimes it might be referred to as the "head" of 
the list, but usually not the "tail"

"child" was used to indicate the thing we get to by traversing the link 
in the list.  The fact that ->parent is the name of the next pointer and 
that it points to something called "child" is confusing here.

So what do I think should be done?  This:

Either
   A) s/child_irq_data/parent_irq_data/g  As this patch does, but leave 
the root_irq_data name unchanged.

   B) Change the name of the ->parent in struct irq_data to ->next

But that is just my $0.02

I fear we risk a Bike Shedding type of discussion here.


David Daney

> Thanks,
> 
> 	M.
> 

Re: [PATCH v4 1/6] irqdomain: rename variables in irq_domain_{push,pop}_irq()

[Masahiro Yamada]

Hi Marc, David,


2017-09-08 2:45 GMT+09:00 David Daney <ddaney@caviumnetworks.com>:
> On 09/07/2017 05:47 AM, Marc Zyngier wrote:
>>
>> On 07/09/17 12:41, Masahiro Yamada wrote:
>>>
>>> The meaning of "root" in irq_domain_{push,pop} is opposite to the
>>> documentation.  Documentation/IRQ-domain.txt depicts the hierarchy
>>> IRQ domain as follows:
>>>
>>>      CPU Vector irq_domain (root irq_domain to manage CPU vectors)
>>>              ^
>>>              |
>>>      Interrupt Remapping irq_domain (manage irq_remapping entries)
>>>              ^
>>>              |
>>>      IOAPIC irq_domain (manage IOAPIC delivery entries/pins)
>>>
>>>  From above, the inner-most domain (nearest to the CPU) is "root".
>>>
>>> The document also says, "When building irq_domain hierarchy, the
>>> irq_domain near to the device is child and the irq_domain near to
>>> CPU is parent."  This is how irq_data->parent_data works.  In
>>> contrast, these function use a variable "child_irq_data" for that.
>>
>> The exact opposite argument could be used for the data structure. The
>> irq_desc is the root of the list ordered with parent_data.
>>
>> Yes, this is confusing, but because we're using the same English words
>> to describe two different things, we're bound to make one thing more
>> difficult. I'm unconvinced that this change helps anything (it certainly
>> confuses me more than anything else).
>>
>
> There may be room for improvement here.
>
> Here is my recollection of how I choose the names:
>
> "root" is the thing embedded in the struct irq_desc, if you think about a
> typical linked list structure like this, we can refer to the starting point
> as the "root".  Sometimes it might be referred to as the "head" of the list,
> but usually not the "tail"
>
> "child" was used to indicate the thing we get to by traversing the link in
> the list.  The fact that ->parent is the name of the next pointer and that
> it points to something called "child" is confusing here.
> So what do I think should be done?  This:
>
> Either
>   A) s/child_irq_data/parent_irq_data/g  As this patch does, but leave the
> root_irq_data name unchanged.


Sounds better than the current situation.




>   B) Change the name of the ->parent in struct irq_data to ->next


This is a bad idea.

irq_data->parent_data corresponds to irq_domain->parent.
We should not lose consistency/symmetry.

irq_domain->parent originates in the "parent" argument
passed to irq_domain_create_hierarchy().
If we change this, it will introduce horrible confusion.

As the document says, when we talk about the hierarchy,
"the irq_domain near to the device is
child and the irq_domain near to CPU is parent"
This is the original concept, and should not be changed.


We can excuse that all the variables in these two helpers
were named from the point of linked-list view,
not talking about the hierarchy.


However, what I thought more confusing was the comment block.

/**
 * irq_domain_push_irq() - Push a domain in to the top of a hierarchy.


This comment is talking about the "hierarchy" at first glance.
So, what is my mind is the picture in Documentation/IRQ-domain.txt

But, from the term "top", this is talking about the linked list here too.
The linked list is just implementation detail...




> But that is just my $0.02
>
> I fear we risk a Bike Shedding type of discussion here.

-- 
Best Regards
Masahiro Yamada

[PATCH v4 2/6] irqdomain: clear trigger type in irq_domain_push_irq()

[Masahiro Yamada]

Prior to the addition of irq_domain_push_irq(), the hierarchy
IRQ domain always allocates IRQs from the outer-most domain.
Each irqchip usually calls irq_domain_alloc_irqs_parent(),
ascending the topology up to the root irqchip.

The brand-new function irq_domain_push_irq() allows us to allocate
IRQs for parent domain first, then add a child irq_data to the
tail of the chain.

For the new use-case, if the parent sets a temporary trigger type,
it may differ from the type requested to the outer-most irqchip,
then irq_create_fwspec_mapping() warns "type mismatch, failed to map..."

Clear the trigger type when a new irq_data is connected to the chain.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v4:
  - Newly added


 kernel/irq/irqdomain.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index da3e0b6..18d11b9 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1532,6 +1532,9 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 	tail_irq_data->chip = NULL;
 	tail_irq_data->chip_data = NULL;
 
+	/* clear the trigger type to avoid "type mismatch" error */
+	irqd_set_trigger_type(tail_irq_data, IRQ_TYPE_NONE);
+
 	/* May (probably does) set hwirq, chip, etc. */
 	rv = irq_domain_alloc_irqs_hierarchy(domain, virq, 1, arg);
 	if (rv) {
-- 
2.7.4

Re: [PATCH v4 2/6] irqdomain: clear trigger type in irq_domain_push_irq()

[Marc Zyngier]

On 07/09/17 12:41, Masahiro Yamada wrote:
> Prior to the addition of irq_domain_push_irq(), the hierarchy
> IRQ domain always allocates IRQs from the outer-most domain.
> Each irqchip usually calls irq_domain_alloc_irqs_parent(),
> ascending the topology up to the root irqchip.
> 
> The brand-new function irq_domain_push_irq() allows us to allocate
> IRQs for parent domain first, then add a child irq_data to the
> tail of the chain.
> 
> For the new use-case, if the parent sets a temporary trigger type,
> it may differ from the type requested to the outer-most irqchip,
> then irq_create_fwspec_mapping() warns "type mismatch, failed to map..."
> 
> Clear the trigger type when a new irq_data is connected to the chain.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v4:
>   - Newly added
> 
> 
>  kernel/irq/irqdomain.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index da3e0b6..18d11b9 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -1532,6 +1532,9 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
>  	tail_irq_data->chip = NULL;
>  	tail_irq_data->chip_data = NULL;
>  
> +	/* clear the trigger type to avoid "type mismatch" error */
> +	irqd_set_trigger_type(tail_irq_data, IRQ_TYPE_NONE);
> +

This feels wrong. The initial interrupt hierarchy does have a set
trigger, because it has been configured already, and switching it to
NONE is hiding the fact that you're setting it to another, conflicting
value.

Your new fwspec should have a type that is really compatible with with
the underlying interrupt, however "temporary". If it is not, you have a
problem.

Thanks,

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

Re: [PATCH v4 2/6] irqdomain: clear trigger type in irq_domain_push_irq()

[Masahiro Yamada]

Hi Marc,


2017-09-07 21:25 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
> On 07/09/17 12:41, Masahiro Yamada wrote:
>> Prior to the addition of irq_domain_push_irq(), the hierarchy
>> IRQ domain always allocates IRQs from the outer-most domain.
>> Each irqchip usually calls irq_domain_alloc_irqs_parent(),
>> ascending the topology up to the root irqchip.
>>
>> The brand-new function irq_domain_push_irq() allows us to allocate
>> IRQs for parent domain first, then add a child irq_data to the
>> tail of the chain.
>>
>> For the new use-case, if the parent sets a temporary trigger type,
>> it may differ from the type requested to the outer-most irqchip,
>> then irq_create_fwspec_mapping() warns "type mismatch, failed to map..."
>>
>> Clear the trigger type when a new irq_data is connected to the chain.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> Changes in v4:
>>   - Newly added
>>
>>
>>  kernel/irq/irqdomain.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index da3e0b6..18d11b9 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -1532,6 +1532,9 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
>>       tail_irq_data->chip = NULL;
>>       tail_irq_data->chip_data = NULL;
>>
>> +     /* clear the trigger type to avoid "type mismatch" error */
>> +     irqd_set_trigger_type(tail_irq_data, IRQ_TYPE_NONE);
>> +
>
> This feels wrong. The initial interrupt hierarchy does have a set
> trigger, because it has been configured already, and switching it to
> NONE is hiding the fact that you're setting it to another, conflicting
> value.
>
> Your new fwspec should have a type that is really compatible with with
> the underlying interrupt, however "temporary". If it is not, you have a
> problem.
>


My motivation is to describe interrupt connection by
DT property, like follows:

interrupts = <48 4>, <49 4>, <50 4>, <51 4>,
        <52 4>, <53 4>, <54 4>, <55 4>,
        <56 4>, <57 4>, <58 4>, <59 4>,
        <60 4>, <61 4>, <62 4>, <63 4>,
        <154 4>, <155 4>, <156 4>, <157 4>,
        <158 4>;


The number of cells is fixed, so I need to
give something as the second parameter.

The "4" may not be a real trigger type,
it is just a temporal value to fill the space.


The device may pass a different trigger type
when it calls gpio_to_irq() && request_irq().
I cannot know the real trigger type until the device is probed.


Having said that, if this is a bad idea,
I will consider a different approach for my GPIO driver.


-- 
Best Regards
Masahiro Yamada

[PATCH v4 3/6] irqdomain: move IRQ_DOMAIN_NAME_ALLOCATED define to the original position

[Masahiro Yamada]

Commit 6a6544e520ab ("genirq/irqdomain: Remove auto-recursive hierarchy
support") not only deleted IRQ_DOMAIN_FLAG_AUTO_RECURSIVE, but also
moved IRQ_DOMAIN_NAME_ALLOCATED up.

Get it back to the original position to sort the enum by the bit shift.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v4:
  - Newly added


 include/linux/irqdomain.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 81e4889..31be32d 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -180,9 +180,6 @@ enum {
 	/* Irq domain is hierarchical */
 	IRQ_DOMAIN_FLAG_HIERARCHY	= (1 << 0),
 
-	/* Irq domain name was allocated in __irq_domain_add() */
-	IRQ_DOMAIN_NAME_ALLOCATED	= (1 << 6),
-
 	/* Irq domain is an IPI domain with virq per cpu */
 	IRQ_DOMAIN_FLAG_IPI_PER_CPU	= (1 << 2),
 
@@ -195,6 +192,9 @@ enum {
 	/* Irq domain implements MSI remapping */
 	IRQ_DOMAIN_FLAG_MSI_REMAP	= (1 << 5),
 
+	/* Irq domain name was allocated in __irq_domain_add() */
+	IRQ_DOMAIN_NAME_ALLOCATED	= (1 << 6),
+
 	/*
 	 * Flags starting from IRQ_DOMAIN_FLAG_NONCORE are reserved
 	 * for implementation specific purposes and ignored by the
-- 
2.7.4

Re: [PATCH v4 3/6] irqdomain: move IRQ_DOMAIN_NAME_ALLOCATED define to the original position

[Marc Zyngier]

On 07/09/17 12:41, Masahiro Yamada wrote:
> Commit 6a6544e520ab ("genirq/irqdomain: Remove auto-recursive hierarchy
> support") not only deleted IRQ_DOMAIN_FLAG_AUTO_RECURSIVE, but also
> moved IRQ_DOMAIN_NAME_ALLOCATED up.
> 
> Get it back to the original position to sort the enum by the bit shift.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v4:
>   - Newly added
> 
> 
>  include/linux/irqdomain.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 81e4889..31be32d 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -180,9 +180,6 @@ enum {
>  	/* Irq domain is hierarchical */
>  	IRQ_DOMAIN_FLAG_HIERARCHY	= (1 << 0),
>  
> -	/* Irq domain name was allocated in __irq_domain_add() */
> -	IRQ_DOMAIN_NAME_ALLOCATED	= (1 << 6),
> -
>  	/* Irq domain is an IPI domain with virq per cpu */
>  	IRQ_DOMAIN_FLAG_IPI_PER_CPU	= (1 << 2),
>  
> @@ -195,6 +192,9 @@ enum {
>  	/* Irq domain implements MSI remapping */
>  	IRQ_DOMAIN_FLAG_MSI_REMAP	= (1 << 5),
>  
> +	/* Irq domain name was allocated in __irq_domain_add() */
> +	IRQ_DOMAIN_NAME_ALLOCATED	= (1 << 6),
> +

The right fix would be to leave it where it is, but to actually fix the
shift, which is what I should have done the first place.

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

Re: [PATCH v4 3/6] irqdomain: move IRQ_DOMAIN_NAME_ALLOCATED define to the original position

[Masahiro Yamada]

2017-09-07 21:04 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
> On 07/09/17 12:41, Masahiro Yamada wrote:
>> Commit 6a6544e520ab ("genirq/irqdomain: Remove auto-recursive hierarchy
>> support") not only deleted IRQ_DOMAIN_FLAG_AUTO_RECURSIVE, but also
>> moved IRQ_DOMAIN_NAME_ALLOCATED up.
>>
>> Get it back to the original position to sort the enum by the bit shift.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> Changes in v4:
>>   - Newly added
>>
>>
>>  include/linux/irqdomain.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>> index 81e4889..31be32d 100644
>> --- a/include/linux/irqdomain.h
>> +++ b/include/linux/irqdomain.h
>> @@ -180,9 +180,6 @@ enum {
>>       /* Irq domain is hierarchical */
>>       IRQ_DOMAIN_FLAG_HIERARCHY       = (1 << 0),
>>
>> -     /* Irq domain name was allocated in __irq_domain_add() */
>> -     IRQ_DOMAIN_NAME_ALLOCATED       = (1 << 6),
>> -
>>       /* Irq domain is an IPI domain with virq per cpu */
>>       IRQ_DOMAIN_FLAG_IPI_PER_CPU     = (1 << 2),
>>
>> @@ -195,6 +192,9 @@ enum {
>>       /* Irq domain implements MSI remapping */
>>       IRQ_DOMAIN_FLAG_MSI_REMAP       = (1 << 5),
>>
>> +     /* Irq domain name was allocated in __irq_domain_add() */
>> +     IRQ_DOMAIN_NAME_ALLOCATED       = (1 << 6),
>> +
>
> The right fix would be to leave it where it is, but to actually fix the
> shift, which is what I should have done the first place.


You are definitely right.

At first, I missed the fact that (1 << 6) was already used,
then I assigned the same value to my new flag.

So, I tried to fix the list before adding my new one.


Without 5/6, I do not have a good reason to
push this cosmetic patch only.



-- 
Best Regards
Masahiro Yamada

[PATCH v4 4/6] irqdomain: set irq domain flags before the irq domain becomes visible

[Masahiro Yamada]

irq_domain_create_hierarchy() sets additional flags after the domain
is successfully allocated, but the domain becomes visible when it is
added to irq_domain_list.  When a consumer gets the domain, the flags
may not have been set yet.

Move "domain->flags |= flags;" into __irq_domain_add().

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v4:
  - Newly added


 include/linux/irqdomain.h | 13 +++++++------
 kernel/irq/irqdomain.c    | 19 +++++++++----------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 31be32d..7609807 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -237,7 +237,8 @@ static inline struct fwnode_handle *irq_domain_alloc_fwnode(void *data)
 }
 
 void irq_domain_free_fwnode(struct fwnode_handle *fwnode);
-struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size,
+struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode,
+				    unsigned int flags, int size,
 				    irq_hw_number_t hwirq_max, int direct_max,
 				    const struct irq_domain_ops *ops,
 				    void *host_data);
@@ -309,14 +310,14 @@ static inline struct irq_domain *irq_domain_add_linear(struct device_node *of_no
 					 const struct irq_domain_ops *ops,
 					 void *host_data)
 {
-	return __irq_domain_add(of_node_to_fwnode(of_node), size, size, 0, ops, host_data);
+	return __irq_domain_add(of_node_to_fwnode(of_node), 0, size, size, 0, ops, host_data);
 }
 static inline struct irq_domain *irq_domain_add_nomap(struct device_node *of_node,
 					 unsigned int max_irq,
 					 const struct irq_domain_ops *ops,
 					 void *host_data)
 {
-	return __irq_domain_add(of_node_to_fwnode(of_node), 0, max_irq, max_irq, ops, host_data);
+	return __irq_domain_add(of_node_to_fwnode(of_node), 0, 0, max_irq, max_irq, ops, host_data);
 }
 static inline struct irq_domain *irq_domain_add_legacy_isa(
 				struct device_node *of_node,
@@ -330,7 +331,7 @@ static inline struct irq_domain *irq_domain_add_tree(struct device_node *of_node
 					 const struct irq_domain_ops *ops,
 					 void *host_data)
 {
-	return __irq_domain_add(of_node_to_fwnode(of_node), 0, ~0, 0, ops, host_data);
+	return __irq_domain_add(of_node_to_fwnode(of_node), 0, 0, ~0, 0, ops, host_data);
 }
 
 static inline struct irq_domain *irq_domain_create_linear(struct fwnode_handle *fwnode,
@@ -338,14 +339,14 @@ static inline struct irq_domain *irq_domain_create_linear(struct fwnode_handle *
 					 const struct irq_domain_ops *ops,
 					 void *host_data)
 {
-	return __irq_domain_add(fwnode, size, size, 0, ops, host_data);
+	return __irq_domain_add(fwnode, 0, size, size, 0, ops, host_data);
 }
 
 static inline struct irq_domain *irq_domain_create_tree(struct fwnode_handle *fwnode,
 					 const struct irq_domain_ops *ops,
 					 void *host_data)
 {
-	return __irq_domain_add(fwnode, 0, ~0, 0, ops, host_data);
+	return __irq_domain_add(fwnode, 0, 0, ~0, 0, ops, host_data);
 }
 
 extern void irq_domain_remove(struct irq_domain *host);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 18d11b9..b317a64 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -115,6 +115,7 @@ EXPORT_SYMBOL_GPL(irq_domain_free_fwnode);
 /**
  * __irq_domain_add() - Allocate a new irq_domain data structure
  * @fwnode: firmware node for the interrupt controller
+ * @flags:	Irq domain flags associated to the domain
  * @size: Size of linear map; 0 for radix mapping only
  * @hwirq_max: Maximum number of interrupts supported by controller
  * @direct_max: Maximum value of direct maps; Use ~0 for no limit; 0 for no
@@ -125,7 +126,8 @@ EXPORT_SYMBOL_GPL(irq_domain_free_fwnode);
  * Allocates and initialize and irq_domain structure.
  * Returns pointer to IRQ domain, or NULL on failure.
  */
-struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size,
+struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode,
+				    unsigned int flags, int size,
 				    irq_hw_number_t hwirq_max, int direct_max,
 				    const struct irq_domain_ops *ops,
 				    void *host_data)
@@ -213,6 +215,7 @@ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, int size,
 	INIT_RADIX_TREE(&domain->revmap_tree, GFP_KERNEL);
 	domain->ops = ops;
 	domain->host_data = host_data;
+	domain->flags |= flags;
 	domain->hwirq_max = hwirq_max;
 	domain->revmap_size = size;
 	domain->revmap_direct_max_irq = direct_max;
@@ -319,7 +322,7 @@ struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
 {
 	struct irq_domain *domain;
 
-	domain = __irq_domain_add(of_node_to_fwnode(of_node), size, size, 0, ops, host_data);
+	domain = __irq_domain_add(of_node_to_fwnode(of_node), 0, size, size, 0, ops, host_data);
 	if (!domain)
 		return NULL;
 
@@ -363,7 +366,7 @@ struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
 {
 	struct irq_domain *domain;
 
-	domain = __irq_domain_add(of_node_to_fwnode(of_node), first_hwirq + size,
+	domain = __irq_domain_add(of_node_to_fwnode(of_node), 0, first_hwirq + size,
 				  first_hwirq + size, 0, ops, host_data);
 	if (domain)
 		irq_domain_associate_many(domain, first_irq, first_hwirq, size);
@@ -1133,14 +1136,10 @@ struct irq_domain *irq_domain_create_hierarchy(struct irq_domain *parent,
 {
 	struct irq_domain *domain;
 
-	if (size)
-		domain = irq_domain_create_linear(fwnode, size, ops, host_data);
-	else
-		domain = irq_domain_create_tree(fwnode, ops, host_data);
-	if (domain) {
+	domain = __irq_domain_add(fwnode, flags, size, size ? size : ~0, 0,
+				  ops, host_data);
+	if (domain)
 		domain->parent = parent;
-		domain->flags |= flags;
-	}
 
 	return domain;
 }
-- 
2.7.4

[PATCH v4 5/6] irqdomain: add IRQ_DOMAIN_FLAG_NO_CREATE flag

[Masahiro Yamada]

When an irqchip driver uses irq_domain_push_irq(), all irqs should be
statically created by the irqchip.

If a device tries to allocate an irq on-the-fly, irq_domain_alloc_irqs()
is called.  It allocates struct irq_data and invokes .alloc() hook
passing fwspec as its argument.  This is probably not what the irqchip
expects (unless .alloc can call it recursively).

This issue could happen when a device tries to get irq after
irq_domain_create_hierarchy(), but before irq_domain_push_irq().

To avoid the race, add IRQ_DOMAIN_FLAG_NO_CREATE flag.  This flag
prevents devices from creating irqs.  Devices are only allowed to get
already existing irqs.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v4:
  - Newly added


 include/linux/irqdomain.h | 3 +++
 kernel/irq/irqdomain.c    | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 7609807..525de32 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -195,6 +195,9 @@ enum {
 	/* Irq domain name was allocated in __irq_domain_add() */
 	IRQ_DOMAIN_NAME_ALLOCATED	= (1 << 6),
 
+	/* Do not allow irq consumers to create irq */
+	IRQ_DOMAIN_FLAG_NO_CREATE	= (1 << 7),
+
 	/*
 	 * Flags starting from IRQ_DOMAIN_FLAG_NONCORE are reserved
 	 * for implementation specific purposes and ignored by the
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index b317a64..ecf107ab 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -806,6 +806,9 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 		return 0;
 	}
 
+	if (domain->flags & IRQ_DOMAIN_FLAG_NO_CREATE)
+		return -EPROBE_DEFER;
+
 	if (irq_domain_is_hierarchy(domain)) {
 		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
 		if (virq <= 0)
-- 
2.7.4

[PATCH v4 6/6] gpio: uniphier: add UniPhier GPIO controller driver

[Masahiro Yamada]

This GPIO controller device is used on UniPhier SoCs.

It also serves as an interrupt controller, but interrupt signals are
just delivered to the parent irqchip without any latching or OR'ing.
This is implemented by using hierarchy IRQ domain.

Implementation note:
Unfortunately, the IRQ mapping from this controller to the parent is
random. (48, 49, ..., 63, 154, 155, ...)
If "interrupts" property is used, IRQ resources may be statically
allocated when platform devices are populated from DT.  This can be
a problem for the hierarchy IRQ domain because IRQ allocation must
happen from the outer-most domain up to the root domain in order to
build up the stacked IRQ.  (https://lkml.org/lkml/2017/7/6/758)
Solutions to work around it could be to hard-code parent hwirqs or
to invent a driver-specific DT property.

Here, the new API irq_domain_push_irq() was merged by v4.14-rc1.
It allows to add irq_data to the existing hierarchy.  It will help
to make this driver work whether the parent has already initialized
the hierarchy or not.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v4:
  - Add COMPILE_TEST and select IRQ_DOMAIN_HIERARCHY
  - Reimplement irqchip part by using irq_domain_push_irq()

Changes in v3:
  - Add .irq_set_affinity() hook
  - Use irq_domain_create_hierarchy() instead of legacy
    irq_domain_add_hierarchy()

Changes in v2:
  - Remove +32 offset for parent interrupts to follow the GIC
    binding convention
  - Let uniphier_gpio_irq_alloc() fail if nr_irqs != 1
  - Allocate gpio_chip statically because just one instance is
    supported
  - Fix suspend and resume hooks

 .../devicetree/bindings/gpio/gpio-uniphier.txt     |  43 ++
 MAINTAINERS                                        |   1 +
 drivers/gpio/Kconfig                               |   8 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-uniphier.c                       | 486 +++++++++++++++++++++
 include/dt-bindings/gpio/uniphier-gpio.h           |  18 +
 6 files changed, 557 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-uniphier.txt
 create mode 100644 drivers/gpio/gpio-uniphier.c
 create mode 100644 include/dt-bindings/gpio/uniphier-gpio.h

diff --git a/Documentation/devicetree/bindings/gpio/gpio-uniphier.txt b/Documentation/devicetree/bindings/gpio/gpio-uniphier.txt
new file mode 100644
index 0000000..10326df
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-uniphier.txt
@@ -0,0 +1,43 @@
+UniPhier GPIO controller
+
+Required properties:
+- compatible: Should be "socionext,uniphier-gpio".
+- reg: Specifies offset and length of the register set for the device.
+- gpio-controller: Marks the device node as a GPIO controller.
+- #gpio-cells: Should be 2.  The first cell is the pin number and the second
+  cell is used to specify optional parameters.
+- interrupt-parent: Specifies the parent interrupt controller.
+- interrupts: Specifies interrupts connected to the parent interrupt controller.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells: Should be 2.  The first cell defines the interrupt number.
+  The second cell bits[3:0] is used to specify trigger type as follows:
+    1 = low-to-high edge triggered
+    2 = high-to-low edge triggered
+    4 = active high level-sensitive
+    8 = active low level-sensitive
+  Valid combinations are 1, 2, 3, 4, 8.
+- ngpios: Specifies the number of GPIO lines.
+- gpio-ranges: Mapping to pin controller pins (as described in gpio.txt)
+
+Optional properties:
+- gpio-ranges-group-names: Used for named gpio ranges (as described in gpio.txt)
+
+Example:
+	gpio: gpio@55000000 {
+		compatible = "socionext,uniphier-gpio";
+		reg = <0x55000000 0x200>;
+		interrupt-parent = <&aidet>;
+		interrupts = <48 4>, <49 4>, <50 4>, <51 4>,
+			     <52 4>, <53 4>, <54 4>, <55 4>,
+			     <56 4>, <57 4>, <58 4>, <59 4>,
+			     <60 4>, <61 4>, <62 4>, <63 4>,
+			     <154 4>, <155 4>, <156 4>, <157 4>,
+			     <158 4>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		gpio-ranges = <&pinctrl 0 0 0>;
+		gpio-ranges-group-names = "gpio_range";
+		ngpios = <248>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 11dde28..df67191 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2007,6 +2007,7 @@ F:	arch/arm/mm/cache-uniphier.c
 F:	arch/arm64/boot/dts/socionext/
 F:	drivers/bus/uniphier-system-bus.c
 F:	drivers/clk/uniphier/
+F:	drivers/gpio/gpio-uniphier.c
 F:	drivers/i2c/busses/i2c-uniphier*
 F:	drivers/irqchip/irq-uniphier-aidet.c
 F:	drivers/pinctrl/uniphier/
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3f80f16..25c0f308 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -475,6 +475,14 @@ config GPIO_TZ1090_PDC
 	help
 	  Say yes here to support Toumaz Xenif TZ1090 PDC GPIOs.
 
+config GPIO_UNIPHIER
+	tristate "UniPhier GPIO support"
+	depends on ARCH_UNIPHIER || COMPILE_TEST
+	depends on OF_GPIO
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  Say yes here to support UniPhier GPIOs.
+
 config GPIO_VF610
 	def_bool y
 	depends on ARCH_MXC && SOC_VF610
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index aeb70e9d..472f675 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -131,6 +131,7 @@ obj-$(CONFIG_GPIO_TWL6040)	+= gpio-twl6040.o
 obj-$(CONFIG_GPIO_TZ1090)	+= gpio-tz1090.o
 obj-$(CONFIG_GPIO_TZ1090_PDC)	+= gpio-tz1090-pdc.o
 obj-$(CONFIG_GPIO_UCB1400)	+= gpio-ucb1400.o
+obj-$(CONFIG_GPIO_UNIPHIER)	+= gpio-uniphier.o
 obj-$(CONFIG_GPIO_VF610)	+= gpio-vf610.o
 obj-$(CONFIG_GPIO_VIPERBOARD)	+= gpio-viperboard.o
 obj-$(CONFIG_GPIO_VR41XX)	+= gpio-vr41xx.o
diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c
new file mode 100644
index 0000000..135c91a
--- /dev/null
+++ b/drivers/gpio/gpio-uniphier.c
@@ -0,0 +1,486 @@
+/*
+ * Copyright (C) 2017 Socionext Inc.
+ *   Author: Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <dt-bindings/gpio/uniphier-gpio.h>
+
+#define UNIPHIER_GPIO_BANK_MASK		\
+				GENMASK((UNIPHIER_GPIO_LINES_PER_BANK) - 1, 0)
+
+#define UNIPHIER_GPIO_IRQ_MAX_NUM	32
+
+#define UNIPHIER_GPIO_PORT_DATA		0x0	/* data */
+#define UNIPHIER_GPIO_PORT_DIR		0x4	/* direction (1:in, 0:out) */
+#define UNIPHIER_GPIO_IRQ_EN		0x90	/* irq enable */
+#define UNIPHIER_GPIO_IRQ_MODE		0x94	/* irq mode (1: both edge) */
+#define UNIPHIER_GPIO_IRQ_FLT_EN	0x98	/* noise filter enable */
+#define UNIPHIER_GPIO_IRQ_FLT_CYC	0x9c	/* noise filter clock cycle */
+
+struct uniphier_gpio_priv {
+	struct gpio_chip chip;
+	struct irq_chip irq_chip;
+	struct irq_domain *domain;
+	void __iomem *regs;
+	spinlock_t lock;
+	unsigned int nirqs;
+	u32 saved_vals[0];
+};
+
+static unsigned int uniphier_gpio_bank_to_reg(unsigned int bank)
+{
+	unsigned int reg;
+
+	reg = (bank + 1) * 8;
+
+	/*
+	 * Unfortunately, the GPIO port registers are not contiguous because
+	 * offset 0x90-0x9f is used for IRQ.  Add 0x10 when crossing the region.
+	 */
+	if (reg >= UNIPHIER_GPIO_IRQ_EN)
+		reg += 0x10;
+
+	return reg;
+}
+
+static void uniphier_gpio_get_bank_and_mask(unsigned int offset,
+					    unsigned int *bank, u32 *mask)
+{
+	*bank = offset / UNIPHIER_GPIO_LINES_PER_BANK;
+	*mask = BIT(offset % UNIPHIER_GPIO_LINES_PER_BANK);
+}
+
+static void uniphier_gpio_reg_update(struct uniphier_gpio_priv *priv,
+				     unsigned int reg, u32 mask, u32 val)
+{
+	unsigned long flags;
+	u32 tmp;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	tmp = readl(priv->regs + reg);
+	tmp &= ~mask;
+	tmp |= mask & val;
+	writel(tmp, priv->regs + reg);
+	spin_unlock_irqrestore(&priv->lock, flags);
+}
+
+static void uniphier_gpio_bank_write(struct gpio_chip *chip, unsigned int bank,
+				     unsigned int reg, u32 mask, u32 val)
+{
+	struct uniphier_gpio_priv *priv = gpiochip_get_data(chip);
+
+	if (!mask)
+		return;
+
+	uniphier_gpio_reg_update(priv, uniphier_gpio_bank_to_reg(bank) + reg,
+				 mask, val);
+}
+
+static void uniphier_gpio_offset_write(struct gpio_chip *chip,
+				       unsigned int offset, unsigned int reg,
+				       int val)
+{
+	unsigned int bank;
+	u32 mask;
+
+	uniphier_gpio_get_bank_and_mask(offset, &bank, &mask);
+
+	uniphier_gpio_bank_write(chip, bank, reg, mask, val ? mask : 0);
+}
+
+static int uniphier_gpio_offset_read(struct gpio_chip *chip,
+				     unsigned int offset, unsigned int reg)
+{
+	struct uniphier_gpio_priv *priv = gpiochip_get_data(chip);
+	unsigned int bank, reg_offset;
+	u32 mask;
+
+	uniphier_gpio_get_bank_and_mask(offset, &bank, &mask);
+	reg_offset = uniphier_gpio_bank_to_reg(bank) + reg;
+
+	return !!(readl(priv->regs + reg_offset) & mask);
+}
+
+static int uniphier_gpio_get_direction(struct gpio_chip *chip,
+				       unsigned int offset)
+{
+	return uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_PORT_DIR);
+}
+
+static int uniphier_gpio_direction_input(struct gpio_chip *chip,
+					 unsigned int offset)
+{
+	uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_PORT_DIR, 1);
+
+	return 0;
+}
+
+static int uniphier_gpio_direction_output(struct gpio_chip *chip,
+					  unsigned int offset, int val)
+{
+	uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_PORT_DATA, val);
+	uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_PORT_DIR, 0);
+
+	return 0;
+}
+
+static int uniphier_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	return uniphier_gpio_offset_read(chip, offset, UNIPHIER_GPIO_PORT_DATA);
+}
+
+static void uniphier_gpio_set(struct gpio_chip *chip,
+			      unsigned int offset, int val)
+{
+	uniphier_gpio_offset_write(chip, offset, UNIPHIER_GPIO_PORT_DATA, val);
+}
+
+static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
+				       unsigned long *mask, unsigned long *bits)
+{
+	unsigned int bank, shift, bank_mask, bank_bits;
+	int i;
+
+	for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_LINES_PER_BANK) {
+		bank = i / UNIPHIER_GPIO_LINES_PER_BANK;
+		shift = i % BITS_PER_LONG;
+		bank_mask = (mask[BIT_WORD(i)] >> shift) &
+						UNIPHIER_GPIO_BANK_MASK;
+		bank_bits = bits[BIT_WORD(i)] >> shift;
+
+		uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_PORT_DATA,
+					 bank_mask, bank_bits);
+	}
+}
+
+static int uniphier_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct uniphier_gpio_priv *priv = gpiochip_get_data(chip);
+
+	if (offset < UNIPHIER_GPIO_IRQ_OFFSET)
+		return -ENXIO;
+
+	return irq_find_mapping(priv->domain,
+				offset - UNIPHIER_GPIO_IRQ_OFFSET);
+}
+
+static void uniphier_gpio_irq_mask(struct irq_data *data)
+{
+	struct uniphier_gpio_priv *priv = data->chip_data;
+	u32 mask = BIT(data->hwirq);
+
+	uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, 0);
+
+	return irq_chip_mask_parent(data);
+}
+
+static void uniphier_gpio_irq_unmask(struct irq_data *data)
+{
+	struct uniphier_gpio_priv *priv = data->chip_data;
+	u32 mask = BIT(data->hwirq);
+
+	uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, mask);
+
+	return irq_chip_unmask_parent(data);
+}
+
+static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct uniphier_gpio_priv *priv = data->chip_data;
+	u32 mask = BIT(data->hwirq);
+	u32 val = 0;
+
+	if (type == IRQ_TYPE_EDGE_BOTH) {
+		val = mask;
+		type = IRQ_TYPE_EDGE_FALLING;
+	}
+
+	uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_MODE, mask, val);
+	/* To enable both edge detection, the noise filter must be enabled. */
+	uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_FLT_EN, mask, val);
+
+	return irq_chip_set_type_parent(data, type);
+}
+
+static int uniphier_gpio_irq_domain_alloc(struct irq_domain *domain,
+					  unsigned int virq,
+					  unsigned int nr_irqs, void *arg)
+{
+	struct uniphier_gpio_priv *priv = domain->host_data;
+	irq_hw_number_t hwirq = (long)arg;
+
+	if (WARN_ON(nr_irqs != 1))
+		return -EINVAL;
+
+	if (WARN_ON(hwirq >= priv->nirqs))
+		return -EINVAL;
+
+	return irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					     &priv->irq_chip, priv);
+}
+
+static void uniphier_gpio_irq_domain_activate(struct irq_domain *domain,
+					      struct irq_data *data)
+{
+	struct uniphier_gpio_priv *priv = domain->host_data;
+	struct gpio_chip *chip = &priv->chip;
+
+	gpiochip_lock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET);
+}
+
+static void uniphier_gpio_irq_domain_deactivate(struct irq_domain *domain,
+						struct irq_data *data)
+{
+	struct uniphier_gpio_priv *priv = domain->host_data;
+	struct gpio_chip *chip = &priv->chip;
+
+	gpiochip_unlock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET);
+}
+
+static int uniphier_gpio_irq_domain_translate(struct irq_domain *domain,
+					      struct irq_fwspec *fwspec,
+					      unsigned long *out_hwirq,
+					      unsigned int *out_type)
+{
+	if (WARN_ON(fwspec->param_count < 2))
+		return -EINVAL;
+
+	*out_hwirq = fwspec->param[0];
+	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+	return 0;
+}
+
+static const struct irq_domain_ops uniphier_gpio_irq_domain_ops = {
+	.alloc = uniphier_gpio_irq_domain_alloc,
+	.free = irq_domain_free_irqs_common,
+	.activate = uniphier_gpio_irq_domain_activate,
+	.deactivate = uniphier_gpio_irq_domain_deactivate,
+	.translate = uniphier_gpio_irq_domain_translate,
+};
+
+static void uniphier_gpio_hw_init(struct uniphier_gpio_priv *priv)
+{
+	/*
+	 * Due to the hardware design, the noise filter must be enabled to
+	 * detect both edge interrupts.  This filter is intended to remove the
+	 * noise from the irq lines.  It does not work for GPIO input, so GPIO
+	 * debounce is not supported.  Unfortunately, the filter period is
+	 * shared among all irq lines.  Just choose a sensible period here.
+	 */
+	writel(0xff, priv->regs + UNIPHIER_GPIO_IRQ_FLT_CYC);
+}
+
+static unsigned int uniphier_gpio_get_nbanks(unsigned int ngpio)
+{
+	return DIV_ROUND_UP(ngpio, UNIPHIER_GPIO_LINES_PER_BANK);
+}
+
+static int uniphier_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *parent_np;
+	struct irq_domain *parent_domain;
+	struct uniphier_gpio_priv *priv;
+	struct gpio_chip *chip;
+	struct irq_chip *irq_chip;
+	struct resource *regs;
+	unsigned int nregs;
+	u32 ngpios;
+	int virq, ret, i;
+
+	parent_np = of_irq_find_parent(dev->of_node);
+	if (!parent_np)
+		return -ENXIO;
+
+	parent_domain = irq_find_host(parent_np);
+	of_node_put(parent_np);
+	if (!parent_domain)
+		return -EPROBE_DEFER;
+
+	ret = of_property_read_u32(dev->of_node, "ngpios", &ngpios);
+	if (ret)
+		return ret;
+
+	nregs = uniphier_gpio_get_nbanks(ngpios) * 2 + 3;
+	priv = devm_kzalloc(dev,
+			    sizeof(*priv) + sizeof(priv->saved_vals[0]) * nregs,
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->nirqs = of_irq_count(dev->of_node);
+	if (priv->nirqs > UNIPHIER_GPIO_IRQ_MAX_NUM)
+		return -EINVAL;
+
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->regs = devm_ioremap_resource(dev, regs);
+	if (IS_ERR(priv->regs))
+		return PTR_ERR(priv->regs);
+
+	spin_lock_init(&priv->lock);
+
+	chip = &priv->chip;
+	chip->label = dev_name(dev);
+	chip->parent = dev;
+	chip->request = gpiochip_generic_request;
+	chip->free = gpiochip_generic_free;
+	chip->get_direction = uniphier_gpio_get_direction;
+	chip->direction_input = uniphier_gpio_direction_input;
+	chip->direction_output = uniphier_gpio_direction_output;
+	chip->get = uniphier_gpio_get;
+	chip->set = uniphier_gpio_set;
+	chip->set_multiple = uniphier_gpio_set_multiple;
+	chip->to_irq = uniphier_gpio_to_irq;
+	chip->base = -1;
+	chip->ngpio = ngpios;
+
+	irq_chip = &priv->irq_chip;
+	irq_chip->name = dev_name(dev);
+	irq_chip->irq_mask = uniphier_gpio_irq_mask;
+	irq_chip->irq_unmask = uniphier_gpio_irq_unmask;
+	irq_chip->irq_eoi = irq_chip_eoi_parent;
+	irq_chip->irq_set_affinity = irq_chip_set_affinity_parent;
+	irq_chip->irq_set_type = uniphier_gpio_irq_set_type;
+
+	uniphier_gpio_hw_init(priv);
+
+	ret = devm_gpiochip_add_data(dev, chip, priv);
+	if (ret)
+		return ret;
+
+	priv->domain = irq_domain_create_hierarchy(
+					parent_domain,
+					IRQ_DOMAIN_FLAG_NO_CREATE, priv->nirqs,
+					of_node_to_fwnode(dev->of_node),
+					&uniphier_gpio_irq_domain_ops, priv);
+	if (!priv->domain)
+		return -ENOMEM;
+
+	for (i = 0; i < priv->nirqs; i++) {
+		virq = platform_get_irq(pdev, i);
+		if (virq < 0)
+			continue;
+
+		ret = irq_domain_push_irq(priv->domain, virq, (void *)(long)i);
+		if (ret)
+			dev_warn(dev,
+				 "failed to push to irq hierarchy. IRQ%d is not usable.",
+				 i);
+	}
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int uniphier_gpio_remove(struct platform_device *pdev)
+{
+	struct uniphier_gpio_priv *priv = platform_get_drvdata(pdev);
+	unsigned int virq;
+	int ret, i;
+
+	for (i = 0; i < priv->nirqs; i++) {
+		virq = irq_find_mapping(priv->domain, i);
+		if (!virq)
+			continue;
+		ret = irq_domain_pop_irq(priv->domain, virq);
+		if (ret)
+			return ret;
+	}
+
+	irq_domain_remove(priv->domain);
+
+	return 0;
+}
+
+static int __maybe_unused uniphier_gpio_suspend(struct device *dev)
+{
+	struct uniphier_gpio_priv *priv = dev_get_drvdata(dev);
+	unsigned int nbanks = uniphier_gpio_get_nbanks(priv->chip.ngpio);
+	u32 *val = priv->saved_vals;
+	unsigned int reg;
+	int i;
+
+	for (i = 0; i < nbanks; i++) {
+		reg = uniphier_gpio_bank_to_reg(i);
+
+		*val++ = readl(priv->regs + reg + UNIPHIER_GPIO_PORT_DATA);
+		*val++ = readl(priv->regs + reg + UNIPHIER_GPIO_PORT_DIR);
+	}
+
+	*val++ = readl(priv->regs + UNIPHIER_GPIO_IRQ_EN);
+	*val++ = readl(priv->regs + UNIPHIER_GPIO_IRQ_MODE);
+	*val++ = readl(priv->regs + UNIPHIER_GPIO_IRQ_FLT_EN);
+
+	return 0;
+}
+
+static int __maybe_unused uniphier_gpio_resume(struct device *dev)
+{
+	struct uniphier_gpio_priv *priv = dev_get_drvdata(dev);
+	unsigned int nbanks = uniphier_gpio_get_nbanks(priv->chip.ngpio);
+	const u32 *val = priv->saved_vals;
+	unsigned int reg;
+	int i;
+
+	for (i = 0; i < nbanks; i++) {
+		reg = uniphier_gpio_bank_to_reg(i);
+
+		writel(*val++, priv->regs + reg + UNIPHIER_GPIO_PORT_DATA);
+		writel(*val++, priv->regs + reg + UNIPHIER_GPIO_PORT_DIR);
+	}
+
+	writel(*val++, priv->regs + UNIPHIER_GPIO_IRQ_EN);
+	writel(*val++, priv->regs + UNIPHIER_GPIO_IRQ_MODE);
+	writel(*val++, priv->regs + UNIPHIER_GPIO_IRQ_FLT_EN);
+
+	uniphier_gpio_hw_init(priv);
+
+	return 0;
+}
+
+static const struct dev_pm_ops uniphier_gpio_pm_ops = {
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(uniphier_gpio_suspend,
+				     uniphier_gpio_resume)
+};
+
+static const struct of_device_id uniphier_gpio_match[] = {
+	{ .compatible = "socionext,uniphier-gpio" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, uniphier_gpio_match);
+
+static struct platform_driver uniphier_gpio_driver = {
+	.probe = uniphier_gpio_probe,
+	.remove = uniphier_gpio_remove,
+	.driver = {
+		.name = "uniphier-gpio",
+		.of_match_table = uniphier_gpio_match,
+		.pm = &uniphier_gpio_pm_ops,
+	},
+};
+module_platform_driver(uniphier_gpio_driver);
+
+MODULE_AUTHOR("Masahiro Yamada <yamada.masahiro@socionext.com>");
+MODULE_DESCRIPTION("UniPhier GPIO driver");
+MODULE_LICENSE("GPL");
diff --git a/include/dt-bindings/gpio/uniphier-gpio.h b/include/dt-bindings/gpio/uniphier-gpio.h
new file mode 100644
index 0000000..9f0ad17
--- /dev/null
+++ b/include/dt-bindings/gpio/uniphier-gpio.h
@@ -0,0 +1,18 @@
+/*
+ * Copyright (C) 2017 Socionext Inc.
+ *   Author: Masahiro Yamada <yamada.masahiro@socionext.com>
+ */
+
+#ifndef _DT_BINDINGS_GPIO_UNIPHIER_H
+#define _DT_BINDINGS_GPIO_UNIPHIER_H
+
+#define UNIPHIER_GPIO_LINES_PER_BANK	8
+
+#define UNIPHIER_GPIO_IRQ_OFFSET	((UNIPHIER_GPIO_LINES_PER_BANK) * 15)
+
+#define UNIPHIER_GPIO_PORT(bank, line)	\
+			((UNIPHIER_GPIO_LINES_PER_BANK) * (bank) + (line))
+
+#define UNIPHIER_GPIO_IRQ(n)		((UNIPHIER_GPIO_IRQ_OFFSET) + (n))
+
+#endif /* _DT_BINDINGS_GPIO_UNIPHIER_H */
-- 
2.7.4

Re: [PATCH v4 6/6] gpio: uniphier: add UniPhier GPIO controller driver

[Rob Herring]

On Thu, Sep 7, 2017 at 6:42 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> This GPIO controller device is used on UniPhier SoCs.
>
> It also serves as an interrupt controller, but interrupt signals are
> just delivered to the parent irqchip without any latching or OR'ing.
> This is implemented by using hierarchy IRQ domain.
>
> Implementation note:
> Unfortunately, the IRQ mapping from this controller to the parent is
> random. (48, 49, ..., 63, 154, 155, ...)
> If "interrupts" property is used, IRQ resources may be statically
> allocated when platform devices are populated from DT.  This can be
> a problem for the hierarchy IRQ domain because IRQ allocation must
> happen from the outer-most domain up to the root domain in order to
> build up the stacked IRQ.  (https://lkml.org/lkml/2017/7/6/758)
> Solutions to work around it could be to hard-code parent hwirqs or
> to invent a driver-specific DT property.
>
> Here, the new API irq_domain_push_irq() was merged by v4.14-rc1.
> It allows to add irq_data to the existing hierarchy.  It will help
> to make this driver work whether the parent has already initialized
> the hierarchy or not.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v4:
>   - Add COMPILE_TEST and select IRQ_DOMAIN_HIERARCHY
>   - Reimplement irqchip part by using irq_domain_push_irq()
>
> Changes in v3:
>   - Add .irq_set_affinity() hook
>   - Use irq_domain_create_hierarchy() instead of legacy
>     irq_domain_add_hierarchy()
>
> Changes in v2:
>   - Remove +32 offset for parent interrupts to follow the GIC
>     binding convention
>   - Let uniphier_gpio_irq_alloc() fail if nr_irqs != 1
>   - Allocate gpio_chip statically because just one instance is
>     supported
>   - Fix suspend and resume hooks
>
>  .../devicetree/bindings/gpio/gpio-uniphier.txt     |  43 ++

What happened to my ack? One line here more that before, but I'm not
going to diff your patches for you.

BTW, it is preferred to split bindings to a separate patch.

>  MAINTAINERS                                        |   1 +
>  drivers/gpio/Kconfig                               |   8 +
>  drivers/gpio/Makefile                              |   1 +
>  drivers/gpio/gpio-uniphier.c                       | 486 +++++++++++++++++++++
>  include/dt-bindings/gpio/uniphier-gpio.h           |  18 +
>  6 files changed, 557 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-uniphier.txt
>  create mode 100644 drivers/gpio/gpio-uniphier.c
>  create mode 100644 include/dt-bindings/gpio/uniphier-gpio.h

Re: [PATCH v4 6/6] gpio: uniphier: add UniPhier GPIO controller driver

[Masahiro Yamada]

Hi Rob,


2017-09-08 4:41 GMT+09:00 Rob Herring <robh+dt@kernel.org>:
> On Thu, Sep 7, 2017 at 6:42 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> This GPIO controller device is used on UniPhier SoCs.
>>
>> It also serves as an interrupt controller, but interrupt signals are
>> just delivered to the parent irqchip without any latching or OR'ing.
>> This is implemented by using hierarchy IRQ domain.
>>
>> Implementation note:
>> Unfortunately, the IRQ mapping from this controller to the parent is
>> random. (48, 49, ..., 63, 154, 155, ...)
>> If "interrupts" property is used, IRQ resources may be statically
>> allocated when platform devices are populated from DT.  This can be
>> a problem for the hierarchy IRQ domain because IRQ allocation must
>> happen from the outer-most domain up to the root domain in order to
>> build up the stacked IRQ.  (https://lkml.org/lkml/2017/7/6/758)
>> Solutions to work around it could be to hard-code parent hwirqs or
>> to invent a driver-specific DT property.
>>
>> Here, the new API irq_domain_push_irq() was merged by v4.14-rc1.
>> It allows to add irq_data to the existing hierarchy.  It will help
>> to make this driver work whether the parent has already initialized
>> the hierarchy or not.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> Changes in v4:
>>   - Add COMPILE_TEST and select IRQ_DOMAIN_HIERARCHY
>>   - Reimplement irqchip part by using irq_domain_push_irq()
>>
>> Changes in v3:
>>   - Add .irq_set_affinity() hook
>>   - Use irq_domain_create_hierarchy() instead of legacy
>>     irq_domain_add_hierarchy()
>>
>> Changes in v2:
>>   - Remove +32 offset for parent interrupts to follow the GIC
>>     binding convention
>>   - Let uniphier_gpio_irq_alloc() fail if nr_irqs != 1
>>   - Allocate gpio_chip statically because just one instance is
>>     supported
>>   - Fix suspend and resume hooks
>>
>>  .../devicetree/bindings/gpio/gpio-uniphier.txt     |  43 ++
>
> What happened to my ack? One line here more that before, but I'm not
> going to diff your patches for you.

I changed the binding for this version.
It includes a new one you have not acked yet.

Maybe I was too worried about it.


> BTW, it is preferred to split bindings to a separate patch.

I will do so next time.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 6/6] gpio: uniphier: add UniPhier GPIO controller driver

[Rob Herring]

On Sat, Sep 09, 2017 at 12:14:45AM +0900, Masahiro Yamada wrote:
> Hi Rob,
> 
> 
> 2017-09-08 4:41 GMT+09:00 Rob Herring <robh+dt@kernel.org>:
> > On Thu, Sep 7, 2017 at 6:42 AM, Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> >> This GPIO controller device is used on UniPhier SoCs.
> >>
> >> It also serves as an interrupt controller, but interrupt signals are
> >> just delivered to the parent irqchip without any latching or OR'ing.
> >> This is implemented by using hierarchy IRQ domain.
> >>
> >> Implementation note:
> >> Unfortunately, the IRQ mapping from this controller to the parent is
> >> random. (48, 49, ..., 63, 154, 155, ...)
> >> If "interrupts" property is used, IRQ resources may be statically
> >> allocated when platform devices are populated from DT.  This can be
> >> a problem for the hierarchy IRQ domain because IRQ allocation must
> >> happen from the outer-most domain up to the root domain in order to
> >> build up the stacked IRQ.  (https://lkml.org/lkml/2017/7/6/758)
> >> Solutions to work around it could be to hard-code parent hwirqs or
> >> to invent a driver-specific DT property.
> >>
> >> Here, the new API irq_domain_push_irq() was merged by v4.14-rc1.
> >> It allows to add irq_data to the existing hierarchy.  It will help
> >> to make this driver work whether the parent has already initialized
> >> the hierarchy or not.
> >>
> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >> ---
> >>
> >> Changes in v4:
> >>   - Add COMPILE_TEST and select IRQ_DOMAIN_HIERARCHY
> >>   - Reimplement irqchip part by using irq_domain_push_irq()
> >>
> >> Changes in v3:
> >>   - Add .irq_set_affinity() hook
> >>   - Use irq_domain_create_hierarchy() instead of legacy
> >>     irq_domain_add_hierarchy()
> >>
> >> Changes in v2:
> >>   - Remove +32 offset for parent interrupts to follow the GIC
> >>     binding convention
> >>   - Let uniphier_gpio_irq_alloc() fail if nr_irqs != 1
> >>   - Allocate gpio_chip statically because just one instance is
> >>     supported
> >>   - Fix suspend and resume hooks
> >>
> >>  .../devicetree/bindings/gpio/gpio-uniphier.txt     |  43 ++
> >
> > What happened to my ack? One line here more that before, but I'm not
> > going to diff your patches for you.
> 
> I changed the binding for this version.
> It includes a new one you have not acked yet.
> 
> Maybe I was too worried about it.

Probably so, but I still don't know what you changed.

Rob

Re: [PATCH v4 6/6] gpio: uniphier: add UniPhier GPIO controller driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

Hi Masahiro,

[auto build test ERROR on tip/irq/core]
[also build test ERROR on next-20170908]
[cannot apply to v4.13]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Masahiro-Yamada/irqdomain-gpio-expand-irq_domain_push_irq-for-DT-use-and-use-it-for-GPIO/20170910-160507
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "of_irq_count" [drivers/gpio/gpio-uniphier.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60564 bytes --]

Re: [PATCH v4 6/6] gpio: uniphier: add UniPhier GPIO controller driver

[Linus Walleij]

On Thu, Sep 7, 2017 at 1:42 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:

> This GPIO controller device is used on UniPhier SoCs.
>
> It also serves as an interrupt controller, but interrupt signals are
> just delivered to the parent irqchip without any latching or OR'ing.
> This is implemented by using hierarchy IRQ domain.
>
> Implementation note:
> Unfortunately, the IRQ mapping from this controller to the parent is
> random. (48, 49, ..., 63, 154, 155, ...)
> If "interrupts" property is used, IRQ resources may be statically
> allocated when platform devices are populated from DT.  This can be
> a problem for the hierarchy IRQ domain because IRQ allocation must
> happen from the outer-most domain up to the root domain in order to
> build up the stacked IRQ.  (https://lkml.org/lkml/2017/7/6/758)
> Solutions to work around it could be to hard-code parent hwirqs or
> to invent a driver-specific DT property.
>
> Here, the new API irq_domain_push_irq() was merged by v4.14-rc1.
> It allows to add irq_data to the existing hierarchy.  It will help
> to make this driver work whether the parent has already initialized
> the hierarchy or not.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v4:
>   - Add COMPILE_TEST and select IRQ_DOMAIN_HIERARCHY
>   - Reimplement irqchip part by using irq_domain_push_irq()

Awesome improvement. There was a build error and I also
would like David Daney to have a look at this so we know we
use things the right way, but overall I am happy after this
so I hope I will be able to apply next version.

Yours,
Linus Walleij

Re: [PATCH v4 6/6] gpio: uniphier: add UniPhier GPIO controller driver

[David Daney]

On 09/12/2017 07:03 AM, Linus Walleij wrote:
> On Thu, Sep 7, 2017 at 1:42 PM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> 
>> This GPIO controller device is used on UniPhier SoCs.
>>
>> It also serves as an interrupt controller, but interrupt signals are
>> just delivered to the parent irqchip without any latching or OR'ing.
>> This is implemented by using hierarchy IRQ domain.
>>
>> Implementation note:
>> Unfortunately, the IRQ mapping from this controller to the parent is
>> random. (48, 49, ..., 63, 154, 155, ...)
>> If "interrupts" property is used, IRQ resources may be statically
>> allocated when platform devices are populated from DT.  This can be
>> a problem for the hierarchy IRQ domain because IRQ allocation must
>> happen from the outer-most domain up to the root domain in order to
>> build up the stacked IRQ.  (https://lkml.org/lkml/2017/7/6/758)
>> Solutions to work around it could be to hard-code parent hwirqs or
>> to invent a driver-specific DT property.
>>
>> Here, the new API irq_domain_push_irq() was merged by v4.14-rc1.
>> It allows to add irq_data to the existing hierarchy.  It will help
>> to make this driver work whether the parent has already initialized
>> the hierarchy or not.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> Changes in v4:
>>    - Add COMPILE_TEST and select IRQ_DOMAIN_HIERARCHY
>>    - Reimplement irqchip part by using irq_domain_push_irq()
> 
> Awesome improvement. There was a build error and I also
> would like David Daney to have a look at this so we know we
> use things the right way,

It looks correct to me.

I haven't verified it, but I think the OF device-tree probing code for 
the platform devices will automatically xlat-and-map all those irqs, so 
that the  irq_domain_push_irq() is required to get the domain hierarchy 
properly configured.  It would be similar to the PCI case where we 
configure all the MSI-X and then do the irq_domain_push_irq() in the 
Cavium ThunderX driver.

If interrupt handling has been verified to work with this driver, I 
would say that we are probably using things "the right way".

David.



> but overall I am happy after this
> so I hope I will be able to apply next version.
> 
> Yours,
> Linus Walleij
> 

Re: [PATCH v4 6/6] gpio: uniphier: add UniPhier GPIO controller driver

[Masahiro Yamada]

Hi.

2017-09-13 0:44 GMT+09:00 David Daney <ddaney@caviumnetworks.com>:
> On 09/12/2017 07:03 AM, Linus Walleij wrote:
>>
>> On Thu, Sep 7, 2017 at 1:42 PM, Masahiro Yamada
>> <yamada.masahiro@socionext.com> wrote:
>>
>>> This GPIO controller device is used on UniPhier SoCs.
>>>
>>> It also serves as an interrupt controller, but interrupt signals are
>>> just delivered to the parent irqchip without any latching or OR'ing.
>>> This is implemented by using hierarchy IRQ domain.
>>>
>>> Implementation note:
>>> Unfortunately, the IRQ mapping from this controller to the parent is
>>> random. (48, 49, ..., 63, 154, 155, ...)
>>> If "interrupts" property is used, IRQ resources may be statically
>>> allocated when platform devices are populated from DT.  This can be
>>> a problem for the hierarchy IRQ domain because IRQ allocation must
>>> happen from the outer-most domain up to the root domain in order to
>>> build up the stacked IRQ.  (https://lkml.org/lkml/2017/7/6/758)
>>> Solutions to work around it could be to hard-code parent hwirqs or
>>> to invent a driver-specific DT property.
>>>
>>> Here, the new API irq_domain_push_irq() was merged by v4.14-rc1.
>>> It allows to add irq_data to the existing hierarchy.  It will help
>>> to make this driver work whether the parent has already initialized
>>> the hierarchy or not.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>> Changes in v4:
>>>    - Add COMPILE_TEST and select IRQ_DOMAIN_HIERARCHY
>>>    - Reimplement irqchip part by using irq_domain_push_irq()
>>
>>
>> Awesome improvement. There was a build error and I also
>> would like David Daney to have a look at this so we know we
>> use things the right way,
>
>
> It looks correct to me.
>
> I haven't verified it, but I think the OF device-tree probing code for the
> platform devices will automatically xlat-and-map all those irqs, so that the
> irq_domain_push_irq() is required to get the domain hierarchy properly
> configured.  It would be similar to the PCI case where we configure all the
> MSI-X and then do the irq_domain_push_irq() in the Cavium ThunderX driver.
>
> If interrupt handling has been verified to work with this driver, I would
> say that we are probably using things "the right way".
>
> David.
>

V4 depends on 5 patches that got negative feedback in irqdomain subsystem.
One more problem for this approach is to virtual IRQs are statically
allocated during the driver probe.
This looks a step backward to me.
Recently, gpio_irqchip migrated to on-the-fly allocation in case some
controllers may have lots of
GPIO lines.


Finally, I came up with another (I think, better) solution.
I think v5 is less controversial,
and works very well in on-the-fly manner.

I am sending it shortly...


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO

[Marc Zyngier]

On 07/09/17 12:41, Masahiro Yamada wrote:
> 
> This series adds a GPIO controller for UniPhier SoC family.
> It also works as an irqchip in hierarchy domain manner.
> 
> My problem is mapping of IRQ from this controller to the parent
> irqchip is not contiguous.
> 
>   IRQ line of GPIO  --->  Parent interrupt
>         0           --->     48
>         1           --->     49
>                 ...
>         15          --->     63
>         16          --->    154
>         17          --->    155
>                 ...
>         20          --->    158
>         21          --->    217
>         22          --->    218
>                 ...
> 
> So, I need to have an array of parent hwirqs somehow.
> 
> Probably, most of people will try to use "interrupts" DT property,
> but I noticed a potential problem for hierarchy IRQ domain.
> If "interrupts" property exists in the device node, IRQ resource
> may be statically allocated when platform devices are populated
> from DT.  I asked this question some time ago:
> https://lkml.org/lkml/2017/7/6/758
> 
> After I tackled this, I decided to put the array in the driver,
> but I could not get a positive response for this.
> The discussion mostly happened in v1 thread:
> http://patchwork.ozlabs.org/patch/797145/
> 
> Recently, the new API irq_domain_push_irq() was merged in the
> mainline.  I thought this might be useful to solve the hierarchy
> domain issue.  Hence, here is a trial.
> 
> I found patch 2 is needed to avoid "type mismatch" error.
> 
> One more thing, I am worried about a race condition.
> 
> I think there is a possibility where a device tries to get IRQ
> after irq_domain_create_hierarchy(), but before irq_domain_push_irq().
> 
> 	priv->domain = irq_domain_create_hierarchy(...)
> 	if (!priv->domain)
> 		return -ENOMEM;
> 
>         [  *** What if a irq consumer device request the irq here? *** ]

We've explicitly forbidden such a use case. There is a (not exactly fool
proof) check in irq_domain_push_irq(), but it is pretty easy to bypass
it. "Don't do it" is the conclusion we reached with David Daney.

If you don't want these interrupts to be requested, you might as well
flag them as IRQ_NOREQUEST, and unflag them when the hierarchy is ready.

Would that work for you?

Thanks,

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

Re: [PATCH v4 0/6] irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO

[Masahiro Yamada]

Hi Marc.

2017-09-07 21:39 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>> I think there is a possibility where a device tries to get IRQ
>> after irq_domain_create_hierarchy(), but before irq_domain_push_irq().
>>
>>       priv->domain = irq_domain_create_hierarchy(...)
>>       if (!priv->domain)
>>               return -ENOMEM;
>>
>>         [  *** What if a irq consumer device request the irq here? *** ]
>
> We've explicitly forbidden such a use case. There is a (not exactly fool
> proof) check in irq_domain_push_irq(), but it is pretty easy to bypass
> it. "Don't do it" is the conclusion we reached with David Daney.
>
> If you don't want these interrupts to be requested, you might as well
> flag them as IRQ_NOREQUEST, and unflag them when the hierarchy is ready.
>
> Would that work for you?


Sorry if my description was unclear.

I do not think IRQ_NOREQUEST is equivalent
to IRQ_DOMAIN_FLAG_NO_CREATE I am trying to add in 5/6.


My intention is to prevent platform_get_irq()
from allocating a new virq.

I think IRQ_NOREQUEST only affects request_irq().



Having said that, this series got negative response
as a whole.

My motivation is to get my GPIO driver (6/6) in
by hook or by crook.
If you do not like this series, please feel free to throw it away.




-- 
Best Regards
Masahiro Yamada