Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

On 10/19/2017 12:57 AM, Gregory Fong wrote:
> Hi Doug,
> 
> On Wed, Oct 04, 2017 at 02:24:37PM -0700, Doug Berger wrote:
>> On 10/03/2017 08:03 PM, Gregory Fong wrote:
>>> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger  wrote:
 The GPIOLIB IRQ chip helpers were very appealing, but badly broke
 the 1:1 mapping between a GPIO controller's device_node and its
 interrupt domain.
>>>
>>> Out of curiosity, what sort of problems have you seen from this?
>>>
>>
>> [snip]
>>
>> When another device-tree node references a GPIO device as its interrupt
>> parent, the irq_create_of_mapping() function looks for the irq domain of
>> the GPIO device and since all bank irq domains reference the same GPIO
>> device node it always resolves to the irq domain of the first bank
>> regardless of which bank the number of the GPIO should resolve. This
>> domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
>> can't be mapped by the device-tree.
> 
> Thanks for clarifying.  This would be great information to include in
> the commit message.
>

Will do.

>>

 This commit consolidates the per bank irq domains to a version
 where we have one larger interrupt domain per GPIO controller
 instance spanning multiple GPIO banks.
>>>
>>> This works (and is reminiscent to my initially submitted
>>> implementation at [1]), but I think it might make sense to keep as-is
>>> (using the gpiolib irqchip helpers), and instead allocate an irqchip
>>> fwnode per bank and use to_of_node() to set it as the of_node for the
>>> gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
>>> might go away...
>>>
>>> Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
>>> says "get rid of this and use gpiochip->parent->of_node everywhere"?
>>> It seems like it would still be beneficial to be able to override the
>>> associated node for a gpiochip, since that's what's used for the
>>> irqdomain, but if that's going away, obviously we don't want to start
>>> using that now.
>>>
>>
>> Yes, this is effectively a reversion to an earlier implementation. I
>> produced an implementation based on the generic irqchip libraries, but
>> that was stripped from this submission when I discovered that no support
>> exists within the generic irqchip libraries for removal of domain
>> generic chips and we wanted to preserve the module support of this driver.
> 
> Considering this is heavily based on my initial implementation (several
> functions are exactly the same), it'd be nice to have some small
> attribution in the commit message. :-)
> 

Yes, I suppose I should have said "a reversion to your earlier
implementation" above :).  I'd be happy to to add your Signed-of-by if
you would like, but you'll have to let me know which email address to
use (the one in the original downstream submission or this one).

>>
>> It is conceivable that the current GPIO device-tree nodes could be
>> broken down into separate devices per bank, but it is believed that this
>> would only confuse things for users of the device as the concept
>> diverges from the concept expressed in device documentation.
> 
> OK, that sounds like a worse alternative.  And since these are all
> actually using the same parent IRQ, it does make sense to keep them all
> in the same IRQ domain.
> 
> 
> On Fri, Sep 29, 2017 at 08:40:56PM -0700, Doug Berger wrote:
>> [snip]
>> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
>> index e2fff559c1ca..752a46ce3589 100644
>> --- a/drivers/gpio/gpio-brcmstb.c
>> +++ b/drivers/gpio/gpio-brcmstb.c
>> [snip]
>> @@ -77,12 +79,18 @@ brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank 
>> *bank)
>>  return status;
>>  }
>>  
>> +static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
>> +struct brcmstb_gpio_bank *bank)
>> +{
>> +return hwirq - (bank->gc.base - bank->parent_priv->gpio_base);
>> +}
>> +
>>  static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
>>  unsigned int offset, bool enable)
>>  {
>>  struct gpio_chip *gc = >gc;
>>  struct brcmstb_gpio_priv *priv = bank->parent_priv;
>> -u32 mask = gc->pin2mask(gc, offset);
>> +u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(offset, bank));
> 
> Consider renaming "offset" to "hwirq".
> 

I could do that, but I was just using the existing argument so now you
are editing yourself ;).  I'll think about it.

