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

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

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