Add wakeup support over UART RX
Subsystems (5)
v5 cover Add wakeup support over UART RX
Changes in V2:
- As per Matthias's comment added wakeup support for all the UARTs of SC7180.
- Added sleep state in sc7180-idp.dts file.
- Modify the if check in set_mctrl API in serial driver to avoid making RFR high during suspend.
Changes in V3:
- As per Matthias's comments modify the idp dts pin config to keep only the required pin settings.
- Remove the extra parentheses from serial driver patch.
Changes in V4:
- As per Matthias's comments, change the commit text to mention why GPIO function needs to be selected in sleep.
- Add separate patch for improvements made in pin conf settings.
Changes in V5:
- Moved pinctrl and interrupt configuration to board specific files.
- Added new patch for trogdor board specific changes.
satya priya (4):
arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and
TX
arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config
for BT UART
arm64: dts: qcom: sc7180-trogdor: Add pinctrl and interrupt config for
BT UART
tty: serial: qcom_geni_serial: Fix the UART wakeup issue
arch/arm64/boot/dts/qcom/sc7180-idp.dts | 58 +++++++++++++++++++++++++--- arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 48 +++++++++++++++++++++++ drivers/tty/serial/qcom_geni_serial.c | 2 +- 3 files changed, 101 insertions(+), 7 deletions(-)
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v5 1/4 arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX
Remove output-high from CTS and TX as this is not really required. During bringup to fix transfer failures this was added to match with console uart settings. Probably some boot loader config was missing then. As it is working fine now, remove it.
---
Changes in V4:
- This is newly added in V4 to separate the improvements in pin settings and wakeup related changes.
Changes in V5:
- As per Doug's comment configured pull-down for CTS pin as earlier.
arch/arm64/boot/dts/qcom/sc7180-idp.dts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/qcom/sc7180-idp.dts +4 -6
@@ -474,32 +474,30 @@ &qup_uart3_default { pinconf-cts { /* - * Configure a pull-down on 38 (CTS) to match the pull of + * Configure a pull-down on CTS to match the pull of * the Bluetooth module. */ pins = "gpio38"; bias-pull-down; - output-high; }; pinconf-rts { - /* We'll drive 39 (RTS), so no pull */ + /* We'll drive RTS, so no pull */ pins = "gpio39"; drive-strength = <2>; bias-disable; }; pinconf-tx { - /* We'll drive 40 (TX), so no pull */ + /* We'll drive TX, so no pull */ pins = "gpio40"; drive-strength = <2>; bias-disable; - output-high; }; pinconf-rx { /* - * Configure a pull-up on 41 (RX). This is needed to avoid + * 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).
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH V5 1/4] arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX
Hi,
Quoted 14 lines · satya priya
Remove output-high from CTS and TX as this is not really required. During bringup to fix transfer failures this was added to match with console uart settings. Probably some boot loader config was missing then. As it is working fine now, remove it.
Signed-off-by satya priya <[email protected]>Reviewed-by Akash Asthana <[email protected]>Reviewed-by Matthias Kaehlcke <[email protected]>---
Changes in V4:
- This is newly added in V4 to separate the improvements in pin settings and wakeup related changes.
Quoted 5 lines
Changes in V5:
- As per Doug's comment configured pull-down for CTS pin as earlier.arch/arm64/boot/dts/qcom/sc7180-idp.dts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
Looks fine to me. Slight nit that this only applies to the IDP board but ${SUBJECT} makes it sound as if this applies to all sc7180. I wouldn't spin just for that, though. If Bjorn agrees, he can always adjust the subject when applying.
Re: [PATCH V5 1/4] arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX
Hi Doug,
Quoted 24 lines · Doug Anderson
Hi,
Quoted 15 lines · satya priya
Remove output-high from CTS and TX as this is not really required. During bringup to fix transfer failures this was added to match with console uart settings. Probably some boot loader config was missing then. As it is working fine now, remove it.
Signed-off-by satya priya <[email protected]>Reviewed-by Akash Asthana <[email protected]>Reviewed-by Matthias Kaehlcke <[email protected]>---
Changes in V4:
- This is newly added in V4 to separate the improvements in pin
settings
and wakeup related changes.Quoted 5 lines
Changes in V5:
- As per Doug's comment configured pull-down for CTS pin as earlier.arch/arm64/boot/dts/qcom/sc7180-idp.dts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
Quoted 6 lines
Looks fine to me. Slight nit that this only applies to the IDP board but ${SUBJECT} makes it sound as if this applies to all sc7180. I wouldn't spin just for that, though. If Bjorn agrees, he can always adjust the subject when applying.
Reviewed-by Douglas Anderson <[email protected]>
Thanks for reviewing the patches, i will correct this nit in my next version.
Thanks,
Satya Priya
PATCH v5 2/4 arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
In reply to satya priyaAdd 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.
---
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(+)
arch/arm64/boot/dts/qcom/sc7180-idp.dts +48 -0
@@ -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. + */ + pins = "gpio39"; + bias-pull-down; + }; + + pinconf-tx { + /* Configure pull-up on TX when it isn't actively driven */ + 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). + */ + pins = "gpio41"; + bias-pull-up; + }; + }; +};
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
Hi,
Quoted 21 lines · satya priya
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.
Signed-off-by satya priya <[email protected]>Reviewed-by Akash Asthana <[email protected]>---
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.
Quoted 6 lines
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(+)
Similar comment to patch #1 in that this applies only to the IDP board but that's not obvious from ${SUBJECT}
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 350
Quoted 12 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +4
-0@@ -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>;
You need a:
/delete-property/interrupts;
...or, alternatively, a patch before this one that converts all the UARTs in sc7180 to just use interrupts-extended.
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 351
Quoted 1 line
arch/arm64/boot/dts/qcom/sc7180-idp.dts +0
-0status = "okay";
Slight nit is that usually I see the status line first. All the other instances in this file have it that way. Can you match?
-Doug
Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
Quoted 28 lines · Doug Anderson
Hi,
Quoted 25 lines · satya priya
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.
Signed-off-by satya priya <[email protected]>Reviewed-by Akash Asthana <[email protected]>---
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.
Quoted 7 lines
Quoted 3 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 +++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)Similar comment to patch #1 in that this applies only to the IDP board but that's not obvious from ${SUBJECT}
Okay.
Quoted 16 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 350
Quoted 13 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +4
-0@@ -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>;You need a:
Quoted 5 lines
/delete-property/interrupts;
...or, alternatively, a patch before this one that converts all the UARTs in sc7180 to just use interrupts-extended.
Sure, I will add this. But I think when both are added, "interrupts-extended" will get priority as per [1] and there wouldn't be any problem.
[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 351
Quoted 2 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +0
-0status = "okay";Slight nit is that usually I see the status line first. All the other instances in this file have it that way. Can you match?
Ok, will correct it.
Quoted 4 lines
-Doug
Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
Quoted 61 lines · On Fri 11 Sep 05:15 CDT 2020, [email protected] wrote:
Quoted 31 lines · Doug Anderson
Hi,
Quoted 27 lines · satya priya
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.Signed-off-by satya priya <[email protected]>Reviewed-by Akash Asthana <[email protected]>---
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.Quoted 7 lines
Quoted 3 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 +++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)Similar comment to patch #1 in that this applies only to the IDP board but that's not obvious from ${SUBJECT}
Okay.
Quoted 19 lines
Quoted 7 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +0
-0arch/arm64/boot/dts/qcom/sc7180-idp.dts line 350
Quoted 7 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +4
-0@@ -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>;You need a:
/delete-property/interrupts;
Quoted 8 lines
Quoted 3 lines
...or, alternatively, a patch before this one that converts all the UARTs in sc7180 to just use interrupts-extended.
Sure, I will add this. But I think when both are added, "interrupts-extended" will get priority as per [1] and there wouldn't be any problem.
You're indeed correct, please stick with what you have.
Regards,
Bjorn
Quoted 15 lines
[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 351
Quoted 2 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +0
-0status = "okay";Slight nit is that usually I see the status line first. All the other instances in this file have it that way. Can you match?
Ok, will correct it.
Quoted 4 lines
-Doug
Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
Hi,
Quoted 15 lines · Bjorn Andersson
Quoted 15 lines
Quoted 14 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 350
Quoted 6 lines
&uart3 {
arch/arm64/boot/dts/qcom/sc7180-idp.dts +4
-0+ 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>;You need a:
/delete-property/interrupts;
...or, alternatively, a patch before this one that converts all the UARTs in sc7180 to just use interrupts-extended.
Quoted 6 lines
Quoted 4 lines
Sure, I will add this. But I think when both are added, "interrupts-extended" will get priority as per [1] and there wouldn't be any problem.
You're indeed correct, please stick with what you have.
IMO this is ugly, but I won't fight too hard on it if you guys really want to do it that way. Reading the documentation it sounds as if allowing for both is intended to be used in cases where the same device tree might be used on old software (that didn't understand interrupts-extended) and on new software (that does). The interrupts-extended property by far predates any sc7180 support, though, so I can't imagine anyone really needing to use that these days. Is it really that bad to add the /delete-property/ to end up with a cleaner final device tree?
-Doug
Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
In reply to satya priyaEl Thu, Sep 10, 2020 at 06:23:56PM +0530 satya priya ha dit:
Quoted 8 lines
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.
Signed-off-by satya priya <[email protected]>Reviewed-by Akash Asthana <[email protected]>
Was this change really reviewed (privately) by Akash or are you still carrying forward this tag from v2? The configuration and the comments have change substantially since v2, IMO you should drop the tag unless Akash really reviewed the current version or something close to it.
Quoted 24 lines
---
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(+)arch/arm64/boot/dts/qcom/sc7180-idp.dts +0
-0
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 350
Quoted 7 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +4
-0@@ -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>;
As Doug already said, you need to delete the 'interrupts' property now that we have 'interrupts-extended'.
Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
Hi Matthias,
Quoted 8 lines · Matthias Kaehlcke
El Thu, Sep 10, 2020 at 06:23:56PM +0530 satya priya ha dit:
Quoted 6 lines
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.
Quoted 8 lines
Quoted 2 lines
Signed-off-by satya priya <[email protected]>Reviewed-by Akash Asthana <[email protected]>Was this change really reviewed (privately) by Akash or are you still carrying forward this tag from v2? The configuration and the comments have change substantially since v2, IMO you should drop the tag unless Akash really reviewed the current version or something close to it.
Okay, will drop the tag.
Quoted 27 lines
Quoted 27 lines
---
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(+)arch/arm64/boot/dts/qcom/sc7180-idp.dts +0
-0@@ -344,6 +344,10 @@ };
Quoted 8 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 350
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +4
-0&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>;As Doug already said, you need to delete the 'interrupts' property now that we have 'interrupts-extended'.
okay.
Thanks,
Satya Priya
Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
In reply to satya priyaQuoted 6 lines · satya priya
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?
This commit will become the reference for all other platforms where we enable the same functionality, so better document it properly.
Quoted 51 lines
Signed-off-by satya priya <[email protected]>Reviewed-by Akash Asthana <[email protected]>---
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(+)arch/arm64/boot/dts/qcom/sc7180-idp.dts +15
-0@@ -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
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 572
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +10
-0+ * 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.
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 579
Quoted 7 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +7
-0+ */ + 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?
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 590
Quoted 11 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +11
-0+ 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?
Regards,
Bjorn
Quoted 9 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +5
-0+ pins = "gpio41"; + bias-pull-up; + }; + }; +};Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
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.
Quoted 9 lines · Bjorn Andersson
Quoted 6 lines · satya priya
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.
Quoted 8 lines
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.
Quoted 4 lines
This commit will become the reference for all other platforms where we enable the same functionality, so better document it properly.
Okay.
Quoted 66 lines
Quoted 52 lines
Signed-off-by satya priya <[email protected]>Reviewed-by Akash Asthana <[email protected]>---
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(+)arch/arm64/boot/dts/qcom/sc7180-idp.dts +15
-0@@ -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 ofarch/arm64/boot/dts/qcom/sc7180-idp.dts line 572
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +10
-0+ * 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?
Quoted 2 lines
I would like this comment to really describe what's actually going on.
Ok, will modify the rationale.
Quoted 11 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 579
Quoted 7 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +7
-0+ */ + 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.
Quoted 6 lines
Quoted 6 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +6
-0+ pins = "gpio40"; + bias-pull-up; + }; + + pinconf-rx { + /*
Quoted 9 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts line 590
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +5
-0+ * 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.
Quoted 12 lines
Regards,
BjornQuoted 9 lines
arch/arm64/boot/dts/qcom/sc7180-idp.dts +5
-1+ 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
PATCH v5 3/4 arm64: dts: qcom: sc7180-trogdor: Add pinctrl and interrupt config for BT UART
In reply to satya priyaAdd 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.
---
Changes in V5:
- Newly added in V5. This patch adds wakeup support for trogdor board files.
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 48 ++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +48 -0
@@ -792,6 +792,11 @@ ap_spi_fp: &spi10 { #include <arm/cros-ec-sbs.dtsi> &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: bluetooth {
@@ -1345,4 +1350,47 @@ ap_spi_fp: &spi10 { drive-strength = <2>; }; }; + + 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. + */ + pins = "gpio39"; + bias-pull-down; + }; + + pinconf-tx { + /* Configure pull-up on TX when it isn't actively driven */ + 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). + */ + pins = "gpio41"; + bias-pull-up; + }; + }; };
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH V5 3/4] arm64: dts: qcom: sc7180-trogdor: Add pinctrl and interrupt config for BT UART
Hi,
Quoted 7 lines · satya priya
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.
Quoted 7 lines
Signed-off-by satya priya <[email protected]>---
Changes in V5:
- Newly added in V5. This patch adds wakeup support for trogdor board files.arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 48 ++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
Note: I can't find this email on any of the mailing lists. Can you check your config? I tried:
http://lore.kernel.org/r/[email protected]
...and also checked patchwork servers. I only see patch 1 and 2. I think Bjorn usually applies from patchwork so this'll likely be a problem...
Quoted 5 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +0
-0
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi ap_spi_fp: &spi10 { · line 800
Quoted 9 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +5
-0@@ -792,6 +792,11 @@ ap_spi_fp: &spi10 { #include <arm/cros-ec-sbs.dtsi> &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";
Same comments here as for patch #2.
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi ap_spi_fp: &spi10 { · line 1354
Quoted 7 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +2
-0bluetooth: bluetooth {@@ -1345,4 +1350,47 @@ ap_spi_fp: &spi10 { drive-strength = <2>; }; }; + + qup_uart3_sleep: qup-uart3-sleep {
I believe things in this section are supposed to be sorted alphabetically. Thus "qup..." should be sorted before "trackpad..."
Re: [PATCH V5 3/4] arm64: dts: qcom: sc7180-trogdor: Add pinctrl and interrupt config for BT UART
Quoted 20 lines · Doug Anderson
Hi,
Quoted 8 lines · satya priya
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.
Signed-off-by satya priya <[email protected]>Quoted 8 lines
---
Changes in V5:
- Newly added in V5. This patch adds wakeup support for trogdor board
files.arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 48 ++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
Quoted 9 lines
Note: I can't find this email on any of the mailing lists. Can you check your config? I tried:
http://lore.kernel.org/r/[email protected]
...and also checked patchwork servers. I only see patch 1 and 2. I think Bjorn usually applies from patchwork so this'll likely be a problem...
I guess it is updated, now it is showing all the 4 patches
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=346637
Quoted 8 lines
Quoted 8 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +0
-0@@ -792,6 +792,11 @@ ap_spi_fp: &spi10 { #include <arm/cros-ec-sbs.dtsi>
Quoted 10 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi ap_spi_fp: &spi10 { · line 800
Quoted 7 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +5
-0&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";Same comments here as for patch #2.
Ok.
Quoted 11 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi ap_spi_fp: &spi10 { · line 1354
Quoted 8 lines
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +2
-0bluetooth: bluetooth {@@ -1345,4 +1350,47 @@ ap_spi_fp: &spi10 { drive-strength = <2>; }; }; + + qup_uart3_sleep: qup-uart3-sleep {I believe things in this section are supposed to be sorted alphabetically. Thus "qup..." should be sorted before "trackpad..."
Ok.
PATCH v5 4/4 tty: serial: qcom_geni_serial: Fix the UART wakeup issue
In reply to satya priyaAs a part of system suspend uart_port_suspend is called from the Serial driver, which calls set_mctrl passing mctrl as 0. This makes RFR high(NOT_READY) during suspend.
Due to this BT SoC is not able to send wakeup bytes to UART during suspend. Include if check for non-suspend case to keep RFR low during suspend.
---
Changes in V2:
- This patch fixes the UART flow control issue during suspend. Newly added in V2.
Changes in V3:
- As per Matthias's comment removed the extra parentheses.
Changes in V4:
- No change.
Changes in V5:
- As per Matthias comment, fixed nit-pick in commit text.
drivers/tty/serial/qcom_geni_serial.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/tty/serial/qcom_geni_serial.c +1 -1
@@ -242,7 +242,7 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport,if (mctrl &TIOCM_LOOP ) port->loopback =RX_TX_CTS_RTS_SORTED ; -if (!(mctrl &TIOCM_RTS )) +if (!(mctrl &TIOCM_RTS ) && !uport->suspended) uart_manual_rfr =UART_MANUAL_RFR_EN |UART_RFR_NOT_READY ;writel (uart_manual_rfr, uport->membase +SE_UART_MANUAL_RFR ); }
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH V5 4/4] tty: serial: qcom_geni_serial: Fix the UART wakeup issue
Quoted 8 lines · satya priya
As a part of system suspend
uart_port_suspendis called from the Serial driver, which callsset_mctrlpassing mctrl as 0. This makes RFRhigh(NOT_READY)during suspend.Due to this BT SoC is not able to send wakeup bytes to UART during suspend. Include if check for non-suspend case to keep RFR low during suspend.
Seems reasonable.
Quoted 2 lines
Signed-off-by satya priya <[email protected]>
Greg, I don't see this depending on anything else, will you pick this patch through your tree? I will take the dts patches through the qcom tree.
Regards,
Bjorn
Quoted 35 lines
Reviewed-by Akash Asthana <[email protected]>---
Changes in V2:
- This patch fixes the UART flow control issue during suspend. Newly added in V2.Changes in V3:
- As per Matthias's comment removed the extra parentheses.Changes in V4:
- No change.Changes in V5:
- As per Matthias comment, fixed nit-pick in commit text.drivers/tty/serial/qcom_geni_serial.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/tty/serial/qcom_geni_serial.c +1
-1@@ -242,7 +242,7 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport,if (mctrl &TIOCM_LOOP ) port->loopback =RX_TX_CTS_RTS_SORTED ; -if (!(mctrl &TIOCM_RTS )) +if (!(mctrl &TIOCM_RTS ) && !uport->suspended) uart_manual_rfr =UART_MANUAL_RFR_EN |UART_RFR_NOT_READY ;writel (uart_manual_rfr, uport->membase +SE_UART_MANUAL_RFR ); }Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Greg KH
maintainerRe: [PATCH V5 4/4] tty: serial: qcom_geni_serial: Fix the UART wakeup issue
Quoted 13 lines · Bjorn Andersson
Quoted 8 lines · satya priya
As a part of system suspend
uart_port_suspendis called from the Serial driver, which callsset_mctrlpassing mctrl as 0. This makes RFRhigh(NOT_READY)during suspend.Due to this BT SoC is not able to send wakeup bytes to UART during suspend. Include if check for non-suspend case to keep RFR low during suspend.
Seems reasonable.
Quoted 8 lines
Reviewed-by Bjorn Andersson <[email protected]>Quoted 2 lines
Signed-off-by satya priya <[email protected]>Greg, I don't see this depending on anything else, will you pick this patch through your tree? I will take the dts patches through the qcom tree.
Sure, will pick it up now, thanks.
greg k-h