[PATCH v2 0/3] serial: 8250_dw: add fractional divisor support

[PATCH v2 0/3] serial: 8250_dw: add fractional divisor support

[Jisheng Zhang]

For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
valid divisor latch fraction register. The fractional divisor width is
4bits ~ 6bits.

patch1 lets serial8250_get_divisor() get uart_port * as param
patch2 introduces necessary hooks to 8250 core.
patch3 implements the fractional divisor support for Synopsys DW 8250.

Since v1:
 - add an extra patch to let serial8250_get_divisor() get uart_port *
   as param
 - take Andy's suggestions to "integrates hooks in the same way like
   it's done for the rest of 8250 ones". Many thanks to Andy.

Jisheng Zhang (3):
  serial: 8250: let serial8250_get_divisor() get uart_port * as param
  serial: 8250: introduce get_divisor() and set_divisor() hook
  serial: 8250_dw: add fractional divisor support

 drivers/tty/serial/8250/8250_core.c |  4 +++
 drivers/tty/serial/8250/8250_dw.c   | 53 +++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_port.c | 33 ++++++++++++++----
 include/linux/serial_core.h         |  7 ++++
 4 files changed, 90 insertions(+), 7 deletions(-)

-- 
2.18.0

[PATCH v2 1/3] serial: 8250: let serial8250_get_divisor() get uart_port * as param

[Jisheng Zhang]

Align serial8250_get_divisor() with serial8250_set_divisor() to accept
uart_port pointer as the first parameter. No functionality changes.

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 drivers/tty/serial/8250/8250_port.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index cf541aab2bd0..709fe6b4265c 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2498,11 +2498,11 @@ static unsigned int npcm_get_divisor(struct uart_8250_port *up,
 	return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2;
 }
 
