On 03/04/2015 03:54 AM, James Hogan wrote:
> On Tue, Mar 03, 2015 at 02:08:45PM -0500, Peter Hurley wrote:
>> On 03/03/2015 05:11 AM, James Hogan wrote:
>>> Hi Peter,
>>>
>>> On Mon, Mar 02, 2015 at 01:37:07PM -0500, Peter Hurley wrote:
>>>> [ +cc common clock maintainers ]
>>>>
>>>> On 02/26/2015 12:34 PM, James Hogan wrote:
>>>>> Hi Peter,
>>>>>
>>>>> On 26/02/15 16:54, Peter Hurley wrote:
>>>>>> On 02/26/2015 09:44 AM, James Hogan wrote:
>>>>>>> On 26/02/15 13:45, Peter Hurley wrote:
>>>>>>>> Hi James,
>>>>>>>>
>>>>>>>> On 02/26/2015 06:25 AM, James Hogan wrote:
>>>>>>>>> Hi Sebastian,
>>>>>>>>>
>>>>>>>>> On 26/02/15 08:51, Sebastian Andrzej Siewior wrote:
>>>>>>>>>> On 02/26/2015 01:21 AM, James Hogan wrote:
>>>>>>>>>>> When the UART clock is set slightly under 1.8432MHz, the 8250 driver
>>>>>>>>>>> core doesn't permit the 115200 baud rate since it calculates the maximum
>>>>>>>>>>> frequency to pass to uart_get_baud_rate by simply dividing the uart
>>>>>>>>>>> clock by 16 which yields a value slightly under 115200, even though the
>>>>>>>>>>> frequency is close enough for the UART to operate reliably.
>>>>>>>>>>>
>>>>>>>>>>> Therefore add some tolerance in the calculation of the maximum baud
>>>>>>>>>>> rate, specifically +1/128th (~0.8%), to allow the 115200 baud rate to
>>>>>>>>>>> be closen for a UART clock as low as 1.8149MHz. For an external divider
>>>>>>>>>>> set as close as possible to 1.8432MHz, this should cover every possible
>>>>>>>>>>> input frequency above 118.9MHz.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
>>>>>>>>>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>>>>>>>>>> Cc: Jiri Slaby <jslaby@xxxxxxx>
>>>>>>>>>>> Cc: Anton Vorontsov <avorontsov@xxxxxxxxxxxxx>
>>>>>>>>>>> Cc: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
>>>>>>>>>>> Cc: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
>>>>>>>>>>> Cc: linux-serial@xxxxxxxxxxxxxxx
>>>>>>>>>>> ---
>>>>>>>>>>> As far as I can tell from reading the link below, this tolerance should
>>>>>>>>>>> be okay, and it definitely covers the range of input frequencies I
>>>>>>>>>>> expect for my particular hardware. I'm open to better ways of handling
>>>>>>>>>>> it though.
>>>>>>>>>>
>>>>>>>>>> What is your hardware?
>>>>>>>>>
>>>>>>>>> SoC is TZ1090 (metag based). UART clock is best derived from system
>>>>>>>>> clock which would normally be somewhere between 180MHz and 240MHz, using
>>>>>>>>> an integer divider.
>>>>>>>>>
>>>>>>>>>> (1814900 + 1814900 / 128) / 16
>>>>>>>>>> 114317
>>>>>>>>>> It is not 115200 but close enough to get you the 115200 baud rate. It
>>>>>>>>>> might make sense to increase the tolerance rather than increasing the
>>>>>>>>>> the clock but it is up to Peter / Alan to decide.
>>>>>>>>>
>>>>>>>>> One random example configuration I have here is:
>>>>>>>>>
>>>>>>>>> # cat /sys/kernel/debug/clk/clk_summary
>>>>>>>>> clock enable_cnt prepare_cnt rate
>>>>>>>>> --------------------------------------------------------------------
>>>>>>>>> xtal1 2 2 24576000
>>>>>>>>> sys_sw 1 1 24576000
>>>>>>>>> sys_pll 1 1 719965090
>>>>>>>>> sys_div 1 1 719965090
>>>>>>>>> sys_x2_undeleted 1 1 719965090
>>>>>>>>> sys_undeleted 2 2 179991273
>>>>>>>>> uart_sw 1 1 179991273
>>>>>>>>> uart_en 1 1 179991273
>>>>>>>>> uart 1 1 1836646
>>>>>>>>>
>>>>>>>>> sys_undeleted divide uart baud error
>>>>>>>>> 179991273 97 1855580 115973 774 = 0.67%
>>>>>>>>> 179991273 98 1836646 114790 -410 = -0.36% (chosen)
>>>>>>>>>
>>>>>>>>> At this sys_undeleted frequency, the difference between two consecutive
>>>>>>>>> divider values is about 1183 Hz.
>>>>>>>>> For round to closest, the max error is about 591.5 Hz = 0.513%.
>>>>>>>>> For round up (i.e. increasing the clock), the max error is about 1183 Hz
>>>>>>>>> = 1.03%.
>>>>>>>>>
>>>>>>>>> So my reasoning is that rounding to closest and fixing 8250 driver to
>>>>>>>>> accept it and getting as accurate a baud rate as possible is better than
>>>>>>>>> having a less accurate baud rate and having to add a new rounding mode
>>>>>>>>> to common clock muxes and dividers.
>>>>>>>>>
>>>>>>>>> Note that AFAICT for faster input clocks the 8250 driver would accept a
>>>>>>>>> 115200 baud rate regardless of how inaccurate the resulting baud rate
>>>>>>>>> would be after division, so arguably the tolerance could be much higher.
>>>>>>>>>
>>>>>>>>> The serial core also calculates the divider with DIV_ROUND_CLOSEST, so
>>>>>>>>> there is precedent for using that rounding mode right up through the
>>>>>>>>> clock tree.
>>>>>>>>
>>>>>>>> Why clock the uart so low?
>>>>>>>
>>>>>>> I tried forcing uart = sys_undeleted / 1 a couple of years ago when
>>>>>>> switching to common clock & DT, relying instead on the divider internal
>>>>>>> to the UART, but it didn't work for everybody. It was presumed at the
>>>>>>> time that the UART input was effectively being overclocked between the
>>>>>>> divider and the uart so wasn't reliable at higher frequencies.
>>>>>>
>>>>>> 180 Mhz uart clock :)
>>>>>>
>>>>>>> The hardware manual lists Fmax/ideal as 25MHz, but I didn't want to set
>>>>>>> it to a specific value such as 25MHz as the combination of the 2
>>>>>>> dividers (the one in the uart block and the external one in the SoC)
>>>>>>> will reduce the precision that can be attained.
>>>>>>>
>>>>>>> So that really only leaves the option of aiming for the right uart
>>>>>>> frequency with the external divider and using an internal divide by 1.
>>>>>>
>>>>>> Dividing down the sys clock by 14 and the uart clock by 7 yields an
>>>>>> identical baud rate of 114790 (as does dividing the sys clock by 7
>>>>>> but that sets the uart clock to 25.713 Mhz which may be too high for
>>>>>> the IP block).
>>>>>>
>>>>>> Also, dividing sys clock by 49 and uart clock by 2 = 114790
>>>>>
>>>>> Sure, but it doesn't generalise very easily. The system/core/DDR clocks
>>>>> are statically configured by the bootloader depending on a whole bunch
>>>>> of board specifics (particularly for the DDR). I.e. it can be virtually
>>>>> anything (including prime number multiples of 115200*16). Linux then
>>>>> configures the rest of the clocks as it needs, using the common clock
>>>>> framework. This would require adding knowledge of max intermediate clock
>>>>> rates to common clock,
>>>>
>>>> The common clock framework will need to support this anyway, because a
>>>> 2-level clock tree can't possibly work with UART, ADC, GPU, etc. off a
>>>> single common clock without independent intermediate stages (maybe for
>>>> your limited application it does, but it doesn't generalize to other
>>>> designs).
>>>>
>>>>> and adding the UART internal divider into the decision making,
>>>>
>>>> Constrained dividers are already supported by many clock drivers.
>>>>
>>>>> i.e. the UART driver trying to set a bunch of rates,
>>>>> multiples of 115200*16 for each internal divider value, and seeing which
>>>>> gives the best baud rate, which would be even more complicated, and its
>>>>> all to appease an arguably nonsensical constraint in the driver.
>>>>
>>>> The true constraint you're trying to workaround is that the common clock
>>>> framework doesn't adequately support your use-case yet. But that's where
>>>> your requirement needs to be solved.
>>>>
>>>> ISTM that the clock api should provide helpers so the intermediate parent
>>>> clock can compute the best-fit clock rate given the constraints of
>>>> the input sys clock rate, the {min,max} intermediate clock rate range,
>>>> and the desired sample rate multiple of the clock consumer.
>>>
>>> Even with those other constraints fully implemented, is it really a
>>> correct constraint that the uart input clock must be strictly >=
>>> 115200*16? A value under this may give the closest baud rate even taking
>>> all other constraints into account.
>>
>> I spent some time last night experimenting with a simple brute-force
>> solver.
>>
>> And while there are input clock rates for which the best-fit output clock is
>> < 115200 * 16 (for that target rate or its multiples), for the same input
>> clock rate there always seems to be faster multiples within some degree
>> of error, although not as low as you initially proposed.
>>
>> For example, an input clock = 152785600 has a best-fit output clock =
>> 1840790 (div=83, baud=115049, error=0.13%). That same input clock rate
>> has fastest multiples with div=82 and div=41 with a error=1.3%
>>
>> However, the error limit you initially proposed (as you noted in the
>> commit log) is only suitable for input clock rates above 118Mhz; lower
>> clock rates would require larger error limits.
>>
>> I plan to experiment further on the actual effects of a slow uart clock
>> and a suitable error limit, but any further information you can provide
>> wrt sample error might be helpful.
I finally had some time to further consider and experiment with this
solution. And while I still believe that the root cause of this problem
needs to be solved within the common clock framework, I see no problem
with allowing a small undershoot at the limits. So,
Reviewed-by: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
PS - I'm certain this patch needs resent, as Greg will no longer have a copy.
> My only other comment is that the overall error tolerance is shared
> with the other end. The following article suggests overall 2% error
> would be pushing it for a "nasty" case of only sampling accurately 50%
> of the bit time (e.g. a long capacitive connection, though I don't know
> how realistic that case is). So if one end is 1.3% fast as in your
> example, the other end only needs to be 0.7% slow for it to be a
> problem.
>
> http://www.maximintegrated.com/en/app-notes/index.mvp/id/2141
>
> This was my rationale for wanting round to closest throughout the
> calculation.
>
> Thanks
> James
>
>>
>> Regards,
>> Peter Hurley
>>
>>
>>>>> At the moment I just need this patch and the following added in the uart
>>>>> DT node, and it should basically work for all system clock rates I care
>>>>> about:
>>>>>
>>>>> assigned-clocks = <&top_clks CLK_TOP_UART>;
>>>>> assigned-clock-rates = <1843200>;
>>>>>
>>>>> Cheers
>>>>> James
>>>>>
>>>>
>>
--
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
