[PATCH 0/3] devfreq: mediatek: introduce MTK cci devfreq

[PATCH 0/3] devfreq: mediatek: introduce MTK cci devfreq

[Tim Chang]

The Cache Coherent Interconnect (CCI) is the management of cache
coherency by hardware. CCI DEVFREQ is DVFS driver for power saving by
scaling clock frequency and supply voltage of CCI. CCI uses the same
input clock source and power rail as LITTLE CPUs on Mediatek SoCs.

Jia-Wei Chang (3):
  dt-bindings: devfreq: mediatek: add mtk cci devfreq dt-bindings
  devfreq: mediatek: add mt8183 cci devfreq driver
  devfreq: mediatek: add platform data to support mt8186

 .../devicetree/bindings/devfreq/mtk-cci.yaml  |  73 +++
 drivers/devfreq/Kconfig                       |  11 +-
 drivers/devfreq/Makefile                      |   2 +-
 drivers/devfreq/mtk-cci-devfreq.c             | 481 ++++++++++++++++++
 4 files changed, 565 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
 create mode 100644 drivers/devfreq/mtk-cci-devfreq.c

-- 
2.18.0

[PATCH 1/3] dt-bindings: devfreq: mediatek: add mtk cci devfreq dt-bindings

[Tim Chang]

add devicetree binding of mtk cci devfreq on MediaTek SoC.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
---
 .../devicetree/bindings/devfreq/mtk-cci.yaml  | 73 +++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/devfreq/mtk-cci.yaml

diff --git a/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
new file mode 100644
index 000000000000..e64ac4c56758
--- /dev/null
+++ b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/devfreq/mtk-cci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mediatek Cache Coherent Interconnect (CCI) Devfreq driver Device Tree Bindings
+
+maintainers:
+  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
+
+description: |
+  This module is used to create CCI DEVFREQ.
+  The performance will depend on both CCI frequency and CPU frequency.
+  For MT8186, CCI co-buck with Little core.
+  Contain CCI opp table for voltage and frequency scaling.
+
+properties:
+  compatible:
+    const: "mediatek,mt8186-cci"
+
+  clocks:
+    items:
+      - description:
+          The first one is the multiplexer for clock input of CPU cluster.
+      - description:
+          The other is used as an intermediate clock source when the original
+          CPU is under transition and not stable yet.
+
+  clock-names:
+    items:
+      - const: "cci"
+      - const: "intermediate"
+
+  operating-points-v2:
+    description:
+      For details, please refer to
+      Documentation/devicetree/bindings/opp/opp-v2.yaml
+
+  opp-table: true
+
+  proc-supply:
+    description:
+      Phandle of the regulator for CCI that provides the supply voltage.
+
+  sram-supply:
+    description:
+      Phandle of the regulator for sram of CCI that provides the supply
+      voltage. When present, the cci devfreq driver needs to do
+      "voltage tracking" to step by step scale up/down Vproc and Vsram to fit
+      SoC specific needs. When absent, the voltage scaling flow is handled by
+      hardware, hence no software "voltage tracking" is needed.
+
+required:
+  - compatible
+  - clocks
+  - clock-names
+  - operating-points-v2
+  - proc-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/mt8186-clk.h>
+    cci: cci {
+      compatible = "mediatek,mt8186-cci";
+      clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys CLK_APMIXED_MAINPLL>;
+      clock-names = "cci", "intermediate";
+      operating-points-v2 = <&cci_opp>;
+      proc-supply = <&mt6358_vproc12_reg>;
+      sram-supply = <&mt6358_vsram_proc12_reg>;
+    };
-- 
2.18.0

[PATCH 2/3] devfreq: mediatek: add mt8183 cci devfreq driver

[Tim Chang]

This adds a devfreq driver for the Cache Coherent Interconnect (CCI) of
the Mediatek MT8183.

On mt8183 the cci is supplied by the same regulator as the little cores.
The driver is notified when the regulator voltage changes (driven by
cpufreq) and adjusts the cci frequency to the maximum possible value.

Add need_voltage_tracking variable to platforma data. if true, it
indicates soc is required to realize the voltage tracking between
voltage of sram and voltage of cci by software approach. otherwise, the
voltage tracking is realized by hardware appraoch.

Add the notifier to cci so that it could react after svs driver changes
opp table of cci.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
---
 drivers/devfreq/Kconfig           |  11 +-
 drivers/devfreq/Makefile          |   2 +-
 drivers/devfreq/mtk-cci-devfreq.c | 471 ++++++++++++++++++++++++++++++
 3 files changed, 482 insertions(+), 2 deletions(-)
 create mode 100644 drivers/devfreq/mtk-cci-devfreq.c

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 1ec36ae93f31..a6be3c6b5691 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -110,7 +110,7 @@ config ARM_IMX8M_DDRC_DEVFREQ
 
 config ARM_MT8183_CCI_DEVFREQ
 	tristate "MT8183 CCI DEVFREQ Driver"
-	depends on ARM_MEDIATEK_CPUFREQ
+	depends on OF && ARM_MEDIATEK_CPUFREQ
 	help
 		This adds a devfreq driver for Cache Coherent Interconnect
 		of Mediatek MT8183, which is shared the same regulator
@@ -130,6 +130,15 @@ config ARM_TEGRA_DEVFREQ
 	  It reads ACTMON counters of memory controllers and adjusts the
 	  operating frequencies and voltages with OPP support.
 
+config ARM_MEDIATEK_CCI_DEVFREQ
+	tristate "MEDIATEK CCI DEVFREQ Driver"
+	depends on OF && ARM_MEDIATEK_CPUFREQ
+	help
+	  This adds a devfreq driver for Mediatek Cache Coherent Interconnect
+	  which is shared the same regulator with cpu cluster. It can track
+	  buck voltage and update a proper CCI frequency. Use notification
+	  to get regulator status.
+
 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 991ef7740759..0493516a16f2 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -11,7 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 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_MT8183_CCI_DEVFREQ)	+= mt8183-cci-devfreq.o
+obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ)	+= mtk-cci-devfreq.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
 
diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-cci-devfreq.c
new file mode 100644
index 000000000000..986f34689f5c
--- /dev/null
+++ b/drivers/devfreq/mtk-cci-devfreq.c
@@ -0,0 +1,471 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 MediaTek Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/regulator/consumer.h>
+
+struct mtk_ccifreq_platform_data;
+
+struct mtk_ccifreq_drv {
+	struct device *cci_dev;
+	struct devfreq *devfreq;
+	struct regulator *proc_reg;
+	struct regulator *sram_reg;
+	struct clk *cci_clk;
+	struct clk *inter_clk;
+	int inter_voltage;
+	int old_voltage;
+	unsigned long old_freq;
+	struct mutex lock;  /* avoid notify and policy race condition */
+	struct notifier_block opp_nb;
+	const struct mtk_ccifreq_platform_data *soc_data;
+};
+
+struct mtk_ccifreq_platform_data {
+	int min_volt_shift;
+	int max_volt_shift;
+	int proc_max_volt;
+	int sram_min_volt;
+	int sram_max_volt;
+	bool need_voltage_tracking;
+};
+
+static int mtk_ccifreq_voltage_tracking(struct mtk_ccifreq_drv *drv, int new_voltage)
+{
+	const struct mtk_ccifreq_platform_data *soc_data = drv->soc_data;
+	struct regulator *proc_reg = drv->proc_reg;
+	struct regulator *sram_reg = drv->sram_reg;
+	int old_voltage, old_vsram, new_vsram, vsram, voltage, ret;
+
+	old_voltage = regulator_get_voltage(proc_reg);
+	if (old_voltage < 0) {
+		pr_err("%s: invalid vproc value: %d\n", __func__, old_voltage);
+		return old_voltage;
+	}
+
+	old_vsram = regulator_get_voltage(sram_reg);
+	if (old_vsram < 0) {
+		pr_err("%s: invalid vsram value: %d\n", __func__, old_vsram);
+		return old_vsram;
+	}
+
+	new_vsram = clamp(new_voltage + soc_data->min_volt_shift,
+			  soc_data->sram_min_volt, soc_data->sram_max_volt);
+
+	do {
+		if (old_voltage <= new_voltage) {
+			vsram = clamp(old_voltage + soc_data->max_volt_shift,
+				      soc_data->sram_min_volt, new_vsram);
+			ret = regulator_set_voltage(sram_reg, vsram,
+						    soc_data->sram_max_volt);
+			if (ret)
+				return ret;
+
+			if (vsram == soc_data->sram_max_volt ||
+			    new_vsram == soc_data->sram_min_volt)
+				voltage = new_voltage;
+			else
+				voltage = vsram - soc_data->min_volt_shift;
+
+			ret = regulator_set_voltage(proc_reg, voltage,
+						    soc_data->proc_max_volt);
+			if (ret) {
+				regulator_set_voltage(sram_reg, old_vsram,
+						      soc_data->sram_max_volt);
+				return ret;
+			}
+		} else if (old_voltage > new_voltage) {
+			voltage = max(new_voltage,
+				    old_vsram - soc_data->max_volt_shift);
+			ret = regulator_set_voltage(proc_reg, voltage,
+						    soc_data->proc_max_volt);
+			if (ret)
+				return ret;
+
+			if (voltage == new_voltage)
+				vsram = new_vsram;
+			else
+				vsram = max(new_vsram,
+					    voltage + soc_data->min_volt_shift);
+
+			ret = regulator_set_voltage(sram_reg, vsram,
+						    soc_data->sram_max_volt);
+			if (ret) {
+				regulator_set_voltage(proc_reg, old_voltage,
+						      soc_data->proc_max_volt);
+				return ret;
+			}
+		}
+
+		old_voltage = voltage;
+		old_vsram = vsram;
+	} while (voltage != new_voltage || vsram != new_vsram);
+
+	return 0;
+}
+
+static int mtk_ccifreq_set_voltage(struct mtk_ccifreq_drv *drv, int voltage)
+{
+	int ret;
+
+	if (drv->soc_data->need_voltage_tracking)
+		ret = mtk_ccifreq_voltage_tracking(drv, voltage);
+	else
+		ret = regulator_set_voltage(drv->proc_reg, voltage,
+					    drv->soc_data->proc_max_volt);
+
+	if (!ret)
+		drv->old_voltage = voltage;
+
+	return ret;
+}
+
+static int mtk_ccifreq_target(struct device *dev, unsigned long *freq,
+			      u32 flags)
+{
+	struct mtk_ccifreq_drv *drv = dev_get_drvdata(dev);
+	struct clk *cci_pll = clk_get_parent(drv->cci_clk);
+	struct dev_pm_opp *opp;
+	unsigned long opp_rate;
+	int voltage, old_voltage, inter_voltage, target_voltage, ret;
+
+	if (!drv)
+		return -EINVAL;
+
+	if (drv->old_freq == *freq)
+		return 0;
+
+	inter_voltage = drv->inter_voltage;
+
+	opp_rate = *freq;
+	opp = devfreq_recommended_opp(dev, &opp_rate, 1);
+	if (IS_ERR(opp)) {
+		pr_err("cci: failed to find opp for freq: %ld\n", opp_rate);
+		return PTR_ERR(opp);
+	}
+	voltage = dev_pm_opp_get_voltage(opp);
+	dev_pm_opp_put(opp);
+
+	old_voltage = drv->old_voltage;
+	if (old_voltage == 0)
+		old_voltage = regulator_get_voltage(drv->proc_reg);
+	if (old_voltage < 0) {
+		pr_err("cci: invalid vproc value: %d\n", old_voltage);
+		return old_voltage;
+	}
+
+	mutex_lock(&drv->lock);
+
+	/* scale up: set voltage first then freq. */
+	target_voltage = max(inter_voltage, voltage);
+	if (old_voltage <= target_voltage) {
+		ret = mtk_ccifreq_set_voltage(drv, target_voltage);
+		if (ret) {
+			pr_err("cci: failed to scale up voltage\n");
+			mtk_ccifreq_set_voltage(drv, old_voltage);
+			mutex_unlock(&drv->lock);
+			return ret;
+		}
+	}
+
+	/* switch the cci clock to intermediate clock source. */
+	ret = clk_set_parent(drv->cci_clk, drv->inter_clk);
+	if (ret) {
+		pr_err("cci: failed to re-parent cci clock\n");
+		mtk_ccifreq_set_voltage(drv, old_voltage);
+		WARN_ON(1);
+		mutex_unlock(&drv->lock);
+		return ret;
+	}
+
+	/* set the original clock to target rate. */
+	ret = clk_set_rate(cci_pll, *freq);
+	if (ret) {
+		pr_err("cci: failed to set cci pll rate: %d\n", ret);
+		clk_set_parent(drv->cci_clk, cci_pll);
+		mtk_ccifreq_set_voltage(drv, old_voltage);
+		mutex_unlock(&drv->lock);
+		return ret;
+	}
+
+	/* switch the cci clock back to the original clock source. */
+	ret = clk_set_parent(drv->cci_clk, cci_pll);
+	if (ret) {
+		pr_err("cci: failed to re-parent cci clock\n");
+		mtk_ccifreq_set_voltage(drv, inter_voltage);
+		WARN_ON(1);
+		mutex_unlock(&drv->lock);
+		return ret;
+	}
+
+	/*
+	 * If the new voltage is lower than the intermediate voltage or the
+	 * original voltage, scale down to the new voltage.
+	 */
+	if (voltage < inter_voltage || voltage < old_voltage) {
+		ret = mtk_ccifreq_set_voltage(drv, voltage);
+		if (ret) {
+			pr_err("cci: failed to scale down voltage\n");
+			WARN_ON(1);
+			mutex_unlock(&drv->lock);
+			return ret;
+		}
+	}
+
+	drv->old_freq = *freq;
+	mutex_unlock(&drv->lock);
+
+	return 0;
+}
+
+static int mtk_ccifreq_opp_notifier(struct notifier_block *nb,
+				    unsigned long event, void *data)
+{
+	struct dev_pm_opp *opp = data;
+	struct mtk_ccifreq_drv *drv;
+	unsigned long freq, volt;
+
+	drv = container_of(nb, struct mtk_ccifreq_drv, opp_nb);
+
+	if (event == OPP_EVENT_ADJUST_VOLTAGE) {
+		freq = dev_pm_opp_get_freq(opp);
+
+		mutex_lock(&drv->lock);
+		/* current opp item is changed */
+		if (freq == drv->old_freq) {
+			volt = dev_pm_opp_get_voltage(opp);
+			mtk_ccifreq_set_voltage(drv, volt);
+		}
+		mutex_unlock(&drv->lock);
+	}
+
+	return 0;
+}
+
+static struct devfreq_dev_profile mtk_ccifreq_profile = {
+	.target = mtk_ccifreq_target,
+};
+
+static int mtk_ccifreq_probe(struct platform_device *pdev)
+{
+	struct device *cci_dev = &pdev->dev;
+	struct mtk_ccifreq_drv *drv;
+	struct devfreq_passive_data *passive_data;
+	struct dev_pm_opp *opp;
+	unsigned long rate, opp_volt;
+	int ret;
+
+	drv = devm_kzalloc(cci_dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	drv->cci_dev = cci_dev;
+	drv->soc_data = (const struct mtk_ccifreq_platform_data *)
+				of_device_get_match_data(&pdev->dev);
+	mutex_init(&drv->lock);
+	platform_set_drvdata(pdev, drv);
+
+	drv->cci_clk = devm_clk_get(cci_dev, "cci");
+	if (IS_ERR(drv->cci_clk)) {
+		ret = PTR_ERR(drv->cci_clk);
+		return dev_err_probe(cci_dev, ret,
+				     "cci: failed to get cci clk: %d\n",
+				     ret);
+	}
+
+	drv->inter_clk = devm_clk_get(cci_dev, "intermediate");
+	if (IS_ERR(drv->inter_clk)) {
+		ret = PTR_ERR(drv->inter_clk);
+		return dev_err_probe(cci_dev, ret,
+				     "cci: failed to get intermediate clk: %d\n",
+				     ret);
+	}
+
+	if (drv->soc_data->need_voltage_tracking) {
+		drv->sram_reg = regulator_get_optional(cci_dev, "sram");
+		if (IS_ERR_OR_NULL(drv->sram_reg)) {
+			ret = PTR_ERR(drv->sram_reg);
+			return dev_err_probe(cci_dev, ret,
+					     "cci: failed to get sram regulator: %d\n",
+					     ret);
+		}
+
+		ret = regulator_enable(drv->sram_reg);
+		if (ret) {
+			dev_warn(cci_dev,
+				 "cci: failed to enable sram regulator\n");
+			return ret;
+		}
+	}
+
+	drv->proc_reg = devm_regulator_get_optional(cci_dev, "proc");
+	if (IS_ERR(drv->proc_reg)) {
+		ret = PTR_ERR(drv->proc_reg);
+		dev_err_probe(cci_dev, ret,
+			      "cci: failed to get proc regulator: %d\n",
+			      ret);
+		goto out_disable_sram_reg;
+	}
+
+	ret = regulator_enable(drv->proc_reg);
+	if (ret) {
+		dev_warn(cci_dev, "cci: failed to enable proc regulator\n");
+		goto out_disable_sram_reg;
+	}
+
+	ret = clk_prepare_enable(drv->cci_clk);
+	if (ret)
+		goto out_disable_proc_reg;
+
+	ret = clk_prepare_enable(drv->inter_clk);
+	if (ret)
+		goto out_disable_cci_clk;
+
+	ret = dev_pm_opp_of_add_table(cci_dev);
+	if (ret) {
+		dev_warn(cci_dev, "cci: failed to add opp table: %d\n", ret);
+		goto out_disable_inter_clk;
+	}
+
+	rate = clk_get_rate(drv->inter_clk);
+	opp = dev_pm_opp_find_freq_ceil(cci_dev, &rate);
+	if (IS_ERR(opp)) {
+		ret = PTR_ERR(opp);
+		dev_err(cci_dev, "cci: failed to get intermediate opp: %d\n",
+			ret);
+		goto out_remove_opp_table;
+	}
+	drv->inter_voltage = dev_pm_opp_get_voltage(opp);
+	dev_pm_opp_put(opp);
+
+	rate = U32_MAX;
+	opp = dev_pm_opp_find_freq_floor(drv->cci_dev, &rate);
+	if (IS_ERR(opp)) {
+		pr_err("failed to get opp\n");
+		ret = PTR_ERR(opp);
+		goto out_remove_opp_table;
+	}
+
+	opp_volt = dev_pm_opp_get_voltage(opp);
+	dev_pm_opp_put(opp);
+	ret = mtk_ccifreq_set_voltage(drv, opp_volt);
+	if (ret) {
+		pr_err("failed to scale to highest voltage %lu in proc_reg\n",
+		       opp_volt);
+		goto out_remove_opp_table;
+	}
+
+	passive_data = devm_kzalloc(cci_dev, sizeof(struct devfreq_passive_data), GFP_KERNEL);
+	if (!passive_data) {
+		ret = -ENOMEM;
+		goto out_remove_opp_table;
+	}
+
+	passive_data->parent_type = CPUFREQ_PARENT_DEV;
+	drv->devfreq = devm_devfreq_add_device(cci_dev,
+					       &mtk_ccifreq_profile,
+					       DEVFREQ_GOV_PASSIVE,
+					       passive_data);
+	if (IS_ERR(drv->devfreq)) {
+		ret = -EPROBE_DEFER;
+		dev_err(cci_dev, "cci: failed to add devfreq device: %d\n",
+			PTR_ERR(drv->devfreq));
+		goto out_remove_opp_table;
+	}
+
+	drv->opp_nb.notifier_call = mtk_ccifreq_opp_notifier;
+	ret = dev_pm_opp_register_notifier(cci_dev, &drv->opp_nb);
+	if (ret) {
+		dev_warn(cci_dev, "cci: failed to register opp notifier: %d\n",
+			 ret);
+		goto out_remove_devfreq_device;
+	}
+
+	return 0;
+
+out_remove_devfreq_device:
+	devm_devfreq_remove_device(cci_dev, drv->devfreq);
+
+out_remove_opp_table:
+	dev_pm_opp_of_remove_table(cci_dev);
+
+out_disable_inter_clk:
+	clk_disable_unprepare(drv->inter_clk);
+
+out_disable_cci_clk:
+	clk_disable_unprepare(drv->cci_clk);
+
+out_disable_proc_reg:
+	regulator_disable(drv->proc_reg);
+
+out_disable_sram_reg:
+	if (drv->soc_data->need_voltage_tracking)
+		regulator_disable(drv->sram_reg);
+
+	return ret;
+}
+
+static int mtk_ccifreq_remove(struct platform_device *pdev)
+{
+	struct device *cci_dev = &pdev->dev;
+	struct mtk_ccifreq_drv *drv;
+
+	drv = platform_get_drvdata(pdev);
+
+	dev_pm_opp_unregister_notifier(cci_dev, &drv->opp_nb);
+	dev_pm_opp_of_remove_table(cci_dev);
+	regulator_disable(drv->proc_reg);
+	if (!IS_ERR(drv->sram_reg))
+		regulator_disable(drv->sram_reg);
+
+	return 0;
+}
+
+static const struct mtk_ccifreq_platform_data mtk_platform_data = {
+	.min_volt_shift = 0,
+	.max_volt_shift = 0,
+	.proc_max_volt = 1150000,
+	.sram_min_volt = 0,
+	.sram_max_volt = 0,
+	.need_voltage_tracking = false,
+};
+
+static const struct of_device_id mtk_ccifreq_machines[] = {
+	{ .compatible = "mediatek,mt8183-cci", .data = &mtk_platform_data },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mtk_ccifreq_machines);
+
+static struct platform_driver mtk_ccifreq_platdrv = {
+	.probe	= mtk_ccifreq_probe,
+	.remove	= mtk_ccifreq_remove,
+	.driver = {
+		.name = "mtk-ccifreq",
+		.of_match_table = of_match_ptr(mtk_ccifreq_machines),
+	},
+};
+
+static int __init mtk_ccifreq_platdrv_init(void)
+{
+	return platform_driver_register(&mtk_ccifreq_platdrv);
+}
+module_init(mtk_ccifreq_platdrv_init)
+
+static void __exit mtk_ccifreq_platdrv_exit(void)
+{
+	platform_driver_unregister(&mtk_ccifreq_platdrv);
+}
+module_exit(mtk_ccifreq_platdrv_exit)
+
+MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
+MODULE_AUTHOR("Jia-Wei Chang <jia-wei.chang@mediatek.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.18.0

[PATCH 3/3] devfreq: mediatek: add platform data to support mt8186

[Tim Chang]

add specific platform data to support mt8186.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
---
 drivers/devfreq/mtk-cci-devfreq.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-cci-devfreq.c
index 986f34689f5c..ade09794b404 100644
--- a/drivers/devfreq/mtk-cci-devfreq.c
+++ b/drivers/devfreq/mtk-cci-devfreq.c
@@ -439,8 +439,18 @@ static const struct mtk_ccifreq_platform_data mtk_platform_data = {
 	.need_voltage_tracking = false,
 };
 
+static const struct mtk_ccifreq_platform_data mt8186_platform_data = {
+	.min_volt_shift = 100000,
+	.max_volt_shift = 250000,
+	.proc_max_volt = 1118750,
+	.sram_min_volt = 850000,
+	.sram_max_volt = 1118750,
+	.need_voltage_tracking = true,
+};
+
 static const struct of_device_id mtk_ccifreq_machines[] = {
 	{ .compatible = "mediatek,mt8183-cci", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8186-cci", .data = &mt8186_platform_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, mtk_ccifreq_machines);
-- 
2.18.0

Re: [PATCH 2/3] devfreq: mediatek: add mt8183 cci devfreq driver

[Hsin-Yi Wang]

On Mon, Mar 7, 2022 at 8:32 PM Tim Chang <jia-wei.chang@mediatek.com> wrote:
>
> This adds a devfreq driver for the Cache Coherent Interconnect (CCI) of
> the Mediatek MT8183.
>
> On mt8183 the cci is supplied by the same regulator as the little cores.
> The driver is notified when the regulator voltage changes (driven by
> cpufreq) and adjusts the cci frequency to the maximum possible value.
>
> Add need_voltage_tracking variable to platforma data. if true, it
> indicates soc is required to realize the voltage tracking between
> voltage of sram and voltage of cci by software approach. otherwise, the
> voltage tracking is realized by hardware appraoch.
>
> Add the notifier to cci so that it could react after svs driver changes
> opp table of cci.
>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
> ---
>  drivers/devfreq/Kconfig           |  11 +-
>  drivers/devfreq/Makefile          |   2 +-
>  drivers/devfreq/mtk-cci-devfreq.c | 471 ++++++++++++++++++++++++++++++
>  3 files changed, 482 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/devfreq/mtk-cci-devfreq.c
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 1ec36ae93f31..a6be3c6b5691 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -110,7 +110,7 @@ config ARM_IMX8M_DDRC_DEVFREQ
>
>  config ARM_MT8183_CCI_DEVFREQ
>         tristate "MT8183 CCI DEVFREQ Driver"
> -       depends on ARM_MEDIATEK_CPUFREQ
> +       depends on OF && ARM_MEDIATEK_CPUFREQ
>         help
>                 This adds a devfreq driver for Cache Coherent Interconnect
>                 of Mediatek MT8183, which is shared the same regulator
> @@ -130,6 +130,15 @@ config ARM_TEGRA_DEVFREQ
>           It reads ACTMON counters of memory controllers and adjusts the
>           operating frequencies and voltages with OPP support.
>
> +config ARM_MEDIATEK_CCI_DEVFREQ
> +       tristate "MEDIATEK CCI DEVFREQ Driver"
> +       depends on OF && ARM_MEDIATEK_CPUFREQ
> +       help
> +         This adds a devfreq driver for Mediatek Cache Coherent Interconnect
> +         which is shared the same regulator with cpu cluster. It can track
> +         buck voltage and update a proper CCI frequency. Use notification
> +         to get regulator status.
> +
>  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 991ef7740759..0493516a16f2 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -11,7 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)     += governor_passive.o
>  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_MT8183_CCI_DEVFREQ)   += mt8183-cci-devfreq.o
> +obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ) += mtk-cci-devfreq.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)   += rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)                += tegra30-devfreq.o
>
> diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-cci-devfreq.c
> new file mode 100644
> index 000000000000..986f34689f5c
> --- /dev/null
> +++ b/drivers/devfreq/mtk-cci-devfreq.c
> @@ -0,0 +1,471 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 MediaTek Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/regulator/consumer.h>
> +
> +struct mtk_ccifreq_platform_data;
> +
> +struct mtk_ccifreq_drv {
> +       struct device *cci_dev;
> +       struct devfreq *devfreq;
> +       struct regulator *proc_reg;
> +       struct regulator *sram_reg;
> +       struct clk *cci_clk;
> +       struct clk *inter_clk;
> +       int inter_voltage;
> +       int old_voltage;
> +       unsigned long old_freq;
> +       struct mutex lock;  /* avoid notify and policy race condition */
> +       struct notifier_block opp_nb;
> +       const struct mtk_ccifreq_platform_data *soc_data;
> +};
> +
> +struct mtk_ccifreq_platform_data {
> +       int min_volt_shift;
> +       int max_volt_shift;
> +       int proc_max_volt;
> +       int sram_min_volt;
> +       int sram_max_volt;
> +       bool need_voltage_tracking;
> +};
> +
> +static int mtk_ccifreq_voltage_tracking(struct mtk_ccifreq_drv *drv, int new_voltage)
> +{
> +       const struct mtk_ccifreq_platform_data *soc_data = drv->soc_data;
> +       struct regulator *proc_reg = drv->proc_reg;
> +       struct regulator *sram_reg = drv->sram_reg;
> +       int old_voltage, old_vsram, new_vsram, vsram, voltage, ret;
> +
> +       old_voltage = regulator_get_voltage(proc_reg);
> +       if (old_voltage < 0) {
> +               pr_err("%s: invalid vproc value: %d\n", __func__, old_voltage);
> +               return old_voltage;
> +       }
> +
> +       old_vsram = regulator_get_voltage(sram_reg);
> +       if (old_vsram < 0) {
> +               pr_err("%s: invalid vsram value: %d\n", __func__, old_vsram);
> +               return old_vsram;
> +       }
> +
> +       new_vsram = clamp(new_voltage + soc_data->min_volt_shift,
> +                         soc_data->sram_min_volt, soc_data->sram_max_volt);
> +
> +       do {
> +               if (old_voltage <= new_voltage) {
> +                       vsram = clamp(old_voltage + soc_data->max_volt_shift,
> +                                     soc_data->sram_min_volt, new_vsram);
> +                       ret = regulator_set_voltage(sram_reg, vsram,
> +                                                   soc_data->sram_max_volt);
> +                       if (ret)
> +                               return ret;
> +
> +                       if (vsram == soc_data->sram_max_volt ||
> +                           new_vsram == soc_data->sram_min_volt)
> +                               voltage = new_voltage;
> +                       else
> +                               voltage = vsram - soc_data->min_volt_shift;
> +
> +                       ret = regulator_set_voltage(proc_reg, voltage,
> +                                                   soc_data->proc_max_volt);
> +                       if (ret) {
> +                               regulator_set_voltage(sram_reg, old_vsram,
> +                                                     soc_data->sram_max_volt);
> +                               return ret;
> +                       }
> +               } else if (old_voltage > new_voltage) {
> +                       voltage = max(new_voltage,
> +                                   old_vsram - soc_data->max_volt_shift);
> +                       ret = regulator_set_voltage(proc_reg, voltage,
> +                                                   soc_data->proc_max_volt);
> +                       if (ret)
> +                               return ret;
> +
> +                       if (voltage == new_voltage)
> +                               vsram = new_vsram;
> +                       else
> +                               vsram = max(new_vsram,
> +                                           voltage + soc_data->min_volt_shift);
> +
> +                       ret = regulator_set_voltage(sram_reg, vsram,
> +                                                   soc_data->sram_max_volt);
> +                       if (ret) {
> +                               regulator_set_voltage(proc_reg, old_voltage,
> +                                                     soc_data->proc_max_volt);
> +                               return ret;
> +                       }
> +               }
> +
> +               old_voltage = voltage;
> +               old_vsram = vsram;
> +       } while (voltage != new_voltage || vsram != new_vsram);
> +
> +       return 0;
> +}
> +
> +static int mtk_ccifreq_set_voltage(struct mtk_ccifreq_drv *drv, int voltage)
> +{
> +       int ret;
> +
> +       if (drv->soc_data->need_voltage_tracking)
> +               ret = mtk_ccifreq_voltage_tracking(drv, voltage);
> +       else
> +               ret = regulator_set_voltage(drv->proc_reg, voltage,
> +                                           drv->soc_data->proc_max_volt);
> +
> +       if (!ret)
> +               drv->old_voltage = voltage;
> +
> +       return ret;
> +}
> +
> +static int mtk_ccifreq_target(struct device *dev, unsigned long *freq,
> +                             u32 flags)
> +{
> +       struct mtk_ccifreq_drv *drv = dev_get_drvdata(dev);
> +       struct clk *cci_pll = clk_get_parent(drv->cci_clk);
> +       struct dev_pm_opp *opp;
> +       unsigned long opp_rate;
> +       int voltage, old_voltage, inter_voltage, target_voltage, ret;
> +
> +       if (!drv)
> +               return -EINVAL;
> +
> +       if (drv->old_freq == *freq)
> +               return 0;
> +
> +       inter_voltage = drv->inter_voltage;
> +
> +       opp_rate = *freq;
> +       opp = devfreq_recommended_opp(dev, &opp_rate, 1);
> +       if (IS_ERR(opp)) {
> +               pr_err("cci: failed to find opp for freq: %ld\n", opp_rate);
> +               return PTR_ERR(opp);
> +       }
> +       voltage = dev_pm_opp_get_voltage(opp);
> +       dev_pm_opp_put(opp);
> +
> +       old_voltage = drv->old_voltage;
> +       if (old_voltage == 0)
> +               old_voltage = regulator_get_voltage(drv->proc_reg);
> +       if (old_voltage < 0) {
> +               pr_err("cci: invalid vproc value: %d\n", old_voltage);
> +               return old_voltage;
> +       }
> +
> +       mutex_lock(&drv->lock);
> +
> +       /* scale up: set voltage first then freq. */
> +       target_voltage = max(inter_voltage, voltage);
> +       if (old_voltage <= target_voltage) {
> +               ret = mtk_ccifreq_set_voltage(drv, target_voltage);
> +               if (ret) {
> +                       pr_err("cci: failed to scale up voltage\n");
> +                       mtk_ccifreq_set_voltage(drv, old_voltage);
> +                       mutex_unlock(&drv->lock);
> +                       return ret;
> +               }
> +       }
> +
> +       /* switch the cci clock to intermediate clock source. */
> +       ret = clk_set_parent(drv->cci_clk, drv->inter_clk);
> +       if (ret) {
> +               pr_err("cci: failed to re-parent cci clock\n");
> +               mtk_ccifreq_set_voltage(drv, old_voltage);
> +               WARN_ON(1);
> +               mutex_unlock(&drv->lock);
> +               return ret;
> +       }
> +
> +       /* set the original clock to target rate. */
> +       ret = clk_set_rate(cci_pll, *freq);
> +       if (ret) {
> +               pr_err("cci: failed to set cci pll rate: %d\n", ret);
> +               clk_set_parent(drv->cci_clk, cci_pll);
> +               mtk_ccifreq_set_voltage(drv, old_voltage);
> +               mutex_unlock(&drv->lock);
> +               return ret;
> +       }
> +
> +       /* switch the cci clock back to the original clock source. */
> +       ret = clk_set_parent(drv->cci_clk, cci_pll);
> +       if (ret) {
> +               pr_err("cci: failed to re-parent cci clock\n");
> +               mtk_ccifreq_set_voltage(drv, inter_voltage);
> +               WARN_ON(1);
> +               mutex_unlock(&drv->lock);
> +               return ret;
> +       }
> +
> +       /*
> +        * If the new voltage is lower than the intermediate voltage or the
> +        * original voltage, scale down to the new voltage.
> +        */
> +       if (voltage < inter_voltage || voltage < old_voltage) {
> +               ret = mtk_ccifreq_set_voltage(drv, voltage);
> +               if (ret) {
> +                       pr_err("cci: failed to scale down voltage\n");
> +                       WARN_ON(1);
> +                       mutex_unlock(&drv->lock);
> +                       return ret;
> +               }
> +       }
> +
> +       drv->old_freq = *freq;
> +       mutex_unlock(&drv->lock);
> +
> +       return 0;
> +}
> +
> +static int mtk_ccifreq_opp_notifier(struct notifier_block *nb,
> +                                   unsigned long event, void *data)
> +{
> +       struct dev_pm_opp *opp = data;
> +       struct mtk_ccifreq_drv *drv;
> +       unsigned long freq, volt;
> +
> +       drv = container_of(nb, struct mtk_ccifreq_drv, opp_nb);
> +
> +       if (event == OPP_EVENT_ADJUST_VOLTAGE) {
> +               freq = dev_pm_opp_get_freq(opp);
> +
> +               mutex_lock(&drv->lock);
> +               /* current opp item is changed */
> +               if (freq == drv->old_freq) {
> +                       volt = dev_pm_opp_get_voltage(opp);
> +                       mtk_ccifreq_set_voltage(drv, volt);
> +               }
> +               mutex_unlock(&drv->lock);
> +       }
> +
> +       return 0;
> +}
> +
> +static struct devfreq_dev_profile mtk_ccifreq_profile = {
> +       .target = mtk_ccifreq_target,
> +};
> +
> +static int mtk_ccifreq_probe(struct platform_device *pdev)
> +{
> +       struct device *cci_dev = &pdev->dev;
> +       struct mtk_ccifreq_drv *drv;
> +       struct devfreq_passive_data *passive_data;
> +       struct dev_pm_opp *opp;
> +       unsigned long rate, opp_volt;
> +       int ret;
> +
> +       drv = devm_kzalloc(cci_dev, sizeof(*drv), GFP_KERNEL);
> +       if (!drv)
> +               return -ENOMEM;
> +
> +       drv->cci_dev = cci_dev;
> +       drv->soc_data = (const struct mtk_ccifreq_platform_data *)
> +                               of_device_get_match_data(&pdev->dev);
> +       mutex_init(&drv->lock);
> +       platform_set_drvdata(pdev, drv);
> +
> +       drv->cci_clk = devm_clk_get(cci_dev, "cci");
> +       if (IS_ERR(drv->cci_clk)) {
> +               ret = PTR_ERR(drv->cci_clk);
> +               return dev_err_probe(cci_dev, ret,
> +                                    "cci: failed to get cci clk: %d\n",
> +                                    ret);
> +       }
> +
> +       drv->inter_clk = devm_clk_get(cci_dev, "intermediate");
> +       if (IS_ERR(drv->inter_clk)) {
> +               ret = PTR_ERR(drv->inter_clk);
> +               return dev_err_probe(cci_dev, ret,
> +                                    "cci: failed to get intermediate clk: %d\n",
> +                                    ret);
> +       }
> +
> +       if (drv->soc_data->need_voltage_tracking) {
> +               drv->sram_reg = regulator_get_optional(cci_dev, "sram");
> +               if (IS_ERR_OR_NULL(drv->sram_reg)) {
> +                       ret = PTR_ERR(drv->sram_reg);
> +                       return dev_err_probe(cci_dev, ret,
> +                                            "cci: failed to get sram regulator: %d\n",
> +                                            ret);
> +               }
> +
> +               ret = regulator_enable(drv->sram_reg);
> +               if (ret) {
> +                       dev_warn(cci_dev,
> +                                "cci: failed to enable sram regulator\n");
> +                       return ret;
> +               }
> +       }
> +
> +       drv->proc_reg = devm_regulator_get_optional(cci_dev, "proc");
> +       if (IS_ERR(drv->proc_reg)) {
> +               ret = PTR_ERR(drv->proc_reg);
> +               dev_err_probe(cci_dev, ret,
> +                             "cci: failed to get proc regulator: %d\n",
> +                             ret);
> +               goto out_disable_sram_reg;
> +       }
> +
> +       ret = regulator_enable(drv->proc_reg);
> +       if (ret) {
> +               dev_warn(cci_dev, "cci: failed to enable proc regulator\n");
> +               goto out_disable_sram_reg;
> +       }
> +
> +       ret = clk_prepare_enable(drv->cci_clk);
> +       if (ret)
> +               goto out_disable_proc_reg;
> +
> +       ret = clk_prepare_enable(drv->inter_clk);
> +       if (ret)
> +               goto out_disable_cci_clk;
> +
> +       ret = dev_pm_opp_of_add_table(cci_dev);
> +       if (ret) {
> +               dev_warn(cci_dev, "cci: failed to add opp table: %d\n", ret);
> +               goto out_disable_inter_clk;
> +       }
> +
> +       rate = clk_get_rate(drv->inter_clk);
> +       opp = dev_pm_opp_find_freq_ceil(cci_dev, &rate);
> +       if (IS_ERR(opp)) {
> +               ret = PTR_ERR(opp);
> +               dev_err(cci_dev, "cci: failed to get intermediate opp: %d\n",
> +                       ret);
> +               goto out_remove_opp_table;
> +       }
> +       drv->inter_voltage = dev_pm_opp_get_voltage(opp);
> +       dev_pm_opp_put(opp);
> +
> +       rate = U32_MAX;
> +       opp = dev_pm_opp_find_freq_floor(drv->cci_dev, &rate);
> +       if (IS_ERR(opp)) {
> +               pr_err("failed to get opp\n");
> +               ret = PTR_ERR(opp);
> +               goto out_remove_opp_table;
> +       }
> +
> +       opp_volt = dev_pm_opp_get_voltage(opp);
> +       dev_pm_opp_put(opp);
> +       ret = mtk_ccifreq_set_voltage(drv, opp_volt);
> +       if (ret) {
> +               pr_err("failed to scale to highest voltage %lu in proc_reg\n",
> +                      opp_volt);
> +               goto out_remove_opp_table;
> +       }
> +
> +       passive_data = devm_kzalloc(cci_dev, sizeof(struct devfreq_passive_data), GFP_KERNEL);
> +       if (!passive_data) {
> +               ret = -ENOMEM;
> +               goto out_remove_opp_table;
> +       }
> +
> +       passive_data->parent_type = CPUFREQ_PARENT_DEV;

It's better to add a note below commit message to state that this
series depends on
https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing

> +       drv->devfreq = devm_devfreq_add_device(cci_dev,
> +                                              &mtk_ccifreq_profile,
> +                                              DEVFREQ_GOV_PASSIVE,
> +                                              passive_data);
> +       if (IS_ERR(drv->devfreq)) {
> +               ret = -EPROBE_DEFER;
> +               dev_err(cci_dev, "cci: failed to add devfreq device: %d\n",
> +                       PTR_ERR(drv->devfreq));
> +               goto out_remove_opp_table;
> +       }
> +
> +       drv->opp_nb.notifier_call = mtk_ccifreq_opp_notifier;
> +       ret = dev_pm_opp_register_notifier(cci_dev, &drv->opp_nb);
> +       if (ret) {
> +               dev_warn(cci_dev, "cci: failed to register opp notifier: %d\n",
> +                        ret);
> +               goto out_remove_devfreq_device;
> +       }
> +
> +       return 0;
> +
> +out_remove_devfreq_device:
> +       devm_devfreq_remove_device(cci_dev, drv->devfreq);
> +
> +out_remove_opp_table:
> +       dev_pm_opp_of_remove_table(cci_dev);
> +
> +out_disable_inter_clk:
> +       clk_disable_unprepare(drv->inter_clk);
> +
> +out_disable_cci_clk:
> +       clk_disable_unprepare(drv->cci_clk);
> +
> +out_disable_proc_reg:
> +       regulator_disable(drv->proc_reg);
> +
> +out_disable_sram_reg:
> +       if (drv->soc_data->need_voltage_tracking)
> +               regulator_disable(drv->sram_reg);
> +
> +       return ret;
> +}
> +
> +static int mtk_ccifreq_remove(struct platform_device *pdev)
> +{
> +       struct device *cci_dev = &pdev->dev;
> +       struct mtk_ccifreq_drv *drv;
> +
> +       drv = platform_get_drvdata(pdev);
> +
> +       dev_pm_opp_unregister_notifier(cci_dev, &drv->opp_nb);
> +       dev_pm_opp_of_remove_table(cci_dev);
> +       regulator_disable(drv->proc_reg);
> +       if (!IS_ERR(drv->sram_reg))
> +               regulator_disable(drv->sram_reg);
> +
> +       return 0;
> +}
> +
> +static const struct mtk_ccifreq_platform_data mtk_platform_data = {
> +       .min_volt_shift = 0,
> +       .max_volt_shift = 0,
> +       .proc_max_volt = 1150000,
> +       .sram_min_volt = 0,
> +       .sram_max_volt = 0,
> +       .need_voltage_tracking = false,
> +};
> +
> +static const struct of_device_id mtk_ccifreq_machines[] = {
> +       { .compatible = "mediatek,mt8183-cci", .data = &mtk_platform_data },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_ccifreq_machines);
> +
> +static struct platform_driver mtk_ccifreq_platdrv = {
> +       .probe  = mtk_ccifreq_probe,
> +       .remove = mtk_ccifreq_remove,
> +       .driver = {
> +               .name = "mtk-ccifreq",
> +               .of_match_table = of_match_ptr(mtk_ccifreq_machines),
> +       },
> +};
> +
> +static int __init mtk_ccifreq_platdrv_init(void)
> +{
> +       return platform_driver_register(&mtk_ccifreq_platdrv);
> +}
> +module_init(mtk_ccifreq_platdrv_init)
> +
> +static void __exit mtk_ccifreq_platdrv_exit(void)
> +{
> +       platform_driver_unregister(&mtk_ccifreq_platdrv);
> +}
> +module_exit(mtk_ccifreq_platdrv_exit)
> +
> +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> +MODULE_AUTHOR("Jia-Wei Chang <jia-wei.chang@mediatek.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.18.0
>

Re: [PATCH 1/3] dt-bindings: devfreq: mediatek: add mtk cci devfreq dt-bindings

[Krzysztof Kozlowski]

On 07/03/2022 13:25, Tim Chang wrote:
> add devicetree binding of mtk cci devfreq on MediaTek SoC.

Start with capital letter.

> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>

This does not match your From. Please fix this in all your submissions.

> ---
>  .../devicetree/bindings/devfreq/mtk-cci.yaml  | 73 +++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> new file mode 100644
> index 000000000000..e64ac4c56758
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/devfreq/mtk-cci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Cache Coherent Interconnect (CCI) Devfreq driver Device Tree Bindings

Similarly to your other patches - the title describes hardware. Please
fix it in all your submissions of all your series.

Remove "driver Device Tree Bindings". "Devfreq" is Linuxism, so this
maybe "bus frequency scaling"? Although later you call the device node
as cci.

> +
> +maintainers:
> +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> +
> +description: |
> +  This module is used to create CCI DEVFREQ.
> +  The performance will depend on both CCI frequency and CPU frequency.
> +  For MT8186, CCI co-buck with Little core.
> +  Contain CCI opp table for voltage and frequency scaling.

Half of this description (first and last sentence) does not describe the
actual hardware. Please describe hardware, not driver.

> +
> +properties:
> +  compatible:
> +    const: "mediatek,mt8186-cci"

No need for quotes.

> +
> +  clocks:
> +    items:
> +      - description:
> +          The first one is the multiplexer for clock input of CPU cluster.
> +      - description:
> +          The other is used as an intermediate clock source when the original
> +          CPU is under transition and not stable yet.
> +
> +  clock-names:
> +    items:
> +      - const: "cci"
> +      - const: "intermediate"

No need for quotes.

> +
> +  operating-points-v2:
> +    description:
> +      For details, please refer to
> +      Documentation/devicetree/bindings/opp/opp-v2.yaml
> +
> +  opp-table: true

Same comments as your CPU freq bindings apply.

> +
> +  proc-supply:
> +    description:
> +      Phandle of the regulator for CCI that provides the supply voltage.
> +
> +  sram-supply:
> +    description:
> +      Phandle of the regulator for sram of CCI that provides the supply
> +      voltage. When present, the cci devfreq driver needs to do
> +      "voltage tracking" to step by step scale up/down Vproc and Vsram to fit
> +      SoC specific needs. When absent, the voltage scaling flow is handled by
> +      hardware, hence no software "voltage tracking" is needed.
> +
> +required:
> +  - compatible
> +  - clocks
> +  - clock-names
> +  - operating-points-v2
> +  - proc-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mt8186-clk.h>
> +    cci: cci {

Node names should be generic and describe type of device. Are you sure
this is a CCI? Maybe "interconnect" suits it better?

> +      compatible = "mediatek,mt8186-cci";
> +      clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys CLK_APMIXED_MAINPLL>;
> +      clock-names = "cci", "intermediate";
> +      operating-points-v2 = <&cci_opp>;
> +      proc-supply = <&mt6358_vproc12_reg>;
> +      sram-supply = <&mt6358_vsram_proc12_reg>;
> +    };


Best regards,
Krzysztof

Re: [PATCH 2/3] devfreq: mediatek: add mt8183 cci devfreq driver

[Krzysztof Kozlowski]

On 07/03/2022 13:25, Tim Chang wrote:
> This adds a devfreq driver for the Cache Coherent Interconnect (CCI) of
> the Mediatek MT8183.
> 
> On mt8183 the cci is supplied by the same regulator as the little cores.
> The driver is notified when the regulator voltage changes (driven by
> cpufreq) and adjusts the cci frequency to the maximum possible value.
> 
> Add need_voltage_tracking variable to platforma data. if true, it
> indicates soc is required to realize the voltage tracking between
> voltage of sram and voltage of cci by software approach. otherwise, the
> voltage tracking is realized by hardware appraoch.
> 
> Add the notifier to cci so that it could react after svs driver changes
> opp table of cci.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
> ---
>  drivers/devfreq/Kconfig           |  11 +-
>  drivers/devfreq/Makefile          |   2 +-
>  drivers/devfreq/mtk-cci-devfreq.c | 471 ++++++++++++++++++++++++++++++
>  3 files changed, 482 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/devfreq/mtk-cci-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 1ec36ae93f31..a6be3c6b5691 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -110,7 +110,7 @@ config ARM_IMX8M_DDRC_DEVFREQ
>  
>  config ARM_MT8183_CCI_DEVFREQ
>  	tristate "MT8183 CCI DEVFREQ Driver"
> -	depends on ARM_MEDIATEK_CPUFREQ
> +	depends on OF && ARM_MEDIATEK_CPUFREQ

Why do you need that dependency? Isn't your arch already OF-only?

>  	help
>  		This adds a devfreq driver for Cache Coherent Interconnect
>  		of Mediatek MT8183, which is shared the same regulator
> @@ -130,6 +130,15 @@ config ARM_TEGRA_DEVFREQ
>  	  It reads ACTMON counters of memory controllers and adjusts the
>  	  operating frequencies and voltages with OPP support.
>  
> +config ARM_MEDIATEK_CCI_DEVFREQ
> +	tristate "MEDIATEK CCI DEVFREQ Driver"
> +	depends on OF && ARM_MEDIATEK_CPUFREQ
> +	help
> +	  This adds a devfreq driver for Mediatek Cache Coherent Interconnect
> +	  which is shared the same regulator with cpu cluster. It can track
> +	  buck voltage and update a proper CCI frequency. Use notification
> +	  to get regulator status.
> +
>  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 991ef7740759..0493516a16f2 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -11,7 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  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_MT8183_CCI_DEVFREQ)	+= mt8183-cci-devfreq.o
> +obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ)	+= mtk-cci-devfreq.o

How is this related? You say in commit subject and description that you
add a devfreq driver. But here you remove some other file...

>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
>  
> diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-cci-devfreq.c
> new file mode 100644
> index 000000000000..986f34689f5c
> --- /dev/null
> +++ b/drivers/devfreq/mtk-cci-devfreq.c
> @@ -0,0 +1,471 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 MediaTek Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/regulator/consumer.h>
> +
> +struct mtk_ccifreq_platform_data;

Move struct mtk_ccifreq_platform_data definition here. No need for
forward declaration.

> +
> +struct mtk_ccifreq_drv {
> +	struct device *cci_dev;
> +	struct devfreq *devfreq;
> +	struct regulator *proc_reg;
> +	struct regulator *sram_reg;
> +	struct clk *cci_clk;
> +	struct clk *inter_clk;
> +	int inter_voltage;
> +	int old_voltage;
> +	unsigned long old_freq;
> +	struct mutex lock;  /* avoid notify and policy race condition */
> +	struct notifier_block opp_nb;
> +	const struct mtk_ccifreq_platform_data *soc_data;
> +};
> +
> +struct mtk_ccifreq_platform_data {
> +	int min_volt_shift;
> +	int max_volt_shift;
> +	int proc_max_volt;
> +	int sram_min_volt;
> +	int sram_max_volt;
> +	bool need_voltage_tracking;
> +};
> +
> +static int mtk_ccifreq_voltage_tracking(struct mtk_ccifreq_drv *drv, int new_voltage)
> +{
> +	const struct mtk_ccifreq_platform_data *soc_data = drv->soc_data;
> +	struct regulator *proc_reg = drv->proc_reg;
> +	struct regulator *sram_reg = drv->sram_reg;
> +	int old_voltage, old_vsram, new_vsram, vsram, voltage, ret;
> +
> +	old_voltage = regulator_get_voltage(proc_reg);
> +	if (old_voltage < 0) {
> +		pr_err("%s: invalid vproc value: %d\n", __func__, old_voltage);
> +		return old_voltage;
> +	}
> +
> +	old_vsram = regulator_get_voltage(sram_reg);
> +	if (old_vsram < 0) {
> +		pr_err("%s: invalid vsram value: %d\n", __func__, old_vsram);
> +		return old_vsram;
> +	}
> +
> +	new_vsram = clamp(new_voltage + soc_data->min_volt_shift,
> +			  soc_data->sram_min_volt, soc_data->sram_max_volt);
> +
> +	do {
> +		if (old_voltage <= new_voltage) {
> +			vsram = clamp(old_voltage + soc_data->max_volt_shift,
> +				      soc_data->sram_min_volt, new_vsram);
> +			ret = regulator_set_voltage(sram_reg, vsram,
> +						    soc_data->sram_max_volt);
> +			if (ret)
> +				return ret;
> +
> +			if (vsram == soc_data->sram_max_volt ||
> +			    new_vsram == soc_data->sram_min_volt)
> +				voltage = new_voltage;
> +			else
> +				voltage = vsram - soc_data->min_volt_shift;
> +
> +			ret = regulator_set_voltage(proc_reg, voltage,
> +						    soc_data->proc_max_volt);
> +			if (ret) {
> +				regulator_set_voltage(sram_reg, old_vsram,
> +						      soc_data->sram_max_volt);
> +				return ret;
> +			}
> +		} else if (old_voltage > new_voltage) {
> +			voltage = max(new_voltage,
> +				    old_vsram - soc_data->max_volt_shift);
> +			ret = regulator_set_voltage(proc_reg, voltage,
> +						    soc_data->proc_max_volt);
> +			if (ret)
> +				return ret;
> +
> +			if (voltage == new_voltage)
> +				vsram = new_vsram;
> +			else
> +				vsram = max(new_vsram,
> +					    voltage + soc_data->min_volt_shift);
> +
> +			ret = regulator_set_voltage(sram_reg, vsram,
> +						    soc_data->sram_max_volt);
> +			if (ret) {
> +				regulator_set_voltage(proc_reg, old_voltage,
> +						      soc_data->proc_max_volt);
> +				return ret;
> +			}
> +		}
> +
> +		old_voltage = voltage;
> +		old_vsram = vsram;
> +	} while (voltage != new_voltage || vsram != new_vsram);
> +
> +	return 0;
> +}
> +
> +static int mtk_ccifreq_set_voltage(struct mtk_ccifreq_drv *drv, int voltage)
> +{
> +	int ret;
> +
> +	if (drv->soc_data->need_voltage_tracking)
> +		ret = mtk_ccifreq_voltage_tracking(drv, voltage);
> +	else
> +		ret = regulator_set_voltage(drv->proc_reg, voltage,
> +					    drv->soc_data->proc_max_volt);
> +
> +	if (!ret)
> +		drv->old_voltage = voltage;
> +
> +	return ret;
> +}
> +
> +static int mtk_ccifreq_target(struct device *dev, unsigned long *freq,
> +			      u32 flags)
> +{
> +	struct mtk_ccifreq_drv *drv = dev_get_drvdata(dev);
> +	struct clk *cci_pll = clk_get_parent(drv->cci_clk);
> +	struct dev_pm_opp *opp;
> +	unsigned long opp_rate;
> +	int voltage, old_voltage, inter_voltage, target_voltage, ret;
> +
> +	if (!drv)
> +		return -EINVAL;
> +
> +	if (drv->old_freq == *freq)
> +		return 0;
> +
> +	inter_voltage = drv->inter_voltage;
> +
> +	opp_rate = *freq;
> +	opp = devfreq_recommended_opp(dev, &opp_rate, 1);
> +	if (IS_ERR(opp)) {
> +		pr_err("cci: failed to find opp for freq: %ld\n", opp_rate);

dev_err() everywhere.

> +		return PTR_ERR(opp);
> +	}
> +	voltage = dev_pm_opp_get_voltage(opp);
> +	dev_pm_opp_put(opp);
> +
> +	old_voltage = drv->old_voltage;
> +	if (old_voltage == 0)
> +		old_voltage = regulator_get_voltage(drv->proc_reg);
> +	if (old_voltage < 0) {
> +		pr_err("cci: invalid vproc value: %d\n", old_voltage);
> +		return old_voltage;
> +	}
> +
> +	mutex_lock(&drv->lock);
> +
> +	/* scale up: set voltage first then freq. */
> +	target_voltage = max(inter_voltage, voltage);
> +	if (old_voltage <= target_voltage) {
> +		ret = mtk_ccifreq_set_voltage(drv, target_voltage);
> +		if (ret) {
> +			pr_err("cci: failed to scale up voltage\n");
> +			mtk_ccifreq_set_voltage(drv, old_voltage);
> +			mutex_unlock(&drv->lock);
> +			return ret;
> +		}
> +	}
> +
> +	/* switch the cci clock to intermediate clock source. */
> +	ret = clk_set_parent(drv->cci_clk, drv->inter_clk);
> +	if (ret) {
> +		pr_err("cci: failed to re-parent cci clock\n");
> +		mtk_ccifreq_set_voltage(drv, old_voltage);
> +		WARN_ON(1);

Debug code... please clean up your driver.

> +		mutex_unlock(&drv->lock);
> +		return ret;
> +	}
> +
> +	/* set the original clock to target rate. */
> +	ret = clk_set_rate(cci_pll, *freq);
> +	if (ret) {
> +		pr_err("cci: failed to set cci pll rate: %d\n", ret);
> +		clk_set_parent(drv->cci_clk, cci_pll);
> +		mtk_ccifreq_set_voltage(drv, old_voltage);
> +		mutex_unlock(&drv->lock);
> +		return ret;
> +	}
> +
> +	/* switch the cci clock back to the original clock source. */
> +	ret = clk_set_parent(drv->cci_clk, cci_pll);
> +	if (ret) {
> +		pr_err("cci: failed to re-parent cci clock\n");
> +		mtk_ccifreq_set_voltage(drv, inter_voltage);
> +		WARN_ON(1);
> +		mutex_unlock(&drv->lock);

Use goto to error handling pattern.

> +		return ret;
> +	}
> +
> +	/*
> +	 * If the new voltage is lower than the intermediate voltage or the
> +	 * original voltage, scale down to the new voltage.
> +	 */
> +	if (voltage < inter_voltage || voltage < old_voltage) {
> +		ret = mtk_ccifreq_set_voltage(drv, voltage);
> +		if (ret) {
> +			pr_err("cci: failed to scale down voltage\n");
> +			WARN_ON(1);
> +			mutex_unlock(&drv->lock);
> +			return ret;
> +		}
> +	}
> +
> +	drv->old_freq = *freq;
> +	mutex_unlock(&drv->lock);
> +
> +	return 0;
> +}
> +
> +static int mtk_ccifreq_opp_notifier(struct notifier_block *nb,
> +				    unsigned long event, void *data)
> +{
> +	struct dev_pm_opp *opp = data;
> +	struct mtk_ccifreq_drv *drv;
> +	unsigned long freq, volt;
> +
> +	drv = container_of(nb, struct mtk_ccifreq_drv, opp_nb);
> +
> +	if (event == OPP_EVENT_ADJUST_VOLTAGE) {
> +		freq = dev_pm_opp_get_freq(opp);
> +
> +		mutex_lock(&drv->lock);
> +		/* current opp item is changed */
> +		if (freq == drv->old_freq) {
> +			volt = dev_pm_opp_get_voltage(opp);
> +			mtk_ccifreq_set_voltage(drv, volt);
> +		}
> +		mutex_unlock(&drv->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile mtk_ccifreq_profile = {
> +	.target = mtk_ccifreq_target,
> +};
> +
> +static int mtk_ccifreq_probe(struct platform_device *pdev)
> +{
> +	struct device *cci_dev = &pdev->dev;
> +	struct mtk_ccifreq_drv *drv;
> +	struct devfreq_passive_data *passive_data;
> +	struct dev_pm_opp *opp;
> +	unsigned long rate, opp_volt;
> +	int ret;
> +
> +	drv = devm_kzalloc(cci_dev, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	drv->cci_dev = cci_dev;
> +	drv->soc_data = (const struct mtk_ccifreq_platform_data *)
> +				of_device_get_match_data(&pdev->dev);
> +	mutex_init(&drv->lock);
> +	platform_set_drvdata(pdev, drv);
> +
> +	drv->cci_clk = devm_clk_get(cci_dev, "cci");
> +	if (IS_ERR(drv->cci_clk)) {
> +		ret = PTR_ERR(drv->cci_clk);
> +		return dev_err_probe(cci_dev, ret,
> +				     "cci: failed to get cci clk: %d\n",
> +				     ret);
> +	}
> +
> +	drv->inter_clk = devm_clk_get(cci_dev, "intermediate");
> +	if (IS_ERR(drv->inter_clk)) {
> +		ret = PTR_ERR(drv->inter_clk);
> +		return dev_err_probe(cci_dev, ret,
> +				     "cci: failed to get intermediate clk: %d\n",
> +				     ret);
> +	}
> +
> +	if (drv->soc_data->need_voltage_tracking) {
> +		drv->sram_reg = regulator_get_optional(cci_dev, "sram");
> +		if (IS_ERR_OR_NULL(drv->sram_reg)) {

NULL pointer cannot be returned.

Best regards,
Krzysztof

Re: [PATCH 3/3] devfreq: mediatek: add platform data to support mt8186

[Krzysztof Kozlowski]

On 07/03/2022 13:25, Tim Chang wrote:
> add specific platform data to support mt8186.
> 

You just added this driver in patch 2. No need to split such actions
because this is initial submission, so it can support two devices.
Squash with previous.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: devfreq: mediatek: add mtk cci devfreq dt-bindings

[Jia-Wei Chang]

Dear Krzysztof,

Thanks for your comments.
Pardon me for my late reply.

On Mon, 2022-03-07 at 22:42 +0100, Krzysztof Kozlowski wrote:
> On 07/03/2022 13:25, Tim Chang wrote:
> > add devicetree binding of mtk cci devfreq on MediaTek SoC.
> 
> Start with capital letter.

Sure, I will update it for the whole series in next version.

> 
> > 
> > Signed-off-by: Jia-Wei Chang <
> > jia-wei.chang@mediatek.corp-partner.google.com>
> 
> This does not match your From. Please fix this in all your
> submissions.

Sure, I will update it for the whole series in next version.

> 
> > ---
> >  .../devicetree/bindings/devfreq/mtk-cci.yaml  | 73
> > +++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/devfreq/mtk-
> > cci.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml 
> > b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> > new file mode 100644
> > index 000000000000..e64ac4c56758
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> > @@ -0,0 +1,73 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/devfreq/mtk-cci.yaml*__;Iw!!CTRNKA9wMg0ARbw!2apx_16V_XMrl28ae1aDO3-2WFga3xJiACU_40mgGydrumBmFuHcQFpW_LnX6DHny5Zpig$
> >  
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!2apx_16V_XMrl28ae1aDO3-2WFga3xJiACU_40mgGydrumBmFuHcQFpW_LnX6DEPqkDN4g$
> >  
> > +
> > +title: Mediatek Cache Coherent Interconnect (CCI) Devfreq driver
> > Device Tree Bindings
> 
> Similarly to your other patches - the title describes hardware.
> Please
> fix it in all your submissions of all your series.

Sure, I will fix them in the next version.

> 
> Remove "driver Device Tree Bindings". "Devfreq" is Linuxism, so this
> maybe "bus frequency scaling"? Although later you call the device
> node
> as cci.

Should I use "Binding for MediaTek's Cache Coherent Interconnect (CCI)
frequency and voltage scaling" as new title?

> 
> > +
> > +maintainers:
> > +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > +
> > +description: |
> > +  This module is used to create CCI DEVFREQ.
> > +  The performance will depend on both CCI frequency and CPU
> > frequency.
> > +  For MT8186, CCI co-buck with Little core.
> > +  Contain CCI opp table for voltage and frequency scaling.
> 
> Half of this description (first and last sentence) does not describe
> the
> actual hardware. Please describe hardware, not driver.

Sure, I will fix it in the next version.

> 
> > +
> > +properties:
> > +  compatible:
> > +    const: "mediatek,mt8186-cci"
> 
> No need for quotes.

Sure, I will fix it in the next version.

> 
> > +
> > +  clocks:
> > +    items:
> > +      - description:
> > +          The first one is the multiplexer for clock input of CPU
> > cluster.
> > +      - description:
> > +          The other is used as an intermediate clock source when
> > the original
> > +          CPU is under transition and not stable yet.
> > +
> > +  clock-names:
> > +    items:
> > +      - const: "cci"
> > +      - const: "intermediate"
> 
> No need for quotes.

Sure, I will fix it in the next version.

> 
> > +
> > +  operating-points-v2:
> > +    description:
> > +      For details, please refer to
> > +      Documentation/devicetree/bindings/opp/opp-v2.yaml
> > +
> > +  opp-table: true
> 
> Same comments as your CPU freq bindings apply.

mtk-cci-devfreq is a new driver and its arch is same as mediatek-
cpufreq so that the properties of mtk-cci are refer to mediatek-cpufreq 
bindings.
operating-point-v2 is used to determine the voltage and frequency of
dvfs which is further utilized by mtk-cci-devfreq.

> 
> > +
> > +  proc-supply:
> > +    description:
> > +      Phandle of the regulator for CCI that provides the supply
> > voltage.
> > +
> > +  sram-supply:
> > +    description:
> > +      Phandle of the regulator for sram of CCI that provides the
> > supply
> > +      voltage. When present, the cci devfreq driver needs to do
> > +      "voltage tracking" to step by step scale up/down Vproc and
> > Vsram to fit
> > +      SoC specific needs. When absent, the voltage scaling flow is
> > handled by
> > +      hardware, hence no software "voltage tracking" is needed.
> > +
> > +required:
> > +  - compatible
> > +  - clocks
> > +  - clock-names
> > +  - operating-points-v2
> > +  - proc-supply
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/mt8186-clk.h>
> > +    cci: cci {
> 
> Node names should be generic and describe type of device. Are you
> sure
> this is a CCI? Maybe "interconnect" suits it better?

Yes, this is a CCI and it is generic type of device like CPU in my
opinion.
If my understanding is correct, CCI is more suitable.

> 
> > +      compatible = "mediatek,mt8186-cci";
> > +      clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys
> > CLK_APMIXED_MAINPLL>;
> > +      clock-names = "cci", "intermediate";
> > +      operating-points-v2 = <&cci_opp>;
> > +      proc-supply = <&mt6358_vproc12_reg>;
> > +      sram-supply = <&mt6358_vsram_proc12_reg>;
> > +    };
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 2/3] devfreq: mediatek: add mt8183 cci devfreq driver

[Jia-Wei Chang]

On Mon, 2022-03-07 at 22:51 +0100, Krzysztof Kozlowski wrote:
> On 07/03/2022 13:25, Tim Chang wrote:
> > This adds a devfreq driver for the Cache Coherent Interconnect
> > (CCI) of
> > the Mediatek MT8183.
> > 
> > On mt8183 the cci is supplied by the same regulator as the little
> > cores.
> > The driver is notified when the regulator voltage changes (driven
> > by
> > cpufreq) and adjusts the cci frequency to the maximum possible
> > value.
> > 
> > Add need_voltage_tracking variable to platforma data. if true, it
> > indicates soc is required to realize the voltage tracking between
> > voltage of sram and voltage of cci by software approach. otherwise,
> > the
> > voltage tracking is realized by hardware appraoch.
> > 
> > Add the notifier to cci so that it could react after svs driver
> > changes
> > opp table of cci.
> > 
> > Signed-off-by: Jia-Wei Chang <
> > jia-wei.chang@mediatek.corp-partner.google.com>
> > ---
> >  drivers/devfreq/Kconfig           |  11 +-
> >  drivers/devfreq/Makefile          |   2 +-
> >  drivers/devfreq/mtk-cci-devfreq.c | 471
> > ++++++++++++++++++++++++++++++
> >  3 files changed, 482 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/devfreq/mtk-cci-devfreq.c
> > 
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 1ec36ae93f31..a6be3c6b5691 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -110,7 +110,7 @@ config ARM_IMX8M_DDRC_DEVFREQ
> >  
> >  config ARM_MT8183_CCI_DEVFREQ
> >  	tristate "MT8183 CCI DEVFREQ Driver"
> > -	depends on ARM_MEDIATEK_CPUFREQ
> > +	depends on OF && ARM_MEDIATEK_CPUFREQ
> 
> Why do you need that dependency? Isn't your arch already OF-only?

Yes, it is OF-only so far.
I will remove it and keep it as before.

> 
> >  	help
> >  		This adds a devfreq driver for Cache Coherent
> > Interconnect
> >  		of Mediatek MT8183, which is shared the same regulator
> > @@ -130,6 +130,15 @@ config ARM_TEGRA_DEVFREQ
> >  	  It reads ACTMON counters of memory controllers and adjusts
> > the
> >  	  operating frequencies and voltages with OPP support.
> >  
> > +config ARM_MEDIATEK_CCI_DEVFREQ
> > +	tristate "MEDIATEK CCI DEVFREQ Driver"
> > +	depends on OF && ARM_MEDIATEK_CPUFREQ
> > +	help
> > +	  This adds a devfreq driver for Mediatek Cache Coherent
> > Interconnect
> > +	  which is shared the same regulator with cpu cluster. It can
> > track
> > +	  buck voltage and update a proper CCI frequency. Use
> > notification
> > +	  to get regulator status.
> > +
> >  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 991ef7740759..0493516a16f2 100644
> > --- a/drivers/devfreq/Makefile
> > +++ b/drivers/devfreq/Makefile
> > @@ -11,7 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+=
> > governor_passive.o
> >  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_MT8183_CCI_DEVFREQ)	+= mt8183-cci-devfreq.o
> > +obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ)	+= mtk-cci-devfreq.o
> 
> How is this related? You say in commit subject and description that
> you
> add a devfreq driver. But here you remove some other file...

Sorry, this is my mistake.
There's no mt8183-cci-devfreq.c on the codebase of linux-next.
I will fix it after rebasing in the next version.

> 
> >  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
> >  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
> >  
> > diff --git a/drivers/devfreq/mtk-cci-devfreq.c
> > b/drivers/devfreq/mtk-cci-devfreq.c
> > new file mode 100644
> > index 000000000000..986f34689f5c
> > --- /dev/null
> > +++ b/drivers/devfreq/mtk-cci-devfreq.c
> > @@ -0,0 +1,471 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2022 MediaTek Inc.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/devfreq.h>
> > +#include <linux/minmax.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +struct mtk_ccifreq_platform_data;
> 
> Move struct mtk_ccifreq_platform_data definition here. No need for
> forward declaration.

Sure, I will update it in the next version.

> 
> > +
> > +struct mtk_ccifreq_drv {
> > +	struct device *cci_dev;
> > +	struct devfreq *devfreq;
> > +	struct regulator *proc_reg;
> > +	struct regulator *sram_reg;
> > +	struct clk *cci_clk;
> > +	struct clk *inter_clk;
> > +	int inter_voltage;
> > +	int old_voltage;
> > +	unsigned long old_freq;
> > +	struct mutex lock;  /* avoid notify and policy race condition
> > */
> > +	struct notifier_block opp_nb;
> > +	const struct mtk_ccifreq_platform_data *soc_data;
> > +};
> > +
> > +struct mtk_ccifreq_platform_data {
> > +	int min_volt_shift;
> > +	int max_volt_shift;
> > +	int proc_max_volt;
> > +	int sram_min_volt;
> > +	int sram_max_volt;
> > +	bool need_voltage_tracking;
> > +};
> > +
> > +static int mtk_ccifreq_voltage_tracking(struct mtk_ccifreq_drv
> > *drv, int new_voltage)
> > +{
> > +	const struct mtk_ccifreq_platform_data *soc_data = drv-
> > >soc_data;
> > +	struct regulator *proc_reg = drv->proc_reg;
> > +	struct regulator *sram_reg = drv->sram_reg;
> > +	int old_voltage, old_vsram, new_vsram, vsram, voltage, ret;
> > +
> > +	old_voltage = regulator_get_voltage(proc_reg);
> > +	if (old_voltage < 0) {
> > +		pr_err("%s: invalid vproc value: %d\n", __func__,
> > old_voltage);
> > +		return old_voltage;
> > +	}
> > +
> > +	old_vsram = regulator_get_voltage(sram_reg);
> > +	if (old_vsram < 0) {
> > +		pr_err("%s: invalid vsram value: %d\n", __func__,
> > old_vsram);
> > +		return old_vsram;
> > +	}
> > +
> > +	new_vsram = clamp(new_voltage + soc_data->min_volt_shift,
> > +			  soc_data->sram_min_volt, soc_data-
> > >sram_max_volt);
> > +
> > +	do {
> > +		if (old_voltage <= new_voltage) {
> > +			vsram = clamp(old_voltage + soc_data-
> > >max_volt_shift,
> > +				      soc_data->sram_min_volt,
> > new_vsram);
> > +			ret = regulator_set_voltage(sram_reg, vsram,
> > +						    soc_data-
> > >sram_max_volt);
> > +			if (ret)
> > +				return ret;
> > +
> > +			if (vsram == soc_data->sram_max_volt ||
> > +			    new_vsram == soc_data->sram_min_volt)
> > +				voltage = new_voltage;
> > +			else
> > +				voltage = vsram - soc_data-
> > >min_volt_shift;
> > +
> > +			ret = regulator_set_voltage(proc_reg, voltage,
> > +						    soc_data-
> > >proc_max_volt);
> > +			if (ret) {
> > +				regulator_set_voltage(sram_reg,
> > old_vsram,
> > +						      soc_data-
> > >sram_max_volt);
> > +				return ret;
> > +			}
> > +		} else if (old_voltage > new_voltage) {
> > +			voltage = max(new_voltage,
> > +				    old_vsram - soc_data-
> > >max_volt_shift);
> > +			ret = regulator_set_voltage(proc_reg, voltage,
> > +						    soc_data-
> > >proc_max_volt);
> > +			if (ret)
> > +				return ret;
> > +
> > +			if (voltage == new_voltage)
> > +				vsram = new_vsram;
> > +			else
> > +				vsram = max(new_vsram,
> > +					    voltage + soc_data-
> > >min_volt_shift);
> > +
> > +			ret = regulator_set_voltage(sram_reg, vsram,
> > +						    soc_data-
> > >sram_max_volt);
> > +			if (ret) {
> > +				regulator_set_voltage(proc_reg,
> > old_voltage,
> > +						      soc_data-
> > >proc_max_volt);
> > +				return ret;
> > +			}
> > +		}
> > +
> > +		old_voltage = voltage;
> > +		old_vsram = vsram;
> > +	} while (voltage != new_voltage || vsram != new_vsram);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_ccifreq_set_voltage(struct mtk_ccifreq_drv *drv,
> > int voltage)
> > +{
> > +	int ret;
> > +
> > +	if (drv->soc_data->need_voltage_tracking)
> > +		ret = mtk_ccifreq_voltage_tracking(drv, voltage);
> > +	else
> > +		ret = regulator_set_voltage(drv->proc_reg, voltage,
> > +					    drv->soc_data-
> > >proc_max_volt);
> > +
> > +	if (!ret)
> > +		drv->old_voltage = voltage;
> > +
> > +	return ret;
> > +}
> > +
> > +static int mtk_ccifreq_target(struct device *dev, unsigned long
> > *freq,
> > +			      u32 flags)
> > +{
> > +	struct mtk_ccifreq_drv *drv = dev_get_drvdata(dev);
> > +	struct clk *cci_pll = clk_get_parent(drv->cci_clk);
> > +	struct dev_pm_opp *opp;
> > +	unsigned long opp_rate;
> > +	int voltage, old_voltage, inter_voltage, target_voltage, ret;
> > +
> > +	if (!drv)
> > +		return -EINVAL;
> > +
> > +	if (drv->old_freq == *freq)
> > +		return 0;
> > +
> > +	inter_voltage = drv->inter_voltage;
> > +
> > +	opp_rate = *freq;
> > +	opp = devfreq_recommended_opp(dev, &opp_rate, 1);
> > +	if (IS_ERR(opp)) {
> > +		pr_err("cci: failed to find opp for freq: %ld\n",
> > opp_rate);
> 
> dev_err() everywhere.

Sure, I will update it in the next version.

> 
> > +		return PTR_ERR(opp);
> > +	}
> > +	voltage = dev_pm_opp_get_voltage(opp);
> > +	dev_pm_opp_put(opp);
> > +
> > +	old_voltage = drv->old_voltage;
> > +	if (old_voltage == 0)
> > +		old_voltage = regulator_get_voltage(drv->proc_reg);
> > +	if (old_voltage < 0) {
> > +		pr_err("cci: invalid vproc value: %d\n", old_voltage);
> > +		return old_voltage;
> > +	}
> > +
> > +	mutex_lock(&drv->lock);
> > +
> > +	/* scale up: set voltage first then freq. */
> > +	target_voltage = max(inter_voltage, voltage);
> > +	if (old_voltage <= target_voltage) {
> > +		ret = mtk_ccifreq_set_voltage(drv, target_voltage);
> > +		if (ret) {
> > +			pr_err("cci: failed to scale up voltage\n");
> > +			mtk_ccifreq_set_voltage(drv, old_voltage);
> > +			mutex_unlock(&drv->lock);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	/* switch the cci clock to intermediate clock source. */
> > +	ret = clk_set_parent(drv->cci_clk, drv->inter_clk);
> > +	if (ret) {
> > +		pr_err("cci: failed to re-parent cci clock\n");
> > +		mtk_ccifreq_set_voltage(drv, old_voltage);
> > +		WARN_ON(1);
> 
> Debug code... please clean up your driver.

Sure, I will update it in the next version.

> 
> > +		mutex_unlock(&drv->lock);
> > +		return ret;
> > +	}
> > +
> > +	/* set the original clock to target rate. */
> > +	ret = clk_set_rate(cci_pll, *freq);
> > +	if (ret) {
> > +		pr_err("cci: failed to set cci pll rate: %d\n", ret);
> > +		clk_set_parent(drv->cci_clk, cci_pll);
> > +		mtk_ccifreq_set_voltage(drv, old_voltage);
> > +		mutex_unlock(&drv->lock);
> > +		return ret;
> > +	}
> > +
> > +	/* switch the cci clock back to the original clock source. */
> > +	ret = clk_set_parent(drv->cci_clk, cci_pll);
> > +	if (ret) {
> > +		pr_err("cci: failed to re-parent cci clock\n");
> > +		mtk_ccifreq_set_voltage(drv, inter_voltage);
> > +		WARN_ON(1);
> > +		mutex_unlock(&drv->lock);
> 
> Use goto to error handling pattern.

Sure, I will use goto statement for error handling instead.

> 
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * If the new voltage is lower than the intermediate voltage or
> > the
> > +	 * original voltage, scale down to the new voltage.
> > +	 */
> > +	if (voltage < inter_voltage || voltage < old_voltage) {
> > +		ret = mtk_ccifreq_set_voltage(drv, voltage);
> > +		if (ret) {
> > +			pr_err("cci: failed to scale down voltage\n");
> > +			WARN_ON(1);
> > +			mutex_unlock(&drv->lock);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	drv->old_freq = *freq;
> > +	mutex_unlock(&drv->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_ccifreq_opp_notifier(struct notifier_block *nb,
> > +				    unsigned long event, void *data)
> > +{
> > +	struct dev_pm_opp *opp = data;
> > +	struct mtk_ccifreq_drv *drv;
> > +	unsigned long freq, volt;
> > +
> > +	drv = container_of(nb, struct mtk_ccifreq_drv, opp_nb);
> > +
> > +	if (event == OPP_EVENT_ADJUST_VOLTAGE) {
> > +		freq = dev_pm_opp_get_freq(opp);
> > +
> > +		mutex_lock(&drv->lock);
> > +		/* current opp item is changed */
> > +		if (freq == drv->old_freq) {
> > +			volt = dev_pm_opp_get_voltage(opp);
> > +			mtk_ccifreq_set_voltage(drv, volt);
> > +		}
> > +		mutex_unlock(&drv->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct devfreq_dev_profile mtk_ccifreq_profile = {
> > +	.target = mtk_ccifreq_target,
> > +};
> > +
> > +static int mtk_ccifreq_probe(struct platform_device *pdev)
> > +{
> > +	struct device *cci_dev = &pdev->dev;
> > +	struct mtk_ccifreq_drv *drv;
> > +	struct devfreq_passive_data *passive_data;
> > +	struct dev_pm_opp *opp;
> > +	unsigned long rate, opp_volt;
> > +	int ret;
> > +
> > +	drv = devm_kzalloc(cci_dev, sizeof(*drv), GFP_KERNEL);
> > +	if (!drv)
> > +		return -ENOMEM;
> > +
> > +	drv->cci_dev = cci_dev;
> > +	drv->soc_data = (const struct mtk_ccifreq_platform_data *)
> > +				of_device_get_match_data(&pdev->dev);
> > +	mutex_init(&drv->lock);
> > +	platform_set_drvdata(pdev, drv);
> > +
> > +	drv->cci_clk = devm_clk_get(cci_dev, "cci");
> > +	if (IS_ERR(drv->cci_clk)) {
> > +		ret = PTR_ERR(drv->cci_clk);
> > +		return dev_err_probe(cci_dev, ret,
> > +				     "cci: failed to get cci clk:
> > %d\n",
> > +				     ret);
> > +	}
> > +
> > +	drv->inter_clk = devm_clk_get(cci_dev, "intermediate");
> > +	if (IS_ERR(drv->inter_clk)) {
> > +		ret = PTR_ERR(drv->inter_clk);
> > +		return dev_err_probe(cci_dev, ret,
> > +				     "cci: failed to get intermediate
> > clk: %d\n",
> > +				     ret);
> > +	}
> > +
> > +	if (drv->soc_data->need_voltage_tracking) {
> > +		drv->sram_reg = regulator_get_optional(cci_dev,
> > "sram");
> > +		if (IS_ERR_OR_NULL(drv->sram_reg)) {
> 
> NULL pointer cannot be returned.

Sure, I will replace it with IS_ERR() instead.

> 
> Best regards,
> Krzysztof

Re: [PATCH 3/3] devfreq: mediatek: add platform data to support mt8186

[Jia-Wei Chang]

On Mon, 2022-03-07 at 22:52 +0100, Krzysztof Kozlowski wrote:
> On 07/03/2022 13:25, Tim Chang wrote:
> > add specific platform data to support mt8186.
> > 
> 
> You just added this driver in patch 2. No need to split such actions
> because this is initial submission, so it can support two devices.
> Squash with previous.

Sure, I will squash this commit with previous one.

> 
> Best regards,
> Krzysztof

Re: [PATCH 1/3] dt-bindings: devfreq: mediatek: add mtk cci devfreq dt-bindings

[Krzysztof Kozlowski]

On 24/03/2022 13:11, Jia-Wei Chang wrote:
>>
>> Remove "driver Device Tree Bindings". "Devfreq" is Linuxism, so this
>> maybe "bus frequency scaling"? Although later you call the device
>> node
>> as cci.
> 
> Should I use "Binding for MediaTek's Cache Coherent Interconnect (CCI)
> frequency and voltage scaling" as new title?

I just suggested to remove word "bindings" so do not add it again. This
should be a title for hardware.

Now what exactly is it - you should know better than me. :)
"MediaTek's Cache Coherent Interconnect (CCI) frequency and voltage
scaling" sounds good to me, assuming that this is the hardware we talk
here about. :)

> 
>>
>>> +
>>> +maintainers:
>>> +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
>>> +
>>> +description: |
>>> +  This module is used to create CCI DEVFREQ.
>>> +  The performance will depend on both CCI frequency and CPU
>>> frequency.
>>> +  For MT8186, CCI co-buck with Little core.
>>> +  Contain CCI opp table for voltage and frequency scaling.
>>
>> Half of this description (first and last sentence) does not describe
>> the
>> actual hardware. Please describe hardware, not driver.
> 
> Sure, I will fix it in the next version.
> 
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: "mediatek,mt8186-cci"
>>
>> No need for quotes.
> 
> Sure, I will fix it in the next version.
> 
>>
>>> +
>>> +  clocks:
>>> +    items:
>>> +      - description:
>>> +          The first one is the multiplexer for clock input of CPU
>>> cluster.
>>> +      - description:
>>> +          The other is used as an intermediate clock source when
>>> the original
>>> +          CPU is under transition and not stable yet.
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: "cci"
>>> +      - const: "intermediate"
>>
>> No need for quotes.
> 
> Sure, I will fix it in the next version.
> 
>>
>>> +
>>> +  operating-points-v2:
>>> +    description:
>>> +      For details, please refer to
>>> +      Documentation/devicetree/bindings/opp/opp-v2.yaml
>>> +
>>> +  opp-table: true
>>
>> Same comments as your CPU freq bindings apply.
> 
> mtk-cci-devfreq is a new driver and its arch is same as mediatek-
> cpufreq so that the properties of mtk-cci are refer to mediatek-cpufreq 
> bindings.
> operating-point-v2 is used to determine the voltage and frequency of
> dvfs which is further utilized by mtk-cci-devfreq.

"operating-point-v2" is understood, but the same as in cpufreq bindings,
I am questioning why do you have "opp-table: true". It's a bit
confusing, so maybe I miss something?

> 
>>
>>> +
>>> +  proc-supply:
>>> +    description:
>>> +      Phandle of the regulator for CCI that provides the supply
>>> voltage.
>>> +
>>> +  sram-supply:
>>> +    description:
>>> +      Phandle of the regulator for sram of CCI that provides the
>>> supply
>>> +      voltage. When present, the cci devfreq driver needs to do
>>> +      "voltage tracking" to step by step scale up/down Vproc and
>>> Vsram to fit
>>> +      SoC specific needs. When absent, the voltage scaling flow is
>>> handled by
>>> +      hardware, hence no software "voltage tracking" is needed.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - clocks
>>> +  - clock-names
>>> +  - operating-points-v2
>>> +  - proc-supply
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/mt8186-clk.h>
>>> +    cci: cci {
>>
>> Node names should be generic and describe type of device. Are you
>> sure
>> this is a CCI? Maybe "interconnect" suits it better?
> 
> Yes, this is a CCI and it is generic type of device like CPU in my
> opinion.
> If my understanding is correct, CCI is more suitable.

OK.

> 
>>
>>> +      compatible = "mediatek,mt8186-cci";
>>> +      clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys
>>> CLK_APMIXED_MAINPLL>;
>>> +      clock-names = "cci", "intermediate";
>>> +      operating-points-v2 = <&cci_opp>;
>>> +      proc-supply = <&mt6358_vproc12_reg>;
>>> +      sram-supply = <&mt6358_vsram_proc12_reg>;
>>> +    };
>>
>>
>> Best regards,
>> Krzysztof
> 


Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: devfreq: mediatek: add mtk cci devfreq dt-bindings

[Jia-Wei Chang]

On Thu, 2022-03-24 at 13:44 +0100, Krzysztof Kozlowski wrote:
> On 24/03/2022 13:11, Jia-Wei Chang wrote:
> > > 
> > > Remove "driver Device Tree Bindings". "Devfreq" is Linuxism, so
> > > this
> > > maybe "bus frequency scaling"? Although later you call the device
> > > node
> > > as cci.
> > 
> > Should I use "Binding for MediaTek's Cache Coherent Interconnect
> > (CCI)
> > frequency and voltage scaling" as new title?
> 
> I just suggested to remove word "bindings" so do not add it again.
> This
> should be a title for hardware.

Sure, I will remove the word "bindings" from title.

> 
> Now what exactly is it - you should know better than me. :)
> "MediaTek's Cache Coherent Interconnect (CCI) frequency and voltage
> scaling" sounds good to me, assuming that this is the hardware we
> talk
> here about. :)

Appreciate your comments.
It's a bit hard to do upstream at first time, thank you for
understanding.

> 
> > 
> > > 
> > > > +
> > > > +maintainers:
> > > > +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > > > +
> > > > +description: |
> > > > +  This module is used to create CCI DEVFREQ.
> > > > +  The performance will depend on both CCI frequency and CPU
> > > > frequency.
> > > > +  For MT8186, CCI co-buck with Little core.
> > > > +  Contain CCI opp table for voltage and frequency scaling.
> > > 
> > > Half of this description (first and last sentence) does not
> > > describe
> > > the
> > > actual hardware. Please describe hardware, not driver.
> > 
> > Sure, I will fix it in the next version.
> > 
> > > 
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: "mediatek,mt8186-cci"
> > > 
> > > No need for quotes.
> > 
> > Sure, I will fix it in the next version.
> > 
> > > 
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description:
> > > > +          The first one is the multiplexer for clock input of
> > > > CPU
> > > > cluster.
> > > > +      - description:
> > > > +          The other is used as an intermediate clock source
> > > > when
> > > > the original
> > > > +          CPU is under transition and not stable yet.
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: "cci"
> > > > +      - const: "intermediate"
> > > 
> > > No need for quotes.
> > 
> > Sure, I will fix it in the next version.
> > 
> > > 
> > > > +
> > > > +  operating-points-v2:
> > > > +    description:
> > > > +      For details, please refer to
> > > > +      Documentation/devicetree/bindings/opp/opp-v2.yaml
> > > > +
> > > > +  opp-table: true
> > > 
> > > Same comments as your CPU freq bindings apply.
> > 
> > mtk-cci-devfreq is a new driver and its arch is same as mediatek-
> > cpufreq so that the properties of mtk-cci are refer to mediatek-
> > cpufreq 
> > bindings.
> > operating-point-v2 is used to determine the voltage and frequency
> > of
> > dvfs which is further utilized by mtk-cci-devfreq.
> 
> "operating-point-v2" is understood, but the same as in cpufreq
> bindings,
> I am questioning why do you have "opp-table: true". It's a bit
> confusing, so maybe I miss something?

Yes, you're correct.
"opp-table: true" should be removed.
I messed it up.

> 
> > 
> > > 
> > > > +
> > > > +  proc-supply:
> > > > +    description:
> > > > +      Phandle of the regulator for CCI that provides the
> > > > supply
> > > > voltage.
> > > > +
> > > > +  sram-supply:
> > > > +    description:
> > > > +      Phandle of the regulator for sram of CCI that provides
> > > > the
> > > > supply
> > > > +      voltage. When present, the cci devfreq driver needs to
> > > > do
> > > > +      "voltage tracking" to step by step scale up/down Vproc
> > > > and
> > > > Vsram to fit
> > > > +      SoC specific needs. When absent, the voltage scaling
> > > > flow is
> > > > handled by
> > > > +      hardware, hence no software "voltage tracking" is
> > > > needed.
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - clocks
> > > > +  - clock-names
> > > > +  - operating-points-v2
> > > > +  - proc-supply
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/clock/mt8186-clk.h>
> > > > +    cci: cci {
> > > 
> > > Node names should be generic and describe type of device. Are you
> > > sure
> > > this is a CCI? Maybe "interconnect" suits it better?
> > 
> > Yes, this is a CCI and it is generic type of device like CPU in my
> > opinion.
> > If my understanding is correct, CCI is more suitable.
> 
> OK.

:)

> 
> > 
> > > 
> > > > +      compatible = "mediatek,mt8186-cci";
> > > > +      clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys
> > > > CLK_APMIXED_MAINPLL>;
> > > > +      clock-names = "cci", "intermediate";
> > > > +      operating-points-v2 = <&cci_opp>;
> > > > +      proc-supply = <&mt6358_vproc12_reg>;
> > > > +      sram-supply = <&mt6358_vsram_proc12_reg>;
> > > > +    };
> > > 
> > > 
> > > Best regards,
> > > Krzysztof
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/3] dt-bindings: devfreq: mediatek: add mtk cci devfreq dt-bindings

[Krzysztof Kozlowski]

On 01/04/2022 15:39, Jia-Wei Chang wrote:
>>>>
>>>>> +
>>>>> +  operating-points-v2:
>>>>> +    description:
>>>>> +      For details, please refer to
>>>>> +      Documentation/devicetree/bindings/opp/opp-v2.yaml
>>>>> +
>>>>> +  opp-table: true
>>>>
>>>> Same comments as your CPU freq bindings apply.
>>>
>>> mtk-cci-devfreq is a new driver and its arch is same as mediatek-
>>> cpufreq so that the properties of mtk-cci are refer to mediatek-
>>> cpufreq 
>>> bindings.
>>> operating-point-v2 is used to determine the voltage and frequency
>>> of
>>> dvfs which is further utilized by mtk-cci-devfreq.
>>
>> "operating-point-v2" is understood, but the same as in cpufreq
>> bindings,
>> I am questioning why do you have "opp-table: true". It's a bit
>> confusing, so maybe I miss something?
> 
> Yes, you're correct.
> "opp-table: true" should be removed.
> I messed it up.

No, I think I was wrong. The opp-table pretty frequently is embedded in
the the device node itself. The operating-points-v2 references it.

You don't use it in the example, but it might be a valid usage, so it
can stay. Sorry for the confusion, it passed some time since I looked at
OPP bindings.


Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: devfreq: mediatek: add mtk cci devfreq dt-bindings

[Jia-Wei Chang]

On Sat, 2022-04-02 at 13:31 +0200, Krzysztof Kozlowski wrote:
> On 01/04/2022 15:39, Jia-Wei Chang wrote:
> > > > > 
> > > > > > +
> > > > > > +  operating-points-v2:
> > > > > > +    description:
> > > > > > +      For details, please refer to
> > > > > > +      Documentation/devicetree/bindings/opp/opp-v2.yaml
> > > > > > +
> > > > > > +  opp-table: true
> > > > > 
> > > > > Same comments as your CPU freq bindings apply.
> > > > 
> > > > mtk-cci-devfreq is a new driver and its arch is same as
> > > > mediatek-
> > > > cpufreq so that the properties of mtk-cci are refer to
> > > > mediatek-
> > > > cpufreq 
> > > > bindings.
> > > > operating-point-v2 is used to determine the voltage and
> > > > frequency
> > > > of
> > > > dvfs which is further utilized by mtk-cci-devfreq.
> > > 
> > > "operating-point-v2" is understood, but the same as in cpufreq
> > > bindings,
> > > I am questioning why do you have "opp-table: true". It's a bit
> > > confusing, so maybe I miss something?
> > 
> > Yes, you're correct.
> > "opp-table: true" should be removed.
> > I messed it up.
> 
> No, I think I was wrong. The opp-table pretty frequently is embedded
> in
> the the device node itself. The operating-points-v2 references it.
> 
> You don't use it in the example, but it might be a valid usage, so it
> can stay. Sorry for the confusion, it passed some time since I looked
> at
> OPP bindings.

You remind me of "opp-table: true" and the reason why I use it here is
exactly as you mentioned. Sorry I was not familiar enough with this to
respond it clearly and confidently.

I think it is proper to keep "opp-table: true" and add a complete opp
table information in dts example here as well.

Thanks for your comments.

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 2/3] devfreq: mediatek: add mt8183 cci devfreq driver

[Chanwoo Choi]

Hi,

It seems that this patch depends on patches[1] of devfreq-testing branch.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing

I'll send the updated patches[1]. And I add the my comment below.


On 3/7/22 9:25 PM, Tim Chang wrote:
> This adds a devfreq driver for the Cache Coherent Interconnect (CCI) of
> the Mediatek MT8183.
> 
> On mt8183 the cci is supplied by the same regulator as the little cores.
> The driver is notified when the regulator voltage changes (driven by
> cpufreq) and adjusts the cci frequency to the maximum possible value.
> 
> Add need_voltage_tracking variable to platforma data. if true, it
> indicates soc is required to realize the voltage tracking between
> voltage of sram and voltage of cci by software approach. otherwise, the
> voltage tracking is realized by hardware appraoch.
> 
> Add the notifier to cci so that it could react after svs driver changes
> opp table of cci.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>
> ---
>  drivers/devfreq/Kconfig           |  11 +-
>  drivers/devfreq/Makefile          |   2 +-
>  drivers/devfreq/mtk-cci-devfreq.c | 471 ++++++++++++++++++++++++++++++
>  3 files changed, 482 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/devfreq/mtk-cci-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 1ec36ae93f31..a6be3c6b5691 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -110,7 +110,7 @@ config ARM_IMX8M_DDRC_DEVFREQ
>  
>  config ARM_MT8183_CCI_DEVFREQ
>  	tristate "MT8183 CCI DEVFREQ Driver"
> -	depends on ARM_MEDIATEK_CPUFREQ
> +	depends on OF && ARM_MEDIATEK_CPUFREQ
>  	help

ARM_MT8183_CCI_DEVFREQ is not existed on mainline kernel.
You need to rework this patch on the latest kernel (either devfreq-next
or linux-next.git).

>  		This adds a devfreq driver for Cache Coherent Interconnect
>  		of Mediatek MT8183, which is shared the same regulator
> @@ -130,6 +130,15 @@ config ARM_TEGRA_DEVFREQ
>  	  It reads ACTMON counters of memory controllers and adjusts the
>  	  operating frequencies and voltages with OPP support.
>  
> +config ARM_MEDIATEK_CCI_DEVFREQ
> +	tristate "MEDIATEK CCI DEVFREQ Driver"
> +	depends on OF && ARM_MEDIATEK_CPUFREQ
> +	help
> +	  This adds a devfreq driver for Mediatek Cache Coherent Interconnect
> +	  which is shared the same regulator with cpu cluster. It can track
> +	  buck voltage and update a proper CCI frequency. Use notification
> +	  to get regulator status.
> +
>  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 991ef7740759..0493516a16f2 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -11,7 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  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_MT8183_CCI_DEVFREQ)	+= mt8183-cci-devfreq.o
> +obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ)	+= mtk-cci-devfreq.o

As I commented, mt8183-cci-devfreq.c is not existed on latest kernel.

>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
>  
> diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-cci-devfreq.c
> new file mode 100644
> index 000000000000..986f34689f5c
> --- /dev/null
> +++ b/drivers/devfreq/mtk-cci-devfreq.c
> @@ -0,0 +1,471 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 MediaTek Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/regulator/consumer.h>
> +
> +struct mtk_ccifreq_platform_data;

Instead of unneeded declaration,
just define 'strcut mmk_ccifreq_platform_data' at here.

> +
> +struct mtk_ccifreq_drv {
> +	struct device *cci_dev;
> +	struct devfreq *devfreq;
> +	struct regulator *proc_reg;
> +	struct regulator *sram_reg;
> +	struct clk *cci_clk;
> +	struct clk *inter_clk;
> +	int inter_voltage;
> +	int old_voltage;
> +	unsigned long old_freq;
> +	struct mutex lock;  /* avoid notify and policy race condition */
> +	struct notifier_block opp_nb;
> +	const struct mtk_ccifreq_platform_data *soc_data;
> +};
> +
> +struct mtk_ccifreq_platform_data {
> +	int min_volt_shift;
> +	int max_volt_shift;
> +	int proc_max_volt;
> +	int sram_min_volt;
> +	int sram_max_volt;
> +	bool need_voltage_tracking;
> +};

As I commented, move it before struct mtk_ccifreq_drv.

> +
> +static int mtk_ccifreq_voltage_tracking(struct mtk_ccifreq_drv *drv, int new_voltage)
> +{
> +	const struct mtk_ccifreq_platform_data *soc_data = drv->soc_data;
> +	struct regulator *proc_reg = drv->proc_reg;
> +	struct regulator *sram_reg = drv->sram_reg;
> +	int old_voltage, old_vsram, new_vsram, vsram, voltage, ret;
> +
> +	old_voltage = regulator_get_voltage(proc_reg);
> +	if (old_voltage < 0) {
> +		pr_err("%s: invalid vproc value: %d\n", __func__, old_voltage);

Use dev_err instead of pr_err and don't use '__func__'.

> +		return old_voltage;
> +	}
> +
> +	old_vsram = regulator_get_voltage(sram_reg);
> +	if (old_vsram < 0) {
> +		pr_err("%s: invalid vsram value: %d\n", __func__, old_vsram);

ditto.

> +		return old_vsram;
> +	}
> +
> +	new_vsram = clamp(new_voltage + soc_data->min_volt_shift,
> +			  soc_data->sram_min_volt, soc_data->sram_max_volt);
> +
> +	do {
> +		if (old_voltage <= new_voltage) {
> +			vsram = clamp(old_voltage + soc_data->max_volt_shift,
> +				      soc_data->sram_min_volt, new_vsram);
> +			ret = regulator_set_voltage(sram_reg, vsram,
> +						    soc_data->sram_max_volt);
> +			if (ret)
> +				return ret;
> +
> +			if (vsram == soc_data->sram_max_volt ||
> +			    new_vsram == soc_data->sram_min_volt)
> +				voltage = new_voltage;
> +			else
> +				voltage = vsram - soc_data->min_volt_shift;
> +
> +			ret = regulator_set_voltage(proc_reg, voltage,
> +						    soc_data->proc_max_volt);
> +			if (ret) {
> +				regulator_set_voltage(sram_reg, old_vsram,
> +						      soc_data->sram_max_volt);
> +				return ret;
> +			}
> +		} else if (old_voltage > new_voltage) {
> +			voltage = max(new_voltage,
> +				    old_vsram - soc_data->max_volt_shift);
> +			ret = regulator_set_voltage(proc_reg, voltage,
> +						    soc_data->proc_max_volt);
> +			if (ret)
> +				return ret;
> +
> +			if (voltage == new_voltage)
> +				vsram = new_vsram;
> +			else
> +				vsram = max(new_vsram,
> +					    voltage + soc_data->min_volt_shift);
> +
> +			ret = regulator_set_voltage(sram_reg, vsram,
> +						    soc_data->sram_max_volt);
> +			if (ret) {
> +				regulator_set_voltage(proc_reg, old_voltage,
> +						      soc_data->proc_max_volt);
> +				return ret;
> +			}
> +		}
> +
> +		old_voltage = voltage;
> +		old_vsram = vsram;
> +	} while (voltage != new_voltage || vsram != new_vsram);
> +
> +	return 0;
> +}
> +
> +static int mtk_ccifreq_set_voltage(struct mtk_ccifreq_drv *drv, int voltage)
> +{
> +	int ret;
> +
> +	if (drv->soc_data->need_voltage_tracking)
> +		ret = mtk_ccifreq_voltage_tracking(drv, voltage);
> +	else
> +		ret = regulator_set_voltage(drv->proc_reg, voltage,
> +					    drv->soc_data->proc_max_volt);
> +
> +	if (!ret)
> +		drv->old_voltage = voltage;
> +
> +	return ret;
> +}
> +
> +static int mtk_ccifreq_target(struct device *dev, unsigned long *freq,
> +			      u32 flags)
> +{

I recommend that you handle the error case by using 'goto' statement
instead of handling them in each error case. You can refer the 
other devfreq device driver for handling error case with goto.

> +	struct mtk_ccifreq_drv *drv = dev_get_drvdata(dev);
> +	struct clk *cci_pll = clk_get_parent(drv->cci_clk);
> +	struct dev_pm_opp *opp;
> +	unsigned long opp_rate;
> +	int voltage, old_voltage, inter_voltage, target_voltage, ret;
> +
> +	if (!drv)
> +		return -EINVAL;
> +
> +	if (drv->old_freq == *freq)
> +		return 0;
> +
> +	inter_voltage = drv->inter_voltage;
> +
> +	opp_rate = *freq;
> +	opp = devfreq_recommended_opp(dev, &opp_rate, 1);
> +	if (IS_ERR(opp)) {
> +		pr_err("cci: failed to find opp for freq: %ld\n", opp_rate);

ditto.

> +		return PTR_ERR(opp);
> +	}
> +	voltage = dev_pm_opp_get_voltage(opp);
> +	dev_pm_opp_put(opp);
> +
> +	old_voltage = drv->old_voltage;
> +	if (old_voltage == 0)
> +		old_voltage = regulator_get_voltage(drv->proc_reg);
> +	if (old_voltage < 0) {
> +		pr_err("cci: invalid vproc value: %d\n", old_voltage);

ditto. Use dev_err instead of pr_err.

> +		return old_voltage;
> +	}
> +
> +	mutex_lock(&drv->lock);
> +
> +	/* scale up: set voltage first then freq. */
> +	target_voltage = max(inter_voltage, voltage);
> +	if (old_voltage <= target_voltage) {
> +		ret = mtk_ccifreq_set_voltage(drv, target_voltage);
> +		if (ret) {
> +			pr_err("cci: failed to scale up voltage\n");

ditto.

> +			mtk_ccifreq_set_voltage(drv, old_voltage);
> +			mutex_unlock(&drv->lock);
> +			return ret;
> +		}
> +	}
> +
> +	/* switch the cci clock to intermediate clock source. */
> +	ret = clk_set_parent(drv->cci_clk, drv->inter_clk);
> +	if (ret) {
> +		pr_err("cci: failed to re-parent cci clock\n");

ditto.

> +		mtk_ccifreq_set_voltage(drv, old_voltage);
> +		WARN_ON(1);

I think that it is not necessary. Please remove it.

> +		mutex_unlock(&drv->lock);
> +		return ret;
> +	}
> +
> +	/* set the original clock to target rate. */
> +	ret = clk_set_rate(cci_pll, *freq);
> +	if (ret) {
> +		pr_err("cci: failed to set cci pll rate: %d\n", ret);

ditto.

> +		clk_set_parent(drv->cci_clk, cci_pll);
> +		mtk_ccifreq_set_voltage(drv, old_voltage);
> +		mutex_unlock(&drv->lock);
> +		return ret;
> +	}
> +
> +	/* switch the cci clock back to the original clock source. */
> +	ret = clk_set_parent(drv->cci_clk, cci_pll);
> +	if (ret) {
> +		pr_err("cci: failed to re-parent cci clock\n");

ditto.

> +		mtk_ccifreq_set_voltage(drv, inter_voltage);
> +		WARN_ON(1);

ditto.

> +		mutex_unlock(&drv->lock);
> +		return ret;
> +	}
> +
> +	/*
> +	 * If the new voltage is lower than the intermediate voltage or the
> +	 * original voltage, scale down to the new voltage.
> +	 */
> +	if (voltage < inter_voltage || voltage < old_voltage) {
> +		ret = mtk_ccifreq_set_voltage(drv, voltage);
> +		if (ret) {
> +			pr_err("cci: failed to scale down voltage\n");

ditto.

> +			WARN_ON(1);

ditto.

> +			mutex_unlock(&drv->lock);
> +			return ret;
> +		}
> +	}
> +
> +	drv->old_freq = *freq;
> +	mutex_unlock(&drv->lock);
> +
> +	return 0;
> +}
> +
> +static int mtk_ccifreq_opp_notifier(struct notifier_block *nb,
> +				    unsigned long event, void *data)
> +{
> +	struct dev_pm_opp *opp = data;
> +	struct mtk_ccifreq_drv *drv;
> +	unsigned long freq, volt;
> +
> +	drv = container_of(nb, struct mtk_ccifreq_drv, opp_nb);
> +
> +	if (event == OPP_EVENT_ADJUST_VOLTAGE) {
> +		freq = dev_pm_opp_get_freq(opp);
> +
> +		mutex_lock(&drv->lock);
> +		/* current opp item is changed */
> +		if (freq == drv->old_freq) {
> +			volt = dev_pm_opp_get_voltage(opp);
> +			mtk_ccifreq_set_voltage(drv, volt);
> +		}
> +		mutex_unlock(&drv->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile mtk_ccifreq_profile = {
> +	.target = mtk_ccifreq_target,
> +};
> +
> +static int mtk_ccifreq_probe(struct platform_device *pdev)
> +{
> +	struct device *cci_dev = &pdev->dev;
> +	struct mtk_ccifreq_drv *drv;
> +	struct devfreq_passive_data *passive_data;
> +	struct dev_pm_opp *opp;
> +	unsigned long rate, opp_volt;
> +	int ret;
> +
> +	drv = devm_kzalloc(cci_dev, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	drv->cci_dev = cci_dev;
> +	drv->soc_data = (const struct mtk_ccifreq_platform_data *)
> +				of_device_get_match_data(&pdev->dev);
> +	mutex_init(&drv->lock);
> +	platform_set_drvdata(pdev, drv);
> +
> +	drv->cci_clk = devm_clk_get(cci_dev, "cci");
> +	if (IS_ERR(drv->cci_clk)) {
> +		ret = PTR_ERR(drv->cci_clk);
> +		return dev_err_probe(cci_dev, ret,
> +				     "cci: failed to get cci clk: %d\n",
> +				     ret);
> +	}
> +
> +	drv->inter_clk = devm_clk_get(cci_dev, "intermediate");
> +	if (IS_ERR(drv->inter_clk)) {
> +		ret = PTR_ERR(drv->inter_clk);
> +		return dev_err_probe(cci_dev, ret,
> +				     "cci: failed to get intermediate clk: %d\n",
> +				     ret);
> +	}
> +
> +	if (drv->soc_data->need_voltage_tracking) {
> +		drv->sram_reg = regulator_get_optional(cci_dev, "sram");
> +		if (IS_ERR_OR_NULL(drv->sram_reg)) {
> +			ret = PTR_ERR(drv->sram_reg);
> +			return dev_err_probe(cci_dev, ret,
> +					     "cci: failed to get sram regulator: %d\n",
> +					     ret);
> +		}
> +
> +		ret = regulator_enable(drv->sram_reg);
> +		if (ret) {
> +			dev_warn(cci_dev,
> +				 "cci: failed to enable sram regulator\n");

If you want to break the probe sequence, better to use 'dev_err'
instead of 'dev_warn' if there is no any reason to use dev_warn.


> +			return ret;
> +		}
> +	}
> +
> +	drv->proc_reg = devm_regulator_get_optional(cci_dev, "proc");
> +	if (IS_ERR(drv->proc_reg)) {
> +		ret = PTR_ERR(drv->proc_reg);
> +		dev_err_probe(cci_dev, ret,
> +			      "cci: failed to get proc regulator: %d\n",
> +			      ret);
> +		goto out_disable_sram_reg;
> +	}
> +
> +	ret = regulator_enable(drv->proc_reg);
> +	if (ret) {
> +		dev_warn(cci_dev, "cci: failed to enable proc regulator\n");

ditto. Use dev_err.

> +		goto out_disable_sram_reg;
> +	}
> +
> +	ret = clk_prepare_enable(drv->cci_clk);
> +	if (ret)
> +		goto out_disable_proc_reg;
> +
> +	ret = clk_prepare_enable(drv->inter_clk);
> +	if (ret)
> +		goto out_disable_cci_clk;
> +
> +	ret = dev_pm_opp_of_add_table(cci_dev);
> +	if (ret) {
> +		dev_warn(cci_dev, "cci: failed to add opp table: %d\n", ret);
> +		goto out_disable_inter_clk;
> +	}
> +
> +	rate = clk_get_rate(drv->inter_clk);
> +	opp = dev_pm_opp_find_freq_ceil(cci_dev, &rate);
> +	if (IS_ERR(opp)) {
> +		ret = PTR_ERR(opp);
> +		dev_err(cci_dev, "cci: failed to get intermediate opp: %d\n",
> +			ret);
> +		goto out_remove_opp_table;
> +	}
> +	drv->inter_voltage = dev_pm_opp_get_voltage(opp);
> +	dev_pm_opp_put(opp);
> +
> +	rate = U32_MAX;
> +	opp = dev_pm_opp_find_freq_floor(drv->cci_dev, &rate);
> +	if (IS_ERR(opp)) {
> +		pr_err("failed to get opp\n");
> +		ret = PTR_ERR(opp);
> +		goto out_remove_opp_table;
> +	}
> +
> +	opp_volt = dev_pm_opp_get_voltage(opp);
> +	dev_pm_opp_put(opp);
> +	ret = mtk_ccifreq_set_voltage(drv, opp_volt);
> +	if (ret) {
> +		pr_err("failed to scale to highest voltage %lu in proc_reg\n",
> +		       opp_volt);

Use dev_err.

> +		goto out_remove_opp_table;
> +	}
> +
> +	passive_data = devm_kzalloc(cci_dev, sizeof(struct devfreq_passive_data), GFP_KERNEL);
> +	if (!passive_data) {
> +		ret = -ENOMEM;
> +		goto out_remove_opp_table;
> +	}
> +
> +	passive_data->parent_type = CPUFREQ_PARENT_DEV;

It seems that depend on patchset[1]. I'll send the updated patch 
within one week.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing

> +	drv->devfreq = devm_devfreq_add_device(cci_dev,
> +					       &mtk_ccifreq_profile,
> +					       DEVFREQ_GOV_PASSIVE,
> +					       passive_data);
> +	if (IS_ERR(drv->devfreq)) {
> +		ret = -EPROBE_DEFER;
> +		dev_err(cci_dev, "cci: failed to add devfreq device: %d\n",
> +			PTR_ERR(drv->devfreq));
> +		goto out_remove_opp_table;
> +	}
> +
> +	drv->opp_nb.notifier_call = mtk_ccifreq_opp_notifier;
> +	ret = dev_pm_opp_register_notifier(cci_dev, &drv->opp_nb);
> +	if (ret) {
> +		dev_warn(cci_dev, "cci: failed to register opp notifier: %d\n",
> +			 ret);
> +		goto out_remove_devfreq_device;
> +	}
> +
> +	return 0;
> +
> +out_remove_devfreq_device:
> +	devm_devfreq_remove_device(cci_dev, drv->devfreq);
> +
> +out_remove_opp_table:
> +	dev_pm_opp_of_remove_table(cci_dev);
> +
> +out_disable_inter_clk:
> +	clk_disable_unprepare(drv->inter_clk);
> +
> +out_disable_cci_clk:
> +	clk_disable_unprepare(drv->cci_clk);
> +
> +out_disable_proc_reg:
> +	regulator_disable(drv->proc_reg);
> +
> +out_disable_sram_reg:
> +	if (drv->soc_data->need_voltage_tracking)
> +		regulator_disable(drv->sram_reg);
> +
> +	return ret;
> +}
> +
> +static int mtk_ccifreq_remove(struct platform_device *pdev)
> +{
> +	struct device *cci_dev = &pdev->dev;
> +	struct mtk_ccifreq_drv *drv;
> +
> +	drv = platform_get_drvdata(pdev);
> +
> +	dev_pm_opp_unregister_notifier(cci_dev, &drv->opp_nb);
> +	dev_pm_opp_of_remove_table(cci_dev);
> +	regulator_disable(drv->proc_reg);
> +	if (!IS_ERR(drv->sram_reg))
> +		regulator_disable(drv->sram_reg);
> +
> +	return 0;
> +}
> +
> +static const struct mtk_ccifreq_platform_data mtk_platform_data = {
> +	.min_volt_shift = 0,
> +	.max_volt_shift = 0,
> +	.proc_max_volt = 1150000,
> +	.sram_min_volt = 0,
> +	.sram_max_volt = 0,
> +	.need_voltage_tracking = false,
> +};
> +
> +static const struct of_device_id mtk_ccifreq_machines[] = {
> +	{ .compatible = "mediatek,mt8183-cci", .data = &mtk_platform_data },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_ccifreq_machines);
> +
> +static struct platform_driver mtk_ccifreq_platdrv = {
> +	.probe	= mtk_ccifreq_probe,
> +	.remove	= mtk_ccifreq_remove,
> +	.driver = {
> +		.name = "mtk-ccifreq",
> +		.of_match_table = of_match_ptr(mtk_ccifreq_machines),
> +	},
> +};
> +
> +static int __init mtk_ccifreq_platdrv_init(void)
> +{
> +	return platform_driver_register(&mtk_ccifreq_platdrv);
> +}
> +module_init(mtk_ccifreq_platdrv_init)
> +
> +static void __exit mtk_ccifreq_platdrv_exit(void)
> +{
> +	platform_driver_unregister(&mtk_ccifreq_platdrv);
> +}
> +module_exit(mtk_ccifreq_platdrv_exit)
> +
> +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> +MODULE_AUTHOR("Jia-Wei Chang <jia-wei.chang@mediatek.com>");
> +MODULE_LICENSE("GPL v2");
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/3] devfreq: mediatek: add mt8183 cci devfreq driver

[Jia-Wei Chang]

On Thu, 2022-04-07 at 12:20 +0900, Chanwoo Choi wrote:
> Hi,
> 
> It seems that this patch depends on patches[1] of devfreq-testing
> branch.
> [1] 
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing__;!!CTRNKA9wMg0ARbw!zpzMoFhloy2fpTawf8kkY5nwKerht5CKCnKTwwmSHYBN2vN5WfVMOcGn5TDb8SPTQTSaHA$
>  
> 
> I'll send the updated patches[1]. And I add the my comment below.
> 

Yes, I will add your patches as my dependency.

> 
> On 3/7/22 9:25 PM, Tim Chang wrote:
> > This adds a devfreq driver for the Cache Coherent Interconnect
> > (CCI) of
> > the Mediatek MT8183.
> > 
> > On mt8183 the cci is supplied by the same regulator as the little
> > cores.
> > The driver is notified when the regulator voltage changes (driven
> > by
> > cpufreq) and adjusts the cci frequency to the maximum possible
> > value.
> > 
> > Add need_voltage_tracking variable to platforma data. if true, it
> > indicates soc is required to realize the voltage tracking between
> > voltage of sram and voltage of cci by software approach. otherwise,
> > the
> > voltage tracking is realized by hardware appraoch.
> > 
> > Add the notifier to cci so that it could react after svs driver
> > changes
> > opp table of cci.
> > 
> > Signed-off-by: Jia-Wei Chang <
> > jia-wei.chang@mediatek.corp-partner.google.com>
> > ---
> >  drivers/devfreq/Kconfig           |  11 +-
> >  drivers/devfreq/Makefile          |   2 +-
> >  drivers/devfreq/mtk-cci-devfreq.c | 471
> > ++++++++++++++++++++++++++++++
> >  3 files changed, 482 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/devfreq/mtk-cci-devfreq.c
> > 
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 1ec36ae93f31..a6be3c6b5691 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -110,7 +110,7 @@ config ARM_IMX8M_DDRC_DEVFREQ
> >  
> >  config ARM_MT8183_CCI_DEVFREQ
> >  	tristate "MT8183 CCI DEVFREQ Driver"
> > -	depends on ARM_MEDIATEK_CPUFREQ
> > +	depends on OF && ARM_MEDIATEK_CPUFREQ
> >  	help
> 
> ARM_MT8183_CCI_DEVFREQ is not existed on mainline kernel.
> You need to rework this patch on the latest kernel (either devfreq-
> next
> or linux-next.git).

Sure, I will rebase my patch onto linux-next.

> 
> >  		This adds a devfreq driver for Cache Coherent
> > Interconnect
> >  		of Mediatek MT8183, which is shared the same regulator
> > @@ -130,6 +130,15 @@ config ARM_TEGRA_DEVFREQ
> >  	  It reads ACTMON counters of memory controllers and adjusts
> > the
> >  	  operating frequencies and voltages with OPP support.
> >  
> > +config ARM_MEDIATEK_CCI_DEVFREQ
> > +	tristate "MEDIATEK CCI DEVFREQ Driver"
> > +	depends on OF && ARM_MEDIATEK_CPUFREQ
> > +	help
> > +	  This adds a devfreq driver for Mediatek Cache Coherent
> > Interconnect
> > +	  which is shared the same regulator with cpu cluster. It can
> > track
> > +	  buck voltage and update a proper CCI frequency. Use
> > notification
> > +	  to get regulator status.
> > +
> >  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 991ef7740759..0493516a16f2 100644
> > --- a/drivers/devfreq/Makefile
> > +++ b/drivers/devfreq/Makefile
> > @@ -11,7 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+=
> > governor_passive.o
> >  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_MT8183_CCI_DEVFREQ)	+= mt8183-cci-devfreq.o
> > +obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ)	+= mtk-cci-devfreq.o
> 
> As I commented, mt8183-cci-devfreq.c is not existed on latest kernel.

Yes, it will be fixed after rebasing onto linux-next.

> 
> >  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
> >  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
> >  
> > diff --git a/drivers/devfreq/mtk-cci-devfreq.c
> > b/drivers/devfreq/mtk-cci-devfreq.c
> > new file mode 100644
> > index 000000000000..986f34689f5c
> > --- /dev/null
> > +++ b/drivers/devfreq/mtk-cci-devfreq.c
> > @@ -0,0 +1,471 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2022 MediaTek Inc.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/devfreq.h>
> > +#include <linux/minmax.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +struct mtk_ccifreq_platform_data;
> 
> Instead of unneeded declaration,
> just define 'strcut mmk_ccifreq_platform_data' at here.

Yes, I will remove this forward declaration.

> 
> > +
> > +struct mtk_ccifreq_drv {
> > +	struct device *cci_dev;
> > +	struct devfreq *devfreq;
> > +	struct regulator *proc_reg;
> > +	struct regulator *sram_reg;
> > +	struct clk *cci_clk;
> > +	struct clk *inter_clk;
> > +	int inter_voltage;
> > +	int old_voltage;
> > +	unsigned long old_freq;
> > +	struct mutex lock;  /* avoid notify and policy race condition
> > */
> > +	struct notifier_block opp_nb;
> > +	const struct mtk_ccifreq_platform_data *soc_data;
> > +};
> > +
> > +struct mtk_ccifreq_platform_data {
> > +	int min_volt_shift;
> > +	int max_volt_shift;
> > +	int proc_max_volt;
> > +	int sram_min_volt;
> > +	int sram_max_volt;
> > +	bool need_voltage_tracking;
> > +};
> 
> As I commented, move it before struct mtk_ccifreq_drv.

Yes, I will update it in the next version.

> 
> > +
> > +static int mtk_ccifreq_voltage_tracking(struct mtk_ccifreq_drv
> > *drv, int new_voltage)
> > +{
> > +	const struct mtk_ccifreq_platform_data *soc_data = drv-
> > >soc_data;
> > +	struct regulator *proc_reg = drv->proc_reg;
> > +	struct regulator *sram_reg = drv->sram_reg;
> > +	int old_voltage, old_vsram, new_vsram, vsram, voltage, ret;
> > +
> > +	old_voltage = regulator_get_voltage(proc_reg);
> > +	if (old_voltage < 0) {
> > +		pr_err("%s: invalid vproc value: %d\n", __func__,
> > old_voltage);
> 
> Use dev_err instead of pr_err and don't use '__func__'.

Yes, I will fix it in the next version.

> 
> > +		return old_voltage;
> > +	}
> > +
> > +	old_vsram = regulator_get_voltage(sram_reg);
> > +	if (old_vsram < 0) {
> > +		pr_err("%s: invalid vsram value: %d\n", __func__,
> > old_vsram);
> 
> ditto.

Yes, I will fix it in the next version.

> 
> > +		return old_vsram;
> > +	}
> > +
> > +	new_vsram = clamp(new_voltage + soc_data->min_volt_shift,
> > +			  soc_data->sram_min_volt, soc_data-
> > >sram_max_volt);
> > +
> > +	do {
> > +		if (old_voltage <= new_voltage) {
> > +			vsram = clamp(old_voltage + soc_data-
> > >max_volt_shift,
> > +				      soc_data->sram_min_volt,
> > new_vsram);
> > +			ret = regulator_set_voltage(sram_reg, vsram,
> > +						    soc_data-
> > >sram_max_volt);
> > +			if (ret)
> > +				return ret;
> > +
> > +			if (vsram == soc_data->sram_max_volt ||
> > +			    new_vsram == soc_data->sram_min_volt)
> > +				voltage = new_voltage;
> > +			else
> > +				voltage = vsram - soc_data-
> > >min_volt_shift;
> > +
> > +			ret = regulator_set_voltage(proc_reg, voltage,
> > +						    soc_data-
> > >proc_max_volt);
> > +			if (ret) {
> > +				regulator_set_voltage(sram_reg,
> > old_vsram,
> > +						      soc_data-
> > >sram_max_volt);
> > +				return ret;
> > +			}
> > +		} else if (old_voltage > new_voltage) {
> > +			voltage = max(new_voltage,
> > +				    old_vsram - soc_data-
> > >max_volt_shift);
> > +			ret = regulator_set_voltage(proc_reg, voltage,
> > +						    soc_data-
> > >proc_max_volt);
> > +			if (ret)
> > +				return ret;
> > +
> > +			if (voltage == new_voltage)
> > +				vsram = new_vsram;
> > +			else
> > +				vsram = max(new_vsram,
> > +					    voltage + soc_data-
> > >min_volt_shift);
> > +
> > +			ret = regulator_set_voltage(sram_reg, vsram,
> > +						    soc_data-
> > >sram_max_volt);
> > +			if (ret) {
> > +				regulator_set_voltage(proc_reg,
> > old_voltage,
> > +						      soc_data-
> > >proc_max_volt);
> > +				return ret;
> > +			}
> > +		}
> > +
> > +		old_voltage = voltage;
> > +		old_vsram = vsram;
> > +	} while (voltage != new_voltage || vsram != new_vsram);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_ccifreq_set_voltage(struct mtk_ccifreq_drv *drv,
> > int voltage)
> > +{
> > +	int ret;
> > +
> > +	if (drv->soc_data->need_voltage_tracking)
> > +		ret = mtk_ccifreq_voltage_tracking(drv, voltage);
> > +	else
> > +		ret = regulator_set_voltage(drv->proc_reg, voltage,
> > +					    drv->soc_data-
> > >proc_max_volt);
> > +
> > +	if (!ret)
> > +		drv->old_voltage = voltage;
> > +
> > +	return ret;
> > +}
> > +
> > +static int mtk_ccifreq_target(struct device *dev, unsigned long
> > *freq,
> > +			      u32 flags)
> > +{
> 
> I recommend that you handle the error case by using 'goto' statement
> instead of handling them in each error case. You can refer the 
> other devfreq device driver for handling error case with goto.

Sure, I will refactor it with 'goto' statement in the next version.

> 
> > +	struct mtk_ccifreq_drv *drv = dev_get_drvdata(dev);
> > +	struct clk *cci_pll = clk_get_parent(drv->cci_clk);
> > +	struct dev_pm_opp *opp;
> > +	unsigned long opp_rate;
> > +	int voltage, old_voltage, inter_voltage, target_voltage, ret;
> > +
> > +	if (!drv)
> > +		return -EINVAL;
> > +
> > +	if (drv->old_freq == *freq)
> > +		return 0;
> > +
> > +	inter_voltage = drv->inter_voltage;
> > +
> > +	opp_rate = *freq;
> > +	opp = devfreq_recommended_opp(dev, &opp_rate, 1);
> > +	if (IS_ERR(opp)) {
> > +		pr_err("cci: failed to find opp for freq: %ld\n",
> > opp_rate);
> 
> ditto.

Ok, I will use 'goto' to handle error.

> 
> > +		return PTR_ERR(opp);
> > +	}
> > +	voltage = dev_pm_opp_get_voltage(opp);
> > +	dev_pm_opp_put(opp);
> > +
> > +	old_voltage = drv->old_voltage;
> > +	if (old_voltage == 0)
> > +		old_voltage = regulator_get_voltage(drv->proc_reg);
> > +	if (old_voltage < 0) {
> > +		pr_err("cci: invalid vproc value: %d\n", old_voltage);
> 
> ditto. Use dev_err instead of pr_err.

Ok, I will use dev_err instread.

> 
> > +		return old_voltage;
> > +	}
> > +
> > +	mutex_lock(&drv->lock);
> > +
> > +	/* scale up: set voltage first then freq. */
> > +	target_voltage = max(inter_voltage, voltage);
> > +	if (old_voltage <= target_voltage) {
> > +		ret = mtk_ccifreq_set_voltage(drv, target_voltage);
> > +		if (ret) {
> > +			pr_err("cci: failed to scale up voltage\n");
> 
> ditto.

Ok, I will use dev_err instread.

> 
> > +			mtk_ccifreq_set_voltage(drv, old_voltage);
> > +			mutex_unlock(&drv->lock);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	/* switch the cci clock to intermediate clock source. */
> > +	ret = clk_set_parent(drv->cci_clk, drv->inter_clk);
> > +	if (ret) {
> > +		pr_err("cci: failed to re-parent cci clock\n");
> 
> ditto.

Ok, I will use dev_err instread.

> 
> > +		mtk_ccifreq_set_voltage(drv, old_voltage);
> > +		WARN_ON(1);
> 
> I think that it is not necessary. Please remove it.

Yes, I will remove it.

> 
> > +		mutex_unlock(&drv->lock);
> > +		return ret;
> > +	}
> > +
> > +	/* set the original clock to target rate. */
> > +	ret = clk_set_rate(cci_pll, *freq);
> > +	if (ret) {
> > +		pr_err("cci: failed to set cci pll rate: %d\n", ret);
> 
> ditto.

Ok, I will use dev_err instread.

> 
> > +		clk_set_parent(drv->cci_clk, cci_pll);
> > +		mtk_ccifreq_set_voltage(drv, old_voltage);
> > +		mutex_unlock(&drv->lock);
> > +		return ret;
> > +	}
> > +
> > +	/* switch the cci clock back to the original clock source. */
> > +	ret = clk_set_parent(drv->cci_clk, cci_pll);
> > +	if (ret) {
> > +		pr_err("cci: failed to re-parent cci clock\n");
> 
> ditto.

Ok, I will use dev_err instread.

> 
> > +		mtk_ccifreq_set_voltage(drv, inter_voltage);
> > +		WARN_ON(1);
> 
> ditto.

Ok, I will remove it.

> 
> > +		mutex_unlock(&drv->lock);
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * If the new voltage is lower than the intermediate voltage or
> > the
> > +	 * original voltage, scale down to the new voltage.
> > +	 */
> > +	if (voltage < inter_voltage || voltage < old_voltage) {
> > +		ret = mtk_ccifreq_set_voltage(drv, voltage);
> > +		if (ret) {
> > +			pr_err("cci: failed to scale down voltage\n");
> 
> ditto.

Ok, I will use dev_err instread.

> 
> > +			WARN_ON(1);
> 
> ditto.

Ok, I will remove it.

> 
> > +			mutex_unlock(&drv->lock);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	drv->old_freq = *freq;
> > +	mutex_unlock(&drv->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_ccifreq_opp_notifier(struct notifier_block *nb,
> > +				    unsigned long event, void *data)
> > +{
> > +	struct dev_pm_opp *opp = data;
> > +	struct mtk_ccifreq_drv *drv;
> > +	unsigned long freq, volt;
> > +
> > +	drv = container_of(nb, struct mtk_ccifreq_drv, opp_nb);
> > +
> > +	if (event == OPP_EVENT_ADJUST_VOLTAGE) {
> > +		freq = dev_pm_opp_get_freq(opp);
> > +
> > +		mutex_lock(&drv->lock);
> > +		/* current opp item is changed */
> > +		if (freq == drv->old_freq) {
> > +			volt = dev_pm_opp_get_voltage(opp);
> > +			mtk_ccifreq_set_voltage(drv, volt);
> > +		}
> > +		mutex_unlock(&drv->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct devfreq_dev_profile mtk_ccifreq_profile = {
> > +	.target = mtk_ccifreq_target,
> > +};
> > +
> > +static int mtk_ccifreq_probe(struct platform_device *pdev)
> > +{
> > +	struct device *cci_dev = &pdev->dev;
> > +	struct mtk_ccifreq_drv *drv;
> > +	struct devfreq_passive_data *passive_data;
> > +	struct dev_pm_opp *opp;
> > +	unsigned long rate, opp_volt;
> > +	int ret;
> > +
> > +	drv = devm_kzalloc(cci_dev, sizeof(*drv), GFP_KERNEL);
> > +	if (!drv)
> > +		return -ENOMEM;
> > +
> > +	drv->cci_dev = cci_dev;
> > +	drv->soc_data = (const struct mtk_ccifreq_platform_data *)
> > +				of_device_get_match_data(&pdev->dev);
> > +	mutex_init(&drv->lock);
> > +	platform_set_drvdata(pdev, drv);
> > +
> > +	drv->cci_clk = devm_clk_get(cci_dev, "cci");
> > +	if (IS_ERR(drv->cci_clk)) {
> > +		ret = PTR_ERR(drv->cci_clk);
> > +		return dev_err_probe(cci_dev, ret,
> > +				     "cci: failed to get cci clk:
> > %d\n",
> > +				     ret);
> > +	}
> > +
> > +	drv->inter_clk = devm_clk_get(cci_dev, "intermediate");
> > +	if (IS_ERR(drv->inter_clk)) {
> > +		ret = PTR_ERR(drv->inter_clk);
> > +		return dev_err_probe(cci_dev, ret,
> > +				     "cci: failed to get intermediate
> > clk: %d\n",
> > +				     ret);
> > +	}
> > +
> > +	if (drv->soc_data->need_voltage_tracking) {
> > +		drv->sram_reg = regulator_get_optional(cci_dev,
> > "sram");
> > +		if (IS_ERR_OR_NULL(drv->sram_reg)) {
> > +			ret = PTR_ERR(drv->sram_reg);
> > +			return dev_err_probe(cci_dev, ret,
> > +					     "cci: failed to get sram
> > regulator: %d\n",
> > +					     ret);
> > +		}
> > +
> > +		ret = regulator_enable(drv->sram_reg);
> > +		if (ret) {
> > +			dev_warn(cci_dev,
> > +				 "cci: failed to enable sram
> > regulator\n");
> 
> If you want to break the probe sequence, better to use 'dev_err'
> instead of 'dev_warn' if there is no any reason to use dev_warn.
> 

Ok, I will use dev_err.

> 
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	drv->proc_reg = devm_regulator_get_optional(cci_dev, "proc");
> > +	if (IS_ERR(drv->proc_reg)) {
> > +		ret = PTR_ERR(drv->proc_reg);
> > +		dev_err_probe(cci_dev, ret,
> > +			      "cci: failed to get proc regulator:
> > %d\n",
> > +			      ret);
> > +		goto out_disable_sram_reg;
> > +	}
> > +
> > +	ret = regulator_enable(drv->proc_reg);
> > +	if (ret) {
> > +		dev_warn(cci_dev, "cci: failed to enable proc
> > regulator\n");
> 
> ditto. Use dev_err.

Ok, I will replace all dev_warn with dev_err in the patch.

> 
> > +		goto out_disable_sram_reg;
> > +	}
> > +
> > +	ret = clk_prepare_enable(drv->cci_clk);
> > +	if (ret)
> > +		goto out_disable_proc_reg;
> > +
> > +	ret = clk_prepare_enable(drv->inter_clk);
> > +	if (ret)
> > +		goto out_disable_cci_clk;
> > +
> > +	ret = dev_pm_opp_of_add_table(cci_dev);
> > +	if (ret) {
> > +		dev_warn(cci_dev, "cci: failed to add opp table: %d\n",
> > ret);
> > +		goto out_disable_inter_clk;
> > +	}
> > +
> > +	rate = clk_get_rate(drv->inter_clk);
> > +	opp = dev_pm_opp_find_freq_ceil(cci_dev, &rate);
> > +	if (IS_ERR(opp)) {
> > +		ret = PTR_ERR(opp);
> > +		dev_err(cci_dev, "cci: failed to get intermediate opp:
> > %d\n",
> > +			ret);
> > +		goto out_remove_opp_table;
> > +	}
> > +	drv->inter_voltage = dev_pm_opp_get_voltage(opp);
> > +	dev_pm_opp_put(opp);
> > +
> > +	rate = U32_MAX;
> > +	opp = dev_pm_opp_find_freq_floor(drv->cci_dev, &rate);
> > +	if (IS_ERR(opp)) {
> > +		pr_err("failed to get opp\n");
> > +		ret = PTR_ERR(opp);
> > +		goto out_remove_opp_table;
> > +	}
> > +
> > +	opp_volt = dev_pm_opp_get_voltage(opp);
> > +	dev_pm_opp_put(opp);
> > +	ret = mtk_ccifreq_set_voltage(drv, opp_volt);
> > +	if (ret) {
> > +		pr_err("failed to scale to highest voltage %lu in
> > proc_reg\n",
> > +		       opp_volt);
> 
> Use dev_err.

Ok, I will use dev_err instread.

> 
> > +		goto out_remove_opp_table;
> > +	}
> > +
> > +	passive_data = devm_kzalloc(cci_dev, sizeof(struct
> > devfreq_passive_data), GFP_KERNEL);
> > +	if (!passive_data) {
> > +		ret = -ENOMEM;
> > +		goto out_remove_opp_table;
> > +	}
> > +
> > +	passive_data->parent_type = CPUFREQ_PARENT_DEV;
> 
> It seems that depend on patchset[1]. I'll send the updated patch 
> within one week.
> [1] 
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing__;!!CTRNKA9wMg0ARbw!zpzMoFhloy2fpTawf8kkY5nwKerht5CKCnKTwwmSHYBN2vN5WfVMOcGn5TDb8SPTQTSaHA$
>  

Should I send my patch after yours? Or the order makes no difference?

> 
> > +	drv->devfreq = devm_devfreq_add_device(cci_dev,
> > +					       &mtk_ccifreq_profile,
> > +					       DEVFREQ_GOV_PASSIVE,
> > +					       passive_data);
> > +	if (IS_ERR(drv->devfreq)) {
> > +		ret = -EPROBE_DEFER;
> > +		dev_err(cci_dev, "cci: failed to add devfreq device:
> > %d\n",
> > +			PTR_ERR(drv->devfreq));
> > +		goto out_remove_opp_table;
> > +	}
> > +
> > +	drv->opp_nb.notifier_call = mtk_ccifreq_opp_notifier;
> > +	ret = dev_pm_opp_register_notifier(cci_dev, &drv->opp_nb);
> > +	if (ret) {
> > +		dev_warn(cci_dev, "cci: failed to register opp
> > notifier: %d\n",
> > +			 ret);
> > +		goto out_remove_devfreq_device;
> > +	}
> > +
> > +	return 0;
> > +
> > +out_remove_devfreq_device:
> > +	devm_devfreq_remove_device(cci_dev, drv->devfreq);
> > +
> > +out_remove_opp_table:
> > +	dev_pm_opp_of_remove_table(cci_dev);
> > +
> > +out_disable_inter_clk:
> > +	clk_disable_unprepare(drv->inter_clk);
> > +
> > +out_disable_cci_clk:
> > +	clk_disable_unprepare(drv->cci_clk);
> > +
> > +out_disable_proc_reg:
> > +	regulator_disable(drv->proc_reg);
> > +
> > +out_disable_sram_reg:
> > +	if (drv->soc_data->need_voltage_tracking)
> > +		regulator_disable(drv->sram_reg);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mtk_ccifreq_remove(struct platform_device *pdev)
> > +{
> > +	struct device *cci_dev = &pdev->dev;
> > +	struct mtk_ccifreq_drv *drv;
> > +
> > +	drv = platform_get_drvdata(pdev);
> > +
> > +	dev_pm_opp_unregister_notifier(cci_dev, &drv->opp_nb);
> > +	dev_pm_opp_of_remove_table(cci_dev);
> > +	regulator_disable(drv->proc_reg);
> > +	if (!IS_ERR(drv->sram_reg))
> > +		regulator_disable(drv->sram_reg);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct mtk_ccifreq_platform_data mtk_platform_data =
> > {
> > +	.min_volt_shift = 0,
> > +	.max_volt_shift = 0,
> > +	.proc_max_volt = 1150000,
> > +	.sram_min_volt = 0,
> > +	.sram_max_volt = 0,
> > +	.need_voltage_tracking = false,
> > +};
> > +
> > +static const struct of_device_id mtk_ccifreq_machines[] = {
> > +	{ .compatible = "mediatek,mt8183-cci", .data =
> > &mtk_platform_data },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_ccifreq_machines);
> > +
> > +static struct platform_driver mtk_ccifreq_platdrv = {
> > +	.probe	= mtk_ccifreq_probe,
> > +	.remove	= mtk_ccifreq_remove,
> > +	.driver = {
> > +		.name = "mtk-ccifreq",
> > +		.of_match_table = of_match_ptr(mtk_ccifreq_machines),
> > +	},
> > +};
> > +
> > +static int __init mtk_ccifreq_platdrv_init(void)
> > +{
> > +	return platform_driver_register(&mtk_ccifreq_platdrv);
> > +}
> > +module_init(mtk_ccifreq_platdrv_init)
> > +
> > +static void __exit mtk_ccifreq_platdrv_exit(void)
> > +{
> > +	platform_driver_unregister(&mtk_ccifreq_platdrv);
> > +}
> > +module_exit(mtk_ccifreq_platdrv_exit)
> > +
> > +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> > +MODULE_AUTHOR("Jia-Wei Chang <jia-wei.chang@mediatek.com>");
> > +MODULE_LICENSE("GPL v2");
> > 
> 
> 

Re: [PATCH 2/3] devfreq: mediatek: add mt8183 cci devfreq driver

[Kevin Hilman]

Hsin-Yi Wang <hsinyi@google.com> writes:

> On Mon, Mar 7, 2022 at 8:32 PM Tim Chang <jia-wei.chang@mediatek.com> wrote:
>>
>> This adds a devfreq driver for the Cache Coherent Interconnect (CCI) of
>> the Mediatek MT8183.
>>
>> On mt8183 the cci is supplied by the same regulator as the little cores.
>> The driver is notified when the regulator voltage changes (driven by
>> cpufreq) and adjusts the cci frequency to the maximum possible value.
>>
>> Add need_voltage_tracking variable to platforma data. if true, it
>> indicates soc is required to realize the voltage tracking between
>> voltage of sram and voltage of cci by software approach. otherwise, the
>> voltage tracking is realized by hardware appraoch.
>>
>> Add the notifier to cci so that it could react after svs driver changes
>> opp table of cci.
>>
>> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.corp-partner.google.com>

[...]

>> +
>> +       passive_data = devm_kzalloc(cci_dev, sizeof(struct devfreq_passive_data), GFP_KERNEL);
>> +       if (!passive_data) {
>> +               ret = -ENOMEM;
>> +               goto out_remove_opp_table;
>> +       }
>> +
>> +       passive_data->parent_type = CPUFREQ_PARENT_DEV;
>
> It's better to add a note below commit message to state that this
> series depends on
> https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing

Not only is this series dependent the previous series from Chanwoo, in
also fails to compile if CONFIG_DEVFREQ_GOV_PASSIVE is not enabled,
because CPUFREQ_PARENT_DEV defined inside of an #ifdef.

Please compile test this with and without CONFIG_DEVFREQ_GOV_PASSIVE
enabled.

Kevin

Re: [PATCH 2/3] devfreq: mediatek: add mt8183 cci devfreq driver

[Johnson Wang]

On Thu, 2022-04-07 at 14:52 -0700, Kevin Hilman wrote:
> Hsin-Yi Wang <hsinyi@google.com> writes:
> 
> > On Mon, Mar 7, 2022 at 8:32 PM Tim Chang <
> > jia-wei.chang@mediatek.com> wrote:
> > > 
> > > This adds a devfreq driver for the Cache Coherent Interconnect
> > > (CCI) of
> > > the Mediatek MT8183.
> > > 
> > > On mt8183 the cci is supplied by the same regulator as the little
> > > cores.
> > > The driver is notified when the regulator voltage changes (driven
> > > by
> > > cpufreq) and adjusts the cci frequency to the maximum possible
> > > value.
> > > 
> > > Add need_voltage_tracking variable to platforma data. if true, it
> > > indicates soc is required to realize the voltage tracking between
> > > voltage of sram and voltage of cci by software approach.
> > > otherwise, the
> > > voltage tracking is realized by hardware appraoch.
> > > 
> > > Add the notifier to cci so that it could react after svs driver
> > > changes
> > > opp table of cci.
> > > 
> > > Signed-off-by: Jia-Wei Chang <
> > > jia-wei.chang@mediatek.corp-partner.google.com>
> 
> [...]
> 
> > > +
> > > +       passive_data = devm_kzalloc(cci_dev, sizeof(struct
> > > devfreq_passive_data), GFP_KERNEL);
> > > +       if (!passive_data) {
> > > +               ret = -ENOMEM;
> > > +               goto out_remove_opp_table;
> > > +       }
> > > +
> > > +       passive_data->parent_type = CPUFREQ_PARENT_DEV;
> > 
> > It's better to add a note below commit message to state that this
> > series depends on
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing__;!!CTRNKA9wMg0ARbw!3dQn_JiiD_cRiN-TmoNWXX7mD2cmF4M5elG4WabiS392Mg9S8wlsagK1o-n9beULMei4AAj9zsoPBElt9R4-UnAd1g$
> >  
> 
> Not only is this series dependent the previous series from Chanwoo,
> in
> also fails to compile if CONFIG_DEVFREQ_GOV_PASSIVE is not enabled,
> because CPUFREQ_PARENT_DEV defined inside of an #ifdef.
> 
> Please compile test this with and without CONFIG_DEVFREQ_GOV_PASSIVE
> enabled.
> 
> Kevin

Hi Kevin,

Thank you for review.
I will fix it in next verison.

BRs,
Johnson Wang