| Message ID | 20180224200732.6116-1-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On February 24, 2018 12:07:31 PM PST, Marc Zyngier <marc.zyngier@arm.com> 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@gmail.com>
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@arm.com> 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, >
Hi, On Tue, Feb 27, 2018 at 7:05 AM, Heiko Stuebner <heiko@sntech.de> 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@arm.com> > > 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
On 27/02/18 18:47, Doug Anderson wrote: > Hi, > > On Tue, Feb 27, 2018 at 7:05 AM, Heiko Stuebner <heiko@sntech.de> 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@arm.com> >> >> 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.
Hi, On Tue, Feb 27, 2018 at 11:06 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 27/02/18 18:47, Doug Anderson wrote: >> Hi, >> >> On Tue, Feb 27, 2018 at 7:05 AM, Heiko Stuebner <heiko@sntech.de> 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@arm.com> >>> >>> 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
On Sat, Feb 24, 2018 at 9:07 PM, Marc Zyngier <marc.zyngier@arm.com> 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@arm.com> 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
On 01/03/18 09:32, Linus Walleij wrote: > On Sat, Feb 24, 2018 at 9:07 PM, Marc Zyngier <marc.zyngier@arm.com> 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@arm.com> > > 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.
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@arm.com> 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@arm.com> > > 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
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,
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@arm.com> --- drivers/pinctrl/pinctrl-rockchip.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)