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/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@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.

I hope the above has addressed your concern.

Thanks,
Praveen
> 
> 
> Thanks,
> Praveen Talari
>>
>> Konrad
>