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

[Stephen Warren]

On 08/14/2015 09:12 PM, Bin Meng wrote:
> On Sat, Aug 15, 2015 at 10:57 AM, 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.
>>
> 
> I agree. So what's our next step? So far I still think using _weak is
> the easiest approach. Adding clock uclass for fixed clock-frequency
> chipset like x86 super i/o and PCI UART does not make sense. The clock
> uclass is only helpful when dealing with dynamic clock frequency
> platforms on PPC and ARM SoC.

I think having the core UART driver call a function to get the clock
rate is perfectly fine. Just don't provide a _weak version of the
function. This will require all users of the generic UART driver to make
an explicit choice about how to implement this "callback" or "hook",
which makes it most likely that it'll be correctly implemented on all
platforms for all UARTs.

Platforms (or DT compatible value handlers, or ACPI whatever handlers,
or ...) could either pass this function into the generic UART driver
during instantiation/initialization, or put a function pointer into some
configuration structure that's passed to the generic UART driver during
instantiation/initialization, or something like that. The function
shouldn't be a global symbol. There can be only one implementation of a
global symbol. We need to support many implementations within the same
U-Boot binary. Consider a board containing both an ARM SoC with some
on-SoC UARTs where the clock rate is dynamically controlled by some
on-chip clock module, and also containing a PCI port into which a PCI
UART card is plugged, which requires a hard-coded clock rate (perhaps
globally fixed for all PCI UARTs, or based on a lookup table from PCI
vendor/device ID).