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

[Simon Glass]

Hi André,

On Tue, 12 May 2020 at 08:27, André Przywara <andre.przywara at arm.com> wrote:
>
> 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.

Yes of course these are U-Boot decisions in some sense. But they are
also hardware-related. There is nothing wrong with having a fixed
clock as a default, for simple software to use.

We have a persistent problem here because of this 'linux' idea that we
cannot have config in the DT. It is generally the only thing available
to U-Boot. It is certainly the only thing available for runtime
config.

Why not put a 'u-boot' prefix on it and be done?

If we could just get over this hangup, it would be so great for U-Boot
:-) It doesn't have the ability to rely on user space for policy.

>
> 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]).

You mean we have to build U-Boot differently depending on what it is
booting from? I wonder if we could pass this information through to
U-Boot instead.

>
> I have a patch for that already, will send it soonish.

Regards,
Simon