| Message ID | 1644843828-20464-1-git-send-email-quic_vnivarth@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: sc7280: Configure cts sleep pinctrl to bias-disable for sc7280-idp | expand |
+Google Reviewers Hello Reviewers, We were wondering if you had a chance to review this patch and provide any comments. Thank you, Vijay/ On 2/14/2022 6:33 PM, Vijaya Krishna Nivarthi wrote: > WLAN rail was leaking power during RBSC/sleep even after turning BT off. > Change sleep pinctrl configuration to handle same. > > Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > index d623d71..de18319 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > @@ -516,10 +516,10 @@ > pins = "gpio28"; > function = "gpio"; > /* > - * Configure a pull-down on CTS to match the pull of > - * the Bluetooth module. > + * Configure a disable on CTS to lower power usage > + * when BT is turned off. > */ > - bias-pull-down; > + bias-disable; > }; > > qup_uart7_sleep_rts: qup-uart7-sleep-rts {
Hi, On Mon, Feb 14, 2022 at 5:04 AM Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> wrote: > > WLAN rail was leaking power during RBSC/sleep even after turning BT off. > Change sleep pinctrl configuration to handle same. > > Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > index d623d71..de18319 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > @@ -516,10 +516,10 @@ > pins = "gpio28"; > function = "gpio"; > /* > - * Configure a pull-down on CTS to match the pull of > - * the Bluetooth module. > + * Configure a disable on CTS to lower power usage > + * when BT is turned off. > */ > - bias-pull-down; > + bias-disable; Why is sc7280 different from all of the previous devices? Did the Bluetooth firmware change or are we measuring a different case? I know we spent a lot of time carefully choosing each of these pulls before so before changing them we should understand what changed. CTS is an input from the AP's perspective, right? From the AP's perspective then the case we need to be careful of is to prevent this line from every floating while the AP is turned on. Specifically, consider this case: 1. AP is powered on but has no pull on this line 2. The Bluetooth chip is powered off or otherwise configured to not drive this line. In that case the line will be floating. Its voltage will wander around, influenced by other parts of the system. The downside here is that, so I'm told, this will cause power draw on the AP. Each time the voltage on the line floats between trigger points that the AP is watching for it will trigger some logic in the AP and cause power consumption. That's really not ideal. So by disabling this pull you need to be _really_ sure that there's no case where the AP is on and the Bluetooth chip is powered off / not driving the line. In the past I don't think we were convinced of this, which is why we configured a pull but tried to match it with what the Bluetooth chip would do anyway. So... How about using bias-bus-hold instead? That has the advantage of keeping the line from floating but also shouldn't cause a constant power draw. I believe it was created _exactly_ to deal with this type of case. I don't think I was even aware that we supported bias-bus-hold the last time we had this discussion and it seems like it would solve the problem nicely. Does it work for you? -Doug
Thank you very much for very useful inputs. We will discuss further and get back as soon as possible. -Vijay/ -----Original Message----- From: Doug Anderson <dianders@chromium.org> Sent: Wednesday, March 2, 2022 10:12 PM To: Vijaya Krishna Nivarthi (Temp) (QUIC) <quic_vnivarth@quicinc.com> Cc: Andy Gross <agross@kernel.org>; bjorn.andersson@linaro.org; Rob Herring <robh+dt@kernel.org>; linux-arm-msm <linux-arm-msm@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Mukesh Savaliya <msavaliy@qti.qualcomm.com>; Matthias Kaehlcke <mka@chromium.org>; Stephen Boyd <swboyd@chromium.org> Subject: Re: [PATCH] arm64: dts: qcom: sc7280: Configure cts sleep pinctrl to bias-disable for sc7280-idp WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros. Hi, On Mon, Feb 14, 2022 at 5:04 AM Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> wrote: > > WLAN rail was leaking power during RBSC/sleep even after turning BT off. > Change sleep pinctrl configuration to handle same. > > Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > index d623d71..de18319 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > @@ -516,10 +516,10 @@ > pins = "gpio28"; > function = "gpio"; > /* > - * Configure a pull-down on CTS to match the pull of > - * the Bluetooth module. > + * Configure a disable on CTS to lower power usage > + * when BT is turned off. > */ > - bias-pull-down; > + bias-disable; Why is sc7280 different from all of the previous devices? Did the Bluetooth firmware change or are we measuring a different case? I know we spent a lot of time carefully choosing each of these pulls before so before changing them we should understand what changed. CTS is an input from the AP's perspective, right? From the AP's perspective then the case we need to be careful of is to prevent this line from every floating while the AP is turned on. Specifically, consider this case: 1. AP is powered on but has no pull on this line 2. The Bluetooth chip is powered off or otherwise configured to not drive this line. In that case the line will be floating. Its voltage will wander around, influenced by other parts of the system. The downside here is that, so I'm told, this will cause power draw on the AP. Each time the voltage on the line floats between trigger points that the AP is watching for it will trigger some logic in the AP and cause power consumption. That's really not ideal. So by disabling this pull you need to be _really_ sure that there's no case where the AP is on and the Bluetooth chip is powered off / not driving the line. In the past I don't think we were convinced of this, which is why we configured a pull but tried to match it with what the Bluetooth chip would do anyway. So... How about using bias-bus-hold instead? That has the advantage of keeping the line from floating but also shouldn't cause a constant power draw. I believe it was created _exactly_ to deal with this type of case. I don't think I was even aware that we supported bias-bus-hold the last time we had this discussion and it seems like it would solve the problem nicely. Does it work for you? -Doug
We are testing power readings with bias-bus-hold change.
Will update progress.
-----Original Message-----
From: Vijaya Krishna Nivarthi (Temp) (QUIC)
Sent: Friday, March 4, 2022 10:28 PM
To: Doug Anderson <dianders@chromium.org>; Vijaya Krishna Nivarthi (Temp) (QUIC) <quic_vnivarth@quicinc.com>
Cc: Andy Gross <agross@kernel.org>; bjorn.andersson@linaro.org; Rob Herring <robh+dt@kernel.org>; linux-arm-msm <linux-arm-msm@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Mukesh Savaliya <msavaliy@qti.qualcomm.com>; Matthias Kaehlcke <mka@chromium.org>; Stephen Boyd <swboyd@chromium.org>; Dilip Kammath <dkammath@qti.qualcomm.com>
Subject: RE: [PATCH] arm64: dts: qcom: sc7280: Configure cts sleep pinctrl to bias-disable for sc7280-idp
Thank you very much for very useful inputs.
We will discuss further and get back as soon as possible.
-Vijay/
Test readings have shown some degradation with bias-bus-hold compared to bias-disable when device is powered up and BT is off.
We are double checking with team.
-----Original Message-----
From: Vijaya Krishna Nivarthi (Temp)
Sent: Wednesday, March 9, 2022 7:02 PM
To: Vijaya Krishna Nivarthi (Temp) (QUIC) <quic_vnivarth@quicinc.com>; 'Doug Anderson' <dianders@chromium.org>
Cc: 'Andy Gross' <agross@kernel.org>; 'bjorn.andersson@linaro.org' <bjorn.andersson@linaro.org>; 'Rob Herring' <robh+dt@kernel.org>; 'linux-arm-msm' <linux-arm-msm@vger.kernel.org>; 'open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS' <devicetree@vger.kernel.org>; 'LKML' <linux-kernel@vger.kernel.org>; Mukesh Savaliya <msavaliy@qti.qualcomm.com>; 'Matthias Kaehlcke' <mka@chromium.org>; 'Stephen Boyd' <swboyd@chromium.org>; Dilip Kammath <dkammath@qti.qualcomm.com>
Subject: RE: [PATCH] arm64: dts: qcom: sc7280: Configure cts sleep pinctrl to bias-disable for sc7280-idp
We are testing power readings with bias-bus-hold change.
Will update progress.
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi index d623d71..de18319 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi @@ -516,10 +516,10 @@ pins = "gpio28"; function = "gpio"; /* - * Configure a pull-down on CTS to match the pull of - * the Bluetooth module. + * Configure a disable on CTS to lower power usage + * when BT is turned off. */ - bias-pull-down; + bias-disable; }; qup_uart7_sleep_rts: qup-uart7-sleep-rts {
WLAN rail was leaking power during RBSC/sleep even after turning BT off. Change sleep pinctrl configuration to handle same. Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com> --- arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)