| Message ID | 20220125141549.747889-1-maxime@cerno.tech (mailing list archive) |
|---|---|
| Headers | show |
| Series | clk: Improve clock range handling | expand |
Hi Stephen, On Tue, Jan 25, 2022 at 03:15:39PM +0100, Maxime Ripard wrote: > Hi, > > This is a follow-up of the discussion here: > https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/ > > and here: > https://lore.kernel.org/all/20210914093515.260031-1-maxime@cerno.tech/ > > While the initial proposal implemented a new API to temporarily raise and lower > clock rates based on consumer workloads, Stephen suggested an > alternative approach implemented here. > > The main issue that needed to be addressed in our case was that in a > situation where we would have multiple calls to clk_set_rate_range, we > would end up with a clock at the maximum of the minimums being set. This > would be expected, but the issue was that if one of the users was to > relax or drop its requirements, the rate would be left unchanged, even > though the ideal rate would have changed. > > So something like > > clk_set_rate(user1_clk, 1000); > clk_set_min_rate(user1_clk, 2000); > clk_set_min_rate(user2_clk, 3000); > clk_set_min_rate(user2_clk, 1000); > > Would leave the clock running at 3000Hz, while the minimum would now be > 2000Hz. > > This was mostly due to the fact that the core only triggers a rate > change in clk_set_rate_range() if the current rate is outside of the > boundaries, but not if it's within the new boundaries. > > That series changes that and will trigger a rate change on every call, > with the former rate being tried again. This way, providers have a > chance to follow whatever policy they see fit for a given clock each > time the boundaries change. > > This series also implements some kunit tests, first to test a few rate > related functions in the CCF, and then extends it to make sure that > behaviour has some test coverage. As far as I know, this should address any concern you had with the previous iterations. Is there something else you'd like to see fixed/improved? Maxime
Hi Maxime and Stephen, We have recently posted a driver for the BCM2711 Unicam CSI-2 receiver (see [1]) which is a perfect candidate for this API, as it needs a minimum rate for the VPU clock. Any chance we can get this series merged ? :-) [1] https://lore.kernel.org/linux-media/20220208155027.891055-1-jeanmichel.hautbois@ideasonboard.com On Tue, Jan 25, 2022 at 03:15:39PM +0100, Maxime Ripard wrote: > Hi, > > This is a follow-up of the discussion here: > https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/ > > and here: > https://lore.kernel.org/all/20210914093515.260031-1-maxime@cerno.tech/ > > While the initial proposal implemented a new API to temporarily raise and lower > clock rates based on consumer workloads, Stephen suggested an > alternative approach implemented here. > > The main issue that needed to be addressed in our case was that in a > situation where we would have multiple calls to clk_set_rate_range, we > would end up with a clock at the maximum of the minimums being set. This > would be expected, but the issue was that if one of the users was to > relax or drop its requirements, the rate would be left unchanged, even > though the ideal rate would have changed. > > So something like > > clk_set_rate(user1_clk, 1000); > clk_set_min_rate(user1_clk, 2000); > clk_set_min_rate(user2_clk, 3000); > clk_set_min_rate(user2_clk, 1000); > > Would leave the clock running at 3000Hz, while the minimum would now be > 2000Hz. > > This was mostly due to the fact that the core only triggers a rate > change in clk_set_rate_range() if the current rate is outside of the > boundaries, but not if it's within the new boundaries. > > That series changes that and will trigger a rate change on every call, > with the former rate being tried again. This way, providers have a > chance to follow whatever policy they see fit for a given clock each > time the boundaries change. > > This series also implements some kunit tests, first to test a few rate > related functions in the CCF, and then extends it to make sure that > behaviour has some test coverage. > > Let me know what you think > Maxime > > Changes from v3: > - Renamed the test file and Kconfig option > - Add option to .kunitconfig > - Switch to kunit_kzalloc > - Use KUNIT_EXPECT_* instead of KUNIT_ASSERT_* where relevant > - Test directly relevant calls instead of going through a temporary variable > - Switch to more precise KUNIT_ASSERT_* macros where relevant > > Changes from v2: > - Rebased on current next > - Rewrote the whole thing according to Stephen reviews > - Implemented some kunit tests > > Changes from v1: > - Return NULL in clk_request_start if clk pointer is NULL > - Test for clk_req pointer in clk_request_done > - Add another user in vc4 > - Rebased on top of v5.15-rc1 > > Maxime Ripard (10): > clk: Introduce Kunit Tests for the framework > clk: Always clamp the rounded rate > clk: Use clamp instead of open-coding our own > clk: Always set the rate on clk_set_range_rate > clk: Add clk_drop_range > clk: bcm: rpi: Add variant structure > clk: bcm: rpi: Set a default minimum rate > clk: bcm: rpi: Run some clocks at the minimum rate allowed > drm/vc4: Add logging and comments > drm/vc4: hdmi: Remove clock rate initialization > > drivers/clk/.kunitconfig | 1 + > drivers/clk/Kconfig | 7 + > drivers/clk/Makefile | 1 + > drivers/clk/bcm/clk-raspberrypi.c | 125 +++++- > drivers/clk/clk-test.c | 621 ++++++++++++++++++++++++++++++ > drivers/clk/clk.c | 51 ++- > drivers/gpu/drm/vc4/vc4_hdmi.c | 13 - > drivers/gpu/drm/vc4/vc4_kms.c | 11 + > include/linux/clk.h | 11 + > 9 files changed, 786 insertions(+), 55 deletions(-) > create mode 100644 drivers/clk/clk-test.c
Quoting Laurent Pinchart (2022-02-14 01:45:56) > Hi Maxime and Stephen, > > We have recently posted a driver for the BCM2711 Unicam CSI-2 receiver > (see [1]) which is a perfect candidate for this API, as it needs a > minimum rate for the VPU clock. Any chance we can get this series merged > ? :-) The rate range API already exists, but it's busted. I can see you like the patch series, can you provide any Reviewed-by or Tested-by tags?
Quoting Maxime Ripard (2022-02-10 02:19:16) > Hi Stephen, > > On Tue, Jan 25, 2022 at 03:15:39PM +0100, Maxime Ripard wrote: > > Hi, > > > > This is a follow-up of the discussion here: > > https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/ > > > > and here: > > https://lore.kernel.org/all/20210914093515.260031-1-maxime@cerno.tech/ > > > > While the initial proposal implemented a new API to temporarily raise and lower > > clock rates based on consumer workloads, Stephen suggested an > > alternative approach implemented here. > > > > The main issue that needed to be addressed in our case was that in a > > situation where we would have multiple calls to clk_set_rate_range, we > > would end up with a clock at the maximum of the minimums being set. This > > would be expected, but the issue was that if one of the users was to > > relax or drop its requirements, the rate would be left unchanged, even > > though the ideal rate would have changed. > > > > So something like > > > > clk_set_rate(user1_clk, 1000); > > clk_set_min_rate(user1_clk, 2000); > > clk_set_min_rate(user2_clk, 3000); > > clk_set_min_rate(user2_clk, 1000); > > > > Would leave the clock running at 3000Hz, while the minimum would now be > > 2000Hz. > > > > This was mostly due to the fact that the core only triggers a rate > > change in clk_set_rate_range() if the current rate is outside of the > > boundaries, but not if it's within the new boundaries. > > > > That series changes that and will trigger a rate change on every call, > > with the former rate being tried again. This way, providers have a > > chance to follow whatever policy they see fit for a given clock each > > time the boundaries change. > > > > This series also implements some kunit tests, first to test a few rate > > related functions in the CCF, and then extends it to make sure that > > behaviour has some test coverage. > > As far as I know, this should address any concern you had with the > previous iterations. > > Is there something else you'd like to see fixed/improved? Looks much improved. Some minor nits and requests for more test cases. I hope we can merge it next week or so. I'll be on the lookout for the next round.
Hi Stephen, (CC'ing Jean-Michel) On Fri, Feb 18, 2022 at 06:24:08PM -0800, Stephen Boyd wrote: > Quoting Laurent Pinchart (2022-02-14 01:45:56) > > Hi Maxime and Stephen, > > > > We have recently posted a driver for the BCM2711 Unicam CSI-2 receiver > > (see [1]) which is a perfect candidate for this API, as it needs a > > minimum rate for the VPU clock. Any chance we can get this series merged > > ? :-) > > The rate range API already exists, but it's busted. I can see you like > the patch series, can you provide any Reviewed-by or Tested-by tags? Jean-Michel, as you're working on upstreaming the RPi Unicam driver which is our use case for this API, could you test this patch series ?