omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Belisko Marek]

Hello,

I'm trying to fix warning (for omap5 board) produced by recent change
to avoid using IRQ_TYPE_NONE like:
[    1.818666] WARNING: CPU: 1 PID: 778 at
drivers/irqchip/irq-gic.c:1016 gic_irq_domain_translate+0x78/0x100
[    1.828839] Modules linked in:

I did look to other commit which did update and without deep knowledge
I just simply do this small change:
diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
b/arch/arm/boot/dts/omap5-board-common.dtsi
index 218892b..ab2df8c 100644
--- a/arch/arm/boot/dts/omap5-board-common.dtsi
+++ b/arch/arm/boot/dts/omap5-board-common.dtsi
@@ -393,7 +393,7 @@

        palmas: palmas@48 {
                compatible = "ti,palmas";
-               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
+               interrupts = <GIC_SPI 7 IRQ_TYPE_HIGH>; /* IRQ_SYS_1N */
                reg = <0x48>;
                interrupt-controller;
                #interrupt-cells = <2>;

and it looks board boots fine. Only issue is that gpadc driver is not
working (at least not getting interrupts at all ADC fails with
timeout). I did look to gpadc driver and driver is not using
interrupts defined in dts but request interrupt directly from palmas
mfd module. Any ideas what needs to be changed to have gpadc again
working with mentioned patch?

I tried to use also IRQ_TYPE_LOW but in this case board cannot mount
rootfs so I was thinking this is not way to go.

Thanks for any hints.

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

* Belisko Marek <marek.belisko@gmail.com> [180620 09:40]:
> Hello,
> 
> I'm trying to fix warning (for omap5 board) produced by recent change
> to avoid using IRQ_TYPE_NONE like:
> [    1.818666] WARNING: CPU: 1 PID: 778 at
> drivers/irqchip/irq-gic.c:1016 gic_irq_domain_translate+0x78/0x100
> [    1.828839] Modules linked in:
> 
> I did look to other commit which did update and without deep knowledge
> I just simply do this small change:
> diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
> b/arch/arm/boot/dts/omap5-board-common.dtsi
> index 218892b..ab2df8c 100644
> --- a/arch/arm/boot/dts/omap5-board-common.dtsi
> +++ b/arch/arm/boot/dts/omap5-board-common.dtsi
> @@ -393,7 +393,7 @@
> 
>         palmas: palmas@48 {
>                 compatible = "ti,palmas";
> -               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
> +               interrupts = <GIC_SPI 7 IRQ_TYPE_HIGH>; /* IRQ_SYS_1N */
>                 reg = <0x48>;
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
> 
> and it looks board boots fine. Only issue is that gpadc driver is not
> working (at least not getting interrupts at all ADC fails with
> timeout). I did look to gpadc driver and driver is not using
> interrupts defined in dts but request interrupt directly from palmas
> mfd module. Any ideas what needs to be changed to have gpadc again
> working with mentioned patch?

Can you try with IRQF_TRIGGER_HIGH added also to the flags to
regmap_add_irq_chip() in drivers/mfd/palmas.c?

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Belisko Marek]

Hi Tony,

On Tue, Jul 3, 2018 at 10:45 AM Tony Lindgren <tony@atomide.com> wrote:
>
> * Belisko Marek <marek.belisko@gmail.com> [180620 09:40]:
> > Hello,
> >
> > I'm trying to fix warning (for omap5 board) produced by recent change
> > to avoid using IRQ_TYPE_NONE like:
> > [    1.818666] WARNING: CPU: 1 PID: 778 at
> > drivers/irqchip/irq-gic.c:1016 gic_irq_domain_translate+0x78/0x100
> > [    1.828839] Modules linked in:
> >
> > I did look to other commit which did update and without deep knowledge
> > I just simply do this small change:
> > diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
> > b/arch/arm/boot/dts/omap5-board-common.dtsi
> > index 218892b..ab2df8c 100644
> > --- a/arch/arm/boot/dts/omap5-board-common.dtsi
> > +++ b/arch/arm/boot/dts/omap5-board-common.dtsi
> > @@ -393,7 +393,7 @@
> >
> >         palmas: palmas@48 {
> >                 compatible = "ti,palmas";
> > -               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
> > +               interrupts = <GIC_SPI 7 IRQ_TYPE_HIGH>; /* IRQ_SYS_1N */
> >                 reg = <0x48>;
> >                 interrupt-controller;
> >                 #interrupt-cells = <2>;
> >
> > and it looks board boots fine. Only issue is that gpadc driver is not
> > working (at least not getting interrupts at all ADC fails with
> > timeout). I did look to gpadc driver and driver is not using
> > interrupts defined in dts but request interrupt directly from palmas
> > mfd module. Any ideas what needs to be changed to have gpadc again
> > working with mentioned patch?
>
> Can you try with IRQF_TRIGGER_HIGH added also to the flags to
> regmap_add_irq_chip() in drivers/mfd/palmas.c?
Nope issue is till present also after this change like:
diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
b/arch/arm/boot/dts/omap5-board-common.dtsi
index 218892b..6912769 100644
--- a/arch/arm/boot/dts/omap5-board-common.dtsi
+++ b/arch/arm/boot/dts/omap5-board-common.dtsi
@@ -393,7 +393,7 @@

        palmas: palmas@48 {
                compatible = "ti,palmas";
-               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
+               interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; /* IRQ_SYS_1N */
                reg = <0x48>;
                interrupt-controller;
                #interrupt-cells = <2>;
@@ -432,9 +432,9 @@

                gpadc: gpadc {
                        compatible = "ti,palmas-gpadc";
-                       interrupts = <18 0
-                                     16 0
-                                     17 0>;
+                       interrupts = <18 IRQ_TYPE_LEVEL_HIGH
+                                     16 IRQ_TYPE_LEVEL_HIGH
+                                     17 IRQ_TYPE_LEVEL_HIGH>;
                        #io-channel-cells = <1>;
                        ti,channel0-current-microamp = <5>;
                        ti,channel3-current-microamp = <10>;
diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
index 663a239..15d23db 100644
--- a/drivers/mfd/palmas.c
+++ b/drivers/mfd/palmas.c
@@ -601,7 +601,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
        regmap_write(palmas->regmap[slave], addr, reg);

        ret = regmap_add_irq_chip(palmas->regmap[slave], palmas->irq,
-                                 IRQF_ONESHOT | pdata->irq_flags, 0,
+                                 IRQF_ONESHOT | IRQF_TRIGGER_HIGH |
pdata->irq_flags, 0,
                                  driver_data->irq_chip, &palmas->irq_data);
        if (ret < 0)
                goto err_i2c;

>
> Regards,
>
> Tony

BR,

marek

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

Hi

* Belisko Marek <marek.belisko@gmail.com> [180703 18:34]:
> Hi Tony,
> 
> On Tue, Jul 3, 2018 at 10:45 AM Tony Lindgren <tony@atomide.com> wrote:
> >
> > * Belisko Marek <marek.belisko@gmail.com> [180620 09:40]:
> > > Hello,
> > >
> > > I'm trying to fix warning (for omap5 board) produced by recent change
> > > to avoid using IRQ_TYPE_NONE like:
> > > [    1.818666] WARNING: CPU: 1 PID: 778 at
> > > drivers/irqchip/irq-gic.c:1016 gic_irq_domain_translate+0x78/0x100
> > > [    1.828839] Modules linked in:
> > >
> > > I did look to other commit which did update and without deep knowledge
> > > I just simply do this small change:
> > > diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
> > > b/arch/arm/boot/dts/omap5-board-common.dtsi
> > > index 218892b..ab2df8c 100644
> > > --- a/arch/arm/boot/dts/omap5-board-common.dtsi
> > > +++ b/arch/arm/boot/dts/omap5-board-common.dtsi
> > > @@ -393,7 +393,7 @@
> > >
> > >         palmas: palmas@48 {
> > >                 compatible = "ti,palmas";
> > > -               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
> > > +               interrupts = <GIC_SPI 7 IRQ_TYPE_HIGH>; /* IRQ_SYS_1N */
> > >                 reg = <0x48>;
> > >                 interrupt-controller;
> > >                 #interrupt-cells = <2>;
> > >
> > > and it looks board boots fine. Only issue is that gpadc driver is not
> > > working (at least not getting interrupts at all ADC fails with
> > > timeout). I did look to gpadc driver and driver is not using
> > > interrupts defined in dts but request interrupt directly from palmas
> > > mfd module. Any ideas what needs to be changed to have gpadc again
> > > working with mentioned patch?
> >
> > Can you try with IRQF_TRIGGER_HIGH added also to the flags to
> > regmap_add_irq_chip() in drivers/mfd/palmas.c?
> Nope issue is till present also after this change like:
> diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
> b/arch/arm/boot/dts/omap5-board-common.dtsi
> index 218892b..6912769 100644
> --- a/arch/arm/boot/dts/omap5-board-common.dtsi
> +++ b/arch/arm/boot/dts/omap5-board-common.dtsi
> @@ -393,7 +393,7 @@
> 
>         palmas: palmas@48 {
>                 compatible = "ti,palmas";
> -               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
> +               interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; /* IRQ_SYS_1N */
>                 reg = <0x48>;
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
> @@ -432,9 +432,9 @@
> 
>                 gpadc: gpadc {
>                         compatible = "ti,palmas-gpadc";
> -                       interrupts = <18 0
> -                                     16 0
> -                                     17 0>;
> +                       interrupts = <18 IRQ_TYPE_LEVEL_HIGH
> +                                     16 IRQ_TYPE_LEVEL_HIGH
> +                                     17 IRQ_TYPE_LEVEL_HIGH>;
>                         #io-channel-cells = <1>;
>                         ti,channel0-current-microamp = <5>;
>                         ti,channel3-current-microamp = <10>;
> diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
> index 663a239..15d23db 100644
> --- a/drivers/mfd/palmas.c
> +++ b/drivers/mfd/palmas.c
> @@ -601,7 +601,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
>         regmap_write(palmas->regmap[slave], addr, reg);
> 
>         ret = regmap_add_irq_chip(palmas->regmap[slave], palmas->irq,
> -                                 IRQF_ONESHOT | pdata->irq_flags, 0,
> +                                 IRQF_ONESHOT | IRQF_TRIGGER_HIGH |
> pdata->irq_flags, 0,
>                                   driver_data->irq_chip, &palmas->irq_data);
>         if (ret < 0)
>                 goto err_i2c;

Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
should be fixed with IRQ_TYPE_HIGH.

No idea about why palmas interrupts would stop working though,
Peter, do you have any ideas on this one?

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [181113 18:07]:
> Hi
> 
> * Belisko Marek <marek.belisko@gmail.com> [180703 18:34]:
> > Hi Tony,
> > 
> > On Tue, Jul 3, 2018 at 10:45 AM Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > * Belisko Marek <marek.belisko@gmail.com> [180620 09:40]:
> > > > Hello,
> > > >
> > > > I'm trying to fix warning (for omap5 board) produced by recent change
> > > > to avoid using IRQ_TYPE_NONE like:
> > > > [    1.818666] WARNING: CPU: 1 PID: 778 at
> > > > drivers/irqchip/irq-gic.c:1016 gic_irq_domain_translate+0x78/0x100
> > > > [    1.828839] Modules linked in:
> > > >
> > > > I did look to other commit which did update and without deep knowledge
> > > > I just simply do this small change:
> > > > diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
> > > > b/arch/arm/boot/dts/omap5-board-common.dtsi
> > > > index 218892b..ab2df8c 100644
> > > > --- a/arch/arm/boot/dts/omap5-board-common.dtsi
> > > > +++ b/arch/arm/boot/dts/omap5-board-common.dtsi
> > > > @@ -393,7 +393,7 @@
> > > >
> > > >         palmas: palmas@48 {
> > > >                 compatible = "ti,palmas";
> > > > -               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
> > > > +               interrupts = <GIC_SPI 7 IRQ_TYPE_HIGH>; /* IRQ_SYS_1N */
> > > >                 reg = <0x48>;
> > > >                 interrupt-controller;
> > > >                 #interrupt-cells = <2>;
> > > >
> > > > and it looks board boots fine. Only issue is that gpadc driver is not
> > > > working (at least not getting interrupts at all ADC fails with
> > > > timeout). I did look to gpadc driver and driver is not using
> > > > interrupts defined in dts but request interrupt directly from palmas
> > > > mfd module. Any ideas what needs to be changed to have gpadc again
> > > > working with mentioned patch?
> > >
> > > Can you try with IRQF_TRIGGER_HIGH added also to the flags to
> > > regmap_add_irq_chip() in drivers/mfd/palmas.c?
> > Nope issue is till present also after this change like:
> > diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
> > b/arch/arm/boot/dts/omap5-board-common.dtsi
> > index 218892b..6912769 100644
> > --- a/arch/arm/boot/dts/omap5-board-common.dtsi
> > +++ b/arch/arm/boot/dts/omap5-board-common.dtsi
> > @@ -393,7 +393,7 @@
> > 
> >         palmas: palmas@48 {
> >                 compatible = "ti,palmas";
> > -               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
> > +               interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; /* IRQ_SYS_1N */
> >                 reg = <0x48>;
> >                 interrupt-controller;
> >                 #interrupt-cells = <2>;
> > @@ -432,9 +432,9 @@
> > 
> >                 gpadc: gpadc {
> >                         compatible = "ti,palmas-gpadc";
> > -                       interrupts = <18 0
> > -                                     16 0
> > -                                     17 0>;
> > +                       interrupts = <18 IRQ_TYPE_LEVEL_HIGH
> > +                                     16 IRQ_TYPE_LEVEL_HIGH
> > +                                     17 IRQ_TYPE_LEVEL_HIGH>;
> >                         #io-channel-cells = <1>;
> >                         ti,channel0-current-microamp = <5>;
> >                         ti,channel3-current-microamp = <10>;
> > diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
> > index 663a239..15d23db 100644
> > --- a/drivers/mfd/palmas.c
> > +++ b/drivers/mfd/palmas.c
> > @@ -601,7 +601,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
> >         regmap_write(palmas->regmap[slave], addr, reg);
> > 
> >         ret = regmap_add_irq_chip(palmas->regmap[slave], palmas->irq,
> > -                                 IRQF_ONESHOT | pdata->irq_flags, 0,
> > +                                 IRQF_ONESHOT | IRQF_TRIGGER_HIGH |
> > pdata->irq_flags, 0,
> >                                   driver_data->irq_chip, &palmas->irq_data);
> >         if (ret < 0)
> >                 goto err_i2c;
> 
> Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
> should be fixed with IRQ_TYPE_HIGH.

Looks like the gpadc interrupts get fixed for IRQ_TYPE_LEVEL_HIGH
if reconfiguring of PALMAS_POLARITY_CTRL_INT_POLARITY is disabled
in drivers/mfd/palmas.c.

The test being just:

modprobe palmas-gpadc
cat /sys/bus/iio/devices/iio:device0/*

> No idea about why palmas interrupts would stop working though,
> Peter, do you have any ideas on this one?

Still no idea why though, it seems tegra is inverting
the interrupt externally because of earlier patches for adding
"ti,irq-externally-inverted" property that never got added.

So I'm guessing the PALMAS_POLARITY_CTRL_INT_POLARITY
is wrongly configured on IRQ_TYPE_LEVEL_HIGH while it should
be done only for IRQ_TYPE_LEVEL_LOW instead?

So adding Laxman to Cc also.

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

Hi,

* Tony Lindgren <tony@atomide.com> [181114 17:04]:
> * Tony Lindgren <tony@atomide.com> [181113 18:07]:
> > Hi
> > 
> > * Belisko Marek <marek.belisko@gmail.com> [180703 18:34]:
> > > Hi Tony,
> > > 
> > > On Tue, Jul 3, 2018 at 10:45 AM Tony Lindgren <tony@atomide.com> wrote:
> > > >
> > > > * Belisko Marek <marek.belisko@gmail.com> [180620 09:40]:
> > > > > Hello,
> > > > >
> > > > > I'm trying to fix warning (for omap5 board) produced by recent change
> > > > > to avoid using IRQ_TYPE_NONE like:
> > > > > [    1.818666] WARNING: CPU: 1 PID: 778 at
> > > > > drivers/irqchip/irq-gic.c:1016 gic_irq_domain_translate+0x78/0x100
> > > > > [    1.828839] Modules linked in:
> > > > >
> > > > > I did look to other commit which did update and without deep knowledge
> > > > > I just simply do this small change:
> > > > > diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
> > > > > b/arch/arm/boot/dts/omap5-board-common.dtsi
> > > > > index 218892b..ab2df8c 100644
> > > > > --- a/arch/arm/boot/dts/omap5-board-common.dtsi
> > > > > +++ b/arch/arm/boot/dts/omap5-board-common.dtsi
> > > > > @@ -393,7 +393,7 @@
> > > > >
> > > > >         palmas: palmas@48 {
> > > > >                 compatible = "ti,palmas";
> > > > > -               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
> > > > > +               interrupts = <GIC_SPI 7 IRQ_TYPE_HIGH>; /* IRQ_SYS_1N */
> > > > >                 reg = <0x48>;
> > > > >                 interrupt-controller;
> > > > >                 #interrupt-cells = <2>;
> > > > >
> > > > > and it looks board boots fine. Only issue is that gpadc driver is not
> > > > > working (at least not getting interrupts at all ADC fails with
> > > > > timeout). I did look to gpadc driver and driver is not using
> > > > > interrupts defined in dts but request interrupt directly from palmas
> > > > > mfd module. Any ideas what needs to be changed to have gpadc again
> > > > > working with mentioned patch?
> > > >
> > > > Can you try with IRQF_TRIGGER_HIGH added also to the flags to
> > > > regmap_add_irq_chip() in drivers/mfd/palmas.c?
> > > Nope issue is till present also after this change like:
> > > diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
> > > b/arch/arm/boot/dts/omap5-board-common.dtsi
> > > index 218892b..6912769 100644
> > > --- a/arch/arm/boot/dts/omap5-board-common.dtsi
> > > +++ b/arch/arm/boot/dts/omap5-board-common.dtsi
> > > @@ -393,7 +393,7 @@
> > > 
> > >         palmas: palmas@48 {
> > >                 compatible = "ti,palmas";
> > > -               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
> > > +               interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; /* IRQ_SYS_1N */
> > >                 reg = <0x48>;
> > >                 interrupt-controller;
> > >                 #interrupt-cells = <2>;
> > > @@ -432,9 +432,9 @@
> > > 
> > >                 gpadc: gpadc {
> > >                         compatible = "ti,palmas-gpadc";
> > > -                       interrupts = <18 0
> > > -                                     16 0
> > > -                                     17 0>;
> > > +                       interrupts = <18 IRQ_TYPE_LEVEL_HIGH
> > > +                                     16 IRQ_TYPE_LEVEL_HIGH
> > > +                                     17 IRQ_TYPE_LEVEL_HIGH>;
> > >                         #io-channel-cells = <1>;
> > >                         ti,channel0-current-microamp = <5>;
> > >                         ti,channel3-current-microamp = <10>;
> > > diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
> > > index 663a239..15d23db 100644
> > > --- a/drivers/mfd/palmas.c
> > > +++ b/drivers/mfd/palmas.c
> > > @@ -601,7 +601,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
> > >         regmap_write(palmas->regmap[slave], addr, reg);
> > > 
> > >         ret = regmap_add_irq_chip(palmas->regmap[slave], palmas->irq,
> > > -                                 IRQF_ONESHOT | pdata->irq_flags, 0,
> > > +                                 IRQF_ONESHOT | IRQF_TRIGGER_HIGH |
> > > pdata->irq_flags, 0,
> > >                                   driver_data->irq_chip, &palmas->irq_data);
> > >         if (ret < 0)
> > >                 goto err_i2c;
> > 
> > Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
> > should be fixed with IRQ_TYPE_HIGH.
> 
> Looks like the gpadc interrupts get fixed for IRQ_TYPE_LEVEL_HIGH
> if reconfiguring of PALMAS_POLARITY_CTRL_INT_POLARITY is disabled
> in drivers/mfd/palmas.c.
> 
> The test being just:
> 
> modprobe palmas-gpadc
> cat /sys/bus/iio/devices/iio:device0/*
> 
> > No idea about why palmas interrupts would stop working though,
> > Peter, do you have any ideas on this one?
> 
> Still no idea why though, it seems tegra is inverting
> the interrupt externally because of earlier patches for adding
> "ti,irq-externally-inverted" property that never got added.
> 
> So I'm guessing the PALMAS_POLARITY_CTRL_INT_POLARITY
> is wrongly configured on IRQ_TYPE_LEVEL_HIGH while it should
> be done only for IRQ_TYPE_LEVEL_LOW instead?
> 
> So adding Laxman to Cc also.

Now really adding Laxman to Cc.

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Peter Ujfalusi]

On 2018-11-13 20:06, Tony Lindgren wrote:
> Hi
> 
> * Belisko Marek <marek.belisko@gmail.com> [180703 18:34]:
>> Hi Tony,
>>
>> On Tue, Jul 3, 2018 at 10:45 AM Tony Lindgren <tony@atomide.com> wrote:
>>>
>>> * Belisko Marek <marek.belisko@gmail.com> [180620 09:40]:
>>>> Hello,
>>>>
>>>> I'm trying to fix warning (for omap5 board) produced by recent change
>>>> to avoid using IRQ_TYPE_NONE like:
>>>> [    1.818666] WARNING: CPU: 1 PID: 778 at
>>>> drivers/irqchip/irq-gic.c:1016 gic_irq_domain_translate+0x78/0x100
>>>> [    1.828839] Modules linked in:
>>>>
>>>> I did look to other commit which did update and without deep knowledge
>>>> I just simply do this small change:
>>>> diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
>>>> b/arch/arm/boot/dts/omap5-board-common.dtsi
>>>> index 218892b..ab2df8c 100644
>>>> --- a/arch/arm/boot/dts/omap5-board-common.dtsi
>>>> +++ b/arch/arm/boot/dts/omap5-board-common.dtsi
>>>> @@ -393,7 +393,7 @@
>>>>
>>>>         palmas: palmas@48 {
>>>>                 compatible = "ti,palmas";
>>>> -               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
>>>> +               interrupts = <GIC_SPI 7 IRQ_TYPE_HIGH>; /* IRQ_SYS_1N */
>>>>                 reg = <0x48>;
>>>>                 interrupt-controller;
>>>>                 #interrupt-cells = <2>;
>>>>
>>>> and it looks board boots fine. Only issue is that gpadc driver is not
>>>> working (at least not getting interrupts at all ADC fails with
>>>> timeout). I did look to gpadc driver and driver is not using
>>>> interrupts defined in dts but request interrupt directly from palmas
>>>> mfd module. Any ideas what needs to be changed to have gpadc again
>>>> working with mentioned patch?
>>>
>>> Can you try with IRQF_TRIGGER_HIGH added also to the flags to
>>> regmap_add_irq_chip() in drivers/mfd/palmas.c?
>> Nope issue is till present also after this change like:
>> diff --git a/arch/arm/boot/dts/omap5-board-common.dtsi
>> b/arch/arm/boot/dts/omap5-board-common.dtsi
>> index 218892b..6912769 100644
>> --- a/arch/arm/boot/dts/omap5-board-common.dtsi
>> +++ b/arch/arm/boot/dts/omap5-board-common.dtsi
>> @@ -393,7 +393,7 @@
>>
>>         palmas: palmas@48 {
>>                 compatible = "ti,palmas";
>> -               interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>; /* IRQ_SYS_1N */
>> +               interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; /* IRQ_SYS_1N */
>>                 reg = <0x48>;
>>                 interrupt-controller;
>>                 #interrupt-cells = <2>;
>> @@ -432,9 +432,9 @@
>>
>>                 gpadc: gpadc {
>>                         compatible = "ti,palmas-gpadc";
>> -                       interrupts = <18 0
>> -                                     16 0
>> -                                     17 0>;
>> +                       interrupts = <18 IRQ_TYPE_LEVEL_HIGH
>> +                                     16 IRQ_TYPE_LEVEL_HIGH
>> +                                     17 IRQ_TYPE_LEVEL_HIGH>;
>>                         #io-channel-cells = <1>;
>>                         ti,channel0-current-microamp = <5>;
>>                         ti,channel3-current-microamp = <10>;
>> diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
>> index 663a239..15d23db 100644
>> --- a/drivers/mfd/palmas.c
>> +++ b/drivers/mfd/palmas.c
>> @@ -601,7 +601,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
>>         regmap_write(palmas->regmap[slave], addr, reg);
>>
>>         ret = regmap_add_irq_chip(palmas->regmap[slave], palmas->irq,
>> -                                 IRQF_ONESHOT | pdata->irq_flags, 0,
>> +                                 IRQF_ONESHOT | IRQF_TRIGGER_HIGH |
>> pdata->irq_flags, 0,
>>                                   driver_data->irq_chip, &palmas->irq_data);
>>         if (ret < 0)
>>                 goto err_i2c;
> 
> Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
> should be fixed with IRQ_TYPE_HIGH.
> 
> No idea about why palmas interrupts would stop working though,
> Peter, do you have any ideas on this one?

No, I don't.
The INT polarity can be changed in Palmas.
based on the pdata->irq_flags (queried via irqd_get_trigger_type())
the code  configures it:

if (pdata->irq_flags & IRQ_TYPE_LEVEL_HIGH)
	reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
else
	reg = 0;

and we pass the same irq_flags to the regmap_add_irq_chip()
IRQ_TYPE_LEVEL_HIGH == IRQF_TRIGGER_HIGH == 0x00000004

A change in DT should be enough, no need to patch palmas.c, imho.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [181119 10:16]:
> On 2018-11-13 20:06, Tony Lindgren wrote:
> > Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
> > should be fixed with IRQ_TYPE_HIGH.
> > 
> > No idea about why palmas interrupts would stop working though,
> > Peter, do you have any ideas on this one?
> 
> No, I don't.
> The INT polarity can be changed in Palmas.
> based on the pdata->irq_flags (queried via irqd_get_trigger_type())
> the code  configures it:
> 
> if (pdata->irq_flags & IRQ_TYPE_LEVEL_HIGH)
> 	reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
> else
> 	reg = 0;
> 
> and we pass the same irq_flags to the regmap_add_irq_chip()
> IRQ_TYPE_LEVEL_HIGH == IRQF_TRIGGER_HIGH == 0x00000004
> 
> A change in DT should be enough, no need to patch palmas.c, imho.

But it's not. I'm now wondering if wakeupgen is inverting the
polarity for this interrupt?

GIC docs say this about SPI interrupts:

"SPI is triggered on a rising edge or is active-HIGH level-sensitive."

So when setting IRQ_TYPE_LEVEL_HIGH in dts, we still must not
invert the polarity in palmas while tegra needs to. So either
tegra114 hardware is inverting the polarity, or omap5 wakeupgen
is.

Does the palmas trm say which way PALMAS_POLARITY_CTRL
triggers if PALMAS_POLARITY_CTRL_INT_POLARITY is set?

Also note that dra7 is using a gpio for palmas interrupt.

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

Hi,

* Tony Lindgren <tony@atomide.com> [181119 16:19]:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [181119 10:16]:
> > On 2018-11-13 20:06, Tony Lindgren wrote:
> > > Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
> > > should be fixed with IRQ_TYPE_HIGH.
> > > 
> > > No idea about why palmas interrupts would stop working though,
> > > Peter, do you have any ideas on this one?
> > 
> > No, I don't.
> > The INT polarity can be changed in Palmas.
> > based on the pdata->irq_flags (queried via irqd_get_trigger_type())
> > the code  configures it:
> > 
> > if (pdata->irq_flags & IRQ_TYPE_LEVEL_HIGH)
> > 	reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
> > else
> > 	reg = 0;
> > 
> > and we pass the same irq_flags to the regmap_add_irq_chip()
> > IRQ_TYPE_LEVEL_HIGH == IRQF_TRIGGER_HIGH == 0x00000004
> > 
> > A change in DT should be enough, no need to patch palmas.c, imho.
> 
> But it's not. I'm now wondering if wakeupgen is inverting the
> polarity for this interrupt?
> 
> GIC docs say this about SPI interrupts:
> 
> "SPI is triggered on a rising edge or is active-HIGH level-sensitive."
> 
> So when setting IRQ_TYPE_LEVEL_HIGH in dts, we still must not
> invert the polarity in palmas while tegra needs to. So either
> tegra114 hardware is inverting the polarity, or omap5 wakeupgen
> is.
> 
> Does the palmas trm say which way PALMAS_POLARITY_CTRL
> triggers if PALMAS_POLARITY_CTRL_INT_POLARITY is set?
> 
> Also note that dra7 is using a gpio for palmas interrupt.

Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
Tegra114 PMIC interrupt") states that tegra114 inverts the
polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.

So it seems that commit df545d1cd01a ("mfd: palmas: Provide
irq flags through DT/platform data") wrongly sets the
PALMAS_POLARITY_CTRL_INT_POLARITY on IRQ_TYPE_LEVEL_HIGH
while it should set it on IRQ_TYPE_LEVEL_LOW.

I think the fix needs to set the polarity using
of_machine_is_compatible() and probably also add a new
compatible to palmas.c for "ti,palmas-tegra114" to properly
deal with the inverted interrupt. Or add a property for
"interrupt-inverted". In any case, it seems that the
of_machine_is_compatible() is also needed too to avoid
breaking use with dtb files.

Jon & Thierry, can you guys please check and confirm this?

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Jon Hunter]

On 19/11/2018 17:14, Tony Lindgren wrote:
> Hi,
> 
> * Tony Lindgren <tony@atomide.com> [181119 16:19]:
>> * Peter Ujfalusi <peter.ujfalusi@ti.com> [181119 10:16]:
>>> On 2018-11-13 20:06, Tony Lindgren wrote:
>>>> Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
>>>> should be fixed with IRQ_TYPE_HIGH.
>>>>
>>>> No idea about why palmas interrupts would stop working though,
>>>> Peter, do you have any ideas on this one?
>>>
>>> No, I don't.
>>> The INT polarity can be changed in Palmas.
>>> based on the pdata->irq_flags (queried via irqd_get_trigger_type())
>>> the code  configures it:
>>>
>>> if (pdata->irq_flags & IRQ_TYPE_LEVEL_HIGH)
>>> 	reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
>>> else
>>> 	reg = 0;
>>>
>>> and we pass the same irq_flags to the regmap_add_irq_chip()
>>> IRQ_TYPE_LEVEL_HIGH == IRQF_TRIGGER_HIGH == 0x00000004
>>>
>>> A change in DT should be enough, no need to patch palmas.c, imho.
>>
>> But it's not. I'm now wondering if wakeupgen is inverting the
>> polarity for this interrupt?
>>
>> GIC docs say this about SPI interrupts:
>>
>> "SPI is triggered on a rising edge or is active-HIGH level-sensitive."
>>
>> So when setting IRQ_TYPE_LEVEL_HIGH in dts, we still must not
>> invert the polarity in palmas while tegra needs to. So either
>> tegra114 hardware is inverting the polarity, or omap5 wakeupgen
>> is.
>>
>> Does the palmas trm say which way PALMAS_POLARITY_CTRL
>> triggers if PALMAS_POLARITY_CTRL_INT_POLARITY is set?
>>
>> Also note that dra7 is using a gpio for palmas interrupt.
> 
> Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
> Tegra114 PMIC interrupt") states that tegra114 inverts the
> polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.

Yes Tegra can invert the polarity of the PMIC interrupt.

> So it seems that commit df545d1cd01a ("mfd: palmas: Provide
> irq flags through DT/platform data") wrongly sets the
> PALMAS_POLARITY_CTRL_INT_POLARITY on IRQ_TYPE_LEVEL_HIGH
> while it should set it on IRQ_TYPE_LEVEL_LOW.
> 
> I think the fix needs to set the polarity using
> of_machine_is_compatible() and probably also add a new
> compatible to palmas.c for "ti,palmas-tegra114" to properly
> deal with the inverted interrupt. Or add a property for
> "interrupt-inverted". In any case, it seems that the
> of_machine_is_compatible() is also needed too to avoid
> breaking use with dtb files.
> 
> Jon & Thierry, can you guys please check and confirm this?
I don't fully understand what is being discussed here, but my
understanding is that the palmas interrupt is active low.

Let me know if this helps.

Cheers
Jon

-- 
nvpublic

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

* Jon Hunter <jonathanh@nvidia.com> [181120 11:14]:
> On 19/11/2018 17:14, Tony Lindgren wrote:
> > Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
> > Tegra114 PMIC interrupt") states that tegra114 inverts the
> > polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.
> 
> Yes Tegra can invert the polarity of the PMIC interrupt.

So is there some IP on Tegra called "Tegra PMC" that is
inverting the interrupt? Or is the "Tegra PMC" that commit
7e9d474954f4 mentions just the palmas configuration for
inverting the interrupt?

The problem I'm having is With omap5 where I can only get the
PMIC interrupts working with IRQ_TYPE_LEVEL_HIGH if
PALMAS_POLARITY_CTRL_INT_POLARITY is not set unlike for
Tegra.

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Thierry Reding]

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

On Fri, Nov 23, 2018 at 08:48:27AM -0800, Tony Lindgren wrote:
> * Jon Hunter <jonathanh@nvidia.com> [181120 11:14]:
> > On 19/11/2018 17:14, Tony Lindgren wrote:
> > > Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
> > > Tegra114 PMIC interrupt") states that tegra114 inverts the
> > > polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.
> > 
> > Yes Tegra can invert the polarity of the PMIC interrupt.
> 
> So is there some IP on Tegra called "Tegra PMC" that is
> inverting the interrupt? Or is the "Tegra PMC" that commit
> 7e9d474954f4 mentions just the palmas configuration for
> inverting the interrupt?

Yes, there's indeed an IP called PMC (Power-Management Controller) on
Tegra. It has a special input that is usually wired up to the PMIC
interrupt and a bit in the control register that configures the polarity
of that interrupt. If the PMIC generates a low-active interrupt we
usually set that bit to make sure it is properly sampled by the PMC.

The symptoms of this being incorrectly configured is usually an
interrupt storm on the PMIC interrupt, which I think typically results
in the system not booting at all, or taking a very long time to boot
because of that storm.

> The problem I'm having is With omap5 where I can only get the
> PMIC interrupts working with IRQ_TYPE_LEVEL_HIGH if
> PALMAS_POLARITY_CTRL_INT_POLARITY is not set unlike for
> Tegra.

Does somebody have access to the Palmas documentation? That should
pretty clearly state what the default polarity is and what it changes to
if you set the interrupt polarity bit.

From what you're saying it sounds like either the logic is the wrong way
around in the Palmas MFD driver (and we correct it by switching it back
to the correct polarity in the PMC) or that you'd need to find some way
of inverting in on OMAP5.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Peter Ujfalusi]

Thierry,

On 11/26/18 11:36 AM, Thierry Reding wrote:
> On Fri, Nov 23, 2018 at 08:48:27AM -0800, Tony Lindgren wrote:
>> * Jon Hunter <jonathanh@nvidia.com> [181120 11:14]:
>>> On 19/11/2018 17:14, Tony Lindgren wrote:
>>>> Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
>>>> Tegra114 PMIC interrupt") states that tegra114 inverts the
>>>> polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.
>>>
>>> Yes Tegra can invert the polarity of the PMIC interrupt.
>>
>> So is there some IP on Tegra called "Tegra PMC" that is
>> inverting the interrupt? Or is the "Tegra PMC" that commit
>> 7e9d474954f4 mentions just the palmas configuration for
>> inverting the interrupt?
> 
> Yes, there's indeed an IP called PMC (Power-Management Controller) on
> Tegra. It has a special input that is usually wired up to the PMIC
> interrupt and a bit in the control register that configures the polarity
> of that interrupt. If the PMIC generates a low-active interrupt we
> usually set that bit to make sure it is properly sampled by the PMC.
> 
> The symptoms of this being incorrectly configured is usually an
> interrupt storm on the PMIC interrupt, which I think typically results
> in the system not booting at all, or taking a very long time to boot
> because of that storm.
> 
>> The problem I'm having is With omap5 where I can only get the
>> PMIC interrupts working with IRQ_TYPE_LEVEL_HIGH if
>> PALMAS_POLARITY_CTRL_INT_POLARITY is not set unlike for
>> Tegra.
> 
> Does somebody have access to the Palmas documentation? That should
> pretty clearly state what the default polarity is and what it changes to
> if you set the interrupt polarity bit.

The register map documentation I have states the following:
bit7 INT_POLARITY Select the polarity of the INT output line
0: Interrupt line (INT) is low when interrupt is pending (default) RW
1: Interrupt line (INT) is high when interrupt is pending

By default the Palmas irq is active low.

> From what you're saying it sounds like either the logic is the wrong way
> around in the Palmas MFD driver (and we correct it by switching it back
> to the correct polarity in the PMC) or that you'd need to find some way
> of inverting in on OMAP5.
> 
> Thierry
> 

- Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Thierry Reding]

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

On Mon, Nov 26, 2018 at 11:49:54AM +0200, Peter Ujfalusi wrote:
> Thierry,
> 
> On 11/26/18 11:36 AM, Thierry Reding wrote:
> > On Fri, Nov 23, 2018 at 08:48:27AM -0800, Tony Lindgren wrote:
> >> * Jon Hunter <jonathanh@nvidia.com> [181120 11:14]:
> >>> On 19/11/2018 17:14, Tony Lindgren wrote:
> >>>> Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
> >>>> Tegra114 PMIC interrupt") states that tegra114 inverts the
> >>>> polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.
> >>>
> >>> Yes Tegra can invert the polarity of the PMIC interrupt.
> >>
> >> So is there some IP on Tegra called "Tegra PMC" that is
> >> inverting the interrupt? Or is the "Tegra PMC" that commit
> >> 7e9d474954f4 mentions just the palmas configuration for
> >> inverting the interrupt?
> > 
> > Yes, there's indeed an IP called PMC (Power-Management Controller) on
> > Tegra. It has a special input that is usually wired up to the PMIC
> > interrupt and a bit in the control register that configures the polarity
> > of that interrupt. If the PMIC generates a low-active interrupt we
> > usually set that bit to make sure it is properly sampled by the PMC.
> > 
> > The symptoms of this being incorrectly configured is usually an
> > interrupt storm on the PMIC interrupt, which I think typically results
> > in the system not booting at all, or taking a very long time to boot
> > because of that storm.
> > 
> >> The problem I'm having is With omap5 where I can only get the
> >> PMIC interrupts working with IRQ_TYPE_LEVEL_HIGH if
> >> PALMAS_POLARITY_CTRL_INT_POLARITY is not set unlike for
> >> Tegra.
> > 
> > Does somebody have access to the Palmas documentation? That should
> > pretty clearly state what the default polarity is and what it changes to
> > if you set the interrupt polarity bit.
> 
> The register map documentation I have states the following:
> bit7 INT_POLARITY Select the polarity of the INT output line
> 0: Interrupt line (INT) is low when interrupt is pending (default) RW
> 1: Interrupt line (INT) is high when interrupt is pending
> 
> By default the Palmas irq is active low.

That would confirm that the driver code is correct. My understanding is
that the PMC on Tegra expects a low-active IRQ from the PMIC, so we need
to invert the interrupt again in the PMC.

> > From what you're saying it sounds like either the logic is the wrong way
> > around in the Palmas MFD driver (and we correct it by switching it back
> > to the correct polarity in the PMC) or that you'd need to find some way
> > of inverting in on OMAP5.

From the above it sounds like there would need to be some mechanism in
OMAP5 to invert the PMIC interrupt, similarly to how it is done in the
PMC on Tegra.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

* Thierry Reding <treding@nvidia.com> [181126 10:25]:
> On Mon, Nov 26, 2018 at 11:49:54AM +0200, Peter Ujfalusi wrote:
> > The register map documentation I have states the following:
> > bit7 INT_POLARITY Select the polarity of the INT output line
> > 0: Interrupt line (INT) is low when interrupt is pending (default) RW
> > 1: Interrupt line (INT) is high when interrupt is pending
> > 
> > By default the Palmas irq is active low.
> 
> That would confirm that the driver code is correct. My understanding is
> that the PMC on Tegra expects a low-active IRQ from the PMIC, so we need
> to invert the interrupt again in the PMC.

But then why Tegra need to set PALMAS_POLARITY_CTRL_INT_POLARITY
if dts has IRQ_TYPE_LEVEL_HIGH? Shouldn't the Palmas default low
setting be correct for Tegra if PMC expects active-low interrupt
and then inverts it for GIC?

What seems to make most sense for me right now is either
this option A:

1. Palmas TRM has the INT_POLARITY register misdocumented
   the wrong way around

2. Tegra really gets a level-low interrupt now from
   Palmas with PALMAS_POLARITY_CTRL_INT_POLARITY set and
   then inverts it to level-high for GIC

3. Omap5 wakeupgen does not invert the interrupt for GIC
   and needs PALMAS_POLARITY_CTRL_INT_POLARITY cleared
   for level-high interrupt from Palmas that gets passed
   as level-high interrupt to GIC

Or else option B:

1. Palmas TRM is correct for INT_POLARITY register

2. Tegra should not set PALMAS_POLARITY_CTRL_INT_POLARITY
   as Tegra PMC already translates Palmas level-low interrupt
   to level-high for GIC

3. Omap5 wkupgen also translates palmas interrupt and must
   not set PALMAS_POLARITY_CTRL_INT_POLARITY

Anybody got better explanations?

BTW, this interrupt is pretty easy to test with the
rtctest tool in Linux kernel:

tools/testing/selftests/rtc/rtctest.c

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Jon Hunter]

On 26/11/2018 19:32, Tony Lindgren wrote:
> * Thierry Reding <treding@nvidia.com> [181126 10:25]:
>> On Mon, Nov 26, 2018 at 11:49:54AM +0200, Peter Ujfalusi wrote:
>>> The register map documentation I have states the following:
>>> bit7 INT_POLARITY Select the polarity of the INT output line
>>> 0: Interrupt line (INT) is low when interrupt is pending (default) RW
>>> 1: Interrupt line (INT) is high when interrupt is pending
>>>
>>> By default the Palmas irq is active low.
>>
>> That would confirm that the driver code is correct. My understanding is
>> that the PMC on Tegra expects a low-active IRQ from the PMIC, so we need
>> to invert the interrupt again in the PMC.
> 
> But then why Tegra need to set PALMAS_POLARITY_CTRL_INT_POLARITY
> if dts has IRQ_TYPE_LEVEL_HIGH? Shouldn't the Palmas default low
> setting be correct for Tegra if PMC expects active-low interrupt
> and then inverts it for GIC?

So I think what is going on here is ...

1. For Tegra, the interrupt parent the palmas interrupt in DT is the GIC
   not the PMC. The PMC does not register an interrupt controller
   (although to be correct probably should have. I think it had been
    discussed in the past and Stephen W may know the history here). So
   the interrupt polarity has to be HIGH otherwise setting the trigger
   type in the GIC will fail (as it only supports rising edge or level
   high IIRC).
2. However, as Thierry mentioned the Tegra PMC wants an active low
   interrupt and so the PMC inverts it on entering the PMC. However,
   given that the GIC interrupts must be active high, the PMC must
   invert again between the PMC and GIC.

So looking back my description in the change 7e9d474954f4 was not quite
accurate because the interrupt from palmas is active high but the PMC
inverts it. I think that this is quite confusing because we don't have a
good way to describe this in the DT. If we made the PMC an interrupt
controller then it would probably be a lot clearer. However, for
historical reasons this was not done.

Cheers
Jon

-- 
nvpublic

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

* Jon Hunter <jonathanh@nvidia.com> [181126 20:17]:
> 
> On 26/11/2018 19:32, Tony Lindgren wrote:
> > * Thierry Reding <treding@nvidia.com> [181126 10:25]:
> >> On Mon, Nov 26, 2018 at 11:49:54AM +0200, Peter Ujfalusi wrote:
> >>> The register map documentation I have states the following:
> >>> bit7 INT_POLARITY Select the polarity of the INT output line
> >>> 0: Interrupt line (INT) is low when interrupt is pending (default) RW
> >>> 1: Interrupt line (INT) is high when interrupt is pending
> >>>
> >>> By default the Palmas irq is active low.
> >>
> >> That would confirm that the driver code is correct. My understanding is
> >> that the PMC on Tegra expects a low-active IRQ from the PMIC, so we need
> >> to invert the interrupt again in the PMC.
> > 
> > But then why Tegra need to set PALMAS_POLARITY_CTRL_INT_POLARITY
> > if dts has IRQ_TYPE_LEVEL_HIGH? Shouldn't the Palmas default low
> > setting be correct for Tegra if PMC expects active-low interrupt
> > and then inverts it for GIC?
> 
> So I think what is going on here is ...
> 
> 1. For Tegra, the interrupt parent the palmas interrupt in DT is the GIC
>    not the PMC. The PMC does not register an interrupt controller
>    (although to be correct probably should have. I think it had been
>     discussed in the past and Stephen W may know the history here). So
>    the interrupt polarity has to be HIGH otherwise setting the trigger
>    type in the GIC will fail (as it only supports rising edge or level
>    high IIRC).
> 2. However, as Thierry mentioned the Tegra PMC wants an active low
>    interrupt and so the PMC inverts it on entering the PMC. However,
>    given that the GIC interrupts must be active high, the PMC must
>    invert again between the PMC and GIC.
> 
> So looking back my description in the change 7e9d474954f4 was not quite
> accurate because the interrupt from palmas is active high but the PMC
> inverts it. I think that this is quite confusing because we don't have a
> good way to describe this in the DT. If we made the PMC an interrupt
> controller then it would probably be a lot clearer. However, for
> historical reasons this was not done.

OK if Tegra PMC inverts the PMIC interrupt coming in from palmas as
active-high to active-low for PMC, and then PMC again inverts it
from active-low to active-high for GIC then it makes sense.

The only option that works for omap5 at palmas end is if
PALMAS_POLARITY_CTRL_INT_POLARITY is cleared. Setting the SoC internal
pulls does not make a difference, so I suspect there is either some
unknown pull-up/pull-down configuration register in palmas, or there
is an external pull resistor.

But as dra7 is using a gpio interrupt, I'll just change omap5 to use
gpio_wk16 instead of sys_nirq1 too. Will send out a patch shortly.

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [181127 17:55]:
> The only option that works for omap5 at palmas end is if
> PALMAS_POLARITY_CTRL_INT_POLARITY is cleared. Setting the SoC internal
> pulls does not make a difference, so I suspect there is either some
> unknown pull-up/pull-down configuration register in palmas, or there
> is an external pull resistor.

Remuxing the palmas interrupt to gpio_wk16 instead of sys_nirq1
makes the palmas interrupt work just fine configured with active-low
or active-high with the internal pulls. So palmas.c is doing the
right thing, and the issue configuring palmas interrupt active-high
with sys_nirq1 on omap5 is somewhere between omap wkupegen and GIC.

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Jon Hunter]

On 23/11/2018 16:48, Tony Lindgren wrote:
> * Jon Hunter <jonathanh@nvidia.com> [181120 11:14]:
>> On 19/11/2018 17:14, Tony Lindgren wrote:
>>> Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
>>> Tegra114 PMIC interrupt") states that tegra114 inverts the
>>> polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.
>>
>> Yes Tegra can invert the polarity of the PMIC interrupt.
> 
> So is there some IP on Tegra called "Tegra PMC" that is
> inverting the interrupt? Or is the "Tegra PMC" that commit
> 7e9d474954f4 mentions just the palmas configuration for
> inverting the interrupt?
> 
> The problem I'm having is With omap5 where I can only get the
> PMIC interrupts working with IRQ_TYPE_LEVEL_HIGH if
> PALMAS_POLARITY_CTRL_INT_POLARITY is not set unlike for
> Tegra.

I see what you are saying and so technically we should not need to
invert with the Tegra PMC as well. This PMIC is used on the Tegra114
Dalmore board which was where I saw the original problem. However, I
have not tested this board for a while (and not part of our current
nightly testing). Do you know if something has changed recently?

Cheers
Jon

-- 
nvpublic

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Laxman Dewangan]

On Monday 19 November 2018 10:44 PM, Tony Lindgren wrote:
> Hi,
>
> * Tony Lindgren <tony@atomide.com> [181119 16:19]:
>> * Peter Ujfalusi <peter.ujfalusi@ti.com> [181119 10:16]:
>>> On 2018-11-13 20:06, Tony Lindgren wrote:
>>>> Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
>>>> should be fixed with IRQ_TYPE_HIGH.
>>>>
>>>> No idea about why palmas interrupts would stop working though,
>>>> Peter, do you have any ideas on this one?
>>> No, I don't.
>>> The INT polarity can be changed in Palmas.
>>> based on the pdata->irq_flags (queried via irqd_get_trigger_type())
>>> the code  configures it:
>>>
>>> if (pdata->irq_flags & IRQ_TYPE_LEVEL_HIGH)
>>> 	reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
>>> else
>>> 	reg = 0;
>>>
>>> and we pass the same irq_flags to the regmap_add_irq_chip()
>>> IRQ_TYPE_LEVEL_HIGH == IRQF_TRIGGER_HIGH == 0x00000004
>>>
>>> A change in DT should be enough, no need to patch palmas.c, imho.
>> But it's not. I'm now wondering if wakeupgen is inverting the
>> polarity for this interrupt?
>>
>> GIC docs say this about SPI interrupts:
>>
>> "SPI is triggered on a rising edge or is active-HIGH level-sensitive."
>>
>> So when setting IRQ_TYPE_LEVEL_HIGH in dts, we still must not
>> invert the polarity in palmas while tegra needs to. So either
>> tegra114 hardware is inverting the polarity, or omap5 wakeupgen
>> is.
>>
>> Does the palmas trm say which way PALMAS_POLARITY_CTRL
>> triggers if PALMAS_POLARITY_CTRL_INT_POLARITY is set?
>>
>> Also note that dra7 is using a gpio for palmas interrupt.
> Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
> Tegra114 PMIC interrupt") states that tegra114 inverts the
> polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.
>
> So it seems that commit df545d1cd01a ("mfd: palmas: Provide
> irq flags through DT/platform data") wrongly sets the
> PALMAS_POLARITY_CTRL_INT_POLARITY on IRQ_TYPE_LEVEL_HIGH
> while it should set it on IRQ_TYPE_LEVEL_LOW.

When I implemented, ARM GIC interrupt driver did not support the 
IRQ_TYPE_LEVEL_LOW. If we set this then it produces warning.
[Commit ID commit df545d1cd01aab3ba3f687d5423e6c3687b069d8
mfd: palmas: Provide irq flags through DT/platform data]

So from DT we can not really set the IRQ_TYPE_LEVEL_LOW as irq flag.

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Thierry Reding]

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

On Mon, Nov 19, 2018 at 09:14:06AM -0800, Tony Lindgren wrote:
> Hi,
> 
> * Tony Lindgren <tony@atomide.com> [181119 16:19]:
> > * Peter Ujfalusi <peter.ujfalusi@ti.com> [181119 10:16]:
> > > On 2018-11-13 20:06, Tony Lindgren wrote:
> > > > Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
> > > > should be fixed with IRQ_TYPE_HIGH.
> > > > 
> > > > No idea about why palmas interrupts would stop working though,
> > > > Peter, do you have any ideas on this one?
> > > 
> > > No, I don't.
> > > The INT polarity can be changed in Palmas.
> > > based on the pdata->irq_flags (queried via irqd_get_trigger_type())
> > > the code  configures it:
> > > 
> > > if (pdata->irq_flags & IRQ_TYPE_LEVEL_HIGH)
> > > 	reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
> > > else
> > > 	reg = 0;
> > > 
> > > and we pass the same irq_flags to the regmap_add_irq_chip()
> > > IRQ_TYPE_LEVEL_HIGH == IRQF_TRIGGER_HIGH == 0x00000004
> > > 
> > > A change in DT should be enough, no need to patch palmas.c, imho.
> > 
> > But it's not. I'm now wondering if wakeupgen is inverting the
> > polarity for this interrupt?
> > 
> > GIC docs say this about SPI interrupts:
> > 
> > "SPI is triggered on a rising edge or is active-HIGH level-sensitive."
> > 
> > So when setting IRQ_TYPE_LEVEL_HIGH in dts, we still must not
> > invert the polarity in palmas while tegra needs to. So either
> > tegra114 hardware is inverting the polarity, or omap5 wakeupgen
> > is.
> > 
> > Does the palmas trm say which way PALMAS_POLARITY_CTRL
> > triggers if PALMAS_POLARITY_CTRL_INT_POLARITY is set?
> > 
> > Also note that dra7 is using a gpio for palmas interrupt.
> 
> Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
> Tegra114 PMIC interrupt") states that tegra114 inverts the
> polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.
> 
> So it seems that commit df545d1cd01a ("mfd: palmas: Provide
> irq flags through DT/platform data") wrongly sets the
> PALMAS_POLARITY_CTRL_INT_POLARITY on IRQ_TYPE_LEVEL_HIGH
> while it should set it on IRQ_TYPE_LEVEL_LOW.

Oops, sorry, you seem to have come to pretty much the same conclusion as
I did. I think what we need to do is find a copy of the TRM and see what
exactly the right behaviour is. Or we need to find someone that can take
measurements of the PMIC interrupt pin.

I was unable to find a working link to a copy of the Palmas TRM. Laxman,
do you know of an internal location where we may have a copy?

> I think the fix needs to set the polarity using
> of_machine_is_compatible() and probably also add a new
> compatible to palmas.c for "ti,palmas-tegra114" to properly
> deal with the inverted interrupt. Or add a property for
> "interrupt-inverted". In any case, it seems that the
> of_machine_is_compatible() is also needed too to avoid
> breaking use with dtb files.
> 
> Jon & Thierry, can you guys please check and confirm this?

I'm not sure we need to go to those lengths. As far as I'm concerned, if
it turns out that we've inverted the logic for Tegra114, that's a bug in
the DTS and we should fix it along with the driver to remove the double
negation. I don't think this would be causing any problems with existing
DTBs since, as far as I can tell, the TPS65913 is only used on Dalmore
(which was never publicly available) or Roth and TN7, neither of which I
think anyone ever ran upstream Linux on other than maybe Alex who added
the support. In all of the above cases, there is no problem updating the
DTB since it's all loaded either from eMMC or from an Android boot image
as far as I understand.

But again, I think we need to make sure to get this right, so we need to
find an authoritative source that tells us what exactly the polarity is,
so that we avoid revisiting this a few years down the road.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

* Thierry Reding <treding@nvidia.com> [181126 10:14]:
> On Mon, Nov 19, 2018 at 09:14:06AM -0800, Tony Lindgren wrote:
> > Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
> > Tegra114 PMIC interrupt") states that tegra114 inverts the
> > polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.
> > 
> > So it seems that commit df545d1cd01a ("mfd: palmas: Provide
> > irq flags through DT/platform data") wrongly sets the
> > PALMAS_POLARITY_CTRL_INT_POLARITY on IRQ_TYPE_LEVEL_HIGH
> > while it should set it on IRQ_TYPE_LEVEL_LOW.
> 
> Oops, sorry, you seem to have come to pretty much the same conclusion as
> I did. I think what we need to do is find a copy of the TRM and see what
> exactly the right behaviour is. Or we need to find someone that can take
> measurements of the PMIC interrupt pin.

Yeah so either tegra inverts the PMIC interrupt twice now,
or we have also omap5 wkupgen also inverting the PMIC interrupt
once.. Santosh, do you remember if omap5 wkupgen might be
inverting the palmas interrupt?

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Santosh Shilimkar]

On 11/26/2018 11:14 AM, Tony Lindgren wrote:
> * Thierry Reding <treding@nvidia.com> [181126 10:14]:
>> On Mon, Nov 19, 2018 at 09:14:06AM -0800, Tony Lindgren wrote:
>>> Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
>>> Tegra114 PMIC interrupt") states that tegra114 inverts the
>>> polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.
>>>
>>> So it seems that commit df545d1cd01a ("mfd: palmas: Provide
>>> irq flags through DT/platform data") wrongly sets the
>>> PALMAS_POLARITY_CTRL_INT_POLARITY on IRQ_TYPE_LEVEL_HIGH
>>> while it should set it on IRQ_TYPE_LEVEL_LOW.
>>
>> Oops, sorry, you seem to have come to pretty much the same conclusion as
>> I did. I think what we need to do is find a copy of the TRM and see what
>> exactly the right behaviour is. Or we need to find someone that can take
>> measurements of the PMIC interrupt pin.
> 
> Yeah so either tegra inverts the PMIC interrupt twice now,
> or we have also omap5 wkupgen also inverting the PMIC interrupt
> once.. Santosh, do you remember if omap5 wkupgen might be
> inverting the palmas interrupt?
> 
 From the memory, omap5 wakeupgen doesn't invert the irqlines and
not anything specific to PMIC specially either.

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Tony Lindgren]

* Thierry Reding <treding@nvidia.com> [181126 10:14]:
> I'm not sure we need to go to those lengths. As far as I'm concerned, if
> it turns out that we've inverted the logic for Tegra114, that's a bug in
> the DTS and we should fix it along with the driver to remove the double
> negation. I don't think this would be causing any problems with existing
> DTBs since, as far as I can tell, the TPS65913 is only used on Dalmore
> (which was never publicly available) or Roth and TN7, neither of which I
> think anyone ever ran upstream Linux on other than maybe Alex who added
> the support. In all of the above cases, there is no problem updating the
> DTB since it's all loaded either from eMMC or from an Android boot image
> as far as I understand.

As Jon explained, Tegra PMC inverts the Palmas interrupt twice, so it
gets inverted total three times. So no need to do anything right now,
I'll just change the omap5 PMIC interrupt to use a GPIO instead like
dra7 is already doing.

Thanks everybody for checking the use on Tegra.

Regards,

Tony

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Peter Ujfalusi]

On 19/11/2018 18.19, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [181119 10:16]:
>> On 2018-11-13 20:06, Tony Lindgren wrote:
>>> Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
>>> should be fixed with IRQ_TYPE_HIGH.
>>>
>>> No idea about why palmas interrupts would stop working though,
>>> Peter, do you have any ideas on this one?
>>
>> No, I don't.
>> The INT polarity can be changed in Palmas.
>> based on the pdata->irq_flags (queried via irqd_get_trigger_type())
>> the code  configures it:
>>
>> if (pdata->irq_flags & IRQ_TYPE_LEVEL_HIGH)
>> 	reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
>> else
>> 	reg = 0;
>>
>> and we pass the same irq_flags to the regmap_add_irq_chip()
>> IRQ_TYPE_LEVEL_HIGH == IRQF_TRIGGER_HIGH == 0x00000004
>>
>> A change in DT should be enough, no need to patch palmas.c, imho.
> 
> But it's not. I'm now wondering if wakeupgen is inverting the
> polarity for this interrupt?
> 
> GIC docs say this about SPI interrupts:
> 
> "SPI is triggered on a rising edge or is active-HIGH level-sensitive."
> 
> So when setting IRQ_TYPE_LEVEL_HIGH in dts, we still must not
> invert the polarity in palmas while tegra needs to. So either
> tegra114 hardware is inverting the polarity, or omap5 wakeupgen
> is.
> 
> Does the palmas trm say which way PALMAS_POLARITY_CTRL
> triggers if PALMAS_POLARITY_CTRL_INT_POLARITY is set?

bit7 INT_POLARITY Select the polarity of the INT output line
0: Interrupt line (INT) is low when interrupt is pending (default) RW
1: Interrupt line (INT) is high when interrupt is pending

> Also note that dra7 is using a gpio for palmas interrupt.
> 
> Regards,
> 
> Tony
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki