[PATCH v2 0/2] serial: 8250_pci patches to address issues with pericom_do_set_divisor()

[PATCH v2 0/2] serial: 8250_pci patches to address issues with pericom_do_set_divisor()

[Andy Shevchenko]

A series patches to address three issues one customer managed to hit
all at once.

1) Fourth port not being setup correctly on some Pericom chips. Fix
the entries in pci_serial_quirks array.

2) Rewrite pericom_do_set_divisor() to always calc divisor and to use
the uartclk instead of a hard coded value. Always calculate divisor
without passing control to serial8250_do_set_divisor().

Jay, can you retest this, please?

Changelog v2:
- dropped no-fixes patch, left only fixes here
- amended refactoring of the ->set_divisor()
- fixed Fixes tags

Jay Dolan (2):
  serial: 8250_pci: Fix ACCES entries in pci_serial_quirks array
  serial: 8250_pci: rewrite pericom_do_set_divisor()

 drivers/tty/serial/8250/8250_pci.c | 39 +++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 14 deletions(-)

-- 
2.33.0

[PATCH v2 1/2] serial: 8250_pci: Fix ACCES entries in pci_serial_quirks array

[Andy Shevchenko]

From: Jay Dolan <jay.dolan@accesio.com>

Fix error in table for PCI_DEVICE_ID_ACCESIO_PCIE_ICM_4S that caused it
and PCI_DEVICE_ID_ACCESIO_PCIE_ICM232_4 to be missing their fourth port.

Fixes: 78d3820b9bd3 ("serial: 8250_pci: Have ACCES cards that use the four port Pericom PI7C9X7954 chip use the pci_pericom_setup()")
Signed-off-by: Jay Dolan <jay.dolan@accesio.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_pci.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 5d43de143f33..b793d848aeb6 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -2291,12 +2291,19 @@ static struct pci_serial_quirk pci_serial_quirks[] = {
 		.setup      = pci_pericom_setup_four_at_eight,
 	},
 	{
-		.vendor     = PCI_DEVICE_ID_ACCESIO_PCIE_ICM_4S,
+		.vendor     = PCI_VENDOR_ID_ACCESIO,
 		.device     = PCI_DEVICE_ID_ACCESIO_PCIE_ICM232_4,
 		.subvendor  = PCI_ANY_ID,
 		.subdevice  = PCI_ANY_ID,
 		.setup      = pci_pericom_setup_four_at_eight,
 	},
