lkml.org 
[lkml]   [2020]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
Hi Matthias,

Thanks for reviewing the patches.

On 2020-08-18 05:03, Matthias Kaehlcke wrote:
> 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.
>

With CTS having no-pull, we are not seeing any power leakages.

>> 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.
>

"output-high" was present in IDP dts since Bring-up, we've validated on
the latest code-base and see that "output-high" is not required, will
remove it.

> 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?
>

We have tested by keeping pull-up for TX in sleep state(removed
output-high) and wakeup is working fine with Bluetooth. Will remove the
output-high from both default and sleep states.

> Thanks
>
> Matthias

Thanks,
Satya Priya

\
 
 \ /
  Last update: 2020-08-19 15:50    [W:2.069 / U:0.728 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site