[RFC] serial: mxc: get the clock frequency from the used clock for the device

[RFC] serial: mxc: get the clock frequency from the used clock for the device

[Heiko Thiery]

With the clock driver enabled for the imx8mq, it was noticed that the
frequency used to calculate the baud rate is always taken from the root
clock of UART1. This can cause problems if UART1 is not used as console
and the settings are different from UART1. The result is that the console
output is garbage. To do this correctly the UART frequency is taken from
the used device. For the implementations that don't have the igp clock
frequency written or can't return it the old way is tried.

Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
---
 drivers/serial/serial_mxc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index e4970a169b..6fdb2b2397 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -3,6 +3,7 @@
  * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
  */
 
+#include <clk.h>
 #include <common.h>
 #include <dm.h>
 #include <errno.h>
@@ -266,9 +267,19 @@ __weak struct serial_device *default_serial_console(void)
 int mxc_serial_setbrg(struct udevice *dev, int baudrate)
 {
 	struct mxc_serial_plat *plat = dev_get_plat(dev);
-	u32 clk = imx_get_uartclk();
+	u32 rate = 0;
+
+	if (IS_ENABLED(CONFIG_CLK)) {
+		struct clk clk;
+		if(!clk_get_by_name(dev, "ipg", &clk))
+			rate = clk_get_rate(&clk);
+	}
+
+	/* as fallback we try to get the clk rate that way */
+	if (rate == 0)
+		rate = imx_get_uartclk();
 
-	_mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
+	_mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
 
 	return 0;
 }
-- 
2.30.2

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Angus Ainslie]

Hi Heiko,

On 2022-03-17 05:41, Heiko Thiery wrote:
> With the clock driver enabled for the imx8mq, it was noticed that the
> frequency used to calculate the baud rate is always taken from the root
> clock of UART1. This can cause problems if UART1 is not used as console
> and the settings are different from UART1. The result is that the 
> console
> output is garbage. To do this correctly the UART frequency is taken 
> from
> the used device. For the implementations that don't have the igp clock
> frequency written or can't return it the old way is tried.
> 
> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> ---
>  drivers/serial/serial_mxc.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> index e4970a169b..6fdb2b2397 100644
> --- a/drivers/serial/serial_mxc.c
> +++ b/drivers/serial/serial_mxc.c
> @@ -3,6 +3,7 @@
>   * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
>   */
> 
> +#include <clk.h>
>  #include <common.h>
>  #include <dm.h>
>  #include <errno.h>
> @@ -266,9 +267,19 @@ __weak struct serial_device 
> *default_serial_console(void)
>  int mxc_serial_setbrg(struct udevice *dev, int baudrate)
>  {
>  	struct mxc_serial_plat *plat = dev_get_plat(dev);
> -	u32 clk = imx_get_uartclk();
> +	u32 rate = 0;
> +
> +	if (IS_ENABLED(CONFIG_CLK)) {
> +		struct clk clk;
> +		if(!clk_get_by_name(dev, "ipg", &clk))
> +			rate = clk_get_rate(&clk);

Is the "ipg" clock the correct name for all of the imx DM boards ?

> +	}
> +
> +	/* as fallback we try to get the clk rate that way */
> +	if (rate == 0)
> +		rate = imx_get_uartclk();

Would it be better to re-write imx_get_uartclk so that both the getting 
and setting of clocks was correct ?

With DM clocks enabled I don't even think it makes sense to call those 
older functions.

Angus
> 
> -	_mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
> +	_mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
> 
>  	return 0;
>  }

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Heiko Thiery]

Hi Angus,

Am Do., 17. März 2022 um 14:19 Uhr schrieb Angus Ainslie <angus@akkea.ca>:
>
> Hi Heiko,
>
> On 2022-03-17 05:41, Heiko Thiery wrote:
> > With the clock driver enabled for the imx8mq, it was noticed that the
> > frequency used to calculate the baud rate is always taken from the root
> > clock of UART1. This can cause problems if UART1 is not used as console
> > and the settings are different from UART1. The result is that the
> > console
> > output is garbage. To do this correctly the UART frequency is taken
> > from
> > the used device. For the implementations that don't have the igp clock
> > frequency written or can't return it the old way is tried.
> >
> > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > ---
> >  drivers/serial/serial_mxc.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> > index e4970a169b..6fdb2b2397 100644
> > --- a/drivers/serial/serial_mxc.c
> > +++ b/drivers/serial/serial_mxc.c
> > @@ -3,6 +3,7 @@
> >   * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
> >   */
> >
> > +#include <clk.h>
> >  #include <common.h>
> >  #include <dm.h>
> >  #include <errno.h>
> > @@ -266,9 +267,19 @@ __weak struct serial_device
> > *default_serial_console(void)
> >  int mxc_serial_setbrg(struct udevice *dev, int baudrate)
> >  {
> >       struct mxc_serial_plat *plat = dev_get_plat(dev);
> > -     u32 clk = imx_get_uartclk();
> > +     u32 rate = 0;
> > +
> > +     if (IS_ENABLED(CONFIG_CLK)) {
> > +             struct clk clk;
> > +             if(!clk_get_by_name(dev, "ipg", &clk))
> > +                     rate = clk_get_rate(&clk);
>
> Is the "ipg" clock the correct name for all of the imx DM boards ?

I checked the dtsi files for all the boards that use the compatible
strings from the serial_mxc and see that there are always 2 clocks:
'ipg' and 'per'.

The imx8 dtsi files describe ipg and per always the same clock.
The imx7 dtsi files descibe ipg and per always the same clock.
The imx6 dtsi files describe ipg and per are 2 different clocks
The imx51 dtsi files describeipg and per are 2 different clocks

So I'm not sure if the ipg clock is the right one for the boards that
has different clock for ipg and per.

> > +     }
> > +
> > +     /* as fallback we try to get the clk rate that way */
> > +     if (rate == 0)
> > +             rate = imx_get_uartclk();
>
> Would it be better to re-write imx_get_uartclk so that both the getting
> and setting of clocks was correct ?

I do not understand what you mean with that.

> With DM clocks enabled I don't even think it makes sense to call those
> older functions.

You mean when DM clocks are available the "new" method should be used
and no fallback to the old mechanism?

>
> Angus
> >
> > -     _mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
> > +     _mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
> >
> >       return 0;
> >  }

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Angus Ainslie]

