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@xxxxxxxxxxxxxx>
Reviewed-by: Akash Asthana <akashast@xxxxxxxxxxxxxx>
---
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
