LinuxLists.cc - [PATCH v3 0/2] Introduce MediaTek CCI devfreq driver

[permalink] [raw]

Subject: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

We introduce a devfreq driver for the MediaTek Cache Coherent Interconnect
(CCI) used by some MediaTek SoCs.

In this driver, we use the passive devfreq driver to get target frequencies
and adjust voltages accordingly. In MT8183 and MT8186, the MediaTek CCI
is supplied by the same regulators with the little core CPUs.

Signed-off-by: Johnson Wang <[email protected]>
Signed-off-by: Jia-Wei Chang <[email protected]>
---
This patch depends on "devfreq-testing"[1].
[1]https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
---
drivers/devfreq/Kconfig | 10 +
drivers/devfreq/Makefile | 1 +
drivers/devfreq/mtk-cci-devfreq.c | 474 ++++++++++++++++++++++++++++++
3 files changed, 485 insertions(+)
create mode 100644 drivers/devfreq/mtk-cci-devfreq.c

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 87eb2b837e68..9754d8b31621 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -120,6 +120,16 @@ 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 ARM_MEDIATEK_CPUFREQ || COMPILE_TEST
+ select DEVFREQ_GOV_PASSIVE
+ help
+ This adds a devfreq driver for MediaTek Cache Coherent Interconnect
+ which is shared the same regulators with the cpu cluster. It can track
+ buck voltages and update a proper CCI frequency. Use the notification
+ to get the 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 0b6be92a25d9..bf40d04928d0 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -11,6 +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_MEDIATEK_CCI_DEVFREQ) += mtk-cci-devfreq.o
obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
obj-$(CONFIG_ARM_SUN8I_A33_MBUS_DEVFREQ) += sun8i-a33-mbus.o
obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o
diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-cci-devfreq.c
new file mode 100644
index 000000000000..b3e31c45a57c
--- /dev/null
+++ b/drivers/devfreq/mtk-cci-devfreq.c
@@ -0,0 +1,474 @@
+// 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 {
+ int min_volt_shift;
+ int max_volt_shift;
+ int proc_max_volt;
+ int sram_min_volt;
+ int sram_max_volt;
+};
+
+struct mtk_ccifreq_drv {
+ struct device *dev;
+ struct devfreq *devfreq;
+ struct regulator *proc_reg;
+ struct regulator *sram_reg;
+ struct clk *cci_clk;
+ struct clk *inter_clk;
+ int inter_voltage;
+ int pre_voltage;
+ unsigned long pre_freq;
+ /* Avoid race condition for regulators between notify and policy */
+ struct mutex reg_lock;
+ struct notifier_block opp_nb;
+ const struct mtk_ccifreq_platform_data *soc_data;
+ int vtrack_max;
+};
+
+static int mtk_ccifreq_set_voltage(struct mtk_ccifreq_drv *drv, int new_voltage)
+{
+ const struct mtk_ccifreq_platform_data *soc_data = drv->soc_data;
+ struct device *dev = drv->dev;
+ int pre_voltage, pre_vsram, new_vsram, vsram, voltage, ret;
+ int retry_max = drv->vtrack_max;
+
+ if (!drv->sram_reg) {
+ ret = regulator_set_voltage(drv->proc_reg, new_voltage,
+ drv->soc_data->proc_max_volt);
+ goto out_set_voltage;
+ }
+
+ pre_voltage = regulator_get_voltage(drv->proc_reg);
+ if (pre_voltage < 0) {
+ dev_err(dev, "invalid vproc value: %d\n", pre_voltage);
+ return pre_voltage;
+ }
+
+ pre_vsram = regulator_get_voltage(drv->sram_reg);
+ if (pre_vsram < 0) {
+ dev_err(dev, "invalid vsram value: %d\n", pre_vsram);
+ return pre_vsram;
+ }
+
+ new_vsram = clamp(new_voltage + soc_data->min_volt_shift,
+ soc_data->sram_min_volt, soc_data->sram_max_volt);
+
+ do {
+ if (pre_voltage <= new_voltage) {
+ vsram = clamp(pre_voltage + soc_data->max_volt_shift,
+ soc_data->sram_min_volt, new_vsram);
+ ret = regulator_set_voltage(drv->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(drv->proc_reg, voltage,
+ soc_data->proc_max_volt);
+ if (ret) {
+ regulator_set_voltage(drv->sram_reg, pre_vsram,
+ soc_data->sram_max_volt);
+ return ret;
+ }
+ } else if (pre_voltage > new_voltage) {
+ voltage = max(new_voltage,
+ pre_vsram - soc_data->max_volt_shift);
+ ret = regulator_set_voltage(drv->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(drv->sram_reg, vsram,
+ soc_data->sram_max_volt);
+ if (ret) {
+ regulator_set_voltage(drv->proc_reg, pre_voltage,
+ soc_data->proc_max_volt);
+ return ret;
+ }
+ }
+
+ pre_voltage = voltage;
+ pre_vsram = vsram;
+
+ if (--retry_max < 0) {
+ dev_err(dev,
+ "over loop count, failed to set voltage\n");
+ return -EINVAL;
+ }
+ } while (voltage != new_voltage || vsram != new_vsram);
+
+out_set_voltage:
+ if (!ret)
+ drv->pre_voltage = new_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, pre_voltage, inter_voltage, target_voltage, ret;
+
+ if (!drv)
+ return -EINVAL;
+
+ if (drv->pre_freq == *freq)
+ return 0;
+
+ inter_voltage = drv->inter_voltage;
+
+ opp_rate = *freq;
+ opp = devfreq_recommended_opp(dev, &opp_rate, 1);
+ if (IS_ERR(opp)) {
+ dev_err(dev, "failed to find opp for freq: %ld\n", opp_rate);
+ return PTR_ERR(opp);
+ }
+
+ mutex_lock(&drv->reg_lock);
+
+ voltage = dev_pm_opp_get_voltage(opp);
+ dev_pm_opp_put(opp);
+
+ if (unlikely(drv->pre_voltage <= 0))
+ pre_voltage = regulator_get_voltage(drv->proc_reg);
+ else
+ pre_voltage = drv->pre_voltage;
+
+ if (pre_voltage < 0) {
+ dev_err(dev, "invalid vproc value: %d\n", pre_voltage);
+ return pre_voltage;
+ }
+
+ /* scale up: set voltage first then freq. */
+ target_voltage = max(inter_voltage, voltage);
+ if (pre_voltage <= target_voltage) {
+ ret = mtk_ccifreq_set_voltage(drv, target_voltage);
+ if (ret) {
+ dev_err(dev, "failed to scale up voltage\n");
+ goto out_restore_voltage;
+ }
+ }
+
+ /* switch the cci clock to intermediate clock source. */
+ ret = clk_set_parent(drv->cci_clk, drv->inter_clk);
+ if (ret) {
+ dev_err(dev, "failed to re-parent cci clock\n");
+ goto out_restore_voltage;
+ }
+
+ /* set the original clock to target rate. */
+ ret = clk_set_rate(cci_pll, *freq);
+ if (ret) {
+ dev_err(dev, "failed to set cci pll rate: %d\n", ret);
+ clk_set_parent(drv->cci_clk, cci_pll);
+ goto out_restore_voltage;
+ }
+
+ /* switch the cci clock back to the original clock source. */
+ ret = clk_set_parent(drv->cci_clk, cci_pll);
+ if (ret) {
+ dev_err(dev, "failed to re-parent cci clock\n");
+ mtk_ccifreq_set_voltage(drv, inter_voltage);
+ goto out_unlock;
+ }
+
+ /*
+ * 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 < pre_voltage) {
+ ret = mtk_ccifreq_set_voltage(drv, voltage);
+ if (ret) {
+ dev_err(dev, "failed to scale down voltage\n");
+ goto out_unlock;
+ }
+ }
+
+ drv->pre_freq = *freq;
+ mutex_unlock(&drv->reg_lock);
+
+ return 0;
+
+out_restore_voltage:
+ mtk_ccifreq_set_voltage(drv, pre_voltage);
+
+out_unlock:
+ mutex_unlock(&drv->reg_lock);
+ return ret;
+}
+
+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->reg_lock);
+ /* current opp item is changed */
+ if (freq == drv->pre_freq) {
+ volt = dev_pm_opp_get_voltage(opp);
+ mtk_ccifreq_set_voltage(drv, volt);
+ }
+ mutex_unlock(&drv->reg_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 *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(dev, sizeof(*drv), GFP_KERNEL);
+ if (!drv)
+ return -ENOMEM;
+
+ drv->dev = dev;
+ drv->soc_data = (const struct mtk_ccifreq_platform_data *)
+ of_device_get_match_data(&pdev->dev);
+ mutex_init(&drv->reg_lock);
+ platform_set_drvdata(pdev, drv);
+
+ drv->cci_clk = devm_clk_get(dev, "cci");
+ if (IS_ERR(drv->cci_clk)) {
+ ret = PTR_ERR(drv->cci_clk);
+ return dev_err_probe(dev, ret,
+ "failed to get cci clk: %d\n", ret);
+ }
+
+ drv->inter_clk = devm_clk_get(dev, "intermediate");
+ if (IS_ERR(drv->inter_clk)) {
+ ret = PTR_ERR(drv->inter_clk);
+ dev_err_probe(dev, ret,
+ "failed to get intermediate clk: %d\n", ret);
+ goto out_free_resources;
+ }
+
+ drv->proc_reg = devm_regulator_get_optional(dev, "proc");
+ if (IS_ERR(drv->proc_reg)) {
+ ret = PTR_ERR(drv->proc_reg);
+ dev_err_probe(dev, ret,
+ "failed to get proc regulator: %d\n", ret);
+ goto out_free_resources;
+ }
+
+ ret = regulator_enable(drv->proc_reg);
+ if (ret) {
+ dev_err(dev, "failed to enable proc regulator\n");
+ goto out_free_resources;
+ }
+
+ drv->sram_reg = regulator_get_optional(dev, "sram");
+ if (IS_ERR(drv->sram_reg))
+ drv->sram_reg = NULL;
+ else {
+ ret = regulator_enable(drv->sram_reg);
+ if (ret) {
+ dev_err(dev, "failed to enable sram regulator\n");
+ goto out_free_resources;
+ }
+ }
+
+ /*
+ * We assume min voltage is 0 and tracking target voltage using
+ * min_volt_shift for each iteration.
+ * The retry_max is 3 times of expeted iteration count.
+ */
+ drv->vtrack_max = 3 * DIV_ROUND_UP(max(drv->soc_data->sram_max_volt,
+ drv->soc_data->proc_max_volt),
+ drv->soc_data->min_volt_shift);
+
+ ret = clk_prepare_enable(drv->cci_clk);
+ if (ret)
+ goto out_free_resources;
+
+ ret = clk_prepare_enable(drv->inter_clk);
+ if (ret)
+ goto out_disable_cci_clk;
+
+ ret = dev_pm_opp_of_add_table(dev);
+ if (ret) {
+ dev_err(dev, "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(dev, &rate);
+ if (IS_ERR(opp)) {
+ ret = PTR_ERR(opp);
+ dev_err(dev, "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->dev, &rate);
+ if (IS_ERR(opp)) {
+ dev_err(dev, "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) {
+ dev_err(dev, "failed to scale to highest voltage %lu in proc_reg\n",
+ opp_volt);
+ goto out_remove_opp_table;
+ }
+
+ passive_data = devm_kzalloc(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(dev, &mtk_ccifreq_profile,
+ DEVFREQ_GOV_PASSIVE,
+ passive_data);
+ if (IS_ERR(drv->devfreq)) {
+ ret = -EPROBE_DEFER;
+ dev_err(dev, "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(dev, &drv->opp_nb);
+ if (ret) {
+ dev_err(dev, "failed to register opp notifier: %d\n", ret);
+ goto out_remove_devfreq_device;
+ }
+ return 0;
+
+out_remove_devfreq_device:
+ devm_devfreq_remove_device(dev, drv->devfreq);
+
+out_remove_opp_table:
+ dev_pm_opp_of_remove_table(dev);
+
+out_disable_inter_clk:
+ clk_disable_unprepare(drv->inter_clk);
+
+out_disable_cci_clk:
+ clk_disable_unprepare(drv->cci_clk);
+
+out_free_resources:
+ if (regulator_is_enabled(drv->proc_reg))
+ regulator_disable(drv->proc_reg);
+ if (drv->sram_reg && regulator_is_enabled(drv->sram_reg))
+ regulator_disable(drv->sram_reg);
+
+ if (!IS_ERR(drv->proc_reg))
+ regulator_put(drv->proc_reg);
+ if (!IS_ERR(drv->sram_reg))
+ regulator_put(drv->sram_reg);
+ if (!IS_ERR(drv->cci_clk))
+ clk_put(drv->cci_clk);
+ if (!IS_ERR(drv->inter_clk))
+ clk_put(drv->inter_clk);
+
+ return ret;
+}
+
+static int mtk_ccifreq_remove(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mtk_ccifreq_drv *drv;
+
+ drv = platform_get_drvdata(pdev);
+
+ dev_pm_opp_unregister_notifier(dev, &drv->opp_nb);
+ dev_pm_opp_of_remove_table(dev);
+ clk_disable_unprepare(drv->inter_clk);
+ clk_disable_unprepare(drv->cci_clk);
+ regulator_disable(drv->proc_reg);
+ if (drv->sram_reg)
+ regulator_disable(drv->sram_reg);
+
+ return 0;
+}
+
+static const struct mtk_ccifreq_platform_data mt8183_platform_data = {
+ .min_volt_shift = 100000,
+ .max_volt_shift = 200000,
+ .proc_max_volt = 1150000,
+ .sram_min_volt = 0,
+ .sram_max_volt = 1150000,
+};
+
+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,
+};
+
+static const struct of_device_id mtk_ccifreq_machines[] = {
+ { .compatible = "mediatek,mt8183-cci", .data = &mt8183_platform_data },
+ { .compatible = "mediatek,mt8186-cci", .data = &mt8186_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 = mtk_ccifreq_machines,
+ },
+};
+module_platform_driver(mtk_ccifreq_platdrv);
+
+MODULE_DESCRIPTION("MediaTek CCI devfreq driver");
+MODULE_AUTHOR("Jia-Wei Chang <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.18.0

2022-04-25 23:27:54

[permalink] [raw]

Subject: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI dt-bindings

Add devicetree binding of MediaTek CCI on MT8183 and MT8186.

Signed-off-by: Johnson Wang <[email protected]>
Signed-off-by: Jia-Wei Chang <[email protected]>
---
.../bindings/interconnect/mediatek,cci.yaml | 139 ++++++++++++++++++
1 file changed, 139 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml

diff --git a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
new file mode 100644
index 000000000000..e5221e17d11b
--- /dev/null
+++ b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interconnect/mediatek,cci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek Cache Coherent Interconnect (CCI) frequency and voltage scaling
+
+maintainers:
+ - Jia-Wei Chang <[email protected]>
+
+description: |
+ MediaTek Cache Coherent Interconnect (CCI) is a hardware engine used by
+ MT8183 and MT8186 SoCs to scale the frequency and adjust the voltage in
+ hardware. It can also optimize the voltage to reduce the power consumption.
+
+properties:
+ compatible:
+ enum:
+ - mediatek,mt8183-cci
+ - mediatek,mt8186-cci
+
+ clocks:
+ items:
+ - description:
+ The multiplexer for clock input of CPU cluster.
+ - description:
+ A parent of "cpu" clock which 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: true
+ 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 it presents, 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/mt8183-clk.h>
+ cci: cci {
+ compatible = "mediatek,mt8183-cci";
+ clocks = <&mcucfg CLK_MCU_BUS_SEL>,
+ <&topckgen CLK_TOP_ARMPLL_DIV_PLL1>;
+ clock-names = "cci", "intermediate";
+ operating-points-v2 = <&cci_opp>;
+ proc-supply = <&mt6358_vproc12_reg>;
+ };
+
+ cci_opp: opp-table-cci {
+ compatible = "operating-points-v2";
+ opp-shared;
+ opp2_00: opp-273000000 {
+ opp-hz = /bits/ 64 <273000000>;
+ opp-microvolt = <650000>;
+ };
+ opp2_01: opp-338000000 {
+ opp-hz = /bits/ 64 <338000000>;
+ opp-microvolt = <687500>;
+ };
+ opp2_02: opp-403000000 {
+ opp-hz = /bits/ 64 <403000000>;
+ opp-microvolt = <718750>;
+ };
+ opp2_03: opp-463000000 {
+ opp-hz = /bits/ 64 <463000000>;
+ opp-microvolt = <756250>;
+ };
+ opp2_04: opp-546000000 {
+ opp-hz = /bits/ 64 <546000000>;
+ opp-microvolt = <800000>;
+ };
+ opp2_05: opp-624000000 {
+ opp-hz = /bits/ 64 <624000000>;
+ opp-microvolt = <818750>;
+ };
+ opp2_06: opp-689000000 {
+ opp-hz = /bits/ 64 <689000000>;
+ opp-microvolt = <850000>;
+ };
+ opp2_07: opp-767000000 {
+ opp-hz = /bits/ 64 <767000000>;
+ opp-microvolt = <868750>;
+ };
+ opp2_08: opp-845000000 {
+ opp-hz = /bits/ 64 <845000000>;
+ opp-microvolt = <893750>;
+ };
+ opp2_09: opp-871000000 {
+ opp-hz = /bits/ 64 <871000000>;
+ opp-microvolt = <906250>;
+ };
+ opp2_10: opp-923000000 {
+ opp-hz = /bits/ 64 <923000000>;
+ opp-microvolt = <931250>;
+ };
+ opp2_11: opp-962000000 {
+ opp-hz = /bits/ 64 <962000000>;
+ opp-microvolt = <943750>;
+ };
+ opp2_12: opp-1027000000 {
+ opp-hz = /bits/ 64 <1027000000>;
+ opp-microvolt = <975000>;
+ };
+ opp2_13: opp-1092000000 {
+ opp-hz = /bits/ 64 <1092000000>;
+ opp-microvolt = <1000000>;
+ };
+ opp2_14: opp-1144000000 {
+ opp-hz = /bits/ 64 <1144000000>;
+ opp-microvolt = <1025000>;
+ };
+ opp2_15: opp-1196000000 {
+ opp-hz = /bits/ 64 <1196000000>;
+ opp-microvolt = <1050000>;
+ };
+ };
--
2.18.0

2022-04-26 06:35:44

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI dt-bindings

On Mon, Apr 25, 2022 at 8:56 PM Johnson Wang <[email protected]> wrote:
>
> Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
>
> Signed-off-by: Johnson Wang <[email protected]>
> Signed-off-by: Jia-Wei Chang <[email protected]>
> ---
> .../bindings/interconnect/mediatek,cci.yaml | 139 ++++++++++++++++++
> 1 file changed, 139 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
>
> diff --git a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> new file mode 100644
> index 000000000000..e5221e17d11b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> @@ -0,0 +1,139 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interconnect/mediatek,cci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Cache Coherent Interconnect (CCI) frequency and voltage scaling
> +
> +maintainers:
> + - Jia-Wei Chang <[email protected]>
> +
> +description: |
> + MediaTek Cache Coherent Interconnect (CCI) is a hardware engine used by
> + MT8183 and MT8186 SoCs to scale the frequency and adjust the voltage in
> + hardware. It can also optimize the voltage to reduce the power consumption.
> +
> +properties:
> + compatible:
> + enum:
> + - mediatek,mt8183-cci
> + - mediatek,mt8186-cci
> +
> + clocks:
> + items:
> + - description:
> + The multiplexer for clock input of CPU cluster.

of the bus, not CPU cluster.

> + - description:
> + A parent of "cpu" clock which is used as an intermediate clock source
> + when the original CPU is under transition and not stable yet.

This really should be handled in the clk controller, and not by every device
that happens to take a clock from a mux with upstream PLLs that can change
in clock rate. The end device hardware only takes one clock input. That's it.

> +
> + clock-names:
> + items:
> + - const: cci
> + - const: intermediate
> +
> + operating-points-v2: true
> + 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 it presents, the cci devfreq driver needs to do

When it is present, the implementation needs to ...

ChenYu

> + "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/mt8183-clk.h>
> + cci: cci {
> + compatible = "mediatek,mt8183-cci";
> + clocks = <&mcucfg CLK_MCU_BUS_SEL>,
> + <&topckgen CLK_TOP_ARMPLL_DIV_PLL1>;
> + clock-names = "cci", "intermediate";
> + operating-points-v2 = <&cci_opp>;
> + proc-supply = <&mt6358_vproc12_reg>;
> + };
> +
> + cci_opp: opp-table-cci {
> + compatible = "operating-points-v2";
> + opp-shared;
> + opp2_00: opp-273000000 {
> + opp-hz = /bits/ 64 <273000000>;
> + opp-microvolt = <650000>;
> + };
> + opp2_01: opp-338000000 {
> + opp-hz = /bits/ 64 <338000000>;
> + opp-microvolt = <687500>;
> + };
> + opp2_02: opp-403000000 {
> + opp-hz = /bits/ 64 <403000000>;
> + opp-microvolt = <718750>;
> + };
> + opp2_03: opp-463000000 {
> + opp-hz = /bits/ 64 <463000000>;
> + opp-microvolt = <756250>;
> + };
> + opp2_04: opp-546000000 {
> + opp-hz = /bits/ 64 <546000000>;
> + opp-microvolt = <800000>;
> + };
> + opp2_05: opp-624000000 {
> + opp-hz = /bits/ 64 <624000000>;
> + opp-microvolt = <818750>;
> + };
> + opp2_06: opp-689000000 {
> + opp-hz = /bits/ 64 <689000000>;
> + opp-microvolt = <850000>;
> + };
> + opp2_07: opp-767000000 {
> + opp-hz = /bits/ 64 <767000000>;
> + opp-microvolt = <868750>;
> + };
> + opp2_08: opp-845000000 {
> + opp-hz = /bits/ 64 <845000000>;
> + opp-microvolt = <893750>;
> + };
> + opp2_09: opp-871000000 {
> + opp-hz = /bits/ 64 <871000000>;
> + opp-microvolt = <906250>;
> + };
> + opp2_10: opp-923000000 {
> + opp-hz = /bits/ 64 <923000000>;
> + opp-microvolt = <931250>;
> + };
> + opp2_11: opp-962000000 {
> + opp-hz = /bits/ 64 <962000000>;
> + opp-microvolt = <943750>;
> + };
> + opp2_12: opp-1027000000 {
> + opp-hz = /bits/ 64 <1027000000>;
> + opp-microvolt = <975000>;
> + };
> + opp2_13: opp-1092000000 {
> + opp-hz = /bits/ 64 <1092000000>;
> + opp-microvolt = <1000000>;
> + };
> + opp2_14: opp-1144000000 {
> + opp-hz = /bits/ 64 <1144000000>;
> + opp-microvolt = <1025000>;
> + };
> + opp2_15: opp-1196000000 {
> + opp-hz = /bits/ 64 <1196000000>;
> + opp-microvolt = <1050000>;
> + };
> + };
> --
> 2.18.0
>
>
> _______________________________________________
> Linux-mediatek mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

2022-04-26 19:02:55

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

Hi Johnson,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v5.18-rc4 next-20220422]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Johnson-Wang/Introduce-MediaTek-CCI-devfreq-driver/20220425-205820
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220426/[email protected]/config)
compiler: arceb-elf-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/98b34c0587837b0e5b880b11a52433f8f0eee19f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Johnson-Wang/Introduce-MediaTek-CCI-devfreq-driver/20220425-205820
git checkout 98b34c0587837b0e5b880b11a52433f8f0eee19f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/devfreq/ sound/core/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/devfreq/mtk-cci-devfreq.c: In function 'mtk_ccifreq_probe':
drivers/devfreq/mtk-cci-devfreq.c:372:21: error: 'struct devfreq_passive_data' has no member named 'parent_type'
372 | passive_data->parent_type = CPUFREQ_PARENT_DEV;
| ^~
drivers/devfreq/mtk-cci-devfreq.c:372:37: error: 'CPUFREQ_PARENT_DEV' undeclared (first use in this function)
372 | passive_data->parent_type = CPUFREQ_PARENT_DEV;
| ^~~~~~~~~~~~~~~~~~
drivers/devfreq/mtk-cci-devfreq.c:372:37: note: each undeclared identifier is reported only once for each function it appears in
In file included from include/linux/device.h:15,
from include/linux/devfreq.h:13,
from drivers/devfreq/mtk-cci-devfreq.c:7:
>> drivers/devfreq/mtk-cci-devfreq.c:378:30: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
378 | dev_err(dev, "failed to add devfreq device: %d\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/devfreq/mtk-cci-devfreq.c:378:17: note: in expansion of macro 'dev_err'
378 | dev_err(dev, "failed to add devfreq device: %d\n",
| ^~~~~~~
drivers/devfreq/mtk-cci-devfreq.c:378:62: note: format string is defined here
378 | dev_err(dev, "failed to add devfreq device: %d\n",
| ~^
| |
| int
| %ld

vim +378 drivers/devfreq/mtk-cci-devfreq.c

255
256 static int mtk_ccifreq_probe(struct platform_device *pdev)
257 {
258 struct device *dev = &pdev->dev;
259 struct mtk_ccifreq_drv *drv;
260 struct devfreq_passive_data *passive_data;
261 struct dev_pm_opp *opp;
262 unsigned long rate, opp_volt;
263 int ret;
264
265 drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
266 if (!drv)
267 return -ENOMEM;
268
269 drv->dev = dev;
270 drv->soc_data = (const struct mtk_ccifreq_platform_data *)
271 of_device_get_match_data(&pdev->dev);
272 mutex_init(&drv->reg_lock);
273 platform_set_drvdata(pdev, drv);
274
275 drv->cci_clk = devm_clk_get(dev, "cci");
276 if (IS_ERR(drv->cci_clk)) {
277 ret = PTR_ERR(drv->cci_clk);
278 return dev_err_probe(dev, ret,
279 "failed to get cci clk: %d\n", ret);
280 }
281
282 drv->inter_clk = devm_clk_get(dev, "intermediate");
283 if (IS_ERR(drv->inter_clk)) {
284 ret = PTR_ERR(drv->inter_clk);
285 dev_err_probe(dev, ret,
286 "failed to get intermediate clk: %d\n", ret);
287 goto out_free_resources;
288 }
289
290 drv->proc_reg = devm_regulator_get_optional(dev, "proc");
291 if (IS_ERR(drv->proc_reg)) {
292 ret = PTR_ERR(drv->proc_reg);
293 dev_err_probe(dev, ret,
294 "failed to get proc regulator: %d\n", ret);
295 goto out_free_resources;
296 }
297
298 ret = regulator_enable(drv->proc_reg);
299 if (ret) {
300 dev_err(dev, "failed to enable proc regulator\n");
301 goto out_free_resources;
302 }
303
304 drv->sram_reg = regulator_get_optional(dev, "sram");
305 if (IS_ERR(drv->sram_reg))
306 drv->sram_reg = NULL;
307 else {
308 ret = regulator_enable(drv->sram_reg);
309 if (ret) {
310 dev_err(dev, "failed to enable sram regulator\n");
311 goto out_free_resources;
312 }
313 }
314
315 /*
316 * We assume min voltage is 0 and tracking target voltage using
317 * min_volt_shift for each iteration.
318 * The retry_max is 3 times of expeted iteration count.
319 */
320 drv->vtrack_max = 3 * DIV_ROUND_UP(max(drv->soc_data->sram_max_volt,
321 drv->soc_data->proc_max_volt),
322 drv->soc_data->min_volt_shift);
323
324 ret = clk_prepare_enable(drv->cci_clk);
325 if (ret)
326 goto out_free_resources;
327
328 ret = clk_prepare_enable(drv->inter_clk);
329 if (ret)
330 goto out_disable_cci_clk;
331
332 ret = dev_pm_opp_of_add_table(dev);
333 if (ret) {
334 dev_err(dev, "failed to add opp table: %d\n", ret);
335 goto out_disable_inter_clk;
336 }
337
338 rate = clk_get_rate(drv->inter_clk);
339 opp = dev_pm_opp_find_freq_ceil(dev, &rate);
340 if (IS_ERR(opp)) {
341 ret = PTR_ERR(opp);
342 dev_err(dev, "failed to get intermediate opp: %d\n", ret);
343 goto out_remove_opp_table;
344 }
345 drv->inter_voltage = dev_pm_opp_get_voltage(opp);
346 dev_pm_opp_put(opp);
347
348 rate = U32_MAX;
349 opp = dev_pm_opp_find_freq_floor(drv->dev, &rate);
350 if (IS_ERR(opp)) {
351 dev_err(dev, "failed to get opp\n");
352 ret = PTR_ERR(opp);
353 goto out_remove_opp_table;
354 }
355
356 opp_volt = dev_pm_opp_get_voltage(opp);
357 dev_pm_opp_put(opp);
358 ret = mtk_ccifreq_set_voltage(drv, opp_volt);
359 if (ret) {
360 dev_err(dev, "failed to scale to highest voltage %lu in proc_reg\n",
361 opp_volt);
362 goto out_remove_opp_table;
363 }
364
365 passive_data = devm_kzalloc(dev, sizeof(struct devfreq_passive_data),
366 GFP_KERNEL);
367 if (!passive_data) {
368 ret = -ENOMEM;
369 goto out_remove_opp_table;
370 }
371
372 passive_data->parent_type = CPUFREQ_PARENT_DEV;
373 drv->devfreq = devm_devfreq_add_device(dev, &mtk_ccifreq_profile,
374 DEVFREQ_GOV_PASSIVE,
375 passive_data);
376 if (IS_ERR(drv->devfreq)) {
377 ret = -EPROBE_DEFER;
> 378 dev_err(dev, "failed to add devfreq device: %d\n",
379 PTR_ERR(drv->devfreq));
380 goto out_remove_opp_table;
381 }
382
383 drv->opp_nb.notifier_call = mtk_ccifreq_opp_notifier;
384 ret = dev_pm_opp_register_notifier(dev, &drv->opp_nb);
385 if (ret) {
386 dev_err(dev, "failed to register opp notifier: %d\n", ret);
387 goto out_remove_devfreq_device;
388 }
389 return 0;
390
391 out_remove_devfreq_device:
392 devm_devfreq_remove_device(dev, drv->devfreq);
393
394 out_remove_opp_table:
395 dev_pm_opp_of_remove_table(dev);
396
397 out_disable_inter_clk:
398 clk_disable_unprepare(drv->inter_clk);
399
400 out_disable_cci_clk:
401 clk_disable_unprepare(drv->cci_clk);
402
403 out_free_resources:
404 if (regulator_is_enabled(drv->proc_reg))
405 regulator_disable(drv->proc_reg);
406 if (drv->sram_reg && regulator_is_enabled(drv->sram_reg))
407 regulator_disable(drv->sram_reg);
408
409 if (!IS_ERR(drv->proc_reg))
410 regulator_put(drv->proc_reg);
411 if (!IS_ERR(drv->sram_reg))
412 regulator_put(drv->sram_reg);
413 if (!IS_ERR(drv->cci_clk))
414 clk_put(drv->cci_clk);
415 if (!IS_ERR(drv->inter_clk))
416 clk_put(drv->inter_clk);
417
418 return ret;
419 }
420

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-04-27 10:20:06

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

Hi Johnson,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v5.18-rc4 next-20220426]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Johnson-Wang/Introduce-MediaTek-CCI-devfreq-driver/20220425-205820
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220427/[email protected]/config)
compiler: arceb-elf-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/98b34c0587837b0e5b880b11a52433f8f0eee19f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Johnson-Wang/Introduce-MediaTek-CCI-devfreq-driver/20220425-205820
git checkout 98b34c0587837b0e5b880b11a52433f8f0eee19f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

drivers/devfreq/mtk-cci-devfreq.c: In function 'mtk_ccifreq_probe':
>> drivers/devfreq/mtk-cci-devfreq.c:372:21: error: 'struct devfreq_passive_data' has no member named 'parent_type'
372 | passive_data->parent_type = CPUFREQ_PARENT_DEV;
| ^~
>> drivers/devfreq/mtk-cci-devfreq.c:372:37: error: 'CPUFREQ_PARENT_DEV' undeclared (first use in this function)
372 | passive_data->parent_type = CPUFREQ_PARENT_DEV;
| ^~~~~~~~~~~~~~~~~~
drivers/devfreq/mtk-cci-devfreq.c:372:37: note: each undeclared identifier is reported only once for each function it appears in
In file included from include/linux/device.h:15,
from include/linux/devfreq.h:13,
from drivers/devfreq/mtk-cci-devfreq.c:7:
drivers/devfreq/mtk-cci-devfreq.c:378:30: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
378 | dev_err(dev, "failed to add devfreq device: %d\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/devfreq/mtk-cci-devfreq.c:378:17: note: in expansion of macro 'dev_err'
378 | dev_err(dev, "failed to add devfreq device: %d\n",
| ^~~~~~~
drivers/devfreq/mtk-cci-devfreq.c:378:62: note: format string is defined here
378 | dev_err(dev, "failed to add devfreq device: %d\n",
| ~^
| |
| int
| %ld

vim +372 drivers/devfreq/mtk-cci-devfreq.c

255
256 static int mtk_ccifreq_probe(struct platform_device *pdev)
257 {
258 struct device *dev = &pdev->dev;
259 struct mtk_ccifreq_drv *drv;
260 struct devfreq_passive_data *passive_data;
261 struct dev_pm_opp *opp;
262 unsigned long rate, opp_volt;
263 int ret;
264
265 drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
266 if (!drv)
267 return -ENOMEM;
268
269 drv->dev = dev;
270 drv->soc_data = (const struct mtk_ccifreq_platform_data *)
271 of_device_get_match_data(&pdev->dev);
272 mutex_init(&drv->reg_lock);
273 platform_set_drvdata(pdev, drv);
274
275 drv->cci_clk = devm_clk_get(dev, "cci");
276 if (IS_ERR(drv->cci_clk)) {
277 ret = PTR_ERR(drv->cci_clk);
278 return dev_err_probe(dev, ret,
279 "failed to get cci clk: %d\n", ret);
280 }
281
282 drv->inter_clk = devm_clk_get(dev, "intermediate");
283 if (IS_ERR(drv->inter_clk)) {
284 ret = PTR_ERR(drv->inter_clk);
285 dev_err_probe(dev, ret,
286 "failed to get intermediate clk: %d\n", ret);
287 goto out_free_resources;
288 }
289
290 drv->proc_reg = devm_regulator_get_optional(dev, "proc");
291 if (IS_ERR(drv->proc_reg)) {
292 ret = PTR_ERR(drv->proc_reg);
293 dev_err_probe(dev, ret,
294 "failed to get proc regulator: %d\n", ret);
295 goto out_free_resources;
296 }
297
298 ret = regulator_enable(drv->proc_reg);
299 if (ret) {
300 dev_err(dev, "failed to enable proc regulator\n");
301 goto out_free_resources;
302 }
303
304 drv->sram_reg = regulator_get_optional(dev, "sram");
305 if (IS_ERR(drv->sram_reg))
306 drv->sram_reg = NULL;
307 else {
308 ret = regulator_enable(drv->sram_reg);
309 if (ret) {
310 dev_err(dev, "failed to enable sram regulator\n");
311 goto out_free_resources;
312 }
313 }
314
315 /*
316 * We assume min voltage is 0 and tracking target voltage using
317 * min_volt_shift for each iteration.
318 * The retry_max is 3 times of expeted iteration count.
319 */
320 drv->vtrack_max = 3 * DIV_ROUND_UP(max(drv->soc_data->sram_max_volt,
321 drv->soc_data->proc_max_volt),
322 drv->soc_data->min_volt_shift);
323
324 ret = clk_prepare_enable(drv->cci_clk);
325 if (ret)
326 goto out_free_resources;
327
328 ret = clk_prepare_enable(drv->inter_clk);
329 if (ret)
330 goto out_disable_cci_clk;
331
332 ret = dev_pm_opp_of_add_table(dev);
333 if (ret) {
334 dev_err(dev, "failed to add opp table: %d\n", ret);
335 goto out_disable_inter_clk;
336 }
337
338 rate = clk_get_rate(drv->inter_clk);
339 opp = dev_pm_opp_find_freq_ceil(dev, &rate);
340 if (IS_ERR(opp)) {
341 ret = PTR_ERR(opp);
342 dev_err(dev, "failed to get intermediate opp: %d\n", ret);
343 goto out_remove_opp_table;
344 }
345 drv->inter_voltage = dev_pm_opp_get_voltage(opp);
346 dev_pm_opp_put(opp);
347
348 rate = U32_MAX;
349 opp = dev_pm_opp_find_freq_floor(drv->dev, &rate);
350 if (IS_ERR(opp)) {
351 dev_err(dev, "failed to get opp\n");
352 ret = PTR_ERR(opp);
353 goto out_remove_opp_table;
354 }
355
356 opp_volt = dev_pm_opp_get_voltage(opp);
357 dev_pm_opp_put(opp);
358 ret = mtk_ccifreq_set_voltage(drv, opp_volt);
359 if (ret) {
360 dev_err(dev, "failed to scale to highest voltage %lu in proc_reg\n",
361 opp_volt);
362 goto out_remove_opp_table;
363 }
364
365 passive_data = devm_kzalloc(dev, sizeof(struct devfreq_passive_data),
366 GFP_KERNEL);
367 if (!passive_data) {
368 ret = -ENOMEM;
369 goto out_remove_opp_table;
370 }
371
> 372 passive_data->parent_type = CPUFREQ_PARENT_DEV;
373 drv->devfreq = devm_devfreq_add_device(dev, &mtk_ccifreq_profile,
374 DEVFREQ_GOV_PASSIVE,
375 passive_data);
376 if (IS_ERR(drv->devfreq)) {
377 ret = -EPROBE_DEFER;
378 dev_err(dev, "failed to add devfreq device: %d\n",
379 PTR_ERR(drv->devfreq));
380 goto out_remove_opp_table;
381 }
382
383 drv->opp_nb.notifier_call = mtk_ccifreq_opp_notifier;
384 ret = dev_pm_opp_register_notifier(dev, &drv->opp_nb);
385 if (ret) {
386 dev_err(dev, "failed to register opp notifier: %d\n", ret);
387 goto out_remove_devfreq_device;
388 }
389 return 0;
390
391 out_remove_devfreq_device:
392 devm_devfreq_remove_device(dev, drv->devfreq);
393
394 out_remove_opp_table:
395 dev_pm_opp_of_remove_table(dev);
396
397 out_disable_inter_clk:
398 clk_disable_unprepare(drv->inter_clk);
399
400 out_disable_cci_clk:
401 clk_disable_unprepare(drv->cci_clk);
402
403 out_free_resources:
404 if (regulator_is_enabled(drv->proc_reg))
405 regulator_disable(drv->proc_reg);
406 if (drv->sram_reg && regulator_is_enabled(drv->sram_reg))
407 regulator_disable(drv->sram_reg);
408
409 if (!IS_ERR(drv->proc_reg))
410 regulator_put(drv->proc_reg);
411 if (!IS_ERR(drv->sram_reg))
412 regulator_put(drv->sram_reg);
413 if (!IS_ERR(drv->cci_clk))
414 clk_put(drv->cci_clk);
415 if (!IS_ERR(drv->inter_clk))
416 clk_put(drv->inter_clk);
417
418 return ret;
419 }
420

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-04-27 11:41:35

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

On Wed, 2022-04-27 at 17:25 +0800, kernel test robot wrote:
> Hi Johnson,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on robh/for-next]
> [also build test WARNING on linus/master v5.18-rc4 next-20220427]
> [If your patch is applied to the wrong git tree, kindly drop us a
> note.
> And when submitting patch, we suggest to use '--base' as documented
> in
>
https://urldefense.com/v3/__https://git-scm.com/docs/git-format-patch__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMS_s9zEZ$
> ]
>
> url:
> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux/commits/Johnson-Wang/Introduce-MediaTek-CCI-devfreq-driver/20220425-205820__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMc1U_tqz$
>
> base:
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMUzTprof$
> for-next
> config: hexagon-allyesconfig (
> https://urldefense.com/v3/__https://download.01.org/0day-ci/archive/20220427/[email protected]/config__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMaVRzbSL$
> )
> compiler: clang version 15.0.0 (
> https://urldefense.com/v3/__https://github.com/llvm/llvm-project__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMRqw5IY-$
> $ 1cddcfdc3c683b393df1a5c9063252eb60e52818)
> reproduce (this is a W=1 build):
> wget
> https://urldefense.com/v3/__https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMQLiD-i9$
> -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> #
> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux/commit/98b34c0587837b0e5b880b11a52433f8f0eee19f__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMU5yd7Y2$
>
> git remote add linux-review
> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMW4ldtnH$
>
> git fetch --no-tags linux-review Johnson-Wang/Introduce-
> MediaTek-CCI-devfreq-driver/20220425-205820
> git checkout 98b34c0587837b0e5b880b11a52433f8f0eee19f
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
> W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash block/partitions/
> drivers/devfreq/ drivers/iio/imu/ drivers/misc/lkdtm/
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> drivers/devfreq/mtk-cci-devfreq.c:372:16: error: no member named
> 'parent_type' in 'struct devfreq_passive_data'
> passive_data->parent_type = CPUFREQ_PARENT_DEV;
> ~~~~~~~~~~~~ ^
> drivers/devfreq/mtk-cci-devfreq.c:372:30: error: use of undeclared
> identifier 'CPUFREQ_PARENT_DEV'
> passive_data->parent_type = CPUFREQ_PARENT_DEV;
> ^
> > > drivers/devfreq/mtk-cci-devfreq.c:379:4: warning: format
> > > specifies type 'int' but the argument has type 'long' [-Wformat]
>
> PTR_ERR(drv->devfreq));
> ^~~~~~~~~~~~~~~~~~~~~
> include/linux/dev_printk.h:144:65: note: expanded from macro
> 'dev_err'
> dev_printk_index_wrap(_dev_err, KERN_ERR, dev,
> dev_fmt(fmt), ##__VA_ARGS__)
> ~~~
> ^~~~~~~~~~~
> include/linux/dev_printk.h:110:23: note: expanded from macro
> 'dev_printk_index_wrap'
> _p_func(dev, fmt,
> ##__VA_ARGS__); \
> ~~~ ^~~~~~~~~~~
> 1 warning and 2 errors generated.
>
>
> vim +379 drivers/devfreq/mtk-cci-devfreq.c
>
> 255
> 256 static int mtk_ccifreq_probe(struct platform_device
> *pdev)
> 257 {
> 258 struct device *dev = &pdev->dev;
> 259 struct mtk_ccifreq_drv *drv;
> 260 struct devfreq_passive_data *passive_data;
> 261 struct dev_pm_opp *opp;
> 262 unsigned long rate, opp_volt;
> 263 int ret;
> 264
> 265 drv = devm_kzalloc(dev, sizeof(*drv),
> GFP_KERNEL);
> 266 if (!drv)
> 267 return -ENOMEM;
> 268
> 269 drv->dev = dev;
> 270 drv->soc_data = (const struct
> mtk_ccifreq_platform_data *)
> 271 of_device_get_match_dat
> a(&pdev->dev);
> 272 mutex_init(&drv->reg_lock);
> 273 platform_set_drvdata(pdev, drv);
> 274
> 275 drv->cci_clk = devm_clk_get(dev, "cci");
> 276 if (IS_ERR(drv->cci_clk)) {
> 277 ret = PTR_ERR(drv->cci_clk);
> 278 return dev_err_probe(dev, ret,
> 279 "failed to get cci
> clk: %d\n", ret);
> 280 }
> 281
> 282 drv->inter_clk = devm_clk_get(dev,
> "intermediate");
> 283 if (IS_ERR(drv->inter_clk)) {
> 284 ret = PTR_ERR(drv->inter_clk);
> 285 dev_err_probe(dev, ret,
> 286 "failed to get
> intermediate clk: %d\n", ret);
> 287 goto out_free_resources;
> 288 }
> 289
> 290 drv->proc_reg =
> devm_regulator_get_optional(dev, "proc");
> 291 if (IS_ERR(drv->proc_reg)) {
> 292 ret = PTR_ERR(drv->proc_reg);
> 293 dev_err_probe(dev, ret,
> 294 "failed to get proc
> regulator: %d\n", ret);
> 295 goto out_free_resources;
> 296 }
> 297
> 298 ret = regulator_enable(drv->proc_reg);
> 299 if (ret) {
> 300 dev_err(dev, "failed to enable proc
> regulator\n");
> 301 goto out_free_resources;
> 302 }
> 303
> 304 drv->sram_reg = regulator_get_optional(dev,
> "sram");
> 305 if (IS_ERR(drv->sram_reg))
> 306 drv->sram_reg = NULL;
> 307 else {
> 308 ret = regulator_enable(drv->sram_reg);
> 309 if (ret) {
> 310 dev_err(dev, "failed to enable
> sram regulator\n");
> 311 goto out_free_resources;
> 312 }
> 313 }
> 314
> 315 /*
> 316 * We assume min voltage is 0 and tracking
> target voltage using
> 317 * min_volt_shift for each iteration.
> 318 * The retry_max is 3 times of expeted
> iteration count.
> 319 */
> 320 drv->vtrack_max = 3 * DIV_ROUND_UP(max(drv-
> >soc_data->sram_max_volt,
> 321 drv-
> >soc_data->proc_max_volt),
> 322 drv-
> >soc_data->min_volt_shift);
> 323
> 324 ret = clk_prepare_enable(drv->cci_clk);
> 325 if (ret)
> 326 goto out_free_resources;
> 327
> 328 ret = clk_prepare_enable(drv->inter_clk);
> 329 if (ret)
> 330 goto out_disable_cci_clk;
> 331
> 332 ret = dev_pm_opp_of_add_table(dev);
> 333 if (ret) {
> 334 dev_err(dev, "failed to add opp table:
> %d\n", ret);
> 335 goto out_disable_inter_clk;
> 336 }
> 337
> 338 rate = clk_get_rate(drv->inter_clk);
> 339 opp = dev_pm_opp_find_freq_ceil(dev, &rate);
> 340 if (IS_ERR(opp)) {
> 341 ret = PTR_ERR(opp);
> 342 dev_err(dev, "failed to get
> intermediate opp: %d\n", ret);
> 343 goto out_remove_opp_table;
> 344 }
> 345 drv->inter_voltage =
> dev_pm_opp_get_voltage(opp);
> 346 dev_pm_opp_put(opp);
> 347
> 348 rate = U32_MAX;
> 349 opp = dev_pm_opp_find_freq_floor(drv->dev,
> &rate);
> 350 if (IS_ERR(opp)) {
> 351 dev_err(dev, "failed to get opp\n");
> 352 ret = PTR_ERR(opp);
> 353 goto out_remove_opp_table;
> 354 }
> 355
> 356 opp_volt = dev_pm_opp_get_voltage(opp);
> 357 dev_pm_opp_put(opp);
> 358 ret = mtk_ccifreq_set_voltage(drv, opp_volt);
> 359 if (ret) {
> 360 dev_err(dev, "failed to scale to
> highest voltage %lu in proc_reg\n",
> 361 opp_volt);
> 362 goto out_remove_opp_table;
> 363 }
> 364
> 365 passive_data = devm_kzalloc(dev, sizeof(struct
> devfreq_passive_data),
> 366 GFP_KERNEL);
> 367 if (!passive_data) {
> 368 ret = -ENOMEM;
> 369 goto out_remove_opp_table;
> 370 }
> 371
> 372 passive_data->parent_type = CPUFREQ_PARENT_DEV;
> 373 drv->devfreq = devm_devfreq_add_device(dev,
> &mtk_ccifreq_profile,
> 374 DEVFREQ_
> GOV_PASSIVE,
> 375 passive_
> data);
> 376 if (IS_ERR(drv->devfreq)) {
> 377 ret = -EPROBE_DEFER;
> 378 dev_err(dev, "failed to add devfreq
> device: %d\n",
> > 379 PTR_ERR(drv->devfreq));
> 380 goto out_remove_opp_table;
> 381 }
> 382
> 383 drv->opp_nb.notifier_call =
> mtk_ccifreq_opp_notifier;
> 384 ret = dev_pm_opp_register_notifier(dev, &drv-
> >opp_nb);
> 385 if (ret) {
> 386 dev_err(dev, "failed to register opp
> notifier: %d\n", ret);
> 387 goto out_remove_devfreq_device;
> 388 }
> 389 return 0;
> 390
> 391 out_remove_devfreq_device:
> 392 devm_devfreq_remove_device(dev, drv->devfreq);
> 393
> 394 out_remove_opp_table:
> 395 dev_pm_opp_of_remove_table(dev);
> 396
> 397 out_disable_inter_clk:
> 398 clk_disable_unprepare(drv->inter_clk);
> 399
> 400 out_disable_cci_clk:
> 401 clk_disable_unprepare(drv->cci_clk);
> 402
> 403 out_free_resources:
> 404 if (regulator_is_enabled(drv->proc_reg))
> 405 regulator_disable(drv->proc_reg);
> 406 if (drv->sram_reg && regulator_is_enabled(drv-
> >sram_reg))
> 407 regulator_disable(drv->sram_reg);
> 408
> 409 if (!IS_ERR(drv->proc_reg))
> 410 regulator_put(drv->proc_reg);
> 411 if (!IS_ERR(drv->sram_reg))
> 412 regulator_put(drv->sram_reg);
> 413 if (!IS_ERR(drv->cci_clk))
> 414 clk_put(drv->cci_clk);
> 415 if (!IS_ERR(drv->inter_clk))
> 416 clk_put(drv->inter_clk);
> 417
> 418 return ret;
> 419 }
> 420
>

Hi "kernel test robot",

Thanks for your review.

This patch is based on chanwoo/devfreq-testing[1]
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing

I will follow your suggestion to use '--base' in the next version.

BRs,
Johnson Wang

2022-04-27 11:43:50

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

Hi Johnson,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v5.18-rc4 next-20220427]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Johnson-Wang/Introduce-MediaTek-CCI-devfreq-driver/20220425-205820
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20220427/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 1cddcfdc3c683b393df1a5c9063252eb60e52818)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/98b34c0587837b0e5b880b11a52433f8f0eee19f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Johnson-Wang/Introduce-MediaTek-CCI-devfreq-driver/20220425-205820
git checkout 98b34c0587837b0e5b880b11a52433f8f0eee19f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash block/partitions/ drivers/devfreq/ drivers/iio/imu/ drivers/misc/lkdtm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/devfreq/mtk-cci-devfreq.c:372:16: error: no member named 'parent_type' in 'struct devfreq_passive_data'
passive_data->parent_type = CPUFREQ_PARENT_DEV;
~~~~~~~~~~~~ ^
drivers/devfreq/mtk-cci-devfreq.c:372:30: error: use of undeclared identifier 'CPUFREQ_PARENT_DEV'
passive_data->parent_type = CPUFREQ_PARENT_DEV;
^
>> drivers/devfreq/mtk-cci-devfreq.c:379:4: warning: format specifies type 'int' but the argument has type 'long' [-Wformat]
PTR_ERR(drv->devfreq));
^~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:144:65: note: expanded from macro 'dev_err'
dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
_p_func(dev, fmt, ##__VA_ARGS__); \
~~~ ^~~~~~~~~~~
1 warning and 2 errors generated.

vim +379 drivers/devfreq/mtk-cci-devfreq.c

255
256 static int mtk_ccifreq_probe(struct platform_device *pdev)
257 {
258 struct device *dev = &pdev->dev;
259 struct mtk_ccifreq_drv *drv;
260 struct devfreq_passive_data *passive_data;
261 struct dev_pm_opp *opp;
262 unsigned long rate, opp_volt;
263 int ret;
264
265 drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
266 if (!drv)
267 return -ENOMEM;
268
269 drv->dev = dev;
270 drv->soc_data = (const struct mtk_ccifreq_platform_data *)
271 of_device_get_match_data(&pdev->dev);
272 mutex_init(&drv->reg_lock);
273 platform_set_drvdata(pdev, drv);
274
275 drv->cci_clk = devm_clk_get(dev, "cci");
276 if (IS_ERR(drv->cci_clk)) {
277 ret = PTR_ERR(drv->cci_clk);
278 return dev_err_probe(dev, ret,
279 "failed to get cci clk: %d\n", ret);
280 }
281
282 drv->inter_clk = devm_clk_get(dev, "intermediate");
283 if (IS_ERR(drv->inter_clk)) {
284 ret = PTR_ERR(drv->inter_clk);
285 dev_err_probe(dev, ret,
286 "failed to get intermediate clk: %d\n", ret);
287 goto out_free_resources;
288 }
289
290 drv->proc_reg = devm_regulator_get_optional(dev, "proc");
291 if (IS_ERR(drv->proc_reg)) {
292 ret = PTR_ERR(drv->proc_reg);
293 dev_err_probe(dev, ret,
294 "failed to get proc regulator: %d\n", ret);
295 goto out_free_resources;
296 }
297
298 ret = regulator_enable(drv->proc_reg);
299 if (ret) {
300 dev_err(dev, "failed to enable proc regulator\n");
301 goto out_free_resources;
302 }
303
304 drv->sram_reg = regulator_get_optional(dev, "sram");
305 if (IS_ERR(drv->sram_reg))
306 drv->sram_reg = NULL;
307 else {
308 ret = regulator_enable(drv->sram_reg);
309 if (ret) {
310 dev_err(dev, "failed to enable sram regulator\n");
311 goto out_free_resources;
312 }
313 }
314
315 /*
316 * We assume min voltage is 0 and tracking target voltage using
317 * min_volt_shift for each iteration.
318 * The retry_max is 3 times of expeted iteration count.
319 */
320 drv->vtrack_max = 3 * DIV_ROUND_UP(max(drv->soc_data->sram_max_volt,
321 drv->soc_data->proc_max_volt),
322 drv->soc_data->min_volt_shift);
323
324 ret = clk_prepare_enable(drv->cci_clk);
325 if (ret)
326 goto out_free_resources;
327
328 ret = clk_prepare_enable(drv->inter_clk);
329 if (ret)
330 goto out_disable_cci_clk;
331
332 ret = dev_pm_opp_of_add_table(dev);
333 if (ret) {
334 dev_err(dev, "failed to add opp table: %d\n", ret);
335 goto out_disable_inter_clk;
336 }
337
338 rate = clk_get_rate(drv->inter_clk);
339 opp = dev_pm_opp_find_freq_ceil(dev, &rate);
340 if (IS_ERR(opp)) {
341 ret = PTR_ERR(opp);
342 dev_err(dev, "failed to get intermediate opp: %d\n", ret);
343 goto out_remove_opp_table;
344 }
345 drv->inter_voltage = dev_pm_opp_get_voltage(opp);
346 dev_pm_opp_put(opp);
347
348 rate = U32_MAX;
349 opp = dev_pm_opp_find_freq_floor(drv->dev, &rate);
350 if (IS_ERR(opp)) {
351 dev_err(dev, "failed to get opp\n");
352 ret = PTR_ERR(opp);
353 goto out_remove_opp_table;
354 }
355
356 opp_volt = dev_pm_opp_get_voltage(opp);
357 dev_pm_opp_put(opp);
358 ret = mtk_ccifreq_set_voltage(drv, opp_volt);
359 if (ret) {
360 dev_err(dev, "failed to scale to highest voltage %lu in proc_reg\n",
361 opp_volt);
362 goto out_remove_opp_table;
363 }
364
365 passive_data = devm_kzalloc(dev, sizeof(struct devfreq_passive_data),
366 GFP_KERNEL);
367 if (!passive_data) {
368 ret = -ENOMEM;
369 goto out_remove_opp_table;
370 }
371
372 passive_data->parent_type = CPUFREQ_PARENT_DEV;
373 drv->devfreq = devm_devfreq_add_device(dev, &mtk_ccifreq_profile,
374 DEVFREQ_GOV_PASSIVE,
375 passive_data);
376 if (IS_ERR(drv->devfreq)) {
377 ret = -EPROBE_DEFER;
378 dev_err(dev, "failed to add devfreq device: %d\n",
> 379 PTR_ERR(drv->devfreq));
380 goto out_remove_opp_table;
381 }
382
383 drv->opp_nb.notifier_call = mtk_ccifreq_opp_notifier;
384 ret = dev_pm_opp_register_notifier(dev, &drv->opp_nb);
385 if (ret) {
386 dev_err(dev, "failed to register opp notifier: %d\n", ret);
387 goto out_remove_devfreq_device;
388 }
389 return 0;
390
391 out_remove_devfreq_device:
392 devm_devfreq_remove_device(dev, drv->devfreq);
393
394 out_remove_opp_table:
395 dev_pm_opp_of_remove_table(dev);
396
397 out_disable_inter_clk:
398 clk_disable_unprepare(drv->inter_clk);
399
400 out_disable_cci_clk:
401 clk_disable_unprepare(drv->cci_clk);
402
403 out_free_resources:
404 if (regulator_is_enabled(drv->proc_reg))
405 regulator_disable(drv->proc_reg);
406 if (drv->sram_reg && regulator_is_enabled(drv->sram_reg))
407 regulator_disable(drv->sram_reg);
408
409 if (!IS_ERR(drv->proc_reg))
410 regulator_put(drv->proc_reg);
411 if (!IS_ERR(drv->sram_reg))
412 regulator_put(drv->sram_reg);
413 if (!IS_ERR(drv->cci_clk))
414 clk_put(drv->cci_clk);
415 if (!IS_ERR(drv->inter_clk))
416 clk_put(drv->inter_clk);
417
418 return ret;
419 }
420

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-04-28 12:53:34

by Chen, Rong A

[permalink] [raw]

Subject: Re: [kbuild-all] Re: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

On 4/27/2022 6:11 PM, Johnson Wang wrote:
> On Wed, 2022-04-27 at 17:25 +0800, kernel test robot wrote:
>> Hi Johnson,
>>
>> Thank you for the patch! Perhaps something to improve:
>>
>> [auto build test WARNING on robh/for-next]
>> [also build test WARNING on linus/master v5.18-rc4 next-20220427]
>> [If your patch is applied to the wrong git tree, kindly drop us a
>> note.
>> And when submitting patch, we suggest to use '--base' as documented
>> in
>>
> https://urldefense.com/v3/__https://git-scm.com/docs/git-format-patch__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMS_s9zEZ$
>> ]
>>
>> url:
>> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux/commits/Johnson-Wang/Introduce-MediaTek-CCI-devfreq-driver/20220425-205820__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMc1U_tqz$
>>
>> base:
>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMUzTprof$
>> for-next
>> config: hexagon-allyesconfig (
>> https://urldefense.com/v3/__https://download.01.org/0day-ci/archive/20220427/[email protected]/config__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMaVRzbSL$
>> )
>> compiler: clang version 15.0.0 (
>> https://urldefense.com/v3/__https://github.com/llvm/llvm-project__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMRqw5IY-$
>> $ 1cddcfdc3c683b393df1a5c9063252eb60e52818)
>> reproduce (this is a W=1 build):
>> wget
>> https://urldefense.com/v3/__https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMQLiD-i9$
>> -O ~/bin/make.cross
>> chmod +x ~/bin/make.cross
>> #
>> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux/commit/98b34c0587837b0e5b880b11a52433f8f0eee19f__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMU5yd7Y2$
>>
>> git remote add linux-review
>> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMW4ldtnH$
>>
>> git fetch --no-tags linux-review Johnson-Wang/Introduce-
>> MediaTek-CCI-devfreq-driver/20220425-205820
>> git checkout 98b34c0587837b0e5b880b11a52433f8f0eee19f
>> # save the config file
>> mkdir build_dir && cp config build_dir/.config
>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
>> W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash block/partitions/
>> drivers/devfreq/ drivers/iio/imu/ drivers/misc/lkdtm/
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <[email protected]>
>>
>> All warnings (new ones prefixed by >>):
>>
>> drivers/devfreq/mtk-cci-devfreq.c:372:16: error: no member named
>> 'parent_type' in 'struct devfreq_passive_data'
>> passive_data->parent_type = CPUFREQ_PARENT_DEV;
>> ~~~~~~~~~~~~ ^
>> drivers/devfreq/mtk-cci-devfreq.c:372:30: error: use of undeclared
>> identifier 'CPUFREQ_PARENT_DEV'
>> passive_data->parent_type = CPUFREQ_PARENT_DEV;
>> ^
>>>> drivers/devfreq/mtk-cci-devfreq.c:379:4: warning: format
>>>> specifies type 'int' but the argument has type 'long' [-Wformat]
>>
>> PTR_ERR(drv->devfreq));
>> ^~~~~~~~~~~~~~~~~~~~~
>> include/linux/dev_printk.h:144:65: note: expanded from macro
>> 'dev_err'
>> dev_printk_index_wrap(_dev_err, KERN_ERR, dev,
>> dev_fmt(fmt), ##__VA_ARGS__)
>> ~~~
>> ^~~~~~~~~~~
>> include/linux/dev_printk.h:110:23: note: expanded from macro
>> 'dev_printk_index_wrap'
>> _p_func(dev, fmt,
>> ##__VA_ARGS__); \
>> ~~~ ^~~~~~~~~~~
>> 1 warning and 2 errors generated.
>>
>>
>> vim +379 drivers/devfreq/mtk-cci-devfreq.c
>>
>> 255
>> 256 static int mtk_ccifreq_probe(struct platform_device
>> *pdev)
>> 257 {
>> 258 struct device *dev = &pdev->dev;
>> 259 struct mtk_ccifreq_drv *drv;
>> 260 struct devfreq_passive_data *passive_data;
>> 261 struct dev_pm_opp *opp;
>> 262 unsigned long rate, opp_volt;
>> 263 int ret;
>> 264
>> 265 drv = devm_kzalloc(dev, sizeof(*drv),
>> GFP_KERNEL);
>> 266 if (!drv)
>> 267 return -ENOMEM;
>> 268
>> 269 drv->dev = dev;
>> 270 drv->soc_data = (const struct
>> mtk_ccifreq_platform_data *)
>> 271 of_device_get_match_dat
>> a(&pdev->dev);
>> 272 mutex_init(&drv->reg_lock);
>> 273 platform_set_drvdata(pdev, drv);
>> 274
>> 275 drv->cci_clk = devm_clk_get(dev, "cci");
>> 276 if (IS_ERR(drv->cci_clk)) {
>> 277 ret = PTR_ERR(drv->cci_clk);
>> 278 return dev_err_probe(dev, ret,
>> 279 "failed to get cci
>> clk: %d\n", ret);
>> 280 }
>> 281
>> 282 drv->inter_clk = devm_clk_get(dev,
>> "intermediate");
>> 283 if (IS_ERR(drv->inter_clk)) {
>> 284 ret = PTR_ERR(drv->inter_clk);
>> 285 dev_err_probe(dev, ret,
>> 286 "failed to get
>> intermediate clk: %d\n", ret);
>> 287 goto out_free_resources;
>> 288 }
>> 289
>> 290 drv->proc_reg =
>> devm_regulator_get_optional(dev, "proc");
>> 291 if (IS_ERR(drv->proc_reg)) {
>> 292 ret = PTR_ERR(drv->proc_reg);
>> 293 dev_err_probe(dev, ret,
>> 294 "failed to get proc
>> regulator: %d\n", ret);
>> 295 goto out_free_resources;
>> 296 }
>> 297
>> 298 ret = regulator_enable(drv->proc_reg);
>> 299 if (ret) {
>> 300 dev_err(dev, "failed to enable proc
>> regulator\n");
>> 301 goto out_free_resources;
>> 302 }
>> 303
>> 304 drv->sram_reg = regulator_get_optional(dev,
>> "sram");
>> 305 if (IS_ERR(drv->sram_reg))
>> 306 drv->sram_reg = NULL;
>> 307 else {
>> 308 ret = regulator_enable(drv->sram_reg);
>> 309 if (ret) {
>> 310 dev_err(dev, "failed to enable
>> sram regulator\n");
>> 311 goto out_free_resources;
>> 312 }
>> 313 }
>> 314
>> 315 /*
>> 316 * We assume min voltage is 0 and tracking
>> target voltage using
>> 317 * min_volt_shift for each iteration.
>> 318 * The retry_max is 3 times of expeted
>> iteration count.
>> 319 */
>> 320 drv->vtrack_max = 3 * DIV_ROUND_UP(max(drv-
>>> soc_data->sram_max_volt,
>> 321 drv-
>>> soc_data->proc_max_volt),
>> 322 drv-
>>> soc_data->min_volt_shift);
>> 323
>> 324 ret = clk_prepare_enable(drv->cci_clk);
>> 325 if (ret)
>> 326 goto out_free_resources;
>> 327
>> 328 ret = clk_prepare_enable(drv->inter_clk);
>> 329 if (ret)
>> 330 goto out_disable_cci_clk;
>> 331
>> 332 ret = dev_pm_opp_of_add_table(dev);
>> 333 if (ret) {
>> 334 dev_err(dev, "failed to add opp table:
>> %d\n", ret);
>> 335 goto out_disable_inter_clk;
>> 336 }
>> 337
>> 338 rate = clk_get_rate(drv->inter_clk);
>> 339 opp = dev_pm_opp_find_freq_ceil(dev, &rate);
>> 340 if (IS_ERR(opp)) {
>> 341 ret = PTR_ERR(opp);
>> 342 dev_err(dev, "failed to get
>> intermediate opp: %d\n", ret);
>> 343 goto out_remove_opp_table;
>> 344 }
>> 345 drv->inter_voltage =
>> dev_pm_opp_get_voltage(opp);
>> 346 dev_pm_opp_put(opp);
>> 347
>> 348 rate = U32_MAX;
>> 349 opp = dev_pm_opp_find_freq_floor(drv->dev,
>> &rate);
>> 350 if (IS_ERR(opp)) {
>> 351 dev_err(dev, "failed to get opp\n");
>> 352 ret = PTR_ERR(opp);
>> 353 goto out_remove_opp_table;
>> 354 }
>> 355
>> 356 opp_volt = dev_pm_opp_get_voltage(opp);
>> 357 dev_pm_opp_put(opp);
>> 358 ret = mtk_ccifreq_set_voltage(drv, opp_volt);
>> 359 if (ret) {
>> 360 dev_err(dev, "failed to scale to
>> highest voltage %lu in proc_reg\n",
>> 361 opp_volt);
>> 362 goto out_remove_opp_table;
>> 363 }
>> 364
>> 365 passive_data = devm_kzalloc(dev, sizeof(struct
>> devfreq_passive_data),
>> 366 GFP_KERNEL);
>> 367 if (!passive_data) {
>> 368 ret = -ENOMEM;
>> 369 goto out_remove_opp_table;
>> 370 }
>> 371
>> 372 passive_data->parent_type = CPUFREQ_PARENT_DEV;
>> 373 drv->devfreq = devm_devfreq_add_device(dev,
>> &mtk_ccifreq_profile,
>> 374 DEVFREQ_
>> GOV_PASSIVE,
>> 375 passive_
>> data);
>> 376 if (IS_ERR(drv->devfreq)) {
>> 377 ret = -EPROBE_DEFER;
>> 378 dev_err(dev, "failed to add devfreq
>> device: %d\n",
>> > 379 PTR_ERR(drv->devfreq));
>> 380 goto out_remove_opp_table;
>> 381 }
>> 382
>> 383 drv->opp_nb.notifier_call =
>> mtk_ccifreq_opp_notifier;
>> 384 ret = dev_pm_opp_register_notifier(dev, &drv-
>>> opp_nb);
>> 385 if (ret) {
>> 386 dev_err(dev, "failed to register opp
>> notifier: %d\n", ret);
>> 387 goto out_remove_devfreq_device;
>> 388 }
>> 389 return 0;
>> 390
>> 391 out_remove_devfreq_device:
>> 392 devm_devfreq_remove_device(dev, drv->devfreq);
>> 393
>> 394 out_remove_opp_table:
>> 395 dev_pm_opp_of_remove_table(dev);
>> 396
>> 397 out_disable_inter_clk:
>> 398 clk_disable_unprepare(drv->inter_clk);
>> 399
>> 400 out_disable_cci_clk:
>> 401 clk_disable_unprepare(drv->cci_clk);
>> 402
>> 403 out_free_resources:
>> 404 if (regulator_is_enabled(drv->proc_reg))
>> 405 regulator_disable(drv->proc_reg);
>> 406 if (drv->sram_reg && regulator_is_enabled(drv-
>>> sram_reg))
>> 407 regulator_disable(drv->sram_reg);
>> 408
>> 409 if (!IS_ERR(drv->proc_reg))
>> 410 regulator_put(drv->proc_reg);
>> 411 if (!IS_ERR(drv->sram_reg))
>> 412 regulator_put(drv->sram_reg);
>> 413 if (!IS_ERR(drv->cci_clk))
>> 414 clk_put(drv->cci_clk);
>> 415 if (!IS_ERR(drv->inter_clk))
>> 416 clk_put(drv->inter_clk);
>> 417
>> 418 return ret;
>> 419 }
>> 420
>>
>
> Hi "kernel test robot",
>
> Thanks for your review.
>
> This patch is based on chanwoo/devfreq-testing[1]
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing

Hi Johnson,

Thanks for the feedback, we'll take a look too.

Best Regards,
Rong Chen

>
> I will follow your suggestion to use '--base' in the next version.
>
> BRs,
> Johnson Wang
> _______________________________________________
> kbuild-all mailing list -- [email protected]
> To unsubscribe send an email to [email protected]
>

2022-05-06 14:20:36

[permalink] [raw]

Subject: Re: [kbuild-all] Re: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

On Thu, Apr 28, 2022 at 7:39 PM Chen, Rong A <[email protected]> wrote:
> On 4/27/2022 6:11 PM, Johnson Wang wrote:
> > On Wed, 2022-04-27 at 17:25 +0800, kernel test robot wrote:
> >> Hi Johnson,
> >>
> >> Thank you for the patch! Perhaps something to improve:
> >>
> >> [auto build test WARNING on robh/for-next]
> >> [also build test WARNING on linus/master v5.18-rc4 next-20220427]
> >> [If your patch is applied to the wrong git tree, kindly drop us a
> >> note.
> >> And when submitting patch, we suggest to use '--base' as documented
> >> in
> >>
> > https://urldefense.com/v3/__https://git-scm.com/docs/git-format-patch__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMS_s9zEZ$
> >> ]
> >>
> >> url:
> >> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux/commits/Johnson-Wang/Introduce-MediaTek-CCI-devfreq-driver/20220425-205820__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMc1U_tqz$
> >>
> >> base:
> >> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMUzTprof$
> >> for-next
> >> config: hexagon-allyesconfig (
> >> https://urldefense.com/v3/__https://download.01.org/0day-ci/archive/20220427/[email protected]/config__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMaVRzbSL$
> >> )
> >> compiler: clang version 15.0.0 (
> >> https://urldefense.com/v3/__https://github.com/llvm/llvm-project__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMRqw5IY-$
> >> $ 1cddcfdc3c683b393df1a5c9063252eb60e52818)
> >> reproduce (this is a W=1 build):
> >> wget
> >> https://urldefense.com/v3/__https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMQLiD-i9$
> >> -O ~/bin/make.cross
> >> chmod +x ~/bin/make.cross
> >> #
> >> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux/commit/98b34c0587837b0e5b880b11a52433f8f0eee19f__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMU5yd7Y2$
> >>
> >> git remote add linux-review
> >> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMW4ldtnH$
> >>
> >> git fetch --no-tags linux-review Johnson-Wang/Introduce-
> >> MediaTek-CCI-devfreq-driver/20220425-205820
> >> git checkout 98b34c0587837b0e5b880b11a52433f8f0eee19f
> >> # save the config file
> >> mkdir build_dir && cp config build_dir/.config
> >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
> >> W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash block/partitions/
> >> drivers/devfreq/ drivers/iio/imu/ drivers/misc/lkdtm/
> >>
> >> If you fix the issue, kindly add following tag as appropriate
> >> Reported-by: kernel test robot <[email protected]>
> >>
> >> All warnings (new ones prefixed by >>):
> >>
> >> drivers/devfreq/mtk-cci-devfreq.c:372:16: error: no member named
> >> 'parent_type' in 'struct devfreq_passive_data'
> >> passive_data->parent_type = CPUFREQ_PARENT_DEV;
> >> ~~~~~~~~~~~~ ^
> >> drivers/devfreq/mtk-cci-devfreq.c:372:30: error: use of undeclared
> >> identifier 'CPUFREQ_PARENT_DEV'
> >> passive_data->parent_type = CPUFREQ_PARENT_DEV;
> >> ^
> >>>> drivers/devfreq/mtk-cci-devfreq.c:379:4: warning: format
> >>>> specifies type 'int' but the argument has type 'long' [-Wformat]
> >>
> >> PTR_ERR(drv->devfreq));
> >> ^~~~~~~~~~~~~~~~~~~~~
> >> include/linux/dev_printk.h:144:65: note: expanded from macro
> >> 'dev_err'
> >> dev_printk_index_wrap(_dev_err, KERN_ERR, dev,
> >> dev_fmt(fmt), ##__VA_ARGS__)
> >> ~~~
> >> ^~~~~~~~~~~
> >> include/linux/dev_printk.h:110:23: note: expanded from macro
> >> 'dev_printk_index_wrap'
> >> _p_func(dev, fmt,
> >> ##__VA_ARGS__); \
> >> ~~~ ^~~~~~~~~~~
> >> 1 warning and 2 errors generated.
> >>
> >>
> >> vim +379 drivers/devfreq/mtk-cci-devfreq.c
> >>
> >> 255
> >> 256 static int mtk_ccifreq_probe(struct platform_device
> >> *pdev)
> >> 257 {
> >> 258 struct device *dev = &pdev->dev;
> >> 259 struct mtk_ccifreq_drv *drv;
> >> 260 struct devfreq_passive_data *passive_data;
> >> 261 struct dev_pm_opp *opp;
> >> 262 unsigned long rate, opp_volt;
> >> 263 int ret;
> >> 264
> >> 265 drv = devm_kzalloc(dev, sizeof(*drv),
> >> GFP_KERNEL);
> >> 266 if (!drv)
> >> 267 return -ENOMEM;
> >> 268
> >> 269 drv->dev = dev;
> >> 270 drv->soc_data = (const struct
> >> mtk_ccifreq_platform_data *)
> >> 271 of_device_get_match_dat
> >> a(&pdev->dev);
> >> 272 mutex_init(&drv->reg_lock);
> >> 273 platform_set_drvdata(pdev, drv);
> >> 274
> >> 275 drv->cci_clk = devm_clk_get(dev, "cci");
> >> 276 if (IS_ERR(drv->cci_clk)) {
> >> 277 ret = PTR_ERR(drv->cci_clk);
> >> 278 return dev_err_probe(dev, ret,
> >> 279 "failed to get cci
> >> clk: %d\n", ret);
> >> 280 }
> >> 281
> >> 282 drv->inter_clk = devm_clk_get(dev,
> >> "intermediate");
> >> 283 if (IS_ERR(drv->inter_clk)) {
> >> 284 ret = PTR_ERR(drv->inter_clk);
> >> 285 dev_err_probe(dev, ret,
> >> 286 "failed to get
> >> intermediate clk: %d\n", ret);
> >> 287 goto out_free_resources;
> >> 288 }
> >> 289
> >> 290 drv->proc_reg =
> >> devm_regulator_get_optional(dev, "proc");
> >> 291 if (IS_ERR(drv->proc_reg)) {
> >> 292 ret = PTR_ERR(drv->proc_reg);
> >> 293 dev_err_probe(dev, ret,
> >> 294 "failed to get proc
> >> regulator: %d\n", ret);
> >> 295 goto out_free_resources;
> >> 296 }
> >> 297
> >> 298 ret = regulator_enable(drv->proc_reg);
> >> 299 if (ret) {
> >> 300 dev_err(dev, "failed to enable proc
> >> regulator\n");
> >> 301 goto out_free_resources;
> >> 302 }
> >> 303
> >> 304 drv->sram_reg = regulator_get_optional(dev,
> >> "sram");
> >> 305 if (IS_ERR(drv->sram_reg))
> >> 306 drv->sram_reg = NULL;
> >> 307 else {
> >> 308 ret = regulator_enable(drv->sram_reg);
> >> 309 if (ret) {
> >> 310 dev_err(dev, "failed to enable
> >> sram regulator\n");
> >> 311 goto out_free_resources;
> >> 312 }
> >> 313 }
> >> 314
> >> 315 /*
> >> 316 * We assume min voltage is 0 and tracking
> >> target voltage using
> >> 317 * min_volt_shift for each iteration.
> >> 318 * The retry_max is 3 times of expeted
> >> iteration count.
> >> 319 */
> >> 320 drv->vtrack_max = 3 * DIV_ROUND_UP(max(drv-
> >>> soc_data->sram_max_volt,
> >> 321 drv-
> >>> soc_data->proc_max_volt),
> >> 322 drv-
> >>> soc_data->min_volt_shift);
> >> 323
> >> 324 ret = clk_prepare_enable(drv->cci_clk);
> >> 325 if (ret)
> >> 326 goto out_free_resources;
> >> 327
> >> 328 ret = clk_prepare_enable(drv->inter_clk);
> >> 329 if (ret)
> >> 330 goto out_disable_cci_clk;
> >> 331
> >> 332 ret = dev_pm_opp_of_add_table(dev);
> >> 333 if (ret) {
> >> 334 dev_err(dev, "failed to add opp table:
> >> %d\n", ret);
> >> 335 goto out_disable_inter_clk;
> >> 336 }
> >> 337
> >> 338 rate = clk_get_rate(drv->inter_clk);
> >> 339 opp = dev_pm_opp_find_freq_ceil(dev, &rate);
> >> 340 if (IS_ERR(opp)) {
> >> 341 ret = PTR_ERR(opp);
> >> 342 dev_err(dev, "failed to get
> >> intermediate opp: %d\n", ret);
> >> 343 goto out_remove_opp_table;
> >> 344 }
> >> 345 drv->inter_voltage =
> >> dev_pm_opp_get_voltage(opp);
> >> 346 dev_pm_opp_put(opp);
> >> 347
> >> 348 rate = U32_MAX;
> >> 349 opp = dev_pm_opp_find_freq_floor(drv->dev,
> >> &rate);
> >> 350 if (IS_ERR(opp)) {
> >> 351 dev_err(dev, "failed to get opp\n");
> >> 352 ret = PTR_ERR(opp);
> >> 353 goto out_remove_opp_table;
> >> 354 }
> >> 355
> >> 356 opp_volt = dev_pm_opp_get_voltage(opp);
> >> 357 dev_pm_opp_put(opp);
> >> 358 ret = mtk_ccifreq_set_voltage(drv, opp_volt);
> >> 359 if (ret) {
> >> 360 dev_err(dev, "failed to scale to
> >> highest voltage %lu in proc_reg\n",
> >> 361 opp_volt);
> >> 362 goto out_remove_opp_table;
> >> 363 }
> >> 364
> >> 365 passive_data = devm_kzalloc(dev, sizeof(struct
> >> devfreq_passive_data),
> >> 366 GFP_KERNEL);
> >> 367 if (!passive_data) {
> >> 368 ret = -ENOMEM;
> >> 369 goto out_remove_opp_table;
> >> 370 }
> >> 371
> >> 372 passive_data->parent_type = CPUFREQ_PARENT_DEV;
> >> 373 drv->devfreq = devm_devfreq_add_device(dev,
> >> &mtk_ccifreq_profile,
> >> 374 DEVFREQ_
> >> GOV_PASSIVE,
> >> 375 passive_
> >> data);
> >> 376 if (IS_ERR(drv->devfreq)) {
> >> 377 ret = -EPROBE_DEFER;
> >> 378 dev_err(dev, "failed to add devfreq
> >> device: %d\n",
> >> > 379 PTR_ERR(drv->devfreq));
> >> 380 goto out_remove_opp_table;
> >> 381 }
> >> 382
> >> 383 drv->opp_nb.notifier_call =
> >> mtk_ccifreq_opp_notifier;
> >> 384 ret = dev_pm_opp_register_notifier(dev, &drv-
> >>> opp_nb);
> >> 385 if (ret) {
> >> 386 dev_err(dev, "failed to register opp
> >> notifier: %d\n", ret);
> >> 387 goto out_remove_devfreq_device;
> >> 388 }
> >> 389 return 0;
> >> 390
> >> 391 out_remove_devfreq_device:
> >> 392 devm_devfreq_remove_device(dev, drv->devfreq);
> >> 393
> >> 394 out_remove_opp_table:
> >> 395 dev_pm_opp_of_remove_table(dev);
> >> 396
> >> 397 out_disable_inter_clk:
> >> 398 clk_disable_unprepare(drv->inter_clk);
> >> 399
> >> 400 out_disable_cci_clk:
> >> 401 clk_disable_unprepare(drv->cci_clk);
> >> 402
> >> 403 out_free_resources:
> >> 404 if (regulator_is_enabled(drv->proc_reg))
> >> 405 regulator_disable(drv->proc_reg);
> >> 406 if (drv->sram_reg && regulator_is_enabled(drv-
> >>> sram_reg))
> >> 407 regulator_disable(drv->sram_reg);
> >> 408
> >> 409 if (!IS_ERR(drv->proc_reg))
> >> 410 regulator_put(drv->proc_reg);
> >> 411 if (!IS_ERR(drv->sram_reg))
> >> 412 regulator_put(drv->sram_reg);
> >> 413 if (!IS_ERR(drv->cci_clk))
> >> 414 clk_put(drv->cci_clk);
> >> 415 if (!IS_ERR(drv->inter_clk))
> >> 416 clk_put(drv->inter_clk);
> >> 417
> >> 418 return ret;
> >> 419 }
> >> 420
> >>
> >
> > Hi "kernel test robot",
> >
> > Thanks for your review.
> >
> > This patch is based on chanwoo/devfreq-testing[1]
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
>
> Hi Johnson,
>
> Thanks for the feedback, we'll take a look too.

I think the last patch on that branch might be broken.

ChenYu

2022-05-09 03:29:27

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

Hi Johnson,

On 22. 4. 25. 21:55, Johnson Wang wrote:
> We introduce a devfreq driver for the MediaTek Cache Coherent Interconnect
> (CCI) used by some MediaTek SoCs.
>
> In this driver, we use the passive devfreq driver to get target frequencies
> and adjust voltages accordingly. In MT8183 and MT8186, the MediaTek CCI
> is supplied by the same regulators with the little core CPUs.
>
> Signed-off-by: Johnson Wang <[email protected]>
> Signed-off-by: Jia-Wei Chang <[email protected]>
> ---
> This patch depends on "devfreq-testing"[1].
> [1]https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
> ---
> drivers/devfreq/Kconfig | 10 +
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/mtk-cci-devfreq.c | 474 ++++++++++++++++++++++++++++++
> 3 files changed, 485 insertions(+)
> create mode 100644 drivers/devfreq/mtk-cci-devfreq.c
Acked-by: Chanwoo Choi <[email protected]>

I updated the passive governor on devfreq-testing[1] branch
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing

Could you please test your patches based on devfreq-testing[1] branch?

--
Best Regards,
Samsung Electronics
Chanwoo Choi

2022-05-09 07:30:27

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

On 22. 5. 6. 20:38, Johnson Wang wrote:
> On Mon, 2022-04-25 at 20:55 +0800, Johnson Wang wrote:
>> We introduce a devfreq driver for the MediaTek Cache Coherent
>> Interconnect
>> (CCI) used by some MediaTek SoCs.
>>
>> In this driver, we use the passive devfreq driver to get target
>> frequencies
>> and adjust voltages accordingly. In MT8183 and MT8186, the MediaTek
>> CCI
>> is supplied by the same regulators with the little core CPUs.
>>
>> Signed-off-by: Johnson Wang <[email protected]>
>> Signed-off-by: Jia-Wei Chang <[email protected]>
>> ---
>> This patch depends on "devfreq-testing"[1].
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
>> ---
>> drivers/devfreq/Kconfig | 10 +
>> drivers/devfreq/Makefile | 1 +
>> drivers/devfreq/mtk-cci-devfreq.c | 474
>> ++++++++++++++++++++++++++++++
>> 3 files changed, 485 insertions(+)
>> create mode 100644 drivers/devfreq/mtk-cci-devfreq.c
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 87eb2b837e68..9754d8b31621 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -120,6 +120,16 @@ 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 ARM_MEDIATEK_CPUFREQ || COMPILE_TEST
>> + select DEVFREQ_GOV_PASSIVE
>> + help
>> + This adds a devfreq driver for MediaTek Cache Coherent
>> Interconnect
>> + which is shared the same regulators with the cpu cluster. It
>> can track
>> + buck voltages and update a proper CCI frequency. Use the
>> notification
>> + to get the 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 0b6be92a25d9..bf40d04928d0 100644
>> --- a/drivers/devfreq/Makefile
>> +++ b/drivers/devfreq/Makefile
>> @@ -11,6 +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_MEDIATEK_CCI_DEVFREQ) += mtk-cci-devfreq.o
>> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
>> obj-$(CONFIG_ARM_SUN8I_A33_MBUS_DEVFREQ) += sun8i-a33-mbus.o
>> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o
>> diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-
>> cci-devfreq.c
>> new file mode 100644
>> index 000000000000..b3e31c45a57c
>> --- /dev/null
>> +++ b/drivers/devfreq/mtk-cci-devfreq.c
>> @@ -0,0 +1,474 @@
>> +// 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 {
>> + int min_volt_shift;
>> + int max_volt_shift;
>> + int proc_max_volt;
>> + int sram_min_volt;
>> + int sram_max_volt;
>> +};
>> +
>> +struct mtk_ccifreq_drv {
>> + struct device *dev;
>> + struct devfreq *devfreq;
>> + struct regulator *proc_reg;
>> + struct regulator *sram_reg;
>> + struct clk *cci_clk;
>> + struct clk *inter_clk;
>> + int inter_voltage;
>> + int pre_voltage;
>> + unsigned long pre_freq;
>> + /* Avoid race condition for regulators between notify and
>> policy */
>> + struct mutex reg_lock;
>> + struct notifier_block opp_nb;
>> + const struct mtk_ccifreq_platform_data *soc_data;
>> + int vtrack_max;
>> +};
>> +
>> +static int mtk_ccifreq_set_voltage(struct mtk_ccifreq_drv *drv, int
>> new_voltage)
>> +{
>> + const struct mtk_ccifreq_platform_data *soc_data = drv-
>>> soc_data;
>> + struct device *dev = drv->dev;
>> + int pre_voltage, pre_vsram, new_vsram, vsram, voltage, ret;
>> + int retry_max = drv->vtrack_max;
>> +
>> + if (!drv->sram_reg) {
>> + ret = regulator_set_voltage(drv->proc_reg, new_voltage,
>> + drv->soc_data-
>>> proc_max_volt);
>> + goto out_set_voltage;
>> + }
>> +
>> + pre_voltage = regulator_get_voltage(drv->proc_reg);
>> + if (pre_voltage < 0) {
>> + dev_err(dev, "invalid vproc value: %d\n", pre_voltage);
>> + return pre_voltage;
>> + }
>> +
>> + pre_vsram = regulator_get_voltage(drv->sram_reg);
>> + if (pre_vsram < 0) {
>> + dev_err(dev, "invalid vsram value: %d\n", pre_vsram);
>> + return pre_vsram;
>> + }
>> +
>> + new_vsram = clamp(new_voltage + soc_data->min_volt_shift,
>> + soc_data->sram_min_volt, soc_data-
>>> sram_max_volt);
>> +
>> + do {
>> + if (pre_voltage <= new_voltage) {
>> + vsram = clamp(pre_voltage + soc_data-
>>> max_volt_shift,
>> + soc_data->sram_min_volt,
>> new_vsram);
>> + ret = regulator_set_voltage(drv->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(drv->proc_reg,
>> voltage,
>> + soc_data-
>>> proc_max_volt);
>> + if (ret) {
>> + regulator_set_voltage(drv->sram_reg,
>> pre_vsram,
>> + soc_data-
>>> sram_max_volt);
>> + return ret;
>> + }
>> + } else if (pre_voltage > new_voltage) {
>> + voltage = max(new_voltage,
>> + pre_vsram - soc_data-
>>> max_volt_shift);
>> + ret = regulator_set_voltage(drv->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(drv->sram_reg,
>> vsram,
>> + soc_data-
>>> sram_max_volt);
>> + if (ret) {
>> + regulator_set_voltage(drv->proc_reg,
>> pre_voltage,
>> + soc_data-
>>> proc_max_volt);
>> + return ret;
>> + }
>> + }
>> +
>> + pre_voltage = voltage;
>> + pre_vsram = vsram;
>> +
>> + if (--retry_max < 0) {
>> + dev_err(dev,
>> + "over loop count, failed to set
>> voltage\n");
>> + return -EINVAL;
>> + }
>> + } while (voltage != new_voltage || vsram != new_vsram);
>> +
>> +out_set_voltage:
>> + if (!ret)
>> + drv->pre_voltage = new_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, pre_voltage, inter_voltage, target_voltage, ret;
>> +
>> + if (!drv)
>> + return -EINVAL;
>> +
>> + if (drv->pre_freq == *freq)
>> + return 0;
>> +
>> + inter_voltage = drv->inter_voltage;
>> +
>> + opp_rate = *freq;
>> + opp = devfreq_recommended_opp(dev, &opp_rate, 1);
>> + if (IS_ERR(opp)) {
>> + dev_err(dev, "failed to find opp for freq: %ld\n",
>> opp_rate);
>> + return PTR_ERR(opp);
>> + }
>> +
>> + mutex_lock(&drv->reg_lock);
>> +
>> + voltage = dev_pm_opp_get_voltage(opp);
>> + dev_pm_opp_put(opp);
>> +
>> + if (unlikely(drv->pre_voltage <= 0))
>> + pre_voltage = regulator_get_voltage(drv->proc_reg);
>> + else
>> + pre_voltage = drv->pre_voltage;
>> +
>> + if (pre_voltage < 0) {
>> + dev_err(dev, "invalid vproc value: %d\n", pre_voltage);
>> + return pre_voltage;
>> + }
>> +
>> + /* scale up: set voltage first then freq. */
>> + target_voltage = max(inter_voltage, voltage);
>> + if (pre_voltage <= target_voltage) {
>> + ret = mtk_ccifreq_set_voltage(drv, target_voltage);
>> + if (ret) {
>> + dev_err(dev, "failed to scale up voltage\n");
>> + goto out_restore_voltage;
>> + }
>> + }
>> +
>> + /* switch the cci clock to intermediate clock source. */
>> + ret = clk_set_parent(drv->cci_clk, drv->inter_clk);
>> + if (ret) {
>> + dev_err(dev, "failed to re-parent cci clock\n");
>> + goto out_restore_voltage;
>> + }
>> +
>> + /* set the original clock to target rate. */
>> + ret = clk_set_rate(cci_pll, *freq);
>> + if (ret) {
>> + dev_err(dev, "failed to set cci pll rate: %d\n", ret);
>> + clk_set_parent(drv->cci_clk, cci_pll);
>> + goto out_restore_voltage;
>> + }
>> +
>> + /* switch the cci clock back to the original clock source. */
>> + ret = clk_set_parent(drv->cci_clk, cci_pll);
>> + if (ret) {
>> + dev_err(dev, "failed to re-parent cci clock\n");
>> + mtk_ccifreq_set_voltage(drv, inter_voltage);
>> + goto out_unlock;
>> + }
>> +
>> + /*
>> + * 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 < pre_voltage) {
>> + ret = mtk_ccifreq_set_voltage(drv, voltage);
>> + if (ret) {
>> + dev_err(dev, "failed to scale down voltage\n");
>> + goto out_unlock;
>> + }
>> + }
>> +
>> + drv->pre_freq = *freq;
>> + mutex_unlock(&drv->reg_lock);
>> +
>> + return 0;
>> +
>> +out_restore_voltage:
>> + mtk_ccifreq_set_voltage(drv, pre_voltage);
>> +
>> +out_unlock:
>> + mutex_unlock(&drv->reg_lock);
>> + return ret;
>> +}
>> +
>> +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->reg_lock);
>> + /* current opp item is changed */
>> + if (freq == drv->pre_freq) {
>> + volt = dev_pm_opp_get_voltage(opp);
>> + mtk_ccifreq_set_voltage(drv, volt);
>> + }
>> + mutex_unlock(&drv->reg_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 *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(dev, sizeof(*drv), GFP_KERNEL);
>> + if (!drv)
>> + return -ENOMEM;
>> +
>> + drv->dev = dev;
>> + drv->soc_data = (const struct mtk_ccifreq_platform_data *)
>> + of_device_get_match_data(&pdev->dev);
>> + mutex_init(&drv->reg_lock);
>> + platform_set_drvdata(pdev, drv);
>> +
>> + drv->cci_clk = devm_clk_get(dev, "cci");
>> + if (IS_ERR(drv->cci_clk)) {
>> + ret = PTR_ERR(drv->cci_clk);
>> + return dev_err_probe(dev, ret,
>> + "failed to get cci clk: %d\n",
>> ret);
>> + }
>> +
>> + drv->inter_clk = devm_clk_get(dev, "intermediate");
>> + if (IS_ERR(drv->inter_clk)) {
>> + ret = PTR_ERR(drv->inter_clk);
>> + dev_err_probe(dev, ret,
>> + "failed to get intermediate clk: %d\n",
>> ret);
>> + goto out_free_resources;
>> + }
>> +
>> + drv->proc_reg = devm_regulator_get_optional(dev, "proc");
>> + if (IS_ERR(drv->proc_reg)) {
>> + ret = PTR_ERR(drv->proc_reg);
>> + dev_err_probe(dev, ret,
>> + "failed to get proc regulator: %d\n",
>> ret);
>> + goto out_free_resources;
>> + }
>> +
>> + ret = regulator_enable(drv->proc_reg);
>> + if (ret) {
>> + dev_err(dev, "failed to enable proc regulator\n");
>> + goto out_free_resources;
>> + }
>> +
>> + drv->sram_reg = regulator_get_optional(dev, "sram");
>> + if (IS_ERR(drv->sram_reg))
>> + drv->sram_reg = NULL;
>> + else {
>> + ret = regulator_enable(drv->sram_reg);
>> + if (ret) {
>> + dev_err(dev, "failed to enable sram
>> regulator\n");
>> + goto out_free_resources;
>> + }
>> + }
>> +
>> + /*
>> + * We assume min voltage is 0 and tracking target voltage using
>> + * min_volt_shift for each iteration.
>> + * The retry_max is 3 times of expeted iteration count.
>> + */
>> + drv->vtrack_max = 3 * DIV_ROUND_UP(max(drv->soc_data-
>>> sram_max_volt,
>> + drv->soc_data-
>>> proc_max_volt),
>> + drv->soc_data-
>>> min_volt_shift);
>> +
>> + ret = clk_prepare_enable(drv->cci_clk);
>> + if (ret)
>> + goto out_free_resources;
>> +
>> + ret = clk_prepare_enable(drv->inter_clk);
>> + if (ret)
>> + goto out_disable_cci_clk;
>> +
>> + ret = dev_pm_opp_of_add_table(dev);
>> + if (ret) {
>> + dev_err(dev, "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(dev, &rate);
>> + if (IS_ERR(opp)) {
>> + ret = PTR_ERR(opp);
>> + dev_err(dev, "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->dev, &rate);
>> + if (IS_ERR(opp)) {
>> + dev_err(dev, "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) {
>> + dev_err(dev, "failed to scale to highest voltage %lu in
>> proc_reg\n",
>> + opp_volt);
>> + goto out_remove_opp_table;
>> + }
>> +
>> + passive_data = devm_kzalloc(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(dev,
>> &mtk_ccifreq_profile,
>> + DEVFREQ_GOV_PASSIVE,
>> + passive_data);
>> + if (IS_ERR(drv->devfreq)) {
>> + ret = -EPROBE_DEFER;
>> + dev_err(dev, "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(dev, &drv->opp_nb);
>> + if (ret) {
>> + dev_err(dev, "failed to register opp notifier: %d\n",
>> ret);
>> + goto out_remove_devfreq_device;
>> + }
>> + return 0;
>> +
>> +out_remove_devfreq_device:
>> + devm_devfreq_remove_device(dev, drv->devfreq);
>> +
>> +out_remove_opp_table:
>> + dev_pm_opp_of_remove_table(dev);
>> +
>> +out_disable_inter_clk:
>> + clk_disable_unprepare(drv->inter_clk);
>> +
>> +out_disable_cci_clk:
>> + clk_disable_unprepare(drv->cci_clk);
>> +
>> +out_free_resources:
>> + if (regulator_is_enabled(drv->proc_reg))
>> + regulator_disable(drv->proc_reg);
>> + if (drv->sram_reg && regulator_is_enabled(drv->sram_reg))
>> + regulator_disable(drv->sram_reg);
>> +
>> + if (!IS_ERR(drv->proc_reg))
>> + regulator_put(drv->proc_reg);
>> + if (!IS_ERR(drv->sram_reg))
>> + regulator_put(drv->sram_reg);
>> + if (!IS_ERR(drv->cci_clk))
>> + clk_put(drv->cci_clk);
>> + if (!IS_ERR(drv->inter_clk))
>> + clk_put(drv->inter_clk);
>> +
>> + return ret;
>> +}
>> +
>> +static int mtk_ccifreq_remove(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct mtk_ccifreq_drv *drv;
>> +
>> + drv = platform_get_drvdata(pdev);
>> +
>> + dev_pm_opp_unregister_notifier(dev, &drv->opp_nb);
>> + dev_pm_opp_of_remove_table(dev);
>> + clk_disable_unprepare(drv->inter_clk);
>> + clk_disable_unprepare(drv->cci_clk);
>> + regulator_disable(drv->proc_reg);
>> + if (drv->sram_reg)
>> + regulator_disable(drv->sram_reg);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct mtk_ccifreq_platform_data mt8183_platform_data =
>> {
>> + .min_volt_shift = 100000,
>> + .max_volt_shift = 200000,
>> + .proc_max_volt = 1150000,
>> + .sram_min_volt = 0,
>> + .sram_max_volt = 1150000,
>> +};
>> +
>> +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,
>> +};
>> +
>> +static const struct of_device_id mtk_ccifreq_machines[] = {
>> + { .compatible = "mediatek,mt8183-cci", .data =
>> &mt8183_platform_data },
>> + { .compatible = "mediatek,mt8186-cci", .data =
>> &mt8186_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 = mtk_ccifreq_machines,
>> + },
>> +};
>> +module_platform_driver(mtk_ccifreq_platdrv);
>> +
>> +MODULE_DESCRIPTION("MediaTek CCI devfreq driver");
>> +MODULE_AUTHOR("Jia-Wei Chang <[email protected]>");
>> +MODULE_LICENSE("GPL v2");
>
> Hi Chanwoo,
>
> Just a kindly ping.
> Could you please give me some suggestion on this patch?
> Thanks!
>
> BRs,
> Johnson Wang
>

Hi Johnson,

Sorry for late reply.But I replied it yesterday as following:
Maybe, this reply[1] has not yet arrrived at your mail box.
[1]
https://lore.kernel.org/lkml/[email protected]/T/#m3bc609d805d6e74ec7a105be0511926b4afbbaef

As I described on reply[1], I updated the patches on devfreq-testing
branch. Could you please test your patches based on devfreq-testing
branch?

--
Best Regards,
Samsung Electronics
Chanwoo Choi

2022-05-09 10:01:54

[permalink] [raw]

Subject: Re: [kbuild-all] Re: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

Hi Chen-Yu,

On Mon, May 9, 2022 at 10:53 AM Chen-Yu Tsai <[email protected]> wrote:
>
> On Thu, Apr 28, 2022 at 7:39 PM Chen, Rong A <[email protected]> wrote:
> > On 4/27/2022 6:11 PM, Johnson Wang wrote:
> > > On Wed, 2022-04-27 at 17:25 +0800, kernel test robot wrote:
> > >> Hi Johnson,
> > >>
> > >> Thank you for the patch! Perhaps something to improve:
> > >>
> > >> [auto build test WARNING on robh/for-next]
> > >> [also build test WARNING on linus/master v5.18-rc4 next-20220427]
> > >> [If your patch is applied to the wrong git tree, kindly drop us a
> > >> note.
> > >> And when submitting patch, we suggest to use '--base' as documented
> > >> in
> > >>
> > > https://urldefense.com/v3/__https://git-scm.com/docs/git-format-patch__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMS_s9zEZ$
> > >> ]
> > >>
> > >> url:
> > >> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux/commits/Johnson-Wang/Introduce-MediaTek-CCI-devfreq-driver/20220425-205820__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMc1U_tqz$
> > >>
> > >> base:
> > >> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMUzTprof$
> > >> for-next
> > >> config: hexagon-allyesconfig (
> > >> https://urldefense.com/v3/__https://download.01.org/0day-ci/archive/20220427/[email protected]/config__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMaVRzbSL$
> > >> )
> > >> compiler: clang version 15.0.0 (
> > >> https://urldefense.com/v3/__https://github.com/llvm/llvm-project__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMRqw5IY-$
> > >> $ 1cddcfdc3c683b393df1a5c9063252eb60e52818)
> > >> reproduce (this is a W=1 build):
> > >> wget
> > >> https://urldefense.com/v3/__https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMQLiD-i9$
> > >> -O ~/bin/make.cross
> > >> chmod +x ~/bin/make.cross
> > >> #
> > >> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux/commit/98b34c0587837b0e5b880b11a52433f8f0eee19f__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMU5yd7Y2$
> > >>
> > >> git remote add linux-review
> > >> https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux__;!!CTRNKA9wMg0ARbw!wdyoWXNLBcYM97vMuNFQXZ9BaajEp-Kmh5-xrvU2Rlmb0o-b9tRvCD0cPzbLMW4ldtnH$
> > >>
> > >> git fetch --no-tags linux-review Johnson-Wang/Introduce-
> > >> MediaTek-CCI-devfreq-driver/20220425-205820
> > >> git checkout 98b34c0587837b0e5b880b11a52433f8f0eee19f
> > >> # save the config file
> > >> mkdir build_dir && cp config build_dir/.config
> > >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
> > >> W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash block/partitions/
> > >> drivers/devfreq/ drivers/iio/imu/ drivers/misc/lkdtm/
> > >>
> > >> If you fix the issue, kindly add following tag as appropriate
> > >> Reported-by: kernel test robot <[email protected]>
> > >>
> > >> All warnings (new ones prefixed by >>):
> > >>
> > >> drivers/devfreq/mtk-cci-devfreq.c:372:16: error: no member named
> > >> 'parent_type' in 'struct devfreq_passive_data'
> > >> passive_data->parent_type = CPUFREQ_PARENT_DEV;
> > >> ~~~~~~~~~~~~ ^
> > >> drivers/devfreq/mtk-cci-devfreq.c:372:30: error: use of undeclared
> > >> identifier 'CPUFREQ_PARENT_DEV'
> > >> passive_data->parent_type = CPUFREQ_PARENT_DEV;
> > >> ^
> > >>>> drivers/devfreq/mtk-cci-devfreq.c:379:4: warning: format
> > >>>> specifies type 'int' but the argument has type 'long' [-Wformat]
> > >>
> > >> PTR_ERR(drv->devfreq));
> > >> ^~~~~~~~~~~~~~~~~~~~~
> > >> include/linux/dev_printk.h:144:65: note: expanded from macro
> > >> 'dev_err'
> > >> dev_printk_index_wrap(_dev_err, KERN_ERR, dev,
> > >> dev_fmt(fmt), ##__VA_ARGS__)
> > >> ~~~
> > >> ^~~~~~~~~~~
> > >> include/linux/dev_printk.h:110:23: note: expanded from macro
> > >> 'dev_printk_index_wrap'
> > >> _p_func(dev, fmt,
> > >> ##__VA_ARGS__); \
> > >> ~~~ ^~~~~~~~~~~
> > >> 1 warning and 2 errors generated.
> > >>
> > >>
> > >> vim +379 drivers/devfreq/mtk-cci-devfreq.c
> > >>
> > >> 255
> > >> 256 static int mtk_ccifreq_probe(struct platform_device
> > >> *pdev)
> > >> 257 {
> > >> 258 struct device *dev = &pdev->dev;
> > >> 259 struct mtk_ccifreq_drv *drv;
> > >> 260 struct devfreq_passive_data *passive_data;
> > >> 261 struct dev_pm_opp *opp;
> > >> 262 unsigned long rate, opp_volt;
> > >> 263 int ret;
> > >> 264
> > >> 265 drv = devm_kzalloc(dev, sizeof(*drv),
> > >> GFP_KERNEL);
> > >> 266 if (!drv)
> > >> 267 return -ENOMEM;
> > >> 268
> > >> 269 drv->dev = dev;
> > >> 270 drv->soc_data = (const struct
> > >> mtk_ccifreq_platform_data *)
> > >> 271 of_device_get_match_dat
> > >> a(&pdev->dev);
> > >> 272 mutex_init(&drv->reg_lock);
> > >> 273 platform_set_drvdata(pdev, drv);
> > >> 274
> > >> 275 drv->cci_clk = devm_clk_get(dev, "cci");
> > >> 276 if (IS_ERR(drv->cci_clk)) {
> > >> 277 ret = PTR_ERR(drv->cci_clk);
> > >> 278 return dev_err_probe(dev, ret,
> > >> 279 "failed to get cci
> > >> clk: %d\n", ret);
> > >> 280 }
> > >> 281
> > >> 282 drv->inter_clk = devm_clk_get(dev,
> > >> "intermediate");
> > >> 283 if (IS_ERR(drv->inter_clk)) {
> > >> 284 ret = PTR_ERR(drv->inter_clk);
> > >> 285 dev_err_probe(dev, ret,
> > >> 286 "failed to get
> > >> intermediate clk: %d\n", ret);
> > >> 287 goto out_free_resources;
> > >> 288 }
> > >> 289
> > >> 290 drv->proc_reg =
> > >> devm_regulator_get_optional(dev, "proc");
> > >> 291 if (IS_ERR(drv->proc_reg)) {
> > >> 292 ret = PTR_ERR(drv->proc_reg);
> > >> 293 dev_err_probe(dev, ret,
> > >> 294 "failed to get proc
> > >> regulator: %d\n", ret);
> > >> 295 goto out_free_resources;
> > >> 296 }
> > >> 297
> > >> 298 ret = regulator_enable(drv->proc_reg);
> > >> 299 if (ret) {
> > >> 300 dev_err(dev, "failed to enable proc
> > >> regulator\n");
> > >> 301 goto out_free_resources;
> > >> 302 }
> > >> 303
> > >> 304 drv->sram_reg = regulator_get_optional(dev,
> > >> "sram");
> > >> 305 if (IS_ERR(drv->sram_reg))
> > >> 306 drv->sram_reg = NULL;
> > >> 307 else {
> > >> 308 ret = regulator_enable(drv->sram_reg);
> > >> 309 if (ret) {
> > >> 310 dev_err(dev, "failed to enable
> > >> sram regulator\n");
> > >> 311 goto out_free_resources;
> > >> 312 }
> > >> 313 }
> > >> 314
> > >> 315 /*
> > >> 316 * We assume min voltage is 0 and tracking
> > >> target voltage using
> > >> 317 * min_volt_shift for each iteration.
> > >> 318 * The retry_max is 3 times of expeted
> > >> iteration count.
> > >> 319 */
> > >> 320 drv->vtrack_max = 3 * DIV_ROUND_UP(max(drv-
> > >>> soc_data->sram_max_volt,
> > >> 321 drv-
> > >>> soc_data->proc_max_volt),
> > >> 322 drv-
> > >>> soc_data->min_volt_shift);
> > >> 323
> > >> 324 ret = clk_prepare_enable(drv->cci_clk);
> > >> 325 if (ret)
> > >> 326 goto out_free_resources;
> > >> 327
> > >> 328 ret = clk_prepare_enable(drv->inter_clk);
> > >> 329 if (ret)
> > >> 330 goto out_disable_cci_clk;
> > >> 331
> > >> 332 ret = dev_pm_opp_of_add_table(dev);
> > >> 333 if (ret) {
> > >> 334 dev_err(dev, "failed to add opp table:
> > >> %d\n", ret);
> > >> 335 goto out_disable_inter_clk;
> > >> 336 }
> > >> 337
> > >> 338 rate = clk_get_rate(drv->inter_clk);
> > >> 339 opp = dev_pm_opp_find_freq_ceil(dev, &rate);
> > >> 340 if (IS_ERR(opp)) {
> > >> 341 ret = PTR_ERR(opp);
> > >> 342 dev_err(dev, "failed to get
> > >> intermediate opp: %d\n", ret);
> > >> 343 goto out_remove_opp_table;
> > >> 344 }
> > >> 345 drv->inter_voltage =
> > >> dev_pm_opp_get_voltage(opp);
> > >> 346 dev_pm_opp_put(opp);
> > >> 347
> > >> 348 rate = U32_MAX;
> > >> 349 opp = dev_pm_opp_find_freq_floor(drv->dev,
> > >> &rate);
> > >> 350 if (IS_ERR(opp)) {
> > >> 351 dev_err(dev, "failed to get opp\n");
> > >> 352 ret = PTR_ERR(opp);
> > >> 353 goto out_remove_opp_table;
> > >> 354 }
> > >> 355
> > >> 356 opp_volt = dev_pm_opp_get_voltage(opp);
> > >> 357 dev_pm_opp_put(opp);
> > >> 358 ret = mtk_ccifreq_set_voltage(drv, opp_volt);
> > >> 359 if (ret) {
> > >> 360 dev_err(dev, "failed to scale to
> > >> highest voltage %lu in proc_reg\n",
> > >> 361 opp_volt);
> > >> 362 goto out_remove_opp_table;
> > >> 363 }
> > >> 364
> > >> 365 passive_data = devm_kzalloc(dev, sizeof(struct
> > >> devfreq_passive_data),
> > >> 366 GFP_KERNEL);
> > >> 367 if (!passive_data) {
> > >> 368 ret = -ENOMEM;
> > >> 369 goto out_remove_opp_table;
> > >> 370 }
> > >> 371
> > >> 372 passive_data->parent_type = CPUFREQ_PARENT_DEV;
> > >> 373 drv->devfreq = devm_devfreq_add_device(dev,
> > >> &mtk_ccifreq_profile,
> > >> 374 DEVFREQ_
> > >> GOV_PASSIVE,
> > >> 375 passive_
> > >> data);
> > >> 376 if (IS_ERR(drv->devfreq)) {
> > >> 377 ret = -EPROBE_DEFER;
> > >> 378 dev_err(dev, "failed to add devfreq
> > >> device: %d\n",
> > >> > 379 PTR_ERR(drv->devfreq));
> > >> 380 goto out_remove_opp_table;
> > >> 381 }
> > >> 382
> > >> 383 drv->opp_nb.notifier_call =
> > >> mtk_ccifreq_opp_notifier;
> > >> 384 ret = dev_pm_opp_register_notifier(dev, &drv-
> > >>> opp_nb);
> > >> 385 if (ret) {
> > >> 386 dev_err(dev, "failed to register opp
> > >> notifier: %d\n", ret);
> > >> 387 goto out_remove_devfreq_device;
> > >> 388 }
> > >> 389 return 0;
> > >> 390
> > >> 391 out_remove_devfreq_device:
> > >> 392 devm_devfreq_remove_device(dev, drv->devfreq);
> > >> 393
> > >> 394 out_remove_opp_table:
> > >> 395 dev_pm_opp_of_remove_table(dev);
> > >> 396
> > >> 397 out_disable_inter_clk:
> > >> 398 clk_disable_unprepare(drv->inter_clk);
> > >> 399
> > >> 400 out_disable_cci_clk:
> > >> 401 clk_disable_unprepare(drv->cci_clk);
> > >> 402
> > >> 403 out_free_resources:
> > >> 404 if (regulator_is_enabled(drv->proc_reg))
> > >> 405 regulator_disable(drv->proc_reg);
> > >> 406 if (drv->sram_reg && regulator_is_enabled(drv-
> > >>> sram_reg))
> > >> 407 regulator_disable(drv->sram_reg);
> > >> 408
> > >> 409 if (!IS_ERR(drv->proc_reg))
> > >> 410 regulator_put(drv->proc_reg);
> > >> 411 if (!IS_ERR(drv->sram_reg))
> > >> 412 regulator_put(drv->sram_reg);
> > >> 413 if (!IS_ERR(drv->cci_clk))
> > >> 414 clk_put(drv->cci_clk);
> > >> 415 if (!IS_ERR(drv->inter_clk))
> > >> 416 clk_put(drv->inter_clk);
> > >> 417
> > >> 418 return ret;
> > >> 419 }
> > >> 420
> > >>
> > >
> > > Hi "kernel test robot",
> > >
> > > Thanks for your review.
> > >
> > > This patch is based on chanwoo/devfreq-testing[1]
> > > [1]
> > > https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing
> >
> > Hi Johnson,
> >
> > Thanks for the feedback, we'll take a look too.
>
> I think the last patch on that branch might be broken.

You mean the patch[1]. Without this patch[1], there are no problems?
[1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=ea1011fba665b95fc28f682c9b131799a88b11ae

When you tested these patches with patchset[2] without last patch[1]
if there are no problems, please reply to patchset[2] with your Tested-by tag.
[2] https://patchwork.kernel.org/project/linux-pm/cover/[email protected]/

--
Best Regards,
Chanwoo Choi

2022-05-09 12:21:22

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

On 22. 5. 9. 14:57, Johnson Wang wrote:
> On Sat, 2022-05-07 at 22:53 +0900, Chanwoo Choi wrote:
>> On 22. 5. 6. 20:38, Johnson Wang wrote:
>>> On Mon, 2022-04-25 at 20:55 +0800, Johnson Wang wrote:
>>>> We introduce a devfreq driver for the MediaTek Cache Coherent
>>>> Interconnect
>>>> (CCI) used by some MediaTek SoCs.
>>>>
>>>> In this driver, we use the passive devfreq driver to get target
>>>> frequencies
>>>> and adjust voltages accordingly. In MT8183 and MT8186, the
>>>> MediaTek
>>>> CCI
>>>> is supplied by the same regulators with the little core CPUs.
>>>>
>>>> Signed-off-by: Johnson Wang <[email protected]>
>>>> Signed-off-by: Jia-Wei Chang <[email protected]>
>>>> ---
>>>> This patch depends on "devfreq-testing"[1].
>>>> [1]
>>>>
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing__;!!CTRNKA9wMg0ARbw!wnTkI_sh3TsliBPFP70DFwtSZGtKyPinFu9h2BwULWX-xrWF0C21rLV-n1HhstfmLw42$
>>>>
>>>> ---
>>>> drivers/devfreq/Kconfig | 10 +
>>>> drivers/devfreq/Makefile | 1 +
>>>> drivers/devfreq/mtk-cci-devfreq.c | 474
>>>> ++++++++++++++++++++++++++++++
>>>> 3 files changed, 485 insertions(+)
>>>> create mode 100644 drivers/devfreq/mtk-cci-devfreq.c
>>>>
>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>> index 87eb2b837e68..9754d8b31621 100644
>>>> --- a/drivers/devfreq/Kconfig
>>>> +++ b/drivers/devfreq/Kconfig
>>>> @@ -120,6 +120,16 @@ 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 ARM_MEDIATEK_CPUFREQ || COMPILE_TEST
>>>> + select DEVFREQ_GOV_PASSIVE
>>>> + help
>>>> + This adds a devfreq driver for MediaTek Cache Coherent
>>>> Interconnect
>>>> + which is shared the same regulators with the cpu cluster. It
>>>> can track
>>>> + buck voltages and update a proper CCI frequency. Use the
>>>> notification
>>>> + to get the 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 0b6be92a25d9..bf40d04928d0 100644
>>>> --- a/drivers/devfreq/Makefile
>>>> +++ b/drivers/devfreq/Makefile
>>>> @@ -11,6 +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_MEDIATEK_CCI_DEVFREQ) += mtk-cci-devfreq.o
>>>> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
>>>> obj-$(CONFIG_ARM_SUN8I_A33_MBUS_DEVFREQ) += sun8i-a33-
>>>> mbus.o
>>>> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o
>>>> diff --git a/drivers/devfreq/mtk-cci-devfreq.c
>>>> b/drivers/devfreq/mtk-
>>>> cci-devfreq.c
>>>> new file mode 100644
>>>> index 000000000000..b3e31c45a57c
>>>> --- /dev/null
>>>> +++ b/drivers/devfreq/mtk-cci-devfreq.c
>>>> @@ -0,0 +1,474 @@
>>>> +// 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 {
>>>> + int min_volt_shift;
>>>> + int max_volt_shift;
>>>> + int proc_max_volt;
>>>> + int sram_min_volt;
>>>> + int sram_max_volt;
>>>> +};
>>>> +
>>>> +struct mtk_ccifreq_drv {
>>>> + struct device *dev;
>>>> + struct devfreq *devfreq;
>>>> + struct regulator *proc_reg;
>>>> + struct regulator *sram_reg;
>>>> + struct clk *cci_clk;
>>>> + struct clk *inter_clk;
>>>> + int inter_voltage;
>>>> + int pre_voltage;
>>>> + unsigned long pre_freq;
>>>> + /* Avoid race condition for regulators between notify and
>>>> policy */
>>>> + struct mutex reg_lock;
>>>> + struct notifier_block opp_nb;
>>>> + const struct mtk_ccifreq_platform_data *soc_data;
>>>> + int vtrack_max;
>>>> +};
>>>> +
>>>> +static int mtk_ccifreq_set_voltage(struct mtk_ccifreq_drv *drv,
>>>> int
>>>> new_voltage)
>>>> +{
>>>> + const struct mtk_ccifreq_platform_data *soc_data = drv-
>>>>> soc_data;
>>>>
>>>> + struct device *dev = drv->dev;
>>>> + int pre_voltage, pre_vsram, new_vsram, vsram, voltage, ret;
>>>> + int retry_max = drv->vtrack_max;
>>>> +
>>>> + if (!drv->sram_reg) {
>>>> + ret = regulator_set_voltage(drv->proc_reg, new_voltage,
>>>> + drv->soc_data-
>>>>> proc_max_volt);
>>>>
>>>> + goto out_set_voltage;
>>>> + }
>>>> +
>>>> + pre_voltage = regulator_get_voltage(drv->proc_reg);
>>>> + if (pre_voltage < 0) {
>>>> + dev_err(dev, "invalid vproc value: %d\n", pre_voltage);
>>>> + return pre_voltage;
>>>> + }
>>>> +
>>>> + pre_vsram = regulator_get_voltage(drv->sram_reg);
>>>> + if (pre_vsram < 0) {
>>>> + dev_err(dev, "invalid vsram value: %d\n", pre_vsram);
>>>> + return pre_vsram;
>>>> + }
>>>> +
>>>> + new_vsram = clamp(new_voltage + soc_data->min_volt_shift,
>>>> + soc_data->sram_min_volt, soc_data-
>>>>> sram_max_volt);
>>>>
>>>> +
>>>> + do {
>>>> + if (pre_voltage <= new_voltage) {
>>>> + vsram = clamp(pre_voltage + soc_data-
>>>>> max_volt_shift,
>>>>
>>>> + soc_data->sram_min_volt,
>>>> new_vsram);
>>>> + ret = regulator_set_voltage(drv->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(drv->proc_reg,
>>>> voltage,
>>>> + soc_data-
>>>>> proc_max_volt);
>>>>
>>>> + if (ret) {
>>>> + regulator_set_voltage(drv->sram_reg,
>>>> pre_vsram,
>>>> + soc_data-
>>>>> sram_max_volt);
>>>>
>>>> + return ret;
>>>> + }
>>>> + } else if (pre_voltage > new_voltage) {
>>>> + voltage = max(new_voltage,
>>>> + pre_vsram - soc_data-
>>>>> max_volt_shift);
>>>>
>>>> + ret = regulator_set_voltage(drv->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(drv->sram_reg,
>>>> vsram,
>>>> + soc_data-
>>>>> sram_max_volt);
>>>>
>>>> + if (ret) {
>>>> + regulator_set_voltage(drv->proc_reg,
>>>> pre_voltage,
>>>> + soc_data-
>>>>> proc_max_volt);
>>>>
>>>> + return ret;
>>>> + }
>>>> + }
>>>> +
>>>> + pre_voltage = voltage;
>>>> + pre_vsram = vsram;
>>>> +
>>>> + if (--retry_max < 0) {
>>>> + dev_err(dev,
>>>> + "over loop count, failed to set
>>>> voltage\n");
>>>> + return -EINVAL;
>>>> + }
>>>> + } while (voltage != new_voltage || vsram != new_vsram);
>>>> +
>>>> +out_set_voltage:
>>>> + if (!ret)
>>>> + drv->pre_voltage = new_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, pre_voltage, inter_voltage, target_voltage, ret;
>>>> +
>>>> + if (!drv)
>>>> + return -EINVAL;
>>>> +
>>>> + if (drv->pre_freq == *freq)
>>>> + return 0;
>>>> +
>>>> + inter_voltage = drv->inter_voltage;
>>>> +
>>>> + opp_rate = *freq;
>>>> + opp = devfreq_recommended_opp(dev, &opp_rate, 1);
>>>> + if (IS_ERR(opp)) {
>>>> + dev_err(dev, "failed to find opp for freq: %ld\n",
>>>> opp_rate);
>>>> + return PTR_ERR(opp);
>>>> + }
>>>> +
>>>> + mutex_lock(&drv->reg_lock);
>>>> +
>>>> + voltage = dev_pm_opp_get_voltage(opp);
>>>> + dev_pm_opp_put(opp);
>>>> +
>>>> + if (unlikely(drv->pre_voltage <= 0))
>>>> + pre_voltage = regulator_get_voltage(drv->proc_reg);
>>>> + else
>>>> + pre_voltage = drv->pre_voltage;
>>>> +
>>>> + if (pre_voltage < 0) {
>>>> + dev_err(dev, "invalid vproc value: %d\n", pre_voltage);
>>>> + return pre_voltage;
>>>> + }
>>>> +
>>>> + /* scale up: set voltage first then freq. */
>>>> + target_voltage = max(inter_voltage, voltage);
>>>> + if (pre_voltage <= target_voltage) {
>>>> + ret = mtk_ccifreq_set_voltage(drv, target_voltage);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to scale up voltage\n");
>>>> + goto out_restore_voltage;
>>>> + }
>>>> + }
>>>> +
>>>> + /* switch the cci clock to intermediate clock source. */
>>>> + ret = clk_set_parent(drv->cci_clk, drv->inter_clk);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to re-parent cci clock\n");
>>>> + goto out_restore_voltage;
>>>> + }
>>>> +
>>>> + /* set the original clock to target rate. */
>>>> + ret = clk_set_rate(cci_pll, *freq);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to set cci pll rate: %d\n", ret);
>>>> + clk_set_parent(drv->cci_clk, cci_pll);
>>>> + goto out_restore_voltage;
>>>> + }
>>>> +
>>>> + /* switch the cci clock back to the original clock source. */
>>>> + ret = clk_set_parent(drv->cci_clk, cci_pll);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to re-parent cci clock\n");
>>>> + mtk_ccifreq_set_voltage(drv, inter_voltage);
>>>> + goto out_unlock;
>>>> + }
>>>> +
>>>> + /*
>>>> + * 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 < pre_voltage) {
>>>> + ret = mtk_ccifreq_set_voltage(drv, voltage);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to scale down voltage\n");
>>>> + goto out_unlock;
>>>> + }
>>>> + }
>>>> +
>>>> + drv->pre_freq = *freq;
>>>> + mutex_unlock(&drv->reg_lock);
>>>> +
>>>> + return 0;
>>>> +
>>>> +out_restore_voltage:
>>>> + mtk_ccifreq_set_voltage(drv, pre_voltage);
>>>> +
>>>> +out_unlock:
>>>> + mutex_unlock(&drv->reg_lock);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +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->reg_lock);
>>>> + /* current opp item is changed */
>>>> + if (freq == drv->pre_freq) {
>>>> + volt = dev_pm_opp_get_voltage(opp);
>>>> + mtk_ccifreq_set_voltage(drv, volt);
>>>> + }
>>>> + mutex_unlock(&drv->reg_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 *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(dev, sizeof(*drv), GFP_KERNEL);
>>>> + if (!drv)
>>>> + return -ENOMEM;
>>>> +
>>>> + drv->dev = dev;
>>>> + drv->soc_data = (const struct mtk_ccifreq_platform_data *)
>>>> + of_device_get_match_data(&pdev->dev);
>>>> + mutex_init(&drv->reg_lock);
>>>> + platform_set_drvdata(pdev, drv);
>>>> +
>>>> + drv->cci_clk = devm_clk_get(dev, "cci");
>>>> + if (IS_ERR(drv->cci_clk)) {
>>>> + ret = PTR_ERR(drv->cci_clk);
>>>> + return dev_err_probe(dev, ret,
>>>> + "failed to get cci clk: %d\n",
>>>> ret);
>>>> + }
>>>> +
>>>> + drv->inter_clk = devm_clk_get(dev, "intermediate");
>>>> + if (IS_ERR(drv->inter_clk)) {
>>>> + ret = PTR_ERR(drv->inter_clk);
>>>> + dev_err_probe(dev, ret,
>>>> + "failed to get intermediate clk: %d\n",
>>>> ret);
>>>> + goto out_free_resources;
>>>> + }
>>>> +
>>>> + drv->proc_reg = devm_regulator_get_optional(dev, "proc");
>>>> + if (IS_ERR(drv->proc_reg)) {
>>>> + ret = PTR_ERR(drv->proc_reg);
>>>> + dev_err_probe(dev, ret,
>>>> + "failed to get proc regulator: %d\n",
>>>> ret);
>>>> + goto out_free_resources;
>>>> + }
>>>> +
>>>> + ret = regulator_enable(drv->proc_reg);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to enable proc regulator\n");
>>>> + goto out_free_resources;
>>>> + }
>>>> +
>>>> + drv->sram_reg = regulator_get_optional(dev, "sram");
>>>> + if (IS_ERR(drv->sram_reg))
>>>> + drv->sram_reg = NULL;
>>>> + else {
>>>> + ret = regulator_enable(drv->sram_reg);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to enable sram
>>>> regulator\n");
>>>> + goto out_free_resources;
>>>> + }
>>>> + }
>>>> +
>>>> + /*
>>>> + * We assume min voltage is 0 and tracking target voltage using
>>>> + * min_volt_shift for each iteration.
>>>> + * The retry_max is 3 times of expeted iteration count.
>>>> + */
>>>> + drv->vtrack_max = 3 * DIV_ROUND_UP(max(drv->soc_data-
>>>>> sram_max_volt,
>>>>
>>>> + drv->soc_data-
>>>>> proc_max_volt),
>>>>
>>>> + drv->soc_data-
>>>>> min_volt_shift);
>>>>
>>>> +
>>>> + ret = clk_prepare_enable(drv->cci_clk);
>>>> + if (ret)
>>>> + goto out_free_resources;
>>>> +
>>>> + ret = clk_prepare_enable(drv->inter_clk);
>>>> + if (ret)
>>>> + goto out_disable_cci_clk;
>>>> +
>>>> + ret = dev_pm_opp_of_add_table(dev);
>>>> + if (ret) {
>>>> + dev_err(dev, "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(dev, &rate);
>>>> + if (IS_ERR(opp)) {
>>>> + ret = PTR_ERR(opp);
>>>> + dev_err(dev, "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->dev, &rate);
>>>> + if (IS_ERR(opp)) {
>>>> + dev_err(dev, "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) {
>>>> + dev_err(dev, "failed to scale to highest voltage %lu in
>>>> proc_reg\n",
>>>> + opp_volt);
>>>> + goto out_remove_opp_table;
>>>> + }
>>>> +
>>>> + passive_data = devm_kzalloc(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(dev,
>>>> &mtk_ccifreq_profile,
>>>> + DEVFREQ_GOV_PASSIVE,
>>>> + passive_data);
>>>> + if (IS_ERR(drv->devfreq)) {
>>>> + ret = -EPROBE_DEFER;
>>>> + dev_err(dev, "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(dev, &drv->opp_nb);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to register opp notifier: %d\n",
>>>> ret);
>>>> + goto out_remove_devfreq_device;
>>>> + }
>>>> + return 0;
>>>> +
>>>> +out_remove_devfreq_device:
>>>> + devm_devfreq_remove_device(dev, drv->devfreq);
>>>> +
>>>> +out_remove_opp_table:
>>>> + dev_pm_opp_of_remove_table(dev);
>>>> +
>>>> +out_disable_inter_clk:
>>>> + clk_disable_unprepare(drv->inter_clk);
>>>> +
>>>> +out_disable_cci_clk:
>>>> + clk_disable_unprepare(drv->cci_clk);
>>>> +
>>>> +out_free_resources:
>>>> + if (regulator_is_enabled(drv->proc_reg))
>>>> + regulator_disable(drv->proc_reg);
>>>> + if (drv->sram_reg && regulator_is_enabled(drv->sram_reg))
>>>> + regulator_disable(drv->sram_reg);
>>>> +
>>>> + if (!IS_ERR(drv->proc_reg))
>>>> + regulator_put(drv->proc_reg);
>>>> + if (!IS_ERR(drv->sram_reg))
>>>> + regulator_put(drv->sram_reg);
>>>> + if (!IS_ERR(drv->cci_clk))
>>>> + clk_put(drv->cci_clk);
>>>> + if (!IS_ERR(drv->inter_clk))
>>>> + clk_put(drv->inter_clk);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int mtk_ccifreq_remove(struct platform_device *pdev)
>>>> +{
>>>> + struct device *dev = &pdev->dev;
>>>> + struct mtk_ccifreq_drv *drv;
>>>> +
>>>> + drv = platform_get_drvdata(pdev);
>>>> +
>>>> + dev_pm_opp_unregister_notifier(dev, &drv->opp_nb);
>>>> + dev_pm_opp_of_remove_table(dev);
>>>> + clk_disable_unprepare(drv->inter_clk);
>>>> + clk_disable_unprepare(drv->cci_clk);
>>>> + regulator_disable(drv->proc_reg);
>>>> + if (drv->sram_reg)
>>>> + regulator_disable(drv->sram_reg);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct mtk_ccifreq_platform_data
>>>> mt8183_platform_data =
>>>> {
>>>> + .min_volt_shift = 100000,
>>>> + .max_volt_shift = 200000,
>>>> + .proc_max_volt = 1150000,
>>>> + .sram_min_volt = 0,
>>>> + .sram_max_volt = 1150000,
>>>> +};
>>>> +
>>>> +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,
>>>> +};
>>>> +
>>>> +static const struct of_device_id mtk_ccifreq_machines[] = {
>>>> + { .compatible = "mediatek,mt8183-cci", .data =
>>>> &mt8183_platform_data },
>>>> + { .compatible = "mediatek,mt8186-cci", .data =
>>>> &mt8186_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 = mtk_ccifreq_machines,
>>>> + },
>>>> +};
>>>> +module_platform_driver(mtk_ccifreq_platdrv);
>>>> +
>>>> +MODULE_DESCRIPTION("MediaTek CCI devfreq driver");
>>>> +MODULE_AUTHOR("Jia-Wei Chang <[email protected]>");
>>>> +MODULE_LICENSE("GPL v2");
>>>
>>> Hi Chanwoo,
>>>
>>> Just a kindly ping.
>>> Could you please give me some suggestion on this patch?
>>> Thanks!
>>>
>>> BRs,
>>> Johnson Wang
>>>
>>
>> Hi Johnson,
>>
>> Sorry for late reply.But I replied it yesterday as following:
>> Maybe, this reply[1] has not yet arrrived at your mail box.
>> [1]
>>
> https://lore.kernel.org/lkml/[email protected]/T/#m3bc609d805d6e74ec7a105be0511926b4afbbaef
>>
>> As I described on reply[1], I updated the patches on devfreq-testing
>> branch. Could you please test your patches based on devfreq-testing
>> branch?
>>
>
> Hi Chanwoo,
>
> Thanks for your information.
> I've tested this patch based on the latest devfreq-testing branch.
> It encounters the same crash as Chen-Yu mentioned[1].
>
> [1]https://lore.kernel.org/linux-pm/[email protected]/T/#u

Hi Johnson,

Thanks for the test. I'll drop the last patch caused of crash.
And I'll send v3 patchset right now.

--
Best Regards,
Samsung Electronics
Chanwoo Choi

2022-05-09 12:35:11

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI dt-bindings

Hi,

On 22. 4. 25. 21:55, Johnson Wang wrote:
> Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
>
> Signed-off-by: Johnson Wang <[email protected]>
> Signed-off-by: Jia-Wei Chang <[email protected]>
> ---
> .../bindings/interconnect/mediatek,cci.yaml | 139 ++++++++++++++++++
> 1 file changed, 139 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
>
> diff --git a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> new file mode 100644
> index 000000000000..e5221e17d11b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> @@ -0,0 +1,139 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interconnect/mediatek,cci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Cache Coherent Interconnect (CCI) frequency and voltage scaling
> +
> +maintainers:
> + - Jia-Wei Chang <[email protected]>

Why did you add your author information?
Please add your author information.

And add this dt-binding information to MAINTAINERS
as following: because I cannot catch the later patch
of modification.

cwchoi00@chanwoo:~/kernel/linux.chanwoo$ d
diff --git a/MAINTAINERS b/MAINTAINERS
index edc96cdb85e8..a11e9c1947b7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5698,6 +5698,7 @@ L: [email protected]
S: Maintained
T: git git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git
F: Documentation/devicetree/bindings/devfreq/
+F: Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
F: drivers/devfreq/
F: include/linux/devfreq.h
F: include/trace/events/devfreq.h

> +
> +description: |
> + MediaTek Cache Coherent Interconnect (CCI) is a hardware engine used by
> + MT8183 and MT8186 SoCs to scale the frequency and adjust the voltage in
> + hardware. It can also optimize the voltage to reduce the power consumption.
> +
> +properties:
> + compatible:
> + enum:
> + - mediatek,mt8183-cci
> + - mediatek,mt8186-cci
> +
> + clocks:
> + items:
> + - description:
> + The multiplexer for clock input of CPU cluster.
> + - description:
> + A parent of "cpu" clock which 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: true
> + 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 it presents, 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/mt8183-clk.h>
> + cci: cci {
> + compatible = "mediatek,mt8183-cci";
> + clocks = <&mcucfg CLK_MCU_BUS_SEL>,
> + <&topckgen CLK_TOP_ARMPLL_DIV_PLL1>;
> + clock-names = "cci", "intermediate";
> + operating-points-v2 = <&cci_opp>;
> + proc-supply = <&mt6358_vproc12_reg>;
> + };
> +
> + cci_opp: opp-table-cci {
> + compatible = "operating-points-v2";
> + opp-shared;
> + opp2_00: opp-273000000 {
> + opp-hz = /bits/ 64 <273000000>;
> + opp-microvolt = <650000>;
> + };
> + opp2_01: opp-338000000 {
> + opp-hz = /bits/ 64 <338000000>;
> + opp-microvolt = <687500>;
> + };
> + opp2_02: opp-403000000 {
> + opp-hz = /bits/ 64 <403000000>;
> + opp-microvolt = <718750>;
> + };
> + opp2_03: opp-463000000 {
> + opp-hz = /bits/ 64 <463000000>;
> + opp-microvolt = <756250>;
> + };
> + opp2_04: opp-546000000 {
> + opp-hz = /bits/ 64 <546000000>;
> + opp-microvolt = <800000>;
> + };
> + opp2_05: opp-624000000 {
> + opp-hz = /bits/ 64 <624000000>;
> + opp-microvolt = <818750>;
> + };
> + opp2_06: opp-689000000 {
> + opp-hz = /bits/ 64 <689000000>;
> + opp-microvolt = <850000>;
> + };
> + opp2_07: opp-767000000 {
> + opp-hz = /bits/ 64 <767000000>;
> + opp-microvolt = <868750>;
> + };
> + opp2_08: opp-845000000 {
> + opp-hz = /bits/ 64 <845000000>;
> + opp-microvolt = <893750>;
> + };
> + opp2_09: opp-871000000 {
> + opp-hz = /bits/ 64 <871000000>;
> + opp-microvolt = <906250>;
> + };
> + opp2_10: opp-923000000 {
> + opp-hz = /bits/ 64 <923000000>;
> + opp-microvolt = <931250>;
> + };
> + opp2_11: opp-962000000 {
> + opp-hz = /bits/ 64 <962000000>;
> + opp-microvolt = <943750>;
> + };
> + opp2_12: opp-1027000000 {
> + opp-hz = /bits/ 64 <1027000000>;
> + opp-microvolt = <975000>;
> + };
> + opp2_13: opp-1092000000 {
> + opp-hz = /bits/ 64 <1092000000>;
> + opp-microvolt = <1000000>;
> + };
> + opp2_14: opp-1144000000 {
> + opp-hz = /bits/ 64 <1144000000>;
> + opp-microvolt = <1025000>;
> + };
> + opp2_15: opp-1196000000 {
> + opp-hz = /bits/ 64 <1196000000>;
> + opp-microvolt = <1050000>;
> + };
> + };

--
Best Regards,
Samsung Electronics
Chanwoo Choi

2022-05-09 12:40:13

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI dt-bindings

Hi Chen-Yu,

On Tue, 2022-04-26 at 11:18 +0800, Chen-Yu Tsai wrote:
> On Mon, Apr 25, 2022 at 8:56 PM Johnson Wang <
> [email protected]> wrote:
> >
> > Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
> >
> > Signed-off-by: Johnson Wang <[email protected]>
> > Signed-off-by: Jia-Wei Chang <[email protected]>
> > ---
> > .../bindings/interconnect/mediatek,cci.yaml | 139
> > ++++++++++++++++++
> > 1 file changed, 139 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > new file mode 100644
> > index 000000000000..e5221e17d11b
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > @@ -0,0 +1,139 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> > https://urldefense.com/v3/__http://devicetree.org/schemas/interconnect/mediatek,cci.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDlQ9pSDO$
> >
> > +$schema:
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDoE9YHyu$
> >
> > +
> > +title: MediaTek Cache Coherent Interconnect (CCI) frequency and
> > voltage scaling
> > +
> > +maintainers:
> > + - Jia-Wei Chang <[email protected]>
> > +
> > +description: |
> > + MediaTek Cache Coherent Interconnect (CCI) is a hardware engine
> > used by
> > + MT8183 and MT8186 SoCs to scale the frequency and adjust the
> > voltage in
> > + hardware. It can also optimize the voltage to reduce the power
> > consumption.
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - mediatek,mt8183-cci
> > + - mediatek,mt8186-cci
> > +
> > + clocks:
> > + items:
> > + - description:
> > + The multiplexer for clock input of CPU cluster.
>
> of the bus, not CPU cluster.

Thanks for your suggestion.
I will correct it in the next version.

>
> > + - description:
> > + A parent of "cpu" clock which is used as an intermediate
> > clock source
> > + when the original CPU is under transition and not stable
> > yet.
>
> This really should be handled in the clk controller, and not by every
> device
> that happens to take a clock from a mux with upstream PLLs that can
> change
> in clock rate. The end device hardware only takes one clock input.
> That's it.
>

To make this intermediate clock works properly, this driver is also
responsible for handling the Vproc voltage and ensures the voltage is
high enough to support intermediate clock rate.

If we move intermediate clock rate control to clock driver, then
intermediate voltage control may be handled by the clock driver itself
as well.

We believe that is not reasonable because clock driver shouldn't handle
voltage control. On the other hand, DVFS driver is more suitable for
doing this job.

> > +
> > + clock-names:
> > + items:
> > + - const: cci
> > + - const: intermediate
> > +
> > + operating-points-v2: true
> > + 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 it presents, the cci devfreq driver needs to
> > do
>
> When it is present, the implementation needs to ...
>
> ChenYu

I will modify it in the next version.

BRs,
Johnson Wang

>
> > + "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/mt8183-clk.h>
> > + cci: cci {
> > + compatible = "mediatek,mt8183-cci";
> > + clocks = <&mcucfg CLK_MCU_BUS_SEL>,
> > + <&topckgen CLK_TOP_ARMPLL_DIV_PLL1>;
> > + clock-names = "cci", "intermediate";
> > + operating-points-v2 = <&cci_opp>;
> > + proc-supply = <&mt6358_vproc12_reg>;
> > + };
> > +
> > + cci_opp: opp-table-cci {
> > + compatible = "operating-points-v2";
> > + opp-shared;
> > + opp2_00: opp-273000000 {
> > + opp-hz = /bits/ 64 <273000000>;
> > + opp-microvolt = <650000>;
> > + };
> > + opp2_01: opp-338000000 {
> > + opp-hz = /bits/ 64 <338000000>;
> > + opp-microvolt = <687500>;
> > + };
> > + opp2_02: opp-403000000 {
> > + opp-hz = /bits/ 64 <403000000>;
> > + opp-microvolt = <718750>;
> > + };
> > + opp2_03: opp-463000000 {
> > + opp-hz = /bits/ 64 <463000000>;
> > + opp-microvolt = <756250>;
> > + };
> > + opp2_04: opp-546000000 {
> > + opp-hz = /bits/ 64 <546000000>;
> > + opp-microvolt = <800000>;
> > + };
> > + opp2_05: opp-624000000 {
> > + opp-hz = /bits/ 64 <624000000>;
> > + opp-microvolt = <818750>;
> > + };
> > + opp2_06: opp-689000000 {
> > + opp-hz = /bits/ 64 <689000000>;
> > + opp-microvolt = <850000>;
> > + };
> > + opp2_07: opp-767000000 {
> > + opp-hz = /bits/ 64 <767000000>;
> > + opp-microvolt = <868750>;
> > + };
> > + opp2_08: opp-845000000 {
> > + opp-hz = /bits/ 64 <845000000>;
> > + opp-microvolt = <893750>;
> > + };
> > + opp2_09: opp-871000000 {
> > + opp-hz = /bits/ 64 <871000000>;
> > + opp-microvolt = <906250>;
> > + };
> > + opp2_10: opp-923000000 {
> > + opp-hz = /bits/ 64 <923000000>;
> > + opp-microvolt = <931250>;
> > + };
> > + opp2_11: opp-962000000 {
> > + opp-hz = /bits/ 64 <962000000>;
> > + opp-microvolt = <943750>;
> > + };
> > + opp2_12: opp-1027000000 {
> > + opp-hz = /bits/ 64 <1027000000>;
> > + opp-microvolt = <975000>;
> > + };
> > + opp2_13: opp-1092000000 {
> > + opp-hz = /bits/ 64 <1092000000>;
> > + opp-microvolt = <1000000>;
> > + };
> > + opp2_14: opp-1144000000 {
> > + opp-hz = /bits/ 64 <1144000000>;
> > + opp-microvolt = <1025000>;
> > + };
> > + opp2_15: opp-1196000000 {
> > + opp-hz = /bits/ 64 <1196000000>;
> > + opp-microvolt = <1050000>;
> > + };
> > + };
> > --
> > 2.18.0
> >
> >
> > _______________________________________________
> > Linux-mediatek mailing list
> > [email protected]
> >
https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-mediatek__;!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDvdNpOFZ$
> >

2022-05-09 12:47:14

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI dt-bindings

On 22. 5. 9. 21:09, Chanwoo Choi wrote:
> Hi,
>
> On 22. 4. 25. 21:55, Johnson Wang wrote:
>> Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
>>
>> Signed-off-by: Johnson Wang <[email protected]>
>> Signed-off-by: Jia-Wei Chang <[email protected]>
>> ---
>> .../bindings/interconnect/mediatek,cci.yaml | 139 ++++++++++++++++++
>> 1 file changed, 139 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
>>
>> diff --git
>> a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
>> b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
>> new file mode 100644
>> index 000000000000..e5221e17d11b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
>> @@ -0,0 +1,139 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/interconnect/mediatek,cci.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MediaTek Cache Coherent Interconnect (CCI) frequency and
>> voltage scaling
>> +
>> +maintainers:
>> + - Jia-Wei Chang <[email protected]>
>
> Why did you add your author information?

I'm sorry. I missed 'not' above sentence.
IMHO, please add your author information.

(snip)

--
Best Regards,
Samsung Electronics
Chanwoo Choi

2022-05-10 09:35:48

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI dt-bindings

On Tue, May 10, 2022 at 11:59 AM Johnson Wang <[email protected]> wrote:
>
> Hi Chanwoo,
>
> On Mon, 2022-05-09 at 21:09 +0900, Chanwoo Choi wrote:
> > Hi,
> >
> > On 22. 4. 25. 21:55, Johnson Wang wrote:
> > > Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
> > >
> > > Signed-off-by: Johnson Wang <[email protected]>
> > > Signed-off-by: Jia-Wei Chang <[email protected]>
> > > ---
> > > .../bindings/interconnect/mediatek,cci.yaml | 139
> > > ++++++++++++++++++
> > > 1 file changed, 139 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > > new file mode 100644
> > > index 000000000000..e5221e17d11b
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > > @@ -0,0 +1,139 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id:
> > > https://urldefense.com/v3/__http://devicetree.org/schemas/interconnect/mediatek,cci.yaml*__;Iw!!CTRNKA9wMg0ARbw!z6ogArqzuIzPR3TYO1aW-Z-scpuZJxIriWMofdfnvrKTXAYBBLZeitAPIKyZayMYZGsR$
> > >
> > > +$schema:
> > > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!z6ogArqzuIzPR3TYO1aW-Z-scpuZJxIriWMofdfnvrKTXAYBBLZeitAPIKyZa9f2pALd$
> > >
> > > +
> > > +title: MediaTek Cache Coherent Interconnect (CCI) frequency and
> > > voltage scaling
> > > +
> > > +maintainers:
> > > + - Jia-Wei Chang <[email protected]>
> >
> > Why did you add your author information?
> > Please add your author information.
>
> Sorry, I don't really understand what you mean.
> Could you please explain your advice again?
>
> The author of this driver is 'Jia-Wei Chang'.
> We have added author information to the driver code and this binding
> document as above.

Firstly, sorry for the confusion.

I have discussed this patch with you. I think that you (Johnson Wang)
should be added to 'maintainers' of this binding document as following:

+maintainers:
+ - Jia-Wei Chang <[email protected]>
+ - Johnson Wang <[email protected]>

>
> >
> > And add this dt-binding information to MAINTAINERS
> > as following: because I cannot catch the later patch
> > of modification.
> >
> > cwchoi00@chanwoo:~/kernel/linux.chanwoo$ d
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index edc96cdb85e8..a11e9c1947b7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5698,6 +5698,7 @@ L: [email protected]
> > S: Maintained
> > T: git
> > git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git
> > F: Documentation/devicetree/bindings/devfreq/
> > +F: Documentation/devicetree/bindings/interconnect/mediatek,cci.y
> > aml
> > F: drivers/devfreq/
> > F: include/linux/devfreq.h
> > F: include/trace/events/devfreq.h
> >
>
> I will add it in the next version.
>
> BRs,
> Johnson Wang
> >
> > > +
> > > +description: |
> > > + MediaTek Cache Coherent Interconnect (CCI) is a hardware engine
> > > used by
> > > + MT8183 and MT8186 SoCs to scale the frequency and adjust the
> > > voltage in
> > > + hardware. It can also optimize the voltage to reduce the power
> > > consumption.
> > > +
> > > +properties:
> > > + compatible:
> > > + enum:
> > > + - mediatek,mt8183-cci
> > > + - mediatek,mt8186-cci
> > > +
> > > + clocks:
> > > + items:
> > > + - description:
> > > + The multiplexer for clock input of CPU cluster.
> > > + - description:
> > > + A parent of "cpu" clock which 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: true
> > > + 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 it presents, 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/mt8183-clk.h>
> > > + cci: cci {
> > > + compatible = "mediatek,mt8183-cci";
> > > + clocks = <&mcucfg CLK_MCU_BUS_SEL>,
> > > + <&topckgen CLK_TOP_ARMPLL_DIV_PLL1>;
> > > + clock-names = "cci", "intermediate";
> > > + operating-points-v2 = <&cci_opp>;
> > > + proc-supply = <&mt6358_vproc12_reg>;
> > > + };
> > > +
> > > + cci_opp: opp-table-cci {
> > > + compatible = "operating-points-v2";
> > > + opp-shared;
> > > + opp2_00: opp-273000000 {
> > > + opp-hz = /bits/ 64 <273000000>;
> > > + opp-microvolt = <650000>;
> > > + };
> > > + opp2_01: opp-338000000 {
> > > + opp-hz = /bits/ 64 <338000000>;
> > > + opp-microvolt = <687500>;
> > > + };
> > > + opp2_02: opp-403000000 {
> > > + opp-hz = /bits/ 64 <403000000>;
> > > + opp-microvolt = <718750>;
> > > + };
> > > + opp2_03: opp-463000000 {
> > > + opp-hz = /bits/ 64 <463000000>;
> > > + opp-microvolt = <756250>;
> > > + };
> > > + opp2_04: opp-546000000 {
> > > + opp-hz = /bits/ 64 <546000000>;
> > > + opp-microvolt = <800000>;
> > > + };
> > > + opp2_05: opp-624000000 {
> > > + opp-hz = /bits/ 64 <624000000>;
> > > + opp-microvolt = <818750>;
> > > + };
> > > + opp2_06: opp-689000000 {
> > > + opp-hz = /bits/ 64 <689000000>;
> > > + opp-microvolt = <850000>;
> > > + };
> > > + opp2_07: opp-767000000 {
> > > + opp-hz = /bits/ 64 <767000000>;
> > > + opp-microvolt = <868750>;
> > > + };
> > > + opp2_08: opp-845000000 {
> > > + opp-hz = /bits/ 64 <845000000>;
> > > + opp-microvolt = <893750>;
> > > + };
> > > + opp2_09: opp-871000000 {
> > > + opp-hz = /bits/ 64 <871000000>;
> > > + opp-microvolt = <906250>;
> > > + };
> > > + opp2_10: opp-923000000 {
> > > + opp-hz = /bits/ 64 <923000000>;
> > > + opp-microvolt = <931250>;
> > > + };
> > > + opp2_11: opp-962000000 {
> > > + opp-hz = /bits/ 64 <962000000>;
> > > + opp-microvolt = <943750>;
> > > + };
> > > + opp2_12: opp-1027000000 {
> > > + opp-hz = /bits/ 64 <1027000000>;
> > > + opp-microvolt = <975000>;
> > > + };
> > > + opp2_13: opp-1092000000 {
> > > + opp-hz = /bits/ 64 <1092000000>;
> > > + opp-microvolt = <1000000>;
> > > + };
> > > + opp2_14: opp-1144000000 {
> > > + opp-hz = /bits/ 64 <1144000000>;
> > > + opp-microvolt = <1025000>;
> > > + };
> > > + opp2_15: opp-1196000000 {
> > > + opp-hz = /bits/ 64 <1196000000>;
> > > + opp-microvolt = <1050000>;
> > > + };
> > > + };
> >
> >
>

--
Best Regards,
Chanwoo Choi

2022-05-11 10:02:55

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

On Wed, May 11, 2022 at 2:14 PM Johnson Wang <[email protected]> wrote:
>
> On Mon, 2022-05-09 at 20:51 +0900, Chanwoo Choi wrote:
> > On 22. 5. 9. 14:57, Johnson Wang wrote:
> > > On Sat, 2022-05-07 at 22:53 +0900, Chanwoo Choi wrote:
> > > > On 22. 5. 6. 20:38, Johnson Wang wrote:
> > > > > On Mon, 2022-04-25 at 20:55 +0800, Johnson Wang wrote:
> > > > > > We introduce a devfreq driver for the MediaTek Cache Coherent
> > > > > > Interconnect
> > > > > > (CCI) used by some MediaTek SoCs.
> > > > > >
> > > > > > In this driver, we use the passive devfreq driver to get
> > > > > > target
> > > > > > frequencies
> > > > > > and adjust voltages accordingly. In MT8183 and MT8186, the
> > > > > > MediaTek
> > > > > > CCI
> > > > > > is supplied by the same regulators with the little core CPUs.
> > > > > >
> > > > > > Signed-off-by: Johnson Wang <[email protected]>
> > > > > > Signed-off-by: Jia-Wei Chang <[email protected]>
> > > > > > ---
> > > > > > This patch depends on "devfreq-testing"[1].
> > > > > > [1]
> > > > > >
> > >
> > >
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing__;!!CTRNKA9wMg0ARbw!wnTkI_sh3TsliBPFP70DFwtSZGtKyPinFu9h2BwULWX-xrWF0C21rLV-n1HhstfmLw42$
> > > > > >
> > > > > > ---
> > > > > > drivers/devfreq/Kconfig | 10 +
> > > > > > drivers/devfreq/Makefile | 1 +
> > > > > > drivers/devfreq/mtk-cci-devfreq.c | 474
> > > > > > ++++++++++++++++++++++++++++++
> > > > > > 3 files changed, 485 insertions(+)
> > > > > > create mode 100644 drivers/devfreq/mtk-cci-devfreq.c
> > > > > >
> > > > > > diff --git a/drivers/devfreq/Kconfig
> > > > > > b/drivers/devfreq/Kconfig
> > > > > > index 87eb2b837e68..9754d8b31621 100644
> > > > > > --- a/drivers/devfreq/Kconfig
> > > > > > +++ b/drivers/devfreq/Kconfig
> > > > > > @@ -120,6 +120,16 @@ 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 ARM_MEDIATEK_CPUFREQ || COMPILE_TEST
> > > > > > + select DEVFREQ_GOV_PASSIVE
> > > > > > + help
> > > > > > + This adds a devfreq driver for MediaTek Cache
> > > > > > Coherent
> > > > > > Interconnect
> > > > > > + which is shared the same regulators with the cpu
> > > > > > cluster. It
> > > > > > can track
> > > > > > + buck voltages and update a proper CCI frequency. Use
> > > > > > the
> > > > > > notification
> > > > > > + to get the 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 0b6be92a25d9..bf40d04928d0 100644
> > > > > > --- a/drivers/devfreq/Makefile
> > > > > > +++ b/drivers/devfreq/Makefile
> > > > > > @@ -11,6 +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_MEDIATEK_CCI_DEVFREQ) += mtk-cci-
> > > > > > devfreq.o
> > > > > > obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
> > > > > > obj-$(CONFIG_ARM_SUN8I_A33_MBUS_DEVFREQ) += sun8i-a33-
> > > > > > mbus.o
> > > > > > obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-
> > > > > > devfreq.o
> > > > > > diff --git a/drivers/devfreq/mtk-cci-devfreq.c
> > > > > > b/drivers/devfreq/mtk-
> > > > > > cci-devfreq.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..b3e31c45a57c
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/devfreq/mtk-cci-devfreq.c
> > > > > > @@ -0,0 +1,474 @@
> > > > > > +// 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 {
> > > > > > + int min_volt_shift;
> > > > > > + int max_volt_shift;
> > > > > > + int proc_max_volt;
> > > > > > + int sram_min_volt;
> > > > > > + int sram_max_volt;
> > > > > > +};
> > > > > > +
> > > > > > +struct mtk_ccifreq_drv {
> > > > > > + struct device *dev;
> > > > > > + struct devfreq *devfreq;
> > > > > > + struct regulator *proc_reg;
> > > > > > + struct regulator *sram_reg;
> > > > > > + struct clk *cci_clk;
> > > > > > + struct clk *inter_clk;
> > > > > > + int inter_voltage;
> > > > > > + int pre_voltage;
> > > > > > + unsigned long pre_freq;
> > > > > > + /* Avoid race condition for regulators between notify
> > > > > > and
> > > > > > policy */
> > > > > > + struct mutex reg_lock;
> > > > > > + struct notifier_block opp_nb;
> > > > > > + const struct mtk_ccifreq_platform_data *soc_data;
> > > > > > + int vtrack_max;
> > > > > > +};
> > > > > > +
> > > > > > +static int mtk_ccifreq_set_voltage(struct mtk_ccifreq_drv
> > > > > > *drv,
> > > > > > int
> > > > > > new_voltage)
> > > > > > +{
> > > > > > + const struct mtk_ccifreq_platform_data *soc_data = drv-
> > > > > > > soc_data;
> > > > > >
> > > > > > + struct device *dev = drv->dev;
> > > > > > + int pre_voltage, pre_vsram, new_vsram, vsram, voltage,
> > > > > > ret;
> > > > > > + int retry_max = drv->vtrack_max;
> > > > > > +
> > > > > > + if (!drv->sram_reg) {
> > > > > > + ret = regulator_set_voltage(drv->proc_reg,
> > > > > > new_voltage,
> > > > > > + drv->soc_data-
> > > > > > > proc_max_volt);
> > > > > >
> > > > > > + goto out_set_voltage;
> > > > > > + }
> > > > > > +
> > > > > > + pre_voltage = regulator_get_voltage(drv->proc_reg);
> > > > > > + if (pre_voltage < 0) {
> > > > > > + dev_err(dev, "invalid vproc value: %d\n",
> > > > > > pre_voltage);
> > > > > > + return pre_voltage;
> > > > > > + }
> > > > > > +
> > > > > > + pre_vsram = regulator_get_voltage(drv->sram_reg);
> > > > > > + if (pre_vsram < 0) {
> > > > > > + dev_err(dev, "invalid vsram value: %d\n",
> > > > > > pre_vsram);
> > > > > > + return pre_vsram;
> > > > > > + }
> > > > > > +
> > > > > > + new_vsram = clamp(new_voltage + soc_data-
> > > > > > >min_volt_shift,
> > > > > > + soc_data->sram_min_volt, soc_data-
> > > > > > > sram_max_volt);
> > > > > >
> > > > > > +
> > > > > > + do {
> > > > > > + if (pre_voltage <= new_voltage) {
> > > > > > + vsram = clamp(pre_voltage + soc_data-
> > > > > > > max_volt_shift,
> > > > > >
> > > > > > + soc_data->sram_min_volt,
> > > > > > new_vsram);
> > > > > > + ret = regulator_set_voltage(drv-
> > > > > > >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(drv-
> > > > > > >proc_reg,
> > > > > > voltage,
> > > > > > + soc_data-
> > > > > > > proc_max_volt);
> > > > > >
> > > > > > + if (ret) {
> > > > > > + regulator_set_voltage(drv-
> > > > > > >sram_reg,
> > > > > > pre_vsram,
> > > > > > + soc_data-
> > > > > > > sram_max_volt);
> > > > > >
> > > > > > + return ret;
> > > > > > + }
> > > > > > + } else if (pre_voltage > new_voltage) {
> > > > > > + voltage = max(new_voltage,
> > > > > > + pre_vsram - soc_data-
> > > > > > > max_volt_shift);
> > > > > >
> > > > > > + ret = regulator_set_voltage(drv-
> > > > > > >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(drv-
> > > > > > >sram_reg,
> > > > > > vsram,
> > > > > > + soc_data-
> > > > > > > sram_max_volt);
> > > > > >
> > > > > > + if (ret) {
> > > > > > + regulator_set_voltage(drv-
> > > > > > >proc_reg,
> > > > > > pre_voltage,
> > > > > > + soc_data-
> > > > > > > proc_max_volt);
> > > > > >
> > > > > > + return ret;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + pre_voltage = voltage;
> > > > > > + pre_vsram = vsram;
> > > > > > +
> > > > > > + if (--retry_max < 0) {
> > > > > > + dev_err(dev,
> > > > > > + "over loop count, failed to set
> > > > > > voltage\n");
> > > > > > + return -EINVAL;
> > > > > > + }
> > > > > > + } while (voltage != new_voltage || vsram != new_vsram);
> > > > > > +
> > > > > > +out_set_voltage:
> > > > > > + if (!ret)
> > > > > > + drv->pre_voltage = new_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, pre_voltage, inter_voltage,
> > > > > > target_voltage, ret;
> > > > > > +
> > > > > > + if (!drv)
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > + if (drv->pre_freq == *freq)
> > > > > > + return 0;
> > > > > > +
> > > > > > + inter_voltage = drv->inter_voltage;
> > > > > > +
> > > > > > + opp_rate = *freq;
> > > > > > + opp = devfreq_recommended_opp(dev, &opp_rate, 1);
> > > > > > + if (IS_ERR(opp)) {
> > > > > > + dev_err(dev, "failed to find opp for freq:
> > > > > > %ld\n",
> > > > > > opp_rate);
> > > > > > + return PTR_ERR(opp);
> > > > > > + }
> > > > > > +
> > > > > > + mutex_lock(&drv->reg_lock);
> > > > > > +
> > > > > > + voltage = dev_pm_opp_get_voltage(opp);
> > > > > > + dev_pm_opp_put(opp);
> > > > > > +
> > > > > > + if (unlikely(drv->pre_voltage <= 0))
> > > > > > + pre_voltage = regulator_get_voltage(drv-
> > > > > > >proc_reg);
> > > > > > + else
> > > > > > + pre_voltage = drv->pre_voltage;
> > > > > > +
> > > > > > + if (pre_voltage < 0) {
> > > > > > + dev_err(dev, "invalid vproc value: %d\n",
> > > > > > pre_voltage);
> > > > > > + return pre_voltage;
> > > > > > + }
> > > > > > +
> > > > > > + /* scale up: set voltage first then freq. */
> > > > > > + target_voltage = max(inter_voltage, voltage);
> > > > > > + if (pre_voltage <= target_voltage) {
> > > > > > + ret = mtk_ccifreq_set_voltage(drv,
> > > > > > target_voltage);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "failed to scale up
> > > > > > voltage\n");
> > > > > > + goto out_restore_voltage;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + /* switch the cci clock to intermediate clock source.
> > > > > > */
> > > > > > + ret = clk_set_parent(drv->cci_clk, drv->inter_clk);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "failed to re-parent cci
> > > > > > clock\n");
> > > > > > + goto out_restore_voltage;
> > > > > > + }
> > > > > > +
> > > > > > + /* set the original clock to target rate. */
> > > > > > + ret = clk_set_rate(cci_pll, *freq);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "failed to set cci pll rate:
> > > > > > %d\n", ret);
> > > > > > + clk_set_parent(drv->cci_clk, cci_pll);
> > > > > > + goto out_restore_voltage;
> > > > > > + }
> > > > > > +
> > > > > > + /* switch the cci clock back to the original clock
> > > > > > source. */
> > > > > > + ret = clk_set_parent(drv->cci_clk, cci_pll);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "failed to re-parent cci
> > > > > > clock\n");
> > > > > > + mtk_ccifreq_set_voltage(drv, inter_voltage);
> > > > > > + goto out_unlock;
> > > > > > + }
> > > > > > +
> > > > > > + /*
> > > > > > + * 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 < pre_voltage) {
> > > > > > + ret = mtk_ccifreq_set_voltage(drv, voltage);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "failed to scale down
> > > > > > voltage\n");
> > > > > > + goto out_unlock;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + drv->pre_freq = *freq;
> > > > > > + mutex_unlock(&drv->reg_lock);
> > > > > > +
> > > > > > + return 0;
> > > > > > +
> > > > > > +out_restore_voltage:
> > > > > > + mtk_ccifreq_set_voltage(drv, pre_voltage);
> > > > > > +
> > > > > > +out_unlock:
> > > > > > + mutex_unlock(&drv->reg_lock);
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +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->reg_lock);
> > > > > > + /* current opp item is changed */
> > > > > > + if (freq == drv->pre_freq) {
> > > > > > + volt = dev_pm_opp_get_voltage(opp);
> > > > > > + mtk_ccifreq_set_voltage(drv, volt);
> > > > > > + }
> > > > > > + mutex_unlock(&drv->reg_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 *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(dev, sizeof(*drv), GFP_KERNEL);
> > > > > > + if (!drv)
> > > > > > + return -ENOMEM;
> > > > > > +
> > > > > > + drv->dev = dev;
> > > > > > + drv->soc_data = (const struct mtk_ccifreq_platform_data
> > > > > > *)
> > > > > > + of_device_get_match_data(&pdev-
> > > > > > >dev);
> > > > > > + mutex_init(&drv->reg_lock);
> > > > > > + platform_set_drvdata(pdev, drv);
> > > > > > +
> > > > > > + drv->cci_clk = devm_clk_get(dev, "cci");
> > > > > > + if (IS_ERR(drv->cci_clk)) {
> > > > > > + ret = PTR_ERR(drv->cci_clk);
> > > > > > + return dev_err_probe(dev, ret,
> > > > > > + "failed to get cci clk:
> > > > > > %d\n",
> > > > > > ret);
> > > > > > + }
> > > > > > +
> > > > > > + drv->inter_clk = devm_clk_get(dev, "intermediate");
> > > > > > + if (IS_ERR(drv->inter_clk)) {
> > > > > > + ret = PTR_ERR(drv->inter_clk);
> > > > > > + dev_err_probe(dev, ret,
> > > > > > + "failed to get intermediate clk:
> > > > > > %d\n",
> > > > > > ret);
> > > > > > + goto out_free_resources;
> > > > > > + }
> > > > > > +
> > > > > > + drv->proc_reg = devm_regulator_get_optional(dev,
> > > > > > "proc");
> > > > > > + if (IS_ERR(drv->proc_reg)) {
> > > > > > + ret = PTR_ERR(drv->proc_reg);
> > > > > > + dev_err_probe(dev, ret,
> > > > > > + "failed to get proc regulator:
> > > > > > %d\n",
> > > > > > ret);
> > > > > > + goto out_free_resources;
> > > > > > + }
> > > > > > +
> > > > > > + ret = regulator_enable(drv->proc_reg);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "failed to enable proc
> > > > > > regulator\n");
> > > > > > + goto out_free_resources;
> > > > > > + }
> > > > > > +
> > > > > > + drv->sram_reg = regulator_get_optional(dev, "sram");
> > > > > > + if (IS_ERR(drv->sram_reg))
> > > > > > + drv->sram_reg = NULL;
> > > > > > + else {
> > > > > > + ret = regulator_enable(drv->sram_reg);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "failed to enable sram
> > > > > > regulator\n");
> > > > > > + goto out_free_resources;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + /*
> > > > > > + * We assume min voltage is 0 and tracking target
> > > > > > voltage using
> > > > > > + * min_volt_shift for each iteration.
> > > > > > + * The retry_max is 3 times of expeted iteration count.
> > > > > > + */
> > > > > > + drv->vtrack_max = 3 * DIV_ROUND_UP(max(drv->soc_data-
> > > > > > > sram_max_volt,
> > > > > >
> > > > > > + drv->soc_data-
> > > > > > > proc_max_volt),
> > > > > >
> > > > > > + drv->soc_data-
> > > > > > > min_volt_shift);
> > > > > >
> > > > > > +
> > > > > > + ret = clk_prepare_enable(drv->cci_clk);
> > > > > > + if (ret)
> > > > > > + goto out_free_resources;
> > > > > > +
> > > > > > + ret = clk_prepare_enable(drv->inter_clk);
> > > > > > + if (ret)
> > > > > > + goto out_disable_cci_clk;
> > > > > > +
> > > > > > + ret = dev_pm_opp_of_add_table(dev);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "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(dev, &rate);
> > > > > > + if (IS_ERR(opp)) {
> > > > > > + ret = PTR_ERR(opp);
> > > > > > + dev_err(dev, "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->dev, &rate);
> > > > > > + if (IS_ERR(opp)) {
> > > > > > + dev_err(dev, "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) {
> > > > > > + dev_err(dev, "failed to scale to highest
> > > > > > voltage %lu in
> > > > > > proc_reg\n",
> > > > > > + opp_volt);
> > > > > > + goto out_remove_opp_table;
> > > > > > + }
> > > > > > +
> > > > > > + passive_data = devm_kzalloc(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(dev,
> > > > > > &mtk_ccifreq_profile,
> > > > > > + DEVFREQ_GOV_PASS
> > > > > > IVE,
> > > > > > + passive_data);
> > > > > > + if (IS_ERR(drv->devfreq)) {
> > > > > > + ret = -EPROBE_DEFER;
> > > > > > + dev_err(dev, "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(dev, &drv->opp_nb);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "failed to register opp notifier:
> > > > > > %d\n",
> > > > > > ret);
> > > > > > + goto out_remove_devfreq_device;
> > > > > > + }
> > > > > > + return 0;
> > > > > > +
> > > > > > +out_remove_devfreq_device:
> > > > > > + devm_devfreq_remove_device(dev, drv->devfreq);
> > > > > > +
> > > > > > +out_remove_opp_table:
> > > > > > + dev_pm_opp_of_remove_table(dev);
> > > > > > +
> > > > > > +out_disable_inter_clk:
> > > > > > + clk_disable_unprepare(drv->inter_clk);
> > > > > > +
> > > > > > +out_disable_cci_clk:
> > > > > > + clk_disable_unprepare(drv->cci_clk);
> > > > > > +
> > > > > > +out_free_resources:
> > > > > > + if (regulator_is_enabled(drv->proc_reg))
> > > > > > + regulator_disable(drv->proc_reg);
> > > > > > + if (drv->sram_reg && regulator_is_enabled(drv-
> > > > > > >sram_reg))
> > > > > > + regulator_disable(drv->sram_reg);
> > > > > > +
> > > > > > + if (!IS_ERR(drv->proc_reg))
> > > > > > + regulator_put(drv->proc_reg);
> > > > > > + if (!IS_ERR(drv->sram_reg))
> > > > > > + regulator_put(drv->sram_reg);
> > > > > > + if (!IS_ERR(drv->cci_clk))
> > > > > > + clk_put(drv->cci_clk);
> > > > > > + if (!IS_ERR(drv->inter_clk))
> > > > > > + clk_put(drv->inter_clk);
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static int mtk_ccifreq_remove(struct platform_device *pdev)
> > > > > > +{
> > > > > > + struct device *dev = &pdev->dev;
> > > > > > + struct mtk_ccifreq_drv *drv;
> > > > > > +
> > > > > > + drv = platform_get_drvdata(pdev);
> > > > > > +
> > > > > > + dev_pm_opp_unregister_notifier(dev, &drv->opp_nb);
> > > > > > + dev_pm_opp_of_remove_table(dev);
> > > > > > + clk_disable_unprepare(drv->inter_clk);
> > > > > > + clk_disable_unprepare(drv->cci_clk);
> > > > > > + regulator_disable(drv->proc_reg);
> > > > > > + if (drv->sram_reg)
> > > > > > + regulator_disable(drv->sram_reg);
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct mtk_ccifreq_platform_data
> > > > > > mt8183_platform_data =
> > > > > > {
> > > > > > + .min_volt_shift = 100000,
> > > > > > + .max_volt_shift = 200000,
> > > > > > + .proc_max_volt = 1150000,
> > > > > > + .sram_min_volt = 0,
> > > > > > + .sram_max_volt = 1150000,
> > > > > > +};
> > > > > > +
> > > > > > +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,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct of_device_id mtk_ccifreq_machines[] = {
> > > > > > + { .compatible = "mediatek,mt8183-cci", .data =
> > > > > > &mt8183_platform_data },
> > > > > > + { .compatible = "mediatek,mt8186-cci", .data =
> > > > > > &mt8186_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 = mtk_ccifreq_machines,
> > > > > > + },
> > > > > > +};
> > > > > > +module_platform_driver(mtk_ccifreq_platdrv);
> > > > > > +
> > > > > > +MODULE_DESCRIPTION("MediaTek CCI devfreq driver");
> > > > > > +MODULE_AUTHOR("Jia-Wei Chang <[email protected]>");
> > > > > > +MODULE_LICENSE("GPL v2");
> > > > >
> > > > > Hi Chanwoo,
> > > > >
> > > > > Just a kindly ping.
> > > > > Could you please give me some suggestion on this patch?
> > > > > Thanks!
> > > > >
> > > > > BRs,
> > > > > Johnson Wang
> > > > >
> > > >
> > > > Hi Johnson,
> > > >
> > > > Sorry for late reply.But I replied it yesterday as following:
> > > > Maybe, this reply[1] has not yet arrrived at your mail box.
> > > > [1]
> > > >
> > >
> > >
> https://lore.kernel.org/lkml/[email protected]/T/#m3bc609d805d6e74ec7a105be0511926b4afbbaef
> > > >
> > > > As I described on reply[1], I updated the patches on devfreq-
> > > > testing
> > > > branch. Could you please test your patches based on devfreq-
> > > > testing
> > > > branch?
> > > >
> > >
> > > Hi Chanwoo,
> > >
> > > Thanks for your information.
> > > I've tested this patch based on the latest devfreq-testing branch.
> > > It encounters the same crash as Chen-Yu mentioned[1].
> > >
> > > [1]
> > > https://lore.kernel.org/linux-pm/[email protected]/T/#u
> >
> > Hi Johnson,
> >
> > Thanks for the test. I'll drop the last patch caused of crash.
> > And I'll send v3 patchset right now.
> >
> >
>
> Hi Chanwoo,
>
> With your v3 patchset, this CCI devfreq driver works properly on the
> target.
> Thanks!

Hi Johnson,

Thanks for the test.
I'll send v4 with a Tested-by tag and some typo fixup And then I'll merge them.
As I know, you will send the next version of mediatek cci driver.
After that, I'll merge your patches. Thanks.

--
Best Regards,
Chanwoo Choi

2022-05-12 05:27:59

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI dt-bindings

On Mon, May 9, 2022 at 8:14 PM Johnson Wang <[email protected]> wrote:
>
> Hi Chen-Yu,
>
> On Tue, 2022-04-26 at 11:18 +0800, Chen-Yu Tsai wrote:
> > On Mon, Apr 25, 2022 at 8:56 PM Johnson Wang <
> > [email protected]> wrote:
> > >
> > > Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
> > >
> > > Signed-off-by: Johnson Wang <[email protected]>
> > > Signed-off-by: Jia-Wei Chang <[email protected]>
> > > ---
> > > .../bindings/interconnect/mediatek,cci.yaml | 139
> > > ++++++++++++++++++
> > > 1 file changed, 139 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > > new file mode 100644
> > > index 000000000000..e5221e17d11b
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml
> > > @@ -0,0 +1,139 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id:
> > > https://urldefense.com/v3/__http://devicetree.org/schemas/interconnect/mediatek,cci.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDlQ9pSDO$
> > >
> > > +$schema:
> > > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDoE9YHyu$
> > >
> > > +
> > > +title: MediaTek Cache Coherent Interconnect (CCI) frequency and
> > > voltage scaling
> > > +
> > > +maintainers:
> > > + - Jia-Wei Chang <[email protected]>
> > > +
> > > +description: |
> > > + MediaTek Cache Coherent Interconnect (CCI) is a hardware engine
> > > used by
> > > + MT8183 and MT8186 SoCs to scale the frequency and adjust the
> > > voltage in
> > > + hardware. It can also optimize the voltage to reduce the power
> > > consumption.
> > > +
> > > +properties:
> > > + compatible:
> > > + enum:
> > > + - mediatek,mt8183-cci
> > > + - mediatek,mt8186-cci
> > > +
> > > + clocks:
> > > + items:
> > > + - description:
> > > + The multiplexer for clock input of CPU cluster.
> >
> > of the bus, not CPU cluster.
>
> Thanks for your suggestion.
> I will correct it in the next version.
>
> >
> > > + - description:
> > > + A parent of "cpu" clock which is used as an intermediate
> > > clock source
> > > + when the original CPU is under transition and not stable
> > > yet.
> >
> > This really should be handled in the clk controller, and not by every
> > device
> > that happens to take a clock from a mux with upstream PLLs that can
> > change
> > in clock rate. The end device hardware only takes one clock input.
> > That's it.
> >
>
> To make this intermediate clock works properly, this driver is also
> responsible for handling the Vproc voltage and ensures the voltage is
> high enough to support intermediate clock rate.
>
> If we move intermediate clock rate control to clock driver, then
> intermediate voltage control may be handled by the clock driver itself
> as well.
>
> We believe that is not reasonable because clock driver shouldn't handle
> voltage control. On the other hand, DVFS driver is more suitable for
> doing this job.

Either way the DVFS driver handles the voltage change.

Right now the driver is doing:

1. Raise voltage if scaling up
2. Mux CCI clock over to stable clock
3. Set rate for CCI PLL
4. Mux CCI clock back to CCI PLL
5. Drop voltage if scaling down

I'm saying that the clock driver should handle 2+4 transparently when any
driver requests a rate change on the CCI clock. So instead the driver would
do:

1. Raise voltage if scaling up
2. Set rate for CCI _clock_
Here the clock driver would do:
a. Mux CCI clock over to stable clock
b. Change clock rate for original parent, i.e. the CCI PLL
c. Mux CCI clock back to original parent, i.e. the CCI PLL
and back to the devfreq driver ...
3. Drop voltage if scaling down

Does that make sense?

Regards
ChenYu

2022-05-14 01:19:05

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: interconnect: Add MediaTek CCI dt-bindings

On Wed, 2022-05-11 at 18:48 +0800, Chen-Yu Tsai wrote:
> On Mon, May 9, 2022 at 8:14 PM Johnson Wang <
> [email protected]> wrote:
> >
> > Hi Chen-Yu,
> >
> > On Tue, 2022-04-26 at 11:18 +0800, Chen-Yu Tsai wrote:
> > > On Mon, Apr 25, 2022 at 8:56 PM Johnson Wang <
> > > [email protected]> wrote:
> > > >
> > > > Add devicetree binding of MediaTek CCI on MT8183 and MT8186.
> > > >
> > > > Signed-off-by: Johnson Wang <[email protected]>
> > > > Signed-off-by: Jia-Wei Chang <[email protected]>
> > > > ---
> > > > .../bindings/interconnect/mediatek,cci.yaml | 139
> > > > ++++++++++++++++++
> > > > 1 file changed, 139 insertions(+)
> > > > create mode 100644
> > > > Documentation/devicetree/bindings/interconnect/mediatek,cci.yam
> > > > l
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/interconnect/mediatek,cci.y
> > > > aml
> > > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.y
> > > > aml
> > > > new file mode 100644
> > > > index 000000000000..e5221e17d11b
> > > > --- /dev/null
> > > > +++
> > > > b/Documentation/devicetree/bindings/interconnect/mediatek,cci.y
> > > > aml
> > > > @@ -0,0 +1,139 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id:
> > > >
https://urldefense.com/v3/__http://devicetree.org/schemas/interconnect/mediatek,cci.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDlQ9pSDO$
> > > >
> > > > +$schema:
> > > >
https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!zuufEcqpKbditY3eqLTHpL8P8humMCyh4D4QWsximmw124tJUPE3ZBUyBqBtDoE9YHyu$
> > > >
> > > > +
> > > > +title: MediaTek Cache Coherent Interconnect (CCI) frequency
> > > > and
> > > > voltage scaling
> > > > +
> > > > +maintainers:
> > > > + - Jia-Wei Chang <[email protected]>
> > > > +
> > > > +description: |
> > > > + MediaTek Cache Coherent Interconnect (CCI) is a hardware
> > > > engine
> > > > used by
> > > > + MT8183 and MT8186 SoCs to scale the frequency and adjust the
> > > > voltage in
> > > > + hardware. It can also optimize the voltage to reduce the
> > > > power
> > > > consumption.
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > + enum:
> > > > + - mediatek,mt8183-cci
> > > > + - mediatek,mt8186-cci
> > > > +
> > > > + clocks:
> > > > + items:
> > > > + - description:
> > > > + The multiplexer for clock input of CPU cluster.
> > >
> > > of the bus, not CPU cluster.
> >
> > Thanks for your suggestion.
> > I will correct it in the next version.
> >
> > >
> > > > + - description:
> > > > + A parent of "cpu" clock which is used as an
> > > > intermediate
> > > > clock source
> > > > + when the original CPU is under transition and not
> > > > stable
> > > > yet.
> > >
> > > This really should be handled in the clk controller, and not by
> > > every
> > > device
> > > that happens to take a clock from a mux with upstream PLLs that
> > > can
> > > change
> > > in clock rate. The end device hardware only takes one clock
> > > input.
> > > That's it.
> > >
> >
> > To make this intermediate clock works properly, this driver is also
> > responsible for handling the Vproc voltage and ensures the voltage
> > is
> > high enough to support intermediate clock rate.
> >
> > If we move intermediate clock rate control to clock driver, then
> > intermediate voltage control may be handled by the clock driver
> > itself
> > as well.
> >
> > We believe that is not reasonable because clock driver shouldn't
> > handle
> > voltage control. On the other hand, DVFS driver is more suitable
> > for
> > doing this job.
>
> Either way the DVFS driver handles the voltage change.
>
> Right now the driver is doing:
>
> 1. Raise voltage if scaling up
> 2. Mux CCI clock over to stable clock
> 3. Set rate for CCI PLL
> 4. Mux CCI clock back to CCI PLL
> 5. Drop voltage if scaling down
>
> I'm saying that the clock driver should handle 2+4 transparently when
> any
> driver requests a rate change on the CCI clock. So instead the driver
> would
> do:
>
> 1. Raise voltage if scaling up
> 2. Set rate for CCI _clock_
> Here the clock driver would do:
> a. Mux CCI clock over to stable clock
> b. Change clock rate for original parent, i.e. the CCI PLL
> c. Mux CCI clock back to original parent, i.e. the CCI PLL
> and back to the devfreq driver ...
> 3. Drop voltage if scaling down
>
> Does that make sense?
>
>
> Regards
> ChenYu

Hi Chen-Yu,

Before we mux the CCI clock to an intermediate clock(MAINPLL), we must
ensure that regulator voltage is high enough (we call it intermediate
voltage) to support the intermediate clock rate.

Based on this concept, if we move mux control to clock driver, there
will be a dilemma about which driver to adjust the voltage.

1)When DVFS calls clk_set_rate(), clock driver scales up the regulator
voltage to higher than intermediate voltage and then mux the CCI clock.

This option is not reasonable because clock driver shouldn't handle the
regulators.

2)DVFS scales up the regulator voltage, then calls clk_set_rate().
Clock driver mux the CCI clock to the intermediate clock.

This option isn't straightforward and makes one confused easily. For a
person who reads this driver, he may not understand why we adjust the
voltage before clk_set_rate().

That's why we put intermediate voltage/freq together in the DVFS.

BRs,
Johnson Wang

2022-05-14 01:36:06