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

[Bin Meng]

On Sat, Aug 15, 2015 at 10:56 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 08/14/2015 05:57 PM, Bin Meng wrote:
>> Hi,
>>
>> On Sat, Aug 15, 2015 at 7:18 AM, Simon Glass <sjg at chromium.org> 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?
>>>
>>
>> I am not sure which bindings you are looking at,
>
> The binding for nvidia,tegra20-uart. In the kernel tree, that's at
> Documentation/devicetree/bindings/serial/nvidia,tegra20-hsuart.txt.
>
>> but I checked the
>> Power.org ePAPR spec v1.1, the clock-frequency is there specially
>> listed for NS16550 compatible nodes.
>>
>> clock-frequency R <u32> Specifies the frequency (in Hz) of the baud
>> rate generator’s input clock
>>
>> If you don't have such clock-frequency in your device tree, I would
>> say you don't follow the spec.
>
> The binding for Tegra UARTs doesn't inherit from the generic NS16550
> binding, so there's that binding isn't relevant.

The kernel bindings has more like dma channels for Tegra hsuart.
U-Boot does not need this as we just need it as a serial console. We
are not going to transfer some big chunk of data via serial line in
U-Boot. In essence if it is NS16550 compatible device, it should just
use that device binding. But I agree, clock-frequency can be optional
for platforms where dynamic clock is used. In such case we cannot
hardcode one frequency in the device tree.

Regards,
Bin