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

[André Przywara]

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.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.
> 
> 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.przywara at arm.com>
>> ---
>>  drivers/serial/serial_pl01x.c | 47 +++++++++++++++++++++++------------
>>  1 file changed, 31 insertions(+), 16 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg at 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
>