Message-ID: <CAMuHMdX8wPNr4LsPJR7zk3Ktb8NnqcSMUEjsAURc2NJ_dpiDHw@mail.gmail.com>
Date:   Wed, 4 Oct 2023 14:30:30 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Claudiu <claudiu.beznea@...on.dev>
Cc:     mturquette@...libre.com, sboyd@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        linus.walleij@...aro.org, gregkh@...uxfoundation.org,
        jirislaby@...nel.org, magnus.damm@...il.com,
        catalin.marinas@....com, will@...nel.org,
        quic_bjorande@...cinc.com, konrad.dybcio@...aro.org, arnd@...db.de,
        neil.armstrong@...aro.org, prabhakar.mahadev-lad.rj@...renesas.com,
        biju.das.jz@...renesas.com, linux-renesas-soc@...r.kernel.org,
        linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-serial@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: 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 <claudiu.beznea@...on.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...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@...renesas.com>
> ---
>
> Changes in v2:
> - removed DIV_NOTIF macro

Thanks for the update!

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -91,6 +91,22 @@ struct sd_mux_hw_data {
>
>  #define to_sd_mux_hw_data(_hw) container_of(_hw, struct sd_mux_hw_data, hw_data)
>
> +/**
> + * struct div_hw_data - divider clock hardware data
> + * @hw_data: clock hw data
> + * @dtable: pointer to divider table
> + * @invalid_rate: invalid rate for divider
> + * @width: divider width
> + */
> +struct div_hw_data {
> +       struct clk_hw_data hw_data;
> +       const struct clk_div_table *dtable;
> +       unsigned long invalid_rate;
> +       u32 width;
> +};
> +
> +#define to_div_hw_data(_hw)    container_of(_hw, struct div_hw_data, hw_data)
> +
>  struct rzg2l_pll5_param {
>         u32 pl5_fracin;
>         u8 pl5_refdiv;
> @@ -200,6 +216,54 @@ int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event
>         return ret;
>  }
>
> +int rzg3s_cpg_div_clk_notifier(struct notifier_block *nb, unsigned long event,
> +                              void *data)
> +{
> +       struct clk_notifier_data *cnd = data;
> +       struct clk_hw *hw = __clk_get_hw(cnd->clk);
> +       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);
> +       u32 bitmask = GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0);
> +       unsigned long flags;
> +       int ret = 0;
> +       u32 val;
> +
> +       if (event != PRE_RATE_CHANGE || !div_hw_data->invalid_rate ||
> +           div_hw_data->invalid_rate % cnd->new_rate)
> +               return 0;

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

> +}
> +
>  static int rzg2l_register_notifier(struct clk_hw *hw, const struct cpg_core_clk *core,
>                                    struct rzg2l_cpg_priv *priv)
>  {
> @@ -217,6 +281,146 @@ static int rzg2l_register_notifier(struct clk_hw *hw, const struct cpg_core_clk
>         return clk_notifier_register(hw->clk, nb);
>  }
>
> +static unsigned long rzg3s_div_clk_recalc_rate(struct clk_hw *hw,
> +                                              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 val;
> +
> +       val = readl(priv->base + GET_REG_OFFSET(clk_hw_data->conf));
> +       val >>= GET_SHIFT(clk_hw_data->conf);
> +       val &= GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0);
> +
> +       return divider_recalc_rate(hw, parent_rate, val, div_hw_data->dtable,
> +                                  CLK_DIVIDER_ROUND_CLOSEST, div_hw_data->width);
> +}
> +
> +static bool rzg3s_div_clk_is_rate_valid(const unsigned long invalid_rate, unsigned long rate)
> +{
> +       if (invalid_rate && rate >= invalid_rate)
> +               return false;
> +
> +       return true;
> +}
> +
> +static long rzg3s_div_clk_round_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);
> +       long round_rate;
> +
> +       round_rate = divider_round_rate(hw, rate, parent_rate, div_hw_data->dtable,
> +                                       div_hw_data->width, CLK_DIVIDER_ROUND_CLOSEST);
> +
> +       if (!rzg3s_div_clk_is_rate_valid(div_hw_data->invalid_rate, round_rate))
> +               return -EINVAL;

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@...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