On 19/01/18 17:02, Andy Shevchenko wrote:
> On Fri, 2018-01-19 at 16:40 +0000, Ed Blake wrote:
>> On 19/01/18 16:02, Andy Shevchenko wrote:
>>> The commit
>>>
>>> de9e33bdfa22 ("serial: 8250_dw: Improve clock rate setting")
>>>
>>> obviously tries to cure symptoms, and not a root cause.
>>>
>>> The root cause is the non-flexible rate calculation inside the
>>> corresponding clock driver.
>> The issue is the clock generating hardware in this particular platform
>> is only capable of generating finite clock rates from a fixed
>> divider.
> Which platform? Is it in upstream? What the driver handles clock for it?
>
> (If it's not yet upstream, it's one more point to revert, btw.)
It's not upstream.
>> So when dw8250_set_termios() requests baud * 16 it may get something
>> quite different - e.g. it requests 1.8432MHz for a baud rate of
>> 115,200
>> and gets back 3MHz from clk_round_rate() as this is the nearest
>> achievable. Before de9e33bdfa22, dw8250_set_termios() then sets the
>> clock to this rate which obviously doesn't work.
> I understand that and agree this is an issue, but I object the approach
> how you "fixed" it.
That's fine, I'm open to other solutions.
>>> What we need is to provide maximum UART
>>> divisor value to the clock driver to allow it do the job
>>> transparently
>>> to the caller.
>> So the clock driver should know that this clock is attached to a UART
>> with a fixed divider, and when 1.8432MHz is requested in the above
>> example it returns say 24MHz from clk_round_rate() because it knows
>> that
>> the UART can divide this down to something within an acceptable range
>> of
>> the requested rate for correct operation?
> Roughly speaking, AFAIU situation, you need your clock driver be based
> on clk-divider with appropriate input / platform data.
>
> If clk-divider is not good enough, then, derive something based on it,
> or expand existing one.
>
But fundamentally you are saying that the clock driver should have
knowledge that this particular clock is connected to a UART which
requires a clock rate that is within a defined range (say +/- 2%) of an
integer multiple of the requested rate.
I don't really like this approach as in my view the clock driver should
only have knowledge of the clock generating hardware and not the
internals of the devices it's connected to.
--
Ed
--
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
