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

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

[Jay Dolan]

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

1) 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()
Tested with 14.7456 and 24 MHz crystals

2) Re-enable higher baud rates on Pericom chips.
serial8250_get_baud_rate() added range checking, but Pericom chips have a wider
range than what is being enforced. Make use of UPF_MAGIC_MULTIPLIER.
Tested with 14.7456 and 24 MHz crystals

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


Jay Dolan (3):
  serial: 8250_pci: rewrite pericom_do_set_divisor
  serial: 8250_pci: Re-enable higher baud rates on Pericom chips
  serial: 8250_pci: Fix ACCES entries in pci_serial_quirks array

 drivers/tty/serial/8250/8250_pci.c | 44 +++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 13 deletions(-)

--
2.25.1

[PATCH 1/3] serial: 8250_pci: rewrite pericom_do_set_divisor

[Jay Dolan]

Fixes: 6bf4e42f1d19de10800f4483b4bb7945aab283cb

Have pericom_do_set_divisor() use the uartclk instead of a hardcoded
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.

Signed-off-by: Jay Dolan <jay.dolan@accesio.com>
---
 drivers/tty/serial/8250/8250_pci.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 5d43de143f33..aea43683b76d 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1325,28 +1325,32 @@ pericom_do_set_divisor(struct uart_port *port, unsigned int baud,
 	int scr;
 	int lcr;
 	int actual_baud;
-	int tolerance;
+	int divisor;
 
-	for (scr = 5 ; scr <= 15 ; scr++) {
-		actual_baud = 921600 * 16 / scr;
-		tolerance = actual_baud / 50;
+	for (scr = 16 ; scr > 4 ; scr--) {
+		if (baud > (port->uartclk / scr) + (baud/50))
+			continue;
 
-		if ((baud < actual_baud + tolerance) &&
-			(baud > actual_baud - tolerance)) {
+		divisor = port->uartclk / scr / baud;
+		if (divisor == 0 ||
+			port->uartclk / scr / divisor - baud > baud/50) {
+			divisor++;
+		}
+
+		if (divisor > 0xffff)
+			continue;
 
+		actual_baud = port->uartclk / scr / divisor;
+		if (abs(actual_baud - baud) < 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.25.1

Re: [PATCH 1/3] serial: 8250_pci: rewrite pericom_do_set_divisor

[Greg KH]

On Sun, Nov 14, 2021 at 10:39:06AM -0800, Jay Dolan wrote:
> Fixes: 6bf4e42f1d19de10800f4483b4bb7945aab283cb

This needs to go down in the signed-off-by line area and use the format
as documented in the submitting patches documentation in the kernel.

thanks,

greg k-h

[PATCH 2/3] serial: 8250_pci: Re-enable higher baud rates on Pericom chips

[Jay Dolan]

Add UPF_MAGIC_MULTIPLIER to Pericom serial ports since there is now
range checking in serial8250_get_baud_rate() in 8250_port.c

Signed-off-by: Jay Dolan <jay.dolan@accesio.com>
---
 drivers/tty/serial/8250/8250_pci.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index aea43683b76d..5615ac1d2352 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1352,6 +1352,11 @@ pericom_do_set_divisor(struct uart_port *port, unsigned int baud,
 		}
 	}
 }
+static int pericom_do_startup(struct uart_port *port)
+{
+	port->flags |= UPF_MAGIC_MULTIPLIER;
+	return serial8250_do_startup(port);
+}
 static int pci_pericom_setup(struct serial_private *priv,
 		  const struct pciserial_board *board,
 		  struct uart_8250_port *port, int idx)
@@ -1372,6 +1377,7 @@ static int pci_pericom_setup(struct serial_private *priv,
 		return 1;
 
 	port->port.set_divisor = pericom_do_set_divisor;
+	port->port.startup = pericom_do_startup;
 
 	return setup_port(priv, port, bar, offset, board->reg_shift);
 }
@@ -1398,6 +1404,7 @@ static int pci_pericom_setup_four_at_eight(struct serial_private *priv,
 		return 1;
 
 	port->port.set_divisor = pericom_do_set_divisor;
+	port->port.startup = pericom_do_startup;
 
 	return setup_port(priv, port, bar, offset, board->reg_shift);
 }
-- 
2.25.1

Re: [PATCH 2/3] serial: 8250_pci: Re-enable higher baud rates on Pericom chips

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 8:39 PM Jay Dolan <jay.dolan@accesio.com> wrote:
>
> Add UPF_MAGIC_MULTIPLIER to Pericom serial ports since there is now
> range checking in serial8250_get_baud_rate() in 8250_port.c

The patch is good per se, but see the comment against the cover letter.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/3] serial: 8250_pci: Fix ACCES entries in pci_serial_quirks array

[Jay Dolan]

Fixes: 78d3820b9bd39028727c6aab7297b63c093db343

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.

Signed-off-by: Jay Dolan <jay.dolan@accesio.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 5615ac1d2352..3f792cb44805 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -2302,12 +2302,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.25.1

Re: [PATCH 3/3] serial: 8250_pci: Fix ACCES entries in pci_serial_quirks array

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 8:40 PM Jay Dolan <jay.dolan@accesio.com> wrote:
>
> Fixes: 78d3820b9bd39028727c6aab7297b63c093db343

Wrong position and wrong format of this tag.

> 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.

Yeah, I have a fix in my local tree, but... see comment to the cover letter.

> Signed-off-by: Jay Dolan <jay.dolan@accesio.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 5615ac1d2352..3f792cb44805 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -2302,12 +2302,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.25.1
>


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 8:40 PM Jay Dolan <jay.dolan@accesio.com> wrote:
>
> A series patches to address three issues one customer managed to hit all at once.
>
> 1) 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()
> Tested with 14.7456 and 24 MHz crystals
>
> 2) Re-enable higher baud rates on Pericom chips.
> serial8250_get_baud_rate() added range checking, but Pericom chips have a wider
> range than what is being enforced. Make use of UPF_MAGIC_MULTIPLIER.
> Tested with 14.7456 and 24 MHz crystals
>
> 3) Fourth port not being setup correctly on some Pericom chips.
> Fix entries in pci_serial_quirks array

Can we start from splitting Pericom to its own file, please?

See my initial work here [1]. And I believe you may do a better job
than me since you have access to many variants of the hardware.

[1]: https://gitlab.com/andy-shev/next/-/commit/71fdb8b5d857691031f566daebb1e850b106f46a

(As a side note:
https://gitlab.com/andy-shev/next/-/commit/ac0dc993fa35b5e2fe67e967b6a687b2e47d0edd)

-- 
With Best Regards,
Andy Shevchenko

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

[Jay Dolan]

On 11/16/21 7:34 AM, Andy Shevchenko wrote:
> On Sun, Nov 14, 2021 at 8:40 PM Jay Dolan <jay.dolan@accesio.com> wrote:
>>
>> A series patches to address three issues one customer managed to hit all at once.
>>
>> 1) 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()
>> Tested with 14.7456 and 24 MHz crystals
>>
>> 2) Re-enable higher baud rates on Pericom chips.
>> serial8250_get_baud_rate() added range checking, but Pericom chips have a wider
>> range than what is being enforced. Make use of UPF_MAGIC_MULTIPLIER.
>> Tested with 14.7456 and 24 MHz crystals
>>
>> 3) Fourth port not being setup correctly on some Pericom chips.
>> Fix entries in pci_serial_quirks array
> 
> Can we start from splitting Pericom to its own file, please?
> 
> See my initial work here [1]. And I believe you may do a better job
> than me since you have access to many variants of the hardware.
> 
> [1]: https://gitlab.com/andy-shev/next/-/commit/71fdb8b5d857691031f566daebb1e850b106f46a
That would be great. Breaking it out has been on my wish list for a 
while, but I doubt I was ever going to have time. I'm not familiar with 
GitLab. Is there a way for me to fetch this branch or should I just 
apply the diff I downloaded?
> 
> (As a side note:
> https://gitlab.com/andy-shev/next/-/commit/ac0dc993fa35b5e2fe67e967b6a687b2e47d0edd)
> 

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

[Andy Shevchenko]

On Wed, Nov 17, 2021 at 5:52 AM Jay Dolan <jay.dolan@accesio.com> wrote:
> On 11/16/21 7:34 AM, Andy Shevchenko wrote:
> > On Sun, Nov 14, 2021 at 8:40 PM Jay Dolan <jay.dolan@accesio.com> wrote:
> >>
> >> A series patches to address three issues one customer managed to hit all at once.
> >>
> >> 1) 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()
> >> Tested with 14.7456 and 24 MHz crystals
> >>
> >> 2) Re-enable higher baud rates on Pericom chips.
> >> serial8250_get_baud_rate() added range checking, but Pericom chips have a wider
> >> range than what is being enforced. Make use of UPF_MAGIC_MULTIPLIER.
> >> Tested with 14.7456 and 24 MHz crystals
> >>
> >> 3) Fourth port not being setup correctly on some Pericom chips.
> >> Fix entries in pci_serial_quirks array
> >
> > Can we start from splitting Pericom to its own file, please?
> >
> > See my initial work here [1]. And I believe you may do a better job
> > than me since you have access to many variants of the hardware.
> >
> > [1]: https://gitlab.com/andy-shev/next/-/commit/71fdb8b5d857691031f566daebb1e850b106f46a
> That would be great. Breaking it out has been on my wish list for a
> while, but I doubt I was ever going to have time. I'm not familiar with
> GitLab. Is there a way for me to fetch this branch or should I just
> apply the diff I downloaded?

I will prepare a patch series where I take your patches and
incorporate mine in between. Stay tuned!

> > (As a side note:
> > https://gitlab.com/andy-shev/next/-/commit/ac0dc993fa35b5e2fe67e967b6a687b2e47d0edd)


-- 
With Best Regards,
Andy Shevchenko