[PATCH net 0/2] ice: dpll: fix updates of the SyncE CGU registers on E825

[PATCH net 0/2] ice: dpll: fix updates of the SyncE CGU registers on E825

[Ivan Vecera]

All E825C PFs share the CGU registers R10 and R11, which hold the mux and
the divider of the two recovered clock outputs. Both patches fix how a link
change updates them, both found in review:

- Patch 1 adds a lock shared by all PFs around the read-modify-write
  sequences of these registers. A link change on one PF could undo an RCLK
  pin change made by another PF.
- Patch 2 lets a link change set the divider of an output only when its
  port drives that output. Every port set it from its own link speed,
  which is wrong when ports run at different speeds.

Both were tested on GNR-D with dual complex E825, details are in the commit
messages.

Patch 1 was also patch 2 of the net-next series "dpll: do not serialize
driver callbacks under global dpll_lock" [1], which needs it. v2 of that
series will drop it.

[1] https://lore.kernel.org/netdev/20261106121005.1327603-1-ivecera@redhat.com/

Ivan Vecera (2):
  ice: dpll: serialize changes of the SyncE CGU registers
  ice: dpll: set the SyncE divider only for the port driving the output

 drivers/net/ethernet/intel/ice/ice_adapter.c |  2 ++
 drivers/net/ethernet/intel/ice/ice_adapter.h |  4 ++++
 drivers/net/ethernet/intel/ice/ice_dpll.c    | 11 ++++++-----
 drivers/net/ethernet/intel/ice/ice_dpll.h    |  4 ++++
 drivers/net/ethernet/intel/ice/ice_ptp.c     |  8 +++++++-
 drivers/net/ethernet/intel/ice/ice_tspll.c   |  8 ++++----
 6 files changed, 27 insertions(+), 10 deletions(-)


base-commit: 1d5e2c57e9ffc134e14b3f837c8d7c53b508bd22
-- 
2.56.0

[PATCH net 1/2] ice: dpll: serialize changes of the SyncE CGU registers

[Ivan Vecera]

All E825C PFs share the CGU registers R10 and R11. On dual complex E825
the second complex reaches the CGU of complex 0 through cgu_peer. RCLK pin
callbacks read, modify and write these registers under dpll_lock and the
pf->dplls.lock of their PF. ice_ptp_link_change() rewrites R10 under the
pf->dplls.lock of its PF only, so a link change on one PF can write back
a stale R10 and undo an RCLK pin change made by another PF.

Add a mutex to struct ice_adapter, which all E825C PFs share, and hold it
around these sequences.

Tested on GNR-D with dual complex E825, with a 2 to 4 ms delay after the
read of R10 in the link change path to widen the window. Toggling the
RCLK pin of one port while another port of the same complex went down
and up 143 times lost 120 of 86814 updates of the mux in R10 without
this patch, and none of 76992 with it.

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_adapter.c |  2 ++
 drivers/net/ethernet/intel/ice/ice_adapter.h |  4 ++++
 drivers/net/ethernet/intel/ice/ice_dpll.c    | 11 ++++++-----
 drivers/net/ethernet/intel/ice/ice_dpll.h    |  4 ++++
 drivers/net/ethernet/intel/ice/ice_ptp.c     |  2 ++
 drivers/net/ethernet/intel/ice/ice_tspll.c   |  4 ++--
 6 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
index 536923b6ae979d..aa2d8addae5bef 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -72,6 +72,7 @@ static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
 	spin_lock_init(&adapter->txq_ctx_lock);
 	for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
 		mutex_init(&adapter->cpi_phy_lock[i]);
+	mutex_init(&adapter->cgu_synce_lock);
 	refcount_set(&adapter->refcount, 1);
 
 	spin_lock_init(&adapter->ports.lock);
@@ -85,6 +86,7 @@ static void ice_adapter_free(struct ice_adapter *adapter)
 	WARN_ON(!list_empty(&adapter->ports.list));
 	for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
 		mutex_destroy(&adapter->cpi_phy_lock[i]);
