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

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

[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*.

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-5wv7dgnIgG8@public.gmane.org>
---
 drivers/pinctrl/pinctrl-rockchip.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 3924779f5578..a3a503e684dc 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -3114,22 +3114,18 @@ static u32 rk3288_grf_gpio6c_iomux;
 static int __maybe_unused rockchip_pinctrl_suspend(struct device *dev)
 {
 	struct rockchip_pinctrl *info = dev_get_drvdata(dev);
-	int ret = pinctrl_force_sleep(info->pctl_dev);
-
-	if (ret)
-		return ret;
 
 	/*
 	 * RK3288 GPIO6_C6 mux would be modified by Maskrom when resume, so save
 	 * the setting here, and restore it at resume.
 	 */
 	if (info->ctrl->type == RK3288) {
+		int ret;
+
 		ret = regmap_read(info->regmap_base, RK3288_GRF_GPIO6C_IOMUX,
 				  &rk3288_grf_gpio6c_iomux);
-		if (ret) {
-			pinctrl_force_default(info->pctl_dev);
+		if (ret)
 			return ret;
-		}
 	}
 
 	return 0;
@@ -3138,14 +3134,18 @@ static int __maybe_unused rockchip_pinctrl_suspend(struct device *dev)
 static int __maybe_unused rockchip_pinctrl_resume(struct device *dev)
 {
 	struct rockchip_pinctrl *info = dev_get_drvdata(dev);
-	int ret = regmap_write(info->regmap_base, RK3288_GRF_GPIO6C_IOMUX,
-			       rk3288_grf_gpio6c_iomux |
-			       GPIO6C6_SEL_WRITE_ENABLE);
 
-	if (ret)
-		return ret;
+	if (info->ctrl->type == RK3288) {
+		int ret;
 
-	return pinctrl_force_default(info->pctl_dev);
+		ret = regmap_write(info->regmap_base, RK3288_GRF_GPIO6C_IOMUX,
+				   rk3288_grf_gpio6c_iomux |
+				   GPIO6C6_SEL_WRITE_ENABLE);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static SIMPLE_DEV_PM_OPS(rockchip_pinctrl_dev_pm_ops, rockchip_pinctrl_suspend,
-- 
2.14.2

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

[Florian Fainelli]

On February 24, 2018 12:07:31 PM PST, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>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*.
>
>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.

Glad you are not reverting the changes I did, but I can't comment whether this is the right think to do on such a Chromebook, FWIW:

Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Florian

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

[Heiko Stuebner]

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*.
> 
> 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-5wv7dgnIgG8@public.gmane.org>

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 :-)
So my guess is that now something bad happens now that the state==oldstate
check is gone.

I've included Doug and Brian who have worked on Gru devices to hopefully
shed some light on this?

@Doug,Brian: In a nutshell, the issue with the commit
"pinctrl: Really force states during suspend/resume" [0] is that now it
really forces the state, instead of first checking of newstate == oldstate
and doing nothing in this case. And as Marc reported this breaks resume
on Kevin with 4.16-rc1.

Any idea what would need to change?


Heiko


[0] https://patchwork.kernel.org/patch/9598969/

>  drivers/pinctrl/pinctrl-rockchip.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index 3924779f5578..a3a503e684dc 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -3114,22 +3114,18 @@ static u32 rk3288_grf_gpio6c_iomux;
>  static int __maybe_unused rockchip_pinctrl_suspend(struct device *dev)
>  {
>  	struct rockchip_pinctrl *info = dev_get_drvdata(dev);
> -	int ret = pinctrl_force_sleep(info->pctl_dev);
> -
> -	if (ret)
> -		return ret;
>  
>  	/*
>  	 * RK3288 GPIO6_C6 mux would be modified by Maskrom when resume, so save
>  	 * the setting here, and restore it at resume.
>  	 */
>  	if (info->ctrl->type == RK3288) {
> +		int ret;
> +
>  		ret = regmap_read(info->regmap_base, RK3288_GRF_GPIO6C_IOMUX,
>  				  &rk3288_grf_gpio6c_iomux);
> -		if (ret) {
> -			pinctrl_force_default(info->pctl_dev);
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
>  	return 0;
> @@ -3138,14 +3134,18 @@ static int __maybe_unused rockchip_pinctrl_suspend(struct device *dev)
>  static int __maybe_unused rockchip_pinctrl_resume(struct device *dev)
>  {
>  	struct rockchip_pinctrl *info = dev_get_drvdata(dev);
> -	int ret = regmap_write(info->regmap_base, RK3288_GRF_GPIO6C_IOMUX,
> -			       rk3288_grf_gpio6c_iomux |
> -			       GPIO6C6_SEL_WRITE_ENABLE);
>  
> -	if (ret)
> -		return ret;
> +	if (info->ctrl->type == RK3288) {
> +		int ret;
>  
> -	return pinctrl_force_default(info->pctl_dev);
> +		ret = regmap_write(info->regmap_base, RK3288_GRF_GPIO6C_IOMUX,
> +				   rk3288_grf_gpio6c_iomux |
> +				   GPIO6C6_SEL_WRITE_ENABLE);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static SIMPLE_DEV_PM_OPS(rockchip_pinctrl_dev_pm_ops, rockchip_pinctrl_suspend,
> 

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

[Doug Anderson]

Hi,

On Tue, Feb 27, 2018 at 7:05 AM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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-5wv7dgnIgG8@public.gmane.org>
>
> 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

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

[Marc Zyngier]

On 27/02/18 18:47, Doug Anderson wrote:
> Hi,
> 
> On Tue, Feb 27, 2018 at 7:05 AM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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?

It sort of resumes, barfs about missing interrupts, various timeouts
from the VOP and thermal sensors, and finally locks up. Not exactly
usable. Either applying this patch or reverting 981ed1bfbc makes it
behave again.

> 
> 
>>> 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-5wv7dgnIgG8@public.gmane.org>
>>
>> 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.

Would you mind posting this patch? I'm happy to give it a go on my setup.

> 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...

How reassuring! ;-)

> 
> ---
> 
> 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.

OK. So this patch is definitely the wrong thing to do.

Thanks for all the context, much appreciated.

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Doug Anderson]

Hi,

On Tue, Feb 27, 2018 at 11:06 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On 27/02/18 18:47, Doug Anderson wrote:
>> Hi,
>>
>> On Tue, Feb 27, 2018 at 7:05 AM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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?
>
> It sort of resumes, barfs about missing interrupts, various timeouts
> from the VOP and thermal sensors, and finally locks up. Not exactly
> usable. Either applying this patch or reverting 981ed1bfbc makes it
> behave again.

OK, then it's the same type of thing I'm seeing.


>>>> 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-5wv7dgnIgG8@public.gmane.org>
>>>
>>> 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.
>
> Would you mind posting this patch? I'm happy to give it a go on my setup.

Sure.  Try out:

https://patchwork.kernel.org/patch/10246027/
arm64: dts: rockchip: Fix rk3399-gru-* s2r (pinctrl hogs, wifi reset)


>> 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...
>
> How reassuring! ;-)

Tell me about it.  It took a REALLY long time to track down a class of
bugs during development because they looked like general instability
(maybe bad voltage, or memory corruption, or bad SDRAM settings, or
...) and it really was just a bad PCIe access.  Sigh.


>> 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.
>
> OK. So this patch is definitely the wrong thing to do.
>
> Thanks for all the context, much appreciated.

-Doug

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

[Linus Walleij]

On Sat, Feb 24, 2018 at 9:07 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:

> 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*.
>
> 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-5wv7dgnIgG8@public.gmane.org>

I assume I should drop this patch for now and that
Dough's long DTS patch with the long explanation was the
right solution to the problem?

Yours,
Linus Walleij

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

[Marc Zyngier]

On 01/03/18 09:32, Linus Walleij wrote:
> On Sat, Feb 24, 2018 at 9:07 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> 
>> 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*.
>>
>> 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-5wv7dgnIgG8@public.gmane.org>
> 
> I assume I should drop this patch for now and that
> Dough's long DTS patch with the long explanation was the
> right solution to the problem?

Absolutely. It also appears that this patch has the potential to break
other Rockchip systems, so let's drop it.

Thanks again,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Heiko Stübner]

Hi Linus,

Am Donnerstag, 1. März 2018, 10:32:22 CET schrieb Linus Walleij:
> On Sat, Feb 24, 2018 at 9:07 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> > 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*.
> > 
> > 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-5wv7dgnIgG8@public.gmane.org>
> 
> I assume I should drop this patch for now and that
> Dough's long DTS patch with the long explanation was the
> right solution to the problem?

Correct this patch is not needed.

Thanks
Heiko