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

[Angus Ainslie <angus@akkea.ca>]

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;
>> >  }