Hi Andre,
On Thu, 26 Mar 2020 at 11:06, André Przywara <andre.przyw...@arm.com> wrote:
>
> 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?
Hmm it might make more sense to migrate the boards?
Reviewed-by: Simon Glass <s...@chromium.org>
Regards,
Simon
