On 5/6/2025 2:35 AM, Przemyslaw Korba wrote:
Add control for E825 input pins: phy clock recovery and clock 1588.
E825 does not provide control over platform level DPLL but it
provides control over PHY clock recovery, and PTP/timestamp driven
inputs for platform level DPLL.
Introduce a software controlled layer of abstraction to:
- create a DPLL of type EEC for E825c,
- create recovered clock pin for each PF, and control them through
writing to registers,
- create pin to control clock 1588 for PF0, and control it through
writing to registers.
Reviewed-by: Milena Olech <milena.ol...@intel.com>
Signed-off-by: Przemyslaw Korba <przemyslaw.ko...@intel.com>
---
drivers/net/ethernet/intel/ice/ice_dpll.c | 856 ++++++++++++++++++--
drivers/net/ethernet/intel/ice/ice_dpll.h | 24 +-
drivers/net/ethernet/intel/ice/ice_main.c | 3 +-
drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 40 +-
drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 2 +
drivers/net/ethernet/intel/ice/ice_tspll.h | 7 +
drivers/net/ethernet/intel/ice/ice_type.h | 6 +
7 files changed, 865 insertions(+), 73 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c
b/drivers/net/ethernet/intel/ice/ice_dpll.c
index e36c5a853593..626436b87843 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_1588_NUM 1
#define ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT 25
#define ICE_DPLL_PIN_GEN_RCLK_FREQ 1953125
#define ICE_DPLL_PIN_PRIO_OUTPUT 0xff
@@ -59,6 +60,7 @@ static const char * const pin_type_name[] = {
static const char * const ice_dpll_sw_pin_sma[] = { "SMA1", "SMA2" };
static const char * const ice_dpll_sw_pin_ufl[] = { "U.FL1", "U.FL2" };
+static const char * const ice_dpll_pin_1588 = "pin_1588";
This looks like it's going to have the same issue as this:
https://lore.kernel.org/netdev/20250206223101.6469-2-przemyslaw.kits...@intel.com/
static const struct dpll_pin_frequency ice_esync_range[] = {
DPLL_PIN_FREQUENCY_RANGE(0, DPLL_PIN_FREQUENCY_1_HZ),
@@ -513,6 +515,107 @@ ice_dpll_pin_disable(struct ice_hw *hw, struct
ice_dpll_pin *pin,
return ret;
}
+/**
...
+static int ice_dpll_rclk_update(struct ice_pf *pf, struct ice_dpll_pin *pin,
+ u8 port_num)
+{
+ int ret = 0;
+
+ for (u8 parent = 0; parent < pf->dplls.rclk.num_parents; parent++) {
+ ret = ice_aq_get_phy_rec_clk_out(&pf->hw, &parent, &port_num,
+ &pin->flags[parent], NULL);
+ if (ret)
+ return ret;
+ SET_PIN_STATE(pin, parent,
+ ICE_AQC_GET_PHY_REC_CLK_OUT_OUT_EN &
+ pin->flags[parent]);
+ }
+
+ return ret;
This could be return 0; initialization would no longer be needed after that.
...
@@ -525,8 +628,7 @@ ice_dpll_pin_disable(struct ice_hw *hw, struct ice_dpll_pin
*pin,
* * 0 - OK
* * negative - error
*/
-static int
-ice_dpll_sw_pins_update(struct ice_pf *pf)
+static int ice_dpll_sw_pins_update(struct ice_pf *pf)
unrelated change.
{
struct ice_dplls *d = &pf->dplls;
struct ice_dpll_pin *p;
@@ -655,22 +757,14 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct
ice_dpll_pin *pin,
}
break;
case ICE_DPLL_PIN_TYPE_RCLK_INPUT:
- for (parent = 0; parent < pf->dplls.rclk.num_parents;
- parent++) {
- u8 p = parent;
-
- ret = ice_aq_get_phy_rec_clk_out(&pf->hw, &p,
- &port_num,
- &pin->flags[parent],
- NULL);
+ if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825) {
+ ret = ice_dpll_rclk_update_e825c(pf, pin);
+ if (ret)
+ goto err;
+ } else {
+ ret = ice_dpll_rclk_update(pf, pin, port_num);
if (ret)
goto err;
- if (ICE_AQC_GET_PHY_REC_CLK_OUT_OUT_EN &
- pin->flags[parent])
- pin->state[parent] = DPLL_PIN_STATE_CONNECTED;
- else
- pin->state[parent] =
- DPLL_PIN_STATE_DISCONNECTED;
}
break;
case ICE_DPLL_PIN_TYPE_SOFTWARE:
@@ -902,7 +996,7 @@ ice_dpll_input_state_set(const struct dpll_pin *pin, void
*pin_priv,
}
/**
- * ice_dpll_pin_state_get - set pin's state on dpll
+ * ice_dpll_pin_state_get - update pin's state
Also unrelated but probably worth another patch to correct this.
* @pin: pointer to a pin
* @pin_priv: private data pointer passed on pin registration
* @dpll: registered dpll pointer
...
+static int ice_dpll_synce_update_e825c(struct ice_hw *hw, bool ena,
+ u32 port_num, enum ice_synce_clk output)
+{
+ int err;
+
+ /* configure the mux to deliver proper signal to DPLL from the MUX */
+ err = ice_dpll_cfg_bypass_mux_e825c(hw, ena, port_num, output,
+ false);
+ if (err)
+ return err;
+
+ err = ice_dpll_cfg_synce_ethdiv_e825c(hw, output);
+ if (err)
+ return err;
+
+ dev_dbg(ice_hw_to_dev(hw), "CLK_SYNCE%u recovered clock: pin %s\n",
+ output, !!ena ? "Enabled" : "Disabled");
str_enabled_disabled() could be used here. Also ena is a bool so '!!'
shouldn't be needed.
+
+ return 0;
+}
+
/**
* ice_dpll_output_esync_set - callback for setting embedded sync
* @pin: pointer to a pin
@@ -2064,12 +2391,15 @@ ice_dpll_rclk_state_on_pin_set(const struct dpll_pin
*pin, void *pin_priv,
enum dpll_pin_state state,
struct netlink_ext_ack *extack)
{
- struct ice_dpll_pin *p = pin_priv, *parent = parent_pin_priv;
bool enable = state == DPLL_PIN_STATE_CONNECTED;
+ struct ice_dpll_pin *parent = parent_pin_priv;
+ struct ice_dpll_pin *p = pin_priv;
Is there a particular reason for this change?
struct ice_pf *pf = p->pf;
int ret = -EINVAL;
u32 hw_idx;
+ struct ice_hw *hw = &pf->hw;
Declarations should all be together. It looks like this won't RCT so the
initialization should be moved out.
https://docs.kernel.org/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs
+
if (ice_dpll_is_reset(pf, extack))
return -EBUSY;
...
+static int ice_dpll_pin_1588_state_on_pin_set(const struct dpll_pin *pin,
+ void *pin_priv,
+ const struct dpll_pin *parent_pin,
+ void *parent_pin_priv,
+ enum dpll_pin_state state,
+ struct netlink_ext_ack *extack)
+{
+ const struct ice_dpll_pin *parent = parent_pin_priv;
+ bool ena = state == DPLL_PIN_STATE_CONNECTED;
+ struct ice_dpll_pin *p = pin_priv;
+ struct ice_pf *pf = p->pf;
+ u32 hw_idx;
+ int ret;
+
+ if (ice_dpll_is_reset(pf, extack))
+ return -EBUSY;
+
+ mutex_lock(&pf->dplls.lock);
+ hw_idx = parent->idx - pf->dplls.base_rclk_idx;
+ if (hw_idx >= pf->dplls.num_inputs)
+ goto unlock;
+
+ if ((ena && p->state[hw_idx] == DPLL_PIN_STATE_CONNECTED) ||
+ (!ena && p->state[hw_idx] == DPLL_PIN_STATE_DISCONNECTED)) {
+ NL_SET_ERR_MSG(extack,
+ "Pin state on parent is already set");
+ goto unlock;
Looks like ret is undefined if we take this path.
+ }
+ ret = ice_dpll_cfg_bypass_mux_e825c(&pf->hw, ena, pf->hw.pf_id,
+ hw_idx, true);
+unlock:
+ mutex_unlock(&pf->dplls.lock);
+
+ return ret;
+}
+
/**
* ice_dpll_rclk_state_on_pin_get - get a state of rclk pin
* @pin: pointer to a pin
@@ -2122,7 +2509,8 @@ ice_dpll_rclk_state_on_pin_get(const struct dpll_pin
*pin, void *pin_priv,
enum dpll_pin_state *state,
struct netlink_ext_ack *extack)
{
- struct ice_dpll_pin *p = pin_priv, *parent = parent_pin_priv;
+ struct ice_dpll_pin *parent = parent_pin_priv;
+ struct ice_dpll_pin *p = pin_priv;
unrelated change.
struct ice_pf *pf = p->pf;
int ret = -EINVAL;
u32 hw_idx;
@@ -2148,12 +2536,76 @@ ice_dpll_rclk_state_on_pin_get(const struct dpll_pin
*pin, void *pin_priv,
return ret;
}
+/**
+ * ice_dpll_pin_1588_state_on_pin_get - get a state of a 1588 clock pin
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @parent_pin: pin parent pointer
+ * @parent_pin_priv: pin parent priv data pointer passed on pin registration
+ * @state: on success holds pin state on parent pin
+ * @extack: error reporting
+ *
+ * dpll subsystem callback, get a state of a 1588 clock pin.
+ *
+ * Context: Acquires pf->dplls.lock
+ * Return:
+ * * 0 - success
+ * * negative - failure
+ */
+static int
+ice_dpll_pin_1588_state_on_pin_get(const struct dpll_pin *pin, void *pin_priv,
+ const struct dpll_pin *parent_pin,
+ void *parent_pin_priv,
+ enum dpll_pin_state *state,
+ struct netlink_ext_ack *extack)
+{
+ const struct ice_dpll_pin *parent = parent_pin_priv;
+ struct ice_dpll_pin *p = pin_priv;
+ struct ice_pf *pf = p->pf;
+ u32 hw_idx;
+ int ret;
+
+ if (ice_dpll_is_reset(pf, extack))
+ return -EBUSY;
+
+ mutex_lock(&pf->dplls.lock);
+ hw_idx = parent->idx - pf->dplls.base_1588_idx;
+ if (hw_idx >= pf->dplls.num_inputs)
+ goto unlock;
Looks like ret would be undefined with the goto.
+
+ ret = ice_dpll_update_pin_1588_e825c(&pf->hw, p,
+ (enum ice_synce_clk)hw_idx);
+ if (ret)
+ goto unlock;
+ *state = p->state[hw_idx];
+unlock:
+ mutex_unlock(&pf->dplls.lock);
+
+ return ret;
+}
+
...
@@ -2593,10 +3051,25 @@ ice_dpll_init_direct_pins(struct ice_pf *pf, bool cgu,
*/
static void ice_dpll_deinit_rclk_pin(struct ice_pf *pf)
{
+ struct ice_dpll_pin *pin_1588 = &pf->dplls.pin_1588;
struct ice_dpll_pin *rclk = &pf->dplls.rclk;
struct ice_vsi *vsi = ice_get_main_vsi(pf);
struct dpll_pin *parent;
- int i;
+ u8 i;
+
+ if (pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825 &&
+ ice_pf_src_tmr_owned(pf)) {
+ for (i = 0; i < pin_1588->num_parents; i++) {
+ struct dpll_pin *parent =
+ pf->dplls.inputs[pin_1588->parent_idx[i]].pin;
missing a newline here. Also, if you need perform a check on a
declaration, please break out the assignment to be with the check.
+ if (!parent)
+ continue;
+ dpll_pin_on_pin_unregister(parent, pin_1588->pin,
+ &ice_dpll_pin_1588_ops,
+ pin_1588);
+ }
+ dpll_pin_put(pin_1588->pin);
+ }
for (i = 0; i < rclk->num_parents; i++) {
parent = pf->dplls.inputs[rclk->parent_idx[i]].pin;
...
+static int ice_dpll_init_info_direct_pins_e825c(struct ice_pf *pf,
+ enum ice_dpll_pin_type pin_type)
+{
+ struct ice_hw *hw = &pf->hw;
+ struct ice_dpll_pin *pins;
+ int num_pins, i, ret = 0;
+ unsigned long caps = 0;
+ bool input;
+
+ switch (pin_type) {
+ case ICE_DPLL_PIN_TYPE_INPUT:
+ pins = pf->dplls.inputs;
+ num_pins = pf->dplls.num_inputs;
+ input = true;
+ break;
+ case ICE_DPLL_PIN_TYPE_OUTPUT:
+ pins = pf->dplls.outputs;
+ num_pins = pf->dplls.num_outputs;
+ input = false;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ for (i = 0; i < num_pins; i++) {
+ pins[i].idx = i;
+ pins[i].prop.board_label = ice_cgu_get_pin_name(hw, i, input);
+ pins[i].prop.type = ice_cgu_get_pin_type(hw, i, input);
+ pins[i].prop.capabilities = caps;
+ pins[i].pf = pf;
+ }
+ return ret;
Looks like ret isn't needed at all. It's only initialized to 0 and
returned here; return 0 directly here.
...
@@ -110,6 +110,7 @@ struct ice_dplls {
struct ice_dpll pps;
struct ice_dpll_pin *inputs;
struct ice_dpll_pin *outputs;
+ struct ice_dpll_pin pin_1588;
struct ice_dpll_pin sma[ICE_DPLL_PIN_SW_NUM];
struct ice_dpll_pin ufl[ICE_DPLL_PIN_SW_NUM];
struct ice_dpll_pin rclk;
@@ -117,6 +118,7 @@ struct ice_dplls {
u8 num_outputs;
u8 sma_data;
u8 base_rclk_idx;
+ u8 base_1588_idx;
I believe kdoc needs to be updated for these.
int cgu_state_acq_err_num;
u64 clock_id;
s32 input_phase_adj_max;
@@ -133,3 +135,23 @@ static inline void ice_dpll_deinit(struct ice_pf *pf) { }
#endif
#endif
+
+#define ICE_CGU_R10 0x28
+#define ICE_CGU_R10_SYNCE_CLKO_SEL GENMASK(8, 5)
+#define ICE_CGU_R10_SYNCE_CLKODIV_M1 GENMASK(13, 9)
+#define ICE_CGU_R10_SYNCE_CLKODIV_LOAD BIT(14)
+#define ICE_CGU_R10_SYNCE_DCK_RST BIT(15)
+#define ICE_CGU_R10_SYNCE_ETHCLKO_SEL GENMASK(18, 16)
+#define ICE_CGU_R10_SYNCE_ETHDIV_M1 GENMASK(23, 19)
+#define ICE_CGU_R10_SYNCE_ETHDIV_LOAD BIT(24)
+#define ICE_CGU_R10_SYNCE_DCK2_RST BIT(25)
+#define ICE_CGU_R10_SYNCE_S_REF_CLK GENMASK(31, 27)
+
+#define ICE_CGU_R11 0x2C
+#define ICE_CGU_R11_SYNCE_S_BYP_CLK GENMASK(6, 1)
+
+#define ICE_CGU_BYPASS_MUX_OFFSET_E825C 3
+
+#define SET_PIN_STATE(_pin, _id, _condition) \
+ _pin->state[_id] = (_condition) ? DPLL_PIN_STATE_CONNECTED : \
+ DPLL_PIN_STATE_DISCONNECTED
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
b/drivers/net/ethernet/intel/ice/ice_main.c
index 3b2233a84f2e..07d4d135795b 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4814,7 +4814,8 @@ static void ice_init_features(struct ice_pf *pf)
ice_gnss_init(pf);
if (ice_is_feature_supported(pf, ICE_F_CGU) ||
- ice_is_feature_supported(pf, ICE_F_PHY_RCLK))
+ ice_is_feature_supported(pf, ICE_F_PHY_RCLK) ||
+ pf->hw.mac_type == ICE_MAC_GENERIC_3K_E825)
ice_dpll_init(pf);
I think it would make more sense to set the proper feature flag based on
this MAC (ice_init_feature_support()) rather than directly checking the
MAC type here.
...
base-commit: daa2036c311e81ee32f8cccc8257e3dfd4985f79
This doesn't apply and the base commit is relatively old. Please be sure
to rebase before sending.
Thanks,
Tony
