| Message ID | CAAfyv37F1s8Cg=pfxiN0dA6YEX7KMB-+uifjRoqAdKGWLeYEwA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
* 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
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
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
* 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
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
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
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
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.
* 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
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
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
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
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
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
* 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
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.
* 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
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
* 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
* 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
* 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
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)