Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Thierry Reding <treding@nvidia.com>]

[-- 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 --]