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

[Jisheng Zhang <Jisheng.Zhang@synaptics.com>]

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.