Re: [PATCH 1/2] clk: renesas: rzv2h-cpg: Add support for clocks without PM

[Geert Uytterhoeven]

Hi Prabhakar,

On Mon, Oct 28, 2024 at 10:29 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Allow certain CPG_MOD clocks to be excluded from Runtime PM support. Some
> clocks, like those in the CRU block, require specific sequences for
> enabling/disabling, making them incompatible with standard Runtime PM
> handling. For instance, initializing the CSI-2 D-PHY involves toggling
> individual clocks, which prevents the use of Runtime PM.
>
> Introduce a `no_pm` flag in the `mod_clock` and `rzv2h_mod_clk` structures
> to indicate clocks that do not support PM. Add a helper function
> `rzv2h_cpg_is_pm_clk()` to determine if a clock should be managed by
> Runtime PM based on this flag. Define new macros like `DEF_MOD_NO_PM` for
> easier specification of such clocks.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c
> @@ -658,6 +661,32 @@ static int rzv2h_cpg_reset_controller_register(struct rzv2h_cpg_priv *priv)
>         return devm_reset_controller_register(priv->dev, &priv->rcdev);
>  }
>
> +static bool rzv2h_cpg_is_pm_clk(struct rzv2h_cpg_priv *priv,
> +                               const struct of_phandle_args *clkspec)
> +{
> +       struct mod_clock *clock;
> +       struct clk_hw *hw;
> +       unsigned int id;
> +

Forgot to check clkspec->np?
Ah, rzg2l_cpg_is_pm_clk() also lacks this.

> +       if (clkspec->args_count != 2)
> +               return true;
> +
> +       id = clkspec->args[1];
> +       if (clkspec->args[0] != CPG_MOD ||
> +           id >= priv->num_core_clks + priv->num_mod_clks)

Adding "priv->num_core_clks" looks wrong to me.

> +               return true;
> +
> +       if (priv->clks[priv->num_core_clks + id] == ERR_PTR(-ENOENT))
> +               return true;
> +
> +       hw = __clk_get_hw(priv->clks[priv->num_core_clks + id]);
> +       clock = to_mod_clock(hw);
> +       if (clock->no_pm)
> +               return false;
> +
> +       return true;

return !clock->no_pm;

> +}
> +
>  /**
>   * struct rzv2h_cpg_pd - RZ/V2H power domain data structure
>   * @priv: pointer to CPG private data structure
> @@ -670,6 +699,8 @@ struct rzv2h_cpg_pd {
>
>  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 rzv2h_cpg_priv *priv = pd->priv;
>         struct device_node *np = dev->of_node;
>         struct of_phandle_args clkspec;
>         bool once = true;
> @@ -679,6 +710,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)) {

So this should have checked that clkspec.np actually points to our
clock provider.  Else all clocks (e.g. the external CAN clock, or
external audio clocks) are controlled through Runtime PM.

> +               if (!rzv2h_cpg_is_pm_clk(priv, &clkspec)) {
> +                       of_node_put(clkspec.np);
> +                       i++;
> +                       continue;
> +               }
> +
>                 if (once) {
>                         once = false;
>                         error = pm_clk_create(dev);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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