[v5,tty-next,2/4] 8250: microchip: pci1xxxx: Add serial8250_pci_setup_port definition in 8250_pcilib.c
Commit Message
Move implementation of setup_port API to serial8250_pci_setup_port
Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
---
Changes in v5:
- This is the new patch added in v5 version of this patchset
- Moved implementation of setup_port from 8250_pci.c to 8250_pcilib.c
---
drivers/tty/serial/8250/8250_pci.c | 24 ++--------------
drivers/tty/serial/8250/8250_pci1xxxx.c | 6 ++++
drivers/tty/serial/8250/8250_pcilib.c | 38 +++++++++++++++++++++++++
drivers/tty/serial/8250/8250_pcilib.h | 9 ++++++
drivers/tty/serial/8250/Kconfig | 5 ++++
drivers/tty/serial/8250/Makefile | 1 +
6 files changed, 61 insertions(+), 22 deletions(-)
create mode 100644 drivers/tty/serial/8250/8250_pcilib.c
create mode 100644 drivers/tty/serial/8250/8250_pcilib.h
Comments
On Thu, Nov 17, 2022 at 10:31:24AM +0530, Kumaravel Thiagarajan wrote:
> Move implementation of setup_port API to serial8250_pci_setup_port
Don't you have a dependency issue here?
The subject also wrong. This should be 'serial: 8250_pci: ...'.
And for your code something like 'serial: 8250_pci1xxxx: ...'
in the rest of the series.
On Thu, 17 Nov 2022, Kumaravel Thiagarajan wrote:
> Move implementation of setup_port API to serial8250_pci_setup_port
>
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> ---
> Changes in v5:
> - This is the new patch added in v5 version of this patchset
> - Moved implementation of setup_port from 8250_pci.c to 8250_pcilib.c
>
> ---
> drivers/tty/serial/8250/8250_pci.c | 24 ++--------------
> drivers/tty/serial/8250/8250_pci1xxxx.c | 6 ++++
> drivers/tty/serial/8250/8250_pcilib.c | 38 +++++++++++++++++++++++++
> drivers/tty/serial/8250/8250_pcilib.h | 9 ++++++
> drivers/tty/serial/8250/Kconfig | 5 ++++
> drivers/tty/serial/8250/Makefile | 1 +
> 6 files changed, 61 insertions(+), 22 deletions(-)
> create mode 100644 drivers/tty/serial/8250/8250_pcilib.c
> create mode 100644 drivers/tty/serial/8250/8250_pcilib.h
>
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 6f66dc2ebacc..69ff367b08de 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -24,6 +24,7 @@
> #include <asm/io.h>
>
> #include "8250.h"
> +#include "8250_pcilib.h"
>
> /*
> * init function returns:
> @@ -89,28 +90,7 @@ static int
> setup_port(struct serial_private *priv, struct uart_8250_port *port,
> u8 bar, unsigned int offset, int regshift)
> {
> - struct pci_dev *dev = priv->dev;
> -
> - if (bar >= PCI_STD_NUM_BARS)
> - return -EINVAL;
> -
> - if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
> - if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
> - return -ENOMEM;
> -
> - port->port.iotype = UPIO_MEM;
> - port->port.iobase = 0;
> - port->port.mapbase = pci_resource_start(dev, bar) + offset;
> - port->port.membase = pcim_iomap_table(dev)[bar] + offset;
> - port->port.regshift = regshift;
> - } else {
> - port->port.iotype = UPIO_PORT;
> - port->port.iobase = pci_resource_start(dev, bar) + offset;
> - port->port.mapbase = 0;
> - port->port.membase = NULL;
> - port->port.regshift = 0;
> - }
> - return 0;
> + return serial8250_pci_setup_port(priv->dev, port, bar, offset, regshift);
> }
>
> /*
> diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c
> index 9dd7aca76e58..02b9c6959dcc 100644
> --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -22,6 +22,7 @@
> #include <asm/byteorder.h>
>
> #include "8250.h"
> +#include "8250_pcilib.h"
>
> #define PCI_DEVICE_ID_EFAR_PCI12000 0xa002
> #define PCI_DEVICE_ID_EFAR_PCI11010 0xa012
> @@ -143,6 +144,7 @@ static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
> {
> int first_offset;
> int offset;
> + int ret;
>
> switch (priv->dev->subsystem_device) {
> case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> @@ -191,6 +193,10 @@ static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
> port->port.set_termios = serial8250_do_set_termios;
> port->port.get_divisor = pci1xxxx_get_divisor;
> port->port.set_divisor = pci1xxxx_set_divisor;
> + ret = serial8250_pci_setup_port(priv->dev, port, 0, offset, 0);
> + if (ret < 0)
> + return ret;
> +
> writeb(UART_BLOCK_SET_ACTIVE, port->port.membase + UART_ACTV_REG);
> writeb(UART_WAKE_SRCS, port->port.membase + UART_WAKE_REG);
> writeb(UART_WAKE_N_PIN, port->port.membase + UART_WAKE_MASK_REG);
> diff --git a/drivers/tty/serial/8250/8250_pcilib.c b/drivers/tty/serial/8250/8250_pcilib.c
> new file mode 100644
> index 000000000000..e5a4a9b22c81
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_pcilib.c
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * 8250 PCI library.
> + *
> + * Copyright (C) 2001 Russell King, All Rights Reserved.
> + */
> +#include <linux/errno.h>
> +#include <linux/ioport.h>
> +#include <linux/pci.h>
> +#include <linux/types.h>
> +
> +#include "8250.h"
> +
> +int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port,
> + u8 bar, unsigned int offset, int regshift)
> +{
> + if (bar >= PCI_STD_NUM_BARS)
> + return -EINVAL;
> +
> + if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
> + if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
> + return -ENOMEM;
> +
> + port->port.iotype = UPIO_MEM;
> + port->port.iobase = 0;
> + port->port.mapbase = pci_resource_start(dev, bar) + offset;
> + port->port.membase = pcim_iomap_table(dev)[bar] + offset;
> + port->port.regshift = regshift;
> + } else {
> + port->port.iotype = UPIO_PORT;
> + port->port.iobase = pci_resource_start(dev, bar) + offset;
> + port->port.mapbase = 0;
> + port->port.membase = NULL;
> + port->port.regshift = 0;
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(serial8250_pci_setup_port);
> diff --git a/drivers/tty/serial/8250/8250_pcilib.h b/drivers/tty/serial/8250/8250_pcilib.h
> new file mode 100644
> index 000000000000..41ef01d5c3c5
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_pcilib.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * 8250 PCI library header file.
> + *
> + * Copyright (C) 2001 Russell King, All Rights Reserved.
> + */
> +
> +int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port, u8 bar,
> + unsigned int offset, int regshift);
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 1c41722d8ac5..f67542470eae 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -132,6 +132,7 @@ config SERIAL_8250_DMA
> config SERIAL_8250_PCI
> tristate "8250/16550 PCI device support"
> depends on SERIAL_8250 && PCI
> + select SERIAL_8250_PCILIB
> default SERIAL_8250
> help
> This builds standard PCI serial support. You may be able to
> @@ -294,6 +295,7 @@ config SERIAL_8250_HUB6
> config SERIAL_8250_PCI1XXXX
> tristate "Microchip 8250 based serial port"
> depends on SERIAL_8250_PCI
> + select SERIAL_8250_PCILIB
> default SERIAL_8250
> help
> Select this option if you have a setup with Microchip PCIe
> @@ -510,6 +512,9 @@ config SERIAL_8250_MID
> Intel Medfield SOC and various other Intel platforms that is not
> covered by the more generic SERIAL_8250_PCI option.
>
> +config SERIAL_8250_PCILIB
> + bool
> +
> config SERIAL_8250_PERICOM
> tristate "Support for Pericom and Acces I/O serial ports"
> default SERIAL_8250
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index fbc7d47c25bd..98202fdf39f8 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_SERIAL_8250) += 8250.o 8250_base.o
> 8250_base-$(CONFIG_SERIAL_8250_DMA) += 8250_dma.o
> 8250_base-$(CONFIG_SERIAL_8250_DWLIB) += 8250_dwlib.o
> 8250_base-$(CONFIG_SERIAL_8250_FINTEK) += 8250_fintek.o
> +8250_base-$(CONFIG_SERIAL_8250_PCILIB) += 8250_pcilib.o
> obj-$(CONFIG_SERIAL_8250_GSC) += 8250_gsc.o
> obj-$(CONFIG_SERIAL_8250_PCI) += 8250_pci.o
> obj-$(CONFIG_SERIAL_8250_EXAR) += 8250_exar.o
You should swap patches 1/4 and 2/4 order so that the pcilib is added
first so that you can use it directly when adding your driver.
> Sent: Thursday, November 17, 2022 12:26 PM
> To: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v5 tty-next 2/4] 8250: microchip: pci1xxxx: Add
> serial8250_pci_setup_port definition in 8250_pcilib.c
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Thu, Nov 17, 2022 at 10:31:24AM +0530, Kumaravel Thiagarajan wrote:
> > Move implementation of setup_port API to serial8250_pci_setup_port
>
> Don't you have a dependency issue here?
Okay, I will explain the need for the changes done in commit description.
Thanks,
Tharun Kumar P
On Fri, Nov 18, 2022 at 09:40:37AM +0000, Tharunkumar.Pasumarthi@microchip.com wrote:
> > Sent: Thursday, November 17, 2022 12:26 PM
> > To: Kumaravel Thiagarajan - I21417
> > <Kumaravel.Thiagarajan@microchip.com>
> > Subject: Re: [PATCH v5 tty-next 2/4] 8250: microchip: pci1xxxx: Add
> > serial8250_pci_setup_port definition in 8250_pcilib.c
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > On Thu, Nov 17, 2022 at 10:31:24AM +0530, Kumaravel Thiagarajan wrote:
> > > Move implementation of setup_port API to serial8250_pci_setup_port
> >
> > Don't you have a dependency issue here?
>
> Okay, I will explain the need for the changes done in commit description.
What I meant is that the 8250_pci patch should be _prerequisite_ to your stuff
and not otherwise.
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Friday, November 18, 2022 4:14 PM
> To: Tharunkumar Pasumarthi - I67821
> <Tharunkumar.Pasumarthi@microchip.com>
> Subject: Re: [PATCH v5 tty-next 2/4] 8250: microchip: pci1xxxx: Add
> serial8250_pci_setup_port definition in 8250_pcilib.c
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> > > Don't you have a dependency issue here?
> >
> > Okay, I will explain the need for the changes done in commit description.
>
> What I meant is that the 8250_pci patch should be _prerequisite_ to your
> stuff and not otherwise.
Hi Andy,
So, do you suggest having these changes done as first patch of the patchset prior to patches
specific for our driver?
Thanks,
Tharun Kumar P
On Sat, Nov 19, 2022 at 03:50:02AM +0000, Tharunkumar.Pasumarthi@microchip.com wrote:
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Friday, November 18, 2022 4:14 PM
> > To: Tharunkumar Pasumarthi - I67821 <Tharunkumar.Pasumarthi@microchip.com>
...
> > > > Don't you have a dependency issue here?
> > >
> > > Okay, I will explain the need for the changes done in commit description.
> >
> > What I meant is that the 8250_pci patch should be _prerequisite_ to your
> > stuff and not otherwise.
>
> Hi Andy,
> So, do you suggest having these changes done as first patch of the patchset prior to patches
> specific for our driver?
Yes.
@@ -24,6 +24,7 @@
#include <asm/io.h>
#include "8250.h"
+#include "8250_pcilib.h"
/*
* init function returns:
@@ -89,28 +90,7 @@ static int
setup_port(struct serial_private *priv, struct uart_8250_port *port,
u8 bar, unsigned int offset, int regshift)
{
- struct pci_dev *dev = priv->dev;
-
- if (bar >= PCI_STD_NUM_BARS)
- return -EINVAL;
-
- if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
- if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
- return -ENOMEM;
-
- port->port.iotype = UPIO_MEM;
- port->port.iobase = 0;
- port->port.mapbase = pci_resource_start(dev, bar) + offset;
- port->port.membase = pcim_iomap_table(dev)[bar] + offset;
- port->port.regshift = regshift;
- } else {
- port->port.iotype = UPIO_PORT;
- port->port.iobase = pci_resource_start(dev, bar) + offset;
- port->port.mapbase = 0;
- port->port.membase = NULL;
- port->port.regshift = 0;
- }
- return 0;
+ return serial8250_pci_setup_port(priv->dev, port, bar, offset, regshift);
}
/*
@@ -22,6 +22,7 @@
#include <asm/byteorder.h>
#include "8250.h"
+#include "8250_pcilib.h"
#define PCI_DEVICE_ID_EFAR_PCI12000 0xa002
#define PCI_DEVICE_ID_EFAR_PCI11010 0xa012
@@ -143,6 +144,7 @@ static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
{
int first_offset;
int offset;
+ int ret;
switch (priv->dev->subsystem_device) {
case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
@@ -191,6 +193,10 @@ static int pci1xxxx_setup(struct pci1xxxx_8250 *priv,
port->port.set_termios = serial8250_do_set_termios;
port->port.get_divisor = pci1xxxx_get_divisor;
port->port.set_divisor = pci1xxxx_set_divisor;
+ ret = serial8250_pci_setup_port(priv->dev, port, 0, offset, 0);
+ if (ret < 0)
+ return ret;
+
writeb(UART_BLOCK_SET_ACTIVE, port->port.membase + UART_ACTV_REG);
writeb(UART_WAKE_SRCS, port->port.membase + UART_WAKE_REG);
writeb(UART_WAKE_N_PIN, port->port.membase + UART_WAKE_MASK_REG);
new file mode 100644
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * 8250 PCI library.
+ *
+ * Copyright (C) 2001 Russell King, All Rights Reserved.
+ */
+#include <linux/errno.h>
+#include <linux/ioport.h>
+#include <linux/pci.h>
+#include <linux/types.h>
+
+#include "8250.h"
+
+int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port,
+ u8 bar, unsigned int offset, int regshift)
+{
+ if (bar >= PCI_STD_NUM_BARS)
+ return -EINVAL;
+
+ if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
+ if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
+ return -ENOMEM;
+
+ port->port.iotype = UPIO_MEM;
+ port->port.iobase = 0;
+ port->port.mapbase = pci_resource_start(dev, bar) + offset;
+ port->port.membase = pcim_iomap_table(dev)[bar] + offset;
+ port->port.regshift = regshift;
+ } else {
+ port->port.iotype = UPIO_PORT;
+ port->port.iobase = pci_resource_start(dev, bar) + offset;
+ port->port.mapbase = 0;
+ port->port.membase = NULL;
+ port->port.regshift = 0;
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(serial8250_pci_setup_port);
new file mode 100644
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * 8250 PCI library header file.
+ *
+ * Copyright (C) 2001 Russell King, All Rights Reserved.
+ */
+
+int serial8250_pci_setup_port(struct pci_dev *dev, struct uart_8250_port *port, u8 bar,
+ unsigned int offset, int regshift);
@@ -132,6 +132,7 @@ config SERIAL_8250_DMA
config SERIAL_8250_PCI
tristate "8250/16550 PCI device support"
depends on SERIAL_8250 && PCI
+ select SERIAL_8250_PCILIB
default SERIAL_8250
help
This builds standard PCI serial support. You may be able to
@@ -294,6 +295,7 @@ config SERIAL_8250_HUB6
config SERIAL_8250_PCI1XXXX
tristate "Microchip 8250 based serial port"
depends on SERIAL_8250_PCI
+ select SERIAL_8250_PCILIB
default SERIAL_8250
help
Select this option if you have a setup with Microchip PCIe
@@ -510,6 +512,9 @@ config SERIAL_8250_MID
Intel Medfield SOC and various other Intel platforms that is not
covered by the more generic SERIAL_8250_PCI option.
+config SERIAL_8250_PCILIB
+ bool
+
config SERIAL_8250_PERICOM
tristate "Support for Pericom and Acces I/O serial ports"
default SERIAL_8250
@@ -12,6 +12,7 @@ obj-$(CONFIG_SERIAL_8250) += 8250.o 8250_base.o
8250_base-$(CONFIG_SERIAL_8250_DMA) += 8250_dma.o
8250_base-$(CONFIG_SERIAL_8250_DWLIB) += 8250_dwlib.o
8250_base-$(CONFIG_SERIAL_8250_FINTEK) += 8250_fintek.o
+8250_base-$(CONFIG_SERIAL_8250_PCILIB) += 8250_pcilib.o
obj-$(CONFIG_SERIAL_8250_GSC) += 8250_gsc.o
obj-$(CONFIG_SERIAL_8250_PCI) += 8250_pci.o
obj-$(CONFIG_SERIAL_8250_EXAR) += 8250_exar.o