Message-ID: <48bede40-584a-409a-9bca-7ae3cc420667@linaro.org>
Date: Mon, 20 Oct 2025 16:35:16 +0300
From: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>
To: Hangxiang Ma <hangxiang.ma@....qualcomm.com>,
 Vijay Kumar Tumati <vijay.tumati@....qualcomm.com>,
 Loic Poulain <loic.poulain@....qualcomm.com>
Cc: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
 Krzysztof Kozlowski <krzk@...nel.org>, Robert Foss <rfoss@...nel.org>,
 Andi Shyti <andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Todor Tomov <todor.too@...il.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, linux-i2c@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 Bryan O'Donoghue <bod@...nel.org>
Subject: Re: [PATCH] media: qcom: camss: Enable setting the rate to
 camnoc_rt_axi clock

Hi Hangxiang.

On 10/20/25 06:23, Hangxiang Ma wrote:
> On 10/17/2025 7:41 PM, Bryan O'Donoghue wrote:
>> On 16/10/2025 21:53, Vijay Kumar Tumati wrote:
>>>
>>> On 10/16/2025 8:31 AM, Bryan O'Donoghue wrote:
>>>> On 16/10/2025 13:22, Loic Poulain wrote:
>>>>>> I'm - perhaps naively - assuming this clock really is required ... and
>>>>>> that both will be needed concurrently.
>>>>> AFAIU, the NRT clock is not in use for the capture part, and only
>>>>> required for the offline processing engine (IPE, OPE), which will
>>>>> likely be described as a separated node.
>>>>
>>>> Maybe yeah though we already have bindings.
>>>>
>>>> @Hangxiang I thought we had discussed this clock was required for your
>>>> setup.
>>>>
>>>> Can you confirm with a test and then
>>>>
>>>> 1. Repost with my RB - I assume you included this on purpose
>>>> 2. Respond that you can live without it.
>>>>
>>>> ---
>>>> bod
>>>>
>>> @Bryan and others, sorry, I am just trying to understand the exact ask
>>> here. Just to add a bit more detail here, On certain architectures,
>>> there is one CAMNOC module that connects all of the camera modules (RT
>>> and NRT) to MMNOC. In these, there is one 'camnoc_axi' clock that needs
>>> to be enabled for it's operation. However, on the newer architectures,
>>> this single CAMNOC is split into two, one for RT modules (TFEs and IFE
>>> Lites) and the other for NRT (IPE and OFE). So, on a given architecture,
>>> we either require 'camnoc_axi' or 'camnoc_rt_axi' for RT operation, not
>>> both. And yes, one of them is a must. As you know, adding the support
>>> for the newer clock in "vfe_match_clock_names" will only enable the
>>> newer chip sets to define this in it's resource information and set the
>>> rate to it based on the pixel clock. In kaanapali vfe resources, we do
>>> not give the 'camnoc_axi_clk'. Hopefully we are all on the same page
>>> now, is it the suggestion to use 'camnoc_axi_clk' name for
>>> CAM_CC_CAMNOC_RT_AXI_CLK ? We thought it would be clearer to use the
>>> name the matches the exact clock. Please advise and thank you.
>>
>> The ask is to make sure this clock is needed @ the same time as the
>> other camnoc clock.
>>
>> If so then update the commit log on v2 to address the concerns given
>> that it may not be necessary.
>>
>> If not then just pining back to this patch "we checked and its not
>> needed" will do.
>>
>> ---
>> bod
> 
> @Bryan, I test two scenarios individually that also consider @Vladimir's
> concern. I confirm this clock rate setting is necessary.
> 1. Remove 'camnoc_rt_axi' from the vfe clock matching function.
> 2. Remove 'camnoc_nrt_axi' from the vfe clock resources in camss.c.
> Both of them block the image buffer write operation. More clearly, we
> will stuck at the stage when all buffers acquired but CAMSS takes no action.
> 
> I agree with @Vijay to keep 'camnoc_rt_axi' to distinguish between the
> new one and 'camnoc_axi'. The disagreement concerns how to standardize
> the camnoc clock name or how to differentiate between RT and NRT clock
> names if a new RT clock name is introduced. Other chips like sm8550,
> sm8775p depend on 'camnoc_axi'. Meanwhile, 'camnoc_rt_axi' and
> 'camnoc_nrt_axi' are both necessary for QCM2290 and X1E80100. But chips
> like QCM2290 and X1E80100 may not need to set the clock rate but
> Kaanapali needs. @Vladimir

Thank you so much for performing the tests.

I would want to add that I've made right the same tests for SM8650 CAMSS,
which also has two 'camnoc_rt_axi' and 'camnoc_nrt_axi' clocks, and due
to my tests the latter one is not needed for the raw image producing, you
may notice that I've excluded it from the v3 series sent for review:

https://lore.kernel.org/linux-media/20251017031131.2232687-2-vladimir.zapolskiy@linaro.org

> We now prefer to add 'camnoc_rt_axi' (Right?). Maybe its better to add
> comment lines to remove the ambiguity whether 'camnoc_axi' denotes to RT
> or NRT. Please advise and correct me. Willing to receive feedback and
> suggestions. Thanks you for all.

-- 
Best wishes,
Vladimir

Powered by blists - more mailing lists