Re: [PATCH] pinctrl/rockchip: Don't call pinctrl_force_* for nothing

[Doug Anderson <dianders@xxxxxxxxxxxx>]

Hi,

On Tue, Feb 27, 2018 at 7:05 AM, Heiko Stuebner <heiko@xxxxxxxxx> wrote:
> Hi Marc,
>
> Am Samstag, 24. Februar 2018, 21:07:31 CET schrieb Marc Zyngier:
>> The rockchip pinctl driver calls pinctrl_force_default and
>> pinctrl_force_sleep on suspend resume, but seems to expect
>> that the outcome of these calls will be that nothing happens,
>> as the core code checks whether we're already in the right
>> state or not.
>>
>> Or at least, that was what the core code was doing until
>> 981ed1bfbc ("pinctrl: Really force states during suspend/resume"),
>> which gives the "force" qualifier its actual meaning.
>>
>> In turn, this breaks suspend/resume on the rk3399. So let's
>> change the rockchip code to do what it should have done from
>> the very begining, which is exactly *nothing*.

Can you explain how exactly the patch broke suspend/resume?  I just
tried this once and it looks like the system resumes that later kinda
barfs all over the place.  Is that right?


>> We take this opportunity to tidy-up the RK3288 GPIO6_C6 mux
>> resume workaround, making it symetrical to the suspend path.
>>
>> Tested on a rk3399-based kevin Chromebook.
>>
>> Fixes: 9198f509c888 ("pinctrl: rockchip: add suspend/resume functions")
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>
> Now that I had time to look at this a bit more, I'm not sure if removing
> that is the right solution.
>
> The force_sleep and force_suspend functions are meant for the "hog" pins,
> meaning the pins that just need to be configured once but are not claimed
> by any actual device - see everything named hog_* in the pinctrl core.
> And judging by the other users of them, using them like now is the expected
> usage.
>
>
> On the rk3399-gru boards, the hogged pins only have a default state and
> no sleep configuration and contain some pins like the ap_pwroff that
> sound somewhat strange :-)

The "ap_pwroff" pin is a special-purpose function on rk3399.  When
this pin is configured as its special purpose it will automatically
indicate whether the system is suspended or not.  ...so we just need
to configure it once at bootup and it will continue to do its job.
There is no "driver" to own this pin which is why it's listed in the
hog.  So I don't think there's any issue with that pin.


> So my guess is that now something bad happens now that the state==oldstate
> check is gone.

If I had to guess I'd first look at "wlan_module_reset_l".
Specifically for this pin:

At bootup we wanted Linux to grab this pin and drive it low ASAP, much
before any WiFi-related drivers could get to it.  Basically the pin
was in a bad state (back-powering the WiFi module IIRC) and we wanted
to get out of that state ASAP.

In reality even this early in kernel was a bit late to do this.  We
really needed the firmware to do it, and certainly before we shipped
we did put this in firmware.  However, we left the kernel change in
there because:
1. It's nice not to rely on firmware.
2. It shouldn't hurt to have this in the kernel.


OK, so in my mind I had a recollection that this pin was claimed by
_both_ the hog and the true user of this pin.  In my mind that was an
OK thing to do because the "hog" state would be the state of this pin
until a real driver came along and claimed true ownership of it.

...but looking at the actual dts that landed (and that I even reviewed
at <http://crosreview.com/368770>), we didn't list the pinctrl in the
actual "regulator" node.  Thus if the hog gets re-applied sometime
after bootup then we'll be in bad state.

---

So what's the fix here?  Probably we can just move
"wlan_module_reset_l" out of the hog and put it in the "wlan-pd-n"
node where it belongs.  We'd essentially be relying on the fact that
this was fixed in firmware shortly after the kernel change landed and
there's a 0% change anyone has the old firmware.


In theory you could also try adding the pinctrl entry to the
"wlan-pd-n" node and leave it as a hog.  I think that's supposed to
work.  ...but after doing this, you'd need to make sure that the hog
properly releases control of the pin as long as it's claimed by
"wlan-pd-n" and doesn't try to re-apply the hog at suspend/resume
time.

---

Anyway, hope that helps.  I tried making this change and doing a
suspend/resume and it didn't barf on me anymore, but I didn't stress
it out.

NOTE: if you're wondering why the WiFi reset could make things so
badly behaved, we've definitely found that when the PCIe bus is in a
bad state that the whole system in general is just pretty broken...

---

Oh, one last thing: we definitely do need to make sure that the
sleep/default hogs are applied on rk3288-veyron.  On those systems
we'd be in pretty bad shape if the "sleep" hogs didn't get applied at
suspend time.


-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html