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

[akashast@codeaurora.org]

On 2019-09-19 23:51, Stephen Boyd wrote:
> Quoting Akash Asthana (2019-09-19 00:16:06)
>> 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.
>> 
>> Cleanup of IRQ registration, moving it to probe from startup function.
> 
> Probably should be a different patch than the one that adds wakeup irq
> handling.
> 

Sure, will do it.

>> 
>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>> ---
>>  drivers/tty/serial/qcom_geni_serial.c | 73 
>> +++++++++++++++++++++++++++++------
>>  1 file changed, 62 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c 
>> b/drivers/tty/serial/qcom_geni_serial.c
>> index 35e5f9c..43d1da4 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -19,6 +19,8 @@
>>  #include <linux/slab.h>
>>  #include <linux/tty.h>
>>  #include <linux/tty_flip.h>
>> +#include <linux/pm_wakeirq.h>
>> +#include <linux/irq.h>
> 
> Can you sort these includes alphabetically? Or at least attempt to 
> place
> this somewhere in there alphabetically?
> 

ok

>> 
>>  /* UART specific GENI registers */
>>  #define SE_UART_LOOPBACK_CFG           0x22c
>> @@ -98,6 +100,8 @@
>>  #define CONSOLE_RX_BYTES_PW 4
>>  #endif
>> 
>> +#define WAKEUP_EVENT_MSEC   2000
> 
> This is used one place. Just inline it and drop this define.
> 

ok

>> +
>>  struct qcom_geni_serial_port {
>>         struct uart_port uport;
>>         struct geni_se se;
>> @@ -115,6 +119,7 @@ struct qcom_geni_serial_port {
>>         bool brk;
>> 
>>         unsigned int tx_remaining;
>> +       int wakeup_irq;
>>  };
>> 
>>  static const struct uart_ops qcom_geni_console_pops;
>> @@ -756,6 +761,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, WAKEUP_EVENT_MSEC);
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>>  static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
>>  {
>>         u32 m_irq_en;
>> @@ -1290,6 +1297,8 @@ 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);
> 
> This looks like an unrelated change?
> 

We moved this change to probe from startup as part of IRQ cleanup. We 
are initializing port->name variable here that is used in 
devm_request_irq call.

>>         irq = platform_get_irq(pdev, 0);
>>         if (irq < 0) {
>>                 dev_err(&pdev->dev, "Failed to get IRQ %d\n", irq);
>> @@ -1297,6 +1306,39 @@ static int qcom_geni_serial_probe(struct 
>> platform_device *pdev)
>>         }
>>         uport->irq = irq;
>> 
>> +       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);
>> +               return ret;
>> +       }
>> +
>> +       if (!console) {
>> +               port->wakeup_irq = platform_get_irq(pdev, 1);
> 
> Can you use dev_pm_set_wake_irq() instead?
> 

okay, we are using this API this will take care of 
enable_irq_wake/disable_irq_wake during suspend and resume but still, we 
need to enable/disable wakeup irq in suspend and resume call.

>> +               if (port->wakeup_irq < 0) {
>> +                       dev_err(&pdev->dev, "Failed to get wakeup IRQ 
>> %d\n",
>> +                                                       
>> port->wakeup_irq);
>> +               } else {
>> +                       dev_info(&pdev->dev, "wakeup_irq =%d\n",
>> +                               port->wakeup_irq);
> 
> Please no dev_info() messages like this.
> 

ok

>> +                       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);
> 
> Don't split format strings across many lines. The arguments can go to a
> different line, but the string can be on a single line.
> 

ok

>> +                               return ret;
>> +                       }
>> +
>> +                       device_init_wakeup(&pdev->dev, true);
>> +                       ret = enable_irq_wake(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;
>> @@ -1311,6 +1353,7 @@ static int qcom_geni_serial_remove(struct 
>> platform_device *pdev)
>>         struct uart_driver *drv = port->uport.private_data;
>> 
>>         uart_remove_one_port(drv, &port->uport);
>> +
>>         return 0;
>>  }
>> 
> 
> This hunk shouldn't be here. Please drop

ok