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

[Simon Glass]

Hi Stephen,

On 14 August 2015 at 20:57, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 08/14/2015 05:18 PM, Simon Glass wrote:
>> Hi,
>>
>> On 14 August 2015 at 16:51, Stephen Warren <swarren at 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 at chromium.org> wrote:
>>>>>
>>>>> Hi Stephen,
>>>>>
>>>>> On 14 August 2015 at 10:58, Stephen Warren <swarren at 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 at 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.

There clearly is a need for it, since I wrote a driver which hard-codes it.

Even if/when someone puts generic clock support into Tegra it is
unlikely to be worth spinning up all that clock tree stuff just to
find out this value in SPL. It would be MUCH simpler if we just added
an optional clock-frequency property to the binding. As Bin points
out, it is commonly used in other bindings.

Regards,
Simon