[PATCH 0/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver

[PATCH 0/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver

[Bryan O'Donoghue]

This short series adds a CSI2 MIPI PHY driver, initially supporting D-PHY
mode. The core logic and init sequences come directly from CAMSS and are
working on at least five separate x1e devices.

The rationale to instantiate CSI2 PHYs as standalone devices instead of as
sub-nodes of CAMSS is as follows.

1. Precedence
   CAMSS has a dedicated I2C bus called CCI Camera Control Interface.
   We model this controller as its own separate device in devicetree.
   This makes sense and CCI/I2C is a well defined bus type already modelled
   in Linux.

   MIPI CSI2 PHY devices similarly fit into a well defined separate
   bus/device structure.

   Contrast to another CAMSS component such as VFE, CSID or TPG these
   components only interact with other CAMSS inputs/outputs unlike CSIPHY
   which interacts with non-SoC components.

2. Hardware pinouts and rails
   The CSI2 PHY has its own data/clock lanes out from the SoC and indeed
   has its own incoming power-rails.

3. Other devicetree schemas
   There are several examples throughout the kernel of CSI PHYs modeled as
   standalone devices which one assumes follows the same reasoning as given
   above.

I've been working on this on-and-off since the end of April:
Link: https://lore.kernel.org/linux-media/c5cf0155-f839-4db9-b865-d39b56bb1e0a@linaro.org

There is another proposal to have the PHYs be subdevices of CAMSS but, I
believe we should go with a "full fat" PHY to match best practices in
drivers/phy/qualcomm/*.

Using the standard PHY API and the parameter passing that goes with it
allows us to move away from custom interfaces in CAMSS and to conform more
clearly to established PHY paradigms such as the QMP combo PHY.

Looking at existing compat strings I settled on
"qcom,x1e80100-mipi-csi2-combo-phy" deliberately omitting reference to the
fact the PHY is built on a four nano-meter process node, which seems to
match recent submissions to QMP PHY.

My first pass at this driver included support for the old two phase
devices:

Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/a504c28d109296c93470340cfe7281231f573bcb#b6e59ed7db94c9da22e492bb03fcda6a4300983c

I realised that the device tree schema changes required to support a
comprehensive conversion of all CAMSS to this driver would be an
almost certainly be unacceptable ABI break or at the very least an enormous
amount of work and verification so I instead aimed to support just one new
SoC in the submission.

I've retained the callback indirections give us scope to add in another type of
future PHY including potentially adding in the 2PH later on.

This driver is tested and working on x1e/Hamoa and has been tested as not
breaking sc8280xp/Makena and sm8250/Kona.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
Bryan O'Donoghue (2):
      dt-bindings: phy: qcom: Add MIPI CSI2 C-PHY/DPHY Combo schema
      phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

 .../phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml     |  95 ++++
 MAINTAINERS                                        |  11 +
 drivers/phy/qualcomm/Kconfig                       |  11 +
 drivers/phy/qualcomm/Makefile                      |   6 +
 drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c | 491 +++++++++++++++++++++
 drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c     | 281 ++++++++++++
 drivers/phy/qualcomm/phy-qcom-mipi-csi2.h          | 101 +++++
 7 files changed, 996 insertions(+)
---
base-commit: 2b0b621d5db55cf01bb200e6e6976b4ff4810544
change-id: 20250710-x1e-csi2-phy-f6434b651d3a

Best regards,
-- 
Bryan O'Donoghue <bryan.odonoghue@linaro.org>

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

[Bryan O'Donoghue]

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@linaro.org>
---
 .../phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml     | 95 ++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..e0976f012516452ae3632ff4732620b5c5402d3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm MIPI CSI2 Combo PHY
+
+maintainers:
+  - Bryan O'Donoghue <bod@kernel.org>
+
+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-mipi-csi2-combo-phy
+
+  reg:
+    maxItems: 1
+
+  "#phy-cells":
+    const: 0
+
+  clocks:
+    maxItems: 4
+
+  clock-names:
+    items:
+      - const: camnoc_axi
+      - const: cpas_ahb
+      - const: csiphy
+      - const: csiphy_timer
+
+  interrupts:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  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.
+
+  phy-type:
+    description: D-PHY or C-PHY mode
+    enum: [ 10, 11 ]
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - reg
+  - "#phy-cells"
+  - clocks
+  - clock-names
+  - vdda-0p8-supply
+  - vdda-1p2-supply
+  - phy-type
+
+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>
+
+    csiphy0: csiphy@ace4000 {
+        compatible = "qcom,x1e80100-mipi-csi2-combo-phy";
+        reg = <0x0ace4000 0x2000>;
+        #phy-cells = <0>;
+
+        clocks = <&camcc CAM_CC_CAMNOC_AXI_RT_CLK>,
+                 <&camcc CAM_CC_CPAS_AHB_CLK>,
+                 <&camcc CAM_CC_CSIPHY0_CLK>,
+                 <&camcc CAM_CC_CSI0PHYTIMER_CLK>;
+        clock-names = "camnoc_axi",
+                      "cpas_ahb",
+                      "csiphy",
+                      "csiphy_timer";
+
+        interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>;
+
+        power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
+
+        vdda-0p8-supply = <&vreg_l2c_0p8>;
+        vdda-1p2-supply = <&vreg_l1c_1p2>;
+
+        phy-type = <PHY_TYPE_DPHY>;
+    };

-- 
2.49.0

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

[Rob Herring]

On Thu, Jul 10, 2025 at 05:16:47PM +0100, 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@linaro.org>
> ---
>  .../phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml     | 95 ++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..e0976f012516452ae3632ff4732620b5c5402d3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm MIPI CSI2 Combo PHY
> +
> +maintainers:
> +  - Bryan O'Donoghue <bod@kernel.org>
> +
> +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-mipi-csi2-combo-phy

Kind of long. CSI2 implies MIPI and is there a non-combo phy for the 
SoC? Could drop either or both mipi and combo...

> +
> +  reg:
> +    maxItems: 1
> +
> +  "#phy-cells":
> +    const: 0
> +
> +  clocks:
> +    maxItems: 4
> +
> +  clock-names:
> +    items:
> +      - const: camnoc_axi
> +      - const: cpas_ahb

These look like the source is included in the name. Is there more than 1 
AXI and AHB bus for this device?

> +      - const: csiphy
> +      - const: csiphy_timer

Module clocks should probably come first.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  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.
> +
> +  phy-type:
> +    description: D-PHY or C-PHY mode
> +    enum: [ 10, 11 ]
> +    $ref: /schemas/types.yaml#/definitions/uint32

Perhaps putting this in phy cells would be better because the consumer 
decides on the mode.

> +
> +required:
> +  - compatible
> +  - reg
> +  - "#phy-cells"
> +  - clocks
> +  - clock-names
> +  - vdda-0p8-supply
> +  - vdda-1p2-supply
> +  - phy-type
> +
> +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>
> +
> +    csiphy0: csiphy@ace4000 {

Drop unused labels.

> +        compatible = "qcom,x1e80100-mipi-csi2-combo-phy";
> +        reg = <0x0ace4000 0x2000>;
> +        #phy-cells = <0>;
> +
> +        clocks = <&camcc CAM_CC_CAMNOC_AXI_RT_CLK>,
> +                 <&camcc CAM_CC_CPAS_AHB_CLK>,
> +                 <&camcc CAM_CC_CSIPHY0_CLK>,
> +                 <&camcc CAM_CC_CSI0PHYTIMER_CLK>;
> +        clock-names = "camnoc_axi",
> +                      "cpas_ahb",
> +                      "csiphy",
> +                      "csiphy_timer";
> +
> +        interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>;
> +
> +        power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +
> +        vdda-0p8-supply = <&vreg_l2c_0p8>;
> +        vdda-1p2-supply = <&vreg_l1c_1p2>;
> +
> +        phy-type = <PHY_TYPE_DPHY>;
> +    };
> 
> -- 
> 2.49.0
> 

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

[Vladimir Zapolskiy]

On 7/10/25 19:16, 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@linaro.org>
> ---
>   .../phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml     | 95 ++++++++++++++++++++++
>   1 file changed, 95 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..e0976f012516452ae3632ff4732620b5c5402d3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm MIPI CSI2 Combo PHY
> +
> +maintainers:
> +  - Bryan O'Donoghue <bod@kernel.org>
> +
> +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-mipi-csi2-combo-phy
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#phy-cells":
> +    const: 0
> +
> +  clocks:
> +    maxItems: 4
> +
> +  clock-names:
> +    items:
> +      - const: camnoc_axi
> +      - const: cpas_ahb
> +      - const: csiphy
> +      - const: csiphy_timer
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  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.
> +
> +  phy-type:
> +    description: D-PHY or C-PHY mode
> +    enum: [ 10, 11 ]
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#phy-cells"
> +  - clocks
> +  - clock-names
> +  - vdda-0p8-supply
> +  - vdda-1p2-supply
> +  - phy-type
> +
> +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>
> +
> +    csiphy0: csiphy@ace4000 {
> +        compatible = "qcom,x1e80100-mipi-csi2-combo-phy";
> +        reg = <0x0ace4000 0x2000>;
> +        #phy-cells = <0>;
> +
> +        clocks = <&camcc CAM_CC_CAMNOC_AXI_RT_CLK>,
> +                 <&camcc CAM_CC_CPAS_AHB_CLK>,
> +                 <&camcc CAM_CC_CSIPHY0_CLK>,
> +                 <&camcc CAM_CC_CSI0PHYTIMER_CLK>;
> +        clock-names = "camnoc_axi",
> +                      "cpas_ahb",
> +                      "csiphy",
> +                      "csiphy_timer";
> +
> +        interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>;
> +
> +        power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +
> +        vdda-0p8-supply = <&vreg_l2c_0p8>;
> +        vdda-1p2-supply = <&vreg_l1c_1p2>;
> +
> +        phy-type = <PHY_TYPE_DPHY>;
> +    };
> 

There is no ports at all, which makes the device tree node unusable,
since you can not provide a way to connect any sensors to the phy.

--
Best wishes,
Vladimir

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

[Bryan O'Donoghue]

On 14/07/2025 15:13, Vladimir Zapolskiy wrote:
> 
> There is no ports at all, which makes the device tree node unusable,
> since you can not provide a way to connect any sensors to the phy.

data-ports should go from sensor to the consumer of the data camss/csid 
not to the PHY.

Documentation/devicetree/bindings/phy/rockchip-inno-csi-dphy.yaml
Documentation/devicetree/bindings/phy/mediatek,mt8365-csi-rx.yaml

https://lore.kernel.org/linux-media/20240220-rk3568-vicap-v9-12-ace1e5cc4a82@collabora.com/

---
bod

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

[Vladimir Zapolskiy]

On 7/14/25 17:42, Bryan O'Donoghue wrote:
> On 14/07/2025 15:13, Vladimir Zapolskiy wrote:
>>
>> There is no ports at all, which makes the device tree node unusable,
>> since you can not provide a way to connect any sensors to the phy.
> 
> data-ports should go from sensor to the consumer of the data camss/csid
> not to the PHY.

No, this is an invalid description of links between hardware IPs, and
this mistake shall not be copied to CSIPHY/CAMSS.

> Documentation/devicetree/bindings/phy/rockchip-inno-csi-dphy.yaml
> Documentation/devicetree/bindings/phy/mediatek,mt8365-csi-rx.yaml
> 
> https://lore.kernel.org/linux-media/20240220-rk3568-vicap-v9-12-ace1e5cc4a82@collabora.com/
> 

-- 
Best wishes,
Vladimir

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

[Bryan O'Donoghue]

On 15/07/2025 07:40, Vladimir Zapolskiy wrote:
>>>
>>> There is no ports at all, which makes the device tree node unusable,
>>> since you can not provide a way to connect any sensors to the phy.
>>
>> data-ports should go from sensor to the consumer of the data camss/csid
>> not to the PHY.
> 
> No, this is an invalid description of links between hardware IPs, and
> this mistake shall not be copied to CSIPHY/CAMSS.

Why is it an invalid description ?

https://lore.kernel.org/linux-media/20240220-rk3568-vicap-v9-12-ace1e5cc4a82@collabora.com/

TBH I took a look at the rockchip stuff and said "nice, excellent, let's 
do it that way".

I'm happy to shamelessly copy their stuff.

---
bod

[PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Bryan O'Donoghue]

Add a new MIPI CSI2 driver in D-PHY mode initially. The entire set of
existing CAMSS CSI PHY init sequences are imported in order to save time
and effort in later patches.

In-line with other PHY drivers the process node name is omitted from the
compat string while the soc name is included.

At the moment we follow the assignment of lane positions - the bitmap of
physical input lanes to logical lane numbers as a linear list per the
existing DPHY @lanes data-member.

This is fine for us in upstream since we also map the lanes contiguously
but, our hardware can support different lane mappings so we should in the
future extend out the DPHY structure to capture the mapping.

The Qualcomm 3PH class of PHYs can do both D-PHY and C-PHY mode. For now only
D-PHY is supported.

In porting some of the logic over from camss-csiphy*.c to here its also
possible to rationalise some of the code.

In particular use of regulator_bulk and clk_bulk as well as dropping the
seemingly useless and unused interrupt handler.

The PHY sequences and a lot of the logic that goes with them are well proven
in CAMSS and mature so the main thing to watch out for here is how to get
the right sequencing of regulators, clocks and register-writes.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 MAINTAINERS                                        |  11 +
 drivers/phy/qualcomm/Kconfig                       |  11 +
 drivers/phy/qualcomm/Makefile                      |   6 +
 drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c | 491 +++++++++++++++++++++
 drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c     | 281 ++++++++++++
 drivers/phy/qualcomm/phy-qcom-mipi-csi2.h          | 101 +++++
 6 files changed, 901 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1ef99240a57ed1ad0d4501998970c7c3b85d3b81..69519e2d6dfb65771a3245735283645bb50a249a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20536,6 +20536,17 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/media/qcom,*-iris.yaml
 F:	drivers/media/platform/qcom/iris/
 
+QUALCOMM MIPI CSI2 PHY DRIVER
+M:	Bryan O'Donoghue <bod@kernel.org>
+L:	linux-phy@lists.infradead.org
+L:	linux-media@vger.kernel.org
+L:	linux-arm-msm@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml
+F:	drivers/phy/qualcomm/phy-qcom-mipi-csi2*.c
+F:	drivers/phy/qualcomm/phy-qcom-mipi-csi2*.h
+F:	include/dt-bindings/phy/phy-qcom-mipi-csi2*
+
 QUALCOMM NAND CONTROLLER DRIVER
 M:	Manivannan Sadhasivam <mani@kernel.org>
 L:	linux-mtd@lists.infradead.org
diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
index ef14f4e33973cff4103d8ea3b07cfd62d344e450..d0ab70827519c2b046d0fb03c14bb4d8ae2ec9a1 100644
--- a/drivers/phy/qualcomm/Kconfig
+++ b/drivers/phy/qualcomm/Kconfig
@@ -28,6 +28,17 @@ config PHY_QCOM_EDP
 	  Enable this driver to support the Qualcomm eDP PHY found in various
 	  Qualcomm chipsets.
 
+config PHY_QCOM_MIPI_CSI2
+	tristate "Qualcomm MIPI CSI2 PHY driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on OF
+	depends on COMMON_CLK
+	select GENERIC_PHY
+	select GENERIC_PHY_MIPI_DPHY
+	help
+	  Enable this to support the MIPI CSI2 PHY driver found in various
+	  Qualcomm chipsets.
+
 config PHY_QCOM_IPQ4019_USB
 	tristate "Qualcomm IPQ4019 USB PHY driver"
 	depends on OF && (ARCH_QCOM || COMPILE_TEST)
diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
index 3851e28a212d4a677a5b41805868f38b9ab49841..67013d27cb0387b9d65dcbe030ea6e5eaaabbe91 100644
--- a/drivers/phy/qualcomm/Makefile
+++ b/drivers/phy/qualcomm/Makefile
@@ -5,6 +5,12 @@ obj-$(CONFIG_PHY_QCOM_EDP)		+= phy-qcom-edp.o
 obj-$(CONFIG_PHY_QCOM_IPQ4019_USB)	+= phy-qcom-ipq4019-usb.o
 obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)	+= phy-qcom-ipq806x-sata.o
 obj-$(CONFIG_PHY_QCOM_M31_USB)		+= phy-qcom-m31.o
+
+phy-qcom-mipi-csi2-objs			+= phy-qcom-mipi-csi2-core.o \
+					   phy-qcom-mipi-csi2-3ph-dphy.o
+
+obj-$(CONFIG_PHY_QCOM_MIPI_CSI2)	+= phy-qcom-mipi-csi2.o
+
 obj-$(CONFIG_PHY_QCOM_PCIE2)		+= phy-qcom-pcie2.o
 
 obj-$(CONFIG_PHY_QCOM_QMP_COMBO)	+= phy-qcom-qmp-combo.o phy-qcom-qmp-usbc.o
diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
new file mode 100644
index 0000000000000000000000000000000000000000..1a99efee88cc94ec0d29a335cd29f88af8a00c02
--- /dev/null
+++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
@@ -0,0 +1,491 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * camss-phy_qcom_mipi_csi2-3ph-1-0.c
+ *
+ * Qualcomm MSM Camera Subsystem - CSIPHY Module 3phase v1.0
+ *
+ * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2016-2025 Linaro Ltd.
+ */
+#define DEBUG
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+
+#include "phy-qcom-mipi-csi2.h"
+
+#define CSIPHY_3PH_LNn_CFG1(n)				(0x000 + 0x100 * (n))
+#define CSIPHY_3PH_LNn_CFG1_SWI_REC_DLY_PRG		(BIT(7) | BIT(6))
+#define CSIPHY_3PH_LNn_CFG2(n)				(0x004 + 0x100 * (n))
+#define CSIPHY_3PH_LNn_CFG2_LP_REC_EN_INT		BIT(3)
+#define CSIPHY_3PH_LNn_CFG3(n)				(0x008 + 0x100 * (n))
+#define CSIPHY_3PH_LNn_CFG4(n)				(0x00c + 0x100 * (n))
+#define CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS		0xa4
+#define CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS_660		0xa5
+#define CSIPHY_3PH_LNn_CFG5(n)				(0x010 + 0x100 * (n))
+#define CSIPHY_3PH_LNn_CFG5_T_HS_DTERM			0x02
+#define CSIPHY_3PH_LNn_CFG5_HS_REC_EQ_FQ_INT		0x50
+#define CSIPHY_3PH_LNn_TEST_IMP(n)			(0x01c + 0x100 * (n))
+#define CSIPHY_3PH_LNn_TEST_IMP_HS_TERM_IMP		0xa
+#define CSIPHY_3PH_LNn_MISC1(n)				(0x028 + 0x100 * (n))
+#define CSIPHY_3PH_LNn_MISC1_IS_CLKLANE			BIT(2)
+#define CSIPHY_3PH_LNn_CFG6(n)				(0x02c + 0x100 * (n))
+#define CSIPHY_3PH_LNn_CFG6_SWI_FORCE_INIT_EXIT		BIT(0)
+#define CSIPHY_3PH_LNn_CFG7(n)				(0x030 + 0x100 * (n))
+#define CSIPHY_3PH_LNn_CFG7_SWI_T_INIT			0x2
+#define CSIPHY_3PH_LNn_CFG8(n)				(0x034 + 0x100 * (n))
+#define CSIPHY_3PH_LNn_CFG8_SWI_SKIP_WAKEUP		BIT(0)
+#define CSIPHY_3PH_LNn_CFG8_SKEW_FILTER_ENABLE		BIT(1)
+#define CSIPHY_3PH_LNn_CFG9(n)				(0x038 + 0x100 * (n))
+#define CSIPHY_3PH_LNn_CFG9_SWI_T_WAKEUP		0x1
+#define CSIPHY_3PH_LNn_CSI_LANE_CTRL15(n)		(0x03c + 0x100 * (n))
+#define CSIPHY_3PH_LNn_CSI_LANE_CTRL15_SWI_SOT_SYMBOL	0xb8
+
+#define CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(offset, n)	((offset) + 0x4 * (n))
+#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE	BIT(7)
+#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B	BIT(0)
+#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID	BIT(1)
+#define CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(offset, n)	((offset) + 0xb0 + 0x4 * (n))
+
+#define CSIPHY_DEFAULT_PARAMS				0
+#define CSIPHY_LANE_ENABLE				1
+#define CSIPHY_SETTLE_CNT_LOWER_BYTE			2
+#define CSIPHY_SETTLE_CNT_HIGHER_BYTE			3
+#define CSIPHY_DNP_PARAMS				4
+#define CSIPHY_2PH_REGS					5
+#define CSIPHY_3PH_REGS					6
+#define CSIPHY_SKEW_CAL					7
+
+/* 4nm 2PH v 2.1.2 2p5Gbps 4 lane DPHY mode */
+static const struct
+mipi_csi2phy_lane_regs lane_regs_x1e80100[] = {
+	/* Power up lanes 2ph mode */
+	{0x1014, 0xD5, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x101C, 0x7A, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x1018, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
+
+	{0x0094, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x00A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0090, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0098, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0094, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
+	{0x0030, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0000, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0038, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x002C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0034, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x001C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0014, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x003C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0004, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0020, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0008, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
+	{0x0010, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0094, 0xD7, 0x00, CSIPHY_SKEW_CAL},
+	{0x005C, 0x00, 0x00, CSIPHY_SKEW_CAL},
+	{0x0060, 0xBD, 0x00, CSIPHY_SKEW_CAL},
+	{0x0064, 0x7F, 0x00, CSIPHY_SKEW_CAL},
+
+	{0x0E94, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0EA0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E90, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E98, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E94, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
+	{0x0E30, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E28, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E00, 0x80, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E0C, 0xFF, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E38, 0x1F, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E2C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E34, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E1C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E14, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E3C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E04, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E20, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0E08, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
+	{0x0E10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
+
+	{0x0494, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x04A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0490, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0498, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0494, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
+	{0x0430, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0400, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0438, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x042C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0434, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x041C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0414, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x043C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0404, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0420, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0408, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
+	{0x0410, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0494, 0xD7, 0x00, CSIPHY_SKEW_CAL},
+	{0x045C, 0x00, 0x00, CSIPHY_SKEW_CAL},
+	{0x0460, 0xBD, 0x00, CSIPHY_SKEW_CAL},
+	{0x0464, 0x7F, 0x00, CSIPHY_SKEW_CAL},
+
+	{0x0894, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x08A0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0890, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0898, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0894, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
+	{0x0830, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0800, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0838, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x082C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0834, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x081C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0814, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x083C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0804, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0820, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0808, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
+	{0x0810, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0894, 0xD7, 0x00, CSIPHY_SKEW_CAL},
+	{0x085C, 0x00, 0x00, CSIPHY_SKEW_CAL},
+	{0x0860, 0xBD, 0x00, CSIPHY_SKEW_CAL},
+	{0x0864, 0x7F, 0x00, CSIPHY_SKEW_CAL},
+
+	{0x0C94, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0CA0, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C90, 0x0f, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C98, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C94, 0x07, 0x01, CSIPHY_DEFAULT_PARAMS},
+	{0x0C30, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C00, 0x8E, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C38, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C2C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C34, 0x0F, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C1C, 0x0A, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C14, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C3C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C04, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C20, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C08, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
+	{0x0C10, 0x52, 0x00, CSIPHY_DEFAULT_PARAMS},
+	{0x0C94, 0xD7, 0x00, CSIPHY_SKEW_CAL},
+	{0x0C5C, 0x00, 0x00, CSIPHY_SKEW_CAL},
+	{0x0C60, 0xBD, 0x00, CSIPHY_SKEW_CAL},
+	{0x0C64, 0x7F, 0x00, CSIPHY_SKEW_CAL},
+};
+
+static inline const struct mipi_csi2phy_device_regs *
+csi2phy_dev_to_regs(const struct mipi_csi2phy_device *csi2phy)
+{
+	return &csi2phy->soc_cfg->reg_info;
+}
+
+static void phy_qcom_mipi_csi2_hw_version_read(struct mipi_csi2phy_device *csi2phy)
+{
+	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
+	u32 hw_version;
+
+	writel(CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID, csi2phy->base +
+	       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));
+
+	hw_version = readl_relaxed(csi2phy->base +
+				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 12));
+	hw_version |= readl_relaxed(csi2phy->base +
+				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 13)) << 8;
+	hw_version |= readl_relaxed(csi2phy->base +
+				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 14)) << 16;
+	hw_version |= readl_relaxed(csi2phy->base +
+				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 15)) << 24;
+
+	csi2phy->hw_version = hw_version;
+
+	dev_dbg(csi2phy->dev, "CSIPHY 3PH HW Version = 0x%08x\n", hw_version);
+}
+
+/*
+ * phy_qcom_mipi_csi2_reset - Perform software reset on CSIPHY module
+ * @phy_qcom_mipi_csi2: CSIPHY device
+ */
+static void phy_qcom_mipi_csi2_reset(struct mipi_csi2phy_device *csi2phy)
+{
+	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
+
+	writel_relaxed(0x1, csi2phy->base +
+		      CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 0));
+	usleep_range(5000, 8000);
+	writel_relaxed(0x0, csi2phy->base +
+		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 0));
+}
+
+static irqreturn_t phy_qcom_mipi_csi2_isr(int irq, void *dev)
+{
+	const struct mipi_csi2phy_device *csi2phy = dev;
+	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
+	int i;
+
+	for (i = 0; i < 11; i++) {
+		int c = i + 22;
+		u8 val = readl_relaxed(csi2phy->base +
+				       CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, i));
+
+		writel_relaxed(val, csi2phy->base +
+			       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, c));
+	}
+
+	writel_relaxed(0x1, csi2phy->base +
+		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 10));
+	writel_relaxed(0x0, csi2phy->base +
+		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 10));
+
+	for (i = 22; i < 33; i++) {
+		writel_relaxed(0x0, csi2phy->base +
+			       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, i));
+	}
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * phy_qcom_mipi_csi2_settle_cnt_calc - Calculate settle count value
+ *
+ * Helper function to calculate settle count value. This is
+ * based on the CSI2 T_hs_settle parameter which in turn
+ * is calculated based on the CSI2 transmitter link frequency.
+ *
+ * Return settle count value or 0 if the CSI2 link frequency
+ * is not available
+ */
+static u8 phy_qcom_mipi_csi2_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
+{
+	u32 ui; /* ps */
+	u32 timer_period; /* ps */
+	u32 t_hs_prepare_max; /* ps */
+	u32 t_hs_settle; /* ps */
+	u8 settle_cnt;
+
+	if (link_freq <= 0)
+		return 0;
+
+	ui = div_u64(1000000000000LL, link_freq);
+	ui /= 2;
+	t_hs_prepare_max = 85000 + 6 * ui;
+	t_hs_settle = t_hs_prepare_max;
+
+	timer_period = div_u64(1000000000000LL, timer_clk_rate);
+	settle_cnt = t_hs_settle / timer_period - 6;
+
+	return settle_cnt;
+}
+
+static void phy_qcom_mipi_csi2_gen1_config_lanes(struct mipi_csi2phy_device *csi2phy,
+						 struct mipi_csi2phy_stream_cfg *cfg,
+						 u8 settle_cnt)
+{
+	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
+	struct mipi_csi2phy_lanes_cfg *lane_cfg = &cfg->lane_cfg;
+	int i, l = 0;
+	u8 val;
+
+	for (i = 0; i <= cfg->num_data_lanes; i++) {
+		if (i == cfg->num_data_lanes)
+			l = 7;
+		else
+			l = lane_cfg->data[i].pos * 2;
+
+		val = CSIPHY_3PH_LNn_CFG1_SWI_REC_DLY_PRG;
+		val |= 0x17;
+		writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_CFG1(l));
+
+		val = CSIPHY_3PH_LNn_CFG2_LP_REC_EN_INT;
+		writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_CFG2(l));
+
+		val = settle_cnt;
+		writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_CFG3(l));
+
+		val = CSIPHY_3PH_LNn_CFG5_T_HS_DTERM |
+			CSIPHY_3PH_LNn_CFG5_HS_REC_EQ_FQ_INT;
+		writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_CFG5(l));
+
+		val = CSIPHY_3PH_LNn_CFG6_SWI_FORCE_INIT_EXIT;
+		writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_CFG6(l));
+
+		val = CSIPHY_3PH_LNn_CFG7_SWI_T_INIT;
+		writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_CFG7(l));
+
+		val = CSIPHY_3PH_LNn_CFG8_SWI_SKIP_WAKEUP |
+			CSIPHY_3PH_LNn_CFG8_SKEW_FILTER_ENABLE;
+		writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_CFG8(l));
+
+		val = CSIPHY_3PH_LNn_CFG9_SWI_T_WAKEUP;
+		writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_CFG9(l));
+
+		val = CSIPHY_3PH_LNn_TEST_IMP_HS_TERM_IMP;
+		writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_TEST_IMP(l));
+
+		val = CSIPHY_3PH_LNn_CSI_LANE_CTRL15_SWI_SOT_SYMBOL;
+		writel_relaxed(val, csi2phy->base +
+				    CSIPHY_3PH_LNn_CSI_LANE_CTRL15(l));
+	}
+
+	val = CSIPHY_3PH_LNn_CFG1_SWI_REC_DLY_PRG;
+	writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_CFG1(l));
+
+	if (regs->generation == GEN1_660)
+		val = CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS_660;
+	else
+		val = CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS;
+	writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_CFG4(l));
+
+	val = CSIPHY_3PH_LNn_MISC1_IS_CLKLANE;
+	writel_relaxed(val, csi2phy->base + CSIPHY_3PH_LNn_MISC1(l));
+}
+
+static void
+phy_qcom_mipi_csi2_gen2_config_lanes(struct mipi_csi2phy_device *csi2phy,
+				     u8 settle_cnt)
+{
+	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
+	const struct mipi_csi2phy_lane_regs *r = regs->init_seq;
+	int i, array_size = regs->lane_array_size;
+	u32 val;
+
+	for (i = 0; i < array_size; i++, r++) {
+		switch (r->mipi_csi2phy_param_type) {
+		case CSIPHY_SETTLE_CNT_LOWER_BYTE:
+			val = settle_cnt & 0xff;
+			break;
+		case CSIPHY_SKEW_CAL:
+			/* TODO: support application of skew from dt flag */
+			continue;
+		case CSIPHY_DNP_PARAMS:
+			continue;
+		default:
+			val = r->reg_data;
+			break;
+		}
+		writel_relaxed(val, csi2phy->base + r->reg_addr);
+		if (r->delay_us)
+			udelay(r->delay_us);
+	}
+}
+
+static bool phy_qcom_mipi_csi2_is_gen2(struct mipi_csi2phy_device *csi2phy)
+{
+	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
+
+	return regs->generation == GEN2;
+}
+
+static int phy_qcom_mipi_csi2_lanes_enable(struct mipi_csi2phy_device *csi2phy,
+					   struct mipi_csi2phy_stream_cfg *cfg)
+{
+	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
+	struct mipi_csi2phy_lanes_cfg *lane_cfg = &cfg->lane_cfg;
+	u8 settle_cnt;
+	u8 val;
+	int i;
+
+	settle_cnt = phy_qcom_mipi_csi2_settle_cnt_calc(cfg->link_freq, csi2phy->timer_clk_rate);
+
+	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
+	for (i = 0; i < cfg->num_data_lanes; i++)
+		val |= BIT(lane_cfg->data[i].pos * 2);
+
+	writel_relaxed(val, csi2phy->base +
+		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 5));
+
+	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B;
+	writel_relaxed(val, csi2phy->base +
+		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));
+
+	val = 0x02;
+	writel_relaxed(val, csi2phy->base +
+		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 7));
+
+	val = 0x00;
+	writel_relaxed(val, csi2phy->base +
+		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 0));
+
+	if (phy_qcom_mipi_csi2_is_gen2(csi2phy))
+		phy_qcom_mipi_csi2_gen2_config_lanes(csi2phy, settle_cnt);
+	else
+		phy_qcom_mipi_csi2_gen1_config_lanes(csi2phy, cfg, settle_cnt);
+
+	/* IRQ_MASK registers - disable all interrupts */
+	for (i = 11; i < 22; i++) {
+		writel_relaxed(0, csi2phy->base +
+			       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, i));
+	}
+
+	return 0;
+}
+
+static void
+phy_qcom_mipi_csi2_lanes_disable(struct mipi_csi2phy_device *csi2phy,
+				 struct mipi_csi2phy_stream_cfg *cfg)
+{
+	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
+
+	writel_relaxed(0, csi2phy->base +
+		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 5));
+
+	writel_relaxed(0, csi2phy->base +
+			  CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));
+}
+
+static int phy_qcom_mipi_csi2_init(struct mipi_csi2phy_device *csi2phy)
+{
+	return 0;
+}
+
+const struct mipi_csi2phy_hw_ops phy_qcom_mipi_csi2_ops_3ph_1_0 = {
+	.hw_version_read = phy_qcom_mipi_csi2_hw_version_read,
+	.reset = phy_qcom_mipi_csi2_reset,
+	.lanes_enable = phy_qcom_mipi_csi2_lanes_enable,
+	.lanes_disable = phy_qcom_mipi_csi2_lanes_disable,
+	.isr = phy_qcom_mipi_csi2_isr,
+	.init = phy_qcom_mipi_csi2_init,
+};
+
+const struct mipi_csi2phy_clk_freq zero = { 0 };
+
+const struct mipi_csi2phy_clk_freq dphy_4nm_x1e_csiphy = {
+	.freq = {
+		300000000, 400000000, 480000000
+	},
+	.num_freq = 3,
+};
+
+const struct mipi_csi2phy_clk_freq dphy_4nm_x1e_csiphy_timer = {
+	.freq = {
+		266666667, 400000000
+	},
+	.num_freq = 2,
+};
+
+const struct mipi_csi2phy_soc_cfg mipi_csi2_dphy_4nm_x1e = {
+	.ops = &phy_qcom_mipi_csi2_ops_3ph_1_0,
+	.reg_info = {
+		.init_seq = lane_regs_x1e80100,
+		.lane_array_size = ARRAY_SIZE(lane_regs_x1e80100),
+		.offset = 0x1000,
+		.generation = GEN2,
+	},
+	.supply_names = (const char *[]){
+		"vdda-0p8",
+		"vdda-1p2"
+	},
+	.num_supplies = 2,
+	.clk_names = (const char *[]) {
+		"camnoc_axi",
+		"cpas_ahb",
+		"csiphy",
+		"csiphy_timer"
+	},
+	.num_clk = 4,
+	.clk_freq = {
+		zero,
+		zero,
+		dphy_4nm_x1e_csiphy,
+		dphy_4nm_x1e_csiphy_timer,
+	},
+};
diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
new file mode 100644
index 0000000000000000000000000000000000000000..1def2d1258d9dd3835c09c6804e08dfffc184248
--- /dev/null
+++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
@@ -0,0 +1,281 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2025, Linaro Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/phy/phy-mipi-dphy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+#include "phy-qcom-mipi-csi2.h"
+
+#define CAMSS_CLOCK_MARGIN_NUMERATOR 105
+#define CAMSS_CLOCK_MARGIN_DENOMINATOR 100
+
+static inline void phy_qcom_mipi_csi2_add_clock_margin(u64 *rate)
+{
+	*rate *= CAMSS_CLOCK_MARGIN_NUMERATOR;
+	*rate = div_u64(*rate, CAMSS_CLOCK_MARGIN_DENOMINATOR);
+}
+
+static int
+phy_qcom_mipi_csi2_set_clock_rates(struct mipi_csi2phy_device *csi2phy,
+				   s64 link_freq)
+{
+	const struct mipi_csi2phy_soc_cfg *soc_cfg = csi2phy->soc_cfg;
+	struct device *dev = csi2phy->dev;
+	int i, j;
+	int ret;
+
+	for (i = 0; i < soc_cfg->num_clk; i++) {
+		const struct mipi_csi2phy_clk_freq *clk_freq = &soc_cfg->clk_freq[i];
+		const char *clk_name = soc_cfg->clk_names[i];
+		struct clk *clk = csi2phy->clks[i].clk;
+		u64 min_rate = link_freq / 4;
+		long round_rate;
+
+		phy_qcom_mipi_csi2_add_clock_margin(&min_rate);
+
+		/* This clock should be enabled only not set */
+		if (!clk_freq->num_freq)
+			continue;
+
+		for (j = 0; j < clk_freq->num_freq; j++)
+			if (min_rate < clk_freq->freq[j])
+				break;
+
+		if (j == clk_freq->num_freq) {
+			dev_err(dev,
+				"Pixel clock %llu is too high for %s\n",
+				min_rate, clk_name);
+			return -EINVAL;
+		}
+
+		/* if sensor pixel clock is not available
+		 * set highest possible CSIPHY clock rate
+		 */
+		if (min_rate == 0)
+			j = clk_freq->num_freq - 1;
+
+		round_rate = clk_round_rate(clk, clk_freq->freq[j]);
+		if (round_rate < 0) {
+			dev_err(dev, "clk round rate failed: %ld\n",
+				round_rate);
+			return -EINVAL;
+		}
+
+		csi2phy->timer_clk_rate = round_rate;
+
+		dev_dbg(dev, "set clk %s %lu Hz\n",
+			clk_name, round_rate);
+
+		ret = clk_set_rate(clk, csi2phy->timer_clk_rate);
+		if (ret < 0) {
+			dev_err(dev, "clk set rate failed: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int phy_qcom_mipi_csi2_configure(struct phy *phy,
+					union phy_configure_opts *opts)
+{
+	struct mipi_csi2phy_device *csi2phy = phy_get_drvdata(phy);
+	struct phy_configure_opts_mipi_dphy *dphy_cfg_opts = &opts->mipi_dphy;
+	struct mipi_csi2phy_stream_cfg *stream_cfg = &csi2phy->stream_cfg;
+	int ret;
+	int i;
+
+	ret = phy_mipi_dphy_config_validate(dphy_cfg_opts);
+	if (ret)
+		return ret;
+
+	if (dphy_cfg_opts->lanes < 1 || dphy_cfg_opts->lanes > CSI2_MAX_DATA_LANES)
+		return -EINVAL;
+
+	stream_cfg->combo_mode = 0;
+	stream_cfg->link_freq = dphy_cfg_opts->hs_clk_rate;
+	stream_cfg->num_data_lanes = dphy_cfg_opts->lanes;
+
+	/*
+	 * phy_configure_opts_mipi_dphy.lanes starts from zero to
+	 * the maximum number of enabled lanes.
+	 *
+	 * TODO: add support for bitmask of enabled lanes and polarities
+	 * of those lanes to the phy_configure_opts_mipi_dphy struct.
+	 * For now take the polarities as zero and the position as fixed
+	 * this is fine as no current upstream implementation maps otherwise.
+	 */
+	for (i = 0; i < stream_cfg->num_data_lanes; i++) {
+		stream_cfg->lane_cfg.data[i].pol = 0;
+		stream_cfg->lane_cfg.data[i].pos = i;
+	}
+
+	stream_cfg->lane_cfg.clk.pol = 0;
+	stream_cfg->lane_cfg.clk.pos = 7;
+
+	return 0;
+}
+
+static int phy_qcom_mipi_csi2_power_on(struct phy *phy)
+{
+	struct mipi_csi2phy_device *csi2phy = phy_get_drvdata(phy);
+	const struct mipi_csi2phy_hw_ops *ops = csi2phy->soc_cfg->ops;
+	struct device *dev = &phy->dev;
+	int ret;
+
+	ret = regulator_bulk_enable(csi2phy->soc_cfg->num_supplies,
+				    csi2phy->supplies);
+	if (ret)
+		return ret;
+
+	ret = phy_qcom_mipi_csi2_set_clock_rates(csi2phy, csi2phy->stream_cfg.link_freq);
+	if (ret)
+		goto poweroff_phy;
+
+	ret = clk_bulk_prepare_enable(csi2phy->soc_cfg->num_clk,
+				      csi2phy->clks);
+	if (ret) {
+		dev_err(dev, "failed to enable clocks, %d\n", ret);
+		goto poweroff_phy;
+	}
+
+	ops->hw_version_read(csi2phy);
+
+	return ops->lanes_enable(csi2phy, &csi2phy->stream_cfg);
+
+poweroff_phy:
+	regulator_bulk_disable(csi2phy->soc_cfg->num_supplies,
+			       csi2phy->supplies);
+
+	return ret;
+}
+
+static int phy_qcom_mipi_csi2_power_off(struct phy *phy)
+{
+	struct mipi_csi2phy_device *csi2phy = phy_get_drvdata(phy);
+
+	clk_bulk_disable_unprepare(csi2phy->soc_cfg->num_clk,
+				   csi2phy->clks);
+	regulator_bulk_disable(csi2phy->soc_cfg->num_supplies,
+			       csi2phy->supplies);
+
+	return 0;
+}
+
+static const struct phy_ops phy_qcom_mipi_csi2_ops = {
+	.configure	= phy_qcom_mipi_csi2_configure,
+	.power_on	= phy_qcom_mipi_csi2_power_on,
+	.power_off	= phy_qcom_mipi_csi2_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int phy_qcom_mipi_csi2_probe(struct platform_device *pdev)
+{
+	unsigned int i, num_clk, num_supplies;
+	struct mipi_csi2phy_device *csi2phy;
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct phy *generic_phy;
+	int ret;
+
+	csi2phy = devm_kzalloc(dev, sizeof(*csi2phy), GFP_KERNEL);
+	if (!csi2phy)
+		return -ENOMEM;
+
+	csi2phy->dev = dev;
+	csi2phy->soc_cfg = device_get_match_data(&pdev->dev);
+
+	if (!csi2phy->soc_cfg)
+		return -EINVAL;
+
+	num_clk = csi2phy->soc_cfg->num_clk;
+	csi2phy->clks = devm_kzalloc(dev, sizeof(*csi2phy->clks) * num_clk, GFP_KERNEL);
+	if (!csi2phy->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < num_clk; i++)
+		csi2phy->clks[i].id = csi2phy->soc_cfg->clk_names[i];
+
+	ret = devm_clk_bulk_get(dev, num_clk, csi2phy->clks);
+	if (ret) {
+		dev_err(dev, "Failed to get clocks %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_bulk_prepare_enable(num_clk, csi2phy->clks);
+	if (ret) {
+		dev_err(dev, "apq8016 clk_enable failed\n");
+		return ret;
+	}
+
+	num_supplies = csi2phy->soc_cfg->num_supplies;
+	csi2phy->supplies = devm_kzalloc(dev, sizeof(*csi2phy->supplies) * num_supplies,
+					 GFP_KERNEL);
+	if (!csi2phy->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < num_supplies; i++)
+		csi2phy->supplies[i].supply = csi2phy->soc_cfg->supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, num_supplies, csi2phy->supplies);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to get regulator supplies\n");
+
+	csi2phy->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(csi2phy->base))
+		return PTR_ERR(csi2phy->base);
+
+	generic_phy = devm_phy_create(dev, NULL, &phy_qcom_mipi_csi2_ops);
+	if (IS_ERR(generic_phy)) {
+		ret = PTR_ERR(generic_phy);
+		dev_err(dev, "failed to create phy, %d\n", ret);
+		return ret;
+	}
+	csi2phy->phy = generic_phy;
+
+	phy_set_drvdata(generic_phy, csi2phy);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (!IS_ERR(phy_provider))
+		dev_dbg(dev, "Registered MIPI CSI2 PHY device\n");
+	else
+		pm_runtime_disable(dev);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id phy_qcom_mipi_csi2_of_match_table[] = {
+	{ .compatible	= "qcom,x1e80100-mipi-csi2-combo-phy", .data = &mipi_csi2_dphy_4nm_x1e },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, phy_qcom_mipi_csi2_of_match_table);
+
+static struct platform_driver phy_qcom_mipi_csi2_driver = {
+	.probe		= phy_qcom_mipi_csi2_probe,
+	.driver = {
+		.name	= "qcom-mipi-csi2-phy",
+		.of_match_table = phy_qcom_mipi_csi2_of_match_table,
+	},
+};
+
+module_platform_driver(phy_qcom_mipi_csi2_driver);
+
+MODULE_DESCRIPTION("Qualcomm MIPI CSI2 PHY driver");
+MODULE_DESCRIPTION("Bryan O'Donoghue <bryan.odonoghue@linaro.org>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2.h b/drivers/phy/qualcomm/phy-qcom-mipi-csi2.h
new file mode 100644
index 0000000000000000000000000000000000000000..adcb371c9bf6fb7b2a3282f810c578966aca9b5f
--- /dev/null
+++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2.h
@@ -0,0 +1,101 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *
+ * Qualcomm MIPI CSI2 CPHY/DPHY driver
+ *
+ * Copyright (C) 2025 Linaro Ltd.
+ */
+#ifndef __PHY_QCOM_MIPI_CSI2_H__
+#define __PHY_QCOM_MIPI_CSI2_H__
+
+#include <linux/phy/phy.h>
+
+#define CSI2_MAX_DATA_LANES 4
+
+struct mipi_csi2phy_lane {
+	u8 pos;
+	u8 pol;
+};
+
+struct mipi_csi2phy_lanes_cfg {
+	struct mipi_csi2phy_lane data[CSI2_MAX_DATA_LANES];
+	struct mipi_csi2phy_lane clk;
+};
+
+struct mipi_csi2phy_stream_cfg {
+	u8 combo_mode;
+	s64 link_freq;
+	u8 num_data_lanes;
+	struct mipi_csi2phy_lanes_cfg lane_cfg;
+};
+
+struct mipi_csi2phy_device;
+
+struct mipi_csi2phy_hw_ops {
+	void (*hw_version_read)(struct mipi_csi2phy_device *csi2phy_dev);
+	void (*reset)(struct mipi_csi2phy_device *csi2phy_dev);
+	int (*lanes_enable)(struct mipi_csi2phy_device *csi2phy_dev,
+			    struct mipi_csi2phy_stream_cfg *cfg);
+	void (*lanes_disable)(struct mipi_csi2phy_device *csi2phy_dev,
+			      struct mipi_csi2phy_stream_cfg *cfg);
+	irqreturn_t (*isr)(int irq, void *dev);
+	int (*init)(struct mipi_csi2phy_device *csi2phy_dev);
+};
+
+struct mipi_csi2phy_lane_regs {
+	const s32 reg_addr;
+	const s32 reg_data;
+	const u32 delay_us;
+	const u32 mipi_csi2phy_param_type;
+};
+
+struct mipi_csi2phy_device_regs {
+	const struct mipi_csi2phy_lane_regs *init_seq;
+	const int lane_array_size;
+	const u32 offset;
+	enum  {
+		GEN1 = 0,
+		GEN1_660,
+		GEN1_670,
+		GEN2,
+	} generation;
+};
+
+#define MAX_CSI2PHY_CLKS 8
+struct mipi_csi2phy_clk_freq {
+	u32 num_freq;
+	u32 freq[MAX_CSI2PHY_CLKS];
+};
+
+struct mipi_csi2phy_soc_cfg {
+	const struct mipi_csi2phy_hw_ops *ops;
+	const struct mipi_csi2phy_device_regs reg_info;
+
+	const char ** const supply_names;
+	const unsigned int num_supplies;
+
+	const char ** const clk_names;
+	const unsigned int num_clk;
+
+	const struct mipi_csi2phy_clk_freq clk_freq[];
+};
+
+struct mipi_csi2phy_device {
+	struct device *dev;
+
+	struct phy *phy;
+	void __iomem *base;
+
+	struct clk_bulk_data *clks;
+	struct regulator_bulk_data *supplies;
+	u32 timer_clk_rate;
+
+	const struct mipi_csi2phy_soc_cfg *soc_cfg;
+	struct mipi_csi2phy_stream_cfg stream_cfg;
+
+	u32 hw_version;
+};
+
+extern const struct mipi_csi2phy_soc_cfg mipi_csi2_dphy_4nm_x1e;
+
+#endif /* __PHY_QCOM_MIPI_CSI2_H__ */

-- 
2.49.0

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Konrad Dybcio]

On 7/10/25 6:16 PM, Bryan O'Donoghue wrote:
> Add a new MIPI CSI2 driver in D-PHY mode initially. The entire set of
> existing CAMSS CSI PHY init sequences are imported in order to save time
> and effort in later patches.
> 
> In-line with other PHY drivers the process node name is omitted from the
> compat string while the soc name is included.
> 
> At the moment we follow the assignment of lane positions - the bitmap of
> physical input lanes to logical lane numbers as a linear list per the
> existing DPHY @lanes data-member.
> 
> This is fine for us in upstream since we also map the lanes contiguously
> but, our hardware can support different lane mappings so we should in the
> future extend out the DPHY structure to capture the mapping.
> 
> The Qualcomm 3PH class of PHYs can do both D-PHY and C-PHY mode. For now only
> D-PHY is supported.
> 
> In porting some of the logic over from camss-csiphy*.c to here its also
> possible to rationalise some of the code.
> 
> In particular use of regulator_bulk and clk_bulk as well as dropping the
> seemingly useless and unused interrupt handler.
> 
> The PHY sequences and a lot of the logic that goes with them are well proven
> in CAMSS and mature so the main thing to watch out for here is how to get
> the right sequencing of regulators, clocks and register-writes.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---

This is a good opporunity to refresh the code a bit

[...]

> +#define CSIPHY_3PH_LNn_CFG1(n)				(0x000 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG1_SWI_REC_DLY_PRG		(BIT(7) | BIT(6))
> +#define CSIPHY_3PH_LNn_CFG2(n)				(0x004 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG2_LP_REC_EN_INT		BIT(3)
> +#define CSIPHY_3PH_LNn_CFG3(n)				(0x008 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG4(n)				(0x00c + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS		0xa4
> +#define CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS_660		0xa5
> +#define CSIPHY_3PH_LNn_CFG5(n)				(0x010 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG5_T_HS_DTERM			0x02
> +#define CSIPHY_3PH_LNn_CFG5_HS_REC_EQ_FQ_INT		0x50
> +#define CSIPHY_3PH_LNn_TEST_IMP(n)			(0x01c + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_TEST_IMP_HS_TERM_IMP		0xa
> +#define CSIPHY_3PH_LNn_MISC1(n)				(0x028 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_MISC1_IS_CLKLANE			BIT(2)
> +#define CSIPHY_3PH_LNn_CFG6(n)				(0x02c + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG6_SWI_FORCE_INIT_EXIT		BIT(0)
> +#define CSIPHY_3PH_LNn_CFG7(n)				(0x030 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG7_SWI_T_INIT			0x2
> +#define CSIPHY_3PH_LNn_CFG8(n)				(0x034 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG8_SWI_SKIP_WAKEUP		BIT(0)
> +#define CSIPHY_3PH_LNn_CFG8_SKEW_FILTER_ENABLE		BIT(1)
> +#define CSIPHY_3PH_LNn_CFG9(n)				(0x038 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG9_SWI_T_WAKEUP		0x1
> +#define CSIPHY_3PH_LNn_CSI_LANE_CTRL15(n)		(0x03c + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CSI_LANE_CTRL15_SWI_SOT_SYMBOL	0xb8

> +
> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(offset, n)	((offset) + 0x4 * (n))
> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE	BIT(7)
> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B	BIT(0)
> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID	BIT(1)
> +#define CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(offset, n)	((offset) + 0xb0 + 0x4 * (n))> +
> +#define CSIPHY_DEFAULT_PARAMS				0
> +#define CSIPHY_LANE_ENABLE				1
> +#define CSIPHY_SETTLE_CNT_LOWER_BYTE			2
> +#define CSIPHY_SETTLE_CNT_HIGHER_BYTE			3
> +#define CSIPHY_DNP_PARAMS				4
> +#define CSIPHY_2PH_REGS					5
> +#define CSIPHY_3PH_REGS					6
> +#define CSIPHY_SKEW_CAL					7

These could be turned into separate sub-sequences instead (like e.g. in
qmpphy drivers)

Specifically with 2PH_REGS/3PH_REGS, I'm assuming 2PH refers to another
piece of hardware which could have its own init sequences..

> +
> +/* 4nm 2PH v 2.1.2 2p5Gbps 4 lane DPHY mode */
> +static const struct
> +mipi_csi2phy_lane_regs lane_regs_x1e80100[] = {
> +	/* Power up lanes 2ph mode */
> +	{0x1014, 0xD5, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	{0x101C, 0x7A, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	{0x1018, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

struct mipi_csi2phy_lane_regs {
	const s32 reg_addr;
	const s32 reg_data;
	const u32 delay_us;
	const u32 mipi_csi2phy_param_type;
};

let's use designated initializers, decimal for delay_us (omit it
where unnecessary)

[...]

> +static inline const struct mipi_csi2phy_device_regs *
> +csi2phy_dev_to_regs(const struct mipi_csi2phy_device *csi2phy)
> +{
> +	return &csi2phy->soc_cfg->reg_info;
> +}

#define?

> +
> +static void phy_qcom_mipi_csi2_hw_version_read(struct mipi_csi2phy_device *csi2phy)
> +{
> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
> +	u32 hw_version;
> +
> +	writel(CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID, csi2phy->base +
> +	       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));

Do we need to clear this later?> +
> +	hw_version = readl_relaxed(csi2phy->base +
> +				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 12));
> +	hw_version |= readl_relaxed(csi2phy->base +
> +				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 13)) << 8;
> +	hw_version |= readl_relaxed(csi2phy->base +
> +				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 14)) << 16;
> +	hw_version |= readl_relaxed(csi2phy->base +
> +				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 15)) << 24;

I don't like how you're ORing the entirety of these 32-bit registers
into the saved value, maybe we can get the lower 8 bits of each and
construct the result with more guardrails

> +
> +	csi2phy->hw_version = hw_version;
> +
> +	dev_dbg(csi2phy->dev, "CSIPHY 3PH HW Version = 0x%08x\n", hw_version);

dev_dbg_once() will suffice

> +}
> +
> +/*
> + * phy_qcom_mipi_csi2_reset - Perform software reset on CSIPHY module
> + * @phy_qcom_mipi_csi2: CSIPHY device
> + */
> +static void phy_qcom_mipi_csi2_reset(struct mipi_csi2phy_device *csi2phy)
> +{
> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
> +
> +	writel_relaxed(0x1, csi2phy->base +
> +		      CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 0));
> +	usleep_range(5000, 8000);
> +	writel_relaxed(0x0, csi2phy->base +
> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 0));

Can we get a name for this BIT(0)?

> +}
> +
> +static irqreturn_t phy_qcom_mipi_csi2_isr(int irq, void *dev)
> +{
> +	const struct mipi_csi2phy_device *csi2phy = dev;
> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
> +	int i;
> +
> +	for (i = 0; i < 11; i++) {
> +		int c = i + 22;

Please add some comments regarding these number choices

> +		u8 val = readl_relaxed(csi2phy->base +
> +				       CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, i));
> +
> +		writel_relaxed(val, csi2phy->base +
> +			       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, c));
> +	}
> +
> +	writel_relaxed(0x1, csi2phy->base +
> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 10));
> +	writel_relaxed(0x0, csi2phy->base +
> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 10));

No need for any delays?

> +
> +	for (i = 22; i < 33; i++) {

Please provide some context here too

> +		writel_relaxed(0x0, csi2phy->base +
> +			       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, i));
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * phy_qcom_mipi_csi2_settle_cnt_calc - Calculate settle count value
> + *
> + * Helper function to calculate settle count value. This is
> + * based on the CSI2 T_hs_settle parameter which in turn
> + * is calculated based on the CSI2 transmitter link frequency.
> + *
> + * Return settle count value or 0 if the CSI2 link frequency
> + * is not available
> + */
> +static u8 phy_qcom_mipi_csi2_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
> +{
> +	u32 ui; /* ps */
> +	u32 timer_period; /* ps */
> +	u32 t_hs_prepare_max; /* ps */
> +	u32 t_hs_settle; /* ps */

picoseconds? you can suffix the variables with _ps

Also, a reverse-Christmas-tree sorting would be neat

> +	u8 settle_cnt;
> +
> +	if (link_freq <= 0)
> +		return 0;
> +
> +	ui = div_u64(1000000000000LL, link_freq);

PSEC_PER_SEC

> +	ui /= 2;
> +	t_hs_prepare_max = 85000 + 6 * ui;
> +	t_hs_settle = t_hs_prepare_max;
> +
> +	timer_period = div_u64(1000000000000LL, timer_clk_rate);

ditto

[...]

> +	for (i = 0; i < array_size; i++, r++) {
> +		switch (r->mipi_csi2phy_param_type) {
> +		case CSIPHY_SETTLE_CNT_LOWER_BYTE:
> +			val = settle_cnt & 0xff;
> +			break;

what about CSIPHY_SETTLE_CNT_HIGHER_BYTE?

> +		case CSIPHY_SKEW_CAL:
> +			/* TODO: support application of skew from dt flag */

Why? What are these?> +			continue;
> +		case CSIPHY_DNP_PARAMS:
> +			continue;

"do not program"? why are they in the tables then?

> +		default:
> +			val = r->reg_data;
> +			break;
> +		}
> +		writel_relaxed(val, csi2phy->base + r->reg_addr);
> +		if (r->delay_us)
> +			udelay(r->delay_us);
> +	}
> +}
> +
> +static bool phy_qcom_mipi_csi2_is_gen2(struct mipi_csi2phy_device *csi2phy)
> +{
> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
> +
> +	return regs->generation == GEN2;
> +}