>>  u32 imask;
>>  unsigned long flags;
>>  
>> @@ -96,6 +104,17 @@ static void brcmstb_gpio_set_imask(struct 
>> brcmstb_gpio_bank *bank,
>>  spin_unlock_irqrestore(>bgpio_lock, flags);
>>  }
>>  
>> +static int brcmstb_gpio_to_irq(struct gpio_chip *gc, unsigned gc_offset)
>> +{
>> +struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
>> +/* gc_offset is relative to this gpio_chip; want real offset */
>> +int offset = gc_offset + (gc->base - priv->gpio_base);
> 
> Consider renaming 

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

On 10/19/2017 12:57 AM, Gregory Fong wrote:
> Hi Doug,
> 
> On Wed, Oct 04, 2017 at 02:24:37PM -0700, Doug Berger wrote:
>> On 10/03/2017 08:03 PM, Gregory Fong wrote:
>>> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger  wrote:
 The GPIOLIB IRQ chip helpers were very appealing, but badly broke
 the 1:1 mapping between a GPIO controller's device_node and its
 interrupt domain.
>>>
>>> Out of curiosity, what sort of problems have you seen from this?
>>>
>>
>> [snip]
>>
>> When another device-tree node references a GPIO device as its interrupt
>> parent, the irq_create_of_mapping() function looks for the irq domain of
>> the GPIO device and since all bank irq domains reference the same GPIO
>> device node it always resolves to the irq domain of the first bank
>> regardless of which bank the number of the GPIO should resolve. This
>> domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
>> can't be mapped by the device-tree.
> 
> Thanks for clarifying.  This would be great information to include in
> the commit message.
>

Will do.

>>

 This commit consolidates the per bank irq domains to a version
 where we have one larger interrupt domain per GPIO controller
 instance spanning multiple GPIO banks.
>>>
>>> This works (and is reminiscent to my initially submitted
>>> implementation at [1]), but I think it might make sense to keep as-is
>>> (using the gpiolib irqchip helpers), and instead allocate an irqchip
>>> fwnode per bank and use to_of_node() to set it as the of_node for the
>>> gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
>>> might go away...
>>>
>>> Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
>>> says "get rid of this and use gpiochip->parent->of_node everywhere"?
>>> It seems like it would still be beneficial to be able to override the
>>> associated node for a gpiochip, since that's what's used for the
>>> irqdomain, but if that's going away, obviously we don't want to start
>>> using that now.
>>>
>>
>> Yes, this is effectively a reversion to an earlier implementation. I
>> produced an implementation based on the generic irqchip libraries, but
>> that was stripped from this submission when I discovered that no support
>> exists within the generic irqchip libraries for removal of domain
>> generic chips and we wanted to preserve the module support of this driver.
> 
> Considering this is heavily based on my initial implementation (several
> functions are exactly the same), it'd be nice to have some small
> attribution in the commit message. :-)
> 

Yes, I suppose I should have said "a reversion to your earlier
implementation" above :).  I'd be happy to to add your Signed-of-by if
you would like, but you'll have to let me know which email address to
use (the one in the original downstream submission or this one).

>>
>> It is conceivable that the current GPIO device-tree nodes could be
>> broken down into separate devices per bank, but it is believed that this
>> would only confuse things for users of the device as the concept
>> diverges from the concept expressed in device documentation.
> 
> OK, that sounds like a worse alternative.  And since these are all
> actually using the same parent IRQ, it does make sense to keep them all
> in the same IRQ domain.
> 
> 
> On Fri, Sep 29, 2017 at 08:40:56PM -0700, Doug Berger wrote:
>> [snip]
>> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
>> index e2fff559c1ca..752a46ce3589 100644
>> --- a/drivers/gpio/gpio-brcmstb.c
>> +++ b/drivers/gpio/gpio-brcmstb.c
>> [snip]
>> @@ -77,12 +79,18 @@ brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank 
>> *bank)
>>  return status;
>>  }
>>  
>> +static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
>> +struct brcmstb_gpio_bank *bank)
>> +{
>> +return hwirq - (bank->gc.base - bank->parent_priv->gpio_base);
>> +}
>> +
>>  static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
>>  unsigned int offset, bool enable)
>>  {
>>  struct gpio_chip *gc = >gc;
>>  struct brcmstb_gpio_priv *priv = bank->parent_priv;
>> -u32 mask = gc->pin2mask(gc, offset);
>> +u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(offset, bank));
> 
> Consider renaming "offset" to "hwirq".
> 

I could do that, but I was just using the existing argument so now you
are editing yourself ;).  I'll think about it.

>>  u32 imask;
>>  unsigned long flags;
>>  
>> @@ -96,6 +104,17 @@ static void brcmstb_gpio_set_imask(struct 
>> brcmstb_gpio_bank *bank,
>>  spin_unlock_irqrestore(>bgpio_lock, flags);
>>  }
>>  
>> +static int brcmstb_gpio_to_irq(struct gpio_chip *gc, unsigned gc_offset)
>> +{
>> +struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
>> +/* gc_offset is relative to this gpio_chip; want real offset */
>> +int offset = gc_offset + (gc->base - priv->gpio_base);
> 
> Consider renaming "gc_offset" to "offset" and 

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Gregory Fong]

Hi Doug,

On Wed, Oct 04, 2017 at 02:24:37PM -0700, Doug Berger wrote:
> On 10/03/2017 08:03 PM, Gregory Fong wrote:
> > On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger  wrote:
> >> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
> >> the 1:1 mapping between a GPIO controller's device_node and its
> >> interrupt domain.
> > 
> > Out of curiosity, what sort of problems have you seen from this?
> > 
> 
> [snip]
> 
> When another device-tree node references a GPIO device as its interrupt
> parent, the irq_create_of_mapping() function looks for the irq domain of
> the GPIO device and since all bank irq domains reference the same GPIO
> device node it always resolves to the irq domain of the first bank
> regardless of which bank the number of the GPIO should resolve. This
> domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
> can't be mapped by the device-tree.

