linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V5 0/4] Add wakeup support over UART RX
@ 2020-09-10 12:53 satya priya
  2020-09-10 12:53 ` [PATCH V5 1/4] arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX satya priya
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: satya priya @ 2020-09-10 12:53 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Matthias Kaehlcke, gregkh, Andy Gross, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, akashast, rojay,
	msavaliy, dianders, 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.

Changes in V4:
 - As per Matthias's comments, change the commit text to mention why
   GPIO function needs to be selected in sleep.
 - Add separate patch for improvements made in pin conf settings.

Changes in V5:
 - Moved pinctrl and interrupt configuration to board specific files.
 - Added new patch for trogdor board specific changes.

satya priya (4):
  arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and
    TX
  arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config
    for BT UART
  arm64: dts: qcom: sc7180-trogdor: Add pinctrl and interrupt config for
    BT UART
  tty: serial: qcom_geni_serial: Fix the UART wakeup issue

 arch/arm64/boot/dts/qcom/sc7180-idp.dts      | 58 +++++++++++++++++++++++++---
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 48 +++++++++++++++++++++++
 drivers/tty/serial/qcom_geni_serial.c        |  2 +-
 3 files changed, 101 insertions(+), 7 deletions(-)

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH V5 1/4] arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX
  2020-09-10 12:53 [PATCH V5 0/4] Add wakeup support over UART RX satya priya
@ 2020-09-10 12:53 ` satya priya
  2020-09-10 14:40   ` Doug Anderson
  2020-09-10 12:53 ` [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART satya priya
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: satya priya @ 2020-09-10 12:53 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Matthias Kaehlcke, gregkh, Andy Gross, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, akashast, rojay,
	msavaliy, dianders, satya priya

Remove output-high from CTS and TX as this is not really required. During
bringup to fix transfer failures this was added to match with console uart
settings. Probably some boot loader config was missing then. As it is
working fine now, remove it.

Signed-off-by: satya priya <skakit@codeaurora.org>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in V4:
 - This is newly added in V4 to separate the improvements in pin settings
   and wakeup related changes.

Changes in V5:
 - As per Doug's comment configured pull-down for CTS pin as earlier.

 arch/arm64/boot/dts/qcom/sc7180-idp.dts | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
index d8b5507..04888df 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
@@ -474,32 +474,30 @@
 &qup_uart3_default {
 	pinconf-cts {
 		/*
-		 * Configure a pull-down on 38 (CTS) to match the pull of
+		 * Configure a pull-down on CTS to match the pull of
 		 * the Bluetooth module.
 		 */
 		pins = "gpio38";
 		bias-pull-down;
-		output-high;
 	};
 
 	pinconf-rts {
-		/* We'll drive 39 (RTS), so no pull */
+		/* We'll drive RTS, so no pull */
 		pins = "gpio39";
 		drive-strength = <2>;
 		bias-disable;
 	};
 
 	pinconf-tx {
-		/* We'll drive 40 (TX), so no pull */
+		/* We'll drive TX, so no pull */
 		pins = "gpio40";
 		drive-strength = <2>;
 		bias-disable;
-		output-high;
 	};
 
 	pinconf-rx {
 		/*
-		 * Configure a pull-up on 41 (RX). This is needed to avoid
+		 * Configure a pull-up on RX. This is needed to avoid
 		 * garbage data when the TX pin of the Bluetooth module is
 		 * in tri-state (module powered off or not driving the
 		 * signal yet).
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
  2020-09-10 12:53 [PATCH V5 0/4] Add wakeup support over UART RX satya priya
  2020-09-10 12:53 ` [PATCH V5 1/4] arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX satya priya
@ 2020-09-10 12:53 ` satya priya
  2020-09-10 14:40   ` Doug Anderson
                     ` (2 more replies)
  2020-09-10 12:53 ` [PATCH V5 3/4] arm64: dts: qcom: sc7180-trogdor: Add " satya priya
  2020-09-10 12:53 ` [PATCH V5 4/4] tty: serial: qcom_geni_serial: Fix the UART wakeup issue satya priya
  3 siblings, 3 replies; 19+ messages in thread
From: satya priya @ 2020-09-10 12:53 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Matthias Kaehlcke, gregkh, Andy Gross, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, akashast, rojay,
	msavaliy, dianders, satya priya

Add a suitable sleep configuration for uart3 to support Bluetooth wakeup.

If QUP function is selected in sleep state, UART RTS/RFR is pulled high
during suspend and BT SoC not able to send wakeup bytes. So, configure
GPIO mode in sleep state to keep it low during suspend.

Signed-off-by: satya priya <skakit@codeaurora.org>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
---
Changes in V2:
 - This patch adds sleep state for BT UART. Newly added in V2.

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

Changes in V4:
 - As per Matthias's comment, removed drive-strength for sleep state
   and fixed nit-pick.

Changes in V5:
 - As per Matthias's comments, moved pinmux change for sleep state,
   pinctrl and interrupt config to the board specific file.

 arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 +++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
index 04888df..e529a41 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
@@ -344,6 +344,10 @@
 };
 
 &uart3 {
+	pinctrl-names = "default", "sleep";
+	pinctrl-1 = <&qup_uart3_sleep>;
+	interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
+				<&tlmm 41 IRQ_TYPE_EDGE_FALLING>;
 	status = "okay";
 
 	bluetooth: wcn3990-bt {
@@ -545,3 +549,47 @@
 	};
 };
 
+&tlmm {
+	qup_uart3_sleep: qup-uart3-sleep {
+		pinmux {
+			pins = "gpio38", "gpio39",
+			       "gpio40", "gpio41";
+			function = "gpio";
+		};
+
+		pinconf-cts {
+			/*
+			 * Configure a pull-down on CTS to match the pull of
+			 * the Bluetooth module.
+			 */
+			pins = "gpio38";
+			bias-pull-down;
+		};
+
+		pinconf-rts {
+			/*
+			 * Configure pull-down on RTS to make sure that the BT SoC can
+			 * wake up the system by sending wakeup bytes during suspend.
+			 */
+			 pins = "gpio39";
+			 bias-pull-down;
+		};
+
+		pinconf-tx {
+			/* Configure pull-up on TX when it isn't actively driven */
+			pins = "gpio40";
+			bias-pull-up;
+		};
+
+		pinconf-rx {
+			/*
+			 * Configure a pull-up on RX. This is needed to avoid
+			 * garbage data when the TX pin of the Bluetooth module is
+			 * in tri-state (module powered off or not driving the
+			 * signal yet).
+			 */
+			pins = "gpio41";
+			bias-pull-up;
+		};
+	};
+};
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH V5 3/4] arm64: dts: qcom: sc7180-trogdor: Add pinctrl and interrupt config for BT UART
  2020-09-10 12:53 [PATCH V5 0/4] Add wakeup support over UART RX satya priya
  2020-09-10 12:53 ` [PATCH V5 1/4] arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX satya priya
  2020-09-10 12:53 ` [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART satya priya
@ 2020-09-10 12:53 ` satya priya
  2020-09-10 14:42   ` Doug Anderson
  2020-09-10 12:53 ` [PATCH V5 4/4] tty: serial: qcom_geni_serial: Fix the UART wakeup issue satya priya
  3 siblings, 1 reply; 19+ messages in thread
From: satya priya @ 2020-09-10 12:53 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Matthias Kaehlcke, gregkh, Andy Gross, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, akashast, rojay,
	msavaliy, dianders, satya priya

Add a suitable sleep configuration for uart3 to support Bluetooth wakeup.

If QUP function is selected in sleep state, UART RTS/RFR is pulled high
during suspend and BT SoC not able to send wakeup bytes. So, configure
GPIO mode in sleep state to keep it low during suspend.

Signed-off-by: satya priya <skakit@codeaurora.org>
---
Changes in V5:
 - Newly added in V5. This patch adds wakeup support for trogdor board files.

 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 48 ++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index a6b9beb..96b5331 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -792,6 +792,11 @@ ap_spi_fp: &spi10 {
 #include <arm/cros-ec-sbs.dtsi>
 
 &uart3 {
+	pinctrl-names = "default", "sleep";
+	pinctrl-1 = <&qup_uart3_sleep>;
+	interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
+				<&tlmm 41 IRQ_TYPE_EDGE_FALLING>;
+
 	status = "okay";
 
 	bluetooth: bluetooth {
@@ -1345,4 +1350,47 @@ ap_spi_fp: &spi10 {
 			drive-strength = <2>;
 		};
 	};
+
+	qup_uart3_sleep: qup-uart3-sleep {
+		pinmux {
+			pins = "gpio38", "gpio39",
+			       "gpio40", "gpio41";
+			function = "gpio";
+		};
+
+		pinconf-cts {
+			/*
+			 * Configure a pull-down on CTS to match the pull of
+			 * the Bluetooth module.
+			 */
+			pins = "gpio38";
+			bias-pull-down;
+		};
+
+		pinconf-rts {
+			/*
+			 * Configure pull-down on RTS to make sure that the BT SoC can
+			 * wake up the system by sending wakeup bytes during suspend.
+			 */
+			 pins = "gpio39";
+			 bias-pull-down;
+		};
+
+		pinconf-tx {
+			/* Configure pull-up on TX when it isn't actively driven */
+			pins = "gpio40";
+			bias-pull-up;
+		};
+
+		pinconf-rx {
+			/*
+			 * Configure a pull-up on RX. This is needed to avoid
+			 * garbage data when the TX pin of the Bluetooth module is
+			 * in tri-state (module powered off or not driving the
+			 * signal yet).
+			 */
+			pins = "gpio41";
+			bias-pull-up;
+		};
+	};
 };
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH V5 4/4] tty: serial: qcom_geni_serial: Fix the UART wakeup issue
  2020-09-10 12:53 [PATCH V5 0/4] Add wakeup support over UART RX satya priya
                   ` (2 preceding siblings ...)
  2020-09-10 12:53 ` [PATCH V5 3/4] arm64: dts: qcom: sc7180-trogdor: Add " satya priya
@ 2020-09-10 12:53 ` satya priya
  2020-09-10 23:06   ` Bjorn Andersson
  3 siblings, 1 reply; 19+ messages in thread
From: satya priya @ 2020-09-10 12:53 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Matthias Kaehlcke, gregkh, Andy Gross, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, akashast, rojay,
	msavaliy, dianders, satya priya

As a part of system suspend uart_port_suspend is called from the
Serial driver, which calls set_mctrl passing mctrl as 0. This
makes RFR high(NOT_READY) during suspend.

Due to this BT SoC is not able to send wakeup bytes to UART during
suspend. Include if check for non-suspend case to keep RFR low
during suspend.

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.

Changes in V4:
 - No change.

Changes in V5:
 - As per Matthias comment, fixed nit-pick in commit text.

 drivers/tty/serial/qcom_geni_serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 3aa29d2..bc63c54 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -242,7 +242,7 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport,
 	if (mctrl & TIOCM_LOOP)
 		port->loopback = RX_TX_CTS_RTS_SORTED;
 
-	if (!(mctrl & TIOCM_RTS))
+	if (!(mctrl & TIOCM_RTS) && !uport->suspended)
 		uart_manual_rfr = UART_MANUAL_RFR_EN | UART_RFR_NOT_READY;
 	writel(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 1/4] arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX
  2020-09-10 12:53 ` [PATCH V5 1/4] arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX satya priya
@ 2020-09-10 14:40   ` Doug Anderson
  2020-09-11 10:14     ` skakit
  0 siblings, 1 reply; 19+ messages in thread
From: Doug Anderson @ 2020-09-10 14:40 UTC (permalink / raw)
  To: satya priya
  Cc: Bjorn Andersson, Matthias Kaehlcke, Greg Kroah-Hartman,
	Andy Gross, Rob Herring, linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Akash Asthana, Roja Rani Yarubandi, msavaliy

Hi,

On Thu, Sep 10, 2020 at 5:55 AM satya priya <skakit@codeaurora.org> wrote:
>
> Remove output-high from CTS and TX as this is not really required. During
> bringup to fix transfer failures this was added to match with console uart
> settings. Probably some boot loader config was missing then. As it is
> working fine now, remove it.
>
> Signed-off-by: satya priya <skakit@codeaurora.org>
> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in V4:
>  - This is newly added in V4 to separate the improvements in pin settings
>    and wakeup related changes.
>
> Changes in V5:
>  - As per Doug's comment configured pull-down for CTS pin as earlier.
>
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

Looks fine to me.  Slight nit that this only applies to the IDP board
but ${SUBJECT} makes it sound as if this applies to all sc7180.  I
wouldn't spin just for that, though.  If Bjorn agrees, he can always
adjust the subject when applying.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
  2020-09-10 12:53 ` [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART satya priya
@ 2020-09-10 14:40   ` Doug Anderson
  2020-09-11 10:15     ` skakit
  2020-09-10 15:35   ` Matthias Kaehlcke
  2020-09-10 23:45   ` Bjorn Andersson
  2 siblings, 1 reply; 19+ messages in thread
From: Doug Anderson @ 2020-09-10 14:40 UTC (permalink / raw)
  To: satya priya
  Cc: Bjorn Andersson, Matthias Kaehlcke, Greg Kroah-Hartman,
	Andy Gross, Rob Herring, linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Akash Asthana, Roja Rani Yarubandi, msavaliy

Hi,

On Thu, Sep 10, 2020 at 5:55 AM satya priya <skakit@codeaurora.org> wrote:
>
> Add a suitable sleep configuration for uart3 to support Bluetooth wakeup.
>
> If QUP function is selected in sleep state, UART RTS/RFR is pulled high
> during suspend and BT SoC not able to send wakeup bytes. So, configure
> GPIO mode in sleep state to keep it low during suspend.
>
> Signed-off-by: satya priya <skakit@codeaurora.org>
> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V2:
>  - This patch adds sleep state for BT UART. Newly added in V2.
>
> Changes in V3:
>  - Remove "output-high" for TX from both sleep and default states
>    as it is not required. Configure pull-up for TX in sleep state.
>
> Changes in V4:
>  - As per Matthias's comment, removed drive-strength for sleep state
>    and fixed nit-pick.
>
> Changes in V5:
>  - As per Matthias's comments, moved pinmux change for sleep state,
>    pinctrl and interrupt config to the board specific file.
>
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 +++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)

Similar comment to patch #1 in that this applies only to the IDP board
but that's not obvious from ${SUBJECT}


> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index 04888df..e529a41 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -344,6 +344,10 @@
>  };
>
>  &uart3 {
> +       pinctrl-names = "default", "sleep";
> +       pinctrl-1 = <&qup_uart3_sleep>;
> +       interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
> +                               <&tlmm 41 IRQ_TYPE_EDGE_FALLING>;

You need a:

/delete-property/interrupts;

...or, alternatively, a patch before this one that converts all the
UARTs in sc7180 to just use interrupts-extended.


>         status = "okay";

Slight nit is that usually I see the status line first.  All the other
instances in this file have it that way.  Can you match?




-Doug

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 3/4] arm64: dts: qcom: sc7180-trogdor: Add pinctrl and interrupt config for BT UART
  2020-09-10 12:53 ` [PATCH V5 3/4] arm64: dts: qcom: sc7180-trogdor: Add " satya priya
@ 2020-09-10 14:42   ` Doug Anderson
  2020-09-11 10:15     ` skakit
  0 siblings, 1 reply; 19+ messages in thread
From: Doug Anderson @ 2020-09-10 14:42 UTC (permalink / raw)
  To: satya priya
  Cc: Bjorn Andersson, Matthias Kaehlcke, Greg Kroah-Hartman,
	Andy Gross, Rob Herring, linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Akash Asthana, Roja Rani Yarubandi, msavaliy

Hi,

On Thu, Sep 10, 2020 at 5:55 AM satya priya <skakit@codeaurora.org> wrote:
>
> Add a suitable sleep configuration for uart3 to support Bluetooth wakeup.
>
> If QUP function is selected in sleep state, UART RTS/RFR is pulled high
> during suspend and BT SoC not able to send wakeup bytes. So, configure
> GPIO mode in sleep state to keep it low during suspend.
>
> Signed-off-by: satya priya <skakit@codeaurora.org>
> ---
> Changes in V5:
>  - Newly added in V5. This patch adds wakeup support for trogdor board files.
>
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 48 ++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)

Note: I can't find this email on any of the mailing lists.  Can you
check your config?  I tried:

http://lore.kernel.org/r/1599742438-16811-4-git-send-email-skakit@codeaurora.org

...and also checked patchwork servers.  I only see patch 1 and 2.  I
think Bjorn usually applies from patchwork so this'll likely be a
problem...


> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index a6b9beb..96b5331 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -792,6 +792,11 @@ ap_spi_fp: &spi10 {
>  #include <arm/cros-ec-sbs.dtsi>
>
>  &uart3 {
> +       pinctrl-names = "default", "sleep";
> +       pinctrl-1 = <&qup_uart3_sleep>;
> +       interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
> +                               <&tlmm 41 IRQ_TYPE_EDGE_FALLING>;
> +
>         status = "okay";

Same comments here as for patch #2.


>         bluetooth: bluetooth {
> @@ -1345,4 +1350,47 @@ ap_spi_fp: &spi10 {
>                         drive-strength = <2>;
>                 };
>         };
> +
> +       qup_uart3_sleep: qup-uart3-sleep {

I believe things in this section are supposed to be sorted
alphabetically.  Thus "qup..." should be sorted before "trackpad..."

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
  2020-09-10 12:53 ` [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART satya priya
  2020-09-10 14:40   ` Doug Anderson
@ 2020-09-10 15:35   ` Matthias Kaehlcke
  2020-09-11 10:15     ` skakit
  2020-09-10 23:45   ` Bjorn Andersson
  2 siblings, 1 reply; 19+ messages in thread
From: Matthias Kaehlcke @ 2020-09-10 15:35 UTC (permalink / raw)
  To: satya priya
  Cc: Bjorn Andersson, Matthias Kaehlcke, gregkh, Andy Gross,
	Rob Herring, linux-arm-msm, devicetree, linux-kernel, akashast,
	rojay, msavaliy, dianders

El Thu, Sep 10, 2020 at 06:23:56PM +0530 satya priya ha dit:

> Add a suitable sleep configuration for uart3 to support Bluetooth wakeup.
> 
> If QUP function is selected in sleep state, UART RTS/RFR is pulled high
> during suspend and BT SoC not able to send wakeup bytes. So, configure
> GPIO mode in sleep state to keep it low during suspend.
> 
> Signed-off-by: satya priya <skakit@codeaurora.org>
> Reviewed-by: Akash Asthana <akashast@codeaurora.org>

Was this change really reviewed (privately) by Akash or are you still
carrying forward this tag from v2? The configuration and the comments
have change substantially since v2, IMO you should drop the tag unless
Akash really reviewed the current version or something close to it.

> ---
> Changes in V2:
>  - This patch adds sleep state for BT UART. Newly added in V2.
> 
> Changes in V3:
>  - Remove "output-high" for TX from both sleep and default states
>    as it is not required. Configure pull-up for TX in sleep state.
> 
> Changes in V4:
>  - As per Matthias's comment, removed drive-strength for sleep state
>    and fixed nit-pick.
> 
> Changes in V5:
>  - As per Matthias's comments, moved pinmux change for sleep state,
>    pinctrl and interrupt config to the board specific file.
> 
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 +++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index 04888df..e529a41 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -344,6 +344,10 @@
>  };
>  
>  &uart3 {
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-1 = <&qup_uart3_sleep>;
> +	interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
> +				<&tlmm 41 IRQ_TYPE_EDGE_FALLING>;

As Doug already said, you need to delete the 'interrupts' property now
that we have 'interrupts-extended'.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 4/4] tty: serial: qcom_geni_serial: Fix the UART wakeup issue
  2020-09-10 12:53 ` [PATCH V5 4/4] tty: serial: qcom_geni_serial: Fix the UART wakeup issue satya priya
@ 2020-09-10 23:06   ` Bjorn Andersson
  2020-09-16 12:25     ` Greg KH
  0 siblings, 1 reply; 19+ messages in thread
From: Bjorn Andersson @ 2020-09-10 23:06 UTC (permalink / raw)
  To: satya priya, gregkh
  Cc: Matthias Kaehlcke, Andy Gross, Rob Herring, linux-arm-msm,
	devicetree, linux-kernel, akashast, rojay, msavaliy, dianders

On Thu 10 Sep 12:53 UTC 2020, satya priya wrote:

> As a part of system suspend uart_port_suspend is called from the
> Serial driver, which calls set_mctrl passing mctrl as 0. This
> makes RFR high(NOT_READY) during suspend.
> 
> Due to this BT SoC is not able to send wakeup bytes to UART during
> suspend. Include if check for non-suspend case to keep RFR low
> during suspend.
> 

Seems reasonable.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> Signed-off-by: satya priya <skakit@codeaurora.org>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Greg, I don't see this depending on anything else, will you pick this
patch through your tree? I will take the dts patches through the qcom
tree.

Regards,
Bjorn

> 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.
> 
> Changes in V4:
>  - No change.
> 
> Changes in V5:
>  - As per Matthias comment, fixed nit-pick in commit text.
> 
>  drivers/tty/serial/qcom_geni_serial.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3aa29d2..bc63c54 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -242,7 +242,7 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport,
>  	if (mctrl & TIOCM_LOOP)
>  		port->loopback = RX_TX_CTS_RTS_SORTED;
>  
> -	if (!(mctrl & TIOCM_RTS))
> +	if (!(mctrl & TIOCM_RTS) && !uport->suspended)
>  		uart_manual_rfr = UART_MANUAL_RFR_EN | UART_RFR_NOT_READY;
>  	writel(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
>  }
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
> of Code Aurora Forum, hosted by The Linux Foundation
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
  2020-09-10 12:53 ` [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART satya priya
  2020-09-10 14:40   ` Doug Anderson
  2020-09-10 15:35   ` Matthias Kaehlcke
@ 2020-09-10 23:45   ` Bjorn Andersson
  2020-09-11 10:16     ` skakit
  2 siblings, 1 reply; 19+ messages in thread
From: Bjorn Andersson @ 2020-09-10 23:45 UTC (permalink / raw)
  To: satya priya
  Cc: Matthias Kaehlcke, gregkh, Andy Gross, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, akashast, rojay,
	msavaliy, dianders

On Thu 10 Sep 12:53 UTC 2020, satya priya wrote:

> Add a suitable sleep configuration for uart3 to support Bluetooth wakeup.
> 
> If QUP function is selected in sleep state, UART RTS/RFR is pulled high
> during suspend and BT SoC not able to send wakeup bytes. So, configure
> GPIO mode in sleep state to keep it low during suspend.
> 

But patch 4 says that you change this behavior, is that patch really
needed if we switch the pins to GPIO, or if this patch really needed if
we merge patch 4?

Could it be that in lower power states we drop the power to the uart
block and rely on the PDC to wait for the BT chip to start sending the
wakeup bytes on the rx pin?


This commit will become the reference for all other platforms where we
enable the same functionality, so better document it properly.

> Signed-off-by: satya priya <skakit@codeaurora.org>
> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V2:
>  - This patch adds sleep state for BT UART. Newly added in V2.
> 
> Changes in V3:
>  - Remove "output-high" for TX from both sleep and default states
>    as it is not required. Configure pull-up for TX in sleep state.
> 
> Changes in V4:
>  - As per Matthias's comment, removed drive-strength for sleep state
>    and fixed nit-pick.
> 
> Changes in V5:
>  - As per Matthias's comments, moved pinmux change for sleep state,
>    pinctrl and interrupt config to the board specific file.
> 
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 +++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index 04888df..e529a41 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -344,6 +344,10 @@
>  };
>  
>  &uart3 {
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-1 = <&qup_uart3_sleep>;
> +	interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
> +				<&tlmm 41 IRQ_TYPE_EDGE_FALLING>;
>  	status = "okay";
>  
>  	bluetooth: wcn3990-bt {
> @@ -545,3 +549,47 @@
>  	};
>  };
>  
> +&tlmm {
> +	qup_uart3_sleep: qup-uart3-sleep {
> +		pinmux {
> +			pins = "gpio38", "gpio39",
> +			       "gpio40", "gpio41";
> +			function = "gpio";
> +		};
> +
> +		pinconf-cts {
> +			/*
> +			 * Configure a pull-down on CTS to match the pull of
> +			 * the Bluetooth module.
> +			 */
> +			pins = "gpio38";
> +			bias-pull-down;
> +		};
> +
> +		pinconf-rts {
> +			/*
> +			 * Configure pull-down on RTS to make sure that the BT SoC can
> +			 * wake up the system by sending wakeup bytes during suspend.

So "request to send" is active low and pulling it low will indicate to
the BT chip that it's allowed to wake us up by pulling rx low?

I would like this comment to really describe what's actually going on.

> +			 */
> +			 pins = "gpio39";
> +			 bias-pull-down;
> +		};
> +
> +		pinconf-tx {
> +			/* Configure pull-up on TX when it isn't actively driven */

Sure, but why? Wouldn't that be to prevent the BT chip from receiving
garbage while the SoC is asleep?

> +			pins = "gpio40";
> +			bias-pull-up;
> +		};
> +
> +		pinconf-rx {
> +			/*
> +			 * Configure a pull-up on RX. This is needed to avoid
> +			 * garbage data when the TX pin of the Bluetooth module is
> +			 * in tri-state (module powered off or not driving the
> +			 * signal yet).
> +			 */

It's nice to avoid "garbage data", but isn't the real reason that the
floating pin on the other side would cause spurious wakeups?

Regards,
Bjorn

> +			pins = "gpio41";
> +			bias-pull-up;
> +		};
> +	};
> +};
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
> of Code Aurora Forum, hosted by The Linux Foundation
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 1/4] arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX
  2020-09-10 14:40   ` Doug Anderson
@ 2020-09-11 10:14     ` skakit
  0 siblings, 0 replies; 19+ messages in thread
From: skakit @ 2020-09-11 10:14 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Bjorn Andersson, Matthias Kaehlcke, Greg Kroah-Hartman,
	Andy Gross, Rob Herring, linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Akash Asthana, Roja Rani Yarubandi, msavaliy

Hi Doug,

On 2020-09-10 20:10, Doug Anderson wrote:
> Hi,
> 
> On Thu, Sep 10, 2020 at 5:55 AM satya priya <skakit@codeaurora.org> 
> wrote:
>> 
>> Remove output-high from CTS and TX as this is not really required. 
>> During
>> bringup to fix transfer failures this was added to match with console 
>> uart
>> settings. Probably some boot loader config was missing then. As it is
>> working fine now, remove it.
>> 
>> Signed-off-by: satya priya <skakit@codeaurora.org>
>> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>> Changes in V4:
>>  - This is newly added in V4 to separate the improvements in pin 
>> settings
>>    and wakeup related changes.
>> 
>> Changes in V5:
>>  - As per Doug's comment configured pull-down for CTS pin as earlier.
>> 
>>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> Looks fine to me.  Slight nit that this only applies to the IDP board
> but ${SUBJECT} makes it sound as if this applies to all sc7180.  I
> wouldn't spin just for that, though.  If Bjorn agrees, he can always
> adjust the subject when applying.
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks for reviewing the patches, i will correct this nit in my next 
version.

Thanks,
Satya Priya

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
  2020-09-10 14:40   ` Doug Anderson
@ 2020-09-11 10:15     ` skakit
  2020-09-11 13:52       ` Bjorn Andersson
  0 siblings, 1 reply; 19+ messages in thread
From: skakit @ 2020-09-11 10:15 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Bjorn Andersson, Matthias Kaehlcke, Greg Kroah-Hartman,
	Andy Gross, Rob Herring, linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Akash Asthana, Roja Rani Yarubandi, msavaliy

On 2020-09-10 20:10, Doug Anderson wrote:
> Hi,
> 
> On Thu, Sep 10, 2020 at 5:55 AM satya priya <skakit@codeaurora.org> 
> wrote:
>> 
>> Add a suitable sleep configuration for uart3 to support Bluetooth 
>> wakeup.
>> 
>> If QUP function is selected in sleep state, UART RTS/RFR is pulled 
>> high
>> during suspend and BT SoC not able to send wakeup bytes. So, configure
>> GPIO mode in sleep state to keep it low during suspend.
>> 
>> Signed-off-by: satya priya <skakit@codeaurora.org>
>> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
>> ---
>> Changes in V2:
>>  - This patch adds sleep state for BT UART. Newly added in V2.
>> 
>> Changes in V3:
>>  - Remove "output-high" for TX from both sleep and default states
>>    as it is not required. Configure pull-up for TX in sleep state.
>> 
>> Changes in V4:
>>  - As per Matthias's comment, removed drive-strength for sleep state
>>    and fixed nit-pick.
>> 
>> Changes in V5:
>>  - As per Matthias's comments, moved pinmux change for sleep state,
>>    pinctrl and interrupt config to the board specific file.
>> 
>>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 
>> +++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
> 
> Similar comment to patch #1 in that this applies only to the IDP board
> but that's not obvious from ${SUBJECT}
> 

Okay.

> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts 
>> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> index 04888df..e529a41 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> @@ -344,6 +344,10 @@
>>  };
>> 
>>  &uart3 {
>> +       pinctrl-names = "default", "sleep";
>> +       pinctrl-1 = <&qup_uart3_sleep>;
>> +       interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
>> +                               <&tlmm 41 IRQ_TYPE_EDGE_FALLING>;
> 
> You need a:
> 
> /delete-property/interrupts;
> 
> ...or, alternatively, a patch before this one that converts all the
> UARTs in sc7180 to just use interrupts-extended.
> 

Sure, I will add this. But I think when both are added, 
"interrupts-extended" will get priority as per [1] and there wouldn't be 
any problem.

[1] 
https://www.kernel.org/doc/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

> 
>>         status = "okay";
> 
> Slight nit is that usually I see the status line first.  All the other
> instances in this file have it that way.  Can you match?
> 

Ok, will correct it.

> 
> 
> 
> -Doug

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 3/4] arm64: dts: qcom: sc7180-trogdor: Add pinctrl and interrupt config for BT UART
  2020-09-10 14:42   ` Doug Anderson
@ 2020-09-11 10:15     ` skakit
  0 siblings, 0 replies; 19+ messages in thread
From: skakit @ 2020-09-11 10:15 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Bjorn Andersson, Matthias Kaehlcke, Greg Kroah-Hartman,
	Andy Gross, Rob Herring, linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Akash Asthana, Roja Rani Yarubandi, msavaliy

On 2020-09-10 20:12, Doug Anderson wrote:
> Hi,
> 
> On Thu, Sep 10, 2020 at 5:55 AM satya priya <skakit@codeaurora.org> 
> wrote:
>> 
>> Add a suitable sleep configuration for uart3 to support Bluetooth 
>> wakeup.
>> 
>> If QUP function is selected in sleep state, UART RTS/RFR is pulled 
>> high
>> during suspend and BT SoC not able to send wakeup bytes. So, configure
>> GPIO mode in sleep state to keep it low during suspend.
>> 
>> Signed-off-by: satya priya <skakit@codeaurora.org>
>> ---
>> Changes in V5:
>>  - Newly added in V5. This patch adds wakeup support for trogdor board 
>> files.
>> 
>>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 48 
>> ++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
> 
> Note: I can't find this email on any of the mailing lists.  Can you
> check your config?  I tried:
> 
> http://lore.kernel.org/r/1599742438-16811-4-git-send-email-skakit@codeaurora.org
> 
> ...and also checked patchwork servers.  I only see patch 1 and 2.  I
> think Bjorn usually applies from patchwork so this'll likely be a
> problem...
> 

I guess it is updated, now it is showing all the 4 patches
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=346637

> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> index a6b9beb..96b5331 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
>> @@ -792,6 +792,11 @@ ap_spi_fp: &spi10 {
>>  #include <arm/cros-ec-sbs.dtsi>
>> 
>>  &uart3 {
>> +       pinctrl-names = "default", "sleep";
>> +       pinctrl-1 = <&qup_uart3_sleep>;
>> +       interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
>> +                               <&tlmm 41 IRQ_TYPE_EDGE_FALLING>;
>> +
>>         status = "okay";
> 
> Same comments here as for patch #2.
> 

Ok.

> 
>>         bluetooth: bluetooth {
>> @@ -1345,4 +1350,47 @@ ap_spi_fp: &spi10 {
>>                         drive-strength = <2>;
>>                 };
>>         };
>> +
>> +       qup_uart3_sleep: qup-uart3-sleep {
> 
> I believe things in this section are supposed to be sorted
> alphabetically.  Thus "qup..." should be sorted before "trackpad..."

Ok.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
  2020-09-10 15:35   ` Matthias Kaehlcke
@ 2020-09-11 10:15     ` skakit
  0 siblings, 0 replies; 19+ messages in thread
From: skakit @ 2020-09-11 10:15 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Bjorn Andersson, gregkh, Andy Gross, Rob Herring, linux-arm-msm,
	devicetree, linux-kernel, akashast, rojay, msavaliy, dianders

Hi Matthias,

On 2020-09-10 21:05, Matthias Kaehlcke wrote:
> El Thu, Sep 10, 2020 at 06:23:56PM +0530 satya priya ha dit:
> 
>> Add a suitable sleep configuration for uart3 to support Bluetooth 
>> wakeup.
>> 
>> If QUP function is selected in sleep state, UART RTS/RFR is pulled 
>> high
>> during suspend and BT SoC not able to send wakeup bytes. So, configure
>> GPIO mode in sleep state to keep it low during suspend.
>> 
>> Signed-off-by: satya priya <skakit@codeaurora.org>
>> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> 
> Was this change really reviewed (privately) by Akash or are you still
> carrying forward this tag from v2? The configuration and the comments
> have change substantially since v2, IMO you should drop the tag unless
> Akash really reviewed the current version or something close to it.
> 

Okay, will drop the tag.

>> ---
>> Changes in V2:
>>  - This patch adds sleep state for BT UART. Newly added in V2.
>> 
>> Changes in V3:
>>  - Remove "output-high" for TX from both sleep and default states
>>    as it is not required. Configure pull-up for TX in sleep state.
>> 
>> Changes in V4:
>>  - As per Matthias's comment, removed drive-strength for sleep state
>>    and fixed nit-pick.
>> 
>> Changes in V5:
>>  - As per Matthias's comments, moved pinmux change for sleep state,
>>    pinctrl and interrupt config to the board specific file.
>> 
>>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 
>> +++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts 
>> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> index 04888df..e529a41 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> @@ -344,6 +344,10 @@
>>  };
>> 
>>  &uart3 {
>> +	pinctrl-names = "default", "sleep";
>> +	pinctrl-1 = <&qup_uart3_sleep>;
>> +	interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
>> +				<&tlmm 41 IRQ_TYPE_EDGE_FALLING>;
> 
> As Doug already said, you need to delete the 'interrupts' property now
> that we have 'interrupts-extended'.

okay.

Thanks,
Satya Priya

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
  2020-09-10 23:45   ` Bjorn Andersson
@ 2020-09-11 10:16     ` skakit
  0 siblings, 0 replies; 19+ messages in thread
From: skakit @ 2020-09-11 10:16 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Matthias Kaehlcke, gregkh, Andy Gross, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, akashast, rojay,
	msavaliy, dianders

Hi Bjorn,

Thanks for reviewing the patches.

On 2020-09-11 05:15, Bjorn Andersson wrote:
> On Thu 10 Sep 12:53 UTC 2020, satya priya wrote:
> 
>> Add a suitable sleep configuration for uart3 to support Bluetooth 
>> wakeup.
>> 
>> If QUP function is selected in sleep state, UART RTS/RFR is pulled 
>> high
>> during suspend and BT SoC not able to send wakeup bytes. So, configure
>> GPIO mode in sleep state to keep it low during suspend.
>> 
> 
> But patch 4 says that you change this behavior, is that patch really
> needed if we switch the pins to GPIO, or if this patch really needed if
> we merge patch 4?
> 
> Could it be that in lower power states we drop the power to the uart
> block and rely on the PDC to wait for the BT chip to start sending the
> wakeup bytes on the rx pin?
> 

As discussed on V4 patch 
https://patchwork.kernel.org/patch/11753971/#23602723
The patch 4 is good to have, to make sure the UART_MANUAL_RFR is in 
ready state to receive the wakeup bytes.

> 
> This commit will become the reference for all other platforms where we
> enable the same functionality, so better document it properly.
> 

Okay.

>> Signed-off-by: satya priya <skakit@codeaurora.org>
>> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
>> ---
>> Changes in V2:
>>  - This patch adds sleep state for BT UART. Newly added in V2.
>> 
>> Changes in V3:
>>  - Remove "output-high" for TX from both sleep and default states
>>    as it is not required. Configure pull-up for TX in sleep state.
>> 
>> Changes in V4:
>>  - As per Matthias's comment, removed drive-strength for sleep state
>>    and fixed nit-pick.
>> 
>> Changes in V5:
>>  - As per Matthias's comments, moved pinmux change for sleep state,
>>    pinctrl and interrupt config to the board specific file.
>> 
>>  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48 
>> +++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts 
>> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> index 04888df..e529a41 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
>> @@ -344,6 +344,10 @@
>>  };
>> 
>>  &uart3 {
>> +	pinctrl-names = "default", "sleep";
>> +	pinctrl-1 = <&qup_uart3_sleep>;
>> +	interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
>> +				<&tlmm 41 IRQ_TYPE_EDGE_FALLING>;
>>  	status = "okay";
>> 
>>  	bluetooth: wcn3990-bt {
>> @@ -545,3 +549,47 @@
>>  	};
>>  };
>> 
>> +&tlmm {
>> +	qup_uart3_sleep: qup-uart3-sleep {
>> +		pinmux {
>> +			pins = "gpio38", "gpio39",
>> +			       "gpio40", "gpio41";
>> +			function = "gpio";
>> +		};
>> +
>> +		pinconf-cts {
>> +			/*
>> +			 * Configure a pull-down on CTS to match the pull of
>> +			 * the Bluetooth module.
>> +			 */
>> +			pins = "gpio38";
>> +			bias-pull-down;
>> +		};
>> +
>> +		pinconf-rts {
>> +			/*
>> +			 * Configure pull-down on RTS to make sure that the BT SoC can
>> +			 * wake up the system by sending wakeup bytes during suspend.
> 
> So "request to send" is active low and pulling it low will indicate to
> the BT chip that it's allowed to wake us up by pulling rx low?
> 
> I would like this comment to really describe what's actually going on.
> 

Ok, will modify the rationale.

>> +			 */
>> +			 pins = "gpio39";
>> +			 bias-pull-down;
>> +		};
>> +
>> +		pinconf-tx {
>> +			/* Configure pull-up on TX when it isn't actively driven */
> 
> Sure, but why? Wouldn't that be to prevent the BT chip from receiving
> garbage while the SoC is asleep?
> 

yes, this is to prevent the BT chip from receiving garbage, will mention 
the same.

>> +			pins = "gpio40";
>> +			bias-pull-up;
>> +		};
>> +
>> +		pinconf-rx {
>> +			/*
>> +			 * Configure a pull-up on RX. This is needed to avoid
>> +			 * garbage data when the TX pin of the Bluetooth module is
>> +			 * in tri-state (module powered off or not driving the
>> +			 * signal yet).
>> +			 */
> 
> It's nice to avoid "garbage data", but isn't the real reason that the
> floating pin on the other side would cause spurious wakeups?
> 

yes, we need pull-up on RX to prevent spurious wakeups, will modify this 
comment to mention it.

> Regards,
> Bjorn
> 
>> +			pins = "gpio41";
>> +			bias-pull-up;
>> +		};
>> +	};
>> +};
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
>> member
>> of Code Aurora Forum, hosted by The Linux Foundation
>> 

Thanks,
Satya Priya

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
  2020-09-11 10:15     ` skakit
@ 2020-09-11 13:52       ` Bjorn Andersson
  2020-09-11 15:30         ` Doug Anderson
  0 siblings, 1 reply; 19+ messages in thread
From: Bjorn Andersson @ 2020-09-11 13:52 UTC (permalink / raw)
  To: skakit
  Cc: Doug Anderson, Matthias Kaehlcke, Greg Kroah-Hartman, Andy Gross,
	Rob Herring, linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Akash Asthana, Roja Rani Yarubandi, msavaliy

On Fri 11 Sep 05:15 CDT 2020, skakit@codeaurora.org wrote:

> On 2020-09-10 20:10, Doug Anderson wrote:
> > Hi,
> > 
> > On Thu, Sep 10, 2020 at 5:55 AM satya priya <skakit@codeaurora.org>
> > wrote:
> > > 
> > > Add a suitable sleep configuration for uart3 to support Bluetooth
> > > wakeup.
> > > 
> > > If QUP function is selected in sleep state, UART RTS/RFR is pulled
> > > high
> > > during suspend and BT SoC not able to send wakeup bytes. So, configure
> > > GPIO mode in sleep state to keep it low during suspend.
> > > 
> > > Signed-off-by: satya priya <skakit@codeaurora.org>
> > > Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> > > ---
> > > Changes in V2:
> > >  - This patch adds sleep state for BT UART. Newly added in V2.
> > > 
> > > Changes in V3:
> > >  - Remove "output-high" for TX from both sleep and default states
> > >    as it is not required. Configure pull-up for TX in sleep state.
> > > 
> > > Changes in V4:
> > >  - As per Matthias's comment, removed drive-strength for sleep state
> > >    and fixed nit-pick.
> > > 
> > > Changes in V5:
> > >  - As per Matthias's comments, moved pinmux change for sleep state,
> > >    pinctrl and interrupt config to the board specific file.
> > > 
> > >  arch/arm64/boot/dts/qcom/sc7180-idp.dts | 48
> > > +++++++++++++++++++++++++++++++++
> > >  1 file changed, 48 insertions(+)
> > 
> > Similar comment to patch #1 in that this applies only to the IDP board
> > but that's not obvious from ${SUBJECT}
> > 
> 
> Okay.
> 
> > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > index 04888df..e529a41 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > @@ -344,6 +344,10 @@
> > >  };
> > > 
> > >  &uart3 {
> > > +       pinctrl-names = "default", "sleep";
> > > +       pinctrl-1 = <&qup_uart3_sleep>;
> > > +       interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
> > > +                               <&tlmm 41 IRQ_TYPE_EDGE_FALLING>;
> > 
> > You need a:
> > 
> > /delete-property/interrupts;
> > 
> > ...or, alternatively, a patch before this one that converts all the
> > UARTs in sc7180 to just use interrupts-extended.
> > 
> 
> Sure, I will add this. But I think when both are added,
> "interrupts-extended" will get priority as per [1] and there wouldn't be any
> problem.
> 

You're indeed correct, please stick with what you have.

Regards,
Bjorn

> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> 
> > 
> > >         status = "okay";
> > 
> > Slight nit is that usually I see the status line first.  All the other
> > instances in this file have it that way.  Can you match?
> > 
> 
> Ok, will correct it.
> 
> > 
> > 
> > 
> > -Doug

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART
  2020-09-11 13:52       ` Bjorn Andersson
@ 2020-09-11 15:30         ` Doug Anderson
  0 siblings, 0 replies; 19+ messages in thread
From: Doug Anderson @ 2020-09-11 15:30 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: satya priya, Matthias Kaehlcke, Greg Kroah-Hartman, Andy Gross,
	Rob Herring, linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Akash Asthana, Roja Rani Yarubandi, msavaliy

Hi,

On Fri, Sep 11, 2020 at 6:52 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> > > >  &uart3 {
> > > > +       pinctrl-names = "default", "sleep";
> > > > +       pinctrl-1 = <&qup_uart3_sleep>;
> > > > +       interrupts-extended = <&intc GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>,
> > > > +                               <&tlmm 41 IRQ_TYPE_EDGE_FALLING>;
> > >
> > > You need a:
> > >
> > > /delete-property/interrupts;
> > >
> > > ...or, alternatively, a patch before this one that converts all the
> > > UARTs in sc7180 to just use interrupts-extended.
> > >
> >
> > Sure, I will add this. But I think when both are added,
> > "interrupts-extended" will get priority as per [1] and there wouldn't be any
> > problem.
> >
>
> You're indeed correct, please stick with what you have.

IMO this is ugly, but I won't fight too hard on it if you guys really
want to do it that way.  Reading the documentation it sounds as if
allowing for both is intended to be used in cases where the same
device tree might be used on old software (that didn't understand
interrupts-extended) and on new software (that does).  The
interrupts-extended property by far predates any sc7180 support,
though, so I can't imagine anyone really needing to use that these
days.  Is it really that bad to add the /delete-property/ to end up
with a cleaner final device tree?

-Doug

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V5 4/4] tty: serial: qcom_geni_serial: Fix the UART wakeup issue
  2020-09-10 23:06   ` Bjorn Andersson
@ 2020-09-16 12:25     ` Greg KH
  0 siblings, 0 replies; 19+ messages in thread
From: Greg KH @ 2020-09-16 12:25 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: satya priya, Matthias Kaehlcke, Andy Gross, Rob Herring,
	linux-arm-msm, devicetree, linux-kernel, akashast, rojay,
	msavaliy, dianders

On Thu, Sep 10, 2020 at 11:06:39PM +0000, Bjorn Andersson wrote:
> On Thu 10 Sep 12:53 UTC 2020, satya priya wrote:
> 
> > As a part of system suspend uart_port_suspend is called from the
> > Serial driver, which calls set_mctrl passing mctrl as 0. This
> > makes RFR high(NOT_READY) during suspend.
> > 
> > Due to this BT SoC is not able to send wakeup bytes to UART during
> > suspend. Include if check for non-suspend case to keep RFR low
> > during suspend.
> > 
> 
> Seems reasonable.
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> > Signed-off-by: satya priya <skakit@codeaurora.org>
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Greg, I don't see this depending on anything else, will you pick this
> patch through your tree? I will take the dts patches through the qcom
> tree.

Sure, will pick it up now, thanks.

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2020-09-16 21:00 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-10 12:53 [PATCH V5 0/4] Add wakeup support over UART RX satya priya
2020-09-10 12:53 ` [PATCH V5 1/4] arm64: dts: qcom: sc7180: Improve the pin config settings for CTS and TX satya priya
2020-09-10 14:40   ` Doug Anderson
2020-09-11 10:14     ` skakit
2020-09-10 12:53 ` [PATCH V5 2/4] arm64: dts: qcom: sc7180: Add necessary pinctrl and interrupt config for BT UART satya priya
2020-09-10 14:40   ` Doug Anderson
2020-09-11 10:15     ` skakit
2020-09-11 13:52       ` Bjorn Andersson
2020-09-11 15:30         ` Doug Anderson
2020-09-10 15:35   ` Matthias Kaehlcke
2020-09-11 10:15     ` skakit
2020-09-10 23:45   ` Bjorn Andersson
2020-09-11 10:16     ` skakit
2020-09-10 12:53 ` [PATCH V5 3/4] arm64: dts: qcom: sc7180-trogdor: Add " satya priya
2020-09-10 14:42   ` Doug Anderson
2020-09-11 10:15     ` skakit
2020-09-10 12:53 ` [PATCH V5 4/4] tty: serial: qcom_geni_serial: Fix the UART wakeup issue satya priya
2020-09-10 23:06   ` Bjorn Andersson
2020-09-16 12:25     ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).