| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d387e02a84d05650990f4efdb4d7ece297ca101d.camel@mediatek.com>
Date: Thu, 14 Apr 2022 13:19:25 +0800
From: Johnson Wang <johnson.wang@...iatek.com>
To: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>, <cw00.choi@...sung.com>,
<krzk+dt@...nel.org>, <robh+dt@...nel.org>,
<kyungmin.park@...sung.com>
CC: <khilman@...nel.org>, <linux-pm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-mediatek@...ts.infradead.org>, <jia-wei.chang@...iatek.com>,
<Project_Global_Chrome_Upstream_Group@...iatek.com>
Subject: Re: [PATCH v2 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI
devfreq driver
On Fri, 2022-04-08 at 13:51 +0200, AngeloGioacchino Del Regno wrote:
> 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@...iatek.com>
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@...iatek.com>
> > ---
> > 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!zNCF7FI5uFmzlA1V1-Pp8ht2HtUk8_oRRDoqzzBcXm0Mo8JOOoVbPPqa5xg4WuYPnKNF$
> >
> > ---
> > 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
Hello Angelo,
Thanks for your suggestion!
Actually, we are going to add an iteration limit inside the while loop.
BRs,
Johnson Wang
Powered by blists - more mailing lists