Thanks for clarifying.  This would be great information to include in
the commit message.

> 
> >>
> >> This commit consolidates the per bank irq domains to a version
> >> where we have one larger interrupt domain per GPIO controller
> >> instance spanning multiple GPIO banks.
> > 
> > This works (and is reminiscent to my initially submitted
> > implementation at [1]), but I think it might make sense to keep as-is
> > (using the gpiolib irqchip helpers), and instead allocate an irqchip
> > fwnode per bank and use to_of_node() to set it as the of_node for the
> > gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
> > might go away...
> > 
> > Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
> > says "get rid of this and use gpiochip->parent->of_node everywhere"?
> > It seems like it would still be beneficial to be able to override the
> > associated node for a gpiochip, since that's what's used for the
> > irqdomain, but if that's going away, obviously we don't want to start
> > using that now.
> > 
> 
> Yes, this is effectively a reversion to an earlier implementation. I
> produced an implementation based on the generic irqchip libraries, but
> that was stripped from this submission when I discovered that no support
> exists within the generic irqchip libraries for removal of domain
> generic chips and we wanted to preserve the module support of this driver.

Considering this is heavily based on my initial implementation (several
functions are exactly the same), it'd be nice to have some small
attribution in the commit message. :-)

> 
> It is conceivable that the current GPIO device-tree nodes could be
> broken down into separate devices per bank, but it is believed that this
> would only confuse things for users of the device as the concept
> diverges from the concept expressed in device documentation.

OK, that sounds like a worse alternative.  And since these are all
actually using the same parent IRQ, it does make sense to keep them all
in the same IRQ domain.


On Fri, Sep 29, 2017 at 08:40:56PM -0700, Doug Berger wrote:
> [snip]
> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
> index e2fff559c1ca..752a46ce3589 100644
> --- a/drivers/gpio/gpio-brcmstb.c
> +++ b/drivers/gpio/gpio-brcmstb.c
> [snip]
> @@ -77,12 +79,18 @@ brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank 
> *bank)
>   return status;
>  }
>  
> +static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
> + struct brcmstb_gpio_bank *bank)
> +{
> + return hwirq - (bank->gc.base - bank->parent_priv->gpio_base);
> +}
> +
>  static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
>   unsigned int offset, bool enable)
>  {
>   struct gpio_chip *gc = >gc;
>   struct brcmstb_gpio_priv *priv = bank->parent_priv;
> - u32 mask = gc->pin2mask(gc, offset);
> + u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(offset, bank));

Consider renaming "offset" to "hwirq".

>   u32 imask;
>   unsigned long flags;
>  
> @@ -96,6 +104,17 @@ static void brcmstb_gpio_set_imask(struct 
> brcmstb_gpio_bank *bank,
>   spin_unlock_irqrestore(>bgpio_lock, flags);
>  }
>  
> +static int brcmstb_gpio_to_irq(struct gpio_chip *gc, unsigned gc_offset)
> +{
> + struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
> + /* gc_offset is relative to this gpio_chip; want real offset */
> + int offset = gc_offset + (gc->base - priv->gpio_base);

Consider renaming "gc_offset" to "offset" and "offset" to "hwirq" to
keep things consistent.

> +
> + if (offset >= priv->num_gpios)
> + return -ENXIO;
> + return irq_create_mapping(priv->irq_domain, offset);
> +}
> +
>  
> [snip]
>
> @@ -226,18 +245,19 @@ static irqreturn_t brcmstb_gpio_wake_irq_handler(int 
> irq, void *data)
>  static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
>  {
>   struct brcmstb_gpio_priv *priv = bank->parent_priv;
> - struct irq_domain *irq_domain = bank->gc.irqdomain;
> + 

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Gregory Fong]

Hi Doug,

On Wed, Oct 04, 2017 at 02:24:37PM -0700, Doug Berger wrote:
> On 10/03/2017 08:03 PM, Gregory Fong wrote:
> > On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger  wrote:
> >> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
> >> the 1:1 mapping between a GPIO controller's device_node and its
> >> interrupt domain.
> > 
> > Out of curiosity, what sort of problems have you seen from this?
> > 
> 
> [snip]
> 
> When another device-tree node references a GPIO device as its interrupt
> parent, the irq_create_of_mapping() function looks for the irq domain of
> the GPIO device and since all bank irq domains reference the same GPIO
> device node it always resolves to the irq domain of the first bank
> regardless of which bank the number of the GPIO should resolve. This
> domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
> can't be mapped by the device-tree.

Thanks for clarifying.  This would be great information to include in
the commit message.

> 
> >>
> >> This commit consolidates the per bank irq domains to a version
> >> where we have one larger interrupt domain per GPIO controller
> >> instance spanning multiple GPIO banks.
> > 
> > This works (and is reminiscent to my initially submitted
> > implementation at [1]), but I think it might make sense to keep as-is
> > (using the gpiolib irqchip helpers), and instead allocate an irqchip
> > fwnode per bank and use to_of_node() to set it as the of_node for the
> > gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
> > might go away...
> > 
> > Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
> > says "get rid of this and use gpiochip->parent->of_node everywhere"?
> > It seems like it would still be beneficial to be able to override the
> > associated node for a gpiochip, since that's what's used for the
> > irqdomain, but if that's going away, obviously we don't want to start
> > using that now.
> > 
> 
> Yes, this is effectively a reversion to an earlier implementation. I
> produced an implementation based on the generic irqchip libraries, but
> that was stripped from this submission when I discovered that no support
> exists within the generic irqchip libraries for removal of domain
> generic chips and we wanted to preserve the module support of this driver.

Considering this is heavily based on my initial implementation (several
functions are exactly the same), it'd be nice to have some small
attribution in the commit message. :-)

> 
> It is conceivable that the current GPIO device-tree nodes could be
> broken down into separate devices per bank, but it is believed that this
> would only confuse things for users of the device as the concept
> diverges from the concept expressed in device documentation.

OK, that sounds like a worse alternative.  And since these are all
actually using the same parent IRQ, it does make sense to keep them all
in the same IRQ domain.


On Fri, Sep 29, 2017 at 08:40:56PM -0700, Doug Berger wrote:
> [snip]
> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
> index e2fff559c1ca..752a46ce3589 100644
> --- a/drivers/gpio/gpio-brcmstb.c
> +++ b/drivers/gpio/gpio-brcmstb.c
> [snip]
> @@ -77,12 +79,18 @@ brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank 
> *bank)
>   return status;
>  }
>  
> +static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
> + struct brcmstb_gpio_bank *bank)
> +{
> + return hwirq - (bank->gc.base - bank->parent_priv->gpio_base);
> +}
> +
>  static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
>   unsigned int offset, bool enable)
>  {
>   struct gpio_chip *gc = >gc;
>   struct brcmstb_gpio_priv *priv = bank->parent_priv;
> - u32 mask = gc->pin2mask(gc, offset);
> + u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(offset, bank));

Consider renaming "offset" to "hwirq".

>   u32 imask;
>   unsigned long flags;
>  
> @@ -96,6 +104,17 @@ static void brcmstb_gpio_set_imask(struct 
> brcmstb_gpio_bank *bank,
>   spin_unlock_irqrestore(>bgpio_lock, flags);
>  }
>  
> +static int brcmstb_gpio_to_irq(struct gpio_chip *gc, unsigned gc_offset)
> +{
> + struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
> + /* gc_offset is relative to this gpio_chip; want real offset */
> + int offset = gc_offset + (gc->base - priv->gpio_base);

Consider renaming "gc_offset" to "offset" and "offset" to "hwirq" to
keep things consistent.

> +
> + if (offset >= priv->num_gpios)
> + return -ENXIO;
> + return irq_create_mapping(priv->irq_domain, offset);
> +}
> +
>  
> [snip]
>
> @@ -226,18 +245,19 @@ static irqreturn_t brcmstb_gpio_wake_irq_handler(int 
> irq, void *data)
>  static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
>  {
>   struct brcmstb_gpio_priv *priv = bank->parent_priv;
> - struct irq_domain *irq_domain = bank->gc.irqdomain;
> + struct 

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

On 10/04/2017 02:24 PM, Doug Berger wrote:
> On 10/03/2017 08:03 PM, Gregory Fong wrote:
>> Hi Doug,
>>
>> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger  wrote:
>>> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
>>> the 1:1 mapping between a GPIO controller's device_node and its
>>> interrupt domain.
>>
>> Out of curiosity, what sort of problems have you seen from this?
>>
> 
> As you know, the BRCMSTB devices conceptually distinguish between an
> always-on GPIO device and a regular GPIO device that each can have many
> more than 32 General Purpose I/Os. The driver supports these by dividing
> the GPIO across a number of banks each of which is implemented as a
> separate gpiochip as an implementation convenience. The main issue is
> that each gpiochip that uses the GPIOLIB IRQ chip helpers creates its
> own irq domain even though they are associated with the same device and
> device-tree node.
> 
> When another device-tree node references a GPIO device as its interrupt
> parent, the irq_create_of_mapping() function looks for the irq domain of
> the GPIO device and since all bank irq domains reference the same GPIO
> device node it always resolves to the irq domain of the first bank
> regardless of which bank the number of the GPIO should resolve. This
> domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
> can't be mapped by the device-tree.
> 
>>>
>>> This commit consolidates the per bank irq domains to a version
>>> where we have one larger interrupt domain per GPIO controller
>>> instance spanning multiple GPIO banks.
>>
>> This works (and is reminiscent to my initially submitted
>> implementation at [1]), but I think it might make sense to keep as-is
>> (using the gpiolib irqchip helpers), and instead allocate an irqchip
>> fwnode per bank and use to_of_node() to set it as the of_node for the
>> gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
>> might go away...
>>
>> Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
>> says "get rid of this and use gpiochip->parent->of_node everywhere"?
>> It seems like it would still be beneficial to be able to override the
>> associated node for a gpiochip, since that's what's used for the
>> irqdomain, but if that's going away, obviously we don't want to start
>> using that now.
>>
> 
> Yes, this is effectively a reversion to an earlier implementation. I
> produced an implementation based on the generic irqchip libraries, but
> that was stripped from this submission when I discovered that no support
> exists within the generic irqchip libraries for removal of domain
> generic chips and we wanted to preserve the module support of this driver.
> 
> It is conceivable that the current GPIO device-tree nodes could be
> broken down into separate devices per bank, but it is believed that this
> would only confuse things for users of the device as the concept
> diverges from the concept expressed in device documentation.
> 
>> Thanks,
>> Gregory
>>
>> [1] https://patchwork.kernel.org/patch/6347811/
>>
> 
> Thanks for the review,
> Doug
> 

Gregory,

Do you have any additional feedback on patches 6 and 7 before I submit a
version 2?

Thanks,
Doug

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

On 10/04/2017 02:24 PM, Doug Berger wrote:
> On 10/03/2017 08:03 PM, Gregory Fong wrote:
>> Hi Doug,
>>
>> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger  wrote:
>>> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
>>> the 1:1 mapping between a GPIO controller's device_node and its
>>> interrupt domain.
>>
>> Out of curiosity, what sort of problems have you seen from this?
>>
> 
> As you know, the BRCMSTB devices conceptually distinguish between an
> always-on GPIO device and a regular GPIO device that each can have many
> more than 32 General Purpose I/Os. The driver supports these by dividing
> the GPIO across a number of banks each of which is implemented as a
> separate gpiochip as an implementation convenience. The main issue is
> that each gpiochip that uses the GPIOLIB IRQ chip helpers creates its
> own irq domain even though they are associated with the same device and
> device-tree node.
> 
> When another device-tree node references a GPIO device as its interrupt
> parent, the irq_create_of_mapping() function looks for the irq domain of
> the GPIO device and since all bank irq domains reference the same GPIO
> device node it always resolves to the irq domain of the first bank
> regardless of which bank the number of the GPIO should resolve. This
> domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
> can't be mapped by the device-tree.
> 
>>>
>>> This commit consolidates the per bank irq domains to a version
>>> where we have one larger interrupt domain per GPIO controller
>>> instance spanning multiple GPIO banks.
>>
>> This works (and is reminiscent to my initially submitted
>> implementation at [1]), but I think it might make sense to keep as-is
>> (using the gpiolib irqchip helpers), and instead allocate an irqchip
>> fwnode per bank and use to_of_node() to set it as the of_node for the
>> gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
>> might go away...
>>
>> Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
>> says "get rid of this and use gpiochip->parent->of_node everywhere"?
>> It seems like it would still be beneficial to be able to override the
>> associated node for a gpiochip, since that's what's used for the
>> irqdomain, but if that's going away, obviously we don't want to start
>> using that now.
>>
> 
> Yes, this is effectively a reversion to an earlier implementation. I
> produced an implementation based on the generic irqchip libraries, but
> that was stripped from this submission when I discovered that no support
> exists within the generic irqchip libraries for removal of domain
> generic chips and we wanted to preserve the module support of this driver.
> 
> It is conceivable that the current GPIO device-tree nodes could be
> broken down into separate devices per bank, but it is believed that this
> would only confuse things for users of the device as the concept
> diverges from the concept expressed in device documentation.
> 
>> Thanks,
>> Gregory
>>
>> [1] https://patchwork.kernel.org/patch/6347811/
>>
> 
> Thanks for the review,
> Doug
> 

Gregory,

Do you have any additional feedback on patches 6 and 7 before I submit a
version 2?

Thanks,
Doug

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

On 10/03/2017 08:03 PM, Gregory Fong wrote:
> Hi Doug,
> 
> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger  wrote:
>> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
>> the 1:1 mapping between a GPIO controller's device_node and its
>> interrupt domain.
> 
> Out of curiosity, what sort of problems have you seen from this?
> 

As you know, the BRCMSTB devices conceptually distinguish between an
always-on GPIO device and a regular GPIO device that each can have many
more than 32 General Purpose I/Os. The driver supports these by dividing
the GPIO across a number of banks each of which is implemented as a
separate gpiochip as an implementation convenience. The main issue is
that each gpiochip that uses the GPIOLIB IRQ chip helpers creates its
own irq domain even though they are associated with the same device and
device-tree node.

When another device-tree node references a GPIO device as its interrupt
parent, the irq_create_of_mapping() function looks for the irq domain of
the GPIO device and since all bank irq domains reference the same GPIO
device node it always resolves to the irq domain of the first bank
regardless of which bank the number of the GPIO should resolve. This
domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
can't be mapped by the device-tree.

>>
>> This commit consolidates the per bank irq domains to a version
>> where we have one larger interrupt domain per GPIO controller
>> instance spanning multiple GPIO banks.
> 
> This works (and is reminiscent to my initially submitted
> implementation at [1]), but I think it might make sense to keep as-is
> (using the gpiolib irqchip helpers), and instead allocate an irqchip
> fwnode per bank and use to_of_node() to set it as the of_node for the
> gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
> might go away...
> 
> Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
> says "get rid of this and use gpiochip->parent->of_node everywhere"?
> It seems like it would still be beneficial to be able to override the
> associated node for a gpiochip, since that's what's used for the
> irqdomain, but if that's going away, obviously we don't want to start
> using that now.
> 

Yes, this is effectively a reversion to an earlier implementation. I
produced an implementation based on the generic irqchip libraries, but
that was stripped from this submission when I discovered that no support
exists within the generic irqchip libraries for removal of domain
generic chips and we wanted to preserve the module support of this driver.

It is conceivable that the current GPIO device-tree nodes could be
broken down into separate devices per bank, but it is believed that this
would only confuse things for users of the device as the concept
diverges from the concept expressed in device documentation.

> Thanks,
> Gregory
> 
> [1] https://patchwork.kernel.org/patch/6347811/
> 

Thanks for the review,
Doug

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

On 10/03/2017 08:03 PM, Gregory Fong wrote:
> Hi Doug,
> 
> On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger  wrote:
>> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
>> the 1:1 mapping between a GPIO controller's device_node and its
>> interrupt domain.
> 
> Out of curiosity, what sort of problems have you seen from this?
> 

As you know, the BRCMSTB devices conceptually distinguish between an
always-on GPIO device and a regular GPIO device that each can have many
more than 32 General Purpose I/Os. The driver supports these by dividing
the GPIO across a number of banks each of which is implemented as a
separate gpiochip as an implementation convenience. The main issue is
that each gpiochip that uses the GPIOLIB IRQ chip helpers creates its
own irq domain even though they are associated with the same device and
device-tree node.

When another device-tree node references a GPIO device as its interrupt
parent, the irq_create_of_mapping() function looks for the irq domain of
the GPIO device and since all bank irq domains reference the same GPIO
device node it always resolves to the irq domain of the first bank
regardless of which bank the number of the GPIO should resolve. This
domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
can't be mapped by the device-tree.

>>
>> This commit consolidates the per bank irq domains to a version
>> where we have one larger interrupt domain per GPIO controller
>> instance spanning multiple GPIO banks.
> 
> This works (and is reminiscent to my initially submitted
> implementation at [1]), but I think it might make sense to keep as-is
> (using the gpiolib irqchip helpers), and instead allocate an irqchip
> fwnode per bank and use to_of_node() to set it as the of_node for the
> gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
> might go away...
> 
> Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
> says "get rid of this and use gpiochip->parent->of_node everywhere"?
> It seems like it would still be beneficial to be able to override the
> associated node for a gpiochip, since that's what's used for the
> irqdomain, but if that's going away, obviously we don't want to start
> using that now.
> 

Yes, this is effectively a reversion to an earlier implementation. I
produced an implementation based on the generic irqchip libraries, but
that was stripped from this submission when I discovered that no support
exists within the generic irqchip libraries for removal of domain
generic chips and we wanted to preserve the module support of this driver.

It is conceivable that the current GPIO device-tree nodes could be
broken down into separate devices per bank, but it is believed that this
would only confuse things for users of the device as the concept
diverges from the concept expressed in device documentation.

> Thanks,
> Gregory
> 
> [1] https://patchwork.kernel.org/patch/6347811/
> 

Thanks for the review,
Doug

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Gregory Fong]

Hi Doug,

On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger  wrote:
> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
> the 1:1 mapping between a GPIO controller's device_node and its
> interrupt domain.

Out of curiosity, what sort of problems have you seen from this?

>
> This commit consolidates the per bank irq domains to a version
> where we have one larger interrupt domain per GPIO controller
> instance spanning multiple GPIO banks.

This works (and is reminiscent to my initially submitted
implementation at [1]), but I think it might make sense to keep as-is
(using the gpiolib irqchip helpers), and instead allocate an irqchip
fwnode per bank and use to_of_node() to set it as the of_node for the
gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
might go away...

Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
says "get rid of this and use gpiochip->parent->of_node everywhere"?
It seems like it would still be beneficial to be able to override the
associated node for a gpiochip, since that's what's used for the
irqdomain, but if that's going away, obviously we don't want to start
using that now.

Thanks,
Gregory

[1] https://patchwork.kernel.org/patch/6347811/

Re: [PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Gregory Fong]

Hi Doug,

On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger  wrote:
> The GPIOLIB IRQ chip helpers were very appealing, but badly broke
> the 1:1 mapping between a GPIO controller's device_node and its
> interrupt domain.

Out of curiosity, what sort of problems have you seen from this?

>
> This commit consolidates the per bank irq domains to a version
> where we have one larger interrupt domain per GPIO controller
> instance spanning multiple GPIO banks.

This works (and is reminiscent to my initially submitted
implementation at [1]), but I think it might make sense to keep as-is
(using the gpiolib irqchip helpers), and instead allocate an irqchip
fwnode per bank and use to_of_node() to set it as the of_node for the
gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
might go away...

Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
says "get rid of this and use gpiochip->parent->of_node everywhere"?
It seems like it would still be beneficial to be able to override the
associated node for a gpiochip, since that's what's used for the
irqdomain, but if that's going away, obviously we don't want to start
using that now.

Thanks,
Gregory

[1] https://patchwork.kernel.org/patch/6347811/

[PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

The GPIOLIB IRQ chip helpers were very appealing, but badly broke
the 1:1 mapping between a GPIO controller's device_node and its
interrupt domain.

This commit consolidates the per bank irq domains to a version
where we have one larger interrupt domain per GPIO controller
instance spanning multiple GPIO banks.

Fixes: 19a7b6940b78 ("gpio: brcmstb: Add interrupt and wakeup source support")
Signed-off-by: Doug Berger 
---
 drivers/gpio/Kconfig|   2 +-
 drivers/gpio/gpio-brcmstb.c | 188 +---
 2 files changed, 145 insertions(+), 45 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 796b11c489ae..28dd05aaa408 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -139,7 +139,7 @@ config GPIO_BRCMSTB
default y if (ARCH_BRCMSTB || BMIPS_GENERIC)
depends on OF_GPIO && (ARCH_BRCMSTB || BMIPS_GENERIC || COMPILE_TEST)
select GPIO_GENERIC
-   select GPIOLIB_IRQCHIP
+   select IRQ_DOMAIN
help
  Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs.
 
diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index e2fff559c1ca..752a46ce3589 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -37,20 +37,22 @@ struct brcmstb_gpio_bank {
struct gpio_chip gc;
struct brcmstb_gpio_priv *parent_priv;
u32 width;
-   struct irq_chip irq_chip;
 };
 
 struct brcmstb_gpio_priv {
struct list_head bank_list;
void __iomem *reg_base;
struct platform_device *pdev;
+   struct irq_domain *irq_domain;
+   struct irq_chip irq_chip;
int parent_irq;
int gpio_base;
+   int num_gpios;
int parent_wake_irq;
struct notifier_block reboot_notifier;
 };
 
-#define MAX_GPIO_PER_BANK   32
+#define MAX_GPIO_PER_BANK   32
 #define GPIO_BANK(gpio) ((gpio) >> 5)
 /* assumes MAX_GPIO_PER_BANK is a multiple of 2 */
 #define GPIO_BIT(gpio)  ((gpio) & (MAX_GPIO_PER_BANK - 1))
@@ -77,12 +79,18 @@ brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
return status;
 }
 
+static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
+   struct brcmstb_gpio_bank *bank)
+{
+   return hwirq - (bank->gc.base - bank->parent_priv->gpio_base);
+}
+
 static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
unsigned int offset, bool enable)
 {
struct gpio_chip *gc = >gc;
struct brcmstb_gpio_priv *priv = bank->parent_priv;
-   u32 mask = gc->pin2mask(gc, offset);
+   u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(offset, bank));
u32 imask;
unsigned long flags;
 
@@ -96,6 +104,17 @@ static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank 
*bank,
spin_unlock_irqrestore(>bgpio_lock, flags);
 }
 
+static int brcmstb_gpio_to_irq(struct gpio_chip *gc, unsigned gc_offset)
+{
+   struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
+   /* gc_offset is relative to this gpio_chip; want real offset */
+   int offset = gc_offset + (gc->base - priv->gpio_base);
+
+   if (offset >= priv->num_gpios)
+   return -ENXIO;
+   return irq_create_mapping(priv->irq_domain, offset);
+}
+
 /*  IRQ chip functions  */
 
 static void brcmstb_gpio_irq_mask(struct irq_data *d)
@@ -119,7 +138,7 @@ static void brcmstb_gpio_irq_ack(struct irq_data *d)
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
struct brcmstb_gpio_priv *priv = bank->parent_priv;
-   u32 mask = BIT(d->hwirq);
+   u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
 
gc->write_reg(priv->reg_base + GIO_STAT(bank->id), mask);
 }
