Re: [PATCH v9 01/17] memory: tegra30: Support interconnect framework

From: Dmitry Osipenko <digetx@gmail.com>
To: Georgi Djakov <georgi.djakov@linaro.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	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>,
	Chanwoo Choi <cw00.choi@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-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v9 01/17] memory: tegra30: Support interconnect framework
Date: Wed, 18 Nov 2020 01:02:33 +0300	[thread overview]
Message-ID: <7e484678-43cc-e612-1017-73ed580f9840@gmail.com> (raw)
In-Reply-To: <61e777d9-b730-02c6-cedf-cf0aa1a50fb8@linaro.org>

17.11.2020 23:24, Georgi Djakov пишет:
> Hi Dmitry,
> 
> Thank you working on this!
> 
> On 15.11.20 23:29, Dmitry Osipenko wrote:
>> Now Internal and External memory controllers are memory interconnection
>> providers. This allows us to use interconnect API for tuning of memory
>> configuration. EMC driver now supports OPPs and DVFS. MC driver now
>> supports tuning of memory arbitration latency, which needs to be done
>> for ISO memory clients, like a Display client for example.
>>
>> Tested-by: Peter Geis <pgwipeout@gmail.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>   drivers/memory/tegra/Kconfig       |   1 +
>>   drivers/memory/tegra/tegra30-emc.c | 349 +++++++++++++++++++++++++++--
>>   drivers/memory/tegra/tegra30.c     | 173 +++++++++++++-
>>   3 files changed, 501 insertions(+), 22 deletions(-)
>>
> [..]> diff --git a/drivers/memory/tegra/tegra30.c
> b/drivers/memory/tegra/tegra30.c
>> index d0314f29608d..ea849003014b 100644
>> --- a/drivers/memory/tegra/tegra30.c
>> +++ b/drivers/memory/tegra/tegra30.c
> [..]
>> +
>> +static int tegra30_mc_icc_set(struct icc_node *src, struct icc_node
>> *dst)
>> +{
>> +    struct tegra_mc *mc = icc_provider_to_tegra_mc(src->provider);
>> +    const struct tegra_mc_client *client = &mc->soc->clients[src->id];
>> +    u64 peak_bandwidth = icc_units_to_bps(src->peak_bw);
>> +
>> +    /*
>> +     * Skip pre-initialization that is done by icc_node_add(), which
>> sets
>> +     * bandwidth to maximum for all clients before drivers are loaded.
>> +     *
>> +     * This doesn't make sense for us because we don't have drivers
>> for all
>> +     * clients and it's okay to keep configuration left from bootloader
>> +     * during boot, at least for today.
>> +     */
>> +    if (src == dst)
>> +        return 0;
> 
> Nit: The "proper" way to express this should be to implement the
> .get_bw() callback to return zero as initial average/peak bandwidth.
> I'm wondering if this will work here?
> 
> The rest looks good to me!

Hello Georgi,

Returning zeros doesn't allow us to skip the initialization that is done
by provider->set(node, node) in icc_node_add(). It will reconfigure
memory latency in accordance to a zero memory bandwidth, which is wrong
to do.

It actually should be more preferred to preset bandwidth to a maximum
before all drivers are synced, but this should be done only once we will
wire up all drivers to use ICC framework. For now it's safer to keep the
default hardware configuration untouched.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel