| 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
| ||
|
Date: Wed, 17 Apr 2019 12:36:35 +0300
From: Dmitry Osipenko <digetx@...il.com>
To: Chanwoo Choi <cw00.choi@...sung.com>,
Thierry Reding <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>,
MyungJoo Ham <myungjoo.ham@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>,
Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc: linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 19/19] PM / devfreq: Introduce driver for NVIDIA
Tegra20
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 <digetx@...il.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 <digetx@...il.com>
>>>> +L: linux-pm@...r.kernel.org
>>>> +L: linux-tegra@...r.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-mm@...ck.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 <digetx@...il.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]
Powered by blists - more mailing lists