Re: [PATCH v8 13/21] drm/rockchip: dw_dp: Implement out-of-band HPD handling

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing runtime PM acquisition in `dw_dp_rockchip_hpd_sw_cfg` and `dw_dp_rockchip_hpd_sw_sel` causes potential SErrors when accessing `vo_grf`.
- [Medium] Logical flaw in `dw_dp_rockchip_get_vo_grf` prevents probing on future platforms, contrary to its comment.
--

--- Patch [13]: [PATCH v8 13/21] drm/rockchip: dw_dp: Implement out-of-band HPD handling ---
commit 1e82a4f2c1a3fedc07fd8e258e2008997986261a
Author: Sebastian Reichel <[email protected]>

drm/rockchip: dw_dp: Implement out-of-band HPD handling

This commit implements out-of-band hotplug handling for USB-C by accessing
the vo_grf directly.

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index 005938dc66c9f..d516b3910b516 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c

[ ... ]

> @@ -23,12 +26,48 @@
>  
>  #include "rockchip_drm_drv.h"
>  
> +#define ROCKCHIP_MAX_CTRLS 2
> +
> +#define ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL BIT(10)
> +#define ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG BIT(11)

[ ... ]

> +static void dw_dp_rockchip_hpd_sw_sel(void *data, bool force_hpd_from_sw)
> +{
> +	struct rockchip_dw_dp *dp = data;
> +	u32 hpd_reg = dp->pdata->hpd_reg[dp->id];
> +
> +	regmap_write(dp->vo_grf, hpd_reg,
> +		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL, force_hpd_from_sw));

[Severity: High]
Does this register access require a runtime PM reference?

The commit message notes that this register requires the power-domain from the
DP controller, and that moving the access here prevents massive SError problems
once runtime PM is enabled. However, it appears this code accesses the register
without acquiring a runtime PM reference (e.g., pm_runtime_get_sync()). 

If the DP controller is runtime suspended when a hotplug event occurs, won't
this regmap_write() still cause an SError?

> +}
> +
> +static void dw_dp_rockchip_hpd_sw_cfg(void *data, bool hpd)
> +{
> +	struct rockchip_dw_dp *dp = data;
> +	u32 hpd_reg = dp->pdata->hpd_reg[dp->id];
> +
> +	dev_dbg(dp->dev, "Force HPD connected=%s\n", str_yes_no(hpd));
> +
> +	regmap_write(dp->vo_grf, hpd_reg,
> +		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG, hpd));

[Severity: High]
Does this register access also require a runtime PM reference to prevent an
SError, for the same reasons as above?

> +}
> +
>  static int dw_dp_encoder_atomic_check(struct drm_encoder *encoder,
>  				      struct drm_crtc_state *crtc_state,
>  				      struct drm_connector_state *conn_state)

[ ... ]

> @@ -71,6 +110,35 @@ static const struct drm_encoder_helper_funcs dw_dp_encoder_helper_funcs = {
>  	.atomic_check		= dw_dp_encoder_atomic_check,
>  };
>  
> +static struct regmap *dw_dp_rockchip_get_vo_grf(struct rockchip_dw_dp *dp)
> +{
> +	struct device_node *np = dev_of_node(dp->dev);
> +	struct of_phandle_args args;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	ret = of_parse_phandle_with_args(np, "phys", "#phy-cells", 0, &args);
> +	if (ret)
> +		return ERR_PTR(-ENODEV);
> +
> +	/*
> +	 * Limit this workaround to RK3576 and RK3588, potential future platforms
> +	 * reusing the driver should just add a VO GRF phandle in the DisplayPort
> +	 * controller DT node.
> +	 */
> +	if (!of_device_is_compatible(args.np, "rockchip,rk3576-usbdp-phy") &&
> +	    !of_device_is_compatible(args.np, "rockchip,rk3588-usbdp-phy")) {
> +		regmap = ERR_PTR(-ENODEV);
> +		goto out_put_node;
> +	}

[Severity: Medium]
Will this compatibility check prevent the driver from probing on future
platforms?

The comment suggests that future platforms should add a "rockchip,vo-grf"
phandle in the DisplayPort controller DT node. However, if the PHY node does
not match the legacy compatibles, this code unconditionally returns
ERR_PTR(-ENODEV) and never attempts to parse the "rockchip,vo-grf" phandle
from the DP controller node.

Does this effectively break probe for any new platforms?

> +
> +	regmap = syscon_regmap_lookup_by_phandle(args.np, "rockchip,vo-grf");
> +
> +out_put_node:
> +	of_node_put(args.np);
> +	return regmap;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-synopsys-dw-dp-improvements-v8-0-ac1e6a75782f@collabora.com?part=13