netdev - Re: [PATCH v4 iwl-next 07/12] ice: Introduce ETH56G PHY model for E825C products

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] [day] [month] [year] [list]

Date: Wed, 3 Apr 2024 16:33:45 -0700
From: Tony Nguyen <anthony.l.nguyen@...el.com>
To: Karol Kolacinski <karol.kolacinski@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, <jesse.brandeburg@...el.com>, Sergey Temerkhanov
	<sergey.temerkhanov@...el.com>, Jacob Keller <jacob.e.keller@...el.com>,
	Michal Michalik <michal.michalik@...el.com>, Przemek Kitszel
	<przemyslaw.kitszel@...el.com>, Arkadiusz Kubalewski
	<arkadiusz.kubalewski@...el.com>
Subject: Re: [PATCH v4 iwl-next 07/12] ice: Introduce ETH56G PHY model for
 E825C products



On 3/29/2024 9:09 AM, Karol Kolacinski wrote:
> From: Sergey Temerkhanov <sergey.temerkhanov@...el.com>
> 
> E825C products feature a new PHY model - ETH56G.
> 
> Introduces all necessary PHY definitions, functions etc. for ETH56G PHY,
> analogous to E82X and E810 ones with addition of a few HW-specific
> functionalities for ETH56G like one-step timestamping.
> 
> It ensures correct PTP initialization and operation for E825C products.
> 
> Co-developed-by: Jacob Keller <jacob.e.keller@...el.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> Co-developed-by: Michal Michalik <michal.michalik@...el.com>
> Signed-off-by: Michal Michalik <michal.michalik@...el.com>
> Signed-off-by: Sergey Temerkhanov <sergey.temerkhanov@...el.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
> Co-developed-by: Karol Kolacinski <karol.kolacinski@...el.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@...el.com>
> ---

...

> +/**
> + * mul_u32_u32_fx_q9 - Multiply two u32 fixed point Q9 values
> + * @a: multiplier value
> + * @b: multiplicand value
> + */
> +static inline u32 mul_u32_u32_fx_q9(u32 a, u32 b)
> +{
> +	return (u32)(((u64)a * b) >> ICE_ETH56G_MAC_CFG_FRAC_W);
> +}

Please don't use 'inline' in c files.

> +
> +/**
> + * add_u32_u32_fx - Add two u32 fixed point values and discard overflow
> + * @a: first value
> + * @b: second value
> + */
> +static inline u32 add_u32_u32_fx(u32 a, u32 b)
> +{
> +	return lower_32_bits(((u64)a + b));
> +}

Here too.

...

> +
> +/**
> + * ice_phy_set_offsets_eth56g - Set Tx/Rx offset values
> + * @hw: pointer to the HW struct
> + * @port: port to configure
> + * @spd: link speed
> + * @cfg: structure to store output values
> + * @fc: FC-FEC enabled
> + * @rs: RS-FEC enabled
> + */
> +static int ice_phy_set_offsets_eth56g(struct ice_hw *hw, u8 port,
> +				      enum ice_eth56g_link_spd spd,
> +				      const struct ice_eth56g_mac_reg_cfg *cfg,
> +				      bool fc, bool rs)
> +{
> +	u32 rx_offset, tx_offset, bs_ds;
> +	bool onestep, sfd;
> +
> +#ifdef SWITCH_MODE
> +	onestep = ice_is_bit_set(hw->ptp.phy.eth56g.onestep_ena, port);
> +	sfd = ice_is_bit_set(hw->ptp.phy.eth56g.sfd_ena, port);
> +#else /* SWITCH_MODE */
> +	onestep = hw->ptp.phy.eth56g.onestep_ena;
> +	sfd = hw->ptp.phy.eth56g.sfd_ena;
> +#endif /* SWITCH_MODE */

I don't believe this is supposed to be here?

> +	bs_ds = cfg->rx_offset.bs_ds;
> +
> +	if (fc)
> +		rx_offset = cfg->rx_offset.fc;
> +	else if (rs)
> +		rx_offset = cfg->rx_offset.rs;
> +	else
> +		rx_offset = cfg->rx_offset.no_fec;
> +

...

> +/**
> + * ice_phy_cfg_intr_eth56g - Configure TX timestamp interrupt
> + * @hw: pointer to the HW struct
> + * @port: the timestamp port
> + * @ena: enable or disable interrupt
> + * @threshold: interrupt threshold
> + *
> + * Configure TX timestamp interrupt for the specified port
> + */
> +int ice_phy_cfg_intr_eth56g(struct ice_hw *hw, u8 port, bool ena, u8 threshold)
> +{
> +	int err;
> +	u32 val;
> +
> +	err = ice_read_ptp_reg_eth56g(hw, port, PHY_REG_TS_INT_CONFIG, &val);
> +	if (err)
> +		return err;
> +
> +	if (ena) {
> +		val |= PHY_TS_INT_CONFIG_ENA_M;
> +		val &= ~PHY_TS_INT_CONFIG_THRESHOLD_M;
> +		val |= FIELD_PREP(PHY_TS_INT_CONFIG_THRESHOLD_M, threshold);
> +	} else {
> +		val &= ~PHY_TS_INT_CONFIG_ENA_M;
> +	}
> +
> +	err = ice_write_ptp_reg_eth56g(hw, port, PHY_REG_TS_INT_CONFIG, val);
> +	return err;

return ice_write_ptp_reg_eth56g()

> +}
> +

...

> +/**
> + * ice_ptp_init_phc_eth56g - Perform E82X specific PHC initialization
> + * @hw: pointer to HW struct
> + *
> + * Perform PHC initialization steps specific to E82X devices.
> + */
> +static int ice_ptp_init_phc_eth56g(struct ice_hw *hw)
> +{
> +	int err;
> +
> +	ice_sb_access_ena_eth56g(hw, true);
> +	/* Initialize the Clock Generation Unit */
> +	err = ice_init_cgu_e82x(hw);
> +
> +	return err;

return ice_init_cgu_e82c();

Also no need for local var after that.

Please check the the patches to ensure that they aren't assigning a 
local var to be returned immediately after (I saw more instances).

> +}