[PATCH 1/2] dt-bindings: interconnect: Add Qualcomm CCI dt-bindings

[PATCH 1/2] dt-bindings: interconnect: Add Qualcomm CCI dt-bindings

[Jun Nie]

Add devicetree binding of Qualcomm CCI on MSM8939.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 .../bindings/interconnect/qcom,cci.yaml       | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,cci.yaml

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
new file mode 100644
index 000000000000..100c440ba220
--- /dev/null
+++ b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interconnect/mediatek,cci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Cache Coherent Interconnect (CCI) frequency and voltage scaling
+
+maintainers:
+  - Jun Nie <jun.nie@linaro.org>
+
+description: |
+  Qualcomm Cache Coherent Interconnect (CCI) is a hardware engine used by
+  MSM8939. The driver is to scale its frequency and adjust the voltage in
+  hardware accordingly. The voltage provider is modeled as power domain on
+  MSM8939, so power domain dts node is required.
+
+properties:
+  compatible:
+    enum:
+      - qcom,msm8939-cci
+
+  clocks:
+    maxItems: 1
+
+  operating-points-v2: true
+  opp-table:
+    type: object
+
+required:
+  - compatible
+  - clocks
+  - operating-points-v2
+  - nvmem-cells
+  - power-domains
+
+additionalProperties: false
+
+examples:
+  - |
+    cci: cci {
+        compatible = "qcom,msm8939-cci";
+	clocks = <&apcs2>;
+	operating-points-v2 = <&cci_opp_table>;
+	power-domains = <&cpr>;
+	nvmem-cells = <&cpr_efuse_speedbin_pvs>;
+    };
+
+    cci_opp_table: cci-opp-table {
+	compatible = "operating-points-v2";
+
+	cci_opp1: opp-200000000 {
+	       opp-hz = /bits/ 64 <200000000>;
+	       opp-supported-hw = <0x3f>;
+	       required-opps = <&cpr_opp3>;
+	};
+
+	cci_opp2: opp-297600000 {
+	       opp-hz = /bits/ 64 <297600000>;
+	       opp-supported-hw = <0x3f>;
+	       required-opps = <&cpr_opp12>;
+	};
+
+	cci_opp3: opp-400000000 {
+	       opp-hz = /bits/ 64 <400000000>;
+	       opp-supported-hw = <0x1>;
+	       required-opps = <&cpr_opp14>;
+	};
+
+	cci_opp4: opp-400000000 {
+	       opp-hz = /bits/ 64 <400000000>;
+	       opp-supported-hw = <0x3e>;
+	       required-opps = <&cpr_opp15>;
+	};
+
+	cci_opp5: opp-595200000 {
+	       opp-hz = /bits/ 64 <595200000>;
+	       opp-supported-hw = <0x3f>;
+	       required-opps = <&cpr_opp17>;
+	};
+    };
-- 
2.34.1

[PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Jun Nie]

Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
driver is introduced so that its freqency can be adjusted. And regulator
associated with opp table can be also adjusted accordingly which is
shared with cpu cluster.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 drivers/devfreq/Kconfig    |   9 +++
 drivers/devfreq/Makefile   |   1 +
 drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+)
 create mode 100644 drivers/devfreq/qcom-cci.c

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 9754d8b31621..6f3f1872f3fb 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -130,6 +130,15 @@ config ARM_MEDIATEK_CCI_DEVFREQ
 	  buck voltages and update a proper CCI frequency. Use the notification
 	  to get the regulator status.
 
+config ARM_QCOM_CCI_DEVFREQ
+	tristate "QUALCOMM CCI DEVFREQ Driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	select DEVFREQ_GOV_PASSIVE
+	help
+	  This adds a devfreq driver for Qualcomm Cache Coherent Interconnect which
+	  shares the same regulator with the cpu cluster. This driver can track a
+	  proper regulator state while CCI frequency is updated.
+
 config ARM_RK3399_DMC_DEVFREQ
 	tristate "ARM RK3399 DMC DEVFREQ Driver"
 	depends on (ARCH_ROCKCHIP && HAVE_ARM_SMCCC) || \
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index bf40d04928d0..f2526d8c168d 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
 obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ)	+= imx-bus.o
 obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
 obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ)	+= mtk-cci-devfreq.o
+obj-$(CONFIG_ARM_QCOM_CCI_DEVFREQ)	+= qcom-cci.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_SUN8I_A33_MBUS_DEVFREQ)	+= sun8i-a33-mbus.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
diff --git a/drivers/devfreq/qcom-cci.c b/drivers/devfreq/qcom-cci.c
new file mode 100644
index 000000000000..21b5f133cddc
--- /dev/null
+++ b/drivers/devfreq/qcom-cci.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2023 Linaro Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of_device.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define SPEED_PVS(s, p) ((s << 16) | p)
+
+struct qcom_cci {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;
+	struct clk *clk;
+};
+
+static int qcom_cci_target(struct device *dev,
+		unsigned long *freq, u32 flags)
+{
+	struct dev_pm_opp *new_opp;
+	int ret;
+
+	new_opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(new_opp)) {
+		ret = PTR_ERR(new_opp);
+		dev_err(dev, "failed to get recommended opp: %d\n", ret);
+		return ret;
+	}
+	dev_pm_opp_put(new_opp);
+
+	return dev_pm_opp_set_rate(dev, *freq);
+}
+
+static int qcom_cci_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+	struct qcom_cci *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static int qcom_get_dev_version(struct nvmem_cell *speedbin_nvmem)
+{
+	int speed = 0, pvs = 0;
+	u8 *speedbin;
+	size_t len;
+
+	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
+	if (IS_ERR(speedbin))
+		return PTR_ERR(speedbin);
+
+	speed = (speedbin[0xc] >> 2) & 0x7;
+	pvs = (speedbin[0x3] >> 5 & 0x1) | ((speedbin[0x6] >> 2 & 0x3) << 1);
+	kfree(speedbin);
+
+	/* Convert speedbin and PVS into version bit map */
+	switch (SPEED_PVS(speed, pvs)) {
+	case SPEED_PVS(0, 0): return 0x1;
+	case SPEED_PVS(2, 0): return 0x2;
+	case SPEED_PVS(2, 2): return 0x4;
+	case SPEED_PVS(4, 0): return 0x8;
+	case SPEED_PVS(5, 0): return 0x10;
+	case SPEED_PVS(5, 6): return 0x20;
+	default:
+		return 0x1;
+	}
+}
+
+static void qcom_cci_exit(struct device *dev)
+{
+	dev_pm_opp_of_remove_table(dev);
+}
+
+static int qcom_cci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qcom_cci *priv;
+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	struct device_node *np = dev->of_node;
+	struct nvmem_cell *speedbin_nvmem;
+	int ret;
+	u32 version;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		ret = PTR_ERR(priv->clk);
+		dev_err(dev, "failed to fetch clk: %d\n", ret);
+		return ret;
+	}
+	platform_set_drvdata(pdev, priv);
+
+	/* Check whether we have profiled speed version per chip */
+	speedbin_nvmem = of_nvmem_cell_get(np, NULL);
+	if (IS_ERR(speedbin_nvmem))
+		return PTR_ERR(speedbin_nvmem);
+
+	version = qcom_get_dev_version(speedbin_nvmem);
+	dev_info(dev, "%s: set opp table version 0x%x\n", __func__, version);
+
+	nvmem_cell_put(speedbin_nvmem);
+	ret = dev_pm_opp_set_supported_hw(dev, &version, 1);
+	if (ret) {
+		dev_err(dev, "Failed to set supported hardware\n");
+		return ret;
+	}
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.target = qcom_cci_target;
+	priv->profile.exit = qcom_cci_exit;
+	priv->profile.get_cur_freq = qcom_cci_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, NULL);
+	if (IS_ERR(priv->devfreq)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to add devfreq device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	dev_pm_opp_of_remove_table(dev);
+	return ret;
+}
+
+static const struct of_device_id qcom_cci_of_match[] = {
+	{ .compatible = "qcom,msm8939-cci"},
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, qcom_cci_of_match);
+
+static struct platform_driver qcom_cci_platdrv = {
+	.probe		= qcom_cci_probe,
+	.driver = {
+		.name	= "qcom-cci-devfreq",
+		.of_match_table = qcom_cci_of_match,
+	},
+};
+module_platform_driver(qcom_cci_platdrv);
+
+MODULE_DESCRIPTION("QCOM cci frequency scaling driver");
+MODULE_AUTHOR("Jun Nie <jun.nie@linaro.org>");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Krzysztof Kozlowski]

On 01/02/2023 09:02, Jun Nie wrote:
> Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
> driver is introduced so that its freqency can be adjusted. And regulator
> associated with opp table can be also adjusted accordingly which is
> shared with cpu cluster.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You need to Cc Qualcomm subarch maintainers.

> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/devfreq/Kconfig    |   9 +++
>  drivers/devfreq/Makefile   |   1 +
>  drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++

Who is going to maintain this file/driver?

>  3 files changed, 172 insertions(+)
>  create mode 100644 drivers/devfreq/qcom-cci.c
> 

(...)

> +
> +static int qcom_cci_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_cci *priv;
> +	const char *gov = DEVFREQ_GOV_USERSPACE;
> +	struct device_node *np = dev->of_node;
> +	struct nvmem_cell *speedbin_nvmem;
> +	int ret;
> +	u32 version;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		ret = PTR_ERR(priv->clk);
> +		dev_err(dev, "failed to fetch clk: %d\n", ret);
> +		return ret;

All these are just return dev_err_probe


> +	}
> +	platform_set_drvdata(pdev, priv);
> +
> +	/* Check whether we have profiled speed version per chip */
> +	speedbin_nvmem = of_nvmem_cell_get(np, NULL);
> +	if (IS_ERR(speedbin_nvmem))
> +		return PTR_ERR(speedbin_nvmem);
> +
> +	version = qcom_get_dev_version(speedbin_nvmem);
> +	dev_info(dev, "%s: set opp table version 0x%x\n", __func__, version);

Drop __func__.

> +
> +	nvmem_cell_put(speedbin_nvmem);
> +	ret = dev_pm_opp_set_supported_hw(dev, &version, 1);
> +	if (ret) {
> +		dev_err(dev, "Failed to set supported hardware\n");

return dev_err_probe

> +		return ret;
> +	}
> +
> +	ret = dev_pm_opp_of_add_table(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get OPP table\n");
> +		return ret;

return dev_err_probe


> +	}
> +
> +	priv->profile.target = qcom_cci_target;
> +	priv->profile.exit = qcom_cci_exit;
> +	priv->profile.get_cur_freq = qcom_cci_get_cur_freq;
> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
> +
> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
> +						gov, NULL);
> +	if (IS_ERR(priv->devfreq)) {
> +		ret = PTR_ERR(priv->devfreq);
> +		dev_err(dev, "failed to add devfreq device: %d\n", ret);


ret = dev_err_probe


> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	dev_pm_opp_of_remove_table(dev);
> +	return ret;
> +}
> +
> +static const struct of_device_id qcom_cci_of_match[] = {
> +	{ .compatible = "qcom,msm8939-cci"},
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_cci_of_match);
> +
> +static struct platform_driver qcom_cci_platdrv = {
> +	.probe		= qcom_cci_probe,
> +	.driver = {
> +		.name	= "qcom-cci-devfreq",
> +		.of_match_table = qcom_cci_of_match,
> +	},
> +};
> +module_platform_driver(qcom_cci_platdrv);
> +
> +MODULE_DESCRIPTION("QCOM cci frequency scaling driver");
> +MODULE_AUTHOR("Jun Nie <jun.nie@linaro.org>");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Jun Nie]

> > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > ---
> >  drivers/devfreq/Kconfig    |   9 +++
> >  drivers/devfreq/Makefile   |   1 +
> >  drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++
>
> Who is going to maintain this file/driver?
>
I will add myself as maintainer of this file.
All other comments will be addressed in next version. Thanks!

- Jun

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Bryan O'Donoghue]

On 01/02/2023 08:02, Jun Nie wrote:
> Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
> driver is introduced so that its freqency can be adjusted. And regulator
> associated with opp table can be also adjusted accordingly which is
> shared with cpu cluster.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>

Nice work.

Thanks for taking the time to follow up on this. I had a look at the 
4.19 out-of-tree version of this we have and this code looks just about 
right.

With Krzysztof's comments addressed please add my

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Chanwoo Choi]

Hi,

On 23. 2. 1. 17:02, Jun Nie wrote:
> Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
> driver is introduced so that its freqency can be adjusted. And regulator
> associated with opp table can be also adjusted accordingly which is
> shared with cpu cluster.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/devfreq/Kconfig    |   9 +++
>  drivers/devfreq/Makefile   |   1 +
>  drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 172 insertions(+)
>  create mode 100644 drivers/devfreq/qcom-cci.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 9754d8b31621..6f3f1872f3fb 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -130,6 +130,15 @@ config ARM_MEDIATEK_CCI_DEVFREQ
>  	  buck voltages and update a proper CCI frequency. Use the notification
>  	  to get the regulator status.
>  
> +config ARM_QCOM_CCI_DEVFREQ
> +	tristate "QUALCOMM CCI DEVFREQ Driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	select DEVFREQ_GOV_PASSIVE
> +	help
> +	  This adds a devfreq driver for Qualcomm Cache Coherent Interconnect which
> +	  shares the same regulator with the cpu cluster. This driver can track a
> +	  proper regulator state while CCI frequency is updated.

Maybe, this driver use the passive governor because as this description,
the regulator is shared with cpu cluster. But, as I commented below,
you use the 'userspace' governor in probe. is it right?

> +
>  config ARM_RK3399_DMC_DEVFREQ
>  	tristate "ARM RK3399 DMC DEVFREQ Driver"
>  	depends on (ARCH_ROCKCHIP && HAVE_ARM_SMCCC) || \
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index bf40d04928d0..f2526d8c168d 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
>  obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ)	+= imx-bus.o
>  obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
>  obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ)	+= mtk-cci-devfreq.o
> +obj-$(CONFIG_ARM_QCOM_CCI_DEVFREQ)	+= qcom-cci.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_SUN8I_A33_MBUS_DEVFREQ)	+= sun8i-a33-mbus.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
> diff --git a/drivers/devfreq/qcom-cci.c b/drivers/devfreq/qcom-cci.c
> new file mode 100644
> index 000000000000..21b5f133cddc
> --- /dev/null
> +++ b/drivers/devfreq/qcom-cci.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2023 Linaro Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define SPEED_PVS(s, p) ((s << 16) | p)

What it meaning of PVS? Could you add the comment for 'PVS' and 's', 'p'?

> +
> +struct qcom_cci {
> +	struct devfreq_dev_profile profile;
> +	struct devfreq *devfreq;
> +	struct clk *clk;
> +};
> +
> +static int qcom_cci_target(struct device *dev,
> +		unsigned long *freq, u32 flags)

Actually, this line is not long. You can type it on one line as following:

static int qcom_cci_target(struct device *dev, unsigned long *freq, u32 flags)

> +{
> +	struct dev_pm_opp *new_opp;
> +	int ret;

As I mentioned belwo, this local variable is not needed
if just return PTR_ERR(new_opp).

> +
> +	new_opp = devfreq_recommended_opp(dev, freq, flags);
> +	if (IS_ERR(new_opp)) {
> +		ret = PTR_ERR(new_opp);
> +		dev_err(dev, "failed to get recommended opp: %d\n", ret);
> +		return ret;

Better to add 'return PTR_ERR(new_opp)' without 'ret' local variable.

> +	}
> +	dev_pm_opp_put(new_opp);
> +
> +	return dev_pm_opp_set_rate(dev, *freq);
> +}
> +
> +static int qcom_cci_get_cur_freq(struct device *dev, unsigned long *freq)
> +{
> +	struct qcom_cci *priv = dev_get_drvdata(dev);
> +
> +	*freq = clk_get_rate(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int qcom_get_dev_version(struct nvmem_cell *speedbin_nvmem)
> +{
> +	int speed = 0, pvs = 0;
> +	u8 *speedbin;
> +	size_t len;
> +
> +	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> +	if (IS_ERR(speedbin))
> +		return PTR_ERR(speedbin);
> +
> +	speed = (speedbin[0xc] >> 2) & 0x7;
> +	pvs = (speedbin[0x3] >> 5 & 0x1) | ((speedbin[0x6] >> 2 & 0x3) << 1);

Actually, 0xc, 0x3, 0x7, 0x1 and so on. It is impossible to understand
the meaning of this hex value. Plesae add the constant defintion 
for the readability. 

> +	kfree(speedbin);
> +
> +	/* Convert speedbin and PVS into version bit map */
> +	switch (SPEED_PVS(speed, pvs)) {
> +	case SPEED_PVS(0, 0): return 0x1;
> +	case SPEED_PVS(2, 0): return 0x2;
> +	case SPEED_PVS(2, 2): return 0x4;
> +	case SPEED_PVS(4, 0): return 0x8;
> +	case SPEED_PVS(5, 0): return 0x10;
> +	case SPEED_PVS(5, 6): return 0x20;
> +	default:
> +		return 0x1;> +	}
> +}
> +
> +static void qcom_cci_exit(struct device *dev)
> +{
> +	dev_pm_opp_of_remove_table(dev);
> +}
> +
> +static int qcom_cci_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_cci *priv;
> +	const char *gov = DEVFREQ_GOV_USERSPACE;

Even if you select 'DEVFREQ_GOV_PASSIVE' on Kconfig,
Is it right that this driver use the userspace governor?


> +	struct device_node *np = dev->of_node;
> +	struct nvmem_cell *speedbin_nvmem;
> +	int ret;
> +	u32 version;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		ret = PTR_ERR(priv->clk);
> +		dev_err(dev, "failed to fetch clk: %d\n", ret);
> +		return ret;

You can use dev_err_probe as following:

	return dev_err_probe(dev, ret, "failed to fetch clk: %d\n", ret);

> +	}
> +	platform_set_drvdata(pdev, priv);
> +
> +	/* Check whether we have profiled speed version per chip */
> +	speedbin_nvmem = of_nvmem_cell_get(np, NULL);
> +	if (IS_ERR(speedbin_nvmem))
> +		return PTR_ERR(speedbin_nvmem);

I recommend that you need to add the fail log with dev_err.

> +
> +	version = qcom_get_dev_version(speedbin_nvmem);
> +	dev_info(dev, "%s: set opp table version 0x%x\n", __func__, version);

Don't need to use '__func__' because dev_info will print the module name.
It is enough.

> +
> +	nvmem_cell_put(speedbin_nvmem);
> +	ret = dev_pm_opp_set_supported_hw(dev, &version, 1);
> +	if (ret) {
> +		dev_err(dev, "Failed to set supported hardware\n");
> +		return ret;
> +	}
> +
> +	ret = dev_pm_opp_of_add_table(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get OPP table\n");
> +		return ret;
> +	}
> +
> +	priv->profile.target = qcom_cci_target;
> +	priv->profile.exit = qcom_cci_exit;
> +	priv->profile.get_cur_freq = qcom_cci_get_cur_freq;
> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
> +
> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
> +						gov, NULL);
> +	if (IS_ERR(priv->devfreq)) {
> +		ret = PTR_ERR(priv->devfreq);
> +		dev_err(dev, "failed to add devfreq device: %d\n", ret);
> +		goto err;

Need to goto 'err_remove_opp_table' instead of 'err'.

> +	}
> +
> +	return 0;
> +
> +err:
> +	dev_pm_opp_of_remove_table(dev);
> +	return ret;

For more correct exception handling, need to change it as following:

err_remove_opp_table:
	dev_pm_opp_of_remove_table(dev);
err:
	return ret;

> +}
> +
> +static const struct of_device_id qcom_cci_of_match[] = {
> +	{ .compatible = "qcom,msm8939-cci"},
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_cci_of_match);
> +
> +static struct platform_driver qcom_cci_platdrv = {
> +	.probe		= qcom_cci_probe,
> +	.driver = {
> +		.name	= "qcom-cci-devfreq",
> +		.of_match_table = qcom_cci_of_match,
> +	},
> +};
> +module_platform_driver(qcom_cci_platdrv);
> +
> +MODULE_DESCRIPTION("QCOM cci frequency scaling driver");

cci is the abbreviation. You need to use the captical letter as following:

MODULE_DESCRIPTION("QCOM CCI frequency scaling driver");

> +MODULE_AUTHOR("Jun Nie <jun.nie@linaro.org>");
> +MODULE_LICENSE("GPL");

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Jun Nie]

Chanwoo Choi <cwchoi00@gmail.com> 于2023年2月1日周三 19:02写道：
>
> Hi,
>
> On 23. 2. 1. 17:02, Jun Nie wrote:
> > Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
> > driver is introduced so that its freqency can be adjusted. And regulator
> > associated with opp table can be also adjusted accordingly which is
> > shared with cpu cluster.
> >
> > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > ---
> >  drivers/devfreq/Kconfig    |   9 +++
> >  drivers/devfreq/Makefile   |   1 +
> >  drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 172 insertions(+)
> >  create mode 100644 drivers/devfreq/qcom-cci.c
> >
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 9754d8b31621..6f3f1872f3fb 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -130,6 +130,15 @@ config ARM_MEDIATEK_CCI_DEVFREQ
> >         buck voltages and update a proper CCI frequency. Use the notification
> >         to get the regulator status.
> >
> > +config ARM_QCOM_CCI_DEVFREQ
> > +     tristate "QUALCOMM CCI DEVFREQ Driver"
> > +     depends on ARCH_QCOM || COMPILE_TEST
> > +     select DEVFREQ_GOV_PASSIVE
> > +     help
> > +       This adds a devfreq driver for Qualcomm Cache Coherent Interconnect which
> > +       shares the same regulator with the cpu cluster. This driver can track a
> > +       proper regulator state while CCI frequency is updated.
>
> Maybe, this driver use the passive governor because as this description,
> the regulator is shared with cpu cluster. But, as I commented below,
> you use the 'userspace' governor in probe. is it right?
>
> > +
> >  config ARM_RK3399_DMC_DEVFREQ
> >       tristate "ARM RK3399 DMC DEVFREQ Driver"
> >       depends on (ARCH_ROCKCHIP && HAVE_ARM_SMCCC) || \
> > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> > index bf40d04928d0..f2526d8c168d 100644
> > --- a/drivers/devfreq/Makefile
> > +++ b/drivers/devfreq/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)        += exynos-bus.o
> >  obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ)    += imx-bus.o
> >  obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o
> >  obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ)       += mtk-cci-devfreq.o
> > +obj-$(CONFIG_ARM_QCOM_CCI_DEVFREQ)   += qcom-cci.o
> >  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
> >  obj-$(CONFIG_ARM_SUN8I_A33_MBUS_DEVFREQ)     += sun8i-a33-mbus.o
> >  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)              += tegra30-devfreq.o
> > diff --git a/drivers/devfreq/qcom-cci.c b/drivers/devfreq/qcom-cci.c
> > new file mode 100644
> > index 000000000000..21b5f133cddc
> > --- /dev/null
> > +++ b/drivers/devfreq/qcom-cci.c
> > @@ -0,0 +1,162 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2023 Linaro Ltd.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/devfreq.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#define SPEED_PVS(s, p) ((s << 16) | p)
>
> What it meaning of PVS? Could you add the comment for 'PVS' and 's', 'p'?
>
This is what I have from Qualcomm code. Maybe others have more info about it.
Process Voltage Scaling(PVS) Tables defines the voltage and frequency
value based on the msm-id in SMEM and speedbin blown in the efuse
combination.

> > +
> > +struct qcom_cci {
> > +     struct devfreq_dev_profile profile;
> > +     struct devfreq *devfreq;
> > +     struct clk *clk;
> > +};
> > +
> > +static int qcom_cci_target(struct device *dev,
> > +             unsigned long *freq, u32 flags)
>
> Actually, this line is not long. You can type it on one line as following:
>
> static int qcom_cci_target(struct device *dev, unsigned long *freq, u32 flags)
>
> > +{
> > +     struct dev_pm_opp *new_opp;
> > +     int ret;
>
> As I mentioned belwo, this local variable is not needed
> if just return PTR_ERR(new_opp).
>
> > +
> > +     new_opp = devfreq_recommended_opp(dev, freq, flags);
> > +     if (IS_ERR(new_opp)) {
> > +             ret = PTR_ERR(new_opp);
> > +             dev_err(dev, "failed to get recommended opp: %d\n", ret);
> > +             return ret;
>
> Better to add 'return PTR_ERR(new_opp)' without 'ret' local variable.
>
> > +     }
> > +     dev_pm_opp_put(new_opp);
> > +
> > +     return dev_pm_opp_set_rate(dev, *freq);
> > +}
> > +
> > +static int qcom_cci_get_cur_freq(struct device *dev, unsigned long *freq)
> > +{
> > +     struct qcom_cci *priv = dev_get_drvdata(dev);
> > +
> > +     *freq = clk_get_rate(priv->clk);
> > +
> > +     return 0;
> > +}
> > +
> > +static int qcom_get_dev_version(struct nvmem_cell *speedbin_nvmem)
> > +{
> > +     int speed = 0, pvs = 0;
> > +     u8 *speedbin;
> > +     size_t len;
> > +
> > +     speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> > +     if (IS_ERR(speedbin))
> > +             return PTR_ERR(speedbin);
> > +
> > +     speed = (speedbin[0xc] >> 2) & 0x7;
> > +     pvs = (speedbin[0x3] >> 5 & 0x1) | ((speedbin[0x6] >> 2 & 0x3) << 1);
>
> Actually, 0xc, 0x3, 0x7, 0x1 and so on. It is impossible to understand
> the meaning of this hex value. Plesae add the constant defintion
> for the readability.
>
Thanks for pointing it. The bit wise manipulation will be converted
into nvmem offset and
bit position in next version. So driver will read out value directly
without bit shift/mask.
Other comments will be adopted in next version. Thanks!

- Jun

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Dmitry Baryshkov]

On 01/02/2023 17:17, Jun Nie wrote:
> Chanwoo Choi <cwchoi00@gmail.com> 于2023年2月1日周三 19:02写道：
>>
>> Hi,
>>
>> On 23. 2. 1. 17:02, Jun Nie wrote:
>>> Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
>>> driver is introduced so that its freqency can be adjusted. And regulator
>>> associated with opp table can be also adjusted accordingly which is
>>> shared with cpu cluster.
>>>
>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>> ---
>>>   drivers/devfreq/Kconfig    |   9 +++
>>>   drivers/devfreq/Makefile   |   1 +
>>>   drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 172 insertions(+)
>>>   create mode 100644 drivers/devfreq/qcom-cci.c
>>>
>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>> index 9754d8b31621..6f3f1872f3fb 100644
>>> --- a/drivers/devfreq/Kconfig
>>> +++ b/drivers/devfreq/Kconfig
>>> @@ -130,6 +130,15 @@ config ARM_MEDIATEK_CCI_DEVFREQ
>>>          buck voltages and update a proper CCI frequency. Use the notification
>>>          to get the regulator status.
>>>
>>> +config ARM_QCOM_CCI_DEVFREQ
>>> +     tristate "QUALCOMM CCI DEVFREQ Driver"
>>> +     depends on ARCH_QCOM || COMPILE_TEST
>>> +     select DEVFREQ_GOV_PASSIVE
>>> +     help
>>> +       This adds a devfreq driver for Qualcomm Cache Coherent Interconnect which
>>> +       shares the same regulator with the cpu cluster. This driver can track a
>>> +       proper regulator state while CCI frequency is updated.
>>
>> Maybe, this driver use the passive governor because as this description,
>> the regulator is shared with cpu cluster. But, as I commented below,
>> you use the 'userspace' governor in probe. is it right?
>>
>>> +
>>>   config ARM_RK3399_DMC_DEVFREQ
>>>        tristate "ARM RK3399 DMC DEVFREQ Driver"
>>>        depends on (ARCH_ROCKCHIP && HAVE_ARM_SMCCC) || \
>>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>>> index bf40d04928d0..f2526d8c168d 100644
>>> --- a/drivers/devfreq/Makefile
>>> +++ b/drivers/devfreq/Makefile
>>> @@ -12,6 +12,7 @@ obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)        += exynos-bus.o
>>>   obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ)    += imx-bus.o
>>>   obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o
>>>   obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ)       += mtk-cci-devfreq.o
>>> +obj-$(CONFIG_ARM_QCOM_CCI_DEVFREQ)   += qcom-cci.o
>>>   obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
>>>   obj-$(CONFIG_ARM_SUN8I_A33_MBUS_DEVFREQ)     += sun8i-a33-mbus.o
>>>   obj-$(CONFIG_ARM_TEGRA_DEVFREQ)              += tegra30-devfreq.o
>>> diff --git a/drivers/devfreq/qcom-cci.c b/drivers/devfreq/qcom-cci.c
>>> new file mode 100644
>>> index 000000000000..21b5f133cddc
>>> --- /dev/null
>>> +++ b/drivers/devfreq/qcom-cci.c
>>> @@ -0,0 +1,162 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright 2023 Linaro Ltd.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/devfreq.h>
>>> +#include <linux/device.h>
>>> +#include <linux/module.h>
>>> +#include <linux/nvmem-consumer.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/pm_opp.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#define SPEED_PVS(s, p) ((s << 16) | p)
>>
>> What it meaning of PVS? Could you add the comment for 'PVS' and 's', 'p'?
>>
> This is what I have from Qualcomm code. Maybe others have more info about it.
> Process Voltage Scaling(PVS) Tables defines the voltage and frequency
> value based on the msm-id in SMEM and speedbin blown in the efuse
> combination.
> 
>>> +
>>> +struct qcom_cci {
>>> +     struct devfreq_dev_profile profile;
>>> +     struct devfreq *devfreq;
>>> +     struct clk *clk;
>>> +};
>>> +
>>> +static int qcom_cci_target(struct device *dev,
>>> +             unsigned long *freq, u32 flags)
>>
>> Actually, this line is not long. You can type it on one line as following:
>>
>> static int qcom_cci_target(struct device *dev, unsigned long *freq, u32 flags)
>>
>>> +{
>>> +     struct dev_pm_opp *new_opp;
>>> +     int ret;
>>
>> As I mentioned belwo, this local variable is not needed
>> if just return PTR_ERR(new_opp).
>>
>>> +
>>> +     new_opp = devfreq_recommended_opp(dev, freq, flags);
>>> +     if (IS_ERR(new_opp)) {
>>> +             ret = PTR_ERR(new_opp);
>>> +             dev_err(dev, "failed to get recommended opp: %d\n", ret);
>>> +             return ret;
>>
>> Better to add 'return PTR_ERR(new_opp)' without 'ret' local variable.
>>
>>> +     }
>>> +     dev_pm_opp_put(new_opp);
>>> +
>>> +     return dev_pm_opp_set_rate(dev, *freq);
>>> +}
>>> +
>>> +static int qcom_cci_get_cur_freq(struct device *dev, unsigned long *freq)
>>> +{
>>> +     struct qcom_cci *priv = dev_get_drvdata(dev);
>>> +
>>> +     *freq = clk_get_rate(priv->clk);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int qcom_get_dev_version(struct nvmem_cell *speedbin_nvmem)
>>> +{
>>> +     int speed = 0, pvs = 0;
>>> +     u8 *speedbin;
>>> +     size_t len;
>>> +
>>> +     speedbin = nvmem_cell_read(speedbin_nvmem, &len);
>>> +     if (IS_ERR(speedbin))
>>> +             return PTR_ERR(speedbin);
>>> +
>>> +     speed = (speedbin[0xc] >> 2) & 0x7;
>>> +     pvs = (speedbin[0x3] >> 5 & 0x1) | ((speedbin[0x6] >> 2 & 0x3) << 1);
>>
>> Actually, 0xc, 0x3, 0x7, 0x1 and so on. It is impossible to understand
>> the meaning of this hex value. Plesae add the constant defintion
>> for the readability.
>>
> Thanks for pointing it. The bit wise manipulation will be converted
> into nvmem offset and
> bit position in next version. So driver will read out value directly
> without bit shift/mask.
> Other comments will be adopted in next version. Thanks!

Please consider handling everything using the nvmem API. If PVS is split 
into two regions it might be better to parse them separately and then 
combine them in the driver(*), rather than performing such parsing in 
the driver.

(*) while doing the rework of the tsens driver I also faced a similar 
issue. It might be nice to have 'combined' nvmem cells.

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Dmitry Baryshkov]

On 01/02/2023 10:02, Jun Nie wrote:
> Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
> driver is introduced so that its freqency can be adjusted. And regulator
> associated with opp table can be also adjusted accordingly which is
> shared with cpu cluster.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>   drivers/devfreq/Kconfig    |   9 +++
>   drivers/devfreq/Makefile   |   1 +
>   drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 172 insertions(+)
>   create mode 100644 drivers/devfreq/qcom-cci.c

Could you please describe in some additional details what are you trying 
to achieve? Should the CCI frequency be scaled manually or does it 
follow the cluster frequency? Do clusters vote on the CCI frequency?

I'm inclined to ask if it is possible to shift this to the cpufreq OPP 
tables?

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Bryan O'Donoghue]

On 01/02/2023 11:32, Dmitry Baryshkov wrote:
> On 01/02/2023 10:02, Jun Nie wrote:
>> Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
>> driver is introduced so that its freqency can be adjusted. And regulator
>> associated with opp table can be also adjusted accordingly which is
>> shared with cpu cluster.
>>
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>>   drivers/devfreq/Kconfig    |   9 +++
>>   drivers/devfreq/Makefile   |   1 +
>>   drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 172 insertions(+)
>>   create mode 100644 drivers/devfreq/qcom-cci.c
> 
> Could you please describe in some additional details what are you trying 
> to achieve? Should the CCI frequency be scaled manually or does it 
> follow the cluster frequency? Do clusters vote on the CCI frequency?
> 
> I'm inclined to ask if it is possible to shift this to the cpufreq OPP 
> tables?
> 

Might not be so easy to just append CCI opps to the cluster frequency opps

                 cci_cache: qcom,cci {
                         compatible = "qcom,msm8939-cci";
                         clock-names = "devfreq_clk";
                         clocks = <&apcs2>;
                         governor = "cpufreq";
                         operating-points-v2 = <&cci_opp_table>;
                         power-domains = <&cpr>;
                         power-domain-names = "cpr";
                         nvmem-cells = <&cpr_efuse_speedbin_pvs>;
                         nvmem-cell-names = "cpr_efuse_speedbin_pvs";
                 };

                 devfreq-cpufreq {
                         cci-cpufreq {
                                 target-dev = <&cci_cache>;
                                 cpu-to-dev-map-0 =
                                         <  200000  200000000 >,
                                         <  345600  200000000 >,
                                         <  400000  200000000 >,
                                         <  533330  297600000 >,
                                         <  800000  297600000 >,
                                         <  960000  297600000 >,
                                         < 1113600  297000000 >,
                                         < 1344000  595200000 >,
                                         < 1459200  595200000 >,
                                         < 1497600  595200000 >,
                                         < 1651200  595200000 >;
                                 cpu-to-dev-map-4 =
                                         <  200000 200000000 >,
                                         <  249600 200000000 >,
                                         <  499200 297600000 >,
                                         <  800000 297600000 >,
                                         <  998400 595200000 >,
                                         < 1113600 595200000 >;
                         };
                 };

         cci_opp_table: cci-opp-table {
                 compatible = "operating-points-v2";

                 opp-200000000 {
                         opp-hz = /bits/ 64 <200000000>;
                         opp-supported-hw = <0x3f>;
                         required-opps = <&cpr_opp3>;
                 };

                 opp-297600000 {
                         opp-hz = /bits/ 64 <297600000>;
                         opp-supported-hw = <0x3f>;
                         required-opps = <&cpr_opp12>;
                 };

                 opp-400000000-cpr14 {
                         opp-hz = /bits/ 64 <400000000>;
                         opp-supported-hw = <0x1>;
                         required-opps = <&cpr_opp14>;
                 };

                 opp-400000000-cpr15 {
                         opp-hz = /bits/ 64 <400000000>;
                         opp-supported-hw = <0x3e>;
                         required-opps = <&cpr_opp15>;
                 };

                 opp-595200000 {
                         opp-hz = /bits/ 64 <595200000>;
                         opp-supported-hw = <0x3f>;
                         required-opps = <&cpr_opp17>;
                 };
         };


         cpr_opp_table: cpr-opp-table {
                 compatible = "operating-points-v2-qcom-level";

                 cpr_opp1: opp1 {
                         opp-hz = /bits/ 64 <200000000>;
                         opp-level = <1>;
                         qcom,opp-fuse-level = <1>;
                 };
                 cpr_opp2: opp2 {
                         opp-hz = /bits/ 64 <345600000>;
                         opp-level = <2>;
                         qcom,opp-fuse-level = <1>;
                 };
                 cpr_opp3: opp3 {
                         opp-hz = /bits/ 64 <400000000>;
                         opp-level = <3>;
                         qcom,opp-fuse-level = <1>;
                 };
                 cpr_opp4: opp4 {
                         opp-hz = /bits/ 64 <422400000>;
                         opp-level = <4>;
                         qcom,opp-fuse-level = <2>;
                 };
                 cpr_opp5: opp5 {
                         opp-hz = /bits/ 64 <499200000>;
                         opp-level = <5>;
                         qcom,opp-fuse-level = <2>;
                 };
                 cpr_opp6: opp6 {
                         opp-hz = /bits/ 64 <533330000>;
                         opp-level = <6>;
                         qcom,opp-fuse-level = <2>;
                 };
                 cpr_opp7: opp7 {
                         opp-hz = /bits/ 64 <652800000>;
                         opp-level = <7>;
                         qcom,opp-fuse-level = <2>;
                 };
                 cpr_opp8: opp8 {
                         opp-hz = /bits/ 64 <729600000>;
                         opp-level = <8>;
                         qcom,opp-fuse-level = <2>;
                 };
                 cpr_opp9: opp9 {
                         opp-hz = /bits/ 64 <800000000>;
                         opp-level = <9>;
                         qcom,opp-fuse-level = <2>;
                 };
                 cpr_opp10: opp10 {
                         opp-hz = /bits/ 64 <806400000>;
                         opp-level = <10>;
                         qcom,opp-fuse-level = <2>;
                 };
                 cpr_opp11: opp11 {
                         opp-hz = /bits/ 64 <883200000>;
                         opp-level = <11>;
                         qcom,opp-fuse-level = <2>;
                 };
                 cpr_opp12: opp12 {
                         opp-hz = /bits/ 64 <960000000>;
                         opp-level = <12>;
                         qcom,opp-fuse-level = <2>;
                 };
         };

---
bod

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Dmitry Baryshkov]

