[v2,05/11] mfd: pm8xxx: disassociate old virq if hwirq mapping already exists
diff mbox series

Message ID 20190208021631.30252-6-masneyb@onstation.org
State New
Headers show
Series
  • qcom: ssbi-gpio: add support for hierarchical IRQ chip
Related show

Commit Message

Brian Masney Feb. 8, 2019, 2:16 a.m. UTC
Check to see if the hwirq is already associated with another virq on
this IRQ domain. If so, then disassociate it before associating the
hwirq with the new virq.

This is a temporary hack that is needed in order to not break git
bisect for existing boards. The next patch in this series converts
ssbi-gpio to be a hierarchical IRQ chip, then there are several patches
to update all of the device tree files, and finally this patch will be
reverted within the same patch series.

IRQs for ssbi-gpio are all initially setup without an IRQ hierarchy
this driver is probed due to the interrupts property in device tree.
Once ssbi-gpio is converted to be a hierarchical IRQ chip in the next
patch, existing users of gpio[d]_to_irq() will call pmic_gpio_to_irq(),
and that will use the new IRQ chip code in ssbi-gpio that sets up the
IRQ in an IRQ hierarchy. The hwirq is now associated with two Linux
virqs and interrupts will not work as expected. This patch corrects
that issue.

This change was tested on an APQ8060 DragonBoard.

Signed-off-by: Brian Masney <masneyb@onstation.org>
Tested-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes since v1:
- None

 drivers/mfd/qcom-pm8xxx.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Lee Jones Feb. 12, 2019, 8:39 a.m. UTC | #1
On Thu, 07 Feb 2019, Brian Masney wrote:

> Check to see if the hwirq is already associated with another virq on
> this IRQ domain. If so, then disassociate it before associating the
> hwirq with the new virq.
> 
> This is a temporary hack that is needed in order to not break git
> bisect for existing boards. The next patch in this series converts
> ssbi-gpio to be a hierarchical IRQ chip, then there are several patches
> to update all of the device tree files, and finally this patch will be
> reverted within the same patch series.
> 
> IRQs for ssbi-gpio are all initially setup without an IRQ hierarchy
> this driver is probed due to the interrupts property in device tree.
> Once ssbi-gpio is converted to be a hierarchical IRQ chip in the next
> patch, existing users of gpio[d]_to_irq() will call pmic_gpio_to_irq(),
> and that will use the new IRQ chip code in ssbi-gpio that sets up the
> IRQ in an IRQ hierarchy. The hwirq is now associated with two Linux
> virqs and interrupts will not work as expected. This patch corrects
> that issue.
> 
> This change was tested on an APQ8060 DragonBoard.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes since v1:
> - None
> 
>  drivers/mfd/qcom-pm8xxx.c | 6 ++++++
>  1 file changed, 6 insertions(+)

For Linus:
  
  Acked-by: Lee Jones <lee.jones@linaro.org>
