Re: [PATCH v6 09/14] memory: tegra: Add EMC scaling support code for Tegra210

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3062 bytes --]

On Thu, Apr 09, 2020 at 10:16:46PM +0300, Dmitry Osipenko wrote:
> 09.04.2020 20:52, Thierry Reding пишет:
> ...
> > +static int tegra210_emc_set_rate(struct device *dev,
> > +				 const struct tegra210_clk_emc_config *config)
> > +{
> > +	struct tegra210_emc *emc = dev_get_drvdata(dev);
> > +	struct tegra210_emc_timing *timing = NULL;
> > +	unsigned long rate = config->rate;
> > +	s64 last_change_delay;
> > +	unsigned long flags;
> > +	unsigned int i;
> > +
> > +	if (rate == emc->last->rate * 1000UL)
> > +		return 0;
> 
> Couldn't all the rates be expressed in Hz? Then you won't need all these
> multiplications by 1000.

The EMC table is generated with kHz and I'd rather not change the values
in those entries. There's only a few cases where we need to convert from
one to the other, and they are always only when we compare a CCF rate to
the EMC rate, so I think it's fairly explicit when it's needed.

> > +	for (i = 0; i < emc->num_timings; i++) {
> > +		if (emc->timings[i].rate * 1000UL == rate) {
> > +			timing = &emc->timings[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!timing)
> > +		return -EINVAL;
> > +
> > +	if (rate > 204000000 && !timing->trained)
> > +		return -EINVAL;
> > +
> > +	emc->next = timing;
> > +	last_change_delay = ktime_us_delta(ktime_get(), emc->clkchange_time);
> > +
> > +	/* XXX use non-busy-looping sleep? */
> > +	if ((last_change_delay >= 0) &&
> > +	    (last_change_delay < emc->clkchange_delay))
> > +		udelay(emc->clkchange_delay - (int)last_change_delay);
> > +
> > +	spin_lock_irqsave(&emc->lock, flags);
> > +	tegra210_emc_set_clock(emc, config->value);
> > +	emc->clkchange_time = ktime_get();
> > +	emc->last = timing;
> > +	spin_unlock_irqrestore(&emc->lock, flags);
> > +
> > +	return 0;
> > +}
> 
> I'd suggest to check how much time invocation of ktime_get() takes, at
> least it came to a surprise to me in a case of the tegra-cpuidle driver.
> 
> It may be well over the emc->clkchange_delay.

I assume that at least each invocation would take roughly the same
amount of time? Since we only use the value to compute the time since
the last clock change the result should always be valid. In the worst
case if ktime_get() really takes that long we may be waiting longer
than we need to, but that wouldn't be all that bad either. Changing
the EMC clock rate isn't something that you want to do a lot.

> ...
> > +static int tegra210_emc_probe(struct platform_device *pdev)
> > +{
> ...
> > +	emc->clkchange_delay = 100;
> > +	emc->training_interval = 100;
> 
> Not sure why these aren't a constant with the code.. ?

The idea is to make them easily tunable without having to go hunt for
them later on. We don't use them in a lot of computations, so making
them constants isn't going to buy us a lot. Also, none of these code
paths are really hot, so I like the flexibility that this gives us in
being able to quickly tweak if we ever need to without having to worry
that we'll forget a location.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]