[PATCH v4 0/7] serial: exar: add Connect Tech serial cards to Exar driver

[PATCH v4 0/7] serial: exar: add Connect Tech serial cards to Exar driver

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

Hello,
These patches add proper support for most of Connect Tech's (CTI) Exar
based serial cards. Previously, only a subset of CTI's cards would work
with the Exar driver while the rest required the CTI out-of-tree driver.
These patches are intended to phase out the out-of-tree driver.

I am new to the mailing lists and contributing to the kernel so please
let me know if I have made any mistakes or if you have any feedback.

Changes in v2:
- Put missing PCI IDs in 8250_exar.c instead of pci_ids.h
- Split large patch into smaller ones

Changes in v3:
- Refactored patches to be easier to follow (based on feedback of v2)
- Patch specific changes listed in corresponding patch

Changes in v4:
- Rebased to tty-testing branch
- Removed v3 patch 8/8, "bug" didn't happen in current driver
- Patch specific changes listed in corresponding patch

Thank you,

Parker Newman (7):
  serial: exar: remove old Connect Tech setup
  serial: exar: added a exar_get_nr_ports function
  serial: exar: add optional board_init function
  serial: exar: moved generic_rs485 further up in 8250_exar.c
  serial: exar: add CTI cards to exar_get_nr_ports
  serial: exar: add CTI specific setup code
  serial: exar: fix checkpach warnings

 drivers/tty/serial/8250/8250_exar.c | 981 ++++++++++++++++++++++++++--
 1 file changed, 916 insertions(+), 65 deletions(-)


base-commit: b86ae40ffcf5a16b9569b1016da4a08c4f352ca2
--
2.43.2

[PATCH v4 1/7] serial: exar: remove old Connect Tech setup

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

Preparatory patch removing existing Connect Tech setup code and
CONNECT_DEVICE macro.

Subsequent patches in this series will add in new UART family specific
setup code and new device definition macros to allow for supporting CTI
serial cards with Exar PCI IDs and CTI PCI IDs.

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
Changes in v3:
- Split code removals to own patch

 drivers/tty/serial/8250/8250_exar.c | 37 -----------------------------
 1 file changed, 37 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 604e5a292d4e..04ce5e8ddb24 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -358,17 +358,6 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 	return 0;
 }

-static int
-pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
-		       struct uart_8250_port *port, int idx)
-{
-	unsigned int offset = idx * 0x200;
-	unsigned int baud = 1843200;
-
-	port->port.uartclk = baud * 16;
-	return default_setup(priv, pcidev, idx, offset, port);
-}
-
 static int
 pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 		   struct uart_8250_port *port, int idx)
@@ -849,10 +838,6 @@ static const struct exar8250_board pbn_fastcom335_8 = {
 	.setup		= pci_fastcom335_setup,
 };

-static const struct exar8250_board pbn_connect = {
-	.setup		= pci_connect_tech_setup,
-};
-
 static const struct exar8250_board pbn_exar_ibm_saturn = {
 	.num_ports	= 1,
 	.setup		= pci_xr17c154_setup,
@@ -897,15 +882,6 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
 	.exit		= pci_xr17v35x_exit,
 };

-#define CONNECT_DEVICE(devid, sdevid, bd) {				\
-	PCI_DEVICE_SUB(							\
-		PCI_VENDOR_ID_EXAR,					\
-		PCI_DEVICE_ID_EXAR_##devid,				\
-		PCI_SUBVENDOR_ID_CONNECT_TECH,				\
-		PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_##sdevid), 0, 0,	\
-		(kernel_ulong_t)&bd					\
-	}
-
 #define EXAR_DEVICE(vend, devid, bd) { PCI_DEVICE_DATA(vend, devid, &bd) }

 #define IBM_DEVICE(devid, sdevid, bd) {			\
@@ -935,19 +911,6 @@ static const struct pci_device_id exar_pci_tbl[] = {
 	EXAR_DEVICE(ACCESSIO, COM_4SM, pbn_exar_XR17C15x),
 	EXAR_DEVICE(ACCESSIO, COM_8SM, pbn_exar_XR17C15x),

-	CONNECT_DEVICE(XR17C152, UART_2_232, pbn_connect),
-	CONNECT_DEVICE(XR17C154, UART_4_232, pbn_connect),
-	CONNECT_DEVICE(XR17C158, UART_8_232, pbn_connect),
-	CONNECT_DEVICE(XR17C152, UART_1_1, pbn_connect),
-	CONNECT_DEVICE(XR17C154, UART_2_2, pbn_connect),
-	CONNECT_DEVICE(XR17C158, UART_4_4, pbn_connect),
-	CONNECT_DEVICE(XR17C152, UART_2, pbn_connect),
-	CONNECT_DEVICE(XR17C154, UART_4, pbn_connect),
-	CONNECT_DEVICE(XR17C158, UART_8, pbn_connect),
-	CONNECT_DEVICE(XR17C152, UART_2_485, pbn_connect),
-	CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect),
-	CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect),
-
 	IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),

 	/* USRobotics USR298x-OEM PCI Modems */
--
2.43.2

[PATCH v4 2/7] serial: exar: added a exar_get_nr_ports function

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

Moved code for getting number of ports from exar_pci_probe() to a
separate exar_get_nr_ports() function. CTI specific code will be added
in another patch in this series.

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
Changes in v3:
- Only moved existing code in this patch, will add CTI code in subsequent
  patch

 drivers/tty/serial/8250/8250_exar.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 04ce5e8ddb24..72385c7d2eda 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -704,6 +704,21 @@ static irqreturn_t exar_misc_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }

+static unsigned int exar_get_nr_ports(struct exar8250_board *board,
+					struct pci_dev *pcidev)
+{
+	unsigned int nr_ports = 0;
+
+	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
+		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
+	else if (board->num_ports)
+		nr_ports = board->num_ports;
+	else
+		nr_ports = pcidev->device & 0x0f;
+
+	return nr_ports;
+}
+
 static int
 exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 {
@@ -723,12 +738,12 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)

 	maxnr = pci_resource_len(pcidev, bar) >> (board->reg_shift + 3);

-	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
-		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
-	else if (board->num_ports)
-		nr_ports = board->num_ports;
-	else
-		nr_ports = pcidev->device & 0x0f;
+	nr_ports = exar_get_nr_ports(board, pcidev);
+	if (nr_ports == 0) {
+		dev_err_probe(&pcidev->dev, -ENODEV,
+				"failed to get number of ports\n");
+		return -ENODEV;
+	}

 	priv = devm_kzalloc(&pcidev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
 	if (!priv)
--
2.43.2

[PATCH v4 3/7] serial: exar: add optional board_init function

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

Add an optional "board_init()" function pointer to struct exar8250_board
which is called once during probe prior to setting up the ports. It will
be used in subsequent patches of this series.

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
Changes in v3:
 - Renamed board_setup to board_init.
 - Changed pci_err to dev_err_probe
 - Added note above about checkpatch fixes

Changes in v4:
 - Removed checkpatch fixes, they will be in their own patch at the end
 - Added pcidev to board_init() args to avoid needing to add to priv

 drivers/tty/serial/8250/8250_exar.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 72385c7d2eda..f14f73d250bb 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -177,12 +177,14 @@ struct exar8250_platform {
  * struct exar8250_board - board information
  * @num_ports: number of serial ports
  * @reg_shift: describes UART register mapping in PCI memory
- * @setup: quirk run at ->probe() stage
+ * @board_init: quirk run once at ->probe() stage before setting up ports
+ * @setup: quirk run at ->probe() stage for each port
  * @exit: quirk run at ->remove() stage
  */
 struct exar8250_board {
 	unsigned int num_ports;
 	unsigned int reg_shift;
+	int     (*board_init)(struct exar8250 *priv, struct pci_dev *pcidev);
 	int	(*setup)(struct exar8250 *, struct pci_dev *,
 			 struct uart_8250_port *, int);
 	void	(*exit)(struct pci_dev *pcidev);
@@ -773,6 +775,15 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;

+	if (board->board_init) {
+		rc = board->board_init(priv, pcidev);
+		if (rc) {
+			dev_err_probe(&pcidev->dev, rc,
+					"failed to init serial board\n");
+			return rc;
+		}
+	}
+
 	for (i = 0; i < nr_ports && i < maxnr; i++) {
 		rc = board->setup(priv, pcidev, &uart, i);
 		if (rc) {
--
2.43.2

[PATCH v4 4/7] serial: exar: moved generic_rs485 further up in 8250_exar.c

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

Preparatory patch moving generic_rs485_config and
generic_rs485_supported higher in the file to allow for CTI setup
functions to use them.

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
Changes in v3:
 - split into separate preparatory patch

 drivers/tty/serial/8250/8250_exar.c | 50 ++++++++++++++---------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index f14f73d250bb..e68029a59122 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -197,6 +197,31 @@ struct exar8250 {
 	int			line[];
 };

+static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
+				struct serial_rs485 *rs485)
+{
+	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
+	u8 __iomem *p = port->membase;
+	u8 value;
+
+	value = readb(p + UART_EXAR_FCTR);
+	if (is_rs485)
+		value |= UART_FCTR_EXAR_485;
+	else
+		value &= ~UART_FCTR_EXAR_485;
+
+	writeb(value, p + UART_EXAR_FCTR);
+
+	if (is_rs485)
+		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
+
+	return 0;
+}
+
+static const struct serial_rs485 generic_rs485_supported = {
+	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
+};
+
 static void exar_pm(struct uart_port *port, unsigned int state, unsigned int old)
 {
 	/*
@@ -459,27 +484,6 @@ static void xr17v35x_unregister_gpio(struct uart_8250_port *port)
 	port->port.private_data = NULL;
 }

-static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
-				struct serial_rs485 *rs485)
-{
-	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
-	u8 __iomem *p = port->membase;
-	u8 value;
-
-	value = readb(p + UART_EXAR_FCTR);
-	if (is_rs485)
-		value |= UART_FCTR_EXAR_485;
-	else
-		value &= ~UART_FCTR_EXAR_485;
-
-	writeb(value, p + UART_EXAR_FCTR);
-
-	if (is_rs485)
-		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
-
-	return 0;
-}
-
 static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
 				  struct serial_rs485 *rs485)
 {
@@ -518,10 +522,6 @@ static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termio
 	return 0;
 }

-static const struct serial_rs485 generic_rs485_supported = {
-	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
-};
-
 static const struct exar8250_platform exar8250_default_platform = {
 	.register_gpio = xr17v35x_register_gpio,
 	.unregister_gpio = xr17v35x_unregister_gpio,
--
2.43.2

[PATCH v4 5/7] serial: exar: add CTI cards to exar_get_nr_ports

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

Add code for getting number of ports of CTI cards to
exar_get_nr_ports().

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
Changes in v3:
- moved to separate patch
- added spaces to single line comments

 drivers/tty/serial/8250/8250_exar.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index e68029a59122..197f45e306ff 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -711,12 +711,28 @@ static unsigned int exar_get_nr_ports(struct exar8250_board *board,
 {
 	unsigned int nr_ports = 0;

-	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
+	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO) {
 		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
-	else if (board->num_ports)
+	} else if (board->num_ports > 0) {
+		// Check if board struct overrides number of ports
 		nr_ports = board->num_ports;
-	else
+	} else if (pcidev->vendor == PCI_VENDOR_ID_EXAR) {
+		// Exar encodes # ports in last nibble of PCI Device ID ex. 0358
 		nr_ports = pcidev->device & 0x0f;
+	} else  if (pcidev->vendor == PCI_VENDOR_ID_CONNECT_TECH) {
+		// Handle CTI FPGA cards
+		switch (pcidev->device) {
+		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
+		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
+			nr_ports = 12;
+			break;
+		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
+			nr_ports = 16;
+			break;
+		default:
+			break;
+		}
+	}

 	return nr_ports;
 }
--
2.43.2

[PATCH v4 6/7] serial: exar: add CTI specific setup code

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

This is a large patch but is only additions. All changes and removals
are made in previous patches in this series.

- Add CTI board_init and port setup functions for each UART type
- Add CTI_EXAR_DEVICE() and CTI_PCI_DEVICE() macros
- Add support for reading a word from the Exar EEPROM.
- Add support for configuring and setting a single MPIO
- Add various helper functions for CTI boards.
- Add osc_freq to struct exar8250

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
Changes in v3:
- Moved all base driver changes and refactoring to preparatory patches
- Switched any user space types to kernel types
- Switched all uses of pci_xxx print functions to dev_xxx
- Added struct device pointer in struct exar8250 to simplify above
- Switched osc_freq and port_flag parsing to use GENMASK() and
  FIELD_GET()/FIELD_PREP()
- Renamed board_setup function pointer to board_init
- Removed some unneeded checks for priv being NULL
- Added various convenience functions instead of relying on bools ex:
  exar_mpio_set_low()/exar_mpio_set_high() instead of exar_mpio_set()
- Renamed some variables and defines for clarity
- Numerous minor formatting fixes

Changes in v4:
- Removed pcidev and dev from struct exar8250
- Removed some debug prints
- Removed some unneeded checks if PCI vendor was Exar
- Changed several functions to take pcidev as arg to avoid adding to priv
- Removed _exar_mpio_config(), only needed exar_mpio_config_output()
- Removed _cti_set_tristate() and _cti_set_plx_int_enable, same as above

 drivers/tty/serial/8250/8250_exar.c | 846 ++++++++++++++++++++++++++++
 1 file changed, 846 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 197f45e306ff..6985aabe13cc 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -20,6 +20,7 @@
 #include <linux/property.h>
 #include <linux/string.h>
 #include <linux/types.h>
+#include <linux/bitfield.h>

 #include <linux/serial_8250.h>
 #include <linux/serial_core.h>
@@ -128,6 +129,19 @@
 #define UART_EXAR_DLD			0x02 /* Divisor Fractional */
 #define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polarity */

+/* EEPROM registers */
+#define UART_EXAR_REGB			0x8e
+#define UART_EXAR_REGB_EECK		BIT(4)
+#define UART_EXAR_REGB_EECS		BIT(5)
+#define UART_EXAR_REGB_EEDI		BIT(6)
+#define UART_EXAR_REGB_EEDO		BIT(7)
+#define UART_EXAR_REGB_EE_ADDR_SIZE	6
+#define UART_EXAR_REGB_EE_DATA_SIZE	16
+
+#define UART_EXAR_XR17C15X_PORT_OFFSET	0x200
+#define UART_EXAR_XR17V25X_PORT_OFFSET	0x200
+#define UART_EXAR_XR17V35X_PORT_OFFSET	0x400
+
 /*
  * IOT2040 MPIO wiring semantics:
  *
@@ -163,6 +177,52 @@
 #define IOT2040_UARTS_ENABLE		0x03
 #define IOT2040_UARTS_GPIO_HI_MODE	0xF8	/* enable & LED as outputs */

+/* CTI EEPROM offsets */
+#define CTI_EE_OFF_XR17C15X_OSC_FREQ	0x04  /* 2 words */
+#define CTI_EE_OFF_XR17V25X_OSC_FREQ	0x08  /* 2 words */
+#define CTI_EE_OFF_XR17C15X_PART_NUM	0x0A  /* 4 words */
+#define CTI_EE_OFF_XR17V25X_PART_NUM	0x0E  /* 4 words */
+#define CTI_EE_OFF_XR17C15X_SERIAL_NUM	0x0E  /* 1 word */
+#define CTI_EE_OFF_XR17V25X_SERIAL_NUM	0x12  /* 1 word */
+#define CTI_EE_OFF_XR17V35X_SERIAL_NUM	0x11  /* 2 word */
+#define CTI_EE_OFF_XR17V35X_BRD_FLAGS	0x13  /* 1 word */
+#define CTI_EE_OFF_XR17V35X_PORT_FLAGS	0x14  /* 1 word */
+
+#define CTI_EE_MASK_PORT_FLAGS_TYPE	GENMASK(7, 0)
+#define CTI_EE_MASK_OSC_FREQ_LOWER	GENMASK(15, 0)
+#define CTI_EE_MASK_OSC_FREQ_UPPER	GENMASK(31, 16)
+
+#define CTI_FPGA_RS485_IO_REG		0x2008
+#define CTI_FPGA_CFG_INT_EN_REG		0x48
+#define CTI_FPGA_CFG_INT_EN_EXT_BIT	BIT(15) /* External int enable bit */
+
+#define CTI_DEFAULT_PCI_OSC_FREQ	29491200
+#define CTI_DEFAULT_PCIE_OSC_FREQ	125000000
+#define CTI_DEFAULT_FPGA_OSC_FREQ	33333333
+
+/*
+ * CTI Serial port line types. These match the values stored in the first
+ * nibble of the CTI EEPROM port_flags word.
+ */
+enum cti_port_type {
+	CTI_PORT_TYPE_NONE = 0,
+	CTI_PORT_TYPE_RS232,            // RS232 ONLY
+	CTI_PORT_TYPE_RS422_485,        // RS422/RS485 ONLY
+	CTI_PORT_TYPE_RS232_422_485_HW, // RS232/422/485 HW ONLY Switchable
+	CTI_PORT_TYPE_RS232_422_485_SW, // RS232/422/485 SW ONLY Switchable
+	CTI_PORT_TYPE_RS232_422_485_4B, // RS232/422/485 HW/SW (4bit ex. BCG004)
+	CTI_PORT_TYPE_RS232_422_485_2B, // RS232/422/485 HW/SW (2bit ex. BBG008)
+	CTI_PORT_TYPE_MAX,
+};
+
+#define CTI_PORT_TYPE_VALID(_port_type) \
+	(((_port_type) > CTI_PORT_TYPE_NONE) && \
+	((_port_type) < CTI_PORT_TYPE_MAX))
+
+#define CTI_PORT_TYPE_RS485(_port_type) \
+	(((_port_type) > CTI_PORT_TYPE_RS232) && \
+	((_port_type) < CTI_PORT_TYPE_MAX))
+
 struct exar8250;

 struct exar8250_platform {
@@ -192,11 +252,201 @@ struct exar8250_board {

 struct exar8250 {
 	unsigned int		nr;
+	unsigned int		osc_freq;
 	struct exar8250_board	*board;
 	void __iomem		*virt;
 	int			line[];
 };

+static inline void exar_write_reg(struct exar8250 *priv,
+				unsigned int reg, u8 value)
+{
+	writeb(value, priv->virt + reg);
+}
+
+static inline u8 exar_read_reg(struct exar8250 *priv, unsigned int reg)
+{
+	return readb(priv->virt + reg);
+}
+
+static inline void exar_ee_select(struct exar8250 *priv)
+{
+	// Set chip select pin high to enable EEPROM reads/writes
+	exar_write_reg(priv, UART_EXAR_REGB, UART_EXAR_REGB_EECS);
+	// Min ~500ns delay needed between CS assert and EEPROM access
+	udelay(1);
+}
+
+static inline void exar_ee_deselect(struct exar8250 *priv)
+{
+	exar_write_reg(priv, UART_EXAR_REGB, 0x00);
+}
+
+static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
+{
+	u8 value = UART_EXAR_REGB_EECS;
+
+	if (bit)
+		value |= UART_EXAR_REGB_EEDI;
+
+	// Clock out the bit on the EEPROM interface
+	exar_write_reg(priv, UART_EXAR_REGB, value);
+	// 2us delay = ~500khz clock speed
+	udelay(2);
+
+	value |= UART_EXAR_REGB_EECK;
+
+	exar_write_reg(priv, UART_EXAR_REGB, value);
+	udelay(2);
+}
+
+static inline u8 exar_ee_read_bit(struct exar8250 *priv)
+{
+	u8 regb;
+	u8 value = UART_EXAR_REGB_EECS;
+
+	// Clock in the bit on the EEPROM interface
+	exar_write_reg(priv, UART_EXAR_REGB, value);
+	// 2us delay = ~500khz clock speed
+	udelay(2);
+
+	value |= UART_EXAR_REGB_EECK;
+
+	exar_write_reg(priv, UART_EXAR_REGB, value);
+	udelay(2);
+
+	regb = exar_read_reg(priv, UART_EXAR_REGB);
+
+	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);
+}
+
+/**
+ * exar_ee_read() - Read a word from the EEPROM
+ * @priv: Device's private structure
+ * @ee_addr: Offset of EEPROM to read word from
+ *
+ * Read a single 16bit word from an Exar UART's EEPROM.
+ *
+ * Return: EEPROM word
+ */
+static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr)
+{
+	int i;
+	u16 data = 0;
+
+	exar_ee_select(priv);
+
+	// Send read command (opcode 110)
+	exar_ee_write_bit(priv, 1);
+	exar_ee_write_bit(priv, 1);
+	exar_ee_write_bit(priv, 0);
+
+	// Send address to read from
+	for (i = 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>= 1)
+		exar_ee_write_bit(priv, (ee_addr & i));
+
+	// Read data 1 bit at a time
+	for (i = 0; i <= UART_EXAR_REGB_EE_DATA_SIZE; i++) {
+		data <<= 1;
+		data |= exar_ee_read_bit(priv);
+	}
+
+	exar_ee_deselect(priv);
+
+	return data;
+}
+
+/**
+ * exar_mpio_config_output() - Configure an Exar MPIO as an output
+ * @priv: Device's private structure
+ * @mpio_num: MPIO number/offset to configure
+ *
+ * Configure a single MPIO as an output and disable tristate. It is reccomended
+ * to set the level with exar_mpio_set_high()/exar_mpio_set_low() prior to
+ * calling this function to ensure default MPIO pin state.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int exar_mpio_config_output(struct exar8250 *priv,
+				unsigned int mpio_num)
+{
+	unsigned int mpio_offset;
+	u8 sel_reg; // MPIO Select register (input/output)
+	u8 tri_reg; // MPIO Tristate register
+	u8 value;
+
+	if (mpio_num < 8) {
+		sel_reg = UART_EXAR_MPIOSEL_7_0;
+		tri_reg = UART_EXAR_MPIO3T_7_0;
+		mpio_offset = mpio_num;
+	} else if (mpio_num >= 8 && mpio_num < 16) {
+		sel_reg = UART_EXAR_MPIOSEL_15_8;
+		tri_reg = UART_EXAR_MPIO3T_15_8;
+		mpio_offset = mpio_num - 8;
+	} else {
+		return -EINVAL;
+	}
+
+	// Disable MPIO pin tri-state
+	value = exar_read_reg(priv, tri_reg);
+	value &= ~BIT(mpio_offset);
+	exar_write_reg(priv, tri_reg, value);
+
+	value = exar_read_reg(priv, sel_reg);
+	value &= ~BIT(mpio_offset);
+	exar_write_reg(priv, sel_reg, value);
+
+	return 0;
+}
+
+/**
+ * _exar_mpio_set() - Set an Exar MPIO output high or low
+ * @priv: Device's private structure
+ * @mpio_num: MPIO number/offset to set
+ * @high: Set MPIO high if true, low if false
+ *
+ * Set a single MPIO high or low. exar_mpio_config_output() must also be called
+ * to configure the pin as an output.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int _exar_mpio_set(struct exar8250 *priv,
+			unsigned int mpio_num, bool high)
+{
+	unsigned int mpio_offset;
+	u8 lvl_reg;
+	u8 value;
+
+	if (mpio_num < 8) {
+		lvl_reg = UART_EXAR_MPIOLVL_7_0;
+		mpio_offset = mpio_num;
+	} else if (mpio_num >= 8 && mpio_num < 16) {
+		lvl_reg = UART_EXAR_MPIOLVL_15_8;
+		mpio_offset = mpio_num - 8;
+	} else {
+		return -EINVAL;
+	}
+
+	value = exar_read_reg(priv, lvl_reg);
+	if (high)
+		value |= BIT(mpio_offset);
+	else
+		value &= ~BIT(mpio_offset);
+	exar_write_reg(priv, lvl_reg, value);
+
+	return 0;
+}
+
+static int exar_mpio_set_low(struct exar8250 *priv, unsigned int mpio_num)
+{
+	return _exar_mpio_set(priv, mpio_num, false);
+}
+
+static int exar_mpio_set_high(struct exar8250 *priv, unsigned int mpio_num)
+{
+	return _exar_mpio_set(priv, mpio_num, true);
+}
+
 static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
 				struct serial_rs485 *rs485)
 {
@@ -385,6 +635,546 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 	return 0;
 }

+/**
+ * cti_tristate_disable() - Disable RS485 transciever tristate
+ * @priv: Device's private structure
+ * @port_num: Port number to set tristate off
+ *
+ * Most RS485 capable cards have a power on tristate jumper/switch that ensures
+ * the RS422/RS485 transciever does not drive a multi-drop RS485 bus when it is
+ * not the master. When this jumper is installed the user must set the RS485
+ * mode to Full or Half duplex to disable tristate prior to using the port.
+ *
+ * Some Exar UARTs have an auto-tristate feature while others require setting
+ * an MPIO to disable the tristate.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int cti_tristate_disable(struct exar8250 *priv, unsigned int port_num)
+{
+	int ret;
+
+	ret = exar_mpio_set_high(priv, port_num);
+	if (ret)
+		return ret;
+
+	return exar_mpio_config_output(priv, port_num);
+}
+
+/**
+ * cti_plx_int_enable() - Enable UART interrupts to PLX bridge
+ * @priv: Device's private structure
+ *
+ * Some older CTI cards require MPIO_0 to be set low to enable the
+ * interupts from the UART to the PLX PCI->PCIe bridge.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int cti_plx_int_enable(struct exar8250 *priv)
+{
+	int ret;
+
+	ret = exar_mpio_set_low(priv, 0);
+	if (ret)
+		return ret;
+
+	return exar_mpio_config_output(priv, 0);
+}
+
+/**
+ * cti_read_osc_freq() - Read the UART oscillator frequency from EEPROM
+ * @priv: Device's private structure
+ * @eeprom_offset: Offset where the oscillator frequency is stored
+ *
+ * CTI XR17x15X and XR17V25X cards have the serial boards oscillator frequency
+ * stored in the EEPROM. FPGA and XR17V35X based cards use the PCI/PCIe clock.
+ *
+ * Return: frequency on success, negative error code on failure
+ */
+static int cti_read_osc_freq(struct exar8250 *priv, u8 eeprom_offset)
+{
+	u16 lower_word;
+	u16 upper_word;
+	int osc_freq;
+
+	lower_word = exar_ee_read(priv, eeprom_offset);
+	// Check if EEPROM word was blank
+	if (lower_word == 0xFFFF)
+		return -EIO;
+
+	upper_word = exar_ee_read(priv, (eeprom_offset + 1));
+	if (upper_word == 0xFFFF)
+		return -EIO;
+
+	osc_freq = FIELD_PREP(CTI_EE_MASK_OSC_FREQ_LOWER, lower_word) |
+		FIELD_PREP(CTI_EE_MASK_OSC_FREQ_UPPER, upper_word);
+
+	return osc_freq;
+}
+
+/**
+ * cti_get_port_type_xr17c15x_xr17v25x() - Get port type of xr17c15x/xr17v25x
+ * @priv: Device's private structure
+ * @port_num: Port to get type of
+ *
+ * CTI xr17c15x and xr17v25x based cards port types are based on PCI IDs.
+ *
+ * Return: port type on success, CTI_PORT_TYPE_NONE on failure
+ */
+static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *priv,
+							struct pci_dev *pcidev,
+							unsigned int port_num)
+{
+	enum cti_port_type port_type;
+
+	switch (pcidev->subsystem_device) {
+	// RS232 only cards
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_232:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_232:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_232:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232_NS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232_NS:
+		port_type = CTI_PORT_TYPE_RS232;
+		break;
+	// 1x RS232, 1x RS422/RS485
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
+		port_type = (port_num == 0) ?
+			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		break;
+	// 2x RS232, 2x RS422/RS485
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
+		port_type = (port_num < 2) ?
+			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		break;
+	// 4x RS232, 4x RS422/RS485
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
+		port_type = (port_num < 4) ?
+			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		break;
+	// RS232/RS422/RS485 HW (jumper) selectable
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_SP_OPTO:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
+		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
+		break;
+	// RS422/RS485 HW (jumper) selectable
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_485:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_485:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_485:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
+		port_type = CTI_PORT_TYPE_RS422_485;
+		break;
+	// 6x RS232, 2x RS422/RS485
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
+		port_type = (port_num < 6) ?
+			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		break;
+	// 2x RS232, 6x RS422/RS485
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
+		port_type = (port_num < 2) ?
+			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
+		break;
+	default:
+		dev_err(&pcidev->dev, "unknown/unsupported device\n");
+		port_type = CTI_PORT_TYPE_NONE;
+	}
+
+	return port_type;
+}
+
+/**
+ * cti_get_port_type_fpga() - Get the port type of a CTI FPGA card
+ * @priv: Device's private structure
+ * @port_num: Port to get type of
+ *
+ * FPGA based cards port types are based on PCI IDs.
+ *
+ * Return: port type on success, CTI_PORT_TYPE_NONE on failure
+ */
+static enum cti_port_type cti_get_port_type_fpga(struct exar8250 *priv,
+						struct pci_dev *pcidev,
+						unsigned int port_num)
+{
+	enum cti_port_type port_type;
+
+	switch (pcidev->device) {
+	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
+	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
+	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
+		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
+		break;
+	default:
+		dev_err(&pcidev->dev, "unknown/unsupported device\n");
+		return CTI_PORT_TYPE_NONE;
+	}
+
+	return port_type;
+}
+
+/**
+ * cti_get_port_type_xr17v35x() - Read port type from the EEPROM
+ * @priv: Device's private structure
+ * @port_num: port offset
+ *
+ * CTI XR17V35X based cards have the port types stored in the EEPROM.
+ * This function reads the port type for a single port.
+ *
+ * Return: port type on success, CTI_PORT_TYPE_NONE on failure
+ */
+static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
+						struct pci_dev *pcidev,
+						unsigned int port_num)
+{
+	enum cti_port_type port_type;
+	u16 port_flags;
+	u8 offset;
+
+	offset = CTI_EE_OFF_XR17V35X_PORT_FLAGS + port_num;
+	port_flags = exar_ee_read(priv, offset);
+
+	port_type = FIELD_GET(CTI_EE_MASK_PORT_FLAGS_TYPE, port_flags);
+	if (!CTI_PORT_TYPE_VALID(port_type)) {
+		/*
+		 * If the port type is missing the card assume it is a
+		 * RS232/RS422/RS485 card to be safe.
+		 *
+		 * There is one known board (BEG013) that only has
+		 * 3 of 4 port types written to the EEPROM so this
+		 * acts as a work around.
+		 */
+		dev_warn(&pcidev->dev,
+			"failed to get port %d type from EEPROM\n", port_num);
+		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
+	}
+
+	return port_type;
+}
+
+static int cti_rs485_config_mpio_tristate(struct uart_port *port,
+					struct ktermios *termios,
+					struct serial_rs485 *rs485)
+{
+	struct exar8250 *priv = (struct exar8250 *)port->private_data;
+	int ret;
+
+	ret = generic_rs485_config(port, termios, rs485);
+	if (ret)
+		return ret;
+
+	// Disable power-on RS485 tri-state via MPIO
+	return cti_tristate_disable(priv, port->port_id);
+}
+
+static int cti_port_setup_common(struct exar8250 *priv,
+				struct pci_dev *pcidev,
+				int idx, unsigned int offset,
+				struct uart_8250_port *port)
+{
+	int ret;
+
+	if (priv->osc_freq == 0)
+		return -EINVAL;
+
+	port->port.port_id = idx;
+	port->port.uartclk = priv->osc_freq;
+
+	ret = serial8250_pci_setup_port(pcidev, port, 0, offset, 0);
+	if (ret) {
+		dev_err(&pcidev->dev,
+			"failed to setup pci for port %d err: %d\n", idx, ret);
+		return ret;
+	}
+
+	port->port.private_data = (void *)priv;
+	port->port.pm = exar_pm;
+	port->port.shutdown = exar_shutdown;
+
+	return 0;
+}
+
+static int cti_port_setup_fpga(struct exar8250 *priv,
+				struct pci_dev *pcidev,
+				struct uart_8250_port *port,
+				int idx)
+{
+	enum cti_port_type port_type;
+	unsigned int offset;
+
+	port_type = cti_get_port_type_fpga(priv, pcidev, idx);
+
+	// FPGA shares port offests with XR17C15X
+	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
+	port->port.type = PORT_XR17D15X;
+
+	port->port.get_divisor = xr17v35x_get_divisor;
+	port->port.set_divisor = xr17v35x_set_divisor;
+	port->port.startup = xr17v35x_startup;
+
+	if (CTI_PORT_TYPE_RS485(port_type)) {
+		port->port.rs485_config = generic_rs485_config;
+		port->port.rs485_supported = generic_rs485_supported;
+	}
+
+	return cti_port_setup_common(priv, pcidev, idx, offset, port);
+}
+
+static int cti_port_setup_xr17v35x(struct exar8250 *priv,
+				struct pci_dev *pcidev,
+				struct uart_8250_port *port,
+				int idx)
+{
+	enum cti_port_type port_type;
+	unsigned int offset;
+	int ret;
+
+	port_type = cti_get_port_type_xr17v35x(priv, pcidev, idx);
+
+	offset = idx * UART_EXAR_XR17V35X_PORT_OFFSET;
+	port->port.type = PORT_XR17V35X;
+
+	port->port.get_divisor = xr17v35x_get_divisor;
+	port->port.set_divisor = xr17v35x_set_divisor;
+	port->port.startup = xr17v35x_startup;
+
+	switch (port_type) {
+	case CTI_PORT_TYPE_RS422_485:
+	case CTI_PORT_TYPE_RS232_422_485_HW:
+		port->port.rs485_config = cti_rs485_config_mpio_tristate;
+		port->port.rs485_supported = generic_rs485_supported;
+		break;
+	case CTI_PORT_TYPE_RS232_422_485_SW:
+	case CTI_PORT_TYPE_RS232_422_485_4B:
+	case CTI_PORT_TYPE_RS232_422_485_2B:
+		port->port.rs485_config = generic_rs485_config;
+		port->port.rs485_supported = generic_rs485_supported;
+		break;
+	default:
+		break;
+	}
+
+	ret = cti_port_setup_common(priv, pcidev, idx, offset, port);
+	if (ret)
+		return ret;
+
+	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
+	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
+	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 128);
+	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 128);
+
+	return 0;
+}
+
+static int cti_port_setup_xr17v25x(struct exar8250 *priv,
+				struct pci_dev *pcidev,
+				struct uart_8250_port *port,
+				int idx)
+{
+	enum cti_port_type port_type;
+	unsigned int offset;
+	int ret;
+
+	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, pcidev, idx);
+
+	offset = idx * UART_EXAR_XR17V25X_PORT_OFFSET;
+	port->port.type = PORT_XR17D15X;
+
+	// XR17V25X supports fractional baudrates
+	port->port.get_divisor = xr17v35x_get_divisor;
+	port->port.set_divisor = xr17v35x_set_divisor;
+	port->port.startup = xr17v35x_startup;
+
+	if (CTI_PORT_TYPE_RS485(port_type)) {
+		switch (pcidev->subsystem_device) {
+		// These cards support power on 485 tri-state via MPIO
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
+			port->port.rs485_config = cti_rs485_config_mpio_tristate;
+			break;
+		// Otherwise auto or no power on 485 tri-state support
+		default:
+			port->port.rs485_config = generic_rs485_config;
+			break;
+		}
+
+		port->port.rs485_supported = generic_rs485_supported;
+	}
+
+	ret = cti_port_setup_common(priv, pcidev, idx, offset, port);
+	if (ret)
+		return ret;
+
+	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
+	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
+	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 32);
+	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 32);
+
+	return 0;
+}
+
+static int cti_port_setup_xr17c15x(struct exar8250 *priv,
+				struct pci_dev *pcidev,
+				struct uart_8250_port *port,
+				int idx)
+{
+	enum cti_port_type port_type;
+	unsigned int offset;
+
+	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, pcidev, idx);
+
+	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
+	port->port.type = PORT_XR17D15X;
+
+	if (CTI_PORT_TYPE_RS485(port_type)) {
+		switch (pcidev->subsystem_device) {
+		// These cards support power on 485 tri-state via MPIO
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
+		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
+			port->port.rs485_config = cti_rs485_config_mpio_tristate;
+			break;
+		// Otherwise auto or no power on 485 tri-state support
+		default:
+			port->port.rs485_config = generic_rs485_config;
+			break;
+		}
+
+		port->port.rs485_supported = generic_rs485_supported;
+	}
+
+	return cti_port_setup_common(priv, pcidev, idx, offset, port);
+}
+
+static int cti_board_init_xr17v35x(struct exar8250 *priv,
+				struct pci_dev *pcidev)
+{
+	// XR17V35X uses the PCIe clock rather than an oscillator
+	priv->osc_freq = CTI_DEFAULT_PCIE_OSC_FREQ;
+
+	return 0;
+}
+
+static int cti_board_init_xr17v25x(struct exar8250 *priv,
+				struct pci_dev *pcidev)
+{
+	int osc_freq;
+
+	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17V25X_OSC_FREQ);
+	if (osc_freq < 0) {
+		dev_warn(&pcidev->dev,
+			"failed to read osc freq from EEPROM, using default\n");
+		osc_freq = CTI_DEFAULT_PCI_OSC_FREQ;
+	}
+
+	priv->osc_freq = osc_freq;
+
+	/* enable interupts on cards that need the "PLX fix" */
+	switch (pcidev->subsystem_device) {
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
+		cti_plx_int_enable(priv);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int cti_board_init_xr17c15x(struct exar8250 *priv,
+				struct pci_dev *pcidev)
+{
+	int osc_freq;
+
+	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17C15X_OSC_FREQ);
+	if (osc_freq <= 0) {
+		dev_warn(&pcidev->dev,
+			"failed to read osc freq from EEPROM, using default\n");
+		osc_freq = CTI_DEFAULT_PCI_OSC_FREQ;
+	}
+
+	priv->osc_freq = osc_freq;
+
+	/* enable interrupts on cards that need the "PLX fix" */
+	switch (pcidev->subsystem_device) {
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
+	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
+		cti_plx_int_enable(priv);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int cti_board_init_fpga(struct exar8250 *priv, struct pci_dev *pcidev)
+{
+	int ret;
+	u16 cfg_val;
+
+	// FPGA OSC is fixed to the 33MHz PCI clock
+	priv->osc_freq = CTI_DEFAULT_FPGA_OSC_FREQ;
+
+	// Enable external interrupts in special cfg space register
+	ret = pci_read_config_word(pcidev, CTI_FPGA_CFG_INT_EN_REG, &cfg_val);
+	if (ret)
+		return ret;
+
+	cfg_val |= CTI_FPGA_CFG_INT_EN_EXT_BIT;
+	ret = pci_write_config_word(pcidev, CTI_FPGA_CFG_INT_EN_REG, cfg_val);
+	if (ret)
+		return ret;
+
+	// RS485 gate needs to be enabled; otherwise RTS/CTS will not work
+	exar_write_reg(priv, CTI_FPGA_RS485_IO_REG, 0x01);
+
+	return 0;
+}
+
 static int
 pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 		   struct uart_8250_port *port, int idx)
@@ -880,6 +1670,26 @@ static const struct exar8250_board pbn_fastcom335_8 = {
 	.setup		= pci_fastcom335_setup,
 };

+static const struct exar8250_board pbn_cti_xr17c15x = {
+	.board_init	= cti_board_init_xr17c15x,
+	.setup		= cti_port_setup_xr17c15x,
+};
+
+static const struct exar8250_board pbn_cti_xr17v25x = {
+	.board_init	= cti_board_init_xr17v25x,
+	.setup		= cti_port_setup_xr17v25x,
+};
+
+static const struct exar8250_board pbn_cti_xr17v35x = {
+	.board_init	= cti_board_init_xr17v35x,
+	.setup		= cti_port_setup_xr17v35x,
+};
+
+static const struct exar8250_board pbn_cti_fpga = {
+	.board_init	= cti_board_init_fpga,
+	.setup		= cti_port_setup_fpga,
+};
+
 static const struct exar8250_board pbn_exar_ibm_saturn = {
 	.num_ports	= 1,
 	.setup		= pci_xr17c154_setup,
@@ -924,6 +1734,26 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
 	.exit		= pci_xr17v35x_exit,
 };

+// For Connect Tech cards with Exar vendor/device PCI IDs
+#define CTI_EXAR_DEVICE(devid, bd) {                    \
+	PCI_DEVICE_SUB(                                 \
+		PCI_VENDOR_ID_EXAR,                     \
+		PCI_DEVICE_ID_EXAR_##devid,             \
+		PCI_SUBVENDOR_ID_CONNECT_TECH,          \
+		PCI_ANY_ID), 0, 0,                      \
+		(kernel_ulong_t)&bd                     \
+	}
+
+// For Connect Tech cards with Connect Tech vendor/device PCI IDs (FPGA based)
+#define CTI_PCI_DEVICE(devid, bd) {                     \
+	PCI_DEVICE_SUB(                                 \
+		PCI_VENDOR_ID_CONNECT_TECH,             \
+		PCI_DEVICE_ID_CONNECT_TECH_PCI_##devid, \
+		PCI_ANY_ID,                             \
+		PCI_ANY_ID), 0, 0,                      \
+		(kernel_ulong_t)&bd                     \
+	}
+
 #define EXAR_DEVICE(vend, devid, bd) { PCI_DEVICE_DATA(vend, devid, &bd) }

 #define IBM_DEVICE(devid, sdevid, bd) {			\
@@ -953,6 +1783,22 @@ static const struct pci_device_id exar_pci_tbl[] = {
 	EXAR_DEVICE(ACCESSIO, COM_4SM, pbn_exar_XR17C15x),
 	EXAR_DEVICE(ACCESSIO, COM_8SM, pbn_exar_XR17C15x),

+	CTI_EXAR_DEVICE(XR17C152,       pbn_cti_xr17c15x),
+	CTI_EXAR_DEVICE(XR17C154,       pbn_cti_xr17c15x),
+	CTI_EXAR_DEVICE(XR17C158,       pbn_cti_xr17c15x),
+
+	CTI_EXAR_DEVICE(XR17V252,       pbn_cti_xr17v25x),
+	CTI_EXAR_DEVICE(XR17V254,       pbn_cti_xr17v25x),
+	CTI_EXAR_DEVICE(XR17V258,       pbn_cti_xr17v25x),
+
+	CTI_EXAR_DEVICE(XR17V352,       pbn_cti_xr17v35x),
+	CTI_EXAR_DEVICE(XR17V354,       pbn_cti_xr17v35x),
+	CTI_EXAR_DEVICE(XR17V358,       pbn_cti_xr17v35x),
+
+	CTI_PCI_DEVICE(XR79X_12_XIG00X, pbn_cti_fpga),
+	CTI_PCI_DEVICE(XR79X_12_XIG01X, pbn_cti_fpga),
+	CTI_PCI_DEVICE(XR79X_16,        pbn_cti_fpga),
+
 	IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),

 	/* USRobotics USR298x-OEM PCI Modems */
--
2.43.2

[PATCH v4 7/7] serial: exar: fix checkpach warnings

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

-Fix several "missing identifier name" warnings from checkpatch in
struct exar8250_platform and struct exar8250_board.
Example:

WARNING: function definition argument <arg> should also have an
identifier name

- Fix space before tab warning from checkpatch:
WARNING: please, no space before tabs
+ * 0^I^I2 ^IMode bit 0$

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
Changes in v4:
- Moved to separate patch

 drivers/tty/serial/8250/8250_exar.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 6985aabe13cc..5e42558cbb01 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -147,7 +147,7 @@
  *
  * MPIO		Port	Function
  * ----		----	--------
- * 0		2 	Mode bit 0
+ * 0		2	Mode bit 0
  * 1		2	Mode bit 1
  * 2		2	Terminate bus
  * 3		-	<reserved>
@@ -229,8 +229,8 @@ struct exar8250_platform {
 	int (*rs485_config)(struct uart_port *port, struct ktermios *termios,
 			    struct serial_rs485 *rs485);
 	const struct serial_rs485 *rs485_supported;
-	int (*register_gpio)(struct pci_dev *, struct uart_8250_port *);
-	void (*unregister_gpio)(struct uart_8250_port *);
+	int (*register_gpio)(struct pci_dev *pcidev, struct uart_8250_port *port);
+	void (*unregister_gpio)(struct uart_8250_port *port);
 };

 /**
@@ -245,8 +245,8 @@ struct exar8250_board {
 	unsigned int num_ports;
 	unsigned int reg_shift;
 	int     (*board_init)(struct exar8250 *priv, struct pci_dev *pcidev);
-	int	(*setup)(struct exar8250 *, struct pci_dev *,
-			 struct uart_8250_port *, int);
+	int	(*setup)(struct exar8250 *priv, struct pci_dev *pcidev,
+			 struct uart_8250_port *port, int idx);
 	void	(*exit)(struct pci_dev *pcidev);
 };

--
2.43.2

Re: [PATCH v4 6/7] serial: exar: add CTI specific setup code

[kernel test robot]

Hi Parker,

kernel test robot noticed the following build warnings:

[auto build test WARNING on b86ae40ffcf5a16b9569b1016da4a08c4f352ca2]

url:    https://github.com/intel-lab-lkp/linux/commits/Parker-Newman/serial-exar-remove-old-Connect-Tech-setup/20240418-043457
base:   b86ae40ffcf5a16b9569b1016da4a08c4f352ca2
patch link:    https://lore.kernel.org/r/ae4a66e7342b686cb8d4b15317585dfb37222cf4.1713382717.git.pnewman%40connecttech.com
patch subject: [PATCH v4 6/7] serial: exar: add CTI specific setup code
config: alpha-defconfig (https://download.01.org/0day-ci/archive/20240418/202404181353.1VIC4cz9-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240418/202404181353.1VIC4cz9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404181353.1VIC4cz9-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/tty/serial/8250/8250_exar.c:727: warning: Function parameter or struct member 'pcidev' not described in 'cti_get_port_type_xr17c15x_xr17v25x'
>> drivers/tty/serial/8250/8250_exar.c:819: warning: Function parameter or struct member 'pcidev' not described in 'cti_get_port_type_fpga'
>> drivers/tty/serial/8250/8250_exar.c:849: warning: Function parameter or struct member 'pcidev' not described in 'cti_get_port_type_xr17v35x'


vim +727 drivers/tty/serial/8250/8250_exar.c

   714	
   715	/**
   716	 * cti_get_port_type_xr17c15x_xr17v25x() - Get port type of xr17c15x/xr17v25x
   717	 * @priv: Device's private structure
   718	 * @port_num: Port to get type of
   719	 *
   720	 * CTI xr17c15x and xr17v25x based cards port types are based on PCI IDs.
   721	 *
   722	 * Return: port type on success, CTI_PORT_TYPE_NONE on failure
   723	 */
   724	static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *priv,
   725								struct pci_dev *pcidev,
   726								unsigned int port_num)
 > 727	{
   728		enum cti_port_type port_type;
   729	
   730		switch (pcidev->subsystem_device) {
   731		// RS232 only cards
   732		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_232:
   733		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_232:
   734		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_232:
   735		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232:
   736		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232_NS:
   737		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232:
   738		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232_NS:
   739			port_type = CTI_PORT_TYPE_RS232;
   740			break;
   741		// 1x RS232, 1x RS422/RS485
   742		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
   743			port_type = (port_num == 0) ?
   744				CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
   745			break;
   746		// 2x RS232, 2x RS422/RS485
   747		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
   748			port_type = (port_num < 2) ?
   749				CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
   750			break;
   751		// 4x RS232, 4x RS422/RS485
   752		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
   753		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
   754			port_type = (port_num < 4) ?
   755				CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
   756			break;
   757		// RS232/RS422/RS485 HW (jumper) selectable
   758		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:
   759		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4:
   760		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8:
   761		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_SP_OPTO:
   762		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_A:
   763		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_B:
   764		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
   765		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
   766		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
   767		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
   768		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
   769		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
   770		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
   771		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
   772		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
   773		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
   774		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
   775		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
   776		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
   777		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
   778		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
   779			port_type = CTI_PORT_TYPE_RS232_422_485_HW;
   780			break;
   781		// RS422/RS485 HW (jumper) selectable
   782		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_485:
   783		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_485:
   784		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_485:
   785		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
   786		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
   787			port_type = CTI_PORT_TYPE_RS422_485;
   788			break;
   789		// 6x RS232, 2x RS422/RS485
   790		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
   791			port_type = (port_num < 6) ?
   792				CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
   793			break;
   794		// 2x RS232, 6x RS422/RS485
   795		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
   796			port_type = (port_num < 2) ?
   797				CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
   798			break;
   799		default:
   800			dev_err(&pcidev->dev, "unknown/unsupported device\n");
   801			port_type = CTI_PORT_TYPE_NONE;
   802		}
   803	
   804		return port_type;
   805	}
   806	
   807	/**
   808	 * cti_get_port_type_fpga() - Get the port type of a CTI FPGA card
   809	 * @priv: Device's private structure
   810	 * @port_num: Port to get type of
   811	 *
   812	 * FPGA based cards port types are based on PCI IDs.
   813	 *
   814	 * Return: port type on success, CTI_PORT_TYPE_NONE on failure
   815	 */
   816	static enum cti_port_type cti_get_port_type_fpga(struct exar8250 *priv,
   817							struct pci_dev *pcidev,
   818							unsigned int port_num)
 > 819	{
   820		enum cti_port_type port_type;
   821	
   822		switch (pcidev->device) {
   823		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
   824		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
   825		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
   826			port_type = CTI_PORT_TYPE_RS232_422_485_HW;
   827			break;
   828		default:
   829			dev_err(&pcidev->dev, "unknown/unsupported device\n");
   830			return CTI_PORT_TYPE_NONE;
   831		}
   832	
   833		return port_type;
   834	}
   835	
   836	/**
   837	 * cti_get_port_type_xr17v35x() - Read port type from the EEPROM
   838	 * @priv: Device's private structure
   839	 * @port_num: port offset
   840	 *
   841	 * CTI XR17V35X based cards have the port types stored in the EEPROM.
   842	 * This function reads the port type for a single port.
   843	 *
   844	 * Return: port type on success, CTI_PORT_TYPE_NONE on failure
   845	 */
   846	static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
   847							struct pci_dev *pcidev,
   848							unsigned int port_num)
 > 849	{
   850		enum cti_port_type port_type;
   851		u16 port_flags;
   852		u8 offset;
   853	
   854		offset = CTI_EE_OFF_XR17V35X_PORT_FLAGS + port_num;
   855		port_flags = exar_ee_read(priv, offset);
   856	
   857		port_type = FIELD_GET(CTI_EE_MASK_PORT_FLAGS_TYPE, port_flags);
   858		if (!CTI_PORT_TYPE_VALID(port_type)) {
   859			/*
   860			 * If the port type is missing the card assume it is a
   861			 * RS232/RS422/RS485 card to be safe.
   862			 *
   863			 * There is one known board (BEG013) that only has
   864			 * 3 of 4 port types written to the EEPROM so this
   865			 * acts as a work around.
   866			 */
   867			dev_warn(&pcidev->dev,
   868				"failed to get port %d type from EEPROM\n", port_num);
   869			port_type = CTI_PORT_TYPE_RS232_422_485_HW;
   870		}
   871	
   872		return port_type;
   873	}
   874	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 6/7] serial: exar: add CTI specific setup code

[Greg Kroah-Hartman]

On Thu, Apr 18, 2024 at 01:29:14PM +0800, kernel test robot wrote:
> Hi Parker,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on b86ae40ffcf5a16b9569b1016da4a08c4f352ca2]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Parker-Newman/serial-exar-remove-old-Connect-Tech-setup/20240418-043457
> base:   b86ae40ffcf5a16b9569b1016da4a08c4f352ca2
> patch link:    https://lore.kernel.org/r/ae4a66e7342b686cb8d4b15317585dfb37222cf4.1713382717.git.pnewman%40connecttech.com
> patch subject: [PATCH v4 6/7] serial: exar: add CTI specific setup code
> config: alpha-defconfig (https://download.01.org/0day-ci/archive/20240418/202404181353.1VIC4cz9-lkp@intel.com/config)
> compiler: alpha-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240418/202404181353.1VIC4cz9-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202404181353.1VIC4cz9-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
> >> drivers/tty/serial/8250/8250_exar.c:727: warning: Function parameter or struct member 'pcidev' not described in 'cti_get_port_type_xr17c15x_xr17v25x'
> >> drivers/tty/serial/8250/8250_exar.c:819: warning: Function parameter or struct member 'pcidev' not described in 'cti_get_port_type_fpga'
> >> drivers/tty/serial/8250/8250_exar.c:849: warning: Function parameter or struct member 'pcidev' not described in 'cti_get_port_type_xr17v35x'
> 
> 
> vim +727 drivers/tty/serial/8250/8250_exar.c
> 
>    714	
>    715	/**
>    716	 * cti_get_port_type_xr17c15x_xr17v25x() - Get port type of xr17c15x/xr17v25x
>    717	 * @priv: Device's private structure
>    718	 * @port_num: Port to get type of
>    719	 *
>    720	 * CTI xr17c15x and xr17v25x based cards port types are based on PCI IDs.
>    721	 *
>    722	 * Return: port type on success, CTI_PORT_TYPE_NONE on failure
>    723	 */
>    724	static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *priv,
>    725								struct pci_dev *pcidev,
>    726								unsigned int port_num)
>  > 727	{

Parker, no need to resend this, you can send a follow-on patch after
this to resolve it.  Simplest way is to just not do kernel-doc formats
for static functions, as it's usually not needed.  Or you can add the
function parameter, either one will silence the warning.

thanks,

greg k-h

Re: [PATCH v4 0/7] serial: exar: add Connect Tech serial cards to Exar driver

[Greg Kroah-Hartman]

On Wed, Apr 17, 2024 at 04:31:22PM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> Hello,
> These patches add proper support for most of Connect Tech's (CTI) Exar
> based serial cards. Previously, only a subset of CTI's cards would work
> with the Exar driver while the rest required the CTI out-of-tree driver.
> These patches are intended to phase out the out-of-tree driver.
> 
> I am new to the mailing lists and contributing to the kernel so please
> let me know if I have made any mistakes or if you have any feedback.
> 
> Changes in v2:
> - Put missing PCI IDs in 8250_exar.c instead of pci_ids.h
> - Split large patch into smaller ones
> 
> Changes in v3:
> - Refactored patches to be easier to follow (based on feedback of v2)
> - Patch specific changes listed in corresponding patch
> 
> Changes in v4:
> - Rebased to tty-testing branch
> - Removed v3 patch 8/8, "bug" didn't happen in current driver
> - Patch specific changes listed in corresponding patch
> 
> Thank you,
> 
> Parker Newman (7):
>   serial: exar: remove old Connect Tech setup
>   serial: exar: added a exar_get_nr_ports function
>   serial: exar: add optional board_init function
>   serial: exar: moved generic_rs485 further up in 8250_exar.c
>   serial: exar: add CTI cards to exar_get_nr_ports
>   serial: exar: add CTI specific setup code
>   serial: exar: fix checkpach warnings
> 
>  drivers/tty/serial/8250/8250_exar.c | 981 ++++++++++++++++++++++++++--
>  1 file changed, 916 insertions(+), 65 deletions(-)

Nice, compared to your first version, this is less code overall in this
file:
   1 file changed, 1019 insertions(+), 70 deletions(-)

so the review process helped!

All now applied to my tree, thanks for the revisions.  And a follow-on
patch to fix up the kbuild warning would be appreciated.

thanks,

greg k-h

Re: [PATCH v4 2/7] serial: exar: added a exar_get_nr_ports function

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]

On Wed, 17 Apr 2024, Parker Newman wrote:

> From: Parker Newman <pnewman@connecttech.com>
> 
> Moved code for getting number of ports from exar_pci_probe() to a
> separate exar_get_nr_ports() function. CTI specific code will be added
> in another patch in this series.
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
> Changes in v3:
> - Only moved existing code in this patch, will add CTI code in subsequent
>   patch
> 
>  drivers/tty/serial/8250/8250_exar.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 04ce5e8ddb24..72385c7d2eda 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -704,6 +704,21 @@ static irqreturn_t exar_misc_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
> 
> +static unsigned int exar_get_nr_ports(struct exar8250_board *board,
> +					struct pci_dev *pcidev)
> +{
> +	unsigned int nr_ports = 0;

It's always set, so no need to initialize.

> +	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
> +		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
> +	else if (board->num_ports)
> +		nr_ports = board->num_ports;
> +	else
> +		nr_ports = pcidev->device & 0x0f;
> +
> +	return nr_ports;
> +}
> +
>  static int
>  exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
>  {
> @@ -723,12 +738,12 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
> 
>  	maxnr = pci_resource_len(pcidev, bar) >> (board->reg_shift + 3);
> 
> -	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
> -		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
> -	else if (board->num_ports)
> -		nr_ports = board->num_ports;
> -	else
> -		nr_ports = pcidev->device & 0x0f;
> +	nr_ports = exar_get_nr_ports(board, pcidev);
> +	if (nr_ports == 0) {
> +		dev_err_probe(&pcidev->dev, -ENODEV,
> +				"failed to get number of ports\n");
> +		return -ENODEV;

		return dev_err_probe(&pcidev->dev, -ENODEV,
				     "failed to get number of ports\n");

Other than those two small things,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v4 3/7] serial: exar: add optional board_init function

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]

On Wed, 17 Apr 2024, Parker Newman wrote:

> From: Parker Newman <pnewman@connecttech.com>
> 
> Add an optional "board_init()" function pointer to struct exar8250_board
> which is called once during probe prior to setting up the ports. It will
> be used in subsequent patches of this series.
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
> Changes in v3:
>  - Renamed board_setup to board_init.
>  - Changed pci_err to dev_err_probe
>  - Added note above about checkpatch fixes
> 
> Changes in v4:
>  - Removed checkpatch fixes, they will be in their own patch at the end
>  - Added pcidev to board_init() args to avoid needing to add to priv
> 
>  drivers/tty/serial/8250/8250_exar.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 72385c7d2eda..f14f73d250bb 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -177,12 +177,14 @@ struct exar8250_platform {
>   * struct exar8250_board - board information
>   * @num_ports: number of serial ports
>   * @reg_shift: describes UART register mapping in PCI memory
> - * @setup: quirk run at ->probe() stage
> + * @board_init: quirk run once at ->probe() stage before setting up ports
> + * @setup: quirk run at ->probe() stage for each port
>   * @exit: quirk run at ->remove() stage
>   */
>  struct exar8250_board {
>  	unsigned int num_ports;
>  	unsigned int reg_shift;
> +	int     (*board_init)(struct exar8250 *priv, struct pci_dev *pcidev);
>  	int	(*setup)(struct exar8250 *, struct pci_dev *,
>  			 struct uart_8250_port *, int);
>  	void	(*exit)(struct pci_dev *pcidev);
> @@ -773,6 +775,15 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
>  	if (rc)
>  		return rc;
> 
> +	if (board->board_init) {
> +		rc = board->board_init(priv, pcidev);
> +		if (rc) {
> +			dev_err_probe(&pcidev->dev, rc,
> +					"failed to init serial board\n");
> +			return rc;
> +		}
> +	}
> +

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v4 4/7] serial: exar: moved generic_rs485 further up in 8250_exar.c

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3101 bytes --]

Hi,

In shortlog (Subject), 

- moved -> move
- exar: -> 8250_exar:

After those, "in 8250_exar.c" is redundant information and can be removed.

> From: Parker Newman <pnewman@connecttech.com>
> 
> Preparatory patch moving generic_rs485_config and

Use () when talking about functions.

After fixing those, feel free to add,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

> generic_rs485_supported higher in the file to allow for CTI setup
> functions to use them.
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
> Changes in v3:
>  - split into separate preparatory patch
> 
>  drivers/tty/serial/8250/8250_exar.c | 50 ++++++++++++++---------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index f14f73d250bb..e68029a59122 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -197,6 +197,31 @@ struct exar8250 {
>  	int			line[];
>  };
> 
> +static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
> +				struct serial_rs485 *rs485)
> +{
> +	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
> +	u8 __iomem *p = port->membase;
> +	u8 value;
> +
> +	value = readb(p + UART_EXAR_FCTR);
> +	if (is_rs485)
> +		value |= UART_FCTR_EXAR_485;
> +	else
> +		value &= ~UART_FCTR_EXAR_485;
> +
> +	writeb(value, p + UART_EXAR_FCTR);
> +
> +	if (is_rs485)
> +		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
> +
> +	return 0;
> +}
> +
> +static const struct serial_rs485 generic_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
> +};
> +
>  static void exar_pm(struct uart_port *port, unsigned int state, unsigned int old)
>  {
>  	/*
> @@ -459,27 +484,6 @@ static void xr17v35x_unregister_gpio(struct uart_8250_port *port)
>  	port->port.private_data = NULL;
>  }
> 
> -static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
> -				struct serial_rs485 *rs485)
> -{
> -	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
> -	u8 __iomem *p = port->membase;
> -	u8 value;
> -
> -	value = readb(p + UART_EXAR_FCTR);
> -	if (is_rs485)
> -		value |= UART_FCTR_EXAR_485;
> -	else
> -		value &= ~UART_FCTR_EXAR_485;
> -
> -	writeb(value, p + UART_EXAR_FCTR);
> -
> -	if (is_rs485)
> -		writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
> -
> -	return 0;
> -}
> -
>  static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
>  				  struct serial_rs485 *rs485)
>  {
> @@ -518,10 +522,6 @@ static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termio
>  	return 0;
>  }
> 
> -static const struct serial_rs485 generic_rs485_supported = {
> -	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND,
> -};
> -
>  static const struct exar8250_platform exar8250_default_platform = {
>  	.register_gpio = xr17v35x_register_gpio,
>  	.unregister_gpio = xr17v35x_unregister_gpio,
> --
> 2.43.2
> 
> 

Re: [PATCH v4 5/7] serial: exar: add CTI cards to exar_get_nr_ports

[Ilpo Järvinen]

On Wed, 17 Apr 2024, Parker Newman wrote:

> From: Parker Newman <pnewman@connecttech.com>
> 
> Add code for getting number of ports of CTI cards to
> exar_get_nr_ports().
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
> Changes in v3:
> - moved to separate patch
> - added spaces to single line comments
> 
>  drivers/tty/serial/8250/8250_exar.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index e68029a59122..197f45e306ff 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -711,12 +711,28 @@ static unsigned int exar_get_nr_ports(struct exar8250_board *board,
>  {
>  	unsigned int nr_ports = 0;
> 
> -	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
> +	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO) {

You can add the braces while you moved the code around so you don't need 
to play with them again here and this patch can be more to the point.

>  		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
> -	else if (board->num_ports)
> +	} else if (board->num_ports > 0) {
> +		// Check if board struct overrides number of ports
>  		nr_ports = board->num_ports;

The comment just tells what the code does, IMO that comment doesn't add 
any value.

> -	else
> +	} else if (pcidev->vendor == PCI_VENDOR_ID_EXAR) {
> +		// Exar encodes # ports in last nibble of PCI Device ID ex. 0358

This comment you can also add while you moved the code around (or make 
another patch out of it after moving).

-- 
 i.

>  		nr_ports = pcidev->device & 0x0f;
> +	} else  if (pcidev->vendor == PCI_VENDOR_ID_CONNECT_TECH) {
> +		// Handle CTI FPGA cards
> +		switch (pcidev->device) {
> +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
> +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
> +			nr_ports = 12;
> +			break;
> +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
> +			nr_ports = 16;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> 
>  	return nr_ports;
>  }
> --
> 2.43.2
> 
> 

Re: [PATCH v4 0/7] serial: exar: add Connect Tech serial cards to Exar driver

[Parker Newman]

On Thu, 18 Apr 2024 08:25:10 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Wed, Apr 17, 2024 at 04:31:22PM -0400, Parker Newman wrote:
> > From: Parker Newman <pnewman@connecttech.com>
> >
> > Hello,
> > These patches add proper support for most of Connect Tech's (CTI) Exar
> > based serial cards. Previously, only a subset of CTI's cards would work
> > with the Exar driver while the rest required the CTI out-of-tree driver.
> > These patches are intended to phase out the out-of-tree driver.
> >
> > I am new to the mailing lists and contributing to the kernel so please
> > let me know if I have made any mistakes or if you have any feedback.
> >
> > Changes in v2:
> > - Put missing PCI IDs in 8250_exar.c instead of pci_ids.h
> > - Split large patch into smaller ones
> >
> > Changes in v3:
> > - Refactored patches to be easier to follow (based on feedback of v2)
> > - Patch specific changes listed in corresponding patch
> >
> > Changes in v4:
> > - Rebased to tty-testing branch
> > - Removed v3 patch 8/8, "bug" didn't happen in current driver
> > - Patch specific changes listed in corresponding patch
> >
> > Thank you,
> >
> > Parker Newman (7):
> >   serial: exar: remove old Connect Tech setup
> >   serial: exar: added a exar_get_nr_ports function
> >   serial: exar: add optional board_init function
> >   serial: exar: moved generic_rs485 further up in 8250_exar.c
> >   serial: exar: add CTI cards to exar_get_nr_ports
> >   serial: exar: add CTI specific setup code
> >   serial: exar: fix checkpach warnings
> >
> >  drivers/tty/serial/8250/8250_exar.c | 981 ++++++++++++++++++++++++++--
> >  1 file changed, 916 insertions(+), 65 deletions(-)
>
> Nice, compared to your first version, this is less code overall in this
> file:
>    1 file changed, 1019 insertions(+), 70 deletions(-)
>
> so the review process helped!
>

Yes thanks again for your patience. I learned a lot about the process :).

> All now applied to my tree, thanks for the revisions.  And a follow-on
> patch to fix up the kbuild warning would be appreciated.
>

I will follow up with a mini-series now to fix the kbuild warnings and a
couple minor things mentioned by Ilpo.

Thanks,
Parker

> thanks,
>
> greg k-h

Re: [PATCH v4 6/7] serial: exar: add CTI specific setup code

[Ilpo Järvinen]

On Wed, 17 Apr 2024, Parker Newman wrote:

> From: Parker Newman <pnewman@connecttech.com>
> 
> This is a large patch but is only additions. All changes and removals
> are made in previous patches in this series.
> 
> - Add CTI board_init and port setup functions for each UART type
> - Add CTI_EXAR_DEVICE() and CTI_PCI_DEVICE() macros
> - Add support for reading a word from the Exar EEPROM.
> - Add support for configuring and setting a single MPIO
> - Add various helper functions for CTI boards.
> - Add osc_freq to struct exar8250
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
> Changes in v3:
> - Moved all base driver changes and refactoring to preparatory patches
> - Switched any user space types to kernel types
> - Switched all uses of pci_xxx print functions to dev_xxx
> - Added struct device pointer in struct exar8250 to simplify above
> - Switched osc_freq and port_flag parsing to use GENMASK() and
>   FIELD_GET()/FIELD_PREP()
> - Renamed board_setup function pointer to board_init
> - Removed some unneeded checks for priv being NULL
> - Added various convenience functions instead of relying on bools ex:
>   exar_mpio_set_low()/exar_mpio_set_high() instead of exar_mpio_set()
> - Renamed some variables and defines for clarity
> - Numerous minor formatting fixes
> 
> Changes in v4:
> - Removed pcidev and dev from struct exar8250
> - Removed some debug prints
> - Removed some unneeded checks if PCI vendor was Exar
> - Changed several functions to take pcidev as arg to avoid adding to priv
> - Removed _exar_mpio_config(), only needed exar_mpio_config_output()
> - Removed _cti_set_tristate() and _cti_set_plx_int_enable, same as above
> 
>  drivers/tty/serial/8250/8250_exar.c | 846 ++++++++++++++++++++++++++++
>  1 file changed, 846 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 197f45e306ff..6985aabe13cc 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -20,6 +20,7 @@
>  #include <linux/property.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
> +#include <linux/bitfield.h>
> 
>  #include <linux/serial_8250.h>
>  #include <linux/serial_core.h>
> @@ -128,6 +129,19 @@
>  #define UART_EXAR_DLD			0x02 /* Divisor Fractional */
>  #define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polarity */
> 
> +/* EEPROM registers */
> +#define UART_EXAR_REGB			0x8e
> +#define UART_EXAR_REGB_EECK		BIT(4)
> +#define UART_EXAR_REGB_EECS		BIT(5)
> +#define UART_EXAR_REGB_EEDI		BIT(6)
> +#define UART_EXAR_REGB_EEDO		BIT(7)
> +#define UART_EXAR_REGB_EE_ADDR_SIZE	6
> +#define UART_EXAR_REGB_EE_DATA_SIZE	16
> +
> +#define UART_EXAR_XR17C15X_PORT_OFFSET	0x200
> +#define UART_EXAR_XR17V25X_PORT_OFFSET	0x200
> +#define UART_EXAR_XR17V35X_PORT_OFFSET	0x400
> +
>  /*
>   * IOT2040 MPIO wiring semantics:
>   *
> @@ -163,6 +177,52 @@
>  #define IOT2040_UARTS_ENABLE		0x03
>  #define IOT2040_UARTS_GPIO_HI_MODE	0xF8	/* enable & LED as outputs */
> 
> +/* CTI EEPROM offsets */
> +#define CTI_EE_OFF_XR17C15X_OSC_FREQ	0x04  /* 2 words */
> +#define CTI_EE_OFF_XR17V25X_OSC_FREQ	0x08  /* 2 words */
> +#define CTI_EE_OFF_XR17C15X_PART_NUM	0x0A  /* 4 words */
> +#define CTI_EE_OFF_XR17V25X_PART_NUM	0x0E  /* 4 words */
> +#define CTI_EE_OFF_XR17C15X_SERIAL_NUM	0x0E  /* 1 word */
> +#define CTI_EE_OFF_XR17V25X_SERIAL_NUM	0x12  /* 1 word */
> +#define CTI_EE_OFF_XR17V35X_SERIAL_NUM	0x11  /* 2 word */
> +#define CTI_EE_OFF_XR17V35X_BRD_FLAGS	0x13  /* 1 word */
> +#define CTI_EE_OFF_XR17V35X_PORT_FLAGS	0x14  /* 1 word */
> +
> +#define CTI_EE_MASK_PORT_FLAGS_TYPE	GENMASK(7, 0)
> +#define CTI_EE_MASK_OSC_FREQ_LOWER	GENMASK(15, 0)
> +#define CTI_EE_MASK_OSC_FREQ_UPPER	GENMASK(31, 16)
> +
> +#define CTI_FPGA_RS485_IO_REG		0x2008
> +#define CTI_FPGA_CFG_INT_EN_REG		0x48
> +#define CTI_FPGA_CFG_INT_EN_EXT_BIT	BIT(15) /* External int enable bit */
> +
> +#define CTI_DEFAULT_PCI_OSC_FREQ	29491200
> +#define CTI_DEFAULT_PCIE_OSC_FREQ	125000000
> +#define CTI_DEFAULT_FPGA_OSC_FREQ	33333333
> +
> +/*
> + * CTI Serial port line types. These match the values stored in the first
> + * nibble of the CTI EEPROM port_flags word.
> + */
> +enum cti_port_type {
> +	CTI_PORT_TYPE_NONE = 0,
> +	CTI_PORT_TYPE_RS232,            // RS232 ONLY
> +	CTI_PORT_TYPE_RS422_485,        // RS422/RS485 ONLY
> +	CTI_PORT_TYPE_RS232_422_485_HW, // RS232/422/485 HW ONLY Switchable
> +	CTI_PORT_TYPE_RS232_422_485_SW, // RS232/422/485 SW ONLY Switchable
> +	CTI_PORT_TYPE_RS232_422_485_4B, // RS232/422/485 HW/SW (4bit ex. BCG004)
> +	CTI_PORT_TYPE_RS232_422_485_2B, // RS232/422/485 HW/SW (2bit ex. BBG008)
> +	CTI_PORT_TYPE_MAX,
> +};
> +
> +#define CTI_PORT_TYPE_VALID(_port_type) \
> +	(((_port_type) > CTI_PORT_TYPE_NONE) && \
> +	((_port_type) < CTI_PORT_TYPE_MAX))
> +
> +#define CTI_PORT_TYPE_RS485(_port_type) \
> +	(((_port_type) > CTI_PORT_TYPE_RS232) && \
> +	((_port_type) < CTI_PORT_TYPE_MAX))
> +
>  struct exar8250;
> 
>  struct exar8250_platform {
> @@ -192,11 +252,201 @@ struct exar8250_board {
> 
>  struct exar8250 {
>  	unsigned int		nr;
> +	unsigned int		osc_freq;
>  	struct exar8250_board	*board;
>  	void __iomem		*virt;
>  	int			line[];
>  };
> 
> +static inline void exar_write_reg(struct exar8250 *priv,
> +				unsigned int reg, u8 value)
> +{
> +	writeb(value, priv->virt + reg);
> +}
> +
> +static inline u8 exar_read_reg(struct exar8250 *priv, unsigned int reg)
> +{
> +	return readb(priv->virt + reg);
> +}

I tried to understand what is going on with this priv->virt in 8250_exar 
in general and why it exists but I failed. It seems to BAR0 is mapped 
there but also serial8250_pci_setup_port() does map the same BAR and 
sets it up into the usual place in membase.

> +static inline void exar_ee_select(struct exar8250 *priv)
> +{
> +	// Set chip select pin high to enable EEPROM reads/writes
> +	exar_write_reg(priv, UART_EXAR_REGB, UART_EXAR_REGB_EECS);
> +	// Min ~500ns delay needed between CS assert and EEPROM access
> +	udelay(1);
> +}
> +
> +static inline void exar_ee_deselect(struct exar8250 *priv)
> +{
> +	exar_write_reg(priv, UART_EXAR_REGB, 0x00);
> +}
> +
> +static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
> +{
> +	u8 value = UART_EXAR_REGB_EECS;
> +
> +	if (bit)
> +		value |= UART_EXAR_REGB_EEDI;
> +
> +	// Clock out the bit on the EEPROM interface
> +	exar_write_reg(priv, UART_EXAR_REGB, value);
> +	// 2us delay = ~500khz clock speed
> +	udelay(2);
> +
> +	value |= UART_EXAR_REGB_EECK;
> +
> +	exar_write_reg(priv, UART_EXAR_REGB, value);
> +	udelay(2);
> +}
> +
> +static inline u8 exar_ee_read_bit(struct exar8250 *priv)
> +{
> +	u8 regb;
> +	u8 value = UART_EXAR_REGB_EECS;
> +
> +	// Clock in the bit on the EEPROM interface
> +	exar_write_reg(priv, UART_EXAR_REGB, value);
> +	// 2us delay = ~500khz clock speed
> +	udelay(2);
> +
> +	value |= UART_EXAR_REGB_EECK;
> +
> +	exar_write_reg(priv, UART_EXAR_REGB, value);
> +	udelay(2);
> +
> +	regb = exar_read_reg(priv, UART_EXAR_REGB);
> +
> +	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);

Unnecessary parenthesis.

> +}
> +
> +/**
> + * exar_ee_read() - Read a word from the EEPROM
> + * @priv: Device's private structure
> + * @ee_addr: Offset of EEPROM to read word from
> + *
> + * Read a single 16bit word from an Exar UART's EEPROM.
> + *
> + * Return: EEPROM word
> + */
> +static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr)
> +{
> +	int i;
> +	u16 data = 0;
> +
> +	exar_ee_select(priv);
> +
> +	// Send read command (opcode 110)
> +	exar_ee_write_bit(priv, 1);
> +	exar_ee_write_bit(priv, 1);
> +	exar_ee_write_bit(priv, 0);
> +
> +	// Send address to read from
> +	for (i = 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>= 1)
> +		exar_ee_write_bit(priv, (ee_addr & i));
> +
> +	// Read data 1 bit at a time
> +	for (i = 0; i <= UART_EXAR_REGB_EE_DATA_SIZE; i++) {
> +		data <<= 1;
> +		data |= exar_ee_read_bit(priv);
> +	}
> +
> +	exar_ee_deselect(priv);
> +
> +	return data;
> +}
> +
> +/**
> + * exar_mpio_config_output() - Configure an Exar MPIO as an output
> + * @priv: Device's private structure
> + * @mpio_num: MPIO number/offset to configure
> + *
> + * Configure a single MPIO as an output and disable tristate. It is reccomended
> + * to set the level with exar_mpio_set_high()/exar_mpio_set_low() prior to
> + * calling this function to ensure default MPIO pin state.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int exar_mpio_config_output(struct exar8250 *priv,
> +				unsigned int mpio_num)
> +{
> +	unsigned int mpio_offset;
> +	u8 sel_reg; // MPIO Select register (input/output)
> +	u8 tri_reg; // MPIO Tristate register
> +	u8 value;
> +
> +	if (mpio_num < 8) {
> +		sel_reg = UART_EXAR_MPIOSEL_7_0;
> +		tri_reg = UART_EXAR_MPIO3T_7_0;
> +		mpio_offset = mpio_num;
> +	} else if (mpio_num >= 8 && mpio_num < 16) {
> +		sel_reg = UART_EXAR_MPIOSEL_15_8;
> +		tri_reg = UART_EXAR_MPIO3T_15_8;
> +		mpio_offset = mpio_num - 8;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	// Disable MPIO pin tri-state
> +	value = exar_read_reg(priv, tri_reg);
> +	value &= ~BIT(mpio_offset);
> +	exar_write_reg(priv, tri_reg, value);
> +
> +	value = exar_read_reg(priv, sel_reg);
> +	value &= ~BIT(mpio_offset);
> +	exar_write_reg(priv, sel_reg, value);
> +
> +	return 0;
> +}
> +
> +/**
> + * _exar_mpio_set() - Set an Exar MPIO output high or low
> + * @priv: Device's private structure
> + * @mpio_num: MPIO number/offset to set
> + * @high: Set MPIO high if true, low if false
> + *
> + * Set a single MPIO high or low. exar_mpio_config_output() must also be called
> + * to configure the pin as an output.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int _exar_mpio_set(struct exar8250 *priv,
> +			unsigned int mpio_num, bool high)
> +{
> +	unsigned int mpio_offset;
> +	u8 lvl_reg;
> +	u8 value;
> +
> +	if (mpio_num < 8) {
> +		lvl_reg = UART_EXAR_MPIOLVL_7_0;
> +		mpio_offset = mpio_num;
> +	} else if (mpio_num >= 8 && mpio_num < 16) {
> +		lvl_reg = UART_EXAR_MPIOLVL_15_8;
> +		mpio_offset = mpio_num - 8;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	value = exar_read_reg(priv, lvl_reg);
> +	if (high)
> +		value |= BIT(mpio_offset);
> +	else
> +		value &= ~BIT(mpio_offset);
> +	exar_write_reg(priv, lvl_reg, value);
> +
> +	return 0;
> +}
> +
> +static int exar_mpio_set_low(struct exar8250 *priv, unsigned int mpio_num)
> +{
> +	return _exar_mpio_set(priv, mpio_num, false);
> +}
> +
> +static int exar_mpio_set_high(struct exar8250 *priv, unsigned int mpio_num)
> +{
> +	return _exar_mpio_set(priv, mpio_num, true);
> +}
> +
>  static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
>  				struct serial_rs485 *rs485)
>  {
> @@ -385,6 +635,546 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>  	return 0;
>  }
> 
> +/**
> + * cti_tristate_disable() - Disable RS485 transciever tristate
> + * @priv: Device's private structure
> + * @port_num: Port number to set tristate off
> + *
> + * Most RS485 capable cards have a power on tristate jumper/switch that ensures
> + * the RS422/RS485 transciever does not drive a multi-drop RS485 bus when it is
> + * not the master. When this jumper is installed the user must set the RS485
> + * mode to Full or Half duplex to disable tristate prior to using the port.
> + *
> + * Some Exar UARTs have an auto-tristate feature while others require setting
> + * an MPIO to disable the tristate.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int cti_tristate_disable(struct exar8250 *priv, unsigned int port_num)
> +{
> +	int ret;
> +
> +	ret = exar_mpio_set_high(priv, port_num);
> +	if (ret)
> +		return ret;
> +
> +	return exar_mpio_config_output(priv, port_num);
> +}
> +
> +/**
> + * cti_plx_int_enable() - Enable UART interrupts to PLX bridge
> + * @priv: Device's private structure
> + *
> + * Some older CTI cards require MPIO_0 to be set low to enable the
> + * interupts from the UART to the PLX PCI->PCIe bridge.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int cti_plx_int_enable(struct exar8250 *priv)
> +{
> +	int ret;
> +
> +	ret = exar_mpio_set_low(priv, 0);
> +	if (ret)
> +		return ret;
> +
> +	return exar_mpio_config_output(priv, 0);
> +}
> +
> +/**
> + * cti_read_osc_freq() - Read the UART oscillator frequency from EEPROM
> + * @priv: Device's private structure
> + * @eeprom_offset: Offset where the oscillator frequency is stored
> + *
> + * CTI XR17x15X and XR17V25X cards have the serial boards oscillator frequency
> + * stored in the EEPROM. FPGA and XR17V35X based cards use the PCI/PCIe clock.
> + *
> + * Return: frequency on success, negative error code on failure
> + */
> +static int cti_read_osc_freq(struct exar8250 *priv, u8 eeprom_offset)
> +{
> +	u16 lower_word;
> +	u16 upper_word;
> +	int osc_freq;
> +
> +	lower_word = exar_ee_read(priv, eeprom_offset);
> +	// Check if EEPROM word was blank
> +	if (lower_word == 0xFFFF)
> +		return -EIO;
> +
> +	upper_word = exar_ee_read(priv, (eeprom_offset + 1));
> +	if (upper_word == 0xFFFF)
> +		return -EIO;
> +
> +	osc_freq = FIELD_PREP(CTI_EE_MASK_OSC_FREQ_LOWER, lower_word) |
> +		FIELD_PREP(CTI_EE_MASK_OSC_FREQ_UPPER, upper_word);
> +
> +	return osc_freq;
> +}
> +
> +/**
> + * cti_get_port_type_xr17c15x_xr17v25x() - Get port type of xr17c15x/xr17v25x
> + * @priv: Device's private structure
> + * @port_num: Port to get type of
> + *
> + * CTI xr17c15x and xr17v25x based cards port types are based on PCI IDs.
> + *
> + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> + */
> +static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *priv,
> +							struct pci_dev *pcidev,
> +							unsigned int port_num)
> +{
> +	enum cti_port_type port_type;
> +
> +	switch (pcidev->subsystem_device) {
> +	// RS232 only cards
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_232:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_232:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_232:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232_NS:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232_NS:
> +		port_type = CTI_PORT_TYPE_RS232;
> +		break;
> +	// 1x RS232, 1x RS422/RS485
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
> +		port_type = (port_num == 0) ?
> +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;

I'd linesplit and align these differently:

		port_type = port_num == 0 ? CTI_PORT_TYPE_RS232 :
					    CTI_PORT_TYPE_RS422_485;

> +		break;
> +	// 2x RS232, 2x RS422/RS485
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
> +		port_type = (port_num < 2) ?
> +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> +		break;
> +	// 4x RS232, 4x RS422/RS485
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
> +		port_type = (port_num < 4) ?
> +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> +		break;
> +	// RS232/RS422/RS485 HW (jumper) selectable
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_SP_OPTO:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
> +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> +		break;
> +	// RS422/RS485 HW (jumper) selectable
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_485:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_485:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_485:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
> +		port_type = CTI_PORT_TYPE_RS422_485;
> +		break;
> +	// 6x RS232, 2x RS422/RS485
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
> +		port_type = (port_num < 6) ?
> +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> +		break;
> +	// 2x RS232, 6x RS422/RS485
> +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
> +		port_type = (port_num < 2) ?
> +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> +		break;
> +	default:
> +		dev_err(&pcidev->dev, "unknown/unsupported device\n");
> +		port_type = CTI_PORT_TYPE_NONE;
> +	}
> +
> +	return port_type;
> +}
> +
> +/**
> + * cti_get_port_type_fpga() - Get the port type of a CTI FPGA card
> + * @priv: Device's private structure
> + * @port_num: Port to get type of
> + *
> + * FPGA based cards port types are based on PCI IDs.
> + *
> + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> + */
> +static enum cti_port_type cti_get_port_type_fpga(struct exar8250 *priv,
> +						struct pci_dev *pcidev,
> +						unsigned int port_num)
> +{
> +	enum cti_port_type port_type;
> +
> +	switch (pcidev->device) {
> +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
> +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
> +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
> +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> +		break;
> +	default:
> +		dev_err(&pcidev->dev, "unknown/unsupported device\n");
> +		return CTI_PORT_TYPE_NONE;
> +	}
> +
> +	return port_type;
> +}
> +
> +/**
> + * cti_get_port_type_xr17v35x() - Read port type from the EEPROM
> + * @priv: Device's private structure
> + * @port_num: port offset
> + *
> + * CTI XR17V35X based cards have the port types stored in the EEPROM.
> + * This function reads the port type for a single port.
> + *
> + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> + */
> +static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
> +						struct pci_dev *pcidev,
> +						unsigned int port_num)
> +{
> +	enum cti_port_type port_type;
> +	u16 port_flags;
> +	u8 offset;
> +
> +	offset = CTI_EE_OFF_XR17V35X_PORT_FLAGS + port_num;
> +	port_flags = exar_ee_read(priv, offset);
> +
> +	port_type = FIELD_GET(CTI_EE_MASK_PORT_FLAGS_TYPE, port_flags);
> +	if (!CTI_PORT_TYPE_VALID(port_type)) {
> +		/*
> +		 * If the port type is missing the card assume it is a
> +		 * RS232/RS422/RS485 card to be safe.
> +		 *
> +		 * There is one known board (BEG013) that only has
> +		 * 3 of 4 port types written to the EEPROM so this
> +		 * acts as a work around.
> +		 */
> +		dev_warn(&pcidev->dev,
> +			"failed to get port %d type from EEPROM\n", port_num);
> +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> +	}
> +
> +	return port_type;
> +}
> +
> +static int cti_rs485_config_mpio_tristate(struct uart_port *port,
> +					struct ktermios *termios,
> +					struct serial_rs485 *rs485)
> +{
> +	struct exar8250 *priv = (struct exar8250 *)port->private_data;
> +	int ret;
> +
> +	ret = generic_rs485_config(port, termios, rs485);
> +	if (ret)
> +		return ret;
> +
> +	// Disable power-on RS485 tri-state via MPIO
> +	return cti_tristate_disable(priv, port->port_id);
> +}
> +
> +static int cti_port_setup_common(struct exar8250 *priv,
> +				struct pci_dev *pcidev,
> +				int idx, unsigned int offset,
> +				struct uart_8250_port *port)
> +{
> +	int ret;
> +
> +	if (priv->osc_freq == 0)
> +		return -EINVAL;
> +
> +	port->port.port_id = idx;
> +	port->port.uartclk = priv->osc_freq;
> +
> +	ret = serial8250_pci_setup_port(pcidev, port, 0, offset, 0);
> +	if (ret) {
> +		dev_err(&pcidev->dev,
> +			"failed to setup pci for port %d err: %d\n", idx, ret);
> +		return ret;
> +	}
> +
> +	port->port.private_data = (void *)priv;
> +	port->port.pm = exar_pm;
> +	port->port.shutdown = exar_shutdown;
> +
> +	return 0;
> +}
> +
> +static int cti_port_setup_fpga(struct exar8250 *priv,
> +				struct pci_dev *pcidev,
> +				struct uart_8250_port *port,
> +				int idx)
> +{
> +	enum cti_port_type port_type;
> +	unsigned int offset;
> +
> +	port_type = cti_get_port_type_fpga(priv, pcidev, idx);
> +
> +	// FPGA shares port offests with XR17C15X
> +	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
> +	port->port.type = PORT_XR17D15X;
> +
> +	port->port.get_divisor = xr17v35x_get_divisor;
> +	port->port.set_divisor = xr17v35x_set_divisor;
> +	port->port.startup = xr17v35x_startup;
> +
> +	if (CTI_PORT_TYPE_RS485(port_type)) {
> +		port->port.rs485_config = generic_rs485_config;
> +		port->port.rs485_supported = generic_rs485_supported;
> +	}
> +
> +	return cti_port_setup_common(priv, pcidev, idx, offset, port);
> +}
> +
> +static int cti_port_setup_xr17v35x(struct exar8250 *priv,
> +				struct pci_dev *pcidev,
> +				struct uart_8250_port *port,
> +				int idx)
> +{
> +	enum cti_port_type port_type;
> +	unsigned int offset;
> +	int ret;
> +
> +	port_type = cti_get_port_type_xr17v35x(priv, pcidev, idx);
> +
> +	offset = idx * UART_EXAR_XR17V35X_PORT_OFFSET;
> +	port->port.type = PORT_XR17V35X;
> +
> +	port->port.get_divisor = xr17v35x_get_divisor;
> +	port->port.set_divisor = xr17v35x_set_divisor;
> +	port->port.startup = xr17v35x_startup;
> +
> +	switch (port_type) {
> +	case CTI_PORT_TYPE_RS422_485:
> +	case CTI_PORT_TYPE_RS232_422_485_HW:
> +		port->port.rs485_config = cti_rs485_config_mpio_tristate;
> +		port->port.rs485_supported = generic_rs485_supported;
> +		break;
> +	case CTI_PORT_TYPE_RS232_422_485_SW:
> +	case CTI_PORT_TYPE_RS232_422_485_4B:
> +	case CTI_PORT_TYPE_RS232_422_485_2B:
> +		port->port.rs485_config = generic_rs485_config;
> +		port->port.rs485_supported = generic_rs485_supported;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	ret = cti_port_setup_common(priv, pcidev, idx, offset, port);
> +	if (ret)
> +		return ret;
> +
> +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 128);
> +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 128);

Unnecessary parenthesis.

> +	return 0;
> +}
> +
> +static int cti_port_setup_xr17v25x(struct exar8250 *priv,
> +				struct pci_dev *pcidev,
> +				struct uart_8250_port *port,
> +				int idx)
> +{
> +	enum cti_port_type port_type;
> +	unsigned int offset;
> +	int ret;
> +
> +	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, pcidev, idx);
> +
> +	offset = idx * UART_EXAR_XR17V25X_PORT_OFFSET;
> +	port->port.type = PORT_XR17D15X;
> +
> +	// XR17V25X supports fractional baudrates
> +	port->port.get_divisor = xr17v35x_get_divisor;
> +	port->port.set_divisor = xr17v35x_set_divisor;
> +	port->port.startup = xr17v35x_startup;
> +
> +	if (CTI_PORT_TYPE_RS485(port_type)) {
> +		switch (pcidev->subsystem_device) {
> +		// These cards support power on 485 tri-state via MPIO
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
> +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
> +			port->port.rs485_config = cti_rs485_config_mpio_tristate;
> +			break;
> +		// Otherwise auto or no power on 485 tri-state support
> +		default:
> +			port->port.rs485_config = generic_rs485_config;
> +			break;
> +		}
> +
> +		port->port.rs485_supported = generic_rs485_supported;
> +	}
> +
> +	ret = cti_port_setup_common(priv, pcidev, idx, offset, port);
> +	if (ret)
> +		return ret;
> +
> +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 32);
> +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 32);

Unnecessary parenthesis.

I recommend you add a helper for this as it is repeated twice. Are the 
values 32 and 128 literal or do they have some specific meaning? If the 
latter case, they should be using named defines (this likely applies to 
the existing trigger code in the driver too).


-- 
 i.

Re: [PATCH v4 6/7] serial: exar: add CTI specific setup code

[Parker Newman]

On Thu, 18 Apr 2024 16:20:15 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Wed, 17 Apr 2024, Parker Newman wrote:
> 
> > From: Parker Newman <pnewman@connecttech.com>
> > 
> > This is a large patch but is only additions. All changes and removals
> > are made in previous patches in this series.
> > 
> > - Add CTI board_init and port setup functions for each UART type
> > - Add CTI_EXAR_DEVICE() and CTI_PCI_DEVICE() macros
> > - Add support for reading a word from the Exar EEPROM.
> > - Add support for configuring and setting a single MPIO
> > - Add various helper functions for CTI boards.
> > - Add osc_freq to struct exar8250
> > 
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> > Changes in v3:
> > - Moved all base driver changes and refactoring to preparatory patches
> > - Switched any user space types to kernel types
> > - Switched all uses of pci_xxx print functions to dev_xxx
> > - Added struct device pointer in struct exar8250 to simplify above
> > - Switched osc_freq and port_flag parsing to use GENMASK() and
> >   FIELD_GET()/FIELD_PREP()
> > - Renamed board_setup function pointer to board_init
> > - Removed some unneeded checks for priv being NULL
> > - Added various convenience functions instead of relying on bools ex:
> >   exar_mpio_set_low()/exar_mpio_set_high() instead of exar_mpio_set()
> > - Renamed some variables and defines for clarity
> > - Numerous minor formatting fixes
> > 
> > Changes in v4:
> > - Removed pcidev and dev from struct exar8250
> > - Removed some debug prints
> > - Removed some unneeded checks if PCI vendor was Exar
> > - Changed several functions to take pcidev as arg to avoid adding to priv
> > - Removed _exar_mpio_config(), only needed exar_mpio_config_output()
> > - Removed _cti_set_tristate() and _cti_set_plx_int_enable, same as above
> > 
> >  drivers/tty/serial/8250/8250_exar.c | 846 ++++++++++++++++++++++++++++
> >  1 file changed, 846 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 197f45e306ff..6985aabe13cc 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/property.h>
> >  #include <linux/string.h>
> >  #include <linux/types.h>
> > +#include <linux/bitfield.h>
> > 
> >  #include <linux/serial_8250.h>
> >  #include <linux/serial_core.h>
> > @@ -128,6 +129,19 @@
> >  #define UART_EXAR_DLD			0x02 /* Divisor Fractional */
> >  #define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polarity */
> > 
> > +/* EEPROM registers */
> > +#define UART_EXAR_REGB			0x8e
> > +#define UART_EXAR_REGB_EECK		BIT(4)
> > +#define UART_EXAR_REGB_EECS		BIT(5)
> > +#define UART_EXAR_REGB_EEDI		BIT(6)
> > +#define UART_EXAR_REGB_EEDO		BIT(7)
> > +#define UART_EXAR_REGB_EE_ADDR_SIZE	6
> > +#define UART_EXAR_REGB_EE_DATA_SIZE	16
> > +
> > +#define UART_EXAR_XR17C15X_PORT_OFFSET	0x200
> > +#define UART_EXAR_XR17V25X_PORT_OFFSET	0x200
> > +#define UART_EXAR_XR17V35X_PORT_OFFSET	0x400
> > +
> >  /*
> >   * IOT2040 MPIO wiring semantics:
> >   *
> > @@ -163,6 +177,52 @@
> >  #define IOT2040_UARTS_ENABLE		0x03
> >  #define IOT2040_UARTS_GPIO_HI_MODE	0xF8	/* enable & LED as outputs */
> > 
> > +/* CTI EEPROM offsets */
> > +#define CTI_EE_OFF_XR17C15X_OSC_FREQ	0x04  /* 2 words */
> > +#define CTI_EE_OFF_XR17V25X_OSC_FREQ	0x08  /* 2 words */
> > +#define CTI_EE_OFF_XR17C15X_PART_NUM	0x0A  /* 4 words */
> > +#define CTI_EE_OFF_XR17V25X_PART_NUM	0x0E  /* 4 words */
> > +#define CTI_EE_OFF_XR17C15X_SERIAL_NUM	0x0E  /* 1 word */
> > +#define CTI_EE_OFF_XR17V25X_SERIAL_NUM	0x12  /* 1 word */
> > +#define CTI_EE_OFF_XR17V35X_SERIAL_NUM	0x11  /* 2 word */
> > +#define CTI_EE_OFF_XR17V35X_BRD_FLAGS	0x13  /* 1 word */
> > +#define CTI_EE_OFF_XR17V35X_PORT_FLAGS	0x14  /* 1 word */
> > +
> > +#define CTI_EE_MASK_PORT_FLAGS_TYPE	GENMASK(7, 0)
> > +#define CTI_EE_MASK_OSC_FREQ_LOWER	GENMASK(15, 0)
> > +#define CTI_EE_MASK_OSC_FREQ_UPPER	GENMASK(31, 16)
> > +
> > +#define CTI_FPGA_RS485_IO_REG		0x2008
> > +#define CTI_FPGA_CFG_INT_EN_REG		0x48
> > +#define CTI_FPGA_CFG_INT_EN_EXT_BIT	BIT(15) /* External int enable bit */
> > +
> > +#define CTI_DEFAULT_PCI_OSC_FREQ	29491200
> > +#define CTI_DEFAULT_PCIE_OSC_FREQ	125000000
> > +#define CTI_DEFAULT_FPGA_OSC_FREQ	33333333
> > +
> > +/*
> > + * CTI Serial port line types. These match the values stored in the first
> > + * nibble of the CTI EEPROM port_flags word.
> > + */
> > +enum cti_port_type {
> > +	CTI_PORT_TYPE_NONE = 0,
> > +	CTI_PORT_TYPE_RS232,            // RS232 ONLY
> > +	CTI_PORT_TYPE_RS422_485,        // RS422/RS485 ONLY
> > +	CTI_PORT_TYPE_RS232_422_485_HW, // RS232/422/485 HW ONLY Switchable
> > +	CTI_PORT_TYPE_RS232_422_485_SW, // RS232/422/485 SW ONLY Switchable
> > +	CTI_PORT_TYPE_RS232_422_485_4B, // RS232/422/485 HW/SW (4bit ex. BCG004)
> > +	CTI_PORT_TYPE_RS232_422_485_2B, // RS232/422/485 HW/SW (2bit ex. BBG008)
> > +	CTI_PORT_TYPE_MAX,
> > +};
> > +
> > +#define CTI_PORT_TYPE_VALID(_port_type) \
> > +	(((_port_type) > CTI_PORT_TYPE_NONE) && \
> > +	((_port_type) < CTI_PORT_TYPE_MAX))
> > +
> > +#define CTI_PORT_TYPE_RS485(_port_type) \
> > +	(((_port_type) > CTI_PORT_TYPE_RS232) && \
> > +	((_port_type) < CTI_PORT_TYPE_MAX))
> > +
> >  struct exar8250;
> > 
> >  struct exar8250_platform {
> > @@ -192,11 +252,201 @@ struct exar8250_board {
> > 
> >  struct exar8250 {
> >  	unsigned int		nr;
> > +	unsigned int		osc_freq;
> >  	struct exar8250_board	*board;
> >  	void __iomem		*virt;
> >  	int			line[];
> >  };
> > 
> > +static inline void exar_write_reg(struct exar8250 *priv,
> > +				unsigned int reg, u8 value)
> > +{
> > +	writeb(value, priv->virt + reg);
> > +}
> > +
> > +static inline u8 exar_read_reg(struct exar8250 *priv, unsigned int reg)
> > +{
> > +	return readb(priv->virt + reg);
> > +}  
> 
> I tried to understand what is going on with this priv->virt in 8250_exar 
> in general and why it exists but I failed. It seems to BAR0 is mapped 
> there but also serial8250_pci_setup_port() does map the same BAR and 
> sets it up into the usual place in membase.
> 

Exar PCI/PCIe UARTs have global configuration registers from 0x80-0x9B.
These registers are for reading the EEPROM, configuring the MPIO, etc.
As these registers are only at 0x80, and not port specific, the driver maps
BAR0 to priv->virt for accessing them. 

> > +static inline void exar_ee_select(struct exar8250 *priv)
> > +{
> > +	// Set chip select pin high to enable EEPROM reads/writes
> > +	exar_write_reg(priv, UART_EXAR_REGB, UART_EXAR_REGB_EECS);
> > +	// Min ~500ns delay needed between CS assert and EEPROM access
> > +	udelay(1);
> > +}
> > +
> > +static inline void exar_ee_deselect(struct exar8250 *priv)
> > +{
> > +	exar_write_reg(priv, UART_EXAR_REGB, 0x00);
> > +}
> > +
> > +static inline void exar_ee_write_bit(struct exar8250 *priv, int bit)
> > +{
> > +	u8 value = UART_EXAR_REGB_EECS;
> > +
> > +	if (bit)
> > +		value |= UART_EXAR_REGB_EEDI;
> > +
> > +	// Clock out the bit on the EEPROM interface
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	// 2us delay = ~500khz clock speed
> > +	udelay(2);
> > +
> > +	value |= UART_EXAR_REGB_EECK;
> > +
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +}
> > +
> > +static inline u8 exar_ee_read_bit(struct exar8250 *priv)
> > +{
> > +	u8 regb;
> > +	u8 value = UART_EXAR_REGB_EECS;
> > +
> > +	// Clock in the bit on the EEPROM interface
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	// 2us delay = ~500khz clock speed
> > +	udelay(2);
> > +
> > +	value |= UART_EXAR_REGB_EECK;
> > +
> > +	exar_write_reg(priv, UART_EXAR_REGB, value);
> > +	udelay(2);
> > +
> > +	regb = exar_read_reg(priv, UART_EXAR_REGB);
> > +
> > +	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);  
> 
> Unnecessary parenthesis.
> 
> > +}
> > +
> > +/**
> > + * exar_ee_read() - Read a word from the EEPROM
> > + * @priv: Device's private structure
> > + * @ee_addr: Offset of EEPROM to read word from
> > + *
> > + * Read a single 16bit word from an Exar UART's EEPROM.
> > + *
> > + * Return: EEPROM word
> > + */
> > +static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr)
> > +{
> > +	int i;
> > +	u16 data = 0;
> > +
> > +	exar_ee_select(priv);
> > +
> > +	// Send read command (opcode 110)
> > +	exar_ee_write_bit(priv, 1);
> > +	exar_ee_write_bit(priv, 1);
> > +	exar_ee_write_bit(priv, 0);
> > +
> > +	// Send address to read from
> > +	for (i = 1 << (UART_EXAR_REGB_EE_ADDR_SIZE - 1); i; i >>= 1)
> > +		exar_ee_write_bit(priv, (ee_addr & i));
> > +
> > +	// Read data 1 bit at a time
> > +	for (i = 0; i <= UART_EXAR_REGB_EE_DATA_SIZE; i++) {
> > +		data <<= 1;
> > +		data |= exar_ee_read_bit(priv);
> > +	}
> > +
> > +	exar_ee_deselect(priv);
> > +
> > +	return data;
> > +}
> > +
> > +/**
> > + * exar_mpio_config_output() - Configure an Exar MPIO as an output
> > + * @priv: Device's private structure
> > + * @mpio_num: MPIO number/offset to configure
> > + *
> > + * Configure a single MPIO as an output and disable tristate. It is reccomended
> > + * to set the level with exar_mpio_set_high()/exar_mpio_set_low() prior to
> > + * calling this function to ensure default MPIO pin state.
> > + *
> > + * Return: 0 on success, negative error code on failure
> > + */
> > +static int exar_mpio_config_output(struct exar8250 *priv,
> > +				unsigned int mpio_num)
> > +{
> > +	unsigned int mpio_offset;
> > +	u8 sel_reg; // MPIO Select register (input/output)
> > +	u8 tri_reg; // MPIO Tristate register
> > +	u8 value;
> > +
> > +	if (mpio_num < 8) {
> > +		sel_reg = UART_EXAR_MPIOSEL_7_0;
> > +		tri_reg = UART_EXAR_MPIO3T_7_0;
> > +		mpio_offset = mpio_num;
> > +	} else if (mpio_num >= 8 && mpio_num < 16) {
> > +		sel_reg = UART_EXAR_MPIOSEL_15_8;
> > +		tri_reg = UART_EXAR_MPIO3T_15_8;
> > +		mpio_offset = mpio_num - 8;
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	// Disable MPIO pin tri-state
> > +	value = exar_read_reg(priv, tri_reg);
> > +	value &= ~BIT(mpio_offset);
> > +	exar_write_reg(priv, tri_reg, value);
> > +
> > +	value = exar_read_reg(priv, sel_reg);
> > +	value &= ~BIT(mpio_offset);
> > +	exar_write_reg(priv, sel_reg, value);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * _exar_mpio_set() - Set an Exar MPIO output high or low
> > + * @priv: Device's private structure
> > + * @mpio_num: MPIO number/offset to set
> > + * @high: Set MPIO high if true, low if false
> > + *
> > + * Set a single MPIO high or low. exar_mpio_config_output() must also be called
> > + * to configure the pin as an output.
> > + *
> > + * Return: 0 on success, negative error code on failure
> > + */
> > +static int _exar_mpio_set(struct exar8250 *priv,
> > +			unsigned int mpio_num, bool high)
> > +{
> > +	unsigned int mpio_offset;
> > +	u8 lvl_reg;
> > +	u8 value;
> > +
> > +	if (mpio_num < 8) {
> > +		lvl_reg = UART_EXAR_MPIOLVL_7_0;
> > +		mpio_offset = mpio_num;
> > +	} else if (mpio_num >= 8 && mpio_num < 16) {
> > +		lvl_reg = UART_EXAR_MPIOLVL_15_8;
> > +		mpio_offset = mpio_num - 8;
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	value = exar_read_reg(priv, lvl_reg);
> > +	if (high)
> > +		value |= BIT(mpio_offset);
> > +	else
> > +		value &= ~BIT(mpio_offset);
> > +	exar_write_reg(priv, lvl_reg, value);
> > +
> > +	return 0;
> > +}
> > +
> > +static int exar_mpio_set_low(struct exar8250 *priv, unsigned int mpio_num)
> > +{
> > +	return _exar_mpio_set(priv, mpio_num, false);
> > +}
> > +
> > +static int exar_mpio_set_high(struct exar8250 *priv, unsigned int mpio_num)
> > +{
> > +	return _exar_mpio_set(priv, mpio_num, true);
> > +}
> > +
> >  static int generic_rs485_config(struct uart_port *port, struct ktermios *termios,
> >  				struct serial_rs485 *rs485)
> >  {
> > @@ -385,6 +635,546 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> >  	return 0;
> >  }
> > 
> > +/**
> > + * cti_tristate_disable() - Disable RS485 transciever tristate
> > + * @priv: Device's private structure
> > + * @port_num: Port number to set tristate off
> > + *
> > + * Most RS485 capable cards have a power on tristate jumper/switch that ensures
> > + * the RS422/RS485 transciever does not drive a multi-drop RS485 bus when it is
> > + * not the master. When this jumper is installed the user must set the RS485
> > + * mode to Full or Half duplex to disable tristate prior to using the port.
> > + *
> > + * Some Exar UARTs have an auto-tristate feature while others require setting
> > + * an MPIO to disable the tristate.
> > + *
> > + * Return: 0 on success, negative error code on failure
> > + */
> > +static int cti_tristate_disable(struct exar8250 *priv, unsigned int port_num)
> > +{
> > +	int ret;
> > +
> > +	ret = exar_mpio_set_high(priv, port_num);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return exar_mpio_config_output(priv, port_num);
> > +}
> > +
> > +/**
> > + * cti_plx_int_enable() - Enable UART interrupts to PLX bridge
> > + * @priv: Device's private structure
> > + *
> > + * Some older CTI cards require MPIO_0 to be set low to enable the
> > + * interupts from the UART to the PLX PCI->PCIe bridge.
> > + *
> > + * Return: 0 on success, negative error code on failure
> > + */
> > +static int cti_plx_int_enable(struct exar8250 *priv)
> > +{
> > +	int ret;
> > +
> > +	ret = exar_mpio_set_low(priv, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return exar_mpio_config_output(priv, 0);
> > +}
> > +
> > +/**
> > + * cti_read_osc_freq() - Read the UART oscillator frequency from EEPROM
> > + * @priv: Device's private structure
> > + * @eeprom_offset: Offset where the oscillator frequency is stored
> > + *
> > + * CTI XR17x15X and XR17V25X cards have the serial boards oscillator frequency
> > + * stored in the EEPROM. FPGA and XR17V35X based cards use the PCI/PCIe clock.
> > + *
> > + * Return: frequency on success, negative error code on failure
> > + */
> > +static int cti_read_osc_freq(struct exar8250 *priv, u8 eeprom_offset)
> > +{
> > +	u16 lower_word;
> > +	u16 upper_word;
> > +	int osc_freq;
> > +
> > +	lower_word = exar_ee_read(priv, eeprom_offset);
> > +	// Check if EEPROM word was blank
> > +	if (lower_word == 0xFFFF)
> > +		return -EIO;
> > +
> > +	upper_word = exar_ee_read(priv, (eeprom_offset + 1));
> > +	if (upper_word == 0xFFFF)
> > +		return -EIO;
> > +
> > +	osc_freq = FIELD_PREP(CTI_EE_MASK_OSC_FREQ_LOWER, lower_word) |
> > +		FIELD_PREP(CTI_EE_MASK_OSC_FREQ_UPPER, upper_word);
> > +
> > +	return osc_freq;
> > +}
> > +
> > +/**
> > + * cti_get_port_type_xr17c15x_xr17v25x() - Get port type of xr17c15x/xr17v25x
> > + * @priv: Device's private structure
> > + * @port_num: Port to get type of
> > + *
> > + * CTI xr17c15x and xr17v25x based cards port types are based on PCI IDs.
> > + *
> > + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> > + */
> > +static enum cti_port_type cti_get_port_type_xr17c15x_xr17v25x(struct exar8250 *priv,
> > +							struct pci_dev *pcidev,
> > +							unsigned int port_num)
> > +{
> > +	enum cti_port_type port_type;
> > +
> > +	switch (pcidev->subsystem_device) {
> > +	// RS232 only cards
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_232:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_232:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_232:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232_NS:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232_NS:
> > +		port_type = CTI_PORT_TYPE_RS232;
> > +		break;
> > +	// 1x RS232, 1x RS422/RS485
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_1_1:
> > +		port_type = (port_num == 0) ?
> > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;  
> 
> I'd linesplit and align these differently:
> 
> 		port_type = port_num == 0 ? CTI_PORT_TYPE_RS232 :
> 					    CTI_PORT_TYPE_RS422_485;
> 
> > +		break;
> > +	// 2x RS232, 2x RS422/RS485
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_2:
> > +		port_type = (port_num < 2) ?
> > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > +		break;
> > +	// 4x RS232, 4x RS422/RS485
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
> > +		port_type = (port_num < 4) ?
> > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > +		break;
> > +	// RS232/RS422/RS485 HW (jumper) selectable
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_SP_OPTO:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
> > +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> > +		break;
> > +	// RS422/RS485 HW (jumper) selectable
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_485:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_485:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_485:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
> > +		port_type = CTI_PORT_TYPE_RS422_485;
> > +		break;
> > +	// 6x RS232, 2x RS422/RS485
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
> > +		port_type = (port_num < 6) ?
> > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > +		break;
> > +	// 2x RS232, 6x RS422/RS485
> > +	case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
> > +		port_type = (port_num < 2) ?
> > +			CTI_PORT_TYPE_RS232 : CTI_PORT_TYPE_RS422_485;
> > +		break;
> > +	default:
> > +		dev_err(&pcidev->dev, "unknown/unsupported device\n");
> > +		port_type = CTI_PORT_TYPE_NONE;
> > +	}
> > +
> > +	return port_type;
> > +}
> > +
> > +/**
> > + * cti_get_port_type_fpga() - Get the port type of a CTI FPGA card
> > + * @priv: Device's private structure
> > + * @port_num: Port to get type of
> > + *
> > + * FPGA based cards port types are based on PCI IDs.
> > + *
> > + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> > + */
> > +static enum cti_port_type cti_get_port_type_fpga(struct exar8250 *priv,
> > +						struct pci_dev *pcidev,
> > +						unsigned int port_num)
> > +{
> > +	enum cti_port_type port_type;
> > +
> > +	switch (pcidev->device) {
> > +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
> > +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
> > +	case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
> > +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> > +		break;
> > +	default:
> > +		dev_err(&pcidev->dev, "unknown/unsupported device\n");
> > +		return CTI_PORT_TYPE_NONE;
> > +	}
> > +
> > +	return port_type;
> > +}
> > +
> > +/**
> > + * cti_get_port_type_xr17v35x() - Read port type from the EEPROM
> > + * @priv: Device's private structure
> > + * @port_num: port offset
> > + *
> > + * CTI XR17V35X based cards have the port types stored in the EEPROM.
> > + * This function reads the port type for a single port.
> > + *
> > + * Return: port type on success, CTI_PORT_TYPE_NONE on failure
> > + */
> > +static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
> > +						struct pci_dev *pcidev,
> > +						unsigned int port_num)
> > +{
> > +	enum cti_port_type port_type;
> > +	u16 port_flags;
> > +	u8 offset;
> > +
> > +	offset = CTI_EE_OFF_XR17V35X_PORT_FLAGS + port_num;
> > +	port_flags = exar_ee_read(priv, offset);
> > +
> > +	port_type = FIELD_GET(CTI_EE_MASK_PORT_FLAGS_TYPE, port_flags);
> > +	if (!CTI_PORT_TYPE_VALID(port_type)) {
> > +		/*
> > +		 * If the port type is missing the card assume it is a
> > +		 * RS232/RS422/RS485 card to be safe.
> > +		 *
> > +		 * There is one known board (BEG013) that only has
> > +		 * 3 of 4 port types written to the EEPROM so this
> > +		 * acts as a work around.
> > +		 */
> > +		dev_warn(&pcidev->dev,
> > +			"failed to get port %d type from EEPROM\n", port_num);
> > +		port_type = CTI_PORT_TYPE_RS232_422_485_HW;
> > +	}
> > +
> > +	return port_type;
> > +}
> > +
> > +static int cti_rs485_config_mpio_tristate(struct uart_port *port,
> > +					struct ktermios *termios,
> > +					struct serial_rs485 *rs485)
> > +{
> > +	struct exar8250 *priv = (struct exar8250 *)port->private_data;
> > +	int ret;
> > +
> > +	ret = generic_rs485_config(port, termios, rs485);
> > +	if (ret)
> > +		return ret;
> > +
> > +	// Disable power-on RS485 tri-state via MPIO
> > +	return cti_tristate_disable(priv, port->port_id);
> > +}
> > +
> > +static int cti_port_setup_common(struct exar8250 *priv,
> > +				struct pci_dev *pcidev,
> > +				int idx, unsigned int offset,
> > +				struct uart_8250_port *port)
> > +{
> > +	int ret;
> > +
> > +	if (priv->osc_freq == 0)
> > +		return -EINVAL;
> > +
> > +	port->port.port_id = idx;
> > +	port->port.uartclk = priv->osc_freq;
> > +
> > +	ret = serial8250_pci_setup_port(pcidev, port, 0, offset, 0);
> > +	if (ret) {
> > +		dev_err(&pcidev->dev,
> > +			"failed to setup pci for port %d err: %d\n", idx, ret);
> > +		return ret;
> > +	}
> > +
> > +	port->port.private_data = (void *)priv;
> > +	port->port.pm = exar_pm;
> > +	port->port.shutdown = exar_shutdown;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cti_port_setup_fpga(struct exar8250 *priv,
> > +				struct pci_dev *pcidev,
> > +				struct uart_8250_port *port,
> > +				int idx)
> > +{
> > +	enum cti_port_type port_type;
> > +	unsigned int offset;
> > +
> > +	port_type = cti_get_port_type_fpga(priv, pcidev, idx);
> > +
> > +	// FPGA shares port offests with XR17C15X
> > +	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
> > +	port->port.type = PORT_XR17D15X;
> > +
> > +	port->port.get_divisor = xr17v35x_get_divisor;
> > +	port->port.set_divisor = xr17v35x_set_divisor;
> > +	port->port.startup = xr17v35x_startup;
> > +
> > +	if (CTI_PORT_TYPE_RS485(port_type)) {
> > +		port->port.rs485_config = generic_rs485_config;
> > +		port->port.rs485_supported = generic_rs485_supported;
> > +	}
> > +
> > +	return cti_port_setup_common(priv, pcidev, idx, offset, port);
> > +}
> > +
> > +static int cti_port_setup_xr17v35x(struct exar8250 *priv,
> > +				struct pci_dev *pcidev,
> > +				struct uart_8250_port *port,
> > +				int idx)
> > +{
> > +	enum cti_port_type port_type;
> > +	unsigned int offset;
> > +	int ret;
> > +
> > +	port_type = cti_get_port_type_xr17v35x(priv, pcidev, idx);
> > +
> > +	offset = idx * UART_EXAR_XR17V35X_PORT_OFFSET;
> > +	port->port.type = PORT_XR17V35X;
> > +
> > +	port->port.get_divisor = xr17v35x_get_divisor;
> > +	port->port.set_divisor = xr17v35x_set_divisor;
> > +	port->port.startup = xr17v35x_startup;
> > +
> > +	switch (port_type) {
> > +	case CTI_PORT_TYPE_RS422_485:
> > +	case CTI_PORT_TYPE_RS232_422_485_HW:
> > +		port->port.rs485_config = cti_rs485_config_mpio_tristate;
> > +		port->port.rs485_supported = generic_rs485_supported;
> > +		break;
> > +	case CTI_PORT_TYPE_RS232_422_485_SW:
> > +	case CTI_PORT_TYPE_RS232_422_485_4B:
> > +	case CTI_PORT_TYPE_RS232_422_485_2B:
> > +		port->port.rs485_config = generic_rs485_config;
> > +		port->port.rs485_supported = generic_rs485_supported;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	ret = cti_port_setup_common(priv, pcidev, idx, offset, port);
> > +	if (ret)
> > +		return ret;
> > +
> > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 128);
> > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 128);  
> 
> Unnecessary parenthesis.
> 
> > +	return 0;
> > +}
> > +
> > +static int cti_port_setup_xr17v25x(struct exar8250 *priv,
> > +				struct pci_dev *pcidev,
> > +				struct uart_8250_port *port,
> > +				int idx)
> > +{
> > +	enum cti_port_type port_type;
> > +	unsigned int offset;
> > +	int ret;
> > +
> > +	port_type = cti_get_port_type_xr17c15x_xr17v25x(priv, pcidev, idx);
> > +
> > +	offset = idx * UART_EXAR_XR17V25X_PORT_OFFSET;
> > +	port->port.type = PORT_XR17D15X;
> > +
> > +	// XR17V25X supports fractional baudrates
> > +	port->port.get_divisor = xr17v35x_get_divisor;
> > +	port->port.set_divisor = xr17v35x_set_divisor;
> > +	port->port.startup = xr17v35x_startup;
> > +
> > +	if (CTI_PORT_TYPE_RS485(port_type)) {
> > +		switch (pcidev->subsystem_device) {
> > +		// These cards support power on 485 tri-state via MPIO
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP:
> > +		case PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485:
> > +			port->port.rs485_config = cti_rs485_config_mpio_tristate;
> > +			break;
> > +		// Otherwise auto or no power on 485 tri-state support
> > +		default:
> > +			port->port.rs485_config = generic_rs485_config;
> > +			break;
> > +		}
> > +
> > +		port->port.rs485_supported = generic_rs485_supported;
> > +	}
> > +
> > +	ret = cti_port_setup_common(priv, pcidev, idx, offset, port);
> > +	if (ret)
> > +		return ret;
> > +
> > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 32);
> > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 32);  
> 
> Unnecessary parenthesis.
> 

I will fix these in my cleanup patches. 

> I recommend you add a helper for this as it is repeated twice. Are the 
> values 32 and 128 literal or do they have some specific meaning? If the 
> latter case, they should be using named defines (this likely applies to 
> the existing trigger code in the driver too).
> 
> 

They are the FIFO trigger levels so they are literally 128 and 32. 

These 4 writes come from Exar's out-of-tree driver and are in 
pci_xr17v35x_setup() and some other vendor specific functions. 

I am not sure why/if these are needed. 

Re: [PATCH v4 6/7] serial: exar: add CTI specific setup code

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4014 bytes --]

On Thu, 18 Apr 2024, Parker Newman wrote:
> On Thu, 18 Apr 2024 16:20:15 +0300 (EEST)
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > On Wed, 17 Apr 2024, Parker Newman wrote:
> > > From: Parker Newman <pnewman@connecttech.com>
> > > 
> > > This is a large patch but is only additions. All changes and removals
> > > are made in previous patches in this series.
> > > 
> > > - Add CTI board_init and port setup functions for each UART type
> > > - Add CTI_EXAR_DEVICE() and CTI_PCI_DEVICE() macros
> > > - Add support for reading a word from the Exar EEPROM.
> > > - Add support for configuring and setting a single MPIO
> > > - Add various helper functions for CTI boards.
> > > - Add osc_freq to struct exar8250
> > > 
> > > Signed-off-by: Parker Newman <pnewman@connecttech.com>

> > > @@ -192,11 +252,201 @@ struct exar8250_board {
> > > 
> > >  struct exar8250 {
> > >  	unsigned int		nr;
> > > +	unsigned int		osc_freq;
> > >  	struct exar8250_board	*board;
> > >  	void __iomem		*virt;
> > >  	int			line[];
> > >  };
> > > 
> > > +static inline void exar_write_reg(struct exar8250 *priv,
> > > +				unsigned int reg, u8 value)
> > > +{
> > > +	writeb(value, priv->virt + reg);
> > > +}
> > > +
> > > +static inline u8 exar_read_reg(struct exar8250 *priv, unsigned int reg)
> > > +{
> > > +	return readb(priv->virt + reg);
> > > +}  
> > 
> > I tried to understand what is going on with this priv->virt in 8250_exar 
> > in general and why it exists but I failed. It seems to BAR0 is mapped 
> > there but also serial8250_pci_setup_port() does map the same BAR and 
> > sets it up into the usual place in membase.
> > 
> 
> Exar PCI/PCIe UARTs have global configuration registers from 0x80-0x9B.
> These registers are for reading the EEPROM, configuring the MPIO, etc.
> As these registers are only at 0x80, and not port specific, the driver maps
> BAR0 to priv->virt for accessing them. 

Okay, thanks for explaining. The naming & lack of comments wasn't exactly 
making it easy to follow this bit (this is not your fault in anyway but 
a pre-existing problem in the driver's code).

I've a follow up question now that it's confirmed they're different, 
see below...

> > > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 128);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 128);  
> > 
> > Unnecessary parenthesis.

> > > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 32);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 32);  
> > 
> > Unnecessary parenthesis.
> > 
> 
> I will fix these in my cleanup patches. 

Based on the wording in your response, I'm not sure you got this right. It 
is code you're adding in this patch so the parenthesis should be removed 
from this change so they never appear in the commit history.

> > I recommend you add a helper for this as it is repeated twice. Are the 
> > values 32 and 128 literal or do they have some specific meaning? If the 
> > latter case, they should be using named defines (this likely applies to 
> > the existing trigger code in the driver too).
> > 
> > 
> 
> They are the FIFO trigger levels so they are literally 128 and 32. 

Okay, no problem then if its 128 characters and 32 characters.

> These 4 writes come from Exar's out-of-tree driver and are in 
> pci_xr17v35x_setup() and some other vendor specific functions. 
> 
> I am not sure why/if these are needed. 

...So the follow-up question. I see the existing code in 
pci_fastcom335_setup() and pci_xr17v35x_setup() writes into membase 
based address but your code uses exar_write_reg() which is priv->virt 
based. Is this difference intentional?

-- 
 i.

Re: [PATCH v4 6/7] serial: exar: add CTI specific setup code

[Parker Newman]

On Thu, 18 Apr 2024 19:29:44 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 18 Apr 2024, Parker Newman wrote:
> > On Thu, 18 Apr 2024 16:20:15 +0300 (EEST)
> > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> >   
> > > On Wed, 17 Apr 2024, Parker Newman wrote:  
> > > > From: Parker Newman <pnewman@connecttech.com>
> > > > 
> > > > This is a large patch but is only additions. All changes and removals
> > > > are made in previous patches in this series.
> > > > 
> > > > - Add CTI board_init and port setup functions for each UART type
> > > > - Add CTI_EXAR_DEVICE() and CTI_PCI_DEVICE() macros
> > > > - Add support for reading a word from the Exar EEPROM.
> > > > - Add support for configuring and setting a single MPIO
> > > > - Add various helper functions for CTI boards.
> > > > - Add osc_freq to struct exar8250
> > > > 
> > > > Signed-off-by: Parker Newman <pnewman@connecttech.com>  
> 
> > > > @@ -192,11 +252,201 @@ struct exar8250_board {
> > > > 
> > > >  struct exar8250 {
> > > >  	unsigned int		nr;
> > > > +	unsigned int		osc_freq;
> > > >  	struct exar8250_board	*board;
> > > >  	void __iomem		*virt;
> > > >  	int			line[];
> > > >  };
> > > > 
> > > > +static inline void exar_write_reg(struct exar8250 *priv,
> > > > +				unsigned int reg, u8 value)
> > > > +{
> > > > +	writeb(value, priv->virt + reg);
> > > > +}
> > > > +
> > > > +static inline u8 exar_read_reg(struct exar8250 *priv, unsigned int reg)
> > > > +{
> > > > +	return readb(priv->virt + reg);
> > > > +}    
> > > 
> > > I tried to understand what is going on with this priv->virt in 8250_exar 
> > > in general and why it exists but I failed. It seems to BAR0 is mapped 
> > > there but also serial8250_pci_setup_port() does map the same BAR and 
> > > sets it up into the usual place in membase.
> > >   
> > 
> > Exar PCI/PCIe UARTs have global configuration registers from 0x80-0x9B.
> > These registers are for reading the EEPROM, configuring the MPIO, etc.
> > As these registers are only at 0x80, and not port specific, the driver maps
> > BAR0 to priv->virt for accessing them.   
> 
> Okay, thanks for explaining. The naming & lack of comments wasn't exactly 
> making it easy to follow this bit (this is not your fault in anyway but 
> a pre-existing problem in the driver's code).
> 
> I've a follow up question now that it's confirmed they're different, 
> see below...
> 
> > > > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > > > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > > > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 128);
> > > > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 128);    
> > > 
> > > Unnecessary parenthesis.  
> 
> > > > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > > > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > > > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 32);
> > > > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 32);    
> > > 
> > > Unnecessary parenthesis.
> > >   
> > 
> > I will fix these in my cleanup patches.   
> 
> Based on the wording in your response, I'm not sure you got this right. It 
> is code you're adding in this patch so the parenthesis should be removed 
> from this change so they never appear in the commit history.
> 

Greg has already merged this series into his testing branch. I assumed any
changes would need to be made in a separate patch series? Sorry if I 
misunderstood. I already sent these fixes in a new mini-series. 

> > > I recommend you add a helper for this as it is repeated twice. Are the 
> > > values 32 and 128 literal or do they have some specific meaning? If the 
> > > latter case, they should be using named defines (this likely applies to 
> > > the existing trigger code in the driver too).
> > > 
> > >   
> > 
> > They are the FIFO trigger levels so they are literally 128 and 32.   
> 
> Okay, no problem then if its 128 characters and 32 characters.
> 
> > These 4 writes come from Exar's out-of-tree driver and are in 
> > pci_xr17v35x_setup() and some other vendor specific functions. 
> > 
> > I am not sure why/if these are needed.   
> 
> ...So the follow-up question. I see the existing code in 
> pci_fastcom335_setup() and pci_xr17v35x_setup() writes into membase 
> based address but your code uses exar_write_reg() which is priv->virt 
> based. Is this difference intentional?
> 

Both methods are effectively the same thing. I used exar_write_reg() to be
consistent with my other code and it is a bit cleaner than:

	u8 __iomem *p;

	p = port->port.membase;

	writeb(0x00, p + UART_EXAR_8XMODE);

Re: [PATCH v4 6/7] serial: exar: add CTI specific setup code

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5612 bytes --]

On Thu, 18 Apr 2024, Parker Newman wrote:

> On Thu, 18 Apr 2024 19:29:44 +0300 (EEST)
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > On Thu, 18 Apr 2024, Parker Newman wrote:
> > > On Thu, 18 Apr 2024 16:20:15 +0300 (EEST)
> > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > >   
> > > > On Wed, 17 Apr 2024, Parker Newman wrote:  
> > > > > From: Parker Newman <pnewman@connecttech.com>
> > > > > 
> > > > > This is a large patch but is only additions. All changes and removals
> > > > > are made in previous patches in this series.
> > > > > 
> > > > > - Add CTI board_init and port setup functions for each UART type
> > > > > - Add CTI_EXAR_DEVICE() and CTI_PCI_DEVICE() macros
> > > > > - Add support for reading a word from the Exar EEPROM.
> > > > > - Add support for configuring and setting a single MPIO
> > > > > - Add various helper functions for CTI boards.
> > > > > - Add osc_freq to struct exar8250
> > > > > 
> > > > > Signed-off-by: Parker Newman <pnewman@connecttech.com>  
> > 
> > > > > @@ -192,11 +252,201 @@ struct exar8250_board {
> > > > > 
> > > > >  struct exar8250 {
> > > > >  	unsigned int		nr;
> > > > > +	unsigned int		osc_freq;
> > > > >  	struct exar8250_board	*board;
> > > > >  	void __iomem		*virt;
> > > > >  	int			line[];
> > > > >  };
> > > > > 
> > > > > +static inline void exar_write_reg(struct exar8250 *priv,
> > > > > +				unsigned int reg, u8 value)
> > > > > +{
> > > > > +	writeb(value, priv->virt + reg);
> > > > > +}
> > > > > +
> > > > > +static inline u8 exar_read_reg(struct exar8250 *priv, unsigned int reg)
> > > > > +{
> > > > > +	return readb(priv->virt + reg);
> > > > > +}    
> > > > 
> > > > I tried to understand what is going on with this priv->virt in 8250_exar 
> > > > in general and why it exists but I failed. It seems to BAR0 is mapped 
> > > > there but also serial8250_pci_setup_port() does map the same BAR and 
> > > > sets it up into the usual place in membase.
> > > >   
> > > 
> > > Exar PCI/PCIe UARTs have global configuration registers from 0x80-0x9B.
> > > These registers are for reading the EEPROM, configuring the MPIO, etc.
> > > As these registers are only at 0x80, and not port specific, the driver maps
> > > BAR0 to priv->virt for accessing them.   
> > 
> > Okay, thanks for explaining. The naming & lack of comments wasn't exactly 
> > making it easy to follow this bit (this is not your fault in anyway but 
> > a pre-existing problem in the driver's code).
> > 
> > I've a follow up question now that it's confirmed they're different, 
> > see below...
> > 
> > > > > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > > > > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > > > > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 128);
> > > > > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 128);    
> > > > 
> > > > Unnecessary parenthesis.  
> > 
> > > > > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > > > > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > > > > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 32);
> > > > > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 32);    
> > > > 
> > > > Unnecessary parenthesis.
> > > >   
> > > 
> > > I will fix these in my cleanup patches.   
> > 
> > Based on the wording in your response, I'm not sure you got this right. It 
> > is code you're adding in this patch so the parenthesis should be removed 
> > from this change so they never appear in the commit history.
> > 
> 
> Greg has already merged this series into his testing branch. I assumed any
> changes would need to be made in a separate patch series? Sorry if I 
> misunderstood. I already sent these fixes in a new mini-series. 

Okay. I was only aware he took the first patch but seemingly he took all 
of them. So you're correct then, ignore my advice which was based on 
incomplete picture.

> > > > I recommend you add a helper for this as it is repeated twice. Are the 
> > > > values 32 and 128 literal or do they have some specific meaning? If the 
> > > > latter case, they should be using named defines (this likely applies to 
> > > > the existing trigger code in the driver too).
> > > > 
> > > >   
> > > 
> > > They are the FIFO trigger levels so they are literally 128 and 32.   
> > 
> > Okay, no problem then if its 128 characters and 32 characters.
> > 
> > > These 4 writes come from Exar's out-of-tree driver and are in 
> > > pci_xr17v35x_setup() and some other vendor specific functions. 
> > > 
> > > I am not sure why/if these are needed.   
> > 
> > ...So the follow-up question. I see the existing code in 
> > pci_fastcom335_setup() and pci_xr17v35x_setup() writes into membase 
> > based address but your code uses exar_write_reg() which is priv->virt 
> > based. Is this difference intentional?
> > 
> 
> Both methods are effectively the same thing. I used exar_write_reg() to be
> consistent with my other code and it is a bit cleaner than:

Those people used to serial subsystem, using membase is way more familiar 
than using some driver private thing.

> 	u8 __iomem *p;
> 
> 	p = port->port.membase;
> 
> 	writeb(0x00, p + UART_EXAR_8XMODE);

It's inconsistent with the other two uses now but since you told they're 
effectively the same thing, I was hoping all of 4 places in 8250_exar 
would be converted to use a helper that is just given the threshold as 
parameter (the helper doesn't exist yet).

-- 
 i.