Re: [PATCH v3 2/2] arm64: dts: qcom: qcm2290: Add CCI node

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 4/11/25 10:02 AM, Loic Poulain wrote:
> Hi Konrad, Bryan,
> 
> On Wed, Apr 9, 2025 at 8:39 PM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
>>
>> On 4/7/25 4:56 PM, Loic Poulain wrote:
>>> Hi Bryan,
>>>
>>>
>>> On Fri, Apr 4, 2025 at 2:10 PM Bryan O'Donoghue
>>> <bryan.odonoghue@linaro.org> wrote:
>>>>
>>>> On 03/04/2025 11:22, Loic Poulain wrote:
>>>>> Add Camera Control Interface (CCI), supporting two I2C masters.
>>>>>
>>>>> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
>>>>> ---
>>>>>   v2: Reorder commits; Update dts properties order and style
>>>>>   v3: No change for this patch
>>>>>
>>>>>   arch/arm64/boot/dts/qcom/qcm2290.dtsi | 50 +++++++++++++++++++++++++++
>>>>>   1 file changed, 50 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/qcm2290.dtsi b/arch/arm64/boot/dts/qcom/qcm2290.dtsi
>>>>> index 7fb5de92bc4c..43fcb4f40a8c 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/qcm2290.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/qcm2290.dtsi
>>>>> @@ -557,6 +557,20 @@ qup_uart4_default: qup-uart4-default-state {
>>>>>                               bias-disable;
>>>>>                       };
>>>>>
>>>>> +                     cci0_default: cci0-default-state {
>>>>> +                             pins = "gpio22", "gpio23";
>>>>> +                             function = "cci_i2c";
>>>>> +                             drive-strength = <2>;
>>>>> +                             bias-disable;
>>>>> +                     };
>>>>> +
>>>>> +                     cci1_default: cci1-default-state {
>>>>> +                             pins = "gpio29", "gpio30";
>>>>> +                             function = "cci_i2c";
>>>>> +                             drive-strength = <2>;
>>>>> +                             bias-disable;
>>>>> +                     };
>>>>> +
>>>>>                       sdc1_state_on: sdc1-on-state {
>>>>>                               clk-pins {
>>>>>                                       pins = "sdc1_clk";
>>>>> @@ -1603,6 +1617,42 @@ adreno_smmu: iommu@59a0000 {
>>>>>                       #iommu-cells = <2>;
>>>>>               };
>>>>>
>>>>> +             cci: cci@5c1b000 {
>>>>> +                     compatible = "qcom,qcm2290-cci", "qcom,msm8996-cci";
>>>>> +                     reg = <0x0 0x5c1b000 0x0 0x1000>;
>>>>> +
>>>>> +                     interrupts = <GIC_SPI 206 IRQ_TYPE_EDGE_RISING>;
>>>>> +
>>>>> +                     clocks = <&gcc GCC_CAMSS_TOP_AHB_CLK>, <&gcc GCC_CAMSS_CCI_0_CLK>;
>>>>> +                     clock-names = "camss_top_ahb", "cci";
>>>>
>>>> do you not need an axi clock GCC_CAMSS_AXI_CLK ?
>>>
>>> AFAIU AXI is not involved for CCI.
>>
>> I'm not able to infer that from statically provided infromation, try
>> forcefully shutting the clock down (both the branch and _SRC) and
>> poking at the CCI
> 
> CCI is a 'slow' device, both control and data flow go through register
> R/W (ahb/cnoc).

This is not always a given ;)

> Below is an experiment showing CAMSS AXI can be off during CCI operation.
> 
> ```
> $ cat /sys/kernel/debug/clk/clk_summary | grep -e camss_axi -e camss_cci
>        gcc_camss_axi_clk_src         0       0        0
> 19200000    0          0     50000      ?         deviceless
>           gcc_camss_axi_clk          0       0        0
> 19200000    0          0     50000      N            5c52000.camss
>               bus
>              gcc_camss_cci_clk_src   0       0        0
> 37500000    0          0     50000      ?               deviceless
>                 gcc_camss_cci_0_clk  0       0        0
> 37500000    0          0     50000      N                  cci@5c1b000
> 
> $ echo on > /sys/bus/platform/drivers/i2c-qcom-cci/5c1b000.cci/power/control
> 
> $ cat /sys/kernel/debug/clk/clk_summary | grep -e camss_axi -e camss_cci
>        gcc_camss_axi_clk_src         0       0        0
> 19200000    0          0     50000      ?         deviceless
>           gcc_camss_axi_clk          0       0        0
> 19200000    0          0     50000      N            5c52000.camss
>               bus
>              gcc_camss_cci_clk_src   1       1        0
> 37500000    0          0     50000      ?               deviceless
>                 gcc_camss_cci_0_clk  1       1        0
> 37500000    0          0     50000      Y                  cci@5c1b000
> 
> $ cat /sys/class/i2c-dev/i2c-2/name
> Qualcomm-CCI
> 
> $ i2cdetect -y -r -a 2
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 60: UU -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 70: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 
> # Dump GCC CAMSS AXI clock registers
> $ dd if=/sys/kernel/debug/regmap/1400000.clock-controller/registers
> count=8 bs=16 skip=90123
> 5802c: 80000000 # CAMSS_AXI_CMD_RCGR => ROOT_EN_DISABLE | ROOT_OFF
> 58030: 00000000 # CAMSS_AXI_CFG_RCGR => SRC_0 | BYPASS | HW_CLK_DISABLE
> [...]
> 58044: 80000220 # CAMSS_AXI_CBCR => CLK_DISABLE | SLEEP_CLOCK2 |
> WAKEUP_CLK2 | CLK_OFF
> 58048: 00010c00 # GCC_CAMSS_AXI_SREGR => MEM_CORE_ON_ACK |
> MEM_PERIPH_ON_ACK | PSCBC_SPARE_CTRL_IN

Amazing, thank you

Konrad