Hi Biju,
On Fri, Mar 18, 2022 at 6:51 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> M3 clock is sourced from DSI Divider (DSIDIVA * DSIDIVB)
>
> This patch add support for DSI divider clk by combaining
> DSIDIVA and DSIDIVB .
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> RFC->V1
> * Removed LUT and added an equation for computing VCLK.
Thanks for the update!
> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -279,6 +282,114 @@ rzg2l_cpg_sd_mux_clk_register(const struct cpg_core_clk *core,
> return clk_hw->clk;
> }
>
> +struct dsi_div_hw_data {
> + struct clk_hw hw;
> + u32 conf;
> + unsigned long rate;
> + struct rzg2l_cpg_priv *priv;
> +};
> +
> +#define to_dsi_div_hw_data(_hw) container_of(_hw, struct dsi_div_hw_data, hw)
> +
> +static unsigned long rzg2l_cpg_dsi_div_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct dsi_div_hw_data *dsi_div = to_dsi_div_hw_data(hw);
> +
> + return dsi_div->rate;
> +}
> +
> +static long rzg2l_cpg_dsi_div_round_rate(struct clk_hw *hw,
> + unsigned long rate,
> + unsigned long *parent_rate)
Please implement the determine_rate() instead.
> +
Please drop the blank line.
> +{
> + struct dsi_div_hw_data *dsi_div = to_dsi_div_hw_data(hw);
> + struct rzg2l_cpg_priv *priv = dsi_div->priv;
> +
> + dsi_div->rate = rate;
> +
> + priv->pll5_params.pl5_intin = rate / MEGA;
.round_rate() (and .determine_rate()) is used to check if a rate
is supported, without actually changing the clock rate. Hence this
should not operate on priv->pll5_params, but on a local variable.
> + priv->pll5_params.pl5_fracin = ((rate % MEGA) << 24) / MEGA;
While this works fine on 64-bit (RZ/G2L is arm64, so that's OK),
"(rate % MEGA) << 24" will overflow when compile-testing on 32-bit.
Taking into account the 64-by-32 division, I think this should be:
div_u64(((u64)rate % MEGA) << 24, MEGA);
> + priv->pll5_params.pl5_refdiv = 2;
> + priv->pll5_params.pl5_postdiv1 = 1;
> + priv->pll5_params.pl5_postdiv2 = 1;
> + priv->pll5_params.clksrc = 1;
> + priv->pll5_params.dsi_div_a = 1;
> + priv->pll5_params.dsi_div_b = 2;
> +
> + priv->pll5_params.frequency =
> + EXTAL_FREQ_IN_MEGA_HZ * MEGA / priv->pll5_params.pl5_refdiv *
> + ((((priv->pll5_params.pl5_intin << 24) + priv->pll5_params.pl5_fracin)) >> 24) /
> + (priv->pll5_params.pl5_postdiv1 * priv->pll5_params.pl5_postdiv2);
> +
> + if (priv->pll5_params.clksrc)
> + priv->pll5_params.frequency /= 2;
> +
> + *parent_rate = priv->pll5_params.frequency;
> +
> + return dsi_div->rate;
> +}
> +
> +static int rzg2l_cpg_dsi_div_set_rate(struct clk_hw *hw,
> + unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct dsi_div_hw_data *dsi_div = to_dsi_div_hw_data(hw);
> + struct rzg2l_cpg_priv *priv = dsi_div->priv;
> +
You should update priv->pll5_params here, instead of in your
.round_rate() callback.
> + writel(CPG_PL5_SDIV_DIV_DSI_A_WEN | CPG_PL5_SDIV_DIV_DSI_B_WEN |
> + (priv->pll5_params.dsi_div_a << 0) | (priv->pll5_params.dsi_div_b << 8),
> + priv->base + CPG_PL5_SDIV);
> +
> + return 0;
> +}
The rest LGTM.
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
