>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Tuesday, June 27, 2023 12:18 PM
>
>Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com wrote:
>
>>v8 -> v9:
>
>Could you please address all the unresolved issues from v8 and send v10?
>I'm not reviewing this one.
>
>Thanks!

Sure, will do, but first missing to-do/discuss list:
1) remove mode_set as not used by any driver
2) remove "no-added-value" static functions descriptions in
   dpll_core/dpll_netlink
3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
   after each patch apply
4) remove function return values descriptions/lists
5) Fix patch [05/10]:
   - status Supported
   - additional maintainers
   - remove callback:
     int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
6) Fix patch [08/10]: rethink ice mutex locking scheme
7) Fix patch [09/10]: multiple comments on
https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
8) add PPS DPLL phase offset to the netlink get-device API

Thank you!
Arkadiusz

>From: Kubalewski, Arkadiusz
>Sent: Wednesday, June 28, 2023 11:15 AM
>
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Tuesday, June 27, 2023 12:18 PM
>>
>>Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com
>wrote:
>>
>>>v8 -> v9:
>>
>>Could you please address all the unresolved issues from v8 and send v10?
>>I'm not reviewing this one.
>>
>>Thanks!
>
>Sure, will do, but first missing to-do/discuss list:
>1) remove mode_set as not used by any driver
>2) remove "no-added-value" static functions descriptions in
>   dpll_core/dpll_netlink
>3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
>   after each patch apply
>4) remove function return values descriptions/lists
>5) Fix patch [05/10]:
>   - status Supported
>   - additional maintainers
>   - remove callback:
>     int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
>6) Fix patch [08/10]: rethink ice mutex locking scheme
>7) Fix patch [09/10]: multiple comments on
>https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
>8) add PPS DPLL phase offset to the netlink get-device API
>
>Thank you!
>Arkadiusz

If someone has any objections please state them now, I will work on
all above except 5) and 7).
Vadim, could you take care of those 2 points?

Thank you!
Arkadiusz

On 28/06/2023 10:27, Kubalewski, Arkadiusz wrote:
>> From: Kubalewski, Arkadiusz
>> Sent: Wednesday, June 28, 2023 11:15 AM
>>
>>> From: Jiri Pirko <jiri@resnulli.us>
>>> Sent: Tuesday, June 27, 2023 12:18 PM
>>>
>>> Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com
>> wrote:
>>>
>>>> v8 -> v9:
>>>
>>> Could you please address all the unresolved issues from v8 and send v10?
>>> I'm not reviewing this one.
>>>
>>> Thanks!
>>
>> Sure, will do, but first missing to-do/discuss list:
>> 1) remove mode_set as not used by any driver
>> 2) remove "no-added-value" static functions descriptions in
>>    dpll_core/dpll_netlink
>> 3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
>>    after each patch apply
>> 4) remove function return values descriptions/lists
>> 5) Fix patch [05/10]:
>>    - status Supported
>>    - additional maintainers
>>    - remove callback:
>>      int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
>> 6) Fix patch [08/10]: rethink ice mutex locking scheme
>> 7) Fix patch [09/10]: multiple comments on
>> https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
>> 8) add PPS DPLL phase offset to the netlink get-device API
>>
>> Thank you!
>> Arkadiusz
> 
> If someone has any objections please state them now, I will work on
> all above except 5) and 7).
> Vadim, could you take care of those 2 points?
> 
Yeah, sure, I'll update 5 and 7.
I'm not sure about 8) - do we really need this info, I believe every 
supported DPLL device exports PTP device as well. But I'm Ok to add this 
feature too.

> Thank you!
> Arkadiusz

