Re: [PATCH v4 2/6] media: qcom: camss: clamp VFE clock rates to the highest level

[Loic Poulain]

On Wed, Oct 28, 2026 at 2:24 PM Atanas Filipov
<atanas.filipov@xxxxxxxxxxxxxxxx> wrote:
>
> The VFE fails to power on when the pixel rate exceeds the highest rate
> listed for any of its clocks. The rate tables of the VFE clocks are
> independent and need not have the same number of levels, so a clock
> with a lower top rate than the VFE core clock, such as the SC7280
> camnoc_axi clock, makes the VFE reject pixel rates the core clock
> still supports.
>
> Keep failing when the pixel rate exceeds the highest rate of the VFE
> core clock, and clamp the other VFE clocks to their highest listed
> rate instead. Take the clamp into account for the other clocks when
> another line powers on an already running VFE, so that a clock
> running at its highest rate does not reject the request.
>
> Signed-off-by: Atanas Filipov <atanas.filipov@xxxxxxxxxxxxxxxx>

Just a minor style nit, but the dev_err() messages could fit on a single line.
Anyway:

Reviewed-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>

> ---
> Changes in v4:
> - Keep -EINVAL for the VFE core clock, clamp only the other clocks,
>   drop the warning (Bryan O'Donoghue, Loic Poulain)
> - Dropped Reviewed-by due to changes
>
>  drivers/media/platform/qcom/camss/camss-vfe.c | 40 +++++++++++++++++--
>  1 file changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 59be37f83df3..bcc2c1929af1 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -918,6 +918,20 @@ static int vfe_match_clock_names(struct vfe_device *vfe,
>                 !strcmp(clock->name, "camnoc_rt_axi"));
>  }
>
> +static bool vfe_match_core_clock_name(struct vfe_device *vfe,
> +                                     struct camss_clock *clock)
> +{
> +       char vfe_name[7]; /* vfeXXX\0 */
> +       char vfe_lite_name[12]; /* vfe_liteXXX\0 */
> +
> +       snprintf(vfe_name, sizeof(vfe_name), "vfe%d", vfe->id);
> +       snprintf(vfe_lite_name, sizeof(vfe_lite_name), "vfe_lite%d", vfe->id);
> +
> +       return (!strcmp(clock->name, vfe_name) ||
> +               !strcmp(clock->name, vfe_lite_name) ||
> +               !strcmp(clock->name, "vfe_lite"));
> +}
> +
>  /*
>   * vfe_check_clock_levels - Calculate and set clock rates on VFE module
>   * @clock: clocks data
> @@ -1001,9 +1015,14 @@ static int vfe_set_clock_rates(struct vfe_device *vfe)
>                                         break;
>
>                         if (j == clock->nfreqs) {
> -                               dev_err(dev,
> -                                       "Pixel clock is too high for VFE");
> -                               return -EINVAL;
> +                               if (vfe_match_core_clock_name(vfe, clock)) {
> +                                       dev_err(dev,
> +                                               "Pixel clock is too high for VFE");
> +                                       return -EINVAL;
> +                               }
> +
> +                               /* Other clocks are clamped to their highest rate */
> +                               j = clock->nfreqs - 1;
>                         }
>
>                         /* if sensor pixel clock is not available */
> @@ -1045,10 +1064,23 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
>                 struct camss_clock *clock = &vfe->clock[i];
>
>                 if (vfe_match_clock_names(vfe, clock) && vfe_check_clock_levels(clock)) {
> +                       u64 req_rate = min_rate;
>                         unsigned long rate;
>
> +                       if (!vfe_match_core_clock_name(vfe, clock)) {
> +                               long max_rate;
> +
> +                               /* A clock already at its highest rate cannot go higher */
> +                               max_rate = clk_round_rate(clock->clk,
> +                                                         clock->freq[clock->nfreqs - 1]);
> +                               if (max_rate < 0)
> +                                       return max_rate;
> +
> +                               req_rate = min_t(u64, min_rate, max_rate);
> +                       }
> +
>                         rate = clk_get_rate(clock->clk);
> -                       if (rate < min_rate)
> +                       if (rate < req_rate)
>                                 return -EBUSY;
>                 }
>         }
> --
> 2.34.1
>