On Wed, 1 Feb 2023 at 13:46, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 01/02/2023 11:32, Dmitry Baryshkov wrote:
> > On 01/02/2023 10:02, Jun Nie wrote:
> >> Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
> >> driver is introduced so that its freqency can be adjusted. And regulator
> >> associated with opp table can be also adjusted accordingly which is
> >> shared with cpu cluster.
> >>
> >> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> >> ---
> >>   drivers/devfreq/Kconfig    |   9 +++
> >>   drivers/devfreq/Makefile   |   1 +
> >>   drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 172 insertions(+)
> >>   create mode 100644 drivers/devfreq/qcom-cci.c
> >
> > Could you please describe in some additional details what are you trying
> > to achieve? Should the CCI frequency be scaled manually or does it
> > follow the cluster frequency? Do clusters vote on the CCI frequency?
> >
> > I'm inclined to ask if it is possible to shift this to the cpufreq OPP
> > tables?
> >
>
> Might not be so easy to just append CCI opps to the cluster frequency opps
>
>                  cci_cache: qcom,cci {
>                          compatible = "qcom,msm8939-cci";
>                          clock-names = "devfreq_clk";
>                          clocks = <&apcs2>;
>                          governor = "cpufreq";
>                          operating-points-v2 = <&cci_opp_table>;
>                          power-domains = <&cpr>;
>                          power-domain-names = "cpr";
>                          nvmem-cells = <&cpr_efuse_speedbin_pvs>;
>                          nvmem-cell-names = "cpr_efuse_speedbin_pvs";
>                  };
>
>                  devfreq-cpufreq {
>                          cci-cpufreq {
>                                  target-dev = <&cci_cache>;
>                                  cpu-to-dev-map-0 =
>                                          <  200000  200000000 >,
>                                          <  345600  200000000 >,
>                                          <  400000  200000000 >,
>                                          <  533330  297600000 >,
>                                          <  800000  297600000 >,
>                                          <  960000  297600000 >,
>                                          < 1113600  297000000 >,
>                                          < 1344000  595200000 >,
>                                          < 1459200  595200000 >,
>                                          < 1497600  595200000 >,
>                                          < 1651200  595200000 >;
>                                  cpu-to-dev-map-4 =
>                                          <  200000 200000000 >,
>                                          <  249600 200000000 >,
>                                          <  499200 297600000 >,
>                                          <  800000 297600000 >,
>                                          <  998400 595200000 >,
>                                          < 1113600 595200000 >;

These should map to existing opp entries.

I ended up doing the interconnect driver that maps a clock to the
interconnect. Then I can use it in the cpu opp tables.

>                          };
>                  };
>
>          cci_opp_table: cci-opp-table {
>                  compatible = "operating-points-v2";
>
>                  opp-200000000 {
>                          opp-hz = /bits/ 64 <200000000>;
>                          opp-supported-hw = <0x3f>;
>                          required-opps = <&cpr_opp3>;

And these should probably map to max(cpu's CPR opp, CCI's CPR opp).

>                  };
>
>                  opp-297600000 {
>                          opp-hz = /bits/ 64 <297600000>;
>                          opp-supported-hw = <0x3f>;
>                          required-opps = <&cpr_opp12>;
>                  };
>
>                  opp-400000000-cpr14 {
>                          opp-hz = /bits/ 64 <400000000>;
>                          opp-supported-hw = <0x1>;
>                          required-opps = <&cpr_opp14>;
>                  };
>
>                  opp-400000000-cpr15 {
>                          opp-hz = /bits/ 64 <400000000>;
>                          opp-supported-hw = <0x3e>;
>                          required-opps = <&cpr_opp15>;
>                  };
>
>                  opp-595200000 {
>                          opp-hz = /bits/ 64 <595200000>;
>                          opp-supported-hw = <0x3f>;
>                          required-opps = <&cpr_opp17>;
>                  };
>          };
>
>
>          cpr_opp_table: cpr-opp-table {
>                  compatible = "operating-points-v2-qcom-level";
>
>                  cpr_opp1: opp1 {
>                          opp-hz = /bits/ 64 <200000000>;
>                          opp-level = <1>;
>                          qcom,opp-fuse-level = <1>;
>                  };
>                  cpr_opp2: opp2 {
>                          opp-hz = /bits/ 64 <345600000>;
>                          opp-level = <2>;
>                          qcom,opp-fuse-level = <1>;
>                  };
>                  cpr_opp3: opp3 {
>                          opp-hz = /bits/ 64 <400000000>;
>                          opp-level = <3>;
>                          qcom,opp-fuse-level = <1>;
>                  };
>                  cpr_opp4: opp4 {
>                          opp-hz = /bits/ 64 <422400000>;
>                          opp-level = <4>;
>                          qcom,opp-fuse-level = <2>;
>                  };
>                  cpr_opp5: opp5 {
>                          opp-hz = /bits/ 64 <499200000>;
>                          opp-level = <5>;
>                          qcom,opp-fuse-level = <2>;
>                  };
>                  cpr_opp6: opp6 {
>                          opp-hz = /bits/ 64 <533330000>;
>                          opp-level = <6>;
>                          qcom,opp-fuse-level = <2>;
>                  };
>                  cpr_opp7: opp7 {
>                          opp-hz = /bits/ 64 <652800000>;
>                          opp-level = <7>;
>                          qcom,opp-fuse-level = <2>;
>                  };
>                  cpr_opp8: opp8 {
>                          opp-hz = /bits/ 64 <729600000>;
>                          opp-level = <8>;
>                          qcom,opp-fuse-level = <2>;
>                  };
>                  cpr_opp9: opp9 {
>                          opp-hz = /bits/ 64 <800000000>;
>                          opp-level = <9>;
>                          qcom,opp-fuse-level = <2>;
>                  };
>                  cpr_opp10: opp10 {
>                          opp-hz = /bits/ 64 <806400000>;
>                          opp-level = <10>;
>                          qcom,opp-fuse-level = <2>;
>                  };
>                  cpr_opp11: opp11 {
>                          opp-hz = /bits/ 64 <883200000>;
>                          opp-level = <11>;
>                          qcom,opp-fuse-level = <2>;
>                  };
>                  cpr_opp12: opp12 {
>                          opp-hz = /bits/ 64 <960000000>;
>                          opp-level = <12>;
>                          qcom,opp-fuse-level = <2>;
>                  };
>          };
>
> ---
> bod



-- 
With best wishes
Dmitry

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Bryan O'Donoghue]

On 01/02/2023 13:41, Dmitry Baryshkov wrote:
>>                           cci-cpufreq {
>>                                   target-dev = <&cci_cache>;
>>                                   cpu-to-dev-map-0 =
>>                                           <  200000  200000000 >,
>>                                           <  345600  200000000 >,
>>                                           <  400000  200000000 >,
>>                                           <  533330  297600000 >,
>>                                           <  800000  297600000 >,
>>                                           <  960000  297600000 >,
>>                                           < 1113600  297000000 >,
>>                                           < 1344000  595200000 >,
>>                                           < 1459200  595200000 >,
>>                                           < 1497600  595200000 >,
>>                                           < 1651200  595200000 >;
>>                                   cpu-to-dev-map-4 =
>>                                           <  200000 200000000 >,
>>                                           <  249600 200000000 >,
>>                                           <  499200 297600000 >,
>>                                           <  800000 297600000 >,
>>                                           <  998400 595200000 >,
>>                                           < 1113600 595200000 >;
> These should map to existing opp entries.
> 
> I ended up doing the interconnect driver that maps a clock to the
> interconnect. Then I can use it in the cpu opp tables.
> 

Can you point us at what it is you are proposing ?

---
bod

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Dmitry Baryshkov]

