From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Hurley Subject: Re: [PATCH v3 10/15] serial: stm32-usart: Add STM32 USART Driver Date: Tue, 24 Mar 2015 13:44:53 -0400 Message-ID: <5511A295.1060103@hurleysoftware.com> References: <1426197361-19290-1-git-send-email-maxime.coquelin@st.com> <1426197361-19290-11-git-send-email-maxime.coquelin@st.com> <550AE41C.8070803@hurleysoftware.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Maxime Coquelin Cc: Andy Shevchenko , =?UTF-8?B?VXdlIEtsZWluZS0=?= =?UTF-8?B?S8O2bmln?= , =?UTF-8?B?QW5kcmVh?= =?UTF-8?B?cyBGw6RyYmVy?= , Geert Uytterhoeven , Rob Herring , Philipp Zabel , Linus Walleij , Arnd Bergmann , Stefan Agner , Peter Meerwald , Paul Bolle , Jonathan Corbet , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Russell King , Daniel Lezcano , Thomas Gleixner , Greg Kroah-Hartman , Jiri Slaby , Andr List-Id: devicetree@vger.kernel.org Hi Maxime, On 03/24/2015 01:21 PM, Maxime Coquelin wrote: > Hi Peter, > > 2015-03-19 18:35 GMT+01:00 Maxime Coquelin : >> 2015-03-19 15:58 GMT+01:00 Peter Hurley : >>> On 03/19/2015 09:55 AM, Maxime Coquelin wrote: >>>>>>>> +static void stm32_set_termios(struct uart_port *port, struct ktermios *termios, >>>>>>>> + struct ktermios *old) >>> [...] >>>>>>>> + usardiv = (port->uartclk * 25) / (baud * 4); >>>>>>>> + mantissa = (usardiv / 100) << USART_BRR_DIV_M_SHIFT; >>>>>>>> + fraction = DIV_ROUND_CLOSEST((usardiv % 100) * 16, 100); >>>>>>>> + if (fraction & ~USART_BRR_DIV_F_MASK) { >>>>>>>> + fraction = 0; >>>>>>>> + mantissa += (1 << USART_BRR_DIV_M_SHIFT); >>>>>>>> + } >>> [...] >>>> Really, I would prefer keeping this fractional divider as it is >>>> implemented today. >>> >>> You have to admit that's basically an unintelligible mess; >>> how would anyone ever be able to refactor and replace that with a >>> common divider implementation? >>> >>> At the very least, please comment on the formula and format. >> >> Ok, I will refactor the implementation, and comment it. > > The implementation was indeed a mess. > I found some time to refactor the code, and also added support for 8 > times oversampling (16 by default). > It will allow to achieve higher speeds, with the side effect of being > less tolerant to clock deviations. > > What do you think about the code below? > > > usartdiv = DIV_ROUND_CLOSEST(port->uartclk, baud); > > /* > * The USART supports 16 or 8 times oversampling. > * By default we prefer 16 times oversampling, so that the receiver > * has a better tolerance to clock deviations. > * 8 times oversampling is only used to achieve higher speeds. > */ > if (usartdiv < 16) { > oversampling = 8; > stm32_set_bits(port, USART_CR1, USART_CR1_OVER8); > } else { > oversampling = 16; > stm32_clr_bits(port, USART_CR1, USART_CR1_OVER8); > } > > mantissa = (usartdiv / oversampling) << USART_BRR_DIV_M_SHIFT; > fraction = usartdiv % oversampling; > writel_relaxed(mantissa | fraction, port->membase + USART_BRR) Thanks! Way better :) Much more obvious this is a fixed-point divisor. Regards, Peter Hurley