+	{
+		.vendor     = PCI_VENDOR_ID_ACCESIO,
+		.device     = PCI_DEVICE_ID_ACCESIO_PCIE_ICM_4S,
+		.subvendor  = PCI_ANY_ID,
+		.subdevice  = PCI_ANY_ID,
+		.setup      = pci_pericom_setup_four_at_eight,
+	},
 	{
 		.vendor     = PCI_VENDOR_ID_ACCESIO,
 		.device     = PCI_DEVICE_ID_ACCESIO_MPCIE_ICM232_4,
-- 
2.33.0

[PATCH v2 2/2] serial: 8250_pci: rewrite pericom_do_set_divisor()

[Andy Shevchenko]

From: Jay Dolan <jay.dolan@accesio.com>

Have pericom_do_set_divisor() use the uartclk instead of a hard coded
value to work with different speed crystals. Tested with 14.7456 and 24
MHz crystals.

Have pericom_do_set_divisor() always calculate the divisor rather than
call serial8250_do_set_divisor() for rates below baud_base.

Do not write registers or call serial8250_do_set_divisor() if valid
divisors could not be found.

Fixes: 6bf4e42f1d19 ("serial: 8250: Add support for higher baud rates to Pericom chips")
Signed-off-by: Jay Dolan <jay.dolan@accesio.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_pci.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index b793d848aeb6..8b29a9381c74 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1324,29 +1324,33 @@ pericom_do_set_divisor(struct uart_port *port, unsigned int baud,
 {
 	int scr;
 	int lcr;
-	int actual_baud;
-	int tolerance;
 
-	for (scr = 5 ; scr <= 15 ; scr++) {
-		actual_baud = 921600 * 16 / scr;
-		tolerance = actual_baud / 50;
+	for (scr = 16; scr > 4; scr--) {
+		unsigned int maxrate = port->uartclk / scr;
+		unsigned int divisor = maxrate / baud;
+		int delta = maxrate / divisor - baud;
 
-		if ((baud < actual_baud + tolerance) &&
-			(baud > actual_baud - tolerance)) {
+		if (baud > maxrate + baud / 50)
+			continue;
 
+		if (divisor == 0 || delta > baud / 50)
+			divisor++;
+
+		if (divisor > 0xffff)
+			continue;
+
+		/* Update delta due to possible divisor change */
+		delta = maxrate / divisor - baud;
+		if (abs(delta) < baud / 50) {
 			lcr = serial_port_in(port, UART_LCR);
 			serial_port_out(port, UART_LCR, lcr | 0x80);
-
-			serial_port_out(port, UART_DLL, 1);
-			serial_port_out(port, UART_DLM, 0);
+			serial_port_out(port, UART_DLL, divisor & 0xff);
+			serial_port_out(port, UART_DLM, divisor >> 8 & 0xff);
 			serial_port_out(port, 2, 16 - scr);
 			serial_port_out(port, UART_LCR, lcr);
 			return;
-		} else if (baud > actual_baud) {
-			break;
 		}
 	}
-	serial8250_do_set_divisor(port, baud, quot, quot_frac);
 }
 static int pci_pericom_setup(struct serial_private *priv,
 		  const struct pciserial_board *board,
-- 
2.33.0

Re: [PATCH v2 0/2] serial: 8250_pci patches to address issues with pericom_do_set_divisor()

[Jay Dolan]

On 11/17/21 6:55 AM, Andy Shevchenko wrote:
> A series patches to address three issues one customer managed to hit
> all at once.
> 
> 1) Fourth port not being setup correctly on some Pericom chips. Fix
> the entries in pci_serial_quirks array.
> 
> 2) Rewrite pericom_do_set_divisor() to always calc divisor and to use
> the uartclk instead of a hard coded value. Always calculate divisor
> without passing control to serial8250_do_set_divisor().
> 
> Jay, can you retest this, please?
I was able to verify that the fourth port was placed at the fixed 
address for the card that previously had it at the wrong offset.
I was also able to verify all of the standard baud rates for 24 and 
14.7456 MHz crystals from 50 to their respective uartclk speed on the 
oscilloscope.
> 
> Changelog v2:
> - dropped no-fixes patch, left only fixes here
> - amended refactoring of the ->set_divisor()
> - fixed Fixes tags
> 
> Jay Dolan (2):
>    serial: 8250_pci: Fix ACCES entries in pci_serial_quirks array
>    serial: 8250_pci: rewrite pericom_do_set_divisor()
> 
>   drivers/tty/serial/8250/8250_pci.c | 39 +++++++++++++++++++-----------
>   1 file changed, 25 insertions(+), 14 deletions(-)
>

Re: [PATCH v2 0/2] serial: 8250_pci patches to address issues with pericom_do_set_divisor()

[Andy Shevchenko]

On Thu, Nov 18, 2021 at 8:09 AM Jay Dolan <jay.dolan@accesio.com> wrote:
> On 11/17/21 6:55 AM, Andy Shevchenko wrote:
> > A series patches to address three issues one customer managed to hit
> > all at once.
> >
> > 1) Fourth port not being setup correctly on some Pericom chips. Fix
> > the entries in pci_serial_quirks array.
> >
> > 2) Rewrite pericom_do_set_divisor() to always calc divisor and to use
> > the uartclk instead of a hard coded value. Always calculate divisor
> > without passing control to serial8250_do_set_divisor().
> >
> > Jay, can you retest this, please?
> I was able to verify that the fourth port was placed at the fixed
> address for the card that previously had it at the wrong offset.
> I was also able to verify all of the standard baud rates for 24 and
> 14.7456 MHz crystals from 50 to their respective uartclk speed on the
> oscilloscope.

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] serial: 8250_pci patches to address issues with pericom_do_set_divisor()

[Andy Shevchenko]

On Thu, Nov 18, 2021 at 11:52:06AM +0200, Andy Shevchenko wrote:
> On Thu, Nov 18, 2021 at 8:09 AM Jay Dolan <jay.dolan@accesio.com> wrote:
> > On 11/17/21 6:55 AM, Andy Shevchenko wrote:

...

> > > Jay, can you retest this, please?
> > I was able to verify that the fourth port was placed at the fixed
> > address for the card that previously had it at the wrong offset.
> > I was also able to verify all of the standard baud rates for 24 and
> > 14.7456 MHz crystals from 50 to their respective uartclk speed on the
> > oscilloscope.
> 
> Thanks!

Greg, there is a subtle bug found in the second patch, I'll issue the v2 soon.

-- 
With Best Regards,
Andy Shevchenko