netdev - RE: [Intel-wired-lan] [PATCH v8 iwl-next] ice: add recovery clock and clock 1588 control for E825c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <IA1PR11MB6219A68EFD8E72E5298842A9922CA@IA1PR11MB6219.namprd11.prod.outlook.com>
Date: Thu, 7 Aug 2025 08:35:11 +0000
From: "Nitka, Grzegorz" <grzegorz.nitka@...el.com>
To: "Nitka, Grzegorz" <grzegorz.nitka@...el.com>, Paul Menzel
	<pmenzel@...gen.mpg.de>, "Korba, Przemyslaw" <przemyslaw.korba@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Kubalewski, Arkadiusz"
	<arkadiusz.kubalewski@...el.com>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "Olech, Milena" <milena.olech@...el.com>
Subject: RE: [Intel-wired-lan] [PATCH v8 iwl-next] ice: add recovery clock and
 clock 1588 control for E825c

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf Of
> Nitka, Grzegorz
> Sent: Thursday, July 31, 2025 5:36 PM
> To: Paul Menzel <pmenzel@...gen.mpg.de>; Korba, Przemyslaw
> <przemyslaw.korba@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Kubalewski,
> Arkadiusz <arkadiusz.kubalewski@...el.com>; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@...el.com>; Olech, Milena <milena.olech@...el.com>
> Subject: Re: [Intel-wired-lan] [PATCH v8 iwl-next] ice: add recovery clock and
> clock 1588 control for E825c
> 
> Dear Paul,
> 
> Thanks for your feedback.
> My responses in-line. I'm going to address your comments in v9.
> 
> > -----Original Message-----
> > From: Paul Menzel <pmenzel@...gen.mpg.de>
> > Sent: Thursday, July 24, 2025 5:00 PM
> > To: Nitka, Grzegorz <grzegorz.nitka@...el.com>; Korba, Przemyslaw
> > <przemyslaw.korba@...el.com>
> > Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Kubalewski,
> > Arkadiusz <arkadiusz.kubalewski@...el.com>; Nguyen, Anthony L
> > <anthony.l.nguyen@...el.com>; Kitszel, Przemyslaw
> > <przemyslaw.kitszel@...el.com>; Olech, Milena <milena.olech@...el.com>
> > Subject: Re: [Intel-wired-lan] [PATCH v8 iwl-next] ice: add recovery clock
> and
> > clock 1588 control for E825c
> >
> > Dear Grzegorz,
> >
> >
> > Thank you for your patch. I have some more minor comments.
> >
> > Am 24.07.25 um 14:27 schrieb Grzegorz Nitka:
> > > From: Przemyslaw Korba <przemyslaw.korba@...el.com>
> > >
> > > Add control for E825 input pins: phy clock recovery and clock 1588.
> > > E825 does not provide control over platform level DPLL but it
> > > provides control over PHY clock recovery, and PTP/timestamp driven
> > > inputs for platform level DPLL.
> > >
> > > Introduce a software controlled layer of abstraction to:
> > > - create a DPLL of type EEC for E825c,
> > > - create recovered clock pin for each PF, and control them through
> > > writing to registers,
> > > - create pin to control clock 1588 for PF0, and control it through
> > > writing to registers.
> >
> > It’d be great if you mentioned the datasheet name, revision, and section.
> >
> 
> Sure, I'm working on internal documentation, but will try to figure out what's
> publicly available.
> 
> > > Usage example:
> > > - to get EEC PLL info
> 
[...]
> > > +{
> > > +	switch (link_speed) {
> > > +	case ICE_AQ_LINK_SPEED_100GB:
> > > +	case ICE_AQ_LINK_SPEED_50GB:
> > > +	case ICE_AQ_LINK_SPEED_25GB:
> > > +		*divider = 10;
> > > +		break;
> > > +	case ICE_AQ_LINK_SPEED_40GB:
> > > +	case ICE_AQ_LINK_SPEED_10GB:
> > > +		*divider = 4;
> > > +		break;
> > > +	case ICE_AQ_LINK_SPEED_5GB:
> > > +	case ICE_AQ_LINK_SPEED_2500MB:
> > > +	case ICE_AQ_LINK_SPEED_1000MB:
> > > +		*divider = 2;
> > > +		break;
> > > +	case ICE_AQ_LINK_SPEED_100MB:
> > > +		*divider = 1;
> > > +		break;
> > > +	default:
> > > +		return -EOPNOTSUPP;
> > > +	}
> > > +
> > > +	return 0;
> >
> > Is there a reason to not create an map/lookup array?
> >
> 
> I'm not aware of any. Will try to change the code as you suggested.
> 

I think the rationale for that was the fact link_speed is just a bitmap.
I'd leave it as is.

> > > +}
> > > +
> > > +/**
> > > + * ice_dpll_cfg_synce_ethdiv_e825c - set the divider on the mux
> > > + * @hw: Pointer to the HW struct
> > > + * @output: Output pin, we have two in E825C
> > > + *
> > > + * Dpll subsystem callback. Set the correct divider for RCLKA or RCLKB.
> > > + *
> > > + * Context: Called under pf->dplls.lock
> > > + * Return:
> > > + * * 0 - success
> > > + * * negative - error
> > > + */
> > > +static int ice_dpll_cfg_synce_ethdiv_e825c(struct ice_hw *hw,
> > > +					   enum ice_synce_clk output)
> > > +{
> > > +	u16 link_speed;
> > > +	u8 divider;
> > > +	u32 val;
> > > +	int err;
> > > +
> > > +	link_speed = hw->port_info->phy.link_info.link_speed;
> > > +	if (!link_speed)
> > > +		return 0;
> >
> > Isn’t this an error? Should a warning be logged?
> >
> 
> Good point. In brief, !link_speed means there is no link.
> As an alternative, this checker might be skipped and let it fail in
> ice_dpll_get_div_e825c() call.
> 

I verified this. No, it's intentional to not report an error here.
User is not blocked to configure synce even if link is down.
What's missing here is to re-appling divider settings once link is up.
I'll add this part in v9 once the window is open again.

> > > +
> > > +	err = ice_dpll_get_div_e825c(link_speed, &divider);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	err = ice_read_cgu_reg(hw, ICE_CGU_R10, &val);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	/* programmable divider value (from 2 to 16) minus 1 for ETHCLKOUT
> > */
> > > +	switch (output) {
> > > +	case ICE_SYNCE_CLK0:
> > > +		val &= ~(ICE_CGU_R10_SYNCE_ETHDIV_M1 |
> > > +			 ICE_CGU_R10_SYNCE_ETHDIV_LOAD);
> > > +		val |= FIELD_PREP(ICE_CGU_R10_SYNCE_ETHDIV_M1,
> > divider - 1);
> > > +		err = ice_write_cgu_reg(hw, ICE_CGU_R10, val);
> > > +		if (err)
> > > +			return err;
> > > +		val |= ICE_CGU_R10_SYNCE_ETHDIV_LOAD;
> > > +		break;
> > > +	case ICE_SYNCE_CLK1:
> > > +		val &= ~(ICE_CGU_R10_SYNCE_CLKODIV_M1 |
> > > +			 ICE_CGU_R10_SYNCE_CLKODIV_LOAD);
> > > +		val |= FIELD_PREP(ICE_CGU_R10_SYNCE_CLKODIV_M1,
> > divider - 1);
> > > +		err = ice_write_cgu_reg(hw, ICE_CGU_R10, val);
> > > +		if (err)
> > > +			return err;
> > > +		val |= ICE_CGU_R10_SYNCE_CLKODIV_LOAD;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	err = ice_write_cgu_reg(hw, ICE_CGU_R10, val);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	return 0;
> > > +}
> > > +

Regards

Grzegorz