Message ID | 20230623123820.42850-1-arkadiusz.kubalewski@intel.com (mailing list archive) |
---|---|
Headers | show |
Series | Create common DPLL configuration API | expand |
Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com wrote: >Implement common API for clock/DPLL configuration and status reporting. >The API utilises netlink interface as transport for commands and event >notifications. This API aim to extend current pin configuration and >make it flexible and easy to cover special configurations. > >Netlink interface is based on ynl spec, it allows use of in-kernel >tools/net/ynl/cli.py application to control the interface with properly >formated command and json attribute strings. Here are few command >examples of how it works with `ice` driver on supported NIC: I don't understand. The discussion in the RFCv8 thread is still going on. The things I mentioned there are ignored. Like for example: 1) mode_set op removal 2) odd ice dpll locking scheme (either fix or describe why it is ok - that's the unfinished discussion) 3) header file bits squash I suggested. Vadim wrote that it sounds reasonable, yet nothing changed I thought we are past this. Why I have to point the same issues over and over?
On Fri, 23 Jun 2023 17:19:11 +0200 Jiri Pirko wrote: > I don't understand. The discussion in the RFCv8 thread is still going > on. The things I mentioned there are ignored. Like for example: > 1) mode_set op removal > 2) odd ice dpll locking scheme (either fix or describe why it is ok - > that's the unfinished discussion) > 3) header file bits squash I suggested. Vadim wrote that it sounds > reasonable, yet nothing changed > > I thought we are past this. Why I have to point the same issues over and > over? FWIW I'm lost in the previous thread, so for me there's value in refreshing the series. But you're right, at the very least there should be a summary of outstanding issues / open items / ongoing discussions in the cover letter.
Fri, Jun 23, 2023 at 05:53:36PM CEST, kuba@kernel.org wrote: >On Fri, 23 Jun 2023 17:19:11 +0200 Jiri Pirko wrote: >> I don't understand. The discussion in the RFCv8 thread is still going >> on. The things I mentioned there are ignored. Like for example: >> 1) mode_set op removal >> 2) odd ice dpll locking scheme (either fix or describe why it is ok - >> that's the unfinished discussion) >> 3) header file bits squash I suggested. Vadim wrote that it sounds >> reasonable, yet nothing changed >> >> I thought we are past this. Why I have to point the same issues over and >> over? > >FWIW I'm lost in the previous thread, so for me there's value in >refreshing the series. > >But you're right, at the very least there should be a summary of >outstanding issues / open items / ongoing discussions in the cover >letter. Well I would like to conclude discussion in one thread before sending the next one. What should I do? Should I start the same discussion pointing out the same issues in this thread again? This can't work. Even concluded items are ignored, like 3) IDK, this is very frustrating for me. I have to double check everything just in case it was not ignored. I don't understand this, there is no justification.
On Sat, 24 Jun 2023 11:23:36 +0200 Jiri Pirko wrote: > Well I would like to conclude discussion in one thread before sending > the next one. What should I do? Should I start the same discussion > pointing out the same issues in this thread again? This can't work. > > Even concluded items are ignored, like 3) > > IDK, this is very frustrating for me. I have to double check everything > just in case it was not ignored. I don't understand this, there is no > justification. Yes, the open items need to be clearly stated on a new posting.
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!
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
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 >
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 >> > >
>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/