Re: [PATCH v8 1/2] dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema
From: Bryan O'Donoghue
Date: Wed Jun 03 2026 - 16:57:42 EST
On 03/06/2026 21:24, Vijay Kumar Tumati wrote:
On 6/3/2026 1:16 PM, Vijay Kumar Tumati wrote:
Hi,
On 6/2/2026 3:51 PM, Bryan O'Donoghue wrote:
On 02/06/2026 22:59, Vladimir Zapolskiy wrote:Either way, do we need to document the constraint of using port@0 or
On 5/23/26 05:48, Bryan O'Donoghue wrote:
Add a base schema initially compatible with x1e80100 to describe
MIPI CSI2
PHY devices.
The hardware can support both CPHY, DPHY and a special split-mode DPHY.
The schema here defines three ports:
port@0:
The first input port where a sensor is always required.
port@1:
A second optional input port which if present implies DPHY
split-mode.
port@2:
A third always required output port which connects to the
controller.
This port numeration is imperfect, because port@0 and port@2 are
required,
while middle port@1 is optional.
Like it was stated before a number of times, it seems natural to operate
with two ports, where input port may have two endpoints rather than 3
ports,
also that approach solves the problem of a hole in the port numeration.
Can you confirm this is what you are after ?
port@0 {
#address-cells = <1>;
#size-cells = <0>;
endpoint@0 { /* primary sensor */
reg = <0>;
data-lanes = <0 1 2 3>;
remote-endpoint = <&sensor0_out>;
};
endpoint@1 { /* split-mode second sensor, optional */
reg = <1>;
data-lanes = <0>;
remote-endpoint = <&sensor1_out>;
};
};
port@1 { /* output to CAMSS, was port@2 */
endpoint { remote-endpoint = <&controller_in>; };
};
This works for me BTW.
endpoint@0 'only' for the 4+1 or 2+1 mode and the other one is for the
1+1 mode? Or is it implicit from this bindings for a developer?
The binding mandates it with an if / else structure
I am not sure of this, Bryan. If you look at the PHY core clockThe CSIPHY devices have their own pinouts on the SoC as well as
their own
individual voltage rails.
The need to model voltage rails on a per-PHY basis leads us to define
CSIPHY devices as individual nodes.
Two nice outcomes in terms of schema and DT arise from this change.
1. The ability to define on a per-PHY basis voltage rails.
2. The ability to require those voltage.
We have had a complete bodge upstream for this where a single set of
voltage rail for all CSIPHYs has been buried inside of CAMSS.
Much like the I2C bus which is dedicated to Camera sensors - the CCI
bus in
CAMSS parlance, the CSIPHY devices should be individually modelled.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
---
.../bindings/phy/qcom,x1e80100-csi2-phy.yaml | 209 ++++++++
+ ++++++++++++
1 file changed, 209 insertions(+)
diff --git a/Documentation/devicetree/bindings/phy/qcom,x1e80100-
csi2-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,x1e80100-
csi2-phy.yaml
new file mode 100644
index 0000000000000..270375f949880
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml
@@ -0,0 +1,209 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/qcom,x1e80100-csi2-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm CSI2 PHY
+
+maintainers:
+ - Bryan O'Donoghue <bod@xxxxxxxxxx>
+
+description:
+ Qualcomm MIPI CSI2 C-PHY/D-PHY combination PHY. Connects MIPI
CSI2 sensors
+ to Qualcomm's Camera CSI Decoder. The PHY supports both C-PHY and
D-PHY
+ modes.
+
+properties:
+ compatible:
+ const: qcom,x1e80100-csi2-phy
+
+ reg:
+ maxItems: 1
+
+ "#phy-cells":
+ const: 1
+ description:
+ The single cell specifies the PHY operating mode.
#phy-cells should be 0, because the PHY operating mode is well defined
by 'bus-type' property of an endpoint on the sensor side, the opposite
side of CAMSS/CSID as a CSIPHY "consumer" should not dictate the PHY
type.
Rob said consumer but, I'm also not very bothered about that. bus-type
is perfectly acceptable to me.
+
+ clocks:
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: core
+ - const: timer
+
+ interrupts:
+ maxItems: 1
+
+ operating-points-v2:
+ maxItems: 1
+
+ power-domains:
+ items:
+ - description: MMCX voltage rail
+ - description: MXC or MXA voltage rail
Only "qcom,x1e80100-csi2-phy" device is supported so far, unlikely it's
the case that "MXC or MXA voltage rail" should be specified, it'd be
just one of two or both.
Hmm. I'm not being clear here if this is your take, I will reword it
to make it clearer this generation of PHY _must_ have either
- MMCX and MXC
or
- MMCX and MXA
separately, sure, that is correct. But all of them, on this platform as
well, share the RCG, which requires all 3 power domains. So
fundamentally, you need to enable all of those from each PHY. You can
make it constant 3 power domains.>
Hmm do you mean the GDSC which I omitted form the example and shouldn't have TITAN_TOP_GDSC or do you mean MMCX, MXC and MXA are required ?
I don't believe the clock definitions say that. Also not what you said in the previous cycle.
I'd be obliged if you could be precise and clear since as you know the PHY as a separate thing is important to release new SoC additions.
Actually, one more thing, Why isn't TITAN TOP GDSC here?>>>> ++
+ power-domain-names:
+ items:
+ - const: mmcx
+ - const: mx
+
+ vdda-0p9-supply:
+ description: Phandle to a 0.9V regulator supply to a PHY.
+
+ vdda-1p2-supply:
+ description: Phandle to 1.2V regulator supply to a PHY.
+
+ ports:
+ $ref: /schemas/graph.yaml#/properties/ports
+
+ properties:
+ port@0:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ description: Sensor input. Always present.
+ unevaluatedProperties: false
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ unevaluatedProperties: false
+ properties:
+ data-lanes:
+ minItems: 1
+ maxItems: 4
+ clock-lanes:
+ maxItems: 1
+ remote-endpoint: true
+ required:
+ - data-lanes
+ - remote-endpoint
+
+ port@1:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ description:
+ Second sensor input. When present, indicates DPHY split
mode.
+ unevaluatedProperties: false
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ unevaluatedProperties: false
+ properties:
+ data-lanes:
+ maxItems: 1
+ clock-lanes:
+ maxItems: 1
+ remote-endpoint: true
+ required:
+ - data-lanes
+ - clock-lanes
+ - remote-endpoint
As it's stated above, it should be converted to a single port with two
endpoints, it'd be done in accordance to video-interfaces.yaml.
+
+ port@2:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ description: Output to CAMSS controller.
+ unevaluatedProperties: false
+
+ properties:
+ endpoint:
+ $ref: /schemas/graph.yaml#/$defs/endpoint-base
+ unevaluatedProperties: false
+ properties:
+ remote-endpoint: true
+ required:
+ - remote-endpoint
+
+ required:
+ - port@0
+ - port@2
+
+required:
+ - compatible
+ - reg
+ - "#phy-cells"
+ - clocks
+ - clock-names
+ - interrupts
+ - operating-points-v2
+ - power-domains
+ - power-domain-names
+ - vdda-0p9-supply
+ - vdda-1p2-supply
+ - ports
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/qcom,x1e80100-camcc.h>
+ #include <dt-bindings/clock/qcom,x1e80100-gcc.h>
+ #include <dt-bindings/power/qcom,rpmhpd.h>
+
+ csiphy4: csiphy@ace4000 {
+ compatible = "qcom,x1e80100-csi2-phy";
+ reg = <0x0ace4000 0x2000>;
+ #phy-cells = <1>;
+
+ clocks = <&camcc CAM_CC_CSIPHY0_CLK>,
+ <&camcc CAM_CC_CSI0PHYTIMER_CLK>;
+ clock-names = "core",
+ "timer";
+
+ operating-points-v2 = <&csiphy_opp_table>;
+
+ interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>;
+
+ power-domains = <&rpmhpd RPMHPD_MMCX>,
+ <&rpmhpd RPMHPD_MX>;
+ power-domain-names = "mmcx",
+ "mx";
Yes the DTSI has TITAN_TOP_GDSC I haven't updated the YAML to capture that.
So it should be
top
mmcx
mx
With obviously on mmcx and mx scalable. We established that CSIPHY4 had MXA whereas the other CSIPHYs had MXC in v5 or v4 - can you be clear if you agreeing with that still or saying something different. Per my memory of reading the docs, there was nothing in the clock tree to indicate both MXA and MXC were required for all PHYs.
I wonder why you would have only one clock here. You should be setting+ vdda-0p9-supply = <&vreg_l2c_0p8>;
+ vdda-1p2-supply = <&vreg_l1c_1p2>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ csiphy0_in_ep: endpoint {
+ data-lanes = <0 1>;
+ clock-lanes = <2>;
+ remote-endpoint = <&sensor_out>;
+ };
+ };
+
+ port@2 {
+ reg = <2>;
+ csiphy0_out_ep: endpoint {
+ remote-endpoint = <&controller_in>;
+ };
+ };
+ };
+ };
+
+ csiphy_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ opp-300000000 {
+ opp-hz = /bits/ 64 <300000000>;
the rate for both the core and timer, isn't it?
Yes the dtsi has it, the example does not. I had pushback from others about the example being too complex - you can't please all of the people all of the time.
I will drop the full table @ v9
+ required-opps =<&rpmhpd_opp_low_svs_d1>,
Same here, it should 3 power domains set.>>> + };+ <&rpmhpd_opp_low_svs_d1>;
Two power domains scaled, one set at least I hope that's what you mean i.e. add the GDSC, already in my code I just didn't add it here as I should have.
- GDSC enabled
- MMCX scaled
- MX scaled
When MX points to MXA the scaling is a NOP @ rpmhpd_opp_low_svs_d1.
Agreed ?
Why is one at svs and the other at svs_d1? Shouldn't both be svs?>>>+
+ opp-400000000 {
+ opp-hz = /bits/ 64 <400000000>;
+ required-opps = <&rpmhpd_opp_low_svs>,
+ <&rpmhpd_opp_low_svs_d1>;
+ };
And here, both should be svs_l1?>>> + };+
+ opp-480000000 {
+ opp-hz = /bits/ 64 <480000000>;
+ required-opps = <&rpmhpd_opp_low_svs>,
+ <&rpmhpd_opp_low_svs_d1>;
Thanks,+ };
--
Best wishes,
Vladimir
Vijay.