From: Claudiu Beznea <[email protected]>
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 <[email protected]>
---
Changes in v2:
- removed DIV_NOTIF macro
drivers/clk/renesas/rzg2l-cpg.c | 207 ++++++++++++++++++++++++++++++++
drivers/clk/renesas/rzg2l-cpg.h | 9 ++
2 files changed, 216 insertions(+)
diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index f4b70e07b9c6..3080e9391f71 100644
--- 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;
+
+ 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);
+ /* 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");
+
+ return ret;
+}
+
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;
+
+ return round_rate;
+}
+
+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;
+
+ 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;
+
+ 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 = 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;
+
+ 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;
+}
+
static struct clk * __init
rzg2l_cpg_div_clk_register(const struct cpg_core_clk *core,
struct clk **clks,
@@ -964,6 +1168,9 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
clk = rzg2l_cpg_div_clk_register(core, priv->clks,
priv->base, priv);
break;
+ case CLK_TYPE_G3S_DIV:
+ clk = rzg3s_cpg_div_clk_register(core, priv->clks, priv->base, priv);
+ break;
case CLK_TYPE_MUX:
clk = rzg2l_cpg_mux_clk_register(core, priv->base, priv);
break;
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index edac34037cf0..8312972cc496 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -89,6 +89,7 @@ struct cpg_core_clk {
unsigned int sconf;
const struct clk_div_table *dtable;
const u32 *mtable;
+ const unsigned long invalid_rate;
const char * const *parent_names;
notifier_fn_t notifier;
u32 flag;
@@ -105,6 +106,7 @@ enum clk_types {
/* Clock with divider */
CLK_TYPE_DIV,
+ CLK_TYPE_G3S_DIV,
/* Clock with clock source selector */
CLK_TYPE_MUX,
@@ -143,6 +145,12 @@ enum clk_types {
DEF_TYPE(_name, _id, CLK_TYPE_DIV, .conf = _conf, \
.parent = _parent, .dtable = _dtable, \
.flag = CLK_DIVIDER_READ_ONLY)
+#define DEF_G3S_DIV(_name, _id, _parent, _conf, _sconf, _dtable, _invalid_rate, \
+ _clk_flags, _notif) \
+ DEF_TYPE(_name, _id, CLK_TYPE_G3S_DIV, .conf = _conf, .sconf = _sconf, \
+ .parent = _parent, .dtable = _dtable, \
+ .invalid_rate = _invalid_rate, .flag = (_clk_flags), \
+ .notifier = _notif)
#define DEF_MUX(_name, _id, _conf, _parent_names) \
DEF_TYPE(_name, _id, CLK_TYPE_MUX, .conf = _conf, \
.parent_names = _parent_names, \
@@ -277,5 +285,6 @@ extern const struct rzg2l_cpg_info r9a07g054_cpg_info;
extern const struct rzg2l_cpg_info r9a09g011_cpg_info;
int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event, void *data);
+int rzg3s_cpg_div_clk_notifier(struct notifier_block *nb, unsigned long event, void *data);
#endif
--
2.39.2
Hi Claudiu,
On Fri, Sep 29, 2023 at 7:39 AM Claudiu <[email protected]> wrote:
> From: Claudiu Beznea <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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 -- [email protected]
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
Hi, Geert,
On 04.10.2023 15:30, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Fri, Sep 29, 2023 at 7:39 AM Claudiu <[email protected]> wrote:
>> From: Claudiu Beznea <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>>
>> 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
Sure!
>
>> +
>> + 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.
Indeed, I messed this up. Though, I've tested quite some use cases and they
all worked... I'll review this anyway, thanks for pointing it up.
>
> 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?
Yes, the idea was to set a safe divider.
>
>> + /* 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
Sure!
>
>> +
>> + return ret;
>
> NOTIFY_OK
Sure!
>
>> +}
>> +
>> 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?
The divider_round_rate() already choose it as the closest rate that it is
actually not supported, thus I chose to just return -EINVAL. I chose it
this way to use divider_round_rate(). Don't know if there is way around
this using divider_round_rate() I'll have a look.
>
>> +
>> + return round_rate;
>> +}
>
> But please implement .determine_rate() instead of .round_rate() in
> new drivers.
Indeed, I missed this one.
>
>> +
>> +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?
I remember I added it here as a result of testing. I'll double check it.
>
>> +
>> + 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, ...);
OK.
>
>> +
>> + 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
Yes, you're right.
Thank you for your review,
Claudiu Beznea
>
>> + 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
>
Hi Claudiu,
On Wed, Oct 4, 2023 at 2:30 PM Geert Uytterhoeven <[email protected]> wrote:
> On Fri, Sep 29, 2023 at 7:39 AM Claudiu <[email protected]> wrote:
> > From: Claudiu Beznea <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> >
> > Changes in v2:
> > - removed DIV_NOTIF macro
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > +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
My apologies for not finishing my sentence; I wanted to write "Isn't
that identical to __fls(max)?". But as the latter generates slightly
worse code, it's not worth making that change.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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