Re: [PATCH v3 06/12] soc: qcom: geni-se: Introduce helper APIs for performance control
From: Praveen Talari
Date: Mon Feb 23 2026 - 08:44:17 EST
Hi Konrad,
On 2/20/2026 10:53 AM, Praveen Talari wrote:
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@xxxxxxxxxxxxxxxx>
---
[...]
+/**
+ * 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:I don't think, it is needed since this is specific to firmware control, not Linux control.
+{
+ struct device *perf_dev = se->pd_list- >pd_devs[DOMAIN_IDX_PERF];
Then, we can do struct device * perf_dev = se->dev;
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@xxxxxxxxxxxxxxxx/
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.
I hope the above has addressed your concern.
Thanks,
Praveen
Thanks,
Praveen Talari
Konrad