| Message ID | 20241028212914.1057715-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: renesas: rzv2h-cpg: Add CRU and CSI clocks | expand |
Hi Prabhakar, On Mon, Oct 28, 2024 at 10:29 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > 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@bp.renesas.com> 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
Hi Geert, Thank you for the review. On Wed, Oct 30, 2024 at 4:56 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Mon, Oct 28, 2024 at 10:29 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > 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@bp.renesas.com> > > 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. > I will add this check for g2l cpg too now. > > + 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. > Agreed, I will drop it. > > + 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; > Agreed. > > +} > > + > > /** > > * 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. > Agreed, I had missed this case, thanks. Cheers, Prabhakar
diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c index b524a9d33610..ed45dbc1cc40 100644 --- a/drivers/clk/renesas/rzv2h-cpg.c +++ b/drivers/clk/renesas/rzv2h-cpg.c @@ -98,6 +98,7 @@ struct pll_clk { * * @priv: CPG private data * @hw: handle between common and hardware-specific interfaces + * @no_pm: flag to indicate PM is not supported * @on_index: register offset * @on_bit: ON/MON bit * @mon_index: monitor register offset @@ -106,6 +107,7 @@ struct pll_clk { struct mod_clock { struct rzv2h_cpg_priv *priv; struct clk_hw hw; + bool no_pm; u8 on_index; u8 on_bit; s8 mon_index; @@ -541,6 +543,7 @@ rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod, clock->on_bit = mod->on_bit; clock->mon_index = mod->mon_index; clock->mon_bit = mod->mon_bit; + clock->no_pm = mod->no_pm; clock->priv = priv; clock->hw.init = &init; @@ -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; + + 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) + 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; +} + /** * 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)) { + if (!rzv2h_cpg_is_pm_clk(priv, &clkspec)) { + of_node_put(clkspec.np); + i++; + continue; + } + 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) \ { \ .name = (_name), \ .parent = (_parent), \ .critical = (_critical), \ + .no_pm = (_no_pm), \ .on_index = (_onindex), \ .on_bit = (_onbit), \ .mon_index = (_monindex), \ @@ -127,10 +130,13 @@ struct rzv2h_mod_clk { } #define DEF_MOD(_name, _parent, _onindex, _onbit, _monindex, _monbit) \ - DEF_MOD_BASE(_name, _parent, false, _onindex, _onbit, _monindex, _monbit) + DEF_MOD_BASE(_name, _parent, false, false, _onindex, _onbit, _monindex, _monbit) #define DEF_MOD_CRITICAL(_name, _parent, _onindex, _onbit, _monindex, _monbit) \ - DEF_MOD_BASE(_name, _parent, true, _onindex, _onbit, _monindex, _monbit) + DEF_MOD_BASE(_name, _parent, true, false, _onindex, _onbit, _monindex, _monbit) + +#define DEF_MOD_NO_PM(_name, _parent, _onindex, _onbit, _monindex, _monbit) \ + DEF_MOD_BASE(_name, _parent, false, true, _onindex, _onbit, _monindex, _monbit) /** * struct rzv2h_reset - Reset definitions