| Message ID | 20200520034307.20435-10-andrew-sh.cheng@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add cpufreq and cci devfreq for mt8183, and SVS support | expand |
On Wed, May 20, 2020 at 11:43:04AM +0800, Andrew-sh.Cheng wrote: > + cci_df->proc_reg = devm_regulator_get_optional(cci_dev, "proc"); > + ret = PTR_ERR_OR_ZERO(cci_df->proc_reg); > + if (ret) { > + if (ret != -EPROBE_DEFER) > + dev_err(cci_dev, "failed to get regulator for CCI: %d\n", > + ret); > + return ret; > + } > + ret = regulator_enable(cci_df->proc_reg); The code appears to require a regulator (and I'm guessing the device needs power) so why is this using regulator_get_optional()?
On Wed, 2020-05-20 at 13:31 +0100, Mark Brown wrote: > On Wed, May 20, 2020 at 11:43:04AM +0800, Andrew-sh.Cheng wrote: > > > + cci_df->proc_reg = devm_regulator_get_optional(cci_dev, "proc"); > > + ret = PTR_ERR_OR_ZERO(cci_df->proc_reg); > > + if (ret) { > > + if (ret != -EPROBE_DEFER) > > + dev_err(cci_dev, "failed to get regulator for CCI: %d\n", > > + ret); > > + return ret; > > + } > > + ret = regulator_enable(cci_df->proc_reg); > > The code appears to require a regulator (and I'm guessing the device > needs power) so why is this using regulator_get_optional()? Hi Mark, Do you mean, why not use regulator_get_exclusive() or regulator_get()? Because cci and cpu litter core shared buck, it cannot use regulator_get_exclusive(). Because both cci and cpu want to tune voltage, it cannot use regulator_get(), otherwise it will get dummy regulator even this buck doesn't register.as regulator. BR, Andrew-sh.Cheng
Hi Andrew-sh.Cheng, On 5/20/20 12:43 PM, Andrew-sh.Cheng wrote: > This adds a devfreq driver for the Cache Coherent Interconnect (CCI) > of the Mediatek MT8183. > > On the MT8183 the CCI is supplied by the same regulator as the LITTLE > cores. The driver is notified when the regulator voltage changes > (driven by cpufreq) and adjusts the CCI frequency to the maximum > possible value. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > --- > drivers/devfreq/Kconfig | 10 ++ > drivers/devfreq/Makefile | 1 + > drivers/devfreq/mt8183-cci-devfreq.c | 206 +++++++++++++++++++++++++++++++++++ The mt8183-cci.c is enough for driver name. > 3 files changed, 217 insertions(+) > create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c > > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig > index d9067950af6a..4ed7116271ee 100644 > --- a/drivers/devfreq/Kconfig > +++ b/drivers/devfreq/Kconfig > @@ -103,6 +103,16 @@ config ARM_IMX8M_DDRC_DEVFREQ > This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows > adjusting DRAM frequency. > > +config ARM_MT8183_CCI_DEVFREQ > + tristate "MT8183 CCI DEVFREQ Driver" > + depends on ARM_MEDIATEK_CPUFREQ > + help > + This adds a devfreq driver for Cache Coherent Interconnect > + of Mediatek MT8183, which is shared the same regulator > + with cpu cluster. > + It can track buck voltage and update a proper cci frequency. s/cci/CCI > + Use notification to get regulator status. > + > config ARM_TEGRA_DEVFREQ > tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver" > depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \ > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile > index 3eb4d5e6635c..5b1b670c954d 100644 > --- a/drivers/devfreq/Makefile > +++ b/drivers/devfreq/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o > # DEVFREQ Drivers > obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o > obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o > +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ) += mt8183-cci-devfreq.o > obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o > obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o > obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o > diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c > new file mode 100644 > index 000000000000..cd7929a83bf8 > --- /dev/null > +++ b/drivers/devfreq/mt8183-cci-devfreq.c > @@ -0,0 +1,206 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2019 MediaTek Inc. s/2019/2020 > + > + * Author: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > + */ > + > +#include <linux/clk.h> > +#include <linux/devfreq.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > +#include <linux/time.h> > + > +#include "governor.h" It is not needed. Please remove it. > + > +#define MAX_VOLT_LIMIT (1150000) > + > +struct cci_devfreq { > + struct devfreq *devfreq; > + struct regulator *proc_reg; 'proc' means the 'processor'? Instead of 'proc_reg', you better to use 'cpu_reg'. > + struct clk *cci_clk; > + int old_vproc; > + unsigned long old_freq; > +}; > + > +static int mtk_cci_set_voltage(struct cci_devfreq *cci_df, int vproc) > +{ > + int ret; > + > + ret = regulator_set_voltage(cci_df->proc_reg, vproc, > + MAX_VOLT_LIMIT); > + if (!ret) > + cci_df->old_vproc = vproc; > + return ret; > +} > + > +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq, > + u32 flags) > +{ > + int ret; > + struct cci_devfreq *cci_df = dev_get_drvdata(dev); > + struct dev_pm_opp *opp; > + unsigned long opp_rate, opp_voltage, old_voltage; > + > + if (!cci_df) > + return -EINVAL; > + > + if (cci_df->old_freq == *freq) > + return 0; > + > + opp_rate = *freq; > + opp = dev_pm_opp_find_freq_floor(dev, &opp_rate); > + opp_voltage = dev_pm_opp_get_voltage(opp); > + dev_pm_opp_put(opp); You can use the helper function for getting the rate with devfreq_recommended_opp(). You can refer the following code in drivers/devfreq/exynos-bus.c opp = devfreq_recommended_opp(dev, freq, flags); if (IS_ERR(opp)) { dev_err(dev, "failed to get recommended opp instance\n"); return PTR_ERR(opp); } dev_pm_opp_put(opp); > + > + old_voltage = cci_df->old_vproc; > + if (old_voltage == 0) > + old_voltage = regulator_get_voltage(cci_df->proc_reg); > + > + // scale up: set voltage first then freq > + if (opp_voltage > old_voltage) { > + ret = mtk_cci_set_voltage(cci_df, opp_voltage); > + if (ret) { > + pr_err("cci: failed to scale up voltage\n"); > + return ret; > + } > + } > + > + ret = clk_set_rate(cci_df->cci_clk, *freq); > + if (ret) { > + pr_err("%s: failed cci to set rate: %d\n", __func__, > + ret); > + mtk_cci_set_voltage(cci_df, old_voltage); > + return ret; > + } > + > + // scale down: set freq first then voltage > + if (opp_voltage < old_voltage) { > + ret = mtk_cci_set_voltage(cci_df, opp_voltage); > + if (ret) { > + pr_err("cci: failed to scale down voltage\n"); > + clk_set_rate(cci_df->cci_clk, cci_df->old_freq); > + return ret; > + } > + } I recommend that dev_pm_opp_set_rate() and dev_pm_opp_set_regulator() instead of 'clk_set_rate' and 'regulator_set_voltage'. In the dev_pm_opp_set_rate(), handle the these sequence. You can refer the merged patch[1]. [1] commit 4294a779bd8dff6c65e7e85ffe7a1ea236e92a68 - PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate() > + > + cci_df->old_freq = *freq; > + > + return 0; > +} > + > +static struct devfreq_dev_profile cci_devfreq_profile = { > + .target = mtk_cci_devfreq_target, Need to add '.exit' for calling dev_pm_opp_of_remove_table(). You can refer the merged devfreq patches like exynos_bus.c, imx-bus.c. > +}; > + > +static int mtk_cci_devfreq_probe(struct platform_device *pdev) > +{ > + struct device *cci_dev = &pdev->dev; > + struct cci_devfreq *cci_df; > + struct devfreq_passive_data *passive_data; > + int ret; > + > + cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL); > + if (!cci_df) > + return -ENOMEM; > + > + cci_df->cci_clk = devm_clk_get(cci_dev, "cci_clock"); > + ret = PTR_ERR_OR_ZERO(cci_df->cci_clk); > + if (ret) { > + if (ret != -EPROBE_DEFER) > + dev_err(cci_dev, "failed to get clock for CCI: %d\n", > + ret); > + return ret; > + } > + cci_df->proc_reg = devm_regulator_get_optional(cci_dev, "proc"); > + ret = PTR_ERR_OR_ZERO(cci_df->proc_reg); > + if (ret) { > + if (ret != -EPROBE_DEFER) > + dev_err(cci_dev, "failed to get regulator for CCI: %d\n", > + ret); > + return ret; > + } I recommend that use dev_pm_opp_set_regulators. You can refer the merged patch[1]. [1] commit 4294a779bd8dff6c65e7e85ffe7a1ea236e92a68 - PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate() > + ret = regulator_enable(cci_df->proc_reg); > + if (ret) { > + pr_warn("enable buck for cci fail\n"); Use dev_err instead of 'pr_warn'. > + return ret; > + } > + > + ret = dev_pm_opp_of_add_table(cci_dev); > + if (ret) { > + dev_err(cci_dev, "Fail to init CCI OPP table: %d\n", ret); How about changing the error log as following because in this driver, use the 'failed to' sentence for error handling? failed to get OPP table for CCI:L %d > + return ret; > + } > + > + platform_set_drvdata(pdev, cci_df); > + > + passive_data = devm_kzalloc(cci_dev, sizeof(*passive_data), GFP_KERNEL); > + if (!passive_data) > + return -ENOMEM; On this error case, you have to call dev_pm_opp_of_remove_table(). You better to make the 'err_opp' jump lable and then add 'goto err_opp'. > + > + passive_data->parent_type = CPUFREQ_PARENT_DEV; > + > + cci_df->devfreq = devm_devfreq_add_device(cci_dev, > + &cci_devfreq_profile, > + DEVFREQ_GOV_PASSIVE, > + passive_data); > + if (IS_ERR(cci_df->devfreq)) { > + ret = PTR_ERR(cci_df->devfreq); > + dev_err(cci_dev, "cannot create cci devfreq device:%d\n", ret); > + dev_pm_opp_of_remove_table(cci_dev); Instead of direct call, use 'goto err_opp'. > + return ret; > + } > + > + return 0; > +} > + > +static int mtk_cci_devfreq_remove(struct platform_device *pdev) > +{ > + struct device *cci_dev = &pdev->dev; > + struct cci_devfreq *cci_df; > + struct notifier_block *opp_nb; > + > + cci_df = platform_get_drvdata(pdev); > + opp_nb = &cci_df->opp_nb; > + > + dev_pm_opp_unregister_notifier(cci_dev, opp_nb); This patch doesn't call the dev_pm_opp_register_notifier. Please remove it. > + devm_devfreq_remove_device(cci_dev, cci_df->devfreq); Don't need to call this function because you used devm_devfreq_add_device(). > + dev_pm_opp_of_remove_table(cci_dev)> + regulator_disable(cci_df->proc_reg); > + > + return 0; > +} > + > +static const __maybe_unused struct of_device_id > + mediatek_cci_devfreq_of_match[] = { Make it on one line and remove '__maybe_unused' keyword. - mediatek_cci_devfreq_of_match-> mediatek_cci_of_match > + { .compatible = "mediatek,mt8183-cci" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match); ditto. > + > +static struct platform_driver cci_devfreq_driver = { > + .probe = mtk_cci_devfreq_probe, > + .remove = mtk_cci_devfreq_remove, > + .driver = { > + .name = "mediatek-cci-devfreq", > + .of_match_table = of_match_ptr(mediatek_cci_devfreq_of_match), ditto. > + }, > +}; > + > +static int __init mtk_cci_devfreq_init(void) > +{ > + return platform_driver_register(&cci_devfreq_driver); > +} > +module_init(mtk_cci_devfreq_init) > + > +static void __exit mtk_cci_devfreq_exit(void) > +{ > + platform_driver_unregister(&cci_devfreq_driver); > +} > +module_exit(mtk_cci_devfreq_exit) Use 'module_platform_driver' instead of module_init and module_exit. > + > +MODULE_DESCRIPTION("Mediatek CCI devfreq driver"); > +MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>"); > +MODULE_LICENSE("GPL v2"); >
Hi Andrew-sh.Cheng, On 5/28/20 4:35 PM, Chanwoo Choi wrote: > Hi Andrew-sh.Cheng, > > On 5/20/20 12:43 PM, Andrew-sh.Cheng wrote: >> This adds a devfreq driver for the Cache Coherent Interconnect (CCI) >> of the Mediatek MT8183. >> >> On the MT8183 the CCI is supplied by the same regulator as the LITTLE >> cores. The driver is notified when the regulator voltage changes >> (driven by cpufreq) and adjusts the CCI frequency to the maximum >> possible value. I understood that the mt8183-cci.c and mt8183 cpufreq driver (ARM_MEDIATEK_CPUFREQ) shared the same regulator. So, when mt8183 cpufreq driver changes the CPU frequency and voltage, the mt8183-cci.c changes the CCI frequency according to the new mt8183 frequency with passive governor. I think that mt8183-cci.c don't need to change the voltage because already mt8183 cpufreq changed the voltage of shared regulator. Why do you change the voltage in this driver? >> >> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> >> --- >> drivers/devfreq/Kconfig | 10 ++ >> drivers/devfreq/Makefile | 1 + >> drivers/devfreq/mt8183-cci-devfreq.c | 206 +++++++++++++++++++++++++++++++++++ > > The mt8183-cci.c is enough for driver name. > >> 3 files changed, 217 insertions(+) >> create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c >> >> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig >> index d9067950af6a..4ed7116271ee 100644 >> --- a/drivers/devfreq/Kconfig >> +++ b/drivers/devfreq/Kconfig >> @@ -103,6 +103,16 @@ config ARM_IMX8M_DDRC_DEVFREQ >> This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows >> adjusting DRAM frequency. >> >> +config ARM_MT8183_CCI_DEVFREQ >> + tristate "MT8183 CCI DEVFREQ Driver" >> + depends on ARM_MEDIATEK_CPUFREQ >> + help >> + This adds a devfreq driver for Cache Coherent Interconnect >> + of Mediatek MT8183, which is shared the same regulator >> + with cpu cluster. >> + It can track buck voltage and update a proper cci frequency. > > s/cci/CCI > >> + Use notification to get regulator status. >> + >> config ARM_TEGRA_DEVFREQ >> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver" >> depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \ >> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile >> index 3eb4d5e6635c..5b1b670c954d 100644 >> --- a/drivers/devfreq/Makefile >> +++ b/drivers/devfreq/Makefile >> @@ -10,6 +10,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o >> # DEVFREQ Drivers >> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o >> obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o >> +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ) += mt8183-cci-devfreq.o >> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o >> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o >> obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o >> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c >> new file mode 100644 >> index 000000000000..cd7929a83bf8 >> --- /dev/null >> +++ b/drivers/devfreq/mt8183-cci-devfreq.c >> @@ -0,0 +1,206 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2019 MediaTek Inc. > > s/2019/2020 > >> + >> + * Author: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/devfreq.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/regulator/consumer.h> >> +#include <linux/time.h> >> + >> +#include "governor.h" > > It is not needed. Please remove it. > >> + >> +#define MAX_VOLT_LIMIT (1150000) >> + >> +struct cci_devfreq { >> + struct devfreq *devfreq; >> + struct regulator *proc_reg; > > 'proc' means the 'processor'? > Instead of 'proc_reg', you better to use 'cpu_reg'. > >> + struct clk *cci_clk; >> + int old_vproc; >> + unsigned long old_freq; >> +}; >> + >> +static int mtk_cci_set_voltage(struct cci_devfreq *cci_df, int vproc) >> +{ >> + int ret; >> + >> + ret = regulator_set_voltage(cci_df->proc_reg, vproc, >> + MAX_VOLT_LIMIT); >> + if (!ret) >> + cci_df->old_vproc = vproc; >> + return ret; >> +} >> + >> +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq, >> + u32 flags) >> +{ >> + int ret; >> + struct cci_devfreq *cci_df = dev_get_drvdata(dev); >> + struct dev_pm_opp *opp; >> + unsigned long opp_rate, opp_voltage, old_voltage; >> + >> + if (!cci_df) >> + return -EINVAL; >> + >> + if (cci_df->old_freq == *freq) >> + return 0; >> + >> + opp_rate = *freq; >> + opp = dev_pm_opp_find_freq_floor(dev, &opp_rate); >> + opp_voltage = dev_pm_opp_get_voltage(opp); >> + dev_pm_opp_put(opp); > > > You can use the helper function for getting the rate > with devfreq_recommended_opp(). You can refer the following code > in drivers/devfreq/exynos-bus.c > > opp = devfreq_recommended_opp(dev, freq, flags); > if (IS_ERR(opp)) { > dev_err(dev, "failed to get recommended opp instance\n"); > return PTR_ERR(opp); > } > dev_pm_opp_put(opp); > >> + >> + old_voltage = cci_df->old_vproc; >> + if (old_voltage == 0) >> + old_voltage = regulator_get_voltage(cci_df->proc_reg); >> + >> + // scale up: set voltage first then freq >> + if (opp_voltage > old_voltage) { >> + ret = mtk_cci_set_voltage(cci_df, opp_voltage); >> + if (ret) { >> + pr_err("cci: failed to scale up voltage\n"); >> + return ret; >> + } >> + } >> + >> + ret = clk_set_rate(cci_df->cci_clk, *freq); >> + if (ret) { >> + pr_err("%s: failed cci to set rate: %d\n", __func__, >> + ret); >> + mtk_cci_set_voltage(cci_df, old_voltage); >> + return ret; >> + } >> + >> + // scale down: set freq first then voltage >> + if (opp_voltage < old_voltage) { >> + ret = mtk_cci_set_voltage(cci_df, opp_voltage); >> + if (ret) { >> + pr_err("cci: failed to scale down voltage\n"); >> + clk_set_rate(cci_df->cci_clk, cci_df->old_freq); >> + return ret; >> + } >> + } > > > I recommend that dev_pm_opp_set_rate() and dev_pm_opp_set_regulator() > instead of 'clk_set_rate' and 'regulator_set_voltage'. > In the dev_pm_opp_set_rate(), handle the these sequence. > You can refer the merged patch[1]. > > [1] commit 4294a779bd8dff6c65e7e85ffe7a1ea236e92a68 > - PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate() > > >> + >> + cci_df->old_freq = *freq; >> + >> + return 0; >> +} >> + >> +static struct devfreq_dev_profile cci_devfreq_profile = { >> + .target = mtk_cci_devfreq_target, > > Need to add '.exit' for calling dev_pm_opp_of_remove_table(). > You can refer the merged devfreq patches like exynos_bus.c, imx-bus.c. > >> +}; >> + >> +static int mtk_cci_devfreq_probe(struct platform_device *pdev) >> +{ >> + struct device *cci_dev = &pdev->dev; >> + struct cci_devfreq *cci_df; >> + struct devfreq_passive_data *passive_data; >> + int ret; >> + >> + cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL); >> + if (!cci_df) >> + return -ENOMEM; >> + >> + cci_df->cci_clk = devm_clk_get(cci_dev, "cci_clock"); >> + ret = PTR_ERR_OR_ZERO(cci_df->cci_clk); >> + if (ret) { >> + if (ret != -EPROBE_DEFER) >> + dev_err(cci_dev, "failed to get clock for CCI: %d\n", >> + ret); >> + return ret; >> + } >> + cci_df->proc_reg = devm_regulator_get_optional(cci_dev, "proc"); >> + ret = PTR_ERR_OR_ZERO(cci_df->proc_reg); >> + if (ret) { >> + if (ret != -EPROBE_DEFER) >> + dev_err(cci_dev, "failed to get regulator for CCI: %d\n", >> + ret); >> + return ret; >> + } > > I recommend that use dev_pm_opp_set_regulators. > You can refer the merged patch[1]. > > [1] commit 4294a779bd8dff6c65e7e85ffe7a1ea236e92a68 > - PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate() > > >> + ret = regulator_enable(cci_df->proc_reg); >> + if (ret) { >> + pr_warn("enable buck for cci fail\n"); > > Use dev_err instead of 'pr_warn'. > >> + return ret; >> + } >> + >> + ret = dev_pm_opp_of_add_table(cci_dev); >> + if (ret) { >> + dev_err(cci_dev, "Fail to init CCI OPP table: %d\n", ret); > > How about changing the error log as following > because in this driver, use the 'failed to' sentence for error handling? > > failed to get OPP table for CCI:L %d > >> + return ret; >> + } >> + >> + platform_set_drvdata(pdev, cci_df); >> + >> + passive_data = devm_kzalloc(cci_dev, sizeof(*passive_data), GFP_KERNEL); >> + if (!passive_data) >> + return -ENOMEM; > > On this error case, you have to call dev_pm_opp_of_remove_table(). > You better to make the 'err_opp' jump lable and then add 'goto err_opp'. > >> + >> + passive_data->parent_type = CPUFREQ_PARENT_DEV; >> + >> + cci_df->devfreq = devm_devfreq_add_device(cci_dev, >> + &cci_devfreq_profile, >> + DEVFREQ_GOV_PASSIVE, >> + passive_data); >> + if (IS_ERR(cci_df->devfreq)) { >> + ret = PTR_ERR(cci_df->devfreq); >> + dev_err(cci_dev, "cannot create cci devfreq device:%d\n", ret); >> + dev_pm_opp_of_remove_table(cci_dev); > > Instead of direct call, use 'goto err_opp'. > >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int mtk_cci_devfreq_remove(struct platform_device *pdev) >> +{ >> + struct device *cci_dev = &pdev->dev; >> + struct cci_devfreq *cci_df; >> + struct notifier_block *opp_nb; >> + >> + cci_df = platform_get_drvdata(pdev); >> + opp_nb = &cci_df->opp_nb; >> + >> + dev_pm_opp_unregister_notifier(cci_dev, opp_nb); > > This patch doesn't call the dev_pm_opp_register_notifier. > Please remove it. > >> + devm_devfreq_remove_device(cci_dev, cci_df->devfreq); > > Don't need to call this function because you used devm_devfreq_add_device(). > >> + dev_pm_opp_of_remove_table(cci_dev)> + regulator_disable(cci_df->proc_reg); >> + >> + return 0; >> +} >> + >> +static const __maybe_unused struct of_device_id >> + mediatek_cci_devfreq_of_match[] = { > > Make it on one line and remove '__maybe_unused' keyword. > - mediatek_cci_devfreq_of_match-> mediatek_cci_of_match > >> + { .compatible = "mediatek,mt8183-cci" }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match); > > ditto. > >> + >> +static struct platform_driver cci_devfreq_driver = { >> + .probe = mtk_cci_devfreq_probe, >> + .remove = mtk_cci_devfreq_remove, >> + .driver = { >> + .name = "mediatek-cci-devfreq", >> + .of_match_table = of_match_ptr(mediatek_cci_devfreq_of_match), > > ditto. > >> + }, >> +}; >> + >> +static int __init mtk_cci_devfreq_init(void) >> +{ >> + return platform_driver_register(&cci_devfreq_driver); >> +} >> +module_init(mtk_cci_devfreq_init) >> + >> +static void __exit mtk_cci_devfreq_exit(void) >> +{ >> + platform_driver_unregister(&cci_devfreq_driver); >> +} >> +module_exit(mtk_cci_devfreq_exit) > > Use 'module_platform_driver' instead of module_init and module_exit. > >> + >> +MODULE_DESCRIPTION("Mediatek CCI devfreq driver"); >> +MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>"); >> +MODULE_LICENSE("GPL v2"); >> > >
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig index d9067950af6a..4ed7116271ee 100644 --- a/drivers/devfreq/Kconfig +++ b/drivers/devfreq/Kconfig @@ -103,6 +103,16 @@ config ARM_IMX8M_DDRC_DEVFREQ This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows adjusting DRAM frequency. +config ARM_MT8183_CCI_DEVFREQ + tristate "MT8183 CCI DEVFREQ Driver" + depends on ARM_MEDIATEK_CPUFREQ + help + This adds a devfreq driver for Cache Coherent Interconnect + of Mediatek MT8183, which is shared the same regulator + with cpu cluster. + It can track buck voltage and update a proper cci frequency. + Use notification to get regulator status. + config ARM_TEGRA_DEVFREQ tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver" depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \ diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile index 3eb4d5e6635c..5b1b670c954d 100644 --- a/drivers/devfreq/Makefile +++ b/drivers/devfreq/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o # DEVFREQ Drivers obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ) += mt8183-cci-devfreq.o obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c new file mode 100644 index 000000000000..cd7929a83bf8 --- /dev/null +++ b/drivers/devfreq/mt8183-cci-devfreq.c @@ -0,0 +1,206 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2019 MediaTek Inc. + + * Author: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> + */ + +#include <linux/clk.h> +#include <linux/devfreq.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/time.h> + +#include "governor.h" + +#define MAX_VOLT_LIMIT (1150000) + +struct cci_devfreq { + struct devfreq *devfreq; + struct regulator *proc_reg; + struct clk *cci_clk; + int old_vproc; + unsigned long old_freq; +}; + +static int mtk_cci_set_voltage(struct cci_devfreq *cci_df, int vproc) +{ + int ret; + + ret = regulator_set_voltage(cci_df->proc_reg, vproc, + MAX_VOLT_LIMIT); + if (!ret) + cci_df->old_vproc = vproc; + return ret; +} + +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq, + u32 flags) +{ + int ret; + struct cci_devfreq *cci_df = dev_get_drvdata(dev); + struct dev_pm_opp *opp; + unsigned long opp_rate, opp_voltage, old_voltage; + + if (!cci_df) + return -EINVAL; + + if (cci_df->old_freq == *freq) + return 0; + + opp_rate = *freq; + opp = dev_pm_opp_find_freq_floor(dev, &opp_rate); + opp_voltage = dev_pm_opp_get_voltage(opp); + dev_pm_opp_put(opp); + + old_voltage = cci_df->old_vproc; + if (old_voltage == 0) + old_voltage = regulator_get_voltage(cci_df->proc_reg); + + // scale up: set voltage first then freq + if (opp_voltage > old_voltage) { + ret = mtk_cci_set_voltage(cci_df, opp_voltage); + if (ret) { + pr_err("cci: failed to scale up voltage\n"); + return ret; + } + } + + ret = clk_set_rate(cci_df->cci_clk, *freq); + if (ret) { + pr_err("%s: failed cci to set rate: %d\n", __func__, + ret); + mtk_cci_set_voltage(cci_df, old_voltage); + return ret; + } + + // scale down: set freq first then voltage + if (opp_voltage < old_voltage) { + ret = mtk_cci_set_voltage(cci_df, opp_voltage); + if (ret) { + pr_err("cci: failed to scale down voltage\n"); + clk_set_rate(cci_df->cci_clk, cci_df->old_freq); + return ret; + } + } + + cci_df->old_freq = *freq; + + return 0; +} + +static struct devfreq_dev_profile cci_devfreq_profile = { + .target = mtk_cci_devfreq_target, +}; + +static int mtk_cci_devfreq_probe(struct platform_device *pdev) +{ + struct device *cci_dev = &pdev->dev; + struct cci_devfreq *cci_df; + struct devfreq_passive_data *passive_data; + int ret; + + cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL); + if (!cci_df) + return -ENOMEM; + + cci_df->cci_clk = devm_clk_get(cci_dev, "cci_clock"); + ret = PTR_ERR_OR_ZERO(cci_df->cci_clk); + if (ret) { + if (ret != -EPROBE_DEFER) + dev_err(cci_dev, "failed to get clock for CCI: %d\n", + ret); + return ret; + } + cci_df->proc_reg = devm_regulator_get_optional(cci_dev, "proc"); + ret = PTR_ERR_OR_ZERO(cci_df->proc_reg); + if (ret) { + if (ret != -EPROBE_DEFER) + dev_err(cci_dev, "failed to get regulator for CCI: %d\n", + ret); + return ret; + } + ret = regulator_enable(cci_df->proc_reg); + if (ret) { + pr_warn("enable buck for cci fail\n"); + return ret; + } + + ret = dev_pm_opp_of_add_table(cci_dev); + if (ret) { + dev_err(cci_dev, "Fail to init CCI OPP table: %d\n", ret); + return ret; + } + + platform_set_drvdata(pdev, cci_df); + + passive_data = devm_kzalloc(cci_dev, sizeof(*passive_data), GFP_KERNEL); + if (!passive_data) + return -ENOMEM; + + passive_data->parent_type = CPUFREQ_PARENT_DEV; + + cci_df->devfreq = devm_devfreq_add_device(cci_dev, + &cci_devfreq_profile, + DEVFREQ_GOV_PASSIVE, + passive_data); + if (IS_ERR(cci_df->devfreq)) { + ret = PTR_ERR(cci_df->devfreq); + dev_err(cci_dev, "cannot create cci devfreq device:%d\n", ret); + dev_pm_opp_of_remove_table(cci_dev); + return ret; + } + + return 0; +} + +static int mtk_cci_devfreq_remove(struct platform_device *pdev) +{ + struct device *cci_dev = &pdev->dev; + struct cci_devfreq *cci_df; + struct notifier_block *opp_nb; + + cci_df = platform_get_drvdata(pdev); + opp_nb = &cci_df->opp_nb; + + dev_pm_opp_unregister_notifier(cci_dev, opp_nb); + devm_devfreq_remove_device(cci_dev, cci_df->devfreq); + dev_pm_opp_of_remove_table(cci_dev); + regulator_disable(cci_df->proc_reg); + + return 0; +} + +static const __maybe_unused struct of_device_id + mediatek_cci_devfreq_of_match[] = { + { .compatible = "mediatek,mt8183-cci" }, + { }, +}; +MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match); + +static struct platform_driver cci_devfreq_driver = { + .probe = mtk_cci_devfreq_probe, + .remove = mtk_cci_devfreq_remove, + .driver = { + .name = "mediatek-cci-devfreq", + .of_match_table = of_match_ptr(mediatek_cci_devfreq_of_match), + }, +}; + +static int __init mtk_cci_devfreq_init(void) +{ + return platform_driver_register(&cci_devfreq_driver); +} +module_init(mtk_cci_devfreq_init) + +static void __exit mtk_cci_devfreq_exit(void) +{ + platform_driver_unregister(&cci_devfreq_driver); +} +module_exit(mtk_cci_devfreq_exit) + +MODULE_DESCRIPTION("Mediatek CCI devfreq driver"); +MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>"); +MODULE_LICENSE("GPL v2");
This adds a devfreq driver for the Cache Coherent Interconnect (CCI) of the Mediatek MT8183. On the MT8183 the CCI is supplied by the same regulator as the LITTLE cores. The driver is notified when the regulator voltage changes (driven by cpufreq) and adjusts the CCI frequency to the maximum possible value. Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> --- drivers/devfreq/Kconfig | 10 ++ drivers/devfreq/Makefile | 1 + drivers/devfreq/mt8183-cci-devfreq.c | 206 +++++++++++++++++++++++++++++++++++ 3 files changed, 217 insertions(+) create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c