Re: [PATCH v5 1/2] dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema
From: Bryan O'Donoghue
Date: Thu Mar 26 2026 - 10:56:32 EST
On 26/03/2026 10:28, Vladimir Zapolskiy wrote:
On 3/26/26 04:03, Bryan O'Donoghue wrote:
On 26/03/2026 01:46, Vladimir Zapolskiy wrote:
On 3/26/26 03:04, 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. We
capture those modes as:
- PHY_QCOM_CSI2_MODE_DPHY
- PHY_QCOM_CSI2_MODE_CPHY
- PHY_QCOM_CSI2_MODE_SPLIT_DPHY
Distinction between PHY_QCOM_CSI2_MODE_DPHY and
PHY_QCOM_CSI2_MODE_SPLIT_DPHY
is
1) insufficient in just this simplistic form, because the assignment of
particular lanes is also needed,
2) and under the assumption that the lane mapping is set somewhere else,
then
there should be no difference between PHY_QCOM_CSI2_MODE_{DPHY,SPLIT_DPHY},
it's just DPHY, and the subtype is deductible from data-lanes property on
the consumer side.
So far the rationale is unclear, why anything above regular PHY_TYPE_DPHY
and PHY_TYPE_CPHY is needed here, those two are sufficient.
Because knowing the split-mode exists and that you have asked about how
such a thing would be supported, I thought about how to represent that
mode right from the start, even if we don't support it.
It is good to think about this hardware confguration in advance, however
the process of describing such hardware setup is incomplete.
To support split phy we will need to pass the parameter.
What you call "split phy" is a DPHY, and "split phy" can not be supported
by adding this parameter, because it does not provide information about
lanes, and after removing this information it is just DPHY.
That's just not true. If you read the camx source code you can see split/combo mode 2+1 1+1 data/clock mode requires special programming of the PHY to support.
https://review.lineageos.org/c/LineageOS/android_kernel_motorola_sm6375/+/423960/1/drivers/cam_sensor_module/cam_csiphy/cam_csiphy_core.c#b285
There is disjunction all over this file depending on the mode.
https://review.lineageos.org/c/LineageOS/android_kernel_motorola_sm6375/+/423960/1/drivers/cam_sensor_module/cam_csiphy/cam_csiphy_core.c#b767
And besides, think about it - you need different init sequences if one of the lanes is clock instead of data...
If we use phy.h::PHY_TYPE_DPHY then that means to support split-mode in the future we need to get that mode represented in phy.h - but really this fixed split mode isn't a generic CSI2 PHY mode, its a Qualcommism.
Nothing wrong with that - but then the mode should reflect the fact it is vendor specific and we absolutely 100% have to do different things in the PHY driver whether we are in regular DPHY mode or in split DPHY mode.
If we use PHY_TYPE_DPHY as I did in the previous patch then we can't convert to a vendor type later on as its an ABI break.
So we have both a sound technical reason hardware will require it to differentiate between DPHY and split-mode DPHY - and we also don't want to be bound to phy.h and then try to upstream a new DPHY_SPLIT_MODE here which a reviewer might reasonably say "why is this special mode from a specific vendor driving new defines in a shared file"
You know you're right its not strictly necessary - its just there to be helpful.
So we define those parameters upfront.
This new header file has to be removed, it does not bring anything valuable.
The 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 | 130 +++++++++++
++++++++++
include/dt-bindings/phy/phy-qcom-mipi-csi2.h | 15 +++
2 files changed, 145 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..63114151104b4
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml
@@ -0,0 +1,130 @@
+# 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.
+ See include/dt-bindings/phy/phy-qcom-mipi-csi2.h for valid values.
include/dt-bindings/phy/phy.h should be good enough as it's stated above.
While include/dt-bindings/phy/phy.h provides generic definitions for
D-PHY and C-PHY, it does not contain a definition for Qualcomm's
proprietary Split D-PHY mode. Because this hardware supports a
What Qualcomm's proprietary Split D-PHY mode is manifested by lane mapping,
there is no need to introduce another PHY mode, it is DPHY.
vendor-specific operating mode, introducing a vendor-specific header to
define that state is necessary.
This is exactly what we do with the QMP to support a similar use-case -
the PHYs do vendor specific things, so we use vendor specific defines.
If we lock to phy.h CPHY/DPHY only then we exclude the possibility of
say adding split-mode to an upstream SoC as the DT ABI will not then
facilitate the mode.
+
+ clocks:
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: core
+ - const: timer
+
+ interrupts:
+ maxItems: 1
+
+ operating-points-v2:
+ maxItems: 1
+
+ power-domains:
+ items:
+ - description: MXC or MXA voltage rail
+ - description: MMCX voltage rail
+
+ power-domain-names:
+ items:
+ - const: mx
+ - const: mmcx
+
+ 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.
+
+required:
+ - compatible
+ - reg
+ - "#phy-cells"
+ - clocks
+ - clock-names
+ - interrupts
+ - operating-points-v2
+ - power-domains
+ - power-domain-names
+ - vdda-0p9-supply
+ - vdda-1p2-supply
+
+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/phy/phy-qcom-mipi-csi2.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_MX>,
+ <&rpmhpd RPMHPD_MMCX>;
+ power-domain-names = "mx",
+ "mmcx";
+
+ vdda-0p9-supply = <&vreg_l2c_0p8>;
+ vdda-1p2-supply = <&vreg_l1c_1p2>;
+ };
+
+ csiphy_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ opp-300000000 {
+ opp-hz = /bits/ 64 <300000000>;
+ required-opps = <&rpmhpd_opp_low_svs_d1>,
+ <&rpmhpd_opp_low_svs_d1>;
+ };
+
+ opp-400000000 {
+ opp-hz = /bits/ 64 <400000000>;
+ required-opps = <&rpmhpd_opp_low_svs>,
+ <&rpmhpd_opp_low_svs>;
+ };
+
+ opp-480000000 {
+ opp-hz = /bits/ 64 <480000000>;
+ required-opps = <&rpmhpd_opp_low_svs>,
+ <&rpmhpd_opp_low_svs>;
+ };
+ };
+
+ isp@acb7000 {
+ phys = <&csiphy4 PHY_QCOM_CSI2_MODE_DPHY>;
+ };
This example is incomplete in sense that it does not include CAMSS
CSIPHY IP hardware configuration in whole.
No that's not the way examples work. You don't replicate entire nodes
from other schemas you just give a terse reference.
If so, then this example makes no sense and it'd be better to remove it.
"Be less helpful" is not usually review feedback I give or take but, I'll drop this anyway.
---
bod