[PATCH v2 1/1] serial: 8250_of: Drop quirk fot NPCM from 8250_port

[PATCH v2 1/1] serial: 8250_of: Drop quirk fot NPCM from 8250_port

[Andy Shevchenko]

We are not supposed to spread quirks in 8250_port module especially
when we have a separate driver for the hardware in question.

Move quirk from generic module to the driver that uses it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: added missing bits.h, reworked error handling in a switch-case
 drivers/tty/serial/8250/8250_of.c   | 44 +++++++++++++++++++++++++++--
 drivers/tty/serial/8250/8250_port.c | 24 ----------------
 2 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index 34f17a9785e7..9dcc17e33269 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -4,7 +4,10 @@
  *
  *    Copyright (C) 2006 Arnd Bergmann <arnd@arndb.de>, IBM Corp.
  */
+
+#include <linux/bits.h>
 #include <linux/console.h>
+#include <linux/math.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/serial_core.h>
@@ -25,6 +28,36 @@ struct of_serial_info {
 	int line;
 };
 
+/* Nuvoton NPCM timeout register */
+#define UART_NPCM_TOR          7
+#define UART_NPCM_TOIE         BIT(7)  /* Timeout Interrupt Enable */
+
+static int npcm_startup(struct uart_port *port)
+{
+	/*
+	 * Nuvoton calls the scratch register 'UART_TOR' (timeout
+	 * register). Enable it, and set TIOC (timeout interrupt
+	 * comparator) to be 0x20 for correct operation.
+	 */
+	serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20);
+
+	return serial8250_do_startup(port);
+}
+
+/* Nuvoton NPCM UARTs have a custom divisor calculation */
+static unsigned int npcm_get_divisor(struct uart_port *port, unsigned int baud,
+				     unsigned int *frac)
+{
+	return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2;
+}
+
+static int npcm_setup(struct uart_port *port)
+{
+	port->get_divisor = npcm_get_divisor;
+	port->startup = npcm_startup;
+	return 0;
+}
+
 /*
  * Fill a struct uart_port for a given device node
  */
@@ -164,10 +197,17 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
 	switch (type) {
 	case PORT_RT2880:
 		ret = rt288x_setup(port);
-		if (ret)
-			goto err_pmruntime;
+		break;
+	case PORT_NPCM:
+		ret = npcm_setup(port);
+		break;
+	default:
+		/* Nothing to do */
+		ret = 0;
 		break;
 	}
+	if (ret)
+		goto err_pmruntime;
 
 	if (IS_REACHABLE(CONFIG_SERIAL_8250_FSL) &&
 	    (of_device_is_compatible(np, "fsl,ns16550") ||
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 175a0b07589c..2699ccf5bfce 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -38,10 +38,6 @@
 
 #include "8250.h"
 
-/* Nuvoton NPCM timeout register */
-#define UART_NPCM_TOR          7
-#define UART_NPCM_TOIE         BIT(7)  /* Timeout Interrupt Enable */
-
 /*
  * Debugging.
  */
@@ -2206,15 +2202,6 @@ int serial8250_do_startup(struct uart_port *port)
 				UART_DA830_PWREMU_MGMT_FREE);
 	}
 
-	if (port->type == PORT_NPCM) {
-		/*
-		 * Nuvoton calls the scratch register 'UART_TOR' (timeout
-		 * register). Enable it, and set TIOC (timeout interrupt
-		 * comparator) to be 0x20 for correct operation.
-		 */
-		serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20);
-	}
-
 #ifdef CONFIG_SERIAL_8250_RSA
 	/*
 	 * If this is an RSA port, see if we can kick it up to the
@@ -2508,15 +2495,6 @@ static void serial8250_shutdown(struct uart_port *port)
 		serial8250_do_shutdown(port);
 }
 
-/* Nuvoton NPCM UARTs have a custom divisor calculation */
-static unsigned int npcm_get_divisor(struct uart_8250_port *up,
-		unsigned int baud)
-{
-	struct uart_port *port = &up->port;
-
-	return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2;
-}
-
 static unsigned int serial8250_do_get_divisor(struct uart_port *port,
 					      unsigned int baud,
 					      unsigned int *frac)
