diff mbox series

[iwl-net,4/5] ice: Remove unnecessary offset calculation for PF scoped registers

Message ID 20240930121022.671217-5-karol.kolacinski@intel.com
State Changes Requested
Headers show
Series Fix E825 initialization | expand

Commit Message

Karol Kolacinski Sept. 30, 2024, 12:08 p.m. UTC 
From: Jacob Keller <jacob.e.keller@intel.com>

The PFTSYN_SEM, PFINT_TSYN_MSK and PFHH_SEM registers are defined in the
datasheet as "PF scoped". PF scoped registers appear in the datasheet to
have a different offset per PF. This is misleading. Each PF has its own
scope of the register, and accessing any of the offsets on a given PF
will read the PF-appropriate register. There is no reason to calculate
different offsets when reading and writing to these registers.

The original code implementing access to the semaphore registers failed
to understand this nature of PF-scoped registers and included additional
offset calculations. Remove these.

This can be tested with direct access to the registers to show that each
PF sees its own scoped register:

  user@system:~ice$ for i in {0..7}; do sudo readpci -q -s 17:00.0 \
                    -a $((0x88880 + 4*i)); done
  0x88880 == 0x00000001
  0x88884 == 0x00000001
  0x88888 == 0x00000001
  0x8888c == 0x00000001
  0x88890 == 0x00000001
  0x88894 == 0x00000001
  0x88898 == 0x00000001
  0x8889c == 0x00000001
  user@system:~ice$ for i in {0..7}; do sudo readpci -q -s 17:00.0 \
                    -a $((0x88880 + 4*i)) -w 0 ; done
  0x88880 == 0x00000000
  0x88884 == 0x00000000
  0x88888 == 0x00000000
  0x8888c == 0x00000000
  0x88890 == 0x00000000
  0x88894 == 0x00000000
  0x88898 == 0x00000000
  0x8889c == 0x00000000
  user@system:~ice$ for i in {0..7}; do sudo readpci -q -s 17:00.0 \
                    -a $((0x88880 + 4*i)); done
  0x88880 == 0x00000001
  0x88884 == 0x00000001
  0x88888 == 0x00000001
  0x8888c == 0x00000001
  0x88890 == 0x00000001
  0x88894 == 0x00000001
  0x88898 == 0x00000001
  0x8889c == 0x00000001

Additionally, you can quickly tell that the PF-offset doesn't matter
because its not an included parameter of the auto-generated register
header file. Other parameters which do matter get generated as part of
the offset macros.

Fix the uses of PFTSYN_SEM, PFINT_TSYN_MSK and PFHH_SEM to stop doing
the unnecessary offset calculation.

Fixes: 7d606a1e2d05 ("ice: unify logic for programming PFINT_TSYN_MSK")
Fixes: 03cb4473be92 ("ice: add low level PTP clock access functions")
Fixes: 13a64f0b9894 ("ice: support crosstimestamping on E822 devices if supported")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c    | 10 +++++-----
 drivers/net/ethernet/intel/ice/ice_ptp.h    |  1 -
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c |  4 ++--
 drivers/net/ethernet/intel/ice/ice_ptp_hw.h |  2 --
 4 files changed, 7 insertions(+), 10 deletions(-)

Comments

Tony Nguyen Oct. 2, 2024, 10:12 p.m. UTC | #1 
On 9/30/2024 5:08 AM, Karol Kolacinski wrote:
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> The PFTSYN_SEM, PFINT_TSYN_MSK and PFHH_SEM registers are defined in the
> datasheet as "PF scoped". PF scoped registers appear in the datasheet to
> have a different offset per PF. This is misleading. Each PF has its own
> scope of the register, and accessing any of the offsets on a given PF
> will read the PF-appropriate register. There is no reason to calculate
> different offsets when reading and writing to these registers.
> 
> The original code implementing access to the semaphore registers failed
> to understand this nature of PF-scoped registers and included additional
> offset calculations. Remove these.
> 
> This can be tested with direct access to the registers to show that each
> PF sees its own scoped register:
> 
>    user@system:~ice$ for i in {0..7}; do sudo readpci -q -s 17:00.0 \
>                      -a $((0x88880 + 4*i)); done
>    0x88880 == 0x00000001
>    0x88884 == 0x00000001
>    0x88888 == 0x00000001
>    0x8888c == 0x00000001
>    0x88890 == 0x00000001
>    0x88894 == 0x00000001
>    0x88898 == 0x00000001
>    0x8889c == 0x00000001
>    user@system:~ice$ for i in {0..7}; do sudo readpci -q -s 17:00.0 \
>                      -a $((0x88880 + 4*i)) -w 0 ; done
>    0x88880 == 0x00000000
>    0x88884 == 0x00000000
>    0x88888 == 0x00000000
>    0x8888c == 0x00000000
>    0x88890 == 0x00000000
>    0x88894 == 0x00000000
>    0x88898 == 0x00000000
>    0x8889c == 0x00000000
>    user@system:~ice$ for i in {0..7}; do sudo readpci -q -s 17:00.0 \
>                      -a $((0x88880 + 4*i)); done
>    0x88880 == 0x00000001
>    0x88884 == 0x00000001
>    0x88888 == 0x00000001
>    0x8888c == 0x00000001
>    0x88890 == 0x00000001
>    0x88894 == 0x00000001
>    0x88898 == 0x00000001
>    0x8889c == 0x00000001
> 
> Additionally, you can quickly tell that the PF-offset doesn't matter
> because its not an included parameter of the auto-generated register
> header file. Other parameters which do matter get generated as part of
> the offset macros.
> 
> Fix the uses of PFTSYN_SEM, PFINT_TSYN_MSK and PFHH_SEM to stop doing
> the unnecessary offset calculation.

This also sounds like -next material rather than a bug fix.

Thanks,
Tony

> Fixes: 7d606a1e2d05 ("ice: unify logic for programming PFINT_TSYN_MSK")
> Fixes: 03cb4473be92 ("ice: add low level PTP clock access functions")
> Fixes: 13a64f0b9894 ("ice: support crosstimestamping on E822 devices if supported")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index ef2e858f49bb..956d5e21e261 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -269,12 +269,12 @@  static void ice_ptp_cfg_tx_interrupt(struct ice_pf *pf)
 	switch (pf->ptp.tx_interrupt_mode) {
 	case ICE_PTP_TX_INTERRUPT_ALL:
 		/* React to interrupts across all quads. */
-		wr32(hw, PFINT_TSYN_MSK + (0x4 * hw->pf_id), (u32)0x1f);
+		wr32(hw, PFINT_TSYN_MSK, 0x1f);
 		enable = true;
 		break;
 	case ICE_PTP_TX_INTERRUPT_NONE:
 		/* Do not react to interrupts on any quad. */
-		wr32(hw, PFINT_TSYN_MSK + (0x4 * hw->pf_id), (u32)0x0);
+		wr32(hw, PFINT_TSYN_MSK, 0);
 		enable = false;
 		break;
 	case ICE_PTP_TX_INTERRUPT_SELF:
@@ -2186,7 +2186,7 @@  ice_ptp_get_syncdevicetime(ktime_t *device,
 
 	for (i = 0; i < MAX_HH_HW_LOCK_TRIES; i++) {
 		/* Get the HW lock */
-		hh_lock = rd32(hw, PFHH_SEM + (PFTSYN_SEM_BYTES * hw->pf_id));
+		hh_lock = rd32(hw, PFHH_SEM);
 		if (hh_lock & PFHH_SEM_BUSY_M) {
 			usleep_range(10000, 15000);
 			continue;
@@ -2236,9 +2236,9 @@  ice_ptp_get_syncdevicetime(ktime_t *device,
 	ice_ptp_src_cmd(hw, ICE_PTP_NOP);
 
 	/* Release HW lock */
-	hh_lock = rd32(hw, PFHH_SEM + (PFTSYN_SEM_BYTES * hw->pf_id));
+	hh_lock = rd32(hw, PFHH_SEM);
 	hh_lock = hh_lock & ~PFHH_SEM_BUSY_M;
-	wr32(hw, PFHH_SEM + (PFTSYN_SEM_BYTES * hw->pf_id), hh_lock);
+	wr32(hw, PFHH_SEM, hh_lock);
 
 	if (i == MAX_HH_CTL_LOCK_TRIES)
 		return -ETIMEDOUT;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 2db2257a0fb2..3a61ea09f406 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -280,7 +280,6 @@  struct ice_ptp {
 #define ptp_info_to_pf(i) \
 	container_of(__ptp_info_to_ptp((i)), struct ice_pf, ptp)
 
-#define PFTSYN_SEM_BYTES		4
 #define PTP_SHARED_CLK_IDX_VALID	BIT(31)
 #define TS_CMD_MASK			0xF
 #define SYNC_EXEC_CMD			0x3
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 41c736868d82..4979633cab98 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -5369,7 +5369,7 @@  bool ice_ptp_lock(struct ice_hw *hw)
 #define MAX_TRIES 15
 
 	for (i = 0; i < MAX_TRIES; i++) {
-		hw_lock = rd32(hw, PFTSYN_SEM + (PFTSYN_SEM_BYTES * hw->pf_id));
+		hw_lock = rd32(hw, PFTSYN_SEM);
 		hw_lock = hw_lock & PFTSYN_SEM_BUSY_M;
 		if (hw_lock) {
 			/* Somebody is holding the lock */
@@ -5392,7 +5392,7 @@  bool ice_ptp_lock(struct ice_hw *hw)
  */
 void ice_ptp_unlock(struct ice_hw *hw)
 {
-	wr32(hw, PFTSYN_SEM + (PFTSYN_SEM_BYTES * hw->pf_id), 0);
+	wr32(hw, PFTSYN_SEM, 0);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
index c731196ace30..70d851bafaa3 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
@@ -452,8 +452,6 @@  static inline u64 ice_get_base_incval(struct ice_hw *hw)
 	}
 }
 
-#define PFTSYN_SEM_BYTES	4
-
 #define ICE_PTP_CLOCK_INDEX_0	0x00
 #define ICE_PTP_CLOCK_INDEX_1	0x01