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

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 4631 bytes --]

Hi Yassine,

On Sun, Apr 10, 2022 at 12:06:55PM +0000, Yassine Oudjana wrote:
> 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.

Could you test without the last patch?

Thanks,
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]