[PATCH v3 2/2] tty: serial: qcom_geni_serial: Wakeup over UART RX

[PATCH v3 2/2] tty: serial: qcom_geni_serial: Wakeup over UART RX

[Akash Asthana]

Add system wakeup capability over UART RX line for wakeup capable UART.
When system is suspended, RX line act as an interrupt to wakeup system
for any communication requests from peer.

Signed-off-by: Akash Asthana <akashast@codeaurora.org>
---
Changes in V3:
 - Address review comments on v2 patch.

 drivers/tty/serial/qcom_geni_serial.c | 44 ++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 12dc007..bc828e49 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -14,6 +14,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/qcom-geni-se.h>
 #include <linux/serial.h>
 #include <linux/serial_core.h>
@@ -116,6 +117,7 @@ struct qcom_geni_serial_port {
 	bool brk;
 
 	unsigned int tx_remaining;
+	int wakeup_irq;
 };
 
 static const struct uart_ops qcom_geni_console_pops;
@@ -755,6 +757,15 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
 		uart_write_wakeup(uport);
 }
 
+static irqreturn_t qcom_geni_serial_wakeup_isr(int isr, void *dev)
+{
+	struct uart_port *uport = dev;
+
+	pm_wakeup_event(uport->dev, 2000);
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
 {
 	u32 m_irq_en;
@@ -1310,6 +1321,29 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	if (!console) {
+		port->wakeup_irq = platform_get_irq_optional(pdev, 1);
+		if (port->wakeup_irq < 0) {
+			dev_err(&pdev->dev, "Failed to get wakeup IRQ %d\n",
+					port->wakeup_irq);
+		} else {
+			irq_set_status_flags(port->wakeup_irq, IRQ_NOAUTOEN);
+			ret = devm_request_irq(uport->dev, port->wakeup_irq,
+				qcom_geni_serial_wakeup_isr,
+				IRQF_TRIGGER_FALLING, "uart_wakeup", uport);
+			if (ret) {
+				dev_err(uport->dev, "Failed to register wakeup IRQ ret %d\n",
+						ret);
+				return ret;
+			}
+
+			device_init_wakeup(&pdev->dev, true);
+			ret = dev_pm_set_wake_irq(&pdev->dev, port->wakeup_irq);
+			if (unlikely(ret))
+				dev_err(uport->dev, "%s:Failed to set IRQ wake:%d\n",
+						__func__, ret);
+		}
+	}
 	uport->private_data = drv;
 	platform_set_drvdata(pdev, port);
 	port->handle_rx = console ? handle_rx_console : handle_rx_uart;
@@ -1332,7 +1366,12 @@ static int __maybe_unused qcom_geni_serial_sys_suspend(struct device *dev)
 	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
 	struct uart_port *uport = &port->uport;
 
-	return uart_suspend_port(uport->private_data, uport);
+	uart_suspend_port(uport->private_data, uport);
+
+	if (port->wakeup_irq > 0)
+		enable_irq(port->wakeup_irq);
+
+	return 0;
 }
 
 static int __maybe_unused qcom_geni_serial_sys_resume(struct device *dev)
@@ -1340,6 +1379,9 @@ static int __maybe_unused qcom_geni_serial_sys_resume(struct device *dev)
 	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
 	struct uart_port *uport = &port->uport;
 
+	if (port->wakeup_irq > 0)
+		disable_irq(port->wakeup_irq);
+
 	return uart_resume_port(uport->private_data, uport);
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH v3 2/2] tty: serial: qcom_geni_serial: Wakeup over UART RX

[Greg KH]

On Tue, Oct 15, 2019 at 11:41:42AM +0530, Akash Asthana wrote:
> Add system wakeup capability over UART RX line for wakeup capable UART.
> When system is suspended, RX line act as an interrupt to wakeup system
> for any communication requests from peer.
> 
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V3:
>  - Address review comments on v2 patch.

That's horribly vague, please be specific as to what you changed.


