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 <digetx@xxxxxxxxx> >> --- >> 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 <digetx@xxxxxxxxx> >> +L: linux-pm@xxxxxxxxxxxxxxx >> +L: linux-tegra@xxxxxxxxxxxxxxx >> +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-mm@xxxxxxxxx >> 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 <digetx@xxxxxxxxx> >> + */ > > 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 >> + >> +#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. >> + >> + tegra->regs = mc->regs; >> + >> + max_rate = clk_round_rate(tegra->clk, ULONG_MAX); >> + >> + for (rate = 0; rate <= max_rate; rate++) { >> + rate = clk_round_rate(tegra->clk, rate); >> + dev_pm_opp_add(&pdev->dev, rate, 0); >> + } >> + >> + writel_relaxed(0x00000000, tegra->regs + MC_STAT_CONTROL); >> + writel_relaxed(0x00000000, tegra->regs + MC_STAT_EMC_CONTROL); >> + writel_relaxed(0xffffffff, tegra->regs + MC_STAT_EMC_CLOCK_LIMIT); > > You better to add the comments of what are the meaning > of 0x00000000/0xffffffff. Without the detailed comments, > it is difficult to understand of meaning. Okay. Although the complete registers description is available in public and someone really curious could just google for the full details. >> + >> + platform_set_drvdata(pdev, tegra); >> + >> + tegra->devfreq = devfreq_add_device(&pdev->dev, &tegra_devfreq_profile, >> + DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL); >> + if (IS_ERR(tegra->devfreq)) >> + return PTR_ERR(tegra->devfreq); >> + >> + return 0; >> +} >> + >> +static int tegra_devfreq_remove(struct platform_device *pdev) >> +{ >> + struct tegra_devfreq *tegra = platform_get_drvdata(pdev); >> + >> + devfreq_remove_device(tegra->devfreq); >> + dev_pm_opp_remove_all_dynamic(&pdev->dev); >> + >> + return 0; >> +} >> + >> +static struct platform_driver tegra_devfeq_driver = { >> + .probe = tegra_devfeq_probe, >> + .remove = tegra_devfreq_remove, >> + .driver = { >> + .name = "tegra20-devfreq", > > How can you bind this driver without compatible name for Devicetree? > And I tried to find the name ("tegra20-devfreq") in > the MFD drivers. As I wrote in the cover-letter, the device for the driver will be created by the EMC driver. I'll send out the patch that creates the device later on because of EMC driver patch-series interdependence, for now I have that patch here [1] if you're curious how it looks like. [1] https://github.com/grate-driver/linux/commit/c34440402bb4fb365647bf43166e85562c124273
