04.12.2020 19:41, Thierry Reding пишет: ... >> +bool tegra124_clk_emc_driver_available(struct clk_hw *hw) >> +{ >> + struct tegra_clk_emc *tegra = container_of(hw, struct tegra_clk_emc, >> hw); >> + >> + return tegra->prepare_timing_change && tegra->complete_timing_change; >> +} > > This looks a bit hackish and I prefer the way this was done for > Tegra210.
I may have an opposite opinion :) > But that's mostly an implementation detail and we can always > restructure this if we want to. This is true. I'm not saying that the current v11 variant is absolutely ideal, but it should be good enough for the starter (IMO) and actually I don't have any ideas right about what could be done better. >> diff --git a/drivers/clk/tegra/clk-tegra124.c >> b/drivers/clk/tegra/clk-tegra124.c >> index e931319dcc9d..934520aab6e3 100644 >> --- a/drivers/clk/tegra/clk-tegra124.c >> +++ b/drivers/clk/tegra/clk-tegra124.c >> @@ -1500,6 +1500,26 @@ static void __init tegra124_132_clock_init_pre(struct >> device_node *np) >> writel(plld_base, clk_base + PLLD_BASE); >> } >> >> +static struct clk *tegra124_clk_src_onecell_get(struct of_phandle_args >> *clkspec, >> + void *data) >> +{ >> + struct clk_hw *hw; >> + struct clk *clk; >> + >> + clk = of_clk_src_onecell_get(clkspec, data); >> + if (IS_ERR(clk)) >> + return clk; >> + >> + hw = __clk_get_hw(clk); >> + >> + if (clkspec->args[0] == TEGRA124_CLK_EMC) { >> + if (!tegra124_clk_emc_driver_available(hw)) >> + return ERR_PTR(-EPROBE_DEFER); >> + } >> + >> + return clk; >> +} > > Hm... why exactly do we need this? On Tegra210 and later, the EMC driver > is the only consumer of the EMC clock and since it also provides some of > the necessary parts to scale the EMC clock, that's a chicken and egg > problem. The T124 EMC driver has an existing active user for the EMC clock, the devfreq/actmon driver which watches and drives the EMC clock rate. The EMC clock shan't be requested by the devfreq driver until EMC driver is ready, the only sensible way to achieve this is implemented by this patch. The devfreq driver doesn't support T210 (yet?) and you should witness the problem if you'll try to implement the T210 support. > I'm not sure I fully understand how this is supposed to work > here and why we can't do this in a similar way than Tegra210. The CCF returns -EPROBE_DEFER for clk_get() only until clock provider is registered, otherwise it returns a dummy/stub clock once provider is available and clk (of the provider) isn't registered. The CCF provider for the EMC clock is the tegra-clk driver, not the EMC driver. Once clk_get() is invoked by a clk user, the CCF performs the clk lookup using the DT specifier and this lookup is aborted with a -EPROBE_DEFER from the clk_src_onecell_get() callback if EMC driver isn't loaded yet. I don't think that there are any other variants to achieve this behaviour. I also prefer to have a clean separation of the clk and EMC drivers because this is a much more expressive variant than mixing drivers together in obscure way. The pre-T210 EMC drivers don't need to touch clk registers for programming of the memory timings, hence those EMC drivers are in a bit better position than the T210 driver. The T210 EMC driver also could have a cleaner separation by using a special tegra-clk API for the clk/EMC functions, instead of shoving a raw clk IO pointer to the EMC driver. It feels like I was already suggesting this about a half-year ago, before the T210 driver was merged.