@@ -2561,8 +2539,6 @@ static unsigned int serial8250_do_get_divisor(struct uart_port *port,
 		quot = 0x8001;
 	else if (magic_multiplier && baud >= port->uartclk / 12)
 		quot = 0x8002;
-	else if (up->port.type == PORT_NPCM)
-		quot = npcm_get_divisor(up, baud);
 	else
 		quot = uart_get_divisor(port, baud);
 
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 1/1] serial: 8250_of: Drop quirk fot NPCM from 8250_port

[Ilpo Järvinen]

On Thu, 15 Feb 2024, Andy Shevchenko wrote:

> We are not supposed to spread quirks in 8250_port module especially
> when we have a separate driver for the hardware in question.
> 
> Move quirk from generic module to the driver that uses it.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: added missing bits.h, reworked error handling in a switch-case
>  drivers/tty/serial/8250/8250_of.c   | 44 +++++++++++++++++++++++++++--
>  drivers/tty/serial/8250/8250_port.c | 24 ----------------
>  2 files changed, 42 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
> index 34f17a9785e7..9dcc17e33269 100644
> --- a/drivers/tty/serial/8250/8250_of.c
> +++ b/drivers/tty/serial/8250/8250_of.c
> @@ -4,7 +4,10 @@
>   *
>   *    Copyright (C) 2006 Arnd Bergmann <arnd@arndb.de>, IBM Corp.
>   */
> +
> +#include <linux/bits.h>
>  #include <linux/console.h>
> +#include <linux/math.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/serial_core.h>
> @@ -25,6 +28,36 @@ struct of_serial_info {
>  	int line;
>  };
>  
> +/* Nuvoton NPCM timeout register */
> +#define UART_NPCM_TOR          7
> +#define UART_NPCM_TOIE         BIT(7)  /* Timeout Interrupt Enable */
> +
> +static int npcm_startup(struct uart_port *port)
> +{
> +	/*
> +	 * Nuvoton calls the scratch register 'UART_TOR' (timeout
> +	 * register). Enable it, and set TIOC (timeout interrupt
> +	 * comparator) to be 0x20 for correct operation.
> +	 */
> +	serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20);
> +
> +	return serial8250_do_startup(port);

I know this matches how it is currently done but I wonder if TOIE 
should not be enabled until ->setup_irq() has been called.

-- 
 i.

Re: [PATCH v2 1/1] serial: 8250_of: Drop quirk fot NPCM from 8250_port

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 06:40:15PM +0200, Ilpo Järvinen wrote:
> On Thu, 15 Feb 2024, Andy Shevchenko wrote:

...

> > +	/*
> > +	 * Nuvoton calls the scratch register 'UART_TOR' (timeout
> > +	 * register). Enable it, and set TIOC (timeout interrupt
> > +	 * comparator) to be 0x20 for correct operation.
> > +	 */
> > +	serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20);
> > +
> > +	return serial8250_do_startup(port);
> 
> I know this matches how it is currently done\

Exactly, I haven't changed the workflow.
Does it mean you are okay with the change?

> but I wonder if TOIE should not be enabled until ->setup_irq()
> has been called.

No idea, this will need an extensive test on the hardware and should
be done separately anyway. I have no HW to test this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] serial: 8250_of: Drop quirk fot NPCM from 8250_port

[Ilpo Järvinen]

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

On Thu, 15 Feb 2024, Andy Shevchenko wrote:

