Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

From: Marek Szyprowski <m.szyprowski@samsung.com>
To: Maxime Ripard <maxime@cerno.tech>
Cc: Jerome Brunet <jbrunet@baylibre.com>,
	Mike Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	linux-clk@vger.kernel.org,
	Naresh Kamboju <naresh.kamboju@linaro.org>,
	Alexander Stein <alexander.stein@ew.tq-group.com>,
	Tony Lindgren <tony@atomide.com>,
	Yassine Oudjana <y.oudjana@protonmail.com>,
	Neil Armstrong <narmstrong@baylibre.com>
Subject: Re: [PATCH 22/22] clk: Prevent a clock without a rate to register
Date: Fri, 8 Apr 2022 15:46:53 +0200	[thread overview]
Message-ID: <34cc8927-3c63-a85e-1fd1-e72f66ada6de@samsung.com> (raw)
In-Reply-To: <20220408122542.jymybls42w3uzxpc@houat>

Hi Maxime,

On 08.04.2022 14:25, Maxime Ripard wrote:
> On Fri, Apr 08, 2022 at 02:17:55PM +0200, Marek Szyprowski wrote:
>> On 08.04.2022 11:18, Jerome Brunet wrote:
>>> On Fri 08 Apr 2022 at 11:10, Maxime Ripard <maxime@cerno.tech> wrote:
>>>
>>>> A rate of 0 for a clock is considered an error, as evidenced by the
>>>> documentation of clk_get_rate() and the code of clk_get_rate() and
>>>> clk_core_get_rate_nolock().
>>>>
>>>> The main source of that error is if the clock is supposed to have a
>>>> parent but is orphan at the moment of the call. This is likely to be
>>>> transient and solved later in the life of the system as more clocks are
>>>> registered.
>>>>
>>>> The corollary is thus that if a clock is not an orphan, has a parent that
>>>> has a rate (so is not an orphan itself either) but returns a rate of 0,
>>>> something is wrong in the driver. Let's return an error in such a case.
>>>>
>>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>>>> ---
>>>>    drivers/clk/clk.c | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>>> index 8bbb6adeeead..e8c55678da85 100644
>>>> --- a/drivers/clk/clk.c
>>>> +++ b/drivers/clk/clk.c
>>>> @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
>>>>    		rate = 0;
>>>>    	core->rate = core->req_rate = rate;
>>>>    
>>>> +	/*
>>>> +	 * If we're not an orphan clock and our parent has a rate, then
>>>> +	 * if our rate is 0, something is badly broken in recalc_rate.
>>>> +	 */
>>>> +	if (!core->orphan && (parent && parent->rate) && !core->rate) {
>>>> +		ret = -EINVAL;
>>>> +		pr_warn("%s: recalc_rate returned a null rate\n", core->name);
>>>> +		goto out;
>>>> +	}
>>>> +
>>> As hinted in the cover letter, I don't really agree with that.
>>>
>>> There are situations where we can't compute the rate. Getting invalid
>>> value in the register is one reason.
>>>
>>> You mentioned the PLLs of the Amlogic SoCs (it is not limited to g12 - all
>>> SoCs would be affected):
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/clk-pll.c#n82
>>> Yes, PLL that have not been previously used (by the ROMCode or the
>>> bootloader) tend to have the value of the divider set to 0 which in
>>> invalid as it would result in a division by zero.
>>>
>>> I don't think this is a bug. It is just what the HW is, an unlocked,
>>> uninitialized PLL. There is no problem here and the PLL can remain like
>>> that until it is needed.
>>>
>>> IMO, whenever possible we should not put default values in the clocks
>>> which is why I chose to leave it like that.
>>>
>>> The PLL being unlocked, it has no rate. It is not set to have any rate.
>>> IMO a returning a rate of 0 is valid here.
>> I agree that it should be possible to register so called unconfigured
>> clock, which doesn't have any rate set yet. It might be used later by
>> some drivers, which will do the whole initialization (set rate and or
>> parents).
> I think we should differentiate between the clock being fine from the
> CCF point of view, and from the device point of view. I'm arguing about
> the former, but the latter should definitely be up to other drivers to
> set it up so that their hardware work.
>
> But in both cases, we should return something valid under the CCF
> semantics.
>
>> I also wonder why the above patch triggers warning on the Amlogic
>> Socs, while on Exynos I still have some unused clocks with 0 rate
>> registered properly.
> Most likely the clocks that return a rate of 0 are orphans (ie, their
> parent isn't registered yet).
Right.
> We can't expect all the parents to be registered before their children,
> so this is fine, and that patch doesn't break it.
>
> Did you test this series for exynos and meson then? Could you give your
> tested-by if you did? :)

Yes, I've tested the whole set. All my Exynos based test boards work 
fine. All my Meson based boards fails to boot due to this patch, thus my 
comment. Feel free to add to all but the last patch:

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland