Re: [PATCH v5 3/5] clk: qcom: common: Add interconnect clocks support

[Stephen Boyd]

Quoting Varadarajan Narayanan (2024-03-29 03:48:24)
> On Thu, Mar 28, 2024 at 02:54:52PM -0700, Stephen Boyd wrote:
> > Quoting Varadarajan Narayanan (2024-03-28 00:59:34)
> > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > > index 75f09e6e057e..9fa271812373 100644
> > > --- a/drivers/clk/qcom/common.c
> > > +++ b/drivers/clk/qcom/common.c
> > > @@ -234,6 +236,41 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
> > >         return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
> > >  }
> > >
> > > +#if IS_ENABLED(CONFIG_INTERCONNECT_CLK)
> > > +static int qcom_cc_icc_register(struct device *dev,
> > > +                               const struct qcom_cc_desc *desc)
> > > +{
> > > +       struct icc_clk_data *icd;
> > > +       int i;
> > > +
> > > +       if (!desc->icc_hws)
> > > +               return 0;
> > > +
> > > +       icd = devm_kcalloc(dev, desc->num_icc_hws, sizeof(*icd), GFP_KERNEL);
> > > +       if (!icd)
> > > +               return -ENOMEM;
> > > +
> > > +       for (i = 0; i < desc->num_icc_hws; i++) {
> > > +               icd[i].clk = devm_clk_hw_get_clk(dev, desc->icc_hws[i], "qcom");
> >
> > Make the con_id "icc" instead please, so we know the consumer is
> > icc_clk.
> 
> Ok.
> 
> > Even better would be for the icc_clk device itself to be the
> > one requesting with devm_clk_hw_get_clk() so that we associate the clk
> > handle with the consumer device. It would also help us make it so that
> > drivers defer probe until their clk isn't an orphan.
> 
> Not sure if I understand the comments correctly.
> 
> In one of the previous patches, had
>         icd[i].clk = clks[noc_clks[i]]->hw.clk;
> 
> This was said to be error prone since the clock would not be
> ref counted. Hence used devm_clk_hw_get_clk before doing
> icc_clk_register.
> 
> Now, are you suggesting to use the direct clock pointer
> and do a devm_clk_hw_get_clk from the consumer driver?
> This will take care of the refcounting. However, we will
> have to add these clock entries to the consumer DT node.
> Is this ok?

Why do they need to be added to the consumer DT node? Why can't the
icc_clk device driver (icc_clk_driver?) use struct clk_hw instead of
struct clk in struct icc_clk_data? The answer cannot be that the icc_clk
driver cannot be changed.

> > > diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> > > index 9c8f7b798d9f..d8ac26d83f3c 100644
> > > --- a/drivers/clk/qcom/common.h
> > > +++ b/drivers/clk/qcom/common.h
> > > @@ -29,6 +29,9 @@ struct qcom_cc_desc {
> > >         size_t num_gdscs;
> > >         struct clk_hw **clk_hws;
> > >         size_t num_clk_hws;
> > > +       struct clk_hw **icc_hws;
> > > +       size_t num_icc_hws;
> > > +       unsigned int first_id;
> >
> > 'first_id' is gross.
> 
> will change it to 'icc_id'.

That's not what I meant :) The whole concept of having to pick some
random number is bad. At the least, hide that in the icc_clk driver so
that we don't have to put this in every clk provider that is also an
interconnect provider.