Hi Claudiu, On Fri, Sep 29, 2023 at 7:39 AM Claudiu <claudiu.beznea@xxxxxxxxx> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > --- > > 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@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