-static unsigned int serial8250_get_divisor(struct uart_8250_port *up,
+static unsigned int serial8250_get_divisor(struct uart_port *port,
 					   unsigned int baud,
 					   unsigned int *frac)
 {
-	struct uart_port *port = &up->port;
+	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned int quot;
 
 	/*
@@ -2636,7 +2636,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	cval = serial8250_compute_lcr(up, termios->c_cflag);
 
 	baud = serial8250_get_baud_rate(port, termios, old);
-	quot = serial8250_get_divisor(up, baud, &frac);
+	quot = serial8250_get_divisor(port, baud, &frac);
 
 	/*
 	 * Ok, we're now changing the port state.  Do it with
@@ -3197,7 +3197,7 @@ static void serial8250_console_restore(struct uart_8250_port *up)
 		termios.c_cflag = port->state->port.tty->termios.c_cflag;
 
 	baud = serial8250_get_baud_rate(port, &termios, NULL);
-	quot = serial8250_get_divisor(up, baud, &frac);
+	quot = serial8250_get_divisor(port, baud, &frac);
 
 	serial8250_set_divisor(port, baud, quot, frac);
 	serial_port_out(port, UART_LCR, up->lcr);
-- 
2.18.0

Re: [PATCH v2 1/3] serial: 8250: let serial8250_get_divisor() get uart_port * as param

[Andy Shevchenko]

On Wed, 2018-07-04 at 17:00 +0800, Jisheng Zhang wrote:
> Align serial8250_get_divisor() with serial8250_set_divisor() to accept
> uart_port pointer as the first parameter. No functionality changes.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c
> b/drivers/tty/serial/8250/8250_port.c
> index cf541aab2bd0..709fe6b4265c 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2498,11 +2498,11 @@ static unsigned int npcm_get_divisor(struct
> uart_8250_port *up,
>  	return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2;
>  }
>  
> -static unsigned int serial8250_get_divisor(struct uart_8250_port *up,
> +static unsigned int serial8250_get_divisor(struct uart_port *port,
>  					   unsigned int baud,
>  					   unsigned int *frac)
>  {
> -	struct uart_port *port = &up->port;
> +	struct uart_8250_port *up = up_to_u8250p(port);
>  	unsigned int quot;
>  
>  	/*
> @@ -2636,7 +2636,7 @@ serial8250_do_set_termios(struct uart_port
> *port, struct ktermios *termios,
>  	cval = serial8250_compute_lcr(up, termios->c_cflag);
>  
>  	baud = serial8250_get_baud_rate(port, termios, old);
> -	quot = serial8250_get_divisor(up, baud, &frac);
> +	quot = serial8250_get_divisor(port, baud, &frac);
>  
>  	/*
>  	 * Ok, we're now changing the port state.  Do it with
> @@ -3197,7 +3197,7 @@ static void serial8250_console_restore(struct
> uart_8250_port *up)
>  		termios.c_cflag = port->state->port.tty-
> >termios.c_cflag;
>  
>  	baud = serial8250_get_baud_rate(port, &termios, NULL);
> -	quot = serial8250_get_divisor(up, baud, &frac);
> +	quot = serial8250_get_divisor(port, baud, &frac);
>  
>  	serial8250_set_divisor(port, baud, quot, frac);
>  	serial_port_out(port, UART_LCR, up->lcr);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v2 2/3] serial: 8250: introduce get_divisor() and set_divisor() hook

[Jisheng Zhang]

Add these two hooks so that they can be overridden with driver specific
implementations.

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 drivers/tty/serial/8250/8250_core.c |  4 ++++
 drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++++----
 include/linux/serial_core.h         |  7 +++++++
 3 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 9342fc2ee7df..a0bb77290747 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1023,6 +1023,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 			uart->port.get_mctrl = up->port.get_mctrl;
 		if (up->port.set_mctrl)
 			uart->port.set_mctrl = up->port.set_mctrl;
+		if (up->port.get_divisor)
+			uart->port.get_divisor = up->port.get_divisor;
+		if (up->port.set_divisor)
+			uart->port.set_divisor = up->port.set_divisor;
 		if (up->port.startup)
 			uart->port.startup = up->port.startup;
 		if (up->port.shutdown)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 709fe6b4265c..ce0dc17f18ee 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2498,9 +2498,9 @@ static unsigned int npcm_get_divisor(struct uart_8250_port *up,
 	return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2;
 }
 
-static unsigned int serial8250_get_divisor(struct uart_port *port,
-					   unsigned int baud,
-					   unsigned int *frac)
+static unsigned int serial8250_do_get_divisor(struct uart_port *port,
+					      unsigned int baud,
+					      unsigned int *frac)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned int quot;
@@ -2532,6 +2532,16 @@ static unsigned int serial8250_get_divisor(struct uart_port *port,
 	return quot;
 }
 
+static unsigned int serial8250_get_divisor(struct uart_port *port,
+					   unsigned int baud,
+					   unsigned int *frac)
+{
+	if (port->get_divisor)
+		return port->get_divisor(port, baud, frac);
+
+	return serial8250_do_get_divisor(port, baud, frac);
+}
+
 static unsigned char serial8250_compute_lcr(struct uart_8250_port *up,
 					    tcflag_t c_cflag)
 {
@@ -2570,7 +2580,7 @@ static unsigned char serial8250_compute_lcr(struct uart_8250_port *up,
 	return cval;
 }
 
-static void serial8250_set_divisor(struct uart_port *port, unsigned int baud,
+static void serial8250_do_set_divisor(struct uart_port *port, unsigned int baud,
 			    unsigned int quot, unsigned int quot_frac)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
@@ -2603,6 +2613,15 @@ static void serial8250_set_divisor(struct uart_port *port, unsigned int baud,
 	}
 }
 
+static void serial8250_set_divisor(struct uart_port *port, unsigned int baud,
+				   unsigned int quot, unsigned int quot_frac)
+{
+	if (port->set_divisor)
+		port->set_divisor(port, baud, quot, quot_frac);
+	else
+		serial8250_do_set_divisor(port, baud, quot, quot_frac);
+}
+
 static unsigned int serial8250_get_baud_rate(struct uart_port *port,
 					     struct ktermios *termios,
 					     struct ktermios *old)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 06ea4eeb09ab..406edae44ca3 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -127,6 +127,13 @@ struct uart_port {
 					     struct ktermios *);
 	unsigned int		(*get_mctrl)(struct uart_port *);
 	void			(*set_mctrl)(struct uart_port *, unsigned int);
+	unsigned int		(*get_divisor)(struct uart_port *,
+					       unsigned int baud,
+					       unsigned int *frac);
+	void			(*set_divisor)(struct uart_port *,
+					       unsigned int baud,
+					       unsigned int quot,
+					       unsigned int quot_frac);
 	int			(*startup)(struct uart_port *port);
 	void			(*shutdown)(struct uart_port *port);
 	void			(*throttle)(struct uart_port *port);
-- 
2.18.0

Re: [PATCH v2 2/3] serial: 8250: introduce get_divisor() and set_divisor() hook

[Andy Shevchenko]

On Wed, 2018-07-04 at 17:02 +0800, Jisheng Zhang wrote:
> Add these two hooks so that they can be overridden with driver
> specific
> implementations.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> ---
>  drivers/tty/serial/8250/8250_core.c |  4 ++++
>  drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++++----
>  include/linux/serial_core.h         |  7 +++++++
>  3 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c
> b/drivers/tty/serial/8250/8250_core.c
> index 9342fc2ee7df..a0bb77290747 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1023,6 +1023,10 @@ int serial8250_register_8250_port(struct
> uart_8250_port *up)
>  			uart->port.get_mctrl = up->port.get_mctrl;
>  		if (up->port.set_mctrl)
>  			uart->port.set_mctrl = up->port.set_mctrl;
> +		if (up->port.get_divisor)
> +			uart->port.get_divisor = up-
> >port.get_divisor;
> +		if (up->port.set_divisor)
> +			uart->port.set_divisor = up-
> >port.set_divisor;
>  		if (up->port.startup)
>  			uart->port.startup = up->port.startup;
>  		if (up->port.shutdown)
> diff --git a/drivers/tty/serial/8250/8250_port.c
> b/drivers/tty/serial/8250/8250_port.c
> index 709fe6b4265c..ce0dc17f18ee 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2498,9 +2498,9 @@ static unsigned int npcm_get_divisor(struct
> uart_8250_port *up,
>  	return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2;
>  }
>  
> -static unsigned int serial8250_get_divisor(struct uart_port *port,
> -					   unsigned int baud,
> -					   unsigned int *frac)
> +static unsigned int serial8250_do_get_divisor(struct uart_port *port,
> +					      unsigned int baud,
> +					      unsigned int *frac)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(port);
>  	unsigned int quot;
> @@ -2532,6 +2532,16 @@ static unsigned int
> serial8250_get_divisor(struct uart_port *port,
>  	return quot;
>  }
>  
> +static unsigned int serial8250_get_divisor(struct uart_port *port,
> +					   unsigned int baud,
> +					   unsigned int *frac)
> +{
> +	if (port->get_divisor)
> +		return port->get_divisor(port, baud, frac);
> +
> +	return serial8250_do_get_divisor(port, baud, frac);
> +}
> +
>  static unsigned char serial8250_compute_lcr(struct uart_8250_port
> *up,
>  					    tcflag_t c_cflag)
>  {
> @@ -2570,7 +2580,7 @@ static unsigned char
> serial8250_compute_lcr(struct uart_8250_port *up,
>  	return cval;
>  }
>  
> -static void serial8250_set_divisor(struct uart_port *port, unsigned
> int baud,
> +static void serial8250_do_set_divisor(struct uart_port *port,
> unsigned int baud,
>  			    unsigned int quot, unsigned int
> quot_frac)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(port);
> @@ -2603,6 +2613,15 @@ static void serial8250_set_divisor(struct
> uart_port *port, unsigned int baud,
>  	}
>  }
>  
> +static void serial8250_set_divisor(struct uart_port *port, unsigned
> int baud,
> +				   unsigned int quot, unsigned int
> quot_frac)
> +{
> +	if (port->set_divisor)
> +		port->set_divisor(port, baud, quot, quot_frac);
> +	else
> +		serial8250_do_set_divisor(port, baud, quot,
> quot_frac);
> +}
> +
>  static unsigned int serial8250_get_baud_rate(struct uart_port *port,
>  					     struct ktermios
> *termios,
>  					     struct ktermios *old)
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 06ea4eeb09ab..406edae44ca3 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -127,6 +127,13 @@ struct uart_port {
>  					     struct ktermios *);
>  	unsigned int		(*get_mctrl)(struct uart_port *);
>  	void			(*set_mctrl)(struct uart_port *,
> unsigned int);
> +	unsigned int		(*get_divisor)(struct uart_port
> *,
> +					       unsigned int baud,
> +					       unsigned int *frac);
> +	void			(*set_divisor)(struct uart_port
> *,
> +					       unsigned int baud,
> +					       unsigned int quot,
> +					       unsigned int
> quot_frac);
>  	int			(*startup)(struct uart_port
> *port);
>  	void			(*shutdown)(struct uart_port
> *port);
>  	void			(*throttle)(struct uart_port
> *port);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v2 3/3] serial: 8250_dw: add fractional divisor support

[Jisheng Zhang]

For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
valid divisor latch fraction register. The fractional divisor width is
4bits ~ 6bits.

Now the preparation is done, it's easy to add the feature support.
This patch firstly checks the component version during probe, if
version >= 4.00a, then calculates the fractional divisor width, then
setups dw specific get_divisor() and set_divisor() hook.

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 drivers/tty/serial/8250/8250_dw.c | 53 +++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index aff04f1de3a5..b9630f633388 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -31,9 +31,12 @@
 
 /* Offsets for the DesignWare specific registers */
 #define DW_UART_USR	0x1f /* UART Status Register */
+#define DW_UART_DLF	0xc0 /* Divisor Latch Fraction Register */
 #define DW_UART_CPR	0xf4 /* Component Parameter Register */
 #define DW_UART_UCV	0xf8 /* UART Component Version */
 
+#define DW_FRAC_MIN_VERS		0x3430302A
+
 /* Component Parameter Register bits */
 #define DW_UART_CPR_ABP_DATA_WIDTH	(3 << 0)
 #define DW_UART_CPR_AFCE_MODE		(1 << 4)
@@ -65,6 +68,7 @@ struct dw8250_data {
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
+	unsigned int		dlf_size:3;
 };
 
 static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)
@@ -351,6 +355,33 @@ static bool dw8250_idma_filter(struct dma_chan *chan, void *param)
 	return param == chan->device->dev->parent;
 }
 