On 01/02/2023 16:45, Bryan O'Donoghue wrote:
> On 01/02/2023 13:41, Dmitry Baryshkov wrote:
>>>                           cci-cpufreq {
>>>                                   target-dev = <&cci_cache>;
>>>                                   cpu-to-dev-map-0 =
>>>                                           <  200000  200000000 >,
>>>                                           <  345600  200000000 >,
>>>                                           <  400000  200000000 >,
>>>                                           <  533330  297600000 >,
>>>                                           <  800000  297600000 >,
>>>                                           <  960000  297600000 >,
>>>                                           < 1113600  297000000 >,
>>>                                           < 1344000  595200000 >,
>>>                                           < 1459200  595200000 >,
>>>                                           < 1497600  595200000 >,
>>>                                           < 1651200  595200000 >;
>>>                                   cpu-to-dev-map-4 =
>>>                                           <  200000 200000000 >,
>>>                                           <  249600 200000000 >,
>>>                                           <  499200 297600000 >,
>>>                                           <  800000 297600000 >,
>>>                                           <  998400 595200000 >,
>>>                                           < 1113600 595200000 >;
>> These should map to existing opp entries.
>>
>> I ended up doing the interconnect driver that maps a clock to the
>> interconnect. Then I can use it in the cpu opp tables.
>>
> 
> Can you point us at what it is you are proposing ?

https://patchwork.kernel.org/project/linux-arm-msm/patch/20230120061417.2623751-9-dmitry.baryshkov@linaro.org/

> 
> ---
> bod

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Bryan O'Donoghue]

On 01/02/2023 14:58, Dmitry Baryshkov wrote:
> On 01/02/2023 16:45, Bryan O'Donoghue wrote:
>> On 01/02/2023 13:41, Dmitry Baryshkov wrote:
>>>>                           cci-cpufreq {
>>>>                                   target-dev = <&cci_cache>;
>>>>                                   cpu-to-dev-map-0 =
>>>>                                           <  200000  200000000 >,
>>>>                                           <  345600  200000000 >,
>>>>                                           <  400000  200000000 >,
>>>>                                           <  533330  297600000 >,
>>>>                                           <  800000  297600000 >,
>>>>                                           <  960000  297600000 >,
>>>>                                           < 1113600  297000000 >,
>>>>                                           < 1344000  595200000 >,
>>>>                                           < 1459200  595200000 >,
>>>>                                           < 1497600  595200000 >,
>>>>                                           < 1651200  595200000 >;
>>>>                                   cpu-to-dev-map-4 =
>>>>                                           <  200000 200000000 >,
>>>>                                           <  249600 200000000 >,
>>>>                                           <  499200 297600000 >,
>>>>                                           <  800000 297600000 >,
>>>>                                           <  998400 595200000 >,
>>>>                                           < 1113600 595200000 >;
>>> These should map to existing opp entries.
>>>
>>> I ended up doing the interconnect driver that maps a clock to the
>>> interconnect. Then I can use it in the cpu opp tables.
>>>
>>
>> Can you point us at what it is you are proposing ?
> 
> https://patchwork.kernel.org/project/linux-arm-msm/patch/20230120061417.2623751-9-dmitry.baryshkov@linaro.org/
> 

Is there no driver code too ?

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Dmitry Baryshkov]

On 01/02/2023 17:17, Bryan O'Donoghue wrote:
> On 01/02/2023 14:58, Dmitry Baryshkov wrote:
>> On 01/02/2023 16:45, Bryan O'Donoghue wrote:
>>> On 01/02/2023 13:41, Dmitry Baryshkov wrote:
>>>>>                           cci-cpufreq {
>>>>>                                   target-dev = <&cci_cache>;
>>>>>                                   cpu-to-dev-map-0 =
>>>>>                                           <  200000  200000000 >,
>>>>>                                           <  345600  200000000 >,
>>>>>                                           <  400000  200000000 >,
>>>>>                                           <  533330  297600000 >,
>>>>>                                           <  800000  297600000 >,
>>>>>                                           <  960000  297600000 >,
>>>>>                                           < 1113600  297000000 >,
>>>>>                                           < 1344000  595200000 >,
>>>>>                                           < 1459200  595200000 >,
>>>>>                                           < 1497600  595200000 >,
>>>>>                                           < 1651200  595200000 >;
>>>>>                                   cpu-to-dev-map-4 =
>>>>>                                           <  200000 200000000 >,
>>>>>                                           <  249600 200000000 >,
>>>>>                                           <  499200 297600000 >,
>>>>>                                           <  800000 297600000 >,
>>>>>                                           <  998400 595200000 >,
>>>>>                                           < 1113600 595200000 >;
>>>> These should map to existing opp entries.
>>>>
>>>> I ended up doing the interconnect driver that maps a clock to the
>>>> interconnect. Then I can use it in the cpu opp tables.
>>>>
>>>
>>> Can you point us at what it is you are proposing ?
>>
>> https://patchwork.kernel.org/project/linux-arm-msm/patch/20230120061417.2623751-9-dmitry.baryshkov@linaro.org/
>>
> Is there no driver code too ?

There are two parts, one is the 'CBF clock' driver, which just provides 
a clock, another part actually connects the clock and interconnect. 
Initially I implemented it as a part of the CBF driver (see 
https://patchwork.kernel.org/project/linux-arm-msm/patch/20230120061417.2623751-5-dmitry.baryshkov@linaro.org/), 
next revision will move the interconnect part to drivers/interconnect.

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Bryan O'Donoghue]

On 01/02/2023 17:12, Dmitry Baryshkov wrote:
> On 01/02/2023 17:17, Bryan O'Donoghue wrote:
>> On 01/02/2023 14:58, Dmitry Baryshkov wrote:
>>> On 01/02/2023 16:45, Bryan O'Donoghue wrote:
>>>> On 01/02/2023 13:41, Dmitry Baryshkov wrote:
>>>>>>                           cci-cpufreq {
>>>>>>                                   target-dev = <&cci_cache>;
>>>>>>                                   cpu-to-dev-map-0 =
>>>>>>                                           <  200000  200000000 >,
>>>>>>                                           <  345600  200000000 >,
>>>>>>                                           <  400000  200000000 >,
>>>>>>                                           <  533330  297600000 >,
>>>>>>                                           <  800000  297600000 >,
>>>>>>                                           <  960000  297600000 >,
>>>>>>                                           < 1113600  297000000 >,
>>>>>>                                           < 1344000  595200000 >,
>>>>>>                                           < 1459200  595200000 >,
>>>>>>                                           < 1497600  595200000 >,
>>>>>>                                           < 1651200  595200000 >;
>>>>>>                                   cpu-to-dev-map-4 =
>>>>>>                                           <  200000 200000000 >,
>>>>>>                                           <  249600 200000000 >,
>>>>>>                                           <  499200 297600000 >,
>>>>>>                                           <  800000 297600000 >,
>>>>>>                                           <  998400 595200000 >,
>>>>>>                                           < 1113600 595200000 >;
>>>>> These should map to existing opp entries.
>>>>>
>>>>> I ended up doing the interconnect driver that maps a clock to the
>>>>> interconnect. Then I can use it in the cpu opp tables.
>>>>>
>>>>
>>>> Can you point us at what it is you are proposing ?
>>>
>>> https://patchwork.kernel.org/project/linux-arm-msm/patch/20230120061417.2623751-9-dmitry.baryshkov@linaro.org/
>>>
>> Is there no driver code too ?
> 
> There are two parts, one is the 'CBF clock' driver, which just provides 
> a clock, another part actually connects the clock and interconnect. 
> Initially I implemented it as a part of the CBF driver (see 
> https://patchwork.kernel.org/project/linux-arm-msm/patch/20230120061417.2623751-5-dmitry.baryshkov@linaro.org/), next revision will move the interconnect part to drivers/interconnect.
> 

Ah so just to be clear - discussing with Dmitry - CCI has its own set of 
fuses.

We have fusebin settings for clusterX and CCI.

So, I think we agree this means a separate driver for cci is warranted.

---
bod

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Dmitry Baryshkov]

On 01/02/2023 19:16, Bryan O'Donoghue wrote:
> On 01/02/2023 17:12, Dmitry Baryshkov wrote:
>> On 01/02/2023 17:17, Bryan O'Donoghue wrote:
>>> On 01/02/2023 14:58, Dmitry Baryshkov wrote:
>>>> On 01/02/2023 16:45, Bryan O'Donoghue wrote:
>>>>> On 01/02/2023 13:41, Dmitry Baryshkov wrote:
>>>>>>>                           cci-cpufreq {
>>>>>>>                                   target-dev = <&cci_cache>;
>>>>>>>                                   cpu-to-dev-map-0 =
>>>>>>>                                           <  200000  200000000 >,
>>>>>>>                                           <  345600  200000000 >,
>>>>>>>                                           <  400000  200000000 >,
>>>>>>>                                           <  533330  297600000 >,
>>>>>>>                                           <  800000  297600000 >,
>>>>>>>                                           <  960000  297600000 >,
>>>>>>>                                           < 1113600  297000000 >,
>>>>>>>                                           < 1344000  595200000 >,
>>>>>>>                                           < 1459200  595200000 >,
>>>>>>>                                           < 1497600  595200000 >,
>>>>>>>                                           < 1651200  595200000 >;
>>>>>>>                                   cpu-to-dev-map-4 =
>>>>>>>                                           <  200000 200000000 >,
>>>>>>>                                           <  249600 200000000 >,
>>>>>>>                                           <  499200 297600000 >,
>>>>>>>                                           <  800000 297600000 >,
>>>>>>>                                           <  998400 595200000 >,
>>>>>>>                                           < 1113600 595200000 >;
>>>>>> These should map to existing opp entries.
>>>>>>
>>>>>> I ended up doing the interconnect driver that maps a clock to the
>>>>>> interconnect. Then I can use it in the cpu opp tables.
>>>>>>
>>>>>
>>>>> Can you point us at what it is you are proposing ?
>>>>
>>>> https://patchwork.kernel.org/project/linux-arm-msm/patch/20230120061417.2623751-9-dmitry.baryshkov@linaro.org/
>>>>
>>> Is there no driver code too ?
>>
>> There are two parts, one is the 'CBF clock' driver, which just 
>> provides a clock, another part actually connects the clock and 
>> interconnect. Initially I implemented it as a part of the CBF driver 
>> (see 
>> https://patchwork.kernel.org/project/linux-arm-msm/patch/20230120061417.2623751-5-dmitry.baryshkov@linaro.org/), next revision will move the interconnect part to drivers/interconnect.
>>
> 
> Ah so just to be clear - discussing with Dmitry - CCI has its own set of 
> fuses.
> 
> We have fusebin settings for clusterX and CCI.
> 
> So, I think we agree this means a separate driver for cci is warranted.

Yes.

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Jun Nie]

Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2023年2月1日周三 21:41写道：
>
> On Wed, 1 Feb 2023 at 13:46, Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> wrote:
> >
> > On 01/02/2023 11:32, Dmitry Baryshkov wrote:
> > > On 01/02/2023 10:02, Jun Nie wrote:
> > >> Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
> > >> driver is introduced so that its freqency can be adjusted. And regulator
> > >> associated with opp table can be also adjusted accordingly which is
> > >> shared with cpu cluster.
> > >>
> > >> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > >> ---
> > >>   drivers/devfreq/Kconfig    |   9 +++
> > >>   drivers/devfreq/Makefile   |   1 +
> > >>   drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++
> > >>   3 files changed, 172 insertions(+)
> > >>   create mode 100644 drivers/devfreq/qcom-cci.c
> > >
> > > Could you please describe in some additional details what are you trying
> > > to achieve? Should the CCI frequency be scaled manually or does it
> > > follow the cluster frequency? Do clusters vote on the CCI frequency?
> > >
> > > I'm inclined to ask if it is possible to shift this to the cpufreq OPP
> > > tables?
> > >
> >
> > Might not be so easy to just append CCI opps to the cluster frequency opps
> >
> >                  cci_cache: qcom,cci {
> >                          compatible = "qcom,msm8939-cci";
> >                          clock-names = "devfreq_clk";
> >                          clocks = <&apcs2>;
> >                          governor = "cpufreq";
> >                          operating-points-v2 = <&cci_opp_table>;
> >                          power-domains = <&cpr>;
> >                          power-domain-names = "cpr";
> >                          nvmem-cells = <&cpr_efuse_speedbin_pvs>;
> >                          nvmem-cell-names = "cpr_efuse_speedbin_pvs";
> >                  };
> >
> >                  devfreq-cpufreq {
> >                          cci-cpufreq {
> >                                  target-dev = <&cci_cache>;
> >                                  cpu-to-dev-map-0 =
> >                                          <  200000  200000000 >,
> >                                          <  345600  200000000 >,
> >                                          <  400000  200000000 >,
> >                                          <  533330  297600000 >,
> >                                          <  800000  297600000 >,
> >                                          <  960000  297600000 >,
> >                                          < 1113600  297000000 >,
> >                                          < 1344000  595200000 >,
> >                                          < 1459200  595200000 >,
> >                                          < 1497600  595200000 >,
> >                                          < 1651200  595200000 >;
> >                                  cpu-to-dev-map-4 =
> >                                          <  200000 200000000 >,
> >                                          <  249600 200000000 >,
> >                                          <  499200 297600000 >,
> >                                          <  800000 297600000 >,
> >                                          <  998400 595200000 >,
> >                                          < 1113600 595200000 >;
>
> These should map to existing opp entries.
>
> I ended up doing the interconnect driver that maps a clock to the
> interconnect. Then I can use it in the cpu opp tables.
>
> >                          };
> >                  };
> >
> >          cci_opp_table: cci-opp-table {
> >                  compatible = "operating-points-v2";
> >
> >                  opp-200000000 {
> >                          opp-hz = /bits/ 64 <200000000>;
> >                          opp-supported-hw = <0x3f>;
> >                          required-opps = <&cpr_opp3>;
>
> And these should probably map to max(cpu's CPR opp, CCI's CPR opp).

The plan is opp framework to handle it when CPU opp requires both cpr
power domain
opp and CCI opp. While CCI opp will also requires specific cpr opp. Because CPU
have a opp match table per pvs/speed, while CCI has another match
table, it seems
 impossible to write the cpr opp requirements in the code statically.

>
> >                  };
> >
> >                  opp-297600000 {
> >                          opp-hz = /bits/ 64 <297600000>;
> >                          opp-supported-hw = <0x3f>;
> >                          required-opps = <&cpr_opp12>;
> >                  };
> >
> >                  opp-400000000-cpr14 {
> >                          opp-hz = /bits/ 64 <400000000>;
> >                          opp-supported-hw = <0x1>;
> >                          required-opps = <&cpr_opp14>;
> >                  };
> >
> >                  opp-400000000-cpr15 {
> >                          opp-hz = /bits/ 64 <400000000>;
> >                          opp-supported-hw = <0x3e>;
> >                          required-opps = <&cpr_opp15>;
> >                  };
> >
> >                  opp-595200000 {
> >                          opp-hz = /bits/ 64 <595200000>;
> >                          opp-supported-hw = <0x3f>;
> >                          required-opps = <&cpr_opp17>;
> >                  };
> >          };
> >
> >
> >          cpr_opp_table: cpr-opp-table {
> >                  compatible = "operating-points-v2-qcom-level";
> >
> >                  cpr_opp1: opp1 {
> >                          opp-hz = /bits/ 64 <200000000>;
> >                          opp-level = <1>;
> >                          qcom,opp-fuse-level = <1>;
> >                  };
> >                  cpr_opp2: opp2 {
> >                          opp-hz = /bits/ 64 <345600000>;
> >                          opp-level = <2>;
> >                          qcom,opp-fuse-level = <1>;
> >                  };
> >                  cpr_opp3: opp3 {
> >                          opp-hz = /bits/ 64 <400000000>;
> >                          opp-level = <3>;
> >                          qcom,opp-fuse-level = <1>;
> >                  };
> >                  cpr_opp4: opp4 {
> >                          opp-hz = /bits/ 64 <422400000>;
> >                          opp-level = <4>;
> >                          qcom,opp-fuse-level = <2>;
> >                  };
> >                  cpr_opp5: opp5 {
> >                          opp-hz = /bits/ 64 <499200000>;
> >                          opp-level = <5>;
> >                          qcom,opp-fuse-level = <2>;
> >                  };
> >                  cpr_opp6: opp6 {
> >                          opp-hz = /bits/ 64 <533330000>;
> >                          opp-level = <6>;
> >                          qcom,opp-fuse-level = <2>;
> >                  };
> >                  cpr_opp7: opp7 {
> >                          opp-hz = /bits/ 64 <652800000>;
> >                          opp-level = <7>;
> >                          qcom,opp-fuse-level = <2>;
> >                  };
> >                  cpr_opp8: opp8 {
> >                          opp-hz = /bits/ 64 <729600000>;
> >                          opp-level = <8>;
> >                          qcom,opp-fuse-level = <2>;
> >                  };
> >                  cpr_opp9: opp9 {
> >                          opp-hz = /bits/ 64 <800000000>;
> >                          opp-level = <9>;
> >                          qcom,opp-fuse-level = <2>;
> >                  };
> >                  cpr_opp10: opp10 {
> >                          opp-hz = /bits/ 64 <806400000>;
> >                          opp-level = <10>;
> >                          qcom,opp-fuse-level = <2>;
> >                  };
> >                  cpr_opp11: opp11 {
> >                          opp-hz = /bits/ 64 <883200000>;
> >                          opp-level = <11>;
> >                          qcom,opp-fuse-level = <2>;
> >                  };
> >                  cpr_opp12: opp12 {
> >                          opp-hz = /bits/ 64 <960000000>;
> >                          opp-level = <12>;
> >                          qcom,opp-fuse-level = <2>;
> >                  };
> >          };
> >
> > ---
> > bod
>
>
>
> --
> With best wishes
> Dmitry

Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Jun Nie]

Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2023年2月1日周三 19:32写道：
>
> On 01/02/2023 10:02, Jun Nie wrote:
> > Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
> > driver is introduced so that its freqency can be adjusted. And regulator
> > associated with opp table can be also adjusted accordingly which is
> > shared with cpu cluster.
> >
> > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > ---
> >   drivers/devfreq/Kconfig    |   9 +++
> >   drivers/devfreq/Makefile   |   1 +
> >   drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++
> >   3 files changed, 172 insertions(+)
> >   create mode 100644 drivers/devfreq/qcom-cci.c
>
> Could you please describe in some additional details what are you trying
> to achieve? Should the CCI frequency be scaled manually or does it
> follow the cluster frequency? Do clusters vote on the CCI frequency?
>
> I'm inclined to ask if it is possible to shift this to the cpufreq OPP
> tables?
>
> --
> With best wishes
> Dmitry
>

The plan is to add CCI opp node as required-opp property in CPU opp, so that
CPU will vote CCI frequency when scaling CPU frequency.
The interconnect bandwidth side is not addressed yet. This patch only address
CCI frequency and power voltage requirement with help of device tree.

- Jun

Re: [PATCH 1/2] dt-bindings: interconnect: Add Qualcomm CCI dt-bindings

[Krzysztof Kozlowski]

On 01/02/2023 09:02, Jun Nie wrote:
> Add devicetree binding of Qualcomm CCI on MSM8939.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

Subject: drop second/last, redundant "dt-bindings". The "dt-bindings"
prefix is already stating that these are bindings.

> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  .../bindings/interconnect/qcom,cci.yaml       | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
> new file mode 100644
> index 000000000000..100c440ba220
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interconnect/mediatek,cci.yaml#

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Cache Coherent Interconnect (CCI) frequency and voltage scaling
> +
> +maintainers:
> +  - Jun Nie <jun.nie@linaro.org>
> +
> +description: |
> +  Qualcomm Cache Coherent Interconnect (CCI) is a hardware engine used by
> +  MSM8939. The driver is to scale its frequency and adjust the voltage in
> +  hardware accordingly. The voltage provider is modeled as power domain on
> +  MSM8939, so power domain dts node is required.

Don't describe other bindings, but the hardware. Last sentence is not
really related. What's more - it does not fit what you wrote below.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,msm8939-cci
> +
> +  clocks:
> +    maxItems: 1
> +
> +  operating-points-v2: true
> +  opp-table:
> +    type: object
> +
> +required:
> +  - compatible
> +  - clocks
> +  - operating-points-v2
> +  - nvmem-cells

?? You cannot require properties which are not present.

> +  - power-domains

Same here.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    cci: cci {
> +        compatible = "qcom,msm8939-cci";
> +	clocks = <&apcs2>;

Messed indentation.

Use 4 spaces for example indentation.

> +	operating-points-v2 = <&cci_opp_table>;
> +	power-domains = <&cpr>;
> +	nvmem-cells = <&cpr_efuse_speedbin_pvs>;

How does it pass testing?

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: interconnect: Add Qualcomm CCI dt-bindings

[Jun Nie]

> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - qcom,msm8939-cci
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  operating-points-v2: true
> > +  opp-table:
> > +    type: object
> > +
> > +required:
> > +  - compatible
> > +  - clocks
> > +  - operating-points-v2
> > +  - nvmem-cells
>
> ?? You cannot require properties which are not present.
>
> > +  - power-domains
>
> Same here.
>

So the properties should be added later, after cpr and fuse nodes are
available in mainline, right?

- Jun

Re: [PATCH 1/2] dt-bindings: interconnect: Add Qualcomm CCI dt-bindings

[Krzysztof Kozlowski]

On 02/02/2023 10:29, Jun Nie wrote:
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - qcom,msm8939-cci
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  operating-points-v2: true
>>> +  opp-table:
>>> +    type: object
>>> +
>>> +required:
>>> +  - compatible
>>> +  - clocks
>>> +  - operating-points-v2
>>> +  - nvmem-cells
>>
>> ?? You cannot require properties which are not present.
>>
>>> +  - power-domains
>>
>> Same here.
>>
> 
> So the properties should be added later, after cpr and fuse nodes are
> available in mainline, right?

No, binding should be complete, so why would you skip some properties? I
don't see here dependency on other bindings. Unless I missed here
something and there is a dependency? But then what kind? Do you
reference other schema?

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: interconnect: Add Qualcomm CCI dt-bindings

[Jun Nie]

Krzysztof Kozlowski <krzk@kernel.org> 于2023年2月2日周四 17:42写道：
>
> On 02/02/2023 10:29, Jun Nie wrote:
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - qcom,msm8939-cci
> >>> +
> >>> +  clocks:
> >>> +    maxItems: 1
> >>> +
> >>> +  operating-points-v2: true
> >>> +  opp-table:
> >>> +    type: object
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - clocks
> >>> +  - operating-points-v2
> >>> +  - nvmem-cells
> >>
> >> ?? You cannot require properties which are not present.
> >>
> >>> +  - power-domains
> >>
> >> Same here.
> >>
> >
> > So the properties should be added later, after cpr and fuse nodes are
> > available in mainline, right?
>
> No, binding should be complete, so why would you skip some properties? I
> don't see here dependency on other bindings. Unless I missed here
> something and there is a dependency? But then what kind? Do you
> reference other schema?
>
> Best regards,
> Krzysztof
>
Sorry, it is a copy/modified error with overlooking some added properties.

+        cci_opp3: opp-cpr14-400000000 {
+            opp-hz = /bits/ 64 <400000000>;
+            opp-supported-hw = <0x1>;
+            required-opps = <&cpr_opp14>;
+        };
+
+        cci_opp4: opp-cpr15-400000000 {

Documentation/devicetree/bindings/interconnect/qcom,cci.example.dtb:
opp-table-cci: Unevaluated properties are not allowed
('opp-cpr14-400000000', 'opp-cpr15-400000000' were unexpected)

Do you know how to fix this dts check warning?
The cci_opp3 and cci_opp4 have the same frequency but with different
requirements to
power domain. So the name of the 2 opp should be identical or with a
different tag, but there
are warnings for both cases for the 2 lines. Thanks!


+            opp-hz = /bits/ 64 <400000000>;
+            opp-supported-hw = <0x3e>;
+            required-opps = <&cpr_opp15>;
+        };

Re: [PATCH 1/2] dt-bindings: interconnect: Add Qualcomm CCI dt-bindings

[Krzysztof Kozlowski]

On 03/02/2023 04:45, Jun Nie wrote:
> Krzysztof Kozlowski <krzk@kernel.org> 于2023年2月2日周四 17:42写道：
>>
>> On 02/02/2023 10:29, Jun Nie wrote:
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - qcom,msm8939-cci
>>>>> +
>>>>> +  clocks:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  operating-points-v2: true
>>>>> +  opp-table:
>>>>> +    type: object
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +  - clocks
>>>>> +  - operating-points-v2
>>>>> +  - nvmem-cells
>>>>
>>>> ?? You cannot require properties which are not present.
>>>>
>>>>> +  - power-domains
>>>>
>>>> Same here.
>>>>
>>>
>>> So the properties should be added later, after cpr and fuse nodes are
>>> available in mainline, right?
>>
>> No, binding should be complete, so why would you skip some properties? I
>> don't see here dependency on other bindings. Unless I missed here
>> something and there is a dependency? But then what kind? Do you
>> reference other schema?
>>
>> Best regards,
>> Krzysztof
>>
> Sorry, it is a copy/modified error with overlooking some added properties.
> 
> +        cci_opp3: opp-cpr14-400000000 {
> +            opp-hz = /bits/ 64 <400000000>;
> +            opp-supported-hw = <0x1>;
> +            required-opps = <&cpr_opp14>;
> +        };
> +
> +        cci_opp4: opp-cpr15-400000000 {

This does not answer my concerns at all. Please include all relevant
properties in your bindings.

> Documentation/devicetree/bindings/interconnect/qcom,cci.example.dtb:
> opp-table-cci: Unevaluated properties are not allowed
> ('opp-cpr14-400000000', 'opp-cpr15-400000000' were unexpected)
> 
> Do you know how to fix this dts check warning?

Don't add properties which are not allowed or do not name properties in
a way it is not allowed.

> The cci_opp3 and cci_opp4 have the same frequency but with different
> requirements to

So you used the same node names... how do you expect such code to
compile? In current form bindings do not allow such setup. If you think
that OS should allow and support it, then change the bindings.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: interconnect: Add Qualcomm CCI dt-bindings

[Chanwoo Choi]

Hi,

In order to track the changes of dt-binding, 
please add the info as following:

diff --git a/MAINTAINERS b/MAINTAINERS
index 7f86d02cb427..e13e61218987 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6047,6 +6047,7 @@ S:        Maintained
 T:     git git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git
 F:     Documentation/devicetree/bindings/devfreq/
 F:     Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
+F:     Documentation/devicetree/bindings/interconnect/qcom,cci.yam
 F:     drivers/devfreq/
 F:     include/linux/devfreq.h
 F:     include/trace/events/devfreq.h


On 23. 2. 1. 17:02, Jun Nie wrote:
> Add devicetree binding of Qualcomm CCI on MSM8939.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  .../bindings/interconnect/qcom,cci.yaml       | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
> new file mode 100644
> index 000000000000..100c440ba220
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interconnect/mediatek,cci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Cache Coherent Interconnect (CCI) frequency and voltage scaling
> +
> +maintainers:
> +  - Jun Nie <jun.nie@linaro.org>
> +
> +description: |
> +  Qualcomm Cache Coherent Interconnect (CCI) is a hardware engine used by
> +  MSM8939. The driver is to scale its frequency and adjust the voltage in
> +  hardware accordingly. The voltage provider is modeled as power domain on
> +  MSM8939, so power domain dts node is required.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,msm8939-cci
> +
> +  clocks:
> +    maxItems: 1
> +
> +  operating-points-v2: true
> +  opp-table:
> +    type: object
> +
> +required:
> +  - compatible
> +  - clocks
> +  - operating-points-v2
> +  - nvmem-cells
> +  - power-domains
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    cci: cci {
> +        compatible = "qcom,msm8939-cci";
> +	clocks = <&apcs2>;
> +	operating-points-v2 = <&cci_opp_table>;
> +	power-domains = <&cpr>;
> +	nvmem-cells = <&cpr_efuse_speedbin_pvs>;
> +    };
> +
> +    cci_opp_table: cci-opp-table {
> +	compatible = "operating-points-v2";
> +
> +	cci_opp1: opp-200000000 {
> +	       opp-hz = /bits/ 64 <200000000>;
> +	       opp-supported-hw = <0x3f>;
> +	       required-opps = <&cpr_opp3>;
> +	};
> +
> +	cci_opp2: opp-297600000 {
> +	       opp-hz = /bits/ 64 <297600000>;
> +	       opp-supported-hw = <0x3f>;
> +	       required-opps = <&cpr_opp12>;
> +	};
> +
> +	cci_opp3: opp-400000000 {
> +	       opp-hz = /bits/ 64 <400000000>;
> +	       opp-supported-hw = <0x1>;
> +	       required-opps = <&cpr_opp14>;
> +	};
> +
> +	cci_opp4: opp-400000000 {
> +	       opp-hz = /bits/ 64 <400000000>;
> +	       opp-supported-hw = <0x3e>;
> +	       required-opps = <&cpr_opp15>;
> +	};
> +
> +	cci_opp5: opp-595200000 {
> +	       opp-hz = /bits/ 64 <595200000>;
> +	       opp-supported-hw = <0x3f>;
> +	       required-opps = <&cpr_opp17>;
> +	};
> +    };

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH 1/2] dt-bindings: interconnect: Add Qualcomm CCI dt-bindings

[Rob Herring]

On Wed, 01 Feb 2023 16:02:26 +0800, Jun Nie wrote:
> Add devicetree binding of Qualcomm CCI on MSM8939.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  .../bindings/interconnect/qcom,cci.yaml       | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/interconnect/qcom,cci.yaml:43:1: [error] syntax error: found character '\t' that cannot start any token (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/interconnect/qcom,cci.example.dts'
Documentation/devicetree/bindings/interconnect/qcom,cci.yaml:43:1: found character '\t' that cannot start any token
make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/interconnect/qcom,cci.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/interconnect/qcom,cci.yaml:43:1: found character '\t' that cannot start any token
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml: ignoring, error parsing file
make: *** [Makefile:1508: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230201080227.473547-1-jun.nie@linaro.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.