| Message ID | 20230428002009.2948020-1-vadfed@meta.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Create common DPLL configuration API | expand |
Fri, Apr 28, 2023 at 02:20:01AM CEST, vadfed@meta.com wrote: >From: Vadim Fedorenko <vadim.fedorenko@linux.dev> > >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. Vadim, I guess you forgot, could you please add some example commands here? Please keep them in for the next V. Thanks! > >v6 -> v7: > * YAML spec: > - remove nested 'pin' attribute > - clean up definitions on top of the latest changes > * pin object: > - pin xarray uses id provided by the driver > - remove usage of PIN_IDX_INVALID in set function > - source_pin_get() returns object instead of idx > - fixes in frequency support API > * device and pin operations are const now > * small fixes in naming in Makefile and in the functions > * single mutex for the subsystem to avoid possible ABBA locks > * no special *_priv() helpers anymore, private data is passed as void* > * no netlink filters by name anymore, only index is supported > * update ptp_ocp and ice drivers to follow new API version > * add mlx5e driver as a new customer of the subsystem >v5 -> v6: > * rework pin part to better fit shared pins use cases > * add YAML spec to easy generate user-space apps > * simple implementation in ptp_ocp is back again >v4 -> v5: > * fix code issues found during last reviews: > - replace cookie with clock id > - follow one naming schema in dpll subsys > - move function comments to dpll_core.c, fix exports > - remove single-use helper functions > - merge device register with alloc > - lock and unlock mutex on dpll device release > - move dpll_type to uapi header > - rename DPLLA_DUMP_FILTER to DPLLA_FILTER > - rename dpll_pin_state to dpll_pin_mode > - rename DPLL_MODE_FORCED to DPLL_MODE_MANUAL > - remove DPLL_CHANGE_PIN_TYPE enum value > * rewrite framework once again (Arkadiusz) > - add clock class: > Provide userspace with clock class value of DPLL with dpll device dump > netlink request. Clock class is assigned by driver allocating a dpll > device. Clock class values are defined as specified in: > ITU-T G.8273.2/Y.1368.2 recommendation. > - dpll device naming schema use new pattern: > "dpll_%s_%d_%d", where: > - %s - dev_name(parent) of parent device, > - %d (1) - enum value of dpll type, > - %d (2) - device index provided by parent device. > - new muxed/shared pin registration: > Let the kernel module to register a shared or muxed pin without finding > it or its parent. Instead use a parent/shared pin description to find > correct pin internally in dpll_core, simplifing a dpll API > * Implement complex DPLL design in ice driver (Arkadiusz) > * Remove ptp_ocp driver from the series for now >v3 -> v4: > * redesign framework to make pins dynamically allocated (Arkadiusz) > * implement shared pins (Arkadiusz) >v2 -> v3: > * implement source select mode (Arkadiusz) > * add documentation > * implementation improvements (Jakub) >v1 -> v2: > * implement returning supported input/output types > * ptp_ocp: follow suggestions from Jonathan > * add linux-clk mailing list >v0 -> v1: > * fix code style and errors > * add linux-arm mailing list > >Arkadiusz Kubalewski (3): > dpll: spec: Add Netlink spec in YAML > ice: add admin commands to access cgu configuration > ice: implement dpll interface to control cgu > >Jiri Pirko (2): > netdev: expose DPLL pin handle for netdevice > mlx5: Implement SyncE support using DPLL infrastructure > >Vadim Fedorenko (3): > dpll: Add DPLL framework base functions > dpll: documentation on DPLL subsystem interface > ptp_ocp: implement DPLL ops > > Documentation/dpll.rst | 408 ++++ > Documentation/netlink/specs/dpll.yaml | 472 ++++ > Documentation/networking/index.rst | 1 + > MAINTAINERS | 8 + > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/dpll/Kconfig | 7 + > drivers/dpll/Makefile | 10 + > drivers/dpll/dpll_core.c | 939 ++++++++ > drivers/dpll/dpll_core.h | 113 + > drivers/dpll/dpll_netlink.c | 991 +++++++++ > drivers/dpll/dpll_netlink.h | 27 + > drivers/dpll/dpll_nl.c | 126 ++ > drivers/dpll/dpll_nl.h | 42 + > drivers/net/ethernet/intel/Kconfig | 1 + > drivers/net/ethernet/intel/ice/Makefile | 3 +- > drivers/net/ethernet/intel/ice/ice.h | 5 + > .../net/ethernet/intel/ice/ice_adminq_cmd.h | 240 +- > drivers/net/ethernet/intel/ice/ice_common.c | 467 ++++ > drivers/net/ethernet/intel/ice/ice_common.h | 43 + > drivers/net/ethernet/intel/ice/ice_dpll.c | 1929 +++++++++++++++++ > drivers/net/ethernet/intel/ice/ice_dpll.h | 101 + > drivers/net/ethernet/intel/ice/ice_lib.c | 17 +- > drivers/net/ethernet/intel/ice/ice_main.c | 7 + > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 414 ++++ > drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 230 ++ > drivers/net/ethernet/intel/ice/ice_type.h | 1 + > .../net/ethernet/mellanox/mlx5/core/Kconfig | 8 + > .../net/ethernet/mellanox/mlx5/core/Makefile | 3 + > drivers/net/ethernet/mellanox/mlx5/core/dev.c | 17 + > .../net/ethernet/mellanox/mlx5/core/dpll.c | 438 ++++ > drivers/ptp/Kconfig | 1 + > drivers/ptp/ptp_ocp.c | 327 ++- > include/linux/dpll.h | 294 +++ > include/linux/mlx5/driver.h | 2 + > include/linux/mlx5/mlx5_ifc.h | 59 +- > include/linux/netdevice.h | 7 + > include/uapi/linux/dpll.h | 204 ++ > include/uapi/linux/if_link.h | 2 + > net/core/dev.c | 20 + > net/core/rtnetlink.c | 38 + > 41 files changed, 7966 insertions(+), 59 deletions(-) > create mode 100644 Documentation/dpll.rst > create mode 100644 Documentation/netlink/specs/dpll.yaml > create mode 100644 drivers/dpll/Kconfig > create mode 100644 drivers/dpll/Makefile > create mode 100644 drivers/dpll/dpll_core.c > create mode 100644 drivers/dpll/dpll_core.h > create mode 100644 drivers/dpll/dpll_netlink.c > create mode 100644 drivers/dpll/dpll_netlink.h > create mode 100644 drivers/dpll/dpll_nl.c > create mode 100644 drivers/dpll/dpll_nl.h > create mode 100644 drivers/net/ethernet/intel/ice/ice_dpll.c > create mode 100644 drivers/net/ethernet/intel/ice/ice_dpll.h > create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/dpll.c > create mode 100644 include/linux/dpll.h > create mode 100644 include/uapi/linux/dpll.h > >-- >2.34.1 >
Fri, Apr 28, 2023 at 02:20:01AM CEST, vadfed@meta.com wrote: >From: Vadim Fedorenko <vadim.fedorenko@linux.dev> [...] >Arkadiusz Kubalewski (3): > dpll: spec: Add Netlink spec in YAML > ice: add admin commands to access cgu configuration > ice: implement dpll interface to control cgu > >Jiri Pirko (2): > netdev: expose DPLL pin handle for netdevice Arkadiusz, could you please expose pin for netdev in ice as well? > mlx5: Implement SyncE support using DPLL infrastructure [...]
Fri, Apr 28, 2023 at 02:20:01AM CEST, vadfed@meta.com wrote: >From: Vadim Fedorenko <vadim.fedorenko@linux.dev> > >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. > >v6 -> v7: > * YAML spec: > - remove nested 'pin' attribute > - clean up definitions on top of the latest changes > * pin object: > - pin xarray uses id provided by the driver > - remove usage of PIN_IDX_INVALID in set function > - source_pin_get() returns object instead of idx > - fixes in frequency support API > * device and pin operations are const now > * small fixes in naming in Makefile and in the functions > * single mutex for the subsystem to avoid possible ABBA locks > * no special *_priv() helpers anymore, private data is passed as void* > * no netlink filters by name anymore, only index is supported > * update ptp_ocp and ice drivers to follow new API version > * add mlx5e driver as a new customer of the subsystem >v5 -> v6: > * rework pin part to better fit shared pins use cases > * add YAML spec to easy generate user-space apps > * simple implementation in ptp_ocp is back again >v4 -> v5: > * fix code issues found during last reviews: > - replace cookie with clock id > - follow one naming schema in dpll subsys > - move function comments to dpll_core.c, fix exports > - remove single-use helper functions > - merge device register with alloc > - lock and unlock mutex on dpll device release > - move dpll_type to uapi header > - rename DPLLA_DUMP_FILTER to DPLLA_FILTER > - rename dpll_pin_state to dpll_pin_mode > - rename DPLL_MODE_FORCED to DPLL_MODE_MANUAL > - remove DPLL_CHANGE_PIN_TYPE enum value > * rewrite framework once again (Arkadiusz) > - add clock class: > Provide userspace with clock class value of DPLL with dpll device dump > netlink request. Clock class is assigned by driver allocating a dpll > device. Clock class values are defined as specified in: > ITU-T G.8273.2/Y.1368.2 recommendation. > - dpll device naming schema use new pattern: > "dpll_%s_%d_%d", where: > - %s - dev_name(parent) of parent device, > - %d (1) - enum value of dpll type, > - %d (2) - device index provided by parent device. > - new muxed/shared pin registration: > Let the kernel module to register a shared or muxed pin without finding > it or its parent. Instead use a parent/shared pin description to find > correct pin internally in dpll_core, simplifing a dpll API > * Implement complex DPLL design in ice driver (Arkadiusz) > * Remove ptp_ocp driver from the series for now >v3 -> v4: > * redesign framework to make pins dynamically allocated (Arkadiusz) > * implement shared pins (Arkadiusz) >v2 -> v3: > * implement source select mode (Arkadiusz) > * add documentation > * implementation improvements (Jakub) >v1 -> v2: > * implement returning supported input/output types > * ptp_ocp: follow suggestions from Jonathan > * add linux-clk mailing list >v0 -> v1: > * fix code style and errors > * add linux-arm mailing list Vadim, did you try ynl monitor? I think there might be something wrong with the yaml spec: # ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --subscribe monitor --sleep 10 Unexpected msg id done while checking for ntf nl_len = 92 (76) nl_flags = 0x0 nl_type = 19
Let me summarize the outcome of the discussion between me and Jakub
regarding attributes, handles and ID lookups in the RFCv7 thread:
--------------------------------------------------------------
** Needed changes for RFCv8 **
1) No scoped indexes.
The indexes passed from driver to dpll core during call of:
dpll_device_get() - device_idx
dpll_pin_get() - pin_idx
should be for INTERNAL kernel use only and NOT EXPOSED over uapi.
Therefore following attributes need to be removed:
DPLL_A_PIN_IDX
DPLL_A_PIN_PARENT_IDX
2) For device, the handle will be DPLL_A_ID == dpll->id.
This will be the only handle for device for every
device related GET, SET command and every device related notification.
Note: this ID is not deterministing and may be different depending on
order of device probes etc.
3) For pin, the handle will be DPLL_A_PIN_ID == pin->id
This will be the only handle for pin for every
pin related GET, SET command and every pin related notification.
Note: this ID is not deterministing and may be different depending on
order of device probes etc.
4) Remove attribute:
DPLL_A_PIN_LABEL
and replace it with:
DPLL_A_PIN_PANEL_LABEL (string)
DPLL_A_PIN_XXX (string)
where XXX is a label type, like for example:
DPLL_A_PIN_BOARD_LABEL
DPLL_A_PIN_BOARD_TRACE
DPLL_A_PIN_PACKAGE_PIN
5) Make sure you expose following attributes for every device and
pin GET/DUMP command reply message:
DPLL_A_MODULE_NAME
DPLL_A_CLOCK_ID
6) Remove attributes:
DPLL_A_DEV_NAME
DPLL_A_BUS_NAME
as they no longer have any value and do no make sense (even in RFCv7)
--------------------------------------------------------------
** Lookup commands **
Basically these would allow user to query DEVICE_ID and PIN_ID
according to provided atributes (see examples below).
These would be from my perspective optional for this patchsets.
I believe we can do it as follow-up if needed. For example for mlx5
I don't have usecase for it, since I can consistently get PIN_ID
using RT netlink for given netdev. But I can imagine that for non-SyncE
dpll driver this would make sense to have.
1) Introduce CMD_GET_ID - query the kernel for a dpll device
specified by given attrs
Example:
-> DPLL_A_MODULE_NAME
DPLL_A_CLOCK_ID
DPLL_A_TYPE
<- DPLL_A_ID
Example:
-> DPLL_A_MODULE_NAME
DPLL_A_CLOCK_ID
<- DPLL_A_ID
Example:
-> DPLL_A_MODULE_NAME
<- -EINVAL (Extack: "multiple devices matched")
If user passes a subset of attrs which would not result in
a single match, kernel returns -EINVAL and proper extack message.
2) Introduce CMD_GET_PIN_ID - query the kernel for a dpll pin
specified by given attrs
Example:
-> DPLL_A_MODULE_NAME
DPLL_A_CLOCK_ID
DPLL_A_PIN_TYPE
DPLL_A_PIN_PANEL_LABEL
<- DPLL_A_PIN_ID
Example:
-> DPLL_A_MODULE_NAME
DPLL_A_CLOCK_ID
<- DPLL_A_PIN_ID (There was only one pin for given module/clock_id)
Example:
-> DPLL_A_MODULE_NAME
DPLL_A_CLOCK_ID
<- -EINVAL (Extack: "multiple pins matched")
If user passes a subset of attrs which would not result in
a single match, kernel returns -EINVAL and proper extack message.
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Tuesday, May 2, 2023 3:04 PM > >Fri, Apr 28, 2023 at 02:20:01AM CEST, vadfed@meta.com wrote: >>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> > >[...] > >>Arkadiusz Kubalewski (3): >> dpll: spec: Add Netlink spec in YAML >> ice: add admin commands to access cgu configuration >> ice: implement dpll interface to control cgu >> >>Jiri Pirko (2): >> netdev: expose DPLL pin handle for netdevice > >Arkadiusz, could you please expose pin for netdev in ice as well? > > >> mlx5: Implement SyncE support using DPLL infrastructure > >[...] Yes, will do. Thank you, Arkadiusz
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, May 11, 2023 9:53 AM > >Fri, Apr 28, 2023 at 02:20:01AM CEST, vadfed@meta.com wrote: >>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> >>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. >> >>v6 -> v7: >> * YAML spec: >> - remove nested 'pin' attribute >> - clean up definitions on top of the latest changes >> * pin object: >> - pin xarray uses id provided by the driver >> - remove usage of PIN_IDX_INVALID in set function >> - source_pin_get() returns object instead of idx >> - fixes in frequency support API >> * device and pin operations are const now >> * small fixes in naming in Makefile and in the functions >> * single mutex for the subsystem to avoid possible ABBA locks >> * no special *_priv() helpers anymore, private data is passed as void* >> * no netlink filters by name anymore, only index is supported >> * update ptp_ocp and ice drivers to follow new API version >> * add mlx5e driver as a new customer of the subsystem >>v5 -> v6: >> * rework pin part to better fit shared pins use cases >> * add YAML spec to easy generate user-space apps >> * simple implementation in ptp_ocp is back again >>v4 -> v5: >> * fix code issues found during last reviews: >> - replace cookie with clock id >> - follow one naming schema in dpll subsys >> - move function comments to dpll_core.c, fix exports >> - remove single-use helper functions >> - merge device register with alloc >> - lock and unlock mutex on dpll device release >> - move dpll_type to uapi header >> - rename DPLLA_DUMP_FILTER to DPLLA_FILTER >> - rename dpll_pin_state to dpll_pin_mode >> - rename DPLL_MODE_FORCED to DPLL_MODE_MANUAL >> - remove DPLL_CHANGE_PIN_TYPE enum value >> * rewrite framework once again (Arkadiusz) >> - add clock class: >> Provide userspace with clock class value of DPLL with dpll device dump >> netlink request. Clock class is assigned by driver allocating a dpll >> device. Clock class values are defined as specified in: >> ITU-T G.8273.2/Y.1368.2 recommendation. >> - dpll device naming schema use new pattern: >> "dpll_%s_%d_%d", where: >> - %s - dev_name(parent) of parent device, >> - %d (1) - enum value of dpll type, >> - %d (2) - device index provided by parent device. >> - new muxed/shared pin registration: >> Let the kernel module to register a shared or muxed pin without finding >> it or its parent. Instead use a parent/shared pin description to find >> correct pin internally in dpll_core, simplifing a dpll API >> * Implement complex DPLL design in ice driver (Arkadiusz) >> * Remove ptp_ocp driver from the series for now >>v3 -> v4: >> * redesign framework to make pins dynamically allocated (Arkadiusz) >> * implement shared pins (Arkadiusz) >>v2 -> v3: >> * implement source select mode (Arkadiusz) >> * add documentation >> * implementation improvements (Jakub) >>v1 -> v2: >> * implement returning supported input/output types >> * ptp_ocp: follow suggestions from Jonathan >> * add linux-clk mailing list >>v0 -> v1: >> * fix code style and errors >> * add linux-arm mailing list > >Vadim, did you try ynl monitor? I think there might be something wrong >with the yaml spec: ># ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml -- >subscribe monitor --sleep 10 >Unexpected msg id done while checking for ntf nl_len = 92 (76) nl_flags = >0x0 nl_type = 19 > One of the commits I have prepared already replaces old notifications with the ones suggested previously, where format of notification is the same as format of get command. It works there, altough not sure if it works only for me or for everyone, I might have done some modifications to ynl-lib itself, need to double check that. Thank you, Arkadiusz
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Wednesday, May 17, 2023 12:19 PM > >Let me summarize the outcome of the discussion between me and Jakub >regarding attributes, handles and ID lookups in the RFCv7 thread: > >-------------------------------------------------------------- >** Needed changes for RFCv8 ** > >1) No scoped indexes. > The indexes passed from driver to dpll core during call of: > dpll_device_get() - device_idx > dpll_pin_get() - pin_idx > should be for INTERNAL kernel use only and NOT EXPOSED over uapi. > Therefore following attributes need to be removed: > DPLL_A_PIN_IDX > DPLL_A_PIN_PARENT_IDX > Seems doable. So just to be clear, configuring a pin-pair (MUXed pins) will now be done with DPLL_A_PIN_PARENT nested attribute. I.e. configuring state of pin on parent: DPLL_CMD_PIN_SET DPLL_A_PIN_ID (id of pin being configured) DPLL_A_PIN_PARENT (nested) DPLL_A_PIN_ID (id of parent pin) DPLL_A_PIN_STATE(expected state) ... (other pin-pair attributes to be set) Is that ok, or we need separated attribute like DPLL_A_PIN_PARENT_ID?? I think there is no need for separated one, documentation shall just reflect that. Also we have nested attribute DPLL_A_DEVICE which is used to show connections between PIN and multiple dpll devices, to make it consistent I will rename it to `DPLL_A_DEVICE_PARENT` and make configuration set cmd for the pin-dpll pair similar to the above: DPLL_CMD_PIN_SET DPLL_A_PIN_ID (id of pin being configured) DPLL_A_DEVICE_PARENT (nested) DPLL_A_ID (id of parent dpll) DPLL_A_PIN_STATE(expected state) ... (other pin-dpll attributes to be set) Does it make sense? >2) For device, the handle will be DPLL_A_ID == dpll->id. > This will be the only handle for device for every > device related GET, SET command and every device related notification. > Note: this ID is not deterministing and may be different depending on > order of device probes etc. > Seems doable. >3) For pin, the handle will be DPLL_A_PIN_ID == pin->id > This will be the only handle for pin for every > pin related GET, SET command and every pin related notification. > Note: this ID is not deterministing and may be different depending on > order of device probes etc. > Seems doable. >4) Remove attribute: > DPLL_A_PIN_LABEL > and replace it with: > DPLL_A_PIN_PANEL_LABEL (string) > DPLL_A_PIN_XXX (string) > where XXX is a label type, like for example: > DPLL_A_PIN_BOARD_LABEL > DPLL_A_PIN_BOARD_TRACE > DPLL_A_PIN_PACKAGE_PIN > Sorry, I don't get this idea, what are those types? What are they for? >5) Make sure you expose following attributes for every device and > pin GET/DUMP command reply message: > DPLL_A_MODULE_NAME > DPLL_A_CLOCK_ID > Seems doable. >6) Remove attributes: > DPLL_A_DEV_NAME > DPLL_A_BUS_NAME > as they no longer have any value and do no make sense (even in RFCv7) > Seems doable. > >-------------------------------------------------------------- >** Lookup commands ** > >Basically these would allow user to query DEVICE_ID and PIN_ID >according to provided atributes (see examples below). > >These would be from my perspective optional for this patchsets. >I believe we can do it as follow-up if needed. For example for mlx5 >I don't have usecase for it, since I can consistently get PIN_ID >using RT netlink for given netdev. But I can imagine that for non-SyncE >dpll driver this would make sense to have. > >1) Introduce CMD_GET_ID - query the kernel for a dpll device > specified by given attrs > Example: > -> DPLL_A_MODULE_NAME > DPLL_A_CLOCK_ID > DPLL_A_TYPE > <- DPLL_A_ID > Example: > -> DPLL_A_MODULE_NAME > DPLL_A_CLOCK_ID > <- DPLL_A_ID > Example: > -> DPLL_A_MODULE_NAME > <- -EINVAL (Extack: "multiple devices matched") > > If user passes a subset of attrs which would not result in > a single match, kernel returns -EINVAL and proper extack message. > Seems ok. >2) Introduce CMD_GET_PIN_ID - query the kernel for a dpll pin > specified by given attrs > Example: > -> DPLL_A_MODULE_NAME > DPLL_A_CLOCK_ID > DPLL_A_PIN_TYPE > DPLL_A_PIN_PANEL_LABEL > <- DPLL_A_PIN_ID > Example: > -> DPLL_A_MODULE_NAME > DPLL_A_CLOCK_ID > <- DPLL_A_PIN_ID (There was only one pin for given module/clock_id) > Example: > -> DPLL_A_MODULE_NAME > DPLL_A_CLOCK_ID > <- -EINVAL (Extack: "multiple pins matched") > > If user passes a subset of attrs which would not result in > a single match, kernel returns -EINVAL and proper extack message. Seems ok. Will try to implement those now. Thank you, Arkadiusz
Fri, May 26, 2023 at 12:14:00PM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Wednesday, May 17, 2023 12:19 PM >> >>Let me summarize the outcome of the discussion between me and Jakub >>regarding attributes, handles and ID lookups in the RFCv7 thread: >> >>-------------------------------------------------------------- >>** Needed changes for RFCv8 ** >> >>1) No scoped indexes. >> The indexes passed from driver to dpll core during call of: >> dpll_device_get() - device_idx >> dpll_pin_get() - pin_idx >> should be for INTERNAL kernel use only and NOT EXPOSED over uapi. >> Therefore following attributes need to be removed: >> DPLL_A_PIN_IDX >> DPLL_A_PIN_PARENT_IDX >> > >Seems doable. >So just to be clear, configuring a pin-pair (MUXed pins) will now be done >with DPLL_A_PIN_PARENT nested attribute. >I.e. configuring state of pin on parent: >DPLL_CMD_PIN_SET > DPLL_A_PIN_ID (id of pin being configured) > DPLL_A_PIN_PARENT (nested) > DPLL_A_PIN_ID (id of parent pin) > DPLL_A_PIN_STATE(expected state) > ... (other pin-pair attributes to be set) > >Is that ok, or we need separated attribute like DPLL_A_PIN_PARENT_ID?? >I think there is no need for separated one, documentation shall just reflect >that. >Also we have nested attribute DPLL_A_DEVICE which is used to show connections >between PIN and multiple dpll devices, to make it consistent I will rename >it to `DPLL_A_DEVICE_PARENT` and make configuration set cmd for the pin-dpll >pair similar to the above: >DPLL_CMD_PIN_SET > DPLL_A_PIN_ID (id of pin being configured) > DPLL_A_DEVICE_PARENT (nested) It is a parent of pin, not device. The name is confusing. But see below. > DPLL_A_ID (id of parent dpll) > DPLL_A_PIN_STATE(expected state) > ... (other pin-dpll attributes to be set) > >Does it make sense? Yeah, good idea. I like this. We will have consistent approach for parent pin and device. To take it even further, we can have one nested attr for parent and decide the parent type according to the id attr given: DPLL_CMD_PIN_SET DPLL_A_PIN_ID (id of pin being configured) DPLL_A_PIN_PARENT (nested) DPLL_A_PIN_ID (id of parent pin) DPLL_A_PIN_STATE(expected state) ... (other pin-pair attributes to be set) DPLL_CMD_PIN_SET DPLL_A_PIN_ID (id of pin being configured) DPLL_A_PIN_PARENT (nested) DPLL_A_ID (id of parent dpll) DPLL_A_PIN_STATE(expected state) ... (other pin-dpll attributes to be set) Same for PIN_GET Makes sense? > > >>2) For device, the handle will be DPLL_A_ID == dpll->id. >> This will be the only handle for device for every >> device related GET, SET command and every device related notification. >> Note: this ID is not deterministing and may be different depending on >> order of device probes etc. >> > >Seems doable. > >>3) For pin, the handle will be DPLL_A_PIN_ID == pin->id >> This will be the only handle for pin for every >> pin related GET, SET command and every pin related notification. >> Note: this ID is not deterministing and may be different depending on >> order of device probes etc. >> > >Seems doable. > >>4) Remove attribute: >> DPLL_A_PIN_LABEL >> and replace it with: >> DPLL_A_PIN_PANEL_LABEL (string) >> DPLL_A_PIN_XXX (string) >> where XXX is a label type, like for example: >> DPLL_A_PIN_BOARD_LABEL >> DPLL_A_PIN_BOARD_TRACE >> DPLL_A_PIN_PACKAGE_PIN >> > >Sorry, I don't get this idea, what are those types? >What are they for? The point is to make the driver developer to think before passing randomly constructed label strings. For example, "board_label" would lead the developer to check how the pin is labeled on the board. The "panel_label" indicates this is label on a panel. Also, developer can fill multiple labels for the same pin. > >>5) Make sure you expose following attributes for every device and >> pin GET/DUMP command reply message: >> DPLL_A_MODULE_NAME >> DPLL_A_CLOCK_ID >> > >Seems doable. > >>6) Remove attributes: >> DPLL_A_DEV_NAME >> DPLL_A_BUS_NAME >> as they no longer have any value and do no make sense (even in RFCv7) >> > >Seems doable. > >> >>-------------------------------------------------------------- >>** Lookup commands ** >> >>Basically these would allow user to query DEVICE_ID and PIN_ID >>according to provided atributes (see examples below). >> >>These would be from my perspective optional for this patchsets. >>I believe we can do it as follow-up if needed. For example for mlx5 >>I don't have usecase for it, since I can consistently get PIN_ID >>using RT netlink for given netdev. But I can imagine that for non-SyncE >>dpll driver this would make sense to have. >> >>1) Introduce CMD_GET_ID - query the kernel for a dpll device >> specified by given attrs >> Example: >> -> DPLL_A_MODULE_NAME >> DPLL_A_CLOCK_ID >> DPLL_A_TYPE >> <- DPLL_A_ID >> Example: >> -> DPLL_A_MODULE_NAME >> DPLL_A_CLOCK_ID >> <- DPLL_A_ID >> Example: >> -> DPLL_A_MODULE_NAME >> <- -EINVAL (Extack: "multiple devices matched") >> >> If user passes a subset of attrs which would not result in >> a single match, kernel returns -EINVAL and proper extack message. >> > >Seems ok. > >>2) Introduce CMD_GET_PIN_ID - query the kernel for a dpll pin >> specified by given attrs >> Example: >> -> DPLL_A_MODULE_NAME >> DPLL_A_CLOCK_ID >> DPLL_A_PIN_TYPE >> DPLL_A_PIN_PANEL_LABEL >> <- DPLL_A_PIN_ID >> Example: >> -> DPLL_A_MODULE_NAME >> DPLL_A_CLOCK_ID >> <- DPLL_A_PIN_ID (There was only one pin for given module/clock_id) >> Example: >> -> DPLL_A_MODULE_NAME >> DPLL_A_CLOCK_ID >> <- -EINVAL (Extack: "multiple pins matched") >> >> If user passes a subset of attrs which would not result in >> a single match, kernel returns -EINVAL and proper extack message. > > >Seems ok. > >Will try to implement those now. Cool, thx! > >Thank you, >Arkadiusz
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Friday, May 26, 2023 12:39 PM > >Fri, May 26, 2023 at 12:14:00PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Wednesday, May 17, 2023 12:19 PM >>> >>>Let me summarize the outcome of the discussion between me and Jakub >>>regarding attributes, handles and ID lookups in the RFCv7 thread: >>> >>>-------------------------------------------------------------- >>>** Needed changes for RFCv8 ** >>> >>>1) No scoped indexes. >>> The indexes passed from driver to dpll core during call of: >>> dpll_device_get() - device_idx >>> dpll_pin_get() - pin_idx >>> should be for INTERNAL kernel use only and NOT EXPOSED over uapi. >>> Therefore following attributes need to be removed: >>> DPLL_A_PIN_IDX >>> DPLL_A_PIN_PARENT_IDX >>> >> >>Seems doable. >>So just to be clear, configuring a pin-pair (MUXed pins) will now be done >>with DPLL_A_PIN_PARENT nested attribute. >>I.e. configuring state of pin on parent: >>DPLL_CMD_PIN_SET >> DPLL_A_PIN_ID (id of pin being configured) >> DPLL_A_PIN_PARENT (nested) >> DPLL_A_PIN_ID (id of parent pin) >> DPLL_A_PIN_STATE(expected state) >> ... (other pin-pair attributes to be set) >> >>Is that ok, or we need separated attribute like DPLL_A_PIN_PARENT_ID?? >>I think there is no need for separated one, documentation shall just >>reflect that. >>Also we have nested attribute DPLL_A_DEVICE which is used to show >connections >>between PIN and multiple dpll devices, to make it consistent I will rename >>it to `DPLL_A_DEVICE_PARENT` and make configuration set cmd for the pin-dpll >>pair similar to the above: >>DPLL_CMD_PIN_SET >> DPLL_A_PIN_ID (id of pin being configured) >> DPLL_A_DEVICE_PARENT (nested) > >It is a parent of pin, not device. The name is confusing. But see below. > > >> DPLL_A_ID (id of parent dpll) >> DPLL_A_PIN_STATE(expected state) >> ... (other pin-dpll attributes to be set) >> >>Does it make sense? > >Yeah, good idea. I like this. We will have consistent approach for >parent pin and device. To take it even further, we can have one nested >attr for parent and decide the parent type according to the id attr >given: > >DPLL_CMD_PIN_SET > DPLL_A_PIN_ID (id of pin being configured) > DPLL_A_PIN_PARENT (nested) > DPLL_A_PIN_ID (id of parent pin) > DPLL_A_PIN_STATE(expected state) > ... (other pin-pair attributes to be set) > >DPLL_CMD_PIN_SET > DPLL_A_PIN_ID (id of pin being configured) > DPLL_A_PIN_PARENT (nested) > DPLL_A_ID (id of parent dpll) > DPLL_A_PIN_STATE(expected state) > ... (other pin-dpll attributes to be set) > > >Same for PIN_GET > >Makes sense? > Sure, fixed. > > >> >> >>>2) For device, the handle will be DPLL_A_ID == dpll->id. >>> This will be the only handle for device for every >>> device related GET, SET command and every device related notification. >>> Note: this ID is not deterministing and may be different depending on >>> order of device probes etc. >>> >> >>Seems doable. >> >>>3) For pin, the handle will be DPLL_A_PIN_ID == pin->id >>> This will be the only handle for pin for every >>> pin related GET, SET command and every pin related notification. >>> Note: this ID is not deterministing and may be different depending on >>> order of device probes etc. >>> >> >>Seems doable. >> >>>4) Remove attribute: >>> DPLL_A_PIN_LABEL >>> and replace it with: >>> DPLL_A_PIN_PANEL_LABEL (string) >>> DPLL_A_PIN_XXX (string) >>> where XXX is a label type, like for example: >>> DPLL_A_PIN_BOARD_LABEL >>> DPLL_A_PIN_BOARD_TRACE >>> DPLL_A_PIN_PACKAGE_PIN >>> >> >>Sorry, I don't get this idea, what are those types? >>What are they for? > >The point is to make the driver developer to think before passing >randomly constructed label strings. For example, "board_label" would lead >the developer to check how the pin is labeled on the board. The >"panel_label" indicates this is label on a panel. Also, developer can >fill multiple labels for the same pin. > Ok, makes sense, added as suggested. Thank you, Arkadiusz > > >> >>>5) Make sure you expose following attributes for every device and >>> pin GET/DUMP command reply message: >>> DPLL_A_MODULE_NAME >>> DPLL_A_CLOCK_ID >>> >> >>Seems doable. >> >>>6) Remove attributes: >>> DPLL_A_DEV_NAME >>> DPLL_A_BUS_NAME >>> as they no longer have any value and do no make sense (even in RFCv7) >>> >> >>Seems doable. >> >>> >>>-------------------------------------------------------------- >>>** Lookup commands ** >>> >>>Basically these would allow user to query DEVICE_ID and PIN_ID >>>according to provided atributes (see examples below). >>> >>>These would be from my perspective optional for this patchsets. >>>I believe we can do it as follow-up if needed. For example for mlx5 >>>I don't have usecase for it, since I can consistently get PIN_ID >>>using RT netlink for given netdev. But I can imagine that for non-SyncE >>>dpll driver this would make sense to have. >>> >>>1) Introduce CMD_GET_ID - query the kernel for a dpll device >>> specified by given attrs >>> Example: >>> -> DPLL_A_MODULE_NAME >>> DPLL_A_CLOCK_ID >>> DPLL_A_TYPE >>> <- DPLL_A_ID >>> Example: >>> -> DPLL_A_MODULE_NAME >>> DPLL_A_CLOCK_ID >>> <- DPLL_A_ID >>> Example: >>> -> DPLL_A_MODULE_NAME >>> <- -EINVAL (Extack: "multiple devices matched") >>> >>> If user passes a subset of attrs which would not result in >>> a single match, kernel returns -EINVAL and proper extack message. >>> >> >>Seems ok. >> >>>2) Introduce CMD_GET_PIN_ID - query the kernel for a dpll pin >>> specified by given attrs >>> Example: >>> -> DPLL_A_MODULE_NAME >>> DPLL_A_CLOCK_ID >>> DPLL_A_PIN_TYPE >>> DPLL_A_PIN_PANEL_LABEL >>> <- DPLL_A_PIN_ID >>> Example: >>> -> DPLL_A_MODULE_NAME >>> DPLL_A_CLOCK_ID >>> <- DPLL_A_PIN_ID (There was only one pin for given module/clock_id) >>> Example: >>> -> DPLL_A_MODULE_NAME >>> DPLL_A_CLOCK_ID >>> <- -EINVAL (Extack: "multiple pins matched") >>> >>> If user passes a subset of attrs which would not result in >>> a single match, kernel returns -EINVAL and proper extack message. >> >> >>Seems ok. >> >>Will try to implement those now. > >Cool, thx! > > >> >>Thank you, >>Arkadiusz
From: Vadim Fedorenko <vadim.fedorenko@linux.dev> 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. v6 -> v7: * YAML spec: - remove nested 'pin' attribute - clean up definitions on top of the latest changes * pin object: - pin xarray uses id provided by the driver - remove usage of PIN_IDX_INVALID in set function - source_pin_get() returns object instead of idx - fixes in frequency support API * device and pin operations are const now * small fixes in naming in Makefile and in the functions * single mutex for the subsystem to avoid possible ABBA locks * no special *_priv() helpers anymore, private data is passed as void* * no netlink filters by name anymore, only index is supported * update ptp_ocp and ice drivers to follow new API version * add mlx5e driver as a new customer of the subsystem v5 -> v6: * rework pin part to better fit shared pins use cases * add YAML spec to easy generate user-space apps * simple implementation in ptp_ocp is back again v4 -> v5: * fix code issues found during last reviews: - replace cookie with clock id - follow one naming schema in dpll subsys - move function comments to dpll_core.c, fix exports - remove single-use helper functions - merge device register with alloc - lock and unlock mutex on dpll device release - move dpll_type to uapi header - rename DPLLA_DUMP_FILTER to DPLLA_FILTER - rename dpll_pin_state to dpll_pin_mode - rename DPLL_MODE_FORCED to DPLL_MODE_MANUAL - remove DPLL_CHANGE_PIN_TYPE enum value * rewrite framework once again (Arkadiusz) - add clock class: Provide userspace with clock class value of DPLL with dpll device dump netlink request. Clock class is assigned by driver allocating a dpll device. Clock class values are defined as specified in: ITU-T G.8273.2/Y.1368.2 recommendation. - dpll device naming schema use new pattern: "dpll_%s_%d_%d", where: - %s - dev_name(parent) of parent device, - %d (1) - enum value of dpll type, - %d (2) - device index provided by parent device. - new muxed/shared pin registration: Let the kernel module to register a shared or muxed pin without finding it or its parent. Instead use a parent/shared pin description to find correct pin internally in dpll_core, simplifing a dpll API * Implement complex DPLL design in ice driver (Arkadiusz) * Remove ptp_ocp driver from the series for now v3 -> v4: * redesign framework to make pins dynamically allocated (Arkadiusz) * implement shared pins (Arkadiusz) v2 -> v3: * implement source select mode (Arkadiusz) * add documentation * implementation improvements (Jakub) v1 -> v2: * implement returning supported input/output types * ptp_ocp: follow suggestions from Jonathan * add linux-clk mailing list v0 -> v1: * fix code style and errors * add linux-arm mailing list Arkadiusz Kubalewski (3): dpll: spec: Add Netlink spec in YAML ice: add admin commands to access cgu configuration ice: implement dpll interface to control cgu Jiri Pirko (2): netdev: expose DPLL pin handle for netdevice mlx5: Implement SyncE support using DPLL infrastructure Vadim Fedorenko (3): dpll: Add DPLL framework base functions dpll: documentation on DPLL subsystem interface ptp_ocp: implement DPLL ops Documentation/dpll.rst | 408 ++++ Documentation/netlink/specs/dpll.yaml | 472 ++++ Documentation/networking/index.rst | 1 + MAINTAINERS | 8 + drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/dpll/Kconfig | 7 + drivers/dpll/Makefile | 10 + drivers/dpll/dpll_core.c | 939 ++++++++ drivers/dpll/dpll_core.h | 113 + drivers/dpll/dpll_netlink.c | 991 +++++++++ drivers/dpll/dpll_netlink.h | 27 + drivers/dpll/dpll_nl.c | 126 ++ drivers/dpll/dpll_nl.h | 42 + drivers/net/ethernet/intel/Kconfig | 1 + drivers/net/ethernet/intel/ice/Makefile | 3 +- drivers/net/ethernet/intel/ice/ice.h | 5 + .../net/ethernet/intel/ice/ice_adminq_cmd.h | 240 +- drivers/net/ethernet/intel/ice/ice_common.c | 467 ++++ drivers/net/ethernet/intel/ice/ice_common.h | 43 + drivers/net/ethernet/intel/ice/ice_dpll.c | 1929 +++++++++++++++++ drivers/net/ethernet/intel/ice/ice_dpll.h | 101 + drivers/net/ethernet/intel/ice/ice_lib.c | 17 +- drivers/net/ethernet/intel/ice/ice_main.c | 7 + drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 414 ++++ drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 230 ++ drivers/net/ethernet/intel/ice/ice_type.h | 1 + .../net/ethernet/mellanox/mlx5/core/Kconfig | 8 + .../net/ethernet/mellanox/mlx5/core/Makefile | 3 + drivers/net/ethernet/mellanox/mlx5/core/dev.c | 17 + .../net/ethernet/mellanox/mlx5/core/dpll.c | 438 ++++ drivers/ptp/Kconfig | 1 + drivers/ptp/ptp_ocp.c | 327 ++- include/linux/dpll.h | 294 +++ include/linux/mlx5/driver.h | 2 + include/linux/mlx5/mlx5_ifc.h | 59 +- include/linux/netdevice.h | 7 + include/uapi/linux/dpll.h | 204 ++ include/uapi/linux/if_link.h | 2 + net/core/dev.c | 20 + net/core/rtnetlink.c | 38 + 41 files changed, 7966 insertions(+), 59 deletions(-) create mode 100644 Documentation/dpll.rst create mode 100644 Documentation/netlink/specs/dpll.yaml create mode 100644 drivers/dpll/Kconfig create mode 100644 drivers/dpll/Makefile create mode 100644 drivers/dpll/dpll_core.c create mode 100644 drivers/dpll/dpll_core.h create mode 100644 drivers/dpll/dpll_netlink.c create mode 100644 drivers/dpll/dpll_netlink.h create mode 100644 drivers/dpll/dpll_nl.c create mode 100644 drivers/dpll/dpll_nl.h create mode 100644 drivers/net/ethernet/intel/ice/ice_dpll.c create mode 100644 drivers/net/ethernet/intel/ice/ice_dpll.h create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/dpll.c create mode 100644 include/linux/dpll.h create mode 100644 include/uapi/linux/dpll.h