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

Message ID 20200407111236.14930-3-andre.przywara@arm.com
State Superseded
Headers show
Series
  • Arm Juno board OF_CONTROL upgrade
Related show

Commit Message

Andre Przywara April 7, 2020, 11:12 a.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>
Reviewed-by: Simon Glass <sjg at chromium.org>
---
 drivers/serial/serial_pl01x.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Tom Rini April 24, 2020, 2:51 p.m. | #1
On Tue, Apr 07, 2020 at 12:12:31PM +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>

This patch breaks rpi_3*/rpi_4/rpi_arm64, please fix, thanks!
Andre Przywara April 27, 2020, 12:21 p.m. | #2
On 24/04/2020 15:51, Tom Rini wrote:

Hi Tom,

> On Tue, Apr 07, 2020 at 12:12:31PM +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>
> 
> This patch breaks rpi_3*/rpi_4/rpi_arm64, please fix, thanks!

Doh, thanks for the heads up and apologies for not spotting this myself.

I fixed this up by catching a possibly undefined CONFIG_PL011_CLOCK.
Looking deeper into this code I think we can get rid of the unofficial
skip-init property as well, but that's another patch.

Will send a (rebased) v3 shortly.

Thanks!
Andre.

Patch

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);
+	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");