On Mon, Mar 09, 2020 at 06:48:47PM -0700, Stephen Boyd wrote: > Quoting Thierry Reding (2020-03-05 09:51:38) > > From: Thierry Reding <treding@xxxxxxxxxx> > > > > As part of the clock frequency change sequence, a driver may need to > > reparent a clock. In that case, the rate will already have been updated > > and the cached parent rate will no longer be valid, so just skip the > > recalculation. > > Can you describe more about what's going on and why this needs to > change? For example, we have 'set_rate_and_parent' for cases where a > driver reparents a clk during a rate change. Is that not sufficient > here? By "driver" above, I mean clock driver. One example where we need this, and currently work around it, is the Tegra124 EMC (external memory controller) clock driver. You can find this here: drivers/clk/clk-emc.c If you look at the emc_recalc_rate() function, it needs to override the parent clock rate because CCF calls it with the clock rate of the parent prior to the rate change, whereas the parent (and/or the rate) potentially change during the rate change. The reason why we can't use ->set_rate_and_parent() here is because the consumers never actually change the parent for the EMC clock. Instead it is up to the EMC driver to select the parent based on the specific rate that is requested. The parent clock is encoded in a table of supported EMC frequencies that a particular board can support. This table is passed to the EMC driver from the firmware or hardcoded in DT, depending on the particular SoC generation. For a bit more background: the use-case is to accumulate a set of bandwidth requests within the EMC driver, which can then be converted into a clock frequency that is required for the EMC to provide that bandwidth. Since the controls for this are sprinkled around a bit, the nicest way to represent this is using an "emc" clock (in the clock and reset controller) and the EMC driver that deals with programming the EMC (which has registers separate from the clock and reset controller). So basically how this works is that the "emc" clock driver needs to call back into the EMC driver to perform the clock change. This being a clock that is needed to operate system memory, we have to be very careful when things happen. So a frequency switch typically requires the old parent to remain enabled while the EMC is programmed with new settings and then switched to the new parent. Only then can the old parent be disabled. The parent change happens fairly deep inside the EMC sequence and can't be isolated from that, unfortunately. In order to deal with that, we basically "fixup" the clock tree after the frequency change by calling clk_hw_reparent() manually. This in turn causes the parent to change, but clk_set_rate() doesn't know about that. Instead, it has the parent rate cached in a local variable, which will no longer be the correct value after the switch has happened. Fortunately, clk_hw_reparent() already causes the rates of the entire subtree of clocks to be recalculated, so there isn't anything left to do when we return from the rate change. This patch detects when this has happened by checking that the current parent against the "cached" parent that the cached parent rate is based on. If they aren't equal, it assumes that the clock driver has manually reparented the clock and does not have to do anything else. Does that clarify things? Thierry > > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx> > > --- > > drivers/clk/clk.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index ebfc1e2103cb..49d92f4785a2 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -2079,7 +2079,14 @@ static void clk_change_rate(struct clk_core *core) > > > > trace_clk_set_rate_complete(core, core->new_rate); > > > > - core->rate = clk_recalc(core, best_parent_rate); > > + /* > > + * Some drivers need to change the parent of a clock as part of the > > + * rate change sequence. In that case, best_parent_rate is no longer > > + * valid. However, reparenting already recalculates the rate for the > > + * entire clock subtree, so we can safely skip this here. > > + */ > > + if (core->parent == parent) > > + core->rate = clk_recalc(core, best_parent_rate); > > > > I wonder if we can undo the recursion and figure out a way to make this > only happen once, when we want it to happen.
Attachment:
signature.asc
Description: PGP signature
