From: Jiri Pirko <jiri@resnulli.us>
To: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
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,
anthony.l.nguyen@intel.com, przemyslaw.kitszel@intel.com,
intel-wired-lan@lists.osuosl.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org,
Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Subject: Re: [PATCH net-next v2 2/2] ice: add callbacks for Embedded SYNC enablement on dpll pins
Date: Thu, 22 Aug 2024 12:28:54 +0200 [thread overview]
Message-ID: <ZscS5jL_Uk0cqiga@nanopsycho.orion> (raw)
In-Reply-To: <20240821213218.232900-3-arkadiusz.kubalewski@intel.com>
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
>
next prev parent reply other threads:[~2024-08-22 10:28 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2024-08-22 22:30 ` Kubalewski, Arkadiusz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZscS5jL_Uk0cqiga@nanopsycho.orion \
--to=jiri@resnulli.us \
--cc=aleksandr.loktionov@intel.com \
--cc=anthony.l.nguyen@intel.com \
--cc=arkadiusz.kubalewski@intel.com \
--cc=corbet@lwn.net \
--cc=davem@davemloft.net \
--cc=donald.hunter@gmail.com \
--cc=edumazet@google.com \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=kuba@kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=przemyslaw.kitszel@intel.com \
--cc=vadim.fedorenko@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox