Message-ID: <557E9244.7090106@arm.com>
Date:	Mon, 15 Jun 2015 09:52:20 +0100
From:	Sudeep Holla <sudeep.holla@....com>
To:	Doug Anderson <dianders@...omium.org>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
CC:	Sudeep Holla <sudeep.holla@....com>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	"linux-samsung-soc@...r.kernel.org" 
	<linux-samsung-soc@...r.kernel.org>,
	Jason Cooper <jason@...edaemon.net>,
	Chanho Park <parkch98@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Kukjin Kim <kgene@...nel.org>,
	Peter Chubb <peter.chubb@...ta.com.au>,
	Shuah Khan <shuahkhan@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Tomasz Figa <tomasz.figa@...il.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set
 on suspend



On 12/06/15 21:17, Doug Anderson wrote:
> Hi,
>
> On Fri, Jun 12, 2015 at 12:36 PM, Javier Martinez Canillas
> <javier.martinez@...labora.co.uk> wrote:
>>>> registers are lost assuming the combiner was powered down, even the
>>>> status register will be lost and you will not know exactly the wakeup
>>>> reason right ?
>>>>
>>>
>>> Good question, I didn't find in the documentation I've access to that
>>> this happen but just found through experimentation that the IRQ enable
>>> set register values are lost after a resume and that saving/restoring
>>> the values makes the interrupts to be triggered again.
>>>
>>
>> I'll share here too the findings I mentioned over IRC. As you suggested I
>> add some printouts and noticed that the ISTRn (Interrupt Status) registers
>> values are indeed preserved on resume so I can know for example if the
>> wakeup source was the power gpio-key or cros_ec keyboard. I've checked the
>> values of the registers against the Exynos manual and they corresponds to
>> the interrupt sources in each case so the values are correct.
>>
>> So as you said, it seems that is not that the IP block loses its state on
>> S2R but that something is blindly writing the IESRn (Interrupt Enable Set)
>> registers.
>
> I'll postulate an alternate theory here.  You can tell me if you buy
> it or if you think I've been out in the sun too long.
>
> Let's say that the interrupt combiner's status registers show the raw
> status as asserted by whatever is hooked up to the combiner.  This
> means that even if the combiner got reset we could still read the
> status register and get the status of the source.  Imagine that the
> combiner is like a GPIO bank.  If you reset the GPIO bank, you'll lose
> all kinds of config (input vs. output, edge interrupt status, maybe
> pulls, etc).  ...but you can still read the state asserted by an
> external source, right?
>

Right, this makes sense. I assumed status was latched output and might
be lost. But that's wrong assumption I believe.

> In this case the combiner's interrupt source is 'EINT 11'.  ...and I'm
> pretty sure that the controller managine 'EINT 11' _doesn't_ lose
> power across suspend/resume...
>
>
> I'll further bolster my theory by saying that _almost nothing_ in the
> exynos keeps power across suspend/resume.  The UART?  Nope.  The GPIO
> controllers?  Nuh-uh.  The GIC?  Sorry, but no.  The clock tree?  It
> might be nice, but you're out of luck.  ...so it would make me
> terribly surprised to see the combiner keep power.
>

Interesting even GIC loses power ? I would be interested in knowing more
details as who will wake up the system then. Is the wakeup source
offloaded to some external power controller ?

>
>> To reduce the possible s/w components that could be doing this, I booted a
>> signed FIT image directly using the RO U-Boot instead of chain loading a
>> mainline nv-uboot. In this configuration I've the same issue so it seems
>> that if something is zeroing those registers on S2R, this can't be changed
>> without void the warranty of these machines.
>>
>> I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that
>> they have a very similar solution than my patch, the IESRn are also saved
>> but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead
>> or registering a syscore ops but the idea is basically the same.
>
> Yup, you can see where kliegs added it in
> <https://chromium-review.googlesource.com/#/c/27964/>.  As per the
> comments in that CL, this was probably broken in:
>
> 063bd6f ARM: EXYNOS: Remove GIC save & restore function
>
>
>> I have to take a look to the U-boot that is shipped on the device, I think
>> the correct branch is [1] but I'm not sure if that is the correct one.
>
> It is the right one.  If U-Boot were touching this (which would
> greatly surprise me) it should be here:
>
> arch/arm/include/asm/arch-exynos/cpu.h
>
> ...and it's not.  Doing a grep for '10440000' (the combiner base
> address) doesn't find anything in U-Boot either, which makes it less
> likely.  ...and it's even less likely since the amount of code that is
> in U-Boot that runs at resume time is a very small subset and I'm
> fairly familiar with it and I would have remembered it touching the
> combiner.
>

Yes, I did search and find nothing, so definitely not U-Boot if I am
looking at the right source.

> It's POSSIBLE that the internal ROM in exynos is clobbering this, but
> as per above it seems crazy unlikely and I think it's just losing
> power.
>

Agreed, not because I believe ROM code are not crazy but because of the
theory you have provided above :)

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists