[v5,0/5] Add interconnect driver for IPQ9574 SoC

MSM platforms manage NoC related clocks and scaling from RPM.
However, in IPQ SoCs, RPM is not involved in managing NoC
related clocks and there is no NoC scaling.

However, there is a requirement to enable some NoC interface
clocks for the accessing the peripherals present in the
system. Hence add a minimalistic interconnect driver that
establishes a path from the processor/memory to those peripherals
and vice versa.

---
v5:
	Split gcc-ipq9574.c and common.c changes into separate patches
	Introduce devm_icc_clk_register
	Fix error handling
v4:
gcc-ipq9574.c
	Use clk_hw instead of indices
common.c
	Do icc register in qcom_cc_probe() call stream
common.h
	Add icc clock info to qcom_cc_desc structure

v3:
qcom,ipq9574.h
	Move 'first id' define to clock driver
gcc-ipq9574.c:
	Use indexed identifiers here to avoid confusion
	Fix error messages and move code to common.c as it can be
	shared with future SoCs

v2:
qcom,ipq9574.h
	Fix license identifier
	Rename macros
qcom,ipq9574-gcc.yaml
	Include interconnect-cells
gcc-ipq9574.c
	Update commit log
	Remove IS_ENABLED(CONFIG_INTERCONNECT) and auto select it from Kconfig
ipq9574.dtsi
	Moved to separate patch
	Include interconnect-cells to clock controller node
drivers/clk/qcom/Kconfig:
	Auto select CONFIG_INTERCONNECT & CONFIG_INTERCONNECT_CLK

Varadarajan Narayanan (5):
  dt-bindings: interconnect: Add Qualcomm IPQ9574 support
  interconnect: icc-clk: Add devm_icc_clk_register
  clk: qcom: common: Add interconnect clocks support
  clk: qcom: ipq9574: Use icc-clk for enabling NoC related clocks
  arm64: dts: qcom: ipq9574: Add icc provider ability to gcc

 .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
 arch/arm64/boot/dts/qcom/ipq9574.dtsi         |  2 +
 drivers/clk/qcom/Kconfig                      |  2 +
 drivers/clk/qcom/common.c                     | 39 +++++++++++-
 drivers/clk/qcom/common.h                     |  3 +
 drivers/clk/qcom/gcc-ipq9574.c                | 54 +++++++++++++++++
 drivers/interconnect/icc-clk.c                | 29 +++++++++
 .../dt-bindings/interconnect/qcom,ipq9574.h   | 59 +++++++++++++++++++
 include/linux/interconnect-clk.h              |  4 ++
 9 files changed, 194 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h

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
> @@ -8,6 +8,8 @@
>  #include <linux/regmap.h>
>  #include <linux/platform_device.h>
>  #include <linux/clk-provider.h>
> +#include <linux/interconnect-clk.h>
> +#include <linux/interconnect-provider.h>

Do we need the second include?

>  #include <linux/reset-controller.h>
>  #include <linux/of.h>
>  
> @@ -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. 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.

> +               if (IS_ERR(icd[i].clk))
> +                       return dev_err_probe(dev, PTR_ERR(icd[i].clk),
> +                                            "get clock failed (%ld)\n",
> +                                            PTR_ERR(icd[i].clk));
> +
> +               icd[i].name = clk_hw_get_name(desc->icc_hws[i]);
> +       }
> +
> +       return PTR_ERR_OR_ZERO(devm_icc_clk_register(dev, desc->first_id,
> +                                                    desc->num_icc_hws, icd));
> +}
> +#else
> +static int qcom_cc_icc_register(struct device *dev,
> +                               const struct qcom_cc_desc *desc)
> +{
> +       return 0;
> +}

Instead of this please have an

	if (!IS_ENABLED(CONFIG_INTERCONNECT_CLK))
		return 0;

> +#endif
> +
>  int qcom_cc_really_probe(struct platform_device *pdev,
>                          const struct qcom_cc_desc *desc, struct regmap *regmap)
>  {
> @@ -303,7 +340,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>         if (ret)
>                 return ret;
>  
> -       return 0;
> +       return qcom_cc_icc_register(dev, desc);
>  }
>  EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
>  
> 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.

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
> > @@ -8,6 +8,8 @@
> >  #include <linux/regmap.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/clk-provider.h>
> > +#include <linux/interconnect-clk.h>
> > +#include <linux/interconnect-provider.h>
>
> Do we need the second include?

Will remove.

> >  #include <linux/reset-controller.h>
> >  #include <linux/of.h>
> >
> > @@ -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?

> > +               if (IS_ERR(icd[i].clk))
> > +                       return dev_err_probe(dev, PTR_ERR(icd[i].clk),
> > +                                            "get clock failed (%ld)\n",
> > +                                            PTR_ERR(icd[i].clk));
> > +
> > +               icd[i].name = clk_hw_get_name(desc->icc_hws[i]);
> > +       }
> > +
> > +       return PTR_ERR_OR_ZERO(devm_icc_clk_register(dev, desc->first_id,
> > +                                                    desc->num_icc_hws, icd));
> > +}
> > +#else
> > +static int qcom_cc_icc_register(struct device *dev,
> > +                               const struct qcom_cc_desc *desc)
> > +{
> > +       return 0;
> > +}
>
> Instead of this please have an
>
> 	if (!IS_ENABLED(CONFIG_INTERCONNECT_CLK))
> 		return 0;

Ok.

> > +#endif
> > +
> >  int qcom_cc_really_probe(struct platform_device *pdev,
> >                          const struct qcom_cc_desc *desc, struct regmap *regmap)
> >  {
> > @@ -303,7 +340,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> >         if (ret)
> >                 return ret;
> >
> > -       return 0;
> > +       return qcom_cc_icc_register(dev, desc);
> >  }
> >  EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
> >
> > 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'.

Thanks
Varada

On 28/03/2024 08:59, Varadarajan Narayanan wrote:
> Add interconnect-cells to clock provider so that it can be
> used as icc provider.
> 
> Add master/slave ids for Qualcomm IPQ9574 Network-On-Chip
> interfaces. This will be used by the gcc-ipq9574 driver
> that will for providing interconnect services using the
> icc-clk framework.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Please remove my Reviewed tag.

It seems you do not use this binding header: neither in your DTS and nor
in the driver.

Consider the patch NAK-ed and send new version which removes unused header.

Please provide link to upstreamed DTS using this, so we can validate
your usage. Otherwise it looks just wrong and you try to upstream
something which will not pass dtbs checks on the first day, for example.

NAK

Best regards,
Krzysztof

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.