Message ID | 20230720091903.297066-1-vadim.fedorenko@linux.dev (mailing list archive) |
---|---|
Headers | show |
Series | Create common DPLL configuration API | expand |
Thu, Jul 20, 2023 at 11:18:52AM CEST, vadim.fedorenko@linux.dev wrote:
>v8 RFC -> v0:
Just to avoid future confusion: In netdev, the count goes from 1.
So this is v1. Next submission will be v2.
There are couple of issues that came up during our internal ci run: 10:16:04 error: drivers/dpll/dpll_netlink.c:452:5: error: no previous prototype for '__dpll_device_change_ntf' [-Werror=missing-prototypes] 10:16:04 error: drivers/dpll/dpll_netlink.c:1283:13: error: no previous prototype for 'dpll_netlink_fini' [-Werror=missing-prototypes] 10:16:04 error: drivers/dpll/dpll_core.c:221:1: error: no previous prototype for 'dpll_xa_ref_dpll_find' [-Werror=missing-prototypes] 10:27:31 error: drivers/dpll/dpll_core.c:220:21: warning: symbol 'dpll_xa_ref_dpll_find' was not declared. Should it be static? 10:27:31 error: drivers/dpll/dpll_netlink.c:452:5: warning: symbol '__dpll_device_change_ntf' was not declared. Should it be static? 10:27:31 error: drivers/dpll/dpll_netlink.c:1283:13: warning: symbol 'dpll_netlink_fini' was not declared. Should it be static? 10:27:41 error: drivers/net/ethernet/intel/ice/ice_dpll.c:461:3: error: a label can only be part of a statement and a declaration is not a statement I believe that you didn't run make with C=2, otherwise you would hit these. Checkpatch issue: 10:29:30 CHECK: struct mutex definition without comment 10:29:30 #6581: FILE: drivers/net/ethernet/intel/ice/ice_dpll.h:85: 10:29:30 + struct mutex lock; Spelling errors: 10:45:08 error: Documentation/netlink/specs/dpll.yaml:165: prority ==> priority 10:45:08 error: include/uapi/linux/dpll.h:128: prority ==> priority 10:45:08 error: drivers/net/ethernet/intel/ice/ice_dpll.c:2008: userpsace ==> userspace 10:45:08 error: drivers/net/ethernet/intel/ice/ice_dpll.h:20: properities ==> properties Thu, Jul 20, 2023 at 11:18:52AM CEST, vadim.fedorenko@linux.dev 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 s/API aim/API aims/ >make it flexible and easy to cover special configurations. I don't follow. How this could "aim to extend current pin configuration" ? This is a new thing. Could you re-phrase? What's "special configuration"? Sounds odd. > >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: > >- dump dpll devices >$# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ "$#" looks a bit odd. Just "$" with "sudo" when you want to emphasize root is needed to perform the command. >--dump device-get >[{'clock-id': 282574471561216, > 'id': 0, > 'lock-status': 'unlocked', > 'mode': 'automatic', > 'module-name': 'ice', > 'type': 'eec'}, > {'clock-id': 282574471561216, > 'id': 1, > 'lock-status': 'unlocked', > 'mode': 'automatic', > 'module-name': 'ice', > 'type': 'pps'}] > >- get single pin info: >$# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >--do pin-get --json '{"pin-id":2}' >{'clock-id': 282574471561216, > 'module-name': 'ice', > 'pin-board-label': 'C827_0-RCLKA', > 'pin-dpll-caps': 6, > 'pin-frequency': 1953125, > 'pin-id': 2, > 'pin-parenti-device': [{'id': 0, This looks like manual edit went wrong :) s/parenti/parent/ > 'pin-direction': 'input', > 'pin-prio': 11, > 'pin-state': 'selectable'}, > {'id': 1, > 'pin-direction': 'input', > 'pin-prio': 9, > 'pin-state': 'selectable'}], > 'pin-type': 'mux'} > >- set pin's state on dpll: >$# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >--do pin-set --json '{"pin-id":2, "pin-parent-device":{"id":1, "pin-state":2}}' > >- set pin's prio on dpll: >$# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >--do pin-set --json '{"pin-id":2, "pin-parent-device":{"id":1, "pin-prio":4}}' > >- set pin's state on parent pin: >$# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >--do pin-set --json '{"pin-id":13, \ > "pin-parent-pin":{"pin-id":2, "pin-state":1}}' > [...]
On 21.07.2023 12:14, Jiri Pirko wrote: > There are couple of issues that came up during our internal ci run: > > 10:16:04 error: drivers/dpll/dpll_netlink.c:452:5: error: no previous prototype for '__dpll_device_change_ntf' [-Werror=missing-prototypes] > 10:16:04 error: drivers/dpll/dpll_netlink.c:1283:13: error: no previous prototype for 'dpll_netlink_fini' [-Werror=missing-prototypes] > 10:16:04 error: drivers/dpll/dpll_core.c:221:1: error: no previous prototype for 'dpll_xa_ref_dpll_find' [-Werror=missing-prototypes] > > 10:27:31 error: drivers/dpll/dpll_core.c:220:21: warning: symbol 'dpll_xa_ref_dpll_find' was not declared. Should it be static? > 10:27:31 error: drivers/dpll/dpll_netlink.c:452:5: warning: symbol '__dpll_device_change_ntf' was not declared. Should it be static? > 10:27:31 error: drivers/dpll/dpll_netlink.c:1283:13: warning: symbol 'dpll_netlink_fini' was not declared. Should it be static? > 10:27:41 error: drivers/net/ethernet/intel/ice/ice_dpll.c:461:3: error: a label can only be part of a statement and a declaration is not a statement > > I believe that you didn't run make with C=2, otherwise you would hit > these. Yeah, I'll re-run the set patch-by-patch with C=2 next time. > > Checkpatch issue: > 10:29:30 CHECK: struct mutex definition without comment > 10:29:30 #6581: FILE: drivers/net/ethernet/intel/ice/ice_dpll.h:85: > 10:29:30 + struct mutex lock; Arkadiusz will take care of "ice" part. > Spelling errors: > 10:45:08 error: Documentation/netlink/specs/dpll.yaml:165: prority ==> priority > 10:45:08 error: include/uapi/linux/dpll.h:128: prority ==> priority > 10:45:08 error: drivers/net/ethernet/intel/ice/ice_dpll.c:2008: userpsace ==> userspace > 10:45:08 error: drivers/net/ethernet/intel/ice/ice_dpll.h:20: properities ==> properties > Will fix it. > > Thu, Jul 20, 2023 at 11:18:52AM CEST, vadim.fedorenko@linux.dev 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 > > s/API aim/API aims/ > > >> make it flexible and easy to cover special configurations. > > I don't follow. How this could "aim to extend current pin configuration" ? > This is a new thing. Could you re-phrase? Not really new. PTP devices have already simple pin configurations, mlx5 is using it for some cards with external pins. The problem is that PTP subsystem covers only simple configuration of the pin and doesn't cover DPLL part at all. > > What's "special configuration"? Sounds odd. > Yeah, "complex configurations" sounds better, will change it. > >> >> 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: >> >> - dump dpll devices >> $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ > > "$#" looks a bit odd. Just "$" with "sudo" when you want to emphasize > root is needed to perform the command. > > >> --dump device-get >> [{'clock-id': 282574471561216, >> 'id': 0, >> 'lock-status': 'unlocked', >> 'mode': 'automatic', >> 'module-name': 'ice', >> 'type': 'eec'}, >> {'clock-id': 282574471561216, >> 'id': 1, >> 'lock-status': 'unlocked', >> 'mode': 'automatic', >> 'module-name': 'ice', >> 'type': 'pps'}] >> >> - get single pin info: >> $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >> --do pin-get --json '{"pin-id":2}' >> {'clock-id': 282574471561216, >> 'module-name': 'ice', >> 'pin-board-label': 'C827_0-RCLKA', >> 'pin-dpll-caps': 6, >> 'pin-frequency': 1953125, >> 'pin-id': 2, >> 'pin-parenti-device': [{'id': 0, > > This looks like manual edit went wrong :) > s/parenti/parent/ > Ahhh... yeah :) > >> 'pin-direction': 'input', >> 'pin-prio': 11, >> 'pin-state': 'selectable'}, >> {'id': 1, >> 'pin-direction': 'input', >> 'pin-prio': 9, >> 'pin-state': 'selectable'}], >> 'pin-type': 'mux'} >> >> - set pin's state on dpll: >> $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >> --do pin-set --json '{"pin-id":2, "pin-parent-device":{"id":1, "pin-state":2}}' >> >> - set pin's prio on dpll: >> $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >> --do pin-set --json '{"pin-id":2, "pin-parent-device":{"id":1, "pin-prio":4}}' >> >> - set pin's state on parent pin: >> $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >> --do pin-set --json '{"pin-id":13, \ >> "pin-parent-pin":{"pin-id":2, "pin-state":1}}' >> > > [...]
Fri, Jul 21, 2023 at 04:42:46PM CEST, vadim.fedorenko@linux.dev wrote: >On 21.07.2023 12:14, Jiri Pirko wrote: >> There are couple of issues that came up during our internal ci run: >> >> 10:16:04 error: drivers/dpll/dpll_netlink.c:452:5: error: no previous prototype for '__dpll_device_change_ntf' [-Werror=missing-prototypes] >> 10:16:04 error: drivers/dpll/dpll_netlink.c:1283:13: error: no previous prototype for 'dpll_netlink_fini' [-Werror=missing-prototypes] >> 10:16:04 error: drivers/dpll/dpll_core.c:221:1: error: no previous prototype for 'dpll_xa_ref_dpll_find' [-Werror=missing-prototypes] >> >> 10:27:31 error: drivers/dpll/dpll_core.c:220:21: warning: symbol 'dpll_xa_ref_dpll_find' was not declared. Should it be static? >> 10:27:31 error: drivers/dpll/dpll_netlink.c:452:5: warning: symbol '__dpll_device_change_ntf' was not declared. Should it be static? >> 10:27:31 error: drivers/dpll/dpll_netlink.c:1283:13: warning: symbol 'dpll_netlink_fini' was not declared. Should it be static? >> 10:27:41 error: drivers/net/ethernet/intel/ice/ice_dpll.c:461:3: error: a label can only be part of a statement and a declaration is not a statement >> >> I believe that you didn't run make with C=2, otherwise you would hit >> these. > >Yeah, I'll re-run the set patch-by-patch with C=2 next time. > >> >> Checkpatch issue: >> 10:29:30 CHECK: struct mutex definition without comment >> 10:29:30 #6581: FILE: drivers/net/ethernet/intel/ice/ice_dpll.h:85: >> 10:29:30 + struct mutex lock; > >Arkadiusz will take care of "ice" part. > > >> Spelling errors: >> 10:45:08 error: Documentation/netlink/specs/dpll.yaml:165: prority ==> priority >> 10:45:08 error: include/uapi/linux/dpll.h:128: prority ==> priority >> 10:45:08 error: drivers/net/ethernet/intel/ice/ice_dpll.c:2008: userpsace ==> userspace >> 10:45:08 error: drivers/net/ethernet/intel/ice/ice_dpll.h:20: properities ==> properties >> > >Will fix it. > >> >> Thu, Jul 20, 2023 at 11:18:52AM CEST, vadim.fedorenko@linux.dev 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 >> >> s/API aim/API aims/ >> >> >> > make it flexible and easy to cover special configurations. >> >> I don't follow. How this could "aim to extend current pin configuration" ? >> This is a new thing. Could you re-phrase? > >Not really new. PTP devices have already simple pin configurations, mlx5 is >using it for some cards with external pins. The problem is that PTP subsystem >covers only simple configuration of the pin and doesn't cover DPLL part at all. Okay, please put the info in. > >> >> What's "special configuration"? Sounds odd. >> > >Yeah, "complex configurations" sounds better, will change it. > >> >> > >> > 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: >> > >> > - dump dpll devices >> > $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >> >> "$#" looks a bit odd. Just "$" with "sudo" when you want to emphasize >> root is needed to perform the command. >> >> >> > --dump device-get >> > [{'clock-id': 282574471561216, >> > 'id': 0, >> > 'lock-status': 'unlocked', >> > 'mode': 'automatic', >> > 'module-name': 'ice', >> > 'type': 'eec'}, >> > {'clock-id': 282574471561216, >> > 'id': 1, >> > 'lock-status': 'unlocked', >> > 'mode': 'automatic', >> > 'module-name': 'ice', >> > 'type': 'pps'}] >> > >> > - get single pin info: >> > $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >> > --do pin-get --json '{"pin-id":2}' >> > {'clock-id': 282574471561216, >> > 'module-name': 'ice', >> > 'pin-board-label': 'C827_0-RCLKA', >> > 'pin-dpll-caps': 6, >> > 'pin-frequency': 1953125, >> > 'pin-id': 2, >> > 'pin-parenti-device': [{'id': 0, >> >> This looks like manual edit went wrong :) >> s/parenti/parent/ >> > >Ahhh... yeah :) > >> >> > 'pin-direction': 'input', >> > 'pin-prio': 11, >> > 'pin-state': 'selectable'}, >> > {'id': 1, >> > 'pin-direction': 'input', >> > 'pin-prio': 9, >> > 'pin-state': 'selectable'}], >> > 'pin-type': 'mux'} >> > >> > - set pin's state on dpll: >> > $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >> > --do pin-set --json '{"pin-id":2, "pin-parent-device":{"id":1, "pin-state":2}}' >> > >> > - set pin's prio on dpll: >> > $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >> > --do pin-set --json '{"pin-id":2, "pin-parent-device":{"id":1, "pin-prio":4}}' >> > >> > - set pin's state on parent pin: >> > $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \ >> > --do pin-set --json '{"pin-id":13, \ >> > "pin-parent-pin":{"pin-id":2, "pin-state":1}}' >> > >> >> [...] >