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

From: Dmitry Baryshkov
Date: Tue Mar 26 2024 - 08:37:48 EST

On Tue, 26 Mar 2024 at 14:14, 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>
> ---
> 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 | 30 ++++++++++++++++
> drivers/clk/qcom/common.h | 2 ++
> drivers/clk/qcom/gcc-ipq9574.c | 64 +++++++++++++++++++++++++++++++++-
> 4 files changed, 97 insertions(+), 1 deletion(-)
>
> 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..b18d38509de5 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>
>
> @@ -337,4 +339,32 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
> }
> EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
>
> +int qcom_cc_icc_register(struct device *dev, struct clk_regmap *clks[],
> + int *noc_clks, int count, unsigned int first_id)
> +{
> + struct icc_provider *provider;
> + struct icc_clk_data *icd;
> + int i;
> +
> + icd = devm_kcalloc(dev, count, sizeof(*icd), GFP_KERNEL);
> + if (IS_ERR_OR_NULL(icd))

Can devm_kcalloc return ERR?

> + return dev_err_probe(dev, PTR_ERR(icd),
> + "malloc for clock data failed\n");

So, this becomes dev_err_prove(dev, 0, "..."). returning 0. Not the
expected result for the error case.

> +
> + for (i = 0; i < count; i++) {
> + icd[i].clk = clks[noc_clks[i]]->hw.clk;
> + if (IS_ERR_OR_NULL(icd[i].clk))
> + return dev_err_probe(dev, -ENOENT,
> + "%d clock not found\n", noc_clks[i]);

This is even better. Potential NULL pointer exception, then useless
ERR_OR_NULL and finally API abuse.
Please use clk_hw_get_clk(), it is there for you.

> + icd[i].name = clk_hw_get_name(&clks[noc_clks[i]]->hw);
> + }
> +
> + provider = icc_clk_register(dev, first_id, count, icd);
> + if (IS_ERR_OR_NULL(provider))
> + return dev_err_probe(dev, PTR_ERR(provider),
> + "icc_clk_register failed\n");
> +
> + return 0;
> +}
> +
> MODULE_LICENSE("GPL v2");
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index 9c8f7b798d9f..4fce5e229fc1 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -65,5 +65,7 @@ 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);
> +int qcom_cc_icc_register(struct device *dev, struct clk_regmap *clks[],
> + int *noc_clks, int count, unsigned int first_id);

Add this function to the qcom_cc_probe() call stream. Change it to
pass an array of clk_hw instead of passing indices.

>
> #endif
> diff --git a/drivers/clk/qcom/gcc-ipq9574.c b/drivers/clk/qcom/gcc-ipq9574.c
> index 0a3f846695b8..c63c44b6740f 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 int noc_clks[] = {
> + [ICC_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
> + [ICC_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
> + [ICC_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
> + [ICC_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
> + [ICC_ANOC_PCIE2] = GCC_ANOC_PCIE2_2LANE_M_CLK,
> + [ICC_SNOC_PCIE2] = GCC_SNOC_PCIE2_2LANE_S_CLK,
> + [ICC_ANOC_PCIE3] = GCC_ANOC_PCIE3_2LANE_M_CLK,
> + [ICC_SNOC_PCIE3] = GCC_SNOC_PCIE3_2LANE_S_CLK,
> + [ICC_SNOC_USB] = GCC_SNOC_USB_CLK,
> + [ICC_ANOC_USB_AXI] = GCC_ANOC_USB_AXI_CLK,
> + [ICC_NSSNOC_NSSCC] = GCC_NSSNOC_NSSCC_CLK,
> + [ICC_NSSNOC_SNOC_0] = GCC_NSSNOC_SNOC_CLK,
> + [ICC_NSSNOC_SNOC_1] = GCC_NSSNOC_SNOC_1_CLK,
> + [ICC_NSSNOC_PCNOC_1] = GCC_NSSNOC_PCNOC_1_CLK,
> + [ICC_NSSNOC_QOSGEN_REF] = GCC_NSSNOC_QOSGEN_REF_CLK,
> + [ICC_NSSNOC_TIMEOUT_REF] = GCC_NSSNOC_TIMEOUT_REF_CLK,
> + [ICC_NSSNOC_XO_DCD] = GCC_NSSNOC_XO_DCD_CLK,
> + [ICC_NSSNOC_ATB] = GCC_NSSNOC_ATB_CLK,
> + [ICC_MEM_NOC_NSSNOC] = GCC_MEM_NOC_NSSNOC_CLK,
> + [ICC_NSSNOC_MEMNOC] = GCC_NSSNOC_MEMNOC_CLK,
> + [ICC_NSSNOC_MEM_NOC_1] = GCC_NSSNOC_MEM_NOC_1_CLK,
> +};
> +
> static const struct of_device_id gcc_ipq9574_match_table[] = {
> { .compatible = "qcom,ipq9574-gcc" },
> { }
> @@ -4327,7 +4378,18 @@ static const struct qcom_cc_desc gcc_ipq9574_desc = {
>
> static int gcc_ipq9574_probe(struct platform_device *pdev)
> {
> - return qcom_cc_probe(pdev, &gcc_ipq9574_desc);
> + int ret;
> +
> + ret = qcom_cc_probe(pdev, &gcc_ipq9574_desc);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "clock probe failed\n");
> +
> + ret = qcom_cc_icc_register(&pdev->dev, gcc_ipq9574_clks, noc_clks,
> + ARRAY_SIZE(noc_clks), IPQ_APPS_ID);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "interconnect register failed\n");

So, even if we had a useful message from the called function, it has
become overridden with the useless "failed" message. Please don't do
that.

> +
> + return 0;
> }
>
> static struct platform_driver gcc_ipq9574_driver = {
> --
> 2.34.1
>
>


--
With best wishes
Dmitry