[PATCH v3 0/8] serial: exar: add Connect Tech serial cards to Exar driver

[PATCH v3 0/8] 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

Thank you,

Parker Newman (8):
  serial: exar: adding missing CTI and Exar PCI ids
  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: fix crash during shutdown if setup fails

 drivers/tty/serial/8250/8250_exar.c | 1079 +++++++++++++++++++++++++--
 1 file changed, 1013 insertions(+), 66 deletions(-)


base-commit: fec50db7033ea478773b159e0e2efb135270e3b7
--
2.43.2

[PATCH v3 1/8] serial: exar: adding missing CTI and Exar PCI ids

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

- Added Connect Tech and Exar IDs not already in pci_ids.h

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 42 +++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 0440df7de1ed..4d1e07343d0b 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -46,8 +46,50 @@
 #define PCI_DEVICE_ID_COMMTECH_4228PCIE		0x0021
 #define PCI_DEVICE_ID_COMMTECH_4222PCIE		0x0022

+#define PCI_VENDOR_ID_CONNECT_TECH				0x12c4
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_SP_OPTO        0x0340
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_A      0x0341
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_SP_OPTO_B      0x0342
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS           0x0350
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_A         0x0351
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_B         0x0352
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS           0x0353
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_A        0x0354
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_16_XPRS_B        0x0355
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XPRS_OPTO      0x0360
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_A    0x0361
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XPRS_OPTO_B    0x0362
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP             0x0370
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232         0x0371
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_485         0x0372
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_SP           0x0373
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_6_2_SP           0x0374
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_6_SP           0x0375
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_SP_232_NS      0x0376
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_LEFT   0x0380
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_XP_OPTO_RIGHT  0x0381
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_XP_OPTO        0x0382
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_4_XPRS_OPTO    0x0392
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP        0x03A0
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232    0x03A1
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_485    0x03A2
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_8_XPRS_LP_232_NS 0x03A3
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XEG001               0x0602
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_BASE           0x1000
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_2              0x1002
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_4              0x1004
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_8              0x1008
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_12             0x100C
+#define PCI_SUBDEVICE_ID_CONNECT_TECH_PCIE_XR35X_16             0x1010
+#define PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X          0x110c
+#define PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X          0x110d
+#define PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16                 0x1110
+
 #define PCI_DEVICE_ID_EXAR_XR17V4358		0x4358
 #define PCI_DEVICE_ID_EXAR_XR17V8358		0x8358
+#define PCI_DEVICE_ID_EXAR_XR17V252		0x0252
+#define PCI_DEVICE_ID_EXAR_XR17V254		0x0254
+#define PCI_DEVICE_ID_EXAR_XR17V258		0x0258

 #define PCI_SUBDEVICE_ID_USR_2980		0x0128
 #define PCI_SUBDEVICE_ID_USR_2981		0x0129
--
2.43.2

Re: [PATCH v3 1/8] serial: exar: adding missing CTI and Exar PCI ids

[Greg Kroah-Hartman]

On Tue, Apr 16, 2024 at 08:55:28AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> - Added Connect Tech and Exar IDs not already in pci_ids.h
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 42 +++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)

I took this one now as it's "obviously" correct :)

thanks,

greg k-h

[PATCH v3 2/8] 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.

Changes in v3:
- Split code removals to own patch

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 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 4d1e07343d0b..3565b880f512 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -357,17 +357,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)
@@ -848,10 +837,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,
@@ -896,15 +881,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) {			\
@@ -934,19 +910,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

Re: [PATCH v3 2/8] serial: exar: remove old Connect Tech setup

[Greg Kroah-Hartman]

On Tue, Apr 16, 2024 at 08:55:29AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> Preparatory patch removing existing Connect Tech setup code and
> CONNECT_DEVICE macro.

Preparatory for what?  You are removing support here, right?  And will
you be adding it back later?

thanks,

greg k-h

[PATCH v3 3/8] 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.

Chnages in v3:
- Only moved existing code in this patch, will add CTI code in subsequent
  patch

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 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 3565b880f512..388dd60ad23a 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -703,6 +703,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)
 {
@@ -722,12 +737,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

Re: [PATCH v3 3/8] serial: exar: added a exar_get_nr_ports function

[Greg Kroah-Hartman]

On Tue, Apr 16, 2024 at 08:55:30AM -0400, 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.
> 
> Chnages in v3:
> - Only moved existing code in this patch, will add CTI code in subsequent
>   patch

Nit, the "changes" need to go below the --- line, otherwise it shows up
in the change logs.

So when you resend this, can you put these below the --- line please?

thanks,

greg k-h

Re: [PATCH v3 3/8] serial: exar: added a exar_get_nr_ports function

[Parker Newman]

On Wed, 17 Apr 2024 13:15:41 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Tue, Apr 16, 2024 at 08:55:30AM -0400, 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.
> >
> > Chnages in v3:
> > - Only moved existing code in this patch, will add CTI code in subsequent
> >   patch
>
> Nit, the "changes" need to go below the --- line, otherwise it shows up
> in the change logs.
>
> So when you resend this, can you put these below the --- line please?
>

Yes sorry see that note online now. Will do in next version.

Thanks,
Parker

> thanks,
>
> greg k-h

[PATCH v3 4/8] 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.

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

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

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

Changes in v3:
 - Renamed board_setup to board_init.
 - Changed pci_err to dev_err_probe
 - Added note above about checkpatch fixes

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 388dd60ad23a..cf7900bd2974 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -133,7 +133,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>
@@ -169,22 +169,24 @@ 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);
 };

 /**
  * 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	(*setup)(struct exar8250 *, struct pci_dev *,
-			 struct uart_8250_port *, int);
+	int     (*board_init)(struct exar8250 *priv);
+	int	(*setup)(struct exar8250 *priv, struct pci_dev *pcidev,
+			 struct uart_8250_port *port, int idx);
 	void	(*exit)(struct pci_dev *pcidev);
 };

@@ -772,6 +774,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);
+		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

Re: [PATCH v3 4/8] serial: exar: add optional board_init function

[Greg Kroah-Hartman]

On Tue, Apr 16, 2024 at 08:55:31AM -0400, 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.
> 
> - Fix several "missing identifier name" warnings from checkpatch in
> struct exar8250_platform and struct exar8250_board:
> 
> WARNING: function definition argument <arg> should also have an
> identifier name
> 
> - Fix warning from checkpatch:
> WARNING: please, no space before tabs
> + * 0^I^I2 ^IMode bit 0$
> 
> Changes in v3:
>  - Renamed board_setup to board_init.
>  - Changed pci_err to dev_err_probe
>  - Added note above about checkpatch fixes
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 388dd60ad23a..cf7900bd2974 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -133,7 +133,7 @@
>   *
>   * MPIO		Port	Function
>   * ----		----	--------
> - * 0		2 	Mode bit 0
> + * 0		2	Mode bit 0

This change isn't related to the exar8250_board change, right?

Just keep exar8250_board change as one patch, and the coding style
warning fixups as one for the very end, after you add all of your new
functionality.

thanks,

greg k-h

[PATCH v3 5/8] 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 it.

Changes in v3:
- split into separate preparatory patch

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 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 cf7900bd2974..7e47a4145c7b 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)
 {
 	/*
@@ -458,27 +483,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)
 {
@@ -517,10 +521,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 v3 6/8] 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()

Changes in v3:
- moved to separate patch
- added spaces to single line comments

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 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 7e47a4145c7b..ada01c6394a3 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -710,12 +710,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 v3 7/8] 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, pcidev, dev to struct exar8250

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

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 899 ++++++++++++++++++++++++++++
 1 file changed, 899 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index ada01c6394a3..501b9f3e9c89 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -20,6 +20,8 @@
 #include <linux/property.h>
 #include <linux/string.h>
 #include <linux/types.h>
+#include <linux/string_choices.h>
+#include <linux/bitfield.h>

 #include <linux/serial_8250.h>
 #include <linux/serial_core.h>
@@ -128,6 +130,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 +178,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 +253,214 @@ struct exar8250_board {

 struct exar8250 {
 	unsigned int		nr;
+	unsigned int		osc_freq;
+	struct pci_dev		*pcidev;
+	struct device		*dev;
 	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() - Configure an Exar MPIO as input or output
+ * @priv: Device's private structure
+ * @mpio_num: MPIO number/offset to configure
+ * @output: Configure as output if true, inout if false
+ *
+ * Configure a single MPIO as an input or output and disable tristate.
+ * If configuring as output it is reccomended to set value 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(struct exar8250 *priv,
+			unsigned int mpio_num, bool output)
+{
+	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);
+	if (output)
+		value &= ~BIT(mpio_offset);
+	else
+		value |= BIT(mpio_offset);
+	exar_write_reg(priv, sel_reg, value);
+
+	return 0;
+}
+
+static int exar_mpio_config_output(struct exar8250 *priv,
+				unsigned int mpio_num)
+{
+	return _exar_mpio_config(priv, mpio_num, true);
+}
+
+/**
+ * _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)
 {
@@ -384,6 +648,582 @@ pci_fastcom335_setup(struct exar8250 *priv, struct pci_dev *pcidev,
 	return 0;
 }

+/**
+ * _cti_set_tristate() - Enable/Disable RS485 transciever tristate
+ * @priv: Device's private structure
+ * @port_num: Port number to set tristate on/off
+ * @enable: Enable tristate if true, disable if false
+ *
+ * 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 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_set_tristate(struct exar8250 *priv,
+			unsigned int port_num, bool enable)
+{
+	int ret = 0;
+
+	if (port_num >= priv->nr)
+		return -EINVAL;
+
+	// Only Exar based cards use MPIO, return 0 otherwise
+	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
+		return 0;
+
+	dev_dbg(priv->dev, "%s tristate for port %u\n",
+		str_enable_disable(enable), port_num);
+
+	if (enable)
+		ret = exar_mpio_set_low(priv, port_num);
+	else
+		ret = exar_mpio_set_high(priv, port_num);
+	if (ret)
+		return ret;
+
+	// Ensure MPIO is an output
+	ret = exar_mpio_config_output(priv, port_num);
+
+	return ret;
+}
+
+static int cti_tristate_disable(struct exar8250 *priv, unsigned int port_num)
+{
+	return _cti_set_tristate(priv, port_num, false);
+}
+
+/**
+ * _cti_set_plx_int_enable() - Enable/Disable PCI interrupts
+ * @priv: Device's private structure
+ * @enable: Enable interrupts if true, disable if false
+ *
+ * Some older CTI cards require MPIO_0 to be set low to enable the PCI
+ * interupts from the UART to the PLX PCI->PCIe bridge.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int _cti_set_plx_int_enable(struct exar8250 *priv, bool enable)
+{
+	int ret = 0;
+
+	// Only Exar based cards use MPIO, return 0 otherwise
+	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
+		return 0;
+
+	if (enable)
+		ret = exar_mpio_set_low(priv, 0);
+	else
+		ret = exar_mpio_set_high(priv, 0);
+	if (ret)
+		return ret;
+
+	// Ensure MPIO is an output
+	ret = exar_mpio_config_output(priv, 0);
+
+	return ret;
+}
+
+static int cti_plx_int_enable(struct exar8250 *priv)
+{
+	return _cti_set_plx_int_enable(priv, true);
+}
+
+/**
+ * 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);
+
+	dev_dbg(priv->dev, "osc_freq from EEPROM %d\n", osc_freq);
+
+	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,
+							unsigned int port_num)
+{
+	enum cti_port_type port_type;
+
+	switch (priv->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(priv->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,
+						unsigned int port_num)
+{
+	enum cti_port_type port_type;
+
+	switch (priv->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(priv->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,
+						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(priv->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,
+				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(priv->pcidev, port, 0, offset, 0);
+	if (ret) {
+		dev_err(priv->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, 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, 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, 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, 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, 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 (priv->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, 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, idx);
+
+	offset = idx * UART_EXAR_XR17C15X_PORT_OFFSET;
+	port->port.type = PORT_XR17D15X;
+
+	if (CTI_PORT_TYPE_RS485(port_type)) {
+		switch (priv->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, idx, offset, port);
+}
+
+static int cti_board_init_xr17v35x(struct exar8250 *priv)
+{
+	// 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)
+{
+	int osc_freq;
+
+	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17V25X_OSC_FREQ);
+	if (osc_freq < 0) {
+		dev_warn(priv->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 (priv->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)
+{
+	int osc_freq;
+
+	osc_freq = cti_read_osc_freq(priv, CTI_EE_OFF_XR17C15X_OSC_FREQ);
+	if (osc_freq <= 0) {
+		dev_warn(priv->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 (priv->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)
+{
+	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(priv->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(priv->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)
@@ -767,6 +1607,8 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 		return -ENOMEM;

 	priv->board = board;
+	priv->pcidev = pcidev;
+	priv->dev = &pcidev->dev;
 	priv->virt = pcim_iomap(pcidev, bar, 0);
 	if (!priv->virt)
 		return -ENOMEM;
@@ -879,6 +1721,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,
@@ -923,6 +1785,27 @@ 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) {			\
@@ -952,6 +1835,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

Re: [PATCH v3 7/8] serial: exar: add CTI specific setup code

[Greg Kroah-Hartman]

On Tue, Apr 16, 2024 at 08:55:34AM -0400, Parker Newman wrote:
>  struct exar8250 {
>  	unsigned int		nr;
> +	unsigned int		osc_freq;
> +	struct pci_dev		*pcidev;
> +	struct device		*dev;

Why do you need both a pci_dev and a device?  Aren't they the same thing
here?

> +/**
> + * _cti_set_tristate() - Enable/Disable RS485 transciever tristate
> + * @priv: Device's private structure
> + * @port_num: Port number to set tristate on/off
> + * @enable: Enable tristate if true, disable if false
> + *
> + * 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 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_set_tristate(struct exar8250 *priv,
> +			unsigned int port_num, bool enable)
> +{
> +	int ret = 0;
> +
> +	if (port_num >= priv->nr)
> +		return -EINVAL;
> +
> +	// Only Exar based cards use MPIO, return 0 otherwise
> +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> +		return 0;

How can this ever happen?  Only the exar devices will call this
function, or am I missing a path here?



> +
> +	dev_dbg(priv->dev, "%s tristate for port %u\n",
> +		str_enable_disable(enable), port_num);
> +
> +	if (enable)
> +		ret = exar_mpio_set_low(priv, port_num);
> +	else
> +		ret = exar_mpio_set_high(priv, port_num);
> +	if (ret)
> +		return ret;
> +
> +	// Ensure MPIO is an output
> +	ret = exar_mpio_config_output(priv, port_num);
> +
> +	return ret;
> +}
> +
> +static int cti_tristate_disable(struct exar8250 *priv, unsigned int port_num)
> +{
> +	return _cti_set_tristate(priv, port_num, false);
> +}

Do you ever call _cti_set_tristate() with "true"?

> +
> +/**
> + * _cti_set_plx_int_enable() - Enable/Disable PCI interrupts
> + * @priv: Device's private structure
> + * @enable: Enable interrupts if true, disable if false

But false is never used here, so why have this at all?

> + *
> + * Some older CTI cards require MPIO_0 to be set low to enable the PCI
> + * interupts from the UART to the PLX PCI->PCIe bridge.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int _cti_set_plx_int_enable(struct exar8250 *priv, bool enable)
> +{
> +	int ret = 0;
> +
> +	// Only Exar based cards use MPIO, return 0 otherwise
> +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> +		return 0;

Same question here.

> +
> +	if (enable)
> +		ret = exar_mpio_set_low(priv, 0);
> +	else
> +		ret = exar_mpio_set_high(priv, 0);
> +	if (ret)
> +		return ret;
> +
> +	// Ensure MPIO is an output
> +	ret = exar_mpio_config_output(priv, 0);
> +
> +	return ret;
> +}
> +
> +static int cti_plx_int_enable(struct exar8250 *priv)
> +{
> +	return _cti_set_plx_int_enable(priv, true);

Again, no wrapper needed if you never actually call that function with
"false", right?  Or am I missing a path here?

thanks,

greg k-h

Re: [PATCH v3 7/8] serial: exar: add CTI specific setup code

[Parker Newman]

On Wed, 17 Apr 2024 13:24:20 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Tue, Apr 16, 2024 at 08:55:34AM -0400, Parker Newman wrote:
> >  struct exar8250 {
> >  	unsigned int		nr;
> > +	unsigned int		osc_freq;
> > +	struct pci_dev		*pcidev;
> > +	struct device		*dev;
>
> Why do you need both a pci_dev and a device?  Aren't they the same thing
> here?
>

I added dev to make the prints cleaner. I personally prefer:

	dev_err(priv->dev, ...);
to
	dev_err(&priv->pcidev->dev, ...);

or to adding a:
	struct device *dev = &priv->pcidev->dev;

to every function just for printing.

However, I do understand your point. I can drop dev if you prefer.

> > +/**
> > + * _cti_set_tristate() - Enable/Disable RS485 transciever tristate
> > + * @priv: Device's private structure
> > + * @port_num: Port number to set tristate on/off
> > + * @enable: Enable tristate if true, disable if false
> > + *
> > + * 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 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_set_tristate(struct exar8250 *priv,
> > +			unsigned int port_num, bool enable)
> > +{
> > +	int ret = 0;
> > +
> > +	if (port_num >= priv->nr)
> > +		return -EINVAL;
> > +
> > +	// Only Exar based cards use MPIO, return 0 otherwise
> > +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> > +		return 0;
>
> How can this ever happen?  Only the exar devices will call this
> function, or am I missing a path here?
>

Yes that can go now, it used to be needed but not now. I will remove.

>
> > +
> > +	dev_dbg(priv->dev, "%s tristate for port %u\n",
> > +		str_enable_disable(enable), port_num);
> > +
> > +	if (enable)
> > +		ret = exar_mpio_set_low(priv, port_num);
> > +	else
> > +		ret = exar_mpio_set_high(priv, port_num);
> > +	if (ret)
> > +		return ret;
> > +
> > +	// Ensure MPIO is an output
> > +	ret = exar_mpio_config_output(priv, port_num);
> > +
> > +	return ret;
> > +}
> > +
> > +static int cti_tristate_disable(struct exar8250 *priv, unsigned int port_num)
> > +{
> > +	return _cti_set_tristate(priv, port_num, false);
> > +}
>
> Do you ever call _cti_set_tristate() with "true"?
>

Currently no. However, re-enabling tristate via a custom ioctl was a feature in
our old out-of-tree driver (which was created prior to linux RS485 support).

I am not sure how it would be activated now, but I left enabling tristate as an
option in to make it easier down the line when we need it.

I can add a note to the patch or remove it if you would prefer.

> > +
> > +/**
> > + * _cti_set_plx_int_enable() - Enable/Disable PCI interrupts
> > + * @priv: Device's private structure
> > + * @enable: Enable interrupts if true, disable if false
>
> But false is never used here, so why have this at all?
>
> > + *
> > + * Some older CTI cards require MPIO_0 to be set low to enable the PCI
> > + * interupts from the UART to the PLX PCI->PCIe bridge.
> > + *
> > + * Return: 0 on success, negative error code on failure
> > + */
> > +static int _cti_set_plx_int_enable(struct exar8250 *priv, bool enable)
> > +{
> > +	int ret = 0;
> > +
> > +	// Only Exar based cards use MPIO, return 0 otherwise
> > +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> > +		return 0;
>
> Same question here.
>

Same as above, not needed. I will remove.

> > +
> > +	if (enable)
> > +		ret = exar_mpio_set_low(priv, 0);
> > +	else
> > +		ret = exar_mpio_set_high(priv, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	// Ensure MPIO is an output
> > +	ret = exar_mpio_config_output(priv, 0);
> > +
> > +	return ret;
> > +}
> > +
> > +static int cti_plx_int_enable(struct exar8250 *priv)
> > +{
> > +	return _cti_set_plx_int_enable(priv, true);
>
> Again, no wrapper needed if you never actually call that function with
> "false", right?  Or am I missing a path here?
>

This one is similar to _cti_set_tristate() but is less likely to be used.

Thanks again,
Parker

> thanks,
>
> greg k-h

Re: [PATCH v3 7/8] serial: exar: add CTI specific setup code

[Greg Kroah-Hartman]

On Wed, Apr 17, 2024 at 09:17:35AM -0400, Parker Newman wrote:
> On Wed, 17 Apr 2024 13:24:20 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Apr 16, 2024 at 08:55:34AM -0400, Parker Newman wrote:
> > >  struct exar8250 {
> > >  	unsigned int		nr;
> > > +	unsigned int		osc_freq;
> > > +	struct pci_dev		*pcidev;
> > > +	struct device		*dev;
> >
> > Why do you need both a pci_dev and a device?  Aren't they the same thing
> > here?
> >
> 
> I added dev to make the prints cleaner. I personally prefer:
> 
> 	dev_err(priv->dev, ...);
> to
> 	dev_err(&priv->pcidev->dev, ...);
> 
> or to adding a:
> 	struct device *dev = &priv->pcidev->dev;
> 
> to every function just for printing.
> 
> However, I do understand your point. I can drop dev if you prefer.

Pick one and stick with it, no need for both.  I too like your first
option, and if you never need the pci device pointer, just stick with
"struct dev".  Note, properly reference count it in case you think it
could go away from underneath you (some paths it could, so be careful.)

> > > +static int cti_tristate_disable(struct exar8250 *priv, unsigned int port_num)
> > > +{
> > > +	return _cti_set_tristate(priv, port_num, false);
> > > +}
> >
> > Do you ever call _cti_set_tristate() with "true"?
> >
> 
> Currently no. However, re-enabling tristate via a custom ioctl was a feature in
> our old out-of-tree driver (which was created prior to linux RS485 support).
> 
> I am not sure how it would be activated now, but I left enabling tristate as an
> option in to make it easier down the line when we need it.
> 
> I can add a note to the patch or remove it if you would prefer.

Just remove it for now, no need to ever add code you "might need in the
future", just add that then, when you need it.  Code for what you need
to do now, that makes things simpler and removes extra stuff that you
would have to force others to review :)

thanks,

greg k-h

[PATCH v3 8/8] serial: exar: fix: fix crash during shutdown if setup fails

[Parker Newman]

From: Parker Newman <pnewman@connecttech.com>

If a port fails to register with serial8250_register_8250_port() the
kernel can crash when shutting down or module removal.

This is because "priv->line[i]" will be set to a negative error code
and in the exar_pci_remove() function serial8250_unregister_port() is
called without checking if the "priv->line[i]" value is valid.

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 501b9f3e9c89..f5a395ed69d1 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -1671,7 +1671,8 @@ static void exar_pci_remove(struct pci_dev *pcidev)
 	unsigned int i;

 	for (i = 0; i < priv->nr; i++)
-		serial8250_unregister_port(priv->line[i]);
+		if (priv->line[i] >= 0)
+			serial8250_unregister_port(priv->line[i]);

 	/* Ensure that every init quirk is properly torn down */
 	if (priv->board->exit)
--
2.43.2

Re: [PATCH v3 8/8] serial: exar: fix: fix crash during shutdown if setup fails

[Greg Kroah-Hartman]

On Tue, Apr 16, 2024 at 08:55:35AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> If a port fails to register with serial8250_register_8250_port() the
> kernel can crash when shutting down or module removal.
> 
> This is because "priv->line[i]" will be set to a negative error code
> and in the exar_pci_remove() function serial8250_unregister_port() is
> called without checking if the "priv->line[i]" value is valid.
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
>  drivers/tty/serial/8250/8250_exar.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 501b9f3e9c89..f5a395ed69d1 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -1671,7 +1671,8 @@ static void exar_pci_remove(struct pci_dev *pcidev)
>  	unsigned int i;
> 
>  	for (i = 0; i < priv->nr; i++)
> -		serial8250_unregister_port(priv->line[i]);
> +		if (priv->line[i] >= 0)
> +			serial8250_unregister_port(priv->line[i]);

Is this a bug in the current driver?  If so, can you resend it on its
own so we can get it merged now?

thanks,

greg k-h

Re: [PATCH v3 8/8] serial: exar: fix: fix crash during shutdown if setup fails

[Parker Newman]

On Wed, 17 Apr 2024 13:19:07 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Tue, Apr 16, 2024 at 08:55:35AM -0400, Parker Newman wrote:
> > From: Parker Newman <pnewman@connecttech.com>
> >
> > If a port fails to register with serial8250_register_8250_port() the
> > kernel can crash when shutting down or module removal.
> >
> > This is because "priv->line[i]" will be set to a negative error code
> > and in the exar_pci_remove() function serial8250_unregister_port() is
> > called without checking if the "priv->line[i]" value is valid.
> >
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > ---
> >  drivers/tty/serial/8250/8250_exar.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 501b9f3e9c89..f5a395ed69d1 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -1671,7 +1671,8 @@ static void exar_pci_remove(struct pci_dev *pcidev)
> >  	unsigned int i;
> >
> >  	for (i = 0; i < priv->nr; i++)
> > -		serial8250_unregister_port(priv->line[i]);
> > +		if (priv->line[i] >= 0)
> > +			serial8250_unregister_port(priv->line[i]);
>
> Is this a bug in the current driver?  If so, can you resend it on its
> own so we can get it merged now?
>

Yes it is, I can split this one out and send it on its own.
Thanks,
Parker

> thanks,
>
> greg k-h

Re: [PATCH v3 8/8] serial: exar: fix: fix crash during shutdown if setup fails

[Greg Kroah-Hartman]

On Wed, Apr 17, 2024 at 08:24:13AM -0400, Parker Newman wrote:
> On Wed, 17 Apr 2024 13:19:07 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Apr 16, 2024 at 08:55:35AM -0400, Parker Newman wrote:
> > > From: Parker Newman <pnewman@connecttech.com>
> > >
> > > If a port fails to register with serial8250_register_8250_port() the
> > > kernel can crash when shutting down or module removal.
> > >
> > > This is because "priv->line[i]" will be set to a negative error code
> > > and in the exar_pci_remove() function serial8250_unregister_port() is
> > > called without checking if the "priv->line[i]" value is valid.
> > >
> > > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > > ---
> > >  drivers/tty/serial/8250/8250_exar.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > > index 501b9f3e9c89..f5a395ed69d1 100644
> > > --- a/drivers/tty/serial/8250/8250_exar.c
> > > +++ b/drivers/tty/serial/8250/8250_exar.c
> > > @@ -1671,7 +1671,8 @@ static void exar_pci_remove(struct pci_dev *pcidev)
> > >  	unsigned int i;
> > >
> > >  	for (i = 0; i < priv->nr; i++)
> > > -		serial8250_unregister_port(priv->line[i]);
> > > +		if (priv->line[i] >= 0)
> > > +			serial8250_unregister_port(priv->line[i]);
> >
> > Is this a bug in the current driver?  If so, can you resend it on its
> > own so we can get it merged now?
> >
> 
> Yes it is, I can split this one out and send it on its own.

Great!  Bonus points if you can find the commit id it fixes and add a
"Fixes:" tag to the signed-off-by area.  If not, I can guess :)

thanks,

greg k-h

Re: [PATCH v3 8/8] serial: exar: fix: fix crash during shutdown if setup fails

[Parker Newman]

On Wed, 17 Apr 2024 15:30:56 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Wed, Apr 17, 2024 at 08:24:13AM -0400, Parker Newman wrote:
> > On Wed, 17 Apr 2024 13:19:07 +0200
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >
> > > On Tue, Apr 16, 2024 at 08:55:35AM -0400, Parker Newman wrote:
> > > > From: Parker Newman <pnewman@connecttech.com>
> > > >
> > > > If a port fails to register with serial8250_register_8250_port() the
> > > > kernel can crash when shutting down or module removal.
> > > >
> > > > This is because "priv->line[i]" will be set to a negative error code
> > > > and in the exar_pci_remove() function serial8250_unregister_port() is
> > > > called without checking if the "priv->line[i]" value is valid.
> > > >
> > > > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> > > > ---
> > > >  drivers/tty/serial/8250/8250_exar.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > > > index 501b9f3e9c89..f5a395ed69d1 100644
> > > > --- a/drivers/tty/serial/8250/8250_exar.c
> > > > +++ b/drivers/tty/serial/8250/8250_exar.c
> > > > @@ -1671,7 +1671,8 @@ static void exar_pci_remove(struct pci_dev *pcidev)
> > > >  	unsigned int i;
> > > >
> > > >  	for (i = 0; i < priv->nr; i++)
> > > > -		serial8250_unregister_port(priv->line[i]);
> > > > +		if (priv->line[i] >= 0)
> > > > +			serial8250_unregister_port(priv->line[i]);
> > >
> > > Is this a bug in the current driver?  If so, can you resend it on its
> > > own so we can get it merged now?
> > >
> >
> > Yes it is, I can split this one out and send it on its own.
>
> Great!  Bonus points if you can find the commit id it fixes and add a
> "Fixes:" tag to the signed-off-by area.  If not, I can guess :)
>
> thanks,
>
> greg k-h

After looking at this again and doing some testing this bug does not actually
happen with the driver in its current state. During my development I had it
happen but that would have been due to me messing around.

When "priv->line[i]" < 0 it breaks out of the for loop and priv->nr is set to "i".
so only the successfully registered ports will be unregistered in exar_pci_remove().

...
        for (i = 0; i < nr_ports && i < maxnr; i++) {
                rc = board->setup(priv, pcidev, &uart, i);
                if (rc) {
                        dev_err(&pcidev->dev, "Failed to setup port %u\n", i);
                        break;
                }

                dev_dbg(&pcidev->dev, "Setup PCI port: port %lx, irq %d, type %d\n",
                        uart.port.iobase, uart.port.irq, uart.port.iotype);

                priv->line[i] = serial8250_register_8250_port(&uart);
                if (priv->line[i] < 0) {
                        dev_err(&pcidev->dev,
                                "Couldn't register serial port %lx, irq %d, type %d, error %d\n",
                                uart.port.iobase, uart.port.irq,
                                uart.port.iotype, priv->line[i]);
                        break;
                }
        }
        priv->nr = i;
...

Thanks,
Parker

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

[Greg Kroah-Hartman]

On Tue, Apr 16, 2024 at 08:55:27AM -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.

Much better.  I took the 1st patch already in my tree to make it
hopefully easire for you to rebase and redo the rest.

thanks,

greg k-h

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

[Parker Newman]

On Wed, 17 Apr 2024 13:24:49 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Tue, Apr 16, 2024 at 08:55:27AM -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.
>
> Much better.  I took the 1st patch already in my tree to make it
> hopefully easire for you to rebase and redo the rest.
>
> thanks,
>
> greg k-h

I will resend with the updates.
I have been using the "main" branch of gregkh/tty.git so far. Is that correct?
Or should I be using "tty-testing"?

Thanks again for bearing with me while I figure this out :).
Parker

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

[Greg Kroah-Hartman]

On Wed, Apr 17, 2024 at 09:26:41AM -0400, Parker Newman wrote:
> On Wed, 17 Apr 2024 13:24:49 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Apr 16, 2024 at 08:55:27AM -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.
> >
> > Much better.  I took the 1st patch already in my tree to make it
> > hopefully easire for you to rebase and redo the rest.
> >
> > thanks,
> >
> > greg k-h
> 
> I will resend with the updates.
> I have been using the "main" branch of gregkh/tty.git so far. Is that correct?
> Or should I be using "tty-testing"?

tty-testing is where things go before they are considered "good enough"
to get into "tty-next" which is never rebased, and is what will be sent
to Linus for the "next" kernel release (not this one.)

Sometimes it is rebased if I mess things up, but usually it's pretty
stable.  Patches only sit in there for 24 hours usually before ending up
in "tty-next"

"main" just tracks Linus's branch, don't use it, I just need a branch to
diff against.

"tty-linus" is what will be sent to Linus for "this" release (i.e. bug
fixes only.)

Or you can just work off of the linux-next tree, which merges in both of
my tty-next and tty-linus branches together (and all other maintainer
trees/branches), every day, and is usually a good base for what will be
the "next" release after this one.

hope this helps,

greg k-h