Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v7 36/47] memory: tegra20-emc: Add devfreq support
To:     Dmitry Osipenko <digetx@gmail.com>,
        Thierry Reding <thierry.reding@gmail.com>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        Georgi Djakov <georgi.djakov@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Peter De Schrijver <pdeschrijver@nvidia.com>,
        MyungJoo Ham <myungjoo.ham@samsung.com>,
        Kyungmin Park <kyungmin.park@samsung.com>,
        Mikko Perttunen <cyndis@kapsi.fi>,
        Viresh Kumar <vireshk@kernel.org>,
        Peter Geis <pgwipeout@gmail.com>,
        Nicolas Chauvet <kwizart@gmail.com>,
        Krzysztof Kozlowski <krzk@kernel.org>
Cc:     linux-tegra@vger.kernel.org, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        devicetree@vger.kernel.org
From:   Chanwoo Choi <cw00.choi@samsung.com>
Organization: Samsung Electronics
Message-ID: <95e289fc-210c-1f8d-1b24-36e33ce96deb@samsung.com>
Date:   Thu, 5 Nov 2020 11:30:32 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101
        Thunderbird/59.0
MIME-Version: 1.0
In-Reply-To: <20201104164923.21238-37-digetx@gmail.com>
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20201104164923.21238-1-digetx@gmail.com>
        <CGME20201104165200epcas1p1c8f692fa2d5a01c01e4f8aca55462fb2@epcas1p1.samsung.com>
        <20201104164923.21238-37-digetx@gmail.com>
Precedence: bulk

On 11/5/20 1:49 AM, Dmitry Osipenko wrote:
> Add devfreq support to the Tegra20 EMC driver. Memory utilization
> statistics will be periodically polled from the memory controller and
> appropriate minimum clock rate will be selected by the devfreq governor.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/memory/tegra/Kconfig       |  2 +
>  drivers/memory/tegra/tegra20-emc.c | 92 ++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
> 
> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
> index ac3dfe155505..76e9a3b10839 100644
> --- a/drivers/memory/tegra/Kconfig
> +++ b/drivers/memory/tegra/Kconfig
> @@ -12,6 +12,8 @@ config TEGRA20_EMC
>  	tristate "NVIDIA Tegra20 External Memory Controller driver"
>  	default y
>  	depends on TEGRA_MC && ARCH_TEGRA_2x_SOC
> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
> +	select PM_DEVFREQ
>  	select PM_OPP

nitpick. If you select PM_DEVFREQ, don't need to select 'PM_OPP'
bacause PM_DEVFREQ use OPP as mandatory with 'select PM_OPP' in Kconfig.


