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 -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
