[PATCH 2/7] uart: pl011: Add proper DM clock support

[André Przywara]

On 26/03/2020 16:20, Simon Glass wrote:

Hi Simon,

> On Wed, 25 Mar 2020 at 08:47, Andre Przywara <andre.przywara at arm.com> wrote:
>>
>> Even though the PL011 UART driver claims to be DM compliant, it does not
>> really a good job with parsing DT nodes. U-Boot seems to adhere to a
>> non-standard binding, either requiring to have a "skip-init" property in
>> the node, or to have an extra "clock" property holding the base
>> *frequency* value for the baud rate generator.
>> DTs in the U-Boot tree seem to have been hacked to match this
>> requirement.
>>
>> The official binding does not mention any of these properties, instead
>> recommends a standard "clocks" property to point to the baud base clock.
>>
>> Some boards use simple "fixed-clock" providers, which U-Boot readily
>> supports, so let's add some simple DM clock code to the PL011 driver to
>> learn the rate of the first clock, as described by the official binding.
>>
>> These clock nodes seem to be not ready very early in the boot process,
>> so provide a fallback value, by re-using the already existing
>> CONFIG_PL011_CLOCK variable.
>>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> ---
>>  drivers/serial/serial_pl01x.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c
>> index 2a5f256184..1ab0ccadb2 100644
>> --- a/drivers/serial/serial_pl01x.c
>> +++ b/drivers/serial/serial_pl01x.c
>> @@ -12,6 +12,7 @@
>>
>>  #include <common.h>
>>  #include <dm.h>
>> +#include <clk.h>
>>  #include <errno.h>
>>  #include <watchdog.h>
>>  #include <asm/io.h>
>> @@ -340,14 +341,21 @@ static const struct udevice_id pl01x_serial_id[] ={
>>  int pl01x_serial_ofdata_to_platdata(struct udevice *dev)
>>  {
>>         struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
>> +       struct clk clk;
>>         fdt_addr_t addr;
>> +       int ret;
>>
>>         addr = devfdt_get_addr(dev);
>>         if (addr == FDT_ADDR_T_NONE)
>>                 return -EINVAL;
>>
>>         plat->base = addr;
>> -       plat->clock = dev_read_u32_default(dev, "clock", 1);
>> +       plat->clock = dev_read_u32_default(dev, "clock", CONFIG_PL011_CLOCK);
> 
> is this needed?

This is to provide the existing behaviour as a fallback. Some SoCs have
a complex clock providing the baud rate clock (HiKey 960, FSL LS2080a),
which U-Boot doesn't suport. I'd rather not break them, but also don't
really want to provide a clock driver ;-)

Also this mimics the !DM_SERIAL behaviour, which sets this clock rate
based on Kconfig, again as a fallback. I needed that because I think the
clock driver wasn't ready that early. It's a bit hard to confirm without
serial output ;-)

So the order should be:
- If there is a clocks property and we support that clock provider
(fixed-clock), then use that value.
- If not, check for a "clock" property in the DT node and use that value.
- If there is no "clock property", use the Kconfig variable.

Just written the other way around in the code.

Does this make sense?

Cheers,
Andre.

>> +       ret = clk_get_by_index(dev, 0, &clk);
>> +       if (!ret) {
>> +               clk_enable(&clk);
>> +               plat->clock = clk_get_rate(&clk);
>> +       }
>>         plat->type = dev_get_driver_data(dev);
>>         plat->skip_init = dev_read_bool(dev, "skip-init");
>>
>> --
>> 2.14.5
>>
> 
> Regards,
> Simon
>