| Message ID | 20220830180054.1998296-3-kumaravel.thiagarajan@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | 8250: microchip: pci1xxxx: Add driver for the pci1xxxx's quad-uart function. | expand |
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/202208310603.ea8ISalW-lkp@intel.com/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 <lkp@intel.com>
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
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 <kumaravel.thiagarajan@microchip.com> > --- > 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.
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Wednesday, August 31, 2022 1:26 AM > To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby > <jirislaby@kernel.org>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>; Uwe > Kleine-König <u.kleine-koenig@pengutronix.de>; Johan Hovold > <johan@kernel.org>; Wander Lairson Costa <wander@redhat.com>; Eric > Tremblay <etremblay@distech-controls.com>; Maciej W. Rozycki > <macro@orcam.me.uk>; Geert Uytterhoeven <geert+renesas@glider.be>; > Jeremy Kerr <jk@ozlabs.org>; Phil Edworthy <phil.edworthy@renesas.com>; > Lukas Wunner <lukas@wunner.de>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; open list:SERIAL DRIVERS <linux- > serial@vger.kernel.org>; UNGLinuxDriver > <UNGLinuxDriver@microchip.com> > 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 > <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 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 <ilpo.jarvinen@linux.intel.com> > Sent: Wednesday, August 31, 2022 3:24 PM > To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby > <jirislaby@kernel.org>; andy.shevchenko@gmail.com; u.kleine- > koenig@pengutronix.de; johan@kernel.org; wander@redhat.com; > etremblay@distech-controls.com; macro@orcam.me.uk; > geert+renesas@glider.be; jk@ozlabs.org; phil.edworthy@renesas.com; > Lukas Wunner <lukas@wunner.de>; LKML <linux-kernel@vger.kernel.org>; > linux-serial <linux-serial@vger.kernel.org>; UNGLinuxDriver > <UNGLinuxDriver@microchip.com> > 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 > > <kumaravel.thiagarajan@microchip.com> > > --- > > 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 > <Kumaravel.Thiagarajan@microchip.com> > Sent: Thursday, September 1, 2022 7:19 PM > To: Andy Shevchenko <andy.shevchenko@gmail.com> > > > -----Original Message----- > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Wednesday, August 31, 2022 1:26 AM > > To: Kumaravel Thiagarajan - I21417 > > <Kumaravel.Thiagarajan@microchip.com> > > > > > > On Tue, Aug 30, 2022 at 9:01 PM 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 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
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, };
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 <kumaravel.thiagarajan@microchip.com> --- drivers/tty/serial/8250/8250_pci1xxxx.c | 122 ++++++++++++++++++++++++ 1 file changed, 122 insertions(+)