Re: [PATCH v2] earlycon: Allow specifying a uartclk in options

[Aaron Durbin]

On Sat, Mar 3, 2018 at 8:38 AM, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
> On Thu, Mar 1, 2018 at 11:24 PM, Aaron Durbin <adurbin@xxxxxxxxxxxx> wrote:
>> On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
>> <andy.shevchenko@xxxxxxxxx> wrote:
>>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@xxxxxxxxxxxx> wrote:
>>>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>>>> wrote:
>>>
>>>> "earlycon simply does not utilize the information".
>>>>
>>>> earlycon parses iotype, mapbase and baud (from options).  However, it is
>>>> hard-coded to assume that the clock used to generate the UART bitclock is
>>>> always "BASE_BAUD * 16" (1843200).  While this may be true for many UARTs,
>>>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>>>> clock.  The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>>>> uses its rate to initialize uartclk.  For AMD CZ/ST, this "baudclk" is
>>>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>>>> with a rate read from the .fixed_clk_rate param of the corresponding
>>>> apd_device_desc.
>>>>
>>>> This patch attempts to add a way to inform earlycon about this clock.  As
>>>> noted above, the information is actually already in the kernel and used by
>>>> 8250_dw - I would happy be to hear recommendations for wiring this data
>>>> into earlycon that doesn't require adding another command line arg.
>>>
>>> And it should not require that for sure!
>>
>> But it does require that. There's an input clock to the uart ip block.
>> That is a design constraint by the hardware and is required to make
>> baud calculation work.
>
> I mean it should not be user's headache to provide this information to
> the system.
>
>> It's not a firmware problem.
>
> If it's ACPI, then it's definitely firmware issue, since SPCR provides
> a baudrate.
>

earlycon is another implemetnation of driver binding/settings that is
done before the rest of the kernel driver stack. SPCR provides
baudrate, but that's only one piece to the puzzle. You need to know
the UART input clock which you admit is hard coded in the current
driver. It's not possible to configure the divisor in the UART without
knowing the external clock. That's a real dependency that can't be
worked around. The moment the code attempts to configure the baud it
has to know the input clock -- otherwise the calculation is inherently
wrong. Or are you suggesting to lie about the baud such that the the
math works out even though the values are not real?

>> Its the driver's problem in that it
>> assumes an input clock to the uart block that does not reflect
>> reality.
>
> So, driver can't get this info from device tree or what?

There is no device tree on x86. And ACPI driver binding is not fully
initialized when earlycon is being processed. Yes, this can be solved
by getting up the entire ACPI stack, but then that's not really
'early' in the kernel's initialization sequence.

>
>>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>>
>> That's only possible if there is a clock divider on the front end of
>> the uart block. For this hardware that's not the case. I actually did
>> this very thing on intel chromebook devices, but it was only possible
>> because there was a hardware divider that could be tuned to reach the
>> assumed clock that the code currently assumes.
>
> OK.
>
> --
> With Best Regards,
> Andy Shevchenko