[PATCH V3 0/2] tty: serial: imx: improve the imx uart wakeup function

[PATCH V3 0/2] tty: serial: imx: improve the imx uart wakeup function

[Sherry Sun]

Make some improvements for imx uart wakeup function. The first patch adds
device_may_wakeup() check before configuring the wake related registers.
The second patch adds the wakeup event reporting support for imx uart.

Changes in V3:
1. Add !!() to make bool type wake_active clearer.
2. Add Reviewed-by tag for patch #1.

Changes in V2:
1. Improve the commit message as Peng suggested.
2. Initialize the may_wake and wake_active variables to avoid build
   warnings. 
3. Move may_wake and wake_active above u32 ucr3.
4. Use linux/irq.h instead of asm/irq.h to avoid build errors.

Sherry Sun (2):
  tty: serial: imx: Only configure the wake register when device is set
    as wakeup source
  tty: serial: imx: Add missing wakeup event reporting

 drivers/tty/serial/imx.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH V3 1/2] tty: serial: imx: Only configure the wake register when device is set as wakeup source

[Sherry Sun]

Currently, the i.MX UART driver enables wake-related registers for all
UART devices by default. However, this is unnecessary for devices that
are not configured as wakeup sources. To address this, add a
device_may_wakeup() check before configuring the UART wake-related
registers.

Fixes: db1a9b55004c ("tty: serial: imx: Allow UART to be a source for wakeup")
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/tty/serial/imx.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 500dfc009d03..87d841c0b22f 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2697,8 +2697,23 @@ static void imx_uart_save_context(struct imx_port *sport)
 /* called with irq off */
 static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
 {
+	struct tty_port *port = &sport->port.state->port;
+	struct tty_struct *tty;
+	struct device *tty_dev;
+	bool may_wake = false;
 	u32 ucr3;
 
+	tty = tty_port_tty_get(port);
+	if (tty) {
+		tty_dev = tty->dev;
+		may_wake = tty_dev && device_may_wakeup(tty_dev);
+		tty_kref_put(tty);
+	}
+
+	/* only configure the wake register when device set as wakeup source */
+	if (!may_wake)
+		return;
+
 	uart_port_lock_irq(&sport->port);
 
 	ucr3 = imx_uart_readl(sport, UCR3);
-- 
2.34.1

Re: [PATCH V3 1/2] tty: serial: imx: Only configure the wake register when device is set as wakeup source

[Jiri Slaby]

On 25. 09. 25, 11:11, Sherry Sun wrote:
> Currently, the i.MX UART driver enables wake-related registers for all
> UART devices by default. However, this is unnecessary for devices that
> are not configured as wakeup sources. To address this, add a
> device_may_wakeup() check before configuring the UART wake-related
> registers.
> 
> Fixes: db1a9b55004c ("tty: serial: imx: Allow UART to be a source for wakeup")
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>   drivers/tty/serial/imx.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 500dfc009d03..87d841c0b22f 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -2697,8 +2697,23 @@ static void imx_uart_save_context(struct imx_port *sport)
>   /* called with irq off */
>   static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
>   {
> +	struct tty_port *port = &sport->port.state->port;
> +	struct tty_struct *tty;
> +	struct device *tty_dev;
> +	bool may_wake = false;
>   	u32 ucr3;
>   
> +	tty = tty_port_tty_get(port);
> +	if (tty) {

Use scoped_guard(tty_port_tty, port) instead.

> +		tty_dev = tty->dev;
> +		may_wake = tty_dev && device_may_wakeup(tty_dev);
> +		tty_kref_put(tty);
> +	}
> +
> +	/* only configure the wake register when device set as wakeup source */
> +	if (!may_wake)
> +		return;
> +
>   	uart_port_lock_irq(&sport->port);
>   
>   	ucr3 = imx_uart_readl(sport, UCR3);

thanks,
-- 
js
suse labs

RE: [PATCH V3 1/2] tty: serial: imx: Only configure the wake register when device is set as wakeup source

[Sherry Sun]

> -----Original Message-----
> From: Jiri Slaby <jirislaby@kernel.org>
> Sent: Monday, September 29, 2025 1:54 PM
> To: Sherry Sun <sherry.sun@nxp.com>; gregkh@linuxfoundation.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; Shenwei Wang <shenwei.wang@nxp.com>; Peng Fan
> <peng.fan@nxp.com>; Frank Li <frank.li@nxp.com>
> Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org;
> imx@lists.linux.dev
> Subject: Re: [PATCH V3 1/2] tty: serial: imx: Only configure the wake register
> when device is set as wakeup source
> 
> On 25. 09. 25, 11:11, Sherry Sun wrote:
> > Currently, the i.MX UART driver enables wake-related registers for all
> > UART devices by default. However, this is unnecessary for devices that
> > are not configured as wakeup sources. To address this, add a
> > device_may_wakeup() check before configuring the UART wake-related
> > registers.
> >
> > Fixes: db1a9b55004c ("tty: serial: imx: Allow UART to be a source for
> > wakeup")
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >   drivers/tty/serial/imx.c | 15 +++++++++++++++
> >   1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> > 500dfc009d03..87d841c0b22f 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -2697,8 +2697,23 @@ static void imx_uart_save_context(struct
> imx_port *sport)
> >   /* called with irq off */
> >   static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
> >   {
> > +	struct tty_port *port = &sport->port.state->port;
> > +	struct tty_struct *tty;
> > +	struct device *tty_dev;
> > +	bool may_wake = false;
> >   	u32 ucr3;
> >
> > +	tty = tty_port_tty_get(port);
> > +	if (tty) {
> 
> Use scoped_guard(tty_port_tty, port) instead.

Sure, will add this in next version.

Best Regards
Sherry

[PATCH V3 2/2] tty: serial: imx: Add missing wakeup event reporting

[Sherry Sun]

Current imx uart wakeup event would not report itself as wakeup source
through sysfs. Add pm_wakeup_event() to support it.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/tty/serial/imx.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 87d841c0b22f..0eb3f5b8f820 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -30,7 +30,7 @@
 #include <linux/iopoll.h>
 #include <linux/dma-mapping.h>
 
-#include <asm/irq.h>
+#include <linux/irq.h>
 #include <linux/dma/imx-dma.h>
 
 #include "serial_mctrl_gpio.h"
@@ -2700,8 +2700,8 @@ static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
 	struct tty_port *port = &sport->port.state->port;
 	struct tty_struct *tty;
 	struct device *tty_dev;
-	bool may_wake = false;
-	u32 ucr3;
+	bool may_wake = false, wake_active = false;
+	u32 ucr3, usr1;
 
 	tty = tty_port_tty_get(port);
 	if (tty) {
@@ -2716,12 +2716,14 @@ static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
 
 	uart_port_lock_irq(&sport->port);
 
+	usr1 = imx_uart_readl(sport, USR1);
 	ucr3 = imx_uart_readl(sport, UCR3);
 	if (on) {
 		imx_uart_writel(sport, USR1_AWAKE, USR1);
 		ucr3 |= UCR3_AWAKEN;
 	} else {
 		ucr3 &= ~UCR3_AWAKEN;
+		wake_active = usr1 & USR1_AWAKE;
 	}
 	imx_uart_writel(sport, ucr3, UCR3);
 
@@ -2732,10 +2734,14 @@ static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
 			ucr1 |= UCR1_RTSDEN;
 		} else {
 			ucr1 &= ~UCR1_RTSDEN;
+			wake_active |= !!(usr1 & USR1_RTSD);
 		}
 		imx_uart_writel(sport, ucr1, UCR1);
 	}
 
+	if (wake_active && irqd_is_wakeup_set(irq_get_irq_data(sport->port.irq)))
+		pm_wakeup_event(tty_port_tty_get(port)->dev, 0);
+
 	uart_port_unlock_irq(&sport->port);
 }
 
-- 
2.34.1

Re: [PATCH V3 2/2] tty: serial: imx: Add missing wakeup event reporting

[Frank Li]

On Thu, Sep 25, 2025 at 05:11:32PM +0800, Sherry Sun wrote:
> Current imx uart wakeup event would not report itself as wakeup source
> through sysfs. Add pm_wakeup_event() to support it.
>
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> ---
>  drivers/tty/serial/imx.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 87d841c0b22f..0eb3f5b8f820 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -30,7 +30,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/dma-mapping.h>
>
> -#include <asm/irq.h>
> +#include <linux/irq.h>
>  #include <linux/dma/imx-dma.h>
>
>  #include "serial_mctrl_gpio.h"
> @@ -2700,8 +2700,8 @@ static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
>  	struct tty_port *port = &sport->port.state->port;
>  	struct tty_struct *tty;
>  	struct device *tty_dev;
> -	bool may_wake = false;
> -	u32 ucr3;
> +	bool may_wake = false, wake_active = false;
> +	u32 ucr3, usr1;
>
>  	tty = tty_port_tty_get(port);
>  	if (tty) {
> @@ -2716,12 +2716,14 @@ static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
>
>  	uart_port_lock_irq(&sport->port);
>
> +	usr1 = imx_uart_readl(sport, USR1);
>  	ucr3 = imx_uart_readl(sport, UCR3);
>  	if (on) {
>  		imx_uart_writel(sport, USR1_AWAKE, USR1);
>  		ucr3 |= UCR3_AWAKEN;
>  	} else {
>  		ucr3 &= ~UCR3_AWAKEN;
> +		wake_active = usr1 & USR1_AWAKE;
>  	}
>  	imx_uart_writel(sport, ucr3, UCR3);
>
> @@ -2732,10 +2734,14 @@ static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
>  			ucr1 |= UCR1_RTSDEN;
>  		} else {
>  			ucr1 &= ~UCR1_RTSDEN;
> +			wake_active |= !!(usr1 & USR1_RTSD);

I think you miss understand my means. suppose bool type only support
||, &&, ==, !=, !

	wake_active = wake_active || (usr1 & USR1_RTSD);

a |= b actually equal to a = a | b.

bool | u32,  bool convert to u32 then bitwise to u32, algthough it is
allowed, but it is strange, like bool++ is strange.  (true | 0x2 is 0x3)

bool || u32, u32 convert to bool, bool || bool is perfered.
(true || 0x2 is true)

your case the result is the same.

If sparse don't report warning, it should be fine.

Frank
  		}
>  		imx_uart_writel(sport, ucr1, UCR1);
>  	}
>
> +	if (wake_active && irqd_is_wakeup_set(irq_get_irq_data(sport->port.irq)))
> +		pm_wakeup_event(tty_port_tty_get(port)->dev, 0);
> +
>  	uart_port_unlock_irq(&sport->port);
>  }
>
> --
> 2.34.1
>

Re: [PATCH V3 2/2] tty: serial: imx: Add missing wakeup event reporting

[Jiri Slaby]

On 25. 09. 25, 17:49, Frank Li wrote:
>> @@ -2732,10 +2734,14 @@ static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
>>   			ucr1 |= UCR1_RTSDEN;
>>   		} else {
>>   			ucr1 &= ~UCR1_RTSDEN;
>> +			wake_active |= !!(usr1 & USR1_RTSD);
> 
> I think you miss understand my means. suppose bool type only support
> ||, &&, ==, !=, !
> 
> 	wake_active = wake_active || (usr1 & USR1_RTSD);

+1 to this. Much easier to understand (for me at least).

thanks,
-- 
js
suse labs

RE: [PATCH V3 2/2] tty: serial: imx: Add missing wakeup event reporting

[Sherry Sun]

> -----Original Message-----
> From: Jiri Slaby <jirislaby@kernel.org>
> Sent: Monday, September 29, 2025 1:58 PM
> To: Frank Li <frank.li@nxp.com>; Sherry Sun <sherry.sun@nxp.com>
> Cc: gregkh@linuxfoundation.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> Shenwei Wang <shenwei.wang@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org;
> imx@lists.linux.dev
> Subject: Re: [PATCH V3 2/2] tty: serial: imx: Add missing wakeup event
> reporting
> 
> On 25. 09. 25, 17:49, Frank Li wrote:
> >> @@ -2732,10 +2734,14 @@ static void imx_uart_enable_wakeup(struct
> imx_port *sport, bool on)
> >>   			ucr1 |= UCR1_RTSDEN;
> >>   		} else {
> >>   			ucr1 &= ~UCR1_RTSDEN;
> >> +			wake_active |= !!(usr1 & USR1_RTSD);
> >
> > I think you miss understand my means. suppose bool type only support
> > ||, &&, ==, !=, !
> >
> > 	wake_active = wake_active || (usr1 & USR1_RTSD);
> 
> +1 to this. Much easier to understand (for me at least).

Hi Frank and Jiri, thanks for the suggestions, now I got your point, will fix it in next version.

Best Regards
Sherry