Re: [PATCH v3 4/6] media: qcom: camss: share VFE clock sources by the highest rate

[Loic Poulain]

On Mon, Oct 26, 2026 at 2:37 PM Atanas Filipov
<atanas.filipov@xxxxxxxxxxxxxxxx> wrote:
>
> Clocks of different VFEs can be fed by one clock source, and each VFE
> sets such a source to the rate it needs. A VFE started later at a lower
> rate then lowers the source under a VFE that is still running.
>
> For example, on SM8250 the two VFE Lite share one core clock source,
> which the VFE sets to 400 or 480 MHz from the pixel rate. With VFE Lite
> 0 streaming the CSID test pattern at 480 MHz, starting an imx577 stream
> on VFE Lite 1 sets the shared source to 400 MHz under the running VFE
> Lite 0.
>
> Request the VFE clock rates as minimums with clk_set_rate_range(), so
> that a shared clock runs at the highest minimum of its running VFEs.
> On power off the VFE drops its minimum and sets the clock to its lowest
> listed rate within the remaining minimums.
>
> Rate ranges are enabled through the vfe_clock_ranges resource flag, so
> far only on SM8250.

Why? I think it would make sense to use it unconditionally.

>
> Signed-off-by: Atanas Filipov <atanas.filipov@xxxxxxxxxxxxxxxx>
> ---
>  drivers/media/platform/qcom/camss/camss-vfe.c | 45 +++++++++++++++++--
>  drivers/media/platform/qcom/camss/camss.c     |  1 +
>  drivers/media/platform/qcom/camss/camss.h     |  1 +
>  3 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 359b5942df89..30504e614e4b 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -1016,7 +1016,10 @@ static int vfe_set_clock_rates(struct vfe_device *vfe)
>                                 return -EINVAL;
>                         }
>
> -                       ret = clk_set_rate(clock->clk, rate);
> +                       if (vfe->camss->res->vfe_clock_ranges)
> +                               ret = clk_set_rate_range(clock->clk, clock->freq[j], ULONG_MAX);
> +                       else
> +                               ret = clk_set_rate(clock->clk, rate);
>                         if (ret < 0) {
>                                 dev_err(dev, "clk set rate failed: %d\n", ret);
>                                 return ret;
> @@ -1060,6 +1063,37 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
>         return 0;
>  }
>
> +/*
> + * vfe_release_clock_rates - Release the minimum clock rates of VFE module
> + * @vfe: VFE device
> + *
> + * Drop the minimum rate requested by the VFE from each of its clocks and set
> + * the clock to its lowest level, or to the highest minimum rate still
> + * requested by the other users of the clock.
> + */
> +static void vfe_release_clock_rates(struct vfe_device *vfe)
> +{
> +       int i;
> +
> +       if (!vfe->camss->res->vfe_clock_ranges)
> +               return;
> +
> +       for (i = 0; i < vfe->nclocks; i++) {
> +               struct camss_clock *clock = &vfe->clock[i];
> +               long rate;
> +
> +               if (!vfe_match_clock_names(vfe, clock) || !vfe_check_clock_levels(clock))
> +                       continue;
> +
> +               clk_drop_range(clock->clk);
> +
> +               /* Round first, so that the rate respects the other users' ranges */
> +               rate = clk_round_rate(clock->clk, clock->freq[0]);
> +               if (rate > 0)
> +                       clk_set_rate(clock->clk, rate);
> +       }
> +}
> +
>  /*
>   * vfe_get - Power up and reset VFE module
>   * @vfe: VFE Device
> @@ -1083,12 +1117,12 @@ int vfe_get(struct vfe_device *vfe)
>
>                 ret = vfe_set_clock_rates(vfe);
>                 if (ret < 0)
> -                       goto error_pm_runtime_get;
> +                       goto error_release_rates;
>
>                 ret = camss_enable_clocks(vfe->nclocks, vfe->clock,
>                                           vfe->camss->dev);
>                 if (ret < 0)
> -                       goto error_pm_runtime_get;
> +                       goto error_release_rates;
>
>                 ret = vfe_reset(vfe);
>                 if (ret < 0)
> @@ -1113,7 +1147,9 @@ int vfe_get(struct vfe_device *vfe)
>  error_reset:
>         camss_disable_clocks(vfe->nclocks, vfe->clock);
>
> -error_pm_runtime_get:
> +error_release_rates:
> +       vfe_release_clock_rates(vfe);
> +
>         pm_runtime_put_sync(vfe->camss->dev);
>  error_domain_off:
>         vfe->res->hw_ops->pm_domain_off(vfe);
> @@ -1141,6 +1177,7 @@ void vfe_put(struct vfe_device *vfe)
>                         vfe->res->hw_ops->vfe_halt(vfe);
>                 }
>                 camss_disable_clocks(vfe->nclocks, vfe->clock);
> +               vfe_release_clock_rates(vfe);
>                 pm_runtime_put_sync(vfe->camss->dev);
>                 vfe->res->hw_ops->pm_domain_off(vfe);
>         }
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 7fe12049b78a..80eab898611d 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -5736,6 +5736,7 @@ static const struct camss_resources sm8250_resources = {
>         .csiphy_num = ARRAY_SIZE(csiphy_res_8250),
>         .csid_num = ARRAY_SIZE(csid_res_8250),
>         .vfe_num = ARRAY_SIZE(vfe_res_8250),
> +       .vfe_clock_ranges = true,
>  };
>
>  static const struct camss_resources sc8280xp_resources = {
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index 7dc2fd9f03e0..bffff5b6bfbe 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -120,6 +120,7 @@ struct camss_resources {
>         const unsigned int tpg_num;
>         const unsigned int csid_num;
>         const unsigned int vfe_num;
> +       const bool vfe_clock_ranges;
>  };
>
>  struct camss {
> --
> 2.34.1
>