| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2e38b9f3-8a35-4a27-82d3-c1d4996a1684@oss.qualcomm.com>
Date: Mon, 8 Dec 2025 15:21:02 -0800
From: Vijay Kumar Tumati <vijay.tumati@....qualcomm.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Hangxiang Ma <hangxiang.ma@....qualcomm.com>,
Loic Poulain <loic.poulain@....qualcomm.com>,
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>,
Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
Krzysztof Kozlowski <krzk@...nel.org>
Subject: Re: [PATCH v9 1/5] media: dt-bindings: Add CAMSS device for Kaanapali
On 12/8/2025 2:48 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 08, 2025 at 01:03:06PM -0800, Vijay Kumar Tumati wrote:
>> On 12/8/2025 11:53 AM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 08, 2025 at 04:39:47AM -0800, Hangxiang Ma wrote:
>>>> Add bindings for qcom,kaanapali-camss to support the Camera Subsystem
>>>> (CAMSS) on the Qualcomm Kaanapali platform.
>>>>
>>>> The Kaanapali platform provides:
>>>>
>>>> - 3 x VFE, 5 RDI per VFE
>>>> - 2 x VFE Lite, 4 RDI per VFE Lite
>>>> - 3 x CSID
>>>> - 2 x CSID Lite
>>>> - 6 x CSIPHY
>>>> - 2 x ICP
>>>> - 1 x IPE
>>>> - 2 x JPEG DMA & Downscaler
>>>> - 2 x JPEG Encoder
>>>> - 1 x OFE
>>>> - 5 x RT CDM
>>>> - 3 x TPG
>>> Please describe the acronyms.
>> Ack.
>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
>>>> Reviewed-by: Krzysztof Kozlowski <krzk@...nel.org>
>>>> Signed-off-by: Hangxiang Ma <hangxiang.ma@....qualcomm.com>
>>>> ---
>>>> .../bindings/media/qcom,kaanapali-camss.yaml | 646 +++++++++++++++++++++
>>>> 1 file changed, 646 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,kaanapali-camss.yaml b/Documentation/devicetree/bindings/media/qcom,kaanapali-camss.yaml
>>>> new file mode 100644
>>>> index 000000000000..3b54620e14c6
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/qcom,kaanapali-camss.yaml
>>>> @@ -0,0 +1,646 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/media/qcom,kaanapali-camss.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Qualcomm Kaanapali Camera Subsystem (CAMSS)
>>>> +
>>>> +maintainers:
>>>> + - Hangxiang Ma <hangxiang.ma@....qualcomm.com>
>>>> +
>>>> +description:
>>>> + Kaanapali camera subsystem includes submodules such as CSIPHY (CSI Physical layer)
>>>> + and CSID (CSI Decoder), which comply with the MIPI CSI2 protocol.
>>>> +
>>>> + The subsystem also integrates a set of real-time image processing engines and their
>>>> + associated configuration modules, as well as non-real-time engines.
>>>> +
>>>> + Additionally, it encompasses a test pattern generator (TPG) submodule.
>>>> +
>>>> +properties:
>>>> + compatible:
>>>> + const: qcom,kaanapali-camss
>>>> +
>>>> + reg:
>>>> + items:
>>>> + - description: Registers for CSID 0
>>>> + - description: Registers for CSID 1
>>>> + - description: Registers for CSID 2
>>>> + - description: Registers for CSID Lite 0
>>>> + - description: Registers for CSID Lite 1
>>>> + - description: Registers for CSIPHY 0
>>>> + - description: Registers for CSIPHY 1
>>>> + - description: Registers for CSIPHY 2
>>>> + - description: Registers for CSIPHY 3
>>>> + - description: Registers for CSIPHY 4
>>>> + - description: Registers for CSIPHY 5
>>>> + - description: Registers for VFE (Video Front End) 0
>>>> + - description: Registers for VFE 1
>>>> + - description: Registers for VFE 2
>>>> + - description: Registers for VFE Lite 0
>>>> + - description: Registers for VFE Lite 1
>>>> + - description: Registers for ICP (Imaging Control Processor) 0
>>>> + - description: Registers for ICP 0 SYS
>>>> + - description: Registers for ICP 1
>>>> + - description: Registers for ICP 1 SYS
>>>> + - description: Registers for IPE (Image Processing Engine)
>>>> + - description: Registers for JPEG DMA & Downscaler
>>>> + - description: Registers for JPEG Encoder
>>>> + - description: Registers for OFE (Offline Front End)
>>>> + - description: Registers for RT CDM (Camera Data Mover) 0
>>>> + - description: Registers for RT CDM 1
>>>> + - description: Registers for RT CDM 2
>>>> + - description: Registers for RT CDM 3
>>>> + - description: Registers for RT CDM 4
>>>> + - description: Registers for TPG 0
>>>> + - description: Registers for TPG 1
>>>> + - description: Registers for TPG 2
>>>> +
>>>> + reg-names:
>>>> + items:
>>>> + - const: csid0
>>>> + - const: csid1
>>>> + - const: csid2
>>>> + - const: csid_lite0
>>>> + - const: csid_lite1
>>>> + - const: csiphy0
>>>> + - const: csiphy1
>>>> + - const: csiphy2
>>>> + - const: csiphy3
>>>> + - const: csiphy4
>>>> + - const: csiphy5
>>>> + - const: vfe0
>>>> + - const: vfe1
>>>> + - const: vfe2
>>>> + - const: vfe_lite0
>>>> + - const: vfe_lite1
>>>> + - const: icp0
>>>> + - const: icp0_sys
>>>> + - const: icp1
>>>> + - const: icp1_sys
>>>> + - const: ipe
>>>> + - const: jpeg_dma
>>>> + - const: jpeg_enc
>>>> + - const: ofe
>>>> + - const: rt_cdm0
>>>> + - const: rt_cdm1
>>>> + - const: rt_cdm2
>>>> + - const: rt_cdm3
>>>> + - const: rt_cdm4
>>>> + - const: tpg0
>>>> + - const: tpg1
>>>> + - const: tpg2
>>>> +
>>>> + clocks:
>>>> + maxItems: 60
>>>> +
>>>> + clock-names:
>>>> + items:
>>>> + - const: camnoc_nrt_axi
>>>> + - const: camnoc_rt_axi
>>>> + - const: camnoc_rt_vfe0
>>>> + - const: camnoc_rt_vfe1
>>>> + - const: camnoc_rt_vfe2
>>>> + - const: camnoc_rt_vfe_lite
>>>> + - const: cpas_ahb
>>>> + - const: cpas_fast_ahb
>>>> + - const: csid
>>>> + - const: csid_csiphy_rx
>>>> + - const: csiphy0
>>>> + - const: csiphy0_timer
>>>> + - const: csiphy1
>>>> + - const: csiphy1_timer
>>>> + - const: csiphy2
>>>> + - const: csiphy2_timer
>>>> + - const: csiphy3
>>>> + - const: csiphy3_timer
>>>> + - const: csiphy4
>>>> + - const: csiphy4_timer
>>>> + - const: csiphy5
>>>> + - const: csiphy5_timer
>>>> + - const: gcc_axi_hf
>>> This clock (and gcc_axi_sf below) still have the gcc_ prefix and GCC name. Why?
>>> It was pointed out in the previous review: clock names should be
>>> describing their purpose, not their source.
>> Hi Dmitry, let me add a bit more detail on this clock. As confirmed by the
>> HW team, the logic that runs based on this clock is still inside the
>> CAMNOC_PDX, just that it is on the CX / MMNOC domain side. Do you think
>> "axi_hf_cx" and "axi_sf_cx" makes sense?
> Why? You are again describing the source. What is the function of?
> bus_hf / bus_sf?
In what I proposed,
axi - represents that we are talking about the axi bus from camera
(against ahb bus).
hf - hf wrapper
cx - logic on the CX side of the bus in CAMNOC.
If you think that 'bus' (even looking from camera client side) by
default means AXI bus and 'hf' and 'sf' implicitly represent the CX side
(which, kind of, in the current design), then yes, "bus_hf" and "bus_sf"
makes sense. Do you advise us to go ahead with these?
>>>> + - const: vfe0
>>>> + - const: vfe0_fast_ahb
>>>> + - const: vfe1
>>>> + - const: vfe1_fast_ahb
>>>> + - const: vfe2
>>>> + - const: vfe2_fast_ahb
>>>> + - const: vfe_lite
>>>> + - const: vfe_lite_ahb
>>>> + - const: vfe_lite_cphy_rx
>>>> + - const: vfe_lite_csid
>>>> + - const: qdss_debug_xo
>>>> + - const: camnoc_ipe_nps
>>>> + - const: camnoc_ofe
>>>> + - const: gcc_axi_sf
>>>> + - const: icp0
>>>> + - const: icp0_ahb
>>>> + - const: icp1
>>>> + - const: icp1_ahb
>>>> + - const: ipe_nps
>>>> + - const: ipe_nps_ahb
>>>> + - const: ipe_nps_fast_ahb
>>>> + - const: ipe_pps
>>>> + - const: ipe_pps_fast_ahb
>>>> + - const: jpeg
>>>> + - const: ofe_ahb
>>>> + - const: ofe_anchor
>>>> + - const: ofe_anchor_fast_ahb
>>>> + - const: ofe_hdr
>>>> + - const: ofe_hdr_fast_ahb
>>>> + - const: ofe_main
>>>> + - const: ofe_main_fast_ahb
>>>> + - const: vfe0_bayer
>>>> + - const: vfe0_bayer_fast_ahb
>>>> + - const: vfe1_bayer
>>>> + - const: vfe1_bayer_fast_ahb
>>>> + - const: vfe2_bayer
>>>> + - const: vfe2_bayer_fast_ahb
>>>> +
>>>> + interrupts:
>>>> + maxItems: 30
>>>> +
>>>> + interrupt-names:
>>>> + items:
>>>> + - const: csid0
>>>> + - const: csid1
>>>> + - const: csid2
>>>> + - const: csid_lite0
>>>> + - const: csid_lite1
>>>> + - const: csiphy0
>>>> + - const: csiphy1
>>>> + - const: csiphy2
>>>> + - const: csiphy3
>>>> + - const: csiphy4
>>>> + - const: csiphy5
>>>> + - const: vfe0
>>>> + - const: vfe1
>>>> + - const: vfe2
>>>> + - const: vfe_lite0
>>>> + - const: vfe_lite1
>>>> + - const: camnoc_nrt
>>>> + - const: camnoc_rt
>>>> + - const: icp0
>>>> + - const: icp1
>>>> + - const: jpeg_dma
>>>> + - const: jpeg_enc
>>>> + - const: rt_cdm0
>>>> + - const: rt_cdm1
>>>> + - const: rt_cdm2
>>>> + - const: rt_cdm3
>>>> + - const: rt_cdm4
>>>> + - const: tpg0
>>>> + - const: tpg1
>>>> + - const: tpg2
>>>> +
>>>> + interconnects:
>>>> + maxItems: 4
>>>> +
>>>> + interconnect-names:
>>>> + items:
>>>> + - const: ahb
>>>> + - const: hf_mnoc
>>>> + - const: sf_icp_mnoc
>>>> + - const: sf_mnoc
>>> You know... Failure to look around is a sin. What are the names of
>>> interconnects used by other devices? What do they actually describe?
>>>
>>> This is an absolute NAK.
>> Please feel free to correct me here but, a couple things.
>>
>> 1. This is consistent with
>> Documentation/devicetree/bindings/media/qcom,qcm2290-camss.yaml. no?
> I see that nobody noticed an issue with Agatti, Lemans and Monaco
> bindings (Krzysztof?)
>
> Usually interconnect names describe the blocks that are connected. Here
> are the top results of a quick git grep of interconnect names through
> arch/arm64/dts/qcom:
>
> 729 "qup-core",
> 717 "qup-config",
> 457 "qup-memory",
> 41 "usb-ddr",
> 41 "apps-usb",
> 39 "pcie-mem",
> 39 "cpu-pcie",
> 28 "sdhc-ddr",
> 28 "cpu-sdhc",
> 28 "cpu-cfg",
> 24 "mdp0-mem",
> 17 "memory",
> 14 "ufs-ddr",
> 14 "mdp1-mem",
> 14 "cpu-ufs",
> 13 "video-mem",
> 13 "gfx-mem",
>
> I hope this gives you a pointer on how to name the interconnects.
>
>> 2. If you are referring to some other targets that use, "cam_" prefix, we
>> may not need that , isn't it? If we look at these interconnects from camera
>> side, as you advised for other things like this?
> See above.
I see, so the names cam-cfg, cam-hf-mem, cam-sf-mem, cam-sf-icp-mem
should be ok?
Or the other option, go exactly like
Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml.
What would you advise?
>
>>>> +
>>>> + iommus:
>>>> + items:
>>>> + - description: VFE non-protected stream
>>>> + - description: ICP0 shared stream
>>>> + - description: ICP1 shared stream
>>>> + - description: IPE CDM non-protected stream
>>>> + - description: IPE non-protected stream
>>>> + - description: JPEG non-protected stream
>>>> + - description: OFE CDM non-protected stream
>>>> + - description: OFE non-protected stream
>>>> + - description: VFE / VFE Lite CDM non-protected stream
>>> This will map all IOMMUs to the same domain. Are you sure that this is
>>> what we want? Or do we wait for iommu-maps to be fixed?
Thanks,
Vijay.
Powered by blists - more mailing lists