[PATCH 0/4] phy: rockchip-samsung-dcphy: support the D-PHY receiver direction

[PATCH 0/4] phy: rockchip-samsung-dcphy: support the D-PHY receiver direction

[Jason Yang via B4 Relay]

The Samsung DC-PHY on RK3588 contains both a MIPI transmitter (DSI)
and a MIPI receiver (CSI) with separate register banks and resets.
Mainline currently models only the transmitter. This series adds the
receiver so a CSI camera can use the PHY, including running both
directions concurrently (a DSI panel and a CSI camera on the same PHY
instance).

Design notes, to answer the obvious questions up front:

- Direction as a phy-cell rather than a phy_set_mode() submode:
  The PHY core reference-counts power_on() per struct phy. Bringing TX
  and RX up and down independently requires one struct phy per
  direction; a submode on a single phy cannot express two concurrent
  owners. The extra cell selects which of the two phys a consumer
  gets, following how phy-exynos-mipi-video exposes multiple phys
  from one register block. The existing single-cell binding still
  resolves to the transmitter, ensuring shipped device trees remain
  unaffected.

- BIAS block coordination:
  The BIAS block is part of the PHY's common block and exists once for
  both directions (RK3588 TRM section 22.2), so it must be programmed
  only once. Whichever direction powers on first enables it; the
  first-on/last-off coordination follows phy-exynos-mipi-video.

- RX tuning:
  RX tuning uses fixed RK3588 defaults. Boards that need finer
  receiver tuning should extend phy_configure_opts_mipi_dphy rather
  than add private device-tree properties.

Patch summary:
  patch 1: dt-bindings: optional second #phy-cells cell for direction
  patch 2: prep: symmetric TX power on/off helpers, no functional change
  patch 3: register one struct phy per direction
  patch 4: implement the receiver

Testing:
The series was tested on an IEI WAFER-RK3588 board across the following
configurations:
- Concurrent TX/RX: IMX219 CSI camera capture (640x480, 1640x1232,
  1920x1080, and 3280x2464 in both RAW10 and RAW8) and OV5640 capture
  (1920x1080) through the DC-PHY RX, running concurrently with a MIPI
  DSI panel (ILI9881C, 720x1280) driven through the TX half of the same
  PHY instance.
- Standalone modes: DSI-only and CSI-only configurations.
- Backward compatibility: A shipped single-cell DSI device tree
  resolving unchanged to the transmitter.

HS-video fix that is not part of this submission; however, the receiver
support added here is exercised and verified independently by the
CSI-only capture tests.

Note: The DSI half of the concurrent test required a local dw-mipi-dsi2
---
Jason Yang (4):
      dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys
      phy: rockchip-samsung-dcphy: factor MIPI D-PHY power on/off into helpers
      phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs
      phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support

 .../bindings/phy/rockchip,rk3588-mipi-dcphy.yaml   |  22 +-
 drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c  | 456 +++++++++++++++++++--
 .../dt-bindings/phy/phy-rockchip-samsung-dcphy.h   |  13 +
 3 files changed, 442 insertions(+), 49 deletions(-)
---
base-commit: 1590cf0329716306e948a8fc29f1d3ee87d3989f
change-id: 20260721-dcphy-rx-v1-9af57f3edfd9

Best regards,
-- 
Jason Yang <jason98166@gmail.com>



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys

[Jason Yang via B4 Relay]

From: Jason Yang <jason98166@gmail.com>

The Samsung MIPI D-/C-PHY block on RK3588 drives a MIPI DSI transmitter
and, on the same PHY, can receive a MIPI CSI camera. Describing both as
independent consumers of one PHY requires the direction to be expressed
in the devicetree.

Allow "#phy-cells" to be 1 or 2. The existing single-cell form still
selects the PHY type and refers to the transmitter, so shipped DSI
device trees stay valid. A second cell adds the direction, letting a
DSI (TX) and a CSI (RX) reference the same PHY node as separate
consumers.

Add include/dt-bindings/phy/phy-rockchip-samsung-dcphy.h for the
RK_DCPHY_DIR_TX and RK_DCPHY_DIR_RX direction constants.

Signed-off-by: Jason Yang <jason98166@gmail.com>
Assisted-by: Claude:claude-opus-4-8
---
 .../bindings/phy/rockchip,rk3588-mipi-dcphy.yaml   | 22 ++++++++++++++++------
 .../dt-bindings/phy/phy-rockchip-samsung-dcphy.h   | 13 +++++++++++++
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/rockchip,rk3588-mipi-dcphy.yaml b/Documentation/devicetree/bindings/phy/rockchip,rk3588-mipi-dcphy.yaml
index c8ff5ba22a86..53d695137566 100644
--- a/Documentation/devicetree/bindings/phy/rockchip,rk3588-mipi-dcphy.yaml
+++ b/Documentation/devicetree/bindings/phy/rockchip,rk3588-mipi-dcphy.yaml
@@ -20,12 +20,22 @@ properties:
     maxItems: 1
 
   "#phy-cells":
-    const: 1
+    enum: [1, 2]
     description: |
-      Argument is mode to operate in. Supported modes are:
-        - PHY_TYPE_DPHY
-        - PHY_TYPE_CPHY
-      See include/dt-bindings/phy/phy.h for constants.
+      A single cell selects the PHY type and always refers to the
+      transmitter (DSI) direction. It is kept for backwards compatibility
+      with existing device trees:
+        - <PHY_TYPE_DPHY>
+        - <PHY_TYPE_CPHY>
+
+      Two cells select the PHY type and the direction, so that the
+      transmitter (DSI) and the receiver (CSI) can be described as
+      independent consumers of the same PHY:
+        - <PHY_TYPE_DPHY RK_DCPHY_DIR_TX>
+        - <PHY_TYPE_DPHY RK_DCPHY_DIR_RX>
+
+      See include/dt-bindings/phy/phy.h and
+      include/dt-bindings/phy/phy-rockchip-samsung-dcphy.h for constants.
 
   clocks:
     maxItems: 2
@@ -82,6 +92,6 @@ examples:
                  <&cru SRST_S_MIPI_DCPHY0>;
         reset-names = "m_phy", "apb", "grf", "s_phy";
         rockchip,grf = <&mipidcphy0_grf>;
-        #phy-cells = <1>;
+        #phy-cells = <2>;
       };
     };
diff --git a/include/dt-bindings/phy/phy-rockchip-samsung-dcphy.h b/include/dt-bindings/phy/phy-rockchip-samsung-dcphy.h
new file mode 100644
index 000000000000..60f6630f58b3
--- /dev/null
+++ b/include/dt-bindings/phy/phy-rockchip-samsung-dcphy.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2026 Jason Yang <jason98166@gmail.com>
+ */
+
+#ifndef _DT_BINDINGS_PHY_ROCKCHIP_SAMSUNG_DCPHY_H
+#define _DT_BINDINGS_PHY_ROCKCHIP_SAMSUNG_DCPHY_H
+
+/* Direction of a Rockchip Samsung MIPI D-/C-PHY combo block */
+#define RK_DCPHY_DIR_TX		0
+#define RK_DCPHY_DIR_RX		1
+
+#endif /* _DT_BINDINGS_PHY_ROCKCHIP_SAMSUNG_DCPHY_H */

-- 
2.43.0



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `#phy-cells` property uses `enum: [1, 2]` instead of a `const` constraint, which violates subsystem guidelines and creates a fundamentally broken device tree parsing design.
- [High] The commit introduces a 2-cell format for `#phy-cells` to support independent TX/RX consumers, but the PHY driver actively rejects anything other than 1 cell, resulting in probe failures.
--

--- Patch [1]: [PATCH 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys ---
commit 0077b5ec50ff5c6cc10afd337f193a9f090d6946
Author: Jason Yang <jason98166@gmail.com>

dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys

The Samsung MIPI D-/C-PHY block on RK3588 drives a MIPI DSI transmitter
and, on the same PHY, can receive a MIPI CSI camera. Describing both as
independent consumers of one PHY requires the direction to be expressed
in the devicetree.

> diff --git a/Documentation/devicetree/bindings/phy/rockchip,rk3588-mipi-dcphy.yaml b/Documentation/devicetree/bindings/phy/rockchip,rk3588-mipi-dcphy.yaml
> index c8ff5ba22a86b..53d6951375667 100644
> --- a/Documentation/devicetree/bindings/phy/rockchip,rk3588-mipi-dcphy.yaml
> +++ b/Documentation/devicetree/bindings/phy/rockchip,rk3588-mipi-dcphy.yaml
> @@ -20,12 +20,22 @@ properties:
>      maxItems: 1
>  
>    "#phy-cells":
> -    const: 1
> +    enum: [1, 2]

[Severity: High]
Does this violate the subsystem requirement that cell-count properties must
use a const constraint?

If a board developer updates the PHY provider node to #phy-cells = <2> to use
the new CSI consumer, will any existing DSI consumer relying on 1-cell parsing
fail to probe?

The function of_parse_phandle_with_args() strictly enforces that the consumer's
argument count exactly matches the provider's #phy-cells count.

Additionally, it looks like the driver hasn't been updated to support this yet.

In drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c, the translation callback
samsung_mipi_dcphy_xlate() still explicitly checks for exactly 1 cell:

	if (args->args_count != 1) {
		dev_err(dev, "invalid number of arguments\n");
		return ERR_PTR(-EINVAL);
	}

Will this cause any device tree attempting to use the newly documented 2-cell
format to fail to initialize at runtime?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-dcphy-rx-v1-v1-0-4fc83c0ccac9@gmail.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 2/4] phy: rockchip-samsung-dcphy: factor MIPI D-PHY power on/off into helpers

[Jason Yang via B4 Relay]

From: Jason Yang <jason98166@gmail.com>

To prepare for adding a receiver (CSI) direction next to the existing
transmitter (DSI) one, rename the transmitter power-on helper to
samsung_mipi_dphy_tx_power_on() and gather its teardown into a new
samsung_mipi_dphy_tx_power_off(), so both directions can later be
powered on and off through symmetric helpers.

No functional change intended.

Signed-off-by: Jason Yang <jason98166@gmail.com>
Assisted-by: Claude:claude-opus-4-8
---
 drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
index cbd780556da8..d0d77421bd4b 100644
--- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
@@ -1332,7 +1332,7 @@ samsung_mipi_dphy_data_lane_timing_init(struct samsung_mipi_dcphy *samsung)
 	regmap_write(samsung->regmap, DPHY_MD3_TIME_CON4, 0x1f4);
 }
 
-static int samsung_mipi_dphy_power_on(struct samsung_mipi_dcphy *samsung)
+static int samsung_mipi_dphy_tx_power_on(struct samsung_mipi_dcphy *samsung)
 {
 	int ret;
 
@@ -1358,6 +1358,14 @@ static int samsung_mipi_dphy_power_on(struct samsung_mipi_dcphy *samsung)
 	return 0;
 }
 
+static int samsung_mipi_dphy_tx_power_off(struct samsung_mipi_dcphy *samsung)
+{
+	samsung_mipi_dphy_lane_disable(samsung);
+	samsung_mipi_dcphy_pll_disable(samsung);
+
+	return 0;
+}
+
 static int samsung_mipi_dcphy_power_on(struct phy *phy)
 {
 	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
@@ -1368,7 +1376,7 @@ static int samsung_mipi_dcphy_power_on(struct phy *phy)
 
 	switch (samsung->type) {
 	case PHY_TYPE_DPHY:
-		return samsung_mipi_dphy_power_on(samsung);
+		return samsung_mipi_dphy_tx_power_on(samsung);
 	default:
 		/* CPHY part to be implemented later */
 		return -EOPNOTSUPP;
@@ -1383,16 +1391,11 @@ static int samsung_mipi_dcphy_power_off(struct phy *phy)
 
 	switch (samsung->type) {
 	case PHY_TYPE_DPHY:
-		samsung_mipi_dphy_lane_disable(samsung);
-		break;
+		return samsung_mipi_dphy_tx_power_off(samsung);
 	default:
 		/* CPHY part to be implemented later */
 		return -EOPNOTSUPP;
 	}
-
-	samsung_mipi_dcphy_pll_disable(samsung);
-
-	return 0;
 }
 
 static int

-- 
2.43.0



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs

[Jason Yang via B4 Relay]

From: Jason Yang <jason98166@gmail.com>

The DC-PHY drives a MIPI DSI transmitter and a MIPI CSI receiver, and on
RK3588 both can be wired to the same PHY as independent consumers. The
PHY core reference-counts power_on() per struct phy, so a single struct
phy cannot bring the two directions up independently.

Register one struct phy per direction and move the per-direction state
(direction and PHY type) into its own driver data. of_xlate() accepts
the legacy single cell as the transmitter and an optional second cell
that selects the direction, so existing single-cell DSI device trees
keep resolving to the transmitter.

The shared BIAS block must be programmed only once for the whole block.
Whichever direction powers on first enables it, checking the peer phy's
power_count under a per-provider mutex so that powering one direction on
does not disturb an already running one. The APB reset is issued on the
transmitter bring-up path.
The receiver bring-up itself is added in a later change.

Signed-off-by: Jason Yang <jason98166@gmail.com>
Assisted-by: Claude:claude-opus-4-8
---
 drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c | 133 +++++++++++++++-------
 1 file changed, 95 insertions(+), 38 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
index d0d77421bd4b..0248424929bb 100644
--- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
@@ -5,6 +5,7 @@
  *      Guochun Huang <hero.huang@rock-chips.com>
  */
 
+#include <dt-bindings/phy/phy-rockchip-samsung-dcphy.h>
 #include <dt-bindings/phy/phy.h>
 #include <linux/bitfield.h>
 #include <linux/clk.h>
@@ -13,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
@@ -280,6 +282,16 @@ struct samsung_mipi_dcphy_plat_data {
 	u32 dphy_tx_max_lane_kbps;
 };
 
+struct samsung_mipi_dcphy;
+
+/* One PHY per direction: transmitter (DSI) and receiver (CSI). */
+struct samsung_mipi_dcphy_dir {
+	struct phy *phy;
+	struct samsung_mipi_dcphy *parent;
+	u8 dir;
+	u8 type;
+};
+
 struct samsung_mipi_dcphy {
 	struct device *dev;
 	struct clk *ref_clk;
@@ -291,8 +303,9 @@ struct samsung_mipi_dcphy {
 	struct reset_control *apb_rst;
 	struct reset_control *grf_apb_rst;
 	unsigned int lanes;
-	struct phy *phy;
-	u8 type;
+	struct samsung_mipi_dcphy_dir phys[2];
+	/* Serialises the two directions' access to the shared PHY state. */
+	struct mutex lock;
 
 	const struct samsung_mipi_dcphy_plat_data *pdata;
 	struct {
@@ -1332,13 +1345,26 @@ samsung_mipi_dphy_data_lane_timing_init(struct samsung_mipi_dcphy *samsung)
 	regmap_write(samsung->regmap, DPHY_MD3_TIME_CON4, 0x1f4);
 }
 
-static int samsung_mipi_dphy_tx_power_on(struct samsung_mipi_dcphy *samsung)
+static int samsung_mipi_dphy_tx_power_on(struct samsung_mipi_dcphy *samsung,
+					 struct phy *peer)
 {
+	bool first = peer->power_count == 0;
 	int ret;
 
+	/*
+	 * The shared BIAS block is brought up by whichever direction powers
+	 * on first, leaving an already active peer undisturbed.
+	 */
+	if (first) {
+		reset_control_assert(samsung->apb_rst);
+		udelay(1);
+		reset_control_deassert(samsung->apb_rst);
+	}
+
 	reset_control_assert(samsung->m_phy_rst);
 
-	samsung_mipi_dcphy_bias_block_enable(samsung);
+	if (first)
+		samsung_mipi_dcphy_bias_block_enable(samsung);
 	samsung_mipi_dcphy_pll_configure(samsung);
 	samsung_mipi_dphy_clk_lane_timing_init(samsung);
 	samsung_mipi_dphy_data_lane_timing_init(samsung);
@@ -1368,34 +1394,41 @@ static int samsung_mipi_dphy_tx_power_off(struct samsung_mipi_dcphy *samsung)
 
 static int samsung_mipi_dcphy_power_on(struct phy *phy)
 {
-	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
-
-	reset_control_assert(samsung->apb_rst);
-	udelay(1);
-	reset_control_deassert(samsung->apb_rst);
+	struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);
+	struct samsung_mipi_dcphy *samsung = pd->parent;
+	struct phy *peer = samsung->phys[!pd->dir].phy;
+	int ret;
 
-	switch (samsung->type) {
-	case PHY_TYPE_DPHY:
-		return samsung_mipi_dphy_tx_power_on(samsung);
-	default:
-		/* CPHY part to be implemented later */
+	if (pd->type != PHY_TYPE_DPHY)
 		return -EOPNOTSUPP;
-	}
 
-	return 0;
+	mutex_lock(&samsung->lock);
+	if (pd->dir == RK_DCPHY_DIR_RX)
+		ret = -EOPNOTSUPP;
+	else
+		ret = samsung_mipi_dphy_tx_power_on(samsung, peer);
+	mutex_unlock(&samsung->lock);
+
+	return ret;
 }
 
 static int samsung_mipi_dcphy_power_off(struct phy *phy)
 {
-	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
+	struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);
+	struct samsung_mipi_dcphy *samsung = pd->parent;
+	int ret;
 
-	switch (samsung->type) {
-	case PHY_TYPE_DPHY:
-		return samsung_mipi_dphy_tx_power_off(samsung);
-	default:
-		/* CPHY part to be implemented later */
+	if (pd->type != PHY_TYPE_DPHY)
 		return -EOPNOTSUPP;
-	}
+
+	if (pd->dir == RK_DCPHY_DIR_RX)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&samsung->lock);
+	ret = samsung_mipi_dphy_tx_power_off(samsung);
+	mutex_unlock(&samsung->lock);
+
+	return ret;
 }
 
 static int
@@ -1488,11 +1521,15 @@ samsung_mipi_dcphy_pll_calc_rate(struct samsung_mipi_dcphy *samsung,
 static int samsung_mipi_dcphy_configure(struct phy *phy,
 					union phy_configure_opts *opts)
 {
-	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
+	struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);
+	struct samsung_mipi_dcphy *samsung = pd->parent;
 	unsigned long long target_rate = opts->mipi_dphy.hs_clk_rate;
 
 	samsung->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
 
+	if (pd->dir == RK_DCPHY_DIR_RX)
+		return -EOPNOTSUPP;
+
 	samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
 	opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
 
@@ -1501,16 +1538,16 @@ static int samsung_mipi_dcphy_configure(struct phy *phy,
 
 static int samsung_mipi_dcphy_init(struct phy *phy)
 {
-	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
+	struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);
 
-	return pm_runtime_resume_and_get(samsung->dev);
+	return pm_runtime_resume_and_get(pd->parent->dev);
 }
 
 static int samsung_mipi_dcphy_exit(struct phy *phy)
 {
-	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
+	struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);
 
-	pm_runtime_put(samsung->dev);
+	pm_runtime_put(pd->parent->dev);
 
 	return 0;
 }
@@ -1536,19 +1573,29 @@ static struct phy *samsung_mipi_dcphy_xlate(struct device *dev,
 					    const struct of_phandle_args *args)
 {
 	struct samsung_mipi_dcphy *samsung = dev_get_drvdata(dev);
+	struct samsung_mipi_dcphy_dir *pd;
+	u8 dir = RK_DCPHY_DIR_TX;
 
-	if (args->args_count != 1) {
+	if (args->args_count < 1 || args->args_count > 2) {
 		dev_err(dev, "invalid number of arguments\n");
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (samsung->type != PHY_NONE && samsung->type != args->args[0])
-		dev_warn(dev, "phy type select %d overwriting type %d\n",
-			 args->args[0], samsung->type);
+	if (args->args_count == 2) {
+		if (args->args[1] > RK_DCPHY_DIR_RX) {
+			dev_err(dev, "invalid direction %u\n", args->args[1]);
+			return ERR_PTR(-EINVAL);
+		}
+		dir = args->args[1];
+	}
 
-	samsung->type = args->args[0];
+	pd = &samsung->phys[dir];
+	if (pd->type != PHY_NONE && pd->type != args->args[0])
+		dev_warn(dev, "phy type select %u overwriting type %u\n",
+			 args->args[0], pd->type);
+	pd->type = args->args[0];
 
-	return samsung->phy;
+	return pd->phy;
 }
 
 static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
@@ -1559,6 +1606,7 @@ static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
 	struct phy_provider *phy_provider;
 	struct resource *res;
 	void __iomem *regs;
+	unsigned int i;
 	int ret;
 
 	samsung = devm_kzalloc(dev, sizeof(*samsung), GFP_KERNEL);
@@ -1568,6 +1616,7 @@ static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
 	samsung->dev = dev;
 	samsung->pdata = device_get_match_data(dev);
 	platform_set_drvdata(pdev, samsung);
+	mutex_init(&samsung->lock);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	regs = devm_ioremap_resource(dev, res);
@@ -1613,11 +1662,19 @@ static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(samsung->grf_apb_rst),
 				     "Failed to get system grf_apb_rst control\n");
 
-	samsung->phy = devm_phy_create(dev, NULL, &samsung_mipi_dcphy_ops);
-	if (IS_ERR(samsung->phy))
-		return dev_err_probe(dev, PTR_ERR(samsung->phy), "Failed to create MIPI DC-PHY\n");
+	for (i = 0; i < ARRAY_SIZE(samsung->phys); i++) {
+		struct phy *phy = devm_phy_create(dev, NULL,
+						  &samsung_mipi_dcphy_ops);
 
-	phy_set_drvdata(samsung->phy, samsung);
+		if (IS_ERR(phy))
+			return dev_err_probe(dev, PTR_ERR(phy),
+					     "Failed to create MIPI DC-PHY\n");
+
+		samsung->phys[i].phy = phy;
+		samsung->phys[i].parent = samsung;
+		samsung->phys[i].dir = i;
+		phy_set_drvdata(phy, &samsung->phys[i]);
+	}
 
 	ret = devm_pm_runtime_enable(dev);
 	if (ret)

-- 
2.43.0



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Concurrent or unsupported PHY configuration clobbers the shared `samsung->lanes` driver state before verifying the direction is supported.
--

--- Patch [3]: [PATCH 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs ---
commit 242b9435d09e0133de4658156f3cc224aa2f84c9
Author: Jason Yang <jason98166@gmail.com>

phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs

This commit models the MIPI DSI transmitter and MIPI CSI receiver as separate
PHYs that share the same hardware block. It registers one struct phy per
direction and adds a per-provider mutex to serialize access to the shared
state.

> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c

[ ... ]

> @@ -1488,11 +1521,15 @@ samsung_mipi_dcphy_pll_calc_rate(struct samsung_mipi_dcphy *samsung,
>  static int samsung_mipi_dcphy_configure(struct phy *phy,
>  					union phy_configure_opts *opts)
>  {
> -	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
> +	struct samsung_mipi_dcphy_dir *pd = phy_get_drvdata(phy);
> +	struct samsung_mipi_dcphy *samsung = pd->parent;
>  	unsigned long long target_rate = opts->mipi_dphy.hs_clk_rate;
>  
>  	samsung->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
>  
> +	if (pd->dir == RK_DCPHY_DIR_RX)
> +		return -EOPNOTSUPP;
> +

[Severity: High]
Does this code modify the shared samsung->lanes state before checking if
the direction is supported?

Since this is done without holding samsung->lock, can a consumer
configuring the RX PHY clobber the active TX PHY's lane count?

If the lane count is corrupted, it looks like it could lead to incorrect lane
power-down sequences when the TX PHY is disabled later, potentially leaving
MIPI lanes actively driven.

Should the pd->dir check and the lock acquisition happen before updating
samsung->lanes?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-dcphy-rx-v1-v1-0-4fc83c0ccac9@gmail.com?part=3

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support

[Jason Yang via B4 Relay]

From: Jason Yang <jason98166@gmail.com>

Implement the receiver (CSI) direction of the D-PHY. Add the RX
register layout and the HS-frequency settle table, and hook the
receiver bring-up and teardown into the per-direction power_on(),
power_off() and configure() paths, all serialized on the provider
mutex alongside the transmitter.

Only the D-PHY receiver is implemented, with fixed RK3588 tuning
defaults. The RX register layout and field definitions follow the
RK3588 TRM (section 22.4); what the TRM does not publish are the
per-data-rate HS settle values (it points to a separate timing-table
supplement and gives only a single example point) and the analog
tuning values, both of which are taken from the Rockchip vendor
kernel. The PLL is left untouched on the receive path: the sensor
supplies the link clock, and the TRM start-up sequence (figure 22-8)
allows the PLL sequence to be skipped when only the receiver (slave)
lanes are used. configure() only records the incoming rate so the
settle table can be indexed.

Signed-off-by: Jason Yang <jason98166@gmail.com>
Assisted-by: Claude:claude-opus-4-8
---
 drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c | 330 +++++++++++++++++++++-
 1 file changed, 320 insertions(+), 10 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
index 0248424929bb..49243ba93bd0 100644
--- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
@@ -237,6 +237,63 @@
 #define DPHY_MD3_TIME_CON4	0x0740
 #define DPHY_MD3_DATA_CON0	0x0744
 
+/* D-PHY receiver direction registers (clock lane + four data lanes) */
+#define DPHY_SC_GNR_CON0	0x0b00
+#define DPHY_SC_GNR_CON1	0x0b04
+#define DPHY_SC_ANA_CON1	0x0b0c
+#define HS_RX_BIAS_CON(x)	FIELD_PREP(GENMASK(15, 11), x)
+#define DPHY_SC_ANA_CON2	0x0b10
+#define HS_TERM_SW(x)		FIELD_PREP(GENMASK(2, 0), x)
+#define DPHY_SC_ANA_CON3	0x0b14
+#define ULPS_HYS_SW_DPHY(x)	FIELD_PREP(GENMASK(10, 8), x)
+#define DPHY_SC_TIME_CON0	0x0b30
+#define T_CLK_SETTLE(x)		FIELD_PREP(GENMASK(7, 0), x)
+#define T_CLK_MISS(x)		FIELD_PREP(GENMASK(11, 8), x)
+#define COMBO_SD0_GNR_CON0	0x0c00
+#define COMBO_SD0_GNR_CON1	0x0c04
+#define COMBO_SD0_ANA_CON1	0x0c0c
+#define COMBO_SD0_ANA_CON2	0x0c10
+#define SKEW_DLYSEL(x)		FIELD_PREP(GENMASK(9, 8), x)
+#define RX_TERM_SW(x)		FIELD_PREP(GENMASK(2, 0), x)
+#define COMBO_SD0_ANA_CON3	0x0c14
+#define SEL_ESCPOL		BIT(11)
+#define LP_HYS_SW(x)		FIELD_PREP(GENMASK(5, 4), x)
+#define COMBO_SD0_ANA_CON7	0x0c24
+#define CLK_DBL_CTRL(x)		FIELD_PREP(GENMASK(7, 6), x)
+#define COMBO_SD0_TIME_CON0	0x0c30
+#define T_HS_SETTLE(x)		FIELD_PREP(GENMASK(7, 0), x)
+#define SETTLE_CLK_SEL		BIT(8)
+#define COMBO_SD0_TIME_CON1	0x0c34
+#define T_ERR_SOT_SYNC(x)	FIELD_PREP(GENMASK(7, 0), x)
+#define COMBO_SD0_DESKEW_CON2	0x0c48
+#define SKEW_CAL_CLK_COARSE_SET(x) FIELD_PREP(GENMASK(4, 0), x)
+#define COMBO_SD1_GNR_CON0	0x0d00
+#define COMBO_SD1_GNR_CON1	0x0d04
+#define COMBO_SD1_ANA_CON1	0x0d0c
+#define COMBO_SD1_ANA_CON2	0x0d10
+#define COMBO_SD1_ANA_CON3	0x0d14
+#define COMBO_SD1_ANA_CON7	0x0d24
+#define COMBO_SD1_TIME_CON0	0x0d30
+#define COMBO_SD1_TIME_CON1	0x0d34
+#define COMBO_SD1_DESKEW_CON2	0x0d48
+#define COMBO_SD2_GNR_CON0	0x0e00
+#define COMBO_SD2_GNR_CON1	0x0e04
+#define COMBO_SD2_ANA_CON1	0x0e0c
+#define COMBO_SD2_ANA_CON2	0x0e10
+#define COMBO_SD2_ANA_CON3	0x0e14
+#define COMBO_SD2_ANA_CON7	0x0e24
+#define COMBO_SD2_TIME_CON0	0x0e30
+#define COMBO_SD2_TIME_CON1	0x0e34
+#define COMBO_SD2_DESKEW_CON2	0x0e48
+#define DPHY_SD3_GNR_CON0	0x0f00
+#define DPHY_SD3_GNR_CON1	0x0f04
+#define DPHY_SD3_ANA_CON1	0x0f0c
+#define DPHY_SD3_ANA_CON2	0x0f10
+#define DPHY_SD3_ANA_CON3	0x0f14
+#define DPHY_SD3_TIME_CON0	0x0f30
+#define DPHY_SD3_TIME_CON1	0x0f34
+#define DPHY_SD3_DESKEW_CON2	0x0f48
+
 #define T_LP_EXIT_SKEW(x)	FIELD_PREP(GENMASK(3, 2), x)
 #define T_LP_ENTRY_SKEW(x)	FIELD_PREP(GENMASK(1, 0), x)
 #define T_HS_ZERO(x)		FIELD_PREP(GENMASK(15, 8), x)
@@ -303,6 +360,7 @@ struct samsung_mipi_dcphy {
 	struct reset_control *apb_rst;
 	struct reset_control *grf_apb_rst;
 	unsigned int lanes;
+	unsigned long long hs_clk_rate;
 	struct samsung_mipi_dcphy_dir phys[2];
 	/* Serialises the two directions' access to the shared PHY state. */
 	struct mutex lock;
@@ -986,6 +1044,210 @@ struct samsung_mipi_dphy_timing samsung_mipi_dphy_timing_table[] = {
 	{  80,  2,   0,  0, 28,  5,  0, 22,  2,  0,  5},
 };
 
+/* D-PHY receiver HS-RX configuration lookup */
+struct samsung_mipi_dphy_rx_hsfreq_range {
+	u32 range_h_mbps;
+	u16 cfg_bit;
+};
+
+/*
+ * HS RX settle values taken from the rk3588 vendor kernel. The TRM
+ * defines these fields but does not publish the per-data-rate table
+ * (it defers to a separate timing-table supplement). Each cfg_bit is a
+ * pre-combined SETTLE_CLK_SEL | T_HS_SETTLE value written to the
+ * data-lane TIME_CON0. Sorted by .range_h_mbps ascending.
+ */
+static const struct samsung_mipi_dphy_rx_hsfreq_range samsung_mipi_dphy_rx_hsfreq_ranges[] = {
+	{   80, 0x105 }, {  100, 0x106 }, {  120, 0x107 }, {  140, 0x108 },
+	{  160, 0x109 }, {  180, 0x10a }, {  200, 0x10b }, {  220, 0x10c },
+	{  240, 0x10d }, {  270, 0x10e }, {  290, 0x10f }, {  310, 0x110 },
+	{  330, 0x111 }, {  350, 0x112 }, {  370, 0x113 }, {  390, 0x114 },
+	{  410, 0x115 }, {  430, 0x116 }, {  450, 0x117 }, {  470, 0x118 },
+	{  490, 0x119 }, {  510, 0x11a }, {  540, 0x11b }, {  560, 0x11c },
+	{  580, 0x11d }, {  600, 0x11e }, {  620, 0x11f }, {  640, 0x120 },
+	{  660, 0x121 }, {  680, 0x122 }, {  700, 0x123 }, {  720, 0x124 },
+	{  740, 0x125 }, {  760, 0x126 }, {  790, 0x127 }, {  810, 0x128 },
+	{  830, 0x129 }, {  850, 0x12a }, {  870, 0x12b }, {  890, 0x12c },
+	{  910, 0x12d }, {  930, 0x12e }, {  950, 0x12f }, {  970, 0x130 },
+	{  990, 0x131 }, { 1010, 0x132 }, { 1030, 0x133 }, { 1060, 0x134 },
+	{ 1080, 0x135 }, { 1100, 0x136 }, { 1120, 0x137 }, { 1140, 0x138 },
+	{ 1160, 0x139 }, { 1180, 0x13a }, { 1200, 0x13b }, { 1220, 0x13c },
+	{ 1240, 0x13d }, { 1260, 0x13e }, { 1280, 0x13f }, { 1310, 0x140 },
+	{ 1330, 0x141 }, { 1350, 0x142 }, { 1370, 0x143 }, { 1390, 0x144 },
+	{ 1410, 0x145 }, { 1430, 0x146 }, { 1450, 0x147 }, { 1470, 0x148 },
+	{ 1490, 0x149 }, { 1580, 0x007 }, { 1740, 0x008 }, { 1910, 0x009 },
+	{ 2070, 0x00a }, { 2240, 0x00b }, { 2410, 0x00c }, { 2570, 0x00d },
+	{ 2740, 0x00e }, { 2910, 0x00f }, { 3070, 0x010 }, { 3240, 0x011 },
+	{ 3410, 0x012 }, { 3570, 0x013 }, { 3740, 0x014 }, { 3890, 0x015 },
+	{ 4070, 0x016 }, { 4240, 0x017 }, { 4400, 0x018 }, { 4500, 0x019 },
+};
+
+static int samsung_mipi_dphy_rx_lookup_hsfreq(u32 lane_mbps, u16 *cfg_bit)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(samsung_mipi_dphy_rx_hsfreq_ranges); i++) {
+		if (lane_mbps <= samsung_mipi_dphy_rx_hsfreq_ranges[i].range_h_mbps) {
+			*cfg_bit = samsung_mipi_dphy_rx_hsfreq_ranges[i].cfg_bit;
+			return 0;
+		}
+	}
+
+	return -ERANGE;
+}
+
+/* SKEW_DLYSEL per data rate, from the RK3588 TRM ANA_CON2 description */
+static u32 samsung_mipi_dphy_rx_data_lane_dlysel(u32 lane_mbps)
+{
+	if (lane_mbps < 1500)
+		return 0;
+	if (lane_mbps < 2000)
+		return SKEW_DLYSEL(3);
+	if (lane_mbps < 3000)
+		return SKEW_DLYSEL(2);
+	if (lane_mbps < 4000)
+		return SKEW_DLYSEL(1);
+	return 0;
+}
+
+/*
+ * Per-data-lane register bases, indexed by lane number. The clock lane is
+ * programmed separately; its enable/settle registers do not follow this
+ * regular spacing.
+ */
+static const u32 rx_data_lane_gnr_con0[] = {
+	COMBO_SD0_GNR_CON0, COMBO_SD1_GNR_CON0,
+	COMBO_SD2_GNR_CON0, DPHY_SD3_GNR_CON0,
+};
+
+static const u32 rx_lane_time_con0[] = {
+	COMBO_SD0_TIME_CON0, COMBO_SD1_TIME_CON0,
+	COMBO_SD2_TIME_CON0, DPHY_SD3_TIME_CON0,
+};
+
+static const u32 rx_lane_time_con1[] = {
+	COMBO_SD0_TIME_CON1, COMBO_SD1_TIME_CON1,
+	COMBO_SD2_TIME_CON1, DPHY_SD3_TIME_CON1,
+};
+
+/*
+ * These RX analog tuning values come from the vendor BSP; the
+ * termination (RX_TERM_SW/HS_TERM_SW = 2, 96.6 ohm) additionally
+ * matches the TRM ANA_CON2 default.
+ */
+static void samsung_mipi_dphy_rx_config_common(struct samsung_mipi_dcphy *samsung)
+{
+	u32 dlysel = samsung_mipi_dphy_rx_data_lane_dlysel(div_u64(samsung->hs_clk_rate,
+							      1000000));
+	u32 ana_con2_common = dlysel | RX_TERM_SW(2);
+
+	/* Clock lane */
+	regmap_write(samsung->regmap, DPHY_SC_GNR_CON1, T_PHY_READY(0x1450));
+	regmap_write(samsung->regmap, DPHY_SC_ANA_CON1, HS_RX_BIAS_CON(0x10));
+	regmap_write(samsung->regmap, DPHY_SC_ANA_CON2, HS_TERM_SW(2));
+	regmap_write(samsung->regmap, DPHY_SC_ANA_CON3, ULPS_HYS_SW_DPHY(6));
+
+	/* Data lane 0 */
+	if (samsung->lanes > 0) {
+		regmap_write(samsung->regmap, COMBO_SD0_GNR_CON1, T_PHY_READY(0x1450));
+		regmap_write(samsung->regmap, COMBO_SD0_ANA_CON1, HS_RX_BIAS_CON(0x10));
+		regmap_write(samsung->regmap, COMBO_SD0_ANA_CON2, ana_con2_common);
+		regmap_write(samsung->regmap, COMBO_SD0_ANA_CON3,
+			     ULPS_HYS_SW_DPHY(6) | LP_HYS_SW(3) | SEL_ESCPOL);
+		regmap_write(samsung->regmap, COMBO_SD0_ANA_CON7, CLK_DBL_CTRL(1));
+		regmap_write(samsung->regmap, COMBO_SD0_DESKEW_CON2, SKEW_CAL_CLK_COARSE_SET(0));
+	}
+
+	/* Data lane 1 */
+	if (samsung->lanes > 1) {
+		regmap_write(samsung->regmap, COMBO_SD1_GNR_CON1, T_PHY_READY(0x1450));
+		regmap_write(samsung->regmap, COMBO_SD1_ANA_CON1, HS_RX_BIAS_CON(0x10));
+		regmap_write(samsung->regmap, COMBO_SD1_ANA_CON2, ana_con2_common);
+		regmap_write(samsung->regmap, COMBO_SD1_ANA_CON3, ULPS_HYS_SW_DPHY(6));
+		regmap_write(samsung->regmap, COMBO_SD1_ANA_CON7, CLK_DBL_CTRL(1));
+		regmap_write(samsung->regmap, COMBO_SD1_DESKEW_CON2, SKEW_CAL_CLK_COARSE_SET(3));
+	}
+
+	/* Data lane 2 */
+	if (samsung->lanes > 2) {
+		regmap_write(samsung->regmap, COMBO_SD2_GNR_CON1, T_PHY_READY(0x1450));
+		regmap_write(samsung->regmap, COMBO_SD2_ANA_CON1, HS_RX_BIAS_CON(0x10));
+		regmap_write(samsung->regmap, COMBO_SD2_ANA_CON2, ana_con2_common);
+		regmap_write(samsung->regmap, COMBO_SD2_ANA_CON3, ULPS_HYS_SW_DPHY(6));
+		regmap_write(samsung->regmap, COMBO_SD2_ANA_CON7, CLK_DBL_CTRL(1));
+		regmap_write(samsung->regmap, COMBO_SD2_DESKEW_CON2, SKEW_CAL_CLK_COARSE_SET(3));
+	}
+
+	/* Data lane 3 */
+	if (samsung->lanes > 3) {
+		regmap_write(samsung->regmap, DPHY_SD3_GNR_CON1, T_PHY_READY(0x1450));
+		regmap_write(samsung->regmap, DPHY_SD3_ANA_CON1, HS_RX_BIAS_CON(0x10));
+		regmap_write(samsung->regmap, DPHY_SD3_ANA_CON2, ana_con2_common);
+		regmap_write(samsung->regmap, DPHY_SD3_ANA_CON3, ULPS_HYS_SW_DPHY(6));
+		regmap_write(samsung->regmap, DPHY_SD3_DESKEW_CON2, SKEW_CAL_CLK_COARSE_SET(3));
+	}
+}
+
+static int samsung_mipi_dphy_rx_config_settle(struct samsung_mipi_dcphy *samsung)
+{
+	u32 lane_mbps = div_u64(samsung->hs_clk_rate, 1000000);
+	u16 cfg_bit;
+	int ret, i;
+
+	ret = samsung_mipi_dphy_rx_lookup_hsfreq(lane_mbps, &cfg_bit);
+	if (ret) {
+		dev_err(samsung->dev,
+			"no RX hsfreq cfg for %u Mbps (limit ~4500 Mbps)\n",
+			lane_mbps);
+		return ret;
+	}
+
+	/*
+	 * Clock-lane settle uses the fixed value from the TRM RX bring-up
+	 * example, unlike the per-rate data-lane settle below.
+	 */
+	regmap_write(samsung->regmap, DPHY_SC_TIME_CON0,
+		     T_CLK_SETTLE(0x01) | T_CLK_MISS(0x03));
+
+	for (i = 0; i < samsung->lanes; i++) {
+		regmap_update_bits(samsung->regmap, rx_lane_time_con0[i],
+				   T_HS_SETTLE(0xff) | SETTLE_CLK_SEL, cfg_bit);
+		regmap_update_bits(samsung->regmap, rx_lane_time_con1[i],
+				   T_ERR_SOT_SYNC(0xff), T_ERR_SOT_SYNC(0x03));
+	}
+
+	return 0;
+}
+
+static int samsung_mipi_dphy_rx_lane_enable(struct samsung_mipi_dcphy *samsung)
+{
+	u32 sts;
+	int ret, i;
+
+	regmap_update_bits(samsung->regmap, DPHY_SC_GNR_CON0, PHY_ENABLE, PHY_ENABLE);
+	for (i = 0; i < samsung->lanes; i++)
+		regmap_update_bits(samsung->regmap, rx_data_lane_gnr_con0[i],
+				   PHY_ENABLE, PHY_ENABLE);
+
+	ret = regmap_read_poll_timeout(samsung->regmap, DPHY_SC_GNR_CON0,
+				       sts, sts & PHY_READY, 200, 4000);
+	if (ret) {
+		dev_err(samsung->dev, "RX clock lane not ready\n");
+		return ret;
+	}
+
+	for (i = 0; i < samsung->lanes; i++) {
+		ret = regmap_read_poll_timeout(samsung->regmap, rx_data_lane_gnr_con0[i],
+					       sts, sts & PHY_READY, 200, 2000);
+		if (ret) {
+			dev_err(samsung->dev, "RX data lane %d not ready\n", i);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static void samsung_mipi_dcphy_bias_block_enable(struct samsung_mipi_dcphy *samsung)
 {
 	regmap_write(samsung->regmap, BIAS_CON0, I_DEV_DIV_6 | I_RES_100_2UA);
@@ -1002,6 +1264,50 @@ static void samsung_mipi_dcphy_bias_block_enable(struct samsung_mipi_dcphy *sams
 			   I_MUX_SEL_MASK, I_MUX_400MV);
 }
 
+static int samsung_mipi_dphy_rx_power_on(struct samsung_mipi_dcphy *samsung,
+					 struct phy *peer)
+{
+	bool first = peer->power_count == 0;
+	int ret;
+
+	if (!samsung->hs_clk_rate)
+		return -EINVAL;
+
+	reset_control_assert(samsung->s_phy_rst);
+
+	if (first)
+		samsung_mipi_dcphy_bias_block_enable(samsung);
+	samsung_mipi_dphy_rx_config_common(samsung);
+
+	ret = samsung_mipi_dphy_rx_config_settle(samsung);
+	if (ret)
+		goto out_deassert;
+
+	ret = samsung_mipi_dphy_rx_lane_enable(samsung);
+
+out_deassert:
+	reset_control_deassert(samsung->s_phy_rst);
+
+	return ret;
+}
+
+static int samsung_mipi_dphy_rx_power_off(struct samsung_mipi_dcphy *samsung)
+{
+	int i;
+
+	reset_control_assert(samsung->s_phy_rst);
+
+	regmap_update_bits(samsung->regmap, DPHY_SC_GNR_CON0, PHY_ENABLE, 0);
+	for (i = 0; i < samsung->lanes; i++)
+		regmap_update_bits(samsung->regmap, rx_data_lane_gnr_con0[i],
+				   PHY_ENABLE, 0);
+
+	reset_control_deassert(samsung->s_phy_rst);
+	usleep_range(500, 1000);
+
+	return 0;
+}
+
 static void samsung_mipi_dphy_lane_enable(struct samsung_mipi_dcphy *samsung)
 {
 	regmap_write(samsung->regmap, DPHY_MC_GNR_CON1, T_PHY_READY(0x2000));
@@ -1404,7 +1710,7 @@ static int samsung_mipi_dcphy_power_on(struct phy *phy)
 
 	mutex_lock(&samsung->lock);
 	if (pd->dir == RK_DCPHY_DIR_RX)
-		ret = -EOPNOTSUPP;
+		ret = samsung_mipi_dphy_rx_power_on(samsung, peer);
 	else
 		ret = samsung_mipi_dphy_tx_power_on(samsung, peer);
 	mutex_unlock(&samsung->lock);
@@ -1421,11 +1727,11 @@ static int samsung_mipi_dcphy_power_off(struct phy *phy)
 	if (pd->type != PHY_TYPE_DPHY)
 		return -EOPNOTSUPP;
 
-	if (pd->dir == RK_DCPHY_DIR_RX)
-		return -EOPNOTSUPP;
-
 	mutex_lock(&samsung->lock);
-	ret = samsung_mipi_dphy_tx_power_off(samsung);
+	if (pd->dir == RK_DCPHY_DIR_RX)
+		ret = samsung_mipi_dphy_rx_power_off(samsung);
+	else
+		ret = samsung_mipi_dphy_tx_power_off(samsung);
 	mutex_unlock(&samsung->lock);
 
 	return ret;
@@ -1525,13 +1831,17 @@ static int samsung_mipi_dcphy_configure(struct phy *phy,
 	struct samsung_mipi_dcphy *samsung = pd->parent;
 	unsigned long long target_rate = opts->mipi_dphy.hs_clk_rate;
 
+	mutex_lock(&samsung->lock);
 	samsung->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
 
-	if (pd->dir == RK_DCPHY_DIR_RX)
-		return -EOPNOTSUPP;
-
-	samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
-	opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
+	if (pd->dir == RK_DCPHY_DIR_RX) {
+		/* The sensor supplies the link clock; the PLL stays off. */
+		samsung->hs_clk_rate = target_rate;
+	} else {
+		samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
+		opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
+	}
+	mutex_unlock(&samsung->lock);
 
 	return 0;
 }

-- 
2.43.0



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The RX power-on sequence fails to toggle the `apb_rst` reset line, potentially leaving the shared APB bus interface uninitialized if the RX direction is powered on before TX.
- [High] If `samsung_mipi_dphy_rx_lane_enable()` fails (e.g., due to a PHY_READY timeout), it returns an error but leaves the `PHY_ENABLE` bits set in the PHY registers and unconditionally deasserts the `s_phy_rst` core reset.
- [High] The `lanes` configuration field is shared between TX and RX, leading to state corruption and incorrect lane cleanup if both directions are configured independently.

Pre-existing issues:
- [High] Concurrent power_on calls for TX and RX can race due to a time-of-check to time-of-use (TOCTOU) vulnerability on the unprotected `peer->power_count`.
--

--- Patch [4]: [PATCH 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support ---
commit 1a7ff981a5098deb181df94fdb6e07f8dc4040a9
Author: Jason Yang <jason98166@gmail.com>
phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support

This commit implements the receiver (CSI) direction of the D-PHY. It adds the
RX register layout, the HS-frequency settle table, and hooks the receiver
bring-up and teardown into the per-direction power and configuration paths.

> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c

[ ... ]

> +static int samsung_mipi_dphy_rx_lane_enable(struct samsung_mipi_dcphy *samsung)
> +{
> +	u32 sts;
> +	int ret, i;
> +
> +	regmap_update_bits(samsung->regmap, DPHY_SC_GNR_CON0, PHY_ENABLE, PHY_ENABLE);
> +	for (i = 0; i < samsung->lanes; i++)
> +		regmap_update_bits(samsung->regmap, rx_data_lane_gnr_con0[i],
> +				   PHY_ENABLE, PHY_ENABLE);
> +
> +	ret = regmap_read_poll_timeout(samsung->regmap, DPHY_SC_GNR_CON0,
> +				       sts, sts & PHY_READY, 200, 4000);
> +	if (ret) {
> +		dev_err(samsung->dev, "RX clock lane not ready\n");
> +		return ret;
> +	}

[Severity: High]
If regmap_read_poll_timeout() fails here, do we leak the previously set
PHY_ENABLE bits?

Returning early leaves the PHY_ENABLE bits set in the PHY registers.
The caller catches the error and propagates it, meaning the framework
will not call power_off() to clean up. On subsequent retries, writing
PHY_ENABLE to a register where it is already 1 will fail to produce the
0 -> 1 edge transition required by the PHY state machine to start, breaking
future attempts to initialize the RX lanes.

[ ... ]

> +static int samsung_mipi_dphy_rx_power_on(struct samsung_mipi_dcphy *samsung,
> +					 struct phy *peer)
> +{
> +	bool first = peer->power_count == 0;

[Severity: High]
This is a pre-existing issue, but is it safe to rely on peer->power_count
here to determine if shared initialization is needed?

The PHY core framework increments phy->power_count outside the driver's lock,
only after the power_on() callback returns. If Thread A (RX) and Thread B
(TX) call phy_power_on() concurrently:

Thread A configures RX, drops samsung->lock, but is preempted before
rx_phy->power_count is incremented.
Thread B acquires samsung->lock, sees rx_phy->power_count is still 0, and
executes its 'first' initialization sequence.

For TX, this involves explicitly asserting the shared APB bus reset
(apb_rst), which would wipe out the entire RX configuration Thread A just
applied. Should the driver track the initialization state internally instead
of relying on power_count?

> +	int ret;
> +
> +	if (!samsung->hs_clk_rate)
> +		return -EINVAL;
> +
> +	reset_control_assert(samsung->s_phy_rst);
> +
> +	if (first)
> +		samsung_mipi_dcphy_bias_block_enable(samsung);

[Severity: High]
If the PHY is used in a receive-first or receive-only role, does this sequence
omit the apb_rst hardware initialization for the shared APB bus?

Looking at the TX power-on sequence, it toggles apb_rst when first is true to
ensure the shared APB bus interface is properly initialized. If RX powers on
first, the apb_rst toggle is skipped here, and a subsequent TX power-on will
see first == false and also skip it. Can this leave the shared APB bus
interface uninitialized and potentially cause system hangs during regmap
accesses?

> +	samsung_mipi_dphy_rx_config_common(samsung);
> +
> +	ret = samsung_mipi_dphy_rx_config_settle(samsung);
> +	if (ret)
> +		goto out_deassert;
> +
> +	ret = samsung_mipi_dphy_rx_lane_enable(samsung);
> +
> +out_deassert:
> +	reset_control_deassert(samsung->s_phy_rst);
> +
> +	return ret;
> +}

[Severity: High]
Does this unconditionally commit the leaked PHY_ENABLE state to the hardware?

If samsung_mipi_dphy_rx_lane_enable() fails due to the timeout mentioned
earlier, it drops through to here and deasserts s_phy_rst. Combined with the
missing PHY_ENABLE cleanup, this commits the broken hardware state.

[ ... ]

> @@ -1525,13 +1831,17 @@ static int samsung_mipi_dcphy_configure(struct phy *phy,
>  	struct samsung_mipi_dcphy *samsung = pd->parent;
>  	unsigned long long target_rate = opts->mipi_dphy.hs_clk_rate;
>  
> +	mutex_lock(&samsung->lock);
>  	samsung->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;

[Severity: High]
Does sharing the samsung->lanes variable between TX and RX lead to state
corruption?

Since the driver models TX and RX as two independent PHY instances, either
instance will blindly overwrite samsung->lanes here. If a consumer configures
TX with 4 lanes and a second consumer configures RX with 2 lanes,
samsung->lanes becomes 2. A subsequent TX power_off() will read this state
and only disable 2 lanes, leaking the other 2 active TX lanes. Should lanes
be tracked per-direction?

> -	if (pd->dir == RK_DCPHY_DIR_RX)
> -		return -EOPNOTSUPP;
> -
> -	samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
> -	opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
> +	if (pd->dir == RK_DCPHY_DIR_RX) {
> +		/* The sensor supplies the link clock; the PLL stays off. */
> +		samsung->hs_clk_rate = target_rate;
> +	} else {
> +		samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
> +		opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
> +	}
> +	mutex_unlock(&samsung->lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-dcphy-rx-v1-v1-0-4fc83c0ccac9@gmail.com?part=4

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy