[U-Boot] [RFC] Merge all ns16550 dm serial drivers into one

[Stephen Warren <swarren@wwwdotorg.org>]

On 08/14/2015 05:18 PM, Simon Glass wrote:
> Hi,
> 
> On 14 August 2015 at 16:51, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/14/2015 04:40 PM, Bin Meng wrote:
>>>
>>> On Sat, Aug 15, 2015 at 12:59 AM, Simon Glass <sjg@chromium.org> wrote:
>>>>
>>>> Hi Stephen,
>>>>
>>>> On 14 August 2015 at 10:58, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>>
>>>>> On 08/14/2015 10:50 AM, Simon Glass wrote:
>>>>>>
>>>>>>
>>>>>> Hi Bin,
>>>>>>
>>>>>> On 14 August 2015 at 03:18, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Currently there are 5 dm serial drivers, all of which are ns16550
>>>>>>> compatible drivers. They are:
>>>>>>>
>>>>>>>     serial_omap.c
>>>>>>>     serial_dw.c
>>>>>>>     serial_tegra.c
>>>>>>>     serial_x86.c
>>>>>>>     serial_ppc.c
>>>>>>>
>>>>>>> All these drivers are pretty much similar. I think we can justmerge
>>>>>>> these into one ns16550 driver.
>>>>>>>
>>>>>>> If you think this is necessary, I will send a patch series to do this.
>>>>>>
>>>>>>
>>>>>>
>>>>>> The tegra one is there because it needs an input clock and Stephen
>>>>>> didn't want to add this to the device tree binding (the kernel has a
>>>>>> clock framework which gets around this problem).
>>>>>>
>>>>>> After that I followed the same pattern. I would support updating the
>>>>>> binding to support an input clock. Even with the new clock framework
>>>>>> in U-Boot it might be painful to fit it into SPL in some cases.
>>>>>
>>>>>
>>>>>
>>>>> The clock is already in the DT, in both Linux and U-Boot's copy, at
>>>>> least
>>>>> for Tegra DTs:
>>>>>
>>>>>          uarta: serial at 0,70006000 {
>>>>>                  compatible = "nvidia,tegra124-uart", ...
>>>>> ...
>>>>>                  clocks = <&tegra_car TEGRA124_CLK_UARTA>;
>>>>>
>>>>
>>>> I mean the clock-frequency property. However if there is a plan to
>>>> implement the clock framework in U-Boot that would be good too.
>>>>
>>>
>>> The clock-frequency is a fixed value on x86 super i/o chipset, and
>>> fixed on the PCI bus too. But for ARM and PPC, it might get
>>> dynamically calculated due to different PLL settings. We can implement
>>> a _weak function like the one in serial_ppc.c get_serial_clock() to
>>> initialize plat->clock with its return value. The _weak function gets
>>> clock-frequency from device tree. If there is not, platform codes
>>> which uses the ns16550 driver should provide the implementation of
>>> get_serial_clock(). Thoughts?
>>
>>
>> There is no clock-frequency property in DT, at least for the Tegra DT
>> binding. It looks like some other bindings have it. To obtain the clock
>> frequency from DT for Tegra, you'd need to parse the clocks property, find
>> the clock driver associated with the phandle in DT, and go and ask that
>> clock driver what the clock frequency is.
>>
>> I'd prefer not to have a weak function that parses clock-frequency, since
>> it's too easy to accidentally use it on systems where parsing that property
>> is incorrect.
>>
>> Certainly, a generic UART driver can call out to a platform-supplied
>> function to retrieve the clock, and we can provide driver-specific
>> implementations for x86 super IO and PCI, and generic implementations that
>> appropriate drivers can call to parse the clocks or clock-frequency property
>> from DT, and finally for Tegra if we can't parse the clocks property right
>> now, call the Tegra clock driver directly to look up the value.
> 
> I'm not a big fan of weak functions either. In fact I think with
> driver model we should avoid them. If we can't call a uclass to get
> the info then perhaps we should wait until we can.
> 
> Pragmatically I wonder if a UART clock frequency would not be a useful
> compromise? Some bindings have it, some do not. Maybe we should just
> add it?

There's no need for it; the binding already has a clocks property, from
which the data can be derived. Adding a clock-frequency property would
just result in two sources of the same data. In all likelihood, all
that'd happen is that the two would get out-of-sync, and code wouldn't
know which to trust.