Re: [PATCH V3] clk: imx: Fix reparenting of UARTs not associated with sdout

[Adam Ford]

On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <abel.vesa@xxxxxxx> wrote:
>
> On 21-01-15 12:29:08, Adam Ford wrote:
>
> ...
>
> > diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
> > index a66cabfbf94f..66192fe0a898 100644
> > --- a/drivers/clk/imx/clk-imx25.c
> > +++ b/drivers/clk/imx/clk-imx25.c
> > @@ -73,16 +73,6 @@ enum mx25_clks {
> >
> >  static struct clk *clk[clk_max];
> >
> > -static struct clk ** const uart_clks[] __initconst = {
> > -     &clk[uart_ipg_per],
> > -     &clk[uart1_ipg],
> > -     &clk[uart2_ipg],
> > -     &clk[uart3_ipg],
> > -     &clk[uart4_ipg],
> > -     &clk[uart5_ipg],
> > -     NULL
> > -};
> > -
>
> I'm assuming there is another patch that updates the dts files. Right ?

I have only been able to test this on an i.MX8M Mini.  I need to set
the parent clock of the i.MX8M Mini to an 80 MHz clock in order to run
the UART at 4Mbps.   With this patch, I can stop enabling the all the
UART clocks early and allow the clock parent configuration to occur.
>From what I can tell, the remaining clocks should get activated as
they are needed, because I was able to use Bluetooth connected to
UART1 running at 4MBps using a 80MHz clock source with this patch, and
the clk_summary shows the clock is running at the proper speed.
Without this patch, the UART fails to re-parent, so I'm stuck at lower
speeds and that means choppy Bluetooth audio.

The Kernel that NXP hosts on Code Aurora that they use for Yocto
attempts scan through stdout to only enable those clocks [1].  I
attempted to push it upstream, but it was rejected [2].  Sascha
suggested creating an array which could be filled when the clocks are
enabled and that array would be used to deactivate the clocks at
shutdown.  That's what I attempted to do here.

I don't have older imx boards to know if their device trees are
configured in such a way without modifications to the device tree or
not, but since it appears to work for NXP in [2], I assumed it would
work here.

[1] - https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/clk/imx/clk.c?h=imx_5.4.47_2.2.0&id=754ae82cc55b7445545fc2f092a70e0f490e9c1b
[2] - https://patchwork.kernel.org/project/linux-arm-kernel/patch/20201229145130.2680442-1-aford173@xxxxxxxxx/

>
> TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> where they belong.

That makes sense.

>
> >  static int __init __mx25_clocks_init(void __iomem *ccm_base)
> >  {
> >       BUG_ON(!ccm_base);
> > @@ -228,7 +218,7 @@ static int __init __mx25_clocks_init(void __iomem *ccm_base)
> >        */
> >       clk_set_parent(clk[cko_sel], clk[ipg]);
> >
> > -     imx_register_uart_clocks(uart_clks);
> > +     imx_register_uart_clocks(6);
>
> Suggestion: Maybe the number of clocks can be determined by the existing clocks in that dts node.
> Hardcoding is not a good ideea here.

The tables were hard-coded before, so the idea was to pass the maximum
number of clocks instead the entire table.  The higher-level clock
code wouldn't necessarily know the maximum number of UART clocks since
the number of UARTs may change depending on the SoC.  So that
hard-coded number was simply the number of entries that were
previously used in the array that was previously passed.  When
creating a table of active clocks, it could use the number passed to
define an array, and fill the array with data grabbed from of_stdout.

If you want, I could leave the existing UART clocks alone, and create
a new function that uses the array of clocks passed to it to count the
number of available clocks.  It would limit the scope of the change to
clk/imx/clk.c.  I think that would be easier than trying to parse the
DT for a bunch of compatible flags looking for a bunch of UARTS and
their respective clocks.

If you'd rather do something in the serial imx driver, I can hold off.

adam
>
> ...
>
> >
> > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> > index 47882c51cb85..158fe302a8b7 100644
> > --- a/drivers/clk/imx/clk.c
> > +++ b/drivers/clk/imx/clk.c
> > @@ -147,8 +147,10 @@ void imx_cscmr1_fixup(u32 *val)
> >  }
> >
> >  #ifndef MODULE
> > -static int imx_keep_uart_clocks;
> > -static struct clk ** const *imx_uart_clocks;
> > +
> > +static bool imx_keep_uart_clocks;
> > +static int imx_enabled_uart_clocks;
> > +static struct clk **imx_uart_clocks;
> >
> >  static int __init imx_keep_uart_clocks_param(char *str)
> >  {
> > @@ -161,24 +163,43 @@ __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[])
> > +void imx_register_uart_clocks(unsigned int clk_count)
> >  {
> > +#ifdef CONFIG_OF
> >       if (imx_keep_uart_clocks) {
> >               int i;
> >
> > -             imx_uart_clocks = clks;
> > -             for (i = 0; imx_uart_clocks[i]; i++)
> > -                     clk_prepare_enable(*imx_uart_clocks[i]);
> > +             imx_uart_clocks = kcalloc(clk_count, sizeof(struct clk *), GFP_KERNEL);
> > +             imx_enabled_uart_clocks = 0;
> > +
> > +             for (i = 0; i < clk_count; i++) {
> > +                     imx_uart_clocks[imx_enabled_uart_clocks] = of_clk_get(of_stdout, i);
> > +
> > +                     /* Stop if there are no more of_stdout references */
> > +                     if (IS_ERR(imx_uart_clocks[imx_enabled_uart_clocks]))
> > +                             return;
> > +
> > +                     /* Only enable the clock if it's not NULL */
> > +                     if (imx_uart_clocks[imx_enabled_uart_clocks])
> > +                             clk_prepare_enable(imx_uart_clocks[imx_enabled_uart_clocks++]);
> > +             }
> >       }
> > +#else
> > +     /* i.MX boards use device trees now.  For build tests without CONFIG_OF, do nothing */
> > +     imx_enabled_uart_clocks = 0;
> > +#endif
>
> Don't really see the point of this #ifdef here. Just makes the code more messy.
>

I added the #ifdef here because I got an e-mail from a bot from a
previous attempt.  The bot failed to build, because the build test
using a different defconfig file that doesn't enable CONFIG_OF.  For
the purposes of using this code, the CONFIG_OF is required, but
without changes to Kconfig, this seemed like an easy way to prevent
build errors for the bot.  I didn't want to break something else by
changing the Kconfig dependencies because I wasn't sure how many build
bots exist.

adam
> >  }
> >
> >  static int __init imx_clk_disable_uart(void)
> >  {
> > -     if (imx_keep_uart_clocks && imx_uart_clocks) {
> > +     if (imx_keep_uart_clocks && imx_enabled_uart_clocks) {
> >               int i;
> >
> > -             for (i = 0; imx_uart_clocks[i]; i++)
> > -                     clk_disable_unprepare(*imx_uart_clocks[i]);
> > +             for (i = 0; i < imx_enabled_uart_clocks; i++) {
> > +                     clk_disable_unprepare(imx_uart_clocks[i]);
> > +                     clk_put(imx_uart_clocks[i]);
> > +             };
> > +             kfree(imx_uart_clocks);
> >       }
> >
> >       return 0;
> > diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> > index 4f04c8287286..7571603bee23 100644
> > --- a/drivers/clk/imx/clk.h
> > +++ b/drivers/clk/imx/clk.h
> > @@ -11,9 +11,9 @@ extern spinlock_t imx_ccm_lock;
> >  void imx_check_clocks(struct clk *clks[], unsigned int count);
> >  void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
> >  #ifndef MODULE
> > -void imx_register_uart_clocks(struct clk ** const clks[]);
> > +void imx_register_uart_clocks(unsigned int clk_count);
> >  #else
> > -static inline void imx_register_uart_clocks(struct clk ** const clks[])
> > +static inline void imx_register_uart_clocks(unsigned int clk_count)
> >  {
> >  }
> >  #endif
> > --
> > 2.25.1
> >