From: Karol Kolacinski <[email protected]> Date: Thu, 4 Apr 2024 11:09:53 +0200
> From: Sergey Temerkhanov <[email protected]> > > Move CGU block to the beginning of ice_ptp_hw.c > > Signed-off-by: Sergey Temerkhanov <[email protected]> > Reviewed-by: Przemek Kitszel <[email protected]> > Reviewed-by: Arkadiusz Kubalewski <[email protected]> > Signed-off-by: Karol Kolacinski <[email protected]> [...] > +static int ice_read_cgu_reg_e82x(struct ice_hw *hw, u32 addr, u32 *val) > +{ > + struct ice_sbq_msg_input cgu_msg; > + int err; > + > + cgu_msg.opcode = ice_sbq_msg_rd; > + cgu_msg.dest_dev = cgu; > + cgu_msg.msg_addr_low = addr; > + cgu_msg.msg_addr_high = 0x0; > + > + err = ice_sbq_rw_reg(hw, &cgu_msg); > + if (err) { > + ice_debug(hw, ICE_DBG_PTP, "Failed to read CGU register 0x%04x, > err %d\n", > + addr, err); > + return err; > + } > + > + *val = cgu_msg.data; > + > + return err; return 0; since @err is already checked above. > +} > + > +/** > + * ice_write_cgu_reg_e82x - Write a CGU register > + * @hw: pointer to the HW struct > + * @addr: Register address to write > + * @val: value to write into the register > + * > + * Write the specified value to a register of the Clock Generation Unit. Only > + * applicable to E822 devices. > + */ > +static int ice_write_cgu_reg_e82x(struct ice_hw *hw, u32 addr, u32 val) > +{ > + struct ice_sbq_msg_input cgu_msg; > + int err; > + > + cgu_msg.opcode = ice_sbq_msg_wr; > + cgu_msg.dest_dev = cgu; > + cgu_msg.msg_addr_low = addr; > + cgu_msg.msg_addr_high = 0x0; > + cgu_msg.data = val; Maybe initialize it when declaring? struct ice_sbq_msg_input cgu_msg = { .opcode = ice_sbq_msg_wr, .dest_dev = cgu, ... }; > + > + err = ice_sbq_rw_reg(hw, &cgu_msg); > + if (err) { > + ice_debug(hw, ICE_DBG_PTP, "Failed to write CGU register > 0x%04x, err %d\n", > + addr, err); > + return err; > + } > + > + return err; return 0; > +} > + > +/** > + * ice_clk_freq_str - Convert time_ref_freq to string > + * @clk_freq: Clock frequency > + * > + * Convert the specified TIME_REF clock frequency to a string. > + */ > +static const char *ice_clk_freq_str(u8 clk_freq) > +{ > + switch ((enum ice_time_ref_freq)clk_freq) { You can declare @clk_freq as enum and avoid this cast. > + case ICE_TIME_REF_FREQ_25_000: > + return "25 MHz"; > + case ICE_TIME_REF_FREQ_122_880: > + return "122.88 MHz"; > + case ICE_TIME_REF_FREQ_125_000: > + return "125 MHz"; > + case ICE_TIME_REF_FREQ_153_600: > + return "153.6 MHz"; > + case ICE_TIME_REF_FREQ_156_250: > + return "156.25 MHz"; > + case ICE_TIME_REF_FREQ_245_760: > + return "245.76 MHz"; > + default: > + return "Unknown"; > + } Array lookup produces more optimized code than switch-case. static const char * const ice_clk_freqs[] = { [ICE_TIME_REF_FREQ_25_000] = "25 MHz", ... [NUM_ICE_TIME_REF_FREQ] = "Unknown", }; static const char *ice_clk_freq_str(enum ice_time_ref_freq clk_freq) { return ice_clk_freqs[min(clk_freq, NUM_ICE_TIME_REF_FREQ)]; } > +} > + > +/** > + * ice_clk_src_str - Convert time_ref_src to string > + * @clk_src: Clock source > + * > + * Convert the specified clock source to its string name. > + */ > +static const char *ice_clk_src_str(u8 clk_src) > +{ > + switch ((enum ice_clk_src)clk_src) { > + case ICE_CLK_SRC_TCX0: > + return "TCX0"; > + case ICE_CLK_SRC_TIME_REF: > + return "TIME_REF"; > + default: > + return "Unknown"; > + } Same as above. [...] > -int > -ice_write_quad_reg_e82x(struct ice_hw *hw, u8 quad, u16 offset, u32 val) > +static int > +ice_write_phy_reg_e82x(struct ice_hw *hw, u8 port, u16 offset, u32 val) > { > struct ice_sbq_msg_input msg = {0}; It's not a changed line, just a reminder that `= { }` is preferred over `= {0}`. [...] > - err = ice_read_quad_reg_e82x(hw, quad, lo_addr, &lo); > + low = (u32)(val & P_REG_40B_LOW_M); > + high = (u32)(val >> P_REG_40B_HIGH_S); low = FIELD_GET(P_REG_40B_LOW_M, val); high = FIELD_GET(P_REG_40B_HIGH_M, val); [...] > - /* Log the current clock configuration */ > - ice_debug(hw, ICE_DBG_PTP, "New CGU configuration -- %s, clk_src %s, > clk_freq %s, PLL %s\n", > - dw24.field.ts_pll_enable ? "enabled" : "disabled", > - ice_clk_src_str(dw24.field.time_ref_sel), > - ice_clk_freq_str(dw9.field.time_ref_freq_sel), > - bwm_lf.field.plllock_true_lock_cri ? "locked" : "unlocked"); > + /* For E822 based internal PHYs, the timestamp is reported with the > + * lower 8 bits in the low register, and the upper 32 bits in the high > + * register. > + */ > + *tstamp = ((u64)hi) << TS_PHY_HIGH_S | ((u64)lo & TS_PHY_LOW_M); *tstamp = FIELD_PREP(TS_PHY_HIGH_M, hi) | FIELD_PREP(TS_PHY_LOW_M, lo); [...] > + * Clear all timestamps from the PHY quad block that is shared between the > + * internal PHYs on the E822 devices. > + */ > +void ice_ptp_reset_ts_memory_quad_e82x(struct ice_hw *hw, u8 quad) > +{ > + ice_write_quad_reg_e82x(hw, quad, Q_REG_TS_CTRL, Q_REG_TS_CTRL_M); > + ice_write_quad_reg_e82x(hw, quad, Q_REG_TS_CTRL, ~(u32)Q_REG_TS_CTRL_M); Is it intended to write the same register twice? Some reset sequence? This (u32) cast before `~` is not needed BTW. [...] Thanks, Olek