+/*
+ * For version >= 4.00a, the dw uarts have a valid divisor latch fraction
+ * register. Calculate divisor with extra 4bits ~ 6bits fractional portion.
+ */
+static unsigned int dw8250_get_divisor(struct uart_port *p,
+				       unsigned int baud,
+				       unsigned int *frac)
+{
+	unsigned int quot;
+	struct dw8250_data *d = p->private_data;
+
+	quot = DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4), baud);
+	*frac = quot & (~0U >> (32 - d->dlf_size));
+
+	return quot >> d->dlf_size;
+}
+
+static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
+			       unsigned int quot, unsigned int quot_frac)
+{
+	struct uart_8250_port *up = up_to_u8250p(p);
+
+	serial_port_out(p, DW_UART_DLF >> p->regshift, quot_frac);
+	serial_port_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
+	serial_dl_write(up, quot);
+}
+
 static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 {
 	if (p->dev->of_node) {
@@ -414,6 +445,28 @@ static void dw8250_setup_port(struct uart_port *p)
 	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
 		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
 
+	/*
+	 * For version >= 4.00a, the dw uarts have a valid divisor latch
+	 * fraction register. Calculate the fractional divisor width.
+	 */
+	if (reg >= DW_FRAC_MIN_VERS) {
+		struct dw8250_data *d = p->private_data;
+
+		if (p->iotype == UPIO_MEM32BE) {
+			iowrite32be(~0U, p->membase + DW_UART_DLF);
+			reg = ioread32be(p->membase + DW_UART_DLF);
+		} else {
+			writel(~0U, p->membase + DW_UART_DLF);
+			reg = readl(p->membase + DW_UART_DLF);
+		}
+		d->dlf_size = fls(reg);
+
+		if (d->dlf_size) {
+			p->get_divisor = dw8250_get_divisor;
+			p->set_divisor = dw8250_set_divisor;
+		}
+	}
+
 	if (p->iotype == UPIO_MEM32BE)
 		reg = ioread32be(p->membase + DW_UART_CPR);
 	else
-- 
2.18.0

Re: [PATCH v2 3/3] serial: 8250_dw: add fractional divisor support

[Andy Shevchenko]

On Wed, 2018-07-04 at 17:03 +0800, Jisheng Zhang wrote:

Thanks for an update, my comments below.

> For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
> valid divisor latch fraction register. The fractional divisor width is
> 4bits ~ 6bits.

I have read 4.00a spec a bit and didn't find this limitation.
The fractional divider can fit up to 32 bits.

> Now the preparation is done, it's easy to add the feature support.
> This patch firstly checks the component version during probe, if
> version >= 4.00a, then calculates the fractional divisor width, then
> setups dw specific get_divisor() and set_divisor() hook.
 
> +#define DW_FRAC_MIN_VERS		0x3430302A

Do we really need this? 

My intuition (I, unfortunately, didn't find any evidence in Synopsys
specs for UART) tells me that when it's unimplemented we would read back
0's, which is fine.

I would test this a bit later.

> 

> +	unsigned int		dlf_size:3;

These bit fields (besides the fact you need 5) more or less for software
quirk flags. In your case I would rather keep a u32 value of DFL mask as
done for msr slightly above in this structure.

>  };
>  
> +/*
> + * For version >= 4.00a, the dw uarts have a valid divisor latch
> fraction
> + * register. Calculate divisor with extra 4bits ~ 6bits fractional
> portion.
> + */

This comment kinda noise. Better to put actual formula from datasheet
how this fractional divider is involved into calculus.

> +static unsigned int dw8250_get_divisor(struct uart_port *p,
> +				       unsigned int baud,
> +				       unsigned int *frac)
> +{
> +	unsigned int quot;
> +	struct dw8250_data *d = p->private_data;
> +

> +	quot = DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4),
> baud);

If we have clock rate like 100MHz and 10 bits of fractional divider it
would give an integer overflow.

4 here is a magic. Needs to be commented / described better.

> +	*frac = quot & (~0U >> (32 - d->dlf_size));
> +

Operating on dfl_mask is simple as

u64 quot = p->uartclk * (p->dfl_mask + 1);

*frac = div64_u64(quot, baud * 16 * (p->dfl_mask + 1);
return quot;

(Perhaps some magic with types is needed, but you get the idea)

> +	return quot >> d->dlf_size;
> +}
> +
> +static void dw8250_set_divisor(struct uart_port *p, unsigned int
> baud,
> +			       unsigned int quot, unsigned int
> quot_frac)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(p);
> +
> +	serial_port_out(p, DW_UART_DLF >> p->regshift, quot_frac);

It should use the helper, not playing tricks with serial_port_out().

> +	serial_port_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> +	serial_dl_write(up, quot);

At some point it would be a helper, I think. We can call
serial8250_do_set_divisor() here. So, perhaps we might export it.

> +}
> +
>  static void dw8250_quirks(struct uart_port *p, struct dw8250_data
> *data)
>  {
>  	if (p->dev->of_node) {
> @@ -414,6 +445,28 @@ static void dw8250_setup_port(struct uart_port
> *p)
>  	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
>  		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) &
> 0xff);
>  
> +	/*
> +	 * For version >= 4.00a, the dw uarts have a valid divisor
> latch
> +	 * fraction register. Calculate the fractional divisor width.
> +	 */
> +	if (reg >= DW_FRAC_MIN_VERS) {
> +		struct dw8250_data *d = p->private_data;
> +

> +		if (p->iotype == UPIO_MEM32BE) {
> +			iowrite32be(~0U, p->membase + DW_UART_DLF);
> +			reg = ioread32be(p->membase + DW_UART_DLF);
> +		} else {
> +			writel(~0U, p->membase + DW_UART_DLF);
> +			reg = readl(p->membase + DW_UART_DLF);
> +		}

This should use some helpers. I'll prepare a patch soon and send it
here, you may include it in your series.

I think you need to clean up back them. So the flow like

1. Write all 1:s
2. Read back the value
3. Write all 0:s

> +		d->dlf_size = fls(reg);

Just save value itself as dfl_mask.

> +
> +		if (d->dlf_size) {
> +			p->get_divisor = dw8250_get_divisor;
> +			p->set_divisor = dw8250_set_divisor;
> +		}
> +	}
> +
>  	if (p->iotype == UPIO_MEM32BE)
>  		reg = ioread32be(p->membase + DW_UART_CPR);
>  	else

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 3/3] serial: 8250_dw: add fractional divisor support

[Jisheng Zhang]

Hi Andy,

On Wed, 04 Jul 2018 19:08:22 +0300 Andy Shevchenko wrote:

