Re: [PATCH v2 2/2] clocksource: Add driver for the Ingenic JZ47xx OST

[Paul Cercueil]

Hi Thomas,


Le jeu., janv. 9, 2020 at 15:28, Thomas Gleixner <tglx@xxxxxxxxxxxxx> a 
écrit :
> Paul Cercueil <paul@xxxxxxxxxxxxxxx> writes:
> >  +static u64 notrace ingenic_ost_clocksource_read64(struct 
> > clocksource *cs)
> >  +{
> >  +	u32 val1, val2;
> >  +	u64 count, recount;
> >  +	s64 diff;
> >  +
> >  +	/*
> >  +	 * The buffering of the upper 32 bits of the timer prevents wrong
> >  +	 * results from the bottom 32 bits overflowing due to the timer 
> > ticking
> >  +	 * along. However, it does not prevent wrong results from 
> > simultaneous
> >  +	 * reads of the timer, which could reset the buffer mid-read.
> >  +	 * Since this kind of wrong read can happen only when the bottom 
> > bits
> >  +	 * overflow, there will be minutes between wrong reads, so if we 
> > read
> >  +	 * twice in succession, at least one of the reads will be correct.
> >  +	 */
> >  +
> >  +	/* Bypass the regmap here as we must return as soon as possible */
>
> I have a hard time to understand this comment. "Bypass the regmap ..."
> and then use a regmap function?

Ah, sorry, it's a leftover from a previous version of the patch. It 
used to bypass the regmap in order to complete as fast as possible.

> >  +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
> >  +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
> >  +	count = (u64)val1 | (u64)val2 << 32;
> >  +
> >  +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
> >  +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
> >  +	recount = (u64)val1 | (u64)val2 << 32;
> >  +
> >  +	/*
> >  +	 * A wrong read will produce a result that is 1<<32 too high: the 
> > bottom
> >  +	 * part from before overflow and the upper part from after 
> > overflow.
> >  +	 * Therefore, the lower value of the two reads is the correct 
> > value.
> >  +	 */
> >  +
> >  +	diff = (s64)(recount - count);
> >  +	if (unlikely(diff < 0))
> >  +		count = recount;
>
> Is this really the right approach here? What is the 64bit readout 
> buying
> you?
>
> The timekeeping code can handle a 32bit counter perfectly fine and the
> only advantage you get is that your maximum possible idle time will be
> longer with a 64bit counter.
>
> But is that really worth the overhead of four MMIO reads versus one 
> in a
> hotpath?

The timer is 64-bit so I thought it made sense to register it as such. 
Using it as just a 32-bit counter sounds better indeed.

Thanks,
-Paul