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