[PATCH 0/4] media: qcom: camss: share VFE clock sources on SM8250

[PATCH 0/4] media: qcom: camss: share VFE clock sources on SM8250

[Atanas Filipov]

On SM8250 several VFE clocks are fed by one clock source: the AREG
clocks of the two full VFEs share the fast AHB source, the two VFE Lite
share one core clock source, and all four VFEs share the camera NoC AXI
source. Each VFE sets such a source to its own rate, so the last VFE to
power on wins and can lower the rate under a VFE that is still running.
The CSID also sets the VFE core clocks to their lowest level when it
powers on after the VFE, and nothing sets the camera NoC AXI rate, so
the camera NoC runs from XO and two concurrent streams stall.

This series lets the VFE own its clock rates and aggregates the shared
sources within CAMSS:

- Patch 1 clamps each VFE clock to its highest listed rate instead of
  failing, as the VFE clock rate tables need not have the same number
  of levels.
- Patch 2 stops the CSID from setting the VFE core clock rates.
- Patch 3 groups the VFE clocks by clock source, so that a shared
  source runs at the highest rate requested by the running VFEs. It is
  enabled per SoC, so far only on SM8250, and also lets the VFE set the
  AREG clock rate.
- Patch 4 drops the unused camera NoC AXI RCG from the VFE clocks and
  lets the VFE set the camera NoC AXI rate.

The aggregation only covers the CAMSS VFEs. Drivers outside CAMSS that
use the same shared clock sources are not covered and are left to a
separate change.

Tested on RB5 (QRB5165) with imx577 and ov9282 sensors and the CSID test
pattern generator, on single, concurrent test pattern plus sensor, and
concurrent dual sensor RDI captures on all four VFEs: all streams
complete, the shared sources stay at the highest requested rate while
another VFE runs, dedicated clocks drop to XO on power off, and the
camera NoC AXI clock runs at 300 or 400 MHz while streaming, without
the concurrent capture stalls seen before the series.

Atanas Filipov (4):
  media: qcom: camss: clamp VFE clock rates to the highest level
  media: qcom: camss: sm8250: do not set VFE core clock rates from CSID
  media: qcom: camss: share VFE clock sources by the highest rate
  media: qcom: camss: sm8250: set the camera NoC AXI rate from the VFE

 drivers/media/platform/qcom/camss/camss-vfe.c |  32 ++-
 drivers/media/platform/qcom/camss/camss.c     | 211 ++++++++++++++++--
 drivers/media/platform/qcom/camss/camss.h     |  13 ++
 3 files changed, 224 insertions(+), 32 deletions(-)


base-commit: bd1ecbb61afefc6a8b876ccbc31e9228b44e9eab
-- 
2.34.1

[PATCH 1/4] media: qcom: camss: clamp VFE clock rates to the highest level

[Atanas Filipov]

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.

Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
---
 drivers/media/platform/qcom/camss/camss-vfe.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 2680a9914765..87096095b1eb 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -982,15 +982,13 @@ static int vfe_set_clock_rates(struct vfe_device *vfe)
 
 			camss_add_clock_margin(&min_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 */
@@ -1041,6 +1039,7 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
 		if (vfe_match_clock_names(vfe, clock) && vfe_check_clock_levels(clock)) {
 			u64 min_rate = 0;
 			unsigned long rate;
+			long max_rate;
 
 			for (j = VFE_LINE_RDI0; j < vfe->res->line_num; j++) {
 				u32 tmp;
@@ -1063,6 +1062,13 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
 
 			camss_add_clock_margin(&min_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;
+
+			min_rate = min_t(u64, min_rate, max_rate);
+
 			rate = clk_get_rate(clock->clk);
 			if (rate < min_rate)
 				return -EBUSY;
-- 
2.34.1

Re: [PATCH 1/4] media: qcom: camss: clamp VFE clock rates to the highest level

[Loic Poulain]

On Fri, Oct 23, 2026 at 5:29 PM Atanas Filipov
<atanas.filipov@oss.qualcomm.com> 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.

IMO, this function has become a bit messy. We should split the core
clock selection into a dedicated helper, as it is directly tied to the
pixel rate. The AXI clock scaling/selection should be handled
separately, potentially through the interconnect (ICC) framework (when
available).

Regards,
Loic

>
> Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
> ---
>  drivers/media/platform/qcom/camss/camss-vfe.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 2680a9914765..87096095b1eb 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -982,15 +982,13 @@ static int vfe_set_clock_rates(struct vfe_device *vfe)
>
>                         camss_add_clock_margin(&min_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 */
> @@ -1041,6 +1039,7 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
>                 if (vfe_match_clock_names(vfe, clock) && vfe_check_clock_levels(clock)) {
>                         u64 min_rate = 0;
>                         unsigned long rate;
> +                       long max_rate;
>
>                         for (j = VFE_LINE_RDI0; j < vfe->res->line_num; j++) {
>                                 u32 tmp;
> @@ -1063,6 +1062,13 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
>
>                         camss_add_clock_margin(&min_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;
> +
> +                       min_rate = min_t(u64, min_rate, max_rate);
> +
>                         rate = clk_get_rate(clock->clk);
>                         if (rate < min_rate)
>                                 return -EBUSY;
> --
> 2.34.1
>

Re: [PATCH 1/4] media: qcom: camss: clamp VFE clock rates to the highest level

[Atanas Filipov]

On 10/23/2026 6:45 PM, Loic Poulain wrote:
> On Fri, Oct 23, 2026 at 5:29 PM Atanas Filipov
> <atanas.filipov@oss.qualcomm.com> 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.
> 
> IMO, this function has become a bit messy. We should split the core
> clock selection into a dedicated helper, as it is directly tied to the
> pixel rate. The AXI clock scaling/selection should be handled
> separately, potentially through the interconnect (ICC) framework (when
> available).
> 
> Regards,
> Loic
> 

I completely agree. I just don't want to mix the two issues, as 
requested by the reviewers. IMO, if we can reach an agreement on the ICC 
migration, that would be great.

Regards,
Atanas

>>
>> Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
>> ---
>>   drivers/media/platform/qcom/camss/camss-vfe.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
>> index 2680a9914765..87096095b1eb 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>> @@ -982,15 +982,13 @@ static int vfe_set_clock_rates(struct vfe_device *vfe)
>>
>>                          camss_add_clock_margin(&min_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 */
>> @@ -1041,6 +1039,7 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
>>                  if (vfe_match_clock_names(vfe, clock) && vfe_check_clock_levels(clock)) {
>>                          u64 min_rate = 0;
>>                          unsigned long rate;
>> +                       long max_rate;
>>
>>                          for (j = VFE_LINE_RDI0; j < vfe->res->line_num; j++) {
>>                                  u32 tmp;
>> @@ -1063,6 +1062,13 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
>>
>>                          camss_add_clock_margin(&min_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;
>> +
>> +                       min_rate = min_t(u64, min_rate, max_rate);
>> +
>>                          rate = clk_get_rate(clock->clk);
>>                          if (rate < min_rate)
>>                                  return -EBUSY;
>> --
>> 2.34.1
>>

Re: [PATCH 1/4] media: qcom: camss: clamp VFE clock rates to the highest level

[Gjorgji Rosikopulos (Consultant)]

Hi Atanas, Loic, All,

On 10/23/2026 6:50 PM, Atanas Filipov wrote:
> On 10/23/2026 6:45 PM, Loic Poulain wrote:
>> On Fri, Oct 23, 2026 at 5:29 PM Atanas Filipov
>> <atanas.filipov@oss.qualcomm.com> 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.
>>
>> IMO, this function has become a bit messy. We should split the core
>> clock selection into a dedicated helper, as it is directly tied to the
>> pixel rate. The AXI clock scaling/selection should be handled
>> separately, potentially through the interconnect (ICC) framework (when
>> available).
>>
>> Regards,
>> Loic
>>
> 
> I completely agree. I just don't want to mix the two issues, as 
> requested by the reviewers. IMO, if we can reach an agreement on the ICC 
> migration, that would be great.
> 
> Regards,
> Atanas
> 

There have been several discussions about this topic. 
I would like to summarize the background here and reopen the discussion
about the direction proposed by Atanas.

The issue initially came up with the JPEG encoder driver, which is
represented as a separate device. Dmitry identified that changing the
camnoc_ax clock from the JPEG driver conflicts with CAMSS.
I will not revisit the discussion about why JPEG is not part of CAMSS, 
as this has already been discussed extensively.

Currently, CAMSS is represented as a single device and drives the front-end
blocks that write data to memory, such as VFE, IFE, and TFE.

Depending on the hardware architecture, CAMSS may contain different number
of front-end blocks. Some camera subsystems also contain offline engines
like JPEG and OPE.
All these engines write data to memory through the internal CAMNOC and
share the camnoc_axi clock.

The relevant topology can be summarized as follows:

CAMSS device: Front end -> CAMNOC -> MNOC
OPE device: OPE -> CAMNOC -> MNOC
JPEG device: JPEG -> CAMNOC -> MNOC

An ICC path is already available for MNOC, and each device can vote on it.

CAMSS currently has two issues on this topic that need to be addressed.

1. Clock aggregation for concurrent streaming, As described in the cover
letter, CAMSS currently does not aggregate the clock requirements when
multiple front-end engines are streaming concurrently.

2. ICC bandwidth voting
The currently available MNOC ICC voting is implemented using max bandwidth
values. This can increase power consumption in single-camera use cases.
This is not included in this patchset.

Addressing the first issue depends, to some extent, on the direction we
choose for CAMNOC ICC support. Atanas has proposed two possible approaches.

1. [PATCH v2 0/3] Add Qualcomm CAMNOC ICC provider

The patchset adds an initial CAMNOC ICC provider with multiple ports, where
each port represents an engine client of the CAMNOC.

This allows a device such as CAMSS, which is represented as a single device,
to vote independently through different CAMNOC ports. The CAMNOC ICC provider
can then aggregate the votes and calculate the appropriate camnoc_axi clock
rate.

1. [PATCH v3 0/3] Use icc-clk for the SM8250 camera NoC AXI clock

The ICC framework, together with CAMCC support, can represent a clock as an
ICC provider. The implementation is relatively simple because it reuses the
existing infrastructure, the camnoc_axi clock only needs to be exposed as an
ICC provider.

The limitation of this approach is that a device such as CAMSS cannot vote for
each engine independently.

I think the patchset should be updated based on the approach we agree to use.
If there are other possible solutions, please share them here as well.

Apologies if this message is not perfectly structured. But i think is
understandable. 

~Gjorgji

Re: [PATCH 1/4] media: qcom: camss: clamp VFE clock rates to the highest level

[Atanas Filipov]

On 10/23/2026 6:45 PM, Loic Poulain wrote:
> On Fri, Oct 23, 2026 at 5:29 PM Atanas Filipov
> <atanas.filipov@oss.qualcomm.com> 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.
> 
> IMO, this function has become a bit messy. We should split the core
> clock selection into a dedicated helper, as it is directly tied to the
> pixel rate. The AXI clock scaling/selection should be handled
> separately, potentially through the interconnect (ICC) framework (when
> available).
> 
> Regards,
> Loic
> 

In any case, I'll split it into at least two parts in the next version.

Regards,
Atanas

>>
>> Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
>> ---
>>   drivers/media/platform/qcom/camss/camss-vfe.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
>> index 2680a9914765..87096095b1eb 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>> @@ -982,15 +982,13 @@ static int vfe_set_clock_rates(struct vfe_device *vfe)
>>
>>                          camss_add_clock_margin(&min_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 */
>> @@ -1041,6 +1039,7 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
>>                  if (vfe_match_clock_names(vfe, clock) && vfe_check_clock_levels(clock)) {
>>                          u64 min_rate = 0;
>>                          unsigned long rate;
>> +                       long max_rate;
>>
>>                          for (j = VFE_LINE_RDI0; j < vfe->res->line_num; j++) {
>>                                  u32 tmp;
>> @@ -1063,6 +1062,13 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
>>
>>                          camss_add_clock_margin(&min_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;
>> +
>> +                       min_rate = min_t(u64, min_rate, max_rate);
>> +
>>                          rate = clk_get_rate(clock->clk);
>>                          if (rate < min_rate)
>>                                  return -EBUSY;
>> --
>> 2.34.1
>>

[PATCH 2/4] media: qcom: camss: sm8250: do not set VFE core clock rates from CSID

[Atanas Filipov]

The SM8250 CSID resources list clock levels for the VFE core clocks and
for the shared VFE Lite core clock. CSID power on runs after the VFE is
powered on and sets these clocks to their lowest level, overriding the
rate selected by the VFE from the pixel rate and lowering the shared
VFE Lite clock under a running VFE Lite.

The VFE core clocks are set by the VFE. Drop their levels from the CSID
resources so that CSID only enables them.

Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
---
 drivers/media/platform/qcom/camss/camss.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index c54185c93a8a..7fe12049b78a 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -2052,7 +2052,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
 		.clock = { "vfe0_csid", "vfe0_cphy_rx", "vfe0", "vfe0_areg", "vfe0_ahb" },
 		.clock_rate = { { 400000000 },
 				{ 400000000 },
-				{ 350000000, 475000000, 576000000, 720000000 },
+				{ 0 },
 				{ 100000000, 200000000, 300000000, 400000000 },
 				{ 0 } },
 		.reg = { "csid0" },
@@ -2069,7 +2069,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
 		.clock = { "vfe1_csid", "vfe1_cphy_rx", "vfe1", "vfe1_areg", "vfe1_ahb" },
 		.clock_rate = { { 400000000 },
 				{ 400000000 },
-				{ 350000000, 475000000, 576000000, 720000000 },
+				{ 0 },
 				{ 100000000, 200000000, 300000000, 400000000 },
 				{ 0 } },
 		.reg = { "csid1" },
@@ -2086,7 +2086,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
 		.clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite",  "vfe_lite_ahb" },
 		.clock_rate = { { 400000000 },
 				{ 400000000 },
-				{ 400000000, 480000000 },
+				{ 0 },
 				{ 0 } },
 		.reg = { "csid2" },
 		.interrupt = { "csid2" },
@@ -2103,7 +2103,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
 		.clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite",  "vfe_lite_ahb" },
 		.clock_rate = { { 400000000 },
 				{ 400000000 },
-				{ 400000000, 480000000 },
+				{ 0 },
 				{ 0 } },
 		.reg = { "csid3" },
 		.interrupt = { "csid3" },
-- 
2.34.1

Re: [PATCH 2/4] media: qcom: camss: sm8250: do not set VFE core clock rates from CSID

[Loic Poulain]

On Fri, Oct 23, 2026 at 5:29 PM Atanas Filipov
<atanas.filipov@oss.qualcomm.com> wrote:
>
> The SM8250 CSID resources list clock levels for the VFE core clocks and
> for the shared VFE Lite core clock. CSID power on runs after the VFE is
> powered on and sets these clocks to their lowest level, overriding the
> rate selected by the VFE from the pixel rate and lowering the shared
> VFE Lite clock under a running VFE Lite.
>
> The VFE core clocks are set by the VFE. Drop their levels from the CSID
> resources so that CSID only enables them.
>
> Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>

Reviewed-by: Loic Poulain <loic.poulain@oss.qualcomm.com>

> ---
>  drivers/media/platform/qcom/camss/camss.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index c54185c93a8a..7fe12049b78a 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -2052,7 +2052,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>                 .clock = { "vfe0_csid", "vfe0_cphy_rx", "vfe0", "vfe0_areg", "vfe0_ahb" },
>                 .clock_rate = { { 400000000 },
>                                 { 400000000 },
> -                               { 350000000, 475000000, 576000000, 720000000 },
> +                               { 0 },
>                                 { 100000000, 200000000, 300000000, 400000000 },
>                                 { 0 } },
>                 .reg = { "csid0" },
> @@ -2069,7 +2069,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>                 .clock = { "vfe1_csid", "vfe1_cphy_rx", "vfe1", "vfe1_areg", "vfe1_ahb" },
>                 .clock_rate = { { 400000000 },
>                                 { 400000000 },
> -                               { 350000000, 475000000, 576000000, 720000000 },
> +                               { 0 },
>                                 { 100000000, 200000000, 300000000, 400000000 },
>                                 { 0 } },
>                 .reg = { "csid1" },
> @@ -2086,7 +2086,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>                 .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite",  "vfe_lite_ahb" },
>                 .clock_rate = { { 400000000 },
>                                 { 400000000 },
> -                               { 400000000, 480000000 },
> +                               { 0 },
>                                 { 0 } },
>                 .reg = { "csid2" },
>                 .interrupt = { "csid2" },
> @@ -2103,7 +2103,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>                 .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite",  "vfe_lite_ahb" },
>                 .clock_rate = { { 400000000 },
>                                 { 400000000 },
> -                               { 400000000, 480000000 },
> +                               { 0 },
>                                 { 0 } },
>                 .reg = { "csid3" },
>                 .interrupt = { "csid3" },
> --
> 2.34.1
>

Re: [PATCH 2/4] media: qcom: camss: sm8250: do not set VFE core clock rates from CSID

[Bryan O'Donoghue]

On 23/10/2026 16:28, Atanas Filipov wrote:
> The SM8250 CSID resources list clock levels for the VFE core clocks and
> for the shared VFE Lite core clock. CSID power on runs after the VFE is
> powered on and sets these clocks to their lowest level, overriding the
> rate selected by the VFE from the pixel rate and lowering the shared
> VFE Lite clock under a running VFE Lite.
> 
> The VFE core clocks are set by the VFE. Drop their levels from the CSID
> resources so that CSID only enables them.
> 
> Signed-off-by: Atanas Filipov<atanas.filipov@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/camss/camss.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index c54185c93a8a..7fe12049b78a 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -2052,7 +2052,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>   		.clock = { "vfe0_csid", "vfe0_cphy_rx", "vfe0", "vfe0_areg", "vfe0_ahb" },
>   		.clock_rate = { { 400000000 },
>   				{ 400000000 },
> -				{ 350000000, 475000000, 576000000, 720000000 },
> +				{ 0 },
>   				{ 100000000, 200000000, 300000000, 400000000 },
>   				{ 0 } },

This sounds like it needs a Fixes: tag

---
bod

Re: [PATCH 2/4] media: qcom: camss: sm8250: do not set VFE core clock rates from CSID

[Atanas Filipov]

On 23/10/2026 22:32, Bryan O'Donoghue wrote:
> This sounds like it needs a Fixes: tag

Yes, I will add it in v2:

Fixes: b4436a18eedb ("media: camss: add support for SM8250 camss")

Regards,
Atanas

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

[Atanas Filipov]

Group the VFE clocks with rate tables by their clock source at probe
time, so that a VFE no longer lowers a shared source under a running
one. A source shared by several VFEs follows the highest rate requested
by its running users and drops to the highest remaining one when a VFE
is powered off; with no user left it keeps its rate, as it may feed
consumers outside CAMSS. A source with a single VFE user is dropped to
its lowest rate on power off. Clocks whose rate the VFE does not set are
left untouched.

Grouping is enabled through the vfe_clock_domains resource flag, so far
only on SM8250, where the VFE AREG clocks share the fast AHB source and
the VFE Lite clocks share one core clock source. The current parent of
an RCG depends on its last rate, so other SoCs keep setting their VFE
clocks directly.

Also let the VFE set the SM8250 AREG clock rate instead of the CSID.

Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
---
 drivers/media/platform/qcom/camss/camss-vfe.c |  14 +-
 drivers/media/platform/qcom/camss/camss.c     | 175 +++++++++++++++++-
 drivers/media/platform/qcom/camss/camss.h     |  13 ++
 3 files changed, 196 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 87096095b1eb..a3133720c765 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -907,12 +907,15 @@ static int vfe_match_clock_names(struct vfe_device *vfe,
 {
 	char vfe_name[7]; /* vfeXXX\0 */
 	char vfe_lite_name[12]; /* vfe_liteXXX\0 */
+	char vfe_areg_name[12]; /* vfeXXX_areg\0 */
 
 	snprintf(vfe_name, sizeof(vfe_name), "vfe%d", vfe->id);
 	snprintf(vfe_lite_name, sizeof(vfe_lite_name), "vfe_lite%d", vfe->id);
+	snprintf(vfe_areg_name, sizeof(vfe_areg_name), "vfe%d_areg", vfe->id);
 
 	return (!strcmp(clock->name, vfe_name) ||
 		!strcmp(clock->name, vfe_lite_name) ||
+		!strcmp(clock->name, vfe_areg_name) ||
 		!strcmp(clock->name, "vfe_lite") ||
 		!strcmp(clock->name, "camnoc_axi") ||
 		!strcmp(clock->name, "camnoc_rt_axi"));
@@ -1002,7 +1005,7 @@ static int vfe_set_clock_rates(struct vfe_device *vfe)
 				return -EINVAL;
 			}
 
-			ret = clk_set_rate(clock->clk, rate);
+			ret = camss_set_clock_rate(vfe->camss, clock, rate);
 			if (ret < 0) {
 				dev_err(dev, "clk set rate failed: %d\n", ret);
 				return ret;
@@ -1101,12 +1104,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)
@@ -1131,7 +1134,9 @@ int vfe_get(struct vfe_device *vfe)
 error_reset:
 	camss_disable_clocks(vfe->nclocks, vfe->clock);
 
-error_pm_runtime_get:
+error_release_rates:
+	camss_release_clock_rates(vfe->camss, vfe->nclocks, vfe->clock);
+
 	pm_runtime_put_sync(vfe->camss->dev);
 error_domain_off:
 	vfe->res->hw_ops->pm_domain_off(vfe);
@@ -1159,6 +1164,7 @@ void vfe_put(struct vfe_device *vfe)
 			vfe->res->hw_ops->vfe_halt(vfe);
 		}
 		camss_disable_clocks(vfe->nclocks, vfe->clock);
+		camss_release_clock_rates(vfe->camss, vfe->nclocks, vfe->clock);
 		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..db5212b14d5a 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -2053,7 +2053,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
 		.clock_rate = { { 400000000 },
 				{ 400000000 },
 				{ 0 },
-				{ 100000000, 200000000, 300000000, 400000000 },
+				{ 0 },
 				{ 0 } },
 		.reg = { "csid0" },
 		.interrupt = { "csid0" },
@@ -2070,7 +2070,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
 		.clock_rate = { { 400000000 },
 				{ 400000000 },
 				{ 0 },
-				{ 100000000, 200000000, 300000000, 400000000 },
+				{ 0 },
 				{ 0 } },
 		.reg = { "csid1" },
 		.interrupt = { "csid1" },
@@ -4509,6 +4509,167 @@ void camss_disable_clocks(int nclocks, struct camss_clock *clock)
 		clk_disable_unprepare(clock[i].clk);
 }
 
+/*
+ * A clock domain is a clock source shared by clocks of different engines.
+ * Its rate follows the highest rate requested by its users, so that an
+ * engine started at a lower rate never lowers the rate under an engine that
+ * is still running.
+ */
+struct camss_clock_domain {
+	struct list_head node;
+	struct clk *parent;
+	struct list_head users;
+	unsigned int nusers;
+};
+
+struct camss_clock_user {
+	struct list_head node;
+	struct camss_clock_domain *domain;
+	struct camss_clock *clock;
+	unsigned long rate;
+};
+
+static int camss_add_clock_user(struct camss *camss, struct camss_clock *clock)
+{
+	struct camss_clock_domain *domain;
+	struct camss_clock_user *user;
+	struct clk *parent;
+
+	if (!clock->nfreqs)
+		return 0;
+
+	parent = clk_get_parent(clock->clk);
+	if (!parent)
+		return 0;
+
+	list_for_each_entry(domain, &camss->clock_domains, node)
+		if (clk_is_match(domain->parent, parent))
+			goto add_user;
+
+	domain = devm_kzalloc(camss->dev, sizeof(*domain), GFP_KERNEL);
+	if (!domain)
+		return -ENOMEM;
+
+	domain->parent = parent;
+	INIT_LIST_HEAD(&domain->users);
+	list_add_tail(&domain->node, &camss->clock_domains);
+
+add_user:
+	user = devm_kzalloc(camss->dev, sizeof(*user), GFP_KERNEL);
+	if (!user)
+		return -ENOMEM;
+
+	user->domain = domain;
+	user->clock = clock;
+	list_add_tail(&user->node, &domain->users);
+	domain->nusers++;
+	clock->user = user;
+
+	return 0;
+}
+
+/*
+ * camss_init_clock_domains - Group the VFE clocks by clock source
+ * @camss: CAMSS device
+ *
+ * A clock whose source feeds no other VFE clock forms a dedicated domain
+ * with a single user. Only SoCs whose VFE sets the rates of clocks with a
+ * fixed source group their clocks; the VFE clocks of other SoCs are set
+ * directly.
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int camss_init_clock_domains(struct camss *camss)
+{
+	int ret, i, j;
+
+	if (!camss->res->vfe_clock_domains)
+		return 0;
+
+	for (i = 0; i < camss->res->vfe_num; i++)
+		for (j = 0; j < camss->vfe[i].nclocks; j++) {
+			ret = camss_add_clock_user(camss, &camss->vfe[i].clock[j]);
+			if (ret)
+				return ret;
+		}
+
+	return 0;
+}
+
+static int camss_apply_clock_domain(struct camss_clock_domain *domain)
+{
+	struct camss_clock_user *user, *top = NULL;
+
+	list_for_each_entry(user, &domain->users, node)
+		if (user->rate && (!top || user->rate > top->rate))
+			top = user;
+
+	if (!top)
+		return 0;
+
+	return clk_set_rate(top->clock->clk, top->rate);
+}
+
+/*
+ * camss_set_clock_rate - Set the rate of a clock
+ * @camss: CAMSS device
+ * @clock: Clock
+ * @rate: Requested rate
+ *
+ * A shared clock is set to the highest rate requested by the running users
+ * of its source.
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+int camss_set_clock_rate(struct camss *camss, struct camss_clock *clock,
+			 unsigned long rate)
+{
+	int ret;
+
+	if (!clock->user)
+		return clk_set_rate(clock->clk, rate);
+
+	mutex_lock(&camss->clock_lock);
+	clock->user->rate = rate;
+	ret = camss_apply_clock_domain(clock->user->domain);
+	mutex_unlock(&camss->clock_lock);
+
+	return ret;
+}
+
+/*
+ * camss_release_clock_rates - Release the rates requested by an engine
+ * @camss: CAMSS device
+ * @nclocks: Number of clocks of the engine
+ * @clock: Clocks of the engine
+ *
+ * A dedicated clock drops to its lowest rate. A shared clock drops to the
+ * highest rate of the users still running, and keeps its rate when none is
+ * left, as its source may feed consumers outside CAMSS.
+ */
+void camss_release_clock_rates(struct camss *camss, int nclocks,
+			       struct camss_clock *clock)
+{
+	int i;
+
+	mutex_lock(&camss->clock_lock);
+
+	for (i = 0; i < nclocks; i++) {
+		struct camss_clock_user *user = clock[i].user;
+
+		if (!user || !user->rate)
+			continue;
+
+		user->rate = 0;
+		if (user->domain->nusers == 1)
+			clk_set_rate(user->clock->clk, 0);
+		else
+			camss_apply_clock_domain(user->domain);
+	}
+
+	mutex_unlock(&camss->clock_lock);
+}
+
 /*
  * camss_find_sensor_pad - Find the media pad via which the sensor is linked
  * @entity: Media entity to start searching from
@@ -5427,6 +5588,11 @@ static int camss_probe(struct platform_device *pdev)
 
 	atomic_set(&camss->ref_count, 0);
 	camss->dev = dev;
+	INIT_LIST_HEAD(&camss->clock_domains);
+
+	ret = devm_mutex_init(dev, &camss->clock_lock);
+	if (ret)
+		return ret;
 	platform_set_drvdata(pdev, camss);
 
 	camss->csiphy = devm_kcalloc(dev, camss->res->csiphy_num,
@@ -5474,6 +5640,10 @@ static int camss_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_genpd_cleanup;
 
+	ret = camss_init_clock_domains(camss);
+	if (ret < 0)
+		goto err_genpd_cleanup;
+
 	ret = dma_set_mask_and_coherent(dev, 0xffffffff);
 	if (ret)
 		goto err_genpd_cleanup;
@@ -5736,6 +5906,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_domains = 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..ad1d49275e0f 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -11,6 +11,8 @@
 #define QC_MSM_CAMSS_H
 
 #include <linux/device.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
 #include <linux/types.h>
 #include <media/v4l2-async.h>
 #include <media/v4l2-device.h>
@@ -120,6 +122,7 @@ struct camss_resources {
 	const unsigned int tpg_num;
 	const unsigned int csid_num;
 	const unsigned int vfe_num;
+	const bool vfe_clock_domains;
 };
 
 struct camss {
@@ -141,6 +144,9 @@ struct camss {
 	struct icc_path *icc_path[ICC_SM8250_COUNT];
 	const struct camss_resources *res;
 	bool legacy_phy;
+	/* Protects clock_domains and the rates requested by their users */
+	struct mutex clock_lock;
+	struct list_head clock_domains;
 };
 
 struct camss_camera_interface {
@@ -153,11 +159,14 @@ struct camss_async_subdev {
 	struct camss_camera_interface interface;
 };
 
+struct camss_clock_user;
+
 struct camss_clock {
 	struct clk *clk;
 	const char *name;
 	u32 *freq;
 	u32 nfreqs;
+	struct camss_clock_user *user;
 };
 
 struct parent_dev_ops {
@@ -170,6 +179,10 @@ void camss_add_clock_margin(u64 *rate);
 int camss_enable_clocks(int nclocks, struct camss_clock *clock,
 			struct device *dev);
 void camss_disable_clocks(int nclocks, struct camss_clock *clock);
+int camss_set_clock_rate(struct camss *camss, struct camss_clock *clock,
+			 unsigned long rate);
+void camss_release_clock_rates(struct camss *camss, int nclocks,
+			       struct camss_clock *clock);
 struct media_pad *camss_find_sensor_pad(struct media_entity *entity);
 s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
 			unsigned int lanes);
-- 
2.34.1

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

[Bryan O'Donoghue]

On 23/10/2026 16:28, Atanas Filipov wrote:
> Group the VFE clocks with rate tables by their clock source at probe
> time, so that a VFE no longer lowers a shared source under a running
> one. A source shared by several VFEs follows the highest rate requested
> by its running users and drops to the highest remaining one when a VFE
> is powered off; with no user left it keeps its rate, as it may feed
> consumers outside CAMSS. A source with a single VFE user is dropped to
> its lowest rate on power off. Clocks whose rate the VFE does not set are
> left untouched.
> 
> Grouping is enabled through the vfe_clock_domains resource flag, so far
> only on SM8250, where the VFE AREG clocks share the fast AHB source and
> the VFE Lite clocks share one core clock source. The current parent of
> an RCG depends on its last rate, so other SoCs keep setting their VFE
> clocks directly.
> 
> Also let the VFE set the SM8250 AREG clock rate instead of the CSID.

You're simultaneously adding a new clock to an existing list and 
introducing a behaviour change at the same time.

Those two things should be split on principle.

> Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/camss/camss-vfe.c |  14 +-
>   drivers/media/platform/qcom/camss/camss.c     | 175 +++++++++++++++++-
>   drivers/media/platform/qcom/camss/camss.h     |  13 ++
>   3 files changed, 196 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 87096095b1eb..a3133720c765 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -907,12 +907,15 @@ static int vfe_match_clock_names(struct vfe_device *vfe,
>   {
>   	char vfe_name[7]; /* vfeXXX\0 */
>   	char vfe_lite_name[12]; /* vfe_liteXXX\0 */
> +	char vfe_areg_name[12]; /* vfeXXX_areg\0 */
>   
>   	snprintf(vfe_name, sizeof(vfe_name), "vfe%d", vfe->id);
>   	snprintf(vfe_lite_name, sizeof(vfe_lite_name), "vfe_lite%d", vfe->id);
> +	snprintf(vfe_areg_name, sizeof(vfe_areg_name), "vfe%d_areg", vfe->id);
>   
>   	return (!strcmp(clock->name, vfe_name) ||
>   		!strcmp(clock->name, vfe_lite_name) ||
> +		!strcmp(clock->name, vfe_areg_name) ||
>   		!strcmp(clock->name, "vfe_lite") ||
>   		!strcmp(clock->name, "camnoc_axi") ||
>   		!strcmp(clock->name, "camnoc_rt_axi"));
> @@ -1002,7 +1005,7 @@ static int vfe_set_clock_rates(struct vfe_device *vfe)
>   				return -EINVAL;
>   			}
>   
> -			ret = clk_set_rate(clock->clk, rate);
> +			ret = camss_set_clock_rate(vfe->camss, clock, rate);
>   			if (ret < 0) {
>   				dev_err(dev, "clk set rate failed: %d\n", ret);
>   				return ret;
> @@ -1101,12 +1104,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)
> @@ -1131,7 +1134,9 @@ int vfe_get(struct vfe_device *vfe)
>   error_reset:
>   	camss_disable_clocks(vfe->nclocks, vfe->clock);
>   
> -error_pm_runtime_get:
> +error_release_rates:
> +	camss_release_clock_rates(vfe->camss, vfe->nclocks, vfe->clock);
> +
>   	pm_runtime_put_sync(vfe->camss->dev);
>   error_domain_off:
>   	vfe->res->hw_ops->pm_domain_off(vfe);
> @@ -1159,6 +1164,7 @@ void vfe_put(struct vfe_device *vfe)
>   			vfe->res->hw_ops->vfe_halt(vfe);
>   		}
>   		camss_disable_clocks(vfe->nclocks, vfe->clock);
> +		camss_release_clock_rates(vfe->camss, vfe->nclocks, vfe->clock);
>   		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..db5212b14d5a 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -2053,7 +2053,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>   		.clock_rate = { { 400000000 },
>   				{ 400000000 },
>   				{ 0 },
> -				{ 100000000, 200000000, 300000000, 400000000 },
> +				{ 0 },
>   				{ 0 } },
>   		.reg = { "csid0" },
>   		.interrupt = { "csid0" },
> @@ -2070,7 +2070,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>   		.clock_rate = { { 400000000 },
>   				{ 400000000 },
>   				{ 0 },
> -				{ 100000000, 200000000, 300000000, 400000000 },
> +				{ 0 },
>   				{ 0 } },
>   		.reg = { "csid1" },
>   		.interrupt = { "csid1" },
> @@ -4509,6 +4509,167 @@ void camss_disable_clocks(int nclocks, struct camss_clock *clock)
>   		clk_disable_unprepare(clock[i].clk);
>   }
>   
> +/*
> + * A clock domain is a clock source shared by clocks of different engines.
> + * Its rate follows the highest rate requested by its users, so that an
> + * engine started at a lower rate never lowers the rate under an engine that
> + * is still running.
> + */
> +struct camss_clock_domain {
> +	struct list_head node;
> +	struct clk *parent;
> +	struct list_head users;
> +	unsigned int nusers;
> +};

It seems to me as a read this that the above text is the abstract 
description of at least one patch - which should have examples in the 
commit log, to make it less abstract.

> +
> +struct camss_clock_user {
> +	struct list_head node;
> +	struct camss_clock_domain *domain;
> +	struct camss_clock *clock;
> +	unsigned long rate;
> +};
> +
> +static int camss_add_clock_user(struct camss *camss, struct camss_clock *clock)
> +{
> +	struct camss_clock_domain *domain;
> +	struct camss_clock_user *user;
> +	struct clk *parent;
> +
> +	if (!clock->nfreqs)
> +		return 0;
> +
> +	parent = clk_get_parent(clock->clk);
> +	if (!parent)
> +		return 0;
> +
> +	list_for_each_entry(domain, &camss->clock_domains, node)
> +		if (clk_is_match(domain->parent, parent))
> +			goto add_user;
> +
> +	domain = devm_kzalloc(camss->dev, sizeof(*domain), GFP_KERNEL);
> +	if (!domain)
> +		return -ENOMEM;
> +
> +	domain->parent = parent;
> +	INIT_LIST_HEAD(&domain->users);
> +	list_add_tail(&domain->node, &camss->clock_domains);
> +
> +add_user:
> +	user = devm_kzalloc(camss->dev, sizeof(*user), GFP_KERNEL);
> +	if (!user)
> +		return -ENOMEM;
> +
> +	user->domain = domain;
> +	user->clock = clock;
> +	list_add_tail(&user->node, &domain->users);
> +	domain->nusers++;
> +	clock->user = user;
> +
> +	return 0;
> +}
> +
> +/*
> + * camss_init_clock_domains - Group the VFE clocks by clock source
> + * @camss: CAMSS device
> + *
> + * A clock whose source feeds no other VFE clock forms a dedicated domain
> + * with a single user. Only SoCs whose VFE sets the rates of clocks with a
> + * fixed source group their clocks; the VFE clocks of other SoCs are set
> + * directly.

What do you mean by user here, its not clear to me. Another engine ? I 
think so - you mean the JPEG has a dependency on the clocks listed in 
CAMSS ?

I thought we decided for full separation of JPEG and CAMSS/IFE ?

> + *
> + * Return 0 on success or a negative error code otherwise
> + */
> +static int camss_init_clock_domains(struct camss *camss)
> +{
> +	int ret, i, j;
> +
> +	if (!camss->res->vfe_clock_domains)
> +		return 0;
> +
> +	for (i = 0; i < camss->res->vfe_num; i++)
> +		for (j = 0; j < camss->vfe[i].nclocks; j++) {
> +			ret = camss_add_clock_user(camss, &camss->vfe[i].clock[j]);
> +			if (ret)
> +				return ret;
> +		}
> +
> +	return 0;
> +}
> +
> +static int camss_apply_clock_domain(struct camss_clock_domain *domain)
> +{
> +	struct camss_clock_user *user, *top = NULL;
> +
> +	list_for_each_entry(user, &domain->users, node)
> +		if (user->rate && (!top || user->rate > top->rate))
> +			top = user;
> +
> +	if (!top)
> +		return 0;
> +
> +	return clk_set_rate(top->clock->clk, top->rate);
> +}
> +
> +/*
> + * camss_set_clock_rate - Set the rate of a clock
> + * @camss: CAMSS device
> + * @clock: Clock
> + * @rate: Requested rate
> + *
> + * A shared clock is set to the highest rate requested by the running users
> + * of its source.
> + *
> + * Return 0 on success or a negative error code otherwise
> + */
> +int camss_set_clock_rate(struct camss *camss, struct camss_clock *clock,
> +			 unsigned long rate)
> +{
> +	int ret;
> +
> +	if (!clock->user)
> +		return clk_set_rate(clock->clk, rate);
> +
> +	mutex_lock(&camss->clock_lock);
> +	clock->user->rate = rate;
> +	ret = camss_apply_clock_domain(clock->user->domain);
> +	mutex_unlock(&camss->clock_lock);
> +
> +	return ret;
> +}
> +
> +/*
> + * camss_release_clock_rates - Release the rates requested by an engine
> + * @camss: CAMSS device
> + * @nclocks: Number of clocks of the engine
> + * @clock: Clocks of the engine
> + *
> + * A dedicated clock drops to its lowest rate. A shared clock drops to the
> + * highest rate of the users still running, and keeps its rate when none is
> + * left, as its source may feed consumers outside CAMSS.
> + */
> +void camss_release_clock_rates(struct camss *camss, int nclocks,
> +			       struct camss_clock *clock)
> +{
> +	int i;
> +
> +	mutex_lock(&camss->clock_lock);
> +
> +	for (i = 0; i < nclocks; i++) {
> +		struct camss_clock_user *user = clock[i].user;
> +
> +		if (!user || !user->rate)
> +			continue;
> +
> +		user->rate = 0;
> +		if (user->domain->nusers == 1)
> +			clk_set_rate(user->clock->clk, 0);
> +		else
> +			camss_apply_clock_domain(user->domain);
> +	}
> +
> +	mutex_unlock(&camss->clock_lock);
> +}
> +
>   /*
>    * camss_find_sensor_pad - Find the media pad via which the sensor is linked
>    * @entity: Media entity to start searching from
> @@ -5427,6 +5588,11 @@ static int camss_probe(struct platform_device *pdev)
>   
>   	atomic_set(&camss->ref_count, 0);
>   	camss->dev = dev;
> +	INIT_LIST_HEAD(&camss->clock_domains);
> +
> +	ret = devm_mutex_init(dev, &camss->clock_lock);
> +	if (ret)
> +		return ret;
>   	platform_set_drvdata(pdev, camss);
>   
>   	camss->csiphy = devm_kcalloc(dev, camss->res->csiphy_num,
> @@ -5474,6 +5640,10 @@ static int camss_probe(struct platform_device *pdev)
>   	if (ret < 0)
>   		goto err_genpd_cleanup;
>   
> +	ret = camss_init_clock_domains(camss);
> +	if (ret < 0)
> +		goto err_genpd_cleanup;
> +
>   	ret = dma_set_mask_and_coherent(dev, 0xffffffff);
>   	if (ret)
>   		goto err_genpd_cleanup;
> @@ -5736,6 +5906,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_domains = 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..ad1d49275e0f 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -11,6 +11,8 @@
>   #define QC_MSM_CAMSS_H
>   
>   #include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
>   #include <linux/types.h>
>   #include <media/v4l2-async.h>
>   #include <media/v4l2-device.h>
> @@ -120,6 +122,7 @@ struct camss_resources {
>   	const unsigned int tpg_num;
>   	const unsigned int csid_num;
>   	const unsigned int vfe_num;
> +	const bool vfe_clock_domains;
>   };
>   
>   struct camss {
> @@ -141,6 +144,9 @@ struct camss {
>   	struct icc_path *icc_path[ICC_SM8250_COUNT];
>   	const struct camss_resources *res;
>   	bool legacy_phy;
> +	/* Protects clock_domains and the rates requested by their users */
> +	struct mutex clock_lock;
> +	struct list_head clock_domains;
>   };
>   
>   struct camss_camera_interface {
> @@ -153,11 +159,14 @@ struct camss_async_subdev {
>   	struct camss_camera_interface interface;
>   };
>   
> +struct camss_clock_user;
> +
>   struct camss_clock {
>   	struct clk *clk;
>   	const char *name;
>   	u32 *freq;
>   	u32 nfreqs;
> +	struct camss_clock_user *user;
>   };
>   
>   struct parent_dev_ops {
> @@ -170,6 +179,10 @@ void camss_add_clock_margin(u64 *rate);
>   int camss_enable_clocks(int nclocks, struct camss_clock *clock,
>   			struct device *dev);
>   void camss_disable_clocks(int nclocks, struct camss_clock *clock);
> +int camss_set_clock_rate(struct camss *camss, struct camss_clock *clock,
> +			 unsigned long rate);
> +void camss_release_clock_rates(struct camss *camss, int nclocks,
> +			       struct camss_clock *clock);
>   struct media_pad *camss_find_sensor_pad(struct media_entity *entity);
>   s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
>   			unsigned int lanes);

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

[Atanas Filipov]

On 23/10/2026 22:41, Bryan O'Donoghue wrote:
> You're simultaneously adding a new clock to an existing list and
> introducing a behaviour change at the same time.
>
> Those two things should be split on principle.

Agreed, I will split it in v2. The grouping goes first, then a
separate patch lets the VFE set the AREG clock rate. In that order the
two VFEs never set the shared fast AHB source without the grouping in
place.

> It seems to me as a read this that the above text is the abstract
> description of at least one patch - which should have examples in the
> commit log, to make it less abstract.

Will do. 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:

  - VFE Lite 0 streams the CSID test pattern and sets the source to
    480 MHz.
  - VFE Lite 1 then starts an imx577 stream, needs only 400 MHz, and
    sets the shared source to 400 MHz under the running VFE Lite 0.

With the grouping the source stays at 480 MHz until VFE Lite 0 stops,
and then drops to the 400 MHz still requested by VFE Lite 1. Once the
VFE also sets the AREG clock rate, the same applies to the fast AHB
source shared by the AREG clocks of VFE0 and VFE1.

> What do you mean by user here, its not clear to me. Another engine ? I
> think so - you mean the JPEG has a dependency on the clocks listed in
> CAMSS ?

No, JPEG is not involved. A user is a VFE clock fed by the shared
source, so the users of the fast AHB source are the AREG clocks of
VFE0 and VFE1, both inside CAMSS. The grouping only covers the VFEs,
which is why the cover letter says that drivers outside CAMSS are not
covered.

I can rename it in v2 if a different name reads better.

> I thought we decided for full separation of JPEG and CAMSS/IFE ?

Yes, and this series keeps that: it only touches CAMSS.

Regards,
Atanas

[PATCH 4/4] media: qcom: camss: sm8250: set the camera NoC AXI rate from the VFE

[Atanas Filipov]

The SM8250 VFE resources list both the camera NoC AXI RCG,
camnoc_axi_src, and its camnoc_axi branch. The camnoc_axi_src levels
are never applied, since vfe_match_clock_names() does not match that
name, and they list 480 MHz, which the RCG does not support. The
camnoc_axi branch has no rates, so the camera NoC runs from XO and two
concurrent streams stall.

Drop camnoc_axi_src, as the camnoc_axi branch keeps it enabled through
its parent, and list the 300 and 400 MHz camera NoC AXI rates for the
camnoc_axi branch, whose rate the VFE sets through the parent. The four
VFEs share its source through a clock domain, so it runs at the highest
rate requested by the running VFEs.

Signed-off-by: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
---
 drivers/media/platform/qcom/camss/camss.c | 28 ++++++++++-------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index db5212b14d5a..71abd01a4286 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -2120,13 +2120,12 @@ static const struct camss_subdev_resources vfe_res_8250[] = {
 	/* VFE0 */
 	{
 		.regulators = {},
-		.clock = { "camnoc_axi_src", "slow_ahb_src", "cpas_ahb",
+		.clock = { "slow_ahb_src", "cpas_ahb",
 			   "camnoc_axi", "vfe0_ahb", "vfe0_areg", "vfe0",
 			   "vfe0_axi", "cam_hf_axi" },
-		.clock_rate = { { 19200000, 300000000, 400000000, 480000000 },
-				{ 19200000, 80000000 },
+		.clock_rate = { { 19200000, 80000000 },
 				{ 19200000 },
-				{ 0 },
+				{ 300000000, 400000000 },
 				{ 0 },
 				{ 100000000, 200000000, 300000000, 400000000 },
 				{ 350000000, 475000000, 576000000, 720000000 },
@@ -2146,13 +2145,12 @@ static const struct camss_subdev_resources vfe_res_8250[] = {
 	/* VFE1 */
 	{
 		.regulators = {},
-		.clock = { "camnoc_axi_src", "slow_ahb_src", "cpas_ahb",
+		.clock = { "slow_ahb_src", "cpas_ahb",
 			   "camnoc_axi", "vfe1_ahb", "vfe1_areg", "vfe1",
 			   "vfe1_axi", "cam_hf_axi" },
-		.clock_rate = { { 19200000, 300000000, 400000000, 480000000 },
-				{ 19200000, 80000000 },
+		.clock_rate = { { 19200000, 80000000 },
 				{ 19200000 },
-				{ 0 },
+				{ 300000000, 400000000 },
 				{ 0 },
 				{ 100000000, 200000000, 300000000, 400000000 },
 				{ 350000000, 475000000, 576000000, 720000000 },
@@ -2172,13 +2170,12 @@ static const struct camss_subdev_resources vfe_res_8250[] = {
 	/* VFE2 (lite) */
 	{
 		.regulators = {},
-		.clock = { "camnoc_axi_src", "slow_ahb_src", "cpas_ahb",
+		.clock = { "slow_ahb_src", "cpas_ahb",
 			   "camnoc_axi", "vfe_lite_ahb", "vfe_lite_axi",
 			   "vfe_lite", "cam_hf_axi" },
-		.clock_rate = { { 19200000, 300000000, 400000000, 480000000 },
-				{ 19200000, 80000000 },
+		.clock_rate = { { 19200000, 80000000 },
 				{ 19200000 },
-				{ 0 },
+				{ 300000000, 400000000 },
 				{ 0 },
 				{ 0 },
 				{ 400000000, 480000000 },
@@ -2196,13 +2193,12 @@ static const struct camss_subdev_resources vfe_res_8250[] = {
 	/* VFE3 (lite) */
 	{
 		.regulators = {},
-		.clock = { "camnoc_axi_src", "slow_ahb_src", "cpas_ahb",
+		.clock = { "slow_ahb_src", "cpas_ahb",
 			   "camnoc_axi", "vfe_lite_ahb", "vfe_lite_axi",
 			   "vfe_lite", "cam_hf_axi" },
-		.clock_rate = { { 19200000, 300000000, 400000000, 480000000 },
-				{ 19200000, 80000000 },
+		.clock_rate = { { 19200000, 80000000 },
 				{ 19200000 },
-				{ 0 },
+				{ 300000000, 400000000 },
 				{ 0 },
 				{ 0 },
 				{ 400000000, 480000000 },
-- 
2.34.1

Re: [PATCH 0/4] media: qcom: camss: share VFE clock sources on SM8250

[Bryan O'Donoghue]

On 23/10/2026 16:28, Atanas Filipov wrote:
> The aggregation only covers the CAMSS VFEs. Drivers outside CAMSS that
> use the same shared clock sources are not covered and are left to a
> separate change.

Hmm.

Lets discuss that change so. We need to represent shared clocks through 
CamNoC anyway right ?

---
bod

Re: [PATCH 0/4] media: qcom: camss: share VFE clock sources on SM8250

[Atanas Filipov]

On 23/10/2026 22:44, Bryan O'Donoghue wrote:
> On 23/10/2026 16:28, Atanas Filipov wrote:
>> The aggregation only covers the CAMSS VFEs. Drivers outside CAMSS that
>> use the same shared clock sources are not covered and are left to a
>> separate change.
>
> Lets discuss that change so. We need to represent shared clocks through
> CamNoC anyway right ?

Yes. The camera NoC AXI clock is shared by everything that writes
through the camera NoC: the CAMSS front ends, and offline engines such
as OPE and JPEG that are separate devices. Today no one aggregates
their requests.

I have posted two versions of this so far:

  - v2 added a CAMNOC interconnect provider with one port per client:
    https://lore.kernel.org/all/20260819093211.1436275-1-atanas.filipov@oss.qualcomm.com/
  - v3 exports the camcc camera NoC AXI RCG through icc-clk, as
    gcc-ipq9574 does:
    https://lore.kernel.org/all/20261016170701.1504338-1-atanas.filipov@oss.qualcomm.com/

On v3 Krzysztof objected that a clock is not an interconnect and
suggested fixing the clock framework instead. Dmitry asked to land
icc-clk first, then the optional CAMSS interconnect, then JPEG. Both
questions are still open. Gjorgji summarised the two options in this
thread.

This series does not depend on that choice. It only stops the VFEs
from overriding each other inside CAMSS. Once the camera NoC is
represented, the VFE stops setting the camera NoC AXI rate and votes
instead; I have that change ready on top of v3.

Which direction do you prefer: per-client ports in a CAMNOC provider
(v2), icc-clk on the camcc RCG (v3), or the clock framework?

Regards,
Atanas