Wed, Jun 28, 2023 at 01:11:19PM CEST, vadim.fedorenko@linux.dev wrote:
>On 28/06/2023 10:27, Kubalewski, Arkadiusz wrote:
>> > From: Kubalewski, Arkadiusz
>> > Sent: Wednesday, June 28, 2023 11:15 AM
>> > 
>> > > From: Jiri Pirko <jiri@resnulli.us>
>> > > Sent: Tuesday, June 27, 2023 12:18 PM
>> > > 
>> > > Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com
>> > wrote:
>> > > 
>> > > > v8 -> v9:
>> > > 
>> > > Could you please address all the unresolved issues from v8 and send v10?
>> > > I'm not reviewing this one.
>> > > 
>> > > Thanks!
>> > 
>> > Sure, will do, but first missing to-do/discuss list:
>> > 1) remove mode_set as not used by any driver
>> > 2) remove "no-added-value" static functions descriptions in
>> >    dpll_core/dpll_netlink
>> > 3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
>> >    after each patch apply
>> > 4) remove function return values descriptions/lists
>> > 5) Fix patch [05/10]:
>> >    - status Supported
>> >    - additional maintainers
>> >    - remove callback:
>> >      int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
>> > 6) Fix patch [08/10]: rethink ice mutex locking scheme
>> > 7) Fix patch [09/10]: multiple comments on
>> > https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
>> > 8) add PPS DPLL phase offset to the netlink get-device API
>> > 
>> > Thank you!
>> > Arkadiusz
>> 
>> If someone has any objections please state them now, I will work on
>> all above except 5) and 7).
>> Vadim, could you take care of those 2 points?
>> 
>Yeah, sure, I'll update 5 and 7.
>I'm not sure about 8) - do we really need this info, I believe every
>supported DPLL device exports PTP device as well. But I'm Ok to add this
>feature too.

Could you add the notification work while you are at it? I don't want
that to be forgotten. Thanks!

>
>> Thank you!
>> Arkadiusz
>

On 28/06/2023 14:09, Jiri Pirko wrote:
> Wed, Jun 28, 2023 at 01:11:19PM CEST, vadim.fedorenko@linux.dev wrote:
>> On 28/06/2023 10:27, Kubalewski, Arkadiusz wrote:
>>>> From: Kubalewski, Arkadiusz
>>>> Sent: Wednesday, June 28, 2023 11:15 AM
>>>>
>>>>> From: Jiri Pirko <jiri@resnulli.us>
>>>>> Sent: Tuesday, June 27, 2023 12:18 PM
>>>>>
>>>>> Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com
>>>> wrote:
>>>>>
>>>>>> v8 -> v9:
>>>>>
>>>>> Could you please address all the unresolved issues from v8 and send v10?
>>>>> I'm not reviewing this one.
>>>>>
>>>>> Thanks!
>>>>
>>>> Sure, will do, but first missing to-do/discuss list:
>>>> 1) remove mode_set as not used by any driver
>>>> 2) remove "no-added-value" static functions descriptions in
>>>>     dpll_core/dpll_netlink
>>>> 3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
>>>>     after each patch apply
>>>> 4) remove function return values descriptions/lists
>>>> 5) Fix patch [05/10]:
>>>>     - status Supported
>>>>     - additional maintainers
>>>>     - remove callback:
>>>>       int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
>>>> 6) Fix patch [08/10]: rethink ice mutex locking scheme
>>>> 7) Fix patch [09/10]: multiple comments on
>>>> https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
>>>> 8) add PPS DPLL phase offset to the netlink get-device API
>>>>
>>>> Thank you!
>>>> Arkadiusz
>>>
>>> If someone has any objections please state them now, I will work on
>>> all above except 5) and 7).
>>> Vadim, could you take care of those 2 points?
>>>
>> Yeah, sure, I'll update 5 and 7.
>> I'm not sure about 8) - do we really need this info, I believe every
>> supported DPLL device exports PTP device as well. But I'm Ok to add this
>> feature too.
> 
> Could you add the notification work while you are at it? I don't want
> that to be forgotten. Thanks!

Sure, Jiri, I'm working on it for ptp_ocp.

>>
>>> Thank you!
>>> Arkadiusz
>>

Wed, Jun 28, 2023 at 03:22:00PM CEST, vadim.fedorenko@linux.dev wrote:
>On 28/06/2023 14:09, Jiri Pirko wrote:
>> Wed, Jun 28, 2023 at 01:11:19PM CEST, vadim.fedorenko@linux.dev wrote:
>> > On 28/06/2023 10:27, Kubalewski, Arkadiusz wrote:
>> > > > From: Kubalewski, Arkadiusz
>> > > > Sent: Wednesday, June 28, 2023 11:15 AM
>> > > > 
>> > > > > From: Jiri Pirko <jiri@resnulli.us>
>> > > > > Sent: Tuesday, June 27, 2023 12:18 PM
>> > > > > 
>> > > > > Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com
>> > > > wrote:
>> > > > > 
>> > > > > > v8 -> v9:
>> > > > > 
>> > > > > Could you please address all the unresolved issues from v8 and send v10?
>> > > > > I'm not reviewing this one.
>> > > > > 
>> > > > > Thanks!
>> > > > 
>> > > > Sure, will do, but first missing to-do/discuss list:
>> > > > 1) remove mode_set as not used by any driver
>> > > > 2) remove "no-added-value" static functions descriptions in
>> > > >     dpll_core/dpll_netlink
>> > > > 3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
>> > > >     after each patch apply
>> > > > 4) remove function return values descriptions/lists
>> > > > 5) Fix patch [05/10]:
>> > > >     - status Supported
>> > > >     - additional maintainers
>> > > >     - remove callback:
>> > > >       int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
>> > > > 6) Fix patch [08/10]: rethink ice mutex locking scheme
>> > > > 7) Fix patch [09/10]: multiple comments on
>> > > > https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
>> > > > 8) add PPS DPLL phase offset to the netlink get-device API
>> > > > 
>> > > > Thank you!
>> > > > Arkadiusz
>> > > 
>> > > If someone has any objections please state them now, I will work on
>> > > all above except 5) and 7).
>> > > Vadim, could you take care of those 2 points?
>> > > 
>> > Yeah, sure, I'll update 5 and 7.
>> > I'm not sure about 8) - do we really need this info, I believe every
>> > supported DPLL device exports PTP device as well. But I'm Ok to add this
>> > feature too.
>> 
>> Could you add the notification work while you are at it? I don't want
>> that to be forgotten. Thanks!
>
>Sure, Jiri, I'm working on it for ptp_ocp.

Yep, cool!

>
>> > 
>> > > Thank you!
>> > > Arkadiusz
>> > 
>

Wed, Jun 28, 2023 at 11:15:11AM CEST, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Tuesday, June 27, 2023 12:18 PM
>>
>>Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>
>>>v8 -> v9:
>>
>>Could you please address all the unresolved issues from v8 and send v10?
>>I'm not reviewing this one.
>>
>>Thanks!
>
>Sure, will do, but first missing to-do/discuss list:
>1) remove mode_set as not used by any driver
>2) remove "no-added-value" static functions descriptions in
>   dpll_core/dpll_netlink
>3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
>   after each patch apply
>4) remove function return values descriptions/lists
>5) Fix patch [05/10]:
>   - status Supported
>   - additional maintainers
>   - remove callback:
>     int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
>6) Fix patch [08/10]: rethink ice mutex locking scheme
>7) Fix patch [09/10]: multiple comments on
>https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
>8) add PPS DPLL phase offset to the netlink get-device API
>

You are missing removal of pin->prop.package_label = dev_name(dev); in
ice.


>Thank you!
>Arkadiusz

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Wednesday, June 28, 2023 1:16 PM
>Wed, Jun 28, 2023 at 11:15:11AM CEST, arkadiusz.kubalewski@intel.com wrote:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Sent: Tuesday, June 27, 2023 12:18 PM
>>>
>>>Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com
>>>wrote:
>>>
>>>>v8 -> v9:
>>>
>>>Could you please address all the unresolved issues from v8 and send v10?
>>>I'm not reviewing this one.
>>>
>>>Thanks!
>>
>>Sure, will do, but first missing to-do/discuss list:
>>1) remove mode_set as not used by any driver

I have implemented in ice (also added back the DPLL_MODE_FREERUN).

>>2) remove "no-added-value" static functions descriptions in
>>   dpll_core/dpll_netlink

Removed.

>>3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
>>   after each patch apply

Hope Vadim will decide on this, the thing is merging in two patches
doesn't make much sense as there won't be any linking until both patches
are there, so most sense it would be if 3 are merged into one, but
then we will be back to one big blob patch issue.

>>4) remove function return values descriptions/lists

Fixed.

>>5) Fix patch [05/10]:
>>   - status Supported
>>   - additional maintainers
>>   - remove callback:
>>     int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
>>6) Fix patch [08/10]: rethink ice mutex locking scheme

Fixed.

>>7) Fix patch [09/10]: multiple comments on
>>https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
>>8) add PPS DPLL phase offset to the netlink get-device API
>>

Added few things on this matter
- 1 dpll level attribute:
  - phase-shift - measuring the phase difference between dpll input
    and it's output
- 1 dpll-pin tuple level attribute:
  - pin-phase-adjust - set/get phase adjust of a pin on a dpll
- 2 pin level attributes:
  - pin-phase-adjust-min - provide user with min value that can be set
  - pin-phase-adjust-max - provide user with max value that can be set
- a constant:
  - DPLL_PHASE_SHIFT_DIVIDER similar to DPLL_TEMP_DIVIDER for producing
    fraction value of measured DPLL_A_PHASE_SHIFT
- implemented in dpll netlink and in ice

>
>You are missing removal of pin->prop.package_label = dev_name(dev); in
>ice.
>

I didn't touch it, as we still need to discuss it, Jakub didn't respond
on v8 thread.
I don't see why we shall not name it the way. This is most meaningful
label for those pins for the user right now.

Thank you!
Arkadiusz

>
>>Thank you!
>>Arkadiusz

Mon, Jul 10, 2023 at 12:07:30PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Wednesday, June 28, 2023 1:16 PM
>>Wed, Jun 28, 2023 at 11:15:11AM CEST, arkadiusz.kubalewski@intel.com wrote:
>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>Sent: Tuesday, June 27, 2023 12:18 PM
>>>>
>>>>Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com
>>>>wrote:
>>>>
>>>>>v8 -> v9:
>>>>
>>>>Could you please address all the unresolved issues from v8 and send v10?
>>>>I'm not reviewing this one.
>>>>
>>>>Thanks!
>>>
>>>Sure, will do, but first missing to-do/discuss list:
>>>1) remove mode_set as not used by any driver
>
>I have implemented in ice (also added back the DPLL_MODE_FREERUN).

Uh :/ Why exactly is it needed in this initial submission?


>
>>>2) remove "no-added-value" static functions descriptions in
>>>   dpll_core/dpll_netlink
>
>Removed.
>
>>>3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
>>>   after each patch apply
>
>Hope Vadim will decide on this, the thing is merging in two patches
>doesn't make much sense as there won't be any linking until both patches
>are there, so most sense it would be if 3 are merged into one, but
>then we will be back to one big blob patch issue.
>
>>>4) remove function return values descriptions/lists
>
>Fixed.
>
>>>5) Fix patch [05/10]:
>>>   - status Supported
>>>   - additional maintainers
>>>   - remove callback:
>>>     int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
>>>6) Fix patch [08/10]: rethink ice mutex locking scheme
>
>Fixed.
>
>>>7) Fix patch [09/10]: multiple comments on
>>>https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
>>>8) add PPS DPLL phase offset to the netlink get-device API
>>>
>
>Added few things on this matter
>- 1 dpll level attribute:
>  - phase-shift - measuring the phase difference between dpll input
>    and it's output
>- 1 dpll-pin tuple level attribute:
>  - pin-phase-adjust - set/get phase adjust of a pin on a dpll
>- 2 pin level attributes:
>  - pin-phase-adjust-min - provide user with min value that can be set
>  - pin-phase-adjust-max - provide user with max value that can be set
>- a constant:
>  - DPLL_PHASE_SHIFT_DIVIDER similar to DPLL_TEMP_DIVIDER for producing
>    fraction value of measured DPLL_A_PHASE_SHIFT

Again, why do we need this in this initial submission? Why it can't be a
follow-up patchset to extend this? This way we never converge :/
Please focus on what we have now and bring it in. Let the extensions to
be addressed later on, please.



>- implemented in dpll netlink and in ice
>
>>
>>You are missing removal of pin->prop.package_label = dev_name(dev); in
>>ice.
>>
>
>I didn't touch it, as we still need to discuss it, Jakub didn't respond
>on v8 thread.
>I don't see why we shall not name it the way. This is most meaningful
>label for those pins for the user right now.

This is not meaningful, at all. dev_name() changes upon which pci slot
you plug the card into. package_label should be an actual label on a
silicon package. Why you think this two are related in aby way, makes me
really wonder. Could you elaborate the meaningfulness of this?


