* [PATCH net-next v2 0/2] Add Embedded SYNC feature for a dpll's pin @ 2024-08-21 21:32 Arkadiusz Kubalewski 2024-08-21 21:32 ` [PATCH net-next v2 1/2] dpll: add Embedded SYNC feature for a pin Arkadiusz Kubalewski 2024-08-21 21:32 ` [PATCH net-next v2 2/2] ice: add callbacks for Embedded SYNC enablement on dpll pins Arkadiusz Kubalewski 0 siblings, 2 replies; 7+ messages in thread From: Arkadiusz Kubalewski @ 2024-08-21 21:32 UTC (permalink / raw) To: netdev Cc: vadim.fedorenko, jiri, corbet, davem, edumazet, kuba, pabeni, donald.hunter, anthony.l.nguyen, przemyslaw.kitszel, intel-wired-lan, linux-doc, linux-kernel, Arkadiusz Kubalewski Introduce and allow DPLL subsystem users to get/set capabilities of Embedded SYNC on a dpll's pin. Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> Arkadiusz Kubalewski (2): dpll: add Embedded SYNC feature for a pin ice: add callbacks for Embedded SYNC enablement on dpll pins Documentation/driver-api/dpll.rst | 21 ++ Documentation/netlink/specs/dpll.yaml | 24 +++ drivers/dpll/dpll_netlink.c | 130 ++++++++++++ drivers/dpll/dpll_nl.c | 5 +- drivers/net/ethernet/intel/ice/ice_dpll.c | 230 +++++++++++++++++++++- drivers/net/ethernet/intel/ice/ice_dpll.h | 1 + include/linux/dpll.h | 15 ++ include/uapi/linux/dpll.h | 3 + 8 files changed, 424 insertions(+), 5 deletions(-) base-commit: d785ed945de6955361aafc2d540d9bb7c6a69a65 -- 2.38.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next v2 1/2] dpll: add Embedded SYNC feature for a pin 2024-08-21 21:32 [PATCH net-next v2 0/2] Add Embedded SYNC feature for a dpll's pin Arkadiusz Kubalewski @ 2024-08-21 21:32 ` Arkadiusz Kubalewski 2024-08-22 10:22 ` Jiri Pirko 2024-08-21 21:32 ` [PATCH net-next v2 2/2] ice: add callbacks for Embedded SYNC enablement on dpll pins Arkadiusz Kubalewski 1 sibling, 1 reply; 7+ messages in thread From: Arkadiusz Kubalewski @ 2024-08-21 21:32 UTC (permalink / raw) To: netdev Cc: vadim.fedorenko, jiri, corbet, davem, edumazet, kuba, pabeni, donald.hunter, anthony.l.nguyen, przemyslaw.kitszel, intel-wired-lan, linux-doc, linux-kernel, Arkadiusz Kubalewski, Aleksandr Loktionov Implement and document new pin attributes for providing Embedded SYNC capabilities to the DPLL subsystem users through a netlink pin-get do/dump messages. Allow the user to set Embedded SYNC frequency with pin-set do netlink message. Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> --- v2: - remove enum for pulse-ratio, instead use plain u32 value, - provide e-sync-frequency attribute and value only if esync was enabled, - rename: e_sync/E_SYNC -> esync/ESYNC, - refactor .esync_get to allow multiple esync range values, - define and use struct dpll_pin_esync for getting esync related info, - rename esync -> freq to better align with existiong code, - remove unneeded line break, - respect 80 chars per line rule, - fix typos, Documentation/driver-api/dpll.rst | 21 +++++ Documentation/netlink/specs/dpll.yaml | 24 +++++ drivers/dpll/dpll_netlink.c | 130 ++++++++++++++++++++++++++ drivers/dpll/dpll_nl.c | 5 +- include/linux/dpll.h | 15 +++ include/uapi/linux/dpll.h | 3 + 6 files changed, 196 insertions(+), 2 deletions(-) diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst index ea8d16600e16..a212b94ad52c 100644 --- a/Documentation/driver-api/dpll.rst +++ b/Documentation/driver-api/dpll.rst @@ -214,6 +214,27 @@ offset values are fractional with 3-digit decimal places and shell be divided with ``DPLL_PIN_PHASE_OFFSET_DIVIDER`` to get integer part and modulo divided to get fractional part. +Embedded SYNC +============= + +Device may provide ability to use Embedded SYNC feature. It allows +to embed additional SYNC signal into the base frequency of a pin - a one +special pulse of base frequency signal every time SYNC signal pulse +happens. The user can configure the frequency of Embedded SYNC. +The Embedded SYNC capability is always related to a given base frequency +and HW capabilities. The user is provided a range of Embedded SYNC +frequencies supported, depending on current base frequency configured for +the pin. + + ========================================= ================================= + ``DPLL_A_PIN_ESYNC_FREQUENCY`` current Embedded SYNC frequency + ``DPLL_A_PIN_ESYNC_FREQUENCY_SUPPORTED`` nest available Embedded SYNC + frequency ranges + ``DPLL_A_PIN_FREQUENCY_MIN`` attr minimum value of frequency + ``DPLL_A_PIN_FREQUENCY_MAX`` attr maximum value of frequency + ``DPLL_A_PIN_ESYNC_PULSE`` pulse type of Embedded SYNC + ========================================= ================================= + Configuration commands group ============================ diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml index 94132d30e0e0..f2894ca35de8 100644 --- a/Documentation/netlink/specs/dpll.yaml +++ b/Documentation/netlink/specs/dpll.yaml @@ -345,6 +345,26 @@ attribute-sets: Value is in PPM (parts per million). This may be implemented for example for pin of type PIN_TYPE_SYNCE_ETH_PORT. + - + name: esync-frequency + type: u64 + doc: | + Frequency of Embedded SYNC signal. If provided, the pin is configured + with a SYNC signal embedded into its base clock frequency. + - + name: esync-frequency-supported + type: nest + multi-attr: true + nested-attributes: frequency-range + doc: | + If provided a pin is capable of embedding a SYNC signal (within given + range) into its base frequency signal. + - + name: esync-pulse + type: u32 + doc: | + A ratio of high to low state of a SYNC signal pulse embedded + into base clock frequency. Value is in percents. - name: pin-parent-device subset-of: pin @@ -510,6 +530,9 @@ operations: - phase-adjust-max - phase-adjust - fractional-frequency-offset + - esync-frequency + - esync-frequency-supported + - esync-pulse dump: request: @@ -536,6 +559,7 @@ operations: - parent-device - parent-pin - phase-adjust + - esync-frequency - name: pin-create-ntf doc: Notification about pin appearing diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c index 98e6ad8528d3..fe1a00ad84d1 100644 --- a/drivers/dpll/dpll_netlink.c +++ b/drivers/dpll/dpll_netlink.c @@ -342,6 +342,51 @@ dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin, return 0; } +static int +dpll_msg_add_pin_esync(struct sk_buff *msg, struct dpll_pin *pin, + struct dpll_pin_ref *ref, struct netlink_ext_ack *extack) +{ + const struct dpll_pin_ops *ops = dpll_pin_ops(ref); + struct dpll_device *dpll = ref->dpll; + struct dpll_pin_esync esync; + struct nlattr *nest; + int ret, i; + + if (!ops->esync_get) + return 0; + ret = ops->esync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll, + dpll_priv(dpll), &esync, extack); + if (ret == -EOPNOTSUPP) + return 0; + else if (ret) + return ret; + if (nla_put_64bit(msg, DPLL_A_PIN_ESYNC_FREQUENCY, sizeof(esync.freq), + &esync.freq, DPLL_A_PIN_PAD)) + return -EMSGSIZE; + if (nla_put_u32(msg, DPLL_A_PIN_ESYNC_PULSE, esync.pulse)) + return -EMSGSIZE; + for (i = 0; i < esync.range_num; i++) { + nest = nla_nest_start(msg, + DPLL_A_PIN_ESYNC_FREQUENCY_SUPPORTED); + if (!nest) + return -EMSGSIZE; + if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, + sizeof(esync.range[i].min), + &esync.range[i].min, DPLL_A_PIN_PAD)) + goto nest_cancel; + if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, + sizeof(esync.range[i].max), + &esync.range[i].max, DPLL_A_PIN_PAD)) + goto nest_cancel; + nla_nest_end(msg, nest); + } + return 0; + +nest_cancel: + nla_nest_cancel(msg, nest); + return -EMSGSIZE; +} + static bool dpll_pin_is_freq_supported(struct dpll_pin *pin, u32 freq) { int fs; @@ -481,6 +526,9 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin, if (ret) return ret; ret = dpll_msg_add_ffo(msg, pin, ref, extack); + if (ret) + return ret; + ret = dpll_msg_add_pin_esync(msg, pin, ref, extack); if (ret) return ret; if (xa_empty(&pin->parent_refs)) @@ -738,6 +786,83 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a, return ret; } +static int +dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a, + struct netlink_ext_ack *extack) +{ + struct dpll_pin_ref *ref, *failed; + const struct dpll_pin_ops *ops; + struct dpll_pin_esync esync; + u64 freq = nla_get_u64(a); + struct dpll_device *dpll; + bool supported = false; + unsigned long i; + int ret; + + xa_for_each(&pin->dpll_refs, i, ref) { + ops = dpll_pin_ops(ref); + if (!ops->esync_set || !ops->esync_get) { + NL_SET_ERR_MSG(extack, + "embedded sync feature is not supported by this device"); + return -EOPNOTSUPP; + } + } + ref = dpll_xa_ref_dpll_first(&pin->dpll_refs); + ops = dpll_pin_ops(ref); + dpll = ref->dpll; + ret = ops->esync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll, + dpll_priv(dpll), &esync, extack); + if (ret) { + NL_SET_ERR_MSG(extack, "unable to get current embedded sync frequency value"); + return ret; + } + if (freq == esync.freq) + return 0; + for (i = 0; i < esync.range_num; i++) + if (freq <= esync.range[i].max && freq >= esync.range[i].min) + supported = true; + if (!supported) { + NL_SET_ERR_MSG_ATTR(extack, a, + "requested embedded sync frequency value is not supported by this device"); + return -EINVAL; + } + + xa_for_each(&pin->dpll_refs, i, ref) { + void *pin_dpll_priv; + + ops = dpll_pin_ops(ref); + dpll = ref->dpll; + pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin); + ret = ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll), + freq, extack); + if (ret) { + failed = ref; + NL_SET_ERR_MSG_FMT(extack, + "embedded sync frequency set failed for dpll_id:%u", + dpll->id); + goto rollback; + } + } + __dpll_pin_change_ntf(pin); + + return 0; + +rollback: + xa_for_each(&pin->dpll_refs, i, ref) { + void *pin_dpll_priv; + + if (ref == failed) + break; + ops = dpll_pin_ops(ref); + dpll = ref->dpll; + pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin); + if (ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll), + esync.freq, extack)) + NL_SET_ERR_MSG(extack, "set embedded sync frequency rollback failed"); + } + return ret; +} + static int dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32 parent_idx, enum dpll_pin_state state, @@ -1039,6 +1164,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info) if (ret) return ret; break; + case DPLL_A_PIN_ESYNC_FREQUENCY: + ret = dpll_pin_esync_set(pin, a, info->extack); + if (ret) + return ret; + break; } } diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c index 1e95f5397cfc..fe9b6893d261 100644 --- a/drivers/dpll/dpll_nl.c +++ b/drivers/dpll/dpll_nl.c @@ -62,7 +62,7 @@ static const struct nla_policy dpll_pin_get_dump_nl_policy[DPLL_A_PIN_ID + 1] = }; /* DPLL_CMD_PIN_SET - do */ -static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST + 1] = { +static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_ESYNC_FREQUENCY + 1] = { [DPLL_A_PIN_ID] = { .type = NLA_U32, }, [DPLL_A_PIN_FREQUENCY] = { .type = NLA_U64, }, [DPLL_A_PIN_DIRECTION] = NLA_POLICY_RANGE(NLA_U32, 1, 2), @@ -71,6 +71,7 @@ static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST + [DPLL_A_PIN_PARENT_DEVICE] = NLA_POLICY_NESTED(dpll_pin_parent_device_nl_policy), [DPLL_A_PIN_PARENT_PIN] = NLA_POLICY_NESTED(dpll_pin_parent_pin_nl_policy), [DPLL_A_PIN_PHASE_ADJUST] = { .type = NLA_S32, }, + [DPLL_A_PIN_ESYNC_FREQUENCY] = { .type = NLA_U64, }, }; /* Ops table for dpll */ @@ -138,7 +139,7 @@ static const struct genl_split_ops dpll_nl_ops[] = { .doit = dpll_nl_pin_set_doit, .post_doit = dpll_pin_post_doit, .policy = dpll_pin_set_nl_policy, - .maxattr = DPLL_A_PIN_PHASE_ADJUST, + .maxattr = DPLL_A_PIN_ESYNC_FREQUENCY, .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, }, }; diff --git a/include/linux/dpll.h b/include/linux/dpll.h index d275736230b3..3baa196d7000 100644 --- a/include/linux/dpll.h +++ b/include/linux/dpll.h @@ -15,6 +15,7 @@ struct dpll_device; struct dpll_pin; +struct dpll_pin_esync; struct dpll_device_ops { int (*mode_get)(const struct dpll_device *dpll, void *dpll_priv, @@ -83,6 +84,13 @@ struct dpll_pin_ops { int (*ffo_get)(const struct dpll_pin *pin, void *pin_priv, const struct dpll_device *dpll, void *dpll_priv, s64 *ffo, struct netlink_ext_ack *extack); + int (*esync_set)(const struct dpll_pin *pin, void *pin_priv, + const struct dpll_device *dpll, void *dpll_priv, + u64 esync_freq, struct netlink_ext_ack *extack); + int (*esync_get)(const struct dpll_pin *pin, void *pin_priv, + const struct dpll_device *dpll, void *dpll_priv, + struct dpll_pin_esync *esync, + struct netlink_ext_ack *extack); }; struct dpll_pin_frequency { @@ -111,6 +119,13 @@ struct dpll_pin_phase_adjust_range { s32 max; }; +struct dpll_pin_esync { + u64 freq; + const struct dpll_pin_frequency *range; + u8 range_num; + u8 pulse; +}; + struct dpll_pin_properties { const char *board_label; const char *panel_label; diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h index 0c13d7f1a1bc..b0654ade7b7e 100644 --- a/include/uapi/linux/dpll.h +++ b/include/uapi/linux/dpll.h @@ -210,6 +210,9 @@ enum dpll_a_pin { DPLL_A_PIN_PHASE_ADJUST, DPLL_A_PIN_PHASE_OFFSET, DPLL_A_PIN_FRACTIONAL_FREQUENCY_OFFSET, + DPLL_A_PIN_ESYNC_FREQUENCY, + DPLL_A_PIN_ESYNC_FREQUENCY_SUPPORTED, + DPLL_A_PIN_ESYNC_PULSE, __DPLL_A_PIN_MAX, DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1) -- 2.38.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next v2 1/2] dpll: add Embedded SYNC feature for a pin 2024-08-21 21:32 ` [PATCH net-next v2 1/2] dpll: add Embedded SYNC feature for a pin Arkadiusz Kubalewski @ 2024-08-22 10:22 ` Jiri Pirko 2024-08-22 22:30 ` Kubalewski, Arkadiusz 0 siblings, 1 reply; 7+ messages in thread From: Jiri Pirko @ 2024-08-22 10:22 UTC (permalink / raw) To: Arkadiusz Kubalewski Cc: netdev, vadim.fedorenko, corbet, davem, edumazet, kuba, pabeni, donald.hunter, anthony.l.nguyen, przemyslaw.kitszel, intel-wired-lan, linux-doc, linux-kernel, Aleksandr Loktionov Wed, Aug 21, 2024 at 11:32:17PM CEST, arkadiusz.kubalewski@intel.com wrote: >Implement and document new pin attributes for providing Embedded SYNC >capabilities to the DPLL subsystem users through a netlink pin-get >do/dump messages. Allow the user to set Embedded SYNC frequency with >pin-set do netlink message. > >Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >--- >v2: >- remove enum for pulse-ratio, instead use plain u32 value, >- provide e-sync-frequency attribute and value only if esync was > enabled, >- rename: e_sync/E_SYNC -> esync/ESYNC, >- refactor .esync_get to allow multiple esync range values, >- define and use struct dpll_pin_esync for getting esync related info, >- rename esync -> freq to better align with existiong code, >- remove unneeded line break, >- respect 80 chars per line rule, >- fix typos, > > Documentation/driver-api/dpll.rst | 21 +++++ > Documentation/netlink/specs/dpll.yaml | 24 +++++ > drivers/dpll/dpll_netlink.c | 130 ++++++++++++++++++++++++++ > drivers/dpll/dpll_nl.c | 5 +- > include/linux/dpll.h | 15 +++ > include/uapi/linux/dpll.h | 3 + > 6 files changed, 196 insertions(+), 2 deletions(-) Looks fine. 2 nitpicks below: > >diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst >index ea8d16600e16..a212b94ad52c 100644 >--- a/Documentation/driver-api/dpll.rst >+++ b/Documentation/driver-api/dpll.rst >@@ -214,6 +214,27 @@ offset values are fractional with 3-digit decimal places and shell be > divided with ``DPLL_PIN_PHASE_OFFSET_DIVIDER`` to get integer part and > modulo divided to get fractional part. > >+Embedded SYNC >+============= >+ >+Device may provide ability to use Embedded SYNC feature. It allows >+to embed additional SYNC signal into the base frequency of a pin - a one >+special pulse of base frequency signal every time SYNC signal pulse >+happens. The user can configure the frequency of Embedded SYNC. >+The Embedded SYNC capability is always related to a given base frequency >+and HW capabilities. The user is provided a range of Embedded SYNC >+frequencies supported, depending on current base frequency configured for >+the pin. >+ >+ ========================================= ================================= >+ ``DPLL_A_PIN_ESYNC_FREQUENCY`` current Embedded SYNC frequency >+ ``DPLL_A_PIN_ESYNC_FREQUENCY_SUPPORTED`` nest available Embedded SYNC >+ frequency ranges >+ ``DPLL_A_PIN_FREQUENCY_MIN`` attr minimum value of frequency >+ ``DPLL_A_PIN_FREQUENCY_MAX`` attr maximum value of frequency >+ ``DPLL_A_PIN_ESYNC_PULSE`` pulse type of Embedded SYNC >+ ========================================= ================================= >+ > Configuration commands group > ============================ > >diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml >index 94132d30e0e0..f2894ca35de8 100644 >--- a/Documentation/netlink/specs/dpll.yaml >+++ b/Documentation/netlink/specs/dpll.yaml >@@ -345,6 +345,26 @@ attribute-sets: > Value is in PPM (parts per million). > This may be implemented for example for pin of type > PIN_TYPE_SYNCE_ETH_PORT. >+ - >+ name: esync-frequency >+ type: u64 >+ doc: | >+ Frequency of Embedded SYNC signal. If provided, the pin is configured >+ with a SYNC signal embedded into its base clock frequency. >+ - >+ name: esync-frequency-supported >+ type: nest >+ multi-attr: true >+ nested-attributes: frequency-range >+ doc: | >+ If provided a pin is capable of embedding a SYNC signal (within given >+ range) into its base frequency signal. >+ - >+ name: esync-pulse >+ type: u32 >+ doc: | >+ A ratio of high to low state of a SYNC signal pulse embedded >+ into base clock frequency. Value is in percents. > - > name: pin-parent-device > subset-of: pin >@@ -510,6 +530,9 @@ operations: > - phase-adjust-max > - phase-adjust > - fractional-frequency-offset >+ - esync-frequency >+ - esync-frequency-supported >+ - esync-pulse > > dump: > request: >@@ -536,6 +559,7 @@ operations: > - parent-device > - parent-pin > - phase-adjust >+ - esync-frequency > - > name: pin-create-ntf > doc: Notification about pin appearing >diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c >index 98e6ad8528d3..fe1a00ad84d1 100644 >--- a/drivers/dpll/dpll_netlink.c >+++ b/drivers/dpll/dpll_netlink.c >@@ -342,6 +342,51 @@ dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin, > return 0; > } > >+static int >+dpll_msg_add_pin_esync(struct sk_buff *msg, struct dpll_pin *pin, >+ struct dpll_pin_ref *ref, struct netlink_ext_ack *extack) >+{ >+ const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >+ struct dpll_device *dpll = ref->dpll; >+ struct dpll_pin_esync esync; >+ struct nlattr *nest; >+ int ret, i; >+ >+ if (!ops->esync_get) >+ return 0; >+ ret = ops->esync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll, >+ dpll_priv(dpll), &esync, extack); >+ if (ret == -EOPNOTSUPP) >+ return 0; >+ else if (ret) >+ return ret; >+ if (nla_put_64bit(msg, DPLL_A_PIN_ESYNC_FREQUENCY, sizeof(esync.freq), >+ &esync.freq, DPLL_A_PIN_PAD)) >+ return -EMSGSIZE; >+ if (nla_put_u32(msg, DPLL_A_PIN_ESYNC_PULSE, esync.pulse)) >+ return -EMSGSIZE; >+ for (i = 0; i < esync.range_num; i++) { >+ nest = nla_nest_start(msg, >+ DPLL_A_PIN_ESYNC_FREQUENCY_SUPPORTED); >+ if (!nest) >+ return -EMSGSIZE; >+ if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, >+ sizeof(esync.range[i].min), >+ &esync.range[i].min, DPLL_A_PIN_PAD)) >+ goto nest_cancel; >+ if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, >+ sizeof(esync.range[i].max), >+ &esync.range[i].max, DPLL_A_PIN_PAD)) >+ goto nest_cancel; >+ nla_nest_end(msg, nest); >+ } >+ return 0; >+ >+nest_cancel: >+ nla_nest_cancel(msg, nest); >+ return -EMSGSIZE; >+} >+ > static bool dpll_pin_is_freq_supported(struct dpll_pin *pin, u32 freq) > { > int fs; >@@ -481,6 +526,9 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin, > if (ret) > return ret; > ret = dpll_msg_add_ffo(msg, pin, ref, extack); >+ if (ret) >+ return ret; >+ ret = dpll_msg_add_pin_esync(msg, pin, ref, extack); > if (ret) > return ret; > if (xa_empty(&pin->parent_refs)) >@@ -738,6 +786,83 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a, > return ret; > } > >+static int >+dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a, >+ struct netlink_ext_ack *extack) >+{ >+ struct dpll_pin_ref *ref, *failed; >+ const struct dpll_pin_ops *ops; >+ struct dpll_pin_esync esync; >+ u64 freq = nla_get_u64(a); >+ struct dpll_device *dpll; >+ bool supported = false; >+ unsigned long i; >+ int ret; >+ >+ xa_for_each(&pin->dpll_refs, i, ref) { >+ ops = dpll_pin_ops(ref); >+ if (!ops->esync_set || !ops->esync_get) { >+ NL_SET_ERR_MSG(extack, >+ "embedded sync feature is not supported by this device"); >+ return -EOPNOTSUPP; >+ } >+ } >+ ref = dpll_xa_ref_dpll_first(&pin->dpll_refs); >+ ops = dpll_pin_ops(ref); >+ dpll = ref->dpll; >+ ret = ops->esync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll, >+ dpll_priv(dpll), &esync, extack); >+ if (ret) { >+ NL_SET_ERR_MSG(extack, "unable to get current embedded sync frequency value"); >+ return ret; >+ } >+ if (freq == esync.freq) >+ return 0; >+ for (i = 0; i < esync.range_num; i++) >+ if (freq <= esync.range[i].max && freq >= esync.range[i].min) >+ supported = true; >+ if (!supported) { >+ NL_SET_ERR_MSG_ATTR(extack, a, >+ "requested embedded sync frequency value is not supported by this device"); >+ return -EINVAL; >+ } >+ >+ xa_for_each(&pin->dpll_refs, i, ref) { >+ void *pin_dpll_priv; >+ >+ ops = dpll_pin_ops(ref); >+ dpll = ref->dpll; >+ pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin); >+ ret = ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll), >+ freq, extack); >+ if (ret) { >+ failed = ref; >+ NL_SET_ERR_MSG_FMT(extack, >+ "embedded sync frequency set failed for dpll_id:%u", Missing space after ":". >+ dpll->id); >+ goto rollback; >+ } >+ } >+ __dpll_pin_change_ntf(pin); >+ >+ return 0; >+ >+rollback: >+ xa_for_each(&pin->dpll_refs, i, ref) { >+ void *pin_dpll_priv; >+ >+ if (ref == failed) >+ break; >+ ops = dpll_pin_ops(ref); >+ dpll = ref->dpll; >+ pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin); >+ if (ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll), >+ esync.freq, extack)) >+ NL_SET_ERR_MSG(extack, "set embedded sync frequency rollback failed"); >+ } >+ return ret; >+} >+ > static int > dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32 parent_idx, > enum dpll_pin_state state, >@@ -1039,6 +1164,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info) > if (ret) > return ret; > break; >+ case DPLL_A_PIN_ESYNC_FREQUENCY: >+ ret = dpll_pin_esync_set(pin, a, info->extack); >+ if (ret) >+ return ret; >+ break; > } > } > >diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c >index 1e95f5397cfc..fe9b6893d261 100644 >--- a/drivers/dpll/dpll_nl.c >+++ b/drivers/dpll/dpll_nl.c >@@ -62,7 +62,7 @@ static const struct nla_policy dpll_pin_get_dump_nl_policy[DPLL_A_PIN_ID + 1] = > }; > > /* DPLL_CMD_PIN_SET - do */ >-static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST + 1] = { >+static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_ESYNC_FREQUENCY + 1] = { > [DPLL_A_PIN_ID] = { .type = NLA_U32, }, > [DPLL_A_PIN_FREQUENCY] = { .type = NLA_U64, }, > [DPLL_A_PIN_DIRECTION] = NLA_POLICY_RANGE(NLA_U32, 1, 2), >@@ -71,6 +71,7 @@ static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST + > [DPLL_A_PIN_PARENT_DEVICE] = NLA_POLICY_NESTED(dpll_pin_parent_device_nl_policy), > [DPLL_A_PIN_PARENT_PIN] = NLA_POLICY_NESTED(dpll_pin_parent_pin_nl_policy), > [DPLL_A_PIN_PHASE_ADJUST] = { .type = NLA_S32, }, >+ [DPLL_A_PIN_ESYNC_FREQUENCY] = { .type = NLA_U64, }, > }; > > /* Ops table for dpll */ >@@ -138,7 +139,7 @@ static const struct genl_split_ops dpll_nl_ops[] = { > .doit = dpll_nl_pin_set_doit, > .post_doit = dpll_pin_post_doit, > .policy = dpll_pin_set_nl_policy, >- .maxattr = DPLL_A_PIN_PHASE_ADJUST, >+ .maxattr = DPLL_A_PIN_ESYNC_FREQUENCY, > .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, > }, > }; >diff --git a/include/linux/dpll.h b/include/linux/dpll.h >index d275736230b3..3baa196d7000 100644 >--- a/include/linux/dpll.h >+++ b/include/linux/dpll.h >@@ -15,6 +15,7 @@ > > struct dpll_device; > struct dpll_pin; >+struct dpll_pin_esync; > > struct dpll_device_ops { > int (*mode_get)(const struct dpll_device *dpll, void *dpll_priv, >@@ -83,6 +84,13 @@ struct dpll_pin_ops { > int (*ffo_get)(const struct dpll_pin *pin, void *pin_priv, > const struct dpll_device *dpll, void *dpll_priv, > s64 *ffo, struct netlink_ext_ack *extack); >+ int (*esync_set)(const struct dpll_pin *pin, void *pin_priv, >+ const struct dpll_device *dpll, void *dpll_priv, >+ u64 esync_freq, struct netlink_ext_ack *extack); s/esync_freq/freq/ >+ int (*esync_get)(const struct dpll_pin *pin, void *pin_priv, >+ const struct dpll_device *dpll, void *dpll_priv, >+ struct dpll_pin_esync *esync, >+ struct netlink_ext_ack *extack); > }; > > struct dpll_pin_frequency { >@@ -111,6 +119,13 @@ struct dpll_pin_phase_adjust_range { > s32 max; > }; > >+struct dpll_pin_esync { >+ u64 freq; >+ const struct dpll_pin_frequency *range; >+ u8 range_num; >+ u8 pulse; >+}; >+ > struct dpll_pin_properties { > const char *board_label; > const char *panel_label; >diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h >index 0c13d7f1a1bc..b0654ade7b7e 100644 >--- a/include/uapi/linux/dpll.h >+++ b/include/uapi/linux/dpll.h >@@ -210,6 +210,9 @@ enum dpll_a_pin { > DPLL_A_PIN_PHASE_ADJUST, > DPLL_A_PIN_PHASE_OFFSET, > DPLL_A_PIN_FRACTIONAL_FREQUENCY_OFFSET, >+ DPLL_A_PIN_ESYNC_FREQUENCY, >+ DPLL_A_PIN_ESYNC_FREQUENCY_SUPPORTED, >+ DPLL_A_PIN_ESYNC_PULSE, > > __DPLL_A_PIN_MAX, > DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1) >-- >2.38.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH net-next v2 1/2] dpll: add Embedded SYNC feature for a pin 2024-08-22 10:22 ` Jiri Pirko @ 2024-08-22 22:30 ` Kubalewski, Arkadiusz 0 siblings, 0 replies; 7+ messages in thread From: Kubalewski, Arkadiusz @ 2024-08-22 22:30 UTC (permalink / raw) To: Jiri Pirko Cc: netdev@vger.kernel.org, vadim.fedorenko@linux.dev, corbet@lwn.net, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, donald.hunter@gmail.com, Nguyen, Anthony L, Kitszel, Przemyslaw, intel-wired-lan@lists.osuosl.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, Loktionov, Aleksandr >From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, August 22, 2024 12:22 PM > >Wed, Aug 21, 2024 at 11:32:17PM CEST, arkadiusz.kubalewski@intel.com wrote: >>Implement and document new pin attributes for providing Embedded SYNC >>capabilities to the DPLL subsystem users through a netlink pin-get >>do/dump messages. Allow the user to set Embedded SYNC frequency with >>pin-set do netlink message. >> >>Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>--- >>v2: >>- remove enum for pulse-ratio, instead use plain u32 value, >>- provide e-sync-frequency attribute and value only if esync was >> enabled, >>- rename: e_sync/E_SYNC -> esync/ESYNC, >>- refactor .esync_get to allow multiple esync range values, >>- define and use struct dpll_pin_esync for getting esync related info, >>- rename esync -> freq to better align with existiong code, >>- remove unneeded line break, >>- respect 80 chars per line rule, >>- fix typos, >> >> Documentation/driver-api/dpll.rst | 21 +++++ >> Documentation/netlink/specs/dpll.yaml | 24 +++++ >> drivers/dpll/dpll_netlink.c | 130 ++++++++++++++++++++++++++ >> drivers/dpll/dpll_nl.c | 5 +- >> include/linux/dpll.h | 15 +++ >> include/uapi/linux/dpll.h | 3 + >> 6 files changed, 196 insertions(+), 2 deletions(-) > > >Looks fine. 2 nitpicks below: > > >> >>diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver- >>api/dpll.rst >>index ea8d16600e16..a212b94ad52c 100644 >>--- a/Documentation/driver-api/dpll.rst >>+++ b/Documentation/driver-api/dpll.rst >>@@ -214,6 +214,27 @@ offset values are fractional with 3-digit decimal places >>and shell be >> divided with ``DPLL_PIN_PHASE_OFFSET_DIVIDER`` to get integer part and >> modulo divided to get fractional part. >> >>+Embedded SYNC >>+============= >>+ >>+Device may provide ability to use Embedded SYNC feature. It allows >>+to embed additional SYNC signal into the base frequency of a pin - a one >>+special pulse of base frequency signal every time SYNC signal pulse >>+happens. The user can configure the frequency of Embedded SYNC. >>+The Embedded SYNC capability is always related to a given base frequency >>+and HW capabilities. The user is provided a range of Embedded SYNC >>+frequencies supported, depending on current base frequency configured for >>+the pin. >>+ >>+ ========================================= >>================================= >>+ ``DPLL_A_PIN_ESYNC_FREQUENCY`` current Embedded SYNC frequency >>+ ``DPLL_A_PIN_ESYNC_FREQUENCY_SUPPORTED`` nest available Embedded SYNC >>+ frequency ranges >>+ ``DPLL_A_PIN_FREQUENCY_MIN`` attr minimum value of frequency >>+ ``DPLL_A_PIN_FREQUENCY_MAX`` attr maximum value of frequency >>+ ``DPLL_A_PIN_ESYNC_PULSE`` pulse type of Embedded SYNC >>+ ========================================= >>================================= >>+ >> Configuration commands group >> ============================ >> >>diff --git a/Documentation/netlink/specs/dpll.yaml >>b/Documentation/netlink/specs/dpll.yaml >>index 94132d30e0e0..f2894ca35de8 100644 >>--- a/Documentation/netlink/specs/dpll.yaml >>+++ b/Documentation/netlink/specs/dpll.yaml >>@@ -345,6 +345,26 @@ attribute-sets: >> Value is in PPM (parts per million). >> This may be implemented for example for pin of type >> PIN_TYPE_SYNCE_ETH_PORT. >>+ - >>+ name: esync-frequency >>+ type: u64 >>+ doc: | >>+ Frequency of Embedded SYNC signal. If provided, the pin is >>configured >>+ with a SYNC signal embedded into its base clock frequency. >>+ - >>+ name: esync-frequency-supported >>+ type: nest >>+ multi-attr: true >>+ nested-attributes: frequency-range >>+ doc: | >>+ If provided a pin is capable of embedding a SYNC signal (within >>given >>+ range) into its base frequency signal. >>+ - >>+ name: esync-pulse >>+ type: u32 >>+ doc: | >>+ A ratio of high to low state of a SYNC signal pulse embedded >>+ into base clock frequency. Value is in percents. >> - >> name: pin-parent-device >> subset-of: pin >>@@ -510,6 +530,9 @@ operations: >> - phase-adjust-max >> - phase-adjust >> - fractional-frequency-offset >>+ - esync-frequency >>+ - esync-frequency-supported >>+ - esync-pulse >> >> dump: >> request: >>@@ -536,6 +559,7 @@ operations: >> - parent-device >> - parent-pin >> - phase-adjust >>+ - esync-frequency >> - >> name: pin-create-ntf >> doc: Notification about pin appearing >>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c >>index 98e6ad8528d3..fe1a00ad84d1 100644 >>--- a/drivers/dpll/dpll_netlink.c >>+++ b/drivers/dpll/dpll_netlink.c >>@@ -342,6 +342,51 @@ dpll_msg_add_pin_freq(struct sk_buff *msg, struct >>dpll_pin *pin, >> return 0; >> } >> >>+static int >>+dpll_msg_add_pin_esync(struct sk_buff *msg, struct dpll_pin *pin, >>+ struct dpll_pin_ref *ref, struct netlink_ext_ack *extack) >>+{ >>+ const struct dpll_pin_ops *ops = dpll_pin_ops(ref); >>+ struct dpll_device *dpll = ref->dpll; >>+ struct dpll_pin_esync esync; >>+ struct nlattr *nest; >>+ int ret, i; >>+ >>+ if (!ops->esync_get) >>+ return 0; >>+ ret = ops->esync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll, >>+ dpll_priv(dpll), &esync, extack); >>+ if (ret == -EOPNOTSUPP) >>+ return 0; >>+ else if (ret) >>+ return ret; >>+ if (nla_put_64bit(msg, DPLL_A_PIN_ESYNC_FREQUENCY, sizeof(esync.freq), >>+ &esync.freq, DPLL_A_PIN_PAD)) >>+ return -EMSGSIZE; >>+ if (nla_put_u32(msg, DPLL_A_PIN_ESYNC_PULSE, esync.pulse)) >>+ return -EMSGSIZE; >>+ for (i = 0; i < esync.range_num; i++) { >>+ nest = nla_nest_start(msg, >>+ DPLL_A_PIN_ESYNC_FREQUENCY_SUPPORTED); >>+ if (!nest) >>+ return -EMSGSIZE; >>+ if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, >>+ sizeof(esync.range[i].min), >>+ &esync.range[i].min, DPLL_A_PIN_PAD)) >>+ goto nest_cancel; >>+ if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, >>+ sizeof(esync.range[i].max), >>+ &esync.range[i].max, DPLL_A_PIN_PAD)) >>+ goto nest_cancel; >>+ nla_nest_end(msg, nest); >>+ } >>+ return 0; >>+ >>+nest_cancel: >>+ nla_nest_cancel(msg, nest); >>+ return -EMSGSIZE; >>+} >>+ >> static bool dpll_pin_is_freq_supported(struct dpll_pin *pin, u32 freq) >> { >> int fs; >>@@ -481,6 +526,9 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin >>*pin, >> if (ret) >> return ret; >> ret = dpll_msg_add_ffo(msg, pin, ref, extack); >>+ if (ret) >>+ return ret; >>+ ret = dpll_msg_add_pin_esync(msg, pin, ref, extack); >> if (ret) >> return ret; >> if (xa_empty(&pin->parent_refs)) >>@@ -738,6 +786,83 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr >>*a, >> return ret; >> } >> >>+static int >>+dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a, >>+ struct netlink_ext_ack *extack) >>+{ >>+ struct dpll_pin_ref *ref, *failed; >>+ const struct dpll_pin_ops *ops; >>+ struct dpll_pin_esync esync; >>+ u64 freq = nla_get_u64(a); >>+ struct dpll_device *dpll; >>+ bool supported = false; >>+ unsigned long i; >>+ int ret; >>+ >>+ xa_for_each(&pin->dpll_refs, i, ref) { >>+ ops = dpll_pin_ops(ref); >>+ if (!ops->esync_set || !ops->esync_get) { >>+ NL_SET_ERR_MSG(extack, >>+ "embedded sync feature is not supported by >>this device"); >>+ return -EOPNOTSUPP; >>+ } >>+ } >>+ ref = dpll_xa_ref_dpll_first(&pin->dpll_refs); >>+ ops = dpll_pin_ops(ref); >>+ dpll = ref->dpll; >>+ ret = ops->esync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll, >>+ dpll_priv(dpll), &esync, extack); >>+ if (ret) { >>+ NL_SET_ERR_MSG(extack, "unable to get current embedded sync >>frequency value"); >>+ return ret; >>+ } >>+ if (freq == esync.freq) >>+ return 0; >>+ for (i = 0; i < esync.range_num; i++) >>+ if (freq <= esync.range[i].max && freq >= esync.range[i].min) >>+ supported = true; >>+ if (!supported) { >>+ NL_SET_ERR_MSG_ATTR(extack, a, >>+ "requested embedded sync frequency value is not >>supported by this device"); >>+ return -EINVAL; >>+ } >>+ >>+ xa_for_each(&pin->dpll_refs, i, ref) { >>+ void *pin_dpll_priv; >>+ >>+ ops = dpll_pin_ops(ref); >>+ dpll = ref->dpll; >>+ pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin); >>+ ret = ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll), >>+ freq, extack); >>+ if (ret) { >>+ failed = ref; >>+ NL_SET_ERR_MSG_FMT(extack, >>+ "embedded sync frequency set failed for >dpll_id:%u", > >Missing space after ":". > Sure, fixed in v3. > >>+ dpll->id); >>+ goto rollback; >>+ } >>+ } >>+ __dpll_pin_change_ntf(pin); >>+ >>+ return 0; >>+ >>+rollback: >>+ xa_for_each(&pin->dpll_refs, i, ref) { >>+ void *pin_dpll_priv; >>+ >>+ if (ref == failed) >>+ break; >>+ ops = dpll_pin_ops(ref); >>+ dpll = ref->dpll; >>+ pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin); >>+ if (ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll), >>+ esync.freq, extack)) >>+ NL_SET_ERR_MSG(extack, "set embedded sync frequency >>rollback failed"); >>+ } >>+ return ret; >>+} >>+ >> static int >> dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32 parent_idx, >> enum dpll_pin_state state, >>@@ -1039,6 +1164,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct >>genl_info *info) >> if (ret) >> return ret; >> break; >>+ case DPLL_A_PIN_ESYNC_FREQUENCY: >>+ ret = dpll_pin_esync_set(pin, a, info->extack); >>+ if (ret) >>+ return ret; >>+ break; >> } >> } >> >>diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c >>index 1e95f5397cfc..fe9b6893d261 100644 >>--- a/drivers/dpll/dpll_nl.c >>+++ b/drivers/dpll/dpll_nl.c >>@@ -62,7 +62,7 @@ static const struct nla_policy >>dpll_pin_get_dump_nl_policy[DPLL_A_PIN_ID + 1] = >> }; >> >> /* DPLL_CMD_PIN_SET - do */ >>-static const struct nla_policy >>dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST + 1] = { >>+static const struct nla_policy >>dpll_pin_set_nl_policy[DPLL_A_PIN_ESYNC_FREQUENCY + 1] = { >> [DPLL_A_PIN_ID] = { .type = NLA_U32, }, >> [DPLL_A_PIN_FREQUENCY] = { .type = NLA_U64, }, >> [DPLL_A_PIN_DIRECTION] = NLA_POLICY_RANGE(NLA_U32, 1, 2), >>@@ -71,6 +71,7 @@ static const struct nla_policy >>dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST + >> [DPLL_A_PIN_PARENT_DEVICE] = >>NLA_POLICY_NESTED(dpll_pin_parent_device_nl_policy), >> [DPLL_A_PIN_PARENT_PIN] = >>NLA_POLICY_NESTED(dpll_pin_parent_pin_nl_policy), >> [DPLL_A_PIN_PHASE_ADJUST] = { .type = NLA_S32, }, >>+ [DPLL_A_PIN_ESYNC_FREQUENCY] = { .type = NLA_U64, }, >> }; >> >> /* Ops table for dpll */ >>@@ -138,7 +139,7 @@ static const struct genl_split_ops dpll_nl_ops[] = { >> .doit = dpll_nl_pin_set_doit, >> .post_doit = dpll_pin_post_doit, >> .policy = dpll_pin_set_nl_policy, >>- .maxattr = DPLL_A_PIN_PHASE_ADJUST, >>+ .maxattr = DPLL_A_PIN_ESYNC_FREQUENCY, >> .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, >> }, >> }; >>diff --git a/include/linux/dpll.h b/include/linux/dpll.h >>index d275736230b3..3baa196d7000 100644 >>--- a/include/linux/dpll.h >>+++ b/include/linux/dpll.h >>@@ -15,6 +15,7 @@ >> >> struct dpll_device; >> struct dpll_pin; >>+struct dpll_pin_esync; >> >> struct dpll_device_ops { >> int (*mode_get)(const struct dpll_device *dpll, void *dpll_priv, >>@@ -83,6 +84,13 @@ struct dpll_pin_ops { >> int (*ffo_get)(const struct dpll_pin *pin, void *pin_priv, >> const struct dpll_device *dpll, void *dpll_priv, >> s64 *ffo, struct netlink_ext_ack *extack); >>+ int (*esync_set)(const struct dpll_pin *pin, void *pin_priv, >>+ const struct dpll_device *dpll, void *dpll_priv, >>+ u64 esync_freq, struct netlink_ext_ack *extack); > >s/esync_freq/freq/ > Ok, fixed in v3. Thank you! Arkadiusz > >>+ int (*esync_get)(const struct dpll_pin *pin, void *pin_priv, >>+ const struct dpll_device *dpll, void *dpll_priv, >>+ struct dpll_pin_esync *esync, >>+ struct netlink_ext_ack *extack); >> }; >> >> struct dpll_pin_frequency { >>@@ -111,6 +119,13 @@ struct dpll_pin_phase_adjust_range { >> s32 max; >> }; >> >>+struct dpll_pin_esync { >>+ u64 freq; >>+ const struct dpll_pin_frequency *range; >>+ u8 range_num; >>+ u8 pulse; >>+}; >>+ >> struct dpll_pin_properties { >> const char *board_label; >> const char *panel_label; >>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h >>index 0c13d7f1a1bc..b0654ade7b7e 100644 >>--- a/include/uapi/linux/dpll.h >>+++ b/include/uapi/linux/dpll.h >>@@ -210,6 +210,9 @@ enum dpll_a_pin { >> DPLL_A_PIN_PHASE_ADJUST, >> DPLL_A_PIN_PHASE_OFFSET, >> DPLL_A_PIN_FRACTIONAL_FREQUENCY_OFFSET, >>+ DPLL_A_PIN_ESYNC_FREQUENCY, >>+ DPLL_A_PIN_ESYNC_FREQUENCY_SUPPORTED, >>+ DPLL_A_PIN_ESYNC_PULSE, >> >> __DPLL_A_PIN_MAX, >> DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1) >>-- >>2.38.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next v2 2/2] ice: add callbacks for Embedded SYNC enablement on dpll pins 2024-08-21 21:32 [PATCH net-next v2 0/2] Add Embedded SYNC feature for a dpll's pin Arkadiusz Kubalewski 2024-08-21 21:32 ` [PATCH net-next v2 1/2] dpll: add Embedded SYNC feature for a pin Arkadiusz Kubalewski @ 2024-08-21 21:32 ` Arkadiusz Kubalewski 2024-08-22 10:28 ` Jiri Pirko 1 sibling, 1 reply; 7+ messages in thread From: Arkadiusz Kubalewski @ 2024-08-21 21:32 UTC (permalink / raw) To: netdev Cc: vadim.fedorenko, jiri, corbet, davem, edumazet, kuba, pabeni, donald.hunter, anthony.l.nguyen, przemyslaw.kitszel, intel-wired-lan, linux-doc, linux-kernel, Arkadiusz Kubalewski, Aleksandr Loktionov Allow the user to get and set configuration of Embedded SYNC feature on the ice driver dpll pins. Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> --- v2: - align to v2 changes of "dpll: add Embedded SYNC feature for a pin" drivers/net/ethernet/intel/ice/ice_dpll.c | 230 +++++++++++++++++++++- drivers/net/ethernet/intel/ice/ice_dpll.h | 1 + 2 files changed, 228 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c index e92be6f130a3..aa6b87281ea6 100644 --- a/drivers/net/ethernet/intel/ice/ice_dpll.c +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c @@ -9,6 +9,7 @@ #define ICE_CGU_STATE_ACQ_ERR_THRESHOLD 50 #define ICE_DPLL_PIN_IDX_INVALID 0xff #define ICE_DPLL_RCLK_NUM_PER_PF 1 +#define ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT 25 /** * enum ice_dpll_pin_type - enumerate ice pin types: @@ -30,6 +31,10 @@ static const char * const pin_type_name[] = { [ICE_DPLL_PIN_TYPE_RCLK_INPUT] = "rclk-input", }; +static const struct dpll_pin_frequency ice_esync_range[] = { + DPLL_PIN_FREQUENCY_RANGE(0, DPLL_PIN_FREQUENCY_1_HZ), +}; + /** * ice_dpll_is_reset - check if reset is in progress * @pf: private board structure @@ -394,8 +399,8 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct ice_dpll_pin *pin, switch (pin_type) { case ICE_DPLL_PIN_TYPE_INPUT: - ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, NULL, NULL, - NULL, &pin->flags[0], + ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, &pin->status, + NULL, NULL, &pin->flags[0], &pin->freq, &pin->phase_adjust); if (ret) goto err; @@ -430,7 +435,7 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct ice_dpll_pin *pin, goto err; parent &= ICE_AQC_GET_CGU_OUT_CFG_DPLL_SRC_SEL; - if (ICE_AQC_SET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) { + if (ICE_AQC_GET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) { pin->state[pf->dplls.eec.dpll_idx] = parent == pf->dplls.eec.dpll_idx ? DPLL_PIN_STATE_CONNECTED : @@ -1098,6 +1103,221 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv, return 0; } +/** + * ice_dpll_output_esync_set - callback for setting embedded sync + * @pin: pointer to a pin + * @pin_priv: private data pointer passed on pin registration + * @dpll: registered dpll pointer + * @dpll_priv: private data pointer passed on dpll registration + * @esync_freq: requested embedded sync frequency + * @extack: error reporting + * + * Dpll subsystem callback. Handler for setting embedded sync frequency value + * on output pin. + * + * Context: Acquires pf->dplls.lock + * Return: + * * 0 - success + * * negative - error + */ +static int +ice_dpll_output_esync_set(const struct dpll_pin *pin, void *pin_priv, + const struct dpll_device *dpll, void *dpll_priv, + u64 esync_freq, struct netlink_ext_ack *extack) +{ + struct ice_dpll_pin *p = pin_priv; + struct ice_dpll *d = dpll_priv; + struct ice_pf *pf = d->pf; + u8 flags = 0; + int ret; + + if (ice_dpll_is_reset(pf, extack)) + return -EBUSY; + mutex_lock(&pf->dplls.lock); + if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_OUT_EN) + flags = ICE_AQC_SET_CGU_OUT_CFG_OUT_EN; + if (esync_freq == DPLL_PIN_FREQUENCY_1_HZ) { + if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) { + ret = 0; + } else { + flags |= ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN; + ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags, + 0, 0, 0); + } + } else { + if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN)) { + ret = 0; + } else { + flags &= ~ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN; + ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags, + 0, 0, 0); + } + } + mutex_unlock(&pf->dplls.lock); + if (ret) + NL_SET_ERR_MSG_FMT(extack, + "err:%d %s failed to set e-sync freq\n", + ret, + ice_aq_str(pf->hw.adminq.sq_last_status)); + return ret; +} + +/** + * ice_dpll_output_esync_get - callback for getting embedded sync config + * @pin: pointer to a pin + * @pin_priv: private data pointer passed on pin registration + * @dpll: registered dpll pointer + * @dpll_priv: private data pointer passed on dpll registration + * @esync: on success holds embedded sync pin properties + * @extack: error reporting + * + * Dpll subsystem callback. Handler for getting embedded sync frequency value + * and capabilities on output pin. + * + * Context: Acquires pf->dplls.lock + * Return: + * * 0 - success + * * negative - error + */ +static int +ice_dpll_output_esync_get(const struct dpll_pin *pin, void *pin_priv, + const struct dpll_device *dpll, void *dpll_priv, + struct dpll_pin_esync *esync, + struct netlink_ext_ack *extack) +{ + struct ice_dpll_pin *p = pin_priv; + struct ice_dpll *d = dpll_priv; + struct ice_pf *pf = d->pf; + + if (ice_dpll_is_reset(pf, extack)) + return -EBUSY; + mutex_lock(&pf->dplls.lock); + if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_ABILITY) || + p->freq != DPLL_PIN_FREQUENCY_10_MHZ) { + mutex_unlock(&pf->dplls.lock); + return -EOPNOTSUPP; + } + esync->range = ice_esync_range; + esync->range_num = ARRAY_SIZE(ice_esync_range); + if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) { + esync->freq = DPLL_PIN_FREQUENCY_1_HZ; + esync->pulse = ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT; + } else { + esync->freq = 0; + esync->pulse = 0; + } + mutex_unlock(&pf->dplls.lock); + return 0; +} + +/** + * ice_dpll_input_esync_set - callback for setting embedded sync + * @pin: pointer to a pin + * @pin_priv: private data pointer passed on pin registration + * @dpll: registered dpll pointer + * @dpll_priv: private data pointer passed on dpll registration + * @esync_freq: requested embedded sync frequency + * @extack: error reporting + * + * Dpll subsystem callback. Handler for setting embedded sync frequency value + * on input pin. + * + * Context: Acquires pf->dplls.lock + * Return: + * * 0 - success + * * negative - error + */ +static int +ice_dpll_input_esync_set(const struct dpll_pin *pin, void *pin_priv, + const struct dpll_device *dpll, void *dpll_priv, + u64 esync_freq, struct netlink_ext_ack *extack) +{ + struct ice_dpll_pin *p = pin_priv; + struct ice_dpll *d = dpll_priv; + struct ice_pf *pf = d->pf; + u8 flags_en = 0; + int ret; + + if (ice_dpll_is_reset(pf, extack)) + return -EBUSY; + mutex_lock(&pf->dplls.lock); + if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_INPUT_EN) + flags_en = ICE_AQC_SET_CGU_IN_CFG_FLG2_INPUT_EN; + if (esync_freq == DPLL_PIN_FREQUENCY_1_HZ) { + if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) { + ret = 0; + } else { + flags_en |= ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN; + ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0, + flags_en, 0, 0); + } + } else { + if (!(p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN)) { + ret = 0; + } else { + flags_en &= ~ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN; + ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0, + flags_en, 0, 0); + } + } + mutex_unlock(&pf->dplls.lock); + if (ret) + NL_SET_ERR_MSG_FMT(extack, + "err:%d %s failed to set e-sync freq\n", + ret, + ice_aq_str(pf->hw.adminq.sq_last_status)); + + return ret; +} + +/** + * ice_dpll_input_esync_get - callback for getting embedded sync config + * @pin: pointer to a pin + * @pin_priv: private data pointer passed on pin registration + * @dpll: registered dpll pointer + * @dpll_priv: private data pointer passed on dpll registration + * @esync: on success holds embedded sync pin properties + * @extack: error reporting + * + * Dpll subsystem callback. Handler for getting embedded sync frequency value + * and capabilities on input pin. + * + * Context: Acquires pf->dplls.lock + * Return: + * * 0 - success + * * negative - error + */ +static int +ice_dpll_input_esync_get(const struct dpll_pin *pin, void *pin_priv, + const struct dpll_device *dpll, void *dpll_priv, + struct dpll_pin_esync *esync, + struct netlink_ext_ack *extack) +{ + struct ice_dpll_pin *p = pin_priv; + struct ice_dpll *d = dpll_priv; + struct ice_pf *pf = d->pf; + + if (ice_dpll_is_reset(pf, extack)) + return -EBUSY; + mutex_lock(&pf->dplls.lock); + if (!(p->status & ICE_AQC_GET_CGU_IN_CFG_STATUS_ESYNC_CAP) || + p->freq != DPLL_PIN_FREQUENCY_10_MHZ) { + mutex_unlock(&pf->dplls.lock); + return -EOPNOTSUPP; + } + esync->range = ice_esync_range; + esync->range_num = ARRAY_SIZE(ice_esync_range); + if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) { + esync->freq = DPLL_PIN_FREQUENCY_1_HZ; + esync->pulse = ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT; + } else { + esync->freq = 0; + esync->pulse = 0; + } + mutex_unlock(&pf->dplls.lock); + return 0; +} + /** * ice_dpll_rclk_state_on_pin_set - set a state on rclk pin * @pin: pointer to a pin @@ -1222,6 +1442,8 @@ static const struct dpll_pin_ops ice_dpll_input_ops = { .phase_adjust_get = ice_dpll_pin_phase_adjust_get, .phase_adjust_set = ice_dpll_input_phase_adjust_set, .phase_offset_get = ice_dpll_phase_offset_get, + .esync_set = ice_dpll_input_esync_set, + .esync_get = ice_dpll_input_esync_get, }; static const struct dpll_pin_ops ice_dpll_output_ops = { @@ -1232,6 +1454,8 @@ static const struct dpll_pin_ops ice_dpll_output_ops = { .direction_get = ice_dpll_output_direction, .phase_adjust_get = ice_dpll_pin_phase_adjust_get, .phase_adjust_set = ice_dpll_output_phase_adjust_set, + .esync_set = ice_dpll_output_esync_set, + .esync_get = ice_dpll_output_esync_get, }; static const struct dpll_device_ops ice_dpll_ops = { diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h index 93172e93995b..c320f1bf7d6d 100644 --- a/drivers/net/ethernet/intel/ice/ice_dpll.h +++ b/drivers/net/ethernet/intel/ice/ice_dpll.h @@ -31,6 +31,7 @@ struct ice_dpll_pin { struct dpll_pin_properties prop; u32 freq; s32 phase_adjust; + u8 status; }; /** ice_dpll - store info required for DPLL control -- 2.38.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next v2 2/2] ice: add callbacks for Embedded SYNC enablement on dpll pins 2024-08-21 21:32 ` [PATCH net-next v2 2/2] ice: add callbacks for Embedded SYNC enablement on dpll pins Arkadiusz Kubalewski @ 2024-08-22 10:28 ` Jiri Pirko 2024-08-22 22:30 ` Kubalewski, Arkadiusz 0 siblings, 1 reply; 7+ messages in thread From: Jiri Pirko @ 2024-08-22 10:28 UTC (permalink / raw) To: Arkadiusz Kubalewski Cc: netdev, vadim.fedorenko, corbet, davem, edumazet, kuba, pabeni, donald.hunter, anthony.l.nguyen, przemyslaw.kitszel, intel-wired-lan, linux-doc, linux-kernel, Aleksandr Loktionov Wed, Aug 21, 2024 at 11:32:18PM CEST, arkadiusz.kubalewski@intel.com wrote: >Allow the user to get and set configuration of Embedded SYNC feature >on the ice driver dpll pins. > >Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >--- >v2: >- align to v2 changes of "dpll: add Embedded SYNC feature for a pin" > > drivers/net/ethernet/intel/ice/ice_dpll.c | 230 +++++++++++++++++++++- > drivers/net/ethernet/intel/ice/ice_dpll.h | 1 + > 2 files changed, 228 insertions(+), 3 deletions(-) Looks ok, couple of nitpicks below: > >diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c >index e92be6f130a3..aa6b87281ea6 100644 >--- a/drivers/net/ethernet/intel/ice/ice_dpll.c >+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c >@@ -9,6 +9,7 @@ > #define ICE_CGU_STATE_ACQ_ERR_THRESHOLD 50 > #define ICE_DPLL_PIN_IDX_INVALID 0xff > #define ICE_DPLL_RCLK_NUM_PER_PF 1 >+#define ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT 25 > > /** > * enum ice_dpll_pin_type - enumerate ice pin types: >@@ -30,6 +31,10 @@ static const char * const pin_type_name[] = { > [ICE_DPLL_PIN_TYPE_RCLK_INPUT] = "rclk-input", > }; > >+static const struct dpll_pin_frequency ice_esync_range[] = { >+ DPLL_PIN_FREQUENCY_RANGE(0, DPLL_PIN_FREQUENCY_1_HZ), >+}; >+ > /** > * ice_dpll_is_reset - check if reset is in progress > * @pf: private board structure >@@ -394,8 +399,8 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct ice_dpll_pin *pin, > > switch (pin_type) { > case ICE_DPLL_PIN_TYPE_INPUT: >- ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, NULL, NULL, >- NULL, &pin->flags[0], >+ ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, &pin->status, >+ NULL, NULL, &pin->flags[0], > &pin->freq, &pin->phase_adjust); > if (ret) > goto err; >@@ -430,7 +435,7 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct ice_dpll_pin *pin, > goto err; > > parent &= ICE_AQC_GET_CGU_OUT_CFG_DPLL_SRC_SEL; >- if (ICE_AQC_SET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) { >+ if (ICE_AQC_GET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) { > pin->state[pf->dplls.eec.dpll_idx] = > parent == pf->dplls.eec.dpll_idx ? > DPLL_PIN_STATE_CONNECTED : >@@ -1098,6 +1103,221 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv, > return 0; > } > >+/** >+ * ice_dpll_output_esync_set - callback for setting embedded sync >+ * @pin: pointer to a pin >+ * @pin_priv: private data pointer passed on pin registration >+ * @dpll: registered dpll pointer >+ * @dpll_priv: private data pointer passed on dpll registration >+ * @esync_freq: requested embedded sync frequency >+ * @extack: error reporting >+ * >+ * Dpll subsystem callback. Handler for setting embedded sync frequency value >+ * on output pin. >+ * >+ * Context: Acquires pf->dplls.lock >+ * Return: >+ * * 0 - success >+ * * negative - error >+ */ >+static int >+ice_dpll_output_esync_set(const struct dpll_pin *pin, void *pin_priv, >+ const struct dpll_device *dpll, void *dpll_priv, >+ u64 esync_freq, struct netlink_ext_ack *extack) s/esync_freq/freq/ >+{ >+ struct ice_dpll_pin *p = pin_priv; >+ struct ice_dpll *d = dpll_priv; >+ struct ice_pf *pf = d->pf; >+ u8 flags = 0; >+ int ret; >+ >+ if (ice_dpll_is_reset(pf, extack)) >+ return -EBUSY; >+ mutex_lock(&pf->dplls.lock); >+ if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_OUT_EN) >+ flags = ICE_AQC_SET_CGU_OUT_CFG_OUT_EN; >+ if (esync_freq == DPLL_PIN_FREQUENCY_1_HZ) { >+ if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) { >+ ret = 0; >+ } else { >+ flags |= ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN; >+ ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags, >+ 0, 0, 0); >+ } >+ } else { >+ if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN)) { >+ ret = 0; >+ } else { >+ flags &= ~ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN; >+ ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags, >+ 0, 0, 0); >+ } >+ } >+ mutex_unlock(&pf->dplls.lock); >+ if (ret) >+ NL_SET_ERR_MSG_FMT(extack, >+ "err:%d %s failed to set e-sync freq\n", >+ ret, >+ ice_aq_str(pf->hw.adminq.sq_last_status)); See my comment to ice_dpll_input_esync_set(), same applies here. >+ return ret; >+} >+ >+/** >+ * ice_dpll_output_esync_get - callback for getting embedded sync config >+ * @pin: pointer to a pin >+ * @pin_priv: private data pointer passed on pin registration >+ * @dpll: registered dpll pointer >+ * @dpll_priv: private data pointer passed on dpll registration >+ * @esync: on success holds embedded sync pin properties >+ * @extack: error reporting >+ * >+ * Dpll subsystem callback. Handler for getting embedded sync frequency value >+ * and capabilities on output pin. >+ * >+ * Context: Acquires pf->dplls.lock >+ * Return: >+ * * 0 - success >+ * * negative - error >+ */ >+static int >+ice_dpll_output_esync_get(const struct dpll_pin *pin, void *pin_priv, >+ const struct dpll_device *dpll, void *dpll_priv, >+ struct dpll_pin_esync *esync, >+ struct netlink_ext_ack *extack) >+{ >+ struct ice_dpll_pin *p = pin_priv; >+ struct ice_dpll *d = dpll_priv; >+ struct ice_pf *pf = d->pf; >+ >+ if (ice_dpll_is_reset(pf, extack)) >+ return -EBUSY; >+ mutex_lock(&pf->dplls.lock); >+ if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_ABILITY) || >+ p->freq != DPLL_PIN_FREQUENCY_10_MHZ) { >+ mutex_unlock(&pf->dplls.lock); >+ return -EOPNOTSUPP; >+ } >+ esync->range = ice_esync_range; >+ esync->range_num = ARRAY_SIZE(ice_esync_range); >+ if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) { >+ esync->freq = DPLL_PIN_FREQUENCY_1_HZ; >+ esync->pulse = ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT; >+ } else { >+ esync->freq = 0; >+ esync->pulse = 0; >+ } >+ mutex_unlock(&pf->dplls.lock); >+ return 0; >+} >+ >+/** >+ * ice_dpll_input_esync_set - callback for setting embedded sync >+ * @pin: pointer to a pin >+ * @pin_priv: private data pointer passed on pin registration >+ * @dpll: registered dpll pointer >+ * @dpll_priv: private data pointer passed on dpll registration >+ * @esync_freq: requested embedded sync frequency >+ * @extack: error reporting >+ * >+ * Dpll subsystem callback. Handler for setting embedded sync frequency value >+ * on input pin. >+ * >+ * Context: Acquires pf->dplls.lock >+ * Return: >+ * * 0 - success >+ * * negative - error >+ */ >+static int >+ice_dpll_input_esync_set(const struct dpll_pin *pin, void *pin_priv, >+ const struct dpll_device *dpll, void *dpll_priv, >+ u64 esync_freq, struct netlink_ext_ack *extack) >+{ >+ struct ice_dpll_pin *p = pin_priv; >+ struct ice_dpll *d = dpll_priv; >+ struct ice_pf *pf = d->pf; >+ u8 flags_en = 0; >+ int ret; >+ >+ if (ice_dpll_is_reset(pf, extack)) >+ return -EBUSY; >+ mutex_lock(&pf->dplls.lock); >+ if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_INPUT_EN) >+ flags_en = ICE_AQC_SET_CGU_IN_CFG_FLG2_INPUT_EN; >+ if (esync_freq == DPLL_PIN_FREQUENCY_1_HZ) { >+ if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) { >+ ret = 0; >+ } else { >+ flags_en |= ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN; >+ ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0, >+ flags_en, 0, 0); >+ } >+ } else { >+ if (!(p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN)) { >+ ret = 0; >+ } else { >+ flags_en &= ~ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN; >+ ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0, >+ flags_en, 0, 0); >+ } >+ } >+ mutex_unlock(&pf->dplls.lock); >+ if (ret) >+ NL_SET_ERR_MSG_FMT(extack, >+ "err:%d %s failed to set e-sync freq\n", Not sure how you do that in ice, but there should be a space after ":". But, in this case, print ret value in the message is redundant as you return ret value to the user. Remove. Moreover, this extack message has no value, as you basically say, that the command user executed failed, which he already knows by non-0 return value :) Either provide some useful details or avoid the extack message completely. >+ ret, >+ ice_aq_str(pf->hw.adminq.sq_last_status)); >+ >+ return ret; >+} >+ >+/** >+ * ice_dpll_input_esync_get - callback for getting embedded sync config >+ * @pin: pointer to a pin >+ * @pin_priv: private data pointer passed on pin registration >+ * @dpll: registered dpll pointer >+ * @dpll_priv: private data pointer passed on dpll registration >+ * @esync: on success holds embedded sync pin properties >+ * @extack: error reporting >+ * >+ * Dpll subsystem callback. Handler for getting embedded sync frequency value >+ * and capabilities on input pin. >+ * >+ * Context: Acquires pf->dplls.lock >+ * Return: >+ * * 0 - success >+ * * negative - error >+ */ >+static int >+ice_dpll_input_esync_get(const struct dpll_pin *pin, void *pin_priv, >+ const struct dpll_device *dpll, void *dpll_priv, >+ struct dpll_pin_esync *esync, >+ struct netlink_ext_ack *extack) >+{ >+ struct ice_dpll_pin *p = pin_priv; >+ struct ice_dpll *d = dpll_priv; >+ struct ice_pf *pf = d->pf; >+ >+ if (ice_dpll_is_reset(pf, extack)) >+ return -EBUSY; >+ mutex_lock(&pf->dplls.lock); >+ if (!(p->status & ICE_AQC_GET_CGU_IN_CFG_STATUS_ESYNC_CAP) || >+ p->freq != DPLL_PIN_FREQUENCY_10_MHZ) { >+ mutex_unlock(&pf->dplls.lock); >+ return -EOPNOTSUPP; >+ } >+ esync->range = ice_esync_range; >+ esync->range_num = ARRAY_SIZE(ice_esync_range); >+ if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) { >+ esync->freq = DPLL_PIN_FREQUENCY_1_HZ; >+ esync->pulse = ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT; >+ } else { >+ esync->freq = 0; >+ esync->pulse = 0; >+ } >+ mutex_unlock(&pf->dplls.lock); >+ return 0; >+} >+ > /** > * ice_dpll_rclk_state_on_pin_set - set a state on rclk pin > * @pin: pointer to a pin >@@ -1222,6 +1442,8 @@ static const struct dpll_pin_ops ice_dpll_input_ops = { > .phase_adjust_get = ice_dpll_pin_phase_adjust_get, > .phase_adjust_set = ice_dpll_input_phase_adjust_set, > .phase_offset_get = ice_dpll_phase_offset_get, >+ .esync_set = ice_dpll_input_esync_set, >+ .esync_get = ice_dpll_input_esync_get, > }; > > static const struct dpll_pin_ops ice_dpll_output_ops = { >@@ -1232,6 +1454,8 @@ static const struct dpll_pin_ops ice_dpll_output_ops = { > .direction_get = ice_dpll_output_direction, > .phase_adjust_get = ice_dpll_pin_phase_adjust_get, > .phase_adjust_set = ice_dpll_output_phase_adjust_set, >+ .esync_set = ice_dpll_output_esync_set, >+ .esync_get = ice_dpll_output_esync_get, > }; > > static const struct dpll_device_ops ice_dpll_ops = { >diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h >index 93172e93995b..c320f1bf7d6d 100644 >--- a/drivers/net/ethernet/intel/ice/ice_dpll.h >+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h >@@ -31,6 +31,7 @@ struct ice_dpll_pin { > struct dpll_pin_properties prop; > u32 freq; > s32 phase_adjust; >+ u8 status; > }; > > /** ice_dpll - store info required for DPLL control >-- >2.38.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH net-next v2 2/2] ice: add callbacks for Embedded SYNC enablement on dpll pins 2024-08-22 10:28 ` Jiri Pirko @ 2024-08-22 22:30 ` Kubalewski, Arkadiusz 0 siblings, 0 replies; 7+ messages in thread From: Kubalewski, Arkadiusz @ 2024-08-22 22:30 UTC (permalink / raw) To: Jiri Pirko Cc: netdev@vger.kernel.org, vadim.fedorenko@linux.dev, corbet@lwn.net, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, donald.hunter@gmail.com, Nguyen, Anthony L, Kitszel, Przemyslaw, intel-wired-lan@lists.osuosl.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, Loktionov, Aleksandr >From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, August 22, 2024 12:29 PM > >Wed, Aug 21, 2024 at 11:32:18PM CEST, arkadiusz.kubalewski@intel.com wrote: >>Allow the user to get and set configuration of Embedded SYNC feature >>on the ice driver dpll pins. >> >>Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> >>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>--- >>v2: >>- align to v2 changes of "dpll: add Embedded SYNC feature for a pin" >> >> drivers/net/ethernet/intel/ice/ice_dpll.c | 230 +++++++++++++++++++++- >> drivers/net/ethernet/intel/ice/ice_dpll.h | 1 + >> 2 files changed, 228 insertions(+), 3 deletions(-) > >Looks ok, couple of nitpicks below: > > > >> >>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c >>b/drivers/net/ethernet/intel/ice/ice_dpll.c >>index e92be6f130a3..aa6b87281ea6 100644 >>--- a/drivers/net/ethernet/intel/ice/ice_dpll.c >>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c >>@@ -9,6 +9,7 @@ >> #define ICE_CGU_STATE_ACQ_ERR_THRESHOLD 50 >> #define ICE_DPLL_PIN_IDX_INVALID 0xff >> #define ICE_DPLL_RCLK_NUM_PER_PF 1 >>+#define ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT 25 >> >> /** >> * enum ice_dpll_pin_type - enumerate ice pin types: >>@@ -30,6 +31,10 @@ static const char * const pin_type_name[] = { >> [ICE_DPLL_PIN_TYPE_RCLK_INPUT] = "rclk-input", >> }; >> >>+static const struct dpll_pin_frequency ice_esync_range[] = { >>+ DPLL_PIN_FREQUENCY_RANGE(0, DPLL_PIN_FREQUENCY_1_HZ), >>+}; >>+ >> /** >> * ice_dpll_is_reset - check if reset is in progress >> * @pf: private board structure >>@@ -394,8 +399,8 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct >>ice_dpll_pin *pin, >> >> switch (pin_type) { >> case ICE_DPLL_PIN_TYPE_INPUT: >>- ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, NULL, NULL, >>- NULL, &pin->flags[0], >>+ ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, &pin->status, >>+ NULL, NULL, &pin->flags[0], >> &pin->freq, &pin->phase_adjust); >> if (ret) >> goto err; >>@@ -430,7 +435,7 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct >>ice_dpll_pin *pin, >> goto err; >> >> parent &= ICE_AQC_GET_CGU_OUT_CFG_DPLL_SRC_SEL; >>- if (ICE_AQC_SET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) { >>+ if (ICE_AQC_GET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) { >> pin->state[pf->dplls.eec.dpll_idx] = >> parent == pf->dplls.eec.dpll_idx ? >> DPLL_PIN_STATE_CONNECTED : >>@@ -1098,6 +1103,221 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, >>void *pin_priv, >> return 0; >> } >> >>+/** >>+ * ice_dpll_output_esync_set - callback for setting embedded sync >>+ * @pin: pointer to a pin >>+ * @pin_priv: private data pointer passed on pin registration >>+ * @dpll: registered dpll pointer >>+ * @dpll_priv: private data pointer passed on dpll registration >>+ * @esync_freq: requested embedded sync frequency >>+ * @extack: error reporting >>+ * >>+ * Dpll subsystem callback. Handler for setting embedded sync frequency >>value >>+ * on output pin. >>+ * >>+ * Context: Acquires pf->dplls.lock >>+ * Return: >>+ * * 0 - success >>+ * * negative - error >>+ */ >>+static int >>+ice_dpll_output_esync_set(const struct dpll_pin *pin, void *pin_priv, >>+ const struct dpll_device *dpll, void *dpll_priv, >>+ u64 esync_freq, struct netlink_ext_ack *extack) > >s/esync_freq/freq/ > Fixed in v3. > >>+{ >>+ struct ice_dpll_pin *p = pin_priv; >>+ struct ice_dpll *d = dpll_priv; >>+ struct ice_pf *pf = d->pf; >>+ u8 flags = 0; >>+ int ret; >>+ >>+ if (ice_dpll_is_reset(pf, extack)) >>+ return -EBUSY; >>+ mutex_lock(&pf->dplls.lock); >>+ if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_OUT_EN) >>+ flags = ICE_AQC_SET_CGU_OUT_CFG_OUT_EN; >>+ if (esync_freq == DPLL_PIN_FREQUENCY_1_HZ) { >>+ if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) { >>+ ret = 0; >>+ } else { >>+ flags |= ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN; >>+ ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags, >>+ 0, 0, 0); >>+ } >>+ } else { >>+ if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN)) { >>+ ret = 0; >>+ } else { >>+ flags &= ~ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN; >>+ ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags, >>+ 0, 0, 0); >>+ } >>+ } >>+ mutex_unlock(&pf->dplls.lock); >>+ if (ret) >>+ NL_SET_ERR_MSG_FMT(extack, >>+ "err:%d %s failed to set e-sync freq\n", >>+ ret, >>+ ice_aq_str(pf->hw.adminq.sq_last_status)); > > >See my comment to ice_dpll_input_esync_set(), same applies here. > OK. > >>+ return ret; >>+} >>+ >>+/** >>+ * ice_dpll_output_esync_get - callback for getting embedded sync config >>+ * @pin: pointer to a pin >>+ * @pin_priv: private data pointer passed on pin registration >>+ * @dpll: registered dpll pointer >>+ * @dpll_priv: private data pointer passed on dpll registration >>+ * @esync: on success holds embedded sync pin properties >>+ * @extack: error reporting >>+ * >>+ * Dpll subsystem callback. Handler for getting embedded sync frequency >>value >>+ * and capabilities on output pin. >>+ * >>+ * Context: Acquires pf->dplls.lock >>+ * Return: >>+ * * 0 - success >>+ * * negative - error >>+ */ >>+static int >>+ice_dpll_output_esync_get(const struct dpll_pin *pin, void *pin_priv, >>+ const struct dpll_device *dpll, void *dpll_priv, >>+ struct dpll_pin_esync *esync, >>+ struct netlink_ext_ack *extack) >>+{ >>+ struct ice_dpll_pin *p = pin_priv; >>+ struct ice_dpll *d = dpll_priv; >>+ struct ice_pf *pf = d->pf; >>+ >>+ if (ice_dpll_is_reset(pf, extack)) >>+ return -EBUSY; >>+ mutex_lock(&pf->dplls.lock); >>+ if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_ABILITY) || >>+ p->freq != DPLL_PIN_FREQUENCY_10_MHZ) { >>+ mutex_unlock(&pf->dplls.lock); >>+ return -EOPNOTSUPP; >>+ } >>+ esync->range = ice_esync_range; >>+ esync->range_num = ARRAY_SIZE(ice_esync_range); >>+ if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) { >>+ esync->freq = DPLL_PIN_FREQUENCY_1_HZ; >>+ esync->pulse = ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT; >>+ } else { >>+ esync->freq = 0; >>+ esync->pulse = 0; >>+ } >>+ mutex_unlock(&pf->dplls.lock); >>+ return 0; >>+} >>+ >>+/** >>+ * ice_dpll_input_esync_set - callback for setting embedded sync >>+ * @pin: pointer to a pin >>+ * @pin_priv: private data pointer passed on pin registration >>+ * @dpll: registered dpll pointer >>+ * @dpll_priv: private data pointer passed on dpll registration >>+ * @esync_freq: requested embedded sync frequency >>+ * @extack: error reporting >>+ * >>+ * Dpll subsystem callback. Handler for setting embedded sync frequency >>value >>+ * on input pin. >>+ * >>+ * Context: Acquires pf->dplls.lock >>+ * Return: >>+ * * 0 - success >>+ * * negative - error >>+ */ >>+static int >>+ice_dpll_input_esync_set(const struct dpll_pin *pin, void *pin_priv, >>+ const struct dpll_device *dpll, void *dpll_priv, >>+ u64 esync_freq, struct netlink_ext_ack *extack) >>+{ >>+ struct ice_dpll_pin *p = pin_priv; >>+ struct ice_dpll *d = dpll_priv; >>+ struct ice_pf *pf = d->pf; >>+ u8 flags_en = 0; >>+ int ret; >>+ >>+ if (ice_dpll_is_reset(pf, extack)) >>+ return -EBUSY; >>+ mutex_lock(&pf->dplls.lock); >>+ if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_INPUT_EN) >>+ flags_en = ICE_AQC_SET_CGU_IN_CFG_FLG2_INPUT_EN; >>+ if (esync_freq == DPLL_PIN_FREQUENCY_1_HZ) { >>+ if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) { >>+ ret = 0; >>+ } else { >>+ flags_en |= ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN; >>+ ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0, >>+ flags_en, 0, 0); >>+ } >>+ } else { >>+ if (!(p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN)) { >>+ ret = 0; >>+ } else { >>+ flags_en &= ~ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN; >>+ ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0, >>+ flags_en, 0, 0); >>+ } >>+ } >>+ mutex_unlock(&pf->dplls.lock); >>+ if (ret) >>+ NL_SET_ERR_MSG_FMT(extack, >>+ "err:%d %s failed to set e-sync freq\n", > >Not sure how you do that in ice, but there should be a space after ":". >But, in this case, print ret value in the message is redundant as you >return ret value to the user. Remove. > >Moreover, this extack message has no value, as you basically say, that >the command user executed failed, which he already knows by non-0 return >value :) Either provide some useful details or avoid the extack message >completely. > OK, makes sense to me, removed in v3. Thank you! Arkadiusz > >>+ ret, >>+ ice_aq_str(pf->hw.adminq.sq_last_status)); >>+ >>+ return ret; >>+} >>+ >>+/** >>+ * ice_dpll_input_esync_get - callback for getting embedded sync config >>+ * @pin: pointer to a pin >>+ * @pin_priv: private data pointer passed on pin registration >>+ * @dpll: registered dpll pointer >>+ * @dpll_priv: private data pointer passed on dpll registration >>+ * @esync: on success holds embedded sync pin properties >>+ * @extack: error reporting >>+ * >>+ * Dpll subsystem callback. Handler for getting embedded sync frequency >>value >>+ * and capabilities on input pin. >>+ * >>+ * Context: Acquires pf->dplls.lock >>+ * Return: >>+ * * 0 - success >>+ * * negative - error >>+ */ >>+static int >>+ice_dpll_input_esync_get(const struct dpll_pin *pin, void *pin_priv, >>+ const struct dpll_device *dpll, void *dpll_priv, >>+ struct dpll_pin_esync *esync, >>+ struct netlink_ext_ack *extack) >>+{ >>+ struct ice_dpll_pin *p = pin_priv; >>+ struct ice_dpll *d = dpll_priv; >>+ struct ice_pf *pf = d->pf; >>+ >>+ if (ice_dpll_is_reset(pf, extack)) >>+ return -EBUSY; >>+ mutex_lock(&pf->dplls.lock); >>+ if (!(p->status & ICE_AQC_GET_CGU_IN_CFG_STATUS_ESYNC_CAP) || >>+ p->freq != DPLL_PIN_FREQUENCY_10_MHZ) { >>+ mutex_unlock(&pf->dplls.lock); >>+ return -EOPNOTSUPP; >>+ } >>+ esync->range = ice_esync_range; >>+ esync->range_num = ARRAY_SIZE(ice_esync_range); >>+ if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) { >>+ esync->freq = DPLL_PIN_FREQUENCY_1_HZ; >>+ esync->pulse = ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT; >>+ } else { >>+ esync->freq = 0; >>+ esync->pulse = 0; >>+ } >>+ mutex_unlock(&pf->dplls.lock); >>+ return 0; >>+} >>+ >> /** >> * ice_dpll_rclk_state_on_pin_set - set a state on rclk pin >> * @pin: pointer to a pin >>@@ -1222,6 +1442,8 @@ static const struct dpll_pin_ops ice_dpll_input_ops = { >> .phase_adjust_get = ice_dpll_pin_phase_adjust_get, >> .phase_adjust_set = ice_dpll_input_phase_adjust_set, >> .phase_offset_get = ice_dpll_phase_offset_get, >>+ .esync_set = ice_dpll_input_esync_set, >>+ .esync_get = ice_dpll_input_esync_get, >> }; >> >> static const struct dpll_pin_ops ice_dpll_output_ops = { >>@@ -1232,6 +1454,8 @@ static const struct dpll_pin_ops ice_dpll_output_ops = >>{ >> .direction_get = ice_dpll_output_direction, >> .phase_adjust_get = ice_dpll_pin_phase_adjust_get, >> .phase_adjust_set = ice_dpll_output_phase_adjust_set, >>+ .esync_set = ice_dpll_output_esync_set, >>+ .esync_get = ice_dpll_output_esync_get, >> }; >> >> static const struct dpll_device_ops ice_dpll_ops = { >>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h >>b/drivers/net/ethernet/intel/ice/ice_dpll.h >>index 93172e93995b..c320f1bf7d6d 100644 >>--- a/drivers/net/ethernet/intel/ice/ice_dpll.h >>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h >>@@ -31,6 +31,7 @@ struct ice_dpll_pin { >> struct dpll_pin_properties prop; >> u32 freq; >> s32 phase_adjust; >>+ u8 status; >> }; >> >> /** ice_dpll - store info required for DPLL control >>-- >>2.38.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-08-22 22:30 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-21 21:32 [PATCH net-next v2 0/2] Add Embedded SYNC feature for a dpll's pin Arkadiusz Kubalewski 2024-08-21 21:32 ` [PATCH net-next v2 1/2] dpll: add Embedded SYNC feature for a pin Arkadiusz Kubalewski 2024-08-22 10:22 ` Jiri Pirko 2024-08-22 22:30 ` Kubalewski, Arkadiusz 2024-08-21 21:32 ` [PATCH net-next v2 2/2] ice: add callbacks for Embedded SYNC enablement on dpll pins Arkadiusz Kubalewski 2024-08-22 10:28 ` Jiri Pirko 2024-08-22 22:30 ` Kubalewski, Arkadiusz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox