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

[Chaoyi Chen <chaoyi.chen@rock-chips.com>]

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