Hi Doug, On 27/02/18 21:47, Douglas Anderson wrote: > Back in the early days when gru devices were still under development > we found an issue where the WiFi reset line needed to be configured as > early as possible during the boot process to avoid the WiFi module > being in a bad state. > > We found that the way to get the kernel to do this in the earliest > possible place was to configure this line in the pinctrl hogs, so > that's what we did. For some history here you can see > <http://crosreview.com/368770>. After the time that change landed in > the kernel, we landed a firmware change to configure this line even > earlier. See <http://crosreview.com/399919>. However, even after the > firmware change landed we kept the kernel change to deal with the fact > that some people working on devices might take a little while to > update their firmware. > > At this there are definitely zero devices out in the wild that have > firmware without the fix in it. Specifically looking in the firmware > branch several critically important fixes for memory stability landed > after the patch in coreboot and I know we didn't ship without those. > Thus, by now, everyone should have the new firmware and it's safe to > not have the kernel set this up in a pinctrl hog. > > Historically, even though it wasn't needed to have this in a pinctrl > hog, we still kept it since it didn't hurt. Pinctrl would apply the > default hog at bootup and then would never touch things again. That > all changed with commit 981ed1bfbc6c ("pinctrl: Really force states > during suspend/resume"). After that commit then we'll re-apply the > default hog at resume time and that can screw up the reset state of > WiFi. ...and on rk3399 if you touch a device on PCIe in the wrong way > then the whole system can go haywire. That's what was happening. > Specifically you'd resume a rk3399-gru-* device and it would mostly > resume, then would crash with some crazy weird crash. > > One could say, perhaps, that the recent pinctrl change was at fault > (and should be fixed) since it changed behavior. ...but that's not > really true. The device tree for rk3399-gru is really to blame. > Specifically since the pinctrl is defined in the hog and not in the > "wlan-pd-n" node then the actual user of this pin doesn't have a > pinctrl entry for it. That's bad. > > Let's fix our problems by just moving the control of > "wlan_module_reset_l pinctrl" out of the hog and put them in the > proper place. > > NOTE: in theory, I think it should actually be possible to have a pin > controlled _both_ by the hog and by an actual device. Once the device > claims the pin I think the hog is supposed to let go. I'm not 100% > sure that this works and in any case this solution would be more > complex than is necessary. > > Reported-by: Marc Zyngier <marc.zyngier@xxxxxxx> > Fixes: 48f4d9796d99 ("arm64: dts: rockchip: add Gru/Kevin DTS") > Fixes: 981ed1bfbc6c ("pinctrl: Really force states during suspend/resume") > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx> > --- > > arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 16 +++------------- > 1 file changed, 3 insertions(+), 13 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi > index 6e50768a34ce..9ad54751d0d8 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi > @@ -406,8 +406,9 @@ > wlan_pd_n: wlan-pd-n { > compatible = "regulator-fixed"; > regulator-name = "wlan_pd_n"; > + pinctrl-names = "default"; > + pinctrl-0 = <&wlan_module_reset_l>; > > - /* Note the wlan_module_reset_l pinctrl */ > enable-active-high; > gpio = <&gpio1 11 GPIO_ACTIVE_HIGH>; > > @@ -988,12 +989,6 @@ ap_i2c_audio: &i2c8 { > pinctrl-0 = < > &ap_pwroff /* AP will auto-assert this when in S3 */ > &clk_32k /* This pin is always 32k on gru boards */ > - > - /* > - * We want this driven low ASAP; firmware should help us, but > - * we can help ourselves too. > - */ > - &wlan_module_reset_l > >; > > pcfg_output_low: pcfg-output-low { > @@ -1173,12 +1168,7 @@ ap_i2c_audio: &i2c8 { > }; > > wlan_module_reset_l: wlan-module-reset-l { > - /* > - * We want this driven low ASAP (As {Soon,Strongly} As > - * Possible), to avoid leakage through the powered-down > - * WiFi. > - */ > - rockchip,pins = <1 11 RK_FUNC_GPIO &pcfg_output_low>; > + rockchip,pins = <1 11 RK_FUNC_GPIO &pcfg_pull_none>; > }; > > bt_host_wake_l: bt-host-wake-l { > In linux-next there are still different issues with s2r, but definitely, this patch has allowed me to enable again the PCIE on my Samsung Chromebook Plus, s2r goes now a bit further :) So many thanks for the patch. Tested-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx> Regards, Enric -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
