All of lore.kernel.org
 help / color / mirror / Atom feed
* Add hierarchical irq_domain for i2c based gpio expander pca9505
@ 2017-10-11 12:20 Abhishek Shah
  2017-10-12  5:10 ` Abhishek Shah
  0 siblings, 1 reply; 3+ messages in thread
From: Abhishek Shah @ 2017-10-11 12:20 UTC (permalink / raw)
  To: Linus Walleij, takahiro.akashi; +Cc: linux-gpio, open list, Sandeep Tripathy

Hi Linus,

I am facing one issue, where; while disabling/ masking interrupts just
before kexec reboot, access to gpio expander pca9505 residing over
i2c bus hangs, because i2c interrupts are disabled prior to writing
pca9505 register.

In our chip, we have 3 irq_domain, namely gicv3, bcm-iproc-gpio [GPIO
controller with ~150 pins] and pca953x [40-pin IO expander pca9505].

bcm-iproc-gpio and i2c interrupts among the other interrupts  are
registered to gicv3 and
pca953x interrupts are registered to bcm-iproc-gpio.

So, interrupt from pca953x gets routed like...
pca953x-> bcm-iproc-gpio-> gicv3

Now, I see that, got GPIO controllers, an independent IRQ domain is
added using gpiochip_irqchip_add_nested
or gpiochip_irqchip_add, which makes use of irq_domain_add_simple like this:
gpiochip->irqdomain = irq_domain_add_simple(of_node,
                                        gpiochip->ngpio, first_irq,
                                        &gpiochip_domain_ops, gpiochip);

Is it possible to add hierarchical irq_domain for pca953x to be child
of bcm-iproc-gpio and bcm-iproc-gpio irq_domain  to be child of gicv3
using irq_domain_add_hierarchy instead of current  irq_domain_add_simple API??

Would that make sure that first all interrupts of child irq_domain
gets disabled?


Now, this is where I am facing issue during kexec_reboot:

static void machine_kexec_mask_interrupts(void)                  /*
function is present in arch/arm64/kernel/machine_kexec.c file */
{
        unsigned int i;
        struct irq_desc *desc;

        for_each_irq_desc(i, desc) {
                struct irq_chip *chip;
                int ret;

                chip = irq_desc_get_chip(desc);
                if (!chip)
                        continue;

                /*
                 * First try to remove the active state. If this
                 * fails, try to EOI the interrupt.
                 */
                ret = irq_set_irqchip_state(i, IRQCHIP_STATE_ACTIVE,
false);   /* access to PCA9505 register hangs here and kexec reboot
fails! */

                if (ret && irqd_irq_inprogress(&desc->irq_data) &&
                    chip->irq_eoi)
                        chip->irq_eoi(&desc->irq_data);

                if (chip->irq_mask)
                        chip->irq_mask(&desc->irq_data);

                if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
                        chip->irq_disable(&desc->irq_data);
        }
}

In my case, when this loop runs, initially it masks all GICv3
interrupts,i.e. i2c interrupts are also disabled.
Now when irq_set_irqchip_state gets executed in process of masking
pca9505 interrupts, .irq_bus_sync_unlock
callback (pca953x_irq_bus_sync_unlock) hangs because i2c interrupts are masked.

Do you think adding "irq_domain_add_hierarchy" for GPIO controllers
can solve this problem?
or do you suggest change in the way "machine_kexec_mask_interrupts" is
implemented?


Regards,
Abhishek

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Add hierarchical irq_domain for i2c based gpio expander pca9505
  2017-10-11 12:20 Add hierarchical irq_domain for i2c based gpio expander pca9505 Abhishek Shah
@ 2017-10-12  5:10 ` Abhishek Shah
  2017-10-12  8:59   ` Linus Walleij
  0 siblings, 1 reply; 3+ messages in thread
From: Abhishek Shah @ 2017-10-12  5:10 UTC (permalink / raw)
  To: Linus Walleij, Takahiro Akashi
  Cc: linux-gpio, open list, Sandeep Tripathy, BCM Kernel Feedback

+BCM kernel feedback.
Sorry for duplicated mails, had HTML formatting issue.

On Wed, Oct 11, 2017 at 5:50 PM, Abhishek Shah
<abhishek.shah@broadcom.com> wrote:
> Hi Linus,
>
> I am facing one issue, where; while disabling/ masking interrupts just
> before kexec reboot, access to gpio expander pca9505 residing over
> i2c bus hangs, because i2c interrupts are disabled prior to writing
> pca9505 register.
>
> In our chip, we have 3 irq_domain, namely gicv3, bcm-iproc-gpio [GPIO
> controller with ~150 pins] and pca953x [40-pin IO expander pca9505].
>
> bcm-iproc-gpio and i2c interrupts among the other interrupts  are
> registered to gicv3 and
> pca953x interrupts are registered to bcm-iproc-gpio.
>
> So, interrupt from pca953x gets routed like...
> pca953x-> bcm-iproc-gpio-> gicv3
>
> Now, I see that, got GPIO controllers, an independent IRQ domain is
> added using gpiochip_irqchip_add_nested
> or gpiochip_irqchip_add, which makes use of irq_domain_add_simple like this:
> gpiochip->irqdomain = irq_domain_add_simple(of_node,
>                                         gpiochip->ngpio, first_irq,
>                                         &gpiochip_domain_ops, gpiochip);
>
> Is it possible to add hierarchical irq_domain for pca953x to be child
> of bcm-iproc-gpio and bcm-iproc-gpio irq_domain  to be child of gicv3
> using irq_domain_add_hierarchy instead of current  irq_domain_add_simple API??
>
> Would that make sure that first all interrupts of child irq_domain
> gets disabled?
>
>
> Now, this is where I am facing issue during kexec_reboot:
>
> static void machine_kexec_mask_interrupts(void)                  /*
> function is present in arch/arm64/kernel/machine_kexec.c file */
> {
>         unsigned int i;
>         struct irq_desc *desc;
>
>         for_each_irq_desc(i, desc) {
>                 struct irq_chip *chip;
>                 int ret;
>
>                 chip = irq_desc_get_chip(desc);
>                 if (!chip)
>                         continue;
>
>                 /*
>                  * First try to remove the active state. If this
>                  * fails, try to EOI the interrupt.
>                  */
>                 ret = irq_set_irqchip_state(i, IRQCHIP_STATE_ACTIVE,
> false);   /* access to PCA9505 register hangs here and kexec reboot
> fails! */
>
>                 if (ret && irqd_irq_inprogress(&desc->irq_data) &&
>                     chip->irq_eoi)
>                         chip->irq_eoi(&desc->irq_data);
>
>                 if (chip->irq_mask)
>                         chip->irq_mask(&desc->irq_data);
>
>                 if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
>                         chip->irq_disable(&desc->irq_data);
>         }
> }
>
> In my case, when this loop runs, initially it masks all GICv3
> interrupts,i.e. i2c interrupts are also disabled.
> Now when irq_set_irqchip_state gets executed in process of masking
> pca9505 interrupts, .irq_bus_sync_unlock
> callback (pca953x_irq_bus_sync_unlock) hangs because i2c interrupts are masked.
>
> Do you think adding "irq_domain_add_hierarchy" for GPIO controllers
> can solve this problem?
> or do you suggest change in the way "machine_kexec_mask_interrupts" is
> implemented?
>
>
> Regards,
> Abhishek

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Add hierarchical irq_domain for i2c based gpio expander pca9505
  2017-10-12  5:10 ` Abhishek Shah
@ 2017-10-12  8:59   ` Linus Walleij
  0 siblings, 0 replies; 3+ messages in thread
From: Linus Walleij @ 2017-10-12  8:59 UTC (permalink / raw)
  To: Abhishek Shah, Ulf Hansson
  Cc: Takahiro Akashi, linux-gpio, open list, Sandeep Tripathy,
	BCM Kernel Feedback

On Thu, Oct 12, 2017 at 7:10 AM, Abhishek Shah
<abhishek.shah@broadcom.com> wrote:

>> I am facing one issue, where; while disabling/ masking interrupts just
>> before kexec reboot, access to gpio expander pca9505 residing over
>> i2c bus hangs, because i2c interrupts are disabled prior to writing
>> pca9505 register.

This dependency between IRQ controllers on slow buses
(I2C, SPI, 1W etc) and the IRQ controller above it is annoying.
We face the same issue with runtime power management.

I think Marc Z and Ulf Hansson may have input on this.
We surely need a strict procedure for how to take IRQ
chips up/down. We know how to take them up, but the procedure
for taking them down (and potentially up again) is a bit so-so.

>> In our chip, we have 3 irq_domain, namely gicv3, bcm-iproc-gpio [GPIO
>> controller with ~150 pins] and pca953x [40-pin IO expander pca9505].
>>
>> bcm-iproc-gpio and i2c interrupts among the other interrupts  are
>> registered to gicv3 and
>> pca953x interrupts are registered to bcm-iproc-gpio.
>>
>> So, interrupt from pca953x gets routed like...
>> pca953x-> bcm-iproc-gpio-> gicv3

I see, this is a pretty straight-forward cascaded interrupt controller.

>> Now, I see that, got GPIO controllers, an independent IRQ domain is
>> added using gpiochip_irqchip_add_nested
>> or gpiochip_irqchip_add, which makes use of irq_domain_add_simple like this:
>> gpiochip->irqdomain = irq_domain_add_simple(of_node,
>>                                         gpiochip->ngpio, first_irq,
>>                                         &gpiochip_domain_ops, gpiochip);

This looks fine.

>> Is it possible to add hierarchical irq_domain for pca953x to be child
>> of bcm-iproc-gpio and bcm-iproc-gpio irq_domain  to be child of gicv3
>> using irq_domain_add_hierarchy instead of current  irq_domain_add_simple API??

This is not what hierarchical IRQ domains are for.

Documentation and naming is a problem here. We really
should work on this. Marc's talk from last years ELCE is
very helpful:
https://elinux.org/images/8/8c/Zyngier.pdf
Also on YouTube:
https://www.youtube.com/watch?v=YE8cRHVIM4E

In short: hierarchic IRQ domains are for interrupt delivery
paths, not for sequencing taking irqchips up/down.

>> Would that make sure that first all interrupts of child irq_domain
>> gets disabled?

No.

>> Do you think adding "irq_domain_add_hierarchy" for GPIO controllers
>> can solve this problem?

No.

>> or do you suggest change in the way "machine_kexec_mask_interrupts" is
>> implemented?

What you need to do is analyze the call graph and figure out
the right way to order things.

There are irqchips that power up/down using syscore ops,
such as drivers/irqchip/irq-mtk-cirq.c
see
commit 9dbbbd33aafaf1f95b1cce940bdf4331cd8b822e
"irqchip: Add Mediatek mtk-cirq driver"

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-10-12  8:59 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-11 12:20 Add hierarchical irq_domain for i2c based gpio expander pca9505 Abhishek Shah
2017-10-12  5:10 ` Abhishek Shah
2017-10-12  8:59   ` Linus Walleij

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.