Re: [PATCH v6 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver
From: AngeloGioacchino Del Regno
Date: Mon May 30 2022 - 08:16:41 EST
Il 27/05/22 13:00, Johnson Wang ha scritto:
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: Jia-Wei Chang <jia-wei.chang@xxxxxxxxxxxx>
Signed-off-by: Johnson Wang <johnson.wang@xxxxxxxxxxxx>
Acked-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
---
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 | 441 ++++++++++++++++++++++++++++++
3 files changed, 452 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..df42da35b312
--- /dev/null
+++ b/drivers/devfreq/mtk-cci-devfreq.c
@@ -0,0 +1,441 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 MediaTek Inc.
+ */
+
..snip..
+};
+
+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);
+ return dev_err_probe(dev, ret,
+ "failed to get intermediate clk: %d\n", ret);
+ }
+
+ drv->proc_reg = devm_regulator_get_optional(dev, "proc");
In the devicetree binding for this driver, the "proc" regulator is *not* optional,
but here you're using devm_regulator_get_optional.
If this is not optional, you should use devm_regulator_get() instead.
+ if (IS_ERR(drv->proc_reg)) {
+ ret = PTR_ERR(drv->proc_reg);
+ return dev_err_probe(dev, ret,
+ "failed to get proc regulator: %d\n", ret);
There's no need to print ret... dev_err_probe() takes care of that for you already:
in this case, you're printing the value of ret twice.
+ }
+
+ ret = regulator_enable(drv->proc_reg);
If you move this call after the devm_regulator_get_optional() call for the sram
vreg, you will be able to use dev_err_probe for the latter as well.
+ if (ret) {
+ dev_err(dev, "failed to enable proc regulator\n");
Why aren't you using dev_err_probe here, like you've done for the other instances?
+ return ret;
+ }
+
+ drv->sram_reg = devm_regulator_get_optional(dev, "sram");
+ if (IS_ERR(drv->sram_reg))
+ drv->sram_reg = NULL;
When you use regulator_get_optional() (including the devm_ variant of it), you
shall return an error, if there's any... that's what the _optional() is for.
if (IS_ERR(drv->sram_reg))
return dev_err_probe(dev, PTR_ERR(drv->proc_reg),
"failed to get sram regulator");
+ else {
+ ret = regulator_enable(drv->sram_reg);
+ if (ret) {
+ dev_err(dev, "failed to enable sram regulator\n");
+ goto out_free_resources;
+ }
+ }
+
Regards,
Angelo