Re: [PATCH v5 2/4] soc: qcom: ice: Add OPP-based clock scaling support for ICE

[Konrad Dybcio]

On 2/11/26 10:47 AM, Abhinaba Rakshit wrote:
> Register optional operation-points-v2 table for ICE device
> and aquire its minimum and maximum frequency during ICE
> device probe.
> 
> Introduce clock scaling API qcom_ice_scale_clk which scale ICE
> core clock based on the target frequency provided and if a valid
> OPP-table is registered. Use flags (if provided) to decide on
> the rounding of the clock freq against OPP-table. Incase no flags
> are provided use default behaviour (CEIL incase of scale_up and FLOOR
> incase of ~scale_up). Disable clock scaling if OPP-table is not
> registered.
> 
> When an ICE-device specific OPP table is available, use the PM OPP
> framework to manage frequency scaling and maintain proper power-domain
> constraints.
> 
> Also, ensure to drop the votes in suspend to prevent power/thermal
> retention. Subsequently restore the frequency in resume from
> core_clk_freq which stores the last ICE core clock operating frequency.
> 
> Signed-off-by: Abhinaba Rakshit <abhinaba.rakshit@xxxxxxxxxxxxxxxx>
> ---

[...]

> +/**
> + * qcom_ice_scale_clk() - Scale ICE clock for DVFS-aware operations
> + * @ice: ICE driver data
> + * @target_freq: requested frequency in Hz
> + * @scale_up: If @flags is 0, choose ceil (true) or floor (false)
> + * @flags: Rounding policy (ICE_CLOCK_ROUND_*); overrides @scale_up
> + *
> + * Clamps @target_freq to the OPP range (min/max), selects an OPP per rounding
> + * policy, then applies it via dev_pm_opp_set_rate() (including voltage/PD
> + * changes).
> + *
> + * Return: 0 on success; -EOPNOTSUPP if no OPP table; or error from
> + *         dev_pm_opp_set_rate()/OPP lookup.
> + */
> +int qcom_ice_scale_clk(struct qcom_ice *ice, unsigned long target_freq,
> +		       bool scale_up, unsigned int flags)
> +{
> +	int ret;
> +	unsigned long ice_freq = target_freq;
> +	struct dev_pm_opp *opp;

Reverse-Christmas-tree ordering would be neat

> +
> +	if (!ice->has_opp)
> +		return -EOPNOTSUPP;
> +
> +	/* Clamp the freq to max if target_freq is beyond supported frequencies */
> +	if (ice->max_freq && target_freq >= ice->max_freq) {
> +		ice_freq = ice->max_freq;
> +		goto scale_clock;
> +	}
> +
> +	/* Clamp the freq to min if target_freq is below supported frequencies */
> +	if (ice->min_freq && target_freq <= ice->min_freq) {
> +		ice_freq = ice->min_freq;
> +		goto scale_clock;
> +	}

The OPP framework won't let you overclock the ICE if this is what these checks
are about. Plus the clk framework will perform rounding for you too

> +
> +	switch (flags) {

Are you going to use these flags? Currently they're dead code

> +	case ICE_CLOCK_ROUND_CEIL:
> +		opp = dev_pm_opp_find_freq_ceil_indexed(ice->dev, &ice_freq, 0);

You never use the index (hardcoded to 0)

> +		break;
> +	case ICE_CLOCK_ROUND_FLOOR:
> +		opp = dev_pm_opp_find_freq_floor_indexed(ice->dev, &ice_freq, 0);
> +		break;
> +	default:
> +		if (scale_up)
> +			opp = dev_pm_opp_find_freq_ceil_indexed(ice->dev, &ice_freq, 0);
> +		else
> +			opp = dev_pm_opp_find_freq_floor_indexed(ice->dev, &ice_freq, 0);

Is this distinction necessary?

Konrad