[PATCH V3 0/3] Add wakeup support over UART RX

[PATCH V3 0/3] Add wakeup support over UART RX

[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.

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.

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 | 54 +++++++++++++++---
 arch/arm64/boot/dts/qcom/sc7180.dtsi    | 98 ++++++++++++++++++++++++++++-----
 drivers/tty/serial/qcom_geni_serial.c   |  2 +-
 3 files changed, 130 insertions(+), 24 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH V3 1/3] arm64: dts: sc7180: Add wakeup support over UART RX

[satya priya]

Add the necessary pinctrl and interrupts to make UART
wakeup capable.

Signed-off-by: satya priya <skakit@codeaurora.org>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
---
Changes in V2:
 - As per Matthias's comment added wakeup support for all the UARTs
   of SC7180.

Changes in V3:
 - No change.

 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 d46b383..855b13e 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -793,9 +793,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>,
@@ -845,9 +847,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>,
@@ -931,9 +935,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>,
@@ -1017,9 +1023,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>,
@@ -1084,9 +1092,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>,
@@ -1256,9 +1266,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>,
@@ -1308,9 +1320,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>,
@@ -1638,6 +1652,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",
@@ -1646,6 +1668,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";
@@ -1661,6 +1691,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";
@@ -1676,6 +1714,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",
@@ -1684,6 +1730,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";
@@ -1713,6 +1767,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",
@@ -1721,6 +1783,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

Re: [PATCH V3 1/3] arm64: dts: sc7180: Add wakeup support over UART RX

[Matthias Kaehlcke]

On Thu, Aug 20, 2020 at 07:21:05PM +0530, satya priya wrote:
> Add the necessary pinctrl and interrupts to make UART
> wakeup capable.
> 
> Signed-off-by: satya priya <skakit@codeaurora.org>
> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V2:
>  - As per Matthias's comment added wakeup support for all the UARTs
>    of SC7180.
> 
> Changes in V3:
>  - No change.
> 
>  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 d46b383..855b13e 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>
> ...
>
> +			qup_uart0_sleep: qup-uart0-sleep {
> +				pinmux {
> +					pins = "gpio34", "gpio35",
> +					       "gpio36", "gpio37";
> +					function = "gpio";

What is the reason that the GPIO function needs to be selected in sleep mode
to support wakeup?

This should be explained in the commit message unless it is evident.

[PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart

[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>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
---
Changes in V2:
 - This patch adds sleep state for BT UART. Newly added in V2.

Changes in V3:
 - Remove "output-high" for TX from both sleep and default states
   as it is not required. Configure pull-up for TX in sleep state.

 arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54 +++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
index d8b5507..806f626 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
@@ -473,20 +473,20 @@
 
 &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-pull-down;
-		output-high;
+		bias-disable;
 	};
 
 	pinconf-rts {
-		/* We'll drive 39 (RTS), so no pull */
+		/*
+		 * Configure pull-down on 39 (RTS). This is needed to avoid a
+		 * floating pin which could mislead Bluetooth controller
+		 * with UART RFR state (READY/NOT_READY).
+		 */
 		pins = "gpio39";
 		drive-strength = <2>;
-		bias-disable;
+		bias-pull-down;
 	};
 
 	pinconf-tx {
@@ -494,7 +494,43 @@
 		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 {
+		/*
+		 * Configure pull-down on 39 (RTS). This is needed to avoid a
+		 * floating pin which could mislead Bluetooth controller
+		 * with UART RFR state (READY/NOT_READY).
+		 */
+		pins = "gpio39";
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+
+	pinconf-tx {
+		/* Configure pull-up on 40 (TX) when it isn't actively driven */
+		pins = "gpio40";
+		drive-strength = <2>;
+		bias-pull-up;
 	};
 
 	pinconf-rx {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart

[Matthias Kaehlcke]

On Thu, Aug 20, 2020 at 07:21:06PM +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>
> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V2:
>  - This patch adds sleep state for BT UART. Newly added in V2.
> 
> Changes in V3:
>  - Remove "output-high" for TX from both sleep and default states
>    as it is not required. Configure pull-up for TX in sleep state.
> 
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54 +++++++++++++++++++++++++++------
>  1 file changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index d8b5507..806f626 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -473,20 +473,20 @@
>  
>  &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-pull-down;
> -		output-high;
> +		bias-disable;

I think it should be ok in functional terms, but I don't like the rationale
and also doubt the change is really needed.

If the pull is removed to match the Bluetooth module, then that sounds as
if the signal was floating on the the BT side, which I think is not the case.
Yes, according to the datasheet there is no pull when the BT controller is
active, but then it drives the signal actively to either high or low. There
seems to be no merit in 'matching' the Bluetooth side in this case, if the
signal was really floating on the BT side we would definitely not want this.

In a reply to v2 you said:

> 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.

What are these conflicting pull states?

The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
boot mode, and no pull in active mode. In reset and boot mode the host
config with a pull down would match, and no pull in active mode doesn't
conflict with the pull-down on the host UART. My understanding is that
the pinconf pulls are weak pulls, so as soon as the BT chip drives its
RTS the pull on the host side shouldn't matter.

Is this change actually related with wakeup support? I have the impression
that multiple things are conflated in this patch. If some of the changes
are just fixing/improving other things they should be in a separate patch,
which could be part of this series, otherwise it's really hard to
distinguish between the pieces that are actually relevant for wakeup and
the rest.

Independently of whether the changes are done in a single or multiple
patches, the commit log should include details on why the changes are
necessary, especially when there are not explantatory comments in the
DT/code itself (e.g. the removal of 'output-high', which seems correct
to me, but no reason is given why it is done).

>  	};
>  
>  	pinconf-rts {
> -		/* We'll drive 39 (RTS), so no pull */
> +		/*
> +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
> +		 * floating pin which could mislead Bluetooth controller
> +		 * with UART RFR state (READY/NOT_READY).
> +		 */
>  		pins = "gpio39";
>  		drive-strength = <2>;
> -		bias-disable;
> +		bias-pull-down;
>  	};

[copy of my comment on v2]

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.

>  
>  	pinconf-tx {
> @@ -494,7 +494,43 @@
>  		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 {
> +		/*
> +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
> +		 * floating pin which could mislead Bluetooth controller
> +		 * with UART RFR state (READY/NOT_READY).
> +		 */
> +		pins = "gpio39";
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};

I don't know all the details, but I have the impression that this is the
relevant pull change for wakeup. From the title of the series I derive
that the UART RX pin is used for signalling wakeup. A pull-down on RTS
indicates the BT controller that it can always send data to wake up the
host.

I think RTS in default mode should remain with no-pull (the UART is driving
the signal), and then change it to pull-down in sleep mode.


> +
> +	pinconf-tx {
> +		/* Configure pull-up on 40 (TX) when it isn't actively driven */

nit: just say '... on TX ...', the GPIO number isn't really interesting and can
easily be determined by looking at 'pins' if needed . Applicable to all comments
involving pins.

> +		pins = "gpio40";
> +		drive-strength = <2>;
> +		bias-pull-up;

This makes sense to me.

Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart

[skakit]

On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> On Thu, Aug 20, 2020 at 07:21:06PM +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>
>> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
>> ---
>> Changes in V2:
>>  - This patch adds sleep state for BT UART. Newly added in V2.
>> 
>> Changes in V3:
>>  - Remove "output-high" for TX from both sleep and default states
>>    as it is not required. Configure pull-up for TX in sleep state.
>> 
>>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54 
>> +++++++++++++++++++++++++++------
>>  1 file changed, 45 insertions(+), 9 deletions(-)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts 
>> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> index d8b5507..806f626 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> @@ -473,20 +473,20 @@
>> 
>>  &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-pull-down;
>> -		output-high;
>> +		bias-disable;
> 
> I think it should be ok in functional terms, but I don't like the 
> rationale
> and also doubt the change is really needed.
> 
> If the pull is removed to match the Bluetooth module, then that sounds 
> as
> if the signal was floating on the the BT side, which I think is not the 
> case.
> Yes, according to the datasheet there is no pull when the BT controller 
> is
> active, but then it drives the signal actively to either high or low. 
> There
> seems to be no merit in 'matching' the Bluetooth side in this case, if 
> the
> signal was really floating on the BT side we would definitely not want 
> this.
> 
> In a reply to v2 you said:
> 
>> 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.
> 
> What are these conflicting pull states?
> 
> The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset 
> and
> boot mode, and no pull in active mode. In reset and boot mode the host
> config with a pull down would match, and no pull in active mode doesn't
> conflict with the pull-down on the host UART. My understanding is that
> the pinconf pulls are weak pulls, so as soon as the BT chip drives its
> RTS the pull on the host side shouldn't matter.
> 

yes, I agree with you, the pinconf pulls are weak. As this is driven by 
BT SoC (pull on HOST side shouldn't matter), we are not mentioning any 
bias configuration from our side and simply putting it as no-pull, just 
to not conflict in any case. It seems that the rationale mentioned is a 
bit confusing i will change it to clearly specify why we are configuring 
no-pull.

> Is this change actually related with wakeup support? I have the 
> impression
> that multiple things are conflated in this patch. If some of the 
> changes
> are just fixing/improving other things they should be in a separate 
> patch,
> which could be part of this series, otherwise it's really hard to
> distinguish between the pieces that are actually relevant for wakeup 
> and
> the rest.
> 
> Independently of whether the changes are done in a single or multiple
> patches, the commit log should include details on why the changes are
> necessary, especially when there are not explantatory comments in the
> DT/code itself (e.g. the removal of 'output-high', which seems correct
> to me, but no reason is given why it is done).
> 

This change is not related to wakeup support, I will make it a separate 
patch, will also mention the details in commit text.

>>  	};
>> 
>>  	pinconf-rts {
>> -		/* We'll drive 39 (RTS), so no pull */
>> +		/*
>> +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
>> +		 * floating pin which could mislead Bluetooth controller
>> +		 * with UART RFR state (READY/NOT_READY).
>> +		 */
>>  		pins = "gpio39";
>>  		drive-strength = <2>;
>> -		bias-disable;
>> +		bias-pull-down;
>>  	};
> 
> [copy of my comment on v2]
> 
> 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.

Yes, RTS is either driven high or low. HW flow control is always enabled 
and only turned off when RX FIFO is full. Whereas SW flow control is 
controlled by upper layers(serial core), also it can be enabled/disabled 
from host by calling set_mctrl.

> 
> 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.
> 
>> 
>>  	pinconf-tx {
>> @@ -494,7 +494,43 @@
>>  		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 {
>> +		/*
>> +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
>> +		 * floating pin which could mislead Bluetooth controller
>> +		 * with UART RFR state (READY/NOT_READY).
>> +		 */
>> +		pins = "gpio39";
>> +		drive-strength = <2>;
>> +		bias-pull-down;
>> +	};
> 
> I don't know all the details, but I have the impression that this is 
> the
> relevant pull change for wakeup. From the title of the series I derive
> that the UART RX pin is used for signalling wakeup. A pull-down on RTS
> indicates the BT controller that it can always send data to wake up the
> host.
> 
> I think RTS in default mode should remain with no-pull (the UART is 
> driving
> the signal), and then change it to pull-down in sleep mode.
> 
> 

As I understand from your previous comment, pinconf pulls are weak and 
cannot override the pull of controller. Although pull down is 
configured, data will be received only if host controller is ready to 
accept it. So, we want to put RTS in pull-down state(known state) 
instead of leaving it in ambiguous state i.e, no-pull(high/low).

>> +
>> +	pinconf-tx {
>> +		/* Configure pull-up on 40 (TX) when it isn't actively driven */
> 
> nit: just say '... on TX ...', the GPIO number isn't really interesting 
> and can
> easily be determined by looking at 'pins' if needed . Applicable to all 
> comments
> involving pins.
> 

Okay.

>> +		pins = "gpio40";
>> +		drive-strength = <2>;
>> +		bias-pull-up;
> 
> This makes sense to me.

Thanks,
Satya Priya

Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart

[Matthias Kaehlcke]

On Tue, Aug 25, 2020 at 06:42:28PM +0530, skakit@codeaurora.org wrote:
> On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> > On Thu, Aug 20, 2020 at 07:21:06PM +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>
> > > Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> > > ---
> > > Changes in V2:
> > >  - This patch adds sleep state for BT UART. Newly added in V2.
> > > 
> > > Changes in V3:
> > >  - Remove "output-high" for TX from both sleep and default states
> > >    as it is not required. Configure pull-up for TX in sleep state.
> > > 
> > >  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > > +++++++++++++++++++++++++++------
> > >  1 file changed, 45 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > index d8b5507..806f626 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > @@ -473,20 +473,20 @@
> > > 
> > >  &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-pull-down;
> > > -		output-high;
> > > +		bias-disable;
> > 
> > I think it should be ok in functional terms, but I don't like the
> > rationale
> > and also doubt the change is really needed.
> > 
> > If the pull is removed to match the Bluetooth module, then that sounds
> > as
> > if the signal was floating on the the BT side, which I think is not the
> > case.
> > Yes, according to the datasheet there is no pull when the BT controller
> > is
> > active, but then it drives the signal actively to either high or low.
> > There
> > seems to be no merit in 'matching' the Bluetooth side in this case, if
> > the
> > signal was really floating on the BT side we would definitely not want
> > this.
> > 
> > In a reply to v2 you said:
> > 
> > > 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.
> > 
> > What are these conflicting pull states?
> > 
> > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
> > boot mode, and no pull in active mode. In reset and boot mode the host
> > config with a pull down would match, and no pull in active mode doesn't
> > conflict with the pull-down on the host UART. My understanding is that
> > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
> > RTS the pull on the host side shouldn't matter.
> > 
> 
> yes, I agree with you, the pinconf pulls are weak. As this is driven by BT
> SoC (pull on HOST side shouldn't matter), we are not mentioning any bias
> configuration from our side and simply putting it as no-pull, just to not
> conflict in any case. It seems that the rationale mentioned is a bit
> confusing i will change it to clearly specify why we are configuring
> no-pull.
> 
> > Is this change actually related with wakeup support? I have the
> > impression
> > that multiple things are conflated in this patch. If some of the changes
> > are just fixing/improving other things they should be in a separate
> > patch,
> > which could be part of this series, otherwise it's really hard to
> > distinguish between the pieces that are actually relevant for wakeup and
> > the rest.
> > 
> > Independently of whether the changes are done in a single or multiple
> > patches, the commit log should include details on why the changes are
> > necessary, especially when there are not explantatory comments in the
> > DT/code itself (e.g. the removal of 'output-high', which seems correct
> > to me, but no reason is given why it is done).
> > 
> 
> This change is not related to wakeup support, I will make it a separate
> patch, will also mention the details in commit text.
> 
> > >  	};
> > > 
> > >  	pinconf-rts {
> > > -		/* We'll drive 39 (RTS), so no pull */
> > > +		/*
> > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
> > > +		 * floating pin which could mislead Bluetooth controller
> > > +		 * with UART RFR state (READY/NOT_READY).
> > > +		 */
> > >  		pins = "gpio39";
> > >  		drive-strength = <2>;
> > > -		bias-disable;
> > > +		bias-pull-down;
> > >  	};
> > 
> > [copy of my comment on v2]
> > 
> > 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.
> 
> Yes, RTS is either driven high or low. HW flow control is always enabled and
> only turned off when RX FIFO is full. Whereas SW flow control is controlled
> by upper layers(serial core), also it can be enabled/disabled from host by
> calling set_mctrl.

As far as I understand the above isn't entirely correct. HW flow control is not
disabled when the RX FIFO is full, rather as part of HW flow control the
hardware deasserts RTS when the FIFO is full. Software flow control isn't really
relevant here, since it doesn't use RTS/CTS but uses transmission of special
codes (XON/XOFF) over TX/RX.

> > 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.
> > 
> > > 
> > >  	pinconf-tx {
> > > @@ -494,7 +494,43 @@
> > >  		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 {
> > > +		/*
> > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
> > > +		 * floating pin which could mislead Bluetooth controller
> > > +		 * with UART RFR state (READY/NOT_READY).
> > > +		 */
> > > +		pins = "gpio39";
> > > +		drive-strength = <2>;

just noticed this: in the sleep config all pins are in GPIO config (see
"arm64: dts: sc7180: Add wakeup support over UART RX" from this series)
and by default they are inputs, hence the drive-strength here is pointless
IIUC.

> > > +		bias-pull-down;
> > > +	};
> > 
> > I don't know all the details, but I have the impression that this is the
> > relevant pull change for wakeup. From the title of the series I derive
> > that the UART RX pin is used for signalling wakeup. A pull-down on RTS
> > indicates the BT controller that it can always send data to wake up the
> > host.
> > 
> > I think RTS in default mode should remain with no-pull (the UART is
> > driving
> > the signal), and then change it to pull-down in sleep mode.
> > 
> > 
> 
> As I understand from your previous comment, pinconf pulls are weak and
> cannot override the pull of controller.

I'm not sure this is an absolute truth. I think there may be cases where
the driver has to increase its drive strength..

> Although pull down is configured,
> data will be received only if host controller is ready to accept it. So, we
> want to put RTS in pull-down state(known state) instead of leaving it in
> ambiguous state i.e, no-pull(high/low).

I disgress. I'm pretty sure that you want RTS to be low to make sure that
the BT SoC can wake up the system by sending whatever data it has to send.
It won't do that if RTS is high (e.g. because that's its floating state
at that time). I just tried configuring a pull-up (also a known
non-ambiguous state), and Bluetooth wakeup doesn't work with that,
supposedly because the BT SoC/UART will wait for its CTS signal to be low.

> > > +
> > > +	pinconf-tx {
> > > +		/* Configure pull-up on 40 (TX) when it isn't actively driven */
> > 
> > nit: just say '... on TX ...', the GPIO number isn't really interesting
> > and can
> > easily be determined by looking at 'pins' if needed . Applicable to all
> > comments
> > involving pins.
> > 
> 
> Okay.
> 
> > > +		pins = "gpio40";
> > > +		drive-strength = <2>;

also not needed, see above

Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart

[skakit]

Hi Matthias,

On 2020-08-25 22:08, Matthias Kaehlcke wrote:
> On Tue, Aug 25, 2020 at 06:42:28PM +0530, skakit@codeaurora.org wrote:
>> On 2020-08-21 22:52, Matthias Kaehlcke wrote:
>> > On Thu, Aug 20, 2020 at 07:21:06PM +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>
>> > > Reviewed-by: Akash Asthana <akashast@codeaurora.org>
>> > > ---
>> > > Changes in V2:
>> > >  - This patch adds sleep state for BT UART. Newly added in V2.
>> > >
>> > > Changes in V3:
>> > >  - Remove "output-high" for TX from both sleep and default states
>> > >    as it is not required. Configure pull-up for TX in sleep state.
>> > >
>> > >  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
>> > > +++++++++++++++++++++++++++------
>> > >  1 file changed, 45 insertions(+), 9 deletions(-)
>> > >
>> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > index d8b5507..806f626 100644
>> > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > @@ -473,20 +473,20 @@
>> > >
>> > >  &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-pull-down;
>> > > -		output-high;
>> > > +		bias-disable;
>> >
>> > I think it should be ok in functional terms, but I don't like the
>> > rationale
>> > and also doubt the change is really needed.
>> >
>> > If the pull is removed to match the Bluetooth module, then that sounds
>> > as
>> > if the signal was floating on the the BT side, which I think is not the
>> > case.
>> > Yes, according to the datasheet there is no pull when the BT controller
>> > is
>> > active, but then it drives the signal actively to either high or low.
>> > There
>> > seems to be no merit in 'matching' the Bluetooth side in this case, if
>> > the
>> > signal was really floating on the BT side we would definitely not want
>> > this.
>> >
>> > In a reply to v2 you said:
>> >
>> > > 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.
>> >
>> > What are these conflicting pull states?
>> >
>> > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
>> > boot mode, and no pull in active mode. In reset and boot mode the host
>> > config with a pull down would match, and no pull in active mode doesn't
>> > conflict with the pull-down on the host UART. My understanding is that
>> > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
>> > RTS the pull on the host side shouldn't matter.
>> >
>> 
>> yes, I agree with you, the pinconf pulls are weak. As this is driven 
>> by BT
>> SoC (pull on HOST side shouldn't matter), we are not mentioning any 
>> bias
>> configuration from our side and simply putting it as no-pull, just to 
>> not
>> conflict in any case. It seems that the rationale mentioned is a bit
>> confusing i will change it to clearly specify why we are configuring
>> no-pull.
>> 
>> > Is this change actually related with wakeup support? I have the
>> > impression
>> > that multiple things are conflated in this patch. If some of the changes
>> > are just fixing/improving other things they should be in a separate
>> > patch,
>> > which could be part of this series, otherwise it's really hard to
>> > distinguish between the pieces that are actually relevant for wakeup and
>> > the rest.
>> >
>> > Independently of whether the changes are done in a single or multiple
>> > patches, the commit log should include details on why the changes are
>> > necessary, especially when there are not explantatory comments in the
>> > DT/code itself (e.g. the removal of 'output-high', which seems correct
>> > to me, but no reason is given why it is done).
>> >
>> 
>> This change is not related to wakeup support, I will make it a 
>> separate
>> patch, will also mention the details in commit text.
>> 
>> > >  	};
>> > >
>> > >  	pinconf-rts {
>> > > -		/* We'll drive 39 (RTS), so no pull */
>> > > +		/*
>> > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
>> > > +		 * floating pin which could mislead Bluetooth controller
>> > > +		 * with UART RFR state (READY/NOT_READY).
>> > > +		 */
>> > >  		pins = "gpio39";
>> > >  		drive-strength = <2>;
>> > > -		bias-disable;
>> > > +		bias-pull-down;
>> > >  	};
>> >
>> > [copy of my comment on v2]
>> >
>> > 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.
>> 
>> Yes, RTS is either driven high or low. HW flow control is always 
>> enabled and
>> only turned off when RX FIFO is full. Whereas SW flow control is 
>> controlled
>> by upper layers(serial core), also it can be enabled/disabled from 
>> host by
>> calling set_mctrl.
> 
> As far as I understand the above isn't entirely correct. HW flow 
> control is not
> disabled when the RX FIFO is full, rather as part of HW flow control 
> the
> hardware deasserts RTS when the FIFO is full. Software flow control 
> isn't really
> relevant here, since it doesn't use RTS/CTS but uses transmission of 
> special
> codes (XON/XOFF) over TX/RX.

Here by Software flow control i meant, we can control the flow from 
SW(explained below).

> 
>> > 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.
>> >
>> > >
>> > >  	pinconf-tx {
>> > > @@ -494,7 +494,43 @@
>> > >  		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 {
>> > > +		/*
>> > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
>> > > +		 * floating pin which could mislead Bluetooth controller
>> > > +		 * with UART RFR state (READY/NOT_READY).
>> > > +		 */
>> > > +		pins = "gpio39";
>> > > +		drive-strength = <2>;
> 
> just noticed this: in the sleep config all pins are in GPIO config (see
> "arm64: dts: sc7180: Add wakeup support over UART RX" from this series)
> and by default they are inputs, hence the drive-strength here is 
> pointless
> IIUC.
> 

CTS and RX are inputs to the HOST whereas RTS and TX are outputs. We 
have added drive-strength for output pins only as they are driven by 
UART(please correct me if wrong).

>> > > +		bias-pull-down;
>> > > +	};
>> >
>> > I don't know all the details, but I have the impression that this is the
>> > relevant pull change for wakeup. From the title of the series I derive
>> > that the UART RX pin is used for signalling wakeup. A pull-down on RTS
>> > indicates the BT controller that it can always send data to wake up the
>> > host.
>> >
>> > I think RTS in default mode should remain with no-pull (the UART is
>> > driving
>> > the signal), and then change it to pull-down in sleep mode.
>> >
>> >
>> 
>> As I understand from your previous comment, pinconf pulls are weak and
>> cannot override the pull of controller.
> 
> I'm not sure this is an absolute truth. I think there may be cases 
> where
> the driver has to increase its drive strength..
> 
>> Although pull down is configured,
>> data will be received only if host controller is ready to accept it. 
>> So, we
>> want to put RTS in pull-down state(known state) instead of leaving it 
>> in
>> ambiguous state i.e, no-pull(high/low).
> 
> I disgress. I'm pretty sure that you want RTS to be low to make sure 
> that
> the BT SoC can wake up the system by sending whatever data it has to 
> send.
> It won't do that if RTS is high (e.g. because that's its floating state
> at that time). I just tried configuring a pull-up (also a known
> non-ambiguous state), and Bluetooth wakeup doesn't work with that,
> supposedly because the BT SoC/UART will wait for its CTS signal to be 
> low.
> 

yes, you are right, we are keeping RTS low to make sure that BT SoC can 
wakeup the system by sending bytes.
My intention here was to explain below case from your comment:

>> > 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.

1. By default our HW flow is enabled(since we are configuring pull-down 
on RTS),and BT SoC can send data anytime.
2. But there is a SW mechanism where we can control the flow from 
software. In that case what ever is configured to UART_MANUAL_RFR(READY 
or NOT_READY) will override the dtsi pinconf pull and the RTS/RFR line 
is pulled high when HOST is not ready(while debugging the wake up issue 
we came across this).

So, as far as i understand, even if pull-down is configured on RTS, BT 
SoC can send data only when HOST is ready.
Can you please let me know which case you mean here, when you say "by 
configuring it as a pull-down the BT SoC can send data regardless of 
whether the host UART actually asserts RTS". Is there any case which we 
are missing here?

>> > > +
>> > > +	pinconf-tx {
>> > > +		/* Configure pull-up on 40 (TX) when it isn't actively driven */
>> >
>> > nit: just say '... on TX ...', the GPIO number isn't really interesting
>> > and can
>> > easily be determined by looking at 'pins' if needed . Applicable to all
>> > comments
>> > involving pins.
>> >
>> 
>> Okay.
>> 
>> > > +		pins = "gpio40";
>> > > +		drive-strength = <2>;
> 
> also not needed, see above


Thanks,
Satya Priya

Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart

[Matthias Kaehlcke]

Hi Satya,

On Wed, Aug 26, 2020 at 09:35:15PM +0530, skakit@codeaurora.org wrote:
> Hi Matthias,
> 
> On 2020-08-25 22:08, Matthias Kaehlcke wrote:
> > On Tue, Aug 25, 2020 at 06:42:28PM +0530, skakit@codeaurora.org wrote:
> > > On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> > > > On Thu, Aug 20, 2020 at 07:21:06PM +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>
> > > > > Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> > > > > ---
> > > > > Changes in V2:
> > > > >  - This patch adds sleep state for BT UART. Newly added in V2.
> > > > >
> > > > > Changes in V3:
> > > > >  - Remove "output-high" for TX from both sleep and default states
> > > > >    as it is not required. Configure pull-up for TX in sleep state.
> > > > >
> > > > >  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > > > > +++++++++++++++++++++++++++------
> > > > >  1 file changed, 45 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > index d8b5507..806f626 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > @@ -473,20 +473,20 @@
> > > > >
> > > > >  &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-pull-down;
> > > > > -		output-high;
> > > > > +		bias-disable;
> > > >
> > > > I think it should be ok in functional terms, but I don't like the
> > > > rationale
> > > > and also doubt the change is really needed.
> > > >
> > > > If the pull is removed to match the Bluetooth module, then that sounds
> > > > as
> > > > if the signal was floating on the the BT side, which I think is not the
> > > > case.
> > > > Yes, according to the datasheet there is no pull when the BT controller
> > > > is
> > > > active, but then it drives the signal actively to either high or low.
> > > > There
> > > > seems to be no merit in 'matching' the Bluetooth side in this case, if
> > > > the
> > > > signal was really floating on the BT side we would definitely not want
> > > > this.
> > > >
> > > > In a reply to v2 you said:
> > > >
> > > > > 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.
> > > >
> > > > What are these conflicting pull states?
> > > >
> > > > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
> > > > boot mode, and no pull in active mode. In reset and boot mode the host
> > > > config with a pull down would match, and no pull in active mode doesn't
> > > > conflict with the pull-down on the host UART. My understanding is that
> > > > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
> > > > RTS the pull on the host side shouldn't matter.
> > > >
> > > 
> > > yes, I agree with you, the pinconf pulls are weak. As this is driven
> > > by BT
> > > SoC (pull on HOST side shouldn't matter), we are not mentioning any
> > > bias
> > > configuration from our side and simply putting it as no-pull, just
> > > to not
> > > conflict in any case. It seems that the rationale mentioned is a bit
> > > confusing i will change it to clearly specify why we are configuring
> > > no-pull.
> > > 
> > > > Is this change actually related with wakeup support? I have the
> > > > impression
> > > > that multiple things are conflated in this patch. If some of the changes
> > > > are just fixing/improving other things they should be in a separate
> > > > patch,
> > > > which could be part of this series, otherwise it's really hard to
> > > > distinguish between the pieces that are actually relevant for wakeup and
> > > > the rest.
> > > >
> > > > Independently of whether the changes are done in a single or multiple
> > > > patches, the commit log should include details on why the changes are
> > > > necessary, especially when there are not explantatory comments in the
> > > > DT/code itself (e.g. the removal of 'output-high', which seems correct
> > > > to me, but no reason is given why it is done).
> > > >
> > > 
> > > This change is not related to wakeup support, I will make it a
> > > separate
> > > patch, will also mention the details in commit text.
> > > 
> > > > >  	};
> > > > >
> > > > >  	pinconf-rts {
> > > > > -		/* We'll drive 39 (RTS), so no pull */
> > > > > +		/*
> > > > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
> > > > > +		 * floating pin which could mislead Bluetooth controller
> > > > > +		 * with UART RFR state (READY/NOT_READY).
> > > > > +		 */
> > > > >  		pins = "gpio39";
> > > > >  		drive-strength = <2>;
> > > > > -		bias-disable;
> > > > > +		bias-pull-down;
> > > > >  	};
> > > >
> > > > [copy of my comment on v2]
> > > >
> > > > 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.
> > > 
> > > Yes, RTS is either driven high or low. HW flow control is always
> > > enabled and
> > > only turned off when RX FIFO is full. Whereas SW flow control is
> > > controlled
> > > by upper layers(serial core), also it can be enabled/disabled from
> > > host by
> > > calling set_mctrl.
> > 
> > As far as I understand the above isn't entirely correct. HW flow control
> > is not
> > disabled when the RX FIFO is full, rather as part of HW flow control the
> > hardware deasserts RTS when the FIFO is full. Software flow control
> > isn't really
> > relevant here, since it doesn't use RTS/CTS but uses transmission of
> > special
> > codes (XON/XOFF) over TX/RX.
> 
> Here by Software flow control i meant, we can control the flow from
> SW(explained below).

Better don't use a term that already has well defined meaning in a
given context when you refer to something else.

> > 
> > > > 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.
> > > >
> > > > >
> > > > >  	pinconf-tx {
> > > > > @@ -494,7 +494,43 @@
> > > > >  		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 {
> > > > > +		/*
> > > > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
> > > > > +		 * floating pin which could mislead Bluetooth controller
> > > > > +		 * with UART RFR state (READY/NOT_READY).
> > > > > +		 */
> > > > > +		pins = "gpio39";
> > > > > +		drive-strength = <2>;
> > 
> > just noticed this: in the sleep config all pins are in GPIO config (see
> > "arm64: dts: sc7180: Add wakeup support over UART RX" from this series)
> > and by default they are inputs, hence the drive-strength here is
> > pointless
> > IIUC.
> > 
> 
> CTS and RX are inputs to the HOST whereas RTS and TX are outputs. We have
> added drive-strength for output pins only as they are driven by UART(please
> correct me if wrong).

True, RTS and TX are outputs in UART mode, however in sleep mode the pins
are (currently) configured as GPIOs and inputs (again, see "arm64: dts:
sc7180: Add wakeup support over UART RX" of this series), hence the
drive-strength attribute does nothing. If needed/preferred you can configure
the pins as outputs and specify the desired state instead of using pulls,
in that case specifying the drive strength can be useful.

> > > > > +		bias-pull-down;
> > > > > +	};
> > > >
> > > > I don't know all the details, but I have the impression that this is the
> > > > relevant pull change for wakeup. From the title of the series I derive
> > > > that the UART RX pin is used for signalling wakeup. A pull-down on RTS
> > > > indicates the BT controller that it can always send data to wake up the
> > > > host.
> > > >
> > > > I think RTS in default mode should remain with no-pull (the UART is
> > > > driving
> > > > the signal), and then change it to pull-down in sleep mode.
> > > >
> > > >
> > > 
> > > As I understand from your previous comment, pinconf pulls are weak and
> > > cannot override the pull of controller.
> > 
> > I'm not sure this is an absolute truth. I think there may be cases where
> > the driver has to increase its drive strength..
> > 
> > > Although pull down is configured,
> > > data will be received only if host controller is ready to accept it.
> > > So, we
> > > want to put RTS in pull-down state(known state) instead of leaving
> > > it in
> > > ambiguous state i.e, no-pull(high/low).
> > 
> > I disgress. I'm pretty sure that you want RTS to be low to make sure
> > that
> > the BT SoC can wake up the system by sending whatever data it has to
> > send.
> > It won't do that if RTS is high (e.g. because that's its floating state
> > at that time). I just tried configuring a pull-up (also a known
> > non-ambiguous state), and Bluetooth wakeup doesn't work with that,
> > supposedly because the BT SoC/UART will wait for its CTS signal to be
> > low.
> > 
> 
> yes, you are right, we are keeping RTS low to make sure that BT SoC can
> wakeup the system by sending bytes.
> My intention here was to explain below case from your comment:
> 
> > > > 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.
> 
> 1. By default our HW flow is enabled(since we are configuring pull-down on
> RTS),and BT SoC can send data anytime.
> 2. But there is a SW mechanism where we can control the flow from software.
> In that case what ever is configured to UART_MANUAL_RFR(READY or NOT_READY)
> will override the dtsi pinconf pull and the RTS/RFR line is pulled high when
> HOST is not ready(while debugging the wake up issue we came across this).

This is generally correct while the system is running, but (with the current
pinconf) not when the system is suspended IIUC. When the system is in suspend
the function of the UART pins is changed to GPIO, hence the UART ceases to
control RTS.

Otherwise how do you explain that wakeup stops working when you configure
a pull-up instead of a pull-down? According to your comment the UART should
be driving the RTS depending on its readyness.

> So, as far as i understand, even if pull-down is configured on RTS, BT SoC
> can send data only when HOST is ready.
> Can you please let me know which case you mean here, when you say "by
> configuring it as a pull-down the BT SoC can send data regardless of whether
> the host UART actually asserts RTS". Is there any case which we are missing
> here?

I'm referring to the case where the system is suspended and the UART doesn't
control RTS (see above).

Thanks

Matthias

Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart

[skakit]

Hi Matthias,

On 2020-08-26 22:10, Matthias Kaehlcke wrote:
> Hi Satya,
> 
> On Wed, Aug 26, 2020 at 09:35:15PM +0530, skakit@codeaurora.org wrote:
>> Hi Matthias,
>> 
>> On 2020-08-25 22:08, Matthias Kaehlcke wrote:
>> > On Tue, Aug 25, 2020 at 06:42:28PM +0530, skakit@codeaurora.org wrote:
>> > > On 2020-08-21 22:52, Matthias Kaehlcke wrote:
>> > > > On Thu, Aug 20, 2020 at 07:21:06PM +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>
>> > > > > Reviewed-by: Akash Asthana <akashast@codeaurora.org>
>> > > > > ---
>> > > > > Changes in V2:
>> > > > >  - This patch adds sleep state for BT UART. Newly added in V2.
>> > > > >
>> > > > > Changes in V3:
>> > > > >  - Remove "output-high" for TX from both sleep and default states
>> > > > >    as it is not required. Configure pull-up for TX in sleep state.
>> > > > >
>> > > > >  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
>> > > > > +++++++++++++++++++++++++++------
>> > > > >  1 file changed, 45 insertions(+), 9 deletions(-)
>> > > > >
>> > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > > > index d8b5507..806f626 100644
>> > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > > > @@ -473,20 +473,20 @@
>> > > > >
>> > > > >  &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-pull-down;
>> > > > > -		output-high;
>> > > > > +		bias-disable;
>> > > >
>> > > > I think it should be ok in functional terms, but I don't like the
>> > > > rationale
>> > > > and also doubt the change is really needed.
>> > > >
>> > > > If the pull is removed to match the Bluetooth module, then that sounds
>> > > > as
>> > > > if the signal was floating on the the BT side, which I think is not the
>> > > > case.
>> > > > Yes, according to the datasheet there is no pull when the BT controller
>> > > > is
>> > > > active, but then it drives the signal actively to either high or low.
>> > > > There
>> > > > seems to be no merit in 'matching' the Bluetooth side in this case, if
>> > > > the
>> > > > signal was really floating on the BT side we would definitely not want
>> > > > this.
>> > > >
>> > > > In a reply to v2 you said:
>> > > >
>> > > > > 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.
>> > > >
>> > > > What are these conflicting pull states?
>> > > >
>> > > > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
>> > > > boot mode, and no pull in active mode. In reset and boot mode the host
>> > > > config with a pull down would match, and no pull in active mode doesn't
>> > > > conflict with the pull-down on the host UART. My understanding is that
>> > > > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
>> > > > RTS the pull on the host side shouldn't matter.
>> > > >
>> > >
>> > > yes, I agree with you, the pinconf pulls are weak. As this is driven
>> > > by BT
>> > > SoC (pull on HOST side shouldn't matter), we are not mentioning any
>> > > bias
>> > > configuration from our side and simply putting it as no-pull, just
>> > > to not
>> > > conflict in any case. It seems that the rationale mentioned is a bit
>> > > confusing i will change it to clearly specify why we are configuring
>> > > no-pull.
>> > >
>> > > > Is this change actually related with wakeup support? I have the
>> > > > impression
>> > > > that multiple things are conflated in this patch. If some of the changes
>> > > > are just fixing/improving other things they should be in a separate
>> > > > patch,
>> > > > which could be part of this series, otherwise it's really hard to
>> > > > distinguish between the pieces that are actually relevant for wakeup and
>> > > > the rest.
>> > > >
>> > > > Independently of whether the changes are done in a single or multiple
>> > > > patches, the commit log should include details on why the changes are
>> > > > necessary, especially when there are not explantatory comments in the
>> > > > DT/code itself (e.g. the removal of 'output-high', which seems correct
>> > > > to me, but no reason is given why it is done).
>> > > >
>> > >
>> > > This change is not related to wakeup support, I will make it a
>> > > separate
>> > > patch, will also mention the details in commit text.
>> > >
>> > > > >  	};
>> > > > >
>> > > > >  	pinconf-rts {
>> > > > > -		/* We'll drive 39 (RTS), so no pull */
>> > > > > +		/*
>> > > > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
>> > > > > +		 * floating pin which could mislead Bluetooth controller
>> > > > > +		 * with UART RFR state (READY/NOT_READY).
>> > > > > +		 */
>> > > > >  		pins = "gpio39";
>> > > > >  		drive-strength = <2>;
>> > > > > -		bias-disable;
>> > > > > +		bias-pull-down;
>> > > > >  	};
>> > > >
>> > > > [copy of my comment on v2]
>> > > >
>> > > > 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.
>> > >
>> > > Yes, RTS is either driven high or low. HW flow control is always
>> > > enabled and
>> > > only turned off when RX FIFO is full. Whereas SW flow control is
>> > > controlled
>> > > by upper layers(serial core), also it can be enabled/disabled from
>> > > host by
>> > > calling set_mctrl.
>> >
>> > As far as I understand the above isn't entirely correct. HW flow control
>> > is not
>> > disabled when the RX FIFO is full, rather as part of HW flow control the
>> > hardware deasserts RTS when the FIFO is full. Software flow control
>> > isn't really
>> > relevant here, since it doesn't use RTS/CTS but uses transmission of
>> > special
>> > codes (XON/XOFF) over TX/RX.
>> 
>> Here by Software flow control i meant, we can control the flow from
>> SW(explained below).
> 
> Better don't use a term that already has well defined meaning in a
> given context when you refer to something else.
> 

Okay.

>> >
>> > > > 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.
>> > > >
>> > > > >
>> > > > >  	pinconf-tx {
>> > > > > @@ -494,7 +494,43 @@
>> > > > >  		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 {
>> > > > > +		/*
>> > > > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
>> > > > > +		 * floating pin which could mislead Bluetooth controller
>> > > > > +		 * with UART RFR state (READY/NOT_READY).
>> > > > > +		 */
>> > > > > +		pins = "gpio39";
>> > > > > +		drive-strength = <2>;
>> >
>> > just noticed this: in the sleep config all pins are in GPIO config (see
>> > "arm64: dts: sc7180: Add wakeup support over UART RX" from this series)
>> > and by default they are inputs, hence the drive-strength here is
>> > pointless
>> > IIUC.
>> >
>> 
>> CTS and RX are inputs to the HOST whereas RTS and TX are outputs. We 
>> have
>> added drive-strength for output pins only as they are driven by 
>> UART(please
>> correct me if wrong).
> 
> True, RTS and TX are outputs in UART mode, however in sleep mode the 
> pins
> are (currently) configured as GPIOs and inputs (again, see "arm64: dts:
> sc7180: Add wakeup support over UART RX" of this series), hence the
> drive-strength attribute does nothing. If needed/preferred you can 
> configure
> the pins as outputs and specify the desired state instead of using 
> pulls,
> in that case specifying the drive strength can be useful.
> 

Ok, will remove the drive-strength from sleep state.

>> > > > > +		bias-pull-down;
>> > > > > +	};
>> > > >
>> > > > I don't know all the details, but I have the impression that this is the
>> > > > relevant pull change for wakeup. From the title of the series I derive
>> > > > that the UART RX pin is used for signalling wakeup. A pull-down on RTS
>> > > > indicates the BT controller that it can always send data to wake up the
>> > > > host.
>> > > >
>> > > > I think RTS in default mode should remain with no-pull (the UART is
>> > > > driving
>> > > > the signal), and then change it to pull-down in sleep mode.
>> > > >
>> > > >
>> > >
>> > > As I understand from your previous comment, pinconf pulls are weak and
>> > > cannot override the pull of controller.
>> >
>> > I'm not sure this is an absolute truth. I think there may be cases where
>> > the driver has to increase its drive strength..
>> >
>> > > Although pull down is configured,
>> > > data will be received only if host controller is ready to accept it.
>> > > So, we
>> > > want to put RTS in pull-down state(known state) instead of leaving
>> > > it in
>> > > ambiguous state i.e, no-pull(high/low).
>> >
>> > I disgress. I'm pretty sure that you want RTS to be low to make sure
>> > that
>> > the BT SoC can wake up the system by sending whatever data it has to
>> > send.
>> > It won't do that if RTS is high (e.g. because that's its floating state
>> > at that time). I just tried configuring a pull-up (also a known
>> > non-ambiguous state), and Bluetooth wakeup doesn't work with that,
>> > supposedly because the BT SoC/UART will wait for its CTS signal to be
>> > low.
>> >
>> 
>> yes, you are right, we are keeping RTS low to make sure that BT SoC 
>> can
>> wakeup the system by sending bytes.
>> My intention here was to explain below case from your comment:
>> 
>> > > > 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.
>> 
>> 1. By default our HW flow is enabled(since we are configuring 
>> pull-down on
>> RTS),and BT SoC can send data anytime.
>> 2. But there is a SW mechanism where we can control the flow from 
>> software.
>> In that case what ever is configured to UART_MANUAL_RFR(READY or 
>> NOT_READY)
>> will override the dtsi pinconf pull and the RTS/RFR line is pulled 
>> high when
>> HOST is not ready(while debugging the wake up issue we came across 
>> this).
> 
> This is generally correct while the system is running, but (with the 
> current
> pinconf) not when the system is suspended IIUC. When the system is in 
> suspend
> the function of the UART pins is changed to GPIO, hence the UART ceases 
> to
> control RTS.
> 
> Otherwise how do you explain that wakeup stops working when you 
> configure
> a pull-up instead of a pull-down? According to your comment the UART 
> should
> be driving the RTS depending on its readyness.
> 

True, I was explaining about UART mode(active case) only, in reply to 
your previous comment:

>> > > > 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.
>> > > >

So, we can keep pull-down in Active case and in sleep state it is 
mandatory to keep pull-down.

>> So, as far as i understand, even if pull-down is configured on RTS, BT 
>> SoC
>> can send data only when HOST is ready.
>> Can you please let me know which case you mean here, when you say "by
>> configuring it as a pull-down the BT SoC can send data regardless of 
>> whether
>> the host UART actually asserts RTS". Is there any case which we are 
>> missing
>> here?
> 
> I'm referring to the case where the system is suspended and the UART 
> doesn't
> control RTS (see above).
> 
> Thanks
> 
> Matthias

Thanks,
Satya Priya

Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart

[Matthias Kaehlcke]

Hi Satya,

On Thu, Aug 27, 2020 at 08:37:33PM +0530, skakit@codeaurora.org wrote:
> Hi Matthias,
> 
> On 2020-08-26 22:10, Matthias Kaehlcke wrote:
> > Hi Satya,
> > 
> > On Wed, Aug 26, 2020 at 09:35:15PM +0530, skakit@codeaurora.org wrote:
> > > Hi Matthias,
> > > 
> > > On 2020-08-25 22:08, Matthias Kaehlcke wrote:
> > > > On Tue, Aug 25, 2020 at 06:42:28PM +0530, skakit@codeaurora.org wrote:
> > > > > On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> > > > > > On Thu, Aug 20, 2020 at 07:21:06PM +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>
> > > > > > > Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> > > > > > > ---
> > > > > > > Changes in V2:
> > > > > > >  - This patch adds sleep state for BT UART. Newly added in V2.
> > > > > > >
> > > > > > > Changes in V3:
> > > > > > >  - Remove "output-high" for TX from both sleep and default states
> > > > > > >    as it is not required. Configure pull-up for TX in sleep state.
> > > > > > >
> > > > > > >  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > > > > > > +++++++++++++++++++++++++++------
> > > > > > >  1 file changed, 45 insertions(+), 9 deletions(-)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > index d8b5507..806f626 100644
> > > > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > @@ -473,20 +473,20 @@
> > > > > > >
> > > > > > >  &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-pull-down;
> > > > > > > -		output-high;
> > > > > > > +		bias-disable;
> > > > > >
> > > > > > I think it should be ok in functional terms, but I don't like the
> > > > > > rationale
> > > > > > and also doubt the change is really needed.
> > > > > >
> > > > > > If the pull is removed to match the Bluetooth module, then that sounds
> > > > > > as
> > > > > > if the signal was floating on the the BT side, which I think is not the
> > > > > > case.
> > > > > > Yes, according to the datasheet there is no pull when the BT controller
> > > > > > is
> > > > > > active, but then it drives the signal actively to either high or low.
> > > > > > There
> > > > > > seems to be no merit in 'matching' the Bluetooth side in this case, if
> > > > > > the
> > > > > > signal was really floating on the BT side we would definitely not want
> > > > > > this.
> > > > > >
> > > > > > In a reply to v2 you said:
> > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > What are these conflicting pull states?
> > > > > >
> > > > > > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
> > > > > > boot mode, and no pull in active mode. In reset and boot mode the host
> > > > > > config with a pull down would match, and no pull in active mode doesn't
> > > > > > conflict with the pull-down on the host UART. My understanding is that
> > > > > > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
> > > > > > RTS the pull on the host side shouldn't matter.
> > > > > >
> > > > >
> > > > > yes, I agree with you, the pinconf pulls are weak. As this is driven
> > > > > by BT
> > > > > SoC (pull on HOST side shouldn't matter), we are not mentioning any
> > > > > bias
> > > > > configuration from our side and simply putting it as no-pull, just
> > > > > to not
> > > > > conflict in any case. It seems that the rationale mentioned is a bit
> > > > > confusing i will change it to clearly specify why we are configuring
> > > > > no-pull.
> > > > >
> > > > > > Is this change actually related with wakeup support? I have the
> > > > > > impression
> > > > > > that multiple things are conflated in this patch. If some of the changes
> > > > > > are just fixing/improving other things they should be in a separate
> > > > > > patch,
> > > > > > which could be part of this series, otherwise it's really hard to
> > > > > > distinguish between the pieces that are actually relevant for wakeup and
> > > > > > the rest.
> > > > > >
> > > > > > Independently of whether the changes are done in a single or multiple
> > > > > > patches, the commit log should include details on why the changes are
> > > > > > necessary, especially when there are not explantatory comments in the
> > > > > > DT/code itself (e.g. the removal of 'output-high', which seems correct
> > > > > > to me, but no reason is given why it is done).
> > > > > >
> > > > >
> > > > > This change is not related to wakeup support, I will make it a
> > > > > separate
> > > > > patch, will also mention the details in commit text.
> > > > >
> > > > > > >  	};
> > > > > > >
> > > > > > >  	pinconf-rts {
> > > > > > > -		/* We'll drive 39 (RTS), so no pull */
> > > > > > > +		/*
> > > > > > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
> > > > > > > +		 * floating pin which could mislead Bluetooth controller
> > > > > > > +		 * with UART RFR state (READY/NOT_READY).
> > > > > > > +		 */
> > > > > > >  		pins = "gpio39";
> > > > > > >  		drive-strength = <2>;
> > > > > > > -		bias-disable;
> > > > > > > +		bias-pull-down;
> > > > > > >  	};
> > > > > >
> > > > > > [copy of my comment on v2]
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Yes, RTS is either driven high or low. HW flow control is always
> > > > > enabled and
> > > > > only turned off when RX FIFO is full. Whereas SW flow control is
> > > > > controlled
> > > > > by upper layers(serial core), also it can be enabled/disabled from
> > > > > host by
> > > > > calling set_mctrl.
> > > >
> > > > As far as I understand the above isn't entirely correct. HW flow control
> > > > is not
> > > > disabled when the RX FIFO is full, rather as part of HW flow control the
> > > > hardware deasserts RTS when the FIFO is full. Software flow control
> > > > isn't really
> > > > relevant here, since it doesn't use RTS/CTS but uses transmission of
> > > > special
> > > > codes (XON/XOFF) over TX/RX.
> > > 
> > > Here by Software flow control i meant, we can control the flow from
> > > SW(explained below).
> > 
> > Better don't use a term that already has well defined meaning in a
> > given context when you refer to something else.
> > 
> 
> Okay.
> 
> > > >
> > > > > > 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.
> > > > > >
> > > > > > >
> > > > > > >  	pinconf-tx {
> > > > > > > @@ -494,7 +494,43 @@
> > > > > > >  		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 {
> > > > > > > +		/*
> > > > > > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
> > > > > > > +		 * floating pin which could mislead Bluetooth controller
> > > > > > > +		 * with UART RFR state (READY/NOT_READY).
> > > > > > > +		 */
> > > > > > > +		pins = "gpio39";
> > > > > > > +		drive-strength = <2>;
> > > >
> > > > just noticed this: in the sleep config all pins are in GPIO config (see
> > > > "arm64: dts: sc7180: Add wakeup support over UART RX" from this series)
> > > > and by default they are inputs, hence the drive-strength here is
> > > > pointless
> > > > IIUC.
> > > >
> > > 
> > > CTS and RX are inputs to the HOST whereas RTS and TX are outputs. We
> > > have
> > > added drive-strength for output pins only as they are driven by
> > > UART(please
> > > correct me if wrong).
> > 
> > True, RTS and TX are outputs in UART mode, however in sleep mode the
> > pins
> > are (currently) configured as GPIOs and inputs (again, see "arm64: dts:
> > sc7180: Add wakeup support over UART RX" of this series), hence the
> > drive-strength attribute does nothing. If needed/preferred you can
> > configure
> > the pins as outputs and specify the desired state instead of using
> > pulls,
> > in that case specifying the drive strength can be useful.
> > 
> 
> Ok, will remove the drive-strength from sleep state.
> 
> > > > > > > +		bias-pull-down;
> > > > > > > +	};
> > > > > >
> > > > > > I don't know all the details, but I have the impression that this is the
> > > > > > relevant pull change for wakeup. From the title of the series I derive
> > > > > > that the UART RX pin is used for signalling wakeup. A pull-down on RTS
> > > > > > indicates the BT controller that it can always send data to wake up the
> > > > > > host.
> > > > > >
> > > > > > I think RTS in default mode should remain with no-pull (the UART is
> > > > > > driving
> > > > > > the signal), and then change it to pull-down in sleep mode.
> > > > > >
> > > > > >
> > > > >
> > > > > As I understand from your previous comment, pinconf pulls are weak and
> > > > > cannot override the pull of controller.
> > > >
> > > > I'm not sure this is an absolute truth. I think there may be cases where
> > > > the driver has to increase its drive strength..
> > > >
> > > > > Although pull down is configured,
> > > > > data will be received only if host controller is ready to accept it.
> > > > > So, we
> > > > > want to put RTS in pull-down state(known state) instead of leaving
> > > > > it in
> > > > > ambiguous state i.e, no-pull(high/low).
> > > >
> > > > I disgress. I'm pretty sure that you want RTS to be low to make sure
> > > > that
> > > > the BT SoC can wake up the system by sending whatever data it has to
> > > > send.
> > > > It won't do that if RTS is high (e.g. because that's its floating state
> > > > at that time). I just tried configuring a pull-up (also a known
> > > > non-ambiguous state), and Bluetooth wakeup doesn't work with that,
> > > > supposedly because the BT SoC/UART will wait for its CTS signal to be
> > > > low.
> > > >
> > > 
> > > yes, you are right, we are keeping RTS low to make sure that BT SoC
> > > can
> > > wakeup the system by sending bytes.
> > > My intention here was to explain below case from your comment:
> > > 
> > > > > > 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.
> > > 
> > > 1. By default our HW flow is enabled(since we are configuring
> > > pull-down on
> > > RTS),and BT SoC can send data anytime.
> > > 2. But there is a SW mechanism where we can control the flow from
> > > software.
> > > In that case what ever is configured to UART_MANUAL_RFR(READY or
> > > NOT_READY)
> > > will override the dtsi pinconf pull and the RTS/RFR line is pulled
> > > high when
> > > HOST is not ready(while debugging the wake up issue we came across
> > > this).
> > 
> > This is generally correct while the system is running, but (with the
> > current
> > pinconf) not when the system is suspended IIUC. When the system is in
> > suspend
> > the function of the UART pins is changed to GPIO, hence the UART ceases
> > to
> > control RTS.
> > 
> > Otherwise how do you explain that wakeup stops working when you
> > configure
> > a pull-up instead of a pull-down? According to your comment the UART
> > should
> > be driving the RTS depending on its readyness.
> > 
> 
> True, I was explaining about UART mode(active case) only, in reply to your
> previous comment:
> 
> > > > > > 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.
> > > > > >
> 
> So, we can keep pull-down in Active case and in sleep state it is mandatory
> to keep pull-down.

Keeping the pull-down in active mode should do no harm, but why do it if
it isn't needed? I think it is better to specify what is required and have
comments explaining the rationale, since there are some nitty gritty details
that may not be obvious at first as we have seen. In this sense I think it's
better to have a comment like "We drive RTS, so no pull", rather than
"Configure a pull-down on RTS to put the pin in a defined state / match sleep
config" or similar.

If there's a good reason to have a pull I'm totally open to configure it, but
so far I haven't seen a convincing argument for it.

Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart

[skakit]

Hi Matthias,

On 2020-08-27 20:53, Matthias Kaehlcke wrote:
> Hi Satya,
> 
> On Thu, Aug 27, 2020 at 08:37:33PM +0530, skakit@codeaurora.org wrote:
>> Hi Matthias,
>> 
>> On 2020-08-26 22:10, Matthias Kaehlcke wrote:
>> > Hi Satya,
>> >
>> > On Wed, Aug 26, 2020 at 09:35:15PM +0530, skakit@codeaurora.org wrote:
>> > > Hi Matthias,
>> > >
>> > > On 2020-08-25 22:08, Matthias Kaehlcke wrote:
>> > > > On Tue, Aug 25, 2020 at 06:42:28PM +0530, skakit@codeaurora.org wrote:
>> > > > > On 2020-08-21 22:52, Matthias Kaehlcke wrote:
>> > > > > > On Thu, Aug 20, 2020 at 07:21:06PM +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>
>> > > > > > > Reviewed-by: Akash Asthana <akashast@codeaurora.org>
>> > > > > > > ---
>> > > > > > > Changes in V2:
>> > > > > > >  - This patch adds sleep state for BT UART. Newly added in V2.
>> > > > > > >
>> > > > > > > Changes in V3:
>> > > > > > >  - Remove "output-high" for TX from both sleep and default states
>> > > > > > >    as it is not required. Configure pull-up for TX in sleep state.
>> > > > > > >
>> > > > > > >  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
>> > > > > > > +++++++++++++++++++++++++++------
>> > > > > > >  1 file changed, 45 insertions(+), 9 deletions(-)
>> > > > > > >
>> > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > > > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > > > > > index d8b5507..806f626 100644
>> > > > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> > > > > > > @@ -473,20 +473,20 @@
>> > > > > > >
>> > > > > > >  &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-pull-down;
>> > > > > > > -		output-high;
>> > > > > > > +		bias-disable;
>> > > > > >
>> > > > > > I think it should be ok in functional terms, but I don't like the
>> > > > > > rationale
>> > > > > > and also doubt the change is really needed.
>> > > > > >
>> > > > > > If the pull is removed to match the Bluetooth module, then that sounds
>> > > > > > as
>> > > > > > if the signal was floating on the the BT side, which I think is not the
>> > > > > > case.
>> > > > > > Yes, according to the datasheet there is no pull when the BT controller
>> > > > > > is
>> > > > > > active, but then it drives the signal actively to either high or low.
>> > > > > > There
>> > > > > > seems to be no merit in 'matching' the Bluetooth side in this case, if
>> > > > > > the
>> > > > > > signal was really floating on the BT side we would definitely not want
>> > > > > > this.
>> > > > > >
>> > > > > > In a reply to v2 you said:
>> > > > > >
>> > > > > > > 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.
>> > > > > >
>> > > > > > What are these conflicting pull states?
>> > > > > >
>> > > > > > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
>> > > > > > boot mode, and no pull in active mode. In reset and boot mode the host
>> > > > > > config with a pull down would match, and no pull in active mode doesn't
>> > > > > > conflict with the pull-down on the host UART. My understanding is that
>> > > > > > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
>> > > > > > RTS the pull on the host side shouldn't matter.
>> > > > > >
>> > > > >
>> > > > > yes, I agree with you, the pinconf pulls are weak. As this is driven
>> > > > > by BT
>> > > > > SoC (pull on HOST side shouldn't matter), we are not mentioning any
>> > > > > bias
>> > > > > configuration from our side and simply putting it as no-pull, just
>> > > > > to not
>> > > > > conflict in any case. It seems that the rationale mentioned is a bit
>> > > > > confusing i will change it to clearly specify why we are configuring
>> > > > > no-pull.
>> > > > >
>> > > > > > Is this change actually related with wakeup support? I have the
>> > > > > > impression
>> > > > > > that multiple things are conflated in this patch. If some of the changes
>> > > > > > are just fixing/improving other things they should be in a separate
>> > > > > > patch,
>> > > > > > which could be part of this series, otherwise it's really hard to
>> > > > > > distinguish between the pieces that are actually relevant for wakeup and
>> > > > > > the rest.
>> > > > > >
>> > > > > > Independently of whether the changes are done in a single or multiple
>> > > > > > patches, the commit log should include details on why the changes are
>> > > > > > necessary, especially when there are not explantatory comments in the
>> > > > > > DT/code itself (e.g. the removal of 'output-high', which seems correct
>> > > > > > to me, but no reason is given why it is done).
>> > > > > >
>> > > > >
>> > > > > This change is not related to wakeup support, I will make it a
>> > > > > separate
>> > > > > patch, will also mention the details in commit text.
>> > > > >
>> > > > > > >  	};
>> > > > > > >
>> > > > > > >  	pinconf-rts {
>> > > > > > > -		/* We'll drive 39 (RTS), so no pull */
>> > > > > > > +		/*
>> > > > > > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
>> > > > > > > +		 * floating pin which could mislead Bluetooth controller
>> > > > > > > +		 * with UART RFR state (READY/NOT_READY).
>> > > > > > > +		 */
>> > > > > > >  		pins = "gpio39";
>> > > > > > >  		drive-strength = <2>;
>> > > > > > > -		bias-disable;
>> > > > > > > +		bias-pull-down;
>> > > > > > >  	};
>> > > > > >
>> > > > > > [copy of my comment on v2]
>> > > > > >
>> > > > > > 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.
>> > > > >
>> > > > > Yes, RTS is either driven high or low. HW flow control is always
>> > > > > enabled and
>> > > > > only turned off when RX FIFO is full. Whereas SW flow control is
>> > > > > controlled
>> > > > > by upper layers(serial core), also it can be enabled/disabled from
>> > > > > host by
>> > > > > calling set_mctrl.
>> > > >
>> > > > As far as I understand the above isn't entirely correct. HW flow control
>> > > > is not
>> > > > disabled when the RX FIFO is full, rather as part of HW flow control the
>> > > > hardware deasserts RTS when the FIFO is full. Software flow control
>> > > > isn't really
>> > > > relevant here, since it doesn't use RTS/CTS but uses transmission of
>> > > > special
>> > > > codes (XON/XOFF) over TX/RX.
>> > >
>> > > Here by Software flow control i meant, we can control the flow from
>> > > SW(explained below).
>> >
>> > Better don't use a term that already has well defined meaning in a
>> > given context when you refer to something else.
>> >
>> 
>> Okay.
>> 
>> > > >
>> > > > > > 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.
>> > > > > >
>> > > > > > >
>> > > > > > >  	pinconf-tx {
>> > > > > > > @@ -494,7 +494,43 @@
>> > > > > > >  		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 {
>> > > > > > > +		/*
>> > > > > > > +		 * Configure pull-down on 39 (RTS). This is needed to avoid a
>> > > > > > > +		 * floating pin which could mislead Bluetooth controller
>> > > > > > > +		 * with UART RFR state (READY/NOT_READY).
>> > > > > > > +		 */
>> > > > > > > +		pins = "gpio39";
>> > > > > > > +		drive-strength = <2>;
>> > > >
>> > > > just noticed this: in the sleep config all pins are in GPIO config (see
>> > > > "arm64: dts: sc7180: Add wakeup support over UART RX" from this series)
>> > > > and by default they are inputs, hence the drive-strength here is
>> > > > pointless
>> > > > IIUC.
>> > > >
>> > >
>> > > CTS and RX are inputs to the HOST whereas RTS and TX are outputs. We
>> > > have
>> > > added drive-strength for output pins only as they are driven by
>> > > UART(please
>> > > correct me if wrong).
>> >
>> > True, RTS and TX are outputs in UART mode, however in sleep mode the
>> > pins
>> > are (currently) configured as GPIOs and inputs (again, see "arm64: dts:
>> > sc7180: Add wakeup support over UART RX" of this series), hence the
>> > drive-strength attribute does nothing. If needed/preferred you can
>> > configure
>> > the pins as outputs and specify the desired state instead of using
>> > pulls,
>> > in that case specifying the drive strength can be useful.
>> >
>> 
>> Ok, will remove the drive-strength from sleep state.
>> 
>> > > > > > > +		bias-pull-down;
>> > > > > > > +	};
>> > > > > >
>> > > > > > I don't know all the details, but I have the impression that this is the
>> > > > > > relevant pull change for wakeup. From the title of the series I derive
>> > > > > > that the UART RX pin is used for signalling wakeup. A pull-down on RTS
>> > > > > > indicates the BT controller that it can always send data to wake up the
>> > > > > > host.
>> > > > > >
>> > > > > > I think RTS in default mode should remain with no-pull (the UART is
>> > > > > > driving
>> > > > > > the signal), and then change it to pull-down in sleep mode.
>> > > > > >
>> > > > > >
>> > > > >
>> > > > > As I understand from your previous comment, pinconf pulls are weak and
>> > > > > cannot override the pull of controller.
>> > > >
>> > > > I'm not sure this is an absolute truth. I think there may be cases where
>> > > > the driver has to increase its drive strength..
>> > > >
>> > > > > Although pull down is configured,
>> > > > > data will be received only if host controller is ready to accept it.
>> > > > > So, we
>> > > > > want to put RTS in pull-down state(known state) instead of leaving
>> > > > > it in
>> > > > > ambiguous state i.e, no-pull(high/low).
>> > > >
>> > > > I disgress. I'm pretty sure that you want RTS to be low to make sure
>> > > > that
>> > > > the BT SoC can wake up the system by sending whatever data it has to
>> > > > send.
>> > > > It won't do that if RTS is high (e.g. because that's its floating state
>> > > > at that time). I just tried configuring a pull-up (also a known
>> > > > non-ambiguous state), and Bluetooth wakeup doesn't work with that,
>> > > > supposedly because the BT SoC/UART will wait for its CTS signal to be
>> > > > low.
>> > > >
>> > >
>> > > yes, you are right, we are keeping RTS low to make sure that BT SoC
>> > > can
>> > > wakeup the system by sending bytes.
>> > > My intention here was to explain below case from your comment:
>> > >
>> > > > > > 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.
>> > >
>> > > 1. By default our HW flow is enabled(since we are configuring
>> > > pull-down on
>> > > RTS),and BT SoC can send data anytime.
>> > > 2. But there is a SW mechanism where we can control the flow from
>> > > software.
>> > > In that case what ever is configured to UART_MANUAL_RFR(READY or
>> > > NOT_READY)
>> > > will override the dtsi pinconf pull and the RTS/RFR line is pulled
>> > > high when
>> > > HOST is not ready(while debugging the wake up issue we came across
>> > > this).
>> >
>> > This is generally correct while the system is running, but (with the
>> > current
>> > pinconf) not when the system is suspended IIUC. When the system is in
>> > suspend
>> > the function of the UART pins is changed to GPIO, hence the UART ceases
>> > to
>> > control RTS.
>> >
>> > Otherwise how do you explain that wakeup stops working when you
>> > configure
>> > a pull-up instead of a pull-down? According to your comment the UART
>> > should
>> > be driving the RTS depending on its readyness.
>> >
>> 
>> True, I was explaining about UART mode(active case) only, in reply to 
>> your
>> previous comment:
>> 
>> > > > > > 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.
>> > > > > >
>> 
>> So, we can keep pull-down in Active case and in sleep state it is 
>> mandatory
>> to keep pull-down.
> 
> Keeping the pull-down in active mode should do no harm, but why do it 
> if
> it isn't needed? I think it is better to specify what is required and 
> have
> comments explaining the rationale, since there are some nitty gritty 
> details
> that may not be obvious at first as we have seen. In this sense I think 
> it's
> better to have a comment like "We drive RTS, so no pull", rather than
> "Configure a pull-down on RTS to put the pin in a defined state / match 
> sleep
> config" or similar.
> 
> If there's a good reason to have a pull I'm totally open to configure 
> it, but
> so far I haven't seen a convincing argument for it.

Okay, will configure no-pull for RTS in active state.

[PATCH V3 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue

[satya priya]

As a part of system suspend uart_port_suspend is called 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. Include 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>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
---
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.

 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..2aad9d7 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