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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

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/