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@xxxxxxxxxxxxxxx>
Intel Finland Oy