Stephen Boyd Feb. 15, 2019, 5:51 a.m. UTC | #2
Quoting Brian Masney (2019-02-07 18:16:25)
> Check to see if the hwirq is already associated with another virq on
> this IRQ domain. If so, then disassociate it before associating the
> hwirq with the new virq.
> 
> This is a temporary hack that is needed in order to not break git
> bisect for existing boards. The next patch in this series converts
> ssbi-gpio to be a hierarchical IRQ chip, then there are several patches
> to update all of the device tree files, and finally this patch will be
> reverted within the same patch series.
> 
> IRQs for ssbi-gpio are all initially setup without an IRQ hierarchy
> this driver is probed due to the interrupts property in device tree.
> Once ssbi-gpio is converted to be a hierarchical IRQ chip in the next
> patch, existing users of gpio[d]_to_irq() will call pmic_gpio_to_irq(),
> and that will use the new IRQ chip code in ssbi-gpio that sets up the
> IRQ in an IRQ hierarchy. The hwirq is now associated with two Linux
> virqs and interrupts will not work as expected. This patch corrects
> that issue.
> 
> This change was tested on an APQ8060 DragonBoard.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> Tested-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes since v1:
> - None
> 
>  drivers/mfd/qcom-pm8xxx.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> index 8eb2528793f9..2f99a98ccee5 100644
> --- a/drivers/mfd/qcom-pm8xxx.c
> +++ b/drivers/mfd/qcom-pm8xxx.c
> @@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
>                                   struct irq_domain *domain, unsigned int irq,
>                                   irq_hw_number_t hwirq, unsigned int type)
>  {
> +       unsigned int old_virq;
> +
> +       old_virq = irq_find_mapping(domain, hwirq);
> +       if (old_virq)
> +               irq_domain_disassociate(domain, old_virq);

Is it possible to pass 'true' for the 'realloc' argument to
__irq_domain_alloc_irqs() and then this disassociate change isn't
needed?

> +
>         irq_domain_set_info(domain, irq, hwirq, chip->pm_irq_data->irq_chip,
>                             chip, handle_level_irq, NULL, NULL);
>         irq_set_noprobe(irq);
Brian Masney Feb. 15, 2019, 1:47 p.m. UTC | #3
On Thu, Feb 14, 2019 at 09:51:26PM -0800, Stephen Boyd wrote:
> > diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> > index 8eb2528793f9..2f99a98ccee5 100644
> > --- a/drivers/mfd/qcom-pm8xxx.c
> > +++ b/drivers/mfd/qcom-pm8xxx.c
> > @@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
> >                                   struct irq_domain *domain, unsigned int irq,
> >                                   irq_hw_number_t hwirq, unsigned int type)
> >  {
> > +       unsigned int old_virq;
> > +
> > +       old_virq = irq_find_mapping(domain, hwirq);
> > +       if (old_virq)
> > +               irq_domain_disassociate(domain, old_virq);
> 
> Is it possible to pass 'true' for the 'realloc' argument to
> __irq_domain_alloc_irqs() and then this disassociate change isn't
> needed?

The kernel doc for __irq_domain_alloc_irqs() says that the realloc
parameter is mainly to support legacy IRQs. I don't think its a good
idea to add new code that'll stay past the end of this patch series
on top of that legacy interface.

Brian
Stephen Boyd Feb. 15, 2019, 9:28 p.m. UTC | #4
Quoting Brian Masney (2019-02-15 05:47:33)
> On Thu, Feb 14, 2019 at 09:51:26PM -0800, Stephen Boyd wrote:
> > > diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> > > index 8eb2528793f9..2f99a98ccee5 100644
> > > --- a/drivers/mfd/qcom-pm8xxx.c
> > > +++ b/drivers/mfd/qcom-pm8xxx.c
> > > @@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
> > >                                   struct irq_domain *domain, unsigned int irq,
> > >                                   irq_hw_number_t hwirq, unsigned int type)
> > >  {
> > > +       unsigned int old_virq;
> > > +
> > > +       old_virq = irq_find_mapping(domain, hwirq);
> > > +       if (old_virq)
> > > +               irq_domain_disassociate(domain, old_virq);
> > 
> > Is it possible to pass 'true' for the 'realloc' argument to
> > __irq_domain_alloc_irqs() and then this disassociate change isn't
> > needed?
> 
> The kernel doc for __irq_domain_alloc_irqs() says that the realloc
> parameter is mainly to support legacy IRQs. I don't think its a good
> idea to add new code that'll stay past the end of this patch series
> on top of that legacy interface.
> 

Ok. The other side of the argument is that this is the only user of
irq_domain_disassociate(), which may also be some sort of legacy
interface that isn't supposed to be used. Looking at the commit that
exposed it, it seems to be that it's there for legacy reasons.

commit 43a775916d63d1c822107b39987192ca5ced445c
Author: Jiang Liu <jiang.liu@linux.intel.com>
Date:   Mon Jun 9 16:20:05 2014 +0800

    genirq: Export irq_domain_disassociate() to architecture interrupt drivers
    
    Export irq_domain_disassociate() to architecture interrupt drivers,
    so it could be used to handle legacy IRQ descriptors on x86.

So maybe we should just use the realloc argument and bury the
disassociate API in irqdomain.c because it's not supposed to be used?
Or does the realloc path not work for some reason?
Brian Masney Feb. 16, 2019, 12:23 a.m. UTC | #5
On Fri, Feb 15, 2019 at 01:28:02PM -0800, Stephen Boyd wrote:
> Quoting Brian Masney (2019-02-15 05:47:33)
> > On Thu, Feb 14, 2019 at 09:51:26PM -0800, Stephen Boyd wrote:
> > > > diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> > > > index 8eb2528793f9..2f99a98ccee5 100644
> > > > --- a/drivers/mfd/qcom-pm8xxx.c
> > > > +++ b/drivers/mfd/qcom-pm8xxx.c
> > > > @@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
> > > >                                   struct irq_domain *domain, unsigned int irq,
> > > >                                   irq_hw_number_t hwirq, unsigned int type)
> > > >  {
> > > > +       unsigned int old_virq;
> > > > +
> > > > +       old_virq = irq_find_mapping(domain, hwirq);
> > > > +       if (old_virq)
> > > > +               irq_domain_disassociate(domain, old_virq);
> > > 
> > > Is it possible to pass 'true' for the 'realloc' argument to
> > > __irq_domain_alloc_irqs() and then this disassociate change isn't
> > > needed?
> > 
> > The kernel doc for __irq_domain_alloc_irqs() says that the realloc
> > parameter is mainly to support legacy IRQs. I don't think its a good
> > idea to add new code that'll stay past the end of this patch series
> > on top of that legacy interface.
> > 
> 
> Ok. The other side of the argument is that this is the only user of
> irq_domain_disassociate(), which may also be some sort of legacy
> interface that isn't supposed to be used. Looking at the commit that
> exposed it, it seems to be that it's there for legacy reasons.
> 
> commit 43a775916d63d1c822107b39987192ca5ced445c
> Author: Jiang Liu <jiang.liu@linux.intel.com>
> Date:   Mon Jun 9 16:20:05 2014 +0800
> 
>     genirq: Export irq_domain_disassociate() to architecture interrupt drivers
>     
>     Export irq_domain_disassociate() to architecture interrupt drivers,
>     so it could be used to handle legacy IRQ descriptors on x86.
> 
> So maybe we should just use the realloc argument and bury the
> disassociate API in irqdomain.c because it's not supposed to be used?
> Or does the realloc path not work for some reason?

I haven't tried the realloc path yet. Let's back up further so that we
are both starting with the same assumptions. Do you want to keep either
your proposed change (realloc argument) or the existing
irq_domain_disassociate change in mainline past the end of this patch
series? If it is going to continue to be a temporary shim that will be
reverted at the end of the patch series (like I did here), then I don't
see the point in this extra work since this patch is only here to keep
it bisectable and works. We're not using any legacy interfaces by the
end of this patch series.

Brian
Stephen Boyd Feb. 21, 2019, 6:47 p.m. UTC | #6
Quoting Brian Masney (2019-02-15 16:23:59)
> On Fri, Feb 15, 2019 at 01:28:02PM -0800, Stephen Boyd wrote:
> > Quoting Brian Masney (2019-02-15 05:47:33)
> > > On Thu, Feb 14, 2019 at 09:51:26PM -0800, Stephen Boyd wrote:
> > > > > diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> > > > > index 8eb2528793f9..2f99a98ccee5 100644
> > > > > --- a/drivers/mfd/qcom-pm8xxx.c
> > > > > +++ b/drivers/mfd/qcom-pm8xxx.c
> > > > > @@ -380,6 +380,12 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
> > > > >                                   struct irq_domain *domain, unsigned int irq,
> > > > >                                   irq_hw_number_t hwirq, unsigned int type)
> > > > >  {
> > > > > +       unsigned int old_virq;
> > > > > +
> > > > > +       old_virq = irq_find_mapping(domain, hwirq);
> > > > > +       if (old_virq)
> > > > > +               irq_domain_disassociate(domain, old_virq);
> > > > 
> > > > Is it possible to pass 'true' for the 'realloc' argument to
> > > > __irq_domain_alloc_irqs() and then this disassociate change isn't
> > > > needed?
> > > 
> > > The kernel doc for __irq_domain_alloc_irqs() says that the realloc
> > > parameter is mainly to support legacy IRQs. I don't think its a good
> > > idea to add new code that'll stay past the end of this patch series
> > > on top of that legacy interface.
> > > 
> > 
> > Ok. The other side of the argument is that this is the only user of
> > irq_domain_disassociate(), which may also be some sort of legacy
> > interface that isn't supposed to be used. Looking at the commit that
> > exposed it, it seems to be that it's there for legacy reasons.
> > 
> > commit 43a775916d63d1c822107b39987192ca5ced445c
> > Author: Jiang Liu <jiang.liu@linux.intel.com>
> > Date:   Mon Jun 9 16:20:05 2014 +0800
> > 
> >     genirq: Export irq_domain_disassociate() to architecture interrupt drivers
> >     
> >     Export irq_domain_disassociate() to architecture interrupt drivers,
> >     so it could be used to handle legacy IRQ descriptors on x86.
> > 
> > So maybe we should just use the realloc argument and bury the
> > disassociate API in irqdomain.c because it's not supposed to be used?
> > Or does the realloc path not work for some reason?
> 
> I haven't tried the realloc path yet. Let's back up further so that we
> are both starting with the same assumptions. Do you want to keep either
> your proposed change (realloc argument) or the existing
> irq_domain_disassociate change in mainline past the end of this patch
> series? If it is going to continue to be a temporary shim that will be
> reverted at the end of the patch series (like I did here), then I don't
> see the point in this extra work since this patch is only here to keep
> it bisectable and works. We're not using any legacy interfaces by the
> end of this patch series.

The main concern I have is that we're changing the binding to avoid a
larger discussion we could have about whether or not the binding is
wrong to list out the hierarchical irqs as part of an 'interrupts'
property. We have one case where hardware irq numbers are remapped to
other hardware irq number spaces and it seems perfectly valid to use the
'interrupts' property to indicate this, i.e. chained interrupt
controllers.

Those controllers typically have a single 'interrupts' specifier for the
chained irq of the parent interrupt controller, but when it comes to
hierarchical interrupts we seem to have various different ways of
expressing the mapping from one number space to another. I've seen
approaches varying between hardcoding everything in the kernel to
hardcoding everything in DT.

I'm mostly wondering if having an interrupts property listing all the
parent interrupts in corresponding specifier slots for the interrupt
controller's hardware irq space is wrong. It seems to describe the
mapping between the two number spaces already, so maybe it would make
sense to use the realloc argument and keep listing them as interrupts in
DT. Obviously things are already moving forward with the new DT binding,
so maybe I just need to be told that having an interrupts property there
is wrong. If it is wrong, then nothing needs to be kept around and the
binding can easily be changed.

Patch
diff mbox series

diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
index 8eb2528793f9..2f99a98ccee5 100644
--- a/drivers/mfd/qcom-pm8xxx.c
+++ b/drivers/mfd/qcom-pm8xxx.c
@@ -380,6 +380,12 @@  static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
 				  struct irq_domain *domain, unsigned int irq,
 				  irq_hw_number_t hwirq, unsigned int type)
 {
+	unsigned int old_virq;
+
+	old_virq = irq_find_mapping(domain, hwirq);
+	if (old_virq)
+		irq_domain_disassociate(domain, old_virq);
+
 	irq_domain_set_info(domain, irq, hwirq, chip->pm_irq_data->irq_chip,
 			    chip, handle_level_irq, NULL, NULL);
 	irq_set_noprobe(irq);