Messages in this thread | | | Date | Wed, 19 Aug 2020 19:18:54 +0530 | From | skakit@codeauro ... | Subject | Re: [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
| |