Message-ID: <IA1PR11MB6219197D989E2DD57307AE8D92C82@IA1PR11MB6219.namprd11.prod.outlook.com>
Date: Tue, 4 Mar 2025 13:03:43 +0000
From: "Nitka, Grzegorz" <grzegorz.nitka@...el.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>, "Kolacinski, Karol"
	<karol.kolacinski@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, Michal Swiatkowski
	<michal.swiatkowski@...ux.intel.com>
Subject: RE: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side
 band access workaround for E825

> -----Original Message-----
> From: Paul Menzel <pmenzel@...gen.mpg.de>
> Sent: Friday, February 21, 2025 10:16 PM
> To: Nitka, Grzegorz <grzegorz.nitka@...el.com>; Kolacinski, Karol
> <karol.kolacinski@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Kitszel,
> Przemyslaw <przemyslaw.kitszel@...el.com>; Michal Swiatkowski
> <michal.swiatkowski@...ux.intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side
> band access workaround for E825
> 
> Dear Grzegorz, dear Karol,
> 
> 
> Thank you for your patch.
> 
> Am 21.02.25 um 13:31 schrieb Grzegorz Nitka:
> > From: Karol Kolacinski <karol.kolacinski@...el.com>
> >
> > Due to the bug in FW/NVM autoload mechanism (wrong default
> > SB_REM_DEV_CTL register settings), the access to peer PHY and CGU
> > clients was disabled by default.
> 
> I’d add a blank line between the paragraphs.
> 
> > As the workaround solution, the register value was overwritten by the
> > driver at the probe or reset handling.
> > Remove workaround as it's not needed anymore. The fix in autoload
> > procedure has been provided with NVM 3.80 version.
> 
> Is this compatible with Linux’ no regression policy? People might only
> update the Linux kernel and not the firmware.
> 
> How did you test this, and how can others test this?
> 
> > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> > Signed-off-by: Karol Kolacinski <karol.kolacinski@...el.com>
> > Signed-off-by: Grzegorz Nitka <grzegorz.nitka@...el.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 23 ---------------------
> >   1 file changed, 23 deletions(-)
> 
> 
> Kind regards,
> 
> Paul
> 
> 

Dear Paul, first of all thank you for your review and apologize for late
response (simply, I was out previous week).

Removing that workaround was a conscious decision.
Rationale for that is, that the 'problematic' workaround was provided
in very early product development stage (~ year ago).  Now, especially
when E825-C product was just officially announced, the customer shall
use official, approved SW ingredients.

Here are more details on this:
- introduction to E825-C devices was provided in kernel 6.6, to allow
  selected customers for early E825-C enablement. At that time E825-C
  product family was in early phase (kind of Alpha), and one of the
  consequences, from today's perspective,  is that it included 'unwanted'
  workarounds like this.

- this change applies to E825-C products only, which is SoC product, not
  a regular NIC card.  What I'd like to emphasize here, it requires even more
  customer support from Intel company in terms of providing matching
  SW stack (like BIOS, firmware, drivers etc.).
  
I see two possibilities now:
1) if the regression policy you mentioned is inviolable, keep the workaround
   in the kernel code like it is today. Maybe we could add NVM version checker
   and apply register updates for older NVMs only.
   But, in my opinion, it would lead to keeping a dead code. There shouldn't be
   official FW (I hope I won't regret these words) on the market which requires
   this workaround.
  
2) remove the workaround like it's implemented in this patch and improve
   commit message to clarify all potential doubts/questions from the user
   perspective.

What's your thoughts?

Kind regards

Grzegorz

> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > index 89bb8461284a..a5df081ffc19 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > @@ -2630,25 +2630,6 @@ int ice_start_phy_timer_eth56g(struct ice_hw
> *hw, u8 port)
> >   	return 0;
> >   }
> >
> > -/**
> > - * ice_sb_access_ena_eth56g - Enable SB devices (PHY and others) access
> > - * @hw: pointer to HW struct
> > - * @enable: Enable or disable access
> > - *
> > - * Enable sideband devices (PHY and others) access.
> > - */
> > -static void ice_sb_access_ena_eth56g(struct ice_hw *hw, bool enable)
> > -{
> > -	u32 val = rd32(hw, PF_SB_REM_DEV_CTL);
> > -
> > -	if (enable)
> > -		val |= BIT(eth56g_phy_0) | BIT(cgu) | BIT(eth56g_phy_1);
> > -	else
> > -		val &= ~(BIT(eth56g_phy_0) | BIT(cgu) | BIT(eth56g_phy_1));
> > -
> > -	wr32(hw, PF_SB_REM_DEV_CTL, val);
> > -}
> > -
> >   /**
> >    * ice_ptp_init_phc_e825 - Perform E825 specific PHC initialization
> >    * @hw: pointer to HW struct
> > @@ -2659,8 +2640,6 @@ static void ice_sb_access_ena_eth56g(struct
> ice_hw *hw, bool enable)
> >    */
> >   static int ice_ptp_init_phc_e825(struct ice_hw *hw)
> >   {
> > -	ice_sb_access_ena_eth56g(hw, true);
> > -
> >   	/* Initialize the Clock Generation Unit */
> >   	return ice_init_cgu_e82x(hw);
> >   }
> > @@ -2747,8 +2726,6 @@ static void ice_ptp_init_phy_e825(struct ice_hw
> *hw)
> >   	params->num_phys = 2;
> >   	ptp->ports_per_phy = 4;
> >   	ptp->num_lports = params->num_phys * ptp->ports_per_phy;
> > -
> > -	ice_sb_access_ena_eth56g(hw, true);
> >   }
> >
> >   /* E822 family functions

Powered by blists - more mailing lists