On 2022-03-18 12:06, Heiko Thiery wrote:
> Hi Angus,
> 
> Am Do., 17. März 2022 um 14:19 Uhr schrieb Angus Ainslie 
> <angus@akkea.ca>:
>> 
>> Hi Heiko,
>> 
>> On 2022-03-17 05:41, Heiko Thiery wrote:
>> > With the clock driver enabled for the imx8mq, it was noticed that the
>> > frequency used to calculate the baud rate is always taken from the root
>> > clock of UART1. This can cause problems if UART1 is not used as console
>> > and the settings are different from UART1. The result is that the
>> > console
>> > output is garbage. To do this correctly the UART frequency is taken
>> > from
>> > the used device. For the implementations that don't have the igp clock
>> > frequency written or can't return it the old way is tried.
>> >
>> > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
>> > ---
>> >  drivers/serial/serial_mxc.c | 15 +++++++++++++--
>> >  1 file changed, 13 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
>> > index e4970a169b..6fdb2b2397 100644
>> > --- a/drivers/serial/serial_mxc.c
>> > +++ b/drivers/serial/serial_mxc.c
>> > @@ -3,6 +3,7 @@
>> >   * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
>> >   */
>> >
>> > +#include <clk.h>
>> >  #include <common.h>
>> >  #include <dm.h>
>> >  #include <errno.h>
>> > @@ -266,9 +267,19 @@ __weak struct serial_device
>> > *default_serial_console(void)
>> >  int mxc_serial_setbrg(struct udevice *dev, int baudrate)
>> >  {
>> >       struct mxc_serial_plat *plat = dev_get_plat(dev);
>> > -     u32 clk = imx_get_uartclk();
>> > +     u32 rate = 0;
>> > +
>> > +     if (IS_ENABLED(CONFIG_CLK)) {
>> > +             struct clk clk;
>> > +             if(!clk_get_by_name(dev, "ipg", &clk))
>> > +                     rate = clk_get_rate(&clk);
>> 
>> Is the "ipg" clock the correct name for all of the imx DM boards ?
> 
> I checked the dtsi files for all the boards that use the compatible
> strings from the serial_mxc and see that there are always 2 clocks:
> 'ipg' and 'per'.
> 
> The imx8 dtsi files describe ipg and per always the same clock.
> The imx7 dtsi files descibe ipg and per always the same clock.
> The imx6 dtsi files describe ipg and per are 2 different clocks
> The imx51 dtsi files describeipg and per are 2 different clocks
> 
> So I'm not sure if the ipg clock is the right one for the boards that
> has different clock for ipg and per.

So I only looked at imx6qdl.dtsi where the clocks are different

clocks = <&clks IMX6QDL_CLK_UART_IPG>,
          <&clks IMX6QDL_CLK_UART_SERIAL>;
clock-names = "ipg", "per";

And from that file it looks like the per clock would be the correct one.

Should the clock be looked up by id instead of by name and then have a 
different code path for each imx board type ?

> 
>> > +     }
>> > +
>> > +     /* as fallback we try to get the clk rate that way */
>> > +     if (rate == 0)
>> > +             rate = imx_get_uartclk();
>> 
>> Would it be better to re-write imx_get_uartclk so that both the 
>> getting
>> and setting of clocks was correct ?
> 
> I do not understand what you mean with that.
> 

There are other places in the code that imx_get_uartclk gets called. If 
an index was added to imx_get_uartclk(int index) then you wouldn't need 
the code above in the mxc_serial_setbrg function. That would also make 
all of the places where imx_get_uartclk gets called return the correct 
value.

It might make sense to create a new function imx7_get_uartclk that gets 
called on newer SOCs so that the imx6 and earlier code doesn't need to 
get changed.

>> With DM clocks enabled I don't even think it makes sense to call those
>> older functions.
> 
> You mean when DM clocks are available the "new" method should be used
> and no fallback to the old mechanism?
> 

For older boards it makes sense to fallback to the single clock. On 
newer boards if it returned an error instead it would have been easier 
for you to figure out where the serial console failed.

Angus

>> 
>> Angus
>> >
>> > -     _mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
>> > +     _mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
>> >
>> >       return 0;
>> >  }

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Heiko Thiery]

Hi Angus,

[snip]

> > So I'm not sure if the ipg clock is the right one for the boards that
> > has different clock for ipg and per.
>
> So I only looked at imx6qdl.dtsi where the clocks are different
>
> clocks = <&clks IMX6QDL_CLK_UART_IPG>,
>           <&clks IMX6QDL_CLK_UART_SERIAL>;
> clock-names = "ipg", "per";
>
> And from that file it looks like the per clock would be the correct one.

Yes, 'per' seems to be the right one.

> Should the clock be looked up by id instead of by name and then have a
> different code path for each imx board type ?

But how to get the right clk id? The id's for all the implementations
are different. Or not?

>
> >
> >> > +     }
> >> > +
> >> > +     /* as fallback we try to get the clk rate that way */
> >> > +     if (rate == 0)
> >> > +             rate = imx_get_uartclk();
> >>
> >> Would it be better to re-write imx_get_uartclk so that both the
> >> getting
> >> and setting of clocks was correct ?
> >
> > I do not understand what you mean with that.
> >
>
> There are other places in the code that imx_get_uartclk gets called. If
> an index was added to imx_get_uartclk(int index) then you wouldn't need
> the code above in the mxc_serial_setbrg function. That would also make
> all of the places where imx_get_uartclk gets called return the correct
> value.

By index do you mean the clk id?

>
> It might make sense to create a new function imx7_get_uartclk that gets
> called on newer SOCs so that the imx6 and earlier code doesn't need to
> get changed.

At what point should this be called instead of the imx_get_uartclk() function?


>
> >> With DM clocks enabled I don't even think it makes sense to call those
> >> older functions.
> >
> > You mean when DM clocks are available the "new" method should be used
> > and no fallback to the old mechanism?
> >
>
> For older boards it makes sense to fallback to the single clock. On
> newer boards if it returned an error instead it would have been easier
> for you to figure out where the serial console failed.

--
Heiko

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Angus Ainslie]

On 2022-03-21 06:50, Heiko Thiery wrote:
> Hi Angus,
> 
> [snip]
> 
>> > So I'm not sure if the ipg clock is the right one for the boards that
>> > has different clock for ipg and per.
>> 
>> So I only looked at imx6qdl.dtsi where the clocks are different
>> 
>> clocks = <&clks IMX6QDL_CLK_UART_IPG>,
>>           <&clks IMX6QDL_CLK_UART_SERIAL>;
>> clock-names = "ipg", "per";
>> 
>> And from that file it looks like the per clock would be the correct 
>> one.
> 
> Yes, 'per' seems to be the right one.
> 
>> Should the clock be looked up by id instead of by name and then have a
>> different code path for each imx board type ?
> 
> But how to get the right clk id? The id's for all the implementations
> are different. Or not?
> 

Yeah you're correct that won't work.

>> 
>> >
>> >> > +     }
>> >> > +
>> >> > +     /* as fallback we try to get the clk rate that way */
>> >> > +     if (rate == 0)
>> >> > +             rate = imx_get_uartclk();
>> >>
>> >> Would it be better to re-write imx_get_uartclk so that both the
>> >> getting
>> >> and setting of clocks was correct ?
>> >
>> > I do not understand what you mean with that.
>> >
>> 
>> There are other places in the code that imx_get_uartclk gets called. 
>> If
>> an index was added to imx_get_uartclk(int index) then you wouldn't 
>> need
>> the code above in the mxc_serial_setbrg function. That would also make
>> all of the places where imx_get_uartclk gets called return the correct
>> value.
> 
> By index do you mean the clk id?
> 

No I was thinking number of the device uart[0-3].

Thinking about it some more it's probably not useful as you already have 
the udevice pointer. I was just trying to think of ways to reduce the 
amount of code change for the older SOCs. Using the 'per' clock is a 
better solution.

>> 
>> It might make sense to create a new function imx7_get_uartclk that 
>> gets
>> called on newer SOCs so that the imx6 and earlier code doesn't need to
>> get changed.
> 
> At what point should this be called instead of the imx_get_uartclk() 
> function?
> 

I was thinking a CONFIG_IS_ENBLED switch between the old version and the 
new one based on SOC.

Angus

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Adam Ford]

On Tue, Mar 22, 2022 at 7:48 AM Angus Ainslie <angus@akkea.ca> wrote:
>
> On 2022-03-21 06:50, Heiko Thiery wrote:
> > Hi Angus,
> >
> > [snip]
> >
> >> > So I'm not sure if the ipg clock is the right one for the boards that
> >> > has different clock for ipg and per.
> >>
> >> So I only looked at imx6qdl.dtsi where the clocks are different
> >>
> >> clocks = <&clks IMX6QDL_CLK_UART_IPG>,
> >>           <&clks IMX6QDL_CLK_UART_SERIAL>;
> >> clock-names = "ipg", "per";
> >>
> >> And from that file it looks like the per clock would be the correct
> >> one.
> >
> > Yes, 'per' seems to be the right one.
> >
> >> Should the clock be looked up by id instead of by name and then have a
> >> different code path for each imx board type ?
> >
> > But how to get the right clk id? The id's for all the implementations
> > are different. Or not?
> >
>
> Yeah you're correct that won't work.
>
> >>
> >> >
> >> >> > +     }
> >> >> > +
> >> >> > +     /* as fallback we try to get the clk rate that way */
> >> >> > +     if (rate == 0)
> >> >> > +             rate = imx_get_uartclk();
> >> >>
> >> >> Would it be better to re-write imx_get_uartclk so that both the
> >> >> getting
> >> >> and setting of clocks was correct ?
> >> >
> >> > I do not understand what you mean with that.
> >> >
> >>
> >> There are other places in the code that imx_get_uartclk gets called.
> >> If
> >> an index was added to imx_get_uartclk(int index) then you wouldn't
> >> need
> >> the code above in the mxc_serial_setbrg function. That would also make
> >> all of the places where imx_get_uartclk gets called return the correct
> >> value.
> >
> > By index do you mean the clk id?
> >
>
> No I was thinking number of the device uart[0-3].
>
> Thinking about it some more it's probably not useful as you already have
> the udevice pointer. I was just trying to think of ways to reduce the
> amount of code change for the older SOCs. Using the 'per' clock is a
> better solution.
>
> >>
> >> It might make sense to create a new function imx7_get_uartclk that
> >> gets
> >> called on newer SOCs so that the imx6 and earlier code doesn't need to
> >> get changed.
> >
> > At what point should this be called instead of the imx_get_uartclk()
> > function?
> >
>
> I was thinking a CONFIG_IS_ENBLED switch between the old version and the
> new one based on SOC.

I was thinking that we could expand the struct mxc_serial_plat to
include both per and igp clocks to cover devices have clocks that are
not the same.  The configuring of the platdata can happen using
mxc_serial_of_to_plat to 'get' both igp and per clocks.  The probe
would enable both per and igp to ensure they are operating, then the
mxc_serial_setbrg could use clk_get_rate(plat->clk_igp) to determine
the clock rate.

I am guessing the clock composite would have to be expanded to include
the UART clocks because from what I can see, they're not included.
However, this could potentially eliminate the need to use some of the
functions in arch/arm/mach-imx/imx8m/clock_imx8mm.

The down-side is that a bunch of SoC's might need to be updated to
support more and more clocks, so we could potentially use
!CONFIG_IS_ENABLED to use the existing functions as a fall-back.

adam
>
> Angus

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Heiko Thiery]

Hi Adam,

[SNIP]

>
> I was thinking that we could expand the struct mxc_serial_plat to
> include both per and igp clocks to cover devices have clocks that are
> not the same.  The configuring of the platdata can happen using
> mxc_serial_of_to_plat to 'get' both igp and per clocks.  The probe
> would enable both per and igp to ensure they are operating, then the
> mxc_serial_setbrg could use clk_get_rate(plat->clk_igp) to determine
> the clock rate.

With your comment I have made the following, does this fit with your suggestion?

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index e4970a169b..7f9f7e8383 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -3,6 +3,7 @@
  * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
  */

+#include <clk.h>
 #include <common.h>
 #include <dm.h>
 #include <errno.h>
@@ -266,9 +267,16 @@ __weak struct serial_device *default_serial_console(void)
 int mxc_serial_setbrg(struct udevice *dev, int baudrate)
 {
        struct mxc_serial_plat *plat = dev_get_plat(dev);
-       u32 clk = imx_get_uartclk();
+       u32 rate = 0;
+
+#if defined(CONFIG_CLK)
+       rate = clk_get_rate(&plat->per_clk);
+#else
+       /* as fallback we try to get the clk rate that way */
+       rate = imx_get_uartclk();
+#endif

-       _mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
+       _mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);

        return 0;
 }
@@ -277,6 +285,11 @@ static int mxc_serial_probe(struct udevice *dev)
 {
        struct mxc_serial_plat *plat = dev_get_plat(dev);

+#if defined(CONFIG_CLK)
+       clk_enable(&plat->ipg_clk);
+       clk_enable(&plat->per_clk);
+#endif
+
        _mxc_serial_init(plat->reg, plat->use_dte);

        return 0;
@@ -339,6 +352,10 @@ static int mxc_serial_of_to_plat(struct udevice *dev)

        plat->use_dte = fdtdec_get_bool(gd->fdt_blob, dev_of_offset(dev),
                                        "fsl,dte-mode");
+#if defined(CONFIG_CLK)
+       clk_get_by_name(dev, "ipg", &plat->ipg_clk);
+       clk_get_by_name(dev, "per", &plat->per_clk);
+#endif
        return 0;
 }

diff --git a/include/dm/platform_data/serial_mxc.h
b/include/dm/platform_data/serial_mxc.h
index cc59eeb1dd..330476f816 100644
--- a/include/dm/platform_data/serial_mxc.h
+++ b/include/dm/platform_data/serial_mxc.h
@@ -10,6 +10,10 @@
 struct mxc_serial_plat {
        struct mxc_uart *reg;  /* address of registers in physical memory */
        bool use_dte;
+#if defined(CONFIG_CLK)
+       struct clk ipg_clk;
+       struct clk per_clk;
+#endif
 };

 #endif

> I am guessing the clock composite would have to be expanded to include
> the UART clocks because from what I can see, they're not included.
> However, this could potentially eliminate the need to use some of the
> functions in arch/arm/mach-imx/imx8m/clock_imx8mm.

For the imx8mq I added this and asked Angus to integrate it into his
patchset. [1]

> The down-side is that a bunch of SoC's might need to be updated to
> support more and more clocks, so we could potentially use
> !CONFIG_IS_ENABLED to use the existing functions as a fall-back.
>
> adam
> >
> > Angus

-- 
Heiko

[1] https://lore.kernel.org/u-boot/CAEyMn7bC-p6o6VCu7YWQyXMm+51EcJ5OVXv_625cjDiaR+3TyQ@mail.gmail.com/

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Adam Ford]