> On Wed, 2018-07-04 at 17:03 +0800, Jisheng Zhang wrote:
> 
> Thanks for an update, my comments below.
> 
> > For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
> > valid divisor latch fraction register. The fractional divisor width is
> > 4bits ~ 6bits.  
> 
> I have read 4.00a spec a bit and didn't find this limitation.
> The fractional divider can fit up to 32 bits.

the limitation isn't put in the register description, but in the description
about fractional divisor width configure parameters. Searching DLF_SIZE will
find it.

From another side, 32bits fractional div is a waste, for example, let's
assume the fract divisor = 0.9, 
if fractional width is 4 bits, DLF reg will be set to UP(0.9*2^4) = 15, the
real frac divisor =  15/2^4 = 0.93;
if fractional width is 32 bits, DLF reg will be set to UP(0.9*2^32) = 3865470567
the real frac divisor = 3865470567/2^32 = 0.90;

> 
> > Now the preparation is done, it's easy to add the feature support.
> > This patch firstly checks the component version during probe, if
> > version >= 4.00a, then calculates the fractional divisor width, then
> > setups dw specific get_divisor() and set_divisor() hook.  
>  
> > +#define DW_FRAC_MIN_VERS		0x3430302A  
> 
> Do we really need this? 
> 
> My intuition (I, unfortunately, didn't find any evidence in Synopsys
> specs for UART) tells me that when it's unimplemented we would read back
> 0's, which is fine.

yeah, I agree with you. I will remove this version check in the new version

> 
> I would test this a bit later.
> 
> >   
> 
> > +	unsigned int		dlf_size:3;  
> 
> These bit fields (besides the fact you need 5) more or less for software
> quirk flags. In your case I would rather keep a u32 value of DFL mask as
> done for msr slightly above in this structure.

OK. When setting the frac div, we use DLF size rather than mask, so could
I only calculate the DLF size once then use an u8 value to hold the
calculated DLF size rather than calculating every time?

> 
> >  };
> >  
> > +/*
> > + * For version >= 4.00a, the dw uarts have a valid divisor latch
> > fraction
> > + * register. Calculate divisor with extra 4bits ~ 6bits fractional
> > portion.
> > + */  
> 
> This comment kinda noise. Better to put actual formula from datasheet
> how this fractional divider is involved into calculus.

yeah. Will do.

> 
> > +static unsigned int dw8250_get_divisor(struct uart_port *p,
> > +				       unsigned int baud,
> > +				       unsigned int *frac)
> > +{
> > +	unsigned int quot;
> > +	struct dw8250_data *d = p->private_data;
> > +  
> 
> > +	quot = DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4),
> > baud);  
> 
> If we have clock rate like 100MHz and 10 bits of fractional divider it
> would give an integer overflow.

This depends on the fraction divider width. If it's only 4~6 bits,
then we are fine.

> 
> 4 here is a magic. Needs to be commented / described better.

Yes. divisor = clk/(16*baud) = BRD(I) + BRD(F)
"I" means integer, "F" means fractional

2^dlf_size*clk/(16*baud) = 2^dlf_size*(BRD(I) + BRD(F))

clk*2^(dlf_size - 4)/baud = 2^dlf_size*((BRD(I) + BRD(F))

the left part is "DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4), baud)",
let's assume it equals quot.

then, BRD(I) = quot / 2^dlf_size = quot >> dlf_size, this is where
"quot >> d->dlf_size" below from.

BRD(F) = quot & dlf_mask = quot & (~0U >> (32 - dlf_size))

> 
> > +	*frac = quot & (~0U >> (32 - d->dlf_size));
> > +  
> 
> Operating on dfl_mask is simple as
> 
> u64 quot = p->uartclk * (p->dfl_mask + 1);

Since the dlf_mask is always 2^n - 1, 
clk * (p->dlf_mask + 1) = clk << p->dlf_size, but the later
is simpler

> 
> *frac = div64_u64(quot, baud * 16 * (p->dfl_mask + 1);
> return quot;

quot = DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4), baud)
*frac = quot & (~0U >> (32 - d->dlf_size))
return quot >> d->dlf_size;

vs.

