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

Alexander Stein
2022-04-11T07:24:32Z

Am Montag, 11. April 2022, 08:25:31 CEST schrieb Alexander Stein:
> 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.

I have to correct myself here a bit. While using my current board .dts fpr
imx8mp works with this patchset, things change if I apply Lucas' HDMI patchset
[1] and enable HDMI devices. In this case I get the following errors:
> hdmi_pclk: recalc_rate returned a null rate
> fsl-samsung-hdmi-phy 32fdff00.phy: error -EINVAL: failed to register clock
> fsl-samsung-hdmi-phy 32fdff00.phy: register clk failed
> fsl-samsung-hdmi-phy: probe of 32fdff00.phy failed with error -22

The offending patch is the last one 'clk: Prevent a clock without a rate to
register'.

Best regards,
Alexander

[1] https://patchwork.kernel.org/project/linux-arm-kernel/cover/
20220406160123.1272911-1-l.stach@pengutronix.de/