* [PATCH V2 0/3] Add wakeup support over UART RX
@ 2020-07-24 3:57 satya priya
2020-07-24 3:58 ` [PATCH V2 1/3] arm64: dts: sc7180: " satya priya
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: satya priya @ 2020-07-24 3:57 UTC (permalink / raw)
To: Bjorn Andersson, gregkh
Cc: Andy Gross, Rob Herring, Matthias Kaehlcke, linux-arm-msm,
devicetree, linux-kernel, akashast, rojay, msavaliy, satya priya
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.
Hi Greg,
These patches are based on qcom tree. Please ack the serial driver
patch to land via qcom-tree.
Thanks,
Satya Priya
satya priya (3):
arm64: dts: sc7180: Add wakeup support over UART RX
arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
tty: serial: qcom_geni_serial: Fix the UART wakeup issue
arch/arm64/boot/dts/qcom/sc7180-idp.dts | 42 ++++++++++++--
arch/arm64/boot/dts/qcom/sc7180.dtsi | 98 ++++++++++++++++++++++++++++-----
drivers/tty/serial/qcom_geni_serial.c | 2 +-
3 files changed, 121 insertions(+), 21 deletions(-)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH V2 1/3] arm64: dts: sc7180: Add wakeup support over UART RX 2020-07-24 3:57 [PATCH V2 0/3] Add wakeup support over UART RX satya priya @ 2020-07-24 3:58 ` satya priya 2020-07-28 5:36 ` Akash Asthana 2020-07-24 3:58 ` [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart satya priya 2020-07-24 3:58 ` [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue satya priya 2 siblings, 1 reply; 17+ messages in thread From: satya priya @ 2020-07-24 3:58 UTC (permalink / raw) To: Bjorn Andersson, gregkh Cc: Andy Gross, Rob Herring, Matthias Kaehlcke, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy, satya priya Add the necessary pinctrl and interrupts to make UART wakeup capable. Signed-off-by: satya priya <skakit@codeaurora.org> --- Changes in V2: - As per Matthias's comment added wakeup support for all the UARTs of SC7180. arch/arm64/boot/dts/qcom/sc7180.dtsi | 98 ++++++++++++++++++++++++++++++------ 1 file changed, 84 insertions(+), 14 deletions(-) diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi index 16df08d..044a4d0 100644 --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi @@ -787,9 +787,11 @@ reg = <0 0x00880000 0 0x4000>; clock-names = "se"; clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>; - pinctrl-names = "default"; + pinctrl-names = "default", "sleep"; pinctrl-0 = <&qup_uart0_default>; - interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-1 = <&qup_uart0_sleep>; + interrupts-extended = <&intc GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>, + <&tlmm 37 IRQ_TYPE_EDGE_FALLING>; power-domains = <&rpmhpd SC7180_CX>; operating-points-v2 = <&qup_opp_table>; interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>, @@ -839,9 +841,11 @@ reg = <0 0x00884000 0 0x4000>; clock-names = "se"; clocks = <&gcc GCC_QUPV3_WRAP0_S1_CLK>; - pinctrl-names = "default"; + pinctrl-names = "default", "sleep"; pinctrl-0 = <&qup_uart1_default>; - interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-1 = <&qup_uart1_sleep>; + interrupts-extended = <&intc GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>, + <&tlmm 3 IRQ_TYPE_EDGE_FALLING>; power-domains = <&rpmhpd SC7180_CX>; operating-points-v2 = <&qup_opp_table>; interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>, @@ -925,9 +929,11 @@ reg = <0 0x0088c000 0 0x4000>; clock-names = "se"; clocks = <&gcc GCC_QUPV3_WRAP0_S3_CLK>; - pinctrl-names = "default"; + pinctrl-names = "default", "sleep"; pinctrl-0 = <&qup_uart3_default>; - interrupts = <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-1 = <&qup_uart3_sleep>; + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; power-domains = <&rpmhpd SC7180_CX>; operating-points-v2 = <&qup_opp_table>; interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>, @@ -1011,9 +1017,11 @@ reg = <0 0x00894000 0 0x4000>; clock-names = "se"; clocks = <&gcc GCC_QUPV3_WRAP0_S5_CLK>; - pinctrl-names = "default"; + pinctrl-names = "default", "sleep"; pinctrl-0 = <&qup_uart5_default>; - interrupts = <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-1 = <&qup_uart5_sleep>; + interrupts-extended = <&intc GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>, + <&tlmm 28 IRQ_TYPE_EDGE_FALLING>; power-domains = <&rpmhpd SC7180_CX>; operating-points-v2 = <&qup_opp_table>; interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>, @@ -1078,9 +1086,11 @@ reg = <0 0x00a80000 0 0x4000>; clock-names = "se"; clocks = <&gcc GCC_QUPV3_WRAP1_S0_CLK>; - pinctrl-names = "default"; + pinctrl-names = "default", "sleep"; pinctrl-0 = <&qup_uart6_default>; - interrupts = <GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-1 = <&qup_uart6_sleep>; + interrupts-extended = <&intc GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>, + <&tlmm 62 IRQ_TYPE_EDGE_FALLING>; power-domains = <&rpmhpd SC7180_CX>; operating-points-v2 = <&qup_opp_table>; interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>, @@ -1250,9 +1260,11 @@ reg = <0 0x00a90000 0 0x4000>; clock-names = "se"; clocks = <&gcc GCC_QUPV3_WRAP1_S4_CLK>; - pinctrl-names = "default"; + pinctrl-names = "default", "sleep"; pinctrl-0 = <&qup_uart10_default>; - interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-1 = <&qup_uart10_sleep>; + interrupts-extended = <&intc GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>, + <&tlmm 89 IRQ_TYPE_EDGE_FALLING>; power-domains = <&rpmhpd SC7180_CX>; operating-points-v2 = <&qup_opp_table>; interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>, @@ -1302,9 +1314,11 @@ reg = <0 0x00a94000 0 0x4000>; clock-names = "se"; clocks = <&gcc GCC_QUPV3_WRAP1_S5_CLK>; - pinctrl-names = "default"; + pinctrl-names = "default", "sleep"; pinctrl-0 = <&qup_uart11_default>; - interrupts = <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-1 = <&qup_uart11_sleep>; + interrupts-extended = <&intc GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>, + <&tlmm 56 IRQ_TYPE_EDGE_FALLING>; power-domains = <&rpmhpd SC7180_CX>; operating-points-v2 = <&qup_opp_table>; interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>, @@ -1632,6 +1646,14 @@ }; }; + qup_uart0_sleep: qup-uart0-sleep { + pinmux { + pins = "gpio34", "gpio35", + "gpio36", "gpio37"; + function = "gpio"; + }; + }; + qup_uart1_default: qup-uart1-default { pinmux { pins = "gpio0", "gpio1", @@ -1640,6 +1662,14 @@ }; }; + qup_uart1_sleep: qup-uart1-sleep { + pinmux { + pins = "gpio0", "gpio1", + "gpio2", "gpio3"; + function = "gpio"; + }; + }; + qup_uart2_default: qup-uart2-default { pinmux { pins = "gpio15", "gpio16"; @@ -1655,6 +1685,14 @@ }; }; + qup_uart3_sleep: qup-uart3-sleep { + pinmux { + pins = "gpio38", "gpio39", + "gpio40", "gpio41"; + function = "gpio"; + }; + }; + qup_uart4_default: qup-uart4-default { pinmux { pins = "gpio115", "gpio116"; @@ -1670,6 +1708,14 @@ }; }; + qup_uart5_sleep: qup-uart5-sleep { + pinmux { + pins = "gpio25", "gpio26", + "gpio27", "gpio28"; + function = "gpio"; + }; + }; + qup_uart6_default: qup-uart6-default { pinmux { pins = "gpio59", "gpio60", @@ -1678,6 +1724,14 @@ }; }; + qup_uart6_sleep: qup-uart6-sleep { + pinmux { + pins = "gpio59", "gpio60", + "gpio61", "gpio62"; + function = "gpio"; + }; + }; + qup_uart7_default: qup-uart7-default { pinmux { pins = "gpio6", "gpio7"; @@ -1707,6 +1761,14 @@ }; }; + qup_uart10_sleep: qup-uart10-sleep { + pinmux { + pins = "gpio86", "gpio87", + "gpio88", "gpio89"; + function = "gpio"; + }; + }; + qup_uart11_default: qup-uart11-default { pinmux { pins = "gpio53", "gpio54", @@ -1715,6 +1777,14 @@ }; }; + qup_uart11_sleep: qup-uart11-sleep { + pinmux { + pins = "gpio53", "gpio54", + "gpio55", "gpio56"; + function = "gpio"; + }; + }; + sdc1_on: sdc1-on { pinconf-clk { pins = "sdc1_clk"; -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH V2 1/3] arm64: dts: sc7180: Add wakeup support over UART RX 2020-07-24 3:58 ` [PATCH V2 1/3] arm64: dts: sc7180: " satya priya @ 2020-07-28 5:36 ` Akash Asthana 0 siblings, 0 replies; 17+ messages in thread From: Akash Asthana @ 2020-07-28 5:36 UTC (permalink / raw) To: satya priya, Bjorn Andersson, gregkh Cc: Andy Gross, Rob Herring, Matthias Kaehlcke, linux-arm-msm, devicetree, linux-kernel, rojay, msavaliy On 7/24/2020 9:28 AM, satya priya wrote: > Add the necessary pinctrl and interrupts to make UART > wakeup capable. Reviewed-by: Akash Asthana <akashast@codeaurora.org> > Signed-off-by: satya priya <skakit@codeaurora.org> > --- > Changes in V2: > - As per Matthias's comment added wakeup support for all the UARTs > of SC7180. > > arch/arm64/boot/dts/qcom/sc7180.dtsi | 98 ++++++++++++++++++++++++++++++------ > 1 file changed, 84 insertions(+), 14 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi > index 16df08d..044a4d0 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi > @@ -787,9 +787,11 @@ > reg = <0 0x00880000 0 0x4000>; > clock-names = "se"; > clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "sleep"; > pinctrl-0 = <&qup_uart0_default>; > - interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-1 = <&qup_uart0_sleep>; > + interrupts-extended = <&intc GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>, > + <&tlmm 37 IRQ_TYPE_EDGE_FALLING>; > power-domains = <&rpmhpd SC7180_CX>; > operating-points-v2 = <&qup_opp_table>; > interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>, > @@ -839,9 +841,11 @@ > reg = <0 0x00884000 0 0x4000>; > clock-names = "se"; > clocks = <&gcc GCC_QUPV3_WRAP0_S1_CLK>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "sleep"; > pinctrl-0 = <&qup_uart1_default>; > - interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-1 = <&qup_uart1_sleep>; > + interrupts-extended = <&intc GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>, > + <&tlmm 3 IRQ_TYPE_EDGE_FALLING>; > power-domains = <&rpmhpd SC7180_CX>; > operating-points-v2 = <&qup_opp_table>; > interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>, > @@ -925,9 +929,11 @@ > reg = <0 0x0088c000 0 0x4000>; > clock-names = "se"; > clocks = <&gcc GCC_QUPV3_WRAP0_S3_CLK>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "sleep"; > pinctrl-0 = <&qup_uart3_default>; > - interrupts = <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-1 = <&qup_uart3_sleep>; > + interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, > + <&tlmm 41 IRQ_TYPE_EDGE_FALLING>; > power-domains = <&rpmhpd SC7180_CX>; > operating-points-v2 = <&qup_opp_table>; > interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>, > @@ -1011,9 +1017,11 @@ > reg = <0 0x00894000 0 0x4000>; > clock-names = "se"; > clocks = <&gcc GCC_QUPV3_WRAP0_S5_CLK>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "sleep"; > pinctrl-0 = <&qup_uart5_default>; > - interrupts = <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-1 = <&qup_uart5_sleep>; > + interrupts-extended = <&intc GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>, > + <&tlmm 28 IRQ_TYPE_EDGE_FALLING>; > power-domains = <&rpmhpd SC7180_CX>; > operating-points-v2 = <&qup_opp_table>; > interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>, > @@ -1078,9 +1086,11 @@ > reg = <0 0x00a80000 0 0x4000>; > clock-names = "se"; > clocks = <&gcc GCC_QUPV3_WRAP1_S0_CLK>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "sleep"; > pinctrl-0 = <&qup_uart6_default>; > - interrupts = <GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-1 = <&qup_uart6_sleep>; > + interrupts-extended = <&intc GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>, > + <&tlmm 62 IRQ_TYPE_EDGE_FALLING>; > power-domains = <&rpmhpd SC7180_CX>; > operating-points-v2 = <&qup_opp_table>; > interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>, > @@ -1250,9 +1260,11 @@ > reg = <0 0x00a90000 0 0x4000>; > clock-names = "se"; > clocks = <&gcc GCC_QUPV3_WRAP1_S4_CLK>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "sleep"; > pinctrl-0 = <&qup_uart10_default>; > - interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-1 = <&qup_uart10_sleep>; > + interrupts-extended = <&intc GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>, > + <&tlmm 89 IRQ_TYPE_EDGE_FALLING>; > power-domains = <&rpmhpd SC7180_CX>; > operating-points-v2 = <&qup_opp_table>; > interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>, > @@ -1302,9 +1314,11 @@ > reg = <0 0x00a94000 0 0x4000>; > clock-names = "se"; > clocks = <&gcc GCC_QUPV3_WRAP1_S5_CLK>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "sleep"; > pinctrl-0 = <&qup_uart11_default>; > - interrupts = <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-1 = <&qup_uart11_sleep>; > + interrupts-extended = <&intc GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>, > + <&tlmm 56 IRQ_TYPE_EDGE_FALLING>; > power-domains = <&rpmhpd SC7180_CX>; > operating-points-v2 = <&qup_opp_table>; > interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>, > @@ -1632,6 +1646,14 @@ > }; > }; > > + qup_uart0_sleep: qup-uart0-sleep { > + pinmux { > + pins = "gpio34", "gpio35", > + "gpio36", "gpio37"; > + function = "gpio"; > + }; > + }; > + > qup_uart1_default: qup-uart1-default { > pinmux { > pins = "gpio0", "gpio1", > @@ -1640,6 +1662,14 @@ > }; > }; > > + qup_uart1_sleep: qup-uart1-sleep { > + pinmux { > + pins = "gpio0", "gpio1", > + "gpio2", "gpio3"; > + function = "gpio"; > + }; > + }; > + > qup_uart2_default: qup-uart2-default { > pinmux { > pins = "gpio15", "gpio16"; > @@ -1655,6 +1685,14 @@ > }; > }; > > + qup_uart3_sleep: qup-uart3-sleep { > + pinmux { > + pins = "gpio38", "gpio39", > + "gpio40", "gpio41"; > + function = "gpio"; > + }; > + }; > + > qup_uart4_default: qup-uart4-default { > pinmux { > pins = "gpio115", "gpio116"; > @@ -1670,6 +1708,14 @@ > }; > }; > > + qup_uart5_sleep: qup-uart5-sleep { > + pinmux { > + pins = "gpio25", "gpio26", > + "gpio27", "gpio28"; > + function = "gpio"; > + }; > + }; > + > qup_uart6_default: qup-uart6-default { > pinmux { > pins = "gpio59", "gpio60", > @@ -1678,6 +1724,14 @@ > }; > }; > > + qup_uart6_sleep: qup-uart6-sleep { > + pinmux { > + pins = "gpio59", "gpio60", > + "gpio61", "gpio62"; > + function = "gpio"; > + }; > + }; > + > qup_uart7_default: qup-uart7-default { > pinmux { > pins = "gpio6", "gpio7"; > @@ -1707,6 +1761,14 @@ > }; > }; > > + qup_uart10_sleep: qup-uart10-sleep { > + pinmux { > + pins = "gpio86", "gpio87", > + "gpio88", "gpio89"; > + function = "gpio"; > + }; > + }; > + > qup_uart11_default: qup-uart11-default { > pinmux { > pins = "gpio53", "gpio54", > @@ -1715,6 +1777,14 @@ > }; > }; > > + qup_uart11_sleep: qup-uart11-sleep { > + pinmux { > + pins = "gpio53", "gpio54", > + "gpio55", "gpio56"; > + function = "gpio"; > + }; > + }; > + > sdc1_on: sdc1-on { > pinconf-clk { > pins = "sdc1_clk"; -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart 2020-07-24 3:57 [PATCH V2 0/3] Add wakeup support over UART RX satya priya 2020-07-24 3:58 ` [PATCH V2 1/3] arm64: dts: sc7180: " satya priya @ 2020-07-24 3:58 ` satya priya 2020-07-28 5:38 ` Akash Asthana 2020-08-17 18:01 ` Matthias Kaehlcke 2020-07-24 3:58 ` [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue satya priya 2 siblings, 2 replies; 17+ messages in thread From: satya priya @ 2020-07-24 3:58 UTC (permalink / raw) To: Bjorn Andersson, gregkh Cc: Andy Gross, Rob Herring, Matthias Kaehlcke, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy, satya priya 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 */ 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 */ + pins = "gpio40"; + drive-strength = <2>; + bias-disable; output-high; }; + pinconf-rx { + /* + * Configure a pull-up on 41 (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; + }; +}; + +&qup_uart3_sleep { + pinconf-cts { + /* Configure no-pull on 38 (CTS) to match Bluetooth module */ + pins = "gpio38"; + bias-disable; + }; + pinconf-rts { - /* We'll drive 39 (RTS), so no pull */ + /* We'll drive 39 (RTS), so configure pull-down */ pins = "gpio39"; drive-strength = <2>; - bias-disable; + bias-pull-down; }; pinconf-tx { -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart 2020-07-24 3:58 ` [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart satya priya @ 2020-07-28 5:38 ` Akash Asthana 2020-08-17 18:01 ` Matthias Kaehlcke 1 sibling, 0 replies; 17+ messages in thread From: Akash Asthana @ 2020-07-28 5:38 UTC (permalink / raw) To: satya priya, Bjorn Andersson, gregkh Cc: Andy Gross, Rob Herring, Matthias Kaehlcke, linux-arm-msm, devicetree, linux-kernel, rojay, msavaliy On 7/24/2020 9:28 AM, satya priya wrote: > Add sleep pin ctrl for BT uart, and also change the bias > configuration to match Bluetooth module. Reviewed-by: Akash Asthana <akashast@codeaurora.org> > 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 */ > 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 */ > + pins = "gpio40"; > + drive-strength = <2>; > + bias-disable; > output-high; > }; > > + pinconf-rx { > + /* > + * Configure a pull-up on 41 (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; > + }; > +}; > + > +&qup_uart3_sleep { > + pinconf-cts { > + /* Configure no-pull on 38 (CTS) to match Bluetooth module */ > + pins = "gpio38"; > + bias-disable; > + }; > + > pinconf-rts { > - /* We'll drive 39 (RTS), so no pull */ > + /* We'll drive 39 (RTS), so configure pull-down */ > pins = "gpio39"; > drive-strength = <2>; > - bias-disable; > + bias-pull-down; > }; > > pinconf-tx { -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart 2020-07-24 3:58 ` [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart satya priya 2020-07-28 5:38 ` Akash Asthana @ 2020-08-17 18:01 ` Matthias Kaehlcke 2020-08-17 23:33 ` Matthias Kaehlcke 2020-08-19 13:49 ` skakit 1 sibling, 2 replies; 17+ messages in thread From: Matthias Kaehlcke @ 2020-08-17 18:01 UTC (permalink / raw) To: satya priya Cc: Bjorn Andersson, gregkh, Andy Gross, Rob Herring, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart 2020-08-17 18:01 ` Matthias Kaehlcke @ 2020-08-17 23:33 ` Matthias Kaehlcke 2020-08-19 13:48 ` skakit 2020-08-19 13:49 ` skakit 1 sibling, 1 reply; 17+ messages in thread From: Matthias Kaehlcke @ 2020-08-17 23:33 UTC (permalink / raw) To: satya priya Cc: Bjorn Andersson, gregkh, Andy Gross, Rob Herring, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy 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. > 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. 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? Thanks Matthias ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart 2020-08-17 23:33 ` Matthias Kaehlcke @ 2020-08-19 13:48 ` skakit 0 siblings, 0 replies; 17+ messages in thread From: skakit @ 2020-08-19 13:48 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Bjorn Andersson, gregkh, Andy Gross, Rob Herring, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart 2020-08-17 18:01 ` Matthias Kaehlcke 2020-08-17 23:33 ` Matthias Kaehlcke @ 2020-08-19 13:49 ` skakit 2020-08-19 16:13 ` Matthias Kaehlcke 1 sibling, 1 reply; 17+ messages in thread From: skakit @ 2020-08-19 13:49 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Bjorn Andersson, gregkh, Andy Gross, Rob Herring, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy On 2020-08-17 23:31, 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? > The previous config was incorrect, so we corrected it to match the pull of BT. >> 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. The rationale for RTS is that we don't want it to be floating and want to make sure that it is pulled down, to receive bytes. Will modify the comment mentioning the same. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart 2020-08-19 13:49 ` skakit @ 2020-08-19 16:13 ` Matthias Kaehlcke 2020-08-20 12:19 ` skakit 0 siblings, 1 reply; 17+ messages in thread From: Matthias Kaehlcke @ 2020-08-19 16:13 UTC (permalink / raw) To: skakit Cc: Bjorn Andersson, gregkh, Andy Gross, Rob Herring, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy On Wed, Aug 19, 2020 at 07:19:25PM +0530, skakit@codeaurora.org wrote: > On 2020-08-17 23:31, 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? > > > > The previous config was incorrect, so we corrected it to match the pull of > BT. The pull config of the BT controller varies depending on its state, could you clarify which state you intend to match? > > > > 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. > > The rationale for RTS is that we don't want it to be floating and want to > make sure that it is pulled down, to receive bytes. Will modify the comment > mentioning the same. Could you clarify what you mean with "to receive bytes"? Thanks Matthias ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart 2020-08-19 16:13 ` Matthias Kaehlcke @ 2020-08-20 12:19 ` skakit 2020-08-21 15:59 ` Matthias Kaehlcke 0 siblings, 1 reply; 17+ messages in thread From: skakit @ 2020-08-20 12:19 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Bjorn Andersson, gregkh, Andy Gross, Rob Herring, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy Hi Matthias, On 2020-08-19 21:43, Matthias Kaehlcke wrote: > On Wed, Aug 19, 2020 at 07:19:25PM +0530, skakit@codeaurora.org wrote: >> On 2020-08-17 23:31, 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? >> > >> >> The previous config was incorrect, so we corrected it to match the >> pull of >> BT. > > The pull config of the BT controller varies depending on its state, > could > you clarify which state you intend to match? > Since this line is driven by BT SoC, they could change their pull(although it's less likely). Recently on cherokee we worked with BT team and came to an agreement to keep no-pull from our side in order to not conflict with their pull in any state. >> >> > > 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. >> >> The rationale for RTS is that we don't want it to be floating and want >> to >> make sure that it is pulled down, to receive bytes. Will modify the >> comment >> mentioning the same. > > Could you clarify what you mean with "to receive bytes"? > When we keep no-pull(floating), sometimes it may be pulled high and UART flow will be turned off(RFR_NOT_READY), due to this BT SoC wont be able to send data even though host is ready. > Thanks > > Matthias Thanks, Satya Priya ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart 2020-08-20 12:19 ` skakit @ 2020-08-21 15:59 ` Matthias Kaehlcke 0 siblings, 0 replies; 17+ messages in thread From: Matthias Kaehlcke @ 2020-08-21 15:59 UTC (permalink / raw) To: skakit Cc: Bjorn Andersson, gregkh, Andy Gross, Rob Herring, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy On Thu, Aug 20, 2020 at 05:49:53PM +0530, skakit@codeaurora.org wrote: > Hi Matthias, > > On 2020-08-19 21:43, Matthias Kaehlcke wrote: > > On Wed, Aug 19, 2020 at 07:19:25PM +0530, skakit@codeaurora.org wrote: > > > On 2020-08-17 23:31, 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? > > > > > > > > > > The previous config was incorrect, so we corrected it to match the > > > pull of > > > BT. > > > > The pull config of the BT controller varies depending on its state, > > could > > you clarify which state you intend to match? > > > > Since this line is driven by BT SoC, they could change their pull(although > it's less likely). Recently on cherokee we worked with BT team and came to > an agreement to keep no-pull from our side in order to not conflict with > their pull in any state. > > > > > > > > > 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. > > > > > > The rationale for RTS is that we don't want it to be floating and > > > want to > > > make sure that it is pulled down, to receive bytes. Will modify the > > > comment > > > mentioning the same. > > > > Could you clarify what you mean with "to receive bytes"? > > > > When we keep no-pull(floating), sometimes it may be pulled high and UART > flow will be turned off(RFR_NOT_READY), due to this BT SoC wont be able to > send data even though host is ready. I'm a bit at a loss here, about two things: RTS is an output pin controlled by the UART. IIUC if the UART port is active and hardware flow control is enabled the RTS signal is either driven to high or low, but not floating. Now lets assume I'm wrong with the above and RTS can be floating. We only want the BT SoC to send data when the host UART is ready to receive them, right? RTS is an active low signal, hence by configuring it as a pull-down the BT SoC can send data regardless of whether the host UART actually asserts RTS, so the host UART may not be ready to receive it. I would argue that if there is really such a thing as a floating RTS signal then it should have a pull-up, to prevent the BT SoC from sending data at any time. I'm not an expert in UART communication and pinconf, so it could be that I got something wrong, but as of now it seems to me that no pull is the correct config for RTS. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue 2020-07-24 3:57 [PATCH V2 0/3] Add wakeup support over UART RX satya priya 2020-07-24 3:58 ` [PATCH V2 1/3] arm64: dts: sc7180: " satya priya 2020-07-24 3:58 ` [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart satya priya @ 2020-07-24 3:58 ` satya priya 2020-07-24 8:42 ` Greg KH ` (2 more replies) 2 siblings, 3 replies; 17+ messages in thread From: satya priya @ 2020-07-24 3:58 UTC (permalink / raw) To: Bjorn Andersson, gregkh Cc: Andy Gross, Rob Herring, Matthias Kaehlcke, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy, satya priya As a part of system suspend we call uart_port_suspend from the Serial driver, which calls set_mctrl passing mctrl as NULL. This makes RFR high(NOT_READY) during suspend. Due to this BT SoC is not able to send wakeup bytes to UART during suspend. Included if check for non-suspend case to keep RFR low during suspend. Signed-off-by: satya priya <skakit@codeaurora.org> --- Changes in V2: - This patch fixes the UART flow control issue during suspend. Newly added in V2. drivers/tty/serial/qcom_geni_serial.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index 07b7b6b..7108dfc 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -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); } -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue 2020-07-24 3:58 ` [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue satya priya @ 2020-07-24 8:42 ` Greg KH 2020-07-28 5:40 ` Akash Asthana 2020-08-17 17:42 ` Matthias Kaehlcke 2 siblings, 0 replies; 17+ messages in thread From: Greg KH @ 2020-07-24 8:42 UTC (permalink / raw) To: satya priya Cc: Bjorn Andersson, Andy Gross, Rob Herring, Matthias Kaehlcke, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy On Fri, Jul 24, 2020 at 09:28:02AM +0530, satya priya wrote: > As a part of system suspend we call uart_port_suspend from the > Serial driver, which calls set_mctrl passing mctrl as NULL. This > makes RFR high(NOT_READY) during suspend. > > Due to this BT SoC is not able to send wakeup bytes to UART during > suspend. Included if check for non-suspend case to keep RFR low > during suspend. > > Signed-off-by: satya priya <skakit@codeaurora.org> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue 2020-07-24 3:58 ` [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue satya priya 2020-07-24 8:42 ` Greg KH @ 2020-07-28 5:40 ` Akash Asthana 2020-08-17 17:42 ` Matthias Kaehlcke 2 siblings, 0 replies; 17+ messages in thread From: Akash Asthana @ 2020-07-28 5:40 UTC (permalink / raw) To: satya priya, Bjorn Andersson, gregkh Cc: Andy Gross, Rob Herring, Matthias Kaehlcke, linux-arm-msm, devicetree, linux-kernel, rojay, msavaliy On 7/24/2020 9:28 AM, satya priya wrote: > As a part of system suspend we call uart_port_suspend from the > Serial driver, which calls set_mctrl passing mctrl as NULL. This > makes RFR high(NOT_READY) during suspend. > > Due to this BT SoC is not able to send wakeup bytes to UART during > suspend. Included if check for non-suspend case to keep RFR low > during suspend. Reviewed-by: Akash Asthana <akashast@codeaurora.org> > Signed-off-by: satya priya <skakit@codeaurora.org> > --- > Changes in V2: > - This patch fixes the UART flow control issue during suspend. > Newly added in V2. > > drivers/tty/serial/qcom_geni_serial.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 07b7b6b..7108dfc 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -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); > } -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue 2020-07-24 3:58 ` [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue satya priya 2020-07-24 8:42 ` Greg KH 2020-07-28 5:40 ` Akash Asthana @ 2020-08-17 17:42 ` Matthias Kaehlcke 2020-08-19 13:49 ` skakit 2 siblings, 1 reply; 17+ messages in thread From: Matthias Kaehlcke @ 2020-08-17 17:42 UTC (permalink / raw) To: satya priya Cc: Bjorn Andersson, gregkh, Andy Gross, Rob Herring, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy On Fri, Jul 24, 2020 at 09:28:02AM +0530, satya priya wrote: > As a part of system suspend we call uart_port_suspend from the > Serial driver, which calls set_mctrl passing mctrl as NULL. This > makes RFR high(NOT_READY) during suspend. > > Due to this BT SoC is not able to send wakeup bytes to UART during > suspend. Included if check for non-suspend case to keep RFR low > during suspend. > > Signed-off-by: satya priya <skakit@codeaurora.org> > --- > Changes in V2: > - This patch fixes the UART flow control issue during suspend. > Newly added in V2. > > drivers/tty/serial/qcom_geni_serial.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 07b7b6b..7108dfc 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -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))) Why all these parentheses, instead of: if (!(mctrl & TIOCM_RTS) && !uport->suspended) ? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue 2020-08-17 17:42 ` Matthias Kaehlcke @ 2020-08-19 13:49 ` skakit 0 siblings, 0 replies; 17+ messages in thread From: skakit @ 2020-08-19 13:49 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Bjorn Andersson, gregkh, Andy Gross, Rob Herring, linux-arm-msm, devicetree, linux-kernel, akashast, rojay, msavaliy On 2020-08-17 23:12, Matthias Kaehlcke wrote: > On Fri, Jul 24, 2020 at 09:28:02AM +0530, satya priya wrote: >> As a part of system suspend we call uart_port_suspend from the >> Serial driver, which calls set_mctrl passing mctrl as NULL. This >> makes RFR high(NOT_READY) during suspend. >> >> Due to this BT SoC is not able to send wakeup bytes to UART during >> suspend. Included if check for non-suspend case to keep RFR low >> during suspend. >> >> Signed-off-by: satya priya <skakit@codeaurora.org> >> --- >> Changes in V2: >> - This patch fixes the UART flow control issue during suspend. >> Newly added in V2. >> >> drivers/tty/serial/qcom_geni_serial.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/qcom_geni_serial.c >> b/drivers/tty/serial/qcom_geni_serial.c >> index 07b7b6b..7108dfc 100644 >> --- a/drivers/tty/serial/qcom_geni_serial.c >> +++ b/drivers/tty/serial/qcom_geni_serial.c >> @@ -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))) > > Why all these parentheses, instead of: > > if (!(mctrl & TIOCM_RTS) && !uport->suspended) > > ? ok. Will remove the extra parentheses. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-08-21 15:59 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-24 3:57 [PATCH V2 0/3] Add wakeup support over UART RX satya priya 2020-07-24 3:58 ` [PATCH V2 1/3] arm64: dts: sc7180: " satya priya 2020-07-28 5:36 ` Akash Asthana 2020-07-24 3:58 ` [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart satya priya 2020-07-28 5:38 ` Akash Asthana 2020-08-17 18:01 ` Matthias Kaehlcke 2020-08-17 23:33 ` Matthias Kaehlcke 2020-08-19 13:48 ` skakit 2020-08-19 13:49 ` skakit 2020-08-19 16:13 ` Matthias Kaehlcke 2020-08-20 12:19 ` skakit 2020-08-21 15:59 ` Matthias Kaehlcke 2020-07-24 3:58 ` [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue satya priya 2020-07-24 8:42 ` Greg KH 2020-07-28 5:40 ` Akash Asthana 2020-08-17 17:42 ` Matthias Kaehlcke 2020-08-19 13:49 ` skakit
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.