quot = p->uartclk * (p->dfl_mask + 1);
*frac = div64_u64(quot, baud * 16 * (p->dfl_mask + 1);
return quot;

shift vs mul.

If the dlf width is only 4~6 bits, the first calculation can avoid
64bit div. I prefer the first calculation.

> 
> (Perhaps some magic with types is needed, but you get the idea)
> 
> > +	return quot >> d->dlf_size;
> > +}
> > +
> > +static void dw8250_set_divisor(struct uart_port *p, unsigned int
> > baud,
> > +			       unsigned int quot, unsigned int
> > quot_frac)
> > +{
> > +	struct uart_8250_port *up = up_to_u8250p(p);
> > +
> > +	serial_port_out(p, DW_UART_DLF >> p->regshift, quot_frac);  
> 
> It should use the helper, not playing tricks with serial_port_out().

I assume the helper here means the one you mentioned below, i.e in

if (p->iotype == UPIO_MEM32BE) {
	...
} else {
	...
}

> 
> > +	serial_port_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> > +	serial_dl_write(up, quot);  
> 
> At some point it would be a helper, I think. We can call
> serial8250_do_set_divisor() here. So, perhaps we might export it.

serial8250_do_set_divisor will drop the frac, that's not we want ;)

> 
> > +}
> > +
> >  static void dw8250_quirks(struct uart_port *p, struct dw8250_data
> > *data)
> >  {
> >  	if (p->dev->of_node) {
> > @@ -414,6 +445,28 @@ static void dw8250_setup_port(struct uart_port
> > *p)
> >  	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
> >  		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) &
> > 0xff);
> >  
> > +	/*
> > +	 * For version >= 4.00a, the dw uarts have a valid divisor
> > latch
> > +	 * fraction register. Calculate the fractional divisor width.
> > +	 */
> > +	if (reg >= DW_FRAC_MIN_VERS) {
> > +		struct dw8250_data *d = p->private_data;
> > +  
> 
> > +		if (p->iotype == UPIO_MEM32BE) {
> > +			iowrite32be(~0U, p->membase + DW_UART_DLF);
> > +			reg = ioread32be(p->membase + DW_UART_DLF);
> > +		} else {
> > +			writel(~0U, p->membase + DW_UART_DLF);
> > +			reg = readl(p->membase + DW_UART_DLF);
> > +		}  
> 
> This should use some helpers. I'll prepare a patch soon and send it
> here, you may include it in your series.

Nice. Thanks.

> 
> I think you need to clean up back them. So the flow like
> 
> 1. Write all 1:s
> 2. Read back the value
> 3. Write all 0:s

oh, yeah! will do

> 
> > +		d->dlf_size = fls(reg);  
> 
> Just save value itself as dfl_mask.

we use the dlf size during calculation, so it's better to hold the dlf_size
instead.

Thanks for the kind review,
Jisheng

Re: [PATCH v2 3/3] serial: 8250_dw: add fractional divisor support

[Jisheng Zhang]

On Thu, 5 Jul 2018 14:39:21 +0800 Jisheng Zhang wrote:

<snip>

> >   
> > > +	serial_port_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> > > +	serial_dl_write(up, quot);    
> > 
> > At some point it would be a helper, I think. We can call
> > serial8250_do_set_divisor() here. So, perhaps we might export it.  
> 
> serial8250_do_set_divisor will drop the frac, that's not we want ;)
> 

And most importantly, serial8250_do_set_divisor() will set a wrong BRD(I)
for fractional capable DW uarts. For example, clk = 25MHZ, baud = 115200.

In fractional capable DW uarts, we should set BRD(I) as
25000000/(16*115200) = 13

but serial8250_do_set_divisor() will set BRD(I) as
DIV_ROUND_CLOSEST(25*1000000, 16*115200)) = 14

Thanks

Re: [PATCH v2 3/3] serial: 8250_dw: add fractional divisor support

[Andy Shevchenko]

On Thu, 2018-07-05 at 14:54 +0800, Jisheng Zhang wrote:
> On Thu, 5 Jul 2018 14:39:21 +0800 Jisheng Zhang wrote:
> 
> <snip>
> 
> > >   
> > > > +	serial_port_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> > > > +	serial_dl_write(up, quot);    
> > > 
> > > At some point it would be a helper, I think. We can call
> > > serial8250_do_set_divisor() here. So, perhaps we might export
> > > it.  
> > 
> > serial8250_do_set_divisor will drop the frac, that's not we want ;)
> > 
> 
> And most importantly, serial8250_do_set_divisor() will set a wrong
> BRD(I)
> for fractional capable DW uarts. For example, clk = 25MHZ, baud =
> 115200.
> 
> In fractional capable DW uarts, we should set BRD(I) as
> 25000000/(16*115200) = 13
> 
> but serial8250_do_set_divisor() will set BRD(I) as
> DIV_ROUND_CLOSEST(25*1000000, 16*115200)) = 14

How come? It doesn't do any calculus (for DW 8250), it just writes few
registers based on input.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 3/3] serial: 8250_dw: add fractional divisor support

[Andy Shevchenko]

On Thu, 2018-07-05 at 14:39 +0800, Jisheng Zhang wrote:
> > On Wed, 2018-07-04 at 17:03 +0800, Jisheng Zhang wrote:

My comments below.

> > > For Synopsys DesignWare 8250 uart which version >= 4.00a, there's
> > > a
> > > valid divisor latch fraction register. The fractional divisor
> > > width is
> > > 4bits ~ 6bits.  
> > 
> > I have read 4.00a spec a bit and didn't find this limitation.
> > The fractional divider can fit up to 32 bits.
> 
> the limitation isn't put in the register description, but in the
> description
> about fractional divisor width configure parameters. Searching
> DLF_SIZE will
> find it.

Found, thanks.

> From another side, 32bits fractional div is a waste, for example,
> let's
> assume the fract divisor = 0.9, 
> if fractional width is 4 bits, DLF reg will be set to UP(0.9*2^4) =
> 15, the
> real frac divisor =  15/2^4 = 0.93;
> if fractional width is 32 bits, DLF reg will be set to UP(0.9*2^32) =
> 3865470567
> the real frac divisor = 3865470567/2^32 = 0.90;

So, your example shows that 32-bit gives better value. Where is
contradiction?

> > I would test this a bit later.

It reads back 0 on our hardware with 3.xx version of IP.
  
