On 26/03/2020 16:20, Simon Glass wrote:
Hi Simon,
> On Wed, 25 Mar 2020 at 08:47, Andre Przywara <andre.przyw...@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.przyw...@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
>
