On Thu, Apr 09, 2020 at 09:24:31PM +0300, Dmitry Osipenko wrote:
> 09.04.2020 20:52, Thierry Reding пишет:
> ...
> > +static long tegra210_clk_emc_round_rate(struct clk_hw *hw, unsigned long rate,
> > +					unsigned long *prate)
> > +{
> > +	struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw);
> > +	struct tegra210_clk_emc_provider *provider = emc->provider;
> > +	unsigned int i;
> > +
> > +	if (!provider || !provider->configs || provider->num_configs == 0)
> > +		return clk_hw_get_rate(hw);
> 
> This still looks wrong to me. Nobody should be able to get EMC clock
> until provider is registered.

The EMC clock is mostly orthogonal to the provider. The provider really
only allows you to actually change the frequency. The clock will still
remain even if the provider goes away, it just will loose the ability to
change rate.

> This is troublesome, especially given that you're allowing the EMC
> driver to be compiled as a loadable module. For example, this won't work
> with the current ACTMON driver because it builds OPP table based on the
> clk-rate rounding during the driver's probe, so it won't be able to do
> it properly if provider is "temporarily" missing.
> 
> ... I think that in a longer run we should stop manually building the
> ACTMON's OPP table and instead define a proper OPP table (per-HW Speedo
> ID, with voltages) in a device-tree. But this is just a vague plans for
> the future for now.

This code only applies to Tegra210 and we don't currently support ACTMON
on Tegra210. I'm also not sure we'll ever do because using interconnects
to describe paths to system memory and then using ICC requests for each
driver to submit memory bandwidth requests seems like a better way of
dealing with this problem than using ACTMON to monitor activity because
that only allows you to react, whereas we really want to be able to
allocate memory bandwidth upfront.

Thierry