[PATCH 00/22] clk: More clock rate fixes and tests

Maxime Ripard
2022-04-08T09:13:26Z

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(-)

--
2.35.1