> -----Original Message-----
> From: Simon Horman <ho...@kernel.org>
> Sent: Tuesday, February 25, 2025 4:05 PM
> To: Nitka, Grzegorz <grzegorz.ni...@intel.com>
> Cc: intel-wired-...@lists.osuosl.org; net...@vger.kernel.org; Kolacinski,
> Karol <karol.kolacin...@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kits...@intel.com>
> Subject: Re: [PATCH iwl-next v1 3/3] ice: enable timesync operation on
> 2xNAC E825 devices
>
> On Fri, Feb 21, 2025 at 01:31:23PM +0100, Grzegorz Nitka wrote:
> > From: Karol Kolacinski <karol.kolacin...@intel.com>
> >
> > According to the E825C specification, SBQ address for ports on a single
> > complex is device 2 for PHY 0 and device 13 for PHY1.
> > For accessing ports on a dual complex E825C (so called 2xNAC mode),
> > the driver should use destination device 2 (referred as phy_0) for
> > the current complex PHY and device 13 (referred as phy_0_peer) for
> > peer complex PHY.
> >
> > Differentiate SBQ destination device by checking if current PF port
> > number is on the same PHY as target port number.
> >
> > Adjust 'ice_get_lane_number' function to provide unique port number for
> > ports from PHY1 in 'dual' mode config (by adding fixed offset for PHY1
> > ports). Cache this value in ice_hw struct.
> >
> > Introduce ice_get_primary_hw wrapper to get access to timesync register
> > not available from second NAC.
> >
> > Reviewed-by: Przemek Kitszel <przemyslaw.kits...@intel.com>
> > Signed-off-by: Karol Kolacinski <karol.kolacin...@intel.com>
> > Co-developed-by: Grzegorz Nitka <grzegorz.ni...@intel.com>
> > Signed-off-by: Grzegorz Nitka <grzegorz.ni...@intel.com>
> > ---
> > drivers/net/ethernet/intel/ice/ice.h | 60 ++++++++++++++++++++-
> > drivers/net/ethernet/intel/ice/ice_common.c | 6 ++-
> > drivers/net/ethernet/intel/ice/ice_ptp.c | 49 ++++++++++++-----
> > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 39 +++++++++++---
> > drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 5 --
> > drivers/net/ethernet/intel/ice/ice_type.h | 1 +
> > 6 files changed, 134 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice.h
> b/drivers/net/ethernet/intel/ice/ice.h
> > index 53b990e2e850..d80ab48402f1 100644
> > --- a/drivers/net/ethernet/intel/ice/ice.h
> > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > @@ -193,8 +193,6 @@
> >
> > #define ice_pf_to_dev(pf) (&((pf)->pdev->dev))
> >
> > -#define ice_pf_src_tmr_owned(pf) ((pf)-
> >hw.func_caps.ts_func_info.src_tmr_owned)
> > -
> > enum ice_feature {
> > ICE_F_DSCP,
> > ICE_F_PHY_RCLK,
> > @@ -1049,4 +1047,62 @@ static inline void ice_clear_rdma_cap(struct
> ice_pf *pf)
> > }
> >
> > extern const struct xdp_metadata_ops ice_xdp_md_ops;
> > +
> > +/**
> > + * ice_is_dual - Check if given config is multi-NAC
> > + * @hw: pointer to HW structure
> > + *
> > + * Return: true if the device is running in mutli-NAC (Network
> > + * Acceleration Complex) configuration variant, false otherwise
> > + * (always false for non-E825 devices).
> > + */
> > +static inline bool ice_is_dual(struct ice_hw *hw)
> > +{
> > + return hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
> > + (hw->dev_caps.nac_topo.mode & ICE_NAC_TOPO_DUAL_M);
> > +}
>
> Thanks for the complete Kernel doc, and not adding unnecessary () around
> the value returned. Overall these helpers seem nice and clean to me.
>
> ...
>
> > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
> b/drivers/net/ethernet/intel/ice/ice_common.c
> > index ed7ef8608cbb..4ff4c99d0872 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_common.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> > @@ -1135,6 +1135,8 @@ int ice_init_hw(struct ice_hw *hw)
> > }
> > }
> >
> > + hw->lane_num = ice_get_phy_lane_number(hw);
> > +
>
> Unfortunately there seems to be a bit of a problem here:
>
> The type of hw->lane number is u8.
> But ice_get_phy_lane_number returns an int,
> which may be a negative error value...
>
> ...
>
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
>
> ...
Hello Simon,
Thanks for your review and apologize for a late response (I was OOO last
week).
Yes, this is actually a good catch!
I have no idea how it happened, most likely my typo when backporting
the patch from our reference code. hw->lane_num is declared as 's8' there.
To be fixed in v2 (along with doc update from your other comment).
>
> > @@ -3177,17 +3203,16 @@ void ice_ptp_init(struct ice_pf *pf)
> > {
> > struct ice_ptp *ptp = &pf->ptp;
> > struct ice_hw *hw = &pf->hw;
> > - int lane_num, err;
> > + int err;
> >
> > ptp->state = ICE_PTP_INITIALIZING;
> >
> > - lane_num = ice_get_phy_lane_number(hw);
> > - if (lane_num < 0) {
> > - err = lane_num;
> > + if (hw->lane_num < 0) {
>
> ... which is checked here.
>
> But because hw->lane_num is unsigned, this condition is always false.
>
> FWIIW, I think it is nice that that hw->lane is a u8.
> But error handling seems broken here.
>
> > + err = hw->lane_num;
> > goto err_exit;
> > }
> > + ptp->port.port_num = hw->lane_num;
> >
> > - ptp->port.port_num = (u8)lane_num;
> > ice_ptp_init_hw(hw);
> >
> > ice_ptp_init_tx_interrupt_mode(pf);
>
> ...
