pci1xxxx's quad-uart function has the capability to wake up the host from
suspend state. Enable wakeup before entering into suspend and disable
wakeup on resume.

Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
---
Changes in v2:
- Use DEFINE_SIMPLE_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS.
- Use pm_sleep_ptr instead of CONFIG_PM_SLEEP.
- Change the return data type of pci1xxxx_port_suspend to bool from int.
---
 drivers/tty/serial/8250/8250_pci1xxxx.c | 112 ++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
index 999e5a284266..0a0459f66177 100644
--- a/drivers/tty/serial/8250/8250_pci1xxxx.c
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -352,6 +352,112 @@ static void pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
 	}
 }
 
+static bool pci1xxxx_port_suspend(int line)
+{
+	struct uart_8250_port *up = serial8250_get_port(line);
+	struct uart_port *port = &up->port;
+	unsigned long flags;
+	u8 wakeup_mask;
+	bool ret = false;
+
+	if (port->suspended == 0 && port->dev) {
+		wakeup_mask = readb(up->port.membase + UART_WAKE_MASK_REG);
+
+		spin_lock_irqsave(&port->lock, flags);
+		port->mctrl &= ~TIOCM_OUT2;
+		port->ops->set_mctrl(port, port->mctrl);
+		spin_unlock_irqrestore(&port->lock, flags);
+
+		if ((wakeup_mask & UART_WAKE_SRCS) != UART_WAKE_SRCS)
+			ret = true;
+	}
+
+	writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
+
+	return ret;
+}
+
+static void pci1xxxx_port_resume(int line)
+{
+	struct uart_8250_port *up = serial8250_get_port(line);
+	struct uart_port *port = &up->port;
+	unsigned long flags;
+
+	writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
+
+	if (port->suspended == 0) {
+		spin_lock_irqsave(&port->lock, flags);
+		port->mctrl |= TIOCM_OUT2;
+		port->ops->set_mctrl(port, port->mctrl);
+		spin_unlock_irqrestore(&port->lock, flags);
+	}
+}
+
+static int pci1xxxx_suspend(struct device *dev)
+{
+	struct pci1xxxx_8250 *priv = dev_get_drvdata(dev);
+	struct pci_dev *pcidev = to_pci_dev(dev);
+	unsigned int data;
+	void __iomem *p;
+	bool wakeup = false;
+	int i;
+
+	for (i = 0; i < priv->nr; i++) {
+		if (priv->line[i] >= 0) {
+			serial8250_suspend_port(priv->line[i]);
+			wakeup |= pci1xxxx_port_suspend(priv->line[i]);
+		}
+	}
+
+	p = pci_ioremap_bar(pcidev, 0);
+	if (!p) {
+		dev_err(dev, "remapping of bar 0 memory failed");
+		return -ENOMEM;
+	}
+
+	data = readl(p + UART_RESET_REG);
+	writel(data | UART_RESET_D3_RESET_DISABLE, p + UART_RESET_REG);
+
+	if (wakeup)
+		writeb(UART_PCI_CTRL_D3_CLK_ENABLE, (p + UART_PCI_CTRL_REG));
+
+	iounmap(p);
+
+	device_set_wakeup_enable(dev, true);
+
+	pci_wake_from_d3(pcidev, true);
+
+	return 0;
+}
+
+static int pci1xxxx_resume(struct device *dev)
+{
+	struct pci1xxxx_8250 *priv = dev_get_drvdata(dev);
+	struct pci_dev *pcidev = to_pci_dev(dev);
+	unsigned int data;
+	void __iomem *p;
+	int i;
+
+	p = pci_ioremap_bar(pcidev, 0);
+	if (!p) {
+		dev_err(dev, "remapping of bar 0 memory failed");
+		return -ENOMEM;
+	}
+
+	data = readl(p + UART_RESET_REG);
+	writel(data & ~UART_RESET_D3_RESET_DISABLE, p + UART_RESET_REG);
+	iounmap(p);
+
+	for (i = 0; i < priv->nr; i++) {
+		if (priv->line[i] >= 0) {
+			pci1xxxx_port_resume(priv->line[i]);
+			serial8250_resume_port(priv->line[i]);
+		}
+	}
+
+	return 0;
+}
+
 static int pci1xxxx_serial_probe(struct pci_dev *dev,
 				 const struct pci_device_id *ent)
 {
@@ -427,6 +533,9 @@ static void pci1xxxx_serial_remove(struct pci_dev *dev)
 	}
 }
 
+static DEFINE_SIMPLE_DEV_PM_OPS(pci1xxxx_pm_ops, pci1xxxx_suspend,
+				pci1xxxx_resume);
+
 static const struct pci_device_id pci1xxxx_pci_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11010) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_MCHP_PCI1XXXX, PCI_DEVICE_ID_MCHP_PCI11101) },
@@ -441,6 +550,9 @@ static struct pci_driver pci1xxxx_pci_driver = {
 	.name		= "pci1xxxx serial",
 	.probe		= pci1xxxx_serial_probe,
 	.remove		= pci1xxxx_serial_remove,
+	.driver	= {
+		.pm     = pm_sleep_ptr(&pci1xxxx_pm_ops),
+	},
 	.id_table	= pci1xxxx_pci_tbl,
 };
 
-- 
2.25.1

On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:

> pci1xxxx's quad-uart function has the capability to wake up the host from
> suspend state. Enable wakeup before entering into suspend and disable
> wakeup on resume.
> 
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> ---
> Changes in v2:
> - Use DEFINE_SIMPLE_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS.
> - Use pm_sleep_ptr instead of CONFIG_PM_SLEEP.
> - Change the return data type of pci1xxxx_port_suspend to bool from int.
> ---
>  drivers/tty/serial/8250/8250_pci1xxxx.c | 112 ++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 999e5a284266..0a0459f66177 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -352,6 +352,112 @@ static void pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
>  	}
>  }
>  
> +static bool pci1xxxx_port_suspend(int line)
> +{
> +	struct uart_8250_port *up = serial8250_get_port(line);
> +	struct uart_port *port = &up->port;
> +	unsigned long flags;
> +	u8 wakeup_mask;
> +	bool ret = false;
> +
> +	if (port->suspended == 0 && port->dev) {
> +		wakeup_mask = readb(up->port.membase + UART_WAKE_MASK_REG);
> +
> +		spin_lock_irqsave(&port->lock, flags);
> +		port->mctrl &= ~TIOCM_OUT2;
> +		port->ops->set_mctrl(port, port->mctrl);
> +		spin_unlock_irqrestore(&port->lock, flags);
> +
> +		if ((wakeup_mask & UART_WAKE_SRCS) != UART_WAKE_SRCS)
> +			ret = true;
> +	}
> +
> +	writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> +
> +	return ret;
> +}
> +
> +static void pci1xxxx_port_resume(int line)
> +{
> +	struct uart_8250_port *up = serial8250_get_port(line);
> +	struct uart_port *port = &up->port;
> +	unsigned long flags;
> +
> +	writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> +
> +	if (port->suspended == 0) {

Is this check the right way around?

> +		spin_lock_irqsave(&port->lock, flags);
> +		port->mctrl |= TIOCM_OUT2;
> +		port->ops->set_mctrl(port, port->mctrl);
> +		spin_unlock_irqrestore(&port->lock, flags);
> +	}
> +}
> +
> +static int pci1xxxx_suspend(struct device *dev)
> +{
> +	struct pci1xxxx_8250 *priv = dev_get_drvdata(dev);
> +	struct pci_dev *pcidev = to_pci_dev(dev);
> +	unsigned int data;
> +	void __iomem *p;
> +	bool wakeup = false;
> +	int i;
> +
> +	for (i = 0; i < priv->nr; i++) {
> +		if (priv->line[i] >= 0) {
> +			serial8250_suspend_port(priv->line[i]);
> +			wakeup |= pci1xxxx_port_suspend(priv->line[i]);

So first serial8250_suspend_port() calls into uart_suspend_port() that
sets port->suspended to 1, then pci1xxxx_port_suspend() checks if it's 0.
Is this intentional?


-- 
 i.

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Monday, October 3, 2022 3:21 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
>  
> On Sat, 1 Oct 2022, Kumaravel Thiagarajan wrote:
> 
> > pci1xxxx's quad-uart function has the capability to wake up the host
> > from suspend state. Enable wakeup before entering into suspend and
> > disable wakeup on resume.
> >
> > Signed-off-by: Kumaravel Thiagarajan
> > <kumaravel.thiagarajan@microchip.com>
> > ---
> > Changes in v2:
> > - Use DEFINE_SIMPLE_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS.
> > - Use pm_sleep_ptr instead of CONFIG_PM_SLEEP.
> > - Change the return data type of pci1xxxx_port_suspend to bool from int.
> > ---
> >  drivers/tty/serial/8250/8250_pci1xxxx.c | 112
> > ++++++++++++++++++++++++
> >  1 file changed, 112 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > index 999e5a284266..0a0459f66177 100644
> > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > @@ -352,6 +352,112 @@ static void pci1xxxx_irq_assign(struct
> pci1xxxx_8250 *priv,
> >       }
> >  }
> >
> > +static bool pci1xxxx_port_suspend(int line) {
> > +     struct uart_8250_port *up = serial8250_get_port(line);
> > +     struct uart_port *port = &up->port;
> > +     unsigned long flags;
> > +     u8 wakeup_mask;
> > +     bool ret = false;
> > +
> > +     if (port->suspended == 0 && port->dev) {
> > +             wakeup_mask = readb(up->port.membase +
> > + UART_WAKE_MASK_REG);
> > +
> > +             spin_lock_irqsave(&port->lock, flags);
> > +             port->mctrl &= ~TIOCM_OUT2;
> > +             port->ops->set_mctrl(port, port->mctrl);
> > +             spin_unlock_irqrestore(&port->lock, flags);
> > +
> > +             if ((wakeup_mask & UART_WAKE_SRCS) != UART_WAKE_SRCS)
> > +                     ret = true;
> > +     }
> > +
> > +     writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> > +
> > +     return ret;
> > +}
> > +
> > +static void pci1xxxx_port_resume(int line) {
> > +     struct uart_8250_port *up = serial8250_get_port(line);
> > +     struct uart_port *port = &up->port;
> > +     unsigned long flags;
> > +
> > +     writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> > +
> > +     if (port->suspended == 0) {
> 
> Is this check the right way around?
Yes. I think port->suspended is not set for wake-up capable ports and the code in this if block gets executed for those ports.
I will check this again.
> 
> > +             spin_lock_irqsave(&port->lock, flags);
> > +             port->mctrl |= TIOCM_OUT2;
> > +             port->ops->set_mctrl(port, port->mctrl);
> > +             spin_unlock_irqrestore(&port->lock, flags);
> > +     }
> > +}
> > +
> > +static int pci1xxxx_suspend(struct device *dev) {
> > +     struct pci1xxxx_8250 *priv = dev_get_drvdata(dev);
> > +     struct pci_dev *pcidev = to_pci_dev(dev);
> > +     unsigned int data;
> > +     void __iomem *p;
> > +     bool wakeup = false;
> > +     int i;
> > +
> > +     for (i = 0; i < priv->nr; i++) {
> > +             if (priv->line[i] >= 0) {
> > +                     serial8250_suspend_port(priv->line[i]);
> > +                     wakeup |= pci1xxxx_port_suspend(priv->line[i]);
> 
> So first serial8250_suspend_port() calls into uart_suspend_port() that sets
> port->suspended to 1, then pci1xxxx_port_suspend() checks if it's 0.
> Is this intentional?
Yes. I think port->suspended does not seem to be set for wake-up capable ports and only 
for those ports, inside pci1xxxx_port_suspend, TIOCM_OUT2 is cleared.
But I must check for the race condition as Andy had pointed out.
Please let me know if there are any questions.

Thank You.

Regards,
Kumar

On Sat, Oct 1, 2022 at 9:15 AM Kumaravel Thiagarajan
<kumaravel.thiagarajan@microchip.com> wrote:
>
> pci1xxxx's quad-uart function has the capability to wake up the host from
> suspend state. Enable wakeup before entering into suspend and disable
> wakeup on resume.

...

> +       port->suspended == 0

How is this check race-protected?

...

> +static void pci1xxxx_port_resume(int line)
> +{

> +       if (port->suspended == 0) {

Ditto.

> +       }
> +}

...

If you have similarities with 8250_pci, probably you need to split it
to 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in
that sense.)

--
With Best Regards,
Andy Shevchenko

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, October 3, 2022 2:57 PM
> To: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>  
> If you have similarities with 8250_pci, probably you need to split it to
> 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> sense.)

Hi Andy,
All the functions used in 8250_pci1xxxx.c  that have similarity with 8250_pci use registers
that are specific to our IP.  The only function that can be moved to common library is the 
setup_port. But, for that the first argument of setup_port must be changed to 
'struct pci_dev *dev' (priv->dev).  Do you suggest doing this?

Thanks,
Tharun Kumar P

On Fri, Nov 4, 2022 at 12:23 PM <Tharunkumar.Pasumarthi@microchip.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Monday, October 3, 2022 2:57 PM

> > If you have similarities with 8250_pci, probably you need to split it to
> > 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> > sense.)
>
> All the functions used in 8250_pci1xxxx.c  that have similarity with 8250_pci use registers
> that are specific to our IP.  The only function that can be moved to common library is the
> setup_port. But, for that the first argument of setup_port must be changed to
> 'struct pci_dev *dev' (priv->dev).  Do you suggest doing this?

So, you can create a common serial8250_setup_port(struct pci_dev *dev,
...) and call it from the static setup_port() inside 8250_pci.c. This
way you won't introduce too many churn.

-- 
With Best Regards,
Andy Shevchenko

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, October 3, 2022 2:57 PM
> To: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>  
> If you have similarities with 8250_pci, probably you need to split it to
> 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> sense.)

Functions related to suspend and resume use registers specific to our IP which other drivers cannot use. I will move functions like pci1xxxx_set_divisor, pci1xxxx_get_divisor, pci1xxxx_setup_port and pci1xxxx_rs485_config to a new file.

Thanks,
Tharun Kumar P

On Mon, 2022-10-03 at 12:26 +0300, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> > +       port->suspended == 0
> 
> How is this check race-protected?

I will use mutex_lock to handle race condition.


Thanks,
Tharun Kumar P

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, October 3, 2022 2:57 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v2 tty-next 3/3] 8250: microchip: pci1xxxx: Add power
> management functions to quad-uart driver.
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Sat, Oct 1, 2022 at 9:15 AM Kumaravel Thiagarajan
> <kumaravel.thiagarajan@microchip.com> wrote:
> >
> > pci1xxxx's quad-uart function has the capability to wake up the host
> > from suspend state. Enable wakeup before entering into suspend and
> > disable wakeup on resume.
> 
> ...
> 
> > +       port->suspended == 0
> 
> How is this check race-protected?
> 
> ...
> 
> > +static void pci1xxxx_port_resume(int line) {
> 
> > +       if (port->suspended == 0) {
> 
> Ditto.
> 
> > +       }
> > +}
> 
> ...
> 
> If you have similarities with 8250_pci, probably you need to split it to
> 8250_pcilib.c and share. (See how 8250_dw /8250_lpss are done in that
> sense.)
I will review my code against all the above comments and get back to you.

Thank You.

Regards,
Kumar