> > > +	unsigned int		dlf_size:3;  
> > 
> > These bit fields (besides the fact you need 5) more or less for
> > software
> > quirk flags. In your case I would rather keep a u32 value of DFL
> > mask as
> > done for msr slightly above in this structure.
> 
> OK. When setting the frac div, we use DLF size rather than mask, so
> could
> I only calculate the DLF size once then use an u8 value to hold the
> calculated DLF size rather than calculating every time?

Let's see below.

> > > +	quot = DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4),
> > > baud);  
> > 
> > If we have clock rate like 100MHz and 10 bits of fractional divider
> > it
> > would give an integer overflow.
> 
> This depends on the fraction divider width. If it's only 4~6 bits,
> then we are fine.

True.

> > 
> > 4 here is a magic. Needs to be commented / described better.
> 
> Yes. divisor = clk/(16*baud) = BRD(I) + BRD(F)
> "I" means integer, "F" means fractional
> 
> 2^dlf_size*clk/(16*baud) = 2^dlf_size*(BRD(I) + BRD(F))
> 
> clk*2^(dlf_size - 4)/baud = 2^dlf_size*((BRD(I) + BRD(F))
> 
> the left part is "DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4),
> baud)",
> let's assume it equals quot.
> 
> then, BRD(I) = quot / 2^dlf_size = quot >> dlf_size, this is where
> "quot >> d->dlf_size" below from.
> 
> BRD(F) = quot & dlf_mask = quot & (~0U >> (32 - dlf_size))

Yes, I understand from where it comes. It's a hard coded value of PS all
over the serial code.

And better use the same idiom as in other code, i.e. * 16 or / 16
depends on context.

> > > +	*frac = quot & (~0U >> (32 - d->dlf_size));
> > > +  
> > 
> > Operating on dfl_mask is simple as
> > 
> > u64 quot = p->uartclk * (p->dfl_mask + 1);
> 
> Since the dlf_mask is always 2^n - 1, 
> clk * (p->dlf_mask + 1) = clk << p->dlf_size, but the later
> is simpler
> 
> > 
> > *frac = div64_u64(quot, baud * 16 * (p->dfl_mask + 1);
> > return quot;
> 
> quot = DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4), baud)
> *frac = quot & (~0U >> (32 - d->dlf_size))
> return quot >> d->dlf_size;
> 
> vs.
> 
> quot = p->uartclk * (p->dfl_mask + 1);
> *frac = div64_u64(quot, baud * 16 * (p->dfl_mask + 1);
> return quot;
> 
> shift vs mul.
> 
> If the dlf width is only 4~6 bits, the first calculation can avoid
> 64bit div. I prefer the first calculation.

OK, taking that into consideration and looking at the code snippets
again I would to
 - keep two values

// mask we get for free because it's needed in intermediate calculus
//
and it doesn't overflow u8 (4-6 bits by spec)
u8 dlf_mask;
u8 dlf_size;

 - implement function like below

unsigned int quot;

/* Consider maximum possible DLF_SIZE, i.e. 6 bits */
quot = DIV_ROUND_CLOSEST(p->uartclk * 4, baud);

*frac = (quot >> (6 - dlf_size)) & dlf_mask;
return quot >> dlf_size;

Would you agree it looks slightly less complicated?

> > > +	serial_port_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> > > +	serial_dl_write(up, quot);  

(1)

> > 
> > At some point it would be a helper, I think. We can call
> > serial8250_do_set_divisor() here. So, perhaps we might export it.
> 
> serial8250_do_set_divisor will drop the frac, that's not we want ;)

Can you check again? What I see is that for DW 8250 the
_do_set_divisor() would be an equivalent to the two lines, i.e. (1).

And basically at the end it should become just those two lines when the
rest will implement their custom set_divisor().

> > This should use some helpers. I'll prepare a patch soon and send it
> > here, you may include it in your series.
> 
> Nice. Thanks.

Sent.

> > > +		d->dlf_size = fls(reg);  
> > 
> > Just save value itself as dfl_mask.
> 
> we use the dlf size during calculation, so it's better to hold the
> dlf_size
> instead.

See above.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 3/3] serial: 8250_dw: add fractional divisor support

[Jisheng Zhang]

On Fri, 6 Jul 2018 20:37:09 +0300 Andy Shevchenko wrote:

> On Thu, 2018-07-05 at 14:39 +0800, Jisheng Zhang wrote:
> > > On Wed, 2018-07-04 at 17:03 +0800, Jisheng Zhang wrote:  
> 
> My comments below.
> 
> > > > For Synopsys DesignWare 8250 uart which version >= 4.00a, there's
> > > > a
> > > > valid divisor latch fraction register. The fractional divisor
> > > > width is
> > > > 4bits ~ 6bits.    
> > > 
> > > I have read 4.00a spec a bit and didn't find this limitation.
> > > The fractional divider can fit up to 32 bits.  
> > 
> > the limitation isn't put in the register description, but in the
> > description
> > about fractional divisor width configure parameters. Searching
> > DLF_SIZE will
> > find it.  
> 
> Found, thanks.
> 
> > From another side, 32bits fractional div is a waste, for example,
> > let's
> > assume the fract divisor = 0.9, 
> > if fractional width is 4 bits, DLF reg will be set to UP(0.9*2^4) =
> > 15, the
> > real frac divisor =  15/2^4 = 0.93;
> > if fractional width is 32 bits, DLF reg will be set to UP(0.9*2^32) =
> > 3865470567
> > the real frac divisor = 3865470567/2^32 = 0.90;  
> 
> So, your example shows that 32-bit gives better value. Where is
> contradiction?

