| 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: <a428f8b9-c338-4404-8dc1-da6daae37d5c@kernel.org>
Date: Fri, 14 Nov 2025 16:09:06 +0000
From: Bryan O'Donoghue <bod@...nel.org>
To: Luca Weiss <luca.weiss@...rphone.com>,
Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
Robert Foss <rfoss@...nel.org>, Todor Tomov <todor.too@...il.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>
Cc: ~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
On 14/11/2025 13:06, Luca Weiss wrote:
> Hi Vladimir,
>
> On Fri Nov 14, 2025 at 1:40 PM CET, Vladimir Zapolskiy wrote:
>> Hi Luca.
>>
>> On 11/14/25 13:15, Luca Weiss wrote:
>>> Add bindings for the Camera Subsystem on the SM6350 SoC.
>>>
>>> Signed-off-by: Luca Weiss <luca.weiss@...rphone.com>
>>> ---
>>> .../bindings/media/qcom,sm6350-camss.yaml | 349 +++++++++++++++++++++
>>> 1 file changed, 349 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>>> new file mode 100644
>>> index 000000000000..d812b5b50c05
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>>> @@ -0,0 +1,349 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/qcom,sm6350-camss.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm SM6350 Camera Subsystem (CAMSS)
>>> +
>>> +maintainers:
>>> + - Luca Weiss <luca.weiss@...rphone.com>
>>> +
>>> +description:
>>> + The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms.
>>> +
>>> +properties:
>>> + compatible:
>>> + const: qcom,sm6350-camss
>>> +
>>> + reg:
>>> + maxItems: 12
>>> +
>>> + reg-names:
>>> + items:
>>> + - const: csid0
>>> + - const: csid1
>>> + - const: csid2
>>> + - const: csid_lite
>>> + - const: csiphy0
>>> + - const: csiphy1
>>> + - const: csiphy2
>>> + - const: csiphy3
>>> + - const: vfe0
>>> + - const: vfe1
>>> + - const: vfe2
>>> + - const: vfe_lite
>>> +
>>> + clocks:
>>> + maxItems: 30
>>> +
>>> + clock-names:
>>> + items:
>>> + - const: cam_ahb_clk
>>> + - const: cam_axi
>>> + - const: soc_ahb
>>> + - const: camnoc_axi
>>> + - const: core_ahb
>>> + - const: cpas_ahb
>>> + - const: csiphy0
>>> + - const: csiphy0_timer
>>> + - const: csiphy1
>>> + - const: csiphy1_timer
>>> + - const: csiphy2
>>> + - const: csiphy2_timer
>>> + - const: csiphy3
>>> + - const: csiphy3_timer
>>> + - const: slow_ahb_src
>>> + - const: vfe0_axi
>>> + - const: vfe0
>>> + - const: vfe0_cphy_rx
>>> + - const: vfe0_csid
>>> + - const: vfe1_axi
>>> + - const: vfe1
>>> + - const: vfe1_cphy_rx
>>> + - const: vfe1_csid
>>> + - const: vfe2_axi
>>> + - const: vfe2
>>> + - const: vfe2_cphy_rx
>>> + - const: vfe2_csid
>>> + - const: vfe_lite
>>> + - const: vfe_lite_cphy_rx
>>> + - const: vfe_lite_csid
>>
>> The sorting order of this list does not follow the sorting order accepted
>> in the past.
>
> What file should I best reference?
Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml
>>
>> I'm very sorry for the vagueness, but I can not pronounce the accepted
>> sorting order name, because it triggers people.
>>
>>> +
>>> + interrupts:
>>> + maxItems: 12
>>> +
>>> + interrupt-names:
>>> + items:
>>> + - const: csid0
>>> + - const: csid1
>>> + - const: csid2
>>> + - const: csid_lite
>>> + - const: csiphy0
>>> + - const: csiphy1
>>> + - const: csiphy2
>>> + - const: csiphy3
>>> + - const: vfe0
>>> + - const: vfe1
>>> + - const: vfe2
>>> + - const: vfe_lite
>>> +
>>> + interconnects:
>>> + maxItems: 4
>>> +
>>> + interconnect-names:
>>> + items:
>>> + - const: ahb
>>> + - const: hf_mnoc
>>> + - const: sf_mnoc
>>> + - const: sf_icp_mnoc
>>
>> Please remove sf_mnoc and sf_icp_mnoc, they are not needed for enabling
>> IP to produce raw images, and one day you may use them somewhere else.
>
> Ack, will give it a try.
Disagree with this.
See the Kanaapali patches. I'm asking new submissions to be as complete
as possible, instead of limiting the hardware description to the RDI.
So listing the ICP noc is the right thing to do.
So please include register banks for
- bps
- cdm
- icp
- ipe
- jpeg
- lrme
>>
>>> +
>>> + iommus:
>>> + maxItems: 4
>>> +
>>> + power-domains:
>>> + items:
>>> + - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
>>> + - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
>>> + - description: IFE2 GDSC - Image Front End, Global Distributed Switch Controller.
>>> + - description: Titan Top GDSC - Titan ISP Block, Global Distributed Switch Controller.
>>> +
>>> + power-domain-names:
>>> + items:
>>> + - const: top
>>> + - const: ife0
>>> + - const: ife1
>>> + - const: ife2
>>
>> Note that the list of items and the list of the item descriptions do not
>> correspond to each other. Titan Top GDSC shall be at the end.
>
> In the v1 the comment was that top can now be put on top (because a
> limitation in the driver was fixed). But yes, forgot to modify
> power-domains description. Will fix.
>
>>
>>> +
>>> + vdd-csiphy-0p9-supply:
>>> + description:
>>> + Phandle to a 0.9V regulator supply to a PHY.
>>> +
>>> + vdd-csiphy-1p25-supply:
>>> + description:
>>> + Phandle to a 1.25V regulator supply to a PHY.
>>> +
>>
>> Please reference to the schematics or SoC TRM, does SM6350 SoC
>> have different pads to get supplies to different CSIPHYx IPs?
>>
>> If so, then please provide hardware properties to get a proper
>> correspondence between supplies and CSIPHYx, and make all these
>> properties optional.
>
> I shared the names in replies to v1.
>
> * VDD_CAMSS_PLL_0P9 - Camera SS PLL 0.9 V circuits
> (not referenced in downstream kernel, connected to vreg_s5a in
> schematics, which is MX)
> * VDD_A_CSI_x_0P9 - MIPI CSIx 0.9 V circuits
> With pad names VDD_A_CSI_0_0P9 to VDD_A_CSI_3_0P9
> * VDD_A_CSI_x_1P25 - MIPI CSIx 1.25 V circuits
> With pad names VDD_A_CSI_0_1P25 to VDD_A_CSI_3_1P25
I'm fine with your proposed rail names, they appear to correspond to the
voltage values in the docs.
>
>>
>>> + ports:
>>> + $ref: /schemas/graph.yaml#/properties/ports
>>> +
>>> + description:
>>> + CSI input ports.
>>> +
>>> + patternProperties:
>>> + "^port@[0-3]$":
>>> + $ref: /schemas/graph.yaml#/$defs/port-base
>>> + unevaluatedProperties: false
>>> +
>>> + description:
>>> + Input port for receiving CSI data from a CSIPHY.
>>> +
>>> + properties:
>>> + endpoint:
>>> + $ref: video-interfaces.yaml#
>>> + unevaluatedProperties: false
>>> +
>>> + properties:
>>> + data-lanes:
>>> + minItems: 1
>>> + maxItems: 4
>>> +
>>> + bus-type:
>>> + enum:
>>> + - 1 # MEDIA_BUS_TYPE_CSI2_CPHY
>>> + - 4 # MEDIA_BUS_TYPE_CSI2_DPHY
>>> +
>>> + required:
>>> + - data-lanes
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - reg-names
>>> + - clocks
>>> + - clock-names
>>> + - interrupts
>>> + - interrupt-names
>>> + - interconnects
>>> + - interconnect-names
>>> + - iommus
>>> + - power-domains
>>> + - power-domain-names
>>> + - vdd-csiphy-0p9-supply
>>> + - vdd-csiphy-1p25-supply
>>
>> When a change to add CSIPHYx specific supplies is done, please remove
>> *-supply properties from the list of the requred ones.
>
> Is this pending some other change that will be posted? Or what do you mean?
He means in the current CSIPHY dt its not possible to require these
properties.
>
>>
>>> + - ports
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/clock/qcom,gcc-sm6350.h>
>>> + #include <dt-bindings/clock/qcom,sm6350-camcc.h>
>>> + #include <dt-bindings/interconnect/qcom,icc.h>
>>> + #include <dt-bindings/interconnect/qcom,sm6350.h>
>>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> + #include <dt-bindings/media/video-interfaces.h>
>>> + #include <dt-bindings/power/qcom-rpmpd.h>
>>> +
>>> + soc {
>>> + #address-cells = <2>;
>>> + #size-cells = <2>;
>>> +
>>> + isp@...3000 {
>>> + compatible = "qcom,sm6350-camss";
>>> +
>>> + reg = <0x0 0x0acb3000 0x0 0x1000>,
>>> + <0x0 0x0acba000 0x0 0x1000>,
>>> + <0x0 0x0acc1000 0x0 0x1000>,
>>> + <0x0 0x0acc8000 0x0 0x1000>,
>>> + <0x0 0x0ac65000 0x0 0x1000>,
>>> + <0x0 0x0ac66000 0x0 0x1000>,
>>> + <0x0 0x0ac67000 0x0 0x1000>,
>>> + <0x0 0x0ac68000 0x0 0x1000>,
>>> + <0x0 0x0acaf000 0x0 0x4000>,
>>> + <0x0 0x0acb6000 0x0 0x4000>,
>>> + <0x0 0x0acbd000 0x0 0x4000>,
>>> + <0x0 0x0acc4000 0x0 0x4000>;
>>> + reg-names = "csid0",
>>> + "csid1",
>>> + "csid2",
>>> + "csid_lite",
>>> + "csiphy0",
>>> + "csiphy1",
>>> + "csiphy2",
>>> + "csiphy3",
>>> + "vfe0",
>>> + "vfe1",
>>> + "vfe2",
>>> + "vfe_lite";
>>> +
>>> + clocks = <&gcc GCC_CAMERA_AHB_CLK>,
>>
>> I believe this clock is critical, and it is set so in the SM6350 GCC driver,
>> therefore it should not be added here.
>
> True, gcc_camera_ahb_clk has CLK_IS_CRITICAL in gcc-sm6350.c
DT describes hardware, not the happenstance of Linux driver setup.
On that basis omitting <&gcc GCC_CAMERA_AHB_CLK> from the clock list is
not correct.
Because being bornign, can I then reuse this DT in FreeBSD ? No I cannot
because it won't describe hardware it will desscirbe Linux-DT which
ain't the same thing.
>>
>> Multiple CAMCC drivers define some of the clocks as "critical" and always
>> enabled, a misconfiguration in this area may cause the reported warning.
>
> Will try to remove it then.
I really object to that. DT is a hardware description. Listing the
clocks here does no harm and is factually accurate, which again is the
point of DT.
>
>>
>>> + <&gcc GCC_CAMERA_AXI_CLK>,
>>> + <&camcc CAMCC_SOC_AHB_CLK>,
>>> + <&camcc CAMCC_CAMNOC_AXI_CLK>,
>>> + <&camcc CAMCC_CORE_AHB_CLK>,
>>> + <&camcc CAMCC_CPAS_AHB_CLK>,
>>> + <&camcc CAMCC_CSIPHY0_CLK>,
>>> + <&camcc CAMCC_CSI0PHYTIMER_CLK>,
>>> + <&camcc CAMCC_CSIPHY1_CLK>,
>>> + <&camcc CAMCC_CSI1PHYTIMER_CLK>,
>>> + <&camcc CAMCC_CSIPHY2_CLK>,
>>> + <&camcc CAMCC_CSI2PHYTIMER_CLK>,
>>> + <&camcc CAMCC_CSIPHY3_CLK>,
>>> + <&camcc CAMCC_CSI3PHYTIMER_CLK>,
>>> + <&camcc CAMCC_SLOW_AHB_CLK_SRC>,
>>> + <&camcc CAMCC_IFE_0_AXI_CLK>,
>>> + <&camcc CAMCC_IFE_0_CLK>,
>>> + <&camcc CAMCC_IFE_0_CPHY_RX_CLK>,
>>> + <&camcc CAMCC_IFE_0_CSID_CLK>,
>>> + <&camcc CAMCC_IFE_1_AXI_CLK>,
>>> + <&camcc CAMCC_IFE_1_CLK>,
>>> + <&camcc CAMCC_IFE_1_CPHY_RX_CLK>,
>>> + <&camcc CAMCC_IFE_1_CSID_CLK>,
>>> + <&camcc CAMCC_IFE_2_AXI_CLK>,
>>> + <&camcc CAMCC_IFE_2_CLK>,
>>> + <&camcc CAMCC_IFE_2_CPHY_RX_CLK>,
>>> + <&camcc CAMCC_IFE_2_CSID_CLK>,
>>> + <&camcc CAMCC_IFE_LITE_CLK>,
>>> + <&camcc CAMCC_IFE_LITE_CPHY_RX_CLK>,
>>> + <&camcc CAMCC_IFE_LITE_CSID_CLK>;
>>> + clock-names = "cam_ahb_clk",
>>> + "cam_axi",
>>> + "soc_ahb",
>>> + "camnoc_axi",
>>> + "core_ahb",
>>> + "cpas_ahb",
>>> + "csiphy0",
>>> + "csiphy0_timer",
>>> + "csiphy1",
>>> + "csiphy1_timer",
>>> + "csiphy2",
>>> + "csiphy2_timer",
>>> + "csiphy3",
>>> + "csiphy3_timer",
>>> + "slow_ahb_src",
>>> + "vfe0_axi",
>>> + "vfe0",
>>> + "vfe0_cphy_rx",
>>> + "vfe0_csid",
>>> + "vfe1_axi",
>>> + "vfe1",
>>> + "vfe1_cphy_rx",
>>> + "vfe1_csid",
>>> + "vfe2_axi",
>>> + "vfe2",
>>> + "vfe2_cphy_rx",
>>> + "vfe2_csid",
>>> + "vfe_lite",
>>> + "vfe_lite_cphy_rx",
>>> + "vfe_lite_csid";
>>> +
>>> + interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 717 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 718 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>;
>>
>> Interrupt types shall be IRQ_TYPE_EDGE_RISING.
>
> Ack
>
>>
>>> + interrupt-names = "csid0",
>>> + "csid1",
>>> + "csid2",
>>> + "csid_lite",
>>> + "csiphy0",
>>> + "csiphy1",
>>> + "csiphy2",
>>> + "csiphy3",
>>> + "vfe0",
>>> + "vfe1",
>>> + "vfe2",
>>> + "vfe_lite";
>>> +
>>> + interconnects = <&gem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ACTIVE_ONLY
>>> + &config_noc SLAVE_CAMERA_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
>>> + <&mmss_noc MASTER_CAMNOC_HF QCOM_ICC_TAG_ALWAYS
>>> + &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
>>> + <&mmss_noc MASTER_CAMNOC_SF QCOM_ICC_TAG_ALWAYS
>>> + &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
>>> + <&mmss_noc MASTER_CAMNOC_ICP QCOM_ICC_TAG_ALWAYS
>>> + &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>;
>>> + interconnect-names = "ahb",
>>> + "hf_mnoc",
>>> + "sf_mnoc",
>>> + "sf_icp_mnoc";
>>> +
>>> + iommus = <&apps_smmu 0x820 0xc0>,
>>> + <&apps_smmu 0x840 0x0>,
>>> + <&apps_smmu 0x860 0xc0>,
>>> + <&apps_smmu 0x880 0x0>;
>>> +
>>> + power-domains = <&camcc TITAN_TOP_GDSC>
>>
>> It should be the last one in the list, if the settled practice is followed.
>
> See above.
>
>>
>>> + <&camcc IFE_0_GDSC>,
>>> + <&camcc IFE_1_GDSC>,
>>> + <&camcc IFE_2_GDSC>;
>>> + power-domain-names = "top",
>>> + "ife0",
>>> + "ife1",
>>> + "ife2";
>>> +
>>> + vdd-csiphy-0p9-supply = <&vreg_l18a>;
>>> + vdd-csiphy-1p25-supply = <&vreg_l22a>;
>>> +
>>> + ports {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + port@0 {
>>> + reg = <0>;
>>> + csiphy0_ep: endpoint {
>>
>> An empty line before a child node is always needed.
>
> Ack
>
>>
>>> + data-lanes = <0 1 2 3>;
>>> + bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
>>> + remote-endpoint = <&sensor_ep>;
>>> + };
>>> + };
>>> + };
>>> + };
>>> + };
>>>
>
Powered by blists - more mailing lists