Re: [PATCH v2 11/28] clk: renesas: rzg2l: add a divider clock for RZ/G3S

Hi Claudiu,

On Fri, Sep 29, 2023 at 7:39 AM Claudiu [off-list ref] wrote:

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Add a divider clock driver for RZ/G3S. This will be used in RZ/G3S
by SDHI, SPI, OCTA, I, I2, I3, P0, P1, P2, P3 core clocks.
The divider has some limitation for SDHI and OCTA clocks:
- SD div cannot be 1 if parent rate is 800MHz
- OCTA div cannot be 1 if parent rate is 400MHz
For these clocks a notifier could be registered from platform specific
clock driver and proper actions are taken before clock rate is changed,
if needed.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- removed DIV_NOTIF macro

Thanks for the update!

NOTIFY_DONE for event != PRE_RATE_CHANGE
NOTIFY_OK for the other cases

+
+       spin_lock_irqsave(&priv->rmw_lock, flags);
+
+       val = readl(priv->base + off);
+       val >>= shift;
+       val &= bitmask;
+
+       /*
+        * There are different constraints for the user of this notifiers as follows:
+        * 1/ SD div cannot be 1 (val == 0) if parent rate is 800MHz
+        * 2/ OCTA div cannot be 1 (val == 0) if parent rate is 400MHz
+        * As SD can have only one parent having 800MHz and OCTA div can have
+        * only one parent having 400MHz we took into account the parent rate
+        * at the beginning of function (by checking invalid_rate % new_rate).
+        * Now it is time to check the hardware divider and update it accordingly.
+        */
+       if (!val) {
+               writel(((bitmask << shift) << 16) | BIT(shift), priv->base + off);

Haven't you exchanged the (single) write-enable bit and the (multi-bit)
division ratio setting?  According to the docs, the write-enable bit
is at 16 + shift, while the division ratio is at shift.

Also, using bitmask as the division ratio means the maximum value
that fits in the bitfield, which would be a prohibited setting in case
of DIV_OCTA.

Now, looking at rzg3s_div_clk_set_rate() below, perhaps you just wanted
to set the ratio to value to 1, but used the wrong size for bitmask?

+               /* Wait for the update done. */
+               ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
+       }
+
+       spin_unlock_irqrestore(&priv->rmw_lock, flags);
+
+       if (ret)
+               dev_err(priv->dev, "Failed to downgrade the div\n");

and return NOTIFY_BAD

+
+       return ret;

NOTIFY_OK

Shouldn't this return the closest rate that is actually supported instead?

+
+       return round_rate;
+}

But please implement .determine_rate() instead of .round_rate() in
new drivers.

+
+static int rzg3s_div_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+                                 unsigned long parent_rate)
+{
+       struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
+       struct div_hw_data *div_hw_data = to_div_hw_data(clk_hw_data);
+       struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
+       u32 off = GET_REG_OFFSET(clk_hw_data->conf);
+       u32 shift = GET_SHIFT(clk_hw_data->conf);
+       unsigned long flags;
+       u32 bitmask, val;
+       int ret;
+
+       /*
+        * Some dividers cannot support some rates:
+        * - SD div cannot support 800 MHz when parent is @800MHz and div = 1
+        * - OCTA div cannot support 400 MHz when parent is @400MHz and div = 1
+        * Check these scenarios.
+        */
+       if (!rzg3s_div_clk_is_rate_valid(div_hw_data->invalid_rate, rate))
+               return -EINVAL;

Can this actually happen? Wouldn't the notifier have prevented us from
getting here?

+
+       val = divider_get_val(rate, parent_rate, div_hw_data->dtable, div_hw_data->width,
+                             CLK_DIVIDER_ROUND_CLOSEST);
+
+       bitmask = (GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0) << shift) << 16;

Is bitmask the (single) write-enable bit?

If yes, that should be BIT(16 + shift), and the variable should be
renamed to reflect that.

I guess there should be a general "#define CPG_WEN BIT(16)", then you
can simply use

    writel((CPG_WEN | val) << shift, ...);

+
+       spin_lock_irqsave(&priv->rmw_lock, flags);
+       writel(bitmask | (val << shift), priv->base + off);
+       /* Wait for the update done. */
+       ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
+       spin_unlock_irqrestore(&priv->rmw_lock, flags);
+
+       return ret;
+}
+
+static const struct clk_ops rzg3s_div_clk_ops = {
+       .recalc_rate = rzg3s_div_clk_recalc_rate,
+       .round_rate = rzg3s_div_clk_round_rate,
+       .set_rate = rzg3s_div_clk_set_rate,
+};
+
+static struct clk * __init
+rzg3s_cpg_div_clk_register(const struct cpg_core_clk *core, struct clk **clks,
+                          void __iomem *base, struct rzg2l_cpg_priv *priv)
+{
+       struct div_hw_data *div_hw_data;
+       struct clk_init_data init = {};
+       const struct clk_div_table *clkt;
+       struct clk_hw *clk_hw;
+       const struct clk *parent;
+       const char *parent_name;
+       u32 max;
+       int ret;
+
+       parent = clks[core->parent & 0xffff];
+       if (IS_ERR(parent))
+               return ERR_CAST(parent);
+
+       parent_name = __clk_get_name(parent);
+
+       div_hw_data = devm_kzalloc(priv->dev, sizeof(*div_hw_data), GFP_KERNEL);
+       if (!div_hw_data)
+               return ERR_PTR(-ENOMEM);
+
+       init.name = core->name;
+       init.flags = core->flag;
+       init.ops = &rzg3s_div_clk_ops;
+       init.parent_names = &parent_name;
+       init.num_parents = 1;
+
+       /* Get the maximum divider to retrieve div width. */
+       for (clkt = core->dtable; clkt->div; clkt++) {
+               if (max < clkt->div)

"max" is used uninitialized

+                       max = clkt->div;
+       }
+
+       div_hw_data->hw_data.priv = priv;
+       div_hw_data->hw_data.conf = core->conf;
+       div_hw_data->hw_data.sconf = core->sconf;
+       div_hw_data->dtable = core->dtable;
+       div_hw_data->invalid_rate = core->invalid_rate;
+       div_hw_data->width = fls(max) - 1;

Isn't that

+
+       clk_hw = &div_hw_data->hw_data.hw;
+       clk_hw->init = &init;
+
+       ret = devm_clk_hw_register(priv->dev, clk_hw);
+       if (ret)
+               return ERR_PTR(ret);
+
+       ret = rzg2l_register_notifier(clk_hw, core, priv);
+       if (ret) {
+               dev_err(priv->dev, "Failed to register notifier for %s\n",
+                       core->name);
+               return ERR_PTR(ret);
+       }
+
+       return clk_hw->clk;
+}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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