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

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

[Bin Meng]

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.

Regards,
Bin

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

[Simon Glass]

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.

Regards,
Simon

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

[Stephen Warren]

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>;

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

[Simon Glass]

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.

Regards,
Simon

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

[Bin Meng]

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?

Regards,
Bin

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

[Stephen Warren]

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.

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

[Simon Glass]

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?

Regards,
Simon

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

[Bin Meng]

Hi,

On Sat, Aug 15, 2015 at 7:18 AM, Simon Glass <sjg@chromium.org> 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?
>

I am not sure which bindings you are looking at, 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.

Regards,
Bin

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

[Stephen Warren]

On 08/14/2015 05:57 PM, Bin Meng wrote:
> Hi,
> 
> On Sat, Aug 15, 2015 at 7:18 AM, Simon Glass <sjg@chromium.org> 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?
>>
> 
> 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.

[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@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@chromium.org> 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?
>>>
>>
>> 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

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

[Stephen Warren]

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.

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

[Bin Meng]

On Sat, Aug 15, 2015 at 10:57 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 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.
>

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.

Regards,
Bin

[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@wwwdotorg.org> wrote:
>> 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.
>>
> 
> 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).

[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@wwwdotorg.org> wrote:
> 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.

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