diff mbox series

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

Message ID 20200325144702.16288-3-andre.przywara@arm.com
State Superseded
Delegated to: Tom Rini
Headers show
Series Arm Juno board OF_CONTROL upgrade | expand

Commit Message

André Przywara March 25, 2020, 2:46 p.m. UTC
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@arm.com>
---
 drivers/serial/serial_pl01x.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Simon Glass March 26, 2020, 4:20 p.m. UTC | #1
Hi Andre,

On Wed, 25 Mar 2020 at 08:47, Andre Przywara <andre.przywara@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.
>
> 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@arm.com>
> ---
>  drivers/serial/serial_pl01x.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> 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);

is this needed?

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

Regards,
Simon
André Przywara March 26, 2020, 5:06 p.m. UTC | #2
On 26/03/2020 16:20, Simon Glass wrote:

Hi Simon,

> On Wed, 25 Mar 2020 at 08:47, Andre Przywara <andre.przywara@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.
>>
>> 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@arm.com>
>> ---
>>  drivers/serial/serial_pl01x.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> 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);
> 
> is this needed?

This is to provide the existing behaviour as a fallback. Some SoCs have
a complex clock providing the baud rate clock (HiKey 960, FSL LS2080a),
which U-Boot doesn't suport. I'd rather not break them, but also don't
really want to provide a clock driver ;-)

Also this mimics the !DM_SERIAL behaviour, which sets this clock rate
based on Kconfig, again as a fallback. I needed that because I think the
clock driver wasn't ready that early. It's a bit hard to confirm without
serial output ;-)

So the order should be:
- If there is a clocks property and we support that clock provider
(fixed-clock), then use that value.
- If not, check for a "clock" property in the DT node and use that value.
- If there is no "clock property", use the Kconfig variable.

Just written the other way around in the code.

Does this make sense?

Cheers,
Andre.

>> +       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");
>>
>> --
>> 2.14.5
>>
> 
> Regards,
> Simon
>
Simon Glass March 26, 2020, 9:30 p.m. UTC | #3
Hi Andre,

On Thu, 26 Mar 2020 at 11:06, André Przywara <andre.przywara@arm.com> wrote:
>
> On 26/03/2020 16:20, Simon Glass wrote:
>
> Hi Simon,
>
> > On Wed, 25 Mar 2020 at 08:47, Andre Przywara <andre.przywara@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.
> >>
> >> 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@arm.com>
> >> ---
> >>  drivers/serial/serial_pl01x.c | 10 +++++++++-
> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> 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);
> >
> > is this needed?
>
> This is to provide the existing behaviour as a fallback. Some SoCs have
> a complex clock providing the baud rate clock (HiKey 960, FSL LS2080a),
> which U-Boot doesn't suport. I'd rather not break them, but also don't
> really want to provide a clock driver ;-)
>
> Also this mimics the !DM_SERIAL behaviour, which sets this clock rate
> based on Kconfig, again as a fallback. I needed that because I think the
> clock driver wasn't ready that early. It's a bit hard to confirm without
> serial output ;-)
>
> So the order should be:
> - If there is a clocks property and we support that clock provider
> (fixed-clock), then use that value.
> - If not, check for a "clock" property in the DT node and use that value.
> - If there is no "clock property", use the Kconfig variable.
>
> Just written the other way around in the code.
>
> Does this make sense?

Hmm it might make more sense to migrate the boards?

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon
Linus Walleij March 26, 2020, 10:34 p.m. UTC | #4
On Wed, Mar 25, 2020 at 3:47 PM Andre Przywara <andre.przywara@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.
>
> 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@arm.com>

A good start for other platforms to follow.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
diff mbox series

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