[PATCH V7 1/2] tty: serial: qcom_geni_serial: Wakeup IRQ cleanup

[PATCH V7 1/2] tty: serial: qcom_geni_serial: Wakeup IRQ cleanup

[Akash Asthana]

This patch is the continuation of below mentioned commits which adds wakeup
feature over the UART RX line.
1)commit 3e4aaea7a039 ("tty: serial: qcom_geni_serial: IRQ cleanup")[v2]
2)commit 8b7103f31950 ("tty: serial: qcom_geni_serial: Wakeup over UART
  RX")[v2]

The following cleanup is done based on upstream comment received on
subsequent versions of the above-mentioned commits to simplifying the code.
 - Use devm_kasprintf API in place of scnprintf.
 - Use dev_pm_set_dedicated_wake_irq API that will take care of
   requesting and attaching wakeup irqs for devices. Also, it sets wakeirq
   status to WAKE_IRQ_DEDICATED_ALLOCATED as a result enabling/disabling of
   wake irq will be managed by suspend/resume framework. We can remove the
   code for enabling and disabling of wake irq from the this driver.
 - Use platform_get_irq_optional API to get optional wakeup IRQ for
   device.
 - Move ISR registration later in probe after uart port gets register with
   serial core.

Patch link:
 - https://patchwork.kernel.org/patch/11189717/ (v3)
 - https://patchwork.kernel.org/patch/11227435/ (v4)
 - https://patchwork.kernel.org/patch/11241669/ (v5)
 - https://patchwork.kernel.org/patch/11258045/ (v6)

Signed-off-by: Akash Asthana <akashast@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 91 +++++++++++++++++------------------
 1 file changed, 43 insertions(+), 48 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index ff63728..55b1d8b 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_runtime.h>
 #include <linux/pm_wakeirq.h>
 #include <linux/qcom-geni-se.h>
 #include <linux/serial.h>
@@ -103,7 +104,7 @@
 struct qcom_geni_serial_port {
 	struct uart_port uport;
 	struct geni_se se;
-	char name[20];
+	const char *name;
 	u32 tx_fifo_depth;
 	u32 tx_fifo_width;
 	u32 rx_fifo_depth;
@@ -757,15 +758,6 @@ 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;
@@ -1302,50 +1294,58 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
 	port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
 
-	scnprintf(port->name, sizeof(port->name), "qcom_geni_serial_%s%d",
-		(uart_console(uport) ? "console" : "uart"), uport->line);
+	port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
+			"qcom_geni_serial_%s%d",
+			uart_console(uport) ? "console" : "uart", uport->line);
+	if (!port->name)
+		return -ENOMEM;
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
 	uport->irq = irq;
 
+	if (!console)
+		port->wakeup_irq = platform_get_irq_optional(pdev, 1);
+
+	uport->private_data = drv;
+	platform_set_drvdata(pdev, port);
+	port->handle_rx = console ? handle_rx_console : handle_rx_uart;
+	if (!console)
+		device_create_file(uport->dev, &dev_attr_loopback);
+
+	ret = uart_add_one_port(drv, uport);
+	if (ret)
+		return ret;
+
 	irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
 	ret = devm_request_irq(uport->dev, uport->irq, qcom_geni_serial_isr,
 			IRQF_TRIGGER_HIGH, port->name, uport);
 	if (ret) {
 		dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
+		uart_remove_one_port(drv, uport);
 		return ret;
 	}
 
-	if (!console) {
-		port->wakeup_irq = platform_get_irq(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);
+	/*
+	 * Set pm_runtime status as ACTIVE so that wakeup_irq gets
+	 * enabled/disabled from dev_pm_arm_wake_irq during system
+	 * suspend/resume respectively.
+	 */
+	pm_runtime_set_active(&pdev->dev);
+
+	if (port->wakeup_irq > 0) {
+		device_init_wakeup(&pdev->dev, true);
+		ret = dev_pm_set_dedicated_wake_irq(&pdev->dev,
+						port->wakeup_irq);
+		if (ret) {
+			device_init_wakeup(&pdev->dev, false);
+			uart_remove_one_port(drv, uport);
+			return ret;
 		}
 	}
-	uport->private_data = drv;
-	platform_set_drvdata(pdev, port);
-	port->handle_rx = console ? handle_rx_console : handle_rx_uart;
-	if (!console)
-		device_create_file(uport->dev, &dev_attr_loopback);
-	return uart_add_one_port(drv, uport);
+
+	return 0;
 }
 
 static int qcom_geni_serial_remove(struct platform_device *pdev)
@@ -1353,7 +1353,10 @@ static int qcom_geni_serial_remove(struct platform_device *pdev)
 	struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
 	struct uart_driver *drv = port->uport.private_data;
 
+	dev_pm_clear_wake_irq(&pdev->dev);
+	device_init_wakeup(&pdev->dev, false);
 	uart_remove_one_port(drv, &port->uport);
+
 	return 0;
 }
 
@@ -1362,12 +1365,7 @@ 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;
 
-	uart_suspend_port(uport->private_data, uport);
-
-	if (port->wakeup_irq > 0)
-		enable_irq(port->wakeup_irq);
-
-	return 0;
+	return uart_suspend_port(uport->private_data, uport);
 }
 
 static int __maybe_unused qcom_geni_serial_sys_resume(struct device *dev)
@@ -1375,9 +1373,6 @@ 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

[PATCH V7 2/2] tty: serial: qcom_geni_serial: Move loopback support to TIOCM_LOOP

[Akash Asthana]

Remove code from the driver that create and maintain loopback sysfs node.
Instead use the ioctl TIOCMSET with TIOCM_LOOP argument to set HW to
loopback mode.

Signed-off-by: Akash Asthana <akashast@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 36 +++++++++--------------------------
 1 file changed, 9 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 55b1d8b..997b262 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -93,7 +93,11 @@
 #define DEF_TX_WM		2
 #define DEF_FIFO_WIDTH_BITS	32
 #define UART_RX_WM		2
-#define MAX_LOOPBACK_CFG	3
+
+/* SE_UART_LOOPBACK_CFG */
+#define RX_TX_SORTED	BIT(0)
+#define CTS_RTS_SORTED	BIT(1)
+#define RX_TX_CTS_RTS_SORTED	(RX_TX_SORTED | CTS_RTS_SORTED)
 
 #ifdef CONFIG_CONSOLE_POLL
 #define CONSOLE_RX_BYTES_PW 1
@@ -165,30 +169,6 @@ static struct qcom_geni_serial_port qcom_geni_uart_ports[GENI_UART_PORTS] = {
 	},
 };
 
-static ssize_t loopback_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
-
-	return snprintf(buf, sizeof(u32), "%d\n", port->loopback);
-}
-
-static ssize_t loopback_store(struct device *dev,
-				struct device_attribute *attr, const char *buf,
-				size_t size)
-{
-	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
-	u32 loopback;
-
-	if (kstrtoint(buf, 0, &loopback) || loopback > MAX_LOOPBACK_CFG) {
-		dev_err(dev, "Invalid input\n");
-		return -EINVAL;
-	}
-	port->loopback = loopback;
-	return size;
-}
-static DEVICE_ATTR_RW(loopback);
-
 static struct qcom_geni_serial_port qcom_geni_console_port = {
 	.uport = {
 		.iotype = UPIO_MEM,
@@ -238,10 +218,14 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport,
 							unsigned int mctrl)
 {
 	u32 uart_manual_rfr = 0;
+	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
 
 	if (uart_console(uport))
 		return;
 
+	if (mctrl & TIOCM_LOOP)
+		port->loopback = RX_TX_CTS_RTS_SORTED;
+
 	if (!(mctrl & TIOCM_RTS))
 		uart_manual_rfr = UART_MANUAL_RFR_EN | UART_RFR_NOT_READY;
 	writel(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
@@ -1311,8 +1295,6 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	uport->private_data = drv;
 	platform_set_drvdata(pdev, port);
 	port->handle_rx = console ? handle_rx_console : handle_rx_uart;
-	if (!console)
-		device_create_file(uport->dev, &dev_attr_loopback);
 
 	ret = uart_add_one_port(drv, uport);
 	if (ret)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V7 1/2] tty: serial: qcom_geni_serial: Wakeup IRQ cleanup

[Stephen Boyd]

Quoting Akash Asthana (2019-11-25 07:08:50)
> This patch is the continuation of below mentioned commits which adds wakeup
> feature over the UART RX line.
> 1)commit 3e4aaea7a039 ("tty: serial: qcom_geni_serial: IRQ cleanup")[v2]
> 2)commit 8b7103f31950 ("tty: serial: qcom_geni_serial: Wakeup over UART
>   RX")[v2]
> 
> The following cleanup is done based on upstream comment received on
> subsequent versions of the above-mentioned commits to simplifying the code.
>  - Use devm_kasprintf API in place of scnprintf.
>  - Use dev_pm_set_dedicated_wake_irq API that will take care of
>    requesting and attaching wakeup irqs for devices. Also, it sets wakeirq
>    status to WAKE_IRQ_DEDICATED_ALLOCATED as a result enabling/disabling of
>    wake irq will be managed by suspend/resume framework. We can remove the
>    code for enabling and disabling of wake irq from the this driver.
>  - Use platform_get_irq_optional API to get optional wakeup IRQ for
>    device.
>  - Move ISR registration later in probe after uart port gets register with
>    serial core.
> 
> Patch link:
>  - https://patchwork.kernel.org/patch/11189717/ (v3)
>  - https://patchwork.kernel.org/patch/11227435/ (v4)
>  - https://patchwork.kernel.org/patch/11241669/ (v5)
>  - https://patchwork.kernel.org/patch/11258045/ (v6)
> 
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Ok sure.

> ---
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index ff63728..55b1d8b 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1302,50 +1294,58 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>         port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>         port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>  
> -       scnprintf(port->name, sizeof(port->name), "qcom_geni_serial_%s%d",
> -               (uart_console(uport) ? "console" : "uart"), uport->line);
> +       port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
> +                       "qcom_geni_serial_%s%d",
> +                       uart_console(uport) ? "console" : "uart", uport->line);
> +       if (!port->name)
> +               return -ENOMEM;
> +
>         irq = platform_get_irq(pdev, 0);
>         if (irq < 0)
>                 return irq;
>         uport->irq = irq;
>  
> +       if (!console)
> +               port->wakeup_irq = platform_get_irq_optional(pdev, 1);

Is there a DT binding update for this? It would be nice if the GENI SE
binding was updated to by YAML.

> +
> +       uport->private_data = drv;
> +       platform_set_drvdata(pdev, port);
> +       port->handle_rx = console ? handle_rx_console : handle_rx_uart;
> +       if (!console)
> +               device_create_file(uport->dev, &dev_attr_loopback);
> +

Re: [PATCH V7 1/2] tty: serial: qcom_geni_serial: Wakeup IRQ cleanup

[Akash Asthana]

On 11/25/2019 9:36 PM, Stephen Boyd wrote:
> Quoting Akash Asthana (2019-11-25 07:08:50)
>> This patch is the continuation of below mentioned commits which adds wakeup
>> feature over the UART RX line.
>> 1)commit 3e4aaea7a039 ("tty: serial: qcom_geni_serial: IRQ cleanup")[v2]
>> 2)commit 8b7103f31950 ("tty: serial: qcom_geni_serial: Wakeup over UART
>>    RX")[v2]
>>
>> The following cleanup is done based on upstream comment received on
>> subsequent versions of the above-mentioned commits to simplifying the code.
>>   - Use devm_kasprintf API in place of scnprintf.
>>   - Use dev_pm_set_dedicated_wake_irq API that will take care of
>>     requesting and attaching wakeup irqs for devices. Also, it sets wakeirq
>>     status to WAKE_IRQ_DEDICATED_ALLOCATED as a result enabling/disabling of
>>     wake irq will be managed by suspend/resume framework. We can remove the
>>     code for enabling and disabling of wake irq from the this driver.
>>   - Use platform_get_irq_optional API to get optional wakeup IRQ for
>>     device.
>>   - Move ISR registration later in probe after uart port gets register with
>>     serial core.
>>
>> Patch link:
>>   - https://patchwork.kernel.org/patch/11189717/ (v3)
>>   - https://patchwork.kernel.org/patch/11227435/ (v4)
>>   - https://patchwork.kernel.org/patch/11241669/ (v5)
>>   - https://patchwork.kernel.org/patch/11258045/ (v6)
>>
>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Ok sure.
>
>> ---
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index ff63728..55b1d8b 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -1302,50 +1294,58 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>          port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>>          port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>>   
>> -       scnprintf(port->name, sizeof(port->name), "qcom_geni_serial_%s%d",
>> -               (uart_console(uport) ? "console" : "uart"), uport->line);
>> +       port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
>> +                       "qcom_geni_serial_%s%d",
>> +                       uart_console(uport) ? "console" : "uart", uport->line);
>> +       if (!port->name)
>> +               return -ENOMEM;
>> +
>>          irq = platform_get_irq(pdev, 0);
>>          if (irq < 0)
>>                  return irq;
>>          uport->irq = irq;
>>   
>> +       if (!console)
>> +               port->wakeup_irq = platform_get_irq_optional(pdev, 1);
> Is there a DT binding update for this? It would be nice if the GENI SE
> binding was updated to by YAML.
Yes, there is DT binding update for this. Ok I will update GENI SE 
binding to YAML
>> +
>> +       uport->private_data = drv;
>> +       platform_set_drvdata(pdev, port);
>> +       port->handle_rx = console ? handle_rx_console : handle_rx_uart;
>> +       if (!console)
>> +               device_create_file(uport->dev, &dev_attr_loopback);
>> +

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