[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 at 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 at 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 at 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 at 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
> >
>
>