linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marc Zyngier <marc.zyngier@arm.com>
To: Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	Douglas Anderson <dianders@chromium.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Brian Norris <briannorris@chromium.org>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Matthias Kaehlcke <mka@chromium.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-rockchip@lists.infradead.org,
	Rob Herring <robh+dt@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	Will Deacon <will.deacon@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Jeffy Chen <jeffy.chen@rock-chips.com>
Subject: Re: [PATCH] arm64: dts: rockchip: Fix rk3399-gru-* s2r (pinctrl hogs, wifi reset)
Date: Wed, 28 Feb 2018 14:01:12 +0000	[thread overview]
Message-ID: <b1c328ac-33e3-363c-9072-3fe4c9e5438e@arm.com> (raw)
In-Reply-To: <fe5c8c52-2d01-9244-23b5-3641bbe3cbd0@collabora.com>

On 28/02/18 12:19, Enric Balletbo i Serra wrote:
> 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@arm.com>
>> Fixes: 48f4d9796d99 ("arm64: dts: rockchip: add Gru/Kevin DTS")
>> Fixes: 981ed1bfbc6c ("pinctrl: Really force states during suspend/resume")
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>
>>  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@collabora.com>
For the record, I maintain a small branch[1] based on the latest
mainline, containing fixes only, and not dragging the whole of -next
into an already fairly unstable kernel. Most things do work (including
s2r), apart from USB3, external display, and some of the sound stuff.
Crucially, kexec is now functional, allowing the kernel to be used as a
bootloader...

	M.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=hack/kevin-4.16
-- 
Jazz is not dead. It just smells funny...

  reply	other threads:[~2018-02-28 14:01 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-27 20:47 [PATCH] arm64: dts: rockchip: Fix rk3399-gru-* s2r (pinctrl hogs, wifi reset) Douglas Anderson
2018-02-28 12:19 ` Enric Balletbo i Serra
2018-02-28 14:01   ` Marc Zyngier [this message]
2018-02-28 13:51 ` Marc Zyngier
2018-03-01  8:43 ` Heiko Stübner
2018-03-06 11:58   ` Marc Zyngier
2018-03-06 18:00     ` Doug Anderson
2018-03-06 18:15       ` Marc Zyngier
2018-03-06 18:49         ` Heiko Stübner
2018-03-06 18:58           ` Marc Zyngier
2018-03-07  4:54             ` Doug Anderson
2018-03-12 13:31               ` Heiko Stübner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b1c328ac-33e3-363c-9072-3fe4c9e5438e@arm.com \
    --to=marc.zyngier@arm.com \
    --cc=briannorris@chromium.org \
    --cc=catalin.marinas@arm.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=enric.balletbo@collabora.com \
    --cc=f.fainelli@gmail.com \
    --cc=heiko@sntech.de \
    --cc=jeffy.chen@rock-chips.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=mark.rutland@arm.com \
    --cc=mka@chromium.org \
    --cc=robh+dt@kernel.org \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).