On 28/04/2020 18:57, Simon Glass wrote: Hi,
sorry for the delay, found this, slightly mouldy already, in my draft folder. First, thanks for the review! I saw the Tom merged this already, but wanted to come back to the DT hacks: > Hi Andre, > > On Mon, 27 Apr 2020 at 12:18, 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. > > One problem is that we want a 'debug UART' to work before the clock > driver is running, so we want to do the *minimum possible* amount of > init to get the UART running. So we don't want to start up driver > model, clock drivers, etc. I understand this very well - having an UART up and running as early as possible is crucial for debugging. > I think we should have useful helpers like the 'clock' property to > avoid lots of parsing very early in U-Boot. Of course such things are > hard for kernel people to understand / agree to but that doesn't make > them wrong. I agree, but I don't think we should mess around with the DT for this purpose. This is basically a U-Boot requirement or debug feature, not a machine property. And deviating from the official DT binding is not a good idea. I think for those U-Boot specific debug features we can just have CONFIG options - for instance we already have CONFIG_PL011_CLOCK. Also I strongly believe that skip-init does not belong into the DT. It's a *U-Boot* decision to not *re*-init the UART, not a machine property. There are PL011 compatible UARTs which should *not* be initialised (SBSA-UART), but both TX1 and RPi don't have those, but instead real PL011s. So if we desperately wanted this in the DT, we could actually use compatible = "arm,sbsa-uart", then we don't need any clock at all. But I was more thinking about turning skip-init into a config symbol and defining this for TX1 and RPi. We do already something similar for the RPi4 in Trusted Firmware [1]. This would allow us to remove the skip-init property from the u-boot.dtsi, and would help with booting with the DT from the SD card instead (for which the GPU firmware puts the pointer to into the beginning of memory [2]). I have a patch for that already, will send it soonish. Cheers, Andre [1] https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/?id=0eda713b9bd65222155900aacf3a67805351f88f [2] https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/?id=c4597e13a2925cc6bf802d9376238f5de18b292a >> >> 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 | 47 +++++++++++++++++++++++------------ >> 1 file changed, 31 insertions(+), 16 deletions(-) > > Reviewed-by: Simon Glass <s...@chromium.org> >> >> diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c >> index 2a5f256184..14040f32ef 100644 >> --- a/drivers/serial/serial_pl01x.c >> +++ b/drivers/serial/serial_pl01x.c >> @@ -12,6 +12,7 @@ > > Regards, > Simon >