The gain -- 0.03 is small, the pay is expensive ;)

> 
> > > I would test this a bit later.  
> 
> It reads back 0 on our hardware with 3.xx version of IP.

Thanks. So the ver check could be removed.

>   
> > > > +	unsigned int		dlf_size:3;    
> > > 
> > > These bit fields (besides the fact you need 5) more or less for
> > > software
> > > quirk flags. In your case I would rather keep a u32 value of DFL
> > > mask as
> > > done for msr slightly above in this structure.  
> > 
> > OK. When setting the frac div, we use DLF size rather than mask, so
> > could
> > I only calculate the DLF size once then use an u8 value to hold the
> > calculated DLF size rather than calculating every time?  
> 
> Let's see below.
> 
> > > > +	quot = DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4),
> > > > baud);    
> > > 
> > > If we have clock rate like 100MHz and 10 bits of fractional divider
> > > it
> > > would give an integer overflow.  
> > 
> > This depends on the fraction divider width. If it's only 4~6 bits,
> > then we are fine.  
> 
> True.
> 
> > > 
> > > 4 here is a magic. Needs to be commented / described better.  
> > 
> > Yes. divisor = clk/(16*baud) = BRD(I) + BRD(F)
> > "I" means integer, "F" means fractional
> > 
> > 2^dlf_size*clk/(16*baud) = 2^dlf_size*(BRD(I) + BRD(F))
> > 
> > clk*2^(dlf_size - 4)/baud = 2^dlf_size*((BRD(I) + BRD(F))
> > 
> > the left part is "DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4),
> > baud)",
> > let's assume it equals quot.
> > 
> > then, BRD(I) = quot / 2^dlf_size = quot >> dlf_size, this is where
> > "quot >> d->dlf_size" below from.
> > 
> > BRD(F) = quot & dlf_mask = quot & (~0U >> (32 - dlf_size))  
> 
> Yes, I understand from where it comes. It's a hard coded value of PS all
> over the serial code.
> 
> And better use the same idiom as in other code, i.e. * 16 or / 16
> depends on context.
> 
> > > > +	*frac = quot & (~0U >> (32 - d->dlf_size));
> > > > +    
> > > 
> > > Operating on dfl_mask is simple as
> > > 
> > > u64 quot = p->uartclk * (p->dfl_mask + 1);  
> > 
> > Since the dlf_mask is always 2^n - 1, 
> > clk * (p->dlf_mask + 1) = clk << p->dlf_size, but the later
> > is simpler
> >   
> > > 
> > > *frac = div64_u64(quot, baud * 16 * (p->dfl_mask + 1);
> > > return quot;  
> > 
> > quot = DIV_ROUND_CLOSEST(p->uartclk << (d->dlf_size - 4), baud)
> > *frac = quot & (~0U >> (32 - d->dlf_size))
> > return quot >> d->dlf_size;
> > 
> > vs.
> > 
> > quot = p->uartclk * (p->dfl_mask + 1);
> > *frac = div64_u64(quot, baud * 16 * (p->dfl_mask + 1);
> > return quot;
> > 
> > shift vs mul.
> > 
> > If the dlf width is only 4~6 bits, the first calculation can avoid
> > 64bit div. I prefer the first calculation.  
> 
> OK, taking that into consideration and looking at the code snippets
> again I would to
>  - keep two values
> 
> // mask we get for free because it's needed in intermediate calculus
> //
> and it doesn't overflow u8 (4-6 bits by spec)
> u8 dlf_mask;
> u8 dlf_size;
> 
>  - implement function like below
> 
> unsigned int quot;
> 
> /* Consider maximum possible DLF_SIZE, i.e. 6 bits */
> quot = DIV_ROUND_CLOSEST(p->uartclk * 4, baud);
> 
> *frac = (quot >> (6 - dlf_size)) & dlf_mask;
> return quot >> dlf_size;
> 
> Would you agree it looks slightly less complicated?

Nice. I will follow this solution.

> 
> > > > +	serial_port_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> > > > +	serial_dl_write(up, quot);    
> 
> (1)
> 
> > > 
> > > At some point it would be a helper, I think. We can call
> > > serial8250_do_set_divisor() here. So, perhaps we might export it.  
> > 
> > serial8250_do_set_divisor will drop the frac, that's not we want ;)  
> 
> Can you check again? What I see is that for DW 8250 the
> _do_set_divisor() would be an equivalent to the two lines, i.e. (1).

My bad, I mixed it with get_divisor.

> 
> And basically at the end it should become just those two lines when the
> rest will implement their custom set_divisor().

yes, makes sense. Will send a new version soon.

Re: [PATCH v2 3/3] serial: 8250_dw: add fractional divisor support

[Andy Shevchenko]

On Mon, Jul 9, 2018 at 9:04 AM, Jisheng Zhang
<Jisheng.Zhang@synaptics.com> wrote:
> On Fri, 6 Jul 2018 20:37:09 +0300 Andy Shevchenko wrote:

> yes, makes sense. Will send a new version soon.
Please, be sure you base it on top of tty-next and there is no
warnings added (makes sense to run make W=1 as well),

-- 
With Best Regards,
Andy Shevchenko