Quoting Maxime Ripard (2022-01-12 03:46:52) > Hi Stephen, > > Thanks for your answer > > On Tue, Jan 11, 2022 at 07:37:15PM -0800, Stephen Boyd wrote: > > Sorry for being super delayed on response here. I'm buried in other > > work. +Jerome for exclusive clk API. > > > > Quoting Maxime Ripard (2021-09-14 02:35:13) > > > It's not unusual to find clocks being shared across multiple devices > > > that need to change the rate depending on what the device is doing at a > > > given time. > > > > > > The SoC found on the RaspberryPi4 (BCM2711) is in such a situation > > > between its two HDMI controllers that share a clock that needs to be > > > raised depending on the output resolution of each controller. > > > > > > The current clk_set_rate API doesn't really allow to support that case > > > since there's really no synchronisation between multiple users, it's > > > essentially a fire-and-forget solution. > > > > I'd also say a "last caller wins" > > > > > > > > clk_set_min_rate does allow for such a synchronisation, but has another > > > drawback: it doesn't allow to reduce the clock rate once the work is > > > over. > > > > What does "work over" mean specifically? Does it mean one of the clk > > consumers has decided to stop using the clk? > > That, or it doesn't need to enforce that minimum anymore. We have > several cases like this on the RPi. For example, during a change of > display mode a (shared) clock needs to be raised to a minimum, but > another shared one needs to raise its minimum based on the resolution. > > In the former case, we only need the minimum to be enforced during the > resolution change, so it's fairly quick, but the latter requires its > minimum for as long as the display is on. > > > Why doesn't clk_set_rate_range() work? Or clk_set_rate_range() combined > > with clk_set_rate_exclusive()? > > clk_set_rate_range could work (it's what we have right now in mainline > after all), but it's suboptimal since the clock is never scaled down. Alright, I didn't see any mention of clk_set_rate_range() in the commit text so did I miss it? Maybe it's used interchangeably with clk_set_min_rate()? > > It's especially showing in my first example where we need to raise the > clock only for the duration of the resolution change. Using > clk_set_min_rate works but we end up with that fairly high clock (at > least 500MHz) for the rest of the system life even though we usually can > get away with using a clock around 200MHz outside of that (short) window. > > This is fairly inefficient, and is mostly what I'm trying to address. Got it! > > > > In our previous example, this means that if we were to raise the > > > resolution of one HDMI controller to the largest resolution and then > > > changing for a smaller one, we would still have the clock running at the > > > largest resolution rate resulting in a poor power-efficiency. > > > > Does this example have two HDMI controllers where they share one clk and > > want to use the most efficient frequency for both of the HDMI devices? I > > think I'm following along but it's hard. It would be clearer if there > > was some psuedo-code explaining how it is both non-workable with current > > APIs and workable with the new APIs. > > The fact that we have two HDMI controllers that share one clock is why > we use clk_set_min_rate in the first place, but you can have that > behavior with clk_set_min_rate only with a single user. > > With pseudo-code, if you do something like > > clk = clk_get(NULL); > clk_set_min_rate(600 * 1000 * 1000); > clk_set_min_rate(1000); > > The clock will still remain at 600MHz, even though you would be totally > fine with the clock running at 1kHz. That looks like a bug. While we could happily ignore the rate floor being lowered because we're still within constraints, it looks like we should always re-evaluate the constraints when they change. > > If you really wanted to make the clock run at 1kHz, you'd need to have: > > clk = clk_get(NULL); > clk_set_min_rate(600 * 1000 * 1000); > clk_set_min_rate(1000); > clk_set_rate(1000); > > And that works fine for a single user. > > If you have a clock shared by multiple drivers though, things get > tricky. Indeed, you can't really find out what the minimum for that > clock is, so figuring out the rate to pass to the clk_set_rate call > would be difficult already. And it wouldn't be atomic anyway. Right. > > It's made even more difficult since in clk_calc_new_rates the core > checks that the rate is within the boundaries and will error out if it > isn't, so even using clk_set_rate(0) wouldn't work. clk_set_rate(0) is pretty gross! > > It could work if the clock driver makes sure in round/determine_rate > that the rate passed in within the boundaries of the clock, but then you > start working around the core and relying on the behavior of clock > drivers, which is a fairly significant abstraction violation. > > > > In order to address both issues, let's create an API that allows user to > > > create temporary requests to increase the rate to a minimum, before > > > going back to the initial rate once the request is done. > > > > > > This introduces mainly two side-effects: > > > > > > * There's an interaction between clk_set_rate and requests. This has > > > been addressed by having clk_set_rate increasing the rate if it's > > > greater than what the requests asked for, and in any case changing > > > the rate the clock will return to once all the requests are done. > > > > > > * Similarly, clk_round_rate has been adjusted to take the requests > > > into account and return a rate that will be greater or equal to the > > > requested rates. > > > > > > > I believe clk_set_rate_range() is broken but it can be fixed. I'm > > forgetting the details though. If the intended user of this new API > > can't use that range API then it would be good to understand why it > > can't be used. I imagine it would be something like > > > > struct clk *clk_hdmi1, *clk_hdmi2; > > > > clk_set_rate_range(&clk_hdmi1, HDMI1_MIN, HDMI1_MAX); > > clk_set_rate_range(&clk_hdmi2, HDMI2_MIN, HDMI2_MAX); > > clk_set_rate_range(&clk_hdmi2, 0, UINT_MAX); > > > > and then the goal would be for HDMI1_MIN to be used, or at the least for > > the last call to clk_set_rate_range() to drop the rate constraint and > > re-evaluate the frequency of the clk again based on hdmi1's rate range. > > This is pretty much what this series was doing. I was being conservative > and didn't really want to modify the behavior of existing functions, but > that will work fine. > I don't see a problem with re-evaluating the rate every time we call clk_set_rate_range(). That's probably the bug that I can't recall. Can you fix the API so it works that way?
