* [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly
@ 2023-07-19 22:05 Jacob Keller
2023-07-19 22:05 ` [Intel-wired-lan] [PATCH iwl-next 2/4] ice: PTP: Rename macros used for PHY/QUAD port definitions Jacob Keller
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Jacob Keller @ 2023-07-19 22:05 UTC (permalink / raw)
To: Intel Wired LAN; +Cc: Karol Kolacinski, Anthony Nguyen
From: Karol Kolacinski <karol.kolacinski@intel.com>
E822 PHY TS registers should not be written and the only way to cleanup
them is to reset QUAD memory.
To ensure that the status bit for the timestamp index is cleared, ensure
that ice_clear_phy_tstamp implementations first read the timestamp out.
Implementations which can write the register continue to do so.
Add a note to indicate this function should only be called on timestamps
which have their valid bit set.
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 66 ++++++++++++---------
1 file changed, 39 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index f174bac58aba..6cab87595690 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -633,34 +633,31 @@ ice_read_phy_tstamp_e822(struct ice_hw *hw, u8 quad, u8 idx, u64 *tstamp)
}
/**
- * ice_clear_phy_tstamp_e822 - Clear a timestamp from the quad block
+ * ice_clear_phy_tstamp_e822 - Drop a timestamp from the quad block
* @hw: pointer to the HW struct
* @quad: the quad to read from
* @idx: the timestamp index to reset
*
- * Clear a timestamp, resetting its valid bit, from the PHY quad block that is
- * shared between the internal PHYs on the E822 devices.
+ * Read the timetamp out of the quad to clear its timestamp status bit from
+ * the PHY quad block that is shared between the internal PHYs of the E822
+ * devices.
+ *
+ * Note that software cannot directly write the quad memory bank registers,
+ * and must use ice_ptp_reset_ts_memory_quad_e822 for that purpose.
+ *
+ * This function should only be called on an idx whose bit is set according to
+ * ice_get_phy_tx_tstamp_ready.
*/
static int
ice_clear_phy_tstamp_e822(struct ice_hw *hw, u8 quad, u8 idx)
{
- u16 lo_addr, hi_addr;
+ u64 unused_tstamp;
int err;
- lo_addr = (u16)TS_L(Q_REG_TX_MEMORY_BANK_START, idx);
- hi_addr = (u16)TS_H(Q_REG_TX_MEMORY_BANK_START, idx);
-
- err = ice_write_quad_reg_e822(hw, quad, lo_addr, 0);
+ err = ice_read_phy_tstamp_e822(hw, quad, idx, &unused_tstamp);
if (err) {
- ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register, err %d\n",
- err);
- return err;
- }
-
- err = ice_write_quad_reg_e822(hw, quad, hi_addr, 0);
- if (err) {
- ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register, err %d\n",
- err);
+ ice_debug(hw, ICE_DBG_PTP, "Failed to read the timestamp register for quad %u, idx %u, err %d\n",
+ quad, idx, err);
return err;
}
@@ -2657,28 +2654,39 @@ ice_read_phy_tstamp_e810(struct ice_hw *hw, u8 lport, u8 idx, u64 *tstamp)
* @lport: the lport to read from
* @idx: the timestamp index to reset
*
- * Clear a timestamp, resetting its valid bit, from the timestamp block of the
- * external PHY on the E810 device.
+ * Read the timestamp and then forcibly overwrite its value to clear the valid
+ * bit from the timestamp block of the external PHY on the E810 device.
+ *
+ * This function should only be called on an idx whose bit is set according to
+ * ice_get_phy_tx_tstamp_ready.
*/
static int ice_clear_phy_tstamp_e810(struct ice_hw *hw, u8 lport, u8 idx)
{
u32 lo_addr, hi_addr;
+ u64 unused_tstamp;
int err;
+ err = ice_read_phy_tstamp_e810(hw, lport, idx, &unused_tstamp);
+ if (err) {
+ ice_debug(hw, ICE_DBG_PTP, "Failed to read the timestamp register for lport %u, idx %u, err %d\n",
+ lport, idx, err);
+ return err;
+ }
+
lo_addr = TS_EXT(LOW_TX_MEMORY_BANK_START, lport, idx);
hi_addr = TS_EXT(HIGH_TX_MEMORY_BANK_START, lport, idx);
err = ice_write_phy_reg_e810(hw, lo_addr, 0);
if (err) {
- ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register, err %d\n",
- err);
+ ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for lport %u, idx %u, err %d\n",
+ lport, idx, err);
return err;
}
err = ice_write_phy_reg_e810(hw, hi_addr, 0);
if (err) {
- ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register, err %d\n",
- err);
+ ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register for lport %u, idx %u, err %d\n",
+ lport, idx, err);
return err;
}
@@ -3326,14 +3334,18 @@ int ice_read_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx, u64 *tstamp)
}
/**
- * ice_clear_phy_tstamp - Clear a timestamp from the timestamp block
+ * ice_clear_phy_tstamp - Drop a timestamp from the timestamp block
* @hw: pointer to the HW struct
* @block: the block to read from
* @idx: the timestamp index to reset
*
- * Clear a timestamp, resetting its valid bit, from the timestamp block. For
- * E822 devices, the block is the quad to clear from. For E810 devices, the
- * block is the logical port to clear from.
+ * Drop a timestamp from the timestamp block by reading it. This will reset
+ * the memory status bit allowing the timestamp index to be reused. For E822
+ * devices, the block is the quad to clear from. For E810 devices, the block
+ * is the logical port to clear from.
+ *
+ * This function should only be called on a timestamp index whose valid bit
+ * is set according to ice_get_phy_tx_tstamp_ready.
*/
int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx)
{
base-commit: 751b10250f576db6fc2429132bccd453ee2e4a52
--
2.41.0.1.g9857a21e0017.dirty
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Intel-wired-lan] [PATCH iwl-next 2/4] ice: PTP: Rename macros used for PHY/QUAD port definitions 2023-07-19 22:05 [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly Jacob Keller @ 2023-07-19 22:05 ` Jacob Keller 2023-07-19 22:05 ` [Intel-wired-lan] [PATCH iwl-next 3/4] ice: PTP: move quad value check inside ice_fill_phy_msg_e822 Jacob Keller ` (2 subsequent siblings) 3 siblings, 0 replies; 6+ messages in thread From: Jacob Keller @ 2023-07-19 22:05 UTC (permalink / raw) To: Intel Wired LAN; +Cc: Karol Kolacinski, Anthony Nguyen From: Karol Kolacinski <karol.kolacinski@intel.com> The ice_fill_phy_msg_e822 function uses several macros to specify the the correct address when sending a sideband message to the PHY block in hardware. The names of these macros are fairly generic and confusing. Future development is going to extend the driver to support new hardware families which have different relationships between PHY and QUAD. Rename the macros for clarity and to indicate that they are E822 specific. This also matches closer to the hardware specification in the data sheet. Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 8 ++++---- drivers/net/ethernet/intel/ice/ice_type.h | 14 +++++++------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c index 6cab87595690..2f01f12a5391 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c @@ -178,9 +178,9 @@ ice_fill_phy_msg_e822(struct ice_sbq_msg_input *msg, u8 port, u16 offset) { int phy_port, phy, quadtype; - phy_port = port % ICE_PORTS_PER_PHY; - phy = port / ICE_PORTS_PER_PHY; - quadtype = (port / ICE_PORTS_PER_QUAD) % ICE_NUM_QUAD_TYPE; + phy_port = port % ICE_PORTS_PER_PHY_E822; + phy = port / ICE_PORTS_PER_PHY_E822; + quadtype = (port / ICE_PORTS_PER_QUAD) % ICE_QUADS_PER_PHY_E822; if (quadtype == 0) { msg->msg_addr_low = P_Q0_L(P_0_BASE + offset, phy_port); @@ -512,7 +512,7 @@ ice_fill_quad_msg_e822(struct ice_sbq_msg_input *msg, u8 quad, u16 offset) msg->dest_dev = rmn_0; - if ((quad % ICE_NUM_QUAD_TYPE) == 0) + if ((quad % ICE_QUADS_PER_PHY_E822) == 0) addr = Q_0_BASE + offset; else addr = Q_1_BASE + offset; diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h index f1231a8162af..6419c83ee077 100644 --- a/drivers/net/ethernet/intel/ice/ice_type.h +++ b/drivers/net/ethernet/intel/ice/ice_type.h @@ -886,13 +886,13 @@ struct ice_hw { /* INTRL granularity in 1 us */ u8 intrl_gran; -#define ICE_PHY_PER_NAC 1 -#define ICE_MAX_QUAD 2 -#define ICE_NUM_QUAD_TYPE 2 -#define ICE_PORTS_PER_QUAD 4 -#define ICE_PHY_0_LAST_QUAD 1 -#define ICE_PORTS_PER_PHY 8 -#define ICE_NUM_EXTERNAL_PORTS ICE_PORTS_PER_PHY +#define ICE_PHY_PER_NAC_E822 1 +#define ICE_MAX_QUAD 2 +#define ICE_QUADS_PER_PHY_E822 2 +#define ICE_PORTS_PER_PHY_E822 8 +#define ICE_PORTS_PER_QUAD 4 +#define ICE_PORTS_PER_PHY_E810 4 +#define ICE_NUM_EXTERNAL_PORTS (ICE_MAX_QUAD * ICE_PORTS_PER_QUAD) /* Active package version (currently active) */ struct ice_pkg_ver active_pkg_ver; -- 2.41.0.1.g9857a21e0017.dirty _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH iwl-next 3/4] ice: PTP: move quad value check inside ice_fill_phy_msg_e822 2023-07-19 22:05 [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly Jacob Keller 2023-07-19 22:05 ` [Intel-wired-lan] [PATCH iwl-next 2/4] ice: PTP: Rename macros used for PHY/QUAD port definitions Jacob Keller @ 2023-07-19 22:05 ` Jacob Keller 2023-07-19 22:05 ` [Intel-wired-lan] [PATCH iwl-next 4/4] ice: Initialize source timer before driving sync for single-PHY commands Jacob Keller 2023-07-20 5:56 ` [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly Paul Menzel 3 siblings, 0 replies; 6+ messages in thread From: Jacob Keller @ 2023-07-19 22:05 UTC (permalink / raw) To: Intel Wired LAN; +Cc: Karol Kolacinski, Anthony Nguyen From: Karol Kolacinski <karol.kolacinski@intel.com> The callers of ice_fill_phy_msg_e822 check for whether the quad number is within the expected range. Move this check inside the ice_fill_phy_msg_e822 function instead of duplicating it twice. Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c index 2f01f12a5391..168e72de06d7 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c @@ -505,11 +505,14 @@ ice_write_64b_phy_reg_e822(struct ice_hw *hw, u8 port, u16 low_addr, u64 val) * Fill a message buffer for accessing a register in a quad shared between * multiple PHYs. */ -static void +static int ice_fill_quad_msg_e822(struct ice_sbq_msg_input *msg, u8 quad, u16 offset) { u32 addr; + if (quad >= ICE_MAX_QUAD) + return -EINVAL; + msg->dest_dev = rmn_0; if ((quad % ICE_QUADS_PER_PHY_E822) == 0) @@ -519,6 +522,8 @@ ice_fill_quad_msg_e822(struct ice_sbq_msg_input *msg, u8 quad, u16 offset) msg->msg_addr_low = lower_16_bits(addr); msg->msg_addr_high = upper_16_bits(addr); + + return 0; } /** @@ -537,10 +542,10 @@ ice_read_quad_reg_e822(struct ice_hw *hw, u8 quad, u16 offset, u32 *val) struct ice_sbq_msg_input msg = {0}; int err; - if (quad >= ICE_MAX_QUAD) - return -EINVAL; + err = ice_fill_quad_msg_e822(&msg, quad, offset); + if (err) + return err; - ice_fill_quad_msg_e822(&msg, quad, offset); msg.opcode = ice_sbq_msg_rd; err = ice_sbq_rw_reg(hw, &msg); @@ -571,10 +576,10 @@ ice_write_quad_reg_e822(struct ice_hw *hw, u8 quad, u16 offset, u32 val) struct ice_sbq_msg_input msg = {0}; int err; - if (quad >= ICE_MAX_QUAD) - return -EINVAL; + err = ice_fill_quad_msg_e822(&msg, quad, offset); + if (err) + return err; - ice_fill_quad_msg_e822(&msg, quad, offset); msg.opcode = ice_sbq_msg_wr; msg.data = val; -- 2.41.0.1.g9857a21e0017.dirty _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH iwl-next 4/4] ice: Initialize source timer before driving sync for single-PHY commands 2023-07-19 22:05 [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly Jacob Keller 2023-07-19 22:05 ` [Intel-wired-lan] [PATCH iwl-next 2/4] ice: PTP: Rename macros used for PHY/QUAD port definitions Jacob Keller 2023-07-19 22:05 ` [Intel-wired-lan] [PATCH iwl-next 3/4] ice: PTP: move quad value check inside ice_fill_phy_msg_e822 Jacob Keller @ 2023-07-19 22:05 ` Jacob Keller 2023-07-20 5:56 ` [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly Paul Menzel 3 siblings, 0 replies; 6+ messages in thread From: Jacob Keller @ 2023-07-19 22:05 UTC (permalink / raw) To: Intel Wired LAN; +Cc: Siddaraju DH, Anthony Nguyen The ice hardware has a synchronization mechanism used to drive the simultaneous application of commands on both PHY ports and the source timer in the MAC. When issuing a sync via ice_ptp_exec_tmr_cmd(), the hardware will simultaneously apply the commands programmed for the main timer and each PHY port. If the driver has previously programmed the main timer, it may still have leftover configuration and could accidentally re-apply that command. We do use ice_ptp_clean_cmd() to cleanup after executing a timer command. However, it is good practice to also initialize the main timer in all cases where we are calling ice_ptp_exec_tmr_cmd(). In order to avoid any side effects, introduce a new ICE_PTP_NOP command which simply leaves the cmd_val bits set to 0, indicating no command is requested. This is important because we want to ensure that the timer index value is still set ensuring that the full timer command will be executed on the proper timer in the hardware. It is not expected to ever need ICE_PTP_NOP for PHY port commands. However for completeness, the value is checked anyways. Use this new command to initialize the main timer command in ice_sync_phy_timer_e822 and ice_start_phy_timer_e822, both functions which operate only on a single PHY port at a time. This makes it clear that these flows do not affect the main timer, and ensure that we do not rely on the behavior of previous executions clearing the main command via ice_ptp_clean_cmd(). Signed-off-by: Siddaraju DH <siddaraju.dh@intel.com> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- I chose not to target net here since I don't think this is strictly fixing a known issue or report. We already call ice_ptp_clean_cmd() to clear the old timer commands so I think there isn't a strict bug being fixed. However I think its still good practice to ensure that we initialize the main timer anyways. drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 ++++++++++++ drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 + 2 files changed, 13 insertions(+) diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c index 168e72de06d7..474bb4b2c839 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c @@ -131,6 +131,8 @@ void ice_ptp_src_cmd(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd) case ICE_PTP_READ_TIME: cmd_val |= GLTSYN_CMD_READ_TIME; break; + case ICE_PTP_NOP: + break; } wr32(hw, GLTSYN_CMD, cmd_val); @@ -1275,6 +1277,8 @@ ice_ptp_one_port_cmd(struct ice_hw *hw, u8 port, enum ice_ptp_tmr_cmd cmd) case ICE_PTP_ADJ_TIME_AT_TIME: cmd_val |= PHY_CMD_ADJ_TIME_AT_TIME; break; + case ICE_PTP_NOP: + break; } /* Tx case */ @@ -2267,6 +2271,9 @@ static int ice_sync_phy_timer_e822(struct ice_hw *hw, u8 port) if (err) goto err_unlock; + /* Do not perform any action on the main timer */ + ice_ptp_src_cmd(hw, ICE_PTP_NOP); + /* Issue the sync to activate the time adjustment */ ice_ptp_exec_tmr_cmd(hw); ice_ptp_clean_cmd(hw); @@ -2389,6 +2396,9 @@ int ice_start_phy_timer_e822(struct ice_hw *hw, u8 port) if (err) return err; + /* Do not perform any action on the main timer */ + ice_ptp_src_cmd(hw, ICE_PTP_NOP); + ice_ptp_exec_tmr_cmd(hw); err = ice_read_phy_reg_e822(hw, port, P_REG_PS, &val); @@ -2875,6 +2885,8 @@ static int ice_ptp_port_cmd_e810(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd) case ICE_PTP_ADJ_TIME_AT_TIME: cmd_val = GLTSYN_CMD_ADJ_INIT_TIME; break; + case ICE_PTP_NOP: + return 0; } /* Read, modify, write */ diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h index ba4ab96073e0..28e0afb06d54 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h @@ -10,6 +10,7 @@ enum ice_ptp_tmr_cmd { ICE_PTP_ADJ_TIME, ICE_PTP_ADJ_TIME_AT_TIME, ICE_PTP_READ_TIME, + ICE_PTP_NOP, }; enum ice_ptp_serdes { -- 2.41.0.1.g9857a21e0017.dirty _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly 2023-07-19 22:05 [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly Jacob Keller ` (2 preceding siblings ...) 2023-07-19 22:05 ` [Intel-wired-lan] [PATCH iwl-next 4/4] ice: Initialize source timer before driving sync for single-PHY commands Jacob Keller @ 2023-07-20 5:56 ` Paul Menzel 2023-07-24 19:07 ` Jacob Keller 3 siblings, 1 reply; 6+ messages in thread From: Paul Menzel @ 2023-07-20 5:56 UTC (permalink / raw) To: Jacob Keller; +Cc: intel-wired-lan, Karol Kolacinski, Anthony Nguyen Dear Jacob, dear Karol, Thank you for the patch. Some minor things, only if you care. Am 20.07.23 um 00:05 schrieb Jacob Keller: > From: Karol Kolacinski <karol.kolacinski@intel.com> > > E822 PHY TS registers should not be written and the only way to cleanup > them is to reset QUAD memory. The verb *clean up* is spelled with a space. … to clean them up … Also in the commit message summary/title: Clean up … > To ensure that the status bit for the timestamp index is cleared, ensure > that ice_clear_phy_tstamp implementations first read the timestamp out. > Implementations which can write the register continue to do so. > > Add a note to indicate this function should only be called on timestamps > which have their valid bit set. Maybe also mention the extended debug messages. > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 66 ++++++++++++--------- > 1 file changed, 39 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > index f174bac58aba..6cab87595690 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c > @@ -633,34 +633,31 @@ ice_read_phy_tstamp_e822(struct ice_hw *hw, u8 quad, u8 idx, u64 *tstamp) > } > > /** > - * ice_clear_phy_tstamp_e822 - Clear a timestamp from the quad block > + * ice_clear_phy_tstamp_e822 - Drop a timestamp from the quad block > * @hw: pointer to the HW struct > * @quad: the quad to read from > * @idx: the timestamp index to reset > * > - * Clear a timestamp, resetting its valid bit, from the PHY quad block that is > - * shared between the internal PHYs on the E822 devices. > + * Read the timetamp out of the quad to clear its timestamp status bit from timestamp > + * the PHY quad block that is shared between the internal PHYs of the E822 > + * devices. > + * > + * Note that software cannot directly write the quad memory bank registers, > + * and must use ice_ptp_reset_ts_memory_quad_e822 for that purpose. I’d add () to functions. > + * > + * This function should only be called on an idx whose bit is set according to > + * ice_get_phy_tx_tstamp_ready. > */ > static int > ice_clear_phy_tstamp_e822(struct ice_hw *hw, u8 quad, u8 idx) > { > - u16 lo_addr, hi_addr; > + u64 unused_tstamp; > int err; > > - lo_addr = (u16)TS_L(Q_REG_TX_MEMORY_BANK_START, idx); > - hi_addr = (u16)TS_H(Q_REG_TX_MEMORY_BANK_START, idx); > - > - err = ice_write_quad_reg_e822(hw, quad, lo_addr, 0); > + err = ice_read_phy_tstamp_e822(hw, quad, idx, &unused_tstamp); > if (err) { > - ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register, err %d\n", > - err); > - return err; > - } > - > - err = ice_write_quad_reg_e822(hw, quad, hi_addr, 0); > - if (err) { > - ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register, err %d\n", > - err); > + ice_debug(hw, ICE_DBG_PTP, "Failed to read the timestamp register for quad %u, idx %u, err %d\n", > + quad, idx, err); > return err; > } > > @@ -2657,28 +2654,39 @@ ice_read_phy_tstamp_e810(struct ice_hw *hw, u8 lport, u8 idx, u64 *tstamp) > * @lport: the lport to read from > * @idx: the timestamp index to reset > * > - * Clear a timestamp, resetting its valid bit, from the timestamp block of the > - * external PHY on the E810 device. > + * Read the timestamp and then forcibly overwrite its value to clear the valid > + * bit from the timestamp block of the external PHY on the E810 device. > + * > + * This function should only be called on an idx whose bit is set according to > + * ice_get_phy_tx_tstamp_ready. > */ > static int ice_clear_phy_tstamp_e810(struct ice_hw *hw, u8 lport, u8 idx) > { > u32 lo_addr, hi_addr; > + u64 unused_tstamp; > int err; > > + err = ice_read_phy_tstamp_e810(hw, lport, idx, &unused_tstamp); > + if (err) { > + ice_debug(hw, ICE_DBG_PTP, "Failed to read the timestamp register for lport %u, idx %u, err %d\n", > + lport, idx, err); > + return err; > + } > + > lo_addr = TS_EXT(LOW_TX_MEMORY_BANK_START, lport, idx); > hi_addr = TS_EXT(HIGH_TX_MEMORY_BANK_START, lport, idx); > > err = ice_write_phy_reg_e810(hw, lo_addr, 0); > if (err) { > - ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register, err %d\n", > - err); > + ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for lport %u, idx %u, err %d\n", > + lport, idx, err); > return err; > } > > err = ice_write_phy_reg_e810(hw, hi_addr, 0); > if (err) { > - ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register, err %d\n", > - err); > + ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register for lport %u, idx %u, err %d\n", > + lport, idx, err); > return err; > } > > @@ -3326,14 +3334,18 @@ int ice_read_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx, u64 *tstamp) > } > > /** > - * ice_clear_phy_tstamp - Clear a timestamp from the timestamp block > + * ice_clear_phy_tstamp - Drop a timestamp from the timestamp block > * @hw: pointer to the HW struct > * @block: the block to read from > * @idx: the timestamp index to reset > * > - * Clear a timestamp, resetting its valid bit, from the timestamp block. For > - * E822 devices, the block is the quad to clear from. For E810 devices, the > - * block is the logical port to clear from. > + * Drop a timestamp from the timestamp block by reading it. This will reset > + * the memory status bit allowing the timestamp index to be reused. For E822 > + * devices, the block is the quad to clear from. For E810 devices, the block > + * is the logical port to clear from. > + * > + * This function should only be called on a timestamp index whose valid bit > + * is set according to ice_get_phy_tx_tstamp_ready. > */ > int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx) > { As the function name still contains *clear*, I am unsure, what the difference between *drop* and *clear* is. Kind regards, Paul _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly 2023-07-20 5:56 ` [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly Paul Menzel @ 2023-07-24 19:07 ` Jacob Keller 0 siblings, 0 replies; 6+ messages in thread From: Jacob Keller @ 2023-07-24 19:07 UTC (permalink / raw) To: Paul Menzel; +Cc: intel-wired-lan, Karol Kolacinski, Anthony Nguyen On 7/19/2023 10:56 PM, Paul Menzel wrote: > Dear Jacob, dear Karol, > > > Thank you for the patch. Some minor things, only if you care. > > Am 20.07.23 um 00:05 schrieb Jacob Keller: >> From: Karol Kolacinski <karol.kolacinski@intel.com> >> >> E822 PHY TS registers should not be written and the only way to cleanup >> them is to reset QUAD memory. > > The verb *clean up* is spelled with a space. > > … to clean them up … > > Also in the commit message summary/title: Clean up … > >> To ensure that the status bit for the timestamp index is cleared, ensure >> that ice_clear_phy_tstamp implementations first read the timestamp out. >> Implementations which can write the register continue to do so. >> >> Add a note to indicate this function should only be called on timestamps >> which have their valid bit set. > > Maybe also mention the extended debug messages. > >> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >> --- >> drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 66 ++++++++++++--------- >> 1 file changed, 39 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c >> index f174bac58aba..6cab87595690 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c >> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c >> @@ -633,34 +633,31 @@ ice_read_phy_tstamp_e822(struct ice_hw *hw, u8 quad, u8 idx, u64 *tstamp) >> } >> >> /** >> - * ice_clear_phy_tstamp_e822 - Clear a timestamp from the quad block >> + * ice_clear_phy_tstamp_e822 - Drop a timestamp from the quad block >> * @hw: pointer to the HW struct >> * @quad: the quad to read from >> * @idx: the timestamp index to reset >> * >> - * Clear a timestamp, resetting its valid bit, from the PHY quad block that is >> - * shared between the internal PHYs on the E822 devices. >> + * Read the timetamp out of the quad to clear its timestamp status bit from > > timestamp Will fix. > >> + * the PHY quad block that is shared between the internal PHYs of the E822 >> + * devices. >> + * >> + * Note that software cannot directly write the quad memory bank registers, >> + * and must use ice_ptp_reset_ts_memory_quad_e822 for that purpose. > > I’d add () to functions. Will fix. > >> + * >> + * This function should only be called on an idx whose bit is set according to >> + * ice_get_phy_tx_tstamp_ready. >> */ >> static int >> ice_clear_phy_tstamp_e822(struct ice_hw *hw, u8 quad, u8 idx) >> { >> - u16 lo_addr, hi_addr; >> + u64 unused_tstamp; >> int err; >> >> - lo_addr = (u16)TS_L(Q_REG_TX_MEMORY_BANK_START, idx); >> - hi_addr = (u16)TS_H(Q_REG_TX_MEMORY_BANK_START, idx); >> - >> - err = ice_write_quad_reg_e822(hw, quad, lo_addr, 0); >> + err = ice_read_phy_tstamp_e822(hw, quad, idx, &unused_tstamp); >> if (err) { >> - ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register, err %d\n", >> - err); >> - return err; >> - } >> - >> - err = ice_write_quad_reg_e822(hw, quad, hi_addr, 0); >> - if (err) { >> - ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register, err %d\n", >> - err); >> + ice_debug(hw, ICE_DBG_PTP, "Failed to read the timestamp register for quad %u, idx %u, err %d\n", >> + quad, idx, err); >> return err; >> } >> >> @@ -2657,28 +2654,39 @@ ice_read_phy_tstamp_e810(struct ice_hw *hw, u8 lport, u8 idx, u64 *tstamp) >> * @lport: the lport to read from >> * @idx: the timestamp index to reset >> * >> - * Clear a timestamp, resetting its valid bit, from the timestamp block of the >> - * external PHY on the E810 device. >> + * Read the timestamp and then forcibly overwrite its value to clear the valid >> + * bit from the timestamp block of the external PHY on the E810 device. >> + * >> + * This function should only be called on an idx whose bit is set according to >> + * ice_get_phy_tx_tstamp_ready. >> */ >> static int ice_clear_phy_tstamp_e810(struct ice_hw *hw, u8 lport, u8 idx) >> { >> u32 lo_addr, hi_addr; >> + u64 unused_tstamp; >> int err; >> >> + err = ice_read_phy_tstamp_e810(hw, lport, idx, &unused_tstamp); >> + if (err) { >> + ice_debug(hw, ICE_DBG_PTP, "Failed to read the timestamp register for lport %u, idx %u, err %d\n", >> + lport, idx, err); >> + return err; >> + } >> + >> lo_addr = TS_EXT(LOW_TX_MEMORY_BANK_START, lport, idx); >> hi_addr = TS_EXT(HIGH_TX_MEMORY_BANK_START, lport, idx); >> >> err = ice_write_phy_reg_e810(hw, lo_addr, 0); >> if (err) { >> - ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register, err %d\n", >> - err); >> + ice_debug(hw, ICE_DBG_PTP, "Failed to clear low PTP timestamp register for lport %u, idx %u, err %d\n", >> + lport, idx, err); >> return err; >> } >> >> err = ice_write_phy_reg_e810(hw, hi_addr, 0); >> if (err) { >> - ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register, err %d\n", >> - err); >> + ice_debug(hw, ICE_DBG_PTP, "Failed to clear high PTP timestamp register for lport %u, idx %u, err %d\n", >> + lport, idx, err); >> return err; >> } >> >> @@ -3326,14 +3334,18 @@ int ice_read_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx, u64 *tstamp) >> } >> >> /** >> - * ice_clear_phy_tstamp - Clear a timestamp from the timestamp block >> + * ice_clear_phy_tstamp - Drop a timestamp from the timestamp block >> * @hw: pointer to the HW struct >> * @block: the block to read from >> * @idx: the timestamp index to reset >> * >> - * Clear a timestamp, resetting its valid bit, from the timestamp block. For >> - * E822 devices, the block is the quad to clear from. For E810 devices, the >> - * block is the logical port to clear from. >> + * Drop a timestamp from the timestamp block by reading it. This will reset >> + * the memory status bit allowing the timestamp index to be reused. For E822 >> + * devices, the block is the quad to clear from. For E810 devices, the block >> + * is the logical port to clear from. >> + * >> + * This function should only be called on a timestamp index whose valid bit >> + * is set according to ice_get_phy_tx_tstamp_ready. >> */ >> int ice_clear_phy_tstamp(struct ice_hw *hw, u8 block, u8 idx) >> { > > As the function name still contains *clear*, I am unsure, what the > difference between *drop* and *clear* is. > I'll try to clarify this in v2. It turns out that one of my colleagues believes this last change should be on net as a fix, so I am going to send that first. Thanks, Jake > > Kind regards, > > Paul _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-24 19:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-19 22:05 [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly Jacob Keller 2023-07-19 22:05 ` [Intel-wired-lan] [PATCH iwl-next 2/4] ice: PTP: Rename macros used for PHY/QUAD port definitions Jacob Keller 2023-07-19 22:05 ` [Intel-wired-lan] [PATCH iwl-next 3/4] ice: PTP: move quad value check inside ice_fill_phy_msg_e822 Jacob Keller 2023-07-19 22:05 ` [Intel-wired-lan] [PATCH iwl-next 4/4] ice: Initialize source timer before driving sync for single-PHY commands Jacob Keller 2023-07-20 5:56 ` [Intel-wired-lan] [PATCH iwl-next 1/4] ice: PTP: Cleanup timestamp registers correctly Paul Menzel 2023-07-24 19:07 ` Jacob Keller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox