Re: [PATCH v4 2/3] clk: qcom: add IPQ9574 interconnect clocks support

From: Dmitry Baryshkov
Date: Wed Mar 27 2024 - 04:50:26 EST

On Wed, 27 Mar 2024 at 10:21, Varadarajan Narayanan
<quic_varada@xxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxx>
> ---
> 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/Kconfig | 2 ++
> drivers/clk/qcom/common.c | 34 ++++++++++++++++++++-
> drivers/clk/qcom/common.h | 4 ++-
> drivers/clk/qcom/gcc-ipq9574.c | 54 ++++++++++++++++++++++++++++++++++

These changes must be separate.

> 4 files changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 8ab08e7b5b6c..af73a0b396eb 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -243,6 +243,8 @@ config IPQ_GCC_8074
>
> config IPQ_GCC_9574
> tristate "IPQ9574 Global Clock Controller"
> + select INTERCONNECT
> + select INTERCONNECT_CLK
> help
> Support for global clock controller on ipq9574 devices.
> Say Y if you want to use peripheral devices such as UART, SPI,
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 75f09e6e057e..523d30d0ccbc 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,36 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
> return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
> }
>
> +static int qcom_cc_icc_register(struct device *dev,
> + const struct qcom_cc_desc *desc)
> +{
> +#if IS_ENABLED(CONFIG_INTERCONNECT_CLK)

General recommendation is to avoid #ifs inside function code. Please
move them outside and add an empty stub.

> + struct icc_provider *provider;
> + 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 dev_err_probe(dev, -ENOMEM, "malloc for icc clock data failed\n");

No need to.

> +
> + for (i = 0; i < desc->num_icc_hws; i++) {
> + icd[i].clk = devm_clk_hw_get_clk(dev, desc->icc_hws[i], "");

Passing an empty string is not correct from the CCF point of view.

> + if (IS_ERR_OR_NULL(icd[i].clk))
> + return dev_err_probe(dev, -ENOENT, "icc clock not found\n");

No, just IS_ERR and PTR_ERR. Don't throw away the resulting code.
In most of the case IS_ERR_OR_NULL is not correct. The function either
returns NULL or returns an error pointer. It can not be both.

> + icd[i].name = clk_hw_get_name(desc->icc_hws[i]);
> + }
> +
> + provider = icc_clk_register(dev, desc->first_id, desc->num_icc_hws, icd);
> + if (IS_ERR_OR_NULL(provider))

just use PTR_ERR_OR_ZERO

Also note that there is no qcom_cc_remove, so please add
devm_icc_clk_register() and use it here:

return PTR_ERR_OR_ZERO(devm_icc_clk_register(dev, ...));

> + return dev_err_probe(dev, PTR_ERR(provider),
> + "icc_clk_register failed\n");
> +#endif
> + return 0;
> +}
> +
> int qcom_cc_really_probe(struct platform_device *pdev,
> const struct qcom_cc_desc *desc, struct regmap *regmap)
> {
> @@ -303,7 +335,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..3c3a07f6dcb9 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;
> };
>
> /**
> @@ -65,5 +68,4 @@ extern int qcom_cc_probe(struct platform_device *pdev,
> const struct qcom_cc_desc *desc);
> extern int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
> const struct qcom_cc_desc *desc);
> -
> #endif
> diff --git a/drivers/clk/qcom/gcc-ipq9574.c b/drivers/clk/qcom/gcc-ipq9574.c
> index 0a3f846695b8..187fd9dcdf49 100644
> --- a/drivers/clk/qcom/gcc-ipq9574.c
> +++ b/drivers/clk/qcom/gcc-ipq9574.c
> @@ -12,6 +12,7 @@
>
> #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
> #include <dt-bindings/reset/qcom,ipq9574-gcc.h>
> +#include <dt-bindings/interconnect/qcom,ipq9574.h>
>
> #include "clk-alpha-pll.h"
> #include "clk-branch.h"
> @@ -4301,6 +4302,56 @@ static const struct qcom_reset_map gcc_ipq9574_resets[] = {
> [GCC_WCSS_Q6_TBU_BCR] = { 0x12054, 0 },
> };
>
> +#define IPQ_APPS_ID 9574 /* some unique value */
> +
> +enum {
> + ICC_ANOC_PCIE0,
> + ICC_SNOC_PCIE0,
> + ICC_ANOC_PCIE1,
> + ICC_SNOC_PCIE1,
> + ICC_ANOC_PCIE2,
> + ICC_SNOC_PCIE2,
> + ICC_ANOC_PCIE3,
> + ICC_SNOC_PCIE3,
> + ICC_SNOC_USB,
> + ICC_ANOC_USB_AXI,
> + ICC_NSSNOC_NSSCC,
> + ICC_NSSNOC_SNOC_0,
> + ICC_NSSNOC_SNOC_1,
> + ICC_NSSNOC_PCNOC_1,
> + ICC_NSSNOC_QOSGEN_REF,
> + ICC_NSSNOC_TIMEOUT_REF,
> + ICC_NSSNOC_XO_DCD,
> + ICC_NSSNOC_ATB,
> + ICC_MEM_NOC_NSSNOC,
> + ICC_NSSNOC_MEMNOC,
> + ICC_NSSNOC_MEM_NOC_1,
> +};
> +
> +static struct clk_hw *icc_ipq9574_hws[] = {
> + [ICC_ANOC_PCIE0] = &gcc_anoc_pcie0_1lane_m_clk.clkr.hw,
> + [ICC_SNOC_PCIE0] = &gcc_anoc_pcie1_1lane_m_clk.clkr.hw,
> + [ICC_ANOC_PCIE1] = &gcc_anoc_pcie2_2lane_m_clk.clkr.hw,
> + [ICC_SNOC_PCIE1] = &gcc_anoc_pcie3_2lane_m_clk.clkr.hw,
> + [ICC_ANOC_PCIE2] = &gcc_snoc_pcie0_1lane_s_clk.clkr.hw,
> + [ICC_SNOC_PCIE2] = &gcc_snoc_pcie1_1lane_s_clk.clkr.hw,
> + [ICC_ANOC_PCIE3] = &gcc_snoc_pcie2_2lane_s_clk.clkr.hw,
> + [ICC_SNOC_PCIE3] = &gcc_snoc_pcie3_2lane_s_clk.clkr.hw,
> + [ICC_SNOC_USB] = &gcc_snoc_usb_clk.clkr.hw,
> + [ICC_ANOC_USB_AXI] = &gcc_anoc_usb_axi_clk.clkr.hw,
> + [ICC_NSSNOC_NSSCC] = &gcc_nssnoc_nsscc_clk.clkr.hw,
> + [ICC_NSSNOC_SNOC_0] = &gcc_nssnoc_snoc_clk.clkr.hw,
> + [ICC_NSSNOC_SNOC_1] = &gcc_nssnoc_snoc_1_clk.clkr.hw,
> + [ICC_NSSNOC_PCNOC_1] = &gcc_nssnoc_pcnoc_1_clk.clkr.hw,
> + [ICC_NSSNOC_QOSGEN_REF] = &gcc_nssnoc_qosgen_ref_clk.clkr.hw,
> + [ICC_NSSNOC_TIMEOUT_REF] = &gcc_nssnoc_timeout_ref_clk.clkr.hw,
> + [ICC_NSSNOC_XO_DCD] = &gcc_nssnoc_xo_dcd_clk.clkr.hw,
> + [ICC_NSSNOC_ATB] = &gcc_nssnoc_atb_clk.clkr.hw,
> + [ICC_MEM_NOC_NSSNOC] = &gcc_mem_noc_nssnoc_clk.clkr.hw,
> + [ICC_NSSNOC_MEMNOC] = &gcc_nssnoc_memnoc_clk.clkr.hw,
> + [ICC_NSSNOC_MEM_NOC_1] = &gcc_nssnoc_mem_noc_1_clk.clkr.hw,
> +};
> +
> static const struct of_device_id gcc_ipq9574_match_table[] = {
> { .compatible = "qcom,ipq9574-gcc" },
> { }
> @@ -4323,6 +4374,9 @@ static const struct qcom_cc_desc gcc_ipq9574_desc = {
> .num_resets = ARRAY_SIZE(gcc_ipq9574_resets),
> .clk_hws = gcc_ipq9574_hws,
> .num_clk_hws = ARRAY_SIZE(gcc_ipq9574_hws),
> + .icc_hws = icc_ipq9574_hws,
> + .num_icc_hws = ARRAY_SIZE(icc_ipq9574_hws),
> + .first_id = IPQ_APPS_ID,
> };
>
> static int gcc_ipq9574_probe(struct platform_device *pdev)
> --
> 2.34.1
>
>


--
With best wishes
Dmitry