You only used this once, I don't think it's very useful

[...]

> +
> +static int phy_qcom_mipi_csi2_lanes_enable(struct mipi_csi2phy_device *csi2phy,
> +					   struct mipi_csi2phy_stream_cfg *cfg)
> +{
> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
> +	struct mipi_csi2phy_lanes_cfg *lane_cfg = &cfg->lane_cfg;
> +	u8 settle_cnt;
> +	u8 val;
> +	int i;
> +
> +	settle_cnt = phy_qcom_mipi_csi2_settle_cnt_calc(cfg->link_freq, csi2phy->timer_clk_rate);
> +
> +	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
> +	for (i = 0; i < cfg->num_data_lanes; i++)
> +		val |= BIT(lane_cfg->data[i].pos * 2);
> +
> +	writel_relaxed(val, csi2phy->base +
> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 5));
> +
> +	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B;
> +	writel_relaxed(val, csi2phy->base +
> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));
> +
> +	val = 0x02;
> +	writel_relaxed(val, csi2phy->base +
> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 7));
> +
> +	val = 0x00;

Can we get some names for the magic (un)set bits?

[...]

> +static void
> +phy_qcom_mipi_csi2_lanes_disable(struct mipi_csi2phy_device *csi2phy,
> +				 struct mipi_csi2phy_stream_cfg *cfg)
> +{
> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
> +
> +	writel_relaxed(0, csi2phy->base +
> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 5));
> +
> +	writel_relaxed(0, csi2phy->base +
> +			  CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));

Does the former write need to complete before the latter?

> +}
> +
> +static int phy_qcom_mipi_csi2_init(struct mipi_csi2phy_device *csi2phy)
> +{
> +	return 0;
> +}
> +
> +const struct mipi_csi2phy_hw_ops phy_qcom_mipi_csi2_ops_3ph_1_0 = {
> +	.hw_version_read = phy_qcom_mipi_csi2_hw_version_read,
> +	.reset = phy_qcom_mipi_csi2_reset,
> +	.lanes_enable = phy_qcom_mipi_csi2_lanes_enable,
> +	.lanes_disable = phy_qcom_mipi_csi2_lanes_disable,
> +	.isr = phy_qcom_mipi_csi2_isr,
> +	.init = phy_qcom_mipi_csi2_init,

You're never calling init(), remove it. If you need it in the future,
make it non-obligatory so that you don't need the above stub

> +};
> +
> +const struct mipi_csi2phy_clk_freq zero = { 0 };

'zero' is a terrible global variable name

> +
> +const struct mipi_csi2phy_clk_freq dphy_4nm_x1e_csiphy = {
> +	.freq = {
> +		300000000, 400000000, 480000000
> +	},
> +	.num_freq = 3,
> +};
> +
> +const struct mipi_csi2phy_clk_freq dphy_4nm_x1e_csiphy_timer = {
> +	.freq = {
> +		266666667, 400000000
> +	},
> +	.num_freq = 2,
> +};

These operating points *require* different RPMh levels, let's
grab them from an OPP table and do the ratesetting through the
related APIs

> +
> +const struct mipi_csi2phy_soc_cfg mipi_csi2_dphy_4nm_x1e = {
> +	.ops = &phy_qcom_mipi_csi2_ops_3ph_1_0,
> +	.reg_info = {
> +		.init_seq = lane_regs_x1e80100,
> +		.lane_array_size = ARRAY_SIZE(lane_regs_x1e80100),
> +		.offset = 0x1000,

common_regs_offset

> +		.generation = GEN2,
> +	},
> +	.supply_names = (const char *[]){
> +		"vdda-0p8",
> +		"vdda-1p2"
> +	},
> +	.num_supplies = 2,
> +	.clk_names = (const char *[]) {
> +		"camnoc_axi",
> +		"cpas_ahb",
> +		"csiphy",
> +		"csiphy_timer"
> +	},
> +	.num_clk = 4,
> +	.clk_freq = {
> +		zero,
> +		zero,
> +		dphy_4nm_x1e_csiphy,
> +		dphy_4nm_x1e_csiphy_timer,

Throw the ratesetting clocks into opp_config and grab the rest
through clk_bulk_get_all()

> +	},
> +};
> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..1def2d1258d9dd3835c09c6804e08dfffc184248
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025, Linaro Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/phy/phy-mipi-dphy.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#include "phy-qcom-mipi-csi2.h"
> +
> +#define CAMSS_CLOCK_MARGIN_NUMERATOR 105
> +#define CAMSS_CLOCK_MARGIN_DENOMINATOR 100
> +
> +static inline void phy_qcom_mipi_csi2_add_clock_margin(u64 *rate)
> +{
> +	*rate *= CAMSS_CLOCK_MARGIN_NUMERATOR;
> +	*rate = div_u64(*rate, CAMSS_CLOCK_MARGIN_DENOMINATOR);
> +}

I can't find downstream doing that

[...]

> +	/*
> +	 * phy_configure_opts_mipi_dphy.lanes starts from zero to
> +	 * the maximum number of enabled lanes.
> +	 *
> +	 * TODO: add support for bitmask of enabled lanes and polarities
> +	 * of those lanes to the phy_configure_opts_mipi_dphy struct.
> +	 * For now take the polarities as zero and the position as fixed
> +	 * this is fine as no current upstream implementation maps otherwise.
> +	 */

Can we at least grab the data and make sure it matches the
default hw configuration now, so that we won't break bad
DTs in the future?

[...]


> +static int phy_qcom_mipi_csi2_power_off(struct phy *phy)
> +{
> +	struct mipi_csi2phy_device *csi2phy = phy_get_drvdata(phy);
> +
> +	clk_bulk_disable_unprepare(csi2phy->soc_cfg->num_clk,
> +				   csi2phy->clks);
> +	regulator_bulk_disable(csi2phy->soc_cfg->num_supplies,
> +			       csi2phy->supplies);

downstream issues a sw reset across a number of registers first

[...]> +static const struct of_device_id phy_qcom_mipi_csi2_of_match_table[] = {
> +	{ .compatible	= "qcom,x1e80100-mipi-csi2-combo-phy", .data = &mipi_csi2_dphy_4nm_x1e },

odd >1 space

Konrad

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Bryan O'Donoghue]

On 10/07/2025 18:08, Konrad Dybcio wrote:
> On 7/10/25 6:16 PM, Bryan O'Donoghue wrote:
>> Add a new MIPI CSI2 driver in D-PHY mode initially. The entire set of
>> existing CAMSS CSI PHY init sequences are imported in order to save time
>> and effort in later patches.
>>
>> In-line with other PHY drivers the process node name is omitted from the
>> compat string while the soc name is included.
>>
>> At the moment we follow the assignment of lane positions - the bitmap of
>> physical input lanes to logical lane numbers as a linear list per the
>> existing DPHY @lanes data-member.
>>
>> This is fine for us in upstream since we also map the lanes contiguously
>> but, our hardware can support different lane mappings so we should in the
>> future extend out the DPHY structure to capture the mapping.
>>
>> The Qualcomm 3PH class of PHYs can do both D-PHY and C-PHY mode. For now only
>> D-PHY is supported.
>>
>> In porting some of the logic over from camss-csiphy*.c to here its also
>> possible to rationalise some of the code.
>>
>> In particular use of regulator_bulk and clk_bulk as well as dropping the
>> seemingly useless and unused interrupt handler.
>>
>> The PHY sequences and a lot of the logic that goes with them are well proven
>> in CAMSS and mature so the main thing to watch out for here is how to get
>> the right sequencing of regulators, clocks and register-writes.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
> 
> This is a good opporunity to refresh the code a bit

>> +}
>> +
>> +static irqreturn_t phy_qcom_mipi_csi2_isr(int irq, void *dev)
>> +{
>> +	const struct mipi_csi2phy_device *csi2phy = dev;
>> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
>> +	int i;
>> +
>> +	for (i = 0; i < 11; i++) {
>> +		int c = i + 22;
> 
> Please add some comments regarding these number choices
> 
>> +		u8 val = readl_relaxed(csi2phy->base +
>> +				       CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, i));
>> +
>> +		writel_relaxed(val, csi2phy->base +
>> +			       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, c));
>> +	}
>> +
>> +	writel_relaxed(0x1, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 10));
>> +	writel_relaxed(0x0, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 10));
> 
> No need for any delays?
> 
>> +
>> +	for (i = 22; i < 33; i++) {
> 
> Please provide some context here too
> 
>> +		writel_relaxed(0x0, csi2phy->base +
>> +			       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, i));
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}

TBH I think I will just drop this function in v2.

If someone comes up with a good reason for this ISR on this generation 
of hardware they are free to submit a patch to eludicate why.

During bringup/debug of this - including when I was getting RX CRC 
errors on the CSIPHY due to lacking one of two clocks, I never saw this 
ISR fire once.

I didn't enumerate the callback and you're entirely right these fixed "i 
= 22; i < 33" is to put it politely "questionable".

Just in case this code is useful on some SoC I haven't tested in the 
CAMSS source, I've left well enough alone.

Happy to drop this in the PHY driver version though.

>> +
>> +/*
>> + * phy_qcom_mipi_csi2_settle_cnt_calc - Calculate settle count value
>> + *
>> + * Helper function to calculate settle count value. This is
>> + * based on the CSI2 T_hs_settle parameter which in turn
>> + * is calculated based on the CSI2 transmitter link frequency.
>> + *
>> + * Return settle count value or 0 if the CSI2 link frequency
>> + * is not available
>> + */
>> +static u8 phy_qcom_mipi_csi2_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
>> +{
>> +	u32 ui; /* ps */
>> +	u32 timer_period; /* ps */
>> +	u32 t_hs_prepare_max; /* ps */
>> +	u32 t_hs_settle; /* ps */
> 
> picoseconds? you can suffix the variables with _ps
> 
> Also, a reverse-Christmas-tree sorting would be neat
> 
>> +	u8 settle_cnt;
>> +
>> +	if (link_freq <= 0)
>> +		return 0;
>> +
>> +	ui = div_u64(1000000000000LL, link_freq);
> 
> PSEC_PER_SEC
> 
>> +	ui /= 2;
>> +	t_hs_prepare_max = 85000 + 6 * ui;
>> +	t_hs_settle = t_hs_prepare_max;
>> +
>> +	timer_period = div_u64(1000000000000LL, timer_clk_rate);
> 
> ditto
> 
> [...]
> 
>> +	for (i = 0; i < array_size; i++, r++) {
>> +		switch (r->mipi_csi2phy_param_type) {
>> +		case CSIPHY_SETTLE_CNT_LOWER_BYTE:
>> +			val = settle_cnt & 0xff;
>> +			break;
> 
> what about CSIPHY_SETTLE_CNT_HIGHER_BYTE?
> 
>> +		case CSIPHY_SKEW_CAL:
>> +			/* TODO: support application of skew from dt flag */
> 
> Why? What are these?

> +			continue;
>> +		case CSIPHY_DNP_PARAMS:
>> +			continue;
> 
> "do not program"? why are they in the tables then?

We try to import downstream init sequences, which themselves are 
sometimes 1:1 from the original Si testbenches unmodified, DNP_PARAMS 
come from those sequences.

I think the testbench/downstream idea here is to allow a series of 
delays and/or readback post write.

I'm certainly willing to drop anything not in the _current_ init 
sequence list.

>> +		default:
>> +			val = r->reg_data;
>> +			break;
>> +		}
>> +		writel_relaxed(val, csi2phy->base + r->reg_addr);
>> +		if (r->delay_us)
>> +			udelay(r->delay_us);
>> +	}
>> +}
>> +
>> +static bool phy_qcom_mipi_csi2_is_gen2(struct mipi_csi2phy_device *csi2phy)
>> +{
>> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
>> +
>> +	return regs->generation == GEN2;
>> +}
> 
> You only used this once, I don't think it's very useful
> 
> [...]
> 
>> +
>> +static int phy_qcom_mipi_csi2_lanes_enable(struct mipi_csi2phy_device *csi2phy,
>> +					   struct mipi_csi2phy_stream_cfg *cfg)
>> +{
>> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
>> +	struct mipi_csi2phy_lanes_cfg *lane_cfg = &cfg->lane_cfg;
>> +	u8 settle_cnt;
>> +	u8 val;
>> +	int i;
>> +
>> +	settle_cnt = phy_qcom_mipi_csi2_settle_cnt_calc(cfg->link_freq, csi2phy->timer_clk_rate);
>> +
>> +	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
>> +	for (i = 0; i < cfg->num_data_lanes; i++)
>> +		val |= BIT(lane_cfg->data[i].pos * 2);
>> +
>> +	writel_relaxed(val, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 5));
>> +
>> +	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B;
>> +	writel_relaxed(val, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));
>> +
>> +	val = 0x02;
>> +	writel_relaxed(val, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 7));
>> +
>> +	val = 0x00;
> 
> Can we get some names for the magic (un)set bits?
> 
> [...]
> 
>> +static void
>> +phy_qcom_mipi_csi2_lanes_disable(struct mipi_csi2phy_device *csi2phy,
>> +				 struct mipi_csi2phy_stream_cfg *cfg)
>> +{
>> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
>> +
>> +	writel_relaxed(0, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 5));
>> +
>> +	writel_relaxed(0, csi2phy->base +
>> +			  CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));
> 
> Does the former write need to complete before the latter?

One assumes so. All of this _relaxed() stuff is way too flaithiúlach for 
me. We tolerate it as legacy code in CAMSS but, you're right, I don't 
think its logical at all.

Dropped.

> 
>> +}
>> +
>> +static int phy_qcom_mipi_csi2_init(struct mipi_csi2phy_device *csi2phy)
>> +{
>> +	return 0;
>> +}
>> +
>> +const struct mipi_csi2phy_hw_ops phy_qcom_mipi_csi2_ops_3ph_1_0 = {
>> +	.hw_version_read = phy_qcom_mipi_csi2_hw_version_read,
>> +	.reset = phy_qcom_mipi_csi2_reset,
>> +	.lanes_enable = phy_qcom_mipi_csi2_lanes_enable,
>> +	.lanes_disable = phy_qcom_mipi_csi2_lanes_disable,
>> +	.isr = phy_qcom_mipi_csi2_isr,
>> +	.init = phy_qcom_mipi_csi2_init,
> 
> You're never calling init(), remove it. If you need it in the future,
> make it non-obligatory so that you don't need the above stub
> 

y

>> +};
>> +
>> +const struct mipi_csi2phy_clk_freq zero = { 0 };
> 
> 'zero' is a terrible global variable name

ah, I forgot to make these static.

> 
>> +
>> +const struct mipi_csi2phy_clk_freq dphy_4nm_x1e_csiphy = {
>> +	.freq = {
>> +		300000000, 400000000, 480000000
>> +	},
>> +	.num_freq = 3,
>> +};
>> +
>> +const struct mipi_csi2phy_clk_freq dphy_4nm_x1e_csiphy_timer = {
>> +	.freq = {
>> +		266666667, 400000000
>> +	},
>> +	.num_freq = 2,
>> +};
> 
> These operating points *require* different RPMh levels, let's
> grab them from an OPP table and do the ratesetting through the
> related APIs

You're right.

I'll also add regulator_set_load() in v2.

> 
>> +
>> +const struct mipi_csi2phy_soc_cfg mipi_csi2_dphy_4nm_x1e = {
>> +	.ops = &phy_qcom_mipi_csi2_ops_3ph_1_0,
>> +	.reg_info = {
>> +		.init_seq = lane_regs_x1e80100,
>> +		.lane_array_size = ARRAY_SIZE(lane_regs_x1e80100),
>> +		.offset = 0x1000,
> 
> common_regs_offset
> 
>> +		.generation = GEN2,
>> +	},
>> +	.supply_names = (const char *[]){
>> +		"vdda-0p8",
>> +		"vdda-1p2"
>> +	},
>> +	.num_supplies = 2,
>> +	.clk_names = (const char *[]) {
>> +		"camnoc_axi",
>> +		"cpas_ahb",
>> +		"csiphy",
>> +		"csiphy_timer"
>> +	},
>> +	.num_clk = 4,
>> +	.clk_freq = {
>> +		zero,
>> +		zero,
>> +		dphy_4nm_x1e_csiphy,
>> +		dphy_4nm_x1e_csiphy_timer,
> 
> Throw the ratesetting clocks into opp_config and grab the rest
> through clk_bulk_get_all()
> 
>> +	},
>> +};
>> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..1def2d1258d9dd3835c09c6804e08dfffc184248
>> --- /dev/null
>> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
>> @@ -0,0 +1,281 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2025, Linaro Ltd.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/phy/phy-mipi-dphy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +
>> +#include "phy-qcom-mipi-csi2.h"
>> +
>> +#define CAMSS_CLOCK_MARGIN_NUMERATOR 105
>> +#define CAMSS_CLOCK_MARGIN_DENOMINATOR 100
>> +
>> +static inline void phy_qcom_mipi_csi2_add_clock_margin(u64 *rate)
>> +{
>> +	*rate *= CAMSS_CLOCK_MARGIN_NUMERATOR;
>> +	*rate = div_u64(*rate, CAMSS_CLOCK_MARGIN_DENOMINATOR);
>> +}
> 
> I can't find downstream doing that

Inherited from CAMSS where it really does something useful.

I'm relucant to change this... I'll try it though.

> 
> [...]
> 
>> +	/*
>> +	 * phy_configure_opts_mipi_dphy.lanes starts from zero to
>> +	 * the maximum number of enabled lanes.
>> +	 *
>> +	 * TODO: add support for bitmask of enabled lanes and polarities
>> +	 * of those lanes to the phy_configure_opts_mipi_dphy struct.
>> +	 * For now take the polarities as zero and the position as fixed
>> +	 * this is fine as no current upstream implementation maps otherwise.
>> +	 */
> 
> Can we at least grab the data and make sure it matches the
> default hw configuration now, so that we won't break bad
> DTs in the future?

Hmm. I'll have a think about that.

---
bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Konrad Dybcio]

On 7/11/25 11:14 AM, Bryan O'Donoghue wrote:
> On 10/07/2025 18:08, Konrad Dybcio wrote:
>> On 7/10/25 6:16 PM, Bryan O'Donoghue wrote:
>>> Add a new MIPI CSI2 driver in D-PHY mode initially. The entire set of
>>> existing CAMSS CSI PHY init sequences are imported in order to save time
>>> and effort in later patches.
>>>
>>> In-line with other PHY drivers the process node name is omitted from the
>>> compat string while the soc name is included.
>>>
>>> At the moment we follow the assignment of lane positions - the bitmap of
>>> physical input lanes to logical lane numbers as a linear list per the
>>> existing DPHY @lanes data-member.
>>>
>>> This is fine for us in upstream since we also map the lanes contiguously
>>> but, our hardware can support different lane mappings so we should in the
>>> future extend out the DPHY structure to capture the mapping.
>>>
>>> The Qualcomm 3PH class of PHYs can do both D-PHY and C-PHY mode. For now only
>>> D-PHY is supported.
>>>
>>> In porting some of the logic over from camss-csiphy*.c to here its also
>>> possible to rationalise some of the code.
>>>
>>> In particular use of regulator_bulk and clk_bulk as well as dropping the
>>> seemingly useless and unused interrupt handler.
>>>
>>> The PHY sequences and a lot of the logic that goes with them are well proven
>>> in CAMSS and mature so the main thing to watch out for here is how to get
>>> the right sequencing of regulators, clocks and register-writes.
>>>
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> ---

[...]

>> +            continue;
>>> +        case CSIPHY_DNP_PARAMS:
>>> +            continue;
>>
>> "do not program"? why are they in the tables then?
> 
> We try to import downstream init sequences, which themselves are sometimes 1:1 from the original Si testbenches unmodified, DNP_PARAMS come from those sequences.

Not sure about camera specifically, but using the testbench sequences
for other kinds of PHYs is supposedly not a good idea, as they are/may
be tuned differently for actual (reference) boards 

> 
> I think the testbench/downstream idea here is to allow a series of delays and/or readback post write.
> 
> I'm certainly willing to drop anything not in the _current_ init sequence list.

If there's a delay necessary, you already added the infra to inject one

[...]

>>> +static void
>>> +phy_qcom_mipi_csi2_lanes_disable(struct mipi_csi2phy_device *csi2phy,
>>> +                 struct mipi_csi2phy_stream_cfg *cfg)
>>> +{
>>> +    const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
>>> +
>>> +    writel_relaxed(0, csi2phy->base +
>>> +               CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 5));
>>> +
>>> +    writel_relaxed(0, csi2phy->base +
>>> +              CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));
>>
>> Does the former write need to complete before the latter?
> 
> One assumes so. All of this _relaxed() stuff is way too flaithiúlach for me. We tolerate it as legacy code in CAMSS but, you're right, I don't think its logical at all.
> 
> Dropped.

Ordering and completion are not the same, see:
https://www.youtube.com/watch?v=i6DayghhA8Q

[...]

>>> +static inline void phy_qcom_mipi_csi2_add_clock_margin(u64 *rate)
>>> +{
>>> +    *rate *= CAMSS_CLOCK_MARGIN_NUMERATOR;
>>> +    *rate = div_u64(*rate, CAMSS_CLOCK_MARGIN_DENOMINATOR);
>>> +}
>>
>> I can't find downstream doing that
> 
> Inherited from CAMSS where it really does something useful.
> 
> I'm relucant to change this... I'll try it though.

There's no room for guessing, this affects the clock rate of a
component that interfaces a physical bus, please try to get an answer
one way or the other

> 
>>
>> [...]
>>
>>> +    /*
>>> +     * phy_configure_opts_mipi_dphy.lanes starts from zero to
>>> +     * the maximum number of enabled lanes.
>>> +     *
>>> +     * TODO: add support for bitmask of enabled lanes and polarities
>>> +     * of those lanes to the phy_configure_opts_mipi_dphy struct.
>>> +     * For now take the polarities as zero and the position as fixed
>>> +     * this is fine as no current upstream implementation maps otherwise.
>>> +     */
>>
>> Can we at least grab the data and make sure it matches the
>> default hw configuration now, so that we won't break bad
>> DTs in the future?
> 
> Hmm. I'll have a think about that.

We certainly don't want the venus situation all over again..

Konrad

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Vladimir Zapolskiy]

On 7/10/25 19:16, Bryan O'Donoghue wrote:
> Add a new MIPI CSI2 driver in D-PHY mode initially. The entire set of
> existing CAMSS CSI PHY init sequences are imported in order to save time
> and effort in later patches.
> 
> In-line with other PHY drivers the process node name is omitted from the
> compat string while the soc name is included.
> 
> At the moment we follow the assignment of lane positions - the bitmap of
> physical input lanes to logical lane numbers as a linear list per the
> existing DPHY @lanes data-member.
> 
> This is fine for us in upstream since we also map the lanes contiguously
> but, our hardware can support different lane mappings so we should in the
> future extend out the DPHY structure to capture the mapping.
> 
> The Qualcomm 3PH class of PHYs can do both D-PHY and C-PHY mode. For now only
> D-PHY is supported.
> 
> In porting some of the logic over from camss-csiphy*.c to here its also
> possible to rationalise some of the code.
> 
> In particular use of regulator_bulk and clk_bulk as well as dropping the
> seemingly useless and unused interrupt handler.
> 
> The PHY sequences and a lot of the logic that goes with them are well proven
> in CAMSS and mature so the main thing to watch out for here is how to get
> the right sequencing of regulators, clocks and register-writes.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   MAINTAINERS                                        |  11 +
>   drivers/phy/qualcomm/Kconfig                       |  11 +
>   drivers/phy/qualcomm/Makefile                      |   6 +
>   drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c | 491 +++++++++++++++++++++
>   drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c     | 281 ++++++++++++
>   drivers/phy/qualcomm/phy-qcom-mipi-csi2.h          | 101 +++++
>   6 files changed, 901 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1ef99240a57ed1ad0d4501998970c7c3b85d3b81..69519e2d6dfb65771a3245735283645bb50a249a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20536,6 +20536,17 @@ S:	Maintained
>   F:	Documentation/devicetree/bindings/media/qcom,*-iris.yaml
>   F:	drivers/media/platform/qcom/iris/
>   
> +QUALCOMM MIPI CSI2 PHY DRIVER
> +M:	Bryan O'Donoghue <bod@kernel.org>
> +L:	linux-phy@lists.infradead.org
> +L:	linux-media@vger.kernel.org
> +L:	linux-arm-msm@vger.kernel.org
> +S:	Supported
> +F:	Documentation/devicetree/bindings/phy/qcom,x1e80100-mipi-csi2-combo-phy.yaml
> +F:	drivers/phy/qualcomm/phy-qcom-mipi-csi2*.c
> +F:	drivers/phy/qualcomm/phy-qcom-mipi-csi2*.h
> +F:	include/dt-bindings/phy/phy-qcom-mipi-csi2*
> +
>   QUALCOMM NAND CONTROLLER DRIVER
>   M:	Manivannan Sadhasivam <mani@kernel.org>
>   L:	linux-mtd@lists.infradead.org
> diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
> index ef14f4e33973cff4103d8ea3b07cfd62d344e450..d0ab70827519c2b046d0fb03c14bb4d8ae2ec9a1 100644
> --- a/drivers/phy/qualcomm/Kconfig
> +++ b/drivers/phy/qualcomm/Kconfig
> @@ -28,6 +28,17 @@ config PHY_QCOM_EDP
>   	  Enable this driver to support the Qualcomm eDP PHY found in various
>   	  Qualcomm chipsets.
>   
> +config PHY_QCOM_MIPI_CSI2
> +	tristate "Qualcomm MIPI CSI2 PHY driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on OF
> +	depends on COMMON_CLK
> +	select GENERIC_PHY
> +	select GENERIC_PHY_MIPI_DPHY
> +	help
> +	  Enable this to support the MIPI CSI2 PHY driver found in various
> +	  Qualcomm chipsets.
> +
>   config PHY_QCOM_IPQ4019_USB
>   	tristate "Qualcomm IPQ4019 USB PHY driver"
>   	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
> index 3851e28a212d4a677a5b41805868f38b9ab49841..67013d27cb0387b9d65dcbe030ea6e5eaaabbe91 100644
> --- a/drivers/phy/qualcomm/Makefile
> +++ b/drivers/phy/qualcomm/Makefile
> @@ -5,6 +5,12 @@ obj-$(CONFIG_PHY_QCOM_EDP)		+= phy-qcom-edp.o
>   obj-$(CONFIG_PHY_QCOM_IPQ4019_USB)	+= phy-qcom-ipq4019-usb.o
>   obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)	+= phy-qcom-ipq806x-sata.o
>   obj-$(CONFIG_PHY_QCOM_M31_USB)		+= phy-qcom-m31.o
> +
> +phy-qcom-mipi-csi2-objs			+= phy-qcom-mipi-csi2-core.o \
> +					   phy-qcom-mipi-csi2-3ph-dphy.o
> +
> +obj-$(CONFIG_PHY_QCOM_MIPI_CSI2)	+= phy-qcom-mipi-csi2.o
> +
>   obj-$(CONFIG_PHY_QCOM_PCIE2)		+= phy-qcom-pcie2.o
>   
>   obj-$(CONFIG_PHY_QCOM_QMP_COMBO)	+= phy-qcom-qmp-combo.o phy-qcom-qmp-usbc.o
> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..1a99efee88cc94ec0d29a335cd29f88af8a00c02
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> @@ -0,0 +1,491 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * camss-phy_qcom_mipi_csi2-3ph-1-0.c
> + *
> + * Qualcomm MSM Camera Subsystem - CSIPHY Module 3phase v1.0
> + *
> + * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2016-2025 Linaro Ltd.
> + */
> +#define DEBUG

Still under debugging?..

Well, the phy should be a multimedia device, and this driver is not
the one, thus you can not use it to connect sensors and put the IP
into a media pipeline.

-- 
Best wishes,
Vladimir

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Bryan O'Donoghue]

On 14/07/2025 15:16, Vladimir Zapolskiy wrote:
>> +#define DEBUG
> 
> Still under debugging?..

oops thanks.

> Well, the phy should be a multimedia device, and this driver is not
> the one, thus you can not use it to connect sensors and put the IP
> into a media pipeline.
Ah no, I don't agree.
Please see my previous email.

---
bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Vladimir Zapolskiy]

On 7/14/25 17:43, Bryan O'Donoghue wrote:
> On 14/07/2025 15:16, Vladimir Zapolskiy wrote:
>>> +#define DEBUG
>>
>> Still under debugging?..
> 
> oops thanks.
> 
>> Well, the phy should be a multimedia device, and this driver is not
>> the one, thus you can not use it to connect sensors and put the IP
>> into a media pipeline.
> Ah no, I don't agree.
> Please see my previous email.
> 

I believe there is very little room to disagree...

This proposed device node scheme does not solve the known and already
discussed technical issue expectedly, namely there is no given way
to describe a combo mode hardware configuration, when two independant
sensors are wired to the same CSIPHY. This is an unsolvable problem
with this design.

Sensors are conneced to CSIPHY IP blocks, CSIPHY is connected to CSID.

--
Best wishes,
Vladimir

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Bryan O'Donoghue]

On 14/07/2025 15:58, Vladimir Zapolskiy wrote:
> 
> This proposed device node scheme does not solve the known and already
> discussed technical issue expectedly, namely there is no given way
> to describe a combo mode hardware configuration, when two independant
> sensors are wired to the same CSIPHY. This is an unsolvable problem
> with this design.

I think that is genuinely something we should handle in camss-csid.c 
maybe with some meta-data inside of the ports/endpoints..

> 
> Sensors are conneced to CSIPHY IP blocks, CSIPHY is connected to CSID.
My understanding of best practice is data-endpoints go into the consumer 
not the PHY.

These are PHYs with their own SoC pins and voltage rails. They should 
look like other PHYs in qcom and across DT, IMO.

---
bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Konrad Dybcio]

On 7/14/25 5:17 PM, Bryan O'Donoghue wrote:
> On 14/07/2025 15:58, Vladimir Zapolskiy wrote:
>>
>> This proposed device node scheme does not solve the known and already
>> discussed technical issue expectedly, namely there is no given way
>> to describe a combo mode hardware configuration, when two independant
>> sensors are wired to the same CSIPHY. This is an unsolvable problem
>> with this design.
> 
> I think that is genuinely something we should handle in camss-csid.c maybe with some meta-data inside of the ports/endpoints..
> 
>>
>> Sensors are conneced to CSIPHY IP blocks, CSIPHY is connected to CSID.
> My understanding of best practice is data-endpoints go into the consumer not the PHY.

At least in the case of USB, the PHY's of_graph includes signals that go
through said PHY (so HS for HS phys, RXTX for SS PHYs)

Konrad

> 
> These are PHYs with their own SoC pins and voltage rails. They should look like other PHYs in qcom and across DT, IMO.
> 
> ---
> bod
> 

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Vladimir Zapolskiy]

On 7/14/25 18:17, Bryan O'Donoghue wrote:
> On 14/07/2025 15:58, Vladimir Zapolskiy wrote:
>>
>> This proposed device node scheme does not solve the known and already
>> discussed technical issue expectedly, namely there is no given way
>> to describe a combo mode hardware configuration, when two independant
>> sensors are wired to the same CSIPHY. This is an unsolvable problem
>> with this design.
> 
> I think that is genuinely something we should handle in camss-csid.c
> maybe with some meta-data inside of the ports/endpoints..
> 

This is a CSIPHY property, a CSIPHY hardware configuration and a wiring
of sensors to a CSIPHY. Where is the relation to CSID here? There is no.

Please share at least a device tree node description, which supports
a connection of two sensors to a single CSIPHY, like it shall be done
expectedly.

-- 
Best wishes,
Vladimir

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Bryan O'Donoghue]

On 14/07/2025 16:30, Vladimir Zapolskiy wrote:
>>
>> I think that is genuinely something we should handle in camss-csid.c
>> maybe with some meta-data inside of the ports/endpoints..
>>
> 
> This is a CSIPHY property, a CSIPHY hardware configuration and a wiring
> of sensors to a CSIPHY. Where is the relation to CSID here? There is no.

All the PHY really needs to know is the # of lanes in aggregate, which 
physical lanes to map to which logical lanes and the pixel clock.

We should add additional support to the Kernel's D-PHY API parameters 
mechanism to support that physical-to-logical mapping but, that's not 
required for this series or for any currently know upstream user of CAMSS.

> Please share at least a device tree node description, which supports
> a connection of two sensors to a single CSIPHY, like it shall be done
> expectedly.
&camss {
     port@0 {
         csiphy0_lanes01_ep: endpoint0 {
             data-lanes = <0 1>;
             remote-endpoint = <&sensor0_ep>;
         };

         csiphy0_lanes23_ep: endpoint0 {
             data-lanes = <2 3>;
             remote-endpoint = <&sensor1_ep>;
         };
      };
};

&csiphy0 {
     status = "okay";

     vdda-0p8-supply = <&vreg_0p8>;
     vdda-1p2-supply = <&vreg_1p2>;
     phy-mode = <PHY_TYPE_DPHY>;
};

sensor0 {
     compatible = "manufacturer,sensor0";
     port {
         sensor0_ep: endpoint {
             data-lanes = <0 1>;
             remote-endpoint = <&csiphy0_lanes01_ep>;
         };
     };
};

sensor1 {
     compatible = "manufacturer,sensor1";
     port {
         sensor1_ep: endpoint {
             data-lanes = <0 1>;
             remote-endpoint = <&csiphy1_lanes23_ep>;
         };
     };
};

---
bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Vladimir Zapolskiy]

On 7/15/25 03:13, Bryan O'Donoghue wrote:
> On 14/07/2025 16:30, Vladimir Zapolskiy wrote:
>>>
>>> I think that is genuinely something we should handle in camss-csid.c
>>> maybe with some meta-data inside of the ports/endpoints..
>>>
>>
>> This is a CSIPHY property, a CSIPHY hardware configuration and a wiring
>> of sensors to a CSIPHY. Where is the relation to CSID here? There is no.
> 
> All the PHY really needs to know is the # of lanes in aggregate, which
> physical lanes to map to which logical lanes and the pixel clock.
> 
> We should add additional support to the Kernel's D-PHY API parameters
> mechanism to support that physical-to-logical mapping but, that's not
> required for this series or for any currently know upstream user of CAMSS.
> 
>> Please share at least a device tree node description, which supports
>> a connection of two sensors to a single CSIPHY, like it shall be done
>> expectedly.
> &camss {
>       port@0 {
>           csiphy0_lanes01_ep: endpoint0 {
>               data-lanes = <0 1>;
>               remote-endpoint = <&sensor0_ep>;
>           };
> 
>           csiphy0_lanes23_ep: endpoint0 {
>               data-lanes = <2 3>;
>               remote-endpoint = <&sensor1_ep>;
>           };
>        };
> };

Don't you understand that this is broken?.. That's no good.

Please listen and reread the messages given to you above, your proposed
"solution" does not support by design a valid hardware setup of two
sensors connected to the same CSIPHY.

I would propose to stop force pushing an uncorrectable dt scheme, it
makes no sense.

-- 
Best wishes,
Vladimir

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Konrad Dybcio]

On 7/15/25 8:35 AM, Vladimir Zapolskiy wrote:
> On 7/15/25 03:13, Bryan O'Donoghue wrote:
>> On 14/07/2025 16:30, Vladimir Zapolskiy wrote:
>>>>
>>>> I think that is genuinely something we should handle in camss-csid.c
>>>> maybe with some meta-data inside of the ports/endpoints..
>>>>
>>>
>>> This is a CSIPHY property, a CSIPHY hardware configuration and a wiring
>>> of sensors to a CSIPHY. Where is the relation to CSID here? There is no.
>>
>> All the PHY really needs to know is the # of lanes in aggregate, which
>> physical lanes to map to which logical lanes and the pixel clock.
>>
>> We should add additional support to the Kernel's D-PHY API parameters
>> mechanism to support that physical-to-logical mapping but, that's not
>> required for this series or for any currently know upstream user of CAMSS.
>>
>>> Please share at least a device tree node description, which supports
>>> a connection of two sensors to a single CSIPHY, like it shall be done
>>> expectedly.
>> &camss {
>>       port@0 {
>>           csiphy0_lanes01_ep: endpoint0 {
>>               data-lanes = <0 1>;
>>               remote-endpoint = <&sensor0_ep>;
>>           };
>>
>>           csiphy0_lanes23_ep: endpoint0 {
>>               data-lanes = <2 3>;
>>               remote-endpoint = <&sensor1_ep>;
>>           };
>>        };
>> };
> 
> Don't you understand that this is broken?.. That's no good.
> 
> Please listen and reread the messages given to you above, your proposed
> "solution" does not support by design a valid hardware setup of two
> sensors connected to the same CSIPHY.
> 
> I would propose to stop force pushing an uncorrectable dt scheme, it
> makes no sense.

If all you're asking for is an ability to grab an of_graph reference
from the camss (v4l2) driver, you can simply do something along the
lines of of_graph_get_remote_port(phy->dev->of_node)

Konrad

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Vladimir Zapolskiy]

On 7/15/25 12:01, Konrad Dybcio wrote:
> On 7/15/25 8:35 AM, Vladimir Zapolskiy wrote:
>> On 7/15/25 03:13, Bryan O'Donoghue wrote:
>>> On 14/07/2025 16:30, Vladimir Zapolskiy wrote:
>>>>>
>>>>> I think that is genuinely something we should handle in camss-csid.c
>>>>> maybe with some meta-data inside of the ports/endpoints..
>>>>>
>>>>
>>>> This is a CSIPHY property, a CSIPHY hardware configuration and a wiring
>>>> of sensors to a CSIPHY. Where is the relation to CSID here? There is no.
>>>
>>> All the PHY really needs to know is the # of lanes in aggregate, which
>>> physical lanes to map to which logical lanes and the pixel clock.
>>>
>>> We should add additional support to the Kernel's D-PHY API parameters
>>> mechanism to support that physical-to-logical mapping but, that's not
>>> required for this series or for any currently know upstream user of CAMSS.
>>>
>>>> Please share at least a device tree node description, which supports
>>>> a connection of two sensors to a single CSIPHY, like it shall be done
>>>> expectedly.
>>> &camss {
>>>        port@0 {
>>>            csiphy0_lanes01_ep: endpoint0 {
>>>                data-lanes = <0 1>;
>>>                remote-endpoint = <&sensor0_ep>;
>>>            };
>>>
>>>            csiphy0_lanes23_ep: endpoint0 {
>>>                data-lanes = <2 3>;
>>>                remote-endpoint = <&sensor1_ep>;
>>>            };
>>>         };
>>> };
>>
>> Don't you understand that this is broken?.. That's no good.
>>
>> Please listen and reread the messages given to you above, your proposed
>> "solution" does not support by design a valid hardware setup of two
>> sensors connected to the same CSIPHY.
>>
>> I would propose to stop force pushing an uncorrectable dt scheme, it
>> makes no sense.
> 
> If all you're asking for is an ability to grab an of_graph reference
> from the camss (v4l2) driver, you can simply do something along the
> lines of of_graph_get_remote_port(phy->dev->of_node)
> 

It's not about the driver specifics, my comment is about a proper
hardware description in dts notation, please see the device tree node
names.

-- 
Best wishes,
Vladimir

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Konrad Dybcio]

On 7/15/25 11:20 AM, Vladimir Zapolskiy wrote:
> On 7/15/25 12:01, Konrad Dybcio wrote:
>> On 7/15/25 8:35 AM, Vladimir Zapolskiy wrote:
>>> On 7/15/25 03:13, Bryan O'Donoghue wrote:
>>>> On 14/07/2025 16:30, Vladimir Zapolskiy wrote:
>>>>>>
>>>>>> I think that is genuinely something we should handle in camss-csid.c
>>>>>> maybe with some meta-data inside of the ports/endpoints..
>>>>>>
>>>>>
>>>>> This is a CSIPHY property, a CSIPHY hardware configuration and a wiring
>>>>> of sensors to a CSIPHY. Where is the relation to CSID here? There is no.
>>>>
>>>> All the PHY really needs to know is the # of lanes in aggregate, which
>>>> physical lanes to map to which logical lanes and the pixel clock.
>>>>
>>>> We should add additional support to the Kernel's D-PHY API parameters
>>>> mechanism to support that physical-to-logical mapping but, that's not
>>>> required for this series or for any currently know upstream user of CAMSS.
>>>>
>>>>> Please share at least a device tree node description, which supports
>>>>> a connection of two sensors to a single CSIPHY, like it shall be done
>>>>> expectedly.
>>>> &camss {
>>>>        port@0 {
>>>>            csiphy0_lanes01_ep: endpoint0 {
>>>>                data-lanes = <0 1>;
>>>>                remote-endpoint = <&sensor0_ep>;
>>>>            };
>>>>
>>>>            csiphy0_lanes23_ep: endpoint0 {
>>>>                data-lanes = <2 3>;
>>>>                remote-endpoint = <&sensor1_ep>;
>>>>            };
>>>>         };
>>>> };
>>>
>>> Don't you understand that this is broken?.. That's no good.
>>>
>>> Please listen and reread the messages given to you above, your proposed
>>> "solution" does not support by design a valid hardware setup of two
>>> sensors connected to the same CSIPHY.
>>>
>>> I would propose to stop force pushing an uncorrectable dt scheme, it
>>> makes no sense.
>>
>> If all you're asking for is an ability to grab an of_graph reference
>> from the camss (v4l2) driver, you can simply do something along the
>> lines of of_graph_get_remote_port(phy->dev->of_node)
>>
> 
> It's not about the driver specifics, my comment is about a proper
> hardware description in dts notation, please see the device tree node
> names.

I'm a little lost on what you're trying to argue for..

I could make out:

1. "the phy should be a multimedia device"
2. "There is no ports at all, which makes the device tree node unusable,
  since you can not provide a way to connect any sensors to the phy."

I don't really understand #1.. maybe that could be the case if the PHY
has a multitude of tunables (which I don't know if it does, but wouldn't
be exactly surprised if it did) that may be usecase/pipeline-specific

As for #2, I do think it makes sense to connect the sensors to the PHY,
as that's a representation of electrical signals travelling from the
producer to the consumer (plus the data passed in e.g. data-lanes is
directly related to the PHY and necessarily consumed by its driver)

Konrad 

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[neil.armstrong]

On 15/07/2025 11:33, Konrad Dybcio wrote:
> On 7/15/25 11:20 AM, Vladimir Zapolskiy wrote:
>> On 7/15/25 12:01, Konrad Dybcio wrote:
>>> On 7/15/25 8:35 AM, Vladimir Zapolskiy wrote:
>>>> On 7/15/25 03:13, Bryan O'Donoghue wrote:
>>>>> On 14/07/2025 16:30, Vladimir Zapolskiy wrote:
>>>>>>>
>>>>>>> I think that is genuinely something we should handle in camss-csid.c
>>>>>>> maybe with some meta-data inside of the ports/endpoints..
>>>>>>>
>>>>>>
>>>>>> This is a CSIPHY property, a CSIPHY hardware configuration and a wiring
>>>>>> of sensors to a CSIPHY. Where is the relation to CSID here? There is no.
>>>>>
>>>>> All the PHY really needs to know is the # of lanes in aggregate, which
>>>>> physical lanes to map to which logical lanes and the pixel clock.
>>>>>
>>>>> We should add additional support to the Kernel's D-PHY API parameters
>>>>> mechanism to support that physical-to-logical mapping but, that's not
>>>>> required for this series or for any currently know upstream user of CAMSS.
>>>>>
>>>>>> Please share at least a device tree node description, which supports
>>>>>> a connection of two sensors to a single CSIPHY, like it shall be done
>>>>>> expectedly.
>>>>> &camss {
>>>>>         port@0 {
>>>>>             csiphy0_lanes01_ep: endpoint0 {
>>>>>                 data-lanes = <0 1>;
>>>>>                 remote-endpoint = <&sensor0_ep>;
>>>>>             };
>>>>>
>>>>>             csiphy0_lanes23_ep: endpoint0 {
>>>>>                 data-lanes = <2 3>;
>>>>>                 remote-endpoint = <&sensor1_ep>;
>>>>>             };
>>>>>          };
>>>>> };
>>>>
>>>> Don't you understand that this is broken?.. That's no good.
>>>>
>>>> Please listen and reread the messages given to you above, your proposed
>>>> "solution" does not support by design a valid hardware setup of two
>>>> sensors connected to the same CSIPHY.
>>>>
>>>> I would propose to stop force pushing an uncorrectable dt scheme, it
>>>> makes no sense.
>>>
>>> If all you're asking for is an ability to grab an of_graph reference
>>> from the camss (v4l2) driver, you can simply do something along the
>>> lines of of_graph_get_remote_port(phy->dev->of_node)
>>>
>>
>> It's not about the driver specifics, my comment is about a proper
>> hardware description in dts notation, please see the device tree node
>> names.
> 
> I'm a little lost on what you're trying to argue for..
> 
> I could make out:
> 
> 1. "the phy should be a multimedia device"
> 2. "There is no ports at all, which makes the device tree node unusable,
>    since you can not provide a way to connect any sensors to the phy."
> 
> I don't really understand #1.. maybe that could be the case if the PHY
> has a multitude of tunables (which I don't know if it does, but wouldn't
> be exactly surprised if it did) that may be usecase/pipeline-specific
> 
> As for #2, I do think it makes sense to connect the sensors to the PHY,
> as that's a representation of electrical signals travelling from the
> producer to the consumer (plus the data passed in e.g. data-lanes is
> directly related to the PHY and necessarily consumed by its driver)

The port/endpoint should represent the data flow, and if the signal is the following:

sensor -> csiphy -> csid

and in addition the "csiphy" can handle up to two sensors by splitting the lanes,
then the DT should take this in account and effectively the "csiphy" should be
an element of the graph and should take it's configuration from the DT graph
to properly configure the lanes configuration.

While it can feel simpler to simply move the csiphy code into standalone
PHY device and just replace camss code with phy_*() calls, it doesn't
solve the proper representation on the data flow and doesn't fix how
the combo mode could be handled.

The solution shared by Vladimir (I think the "csiphy" node should be out
of the camss node), solves all this and will be able to handle more
complex situations with shared resources between "csiphys" and start
moving elements out of the gigantic camss node.

Neil


> 
> Konrad
> 

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Bryan O'Donoghue]

On 21/07/2025 16:46, neil.armstrong@linaro.org wrote:
> On 15/07/2025 11:33, Konrad Dybcio wrote:
>> On 7/15/25 11:20 AM, Vladimir Zapolskiy wrote:
>>> On 7/15/25 12:01, Konrad Dybcio wrote:
>>>> On 7/15/25 8:35 AM, Vladimir Zapolskiy wrote:
>>>>> On 7/15/25 03:13, Bryan O'Donoghue wrote:
>>>>>> On 14/07/2025 16:30, Vladimir Zapolskiy wrote:
>>>>>>>>
>>>>>>>> I think that is genuinely something we should handle in camss-csid.c
>>>>>>>> maybe with some meta-data inside of the ports/endpoints..
>>>>>>>>
>>>>>>>
>>>>>>> This is a CSIPHY property, a CSIPHY hardware configuration and a wiring
>>>>>>> of sensors to a CSIPHY. Where is the relation to CSID here? There is no.
>>>>>>
>>>>>> All the PHY really needs to know is the # of lanes in aggregate, which
>>>>>> physical lanes to map to which logical lanes and the pixel clock.
>>>>>>
>>>>>> We should add additional support to the Kernel's D-PHY API parameters
>>>>>> mechanism to support that physical-to-logical mapping but, that's not
>>>>>> required for this series or for any currently know upstream user of CAMSS.
>>>>>>
>>>>>>> Please share at least a device tree node description, which supports
>>>>>>> a connection of two sensors to a single CSIPHY, like it shall be done
>>>>>>> expectedly.
>>>>>> &camss {
>>>>>>          port@0 {
>>>>>>              csiphy0_lanes01_ep: endpoint0 {
>>>>>>                  data-lanes = <0 1>;
>>>>>>                  remote-endpoint = <&sensor0_ep>;
>>>>>>              };
>>>>>>
>>>>>>              csiphy0_lanes23_ep: endpoint0 {
>>>>>>                  data-lanes = <2 3>;
>>>>>>                  remote-endpoint = <&sensor1_ep>;
>>>>>>              };
>>>>>>           };
>>>>>> };
>>>>>
>>>>> Don't you understand that this is broken?.. That's no good.
>>>>>
>>>>> Please listen and reread the messages given to you above, your proposed
>>>>> "solution" does not support by design a valid hardware setup of two
>>>>> sensors connected to the same CSIPHY.
>>>>>
>>>>> I would propose to stop force pushing an uncorrectable dt scheme, it
>>>>> makes no sense.
>>>>
>>>> If all you're asking for is an ability to grab an of_graph reference
>>>> from the camss (v4l2) driver, you can simply do something along the
>>>> lines of of_graph_get_remote_port(phy->dev->of_node)
>>>>
>>>
>>> It's not about the driver specifics, my comment is about a proper
>>> hardware description in dts notation, please see the device tree node
>>> names.
>>
>> I'm a little lost on what you're trying to argue for..
>>
>> I could make out:
>>
>> 1. "the phy should be a multimedia device"
>> 2. "There is no ports at all, which makes the device tree node unusable,
>>     since you can not provide a way to connect any sensors to the phy."
>>
>> I don't really understand #1.. maybe that could be the case if the PHY
>> has a multitude of tunables (which I don't know if it does, but wouldn't
>> be exactly surprised if it did) that may be usecase/pipeline-specific
>>
>> As for #2, I do think it makes sense to connect the sensors to the PHY,
>> as that's a representation of electrical signals travelling from the
>> producer to the consumer (plus the data passed in e.g. data-lanes is
>> directly related to the PHY and necessarily consumed by its driver)
> 
> The port/endpoint should represent the data flow, and if the signal is the following:
> 
> sensor -> csiphy -> csid

I'll be honest.

I looked at your upstreamed code

drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c 
Documentation/devicetree/bindings/parch/arm64/boot/dts/amlogic/meson-khadas-vim3-ts050.dtsoc/meson-axg.dtsi

And didn't really think CSIPHY needed to be included in the data-graph.

---
bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Bryan O'Donoghue]

On 21/07/2025 17:16, Bryan O'Donoghue wrote:
> drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c Documentation/devicetree/ 
> bindings/parch/arm64/boot/dts/amlogic/meson-khadas-vim3-ts050.dtsoc/ 
> meson-axg.dtsi

Documentation/devicetree/bindings/phDocumentation/devicetree/bindings/phy/amlogic,axg-mipi-dphy.yaml

Rockchip, Broadcom, etc.

The allocation of lanes is known by CAMSS and easily communicated to a 
separate standalone node.

---
bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Bryan O'Donoghue]

On 21/07/2025 17:22, Bryan O'Donoghue wrote:
> On 21/07/2025 17:16, Bryan O'Donoghue wrote:
>> drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c Documentation/ 
>> devicetree/ bindings/parch/arm64/boot/dts/amlogic/meson-khadas-vim3- 
>> ts050.dtsoc/ meson-axg.dtsi
> 
> Documentation/devicetree/bindings/phDocumentation/devicetree/bindings/ 
> phy/amlogic,axg-mipi-dphy.yaml
> 
> Rockchip, Broadcom, etc.
> 
> The allocation of lanes is known by CAMSS and easily communicated to a 
> separate standalone node.
> 
> ---
> bod

In fact its part of the dphy params data structure...

---
bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Neil Armstrong]

On 21/07/2025 18:16, Bryan O'Donoghue wrote:
> On 21/07/2025 16:46, neil.armstrong@linaro.org wrote:
>> On 15/07/2025 11:33, Konrad Dybcio wrote:
>>> On 7/15/25 11:20 AM, Vladimir Zapolskiy wrote:
>>>> On 7/15/25 12:01, Konrad Dybcio wrote:
>>>>> On 7/15/25 8:35 AM, Vladimir Zapolskiy wrote:
>>>>>> On 7/15/25 03:13, Bryan O'Donoghue wrote:
>>>>>>> On 14/07/2025 16:30, Vladimir Zapolskiy wrote:
>>>>>>>>>
>>>>>>>>> I think that is genuinely something we should handle in camss-csid.c
>>>>>>>>> maybe with some meta-data inside of the ports/endpoints..
>>>>>>>>>
>>>>>>>>
>>>>>>>> This is a CSIPHY property, a CSIPHY hardware configuration and a wiring
>>>>>>>> of sensors to a CSIPHY. Where is the relation to CSID here? There is no.
>>>>>>>
>>>>>>> All the PHY really needs to know is the # of lanes in aggregate, which
>>>>>>> physical lanes to map to which logical lanes and the pixel clock.
>>>>>>>
>>>>>>> We should add additional support to the Kernel's D-PHY API parameters
>>>>>>> mechanism to support that physical-to-logical mapping but, that's not
>>>>>>> required for this series or for any currently know upstream user of CAMSS.
>>>>>>>
>>>>>>>> Please share at least a device tree node description, which supports
>>>>>>>> a connection of two sensors to a single CSIPHY, like it shall be done
>>>>>>>> expectedly.
>>>>>>> &camss {
>>>>>>>          port@0 {
>>>>>>>              csiphy0_lanes01_ep: endpoint0 {
>>>>>>>                  data-lanes = <0 1>;
>>>>>>>                  remote-endpoint = <&sensor0_ep>;
>>>>>>>              };
>>>>>>>
>>>>>>>              csiphy0_lanes23_ep: endpoint0 {
>>>>>>>                  data-lanes = <2 3>;
>>>>>>>                  remote-endpoint = <&sensor1_ep>;
>>>>>>>              };
>>>>>>>           };
>>>>>>> };
>>>>>>
>>>>>> Don't you understand that this is broken?.. That's no good.
>>>>>>
>>>>>> Please listen and reread the messages given to you above, your proposed
>>>>>> "solution" does not support by design a valid hardware setup of two
>>>>>> sensors connected to the same CSIPHY.
>>>>>>
>>>>>> I would propose to stop force pushing an uncorrectable dt scheme, it
>>>>>> makes no sense.
>>>>>
>>>>> If all you're asking for is an ability to grab an of_graph reference
>>>>> from the camss (v4l2) driver, you can simply do something along the
>>>>> lines of of_graph_get_remote_port(phy->dev->of_node)
>>>>>
>>>>
>>>> It's not about the driver specifics, my comment is about a proper
>>>> hardware description in dts notation, please see the device tree node
>>>> names.
>>>
>>> I'm a little lost on what you're trying to argue for..
>>>
>>> I could make out:
>>>
>>> 1. "the phy should be a multimedia device"
>>> 2. "There is no ports at all, which makes the device tree node unusable,
>>>     since you can not provide a way to connect any sensors to the phy."
>>>
>>> I don't really understand #1.. maybe that could be the case if the PHY
>>> has a multitude of tunables (which I don't know if it does, but wouldn't
>>> be exactly surprised if it did) that may be usecase/pipeline-specific
>>>
>>> As for #2, I do think it makes sense to connect the sensors to the PHY,
>>> as that's a representation of electrical signals travelling from the
>>> producer to the consumer (plus the data passed in e.g. data-lanes is
>>> directly related to the PHY and necessarily consumed by its driver)
>>
>> The port/endpoint should represent the data flow, and if the signal is the following:
>>
>> sensor -> csiphy -> csid
> 
> I'll be honest.
> 
> I looked at your upstreamed code
> 
> drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c Documentation/devicetree/bindings/parch/arm64/boot/dts/amlogic/meson-khadas-vim3-ts050.dtsoc/meson-axg.dtsi
> 
> And didn't really think CSIPHY needed to be included in the data-graph.

This is DSI, but I understand your point.

The whole key point here is the combo mode, as I understood the combo mode feature
makes the PHY lanes available as 2 separate streams, like if you got 2 "controllers"
attached to the same PHY. So in fact, the PHY should have a single node, but 2 PHY
interfaces in combo mode.

This makes all this controller/phy model very complex to handle and add a lot of
logic in the camss side. Moving the "csiphy" as an independent media device that
can declare up to 2 endpoints in combo mode makes things much simpler, and allows
us to attach each "csiphy" stream to any "controller" side of camss.

Neil

> 
> ---
> bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Bryan O'Donoghue]

On 22/07/2025 09:32, Neil Armstrong wrote:
> The whole key point here is the combo mode, as I understood the combo 
> mode feature
> makes the PHY lanes available as 2 separate streams, like if you got 2 
> "controllers"
> attached to the same PHY. So in fact, the PHY should have a single node, 
> but 2 PHY
> interfaces in combo mode.
> 
> This makes all this controller/phy model very complex to handle and add 
> a lot of
> logic in the camss side. Moving the "csiphy" as an independent media 
> device that
> can declare up to 2 endpoints in combo mode makes things much simpler, 
> and allows
> us to attach each "csiphy" stream to any "controller" side of camss.

I think there should be a generic extension to PHY/linux-media to 
support that instead of something Qualcomm specific.

The first task is qcom/CAMSS specific which is separate the CSIPHYs out 
from the CAMSS block - done in this series and do so in a way that 
doesn't break the existing ABI - done in the context of adding Hamoa/x1e.

The second step should be to extend the existing linux-media and PHY API 
to support multiple sensors on the same CSIPHY in a generic way.

If you want to ship me some hardware, I'll help.

---
bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Neil Armstrong]

On 22/07/2025 11:08, Bryan O'Donoghue wrote:
> On 22/07/2025 09:32, Neil Armstrong wrote:
>> The whole key point here is the combo mode, as I understood the combo mode feature
>> makes the PHY lanes available as 2 separate streams, like if you got 2 "controllers"
>> attached to the same PHY. So in fact, the PHY should have a single node, but 2 PHY
>> interfaces in combo mode.
>>
>> This makes all this controller/phy model very complex to handle and add a lot of
>> logic in the camss side. Moving the "csiphy" as an independent media device that
>> can declare up to 2 endpoints in combo mode makes things much simpler, and allows
>> us to attach each "csiphy" stream to any "controller" side of camss.
> 
> I think there should be a generic extension to PHY/linux-media to support that instead of something Qualcomm specific.

Can you point out what's missing ? AFAIK it's more a matter of proper representation of all
the CAMSS components with a proper ports/endpoint graph design that adding new kernel APIs.

Neil

> 
> The first task is qcom/CAMSS specific which is separate the CSIPHYs out from the CAMSS block - done in this series and do so in a way that doesn't break the existing ABI - done in the context of adding Hamoa/x1e.
> 
> The second step should be to extend the existing linux-media and PHY API to support multiple sensors on the same CSIPHY in a generic way.
> 
> If you want to ship me some hardware, I'll help.
> 
> ---
> bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Bryan O'Donoghue]

On 22/07/2025 10:59, Neil Armstrong wrote:
> On 22/07/2025 11:08, Bryan O'Donoghue wrote:
>> On 22/07/2025 09:32, Neil Armstrong wrote:
>>> The whole key point here is the combo mode, as I understood the combo 
>>> mode feature
>>> makes the PHY lanes available as 2 separate streams, like if you got 
>>> 2 "controllers"
>>> attached to the same PHY. So in fact, the PHY should have a single 
>>> node, but 2 PHY
>>> interfaces in combo mode.
>>>
>>> This makes all this controller/phy model very complex to handle and 
>>> add a lot of
>>> logic in the camss side. Moving the "csiphy" as an independent media 
>>> device that
>>> can declare up to 2 endpoints in combo mode makes things much 
>>> simpler, and allows
>>> us to attach each "csiphy" stream to any "controller" side of camss.
>>
>> I think there should be a generic extension to PHY/linux-media to 
>> support that instead of something Qualcomm specific.
> 
> Can you point out what's missing ? AFAIK it's more a matter of proper 
> representation of all
> the CAMSS components with a proper ports/endpoint graph design that 
> adding new kernel APIs.

Perhaps I'm not understanding the pushback.

Vlad's design puts the CSIPHY nodes under CAMSS and doesn't use the 
upstream PHY API, which if I've understood right is done to facilitate 
multiple sensors on the same CSIPHY.

If the kernel APIs or standard representations of CSIPHYs in the 
upstream kernel are insufficent to facilitate this model, then I think 
that change should be done separately so that all of the existing 
upstream stuff can benefit.

CAMSS should have a standard PHY interface. That's what this series 
provides.

If multiple sensors on the CSIPHY can't fit into that standard model, 
then we need a series to rectify.

I've given an example of how two sensors could be routed to one CSIPHY 
in DT. Another possibility is virtual channels.

I don't know if your sensors support VCs, have you explored that ?

If the message is "we need a custom PHY interface in CAMSS for multiple 
sensors" then I think in fact what that points to additional work that 
needs to be done in CAMSS and perhaps in the kernel linux-media and PHY 
layer to facilitate.

Like I say I'm happy to help you guys do that, ship me some hardware.

---
bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[neil.armstrong]

On 10/07/2025 18:16, Bryan O'Donoghue wrote:
> Add a new MIPI CSI2 driver in D-PHY mode initially. The entire set of
> existing CAMSS CSI PHY init sequences are imported in order to save time
> and effort in later patches.
> 
> In-line with other PHY drivers the process node name is omitted from the
> compat string while the soc name is included.
> 
> At the moment we follow the assignment of lane positions - the bitmap of
> physical input lanes to logical lane numbers as a linear list per the
> existing DPHY @lanes data-member.
> 
> This is fine for us in upstream since we also map the lanes contiguously
> but, our hardware can support different lane mappings so we should in the
> future extend out the DPHY structure to capture the mapping.
> 
> The Qualcomm 3PH class of PHYs can do both D-PHY and C-PHY mode. For now only
> D-PHY is supported.
> 
> In porting some of the logic over from camss-csiphy*.c to here its also
> possible to rationalise some of the code.
> 
> In particular use of regulator_bulk and clk_bulk as well as dropping the
> seemingly useless and unused interrupt handler.
> 
> The PHY sequences and a lot of the logic that goes with them are well proven
> in CAMSS and mature so the main thing to watch out for here is how to get
> the right sequencing of regulators, clocks and register-writes.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   MAINTAINERS                                        |  11 +
>   drivers/phy/qualcomm/Kconfig                       |  11 +
>   drivers/phy/qualcomm/Makefile                      |   6 +
>   drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c | 491 +++++++++++++++++++++
>   drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c     | 281 ++++++++++++
>   drivers/phy/qualcomm/phy-qcom-mipi-csi2.h          | 101 +++++
>   6 files changed, 901 insertions(+)
> 
>
<snip>

> +const struct mipi_csi2phy_clk_freq zero = { 0 };
> +
> +const struct mipi_csi2phy_clk_freq dphy_4nm_x1e_csiphy = {
> +	.freq = {
> +		300000000, 400000000, 480000000
> +	},
> +	.num_freq = 3,
> +};
> +
> +const struct mipi_csi2phy_clk_freq dphy_4nm_x1e_csiphy_timer = {
> +	.freq = {
> +		266666667, 400000000
> +	},
> +	.num_freq = 2,
> +};
> +
> +const struct mipi_csi2phy_soc_cfg mipi_csi2_dphy_4nm_x1e = {
> +	.ops = &phy_qcom_mipi_csi2_ops_3ph_1_0,
> +	.reg_info = {
> +		.init_seq = lane_regs_x1e80100,
> +		.lane_array_size = ARRAY_SIZE(lane_regs_x1e80100),
> +		.offset = 0x1000,
> +		.generation = GEN2,
> +	},
> +	.supply_names = (const char *[]){
> +		"vdda-0p8",
> +		"vdda-1p2"
> +	},
> +	.num_supplies = 2,
> +	.clk_names = (const char *[]) {
> +		"camnoc_axi",
> +		"cpas_ahb",
> +		"csiphy",
> +		"csiphy_timer"
> +	},
> +	.num_clk = 4,
> +	.clk_freq = {
> +		zero,

It seems clang doesn't like this at all:
drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c:486:3: error: initializer element is not a compile-time constant
                 zero,
                 ^~~~
1 error generated.

> +		zero,
> +		dphy_4nm_x1e_csiphy,
> +		dphy_4nm_x1e_csiphy_timer,
> +	},
> +};
<snip>

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Bryan O'Donoghue]

On 12/08/2025 14:39, neil.armstrong@linaro.org wrote:
>> +    .clk_freq = {
>> +        zero,
> 
> It seems clang doesn't like this at all:
> drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c:486:3: error: 
> initializer element is not a compile-time constant
>                  zero,
>                  ^~~~
> 1 error generated.

Weirdly I compile with clang..

thx though

---
bod

Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Neil Armstrong]

On 12/08/2025 17:05, Bryan O'Donoghue wrote:
> On 12/08/2025 14:39, neil.armstrong@linaro.org wrote:
>>> +    .clk_freq = {
>>> +        zero,
>>
>> It seems clang doesn't like this at all:
>> drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c:486:3: error: initializer element is not a compile-time constant
>>                  zero,
>>                  ^~~~
>> 1 error generated.
> 
> Weirdly I compile with clang..

The tuxmake log is available at https://storage.tuxsuite.com/public/linaro/neil.armstrong/builds/31Boss31K6meeYAxqhJUZo8yhwK/build.log

It uses clang-16

Neil

> 
> thx though
> 
> ---
> bod