[PATCH 0/4] drm/rockchip: add RK3568 LVDS support

[PATCH 0/4] drm/rockchip: add RK3568 LVDS support

[Rok Markovic]

RK3568 has a native LVDS transmitter that mainline does not support:
rockchip_lvds.c only binds rk3288 and px30. This series adds it.

The transmitter has no register block of its own. It is programmed
entirely through the GRF and re-uses the MIPI DSI0 D-PHY in
PHY_MODE_LVDS, which phy-rockchip-inno-dsidphy already supports. So the
DT node is reg-less and carries a phy reference, like px30, but unlike
px30 it also takes a pclk (PCLK_DSITX_0, borrowed from the DSI
controller the D-PHY belongs to).

Patch 1 is an independent fix: rockchip_lvds_encoder_atomic_check()
never copied the connector's bus_flags into the CRTC state, so a panel's
pixelclk-active could not reach the hardware at all. It is a no-op for
the SoCs supported today - rk3288 and px30 pair with the VOP1 driver,
which never reads bus_flags; only VOP2 consumes it - and becomes live
with patch 3.

Patches 2-4 add the binding, the driver and the SoC DT node. No existing
code paths are modified: patch 3 only adds rk3568 functions, and every
rk3288/px30 function is left byte-identical.

The driver work is based on Alibek Omarov's 2023 posting [1], which was
never merged. The changes on top of it are described in patch 3's
changelog. The important one: phy_power_on() must be called from the
encoder enable path, not from probe. Called at probe it runs the phy's
LVDS bring-up - PLL power-on, settle, mode select, serializer reset
pulse, lane enables - while the GRF has not yet switched the block to
LVDS mode and the VOP is not driving dclk. The serializer is clocked
from dclk and latches dead coming out of that reset. The failure mode is
nasty: every register in the phy, the GRF and the VOP reads back
byte-identical to a working system while the lanes sit at common mode
and never toggle.

[1] https://lore.kernel.org/all/20230119184807.171132-1-a1ba.omarov@gmail.com/

Not covered: LVDS1 / dual-channel. The GRF bits exist (VO_CON3) but I
have no hardware for it, so only LVDS0 is wired up.

One integration note for anyone using this: VOP2 spreads its windows
only over video ports that have a connected endpoint, and writes
cumulative layer boundaries into OVL_PORT_SEL. If LVDS is put on vp2
while vp1 has no endpoint, the boundaries come out non-monotonic
(PORT0=2, PORT1=8, PORT2=5) and no window ever reaches vp2. Putting LVDS
on vp1 avoids it. That looks like a separate VOP2 bug and is not
addressed here.

Testing
-------

Tested on two RK3568 boards:

  - MYIR MYD-LR3568X eval board, 1024x600 LVDS panel at 59.4 MHz
  - Kanardia Nesis V (custom board, MYIR MY3568 SOM), 1280x800 at 72 MHz

Both light up and scan out correctly; VOP2 reports the programmed mode
with real_clk matching the requested pixel clock.

What has *not* been tested:

  - The boot testing above was done on 6.19.5, and with an earlier
    revision of patch 3 that used regmap_update_bits() and powered the
    phy from a shared helper. This series is rebased onto 7.1.3 and the
    GRF accesses were reworked to regmap_write() after review; it is
    compile-tested and the DTB builds, but this exact code has not been
    booted. The register writes are equivalent by inspection.
  - Patch 1 has not been tested on rk3288 or px30 hardware. The argument
    that it cannot regress them is by inspection - VOP1 has no reader of
    bus_flags - not by measurement. Review of that claim is welcome.
  - Dual-channel/LVDS1 is untested (not implemented).
  - Suspend/resume and DPMS cycling are untested. The P2S_EN re-assert
    in patch 3 is what should make a second enable work; it is reasoned
    from the poweroff path clearing it, not from a measured DPMS cycle.

dt_binding_check passes on patch 2. dtbs_check on the board DTB reports
no issues against the new lvds schema.

Tooling disclosure
------------------

Per Documentation/process/coding-assistants.rst and
generated-content.rst: this series was produced with the help of Claude
(claude-opus-4-8), used interactively across a long debugging session
rather than from a single prompt. The assistant read the vendor BSP
driver and the phy/VOP2 sources, identified the phy-ordering bug that
patch 3 fixes, and wrote the patch text and changelogs. All hardware
testing, every oscilloscope measurement and register dump, and the two
decisive debugging ideas - diffing register state against a working
vendor board, and using MIPI DSI on the shared D-PHY to prove the lanes
were good - were mine. I have reviewed every line and can defend it.
The Signed-off-by is mine alone.

