Re: [PATCH v3 02/10] clk/qcom: add initial clock driver for ipq5210

[Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>]

On Fri, Apr 17, 2026 at 08:52:10AM +1200, Simon Glass wrote:
> Hi Varadarajan,
>
> On 2026-04-16T05:39:18, Varadarajan Narayanan
> <varadarajan.narayanan@oss.qualcomm.com> wrote:
> > clk/qcom: add initial clock driver for ipq5210
> >
> > Add initial set of clocks and resets for enabling U-Boot on ipq5210
> > based RDP platforms.
> >
> > Signed-off-by: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>
> >
> > drivers/clk/qcom/Kconfig         |  8 ++++
> >  drivers/clk/qcom/Makefile        |  1 +
> >  drivers/clk/qcom/clock-ipq5210.c | 97 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 106 insertions(+)
>
> > +#include <dm/device-internal.h>
>
> Is this needed?

Will remove.

> > +static ulong ipq5210_set_rate(struct clk *clk, ulong rate)
> > +{
> > +     struct msm_clk_priv *priv = dev_get_priv(clk->dev);
> > +
> > +     switch (clk->id) {
> > +     case GCC_QUPV3_WRAP_SE1_CLK:
> > +             clk_rcg_set_rate_mnd(priv->base, priv->data->clks[clk->id].cbcr_reg,
>
> cbcr_reg is a bit confusing since that name suggests a CBCR register -
> see the ipq5424 driver which uses 'reg'. What do you think about using
> reg consistently, or maybe add a comment explaining this?

ipq5424 used GATE_CLK (defined in drivers/clk/qcom/clock-qcom.h).
That has been deprecated and ipq5210 uses GATE_CLK_POLLED. Hence
the difference.

> > +static int ipq5210_enable(struct clk *clk)
> > +{
> > +     struct msm_clk_priv *priv = dev_get_priv(clk->dev);
> > +
> > +     if (priv->data->num_clks <= clk->id) {
> > +             debug("%s: unknown clk id %lu\n", __func__, clk->id);
> > +             return 0;
> > +     }
>
> Shouldn't the comparison be <  ?

No, this check is right. The clock id defines range from zero to the
highest_id. So num_clks will get set to highest_id + 1. So a clock id of
highest_id + 1 is invalid.

> Also, should return an error, not 0.

Ok.

> > +     qcom_gate_clk_en(priv, clk->id);
> > +
> > +     return 0;
>
> Should return any error from qcom_gate_clk_en()

Ok.

Thanks
Varada