>  	help
>  	  This driver is for the External Memory Controller (EMC) found on
> diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
> index 5e10aa97809f..9946b957bb01 100644
> --- a/drivers/memory/tegra/tegra20-emc.c
> +++ b/drivers/memory/tegra/tegra20-emc.c
> @@ -8,6 +8,7 @@
>  #include <linux/clk.h>
>  #include <linux/clk/tegra.h>
>  #include <linux/debugfs.h>
> +#include <linux/devfreq.h>
>  #include <linux/err.h>
>  #include <linux/interconnect-provider.h>
>  #include <linux/interrupt.h>
> @@ -102,6 +103,10 @@
>  
>  #define EMC_FBIO_CFG5_DRAM_WIDTH_X16		BIT(4)
>  
> +#define EMC_PWR_GATHER_CLEAR			(1 << 8)
> +#define EMC_PWR_GATHER_DISABLE			(2 << 8)
> +#define EMC_PWR_GATHER_ENABLE			(3 << 8)
> +
>  static const u16 emc_timing_registers[] = {
>  	EMC_RC,
>  	EMC_RFC,
> @@ -157,6 +162,7 @@ struct emc_timing {
>  };
>  
>  enum emc_rate_request_type {
> +	EMC_RATE_DEVFREQ,
>  	EMC_RATE_DEBUG,
>  	EMC_RATE_ICC,
>  	EMC_RATE_TYPE_MAX,
> @@ -193,6 +199,9 @@ struct tegra_emc {
>  
>  	/* protect shared rate-change code path */
>  	struct mutex rate_lock;
> +
> +	struct devfreq_simple_ondemand_data ondemand_data;
> +	struct devfreq *devfreq;
>  };
>  
>  static irqreturn_t tegra_emc_isr(int irq, void *data)
> @@ -952,6 +961,88 @@ static int tegra_emc_opp_table_init(struct tegra_emc *emc)
>  	return err;
>  }
>  
> +static int tegra_emc_devfreq_target(struct device *dev, unsigned long *freq,
> +				    u32 flags)
> +{
> +	struct tegra_emc *emc = dev_get_drvdata(dev);
> +	struct dev_pm_opp *opp;
> +	unsigned long rate;
> +
> +	opp = devfreq_recommended_opp(dev, freq, flags);
> +	if (IS_ERR(opp)) {
> +		dev_err(dev, "failed to find opp for %lu Hz\n", *freq);
> +		return PTR_ERR(opp);
> +	}
> +
> +	rate = dev_pm_opp_get_freq(opp);
> +	dev_pm_opp_put(opp);
> +
> +	return emc_set_min_rate(emc, rate, EMC_RATE_DEVFREQ);
> +}
> +
> +static int tegra_emc_devfreq_get_dev_status(struct device *dev,
> +					    struct devfreq_dev_status *stat)
> +{
> +	struct tegra_emc *emc = dev_get_drvdata(dev);
> +
> +	/* freeze counters */
> +	writel_relaxed(EMC_PWR_GATHER_DISABLE, emc->regs + EMC_STAT_CONTROL);
> +
> +	/*
> +	 * busy_time:  number of clocks EMC request was accepted
> +	 * total_time: number of clocks PWR_GATHER control was set to ENABLE
> +	 */
> +	stat->busy_time = readl_relaxed(emc->regs + EMC_STAT_PWR_COUNT);
> +	stat->total_time = readl_relaxed(emc->regs + EMC_STAT_PWR_CLOCKS);
> +	stat->current_frequency = clk_get_rate(emc->clk);
> +
> +	/* clear counters and restart */
> +	writel_relaxed(EMC_PWR_GATHER_CLEAR, emc->regs + EMC_STAT_CONTROL);
> +	writel_relaxed(EMC_PWR_GATHER_ENABLE, emc->regs + EMC_STAT_CONTROL);
> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile tegra_emc_devfreq_profile = {
> +	.polling_ms	= 30,
> +	.target		= tegra_emc_devfreq_target,
> +	.get_dev_status	= tegra_emc_devfreq_get_dev_status,
> +};
> +
> +static int tegra_emc_devfreq_init(struct tegra_emc *emc)
> +{
> +	int err;
> +
> +	/*
> +	 * PWR_COUNT is 1/2 of PWR_CLOCKS at max, and thus, the up-threshold
> +	 * should be less than 50.  Secondly, multiple active memory clients
> +	 * may cause over 20% of lost clock cycles due to stalls caused by
> +	 * competing memory accesses.  This means that threshold should be
> +	 * set to a less than 30 in order to have a properly working governor.
> +	 */
> +	emc->ondemand_data.upthreshold = 20;
> +
> +	/*
> +	 * Reset statistic gathers state, select global bandwidth for the
> +	 * statistics collection mode and set clocks counter saturation
> +	 * limit to maximum.
> +	 */
> +	writel_relaxed(0x00000000, emc->regs + EMC_STAT_CONTROL);
> +	writel_relaxed(0x00000000, emc->regs + EMC_STAT_LLMC_CONTROL);
> +	writel_relaxed(0xffffffff, emc->regs + EMC_STAT_PWR_CLOCK_LIMIT);
> +
> +	emc->devfreq = devfreq_add_device(emc->dev, &tegra_emc_devfreq_profile,
> +					  DEVFREQ_GOV_SIMPLE_ONDEMAND,
> +					  &emc->ondemand_data);

Do you want to use 'devfreq_add_device' instead of
'devm_devfreq_add_device()'? If you have to use 'devfreq_add_device'
due to some reason, you need to call 'devfreq_remove_device' on exit.

> +	if (IS_ERR(emc->devfreq)) {
> +		err = PTR_ERR(emc->devfreq);
> +		dev_err(emc->dev, "failed to initialize devfreq: %d", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
>  static int tegra_emc_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np;
> @@ -1019,6 +1110,7 @@ static int tegra_emc_probe(struct platform_device *pdev)
>  	tegra_emc_rate_requests_init(emc);
>  	tegra_emc_debugfs_init(emc);
>  	tegra_emc_interconnect_init(emc);
> +	tegra_emc_devfreq_init(emc);
>  
>  	/*
>  	 * Don't allow the kernel module to be unloaded. Unloading adds some
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics