Re: [PATCH 1/2] clocksource: tegra: Use rating when registering clock source

[Thierry Reding]

On Fri, Jun 14, 2019 at 03:24:07PM +0300, Dmitry Osipenko wrote:
> 14.06.2019 13:47, Thierry Reding ÐÐÑÐÑ:
> > From: Thierry Reding <treding@xxxxxxxxxx>
> > 
> > The rating is parameterized depending on SoC generation to make sure it
> > takes precedence on implementations where the architected timer can't be
> > used. This rating is already used for the clock event device. Use the
> > same rating for the clock source to be consistent.
> > 
> > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> > ---
> >  drivers/clocksource/timer-tegra.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> > index f6a8eb0d7322..e6608141cccb 100644
> > --- a/drivers/clocksource/timer-tegra.c
> > +++ b/drivers/clocksource/timer-tegra.c
> > @@ -318,7 +318,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
> >  	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
> >  
> >  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
> > -				    "timer_us", TIMER_1MHz, 300, 32,
> > +				    "timer_us", TIMER_1MHz, rating, 32,
> >  				    clocksource_mmio_readl_up);
> >  	if (ret)
> >  		pr_err("failed to register clocksource: %d\n", ret);
> > 
> 
> Looks good. Although, could you please clarify whether arch-timer stops on T210 when CPU
> enters deepest (powerdown) idle state? I'm starting to lose track a bit already. Because
> if arch-timer stops in the deepest idle state, then it's a bit odd that Joseph didn't add
> the clocksource for T210 in the first place and v5.1 probably shouldn't work well because
> of that already.

Yes, the architected timer doesn't work across an SC7 (which is what the
deepest idle state is called on Tegra210) transition, hence why we can't
use it as a suspend clocksource. I actually sent out a patch to do that,
earlier.

And yes, it's entirely possible that v5.1 doesn't work in this regard,
but we're not noticing that because we don't have suspend/resume support
for Tegra210 anyway. There are a couple of missing pieces that we need
in order to make it work.

This change in particular is only going to affect the CPU idle state
(CC7). Since the architected timer doesn't survive that either, we need
the Tegra timer to be preferred over the architected timer for normal
operation.

All of these issues go away on Tegra186 and later, where the architected
timer is in an always-on partition and has a PLL that remains on during
SC7 (and CC7).

Thierry

Attachment: signature.asc
Description: PGP signature