On Thu, Mar 24, 2022 at 4:58 AM Heiko Thiery <heiko.thiery@gmail.com> wrote:
>
> Hi Adam,
>
> [SNIP]
>
> >
> > I was thinking that we could expand the struct mxc_serial_plat to
> > include both per and igp clocks to cover devices have clocks that are
> > not the same.  The configuring of the platdata can happen using
> > mxc_serial_of_to_plat to 'get' both igp and per clocks.  The probe
> > would enable both per and igp to ensure they are operating, then the
> > mxc_serial_setbrg could use clk_get_rate(plat->clk_igp) to determine
> > the clock rate.
>
> With your comment I have made the following, does this fit with your suggestion?
>
> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> index e4970a169b..7f9f7e8383 100644
> --- a/drivers/serial/serial_mxc.c
> +++ b/drivers/serial/serial_mxc.c
> @@ -3,6 +3,7 @@
>   * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
>   */
>
> +#include <clk.h>
>  #include <common.h>
>  #include <dm.h>
>  #include <errno.h>
> @@ -266,9 +267,16 @@ __weak struct serial_device *default_serial_console(void)
>  int mxc_serial_setbrg(struct udevice *dev, int baudrate)
>  {
>         struct mxc_serial_plat *plat = dev_get_plat(dev);
> -       u32 clk = imx_get_uartclk();
> +       u32 rate = 0;
> +
> +#if defined(CONFIG_CLK)
> +       rate = clk_get_rate(&plat->per_clk);
> +#else
> +       /* as fallback we try to get the clk rate that way */
> +       rate = imx_get_uartclk();
> +#endif
>
> -       _mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
> +       _mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
>
>         return 0;
>  }
> @@ -277,6 +285,11 @@ static int mxc_serial_probe(struct udevice *dev)
>  {
>         struct mxc_serial_plat *plat = dev_get_plat(dev);
>
> +#if defined(CONFIG_CLK)
> +       clk_enable(&plat->ipg_clk);
> +       clk_enable(&plat->per_clk);
> +#endif
> +
>         _mxc_serial_init(plat->reg, plat->use_dte);
>
>         return 0;
> @@ -339,6 +352,10 @@ static int mxc_serial_of_to_plat(struct udevice *dev)
>
>         plat->use_dte = fdtdec_get_bool(gd->fdt_blob, dev_of_offset(dev),
>                                         "fsl,dte-mode");
> +#if defined(CONFIG_CLK)
> +       clk_get_by_name(dev, "ipg", &plat->ipg_clk);
> +       clk_get_by_name(dev, "per", &plat->per_clk);
> +#endif
>         return 0;
>  }
>
> diff --git a/include/dm/platform_data/serial_mxc.h
> b/include/dm/platform_data/serial_mxc.h
> index cc59eeb1dd..330476f816 100644
> --- a/include/dm/platform_data/serial_mxc.h
> +++ b/include/dm/platform_data/serial_mxc.h
> @@ -10,6 +10,10 @@
>  struct mxc_serial_plat {
>         struct mxc_uart *reg;  /* address of registers in physical memory */
>         bool use_dte;
> +#if defined(CONFIG_CLK)
> +       struct clk ipg_clk;
> +       struct clk per_clk;
> +#endif
>  };
>
>  #endif
>
> > I am guessing the clock composite would have to be expanded to include
> > the UART clocks because from what I can see, they're not included.
> > However, this could potentially eliminate the need to use some of the
> > functions in arch/arm/mach-imx/imx8m/clock_imx8mm.
>
> For the imx8mq I added this and asked Angus to integrate it into his
> patchset. [1]

That's basically what I was thinking.  It probably wouldn't hurt to
add some error handling.  What's is not clear to me is the impact of
every other IMX platform out there.  I am guessing the clk_get_by_name
calls will fail if the UART clocks haven't been configured or added to
the SoC's respective clock driver.  We also might want to check for
the presence of plat->ipg_clk and plat->per_clk before requesting
their clock rate.  If either the UART clocks don't exist or ipg or per
don't exist, you'll likely need to fallback on the older functions as
well to avoid crashing.

adam
>
> > The down-side is that a bunch of SoC's might need to be updated to
> > support more and more clocks, so we could potentially use
> > !CONFIG_IS_ENABLED to use the existing functions as a fall-back.
> >
> > adam
> > >
> > > Angus
>
> --
> Heiko
>
> [1] https://lore.kernel.org/u-boot/CAEyMn7bC-p6o6VCu7YWQyXMm+51EcJ5OVXv_625cjDiaR+3TyQ@mail.gmail.com/

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Sean Anderson]

Hi Heiko,

On 3/17/22 8:41 AM, Heiko Thiery wrote:
> With the clock driver enabled for the imx8mq, it was noticed that the
> frequency used to calculate the baud rate is always taken from the root
> clock of UART1. This can cause problems if UART1 is not used as console
> and the settings are different from UART1. The result is that the console
> output is garbage. To do this correctly the UART frequency is taken from
> the used device. For the implementations that don't have the igp clock
> frequency written or can't return it the old way is tried.
> 
> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> ---
>   drivers/serial/serial_mxc.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> index e4970a169b..6fdb2b2397 100644
> --- a/drivers/serial/serial_mxc.c
> +++ b/drivers/serial/serial_mxc.c
> @@ -3,6 +3,7 @@
>    * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
>    */
>   
> +#include <clk.h>
>   #include <common.h>
>   #include <dm.h>
>   #include <errno.h>
> @@ -266,9 +267,19 @@ __weak struct serial_device *default_serial_console(void)
>   int mxc_serial_setbrg(struct udevice *dev, int baudrate)
>   {
>   	struct mxc_serial_plat *plat = dev_get_plat(dev);
> -	u32 clk = imx_get_uartclk();
> +	u32 rate = 0;
> +
> +	if (IS_ENABLED(CONFIG_CLK)) {

CONFIG_IS_ENABLED?

mx6ull at least does not have CONFIG_SPL_CLK enabled.

> +		struct clk clk;
> +		if(!clk_get_by_name(dev, "ipg", &clk))
> +			rate = clk_get_rate(&clk);
> +	}
> +
> +	/* as fallback we try to get the clk rate that way */
> +	if (rate == 0)

!rate || IS_ERR_VALUE(rate)

> +		rate = imx_get_uartclk();
>   
> -	_mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
> +	_mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
>   
>   	return 0;
>   }
> 
--Sean

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Michael Walle]

