serial: 8250_dw: add fractional divisor support

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 <[email protected]>

---
 drivers/tty/serial/8250/8250_port.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/tty/serial/8250/8250_port.c +2 -2

index cf541aab2bd0..709fe6b4265c 100644
@@ -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;
 

drivers/tty/serial/8250/8250_port.c +1 -1

 	/*

@@ -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);
 
 	/*

drivers/tty/serial/8250/8250_port.c +1 -1

 	 * 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);
 

drivers/tty/serial/8250/8250_port.c +0 -0

 	serial8250_set_divisor(port, baud, quot, frac);
 	serial_port_out(port, UART_LCR, up->lcr);

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

Signed-off-by Jisheng Zhang <[email protected]>

---
 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(-)

drivers/tty/serial/8250/8250_core.c +4 -0

index 9342fc2ee7df..a0bb77290747 100644
@@ -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;

drivers/tty/serial/8250/8250_core.c +0 -0

 		if (up->port.startup)
 			uart->port.startup = up->port.startup;
 		if (up->port.shutdown)

drivers/tty/serial/8250/8250_port.c +3 -3

index 709fe6b4265c..ce0dc17f18ee 100644
@@ -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);

drivers/tty/serial/8250/8250_port.c +10 -0

 	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,

drivers/tty/serial/8250/8250_port.c +1 -1

 					    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)
{

drivers/tty/serial/8250/8250_port.c +7 -0

 	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);

drivers/tty/serial/8250/8250_port.c +2 -0

+}
+
 static unsigned int serial8250_get_baud_rate(struct uart_port *port,
 					     struct ktermios

*termios,

drivers/tty/serial/8250/8250_port.c +0 -0

 					     struct ktermios *old)

include/linux/serial_core.h +7 -0

index 06ea4eeb09ab..406edae44ca3 100644
@@ -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);

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.

+#define DW_FRAC_MIN_VERS 0x3430302A

};

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

drivers/tty/serial/8250/8250_dw.c +8 -0

+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);

drivers/tty/serial/8250/8250_dw.c +2 -0

+}
+
 static void dw8250_quirks(struct uart_port *p, struct dw8250_data

*data)
{

drivers/tty/serial/8250/8250_dw.c +2 -0

 	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

drivers/tty/serial/8250/8250_dw.c +3 -0

+	 * fraction register. Calculate the fractional divisor width.
+	 */
+	if (reg >= DW_FRAC_MIN_VERS) {

drivers/tty/serial/8250/8250_dw.c +7 -0

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

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

Thanks for an update, my comments below.

Quoted 3 lines

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.

From another side, 32bits fractional div is a waste, for example, let's

Quoted 5 lines

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.

Quoted 1 line

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

drivers/tty/serial/8250/8250_dw.c struct dw8250_data { · line 71

Quoted 3 lines

drivers/tty/serial/8250/8250_dw.c +1 -0

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

Quoted 9 lines

};

+/*
+ * 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.

Quoted 8 lines

drivers/tty/serial/8250/8250_dw.c +7 -0

+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;
+  

Quoted 2 lines

(quoted diff) +1 -0

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

Review

Quoted 3 lines

(quoted diff) +2 -0

+	*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)

drivers/tty/serial/8250/8250_dw.c static bool dw8250_idma_filter(struct dma_chan *chan, void *param) · line 380

Quoted 11 lines

drivers/tty/serial/8250/8250_dw.c +4 -0

+	return quot >> d->dlf_size;
+}
+
+static void dw8250_set_divisor(struct uart_port *p, unsigned int

baud,

(quoted diff) +1 -0

+			       unsigned int quot, unsigned int

quot_frac)

drivers/tty/serial/8250/8250_dw.c +4 -0

+{
+	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().

drivers/tty/serial/8250/8250_dw.c static bool dw8250_idma_filter(struct dma_chan *chan, void *param) · line 382

Quoted 3 lines

drivers/tty/serial/8250/8250_dw.c +2 -0

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

Quoted 21 lines

drivers/tty/serial/8250/8250_dw.c +2 -0

+}
+
 static void dw8250_quirks(struct uart_port *p, struct dw8250_data

*data)
{

drivers/tty/serial/8250/8250_dw.c +2 -0

 	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

drivers/tty/serial/8250/8250_dw.c +5 -0

+	 * fraction register. Calculate the fractional divisor width.
+	 */
+	if (reg >= DW_FRAC_MIN_VERS) {
+		struct dw8250_data *d = p->private_data;
+  

drivers/tty/serial/8250/8250_dw.c static void dw8250_setup_port(struct uart_port *p) · line 461

Quoted 7 lines

drivers/tty/serial/8250/8250_dw.c +7 -0

+		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

drivers/tty/serial/8250/8250_dw.c static void dw8250_setup_port(struct uart_port *p) · line 462

Quoted 2 lines

drivers/tty/serial/8250/8250_dw.c +1 -0

+		d->dlf_size = fls(reg);  

Just save value itself as dfl_mask.

Quoted 6 lines

drivers/tty/serial/8250/8250_dw.c static bool dw8250_idma_filter(struct dma_chan *chan, void *param) · line 382

Quoted 3 lines

drivers/tty/serial/8250/8250_dw.c +2 -0

+	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 ;)

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

<snip>

Quoted 10 lines

Quoted 7 lines

drivers/tty/serial/8250/8250_dw.c static bool dw8250_idma_filter(struct dma_chan *chan, void *param) · line 382

Quoted 3 lines

drivers/tty/serial/8250/8250_dw.c +2 -0

+	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

Quoted 1 line · Jisheng Zhang

Quoted 6 lines

Quoted 5 lines

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

Quoted 2 lines

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;

Quoted 1 line

I would test this a bit later.

Quoted 7 lines

drivers/tty/serial/8250/8250_dw.c struct dw8250_data { · line 71

Quoted 1 line

drivers/tty/serial/8250/8250_dw.c +1 -0

+	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?

Quoted 6 lines

Quoted 2 lines

(quoted diff) +1 -0

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

Quoted 2 lines

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

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	*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

Quoted 3 lines

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

Quoted 2 lines

drivers/tty/serial/8250/8250_dw.c static bool dw8250_idma_filter(struct dma_chan *chan, void *param) · line 382

Quoted 2 lines

drivers/tty/serial/8250/8250_dw.c +2 -0

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

Quoted 3 lines

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 ;)

Quoted 2 lines

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

Nice. Thanks.

Quoted 3 lines

drivers/tty/serial/8250/8250_dw.c static void dw8250_setup_port(struct uart_port *p) · line 462

Quoted 1 line

drivers/tty/serial/8250/8250_dw.c +1 -0

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

Quoted 1 line · Jisheng Zhang

Quoted 1 line · Jisheng Zhang

My comments below.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 9 lines

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?

Quoted 2 lines

Quoted 2 lines

I would test this a bit later.

It reads back 0 on our hardware with 3.xx version of IP.

Quoted 9 lines

Quoted 8 lines

drivers/tty/serial/8250/8250_dw.c struct dw8250_data { · line 71

Quoted 2 lines

drivers/tty/serial/8250/8250_dw.c +1 -0

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

Quoted 4 lines

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.

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

(quoted diff) +1 -0

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

Quoted 10 lines

Quoted 2 lines

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

Quoted 8 lines

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.

Quoted 21 lines

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	*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

Quoted 3 lines

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

Quoted 8 lines

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?

Quoted 3 lines

Quoted 3 lines

drivers/tty/serial/8250/8250_dw.c static bool dw8250_idma_filter(struct dma_chan *chan, void *param) · line 382

Quoted 3 lines

drivers/tty/serial/8250/8250_dw.c +2 -0

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

(1)

Quoted 1 line

Quoted 1 line

Quoted 4 lines

Quoted 2 lines

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

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

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