[PATCH V8 1/2] serial: exar: Revert "serial: exar: Add support for Sealevel 7xxxC serial cards"

[PATCH V8 1/2] serial: exar: Revert "serial: exar: Add support for Sealevel 7xxxC serial cards"

[Matthew Howell]

From: Matthew Howell <matthew.howell@sealevel.com>

Hardware ID of Sealevel 7xxxC cards changed prior to release.
This has rendered 14ee78d5932a redundant.

Revert "serial: exar: Add support for Sealevel 7xxxC serial cards"

This reverts commit 14ee78d5932afeb710c8305196a676a715bfdea8.

Fixes: 14ee78d5932a ("Add support for Sealevel 7xxxC serial cards")
Signed-off-by: Matthew Howell <matthew.howell@sealevel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 077c3ba3539e..3886f78ecbbf 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -47,12 +47,6 @@
 #define PCI_SUBDEVICE_ID_USR_2980		0x0128
 #define PCI_SUBDEVICE_ID_USR_2981		0x0129

-#define PCI_DEVICE_ID_SEALEVEL_710xC		0x1001
-#define PCI_DEVICE_ID_SEALEVEL_720xC		0x1002
-#define PCI_DEVICE_ID_SEALEVEL_740xC		0x1004
-#define PCI_DEVICE_ID_SEALEVEL_780xC		0x1008
-#define PCI_DEVICE_ID_SEALEVEL_716xC		0x1010
-
 #define UART_EXAR_INT0		0x80
 #define UART_EXAR_8XMODE	0x88	/* 8X sampling rate select */
 #define UART_EXAR_SLEEP		0x8b	/* Sleep mode */
@@ -652,8 +646,6 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
 	else if (board->num_ports)
 		nr_ports = board->num_ports;
-	else if (pcidev->vendor == PCI_VENDOR_ID_SEALEVEL)
-		nr_ports = pcidev->device & 0xff;
 	else
 		nr_ports = pcidev->device & 0x0f;

@@ -893,12 +885,6 @@ static const struct pci_device_id exar_pci_tbl[] = {
 	EXAR_DEVICE(COMMTECH, 4224PCI335, pbn_fastcom335_4),
 	EXAR_DEVICE(COMMTECH, 2324PCI335, pbn_fastcom335_4),
 	EXAR_DEVICE(COMMTECH, 2328PCI335, pbn_fastcom335_8),
-
-	EXAR_DEVICE(SEALEVEL, 710xC, pbn_exar_XR17V35x),
-	EXAR_DEVICE(SEALEVEL, 720xC, pbn_exar_XR17V35x),
-	EXAR_DEVICE(SEALEVEL, 740xC, pbn_exar_XR17V35x),
-	EXAR_DEVICE(SEALEVEL, 780xC, pbn_exar_XR17V35x),
-	EXAR_DEVICE(SEALEVEL, 716xC, pbn_exar_XR17V35x),
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, exar_pci_tbl);

[PATCH V8 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Matthew Howell]

From: Matthew Howell <matthew.howell@sealevel.com>

Sealevel XR17V35X based cards utilize DTR to control RS-485 Enable, but
the current implementation of 8250_exar uses RTS for the auto-RS485-Enable
mode of the XR17V35X UARTs. This patch implements DTR Auto-RS485 on
Sealevel cards.

Signed-off-by: Matthew Howell <matthew.howell@sealevel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
V7->V8
Added space to align text in sealevel_rs485_config()
Removed unneeded empty line.
Switched email clients. New client was adding the in-reply-to 
header in testing so hopefully threading actually works this time.

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 3886f78ecbbf..8ab80447fe8c 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -78,6 +78,9 @@

 #define UART_EXAR_RS485_DLY(x)	((x) << 4)

+#define UART_EXAR_DLD			0x02 /* Divisor Fractional */
+#define UART_EXAR_DLD_485_POLARITY	0x80 /* RS-485 Enable Signal Polarity */
+
 /*
  * IOT2040 MPIO wiring semantics:
  *
@@ -439,6 +442,44 @@ static int generic_rs485_config(struct uart_port *port, struct ktermios *termios
 	return 0;
 }

+static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
+				 struct serial_rs485 *rs485)
+{
+	u8 __iomem *p = port->membase;
+	u8 old_lcr;
+	u8 efr;
+	u8 dld;
+	int ret;
+
+	ret = generic_rs485_config(port, termios, rs485);
+	if (ret)
+		return ret;
+
+	if (rs485->flags & SER_RS485_ENABLED) {
+		old_lcr = readb(p + UART_LCR);
+
+		/* Set EFR[4]=1 to enable enhanced feature registers */
+		efr = readb(p + UART_XR_EFR);
+		efr |= UART_EFR_ECB;
+		writeb(efr, p + UART_XR_EFR);
+
+		/* Set MCR to use DTR as Auto-RS485 Enable signal */
+		writeb(UART_MCR_OUT1, p + UART_MCR);
+
+		/* Set LCR[7]=1 to enable access to DLD register */
+		writeb(old_lcr | UART_LCR_DLAB, p + UART_LCR);
+
+		/* Set DLD[7]=1 for inverted RS485 Enable logic */
+		dld = readb(p + UART_EXAR_DLD);
+		dld |= UART_EXAR_DLD_485_POLARITY;
+		writeb(dld, p + UART_EXAR_DLD);
+
+		writeb(old_lcr, p + UART_LCR);
+	}
+
+	return 0;
+}
+
 static const struct serial_rs485 generic_rs485_supported = {
 	.flags = SER_RS485_ENABLED,
 };
@@ -744,6 +785,20 @@ static int __maybe_unused exar_resume(struct device *dev)
 	return 0;
 }

+static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
+		   struct uart_8250_port *port, int idx)
+{
+	int ret;
+
+	ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
+	if (ret)
+		return ret;
+
+	port->port.rs485_config = sealevel_rs485_config;
+
+	return 0;
+}
+
 static SIMPLE_DEV_PM_OPS(exar_pci_pm, exar_suspend, exar_resume);

 static const struct exar8250_board pbn_fastcom335_2 = {
@@ -809,6 +864,17 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
 	.exit		= pci_xr17v35x_exit,
 };

+static const struct exar8250_board pbn_sealevel = {
+	.setup		= pci_sealevel_setup,
+	.exit		= pci_xr17v35x_exit,
+};
+
+static const struct exar8250_board pbn_sealevel_16 = {
+	.num_ports	= 16,
+	.setup		= pci_sealevel_setup,
+	.exit		= pci_xr17v35x_exit,
+};
+
 #define CONNECT_DEVICE(devid, sdevid, bd) {				\
 	PCI_DEVICE_SUB(							\
 		PCI_VENDOR_ID_EXAR,					\
@@ -838,6 +904,15 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
 		(kernel_ulong_t)&bd			\
 	}

+#define SEALEVEL_DEVICE(devid, bd) {			\
+	PCI_DEVICE_SUB(					\
+		PCI_VENDOR_ID_EXAR,			\
+		PCI_DEVICE_ID_EXAR_##devid,		\
+		PCI_VENDOR_ID_SEALEVEL,			\
+		PCI_ANY_ID), 0, 0,			\
+		(kernel_ulong_t)&bd			\
+	}
+
 static const struct pci_device_id exar_pci_tbl[] = {
 	EXAR_DEVICE(ACCESSIO, COM_2S, pbn_exar_XR17C15x),
 	EXAR_DEVICE(ACCESSIO, COM_4S, pbn_exar_XR17C15x),
@@ -860,6 +935,12 @@ static const struct pci_device_id exar_pci_tbl[] = {
 	CONNECT_DEVICE(XR17C154, UART_4_485, pbn_connect),
 	CONNECT_DEVICE(XR17C158, UART_8_485, pbn_connect),

+	SEALEVEL_DEVICE(XR17V352, pbn_sealevel),
+	SEALEVEL_DEVICE(XR17V354, pbn_sealevel),
+	SEALEVEL_DEVICE(XR17V358, pbn_sealevel),
+	SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16),
+	SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16),
+
 	IBM_DEVICE(XR17C152, SATURN_SERIAL_ONE_PORT, pbn_exar_ibm_saturn),

 	/* USRobotics USR298x-OEM PCI Modems */

Re: [PATCH V8 1/2] serial: exar: Revert "serial: exar: Add support for Sealevel 7xxxC serial cards"

[andriy.shevchenko]

On Fri, Sep 22, 2023 at 02:20:23PM +0000, Matthew Howell wrote:
> From: Matthew Howell <matthew.howell@sealevel.com>
> 
> Hardware ID of Sealevel 7xxxC cards changed prior to release.
> This has rendered 14ee78d5932a redundant.

> Revert "serial: exar: Add support for Sealevel 7xxxC serial cards"

Aside:
This line basically is not needed (and misses period) as below does
the equivalent.

> This reverts commit 14ee78d5932afeb710c8305196a676a715bfdea8.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V8 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[andriy.shevchenko]

Bingo, you have threaded emails!

On Fri, Sep 22, 2023 at 02:22:11PM +0000, Matthew Howell wrote:
> From: Matthew Howell <matthew.howell@sealevel.com>
> 
> Sealevel XR17V35X based cards utilize DTR to control RS-485 Enable, but
> the current implementation of 8250_exar uses RTS for the auto-RS485-Enable
> mode of the XR17V35X UARTs. This patch implements DTR Auto-RS485 on
> Sealevel cards.

Btw, few ideas for further improvements / questions.

...

> +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +		   struct uart_8250_port *port, int idx)
> +{
> +	int ret;
> +
> +	ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
> +	if (ret)
> +		return ret;
> +
> +	port->port.rs485_config = sealevel_rs485_config;
> +
> +	return 0;
> +}

This actually can be embedded into original pci_xr17v35x_setup() as

	if (pdev->subsystem_vendor == PCI_VENDOR_ID_SEALEVEL)
		port->port.rs485_config = sealevel_rs485_config;

...

> +	SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16),

This is kinda worries me. Original Exar card has12 port, why 16 is in use for this one?

> +	SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16),

With the above suggestion this will be fixed automatically.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V8 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[Matthew Howell]

On Fri, 2023-09-22 at 17:48 +0300, andriy.shevchenko@intel.com wrote:
> ⚠Caution: External email. Exercise extreme caution with links or attachments.⚠
> 
> 
> Bingo, you have threaded emails!
> 
> On Fri, Sep 22, 2023 at 02:22:11PM +0000, Matthew Howell wrote:
> > From: Matthew Howell <matthew.howell@sealevel.com>
> > 
> > Sealevel XR17V35X based cards utilize DTR to control RS-485 Enable, but
> > the current implementation of 8250_exar uses RTS for the auto-RS485-Enable
> > mode of the XR17V35X UARTs. This patch implements DTR Auto-RS485 on
> > Sealevel cards.
> 
> Btw, few ideas for further improvements / questions.
> 
> ...
> 
> > +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> > +                struct uart_8250_port *port, int idx)
> > +{
> > +     int ret;
> > +
> > +     ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
> > +     if (ret)
> > +             return ret;
> > +
> > +     port->port.rs485_config = sealevel_rs485_config;
> > +
> > +     return 0;
> > +}
> 
> This actually can be embedded into original pci_xr17v35x_setup() as
> 
>         if (pdev->subsystem_vendor == PCI_VENDOR_ID_SEALEVEL)
>                 port->port.rs485_config = sealevel_rs485_config;
> 
> ...
> 

That was my original thought prior to the first submission, but I wasn't
sure about adding vendor-specific changes into pci_xr17v35x_setup()
since it appears that the convention in 8250_exar.c has been for vendors
to have their own setup function when they needed to change the
init/setup behaviour.

If that is not the case though and having this in pci_xr17v35x_setup()
is more appropriate I'll be happy to put it there instead of having yet
another setup function.

> > +     SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16),
> 
> This is kinda worries me. Original Exar card has12 port, why 16 is in use for this one?
> 

Ah, good catch. I had actually forgotten about the 12 port version. Will
either fix or make redundant with move to pci_xr17v35x_setup() depending
on feedback from my statement above about what is most appropriate.

> > +     SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16),
> With the above suggestion this will be fixed automatically.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH V8 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

[andriy.shevchenko]

On Fri, Sep 22, 2023 at 03:33:05PM +0000, Matthew Howell wrote:
> On Fri, 2023-09-22 at 17:48 +0300, andriy.shevchenko@intel.com wrote:
> > On Fri, Sep 22, 2023 at 02:22:11PM +0000, Matthew Howell wrote:

...

> > > +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> > > +                struct uart_8250_port *port, int idx)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     port->port.rs485_config = sealevel_rs485_config;
> > > +
> > > +     return 0;
> > > +}
> > 
> > This actually can be embedded into original pci_xr17v35x_setup() as
> > 
> >         if (pdev->subsystem_vendor == PCI_VENDOR_ID_SEALEVEL)
> >                 port->port.rs485_config = sealevel_rs485_config;
> 
> That was my original thought prior to the first submission, but I wasn't
> sure about adding vendor-specific changes into pci_xr17v35x_setup()
> since it appears that the convention in 8250_exar.c has been for vendors
> to have their own setup function when they needed to change the
> init/setup behaviour.
> 
> If that is not the case though and having this in pci_xr17v35x_setup()

Just use the common sense. The case you are now adding is using PCI IDs
(vendor and device) that are Exar's. So, I do not see any violation of
the above assumption. Checking for _sub_ IDs is fine, it's just a quirk
for the Exar based chips.

> is more appropriate I'll be happy to put it there instead of having yet
> another setup function.

...

> > > +     SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16),
> > 
> > This is kinda worries me. Original Exar card has 12 ports, why 16 is in use
> > for this one?
> 
> Ah, good catch. I had actually forgotten about the 12 port version. Will
> either fix or make redundant with move to pci_xr17v35x_setup() depending
> on feedback from my statement above about what is most appropriate.
> 
> > > +     SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16),
> > With the above suggestion this will be fixed automatically.

-- 
With Best Regards,
Andy Shevchenko