No coccinelle/sparse/smatch runs were involved.

Rok Markovic (4):
  drm/rockchip: lvds: propagate bus_flags to the CRTC state
  dt-bindings: display: rockchip,lvds: add RK3568
  drm/rockchip: lvds: add RK3568 support
  arm64: dts: rockchip: rk356x: add LVDS node

 .../display/rockchip/rockchip,lvds.yaml       |  22 +++
 arch/arm64/boot/dts/rockchip/rk356x-base.dtsi |  25 +++
 drivers/gpu/drm/rockchip/rockchip_lvds.c      | 171 +++++++++++++++++-
 drivers/gpu/drm/rockchip/rockchip_lvds.h      |  10 +
 4 files changed, 227 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/4] drm/rockchip: lvds: propagate bus_flags to the CRTC state

[Rok Markovic]

rockchip_lvds_encoder_atomic_check() sets output_mode and output_type
but never copies the connector's bus_flags into the Rockchip CRTC
state, unlike dw_dp-rockchip.c and dw-mipi-dsi2-rockchip.c which both
do.

panel-lvds parses pixelclk-active from the DT display timing and
publishes it on the connector as DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.
VOP2 reads that flag back out of the CRTC state to decide
POLFLAG_DCLK_INV. With the flag dropped here, vcstate->bus_flags is
always zero, so a panel that asks to be clocked on the falling edge is
driven on the rising one, and there is no way to express the panel's
requirement from DT at all.

No functional change for the SoCs currently supported by this driver:
rk3288 and px30 pair with the VOP1 driver (rockchip_drm_vop.c), which
never reads bus_flags - only VOP2 consumes it. The flag becomes live
with the RK3568 support added later in this series.

Signed-off-by: Rok Markovic <rok@kanardia.eu>
Assisted-by: Claude:claude-opus-4-8
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 75f898a..95fa0a9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -129,7 +129,15 @@ rockchip_lvds_encoder_atomic_check(struct drm_encoder *encoder,
 				   struct drm_connector_state *conn_state)
 {
 	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
-
+	struct drm_connector *connector = conn_state->connector;
+
+	/*
+	 * The VOP derives the pixel clock polarity from this. Without it a
+	 * panel that declares pixelclk-active = <0> is clocked on the wrong
+	 * edge. panel-lvds fills the connector's bus_flags in from the DT
+	 * display timing.
+	 */
+	s->bus_flags = connector->display_info.bus_flags;
 	s->output_mode = ROCKCHIP_OUT_MODE_P888;
 	s->output_type = DRM_MODE_CONNECTOR_LVDS;
 
-- 
2.43.0

[PATCH 2/4] dt-bindings: display: rockchip,lvds: add RK3568

[Rok Markovic]

The RK3568 LVDS transmitter has no register block of its own: it is
programmed entirely through the GRF and re-uses the MIPI DSI0 D-PHY
(rockchip,rk3568-dsi-dphy) in LVDS mode. It therefore needs
phys/phy-names like px30, but unlike px30 it does take a pclk
(PCLK_DSITX_0, shared with the DSI controller the D-PHY belongs to).

It has no analog supplies of its own and no pinctrl: the lanes are
dedicated D-PHY pins, not muxable GPIOs.

Signed-off-by: Rok Markovic <rok@kanardia.eu>
Assisted-by: Claude:claude-opus-4-8
---
 .../display/rockchip/rockchip,lvds.yaml       | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,lvds.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,lvds.yaml
index 03b002a..6280f7e 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip,lvds.yaml
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,lvds.yaml
@@ -15,6 +15,7 @@ properties:
     enum:
       - rockchip,px30-lvds
       - rockchip,rk3288-lvds
+      - rockchip,rk3568-lvds
 
   reg:
     maxItems: 1
@@ -121,6 +122,27 @@ allOf:
         - avdd1v8-supply
         - avdd3v3-supply
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: rockchip,rk3568-lvds
+
+    then:
+      properties:
+        reg: false
+        avdd1v0-supply: false
+        avdd1v8-supply: false
+        avdd3v3-supply: false
+        pinctrl-names: false
+        pinctrl-0: false
+
+      required:
+        - clocks
+        - clock-names
+        - phys
+        - phy-names
+
 additionalProperties: false
 
 examples:
-- 
2.43.0

[PATCH 3/4] drm/rockchip: lvds: add RK3568 support

[Rok Markovic]

The RK3568 LVDS transmitter has no register block of its own. It is
driven entirely through the GRF and re-uses the MIPI DSI0 D-PHY in
PHY_MODE_LVDS, which phy-rockchip-inno-dsidphy already supports.

Based on Alibek Omarov's earlier posting [1], with the changes below.

Power the D-PHY from the encoder enable path rather than from probe.
phy_power_on() runs the phy driver's whole LVDS bring-up: PLL and
bandgap power-on, a settle, PLL mode select, then a reset pulse of the
serializer and the lane enables. None of that is safe at probe time -
the GRF has not yet switched the block to LVDS mode (LVDS0_MODE_EN is
set from rk3568_lvds_poweron(), i.e. the enable path) and the VOP is
not driving dclk. The serializer is clocked from dclk and latches its
state coming out of that reset, so it comes up dead and stays dead.
The failure is silent: every register in the phy, the GRF and the VOP
reads back exactly as on a working system while the lanes sit at
common mode and never toggle.

Program RK3568_LVDS0_DCLK_INV_SEL from the CRTC state's bus_flags so
the panel's declared pixelclk-active is honoured on the LVDS block as
well as on the VOP pin polarity. Both have to agree with the edge the
panel samples on.

Re-assert RK3568_LVDS0_P2S_EN in rk3568_lvds_poweron().
rk3568_lvds_poweroff() clears MODE_EN and P2S_EN together, so setting
P2S_EN once at probe would leave the parallel-to-serial converter off
after the first disable/enable cycle.

Use regmap_write() rather than regmap_update_bits() for the GRF. These
registers are write-masked - the upper 16 bits select which of the
lower 16 a write may change - so there is nothing to preserve and no
reason to read first. Passing a FIELD_PREP_WM16() value as both the
mask and the value of an update_bits() applies the masking twice and
only works by accident.

Between the two, nothing needs programming at probe at all: the GRF is
written entirely from the enable path, so px30_lvds_probe() is left
alone rather than being refactored into a shared phy helper.

[1] https://lore.kernel.org/all/20230119184807.171132-1-a1ba.omarov@gmail.com/

Co-developed-by: Alibek Omarov <a1ba.omarov@gmail.com>
Signed-off-by: Alibek Omarov <a1ba.omarov@gmail.com>
Signed-off-by: Rok Markovic <rok@kanardia.eu>
Assisted-by: Claude:claude-opus-4-8
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 161 +++++++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_lvds.h |  10 ++
 2 files changed, 171 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 95fa0a9..f45d04a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -435,6 +435,133 @@ static void px30_lvds_encoder_disable(struct drm_encoder *encoder)
 	drm_panel_unprepare(lvds->panel);
 }
 
+static int rk3568_lvds_poweron(struct rockchip_lvds *lvds)
+{
+	int ret;
+
+	ret = clk_enable(lvds->pclk);
+	if (ret < 0) {
+		DRM_DEV_ERROR(lvds->dev, "failed to enable lvds pclk %d\n", ret);
+		return ret;
+	}
+
+	ret = pm_runtime_get_sync(lvds->dev);
+	if (ret < 0) {
+		DRM_DEV_ERROR(lvds->dev, "failed to get pm runtime: %d\n", ret);
+		clk_disable(lvds->pclk);
+		return ret;
+	}
+
+	/*
+	 * Enable LVDS mode and the parallel-to-serial converter. These are
+	 * write-masked registers, so a plain write only touches the bits named
+	 * here; there is nothing to preserve and no need to read first.
+	 */
+	return regmap_write(lvds->grf, RK3568_GRF_VO_CON2,
+			    RK3568_LVDS0_MODE_EN(1) |
+			    RK3568_LVDS0_P2S_EN(1));
+}
+
+static void rk3568_lvds_poweroff(struct rockchip_lvds *lvds)
+{
+	regmap_write(lvds->grf, RK3568_GRF_VO_CON2,
+		     RK3568_LVDS0_MODE_EN(0) | RK3568_LVDS0_P2S_EN(0));
+
+	pm_runtime_put(lvds->dev);
+	clk_disable(lvds->pclk);
+}
+
+static int rk3568_lvds_grf_config(struct drm_encoder *encoder,
+				  struct drm_display_mode *mode)
+{
+	struct rockchip_lvds *lvds = encoder_to_lvds(encoder);
+	struct rockchip_crtc_state *s =
+		to_rockchip_crtc_state(encoder->crtc->state);
+	bool negedge = !!(s->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE);
+
+	if (lvds->output != DISPLAY_OUTPUT_LVDS) {
+		DRM_DEV_ERROR(lvds->dev, "Unsupported display output %d\n",
+			      lvds->output);
+		return -EINVAL;
+	}
+
+	/*
+	 * The LVDS block has its own dclk inversion select, separate from the
+	 * VOP's pin polarity. Both have to agree with what the panel samples on.
+	 */
+	regmap_write(lvds->grf, RK3568_GRF_VO_CON2,
+		     RK3568_LVDS0_DCLK_INV_SEL(negedge));
+
+	/* Set format */
+	return regmap_write(lvds->grf, RK3568_GRF_VO_CON0,
+			    RK3568_LVDS0_SELECT(lvds->format) |
+			    RK3568_LVDS0_MSBSEL(1));
+}
+
+static void rk3568_lvds_encoder_enable(struct drm_encoder *encoder)
+{
+	struct rockchip_lvds *lvds = encoder_to_lvds(encoder);
+	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
+	int ret;
+
+	drm_panel_prepare(lvds->panel);
+
+	ret = rk3568_lvds_poweron(lvds);
+	if (ret) {
+		DRM_DEV_ERROR(lvds->dev, "failed to power on LVDS: %d\n", ret);
+		drm_panel_unprepare(lvds->panel);
+		return;
+	}
+
+	ret = rk3568_lvds_grf_config(encoder, mode);
+	if (ret) {
+		DRM_DEV_ERROR(lvds->dev, "failed to configure LVDS: %d\n", ret);
+		drm_panel_unprepare(lvds->panel);
+		return;
+	}
+
+	/*
+	 * Only now bring the D-PHY up. phy_power_on() runs the whole
+	 * inno_dsidphy_lvds_mode_enable() sequence - PLL and bandgap power-on,
+	 * a settle, PLL mode select, then a reset pulse of the serializer and
+	 * the lane enables. All of that has to happen with the block already
+	 * switched to LVDS mode in the GRF (above) and with the VOP's dclk
+	 * running, because the serializer is clocked from dclk and latches its
+	 * state out of that reset.
+	 *
+	 * Doing it at probe instead - as this driver used to - resets and
+	 * enables the serializer against a block that is not in LVDS mode yet
+	 * and has no input clock. Every register then reads back correct while
+	 * the lanes sit at common mode forever. Rockchip's BSP orders it this
+	 * way (GRF writes, then phy_set_mode + phy_power_on).
+	 */
+	ret = phy_set_mode(lvds->dphy, PHY_MODE_LVDS);
+	if (ret) {
+		DRM_DEV_ERROR(lvds->dev, "failed to set phy mode: %d\n", ret);
+		drm_panel_unprepare(lvds->panel);
+		return;
+	}
+
+	ret = phy_power_on(lvds->dphy);
+	if (ret) {
+		DRM_DEV_ERROR(lvds->dev, "failed to power on phy: %d\n", ret);
+		drm_panel_unprepare(lvds->panel);
+		return;
+	}
+
+	drm_panel_enable(lvds->panel);
+}
+
+static void rk3568_lvds_encoder_disable(struct drm_encoder *encoder)
+{
+	struct rockchip_lvds *lvds = encoder_to_lvds(encoder);
+
+	drm_panel_disable(lvds->panel);
+	phy_power_off(lvds->dphy);
+	rk3568_lvds_poweroff(lvds);
+	drm_panel_unprepare(lvds->panel);
+}
+
 static const
 struct drm_encoder_helper_funcs rk3288_lvds_encoder_helper_funcs = {
 	.enable = rk3288_lvds_encoder_enable,
@@ -449,6 +576,13 @@ struct drm_encoder_helper_funcs px30_lvds_encoder_helper_funcs = {
 	.atomic_check = rockchip_lvds_encoder_atomic_check,
 };
 
+static const
+struct drm_encoder_helper_funcs rk3568_lvds_encoder_helper_funcs = {
+	.enable = rk3568_lvds_encoder_enable,
+	.disable = rk3568_lvds_encoder_disable,
+	.atomic_check = rockchip_lvds_encoder_atomic_check,
+};
+
 static int rk3288_lvds_probe(struct platform_device *pdev,
 			     struct rockchip_lvds *lvds)
 {
@@ -512,6 +646,22 @@ static int px30_lvds_probe(struct platform_device *pdev,
 	return phy_power_on(lvds->dphy);
 }
 
+static int rk3568_lvds_probe(struct platform_device *pdev,
+			     struct rockchip_lvds *lvds)
+{
+	/*
+	 * Grab and init the phy, but do NOT power it on here - that is done in
+	 * rk3568_lvds_encoder_enable() once the GRF is in LVDS mode and dclk is
+	 * running. See the comment there. The GRF is not touched at probe
+	 * either: every bit of it is programmed from the enable path.
+	 */
+	lvds->dphy = devm_phy_get(&pdev->dev, "dphy");
+	if (IS_ERR(lvds->dphy))
+		return PTR_ERR(lvds->dphy);
+
+	return phy_init(lvds->dphy);
+}
+
 static const struct rockchip_lvds_soc_data rk3288_lvds_data = {
 	.probe = rk3288_lvds_probe,
 	.helper_funcs = &rk3288_lvds_encoder_helper_funcs,
@@ -522,6 +672,11 @@ static const struct rockchip_lvds_soc_data px30_lvds_data = {
 	.helper_funcs = &px30_lvds_encoder_helper_funcs,
 };
 
+static const struct rockchip_lvds_soc_data rk3568_lvds_data = {
+	.probe = rk3568_lvds_probe,
+	.helper_funcs = &rk3568_lvds_encoder_helper_funcs,
+};
+
 static const struct of_device_id rockchip_lvds_dt_ids[] = {
 	{
 		.compatible = "rockchip,rk3288-lvds",
@@ -531,6 +686,10 @@ static const struct of_device_id rockchip_lvds_dt_ids[] = {
 		.compatible = "rockchip,px30-lvds",
 		.data = &px30_lvds_data
 	},
+	{
+		.compatible = "rockchip,rk3568-lvds",
+		.data = &rk3568_lvds_data
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, rockchip_lvds_dt_ids);
@@ -601,6 +760,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 	encoder = &lvds->encoder.encoder;
 	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev,
 							     dev->of_node);
+	rockchip_drm_encoder_set_crtc_endpoint_id(&lvds->encoder,
+						  dev->of_node, 0, 0);
 
 	ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_LVDS);
 	if (ret < 0) {
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.h b/drivers/gpu/drm/rockchip/rockchip_lvds.h
index 2d92447..93d3415 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.h
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.h
@@ -121,4 +121,14 @@
 #define   PX30_LVDS_P2S_EN(val)			FIELD_PREP_WM16(BIT(6), (val))
 #define   PX30_LVDS_VOP_SEL(val)		FIELD_PREP_WM16(BIT(1), (val))
 
+#define RK3568_GRF_VO_CON0			0x0360
+#define   RK3568_LVDS0_SELECT(val)		FIELD_PREP_WM16(GENMASK(5, 4), (val))
+#define   RK3568_LVDS0_MSBSEL(val)		FIELD_PREP_WM16(BIT(3), (val))
+
+#define RK3568_GRF_VO_CON2			0x0368
+#define   RK3568_LVDS0_DCLK_INV_SEL(val)	FIELD_PREP_WM16(BIT(9), (val))
+#define   RK3568_LVDS0_DCLK_DIV2_SEL(val)	FIELD_PREP_WM16(BIT(8), (val))
+#define   RK3568_LVDS0_MODE_EN(val)		FIELD_PREP_WM16(BIT(1), (val))
+#define   RK3568_LVDS0_P2S_EN(val)		FIELD_PREP_WM16(BIT(0), (val))
+
 #endif /* _ROCKCHIP_LVDS_ */
-- 
2.43.0

[PATCH 4/4] arm64: dts: rockchip: rk356x: add LVDS node

[Rok Markovic]

Add the LVDS transmitter node. It has no register block of its own -
it is programmed through the GRF and borrows the MIPI DSI0 D-PHY - so
it carries no reg, only the DSITX_0 pclk, the phy and the VO power
domain.

Disabled by default; boards that route LVDS enable it along with
dsi_dphy0 and wire lvds_in to a VOP2 video port.

Signed-off-by: Rok Markovic <rok@kanardia.eu>
Assisted-by: Claude:claude-opus-4-8
---
 arch/arm64/boot/dts/rockchip/rk356x-base.dtsi | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
index 64bdd8b..9244b55 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
@@ -878,6 +878,31 @@
 		};
 	};
 
+	lvds: lvds {
+		compatible = "rockchip,rk3568-lvds";
+		clocks = <&cru PCLK_DSITX_0>;
+		clock-names = "pclk_lvds";
+		phys = <&dsi_dphy0>;
+		phy-names = "dphy";
+		power-domains = <&power RK3568_PD_VO>;
+		rockchip,grf = <&grf>;
+		rockchip,output = "lvds";
+		status = "disabled";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			lvds_in: port@0 {
+				reg = <0>;
+			};
+
+			lvds_out: port@1 {
+				reg = <1>;
+			};
+		};
+	};
+
 	qos_gpu: qos@fe128000 {
 		compatible = "rockchip,rk3568-qos", "syscon";
 		reg = <0x0 0xfe128000 0x0 0x20>;
-- 
2.43.0

Re: [PATCH 4/4] arm64: dts: rockchip: rk356x: add LVDS node

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing Hardware Clock Initialization for RK3568 LVDS
- [Medium] Unbalanced Hardware Resource Teardown in Error Paths
--

--- Patch [4]: [PATCH 4/4] arm64: dts: rockchip: rk356x: add LVDS node ---
commit bfe12cef9ca639b004d58e050ef9175743345f7b
Author: Rok Markovic <rok@kanardia.eu>

arm64: dts: rockchip: rk356x: add LVDS node

This commit adds the LVDS transmitter node for the RK356x SoC, assigning
its clock, physical interface, and power domains. It is left disabled by
default, to be enabled by specific board files.

> diff --git a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> index a5832895bd392..2b3beab5507b1 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> @@ -879,6 +879,31 @@ hdmi_out: port@1 {
>  		};
>  	};
>  
> +	lvds: lvds {
> +		compatible = "rockchip,rk3568-lvds";
> +		clocks = <&cru PCLK_DSITX_0>;
> +		clock-names = "pclk_lvds";

[Severity: High]
Does the driver actually use this clock?

Looking at rk3568_lvds_probe() in drivers/gpu/drm/rockchip/rockchip_lvds.c,
it seems to completely omit fetching the required pclk_lvds clock from the
device tree:

drivers/gpu/drm/rockchip/rockchip_lvds.c:rk3568_lvds_probe() {
    ...
    lvds->dphy = devm_phy_get(&pdev->dev, "dphy");
    if (IS_ERR(lvds->dphy))
        return PTR_ERR(lvds->dphy);

    return phy_init(lvds->dphy);
}

Because lvds->pclk is left NULL, the clk_enable() call in
rk3568_lvds_poweron() acts as a no-op:

drivers/gpu/drm/rockchip/rockchip_lvds.c:rk3568_lvds_poweron() {
    ...
    ret = clk_enable(lvds->pclk);
    if (ret < 0) {
    ...
}

Will this cause hardware registers to be accessed without a running clock,
potentially leading to system hangs on device probe or modeset?

> +		phys = <&dsi_dphy0>;
> +		phy-names = "dphy";
> +		power-domains = <&power RK3568_PD_VO>;
> +		rockchip,grf = <&grf>;
> +		rockchip,output = "lvds";
> +		status = "disabled";
> +
> +		ports {

[Severity: Medium]
Will enabling this hardware expose an unbalanced resource teardown in the
driver's error paths?

If intermediate steps like phy_set_mode() fail in
rk3568_lvds_encoder_enable(), the function returns early without rolling
back earlier successful calls:

drivers/gpu/drm/rockchip/rockchip_lvds.c:rk3568_lvds_encoder_enable() {
    ...
    ret = phy_set_mode(lvds->dphy, PHY_MODE_LVDS);
    if (ret) {
        DRM_DEV_ERROR(lvds->dev, "failed to set phy mode: %d\n", ret);
        drm_panel_unprepare(lvds->panel);
        return;
    }
    ...
}

Because .enable() returns void, the DRM core assumes it succeeded and will
later call .disable(). The rk3568_lvds_encoder_disable() function
unconditionally tears down all resources:

drivers/gpu/drm/rockchip/rockchip_lvds.c:rk3568_lvds_encoder_disable() {
    struct rockchip_lvds *lvds = encoder_to_lvds(encoder);

    drm_panel_disable(lvds->panel);
    phy_power_off(lvds->dphy);
    rk3568_lvds_poweroff(lvds);
    drm_panel_unprepare(lvds->panel);
}

Could this cause refcount underflows for PHY, PM runtime, and clocks,
resulting in kernel warnings if they were never successfully enabled?

> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			lvds_in: port@0 {
> +				reg = <0>;
> +			};
> +
> +			lvds_out: port@1 {
> +				reg = <1>;
> +			};
> +		};
> +	};
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717120005.2087386-1-rok@kanardia.eu?part=4

Re: [PATCH 1/4] drm/rockchip: lvds: propagate bus_flags to the CRTC state

[sashiko-bot]

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

New issues:
- [Medium] The newly introduced bus_flags assignment bypasses DRM bridge state negotiation by reading flags directly from the final connector instead of the adjacent bridge.

Pre-existing issues:
- [High] The driver unconditionally overwrites the function pointers and private data of external bridge objects, causing type confusion and completely breaking external bridge support.
--

--- Patch [1]: [PATCH 1/4] drm/rockchip: lvds: propagate bus_flags to the CRTC state ---
commit e03a59004874598201254d64c0f6e0e88de3ff1a
Author: Rok Markovic <rok@kanardia.eu>

drm/rockchip: lvds: propagate bus_flags to the CRTC state

This commit updates the Rockchip LVDS encoder's atomic_check function to
propagate bus_flags from the connector's display_info to the CRTC state.
This ensures that the VOP can correctly set the pixel clock polarity based
on the panel's requirements.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 7a0c4fa29f2f0..150299c12298a 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -129,7 +129,15 @@ rockchip_lvds_encoder_atomic_check(struct drm_encoder *encoder,
>  				   struct drm_connector_state *conn_state)
>  {
>  	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
> -
> +	struct drm_connector *connector = conn_state->connector;
> +
> +	/*
> +	 * The VOP derives the pixel clock polarity from this. Without it a
> +	 * panel that declares pixelclk-active = <0> is clocked on the wrong
> +	 * edge. panel-lvds fills the connector's bus_flags in from the DT
> +	 * display timing.
> +	 */
> +	s->bus_flags = connector->display_info.bus_flags;

[Severity: Medium]
Does this bypass DRM bridge state negotiation by reading flags directly from
the final connector instead of the adjacent bridge?

By reading from connector->display_info.bus_flags in
rockchip_lvds_encoder_atomic_check(), the VOP pixel clock polarity is
configured based on the final sink (e.g., an HDMI monitor) rather than the
LVDS bridge's input requirements.

This causes incorrect pixel clock polarity for external bridge setups, as it
violates DRM bridge chaining contracts which expect the state to be retrieved
via drm_atomic_get_new_bridge_state().

>  	s->output_mode = ROCKCHIP_OUT_MODE_P888;
>  	s->output_type = DRM_MODE_CONNECTOR_LVDS;

[Severity: High]
This isn't a bug introduced by this patch, but does the driver unconditionally
overwrite the function pointers and private data of external bridge objects in
rockchip_lvds_bind()?

When a valid external bridge is present, the initialization path overwrites
the bridge data:

rockchip_lvds_bind() {
    ...
    if (lvds->bridge) {
        lvds->bridge->driver_private = lvds;
        lvds->bridge->ops = DRM_BRIDGE_OP_MODES;
        lvds->bridge->funcs = &rockchip_lvds_bridge_funcs;
    ...
}

Does this hijack the external bridge by replacing its callbacks and private
data with LVDS-specific values?

This causes type confusion and prevents external bridges from ever being
enabled, since their callbacks are replaced by the LVDS functions. It also
seems likely to cause panics if the external bridge driver attempts to access
its original driver_private data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717120005.2087386-1-rok@kanardia.eu?part=1

Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unbalanced phy_power_off() in disable path permanently corrupts PHY state after a transient enable error.
- [Low] Silently ignored regmap_write() error in rk3568_lvds_grf_config().
--

--- Patch [3]: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support ---
commit 42ee5aa3ee6adfe018b6f3294b12a2e7ab2076e3
Author: Rok Markovic <rok@kanardia.eu>
Date:   Fri Jul 17 12:00:04 2026 +0000

    drm/rockchip: lvds: add RK3568 support

This commit adds support for the RK3568 LVDS transmitter to the rockchip_lvds
driver. It configures the MIPI DSI0 D-PHY in PHY_MODE_LVDS and powers the
D-PHY from the encoder enable path instead of probe.

Link: https://lore.kernel.org/all/20230119184807.171132-1-a1ba.omarov@gmail.com/

> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 150299c12298a..cc56ca2580a66 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
[ ... ]
> @@ -435,6 +435,133 @@ static void px30_lvds_encoder_disable(struct drm_encoder *encoder)
> +static int rk3568_lvds_grf_config(struct drm_encoder *encoder,
> +				  struct drm_display_mode *mode)
> +{
[ ... ]
> +	/*
> +	 * The LVDS block has its own dclk inversion select, separate from the
> +	 * VOP's pin polarity. Both have to agree with what the panel samples on.
> +	 */
> +	regmap_write(lvds->grf, RK3568_GRF_VO_CON2,
> +		     RK3568_LVDS0_DCLK_INV_SEL(negedge));
> +

[Severity: Low]
Does this code silently ignore a potential error from regmap_write()?

If this first regmap_write() to RK3568_GRF_VO_CON2 fails, the DCLK inversion
configuration is not applied, but the function proceeds and only returns the
result of the second regmap_write() below.

> +	/* Set format */
> +	return regmap_write(lvds->grf, RK3568_GRF_VO_CON0,
> +			    RK3568_LVDS0_SELECT(lvds->format) |
> +			    RK3568_LVDS0_MSBSEL(1));
> +}
> +
> +static void rk3568_lvds_encoder_enable(struct drm_encoder *encoder)
> +{
[ ... ]
> +	ret = rk3568_lvds_poweron(lvds);
> +	if (ret) {
> +		DRM_DEV_ERROR(lvds->dev, "failed to power on LVDS: %d\n", ret);
> +		drm_panel_unprepare(lvds->panel);
> +		return;
> +	}
> +
> +	ret = rk3568_lvds_grf_config(encoder, mode);
> +	if (ret) {
> +		DRM_DEV_ERROR(lvds->dev, "failed to configure LVDS: %d\n", ret);
> +		drm_panel_unprepare(lvds->panel);
> +		return;
> +	}
> +
> +	/*
> +	 * Only now bring the D-PHY up. phy_power_on() runs the whole
> +	 * inno_dsidphy_lvds_mode_enable() sequence - PLL and bandgap power-on,
[ ... ]
> +	ret = phy_set_mode(lvds->dphy, PHY_MODE_LVDS);
> +	if (ret) {
> +		DRM_DEV_ERROR(lvds->dev, "failed to set phy mode: %d\n", ret);
> +		drm_panel_unprepare(lvds->panel);
> +		return;
> +	}
> +

[Severity: High]
Can an early return here lead to an unbalanced phy_power_off() later?

If rk3568_lvds_encoder_enable() encounters an error and returns early before
calling phy_power_on(), the DRM core still treats the encoder as active since
the enable callback returns void.

When the display pipeline is subsequently disabled, the DRM core unconditionally
calls rk3568_lvds_encoder_disable(), which does this:

static void rk3568_lvds_encoder_disable(...)
{
    ...
    phy_power_off(lvds->dphy);
    ...
}

This would decrement the PHY's power_count from 0 to -1. Future calls to
phy_power_on() will see a negative count, skip the actual hardware power-on
routine, and just increment the counter to 0, which leaves the PHY permanently
broken until reboot.

> +	ret = phy_power_on(lvds->dphy);
> +	if (ret) {
> +		DRM_DEV_ERROR(lvds->dev, "failed to power on phy: %d\n", ret);
> +		drm_panel_unprepare(lvds->panel);
> +		return;
> +	}
> +
> +	drm_panel_enable(lvds->panel);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717120005.2087386-1-rok@kanardia.eu?part=3