Re: [PATCH net 2/2] ice: dpll: set the SyncE divider only for the port driving the output

[Petr Oros <poros@redhat.com>]

On 11/10/26 10:43 AM, Ivan Vecera wrote:
> R10 holds one divider for each recovered clock output, shared by all
> ports. ice_ptp_link_change() sets it from the link speed of its own port
> even when another port drives the output, and checks the mux only to
> decide whether an error is worth reporting. With ports running at
> different speeds, a link change on one port leaves the output of another
> port with a wrong divider.
> 
> Skip the outputs that the port does not drive, and check the mux under
> cgu_synce_lock, so that another PF cannot select its port in between.
> 
> The check computes the mux value as port % ports_per_phy + 3, while
> ice_tspll_cfg_bypass_mux_e825c() and ice_dpll_rclk_update_e825c() use
> port + 3. With the skip, a port from ports_per_phy up would never update
> the divider, so use port + 3 as they do.
> 
> Tested on GNR-D with dual complex E825. With the RCLK pin of one port
> connected, five down and up cycles of another port of the same complex
> called ice_tspll_cfg_synce_ethdiv_e825c() 20 times without this patch
> and never with it. Five cycles of the port driving the output still
> called it 10 times. Both ports ran at 25 Gb/s, so the divider value did
> not change on this setup.
> 
> Fixes: ad1df4f2d591 ("ice: dpll: Support E825-C SyncE and dynamic pin discovery")
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_ptp.c   | 10 +++++++---
>   drivers/net/ethernet/intel/ice/ice_tspll.c |  4 ++--
>   2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 449819cf8c8791..81565bbb692e9a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1350,6 +1350,7 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>   		int pin, err;
>   
>   		mutex_lock(&pf->dplls.lock);
> +		mutex_lock(&pf->adapter->cgu_synce_lock);
>   		for (pin = 0; pin < ICE_SYNCE_CLK_NUM; pin++) {
>   			enum ice_synce_clk clk_pin;
>   			bool active;
> @@ -1368,16 +1369,19 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>   				break;
>   			}
>   
> -			mutex_lock(&pf->adapter->cgu_synce_lock);
> +			/* The divider is shared, set it only for our port */
> +			if (!active)
> +				continue;
> +
>   			err = ice_tspll_cfg_synce_ethdiv_e825c(hw, clk_pin);
> -			mutex_unlock(&pf->adapter->cgu_synce_lock);
> -			if (active && err) {
> +			if (err) {
>   				dev_err_once(ice_pf_to_dev(pf),
>   					     "Failed to configure SyncE ETH divider for pin %d, err %d\n",
>   					     pin, err);
>   				break;
>   			}
>   		}
> +		mutex_unlock(&pf->adapter->cgu_synce_lock);
>   		mutex_unlock(&pf->dplls.lock);
>   
>   		if (linkup)
> diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.c b/drivers/net/ethernet/intel/ice/ice_tspll.c
> index b99e02cfe559bd..0bfbea43529523 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tspll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tspll.c
> @@ -746,6 +746,7 @@ int ice_tspll_init(struct ice_hw *hw)
>    *
>    * Check if given port is selected as recovered clock source for given output.
>    *
> + * Context: Called under pf->dplls.lock and adapter->cgu_synce_lock
>    * Return:
>    * * 0 - success
>    * * negative - error
> @@ -774,8 +775,7 @@ int ice_tspll_bypass_mux_active_e825c(struct ice_hw *hw, u8 port, bool *active,
>   		return -EINVAL;
>   	}
>   
> -	if (active_clk == port % hw->ptp.ports_per_phy +
> -			  ICE_CGU_BYPASS_MUX_OFFSET_E825C)
> +	if (active_clk == port + ICE_CGU_BYPASS_MUX_OFFSET_E825C)
>   		*active = true;
>   	else
>   		*active = false;
Reviewed-by: Petr Oros <poros@redhat.com>