Re: [PATCH v1 4/6] pps: generators: Add PPS Generator TIO Driver

[Peter Hilber <peter.hilber@opensynergy.com>]

On 01.12.23 11:22, D, Lakshmi Sowjanya wrote:
> 
> 
>> -----Original Message-----
>> From: Peter Hilber <peter.hilber@opensynergy.com>
>> Sent: Friday, October 20, 2023 9:12 PM
>> To: Thomas Gleixner <tglx@linutronix.de>; D, Lakshmi Sowjanya
>> <lakshmi.sowjanya.d@intel.com>; jstultz@google.com;
>> giometti@enneenne.com; corbet@lwn.net; linux-kernel@vger.kernel.org
>> Cc: x86@kernel.org; linux-doc@vger.kernel.org;
>> andriy.shevchenko@linux.intel.com; Dong, Eddie <eddie.dong@intel.com>; Hall,
>> Christopher S <christopher.s.hall@intel.com>; N, Pandith
>> <pandith.n@intel.com>; Sangannavar, Mallikarjunappa
>> <mallikarjunappa.sangannavar@intel.com>; T R, Thejesh Reddy
>> <thejesh.reddy.t.r@intel.com>
>> Subject: Re: [PATCH v1 4/6] pps: generators: Add PPS Generator TIO Driver
>>
>> On 17.10.23 18:27, Thomas Gleixner wrote:
>>> On Tue, Oct 17 2023 at 10:54, lakshmi.sowjanya.d@intel.com wrote:
>>>> +	guard(spinlock_irqsave)(&tio->lock);
>>>> +	if (enable && !tio->enabled) {
>>>> +		if (!is_current_clocksource_art_related()) {
>>>> +			dev_err(tio->dev, "PPS cannot be started as clock is not
>> related to ART");
>>>> +			return -EPERM;
>>>> +		}
>>>
>>> Ah. Here is the usecase for this magic patch 3/6 hackery. Again, it's
>>> the wrong abstraction. You want something like:
>>>
>>>     timekeeping_clocksource_has_base(CSID_X86_ART);
>>>
>>> or something like this, which can be handled completely in the core
>>> code.
>>>
>>> All of this needs some serious rework. See the below disfunctional
>>> mockup patch for illustration.
>>>
>>> There is also a patch series, which tried to replace the clocksource
>>> pointer in system_counterval_t with a clocksource ID:
>>>
>>>
>>> https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2fall%2f20230818011256.211078%2d1%2dpeter.hilber%40opens&umid=463c9a27-09dd-442f-8b69-49391ffc0193&auth=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-31f616e9a1d239a660d3c4b60c2f3655ce46f5d4
>>> ynergy.com
>>>
>>> That went nowhere, but has some valid points. I took some of Peter's
>>> (cc'ed) ideas into the mockup, but did it slightly different to make
>>> all of this indirection mess go away.
>>>
>>> There are certainly bugs and thinkos in that mockup. If you find them,
>>> you can keep and fix them :)
>>>
>>
>> Hi Sowjanya,
>>
>> I am working on another iteration of the patch series cited by Thomas, which
>> would implement part of the mockup. But the scope of the original patch series
>> would remain, so no changes to ART conversion code, no introduction of the
>> clocksource_base concept... The patch series should still be compatible with
>> Thomas' proposal, though.
>>
>> I was wondering if it would make sense to coordinate the posting of my patch
>> series. I planned to post the series sometime in November along with other
>> stuff, but I could potentially post it earlier if this could help to align.
>>
>> In case of interest in aligning, I uploaded an unverified preview for the upcoming
>> series to [1].
>>
>> Best regards,
>>
>> Peter
>>
>> [1] https://github.com/OpenSynergy/linux.git clocksource-id-for-
>> get_device_system_crosststamp-v2-preview
> 
> Hi Peter,
> 
> Thanks for the patches.
>  
> Along with the patch you shared, we have verified the mockup code shared by Thomas.
> 
> Please let us know when you are planning to post the patches, we can co-ordinate and post. 
> 
> Regards
> Lakshmi Sowjanya D

Hi Sowjanya,

thank you for your response. I am unfortunately late with my other, not
directly related, patches; my plan is to post them all together until
December 15th. Is this OK with you?

Regards,

Peter