Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 17 Aug 2020 16:33:41 -0700
From:   Matthias Kaehlcke <mka@chromium.org>
To:     satya priya <skakit@codeaurora.org>
Cc:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        gregkh@linuxfoundation.org, Andy Gross <agross@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, akashast@codeaurora.org,
        rojay@codeaurora.org, msavaliy@qti.qualcomm.com
Subject: Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for
 BT uart
Message-ID: <20200817233341.GE2995789@google.com>
References: <1595563082-2353-1-git-send-email-skakit@codeaurora.org>
 <1595563082-2353-3-git-send-email-skakit@codeaurora.org>
 <20200817180158.GD2995789@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200817180158.GD2995789@google.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Aug 17, 2020 at 11:01:58AM -0700, Matthias Kaehlcke wrote:
> On Fri, Jul 24, 2020 at 09:28:01AM +0530, satya priya wrote:
> > Add sleep pin ctrl for BT uart, and also change the bias
> > configuration to match Bluetooth module.
> > 
> > Signed-off-by: satya priya <skakit@codeaurora.org>
> > ---
> > Changes in V2:
> >  - This patch adds sleep state for BT UART. Newly added in V2.
> > 
> >  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 42 ++++++++++++++++++++++++++++-----
> >  1 file changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > index 26cc491..bc919f2 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > @@ -469,20 +469,50 @@
> >  
> >  &qup_uart3_default {
> >  	pinconf-cts {
> > -		/*
> > -		 * Configure a pull-down on 38 (CTS) to match the pull of
> > -		 * the Bluetooth module.
> > -		 */
> > +		/* Configure no pull on 38 (CTS) to match Bluetooth module */
> 
> Has the pull from the Bluetooth module been removed or did the previous config
> incorrectly claim that the Bluetooth module has a pull-down?
> 
> >  		pins = "gpio38";
> > +		bias-disable;
> > +	};
> > +
> > +	pinconf-rts {
> > +		/* We'll drive 39 (RTS), so configure pull-down */
> > +		pins = "gpio39";
> > +		drive-strength = <2>;
> >  		bias-pull-down;
> > +	};
> > +
> > +	pinconf-tx {
> > +		/* We'll drive 40 (TX), so no pull */
> 
> The rationales for RTS and TX contradict each other. According to the comment
> the reason to configure a pull-down on RTS is that it is driven by the host.
> Then for TX the reason to configure no pull is that it is driven by the host.
> 
> Please make sure the comments *really* describe the rationale, otherwise they
> are just confusing.

Ok, let's try to reason about the configurations.

I didn't find the datasheet for the WCN3991, but my understanding is that
it is an evolution of the WCN3998, so probably the states of the UART pins
are the same (signal names from the BT chip perspective):

     active   reset
CTS    NP      PD
RTS    NP      PD
RX     NP      PU
TX     NP      PD

Since this patch changes the DT let's use the signal names from the host side
in the following.

> RTS: NP => PD

I can see that this could make sense, a floating pin could indicate
the Bluetooth controller that the host is ready to receive data, when it is
not.

> CTS: PD => NP

From a signalling perspective this should be no problem, since the WCN399x
has a pull-down on its RTS signal in reset, and otherwise will drive it.
IIUC there should be no power leakage without a pull, so I think this
should be ok.

> TX: +output-high

IIUC this only has an impact when the pin is in GPIO mode, i.e. in the sleep
config. If that's correct, does it even make sense to specify it in the default
config?

Besides that, what is the reason for this change? I was told in another forum
that Qualcomm found this to fix problems at UART initialization and wakeup,
without really understanding why. That's not great.

I'm no expert in this area, but my guess is that forcing the TX signal to high
in certain states is needed to not have it floating (no pull is configured),
which could generate garbage on the Bluetooth RX side. But is it really
necessary to actively drive it to high? Wouldn't it be enough to configure a
pull-up when it isn't actively driven (i.e. in sleep mode)?

In a quick test wakeup from Bluetooth worked when configuring a pull-up only in
sleep mode. Could you test this on your side or provide a rationale why TX needs
to be actively driven to high?

Thanks

Matthias