Re: [PATCH 1/2] clk: imx: enable the earlycon uart clocks by parsing from dt

[Adam Ford <aford173@gmail.com>]

On Mon, Jan 4, 2021 at 1:12 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> Hi Adam,
>
> On Tue, Dec 29, 2020 at 08:51:28AM -0600, Adam Ford wrote:
> > Remove the earlycon uart clocks that are hard cord in platforms
> > clock driver, instead of parsing the earlycon uart port from dt
>
> "instead parse the earlycon uart..."
>
> Otherwise it's confusing what you mean here.
>
> > and enable these clocks from clock property in dt node.
> >
> > Fixes: 9461f7b33d11c ("clk: fix CLK_SET_RATE_GATE with clock rate protection")
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> > Based on NXP's code base and adapted for 5.11-rc1.
> > https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/clk/imx/clk.c?h=imx_5.4.47_2.2.0&id=754ae82cc55b7445545fc2f092a70e0f490e9c1b
> >
> > The original signed-off was retained.
> > Added the fixes tag.
> > ---
> >  drivers/clk/imx/clk.c | 43 +++++++++++++++++++++++++++++--------------
> >  1 file changed, 29 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> > index 47882c51cb85..c32b46890945 100644
> > --- a/drivers/clk/imx/clk.c
> > +++ b/drivers/clk/imx/clk.c
> > @@ -148,7 +148,7 @@ void imx_cscmr1_fixup(u32 *val)
> >
> >  #ifndef MODULE
> >  static int imx_keep_uart_clocks;
> > -static struct clk ** const *imx_uart_clocks;
> > +static bool imx_uart_clks_on;
> >
> >  static int __init imx_keep_uart_clocks_param(char *str)
> >  {
> > @@ -161,25 +161,40 @@ __setup_param("earlycon", imx_keep_uart_earlycon,
> >  __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> >             imx_keep_uart_clocks_param, 0);
> >
> > -void imx_register_uart_clocks(struct clk ** const clks[])
> > +static void imx_earlycon_uart_clks_onoff(bool is_on)
>
> "is_on" sounds like it's the current state of the clock, but actually
> the variable is used for the desired state, so I suggest using plain
> "on" as name.

Sascha,

I think I'll try to keep the existing structure of imx/clk.c in place
so this function won't be needed.  It was part of NXP's custom kernel,
but I have a different idea that I'll explain below.
>
> >  {
> > -     if (imx_keep_uart_clocks) {
> > -             int i;
> > +     struct clk *uart_clk;
> > +     int i = 0;
> >
> > -             imx_uart_clocks = clks;
> > -             for (i = 0; imx_uart_clocks[i]; i++)
> > -                     clk_prepare_enable(*imx_uart_clocks[i]);
> > -     }
> > +     if (!imx_keep_uart_clocks || (!is_on && !imx_uart_clks_on))
> > +             return;
> > +
> > +     /* only support dt */
> > +     if (!of_stdout)
> > +             return;
> > +
> > +     do {
> > +             uart_clk = of_clk_get(of_stdout, i++);
>
> of_clk_get() allocates memory and gets you a reference to the clock. You
> have to release the clock with clk_put(). I think what you have to do
> here is to fill an array with clks when called from
> imx_register_uart_clocks() and when called from imx_clk_disable_uart()
> use that array to clk_disable_unprepare()/clk_put() the clocks.

I have another revision pending which modifies
imx_register_uart_clocks() to receive the number of available UART
clocks from the calling SoC. It will then allocate an array of clock
structures equal to that size.   Instead of enabling all UART clocks,
it will then go through of_clk_get(of_stdout, ...) to fill the array
and keep track of the number of devices it's assigned to the array.
Most likely the array will be larger than the number of of_stdout
entries.

Because it keeps track of the number of enabled UART's, it will use
that to go through the array and only try to unprepare/disable and put
that many clocks.  Once all the clocks have been disabled and put, the
entire clock array will be freed.

It will be more closely related to how the current imx/clk.c file is
now instead of using NXP's custom kernel, but it will also allow me to
remove the static arrays setting up the UART clocks for each SoC.

Does that sound OK to you?

I need to run some tests on my i.MX6Q board before I submit it, but
tests on my i.MX8MM are looking promising.  I can re-parent the UART
that I need reparented, and it fails if I try to reparent when that
UART is assigned to stdout.

adam
>
> Sascha
>
> > +             if (IS_ERR(uart_clk))
> > +                     break;
> > +
> > +             if (is_on)
> > +                     clk_prepare_enable(uart_clk);
> > +             else
> > +                     clk_disable_unprepare(uart_clk);
> > +     } while (true);
> > +
> > +     if (is_on)
> > +             imx_uart_clks_on = true;
> > +}
> > +void imx_register_uart_clocks(struct clk ** const clks[])
> > +{
> > +     imx_earlycon_uart_clks_onoff(true);
> >  }
> >
> >  static int __init imx_clk_disable_uart(void)
> >  {
> > -     if (imx_keep_uart_clocks && imx_uart_clocks) {
> > -             int i;
> > -
> > -             for (i = 0; imx_uart_clocks[i]; i++)
> > -                     clk_disable_unprepare(*imx_uart_clocks[i]);
> > -     }
> > +     imx_earlycon_uart_clks_onoff(false);
> >
> >       return 0;
> >  }
> > --
> > 2.25.1
> >
> >
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |