Re: [PATCH v4 1/2] dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema

[Konrad Dybcio]

On 3/17/26 9:25 PM, Vijay Kumar Tumati wrote:
> 
> 
> On 3/16/2026 10:26 PM, Bryan O'Donoghue wrote:
>> On 16/03/2026 21:31, Vijay Kumar Tumati wrote:
>>> Hi Bryan,
>>>
>>> On 3/15/2026 4:52 PM, Bryan O'Donoghue wrote:
>>>> Add a base schema initially compatible with x1e80100 to describe MIPI CSI2
>>>> PHY devices.
>>>>
>>>> The hardware can support both C-PHY and D-PHY modes. 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       | 133 +++++++++ ++++++++++++
>>>>    1 file changed, 133 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..b83c2d65ebc6e
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml
>>>> @@ -0,0 +1,133 @@
>>>> +# 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
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 4
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: csiphy
>>>> +      - const: csiphy_timer
>>>> +      - const: camnoc_axi
>>>> +      - const: cpas_ahb
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  operating-points-v2:
>>>> +    maxItems: 1
>>>> +
>>>> +  power-domains:
>>>> +    items:
>>>> +      - description: TITAN TOP GDSC
>>>> +      - description: MXC or MXA voltage rail
>>> Would it be better to provision MXA or MXC as an additional optional
>>> power domain? I see 'cam_cc_cphy_rx_clk_src', the parent of all CSIPHYx
>>> clocks, need all three power domains on this chipset.
>>
>> I don't think this should be optional. Have the dts point to an "mx" power-domain and then select which one is right for a PHY MX/MXA or MXC.
>>
>> Your worst case here is some future PHY which has more or fewer PDs which is then either a special case in this file or a whole new file for that compat.
>>
> I think it is the case on x1e* as well, Bryan.
>>>> +      - description: MMCX voltage rail
>>>> +
>>>> +  power-domain-names:
>>>> +    items:
>>>> +      - const: top
>>>> +      - const: mx
>>>> +      - const: mmcx
>>>> +
>>>> +  vdda-0p8-supply:
>>>> +    description: Phandle to a 0.8V 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-0p8-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.h>
>>>> +    #include <dt-bindings/power/qcom,rpmhpd.h>
>>>> +
>>>> +    csiphy@ace4000 {
>>>> +        compatible = "qcom,x1e80100-csi2-phy";
>>>> +        reg = <0x0ace4000 0x2000>;
>>>> +        #phy-cells = <1>;
>>>> +
>>>> +        clocks = <&camcc CAM_CC_CSIPHY0_CLK>,
>>>> +                 <&camcc CAM_CC_CSI0PHYTIMER_CLK>,
>>>> +                 <&camcc CAM_CC_CAMNOC_AXI_RT_CLK>,
>>>> +                 <&camcc CAM_CC_CPAS_AHB_CLK>;
>>>> +        clock-names = "csiphy",
>>>> +                      "csiphy_timer",
>>>> +                      "camnoc_axi",
>>>> +                      "cpas_ahb";
>>> Although it's not a concern from my side, just want to be explicitly
>>> sure that everyone is happy with the clock names, just to avoid any
>>> changes later on when other modules are separated out.
>>
>> These are the names we already use in CAMSS so ... they're good enough to start from.
>>
> Sure, FYI: Dmitry, Konrad.

I'll admit I haven't yet read up on all of the background discussions that you
guys had on LKML, but *if* we're going to put the PHY under camss, the GDSC and
CPAS_AHB/CAMNOC_AXI_RT references should be unnecessary, given they're not
related strictly to this PHY itself, rather it sitting in a specific corner of
the subsystem that needs them to be active (see related:
https://lore.kernel.org/linux-arm-msm/cb2430f2-8601-4c72-af6b-10f1ff16c188@xxxxxxxxxxxxxxxx/
)

For the other names, I *think* we won't need to rely on them (i.e. only operate
the resources through PHY APIs from the V4L2 driver) and can come up with new
ones. And hence I think we can turn "csiphy" to "core" and "csiphy_timer" to
"timer" (because we really don't need to repeat the csiphy_ prefix)

Konrad