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/[email protected]/ > > Co-developed-by: Alibek Omarov <[email protected]> > Signed-off-by: Alibek Omarov <[email protected]> > Signed-off-by: Rok Markovic <[email protected]> > 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
