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

Message ID 20200427181804.15787-3-andre.przywara@arm.com
State Accepted
Commit e3e2d662a273d5ac67998f390529966a8d8c8a3b
Headers show
Series
  • Arm Juno board OF_CONTROL upgrade
Related show

Commit Message

Andre Przywara April 27, 2020, 6:17 p.m.
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.przywara at arm.com>
---
 drivers/serial/serial_pl01x.c | 47 +++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 16 deletions(-)

Comments

Simon Glass April 28, 2020, 5:57 p.m. | #1
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 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.

>
> 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
Tom Rini May 7, 2020, 1:03 p.m. | #2
On Mon, Apr 27, 2020 at 07:17:59PM +0100, Andre Przywara 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.przywara at arm.com>
> Reviewed-by: Simon Glass <sjg at chromium.org>

Applied to u-boot/master, thanks!
Andre Przywara May 12, 2020, 2:26 p.m. | #3
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
>
Simon Glass May 20, 2020, 3:07 a.m. | #4
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

Patch

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 @@ 
 
 #include <common.h>
 #include <dm.h>
+#include <clk.h>
 #include <errno.h>
 #include <watchdog.h>
 #include <asm/io.h>
@@ -149,21 +150,24 @@  static int pl01x_generic_setbrg(struct pl01x_regs *regs, enum pl01x_type type,
 		unsigned int remainder;
 		unsigned int fraction;
 
-		/*
-		* Set baud rate
-		*
-		* IBRD = UART_CLK / (16 * BAUD_RATE)
-		* FBRD = RND((64 * MOD(UART_CLK,(16 * BAUD_RATE)))
-		*		/ (16 * BAUD_RATE))
-		*/
-		temp = 16 * baudrate;
-		divider = clock / temp;
-		remainder = clock % temp;
-		temp = (8 * remainder) / baudrate;
-		fraction = (temp >> 1) + (temp & 1);
-
-		writel(divider, &regs->pl011_ibrd);
-		writel(fraction, &regs->pl011_fbrd);
+		/* Without a valid clock rate we cannot set up the baudrate. */
+		if (clock) {
+			/*
+			 * Set baud rate
+			 *
+			 * IBRD = UART_CLK / (16 * BAUD_RATE)
+			 * FBRD = RND((64 * MOD(UART_CLK,(16 * BAUD_RATE)))
+			 *		/ (16 * BAUD_RATE))
+			 */
+			temp = 16 * baudrate;
+			divider = clock / temp;
+			remainder = clock % temp;
+			temp = (8 * remainder) / baudrate;
+			fraction = (temp >> 1) + (temp & 1);
+
+			writel(divider, &regs->pl011_ibrd);
+			writel(fraction, &regs->pl011_fbrd);
+		}
 
 		pl011_set_line_control(regs);
 		/* Finally, enable the UART */
@@ -337,17 +341,28 @@  static const struct udevice_id pl01x_serial_id[] ={
 	{}
 };
 
+#ifndef CONFIG_PL011_CLOCK
+#define CONFIG_PL011_CLOCK 0
+#endif
+
 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);
+	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");