Messages in this thread | | | Date | Mon, 30 May 2022 14:16:31 +0200 | Subject | Re: [PATCH v6 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver | From | AngeloGioacchino Del Regno <> |
| |
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@mediatek.com> > Signed-off-by: Johnson Wang <johnson.wang@mediatek.com> > Acked-by: Chanwoo Choi <cw00.choi@samsung.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 | 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
| |