Message-ID: <CAMuHMdX0p4KhTNpNKMfZBjrCDxsyGO7sSwcJZWxBgxoHBZy9jQ@mail.gmail.com>
Date: Wed, 27 Nov 2024 10:53:46 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v2 1/2] clk: renesas: rzv2h-cpg: Add selective Runtime PM
 support for clocks

Hi Prabhakar,

Thanks for your patch!

s/rzv2h-cpg/rzv2h/

On Tue, Nov 5, 2024 at 12:24 AM Prabhakar <prabhakar.csengg@...il.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Update `rzv2h_cpg_attach_dev` to prevent external clocks not tied to the
> power domain from being managed by Runtime PM. This ensures that only
> clocks originating from the domain are controlled, thereby avoiding
> unintended handling of external clocks.
>
> Additionally, introduce a `no_pm` flag in `mod_clock` and `rzv2h_mod_clk`
> structures to exclude specific clocks from Runtime PM when needed. Some
> clocks, such as those in the CRU block, require unique enable/disable
> sequences that are incompatible with standard Runtime PM. For example,
> the CSI-2 D-PHY clock initialization requires toggling individual clocks,
> making Runtime PM unsuitable.
>
> The helper function `rzv2h_cpg_is_pm_clk()` checks whether a clock should
> be managed by Runtime PM based on this `no_pm` flag. New macros, such as
> `DEF_MOD_NO_PM`, allow straightforward declaration of clocks that bypass
> PM.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> ---
> v1->v2
> - Updated code to skip external clocks to be controlled from runtime PM
> - Updated id range check
> - Updated commit message

Thanks for the update!

> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c
> @@ -668,8 +671,38 @@ struct rzv2h_cpg_pd {
>         struct generic_pm_domain genpd;
>  };
>
> +static bool rzv2h_cpg_is_pm_clk(struct rzv2h_cpg_pd *pd,
> +                               const struct of_phandle_args *clkspec)
> +{
> +       if (clkspec->np != pd->genpd.dev.of_node || clkspec->args_count != 2)
> +               return false;
> +
> +       switch (clkspec->args[0]) {
> +       case CPG_MOD: {
> +               struct rzv2h_cpg_priv *priv = pd->priv;
> +               unsigned int id = clkspec->args[1];
> +               struct mod_clock *clock;
> +
> +               if (id >= priv->num_mod_clks)
> +                       return true;
> +
> +               if (priv->clks[priv->num_core_clks + id] == ERR_PTR(-ENOENT))
> +                       return true;

Shouldn't this return false for the two invalid cases above?

> +
> +               clock = to_mod_clock(__clk_get_hw(priv->clks[priv->num_core_clks + id]));
> +
> +               return !clock->no_pm;
> +       }
> +
> +       case CPG_CORE:
> +       default:
> +               return true;

False? (I know the code treated it is a PM clock before)

> +       }
> +}
> +
>  static int rzv2h_cpg_attach_dev(struct generic_pm_domain *domain, struct device *dev)
>  {
> +       struct rzv2h_cpg_pd *pd = container_of(domain, struct rzv2h_cpg_pd, genpd);
>         struct device_node *np = dev->of_node;
>         struct of_phandle_args clkspec;
>         bool once = true;
> @@ -679,6 +712,12 @@ static int rzv2h_cpg_attach_dev(struct generic_pm_domain *domain, struct device
>
>         while (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", i,
>                                            &clkspec)) {
> +               if (!rzv2h_cpg_is_pm_clk(pd, &clkspec)) {
> +                       of_node_put(clkspec.np);
> +                       i++;
> +                       continue;

This loop may start to loop nicer using
"for (i = 0; !of_parse_phandle_with_args(...); i++)".

> +               }
> +
>                 if (once) {
>                         once = false;
>                         error = pm_clk_create(dev);
> diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
> index 819029c81904..0723df4c1134 100644
> --- a/drivers/clk/renesas/rzv2h-cpg.h
> +++ b/drivers/clk/renesas/rzv2h-cpg.h
> @@ -100,6 +100,7 @@ enum clk_types {
>   * @name: handle between common and hardware-specific interfaces
>   * @parent: id of parent clock
>   * @critical: flag to indicate the clock is critical
> + * @no_pm: flag to indicate PM is not supported
>   * @on_index: control register index
>   * @on_bit: ON bit
>   * @mon_index: monitor register index
> @@ -109,17 +110,19 @@ struct rzv2h_mod_clk {
>         const char *name;
>         u16 parent;
>         bool critical;
> +       bool no_pm;
>         u8 on_index;
>         u8 on_bit;
>         s8 mon_index;
>         u8 mon_bit;
>  };
>
> -#define DEF_MOD_BASE(_name, _parent, _critical, _onindex, _onbit, _monindex, _monbit) \
> +#define DEF_MOD_BASE(_name, _parent, _critical, _no_pm, _onindex, _onbit, _monindex, _monbit) \

Note that this series conflicts with "[PATCH 00/12] Add support for
Renesas RZ/G3E SoC and SMARC-EVK platform", which you are probably
already aware of.

[1] https://lore.kernel.org/all/20241122124558.149827-1-biju.das.jz@bp.renesas.com/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Powered by blists - more mailing lists