On 6/18/26 12:39 PM, Philipp Zabel wrote:
> On Mi, 2026-06-17 at 21:51 +0300, Cristian Ciocaltea wrote:
>> Assert the AXI reset in the CRTC disable path, and the VP DCLK reset in
>> the enable path.
>>
>> These resets are intended to leave the hardware in a clean state for the
>> next use, helping recover from exceptions such as IOMMU page faults, as
>> well as to prevent random display output glitches, such as a blank
>> image, observed when switching modes that also change the color format,
>> e.g. from RGB to YUV420 and vice versa.
>>
>> For now this seems to affect only the RK3588, hence the resets are
>> optional and will be provided in the device tree for this SoC only.
>>
>> Co-developed-by: Andy Yan <[email protected]>
>> Signed-off-by: Andy Yan <[email protected]>
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>> ---
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 35
>> ++++++++++++++++++++++++++++
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 4 ++++
>> 2 files changed, 39 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index 4cce3e336f5b..2833fb49ad81 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -17,6 +17,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/regmap.h>
>> +#include <linux/reset.h>
>> #include <linux/swab.h>
>>
>> #include <drm/drm.h>
>> @@ -860,6 +861,26 @@ static int vop2_core_clks_prepare_enable(struct vop2
>> *vop2)
>> return ret;
>> }
>>
>> +static void vop2_clk_reset(struct vop2 *vop2, struct reset_control *rstc)
>
> The _clk part of the function name is misleading ...
Ack.
We need to make this clearly distinct from another similarly named function,
vop2_crtc_reset(), hence I'd propose:
- vop2_reset_assert_deassert()
- vop2_reset_cycle()
- vop2_do_reset()
Any preference / alternative suggestions?
>
> [...]
>> @@ -938,6 +959,8 @@ static void vop2_disable(struct vop2 *vop2)
>> {
>> rockchip_drm_dma_detach_device(vop2->drm, vop2->dev);
>>
>> + vop2_clk_reset(vop2, vop2->axi_rst);
>
> ... because this function is also called with the AXI reset control.
>
>> +
>> pm_runtime_put_sync(vop2->dev);
>>
>> regcache_drop_region(vop2->map, 0, vop2_regmap_config.max_register);
>> @@ -1948,6 +1971,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc
>> *crtc,
>>
>> vop2_crtc_atomic_try_set_gamma(vop2, vp, crtc, crtc_state);
>>
>> + vop2_clk_reset(vop2, vp->dclk_rst);
>> +
>> drm_crtc_vblank_on(crtc);
>>
>> vop2_unlock(vop2);
>> @@ -2531,6 +2556,11 @@ static int vop2_create_crtcs(struct vop2 *vop2)
>> return dev_err_probe(drm->dev, PTR_ERR(vp->dclk),
>> "failed to get %s\n", dclk_name);
>>
>> + vp->dclk_rst = devm_reset_control_get_optional(vop2->dev,
>> dclk_name);
>
> Please use devm_reset_control_get_optional_exclusive() directly.
Thanks for pointing this out, I missed the comment mentioning the explicit API
transition.
>> + if (IS_ERR(vp->dclk_rst))
>> + return dev_err_probe(drm->dev, PTR_ERR(vp->dclk_rst),
>> + "failed to get %s reset\n",
>> dclk_name);
>> +
>> np = of_graph_get_remote_node(dev->of_node, i, -1);
>> if (!np) {
>> drm_dbg(vop2->drm, "%s: No remote for vp%d\n",
>> __func__, i);
>> @@ -2890,6 +2920,11 @@ static int vop2_bind(struct device *dev, struct
>> device *master, void *data)
>> return dev_err_probe(drm->dev, PTR_ERR(vop2->pll_hdmiphy1),
>> "failed to get pll_hdmiphy1\n");
>>
>> + vop2->axi_rst = devm_reset_control_get_optional(vop2->dev, "axi");
>
> Same as above, devm_reset_control_get_optional_exclusive().
Ack.
Thanks for reviewing,
Cristian