@@ -129,7 +148,7 @@ static int brcmstb_gpio_irq_set_type(struct irq_data *d, 
unsigned int type)
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
struct brcmstb_gpio_priv *priv = bank->parent_priv;
-   u32 mask = BIT(d->hwirq);
+   u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
u32 edge_insensitive, iedge_insensitive;
u32 edge_config, iedge_config;
u32 level, ilevel;
@@ -226,18 +245,19 @@ static irqreturn_t brcmstb_gpio_wake_irq_handler(int irq, 
void *data)
 static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
 {
struct brcmstb_gpio_priv *priv = bank->parent_priv;
-   struct irq_domain *irq_domain = bank->gc.irqdomain;
+   struct irq_domain *domain = priv->irq_domain;
+   int hwbase = bank->gc.base - priv->gpio_base;
unsigned long status;
+   unsigned int irq;
 
while ((status = brcmstb_gpio_get_active_irqs(bank))) {
-   int bit;
-
-   for_each_set_bit(bit, , 

[PATCH 6/7] gpio: brcmstb: consolidate interrupt domains

[Doug Berger]

The GPIOLIB IRQ chip helpers were very appealing, but badly broke
the 1:1 mapping between a GPIO controller's device_node and its
interrupt domain.

This commit consolidates the per bank irq domains to a version
where we have one larger interrupt domain per GPIO controller
instance spanning multiple GPIO banks.

Fixes: 19a7b6940b78 ("gpio: brcmstb: Add interrupt and wakeup source support")
Signed-off-by: Doug Berger 
---
 drivers/gpio/Kconfig|   2 +-
 drivers/gpio/gpio-brcmstb.c | 188 +---
 2 files changed, 145 insertions(+), 45 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 796b11c489ae..28dd05aaa408 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -139,7 +139,7 @@ config GPIO_BRCMSTB
default y if (ARCH_BRCMSTB || BMIPS_GENERIC)
depends on OF_GPIO && (ARCH_BRCMSTB || BMIPS_GENERIC || COMPILE_TEST)
select GPIO_GENERIC
-   select GPIOLIB_IRQCHIP
+   select IRQ_DOMAIN
help
  Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs.
 
diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index e2fff559c1ca..752a46ce3589 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -37,20 +37,22 @@ struct brcmstb_gpio_bank {
struct gpio_chip gc;
struct brcmstb_gpio_priv *parent_priv;
u32 width;
-   struct irq_chip irq_chip;
 };
 
 struct brcmstb_gpio_priv {
struct list_head bank_list;
void __iomem *reg_base;
struct platform_device *pdev;
+   struct irq_domain *irq_domain;
+   struct irq_chip irq_chip;
int parent_irq;
int gpio_base;
+   int num_gpios;
int parent_wake_irq;
struct notifier_block reboot_notifier;
 };
 
-#define MAX_GPIO_PER_BANK   32
+#define MAX_GPIO_PER_BANK   32
 #define GPIO_BANK(gpio) ((gpio) >> 5)
 /* assumes MAX_GPIO_PER_BANK is a multiple of 2 */
 #define GPIO_BIT(gpio)  ((gpio) & (MAX_GPIO_PER_BANK - 1))
@@ -77,12 +79,18 @@ brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
return status;
 }
 
+static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
+   struct brcmstb_gpio_bank *bank)
+{
+   return hwirq - (bank->gc.base - bank->parent_priv->gpio_base);
+}
+
 static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
unsigned int offset, bool enable)
 {
struct gpio_chip *gc = >gc;
struct brcmstb_gpio_priv *priv = bank->parent_priv;
-   u32 mask = gc->pin2mask(gc, offset);
+   u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(offset, bank));
u32 imask;
unsigned long flags;
 
@@ -96,6 +104,17 @@ static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank 
*bank,
spin_unlock_irqrestore(>bgpio_lock, flags);
 }
 
+static int brcmstb_gpio_to_irq(struct gpio_chip *gc, unsigned gc_offset)
+{
+   struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc);
+   /* gc_offset is relative to this gpio_chip; want real offset */
+   int offset = gc_offset + (gc->base - priv->gpio_base);
+
+   if (offset >= priv->num_gpios)
+   return -ENXIO;
+   return irq_create_mapping(priv->irq_domain, offset);
+}
+
 /*  IRQ chip functions  */
 
 static void brcmstb_gpio_irq_mask(struct irq_data *d)
@@ -119,7 +138,7 @@ static void brcmstb_gpio_irq_ack(struct irq_data *d)
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
struct brcmstb_gpio_priv *priv = bank->parent_priv;
-   u32 mask = BIT(d->hwirq);
+   u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
 
gc->write_reg(priv->reg_base + GIO_STAT(bank->id), mask);
 }
@@ -129,7 +148,7 @@ static int brcmstb_gpio_irq_set_type(struct irq_data *d, 
unsigned int type)
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct brcmstb_gpio_bank *bank = gpiochip_get_data(gc);
struct brcmstb_gpio_priv *priv = bank->parent_priv;
-   u32 mask = BIT(d->hwirq);
+   u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
u32 edge_insensitive, iedge_insensitive;
u32 edge_config, iedge_config;
u32 level, ilevel;
@@ -226,18 +245,19 @@ static irqreturn_t brcmstb_gpio_wake_irq_handler(int irq, 
void *data)
 static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
 {
struct brcmstb_gpio_priv *priv = bank->parent_priv;
-   struct irq_domain *irq_domain = bank->gc.irqdomain;
+   struct irq_domain *domain = priv->irq_domain;
+   int hwbase = bank->gc.base - priv->gpio_base;
unsigned long status;
+   unsigned int irq;
 
while ((status = brcmstb_gpio_get_active_irqs(bank))) {
-   int bit;
-
-   for_each_set_bit(bit, , 32) {
-