* [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
@ 2026-07-17 12:00 ` Rok Markovic
0 siblings, 0 replies; 26+ messages in thread
From: Rok Markovic @ 2026-07-17 12:00 UTC (permalink / raw)
To: Heiko Stuebner, Sandy Huang, Andy Yan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: dri-devel, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel, Alibek Omarov, 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
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
2026-07-17 12:00 ` Rok Markovic
(?)
@ 2026-07-17 12:16 ` sashiko-bot
-1 siblings, 0 replies; 26+ messages in thread
From: sashiko-bot @ 2026-07-17 12:16 UTC (permalink / raw)
To: Rok Markovic; +Cc: devicetree, conor+dt, dri-devel, robh
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
2026-07-17 12:00 ` Rok Markovic
@ 2026-07-20 1:54 ` Chaoyi Chen
-1 siblings, 0 replies; 26+ messages in thread
From: Chaoyi Chen @ 2026-07-20 1:54 UTC (permalink / raw)
To: Rok Markovic, Heiko Stuebner, Sandy Huang, Andy Yan,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: dri-devel, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel, Alibek Omarov
Hello Rok,
Thank you for your patch. Please see the comment below:
On 7/17/2026 8:00 PM, Rok Markovic wrote:
> 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.
>
Could you please confirm this? Based on my earlier tests, after PHY
poweron, re-disabling and re-enabling the GRF did not reproduce the
issue you described.
> 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.
> + */
I think this comment is redundant. We all know this is a common design
on Rockchip platform, right? :)
> + 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));
> +}
I think rk3568_lvds_poweron() and rk3568_lvds_grf_config() can be merged
to reduce extra register operations.
> +
> +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).
> + */
I think this comment is redundant. These are internal details of
phy_set_mode() and don't need to be explained here. Also, the
ordering requirements are quite common. You can describe them in the
commit message.
> + 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.
> + */
Please see the comments above.
> + 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_ */
--
Best,
Chaoyi
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
@ 2026-07-20 1:54 ` Chaoyi Chen
0 siblings, 0 replies; 26+ messages in thread
From: Chaoyi Chen @ 2026-07-20 1:54 UTC (permalink / raw)
To: Rok Markovic, Heiko Stuebner, Sandy Huang, Andy Yan,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: dri-devel, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel, Alibek Omarov
Hello Rok,
Thank you for your patch. Please see the comment below:
On 7/17/2026 8:00 PM, Rok Markovic wrote:
> 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.
>
Could you please confirm this? Based on my earlier tests, after PHY
poweron, re-disabling and re-enabling the GRF did not reproduce the
issue you described.
> 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.
> + */
I think this comment is redundant. We all know this is a common design
on Rockchip platform, right? :)
> + 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));
> +}
I think rk3568_lvds_poweron() and rk3568_lvds_grf_config() can be merged
to reduce extra register operations.
> +
> +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).
> + */
I think this comment is redundant. These are internal details of
phy_set_mode() and don't need to be explained here. Also, the
ordering requirements are quite common. You can describe them in the
commit message.
> + 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.
> + */
Please see the comments above.
> + 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_ */
--
Best,
Chaoyi
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
2026-07-20 1:54 ` Chaoyi Chen
@ 2026-07-21 8:31 ` Rok Markovič
-1 siblings, 0 replies; 26+ messages in thread
From: Rok Markovič @ 2026-07-21 8:31 UTC (permalink / raw)
To: Chaoyi Chen
Cc: Heiko Stuebner, Sandy Huang, Andy Yan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, dri-devel,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel,
Alibek Omarov
20.7.2026 3:54, je Chaoyi Chen napisal
> Hello Rok,
>
> Thank you for your patch. Please see the comment below:
>
> On 7/17/2026 8:00 PM, Rok Markovic wrote:
>> 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.
>>
>
> Could you please confirm this? Based on my earlier tests, after PHY
> poweron, re-disabling and re-enabling the GRF did not reproduce the
> issue you described.
I can confirm that this is not working but I testedwith writing
registers
manually with devmem and I could make something wrong.
Should I leave it this way or should I move this back to probe and try
to
make it work in enable by some trick (disabel/enable GRF)?
>
>> 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.
>> + */
>
> I think this comment is redundant. We all know this is a common design
> on Rockchip platform, right? :)
>
Done
>> + 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));
>> +}
>
> I think rk3568_lvds_poweron() and rk3568_lvds_grf_config() can be
> merged
> to reduce extra register operations.
Done
>> +
>> +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).
>> + */
>
> I think this comment is redundant. These are internal details of
> phy_set_mode() and don't need to be explained here. Also, the
> ordering requirements are quite common. You can describe them in the
> commit message.
>
Done
>> + 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.
>> + */
>
> Please see the comments above.
Done
>
>> + 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_ */
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
@ 2026-07-21 8:31 ` Rok Markovič
0 siblings, 0 replies; 26+ messages in thread
From: Rok Markovič @ 2026-07-21 8:31 UTC (permalink / raw)
To: Chaoyi Chen
Cc: Heiko Stuebner, Sandy Huang, Andy Yan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, dri-devel,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel,
Alibek Omarov
20.7.2026 3:54, je Chaoyi Chen napisal
> Hello Rok,
>
> Thank you for your patch. Please see the comment below:
>
> On 7/17/2026 8:00 PM, Rok Markovic wrote:
>> 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.
>>
>
> Could you please confirm this? Based on my earlier tests, after PHY
> poweron, re-disabling and re-enabling the GRF did not reproduce the
> issue you described.
I can confirm that this is not working but I testedwith writing
registers
manually with devmem and I could make something wrong.
Should I leave it this way or should I move this back to probe and try
to
make it work in enable by some trick (disabel/enable GRF)?
>
>> 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.
>> + */
>
> I think this comment is redundant. We all know this is a common design
> on Rockchip platform, right? :)
>
Done
>> + 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));
>> +}
>
> I think rk3568_lvds_poweron() and rk3568_lvds_grf_config() can be
> merged
> to reduce extra register operations.
Done
>> +
>> +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).
>> + */
>
> I think this comment is redundant. These are internal details of
> phy_set_mode() and don't need to be explained here. Also, the
> ordering requirements are quite common. You can describe them in the
> commit message.
>
Done
>> + 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.
>> + */
>
> Please see the comments above.
Done
>
>> + 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_ */
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
2026-07-21 8:31 ` Rok Markovič
@ 2026-07-21 8:48 ` Chaoyi Chen
-1 siblings, 0 replies; 26+ messages in thread
From: Chaoyi Chen @ 2026-07-21 8:48 UTC (permalink / raw)
To: Rok Markovič
Cc: Heiko Stuebner, Sandy Huang, Andy Yan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, dri-devel,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel,
Alibek Omarov
On 7/21/2026 4:31 PM, Rok Markovič wrote:
> 20.7.2026 3:54, je Chaoyi Chen napisal
>> Hello Rok,
>>
>> Thank you for your patch. Please see the comment below:
>>
>> On 7/17/2026 8:00 PM, Rok Markovic wrote:
>>> 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.
>>>
>>
>> Could you please confirm this? Based on my earlier tests, after PHY
>> poweron, re-disabling and re-enabling the GRF did not reproduce the
>> issue you described.
>
> I can confirm that this is not working but I testedwith writing registers
> manually with devmem and I could make something wrong.
>
> Should I leave it this way or should I move this back to probe and try to
> make it work in enable by some trick (disabel/enable GRF)?
>
Just out of curiosity. I think it's fine as it is now :)
>>
>>> 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.
>>> + */
>>
>> I think this comment is redundant. We all know this is a common design
>> on Rockchip platform, right? :)
>>
>
> Done
>
>>> + 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));
>>> +}
>>
>> I think rk3568_lvds_poweron() and rk3568_lvds_grf_config() can be merged
>> to reduce extra register operations.
>
> Done
>
>>> +
>>> +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).
>>> + */
>>
>> I think this comment is redundant. These are internal details of
>> phy_set_mode() and don't need to be explained here. Also, the
>> ordering requirements are quite common. You can describe them in the
>> commit message.
>>
>
> Done
>
>>> + 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.
>>> + */
>>
>> Please see the comments above.
>
> Done
>
>>
>>> + 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_ */
>
>
--
Best,
Chaoyi
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
@ 2026-07-21 8:48 ` Chaoyi Chen
0 siblings, 0 replies; 26+ messages in thread
From: Chaoyi Chen @ 2026-07-21 8:48 UTC (permalink / raw)
To: Rok Markovič
Cc: Heiko Stuebner, Sandy Huang, Andy Yan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, dri-devel,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel,
Alibek Omarov
On 7/21/2026 4:31 PM, Rok Markovič wrote:
> 20.7.2026 3:54, je Chaoyi Chen napisal
>> Hello Rok,
>>
>> Thank you for your patch. Please see the comment below:
>>
>> On 7/17/2026 8:00 PM, Rok Markovic wrote:
>>> 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.
>>>
>>
>> Could you please confirm this? Based on my earlier tests, after PHY
>> poweron, re-disabling and re-enabling the GRF did not reproduce the
>> issue you described.
>
> I can confirm that this is not working but I testedwith writing registers
> manually with devmem and I could make something wrong.
>
> Should I leave it this way or should I move this back to probe and try to
> make it work in enable by some trick (disabel/enable GRF)?
>
Just out of curiosity. I think it's fine as it is now :)
>>
>>> 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.
>>> + */
>>
>> I think this comment is redundant. We all know this is a common design
>> on Rockchip platform, right? :)
>>
>
> Done
>
>>> + 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));
>>> +}
>>
>> I think rk3568_lvds_poweron() and rk3568_lvds_grf_config() can be merged
>> to reduce extra register operations.
>
> Done
>
>>> +
>>> +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).
>>> + */
>>
>> I think this comment is redundant. These are internal details of
>> phy_set_mode() and don't need to be explained here. Also, the
>> ordering requirements are quite common. You can describe them in the
>> commit message.
>>
>
> Done
>
>>> + 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.
>>> + */
>>
>> Please see the comments above.
>
> Done
>
>>
>>> + 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_ */
>
>
--
Best,
Chaoyi
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
2026-07-17 12:00 ` Rok Markovic
@ 2026-07-20 5:39 ` Alibek Omarov
-1 siblings, 0 replies; 26+ messages in thread
From: Alibek Omarov @ 2026-07-20 5:39 UTC (permalink / raw)
To: Rok Markovic
Cc: Heiko Stuebner, Sandy Huang, Andy Yan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, dri-devel,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel,
Jakob Loeschke
> Based on Alibek Omarov's earlier posting [1], with the changes below.
Hi, I'm Alibek Omarov and thanks for picking up my patches. I didn't
had time to finish them and these days I don't work on hardware that
uses any of Rockchip SoCs. However, if I'm not wrong, it's still used
in production and I had report from Jakob Loeschke (added in CC) who
confirmed it worked in their setup as well.
Thanks.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
@ 2026-07-20 5:39 ` Alibek Omarov
0 siblings, 0 replies; 26+ messages in thread
From: Alibek Omarov @ 2026-07-20 5:39 UTC (permalink / raw)
To: Rok Markovic
Cc: Heiko Stuebner, Sandy Huang, Andy Yan, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, dri-devel,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel,
Jakob Loeschke
> Based on Alibek Omarov's earlier posting [1], with the changes below.
Hi, I'm Alibek Omarov and thanks for picking up my patches. I didn't
had time to finish them and these days I don't work on hardware that
uses any of Rockchip SoCs. However, if I'm not wrong, it's still used
in production and I had report from Jakob Loeschke (added in CC) who
confirmed it worked in their setup as well.
Thanks.
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 26+ messages in thread
* AW: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
2026-07-20 5:39 ` Alibek Omarov
@ 2026-07-20 12:20 ` Jakob Loe
-1 siblings, 0 replies; 26+ messages in thread
From: Jakob Loe @ 2026-07-20 12:20 UTC (permalink / raw)
To: 'Alibek Omarov', 'Rok Markovic'
Cc: 'Heiko Stuebner', 'Sandy Huang',
'Andy Yan', 'Maarten Lankhorst',
'Maxime Ripard', 'Thomas Zimmermann',
'David Airlie', 'Simona Vetter',
'Rob Herring', 'Krzysztof Kozlowski',
'Conor Dooley', dri-devel, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
Hello!
Nice to see this is still being used.
I can confirm that this actually works and outputs fine LVDS on a RK3566. Also reliably so far...
Any questions, I will see what I can help.
All the best
Jakob
-----Ursprüngliche Nachricht-----
Von: Alibek Omarov <a1ba.omarov@gmail.com>
Gesendet: Montag, 20. Juli 2026 07:39
An: Rok Markovic <rok@kanardia.eu>
Cc: Heiko Stuebner <heiko@sntech.de>; Sandy Huang <hjc@rock-chips.com>; Andy Yan <andy.yan@rock-chips.com>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David Airlie <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; dri-devel@lists.freedesktop.org; devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org; linux-kernel@vger.kernel.org; Jakob Loeschke <jakob@loe-web.de>
Betreff: Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
> Based on Alibek Omarov's earlier posting [1], with the changes below.
Hi, I'm Alibek Omarov and thanks for picking up my patches. I didn't had time to finish them and these days I don't work on hardware that uses any of Rockchip SoCs. However, if I'm not wrong, it's still used in production and I had report from Jakob Loeschke (added in CC) who confirmed it worked in their setup as well.
Thanks.
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 26+ messages in thread
* AW: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
@ 2026-07-20 12:20 ` Jakob Loe
0 siblings, 0 replies; 26+ messages in thread
From: Jakob Loe @ 2026-07-20 12:20 UTC (permalink / raw)
To: 'Alibek Omarov', 'Rok Markovic'
Cc: 'Heiko Stuebner', 'Sandy Huang',
'Andy Yan', 'Maarten Lankhorst',
'Maxime Ripard', 'Thomas Zimmermann',
'David Airlie', 'Simona Vetter',
'Rob Herring', 'Krzysztof Kozlowski',
'Conor Dooley', dri-devel, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
Hello!
Nice to see this is still being used.
I can confirm that this actually works and outputs fine LVDS on a RK3566. Also reliably so far...
Any questions, I will see what I can help.
All the best
Jakob
-----Ursprüngliche Nachricht-----
Von: Alibek Omarov <a1ba.omarov@gmail.com>
Gesendet: Montag, 20. Juli 2026 07:39
An: Rok Markovic <rok@kanardia.eu>
Cc: Heiko Stuebner <heiko@sntech.de>; Sandy Huang <hjc@rock-chips.com>; Andy Yan <andy.yan@rock-chips.com>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David Airlie <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; dri-devel@lists.freedesktop.org; devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org; linux-kernel@vger.kernel.org; Jakob Loeschke <jakob@loe-web.de>
Betreff: Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support
> Based on Alibek Omarov's earlier posting [1], with the changes below.
Hi, I'm Alibek Omarov and thanks for picking up my patches. I didn't had time to finish them and these days I don't work on hardware that uses any of Rockchip SoCs. However, if I'm not wrong, it's still used in production and I had report from Jakob Loeschke (added in CC) who confirmed it worked in their setup as well.
Thanks.
^ permalink raw reply [flat|nested] 26+ messages in thread