| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <PH0PR11MB4904D7B63E201423B6B85C369462A@PH0PR11MB4904.namprd11.prod.outlook.com>
Date: Mon, 2 Jun 2025 14:34:17 +0000
From: "Korba, Przemyslaw" <przemyslaw.korba@...el.com>
To: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>, "Olech, Milena" <milena.olech@...el.com>
Subject: RE: [PATCH iwl-next] ice: add recovery clock and clock 1588 control
for E825c
On 9/5/2025 10:53 PM, Anthony Nguyen wrote:
>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.olech@...el.com>
>> Signed-off-by: Przemyslaw Korba <przemyslaw.korba@...el.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.kitszel@intel.com/
>
True, I modified it
Thank you very much for your review, and sorry for taking long to respond...
>>
>> 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.
>
True! implemented
>...
>
>> @@ -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.
True! implemented
>
>> {
>> 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.
>
True! I modified it
>> * @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.
>
True! implemented
>> +
>> + 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?
>
Not really, I forgot why I even did it this way, I will revert back.
>> 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
True! I've changed it.
>
>> +
>> 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.
True, I've modified it.
>
>> + }
>> + 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.
True, I've modified it.
>
>> 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.
True, I've modified it
>
>> +
>> + 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.
True, I've modified it
>
>> + 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.
True, I've modified it.
>
>...
>
>> @@ -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.
True, I've modified it.
>
>> 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.
True, I did not know I can do it that way, thanks for letting me know.
>
>...
>> base-commit: daa2036c311e81ee32f8cccc8257e3dfd4985f79
>This doesn't apply and the base commit is relatively old. Please be sure to rebase before sending.
True, will keep that in mind. I will send v2 in a few days. Thank you for your review and patience.
>
>Thanks,
>Tony
>
>
Powered by blists - more mailing lists