Re: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

[Marc Zyngier]

On Mon, 19 Feb 2018 18:57:23 +0000,
Heiko Stuebner wrote:
> 
> Am Montag, 19. Februar 2018, 19:03:27 CET schrieb Florian Fainelli:
> > On February 19, 2018 9:25:26 AM PST, Marc Zyngier <maz@xxxxxxxxxxxxxxx> wrote:
> > >Hi all,
> > >
> > >On 2017-03-01 18:32, Florian Fainelli wrote:
> > >> In case a platform only defaults a "default" set of pins, but not a
> > >> "sleep" set of pins, and this particular platform suspends and 
> > >> resumes
> > >> in a way that the pin states are not preserved by the hardware, when 
> > >> we
> > >> resume, we would call pinctrl_single_resume() -> 
> > >> pinctrl_force_default()
> > >> -> pinctrl_select_state() and the first thing we do is check that the
> > >> pins state is the same as before, and do nothing.
> > >>
> > >> In order to fix this, decouple the actual state change from
> > >> pinctrl_select_state() and move it pinctrl_commit_state(), while 
> > >> keeping
> > >> the p->state == state check in pinctrl_select_state() not to change 
> > >> the
> > >> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default()
> > >> are updated to bypass the state check by calling 
> > >> pinctrl_commit_state().
> > >>
> > >> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states
> > >> per device")
> > >> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> > >> ---
> > >> Changes in v3:
> > >>
> > >> - move the state check to pinctrl_select_state
> > >>
> > >> Changes in v2:
> > >>
> > >> - rename __pinctrl_select_state to pinctrl_commit_state
> > >>
> > >>  drivers/pinctrl/core.c | 24 +++++++++++++++++-------
> > >>  1 file changed, 17 insertions(+), 7 deletions(-)
> > >>

Hey Heiko,

> > Hey Marc,
> > 
> > >
> > >I don't often go back over a year worth of LKML, but since this patch 
> > >recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an 
> > >anchor to report the following:
> > >
> > >It turns out that this patch completely breaks resume on my 
> > >rk3399-based Chromebook. Most things are timing out, the box is 
> > >unusable. And since this is my everyday tool, I'm mildly grumpy. Please
> > >
> > >don't break my toys! ;-) Reverting this patch on top of 4.16-rc2 makes 
> > >me productive again...
> > >
> > >More seriously, I have no idea what's wrong here. It could be a 
> > >SoC-related issue, hence Heiko on Cc. I'm happy to test any idea you 
> > >could have.
> > 
> > Can you indicate which DTS file is used for your Chromebook model? Sorry about the breakage.
> 
> that should be
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> 
> I'm vacationing right now, so don't think I'll find time to dive into
> Rockchip pinctrl this week. But I'd guess it could be somehow
> related to the ATF touching pins during suspend/resume?

That'd be really unfortunate. I would have assumed that ATF would
leave things as they were instead of re-configuring them to whatever
default.

The most annoying thing is that if that's indeed the case, we need to
find a solution that will cope with the current state of the
firmware. I guess that'd mean eagerly saving/restoring the pin state
across suspend/resume, irrespective of what firmware could do?

Enjoy your holiday!

	M.

-- 
Jazz is not dead, it just smell funny.