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 upon resume.
Signed-off-by: Kumaravel Thiagarajan <[email protected]>
---
drivers/tty/serial/8250/8250_pci1xxxx.c | 122 ++++++++++++++++++++++++
1 file changed, 122 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
index 56852ae0585e..38c2a6a9e5d5 100644
--- a/drivers/tty/serial/8250/8250_pci1xxxx.c
+++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
@@ -70,6 +70,7 @@
#define UART_PCI_CTRL_REG 0x80
#define UART_PCI_CTRL_SET_MULTIPLE_MSI BIT(4)
+#define UART_PCI_CTRL_D3_CLK_ENABLE BIT(0)
#define UART_WAKE_REG 0x8C
#define UART_WAKE_MASK_REG 0x90
@@ -78,6 +79,9 @@
#define UART_WAKE_INT BIT(0)
#define UART_WAKE_SRCS (UART_WAKE_N_PIN | UART_WAKE_NCTS | UART_WAKE_INT)
+#define UART_RESET_REG 0x94
+#define UART_RESET_D3_RESET_DISABLE BIT(16)
+
#define UART_BIT_SAMPLE_CNT 16
struct pci1xxxx_8250 {
@@ -439,6 +443,121 @@ static void pci1xxxx_serial_remove(struct pci_dev *dev)
serial8250_unregister_port(priv->line[i]);
}
+#ifdef CONFIG_PM_SLEEP
+
+static char 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;
+ char ret = 0;
+
+ 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 = 0x01;
+ else
+ ret = 0x00;
+ }
+
+ writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
+
+ return ret;
+}
+
+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;
+ char wakeup = 0;
+ 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;
+}
+
+#endif
+
+static 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) },
@@ -453,6 +572,9 @@ static struct pci_driver pci1xxxx_pci_driver = {
.name = "pci1xxxx serial",
.probe = pci1xxxx_serial_probe,
.remove = pci1xxxx_serial_remove,
+ .driver = {
+ .pm = &pci1xxxx_pm_ops,
+ },
.id_table = pci1xxxx_pci_tbl,
};
--
2.25.1
On Tue, Aug 30, 2022 at 9:01 PM Kumaravel Thiagarajan
<[email protected]> 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 upon resume.
on resume
...
First of all, we have pm_ptr() and pm_sleep_ptr() with corresponding
other macros in pm.h. Use them.
Second, if you need to duplicate a lot of code from 8250_pci, split
that code to 8250_pcilib.c and use it in the driver.
--
With Best Regards,
Andy Shevchenko
Hi Kumaravel,
I love your patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on linus/master v6.0-rc3 next-20220830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kumaravel-Thiagarajan/8250-microchip-pci1xxxx-Add-driver-for-the-pci1xxxx-s-quad-uart-function/20220831-020314
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20220831/[email protected]/config)
compiler: mips-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a994946078a1bca8361d4f3245ad306515291b6e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kumaravel-Thiagarajan/8250-microchip-pci1xxxx-Add-driver-for-the-pci1xxxx-s-quad-uart-function/20220831-020314
git checkout a994946078a1bca8361d4f3245ad306515291b6e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/tty/serial/8250/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/tty/serial/8250/8250_pci1xxxx.c: In function 'mchp_pci1xxxx_setup':
drivers/tty/serial/8250/8250_pci1xxxx.c:293:32: error: assignment to 'void (*)(struct uart_port *, struct ktermios *, const struct ktermios *)' from incompatible pointer type 'void (*)(struct uart_port *, struct ktermios *, struct ktermios *)' [-Werror=incompatible-pointer-types]
293 | port->port.set_termios = mchp_pci1xxxx_set_termios;
| ^
drivers/tty/serial/8250/8250_pci1xxxx.c: At top level:
drivers/tty/serial/8250/8250_pci1xxxx.c:305:6: warning: no previous prototype for 'mchp_pci1xxxx_irq_assign' [-Wmissing-prototypes]
305 | void mchp_pci1xxxx_irq_assign(struct pci1xxxx_8250 *priv,
| ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/tty/serial/8250/8250_pci1xxxx.c:475:6: warning: no previous prototype for 'pci1xxxx_port_resume' [-Wmissing-prototypes]
475 | void pci1xxxx_port_resume(int line)
| ^~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/pci1xxxx_port_resume +475 drivers/tty/serial/8250/8250_pci1xxxx.c
474
> 475 void pci1xxxx_port_resume(int line)
476 {
477 struct uart_8250_port *up = serial8250_get_port(line);
478 struct uart_port *port = &up->port;
479 unsigned long flags;
480
481 writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
482
483 if (port->suspended == 0) {
484 spin_lock_irqsave(&port->lock, flags);
485 port->mctrl |= TIOCM_OUT2;
486 port->ops->set_mctrl(port, port->mctrl);
487 spin_unlock_irqrestore(&port->lock, flags);
488 }
489 }
490
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Tue, 30 Aug 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 upon resume.
>
> Signed-off-by: Kumaravel Thiagarajan <[email protected]>
> ---
> drivers/tty/serial/8250/8250_pci1xxxx.c | 122 ++++++++++++++++++++++++
> 1 file changed, 122 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 56852ae0585e..38c2a6a9e5d5 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -70,6 +70,7 @@
>
> #define UART_PCI_CTRL_REG 0x80
> #define UART_PCI_CTRL_SET_MULTIPLE_MSI BIT(4)
> +#define UART_PCI_CTRL_D3_CLK_ENABLE BIT(0)
Reorder.
> +static char pci1xxxx_port_suspend(int line)
Why char???
> +{
> + struct uart_8250_port *up = serial8250_get_port(line);
> + struct uart_port *port = &up->port;
> + unsigned long flags;
> + u8 wakeup_mask;
> + char ret = 0;
> +
> + 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 = 0x01;
> + else
> + ret = 0x00;
Is it a bool or should you name these it with a #define?
> + }
> +
> + writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> +
> + return ret;
> +}
> +
> +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;
> + char wakeup = 0;
> + 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)
It looks you'd want bool instead of char here.
--
i.
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Wednesday, August 31, 2022 1:26 AM
> To: Kumaravel Thiagarajan - I21417 <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Jiri Slaby
> <[email protected]>; Ilpo Järvinen <[email protected]>; Uwe
> Kleine-König <[email protected]>; Johan Hovold
> <[email protected]>; Wander Lairson Costa <[email protected]>; Eric
> Tremblay <[email protected]>; Maciej W. Rozycki
> <[email protected]>; Geert Uytterhoeven <[email protected]>;
> Jeremy Kerr <[email protected]>; Phil Edworthy <[email protected]>;
> Lukas Wunner <[email protected]>; Linux Kernel Mailing List <linux-
> [email protected]>; open list:SERIAL DRIVERS <linux-
> [email protected]>; UNGLinuxDriver
> <[email protected]>
> Subject: Re: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power
> management functions to pci1xxxx's quad-uart driver.
>
>
> On Tue, Aug 30, 2022 at 9:01 PM Kumaravel Thiagarajan
> <[email protected]> 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 upon resume.
>
> on resume
Ok. I will modify this.
>
> ...
>
> First of all, we have pm_ptr() and pm_sleep_ptr() with corresponding other
> macros in pm.h. Use them.
> Second, if you need to duplicate a lot of code from 8250_pci, split that code
> to 8250_pcilib.c and use it in the driver.
Ok. I will review and get back to you.
Thank You.
Regards,
Kumaravel
> -----Original Message-----
> From: Ilpo J?rvinen <[email protected]>
> Sent: Wednesday, August 31, 2022 3:24 PM
> To: Kumaravel Thiagarajan - I21417 <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Jiri Slaby
> <[email protected]>; [email protected]; u.kleine-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Lukas Wunner <[email protected]>; LKML <[email protected]>;
> linux-serial <[email protected]>; UNGLinuxDriver
> <[email protected]>
> Subject: Re: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power
> management functions to pci1xxxx's quad-uart driver.
>
>
> On Tue, 30 Aug 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 upon resume.
> >
> > Signed-off-by: Kumaravel Thiagarajan
> > <[email protected]>
> > ---
> > drivers/tty/serial/8250/8250_pci1xxxx.c | 122
> > ++++++++++++++++++++++++
> > 1 file changed, 122 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > index 56852ae0585e..38c2a6a9e5d5 100644
> > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > @@ -70,6 +70,7 @@
> >
> > #define UART_PCI_CTRL_REG 0x80
> > #define UART_PCI_CTRL_SET_MULTIPLE_MSI BIT(4)
> > +#define UART_PCI_CTRL_D3_CLK_ENABLE BIT(0)
>
> Reorder.
I have ordered this like - Register offset followed by individual bits from MSB to LSB.
Should I reorder this based on some other criteria?
>
> > +static char pci1xxxx_port_suspend(int line)
>
> Why char???
I will modify this to bool.
>
> > +{
> > + struct uart_8250_port *up = serial8250_get_port(line);
> > + struct uart_port *port = &up->port;
> > + unsigned long flags;
> > + u8 wakeup_mask;
> > + char ret = 0;
> > +
> > + 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 = 0x01;
> > + else
> > + ret = 0x00;
>
> Is it a bool or should you name these it with a #define?
bool is the correct data type. I will fix this.
>
> > + }
> > +
> > + writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> > +
> > + return ret;
> > +}
> > +
> > +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;
> > + char wakeup = 0;
> > + 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)
>
> It looks you'd want bool instead of char here.
Yes. I will modify this to bool.
Thank You.
Regards,
Kumaravel
> -----Original Message-----
> From: Kumaravel Thiagarajan - I21417
> <[email protected]>
> Sent: Thursday, September 1, 2022 7:19 PM
> To: Andy Shevchenko <[email protected]>
>
> > -----Original Message-----
> > From: Andy Shevchenko <[email protected]>
> > Sent: Wednesday, August 31, 2022 1:26 AM
> > To: Kumaravel Thiagarajan - I21417
> > <[email protected]>
> >
> >
> > On Tue, Aug 30, 2022 at 9:01 PM Kumaravel Thiagarajan
> > <[email protected]> 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 upon resume.
> >
> > on resume
> Ok. I will modify this.
>
> >
> > ...
> >
> > First of all, we have pm_ptr() and pm_sleep_ptr() with corresponding
> > other macros in pm.h. Use them.
> > Second, if you need to duplicate a lot of code from 8250_pci, split
> > that code to 8250_pcilib.c and use it in the driver.
> Ok. I will review and get back to you.
Andy, I was able to start on the v2 for the patch only a few days ago and about to complete it now.
I have absorbed most of the changes suggested whereas for the above change I felt it may not be
required to split 8250_pci.c at least now as there is only one function setup_port which I have
duplicated in 8250_pci1xxxx.c. Please let me know your thoughts.
I will submit the v2 patch for review soon.
Thank You.
Regards,
Kumar