Re: [PATCH v3 06/12] soc: qcom: geni-se: Introduce helper APIs for performance control

[Praveen Talari <praveen.talari@oss.qualcomm.com>]

Hi Konrad,

On 2/17/2026 5:55 PM, Konrad Dybcio wrote:
> On 2/4/26 6:42 AM, Praveen Talari wrote:
>> Hi Konrad,
>>
>> On 2/3/2026 4:44 PM, Konrad Dybcio wrote:
>>> On 1/30/26 5:54 PM, Praveen Talari wrote:
>>>> Hi Konrad
>>>>
>>>> On 1/30/2026 5:53 PM, Konrad Dybcio wrote:
>>>>> On 1/12/26 11:47 AM, Praveen Talari wrote:
>>>>>> The GENI Serial Engine (SE) drivers (I2C, SPI, and SERIAL) currently
>>>>>> manage performance levels and operating points directly. This resulting
>>>>>> in code duplication across drivers. such as configuring a specific level
>>>>>> or find and apply an OPP based on a clock frequency.
>>>>>>
>>>>>> Introduce two new helper APIs, geni_se_set_perf_level() and
>>>>>> geni_se_set_perf_opp(), addresses this issue by providing a streamlined
>>>>>> method for the GENI Serial Engine (SE) drivers to find and set the OPP
>>>>>> based on the desired performance level, thereby eliminating redundancy.
>>>>>>
>>>>>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>>>>>> ---
>>>>>
>>>>> [...]
>>>>>
>>>>>> +/**
>>>>>> + * geni_se_set_perf_level() - Set performance level for GENI SE.
>>>>>> + * @se: Pointer to the struct geni_se instance.
>>>>>> + * @level: The desired performance level.
>>>>>> + *
>>>>>> + * Sets the performance level by directly calling dev_pm_opp_set_level
>>>>>> + * on the performance device associated with the SE.
>>>>>> + *
>>>>>> + * Return: 0 on success, or a negative error code on failure.
>>>>>> + */
>>>>>> +int geni_se_set_perf_level(struct geni_se *se, unsigned long level)
>>>>>> +{
>>>>>> +    return dev_pm_opp_set_level(se->pd_list->pd_devs[DOMAIN_IDX_PERF], level);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(geni_se_set_perf_level);
>>>>>
>>>>> This function is never used
>>>>
>>>> it will be used by UART driver, not for I2C/SPI.
>>>
>>> Adding unused exported symbols is "eeeh"..
>>
>> I keep in mind for UART, i have added this API.
>>>
>>>>>
>>>>>> +
>>>>>> +/**
>>>>>> + * geni_se_set_perf_opp() - Set performance OPP for GENI SE by frequency.
>>>>>> + * @se: Pointer to the struct geni_se instance.
>>>>>> + * @clk_freq: The requested clock frequency.
>>>>>> + *
>>>>>> + * Finds the nearest operating performance point (OPP) for the given
>>>>>> + * clock frequency and applies it to the SE's performance device.
>>>>>> + *
>>>>>> + * Return: 0 on success, or a negative error code on failure.
>>>>>> + */
>>>>>> +int geni_se_set_perf_opp(struct geni_se *se, unsigned long clk_freq)
>>>>>
>>>>> I think with the SPI driver in mind (which seems to do a simple rateset
>>>>
>>>> APIs were added as generic interfaces shared across I²C/SPI which is specific to firmware control, not Linux control.
>>>>
>>>>> for both backends) we could do:
>>>>>
>>>>>> +{
>>>>>> +    struct device *perf_dev = se->pd_list->pd_devs[DOMAIN_IDX_PERF];
>>>>>
>>>>> Then, we can do struct device * perf_dev = se->dev;
>>>> I don't think, it is needed since this is specific to firmware control, not Linux control.
>>>
>>> My point is that it doesn't have to be specific to the auto usecase,
>>> further commonizing the code.
>>
>> This API will not useful for non-auto cases as well.
> 
> This is only because you make it so, with the above suggestion we could
> do without the .set_rate abstraction in the SPI driver which only does
> an opp_set_rate in the generic case

For the generic .set_rate path (which is managed by Linux), we do more 
than just call dev_pm_opp_set_rate(). The .set_rate callback also 
performs additional hardware‑specific configuration as part of the rate 
change.

SPI enablement on SA8255P follows this generic (non‑SCMI) path:
https://lore.kernel.org/all/20260112190134.1526646-5-praveen.talari@oss.qualcomm.com/

Below is the reference implementation of .set_rate used for the generic 
(non‑SCMI) case:

+static const struct geni_spi_desc geni_spi = {
+	.resources_init = geni_se_resources_init,
+	.set_rate = geni_spi_set_clock_and_bw,
+	.power_on = geni_se_resources_activate,
+	.power_off = geni_se_resources_deactivate,
+};


static int geni_spi_set_clock_and_bw(struct spi_geni_master *mas,
					unsigned long clk_hz)
{
	u32 clk_sel, m_clk_cfg, idx, div;
	struct geni_se *se = &mas->se;
	int ret;

	if (clk_hz == mas->cur_speed_hz)
		return 0;

	ret = get_spi_clk_cfg(clk_hz, mas, &idx, &div);
	if (ret) {
		dev_err(mas->dev, "Err setting clk to %lu: %d\n", clk_hz, ret);
		return ret;
	}

	/*
	 * SPI core clock gets configured with the requested frequency
	 * or the frequency closer to the requested frequency.
	 * For that reason requested frequency is stored in the
	 * cur_speed_hz and referred in the consecutive transfer instead
	 * of calling clk_get_rate() API.
	 */
	mas->cur_speed_hz = clk_hz;

	clk_sel = idx & CLK_SEL_MSK;
	m_clk_cfg = (div << CLK_DIV_SHFT) | SER_CLK_EN;
	writel(clk_sel, se->base + SE_GENI_CLK_SEL);
	writel(m_clk_cfg, se->base + GENI_SER_M_CLK_CFG);

	/* Set BW quota for CPU as driver supports FIFO mode only. */
	se->icc_paths[CPU_TO_GENI].avg_bw = Bps_to_icc(mas->cur_speed_hz);
	ret = geni_icc_set_bw(se);
	if (ret)
		return ret;

	return 0;
}

In geni_spi_set_clock_and_bw(), the driver not only programs the SPI 
clock, but also updates internal state and configures ICC bandwidth.

In particular, dev_pm_opp_set_rate() and dev_pm_opp_set_opp() serve 
different purposes and are not interchangeable.
dev_pm_opp_set_rate() selects an OPP based on the requested frequency, 
performs clock rounding, and programs both clocks and power supplies 
accordingly.

On the other hand, dev_pm_opp_set_opp() assumes that the target OPP is 
already known and applies it directly, without any frequency‑based 
selection or rounding.

Given these differences, replacing dev_pm_opp_set_rate() with 
geni_se_set_perf_opp() would not be equivalent in the generic .set_rate 
flow.



/**
  * dev_pm_opp_set_rate() - Configure new OPP based on frequency
  * @dev:	 device for which we do this operation
  * @target_freq: frequency to achieve
  *
  * This configures the power-supplies to the levels specified by the OPP
  * corresponding to the target_freq, and programs the clock to a value <=
  * target_freq, as rounded by clk_round_rate(). Device wanting to run 
at fmax
  * provided by the opp, should have already rounded to the target OPP's
  * frequency.
  */
int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
{
	struct opp_table *opp_table __free(put_opp_table);
	struct dev_pm_opp *opp __free(put_opp) = NULL;
	unsigned long freq = 0, temp_freq;
	bool forced = false;

	opp_table = _find_opp_table(dev);
	if (IS_ERR(opp_table)) {
		dev_err(dev, "%s: device's opp table doesn't exist\n", __func__);
		return PTR_ERR(opp_table);
	}

	if (target_freq) {
		/*
		 * For IO devices which require an OPP on some platforms/SoCs
		 * while just needing to scale the clock on some others
		 * we look for empty OPP tables with just a clock handle and
		 * scale only the clk. This makes dev_pm_opp_set_rate()
		 * equivalent to a clk_set_rate()
		 */
		if (!_get_opp_count(opp_table)) {
			return opp_table->config_clks(dev, opp_table, NULL,
						      &target_freq, false);
		}

		freq = clk_round_rate(opp_table->clk, target_freq);
		if ((long)freq <= 0)
			freq = target_freq;

		/*
		 * The clock driver may support finer resolution of the
		 * frequencies than the OPP table, don't update the frequency we
		 * pass to clk_set_rate() here.
		 */
		temp_freq = freq;
		opp = _find_freq_ceil(opp_table, &temp_freq);
		if (IS_ERR(opp)) {
			dev_err(dev, "%s: failed to find OPP for freq %lu (%ld)\n",
				__func__, freq, PTR_ERR(opp));
			return PTR_ERR(opp);
		}

		/*
		 * An OPP entry specifies the highest frequency at which other
		 * properties of the OPP entry apply. Even if the new OPP is
		 * same as the old one, we may still reach here for a different
		 * value of the frequency. In such a case, do not abort but
		 * configure the hardware to the desired frequency forcefully.
		 */
		forced = opp_table->current_rate_single_clk != freq;
	}

	return _set_opp(dev, opp_table, opp, &freq, forced);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);

/**
  * dev_pm_opp_set_opp() - Configure device for OPP
  * @dev: device for which we do this operation
  * @opp: OPP to set to
  *
  * This configures the device based on the properties of the OPP passed 
to this
  * routine.
  *
  * Return: 0 on success, a negative error number otherwise.
  */
int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
{
	struct opp_table *opp_table __free(put_opp_table);

	opp_table = _find_opp_table(dev);
	if (IS_ERR(opp_table)) {
		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
		return PTR_ERR(opp_table);
	}

	return _set_opp(dev, opp_table, opp, NULL, false);
}

Please correct me if my understanding is incorrect.


Thanks,
Praveen Talari
> 
> Konrad