Hi Biju,
On Sun, Aug 15, 2021 at 12:30 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> The AXI and CHI clocks use the same register bit for controlling clock
> output. Add a new clock type for coupled clocks, which sets the
> CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
> clears the bit only when both clocks are disabled.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
> v2->v3:
> * Reworked as per Geert's suggestion
> * Added enabled flag to track the status of clock, if it is coupled
> with another clock
> * Introduced siblings pointer which points to the other coupled
> clock
> * coupled clock linking is done during module clk register.
> * rzg2l_mod_clock_is_enabled function returns soft state of the
> module clocks, if it is coupled with another clock
> * Updated the commit header
Thanks for the update!
> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -392,11 +396,35 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
>
> static int rzg2l_mod_clock_enable(struct clk_hw *hw)
> {
> + struct mstp_clock *clock = to_mod_clock(hw);
> + struct mstp_clock *siblings = clock->siblings;
> +
> + if (siblings) {
> + if (siblings->enabled) {
> + clock->enabled = true;
> + return 0;
> + }
> +
> + clock->enabled = true;
clock->enabled is set to true regardless of the if condition.
This also needs locking, in case both clocks are changed concurrently:
spin_lock_irqsave(&priv->rmw_lock, flags);
enabled = siblings->enabled;
clock->enabled = true;
spin_unlock_irqrestore(&priv->rmw_lock, flags);
if (enabled)
return 0;
> + }
> +
> return rzg2l_mod_clock_endisable(hw, true);
> }
>
> static void rzg2l_mod_clock_disable(struct clk_hw *hw)
> {
> + struct mstp_clock *clock = to_mod_clock(hw);
> + struct mstp_clock *siblings = clock->siblings;
> +
> + if (siblings) {
> + if (siblings->enabled) {
> + clock->enabled = false;
> + return;
> + }
> +
> + clock->enabled = false;
Likewise.
> + }
> +
> rzg2l_mod_clock_endisable(hw, false);
> }
>
> @@ -412,6 +440,9 @@ static int rzg2l_mod_clock_is_enabled(struct clk_hw *hw)
> return 1;
> }
>
> + if (clock->siblings)
> + return clock->enabled;
> +
> value = readl(priv->base + CLK_MON_R(clock->off));
>
> return !(value & bitmask);
> @@ -423,11 +454,33 @@ static const struct clk_ops rzg2l_mod_clock_ops = {
> .is_enabled = rzg2l_mod_clock_is_enabled,
> };
>
> +static struct mstp_clock
> +*rzg2l_cpg_get_sibling_clk(struct mstp_clock *clock,
> + struct rzg2l_cpg_priv *priv)
> +{
> + struct mstp_clock *sibl_clk = NULL;
clk?
It's only a sibling when the condition inside the loop is true.
> + struct clk_hw *hw;
> + unsigned int i;
> +
> + for (i = 0; i < priv->num_mod_clks; i++) {
> + if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
> + continue;
> +
> + hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> + sibl_clk = to_mod_clock(hw);
> + if (clock->off == sibl_clk->off && clock->bit == sibl_clk->bit)
> + break;
> + }
> +
> + return sibl_clk;
> +}
> +
> static void __init
> rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
> const struct rzg2l_cpg_info *info,
> struct rzg2l_cpg_priv *priv)
> {
> + struct mstp_clock *sibling_clock = NULL;
> struct mstp_clock *clock = NULL;
> struct device *dev = priv->dev;
> unsigned int id = mod->id;
> @@ -484,6 +537,15 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
>
> dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk, clk_get_rate(clk));
> priv->clks[id] = clk;
> +
> + if (mod->is_coupled) {
> + sibling_clock = rzg2l_cpg_get_sibling_clk(clock, priv);
> + if (sibling_clock) {
> + clock->siblings = sibling_clock;
> + sibling_clock->siblings = clock;
> + }
You forgot to initialize mstp_clock.enabled to match the current
hardware state.
> + }
> +
> return;
>
> fail:
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
