Re: [PATCH 1/1] timecounter: Make cyclecounter struct part of timecounter struct

[Sagar Arun Kamble]

On 12/2/2017 11:34 PM, Richard Cochran wrote:
> On Sat, Dec 02, 2017 at 10:01:35AM +0530, Sagar Arun Kamble wrote:
> > There is no real need for the users of timecounters to define cyclecounter
> > and timecounter variables separately. Since timecounter will always be
> > based on cyclecounter, have cyclecounter struct as member of timecounter
> > struct.
> Overall, this is a welcome change.  However, it doesn't go far enough,
> IMHO, and I'll explain that more below.
>
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> > index 0247885..35987b5 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> ...
>
> As it stands now, timecounter_init() is used for two totally different
> reasons.  Some callers only want to set the time, ...
>
> > @@ -207,7 +207,7 @@ static int mlx4_en_phc_settime(struct ptp_clock_info *ptp,
> >   
> >   	/* reset the timecounter */
> >   	write_seqlock_irqsave(&mdev->clock_lock, flags);
> > -	timecounter_init(&mdev->clock, &mdev->cycles, ns);
> > +	timecounter_init(&mdev->clock, ns);
> >   	write_sequnlock_irqrestore(&mdev->clock_lock, flags);
> >   
> >   	return 0;
> ... while others initialize the data structure the first time:
>
> > @@ -274,17 +274,17 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
> >   
> >   	seqlock_init(&mdev->clock_lock);
> >   
> > -	memset(&mdev->cycles, 0, sizeof(mdev->cycles));
> > -	mdev->cycles.read = mlx4_en_read_clock;
> > -	mdev->cycles.mask = CLOCKSOURCE_MASK(48);
> > -	mdev->cycles.shift = freq_to_shift(dev->caps.hca_core_clock);
> > -	mdev->cycles.mult =
> > -		clocksource_khz2mult(1000 * dev->caps.hca_core_clock, mdev->cycles.shift);
> > -	mdev->nominal_c_mult = mdev->cycles.mult;
> > +	memset(&mdev->clock.cc, 0, sizeof(mdev->clock.cc));
> > +	mdev->clock.cc.read = mlx4_en_read_clock;
> > +	mdev->clock.cc.mask = CLOCKSOURCE_MASK(48);
> > +	mdev->clock.cc.shift = freq_to_shift(dev->caps.hca_core_clock);
> > +	mdev->clock.cc.mult =
> > +		clocksource_khz2mult(1000 * dev->caps.hca_core_clock,
> > +				     mdev->clock.cc.shift);
> > +	mdev->nominal_c_mult = mdev->clock.cc.mult;
> >   
> >   	write_seqlock_irqsave(&mdev->clock_lock, flags);
> > -	timecounter_init(&mdev->clock, &mdev->cycles,
> > -			 ktime_to_ns(ktime_get_real()));
> > +	timecounter_init(&mdev->clock, ktime_to_ns(ktime_get_real()));
> I'd like to see two followup patches to this one:
>
> 1. Convert timecounter_init() callers to a new timecounter_reset()
>     function where the intent is to reset the time.
>
> 2. Change timecounter_init() to take the cyclecounter fields as
>     arguments.
>
> 	void timecounter_init(struct timecounter *tc,
> 			      u64 (*read)(const struct cyclecounter *cc),
> 			      u64 mask,
> 			      u32 mult,
> 			      u32 shift,
> 			      u64 start_tstamp);
>
> Then we can clean up all this stuff:
>
> 	mdev->clock.cc.read = mlx4_en_read_clock;
> 	mdev->clock.cc.mask = CLOCKSOURCE_MASK(48);
> 	mdev->clock.cc.shift = freq_to_shift(dev->caps.hca_core_clock);
> 	mdev->clock.cc.mult = clocksource_khz2mult(...);
>
> This second step can be phased in by calling the new function
> timecounter_initialize() and converting the drivers one by one.

Yes. Will make these changes and share new patchset.

Thank you for the review Richard.

Regards,
Sagar

> > diff --git a/include/linux/timecounter.h b/include/linux/timecounter.h
> > index 2496ad4..6daca06 100644
> > --- a/include/linux/timecounter.h
> > +++ b/include/linux/timecounter.h
> ...
> > @@ -98,7 +98,6 @@ static inline void timecounter_adjtime(struct timecounter *tc, s64 delta)
> >   /**
> >    * timecounter_init - initialize a time counter
> >    * @tc:			Pointer to time counter which is to be initialized/reset
> > - * @cc:			A cycle counter, ready to be used.
> This "ready to used" requirement should go.  The init() function
> should make the instance ready to be used all at once.
>
> Thanks,
> Richard