On Thu, 2018-07-05 at 14:39 +0800, Jisheng Zhang wrote:

quoted

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

My comments below.

quoted

quoted

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?

quoted

I would test this a bit later.

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

quoted

quoted

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

quoted

quoted

+	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

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.

quoted

quoted

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

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

quoted

quoted

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

(1)

quoted

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

quoted

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.

quoted

quoted

+		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 [off-list ref]
Intel Finland Oy