Re: [PATCH iwl-next v1] ice: add support for unmanaged dpll on E830 NIC

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 8/26/25 17:31, Arkadiusz Kubalewski wrote:
> Hardware variants of E830 may support an unmanaged DPLL where the
> configuration is hardcoded within the hardware and firmware, meaning
> users cannot modify settings. However, users are able to check the DPLL
> lock status and obtain configuration information through the Linux DPLL
> subsystem.
> 
> Availability of 'loss of lock' health status code determines if such
> support is available, if true, register single dpll device with 1 input
> and 1 output and provide hardcoded/read only properties of a pin and
> dpll device. User is only allowed to check dpll device status and receive
> notifications on dpll lock status change.
> 
> When present, the DPLL device locks to an external signal provided
> through the PCIe/OCP pin. The expected input signal is 1PPS
> (1 Pulse Per Second) embedded on a 10MHz reference clock.
> The DPLL produces output:
> - for MAC (Media Access Control) & PHY (Physical Layer) clocks,
> - 1PPS for synchronization of onboard PHC (Precision Hardware Clock) timer.
> 
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> ---
>   .../device_drivers/ethernet/intel/ice.rst     |  83 +++++
>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   4 +
>   drivers/net/ethernet/intel/ice/ice_common.c   | 110 +++++++
>   drivers/net/ethernet/intel/ice/ice_common.h   |   7 +
>   drivers/net/ethernet/intel/ice/ice_dpll.c     | 306 ++++++++++++++++--
>   drivers/net/ethernet/intel/ice/ice_dpll.h     |  11 +
>   drivers/net/ethernet/intel/ice/ice_main.c     |  14 +-
>   drivers/net/ethernet/intel/ice/ice_ptp_hw.c   |  46 +++
>   drivers/net/ethernet/intel/ice/ice_ptp_hw.h   |   1 +
>   9 files changed, 560 insertions(+), 22 deletions(-)
> 


> +int ice_is_health_status_code_supported(struct ice_hw *hw, u16 code,
> +					bool *supported)
> +{
> +	struct ice_aqc_health_status_elem *buff __free(kfree) = NULL;

observation: netdev maintainers don't like __free

> +	const int BUFF_SIZE = ICE_AQC_HEALTH_STATUS_CODE_NUM;
> +	int ret;
> +
> +	*supported = false;

it's best to don't change output pointers on error

> +
> +	buff = kcalloc(BUFF_SIZE, sizeof(*buff), GFP_KERNEL);
> +	if (!buff)
> +		return -ENOMEM;
> +	ret = ice_aq_get_health_status_supported(hw, buff, BUFF_SIZE);
> +	if (ret)
> +		return ret;
> +	for (int i = 0; i < BUFF_SIZE && buff[i].health_status_code; i++)
> +		if (le16_to_cpu(buff[i].health_status_code) == code) {
> +			*supported = true;

so, this line should be enough

> +			break;
> +		}
> +
> +	return 0;
> +}
> +
> +/**
> + * ice_get_last_health_status_code - get last health status for given code
> + * @hw: pointer to the hardware structure
> + * @out: pointer to the health status struct to be filled
> + * @code: health status code to check
> + *
> + * Return: 0 on success, negative error code otherwise
> + */
> +int ice_get_last_health_status_code(struct ice_hw *hw,
> +				    struct ice_aqc_health_status_elem *out,
> +				    u16 code)
> +{
> +	struct ice_aqc_health_status_elem *buff __free(kfree) = NULL;
> +	const int BUFF_SIZE = ICE_AQC_HEALTH_STATUS_CODE_NUM;
> +	int last_status = -1, ret;

nit: variables with initial value set should be on the end of given
declaration line (here @ret should be first)

> +
> +	buff = kcalloc(BUFF_SIZE, sizeof(*buff), GFP_KERNEL);
> +	if (!buff)
> +		return -ENOMEM;
> +	ret = ice_aq_get_health_status(hw, buff, BUFF_SIZE);
> +	if (ret)
> +		return ret;
> +	for (int i = 0; i < BUFF_SIZE && buff[i].health_status_code; i++)
> +		if (le16_to_cpu(buff[i].health_status_code) == code)
> +			last_status = i;
> +
> +	if (last_status >= 0 && last_status < BUFF_SIZE)

second part of the contidion is always true

> +		memcpy(out, &buff[last_status], sizeof(*out));
> +	else
> +		memset(out, 0, sizeof(*out));

weird, but let it be

> +
> +	return ret;
> +}