17.04.2019 3:26, Chanwoo Choi пишет: > Hi, > > On 19. 4. 17. 오전 1:11, Dmitry Osipenko wrote: >> 16.04.2019 11:31, Chanwoo Choi пишет: >>> Hi, >>> >>> On 19. 4. 15. 오후 11:55, Dmitry Osipenko wrote: >>>> Add devfreq driver for NVIDIA Tegra20 SoC's. The driver periodically >>>> reads out Memory Controller counters and adjusts memory frequency based >>>> on the memory clients activity. >>>> >>>> Signed-off-by: Dmitry Osipenko <dig...@gmail.com> >>>> --- >>>> MAINTAINERS | 8 ++ >>>> drivers/devfreq/Kconfig | 10 ++ >>>> drivers/devfreq/Makefile | 1 + >>>> drivers/devfreq/tegra20-devfreq.c | 177 ++++++++++++++++++++++++++++++ >>>> 4 files changed, 196 insertions(+) >>>> create mode 100644 drivers/devfreq/tegra20-devfreq.c >>>> >>>> diff --git a/MAINTAINERS b/MAINTAINERS >>>> index 80b59db1b6e4..91f475ec4545 100644 >>>> --- a/MAINTAINERS >>>> +++ b/MAINTAINERS >>>> @@ -10056,6 +10056,14 @@ F: include/linux/memblock.h >>>> F: mm/memblock.c >>>> F: Documentation/core-api/boot-time-mm.rst >>>> >>>> +MEMORY FREQUENCY SCALING DRIVER FOR NVIDIA TEGRA20 >>>> +M: Dmitry Osipenko <dig...@gmail.com> >>>> +L: linux...@vger.kernel.org >>>> +L: linux-te...@vger.kernel.org >>>> +T: git >>>> git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git >>>> +S: Maintained >>>> +F: drivers/devfreq/tegra20-devfreq.c >>>> + >>>> MEMORY MANAGEMENT >>>> L: linux...@kvack.org >>>> W: http://www.linux-mm.org >>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig >>>> index bd6652863e7d..af4c86c4e0f6 100644 >>>> --- a/drivers/devfreq/Kconfig >>>> +++ b/drivers/devfreq/Kconfig >>>> @@ -100,6 +100,16 @@ config ARM_TEGRA_DEVFREQ >>>> It reads ACTMON counters of memory controllers and adjusts the >>>> operating frequencies and voltages with OPP support. >>>> >>>> +config ARM_TEGRA20_DEVFREQ >>>> + tristate "NVIDIA Tegra20 DEVFREQ Driver" >>>> + depends on (TEGRA_MC && TEGRA20_EMC) || COMPILE_TEST >>>> + select DEVFREQ_GOV_SIMPLE_ONDEMAND >>>> + select PM_OPP >>>> + help >>>> + This adds the DEVFREQ driver for the Tegra20 family of SoCs. >>>> + It reads Memory Controller counters and adjusts the operating >>>> + frequencies and voltages with OPP support. >>>> + >>>> config ARM_RK3399_DMC_DEVFREQ >>>> tristate "ARM RK3399 DMC DEVFREQ Driver" >>>> depends on ARCH_ROCKCHIP >>>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile >>>> index 32b8d4d3f12c..6fcc5596b8b7 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_RK3399_DMC_DEVFREQ) += rk3399_dmc.o >>>> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra-devfreq.o >>>> +obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o >>>> >>>> # DEVFREQ Event Drivers >>>> obj-$(CONFIG_PM_DEVFREQ_EVENT) += event/ >>>> diff --git a/drivers/devfreq/tegra20-devfreq.c >>>> b/drivers/devfreq/tegra20-devfreq.c >>>> new file mode 100644 >>>> index 000000000000..18c9aad7a9d7 >>>> --- /dev/null >>>> +++ b/drivers/devfreq/tegra20-devfreq.c >>>> @@ -0,0 +1,177 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * NVIDIA Tegra20 devfreq driver >>>> + * >>>> + * Author: Dmitry Osipenko <dig...@gmail.com> >>>> + */ >>> >>> It doesn't any "Copyright (c) 2019 ..." sentence. >> >> I'll add one in v3. >> >>>> + >>>> +#include <linux/clk.h> >>>> +#include <linux/devfreq.h> >>>> +#include <linux/io.h> >>>> +#include <linux/kernel.h> >>>> +#include <linux/module.h> >>>> +#include <linux/of_device.h> >>>> +#include <linux/platform_device.h> >>>> +#include <linux/pm_opp.h> >>>> +#include <linux/slab.h> >>>> + >>>> +#include <soc/tegra/mc.h> >>> >>> I can find the '<soc/tegra/mc.h>' header file >>> on mainline branch. But mc.h is included in linux-next.git. >>> >>> If you don't share the patch related to mc.h, >>> the kernel build will be failed when apply it the devfreq.git >>> on final step. Actually, it should make the immutable branch >>> between two related maintainers in order to remove the build fail. >> >> The '<soc/tegra/mc.h>' header file exists since v3.18 [0], seems you just >> missed something. >> >> [0] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/soc/tegra/mc.h?h=v3.18&id=8918465163171322c77a19d5258a95f56d89d2e4 > > Sorry. It is my missing point. When I tried to find it, > it is included in the mainline kernel. > >> >>>> + >>>> +#include "governor.h" >>>> + >>>> +#define MC_STAT_CONTROL 0x90 >>>> +#define MC_STAT_EMC_CLOCK_LIMIT 0xa0 >>>> +#define MC_STAT_EMC_CLOCKS 0xa4 >>>> +#define MC_STAT_EMC_CONTROL 0xa8 >>>> +#define MC_STAT_EMC_COUNT 0xb8 >>>> + >>>> +#define EMC_GATHER_CLEAR (1 << 8) >>>> +#define EMC_GATHER_ENABLE (3 << 8) >>>> + >>>> +struct tegra_devfreq { >>>> + struct devfreq *devfreq; >>>> + struct clk *clk; >>>> + void __iomem *regs; >>>> +}; >>>> + >>>> +static int tegra_devfreq_target(struct device *dev, unsigned long *freq, >>>> + u32 flags) >>>> +{ >>>> + struct tegra_devfreq *tegra = dev_get_drvdata(dev); >>>> + struct dev_pm_opp *opp; >>>> + unsigned long rate; >>>> + int err; >>>> + >>>> + opp = devfreq_recommended_opp(dev, freq, flags); >>>> + if (IS_ERR(opp)) >>>> + return PTR_ERR(opp); >>>> + >>>> + rate = dev_pm_opp_get_freq(opp); >>>> + dev_pm_opp_put(opp); >>>> + >>>> + err = clk_set_min_rate(tegra->clk, rate); >>>> + if (err) >>>> + return err; >>>> + >>>> + err = clk_set_rate(tegra->clk, 0); >>>> + if (err) >>> >>> When fail happen, I think that you have to control >>> the restoring sequence for previous operation like clk_set_min_rate(). >> >> Okay, thanks. >> >>>> + return err; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int tegra_devfreq_get_dev_status(struct device *dev, >>>> + struct devfreq_dev_status *stat) >>>> +{ >>>> + struct tegra_devfreq *tegra = dev_get_drvdata(dev); >>>> + >>>> + stat->busy_time = readl_relaxed(tegra->regs + MC_STAT_EMC_COUNT); >>>> + stat->total_time = readl_relaxed(tegra->regs + MC_STAT_EMC_CLOCKS) / 8; >>>> + stat->current_frequency = clk_get_rate(tegra->clk); >>>> + >>>> + writel_relaxed(EMC_GATHER_CLEAR, tegra->regs + MC_STAT_CONTROL); >>>> + writel_relaxed(EMC_GATHER_ENABLE, tegra->regs + MC_STAT_CONTROL); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static struct devfreq_dev_profile tegra_devfreq_profile = { >>>> + .polling_ms = 500, >>>> + .target = tegra_devfreq_target, >>>> + .get_dev_status = tegra_devfreq_get_dev_status, >>>> +}; >>>> + >>>> +static struct tegra_mc *tegra_get_memory_controller(void) >>>> +{ >>>> + struct platform_device *pdev; >>>> + struct device_node *np; >>>> + struct tegra_mc *mc; >>>> + >>>> + np = of_find_compatible_node(NULL, NULL, "nvidia,tegra20-mc-gart"); >>>> + if (!np) >>>> + return ERR_PTR(-ENOENT); >>>> + >>>> + pdev = of_find_device_by_node(np); >>>> + of_node_put(np); >>>> + if (!pdev) >>>> + return ERR_PTR(-ENODEV); >>>> + >>>> + mc = platform_get_drvdata(pdev); >>>> + if (!mc) >>>> + return ERR_PTR(-EPROBE_DEFER); >>>> + >>>> + return mc; >>>> +} >>>> + >>>> +static int tegra_devfeq_probe(struct platform_device *pdev) >>>> +{ >>>> + struct tegra_devfreq *tegra; >>>> + struct tegra_mc *mc; >>>> + unsigned long max_rate; >>>> + unsigned long rate; >>>> + int err; >>>> + >>>> + mc = tegra_get_memory_controller(); >>>> + if (IS_ERR(mc)) { >>>> + err = PTR_ERR(mc); >>>> + dev_err(&pdev->dev, "failed to get mc: %d\n", err); >>> >>> How about using 'memory controller' instead of 'mc'? >>> Because 'mc' is not standard expression. >> >> Sounds good, thanks. >> >>>> + return err; >>>> + } >>>> + >>>> + tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL); >>>> + if (!tegra) >>>> + return -ENOMEM; >>>> + >>>> + tegra->clk = devm_clk_get(&pdev->dev, "emc"); >>>> + if (IS_ERR(tegra->clk)) { >>>> + err = PTR_ERR(tegra->clk); >>>> + dev_err(&pdev->dev, "failed to get emc clock: %d\n", err); >>>> + return err; >>>> + } >>> >>> Don't you need to enable the 'emc' clock'? >>> Because this patch doesn't enable this clock. >> >> EMC is a system critical clock (marked as critical by the clock driver), it >> is guaranteed to be always enabled. I think it is fine to omit >> clock-enabling in this case. > > If you think don't need to enable it due to the critical clock, > instead, please add the comment about that emc clock is critical. > In my case, it looks like the strange use-case because this driver > doesn't contain the any enable code for clock.
Okay, thank you for the review again. [snip]
