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

Yassine Oudjana
2022-04-10T12:07:06Z

On Friday, April 8th, 2022 at 1:10 PM, Maxime Ripard <maxime@cerno.tech> wrote:
> 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

It appears that this series breaks mmcc-msm8996:

[ 8.713810] mmpll2_early: recalc_rate returned a null rate
[ 8.713864] mmcc-msm8996: probe of 8c0000.clock-controller failed with error -22

I haven't yet tried to figure out the exact patch that causes
this or how it does. I just wanted to report it as soon as possible.