> 
>  drivers/tty/serial/qcom_geni_serial.c | 44 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 12dc007..bc828e49 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -14,6 +14,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
>  #include <linux/qcom-geni-se.h>
>  #include <linux/serial.h>
>  #include <linux/serial_core.h>
> @@ -116,6 +117,7 @@ struct qcom_geni_serial_port {
>  	bool brk;
>  
>  	unsigned int tx_remaining;
> +	int wakeup_irq;
>  };
>  
>  static const struct uart_ops qcom_geni_console_pops;
> @@ -755,6 +757,15 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
>  		uart_write_wakeup(uport);
>  }
>  
> +static irqreturn_t qcom_geni_serial_wakeup_isr(int isr, void *dev)
> +{
> +	struct uart_port *uport = dev;
> +
> +	pm_wakeup_event(uport->dev, 2000);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
>  {
>  	u32 m_irq_en;
> @@ -1310,6 +1321,29 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	if (!console) {
> +		port->wakeup_irq = platform_get_irq_optional(pdev, 1);
> +		if (port->wakeup_irq < 0) {
> +			dev_err(&pdev->dev, "Failed to get wakeup IRQ %d\n",
> +					port->wakeup_irq);

Is this error message needed?

> +		} else {
> +			irq_set_status_flags(port->wakeup_irq, IRQ_NOAUTOEN);
> +			ret = devm_request_irq(uport->dev, port->wakeup_irq,
> +				qcom_geni_serial_wakeup_isr,
> +				IRQF_TRIGGER_FALLING, "uart_wakeup", uport);
> +			if (ret) {
> +				dev_err(uport->dev, "Failed to register wakeup IRQ ret %d\n",
> +						ret);

Same here.

> +				return ret;
> +			}
> +
> +			device_init_wakeup(&pdev->dev, true);
> +			ret = dev_pm_set_wake_irq(&pdev->dev, port->wakeup_irq);
> +			if (unlikely(ret))

ONLY ever use likely/unlikely if you can benchmark the difference.
Otherwise don't use it, as you will get it wrong and cpus and compliers
know how to do this much better than we do.

> +				dev_err(uport->dev, "%s:Failed to set IRQ wake:%d\n",
> +						__func__, ret);

Again, is this needed?  And why the function name if it is, the other
error messages you created didn't have that in it.

Consistancy is key.

thanks,

greg k-h

Re: [PATCH v3 2/2] tty: serial: qcom_geni_serial: Wakeup over UART RX

[Akash Asthana]

On 10/16/2019 12:29 AM, Greg KH wrote:
> On Tue, Oct 15, 2019 at 11:41:42AM +0530, Akash Asthana wrote:
>> Add system wakeup capability over UART RX line for wakeup capable UART.
>> When system is suspended, RX line act as an interrupt to wakeup system
>> for any communication requests from peer.
>>
>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>> ---
>> Changes in V3:
>>   - Address review comments on v2 patch.
> That's horribly vague, please be specific as to what you changed.
Ok, I will take care of this in future.
>
>>   drivers/tty/serial/qcom_geni_serial.c | 44 ++++++++++++++++++++++++++++++++++-
>>   1 file changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index 12dc007..bc828e49 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/of.h>
>>   #include <linux/of_device.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/pm_wakeirq.h>
>>   #include <linux/qcom-geni-se.h>
>>   #include <linux/serial.h>
>>   #include <linux/serial_core.h>
>> @@ -116,6 +117,7 @@ struct qcom_geni_serial_port {
>>   	bool brk;
>>   
>>   	unsigned int tx_remaining;
>> +	int wakeup_irq;
>>   };
>>   
>>   static const struct uart_ops qcom_geni_console_pops;
>> @@ -755,6 +757,15 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
>>   		uart_write_wakeup(uport);
>>   }
>>   
>> +static irqreturn_t qcom_geni_serial_wakeup_isr(int isr, void *dev)
>> +{
>> +	struct uart_port *uport = dev;
>> +
>> +	pm_wakeup_event(uport->dev, 2000);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>   static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
>>   {
>>   	u32 m_irq_en;
>> @@ -1310,6 +1321,29 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>   		return ret;
>>   	}
>>   
>> +	if (!console) {
>> +		port->wakeup_irq = platform_get_irq_optional(pdev, 1);
>> +		if (port->wakeup_irq < 0) {
>> +			dev_err(&pdev->dev, "Failed to get wakeup IRQ %d\n",
>> +					port->wakeup_irq);
> Is this error message needed?
I will remove this.
>> +		} else {
>> +			irq_set_status_flags(port->wakeup_irq, IRQ_NOAUTOEN);
>> +			ret = devm_request_irq(uport->dev, port->wakeup_irq,
>> +				qcom_geni_serial_wakeup_isr,
>> +				IRQF_TRIGGER_FALLING, "uart_wakeup", uport);
>> +			if (ret) {
>> +				dev_err(uport->dev, "Failed to register wakeup IRQ ret %d\n",
>> +						ret);
> Same here.
devm_request_irq function doesn't print error for every failure paths.
>
>> +				return ret;
>> +			}
>> +
>> +			device_init_wakeup(&pdev->dev, true);
>> +			ret = dev_pm_set_wake_irq(&pdev->dev, port->wakeup_irq);
>> +			if (unlikely(ret))
> ONLY ever use likely/unlikely if you can benchmark the difference.
> Otherwise don't use it, as you will get it wrong and cpus and compliers
> know how to do this much better than we do.
Ok, I will remove it.
>> +				dev_err(uport->dev, "%s:Failed to set IRQ wake:%d\n",
>> +						__func__, ret);
> Again, is this needed?  And why the function name if it is, the other
> error messages you created didn't have that in it.
  I will remove this.
> Consistancy is key.
>
> thanks,
>
> greg k-h

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project