>> +		struct clk clk;
>> +		if(!clk_get_by_name(dev, "ipg", &clk))
>> +			rate = clk_get_rate(&clk);
>> +	}
>> +
>> +	/* as fallback we try to get the clk rate that way */
>> +	if (rate == 0)
> 
> !rate || IS_ERR_VALUE(rate)

This looked so weird I had to actually look at clk_get_rate()
in u-boot.

/**
  * clk_get_rate() - Get current clock rate.
  * @clk:	A clock struct that was previously successfully requested by
  *		clk_request/get_by_*().
  *
  * Return: clock rate in Hz on success, 0 for invalid clock, or -ve 
error code
  *	   for other errors.
  */
ulong clk_get_rate(struct clk *clk);

How can an ulong return a negative error value? What if the clock speed
happens to be the same as -errno?

-michael

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Sean Anderson]

On 3/17/22 10:47 AM, Michael Walle wrote:
> 
>>> +        struct clk clk;
>>> +        if(!clk_get_by_name(dev, "ipg", &clk))
>>> +            rate = clk_get_rate(&clk);
>>> +    }
>>> +
>>> +    /* as fallback we try to get the clk rate that way */
>>> +    if (rate == 0)
>>
>> !rate || IS_ERR_VALUE(rate)
> 
> This looked so weird I had to actually look at clk_get_rate()
> in u-boot.
> 
> /**
>   * clk_get_rate() - Get current clock rate.
>   * @clk:    A clock struct that was previously successfully requested by
>   *        clk_request/get_by_*().
>   *
>   * Return: clock rate in Hz on success, 0 for invalid clock, or -ve error code
>   *       for other errors.
>   */
> ulong clk_get_rate(struct clk *clk);
> 
> How can an ulong return a negative error value?

It can't. Which is why we need IS_ERR_VALUE and not < 0.

> What if the clock speed happens to be the same as -errno?

Due to physical constraints, this isn't an issue on 64 bit systems. And you
could run into trouble if you have a 32-bit system with a 4GHz processor.

I don't think it's the best interface. In e.g. Linux clk_get_rate returns 0 on
failure, which I think is reasonable. If you are interested, please send a patch series :)

--Sean

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2484 bytes --]

Hi Sean,

> Hi Heiko,
> 
> On 3/17/22 8:41 AM, Heiko Thiery wrote:
> > With the clock driver enabled for the imx8mq, it was noticed that
> > the frequency used to calculate the baud rate is always taken from
> > the root clock of UART1. This can cause problems if UART1 is not
> > used as console and the settings are different from UART1. The
> > result is that the console output is garbage. To do this correctly
> > the UART frequency is taken from the used device. For the
> > implementations that don't have the igp clock frequency written or
> > can't return it the old way is tried.
> > 
> > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > ---
> >   drivers/serial/serial_mxc.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/serial/serial_mxc.c
> > b/drivers/serial/serial_mxc.c index e4970a169b..6fdb2b2397 100644
> > --- a/drivers/serial/serial_mxc.c
> > +++ b/drivers/serial/serial_mxc.c
> > @@ -3,6 +3,7 @@
> >    * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
> >    */
> >   
> > +#include <clk.h>
> >   #include <common.h>
> >   #include <dm.h>
> >   #include <errno.h>
> > @@ -266,9 +267,19 @@ __weak struct serial_device
> > *default_serial_console(void) int mxc_serial_setbrg(struct udevice
> > *dev, int baudrate) {
> >   	struct mxc_serial_plat *plat = dev_get_plat(dev);
> > -	u32 clk = imx_get_uartclk();
> > +	u32 rate = 0;
> > +
> > +	if (IS_ENABLED(CONFIG_CLK)) {  
> 
> CONFIG_IS_ENABLED?
> 
> mx6ull at least does not have CONFIG_SPL_CLK enabled.

The problem with serial is that not all boards support DM clock in SPL.
I'm wondering if this patch has passed the CI tests for all boards.

> 
> > +		struct clk clk;
> > +		if(!clk_get_by_name(dev, "ipg", &clk))
> > +			rate = clk_get_rate(&clk);
> > +	}
> > +
> > +	/* as fallback we try to get the clk rate that way */
> > +	if (rate == 0)  
> 
> !rate || IS_ERR_VALUE(rate)
> 
> > +		rate = imx_get_uartclk();
> >   
> > -	_mxc_serial_setbrg(plat->reg, clk, baudrate,
> > plat->use_dte);
> > +	_mxc_serial_setbrg(plat->reg, rate, baudrate,
> > plat->use_dte); 
> >   	return 0;
> >   }
> >   
> --Sean




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Heiko Thiery]

Hi Sean,

Am Do., 17. März 2022 um 15:38 Uhr schrieb Sean Anderson <seanga2@gmail.com>:
>
> Hi Heiko,
>
> On 3/17/22 8:41 AM, Heiko Thiery wrote:
> > With the clock driver enabled for the imx8mq, it was noticed that the
> > frequency used to calculate the baud rate is always taken from the root
> > clock of UART1. This can cause problems if UART1 is not used as console
> > and the settings are different from UART1. The result is that the console
> > output is garbage. To do this correctly the UART frequency is taken from
> > the used device. For the implementations that don't have the igp clock
> > frequency written or can't return it the old way is tried.
> >
> > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > ---
> >   drivers/serial/serial_mxc.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> > index e4970a169b..6fdb2b2397 100644
> > --- a/drivers/serial/serial_mxc.c
> > +++ b/drivers/serial/serial_mxc.c
> > @@ -3,6 +3,7 @@
> >    * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
> >    */
> >
> > +#include <clk.h>
> >   #include <common.h>
> >   #include <dm.h>
> >   #include <errno.h>
> > @@ -266,9 +267,19 @@ __weak struct serial_device *default_serial_console(void)
> >   int mxc_serial_setbrg(struct udevice *dev, int baudrate)
> >   {
> >       struct mxc_serial_plat *plat = dev_get_plat(dev);
> > -     u32 clk = imx_get_uartclk();
> > +     u32 rate = 0;
> > +
> > +     if (IS_ENABLED(CONFIG_CLK)) {
>
> CONFIG_IS_ENABLED?

This should be correct. The CONFIG_IS_ENABLED is a preprocessor macro
and the IS_ENABLED can be used in c code. Or do you mean something
else?

>
> mx6ull at least does not have CONFIG_SPL_CLK enabled.

I expect that in that case it will fallback to the old behavior ... not?

>
> > +             struct clk clk;
> > +             if(!clk_get_by_name(dev, "ipg", &clk))
> > +                     rate = clk_get_rate(&clk);
> > +     }
> > +
> > +     /* as fallback we try to get the clk rate that way */
> > +     if (rate == 0)
>
> !rate || IS_ERR_VALUE(rate)
>
> > +             rate = imx_get_uartclk();
> >
> > -     _mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
> > +     _mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
> >
> >       return 0;
> >   }
> >
> --Sean

-- 
Heiko

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Sean Anderson]

On 3/17/22 3:14 PM, Heiko Thiery wrote:
> Hi Sean,
> 
> Am Do., 17. März 2022 um 15:38 Uhr schrieb Sean Anderson <seanga2@gmail.com>:
>>
>> Hi Heiko,
>>
>> On 3/17/22 8:41 AM, Heiko Thiery wrote:
>>> With the clock driver enabled for the imx8mq, it was noticed that the
>>> frequency used to calculate the baud rate is always taken from the root
>>> clock of UART1. This can cause problems if UART1 is not used as console
>>> and the settings are different from UART1. The result is that the console
>>> output is garbage. To do this correctly the UART frequency is taken from
>>> the used device. For the implementations that don't have the igp clock
>>> frequency written or can't return it the old way is tried.
>>>
>>> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
>>> ---
>>>    drivers/serial/serial_mxc.c | 15 +++++++++++++--
>>>    1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
>>> index e4970a169b..6fdb2b2397 100644
>>> --- a/drivers/serial/serial_mxc.c
>>> +++ b/drivers/serial/serial_mxc.c
>>> @@ -3,6 +3,7 @@
>>>     * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
>>>     */
>>>
>>> +#include <clk.h>
>>>    #include <common.h>
>>>    #include <dm.h>
>>>    #include <errno.h>
>>> @@ -266,9 +267,19 @@ __weak struct serial_device *default_serial_console(void)
>>>    int mxc_serial_setbrg(struct udevice *dev, int baudrate)
>>>    {
>>>        struct mxc_serial_plat *plat = dev_get_plat(dev);
>>> -     u32 clk = imx_get_uartclk();
>>> +     u32 rate = 0;
>>> +
>>> +     if (IS_ENABLED(CONFIG_CLK)) {
>>
>> CONFIG_IS_ENABLED?
> 
> This should be correct. The CONFIG_IS_ENABLED is a preprocessor macro
> and the IS_ENABLED can be used in c code. Or do you mean something
> else?

I mean you should be using CONFIG_IS_ENABLED(CLK) so that this code is
correct for both SPL and U-Boot proper. But it is also fine to "try and
fail" (making this check unnecessary).

>>
>> mx6ull at least does not have CONFIG_SPL_CLK enabled.
> 
> I expect that in that case it will fallback to the old behavior ... not?

Yes, if you handle -ENOSYS correctly.

>>
>>> +             struct clk clk;
>>> +             if(!clk_get_by_name(dev, "ipg", &clk))
>>> +                     rate = clk_get_rate(&clk);

You may also need to enable this clock.

>>> +     }
>>> +
>>> +     /* as fallback we try to get the clk rate that way */
>>> +     if (rate == 0)
>>
>> !rate || IS_ERR_VALUE(rate)
>>
>>> +             rate = imx_get_uartclk();
>>>
>>> -     _mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
>>> +     _mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
>>>
>>>        return 0;
>>>    }
>>>
>> --Sean
> 

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Heiko Thiery]

Hi Sean,

Am Fr., 18. März 2022 um 03:19 Uhr schrieb Sean Anderson <seanga2@gmail.com>:
>
> On 3/17/22 3:14 PM, Heiko Thiery wrote:
> > Hi Sean,
> >
> > Am Do., 17. März 2022 um 15:38 Uhr schrieb Sean Anderson <seanga2@gmail.com>:
> >>
> >> Hi Heiko,
> >>
> >> On 3/17/22 8:41 AM, Heiko Thiery wrote:
> >>> With the clock driver enabled for the imx8mq, it was noticed that the
> >>> frequency used to calculate the baud rate is always taken from the root
> >>> clock of UART1. This can cause problems if UART1 is not used as console
> >>> and the settings are different from UART1. The result is that the console
> >>> output is garbage. To do this correctly the UART frequency is taken from
> >>> the used device. For the implementations that don't have the igp clock
> >>> frequency written or can't return it the old way is tried.
> >>>
> >>> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> >>> ---
> >>>    drivers/serial/serial_mxc.c | 15 +++++++++++++--
> >>>    1 file changed, 13 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> >>> index e4970a169b..6fdb2b2397 100644
> >>> --- a/drivers/serial/serial_mxc.c
> >>> +++ b/drivers/serial/serial_mxc.c
> >>> @@ -3,6 +3,7 @@
> >>>     * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
> >>>     */
> >>>
> >>> +#include <clk.h>
> >>>    #include <common.h>
> >>>    #include <dm.h>
> >>>    #include <errno.h>
> >>> @@ -266,9 +267,19 @@ __weak struct serial_device *default_serial_console(void)
> >>>    int mxc_serial_setbrg(struct udevice *dev, int baudrate)
> >>>    {
> >>>        struct mxc_serial_plat *plat = dev_get_plat(dev);
> >>> -     u32 clk = imx_get_uartclk();
> >>> +     u32 rate = 0;
> >>> +
> >>> +     if (IS_ENABLED(CONFIG_CLK)) {
> >>
> >> CONFIG_IS_ENABLED?
> >
> > This should be correct. The CONFIG_IS_ENABLED is a preprocessor macro
> > and the IS_ENABLED can be used in c code. Or do you mean something
> > else?
>
> I mean you should be using CONFIG_IS_ENABLED(CLK) so that this code is
> correct for both SPL and U-Boot proper. But it is also fine to "try and
> fail" (making this check unnecessary).
>
> >>
> >> mx6ull at least does not have CONFIG_SPL_CLK enabled.
> >
> > I expect that in that case it will fallback to the old behavior ... not?
>
> Yes, if you handle -ENOSYS correctly.
>
> >>
> >>> +             struct clk clk;
> >>> +             if(!clk_get_by_name(dev, "ipg", &clk))
> >>> +                     rate = clk_get_rate(&clk);
>
> You may also need to enable this clock.

What would be the right place to enable the clock? in mxc_serial_probe()?
>
> >>> +     }
> >>> +
> >>> +     /* as fallback we try to get the clk rate that way */
> >>> +     if (rate == 0)
> >>
> >> !rate || IS_ERR_VALUE(rate)
> >>
> >>> +             rate = imx_get_uartclk();
> >>>
> >>> -     _mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
> >>> +     _mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
> >>>
> >>>        return 0;
> >>>    }
> >>>
> >> --Sean
> >
>
>

Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Sean Anderson]

On 3/18/22 4:05 AM, Heiko Thiery wrote:
> Hi Sean,
> 
> Am Fr., 18. März 2022 um 03:19 Uhr schrieb Sean Anderson <seanga2@gmail.com>:
>>
>> On 3/17/22 3:14 PM, Heiko Thiery wrote:
>>> Hi Sean,
>>>
>>> Am Do., 17. März 2022 um 15:38 Uhr schrieb Sean Anderson <seanga2@gmail.com>:
>>>>
>>>> Hi Heiko,
>>>>
>>>> On 3/17/22 8:41 AM, Heiko Thiery wrote:
>>>>> With the clock driver enabled for the imx8mq, it was noticed that the
>>>>> frequency used to calculate the baud rate is always taken from the root
>>>>> clock of UART1. This can cause problems if UART1 is not used as console
>>>>> and the settings are different from UART1. The result is that the console
>>>>> output is garbage. To do this correctly the UART frequency is taken from
>>>>> the used device. For the implementations that don't have the igp clock
>>>>> frequency written or can't return it the old way is tried.
>>>>>
>>>>> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
>>>>> ---
>>>>>     drivers/serial/serial_mxc.c | 15 +++++++++++++--
>>>>>     1 file changed, 13 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
>>>>> index e4970a169b..6fdb2b2397 100644
>>>>> --- a/drivers/serial/serial_mxc.c
>>>>> +++ b/drivers/serial/serial_mxc.c
>>>>> @@ -3,6 +3,7 @@
>>>>>      * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
>>>>>      */
>>>>>
>>>>> +#include <clk.h>
>>>>>     #include <common.h>
>>>>>     #include <dm.h>
>>>>>     #include <errno.h>
>>>>> @@ -266,9 +267,19 @@ __weak struct serial_device *default_serial_console(void)
>>>>>     int mxc_serial_setbrg(struct udevice *dev, int baudrate)
>>>>>     {
>>>>>         struct mxc_serial_plat *plat = dev_get_plat(dev);
>>>>> -     u32 clk = imx_get_uartclk();
>>>>> +     u32 rate = 0;
>>>>> +
>>>>> +     if (IS_ENABLED(CONFIG_CLK)) {
>>>>
>>>> CONFIG_IS_ENABLED?
>>>
>>> This should be correct. The CONFIG_IS_ENABLED is a preprocessor macro
>>> and the IS_ENABLED can be used in c code. Or do you mean something
>>> else?
>>
>> I mean you should be using CONFIG_IS_ENABLED(CLK) so that this code is
>> correct for both SPL and U-Boot proper. But it is also fine to "try and
>> fail" (making this check unnecessary).
>>
>>>>
>>>> mx6ull at least does not have CONFIG_SPL_CLK enabled.
>>>
>>> I expect that in that case it will fallback to the old behavior ... not?
>>
>> Yes, if you handle -ENOSYS correctly.
>>
>>>>
>>>>> +             struct clk clk;
>>>>> +             if(!clk_get_by_name(dev, "ipg", &clk))
>>>>> +                     rate = clk_get_rate(&clk);
>>
>> You may also need to enable this clock.
> 
> What would be the right place to enable the clock? in mxc_serial_probe()?

Yes.

--Sean