[PATCH v2 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver
AngeloGioacchino Del Regno
angelogioacchino.delregno at collabora.com
Fri Apr 8 04:51:43 PDT 2022
Il 08/04/22 07:21, 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: Johnson Wang <johnson.wang at mediatek.com>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang at mediatek.com>
> ---
> 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 | 479 ++++++++++++++++++++++++++++++
> 3 files changed, 490 insertions(+)
> create mode 100644 drivers/devfreq/mtk-cci-devfreq.c
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 87eb2b837e68..d985597f343f 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
> + 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..53a28e2c88bd
> --- /dev/null
> +++ b/drivers/devfreq/mtk-cci-devfreq.c
> @@ -0,0 +1,479 @@
> +// 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 *cci_dev;
> + struct devfreq *devfreq;
> + struct regulator *proc_reg;
> + struct regulator *sram_reg;
> + struct clk *cci_clk;
> + struct clk *inter_clk;
> + int inter_voltage;
> + int old_voltage;
> + unsigned long old_freq;
> + bool need_voltage_tracking;
> + /* 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;
> +};
> +
> +static int mtk_ccifreq_voltage_tracking(struct mtk_ccifreq_drv *drv,
> + int new_voltage)
> +{
> + const struct mtk_ccifreq_platform_data *soc_data = drv->soc_data;
> + struct device *dev = drv->cci_dev;
> + struct regulator *proc_reg = drv->proc_reg;
> + struct regulator *sram_reg = drv->sram_reg;
> + int old_voltage, old_vsram, new_vsram, vsram, voltage, ret;
> +
> + old_voltage = regulator_get_voltage(proc_reg);
> + if (old_voltage < 0) {
> + dev_err(dev, "invalid vproc value: %d\n", old_voltage);
> + return old_voltage;
> + }
> +
> + old_vsram = regulator_get_voltage(sram_reg);
> + if (old_vsram < 0) {
> + dev_err(dev, "invalid vsram value: %d\n", old_vsram);
> + return old_vsram;
> + }
> +
> + new_vsram = clamp(new_voltage + soc_data->min_volt_shift,
> + soc_data->sram_min_volt, soc_data->sram_max_volt);
> +
> + do {
> + if (old_voltage <= new_voltage) {
> + vsram = clamp(old_voltage + soc_data->max_volt_shift,
> + soc_data->sram_min_volt, new_vsram);
> + ret = regulator_set_voltage(sram_reg, vsram,
> + soc_data->sram_max_volt);
> + if (ret)
> + return ret;
> +
> + if (vsram == soc_data->sram_max_volt ||
> + new_vsram == soc_data->sram_min_volt)
> + voltage = new_voltage;
> + else
> + voltage = vsram - soc_data->min_volt_shift;
> +
> + ret = regulator_set_voltage(proc_reg, voltage,
> + soc_data->proc_max_volt);
> + if (ret) {
> + regulator_set_voltage(sram_reg, old_vsram,
> + soc_data->sram_max_volt);
> + return ret;
> + }
> + } else if (old_voltage > new_voltage) {
> + voltage = max(new_voltage,
> + old_vsram - soc_data->max_volt_shift);
> + ret = regulator_set_voltage(proc_reg, voltage,
> + soc_data->proc_max_volt);
> + if (ret)
> + return ret;
> +
> + if (voltage == new_voltage)
> + vsram = new_vsram;
> + else
> + vsram = max(new_vsram,
> + voltage + soc_data->min_volt_shift);
> +
> + ret = regulator_set_voltage(sram_reg, vsram,
> + soc_data->sram_max_volt);
> + if (ret) {
> + regulator_set_voltage(proc_reg, old_voltage,
> + soc_data->proc_max_volt);
> + return ret;
> + }
> + }
> +
> + old_voltage = voltage;
> + old_vsram = vsram;
> + } while (voltage != new_voltage || vsram != new_vsram);
Hello Johnson,
are you extremely sure that there will *always* be a way out of this while loop?
For safety purposes, I would set an iteration limit in order to avoid getting
an infinite loop here.
Probably, something like twice or thrice the expected number of iterations will
also be fine.
P.S.: Krzysztof's review also contains exactly all the rest of what I would
also say here (thanks!).
Regards,
Angelo
More information about the linux-arm-kernel
mailing list