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 v11 04/10] memory: tegra124-emc: Make driver modular
To:     Thierry Reding <thierry.reding@gmail.com>
Cc:     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>,
        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>,
        linux-tegra@vger.kernel.org, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        devicetree@vger.kernel.org
References: <20201203192439.16177-1-digetx@gmail.com>
 <20201203192439.16177-5-digetx@gmail.com> <X8pmqVRFaBtkwDtr@ulmo>
From:   Dmitry Osipenko <digetx@gmail.com>
Message-ID: <76eb57bf-feaf-ad86-a2d6-152897af7f2f@gmail.com>
Date:   Sat, 5 Dec 2020 22:52:38 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.2
MIME-Version: 1.0
In-Reply-To: <X8pmqVRFaBtkwDtr@ulmo>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

04.12.2020 19:41, Thierry Reding пишет:
...
>> +bool tegra124_clk_emc_driver_available(struct clk_hw *hw)
>> +{
>> +	struct tegra_clk_emc *tegra = container_of(hw, struct tegra_clk_emc, hw);
>> +
>> +	return tegra->prepare_timing_change && tegra->complete_timing_change;
>> +}
> 
> This looks a bit hackish and I prefer the way this was done for
> Tegra210.

I may have an opposite opinion :)

> But that's mostly an implementation detail and we can always
> restructure this if we want to.

This is true. I'm not saying that the current v11 variant is absolutely
ideal, but it should be good enough for the starter (IMO) and actually I
don't have any ideas right about what could be done better.

>> diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
>> index e931319dcc9d..934520aab6e3 100644
>> --- a/drivers/clk/tegra/clk-tegra124.c
>> +++ b/drivers/clk/tegra/clk-tegra124.c
>> @@ -1500,6 +1500,26 @@ static void __init tegra124_132_clock_init_pre(struct device_node *np)
>>  	writel(plld_base, clk_base + PLLD_BASE);
>>  }
>>  
>> +static struct clk *tegra124_clk_src_onecell_get(struct of_phandle_args *clkspec,
>> +						void *data)
>> +{
>> +	struct clk_hw *hw;
>> +	struct clk *clk;
>> +
>> +	clk = of_clk_src_onecell_get(clkspec, data);
>> +	if (IS_ERR(clk))
>> +		return clk;
>> +
>> +	hw = __clk_get_hw(clk);
>> +
>> +	if (clkspec->args[0] == TEGRA124_CLK_EMC) {
>> +		if (!tegra124_clk_emc_driver_available(hw))
>> +			return ERR_PTR(-EPROBE_DEFER);
>> +	}
>> +
>> +	return clk;
>> +}
> 
> Hm... why exactly do we need this? On Tegra210 and later, the EMC driver
> is the only consumer of the EMC clock and since it also provides some of
> the necessary parts to scale the EMC clock, that's a chicken and egg
> problem.

The T124 EMC driver has an existing active user for the EMC clock, the
devfreq/actmon driver which watches and drives the EMC clock rate. The
EMC clock shan't be requested by the devfreq driver until EMC driver is
ready, the only sensible way to achieve this is implemented by this patch.

The devfreq driver doesn't support T210 (yet?) and you should witness
the problem if you'll try to implement the T210 support.

> I'm not sure I fully understand how this is supposed to work
> here and why we can't do this in a similar way than Tegra210.

The CCF returns -EPROBE_DEFER for clk_get() only until clock provider is
registered, otherwise it returns a dummy/stub clock once provider is
available and clk (of the provider) isn't registered. The CCF provider
for the EMC clock is the tegra-clk driver, not the EMC driver.

Once clk_get() is invoked by a clk user, the CCF performs the clk lookup
using the DT specifier and this lookup is aborted with a -EPROBE_DEFER
from the clk_src_onecell_get() callback if EMC driver isn't loaded yet.
I don't think that there are any other variants to achieve this behaviour.

I also prefer to have a clean separation of the clk and EMC drivers
because this is a much more expressive variant than mixing drivers
together in obscure way. The pre-T210 EMC drivers don't need to touch
clk registers for programming of the memory timings, hence those EMC
drivers are in a bit better position than the T210 driver.

The T210 EMC driver also could have a cleaner separation by using a
special tegra-clk API for the clk/EMC functions, instead of shoving a
raw clk IO pointer to the EMC driver. It feels like I was already
suggesting this about a half-year ago, before the T210 driver was merged.