Message-ID: <552D6D34.2030806@kapsi.fi>
Date: Tue, 14 Apr 2015 22:40:36 +0300
From: Mikko Perttunen <mikko.perttunen@kapsi.fi>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0
MIME-Version: 1.0
To: Boris Brezillon <boris.brezillon@free-electrons.com>
CC: Michael Turquette <mturquette@linaro.org>,
        Thierry Reding <thierry.reding@gmail.com>, swarren@wwwdotorg.org,
        gnurou@gmail.com, pdeschrijver@nvidia.com, rjw@rjwysocki.net,
        viresh.kumar@linaro.org, pwalmsley@nvidia.com, vinceh@nvidia.com,
        pgaikwad@nvidia.com, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, linux-tegra@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, tuomas.tynkkynen@iki.fi
Subject: Re: [PATCH v8 00/18] Tegra124 CL-DVFS / DFLL clocksource + cpufreq
References: <1425213881-5262-1-git-send-email-mikko.perttunen@kapsi.fi>	<20150311100741.GK19577@ulmo.nvidia.com>	<20150410211157.14369.51754@quantum>	<552CF947.9030609@kapsi.fi> <20150414192108.6a376ce8@bbrezillon>
In-Reply-To: <20150414192108.6a376ce8@bbrezillon>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3035
Lines: 87

On 04/14/2015 08:21 PM, Boris Brezillon wrote:
> Hi Mikko,
>
> On Tue, 14 Apr 2015 14:25:59 +0300
> Mikko Perttunen <mikko.perttunen@kapsi.fi> wrote:
>
>> On 04/11/2015 12:11 AM, Michael Turquette wrote:
>>> Quoting Thierry Reding (2015-03-11 03:07:43)
>>>> Hi Mike,
>>>>
>>>> Have you had a chance to look at these changes to the Tegra clock
>>>> driver? If you're fine with it, I'd like to take these patches through
>>>> the Tegra tree because the rest of the series depends on them. I can
>>>> provide a stable branch in case we need to base other Tegra clock
>>>> changes on top of this.
>>>
>>> Hi Thierry,
>>>
>>> Clock patches (and corresponding DT binding descriptions and changes to
>>> DTS) look fine to me. Please add:
>>>
>>> Acked-by: Michael Turquette <mturquette@linaro.org>
>>>
>>> I did have a question about the beahvior of clk_put in one of Mikko's
>>> patches but it should not gate this series. I'm just trying to find out
>>> if we have a bug in the framework or if the Tegra driver is a special
>>> case.
>>>
>>> Also I do not think a stable branch is necessary.
>>>
>>> Regards,
>>> Mike
>>>
>>
>> Looks like in the meantime, this has been partially broken by
>> 03bc10ab5b0f "clk: check ->determine/round_rate() return value in
>> clk_calc_new_rates". The highest rates supported by the DFLL clock have
>> 1 in the MSB, so those cannot be entered after the aforementioned patch,
>> as the return value of round_rate is interpreted as an error. Avenues
>> that I can see: 1) revert the above patch 2) restrict the cpu clock rate
>> to those with 0 in the MSB 3) move to 64-bit clock rates.
>
> How about changing ->determine_rate() and ->round_rate() prototypes so
> that they always return 0 or an error code and passing the adjusted_rate
> as an argument ?
>
> Something like that:
>
> 	int (*round_rate)(struct clk_hw *hw, unsigned long *rate,
> 			  unsigned long *parent_rate);
> 	int (*determine_rate)(struct clk_hw *hw, unsigned long *rate,
> 			      unsigned long min_rate,
> 			      unsigned long max_rate,
> 			      unsigned long *best_parent_rate,
> 			      struct clk_hw **best_parent_hw);
>
> I know this implies a lot of changes (in all clock drivers and in the
> core infrastructure), but I really think we should not mix error codes
> and clock frequencies (even if we decide to move to a 64 bits rate
> approach).

This sounds like a good idea, too.

>
> Anyway, IMHO the only alternative to this solution is solution #3,
> because #1 implies re-introducing another bug where
> ->round_rate()/->determine_rate() are silently ignored, and #2 implies
> lying about your DFLL capabilities.
>

Yeah, #1 and #2 weren't really meant as realistic options to end up with :)

> Mike, what's your opinion ?
>
> Best Regards,
>
> Boris
>

Cheers,
Mikko.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/