+	mutex_destroy(&adapter->cgu_synce_lock);
 
 	cleanup_srcu_struct(&adapter->ports.srcu);
 
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
index c61bd645e460de..fd5585005d91ba 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -41,6 +41,8 @@ struct ice_port_list {
  * @txq_ctx_lock: Spinlock protecting access to the GLCOMM_QTX_CNTX_CTL register
  * @cpi_phy_lock: Per-PHY mutex serializing CPI REQ/ACK transactions.
  *               Index 0 = PHY0, index 1 = PHY1. Used on E825C devices.
+ * @cgu_synce_lock: Mutex serializing changes of the CGU registers R10 and R11,
+ *                  which all PFs reach. Used on E825C devices.
  * @ctrl_pf: Control PF of the adapter
  * @ports: Ports list
  * @index: 64-bit index cached for collision detection on 32bit systems
@@ -53,6 +55,8 @@ struct ice_adapter {
 	spinlock_t txq_ctx_lock;
 	/* Serialize CPI REQ/ACK transactions per PHY (E825C only) */
 	struct mutex cpi_phy_lock[ICE_E825_MAX_PHYS];
+	/* Serialize changes of the SyncE CGU registers (E825C only) */
+	struct mutex cgu_synce_lock;
 
 	struct ice_pf *ctrl_pf;
 	struct ice_port_list ports;
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index ade1fb3dd06743..5148e419f49a07 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -2027,7 +2027,7 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
  *
  * DPLL subsystem callback. Set proper signals to recover clock from port.
  *
- * Context: Called under pf->dplls.lock
+ * Context: Called under pf->dplls.lock, acquires adapter->cgu_synce_lock
  * Return:
  * * 0 - success
  * * negative - error
@@ -2035,14 +2035,15 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
 static int ice_dpll_synce_update_e825c(struct ice_hw *hw, bool ena,
 				       u32 port_num, enum ice_synce_clk output)
 {
+	struct ice_pf *pf = hw->back;
 	int err;
 
+	mutex_lock(&pf->adapter->cgu_synce_lock);
 	/* configure the mux to deliver proper signal to DPLL from the MUX */
 	err = ice_tspll_cfg_bypass_mux_e825c(hw, ena, port_num, output);
-	if (err)
-		return err;
-
-	err = ice_tspll_cfg_synce_ethdiv_e825c(hw, output);
+	if (!err)
+		err = ice_tspll_cfg_synce_ethdiv_e825c(hw, output);
+	mutex_unlock(&pf->adapter->cgu_synce_lock);
 	if (err)
 		return err;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
index bce0bea9a664bc..3f9a993129acef 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
@@ -158,6 +158,7 @@ struct ice_dpll {
  *     txclk_notify_rwsem (read)
  *       -> pf->dplls.lock
  *         -> ctrl_pf->dplls.lock
+ *         -> adapter->cgu_synce_lock
  *
  *   - @lock serializes all DPLL state mutations on this PF. When the
  *     controlling PF's lock must also be taken (e.g. updating the shared
@@ -169,6 +170,9 @@ struct ice_dpll {
  *     dpll_*_change_ntf() calls. ice_dpll_deinit() takes the write side
  *     standalone (not nested under any other lock) to drain in-flight
  *     readers before pins and the TXC DPLL device are freed.
+ *   - adapter->cgu_synce_lock serializes changes of the CGU registers R10 and
+ *     R11. All E825C PFs reach one CGU, so @lock, which is per PF, does not
+ *     cover them.
  */
 struct ice_dplls {
 	struct kthread_worker *kworker;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 7980eaf9ce2535..449819cf8c8791 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1368,7 +1368,9 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
 				break;
 			}
 
+			mutex_lock(&pf->adapter->cgu_synce_lock);
 			err = ice_tspll_cfg_synce_ethdiv_e825c(hw, clk_pin);
+			mutex_unlock(&pf->adapter->cgu_synce_lock);
 			if (active && err) {
 				dev_err_once(ice_pf_to_dev(pf),
 					     "Failed to configure SyncE ETH divider for pin %d, err %d\n",
diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.c b/drivers/net/ethernet/intel/ice/ice_tspll.c
index 85bacb7c3a7903..b99e02cfe559bd 100644
--- a/drivers/net/ethernet/intel/ice/ice_tspll.c
+++ b/drivers/net/ethernet/intel/ice/ice_tspll.c
@@ -792,7 +792,7 @@ int ice_tspll_bypass_mux_active_e825c(struct ice_hw *hw, u8 port, bool *active,
  *
  * Set reference clock source and output clock selection.
  *
- * Context: Called under pf->dplls.lock
+ * Context: Called under pf->dplls.lock and adapter->cgu_synce_lock
  * Return:
  * * 0 - success
  * * negative - error
@@ -898,7 +898,7 @@ static int ice_tspll_get_div_e825c(u16 link_speed, unsigned int *divider)
  *
  * Set the correct CGU divider for RCLKA or RCLKB.
  *
- * Context: Called under pf->dplls.lock
+ * Context: Called under pf->dplls.lock and adapter->cgu_synce_lock
  * Return:
  * * 0 - success
  * * negative - error
-- 
2.56.0

Re: [PATCH net 1/2] ice: dpll: serialize changes of the SyncE CGU registers

[Petr Oros]

On 11/10/26 10:43 AM, Ivan Vecera wrote:
> All E825C PFs share the CGU registers R10 and R11. On dual complex E825
> the second complex reaches the CGU of complex 0 through cgu_peer. RCLK pin
> callbacks read, modify and write these registers under dpll_lock and the
> pf->dplls.lock of their PF. ice_ptp_link_change() rewrites R10 under the
> pf->dplls.lock of its PF only, so a link change on one PF can write back
> a stale R10 and undo an RCLK pin change made by another PF.
> 
> Add a mutex to struct ice_adapter, which all E825C PFs share, and hold it
> around these sequences.
> 
> Tested on GNR-D with dual complex E825, with a 2 to 4 ms delay after the
> read of R10 in the link change path to widen the window. Toggling the
> RCLK pin of one port while another port of the same complex went down
> and up 143 times lost 120 of 86814 updates of the mux in R10 without
> this patch, and none of 76992 with it.
> 
> 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_adapter.c |  2 ++
>   drivers/net/ethernet/intel/ice/ice_adapter.h |  4 ++++
>   drivers/net/ethernet/intel/ice/ice_dpll.c    | 11 ++++++-----
>   drivers/net/ethernet/intel/ice/ice_dpll.h    |  4 ++++
>   drivers/net/ethernet/intel/ice/ice_ptp.c     |  2 ++
>   drivers/net/ethernet/intel/ice/ice_tspll.c   |  4 ++--
>   6 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
> index 536923b6ae979d..aa2d8addae5bef 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.c
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
> @@ -72,6 +72,7 @@ static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
>   	spin_lock_init(&adapter->txq_ctx_lock);
>   	for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
>   		mutex_init(&adapter->cpi_phy_lock[i]);
> +	mutex_init(&adapter->cgu_synce_lock);
>   	refcount_set(&adapter->refcount, 1);
>   
>   	spin_lock_init(&adapter->ports.lock);
> @@ -85,6 +86,7 @@ static void ice_adapter_free(struct ice_adapter *adapter)
>   	WARN_ON(!list_empty(&adapter->ports.list));
>   	for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
>   		mutex_destroy(&adapter->cpi_phy_lock[i]);
> +	mutex_destroy(&adapter->cgu_synce_lock);
>   
>   	cleanup_srcu_struct(&adapter->ports.srcu);
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
> index c61bd645e460de..fd5585005d91ba 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
> @@ -41,6 +41,8 @@ struct ice_port_list {
>    * @txq_ctx_lock: Spinlock protecting access to the GLCOMM_QTX_CNTX_CTL register
>    * @cpi_phy_lock: Per-PHY mutex serializing CPI REQ/ACK transactions.
>    *               Index 0 = PHY0, index 1 = PHY1. Used on E825C devices.
> + * @cgu_synce_lock: Mutex serializing changes of the CGU registers R10 and R11,
> + *                  which all PFs reach. Used on E825C devices.
>    * @ctrl_pf: Control PF of the adapter
>    * @ports: Ports list
>    * @index: 64-bit index cached for collision detection on 32bit systems
> @@ -53,6 +55,8 @@ struct ice_adapter {
>   	spinlock_t txq_ctx_lock;
>   	/* Serialize CPI REQ/ACK transactions per PHY (E825C only) */
>   	struct mutex cpi_phy_lock[ICE_E825_MAX_PHYS];
> +	/* Serialize changes of the SyncE CGU registers (E825C only) */
> +	struct mutex cgu_synce_lock;
>   
>   	struct ice_pf *ctrl_pf;
>   	struct ice_port_list ports;
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index ade1fb3dd06743..5148e419f49a07 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
> @@ -2027,7 +2027,7 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
>    *
>    * DPLL subsystem callback. Set proper signals to recover clock from port.
>    *
> - * Context: Called under pf->dplls.lock
> + * Context: Called under pf->dplls.lock, acquires adapter->cgu_synce_lock
>    * Return:
>    * * 0 - success
>    * * negative - error
> @@ -2035,14 +2035,15 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
>   static int ice_dpll_synce_update_e825c(struct ice_hw *hw, bool ena,
>   				       u32 port_num, enum ice_synce_clk output)
>   {
> +	struct ice_pf *pf = hw->back;
>   	int err;
>   
> +	mutex_lock(&pf->adapter->cgu_synce_lock);
>   	/* configure the mux to deliver proper signal to DPLL from the MUX */
>   	err = ice_tspll_cfg_bypass_mux_e825c(hw, ena, port_num, output);
> -	if (err)
> -		return err;
> -
> -	err = ice_tspll_cfg_synce_ethdiv_e825c(hw, output);
> +	if (!err)
> +		err = ice_tspll_cfg_synce_ethdiv_e825c(hw, output);
> +	mutex_unlock(&pf->adapter->cgu_synce_lock);
>   	if (err)
>   		return err;
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
> index bce0bea9a664bc..3f9a993129acef 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.h
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
> @@ -158,6 +158,7 @@ struct ice_dpll {
>    *     txclk_notify_rwsem (read)
>    *       -> pf->dplls.lock
>    *         -> ctrl_pf->dplls.lock
> + *         -> adapter->cgu_synce_lock
>    *
>    *   - @lock serializes all DPLL state mutations on this PF. When the
>    *     controlling PF's lock must also be taken (e.g. updating the shared
> @@ -169,6 +170,9 @@ struct ice_dpll {
>    *     dpll_*_change_ntf() calls. ice_dpll_deinit() takes the write side
>    *     standalone (not nested under any other lock) to drain in-flight
>    *     readers before pins and the TXC DPLL device are freed.
> + *   - adapter->cgu_synce_lock serializes changes of the CGU registers R10 and
> + *     R11. All E825C PFs reach one CGU, so @lock, which is per PF, does not
> + *     cover them.
>    */
>   struct ice_dplls {
>   	struct kthread_worker *kworker;
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 7980eaf9ce2535..449819cf8c8791 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1368,7 +1368,9 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>   				break;
>   			}
>   
> +			mutex_lock(&pf->adapter->cgu_synce_lock);
>   			err = ice_tspll_cfg_synce_ethdiv_e825c(hw, clk_pin);
> +			mutex_unlock(&pf->adapter->cgu_synce_lock);
>   			if (active && err) {
>   				dev_err_once(ice_pf_to_dev(pf),
>   					     "Failed to configure SyncE ETH divider for pin %d, err %d\n",
> diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.c b/drivers/net/ethernet/intel/ice/ice_tspll.c
> index 85bacb7c3a7903..b99e02cfe559bd 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tspll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tspll.c
> @@ -792,7 +792,7 @@ int ice_tspll_bypass_mux_active_e825c(struct ice_hw *hw, u8 port, bool *active,
>    *
>    * Set reference clock source and output clock selection.
>    *
> - * Context: Called under pf->dplls.lock
> + * Context: Called under pf->dplls.lock and adapter->cgu_synce_lock
>    * Return:
>    * * 0 - success
>    * * negative - error
> @@ -898,7 +898,7 @@ static int ice_tspll_get_div_e825c(u16 link_speed, unsigned int *divider)
>    *
>    * Set the correct CGU divider for RCLKA or RCLKB.
>    *
> - * Context: Called under pf->dplls.lock
> + * Context: Called under pf->dplls.lock and adapter->cgu_synce_lock
>    * Return:
>    * * 0 - success
>    * * negative - error
Reviewed-by: Petr Oros <poros@redhat.com>

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

[Ivan Vecera]

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;
-- 
2.56.0

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

[Petr Oros]

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>