* [PATCH v2 0/4] phy: rockchip-samsung-dcphy: support the D-PHY receiver direction
@ 2026-07-26 14:55 Jason Yang via B4 Relay
2026-07-26 14:55 ` [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys Jason Yang via B4 Relay
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Jason Yang via B4 Relay @ 2026-07-26 14:55 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Heiko Stuebner, Guochun Huang, Philipp Zabel
Cc: linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel, Jason Yang
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, and it
stays enabled until runtime PM suspends the PHY, matching the
existing transmitter-only driver.
- 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 (RAW10, multiple
resolutions up to 3280x2464) 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.
- DSI-only and CSI-only configurations, and a shipped single-cell DSI
device tree resolving unchanged to the transmitter.
- The board wires two data lanes in each direction. 1-lane receive was
additionally exercised end-to-end concurrently with the 2-lane
transmitter (with a locally patched ov5640 sensor driver, as the
mainline driver hard-codes the sensor's 2-lane mode), which also
exercises the per-direction lane bookkeeping introduced in v2. The
3- and 4-lane paths share the same table-driven programming but are
untested here.
The DSI half of the concurrent tests additionally required a local
dw-mipi-dsi2 HS-video fix that is not part of this submission; the
receiver support added here is exercised independently by the CSI-only
capture.
---
Changes in v2:
- Rename the dt-bindings header to rockchip,rk3588-mipi-dcphy.h
(Krzysztof)
- Move the lane count and the receiver link rate into per-direction
state. A receiver configure() could previously overwrite the
transmitter's lane count; observed on hardware as the transmitter
leaving its second data lane enabled on power-off after the receiver
had been configured for one lane.
- Reject the unsupported direction in configure() before touching any
state, and drop the provider lock from configure() (per-direction
state no longer needs it).
- Track each direction's powered state under the provider mutex instead
of reading the peer phy's power_count.
- Switch already-enabled lanes back off when the receiver bring-up
fails.
- Link to v1: https://lore.kernel.org/r/20260721-dcphy-rx-v1-v1-0-4fc83c0ccac9@gmail.com
---
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 | 478 +++++++++++++++++++--
.../dt-bindings/phy/rockchip,rk3588-mipi-dcphy.h | 13 +
3 files changed, 460 insertions(+), 53 deletions(-)
---
base-commit: 1590cf0329716306e948a8fc29f1d3ee87d3989f
change-id: 20260721-dcphy-rx-v1-9af57f3edfd9
Best regards,
--
Jason Yang <jason98166@gmail.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys
2026-07-26 14:55 [PATCH v2 0/4] phy: rockchip-samsung-dcphy: support the D-PHY receiver direction Jason Yang via B4 Relay
@ 2026-07-26 14:55 ` Jason Yang via B4 Relay
2026-07-26 15:02 ` sashiko-bot
` (2 more replies)
2026-07-26 14:55 ` [PATCH v2 2/4] phy: rockchip-samsung-dcphy: factor MIPI D-PHY power on/off into helpers Jason Yang via B4 Relay
` (2 subsequent siblings)
3 siblings, 3 replies; 15+ messages in thread
From: Jason Yang via B4 Relay @ 2026-07-26 14:55 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Heiko Stuebner, Guochun Huang, Philipp Zabel
Cc: linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel, Jason Yang
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; bumping the cell count unconditionally would
invalidate them. 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/rockchip,rk3588-mipi-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/rockchip,rk3588-mipi-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..68a43db9c669 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/rockchip,rk3588-mipi-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/rockchip,rk3588-mipi-dcphy.h b/include/dt-bindings/phy/rockchip,rk3588-mipi-dcphy.h
new file mode 100644
index 000000000000..5029838fa275
--- /dev/null
+++ b/include/dt-bindings/phy/rockchip,rk3588-mipi-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_RK3588_MIPI_DCPHY_H
+#define _DT_BINDINGS_PHY_ROCKCHIP_RK3588_MIPI_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_RK3588_MIPI_DCPHY_H */
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/4] phy: rockchip-samsung-dcphy: factor MIPI D-PHY power on/off into helpers
2026-07-26 14:55 [PATCH v2 0/4] phy: rockchip-samsung-dcphy: support the D-PHY receiver direction Jason Yang via B4 Relay
2026-07-26 14:55 ` [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys Jason Yang via B4 Relay
@ 2026-07-26 14:55 ` Jason Yang via B4 Relay
2026-08-06 16:17 ` Bryan O'Donoghue
2026-07-26 14:55 ` [PATCH v2 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs Jason Yang via B4 Relay
2026-07-26 14:55 ` [PATCH v2 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support Jason Yang via B4 Relay
3 siblings, 1 reply; 15+ messages in thread
From: Jason Yang via B4 Relay @ 2026-07-26 14:55 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Heiko Stuebner, Guochun Huang, Philipp Zabel
Cc: linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel, Jason Yang
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs
2026-07-26 14:55 [PATCH v2 0/4] phy: rockchip-samsung-dcphy: support the D-PHY receiver direction Jason Yang via B4 Relay
2026-07-26 14:55 ` [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys Jason Yang via B4 Relay
2026-07-26 14:55 ` [PATCH v2 2/4] phy: rockchip-samsung-dcphy: factor MIPI D-PHY power on/off into helpers Jason Yang via B4 Relay
@ 2026-07-26 14:55 ` Jason Yang via B4 Relay
2026-08-03 12:05 ` Michael Riesch
2026-07-26 14:55 ` [PATCH v2 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support Jason Yang via B4 Relay
3 siblings, 1 reply; 15+ messages in thread
From: Jason Yang via B4 Relay @ 2026-07-26 14:55 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Heiko Stuebner, Guochun Huang, Philipp Zabel
Cc: linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel, Jason Yang
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, PHY type, lane count and powered flag) 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.
configure() rejects the not yet supported receiver before touching any
state, and each direction keeps its own lane count.
The PHY's common block contains a single BIAS block shared by both
directions (RK3588 TRM section 22.2), so it must be programmed only
once. Whichever direction powers on first enables it; each direction
records its powered state under the 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 | 143 +++++++++++++++-------
1 file changed, 102 insertions(+), 41 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
index d0d77421bd4b..95eb1200cab4 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/rockchip,rk3588-mipi-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,19 @@ 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;
+ unsigned int lanes;
+ /* Written under the parent's lock. */
+ bool powered;
+};
+
struct samsung_mipi_dcphy {
struct device *dev;
struct clk *ref_clk;
@@ -290,9 +305,9 @@ struct samsung_mipi_dcphy {
struct reset_control *s_phy_rst;
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 {
@@ -995,7 +1010,7 @@ static void samsung_mipi_dphy_lane_enable(struct samsung_mipi_dcphy *samsung)
regmap_update_bits(samsung->regmap, DPHY_MC_GNR_CON0,
PHY_ENABLE, PHY_ENABLE);
- switch (samsung->lanes) {
+ switch (samsung->phys[RK_DCPHY_DIR_TX].lanes) {
case 4:
regmap_write(samsung->regmap, DPHY_MD3_GNR_CON1,
T_PHY_READY(0x2000));
@@ -1026,7 +1041,7 @@ static void samsung_mipi_dphy_lane_enable(struct samsung_mipi_dcphy *samsung)
static void samsung_mipi_dphy_lane_disable(struct samsung_mipi_dcphy *samsung)
{
- switch (samsung->lanes) {
+ switch (samsung->phys[RK_DCPHY_DIR_TX].lanes) {
case 4:
regmap_update_bits(samsung->regmap, DPHY_MD3_GNR_CON0,
PHY_ENABLE, 0);
@@ -1334,11 +1349,23 @@ samsung_mipi_dphy_data_lane_timing_init(struct samsung_mipi_dcphy *samsung)
static int samsung_mipi_dphy_tx_power_on(struct samsung_mipi_dcphy *samsung)
{
+ bool first = !samsung->phys[RK_DCPHY_DIR_RX].powered;
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 +1395,44 @@ 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;
+ 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);
+ if (!ret)
+ pd->powered = true;
+ 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);
+ if (!ret)
+ pd->powered = false;
+ mutex_unlock(&samsung->lock);
+
+ return ret;
}
static int
@@ -1488,10 +1525,14 @@ 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;
+
+ pd->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
@@ -1501,16 +1542,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 +1577,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 +1610,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 +1620,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 +1666,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support
2026-07-26 14:55 [PATCH v2 0/4] phy: rockchip-samsung-dcphy: support the D-PHY receiver direction Jason Yang via B4 Relay
` (2 preceding siblings ...)
2026-07-26 14:55 ` [PATCH v2 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs Jason Yang via B4 Relay
@ 2026-07-26 14:55 ` Jason Yang via B4 Relay
2026-07-26 15:07 ` sashiko-bot
3 siblings, 1 reply; 15+ messages in thread
From: Jason Yang via B4 Relay @ 2026-07-26 14:55 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Heiko Stuebner, Guochun Huang, Philipp Zabel
Cc: linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel, Jason Yang
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() and
power_off() paths, serialized on the provider mutex alongside the
transmitter. configure() records the receiver's lane count and link
rate in its own per-direction state. If a lane does not report ready
during bring-up, the lanes enabled so far are switched back off before
the error is returned.
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.
Signed-off-by: Jason Yang <jason98166@gmail.com>
Assisted-by: Claude:claude-opus-4-8
---
drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c | 340 +++++++++++++++++++++-
1 file changed, 330 insertions(+), 10 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
index 95eb1200cab4..f6d112935c01 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)
@@ -291,6 +348,7 @@ struct samsung_mipi_dcphy_dir {
u8 dir;
u8 type;
unsigned int lanes;
+ unsigned long long hs_clk_rate;
/* Written under the parent's lock. */
bool powered;
};
@@ -988,6 +1046,228 @@ 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)
+{
+ struct samsung_mipi_dcphy_dir *pd = &samsung->phys[RK_DCPHY_DIR_RX];
+ u32 dlysel = samsung_mipi_dphy_rx_data_lane_dlysel(div_u64(pd->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 (pd->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 (pd->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 (pd->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 (pd->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)
+{
+ struct samsung_mipi_dcphy_dir *pd = &samsung->phys[RK_DCPHY_DIR_RX];
+ u32 lane_mbps = div_u64(pd->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 < pd->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 void samsung_mipi_dphy_rx_lane_disable(struct samsung_mipi_dcphy *samsung)
+{
+ struct samsung_mipi_dcphy_dir *pd = &samsung->phys[RK_DCPHY_DIR_RX];
+ int i;
+
+ regmap_update_bits(samsung->regmap, DPHY_SC_GNR_CON0, PHY_ENABLE, 0);
+ for (i = 0; i < pd->lanes; i++)
+ regmap_update_bits(samsung->regmap, rx_data_lane_gnr_con0[i],
+ PHY_ENABLE, 0);
+}
+
+static int samsung_mipi_dphy_rx_lane_enable(struct samsung_mipi_dcphy *samsung)
+{
+ struct samsung_mipi_dcphy_dir *pd = &samsung->phys[RK_DCPHY_DIR_RX];
+ u32 sts;
+ int ret, i;
+
+ regmap_update_bits(samsung->regmap, DPHY_SC_GNR_CON0, PHY_ENABLE, PHY_ENABLE);
+ for (i = 0; i < pd->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");
+ goto err_lane_disable;
+ }
+
+ for (i = 0; i < pd->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);
+ goto err_lane_disable;
+ }
+ }
+
+ return 0;
+
+err_lane_disable:
+ samsung_mipi_dphy_rx_lane_disable(samsung);
+ return ret;
+}
+
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);
@@ -1004,6 +1284,44 @@ 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)
+{
+ bool first = !samsung->phys[RK_DCPHY_DIR_TX].powered;
+ int ret;
+
+ if (!samsung->phys[RK_DCPHY_DIR_RX].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)
+{
+ reset_control_assert(samsung->s_phy_rst);
+
+ samsung_mipi_dphy_rx_lane_disable(samsung);
+
+ 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 +1722,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);
else
ret = samsung_mipi_dphy_tx_power_on(samsung);
if (!ret)
@@ -1423,11 +1741,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);
if (!ret)
pd->powered = false;
mutex_unlock(&samsung->lock);
@@ -1529,13 +1847,15 @@ 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;
- if (pd->dir == RK_DCPHY_DIR_RX)
- return -EOPNOTSUPP;
-
pd->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
- 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. */
+ pd->hs_clk_rate = target_rate;
+ } else {
+ samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
+ opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
+ }
return 0;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys
2026-07-26 14:55 ` [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys Jason Yang via B4 Relay
@ 2026-07-26 15:02 ` sashiko-bot
2026-08-03 11:17 ` Michael Riesch
2026-08-06 16:14 ` Bryan O'Donoghue
2 siblings, 0 replies; 15+ messages in thread
From: sashiko-bot @ 2026-07-26 15:02 UTC (permalink / raw)
To: Jason Yang
Cc: neil.armstrong, devicetree, olteanv, robh, linux-phy, conor+dt,
vkoul
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The dt-binding allows `#phy-cells = 2` to represent PHY direction, but the corresponding driver `phy-rockchip-samsung-dcphy.c` was not updated to support this.
--
--- Patch [1]: [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys ---
commit 421d266fa3318c44780c09224a034b0edeca9db3
Author: Jason Yang <jason98166@gmail.com>
Subject: dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys
This commit updates the Rockchip RK3588 MIPI D-/C-PHY device tree binding
to support per-direction configuration. It changes the #phy-cells property
to allow either 1 or 2 cells, where the optional second cell specifies the
PHY direction (TX or RX).
> diff --git a/Documentation/devicetree/bindings/phy/rockchip,rk3588-mipi-dcphy.yaml b/Documentation/devicetree/bindings/phy/rockchip,rk3588-mipi-dcphy.yaml
> index c8ff5ba22a86b..68a43db9c6690 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: Medium]
Will this new 2-cell configuration work with the current PHY driver?
Looking at samsung_mipi_dcphy_xlate() in
drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c, the driver explicitly
rejects anything other than 1 cell:
if (args->args_count != 1) {
dev_err(dev, "invalid number of arguments\n");
return ERR_PTR(-EINVAL);
}
If a device tree is updated to use the newly allowed 2-cell configuration
to specify TX/RX direction, won't phy_get() fail and return -EINVAL? Should
the driver be updated alongside the binding so consumers can successfully
acquire the PHY?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-dcphy-rx-v1-v2-0-cf9cc34a412a@gmail.com?part=1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support
2026-07-26 14:55 ` [PATCH v2 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support Jason Yang via B4 Relay
@ 2026-07-26 15:07 ` sashiko-bot
2026-07-27 2:01 ` 楊智成
0 siblings, 1 reply; 15+ messages in thread
From: sashiko-bot @ 2026-07-26 15:07 UTC (permalink / raw)
To: Jason Yang
Cc: neil.armstrong, olteanv, conor+dt, vkoul, linux-phy, robh,
devicetree
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The RX power-on sequence fails to toggle the shared APB reset (`apb_rst`) when it is the first PHY direction to power on, potentially leaving shared PHY state uninitialized.
- [High] The RX lane enable function polls for `PHY_READY` while the PHY is still held in reset, guaranteeing a timeout failure.
--
--- Patch [4]: [PATCH v2 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support ---
commit 8b1f5adbd06df56ad03e2b8d081a6587be5fe71d
Author: Jason Yang <jason98166@gmail.com>
phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support
This patch implements the receiver (CSI) direction of the D-PHY for the
Rockchip Samsung driver. It adds the RX register layout, HS-frequency settle
table, and hooks the receiver bring-up and teardown into the per-direction
power paths. The PLL sequence is also skipped for the receive path as the
sensor supplies the link clock.
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> index 95eb1200cab48..f6d112935c016 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
[ ... ]
> @@ -988,6 +1046,228 @@ struct samsung_mipi_dphy_timing samsung_mipi_dphy_timing_table[] = {
[ ... ]
> +static int samsung_mipi_dphy_rx_lane_enable(struct samsung_mipi_dcphy *samsung)
> +{
> + struct samsung_mipi_dcphy_dir *pd = &samsung->phys[RK_DCPHY_DIR_RX];
> + u32 sts;
> + int ret, i;
> +
> + regmap_update_bits(samsung->regmap, DPHY_SC_GNR_CON0, PHY_ENABLE, PHY_ENABLE);
> + for (i = 0; i < pd->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);
[Severity: High]
Does this code poll for PHY_READY while the PHY is still held in reset?
Looking at samsung_mipi_dphy_rx_power_on(), samsung->s_phy_rst is asserted,
then samsung_mipi_dphy_rx_lane_enable() is called, and only after that is
s_phy_rst deasserted.
Will this cause the PHY_READY poll to always timeout?
> + if (ret) {
> + dev_err(samsung->dev, "RX clock lane not ready\n");
> + goto err_lane_disable;
> + }
[ ... ]
> @@ -1004,6 +1284,44 @@ static void samsung_mipi_dcphy_bias_block_enable(struct samsung_mipi_dcphy *sams
[ ... ]
> +static int samsung_mipi_dphy_rx_power_on(struct samsung_mipi_dcphy *samsung)
> +{
> + bool first = !samsung->phys[RK_DCPHY_DIR_TX].powered;
> + int ret;
> +
> + if (!samsung->phys[RK_DCPHY_DIR_RX].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 RX direction is powered on first, does it need to toggle the shared APB
reset (apb_rst) before enabling the bias block?
In samsung_mipi_dphy_tx_power_on(), when first is true, it does:
if (first) {
reset_control_assert(samsung->apb_rst);
udelay(1);
reset_control_deassert(samsung->apb_rst);
}
Without this in the RX path, could the shared APB bus interface and BIAS
block remain in an uninitialized state from the bootloader?
> + 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;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-dcphy-rx-v1-v2-0-cf9cc34a412a@gmail.com?part=4
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support
2026-07-26 15:07 ` sashiko-bot
@ 2026-07-27 2:01 ` 楊智成
0 siblings, 0 replies; 15+ messages in thread
From: 楊智成 @ 2026-07-27 2:01 UTC (permalink / raw)
To: sashiko-reviews
Cc: neil.armstrong, olteanv, conor+dt, vkoul, linux-phy, robh,
devicetree
> Does this code poll for PHY_READY while the PHY is still held in reset?
Yes, and that is the sequence the TRM specifies: the RK3588 TRM
start-up sequence (figure 22-8) asserts S_RESETN, programs the settle
registers, enables the lanes, waits for PHY_READY to go high, and only
then deasserts S_RESETN. The poll does succeed on real hardware -
every capture listed in the cover's Testing section went through this
exact path.
> Without this in the RX path, could the shared APB bus interface and BIAS
> block remain in an uninitialized state from the bootloader?
This one came up on v1 as well; summarizing the disposition: the
vendor BSP's RX bring-up does not touch the APB reset either, the TRM
does not require it for the receiver, and the CSI-only configuration -
where the receiver is the only direction ever powered - captures
frames on real hardware without the APB reset ever being issued. The
BIAS block is programmed explicitly by whichever direction powers on
first, so nothing relies on bootloader state.
Happy to revisit if either assessment looks wrong.
Thanks,
Jason
<sashiko-bot@kernel.org> 於 2026年7月26日週日 下午11:07寫道：
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The RX power-on sequence fails to toggle the shared APB reset (`apb_rst`) when it is the first PHY direction to power on, potentially leaving shared PHY state uninitialized.
> - [High] The RX lane enable function polls for `PHY_READY` while the PHY is still held in reset, guaranteeing a timeout failure.
> --
>
> --- Patch [4]: [PATCH v2 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support ---
> commit 8b1f5adbd06df56ad03e2b8d081a6587be5fe71d
> Author: Jason Yang <jason98166@gmail.com>
>
> phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support
>
> This patch implements the receiver (CSI) direction of the D-PHY for the
> Rockchip Samsung driver. It adds the RX register layout, HS-frequency settle
> table, and hooks the receiver bring-up and teardown into the per-direction
> power paths. The PLL sequence is also skipped for the receive path as the
> sensor supplies the link clock.
>
> > diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> > index 95eb1200cab48..f6d112935c016 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> [ ... ]
> > @@ -988,6 +1046,228 @@ struct samsung_mipi_dphy_timing samsung_mipi_dphy_timing_table[] = {
> [ ... ]
> > +static int samsung_mipi_dphy_rx_lane_enable(struct samsung_mipi_dcphy *samsung)
> > +{
> > + struct samsung_mipi_dcphy_dir *pd = &samsung->phys[RK_DCPHY_DIR_RX];
> > + u32 sts;
> > + int ret, i;
> > +
> > + regmap_update_bits(samsung->regmap, DPHY_SC_GNR_CON0, PHY_ENABLE, PHY_ENABLE);
> > + for (i = 0; i < pd->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);
>
> [Severity: High]
> Does this code poll for PHY_READY while the PHY is still held in reset?
>
> Looking at samsung_mipi_dphy_rx_power_on(), samsung->s_phy_rst is asserted,
> then samsung_mipi_dphy_rx_lane_enable() is called, and only after that is
> s_phy_rst deasserted.
>
> Will this cause the PHY_READY poll to always timeout?
>
> > + if (ret) {
> > + dev_err(samsung->dev, "RX clock lane not ready\n");
> > + goto err_lane_disable;
> > + }
> [ ... ]
> > @@ -1004,6 +1284,44 @@ static void samsung_mipi_dcphy_bias_block_enable(struct samsung_mipi_dcphy *sams
> [ ... ]
> > +static int samsung_mipi_dphy_rx_power_on(struct samsung_mipi_dcphy *samsung)
> > +{
> > + bool first = !samsung->phys[RK_DCPHY_DIR_TX].powered;
> > + int ret;
> > +
> > + if (!samsung->phys[RK_DCPHY_DIR_RX].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 RX direction is powered on first, does it need to toggle the shared APB
> reset (apb_rst) before enabling the bias block?
>
> In samsung_mipi_dphy_tx_power_on(), when first is true, it does:
>
> if (first) {
> reset_control_assert(samsung->apb_rst);
> udelay(1);
> reset_control_deassert(samsung->apb_rst);
> }
>
> Without this in the RX path, could the shared APB bus interface and BIAS
> block remain in an uninitialized state from the bootloader?
>
> > + 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;
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260726-dcphy-rx-v1-v2-0-cf9cc34a412a@gmail.com?part=4
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys
2026-07-26 14:55 ` [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys Jason Yang via B4 Relay
2026-07-26 15:02 ` sashiko-bot
@ 2026-08-03 11:17 ` Michael Riesch
2026-08-06 15:56 ` Vinod Koul
2026-08-06 16:14 ` Bryan O'Donoghue
2 siblings, 1 reply; 15+ messages in thread
From: Michael Riesch @ 2026-08-03 11:17 UTC (permalink / raw)
To: jason98166, Vinod Koul, Neil Armstrong, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner, Guochun Huang,
Philipp Zabel
Cc: linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
Hi Jason,
Thanks for your patches!
On 7/26/26 16:55, Jason Yang via B4 Relay wrote:
> 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; bumping the cell count unconditionally would
> invalidate them. 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/rockchip,rk3588-mipi-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/rockchip,rk3588-mipi-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..68a43db9c669 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>
How is that Rockchip specific? As you state above this is either MIPI
CSI or MIPI DSI, two perfectly standard protocols.
@all Would it be reasonable to introduce PHY_TYPE_CSI and PHY_TYPE_DSI?
The rest of the patch LGTM.
Best regards,
Michael
> +
> + See include/dt-bindings/phy/phy.h and
> + include/dt-bindings/phy/rockchip,rk3588-mipi-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/rockchip,rk3588-mipi-dcphy.h b/include/dt-bindings/phy/rockchip,rk3588-mipi-dcphy.h
> new file mode 100644
> index 000000000000..5029838fa275
> --- /dev/null
> +++ b/include/dt-bindings/phy/rockchip,rk3588-mipi-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_RK3588_MIPI_DCPHY_H
> +#define _DT_BINDINGS_PHY_ROCKCHIP_RK3588_MIPI_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_RK3588_MIPI_DCPHY_H */
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs
2026-07-26 14:55 ` [PATCH v2 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs Jason Yang via B4 Relay
@ 2026-08-03 12:05 ` Michael Riesch
2026-08-04 5:38 ` 楊智成
0 siblings, 1 reply; 15+ messages in thread
From: Michael Riesch @ 2026-08-03 12:05 UTC (permalink / raw)
To: jason98166, Vinod Koul, Neil Armstrong, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner, Guochun Huang,
Philipp Zabel
Cc: linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
Hi Jason,
Thanks for the patch!
On 7/26/26 16:55, Jason Yang via B4 Relay wrote:
> 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, PHY type, lane count and powered flag) 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.
> configure() rejects the not yet supported receiver before touching any
> state, and each direction keeps its own lane count.
>
> The PHY's common block contains a single BIAS block shared by both
> directions (RK3588 TRM section 22.2), so it must be programmed only
> once. Whichever direction powers on first enables it; each direction
> records its powered state under the 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 | 143 +++++++++++++++-------
> 1 file changed, 102 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> index d0d77421bd4b..95eb1200cab4 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/rockchip,rk3588-mipi-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,19 @@ 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 {
This may be bike-shedding, but I am not a big fan of the notion of
"direction". This is a combo PHY that in essence contains different
PHYs, so why not call this "samsung_mipi_phy" or something?
> + struct phy *phy;
> + struct samsung_mipi_dcphy *parent;
> + u8 dir;
> + u8 type;
> + unsigned int lanes;
> + /* Written under the parent's lock. */
> + bool powered;
> +};
> +
> struct samsung_mipi_dcphy {
> struct device *dev;
> struct clk *ref_clk;
> @@ -290,9 +305,9 @@ struct samsung_mipi_dcphy {
> struct reset_control *s_phy_rst;
> 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];
This would look like this then:
struct samsung_mipi_phy phys[2];
which seems clean and simple to me.
> + /* Serialises the two directions' access to the shared PHY state. */
> + struct mutex lock;
>
> const struct samsung_mipi_dcphy_plat_data *pdata;
> struct {
> @@ -995,7 +1010,7 @@ static void samsung_mipi_dphy_lane_enable(struct samsung_mipi_dcphy *samsung)
> regmap_update_bits(samsung->regmap, DPHY_MC_GNR_CON0,
> PHY_ENABLE, PHY_ENABLE);
>
> - switch (samsung->lanes) {
> + switch (samsung->phys[RK_DCPHY_DIR_TX].lanes) {
> case 4:
> regmap_write(samsung->regmap, DPHY_MD3_GNR_CON1,
> T_PHY_READY(0x2000));
> @@ -1026,7 +1041,7 @@ static void samsung_mipi_dphy_lane_enable(struct samsung_mipi_dcphy *samsung)
>
> static void samsung_mipi_dphy_lane_disable(struct samsung_mipi_dcphy *samsung)
> {
> - switch (samsung->lanes) {
> + switch (samsung->phys[RK_DCPHY_DIR_TX].lanes) {
> case 4:
> regmap_update_bits(samsung->regmap, DPHY_MD3_GNR_CON0,
> PHY_ENABLE, 0);
> @@ -1334,11 +1349,23 @@ samsung_mipi_dphy_data_lane_timing_init(struct samsung_mipi_dcphy *samsung)
>
> static int samsung_mipi_dphy_tx_power_on(struct samsung_mipi_dcphy *samsung)
> {
> + bool first = !samsung->phys[RK_DCPHY_DIR_RX].powered;
Note to myself: the lock is acquired in the calling method, all is well.
> 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);
This approach with the "first" variable seems unintuitive. If you need
reference counting on the BIAS block, then use a state variable
"bias_powered", use a mutex that protects it, and call
samsung_mipi_dcphy_bias_block_{en,dis}able (from
samsung_mipi_dcphy_power{on, off}) unconditionally. Those methods shall
acquire the mutex, check the state variable, {en,dis}able the BIAS
block, set the state variable accordingly, release the mutex.
> samsung_mipi_dcphy_pll_configure(samsung);
> samsung_mipi_dphy_clk_lane_timing_init(samsung);
> samsung_mipi_dphy_data_lane_timing_init(samsung);
> @@ -1368,34 +1395,44 @@ 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);
Again, this may be bike-shedding, but "pd" seems misleading to me.
Please reconsider the naming. Maybe "samsung_phy" for the individual PHY
and "samsung" for the complete combo PHY? Or "phy_data" and
"dcphy_data"? ...?
> + struct samsung_mipi_dcphy *samsung = pd->parent;
> + 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);
> + if (!ret)
> + pd->powered = true;
Not sure whether you actually need to track the power status of the
individual PHYs (maybe you actually need to track the BIAS block status
alone) but anyway I would appreciate if
"samsung_mipi_dphy_tx_power_{on,off}" would set the "pd->powered" variable.
Best regards,
Michael
> + 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);
> + if (!ret)
> + pd->powered = false;
> + mutex_unlock(&samsung->lock);
> +
> + return ret;
> }
>
> static int
> @@ -1488,10 +1525,14 @@ 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;
> +
> + pd->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
>
> samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
> opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
> @@ -1501,16 +1542,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 +1577,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 +1610,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 +1620,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 +1666,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)
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs
2026-08-03 12:05 ` Michael Riesch
@ 2026-08-04 5:38 ` 楊智成
0 siblings, 0 replies; 15+ messages in thread
From: 楊智成 @ 2026-08-04 5:38 UTC (permalink / raw)
To: Michael Riesch
Cc: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Heiko Stuebner, Guochun Huang, Philipp Zabel,
linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
Hi Michael,
Thanks for the review.
> This may be bike-shedding, but I am not a big fan of the notion of
> "direction". This is a combo PHY that in essence contains different
> PHYs, so why not call this "samsung_mipi_phy" or something?
>
> struct samsung_mipi_phy phys[2];
>
> which seems clean and simple to me.
That reads better - the per-PHY structure becomes samsung_mipi_phy.
Taking your other naming comment together with this one:
> Again, this may be bike-shedding, but "pd" seems misleading to me.
> Please reconsider the naming. Maybe "samsung_phy" for the individual
> PHY and "samsung" for the complete combo PHY?
Sure, "pd" becomes "samsung_phy".
> This approach with the "first" variable seems unintuitive. If you need
> reference counting on the BIAS block, then use a state variable
> "bias_powered", use a mutex that protects it, and call
> samsung_mipi_dcphy_bias_block_{en,dis}able (from
> samsung_mipi_dcphy_power{on, off}) unconditionally. Those methods shall
> acquire the mutex, check the state variable, {en,dis}able the BIAS
> block, set the state variable accordingly, release the mutex.
>
> [...]
>
> Not sure whether you actually need to track the power status of the
> individual PHYs (maybe you actually need to track the BIAS block status
> alone) but anyway I would appreciate if
> "samsung_mipi_dphy_tx_power_{on,off}" would set the "pd->powered"
> variable.
Both of these lead to the same change. You are right to be unsure
about the per-PHY flag - I checked, and its only readers are the two
"first" computations, which only decide about the shared block. So the
two flags become one count in the parent, taken and dropped inside
samsung_mipi_dphy_{tx,rx}_power_{on,off}, and the "first" computations
go away. I went for a count rather than a single flag because the
release side has to distinguish "the other direction is still using
it" from "nobody is left".
The shared-block helper will use lockdep_assert_held() rather than
take the mutex itself, since samsung_mipi_dcphy_power_{on,off} already
hold it, as you noted.
One thing I am unsure about is the enable/disable pairing. I could not
find an enable for this block - the four BIAS registers only select
bias currents, reference voltages, the HS output level and the BGR
chopper divider, while PLL_CON0 in the same common block does have a
PLL_EN. If that reading is right, there is nothing to turn off and the
release side would only drop the count - in which case maybe the
"enable" name is worth revisiting as well. Does that match your
understanding of this block?
Separately, I noticed "first" also guards the APB reset, and only in
the transmitter path - once 4/4 enables the receiver, a receiver-first
power-on does not get one. I plan to move it into the same shared-block
path so both directions behave the same, and check both power-on
orders on the board before sending v3.
I would like to fold all of this into v3 together with whatever comes
out of the PHY_TYPE_CSI/PHY_TYPE_DSI discussion in patch 1/4, since
that decides how the two PHYs are told apart.
Thanks,
Jason
Michael Riesch <michael.riesch@collabora.com> 於 2026年8月3日週一 下午8:05寫道：
>
> Hi Jason,
>
> Thanks for the patch!
>
> On 7/26/26 16:55, Jason Yang via B4 Relay wrote:
> > 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, PHY type, lane count and powered flag) 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.
> > configure() rejects the not yet supported receiver before touching any
> > state, and each direction keeps its own lane count.
> >
> > The PHY's common block contains a single BIAS block shared by both
> > directions (RK3588 TRM section 22.2), so it must be programmed only
> > once. Whichever direction powers on first enables it; each direction
> > records its powered state under the 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 | 143 +++++++++++++++-------
> > 1 file changed, 102 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> > index d0d77421bd4b..95eb1200cab4 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/rockchip,rk3588-mipi-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,19 @@ 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 {
>
> This may be bike-shedding, but I am not a big fan of the notion of
> "direction". This is a combo PHY that in essence contains different
> PHYs, so why not call this "samsung_mipi_phy" or something?
>
> > + struct phy *phy;
> > + struct samsung_mipi_dcphy *parent;
> > + u8 dir;
> > + u8 type;
> > + unsigned int lanes;
> > + /* Written under the parent's lock. */
> > + bool powered;
> > +};
> > +
> > struct samsung_mipi_dcphy {
> > struct device *dev;
> > struct clk *ref_clk;
> > @@ -290,9 +305,9 @@ struct samsung_mipi_dcphy {
> > struct reset_control *s_phy_rst;
> > 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];
>
> This would look like this then:
>
> struct samsung_mipi_phy phys[2];
>
> which seems clean and simple to me.
>
> > + /* Serialises the two directions' access to the shared PHY state. */
> > + struct mutex lock;
> >
> > const struct samsung_mipi_dcphy_plat_data *pdata;
> > struct {
> > @@ -995,7 +1010,7 @@ static void samsung_mipi_dphy_lane_enable(struct samsung_mipi_dcphy *samsung)
> > regmap_update_bits(samsung->regmap, DPHY_MC_GNR_CON0,
> > PHY_ENABLE, PHY_ENABLE);
> >
> > - switch (samsung->lanes) {
> > + switch (samsung->phys[RK_DCPHY_DIR_TX].lanes) {
> > case 4:
> > regmap_write(samsung->regmap, DPHY_MD3_GNR_CON1,
> > T_PHY_READY(0x2000));
> > @@ -1026,7 +1041,7 @@ static void samsung_mipi_dphy_lane_enable(struct samsung_mipi_dcphy *samsung)
> >
> > static void samsung_mipi_dphy_lane_disable(struct samsung_mipi_dcphy *samsung)
> > {
> > - switch (samsung->lanes) {
> > + switch (samsung->phys[RK_DCPHY_DIR_TX].lanes) {
> > case 4:
> > regmap_update_bits(samsung->regmap, DPHY_MD3_GNR_CON0,
> > PHY_ENABLE, 0);
> > @@ -1334,11 +1349,23 @@ samsung_mipi_dphy_data_lane_timing_init(struct samsung_mipi_dcphy *samsung)
> >
> > static int samsung_mipi_dphy_tx_power_on(struct samsung_mipi_dcphy *samsung)
> > {
> > + bool first = !samsung->phys[RK_DCPHY_DIR_RX].powered;
>
> Note to myself: the lock is acquired in the calling method, all is well.
>
> > 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);
>
> This approach with the "first" variable seems unintuitive. If you need
> reference counting on the BIAS block, then use a state variable
> "bias_powered", use a mutex that protects it, and call
> samsung_mipi_dcphy_bias_block_{en,dis}able (from
> samsung_mipi_dcphy_power{on, off}) unconditionally. Those methods shall
> acquire the mutex, check the state variable, {en,dis}able the BIAS
> block, set the state variable accordingly, release the mutex.
>
>
> > samsung_mipi_dcphy_pll_configure(samsung);
> > samsung_mipi_dphy_clk_lane_timing_init(samsung);
> > samsung_mipi_dphy_data_lane_timing_init(samsung);
> > @@ -1368,34 +1395,44 @@ 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);
>
> Again, this may be bike-shedding, but "pd" seems misleading to me.
> Please reconsider the naming. Maybe "samsung_phy" for the individual PHY
> and "samsung" for the complete combo PHY? Or "phy_data" and
> "dcphy_data"? ...?
>
> > + struct samsung_mipi_dcphy *samsung = pd->parent;
> > + 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);
> > + if (!ret)
> > + pd->powered = true;
>
> Not sure whether you actually need to track the power status of the
> individual PHYs (maybe you actually need to track the BIAS block status
> alone) but anyway I would appreciate if
> "samsung_mipi_dphy_tx_power_{on,off}" would set the "pd->powered" variable.
>
> Best regards,
> Michael
>
>
> > + 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);
> > + if (!ret)
> > + pd->powered = false;
> > + mutex_unlock(&samsung->lock);
> > +
> > + return ret;
> > }
> >
> > static int
> > @@ -1488,10 +1525,14 @@ 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;
> > +
> > + pd->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
> >
> > samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
> > opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
> > @@ -1501,16 +1542,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 +1577,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 +1610,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 +1620,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 +1666,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)
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys
2026-08-03 11:17 ` Michael Riesch
@ 2026-08-06 15:56 ` Vinod Koul
0 siblings, 0 replies; 15+ messages in thread
From: Vinod Koul @ 2026-08-06 15:56 UTC (permalink / raw)
To: Michael Riesch
Cc: jason98166, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Heiko Stuebner, Guochun Huang, Philipp Zabel,
linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
On 03-08-26, 13:17, Michael Riesch wrote:
> > + 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>
>
> How is that Rockchip specific? As you state above this is either MIPI
> CSI or MIPI DSI, two perfectly standard protocols.
>
> @all Would it be reasonable to introduce PHY_TYPE_CSI and PHY_TYPE_DSI?
I think yes...
--
~Vinod
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys
2026-07-26 14:55 ` [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys Jason Yang via B4 Relay
2026-07-26 15:02 ` sashiko-bot
2026-08-03 11:17 ` Michael Riesch
@ 2026-08-06 16:14 ` Bryan O'Donoghue
2 siblings, 0 replies; 15+ messages in thread
From: Bryan O'Donoghue @ 2026-08-06 16:14 UTC (permalink / raw)
To: jason98166, Vinod Koul, Neil Armstrong, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner, Guochun Huang,
Philipp Zabel
Cc: linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
On 26/07/2026 15:55, Jason Yang via B4 Relay wrote:
> 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; bumping the cell count unconditionally would
> invalidate them. 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/rockchip,rk3588-mipi-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
LGTM
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] phy: rockchip-samsung-dcphy: factor MIPI D-PHY power on/off into helpers
2026-07-26 14:55 ` [PATCH v2 2/4] phy: rockchip-samsung-dcphy: factor MIPI D-PHY power on/off into helpers Jason Yang via B4 Relay
@ 2026-08-06 16:17 ` Bryan O'Donoghue
2026-08-06 19:31 ` Sebastian Reichel
0 siblings, 1 reply; 15+ messages in thread
From: Bryan O'Donoghue @ 2026-08-06 16:17 UTC (permalink / raw)
To: jason98166, Vinod Koul, Neil Armstrong, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner, Guochun Huang,
Philipp Zabel
Cc: linux-phy, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
On 26/07/2026 15:55, Jason Yang via B4 Relay wrote:
> 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;
> }
This looks a bit funny.
Do you have an int method() {} with a final path that returns no value ?
Tooling/compilers will definitely and rightly complain about that.
---
bod
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] phy: rockchip-samsung-dcphy: factor MIPI D-PHY power on/off into helpers
2026-08-06 16:17 ` Bryan O'Donoghue
@ 2026-08-06 19:31 ` Sebastian Reichel
0 siblings, 0 replies; 15+ messages in thread
From: Sebastian Reichel @ 2026-08-06 19:31 UTC (permalink / raw)
To: Bryan O'Donoghue
Cc: jason98166, Vinod Koul, Neil Armstrong, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner, Guochun Huang,
Philipp Zabel, linux-phy, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]
Hi,
On Thu, Aug 06, 2026 at 05:17:45PM +0100, Bryan O'Donoghue wrote:
> On 26/07/2026 15:55, Jason Yang via B4 Relay wrote:
> > 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;
> > }
>
> This looks a bit funny.
>
> Do you have an int method() {} with a final path that returns no value ?
>
> Tooling/compilers will definitely and rightly complain about that.
int function() {
...
switch(foo) {
case 42:
return 1;
default:
return 0;
}
/* cannot be reached */
}
Greetings,
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2026-08-06 19:31 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-26 14:55 [PATCH v2 0/4] phy: rockchip-samsung-dcphy: support the D-PHY receiver direction Jason Yang via B4 Relay
2026-07-26 14:55 ` [PATCH v2 1/4] dt-bindings: phy: rockchip,rk3588-mipi-dcphy: support per-direction phys Jason Yang via B4 Relay
2026-07-26 15:02 ` sashiko-bot
2026-08-03 11:17 ` Michael Riesch
2026-08-06 15:56 ` Vinod Koul
2026-08-06 16:14 ` Bryan O'Donoghue
2026-07-26 14:55 ` [PATCH v2 2/4] phy: rockchip-samsung-dcphy: factor MIPI D-PHY power on/off into helpers Jason Yang via B4 Relay
2026-08-06 16:17 ` Bryan O'Donoghue
2026-08-06 19:31 ` Sebastian Reichel
2026-07-26 14:55 ` [PATCH v2 3/4] phy: rockchip-samsung-dcphy: model TX and RX as separate PHYs Jason Yang via B4 Relay
2026-08-03 12:05 ` Michael Riesch
2026-08-04 5:38 ` 楊智成
2026-07-26 14:55 ` [PATCH v2 4/4] phy: rockchip-samsung-dcphy: add MIPI D-PHY receiver support Jason Yang via B4 Relay
2026-07-26 15:07 ` sashiko-bot
2026-07-27 2:01 ` 楊智成
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox