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

[Loic Poulain]

On Mon, Oct 26, 2026 at 2:37 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.
>
> Clamp each clock to its highest listed rate instead, and warn that the
> pixel rate exceeds it when the clock lists more than one rate. Take the
> clamp into account when another line powers on an already running VFE,
> so that a clock running at its highest rate does not reject the
> request.

Again, we retrieve the pixel clock to derive the required VFE **core**
clock rate. If the VFE cannot sustain that rate, failing here seems
like the correct and sane behavior.

What I don't agree with is applying the same logic to other clock
domains, such as AXI/CAMNOC clocks. Their frequency is not directly
tied to the pixel clock, but rather to the aggregated bandwidth
requirements of all active clients and data paths. As such, deriving
their rate from the pixel clock requirement feels inaccurate and, in
my opinion, is an abuse of the existing logic.

So I think the implementation should either handle or delegate to
separate functions depending on the clock type:
- Set the VFE core clock based on the pixel clock requirements
(typically vfe_clk_rate >= pixel_rate).
- For all other clocks (like camnoc_axi), and until we have proper
aggregation or voting support, a simpler and safer approach would be
to select the highest available frequency.

Mixing fundamentally different clocks under the same rate calculation
does not seem conceptually correct to me.

Regards,
Loic

>
> Signed-off-by: Atanas Filipov <atanas.filipov@xxxxxxxxxxxxxxxx>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> ---
>  drivers/media/platform/qcom/camss/camss-vfe.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 59be37f83df3..359b5942df89 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -996,15 +996,13 @@ static int vfe_set_clock_rates(struct vfe_device *vfe)
>                 if (vfe_match_clock_names(vfe, clock) && vfe_check_clock_levels(clock)) {
>                         long rate;
>
> -                       for (j = 0; j < clock->nfreqs; j++)
> +                       for (j = 0; j < clock->nfreqs - 1; j++)
>                                 if (min_rate < clock->freq[j])
>                                         break;
>
> -                       if (j == clock->nfreqs) {
> -                               dev_err(dev,
> -                                       "Pixel clock is too high for VFE");
> -                               return -EINVAL;
> -                       }
> +                       if (clock->nfreqs > 1 && min_rate >= clock->freq[j])
> +                               dev_warn(dev, "Pixel clock is too high for VFE clock %s\n",
> +                                        clock->name);
>
>                         /* if sensor pixel clock is not available */
>                         /* set highest possible VFE clock rate */
> @@ -1046,9 +1044,15 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
>
>                 if (vfe_match_clock_names(vfe, clock) && vfe_check_clock_levels(clock)) {
>                         unsigned long rate;
> +                       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;
>
>                         rate = clk_get_rate(clock->clk);
> -                       if (rate < min_rate)
> +                       if (rate < min_t(u64, min_rate, max_rate))
>                                 return -EBUSY;
>                 }
>         }
> --
> 2.34.1
>