Messages in this thread | | | Date | Fri, 11 Sep 2020 10:16:04 +0000 | From | skakit@codeauro ... | Subject | Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART |
| |
Hi Bjorn,
Thanks for reviewing the patches.
On 2020-09-11 05:15, Bjorn Andersson wrote: > On Thu 10 Sep 12:53 UTC 2020, satya priya wrote: > >> Add a suitable sleep configuration for uart3 to support Bluetooth >> wakeup. >> >> If QUP function is selected in sleep state, UART RTS/RFR is pulled >> high >> during suspend and BT SoC not able to send wakeup bytes. So, configure >> GPIO mode in sleep state to keep it low during suspend. >> > > But patch 4 says that you change this behavior, is that patch really > needed if we switch the pins to GPIO, or if this patch really needed if > we merge patch 4? > > Could it be that in lower power states we drop the power to the uart > block and rely on the PDC to wait for the BT chip to start sending the > wakeup bytes on the rx pin? >
As discussed on V4 patch https://patchwork.kernel.org/patch/11753971/#23602723 The patch 4 is good to have, to make sure the UART_MANUAL_RFR is in ready state to receive the wakeup bytes.
> > This commit will become the reference for all other platforms where we > enable the same functionality, so better document it properly. >
Okay.
>> Signed-off-by: satya priya <skakit@codeaurora.org> >> Reviewed-by: Akash Asthana <akashast@codeaurora.org> >> --- >> Changes in V2: >> - This patch adds sleep state for BT UART. Newly added in V2. >> >> Changes in V3: >> - Remove "output-high" for TX from both sleep and default states >> as it is not required. Configure pull-up for TX in sleep state. >> >> Changes in V4: >> - As per Matthias's comment, removed drive-strength for sleep state >> and fixed nit-pick. >> >> Changes in V5: >> - As per Matthias's comments, moved pinmux change for sleep state, >> pinctrl and interrupt config to the board specific file. >> >> arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 >> +++++++++++++++++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> index 04888df..e529a41 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts >> @@ -344,6 +344,10 @@ >> }; >> >> &uart3 { >> + pinctrl-names = "default", "sleep"; >> + pinctrl-1 = <&qup_uart3_sleep>; >> + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, >> + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; >> status = "okay"; >> >> bluetooth: wcn3990-bt { >> @@ -545,3 +549,47 @@ >> }; >> }; >> >> +&tlmm { >> + qup_uart3_sleep: qup-uart3-sleep { >> + pinmux { >> + pins = "gpio38", "gpio39", >> + "gpio40", "gpio41"; >> + function = "gpio"; >> + }; >> + >> + pinconf-cts { >> + /* >> + * Configure a pull-down on CTS to match the pull of >> + * the Bluetooth module. >> + */ >> + pins = "gpio38"; >> + bias-pull-down; >> + }; >> + >> + pinconf-rts { >> + /* >> + * Configure pull-down on RTS to make sure that the BT SoC can >> + * wake up the system by sending wakeup bytes during suspend. > > So "request to send" is active low and pulling it low will indicate to > the BT chip that it's allowed to wake us up by pulling rx low? > > I would like this comment to really describe what's actually going on. >
Ok, will modify the rationale.
>> + */ >> + pins = "gpio39"; >> + bias-pull-down; >> + }; >> + >> + pinconf-tx { >> + /* Configure pull-up on TX when it isn't actively driven */ > > Sure, but why? Wouldn't that be to prevent the BT chip from receiving > garbage while the SoC is asleep? >
yes, this is to prevent the BT chip from receiving garbage, will mention the same.
>> + pins = "gpio40"; >> + bias-pull-up; >> + }; >> + >> + pinconf-rx { >> + /* >> + * Configure a pull-up on RX. This is needed to avoid >> + * garbage data when the TX pin of the Bluetooth module is >> + * in tri-state (module powered off or not driving the >> + * signal yet). >> + */ > > It's nice to avoid "garbage data", but isn't the real reason that the > floating pin on the other side would cause spurious wakeups? >
yes, we need pull-up on RX to prevent spurious wakeups, will modify this comment to mention it.
> Regards, > Bjorn > >> + pins = "gpio41"; >> + bias-pull-up; >> + }; >> + }; >> +}; >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a >> member >> of Code Aurora Forum, hosted by The Linux Foundation >>
Thanks, Satya Priya
| |