| Message ID | 20240930121022.671217-5-karol.kolacinski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix E825 initialization | expand |
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 --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