Re: [PATCH v4 0/3] Add CAMSS support for SM6350

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 04/05/2026 08:14, Luca Weiss wrote:
> Hi Bryan,
> 
> On Sat May 2, 2026 at 12:32 AM CEST, Bryan O'Donoghue wrote:
>> On 01/05/2026 11:12, Luca Weiss wrote:
>>> Hi Bryan,
>>>
>>> On Fri Apr 3, 2026 at 5:07 PM CEST, Bryan O'Donoghue wrote:
>>>> On 03/04/2026 09:09, Luca Weiss wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> On Tue Mar 31, 2026 at 12:49 AM CEST, Vladimir Zapolskiy wrote:
>>>>>> Hi Luca,
>>>>>>
>>>>>> On 2/16/26 10:54, Luca Weiss wrote:
>>>>>>> Add bindings, driver and dts to support the Camera Subsystem on the
>>>>>>> SM6350 SoC.
>>>>>>>
>>>>>>> These patches were tested on a Fairphone 4 smartphone with WIP sensor
>>>>>>> drivers (Sony IMX576 and IMX582), the camera pipeline works properly as
>>>>>>> far as I can tell.
>>>>>>>
>>>>>>> Though when stopping the camera stream, the following clock warning
>>>>>>> appears in dmesg. But it does not interfere with any functionality,
>>>>>>> starting and stopping the stream works and debugcc is showing 426.4 MHz
>>>>>>> while the clock is on, and 'off' while it's off.
>>>>>>>
>>>>>>> Any suggestion how to fix this, is appreciated.
>>>>>>
>>>>>> I've looked at CAMCC recently, and I do notice that SM6350 CAMCC does not
>>>>>> set '.use_rpm = true' flag for whatever reason.
>>>>>>
>>>>>> If you find a free minute, can you test the change below?..
>>>>>
>>>>> Unfortunately that change does not resolve the "gcc_camera_axi_clk
>>>>> status stuck at 'on'" warning.
>>>>>
>>>>> fairphone-fp4:~$ cat /sys/bus/platform/drivers/sm6350-camcc/ad00000.clock-controller/power/runtime_status
>>>>> active
>>>>>
>>>>> fairphone-fp4:~$ cat /sys/bus/platform/drivers/sm6350-camcc/ad00000.clock-controller/power/runtime_status
>>>>> suspended
>>>>>
>>>>>>
>>>>>> ----8<----
>>>>>> diff --git a/drivers/clk/qcom/camcc-sm6350.c b/drivers/clk/qcom/camcc-sm6350.c
>>>>>> index 7df12c1311c6..ba880e4edcaf 100644
>>>>>> --- a/drivers/clk/qcom/camcc-sm6350.c
>>>>>> +++ b/drivers/clk/qcom/camcc-sm6350.c
>>>>>> @@ -1880,6 +1880,7 @@ static const struct qcom_cc_desc camcc_sm6350_desc = {
>>>>>>      	.num_clks = ARRAY_SIZE(camcc_sm6350_clocks),
>>>>>>      	.gdscs = camcc_sm6350_gdscs,
>>>>>>      	.num_gdscs = ARRAY_SIZE(camcc_sm6350_gdscs),
>>>>>> +	.use_rpm = true,
>>>>>>      };
>>>>>>
>>>>>>      static const struct of_device_id camcc_sm6350_match_table[] = {
>>>>>> ----8<----
>>>>>>
>>>>>> This change could be considered to be included in any case, I believe.
>>>>>
>>>>> I guess this change is now the way to enable pm_runtime, I had this
>>>>> series 3 years ago in February 2023:
>>>>> https://lore.kernel.org/linux-arm-msm/20230213-sm6350-camcc-runtime_pm-v3-0-d35e0d833cc4@fairphone.com/
>>>>>
>>>>> But I never followed up due to me not understanding pm_runtime well and
>>>>> no direct need for it.
>>>>>
>>>>> But I guess reviving that with use_rpm = true, add power-domains &
>>>>> required-opps to dt-bindings and sm6350.dtsi should be a good idea?
>>>>>
>>>>> Regards
>>>>> Luca
>>>>>
>>>>>>
>>>>>>> [ 5738.590980] ------------[ cut here ]------------
>>>>>>> [ 5738.591009] gcc_camera_axi_clk status stuck at 'on'
>>>>>>> [ 5738.591049] WARNING: CPU: 0 PID: 6918 at drivers/clk/qcom/clk-branch.c:87 clk_branch_toggle+0x170/0x190
>>>>>>> [ 5738.591081] Modules linked in:
>>>>>>> [ 5738.591099] CPU: 0 UID: 10000 PID: 6918 Comm: plasma-camera Tainted: G        W           6.17.0-00057-ge6b67db49622 #71 NONE
>>>>>>> [ 5738.591118] Tainted: [W]=WARN
>>>>>>> [ 5738.591126] Hardware name: Fairphone 4 (DT)
>>>>>>> [ 5738.591136] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>>>>>> [ 5738.591150] pc : clk_branch_toggle+0x170/0x190
>>>>>>> [ 5738.591164] lr : clk_branch_toggle+0x170/0x190
>>>>>>> [ 5738.591177] sp : ffff800086ed3980
>>>>>>> [ 5738.591184] x29: ffff800086ed3990 x28: 0000000000000001 x27: ffff800086ed3cd8
>>>>>>> [ 5738.591208] x26: 0000000000000000 x25: ffffda14fcfbd250 x24: 0000000000000000
>>>>>>> [ 5738.591230] x23: 0000000000000000 x22: ffffda14fc38bce0 x21: 0000000000000000
>>>>>>> [ 5738.591252] x20: ffffda14fd33e618 x19: 0000000000000000 x18: 00000000000064c8
>>>>>>> [ 5738.591274] x17: 0000000000000000 x16: 00001ae003667e9e x15: ffffda14fd2a07b0
>>>>>>> [ 5738.591295] x14: 0000000000000000 x13: 6f27207461206b63 x12: 7574732073757461
>>>>>>> [ 5738.591317] x11: 0000000000000058 x10: 0000000000000018 x9 : ffffda14fd2a0838
>>>>>>> [ 5738.591338] x8 : 0000000000057fa8 x7 : 0000000000000a16 x6 : ffffda14fd2f8838
>>>>>>> [ 5738.591360] x5 : ffff0001f6f59788 x4 : 0000000000000a15 x3 : ffff25ecf9d7e000
>>>>>>> [ 5738.591381] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000baf5c100
>>>>>>> [ 5738.591403] Call trace:
>>>>>>> [ 5738.591412]  clk_branch_toggle+0x170/0x190 (P)
>>>>>>> [ 5738.591429]  clk_branch2_disable+0x1c/0x30
>>>>>>> [ 5738.591445]  clk_core_disable+0x5c/0xb4
>>>>>>> [ 5738.591462]  clk_disable+0x38/0x60
>>>>>>> [ 5738.591478]  camss_disable_clocks+0x44/0x78
>>>>>>> [ 5738.591496]  vfe_put+0x7c/0xc0
>>>>>>> [ 5738.591512]  vfe_set_power+0x40/0x50
>>>>>>> [ 5738.591528]  pipeline_pm_power_one+0x14c/0x150
>>>>>>> [ 5738.591546]  pipeline_pm_power+0x74/0xf4
>>>>>>> [ 5738.591561]  v4l2_pipeline_pm_use+0x54/0x9c
>>>>>>> [ 5738.591577]  v4l2_pipeline_pm_put+0x14/0x40
>>>>>>> [ 5738.591592]  video_unprepare_streaming+0x18/0x24
>>>>>>> [ 5738.591608]  __vb2_queue_cancel+0x4c/0x314
>>>>>>> [ 5738.591626]  vb2_core_streamoff+0x24/0xc8
>>>>>>> [ 5738.591643]  vb2_ioctl_streamoff+0x58/0x98
>>>>>>> [ 5738.591657]  v4l_streamoff+0x24/0x30
>>>>>>> [ 5738.591672]  __video_do_ioctl+0x430/0x4a8
>>>>>>> [ 5738.591689]  video_usercopy+0x2ac/0x680
>>>>>>> [ 5738.591705]  video_ioctl2+0x18/0x40
>>>>>>> [ 5738.591720]  v4l2_ioctl+0x40/0x60
>>>>>>> [ 5738.591734]  __arm64_sys_ioctl+0x90/0xf0
>>>>>>> [ 5738.591750]  invoke_syscall.constprop.0+0x40/0xf0
>>>>>>> [ 5738.591769]  el0_svc_common.constprop.0+0x38/0xd8
>>>>>>> [ 5738.591785]  do_el0_svc+0x1c/0x28
>>>>>>> [ 5738.591801]  el0_svc+0x34/0xe8
>>>>>>> [ 5738.591820]  el0t_64_sync_handler+0xa0/0xe4
>>>>>>> [ 5738.591838]  el0t_64_sync+0x198/0x19c
>>>>>>> [ 5738.591854] ---[ end trace 0000000000000000 ]---
>>>>>>>
>>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>>> ---
>>>>>>> Changes in v4:
>>>>>>> - Update power-domain-names order (Krzysztof)
>>>>>>> - Make hex numbers lower case in init seq (David)
>>>>>>> - Pick up tags
>>>>>>> - Link to v3: https://lore.kernel.org/r/20260213-sm6350-camss-v3-0-30a845b0b7cc@fairphone.com
>>>>>>
>>>>>> Should find some time myself to issue RBs, sorry for the delay.
>>>>>>
>>>>>>> Changes in v3:
>>>>>>> - Update dt-bindings to include everything related to camss
>>>>>>> - Update regulator names
>>>>>>> - Remove slow_ahb_src
>>>>>>> - Link to v2: https://lore.kernel.org/r/20251114-sm6350-camss-v2-0-d1ff67da33b6@fairphone.com
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>> - Remove prefix from interconnect-names
>>>>>>> - Move 'top' power-domain to the top of list
>>>>>>> - Update regulator supply names
>>>>>>> - Link to v1: https://lore.kernel.org/r/20251024-sm6350-camss-v1-0-63d626638add@fairphone.com
>>>>>>>
>>>>>>> ---
>>>>>>> Luca Weiss (3):
>>>>>>>           dt-bindings: media: camss: Add qcom,sm6350-camss
>>>>>>>           media: qcom: camss: Add SM6350 support
>>>>>>>           arm64: dts: qcom: sm6350: Add CAMSS node
>>>>>>>
>>>>>>>      .../bindings/media/qcom,sm6350-camss.yaml          | 471 +++++++++++++++++++++
>>>>>>>      arch/arm64/boot/dts/qcom/sm6350.dtsi               | 233 ++++++++++
>>>>>>>      .../platform/qcom/camss/camss-csiphy-3ph-1-0.c     | 125 ++++++
>>>>>>>      drivers/media/platform/qcom/camss/camss-vfe.c      |   2 +
>>>>>>>      drivers/media/platform/qcom/camss/camss.c          | 261 ++++++++++++
>>>>>>>      drivers/media/platform/qcom/camss/camss.h          |   1 +
>>>>>>>      6 files changed, 1093 insertions(+)
>>>>>>> ---
>>>>>>> base-commit: 3daf23347bb5f4a375d0101ed29c97ce1a99721b
>>>>>>> change-id: 20251024-sm6350-camss-9c404bf9cfdd
>>>>>>>
>>>>>>> Best regards,
>>>>>
>>>>
>>>> What about taking the clock out of hardware gated mode ?
>>>>
>>>> ┌─[deckard@sagittarius-a] - [~/Development/qualcomm/qlt-kernel] - [Fri
>>>> Apr 03, 16:05]
>>>> └─[$]> git diff
>>>> diff --git a/drivers/clk/qcom/gcc-sm6350.c b/drivers/clk/qcom/gcc-sm6350.c
>>>> index a4d6dff9d0f7f..f98cb35bcd408 100644
>>>> --- a/drivers/clk/qcom/gcc-sm6350.c
>>>> +++ b/drivers/clk/qcom/gcc-sm6350.c
>>>> @@ -909,8 +909,6 @@ static struct clk_branch gcc_camera_ahb_clk = {
>>>>     static struct clk_branch gcc_camera_axi_clk = {
>>>>            .halt_reg = 0x17018,
>>>>            .halt_check = BRANCH_HALT,
>>>> -       .hwcg_reg = 0x17018,
>>>> -       .hwcg_bit = 1,
>>>
>>> Unfortunately this change has no effect, still getting the same error
>>>
>>> [  192.154311] ------------[ cut here ]------------
>>> [  192.154339] gcc_camera_axi_clk status stuck at 'on'
>>> [  192.154364] WARNING: drivers/clk/qcom/clk-branch.c:87 at clk_branch_toggle+0x170/0x190, CPU#5: CameraManager/5996
>>> [  192.154387] Modules linked in:
>>> [  192.154403] CPU: 5 UID: 10000 PID: 5996 Comm: CameraManager Tainted: G        W           7.0.0-00074-gb9262f98394c-dirty #31 PREEMPTLAZY
>>>
>>> Regards
>>> Luca
>>
>> Sorry wait a second did you say you had a fix for this around CX ?
>>
>> https://lore.kernel.org/linux-arm-msm/20230213-sm6350-camcc-runtime_pm-v3-2-d35e0d833cc4@fairphone.com/
>>
>> Is this series adding or missing power-domains = <CX> ?
>>
>> Shouldn't this be in the gcc node ?
>>
>> +			power-domains = <&rpmhpd SM6350_CX>;
>> +			required-opps = <&rpmhpd_opp_low_svs>;
> 
> In this thread I've tried ".use_rpm = true" for *camcc* - which I
> believe is the modern equivalent to all the code added in the linked
> series.
> 
> For gcc I don't believe I've tried adding anything.

gcc_camera_axi_clk is a GCC clock though.

drivers/clk/qcom/gcc-sm6350.c:static struct clk_branch 
gcc_camera_axi_clk = {
drivers/clk/qcom/gcc-sm6350.c:			.name = "gcc_camera_axi_clk",


> But practically, isn't CX always on anyways, especially with screen on
> and everthing, so practically it shouldn't make any difference?
> 
> Regards
> Luca

Yeah no it shouldn't which is why I wonder the patch you had did 
anything. No its not CX you're right, its your runtime PM patch and the 
ordering of the clock disables.

I think three things to check.

1. Does your original patch still work with current upstream
    => ordering is still the issue

2. Can we change the ordering of the enable/disable in camss
    placing the AXI clock last ?

3. BRANCH_HALT_VOTED for gcc_camera_axi_clk

#3 I don't think is really the reason but it might be worth an experiment.

Lets think about this the camera GCC AXI clock is needed to access the 
registers inside the camera block, so logically it should be the last 
clock to be switched off and the first clock to be switched on.

---
bod