> On Thu, Feb 15, 2024 at 06:40:15PM +0200, Ilpo Järvinen wrote:
> > On Thu, 15 Feb 2024, Andy Shevchenko wrote:
> 
> ...
> 
> > > +	/*
> > > +	 * Nuvoton calls the scratch register 'UART_TOR' (timeout
> > > +	 * register). Enable it, and set TIOC (timeout interrupt
> > > +	 * comparator) to be 0x20 for correct operation.
> > > +	 */
> > > +	serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20);
> > > +
> > > +	return serial8250_do_startup(port);
> > 
> > I know this matches how it is currently done\
> 
> Exactly, I haven't changed the workflow.
> Does it mean you are okay with the change?

Mostly. Another thing I was let bit unsure if it's okay to move that 
serial_port_out() outside of RPM get/put that is inside 
serial8250_do_startup().

> > but I wonder if TOIE should not be enabled until ->setup_irq()
> > has been called.
> 
> No idea, this will need an extensive test on the hardware and should
> be done separately anyway. I have no HW to test this.

Okay.

-- 
 i.

Re: [PATCH v2 1/1] serial: 8250_of: Drop quirk fot NPCM from 8250_port

[Andy Shevchenko]

On Fri, Feb 16, 2024 at 11:31:01AM +0200, Ilpo Järvinen wrote:
> On Thu, 15 Feb 2024, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 06:40:15PM +0200, Ilpo Järvinen wrote:
> > > On Thu, 15 Feb 2024, Andy Shevchenko wrote:

...

> > > > +	/*
> > > > +	 * Nuvoton calls the scratch register 'UART_TOR' (timeout
> > > > +	 * register). Enable it, and set TIOC (timeout interrupt
> > > > +	 * comparator) to be 0x20 for correct operation.
> > > > +	 */
> > > > +	serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20);
> > > > +
> > > > +	return serial8250_do_startup(port);
> > > 
> > > I know this matches how it is currently done\
> > 
> > Exactly, I haven't changed the workflow.
> > Does it mean you are okay with the change?
> 
> Mostly. Another thing I was let bit unsure if it's okay to move that 
> serial_port_out() outside of RPM get/put that is inside 
> serial8250_do_startup().

We have other (actively used AFAIK) drivers which do the same.
In any case this driver does not use RPM anyway, and in the future,
when Tony finalizes his RPM work those 8250 RPM wrappers should gone.
If somebody implements RPM in this driver, it may be done via standard
RPM calls.

TL;DR: for now it's okay, I am sure.

> > > but I wonder if TOIE should not be enabled until ->setup_irq()
> > > has been called.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] serial: 8250_of: Drop quirk fot NPCM from 8250_port

[Ilpo Järvinen]

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

On Fri, 16 Feb 2024, Andy Shevchenko wrote:

> On Fri, Feb 16, 2024 at 11:31:01AM +0200, Ilpo Järvinen wrote:
> > On Thu, 15 Feb 2024, Andy Shevchenko wrote:
> > > On Thu, Feb 15, 2024 at 06:40:15PM +0200, Ilpo Järvinen wrote:
> > > > On Thu, 15 Feb 2024, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > +	/*
> > > > > +	 * Nuvoton calls the scratch register 'UART_TOR' (timeout
> > > > > +	 * register). Enable it, and set TIOC (timeout interrupt
> > > > > +	 * comparator) to be 0x20 for correct operation.
> > > > > +	 */
> > > > > +	serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20);
> > > > > +
> > > > > +	return serial8250_do_startup(port);
> > > > 
> > > > I know this matches how it is currently done\
> > > 
> > > Exactly, I haven't changed the workflow.
> > > Does it mean you are okay with the change?
> > 
> > Mostly. Another thing I was let bit unsure if it's okay to move that 
> > serial_port_out() outside of RPM get/put that is inside 
> > serial8250_do_startup().
> 
> We have other (actively used AFAIK) drivers which do the same.
> In any case this driver does not use RPM anyway, and in the future,
> when Tony finalizes his RPM work those 8250 RPM wrappers should gone.
> If somebody implements RPM in this driver, it may be done via standard
> RPM calls.
> 
> TL;DR: for now it's okay, I am sure.

Okay,

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

-- 
 i.