>From: Jiri Pirko <jiri@xxxxxxxxxxx>
>Sent: Thursday, August 22, 2024 12:29 PM
>
>Wed, Aug 21, 2024 at 11:32:18PM CEST, arkadiusz.kubalewski@xxxxxxxxx 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@xxxxxxxxx>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
>>---
>>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
>>
