| Message ID | 20240328075936.223461-4-quic_varada@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add interconnect driver for IPQ9574 SoC | expand |
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
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.
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> #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"); + 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; +} +#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; }; /**
Unlike MSM platforms that manage NoC related clocks and scaling from RPM, IPQ SoCs dont involve RPM in managing NoC related clocks and there is no NoC scaling. However, there is a requirement to enable some NoC interface clocks for accessing the peripheral controllers present on these NoCs. Though exposing these as normal clocks would work, having a minimalistic interconnect driver to handle these clocks would make it consistent with other Qualcomm platforms resulting in common code paths. This is similar to msm8996-cbf's usage of icc-clk framework. Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> --- v5: Split changes in common.c to separate patch Fix error handling Use devm_icc_clk_register instead of icc_clk_register v4: Use clk_hw instead of indices Do icc register in qcom_cc_probe() call stream Add icc clock info to qcom_cc_desc structure v3: Use indexed identifiers here to avoid confusion Fix error messages and move to common.c v2: Move DTS to separate patch Update commit log Auto select CONFIG_INTERCONNECT & CONFIG_INTERCONNECT_CLK to fix build error --- drivers/clk/qcom/common.c | 39 ++++++++++++++++++++++++++++++++++++++- drivers/clk/qcom/common.h | 3 +++ 2 files changed, 41 insertions(+), 1 deletion(-)