Re: (EXT) [PATCH 00/22] clk: More clock rate fixes and tests

From: Alexander Stein <alexander.stein@ew.tq-group.com>
To: Maxime Ripard <maxime@cerno.tech>, Maxime Ripard <maxime@cerno.tech>
Cc: Mike Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	linux-clk@vger.kernel.org,
	Naresh Kamboju <naresh.kamboju@linaro.org>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Tony Lindgren <tony@atomide.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Yassine Oudjana <y.oudjana@protonmail.com>,
	Neil Armstrong <narmstrong@baylibre.com>
Subject: Re: (EXT) [PATCH 00/22] clk: More clock rate fixes and tests
Date: Mon, 11 Apr 2022 08:25:31 +0200	[thread overview]
Message-ID: <11958147.O9o76ZdvQC@steina-w> (raw)
In-Reply-To: <20220408091037.2041955-1-maxime@cerno.tech>

Hello Maxime,

Am Freitag, 8. April 2022, 11:10:15 CEST schrieb Maxime Ripard:
> Hi,
> 
> Thanks to the feedback I got on the previous series, I found and fixed a
> number of bugs in the clock framework and how it deals with rates,
> especially when it comes to orphan clocks.
> 
> In order to make sure this doesn't pop up again as a regression, I've
> extended the number of tests.
> 
> The first patch reintroduces the clk_set_rate_range call on clk_put, but
> this time will only do so if there was a range set on that clock to
> begin with. It should be less intrusive, and reduce the number of
> potential side effects considerably.
> 
> All the other patches should be probably be flagged as fixes, but
> they've never seem to have shown any real-world issues until now, and
> they aren't all really trivial to backport either, so I'm not sure it's
> worth it.
> 
> The last patch will probably prove to be controversial, but it can be
> left out without affecting the rest of the series. It will affect the
> meson clock drivers for the g12b SoC at least. It stems from the
> realisation that on the g12b, 4 PLLs (and thus all their children) have
> a rate of 0, pretty much forever which feels like a bug considering the
> rate 0 is used as an error in most places.
> 
> Those 4 PLLs have a rate of 0 because meson_clk_pll_recalc_rate will
> return 0 if the diviser of the PLL is set to 0 in the register, but:
> 
>   - pcie_pll_dco has a few registers to initialize set in
>     g12a_pcie_pll_dco, but meson_clk_pcie_pll_ops don't set the init
>     hook and will instead call it in the enable hook. This looks like a
>     bug and could be easily fixed by adding that init hook.
> 
>   - gp0_pll_dco and hifi_pll_dco both don't set any of there n field in
>     the initialisation of their register (g12a_gp0_init_regs and
>     g12a_hifi_init_regs). So if the bootloader doesn't set it, and as
>     long as set_rate isn't called, that field is going to remain at 0. And
>     since all but one users don't have CLK_SET_RATE_PARENT, this is
>     still fairly unlikely.
> 
>   - hdmi_pll_dco doesn't set the n field in the initialisation either,
>     but also doesn't have a set_rate implementation. Thus, if the
>     bootloader doesn't set it, this clock and all its children will
>     always report a rate of 0, even if the clock is functional.
> 
> During the discussion with amlogic clock maintainers, we kind of ended
> up on a disagreement of whether returning 0 was ok or not, hence the
> expected controversy :)
> 
> Let me know what you think,
> Maxime
> 
> Maxime Ripard (22):
>   clk: Drop the rate range on clk_put()
>   clk: tests: Add test suites description
>   clk: tests: Add reference to the orphan mux bug report
>   clk: tests: Add tests for uncached clock
>   clk: tests: Add tests for single parent mux
>   clk: tests: Add tests for mux with multiple parents
>   clk: tests: Add some tests for orphan with multiple parents
>   clk: Take into account uncached clocks in clk_set_rate_range()
>   clk: Fix clk_get_parent() documentation
>   clk: Set req_rate on reparenting
>   clk: Skip set_rate_range if our clock is orphan
>   clk: Add our request boundaries in clk_core_init_rate_req
>   clk: Change clk_core_init_rate_req prototype
>   clk: Introduce clk_hw_init_rate_request()
>   clk: Add missing clk_core_init_rate_req calls
>   clk: Remove redundant clk_core_init_rate_req() call
>   clk: Switch from __clk_determine_rate to clk_core_round_rate_nolock
>   clk: Introduce clk_core_has_parent()
>   clk: Stop forwarding clk_rate_requests to the parent
>   clk: Zero the clk_rate_request structure
>   clk: Test the clock pointer in clk_hw_get_name()
>   clk: Prevent a clock without a rate to register
> 
>  drivers/clk/clk.c            |  239 +++++--
>  drivers/clk/clk_test.c       | 1256 +++++++++++++++++++++++++++++++++-
>  include/linux/clk-provider.h |    6 +
>  include/linux/clk.h          |    5 +-
>  4 files changed, 1439 insertions(+), 67 deletions(-)

Thanks for another round of patches.
On top my patchset this one does *not* break my imx8mp based setup. Booting is 
as usual without any changes in dmesg.
Given this patch set I suspect the other one on github you sent me on Thursday 
is obsolete now.

Best regards,
Alexander