>
>Thank you!
>Arkadiusz
>
>>
>>>Thank you!
>>>Arkadiusz

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Monday, July 10, 2023 2:10 PM
>
>Mon, Jul 10, 2023 at 12:07:30PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Sent: Wednesday, June 28, 2023 1:16 PM
>>>Wed, Jun 28, 2023 at 11:15:11AM CEST, arkadiusz.kubalewski@intel.com
>wrote:
>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>Sent: Tuesday, June 27, 2023 12:18 PM
>>>>>
>>>>>Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com
>>>>>wrote:
>>>>>
>>>>>>v8 -> v9:
>>>>>
>>>>>Could you please address all the unresolved issues from v8 and send v10?
>>>>>I'm not reviewing this one.
>>>>>
>>>>>Thanks!
>>>>
>>>>Sure, will do, but first missing to-do/discuss list:
>>>>1) remove mode_set as not used by any driver
>>
>>I have implemented in ice (also added back the DPLL_MODE_FREERUN).
>
>Uh :/ Why exactly is it needed in this initial submission?
>

Without mode-set there is no need for device-set at all, right?
So it is better to implement at least one set command, so we don't
need remove device-set command entirely.

>
>>
>>>>2) remove "no-added-value" static functions descriptions in
>>>>   dpll_core/dpll_netlink
>>
>>Removed.
>>
>>>>3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
>>>>   after each patch apply
>>
>>Hope Vadim will decide on this, the thing is merging in two patches
>>doesn't make much sense as there won't be any linking until both patches
>>are there, so most sense it would be if 3 are merged into one, but
>>then we will be back to one big blob patch issue.
>>
>>>>4) remove function return values descriptions/lists
>>
>>Fixed.
>>
>>>>5) Fix patch [05/10]:
>>>>   - status Supported
>>>>   - additional maintainers
>>>>   - remove callback:
>>>>     int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
>>>>6) Fix patch [08/10]: rethink ice mutex locking scheme
>>
>>Fixed.
>>
>>>>7) Fix patch [09/10]: multiple comments on
>>>>https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
>>>>8) add PPS DPLL phase offset to the netlink get-device API
>>>>
>>
>>Added few things on this matter
>>- 1 dpll level attribute:
>>  - phase-shift - measuring the phase difference between dpll input
>>    and it's output
>>- 1 dpll-pin tuple level attribute:
>>  - pin-phase-adjust - set/get phase adjust of a pin on a dpll
>>- 2 pin level attributes:
>>  - pin-phase-adjust-min - provide user with min value that can be set
>>  - pin-phase-adjust-max - provide user with max value that can be set
>>- a constant:
>>  - DPLL_PHASE_SHIFT_DIVIDER similar to DPLL_TEMP_DIVIDER for producing
>>    fraction value of measured DPLL_A_PHASE_SHIFT
>
>Again, why do we need this in this initial submission? Why it can't be a
>follow-up patchset to extend this? This way we never converge :/
>Please focus on what we have now and bring it in. Let the extensions to
>be addressed later on, please.
>

Well AFAIK, RHEL is doing some monitoring software, so the end-users need this.

>
>
>>- implemented in dpll netlink and in ice
>>
>>>
>>>You are missing removal of pin->prop.package_label = dev_name(dev); in
>>>ice.
>>>
>>
>>I didn't touch it, as we still need to discuss it, Jakub didn't respond
>>on v8 thread.
>>I don't see why we shall not name it the way. This is most meaningful
>>label for those pins for the user right now.
>
>This is not meaningful, at all. dev_name() changes upon which pci slot
>you plug the card into. package_label should be an actual label on a
>silicon package. Why you think this two are related in aby way, makes me
>really wonder. Could you elaborate the meaningfulness of this?
>

Without this, from end-user perspective, it would be very confusing.
As in ice without any label there would 4 pins which differs only with id.
What names would you suggest?

Thank you!
Arkadiusz

>
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>>Thank you!
>>>>Arkadiusz

Tue, Jul 11, 2023 at 12:34:11PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Monday, July 10, 2023 2:10 PM
>>
>>Mon, Jul 10, 2023 at 12:07:30PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>Sent: Wednesday, June 28, 2023 1:16 PM
>>>>Wed, Jun 28, 2023 at 11:15:11AM CEST, arkadiusz.kubalewski@intel.com
>>wrote:
>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>Sent: Tuesday, June 27, 2023 12:18 PM
>>>>>>
>>>>>>Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com
>>>>>>wrote:
>>>>>>
>>>>>>>v8 -> v9:
>>>>>>
>>>>>>Could you please address all the unresolved issues from v8 and send v10?
>>>>>>I'm not reviewing this one.
>>>>>>
>>>>>>Thanks!
>>>>>
>>>>>Sure, will do, but first missing to-do/discuss list:
>>>>>1) remove mode_set as not used by any driver
>>>
>>>I have implemented in ice (also added back the DPLL_MODE_FREERUN).
>>
>>Uh :/ Why exactly is it needed in this initial submission?
>>
>
>Without mode-set there is no need for device-set at all, right?
>So it is better to implement at least one set command, so we don't
>need remove device-set command entirely.

The enum cmd valu could stay as a placeholder, the rest can go.


>
>>
>>>
>>>>>2) remove "no-added-value" static functions descriptions in
>>>>>   dpll_core/dpll_netlink
>>>
>>>Removed.
>>>
>>>>>3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
>>>>>   after each patch apply
>>>
>>>Hope Vadim will decide on this, the thing is merging in two patches
>>>doesn't make much sense as there won't be any linking until both patches
>>>are there, so most sense it would be if 3 are merged into one, but
>>>then we will be back to one big blob patch issue.
>>>
>>>>>4) remove function return values descriptions/lists
>>>
>>>Fixed.
>>>
>>>>>5) Fix patch [05/10]:
>>>>>   - status Supported
>>>>>   - additional maintainers
>>>>>   - remove callback:
>>>>>     int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
>>>>>6) Fix patch [08/10]: rethink ice mutex locking scheme
>>>
>>>Fixed.
>>>
>>>>>7) Fix patch [09/10]: multiple comments on
>>>>>https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
>>>>>8) add PPS DPLL phase offset to the netlink get-device API
>>>>>
>>>
>>>Added few things on this matter
>>>- 1 dpll level attribute:
>>>  - phase-shift - measuring the phase difference between dpll input
>>>    and it's output
>>>- 1 dpll-pin tuple level attribute:
>>>  - pin-phase-adjust - set/get phase adjust of a pin on a dpll
>>>- 2 pin level attributes:
>>>  - pin-phase-adjust-min - provide user with min value that can be set
>>>  - pin-phase-adjust-max - provide user with max value that can be set
>>>- a constant:
>>>  - DPLL_PHASE_SHIFT_DIVIDER similar to DPLL_TEMP_DIVIDER for producing
>>>    fraction value of measured DPLL_A_PHASE_SHIFT
>>
>>Again, why do we need this in this initial submission? Why it can't be a
>>follow-up patchset to extend this? This way we never converge :/
>>Please focus on what we have now and bring it in. Let the extensions to
>>be addressed later on, please.
>>
>
>Well AFAIK, RHEL is doing some monitoring software, so the end-users need this.

They need it for the initial submission? Why? Why can't they wait 1 week
for follow-up patchset?


>
>>
>>
>>>- implemented in dpll netlink and in ice
>>>
>>>>
>>>>You are missing removal of pin->prop.package_label = dev_name(dev); in
>>>>ice.
>>>>
>>>
>>>I didn't touch it, as we still need to discuss it, Jakub didn't respond
>>>on v8 thread.
>>>I don't see why we shall not name it the way. This is most meaningful
>>>label for those pins for the user right now.
>>
>>This is not meaningful, at all. dev_name() changes upon which pci slot
>>you plug the card into. package_label should be an actual label on a
>>silicon package. Why you think this two are related in aby way, makes me
>>really wonder. Could you elaborate the meaningfulness of this?
>>
>
>Without this, from end-user perspective, it would be very confusing.
>As in ice without any label there would 4 pins which differs only with id.

There you go, it does not have any label, yet you are trying hard to
make up some. Does not make sense.


>What names would you suggest?

That is the point I made previously. For synce usecase, the label does
not make sense. There should be no label. You reference the pin by ID
from netdev, that is enough.

I think better to add the check to pin-register so future synce pin
users don't have similar weird ideas. Could you please add this check?

Thanks!



>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>Thank you!
>>>Arkadiusz
>>>
>>>>
>>>>>Thank you!
>>>>>Arkadiusz

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Tuesday, July 11, 2023 1:53 PM
>
>Tue, Jul 11, 2023 at 12:34:11PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Sent: Monday, July 10, 2023 2:10 PM
>>>
>>>Mon, Jul 10, 2023 at 12:07:30PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>Sent: Wednesday, June 28, 2023 1:16 PM
>>>>>Wed, Jun 28, 2023 at 11:15:11AM CEST, arkadiusz.kubalewski@intel.com
>>>>>wrote:
>>>>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>>>>Sent: Tuesday, June 27, 2023 12:18 PM
>>>>>>>
>>>>>>>Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com
>>>>>>>wrote:
>>>>>>>
>>>>>>>>v8 -> v9:
>>>>>>>
>>>>>>>Could you please address all the unresolved issues from v8 and send v10?
>>>>>>>I'm not reviewing this one.
>>>>>>>
>>>>>>>Thanks!
>>>>>>
>>>>>>Sure, will do, but first missing to-do/discuss list:
>>>>>>1) remove mode_set as not used by any driver
>>>>
>>>>I have implemented in ice (also added back the DPLL_MODE_FREERUN).
>>>
>>>Uh :/ Why exactly is it needed in this initial submission?
>>>
>>
>>Without mode-set there is no need for device-set at all, right?
>>So it is better to implement at least one set command, so we don't
>>need remove device-set command entirely.
>
>The enum cmd valu could stay as a placeholder, the rest can go.
>

It doesn't make much sense to have a command which is not implemented, same
as you wanted to remove enum values which are not used.

>
>>
>>>
>>>>
>>>>>>2) remove "no-added-value" static functions descriptions in
>>>>>>   dpll_core/dpll_netlink
>>>>
>>>>Removed.
>>>>
>>>>>>3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
>>>>>>   after each patch apply
>>>>
>>>>Hope Vadim will decide on this, the thing is merging in two patches
>>>>doesn't make much sense as there won't be any linking until both patches
>>>>are there, so most sense it would be if 3 are merged into one, but
>>>>then we will be back to one big blob patch issue.
>>>>
>>>>>>4) remove function return values descriptions/lists
>>>>
>>>>Fixed.
>>>>
>>>>>>5) Fix patch [05/10]:
>>>>>>   - status Supported
>>>>>>   - additional maintainers
>>>>>>   - remove callback:
>>>>>>     int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
>>>>>>6) Fix patch [08/10]: rethink ice mutex locking scheme
>>>>
>>>>Fixed.
>>>>
>>>>>>7) Fix patch [09/10]: multiple comments on
>>>>>>https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t
>>>>>>8) add PPS DPLL phase offset to the netlink get-device API
>>>>>>
>>>>
>>>>Added few things on this matter
>>>>- 1 dpll level attribute:
>>>>  - phase-shift - measuring the phase difference between dpll input
>>>>    and it's output
>>>>- 1 dpll-pin tuple level attribute:
>>>>  - pin-phase-adjust - set/get phase adjust of a pin on a dpll
>>>>- 2 pin level attributes:
>>>>  - pin-phase-adjust-min - provide user with min value that can be set
>>>>  - pin-phase-adjust-max - provide user with max value that can be set
>>>>- a constant:
>>>>  - DPLL_PHASE_SHIFT_DIVIDER similar to DPLL_TEMP_DIVIDER for producing
>>>>    fraction value of measured DPLL_A_PHASE_SHIFT
>>>
>>>Again, why do we need this in this initial submission? Why it can't be a
>>>follow-up patchset to extend this? This way we never converge :/
>>>Please focus on what we have now and bring it in. Let the extensions to
>>>be addressed later on, please.
>>>
>>
>>Well AFAIK, RHEL is doing some monitoring software, so the end-users need
>>this.
>
>They need it for the initial submission? Why? Why can't they wait 1 week
>for follow-up patchset?
>

Probably best if they could respond here, though I know they are waiting for
the dpll interface for a long time already.

>
>>
>>>
>>>
>>>>- implemented in dpll netlink and in ice
>>>>
>>>>>
>>>>>You are missing removal of pin->prop.package_label = dev_name(dev); in
>>>>>ice.
>>>>>
>>>>
>>>>I didn't touch it, as we still need to discuss it, Jakub didn't respond
>>>>on v8 thread.
>>>>I don't see why we shall not name it the way. This is most meaningful
>>>>label for those pins for the user right now.
>>>
>>>This is not meaningful, at all. dev_name() changes upon which pci slot
>>>you plug the card into. package_label should be an actual label on a
>>>silicon package. Why you think this two are related in aby way, makes me
>>>really wonder. Could you elaborate the meaningfulness of this?
>>>
>>
>>Without this, from end-user perspective, it would be very confusing.
>>As in ice without any label there would 4 pins which differs only with id.
>
>There you go, it does not have any label, yet you are trying hard to
>make up some. Does not make sense.
>

Don't get this, they have the label, but you ask to remove it..

>
>>What names would you suggest?
>
>That is the point I made previously. For synce usecase, the label does
>not make sense. There should be no label. You reference the pin by ID
>from netdev, that is enough.
>

Yea I understand, you are trying to hide this information from the user,
while I am trying not to hide anything, and let the user know all the
information can be somehow useful, this is the difference.

The one might want to ask how label indicating internal pin label is useful,
it is transparency, the label is there for identification nothing else.

You mean SyncE use case where a software daemon controls the dpll for
SyncE implementation, and this is already implemented and works as you
described.

>I think better to add the check to pin-register so future synce pin
>users don't have similar weird ideas. Could you please add this check?
>

Don't think it is way to go, and I don't think there is anything good
with preventing device drivers from labeling their pins the way they want.

I don't understand why all the pins shall be targeted differently then SyncE
ones, I mean the SyncE pins are special case (of dpll subsystem), and for that
case, they are also targetable by netdevice, but what I don't understand is
why they shall be only targetable with netdevice, they are still part of dpll
subsystem not a SyncE subsystem, and with this understanding shall be
targetable like all the other pins. While without the label this will not be
possible.

Thank you!
Arkadiusz

>Thanks!
>
>

[...]

On Tue, 11 Jul 2023 17:17:51 +0000 Kubalewski, Arkadiusz wrote:
> >I think better to add the check to pin-register so future synce pin
> >users don't have similar weird ideas. Could you please add this check?
> 
> Don't think it is way to go, and I don't think there is anything good
> with preventing device drivers from labeling their pins the way they want.

We had a long argument about how label should have a clearly defined
meaning. We're not going to rehash it on every revision. What did 
I miss :|

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Tuesday, July 11, 2023 10:15 PM
>
>On Tue, 11 Jul 2023 17:17:51 +0000 Kubalewski, Arkadiusz wrote:
>> >I think better to add the check to pin-register so future synce pin
>> >users don't have similar weird ideas. Could you please add this check?
>>
>> Don't think it is way to go, and I don't think there is anything good
>> with preventing device drivers from labeling their pins the way they
>>want.
>
>We had a long argument about how label should have a clearly defined
>meaning. We're not going to rehash it on every revision. What did I miss :|

Well, as I understand we are discussing if dpll subsystem shall prevent
labeling the SyncE type pins. I have labeled them in ice explicitly with
the name of a pci device they belong to.

You haven't miss much, mostly the problem is described in this thread.

Thank you!
Arkadiusz

On Wed, 12 Jul 2023 09:19:53 +0000 Kubalewski, Arkadiusz wrote:
> >> Don't think it is way to go, and I don't think there is anything good
> >> with preventing device drivers from labeling their pins the way they
> >> want.  
> >
> >We had a long argument about how label should have a clearly defined
> >meaning. We're not going to rehash it on every revision. What did I miss :|  
> 
> Well, as I understand we are discussing if dpll subsystem shall prevent
> labeling the SyncE type pins. I have labeled them in ice explicitly with
> the name of a pci device they belong to.
> 
> You haven't miss much, mostly the problem is described in this thread.

Please read this thread:

https://lore.kernel.org/all/20230503191643.12a6e559@kernel.org/