Re: [PATCH v9 14/22] drm/rockchip: dw_dp: Implement out-of-band HPD handling

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unpowered Register Access (SError) in OOB HPD callbacks.
- [Medium] Unreachable fallback logic in `dw_dp_rockchip_get_vo_grf`.
- [Medium] Physical IO address truncation due to `u32` typing in `ctrl_ids`.
--

--- Patch [14]: [PATCH v9 14/22] drm/rockchip: dw_dp: Implement out-of-band HPD handling ---
commit 48b6797379434de3ea2e6ba62d355e782a42ada0
Author: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/[email protected]>

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

This patch implements out-of-band hotplug handling to receive external
hotplug information from the USB-C state machine via the vo_grf regmap.
It aims to avoid SError problems when runtime PM is enabled.

> 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)
> +
> +struct rockchip_dw_dp_plat_data {
> +	u8 num_ctrls;
> +	u32 ctrl_ids[ROCKCHIP_MAX_CTRLS];

[Severity: Medium]
Will this truncate physical IO addresses on 64-bit architectures? 

The ctrl_ids array stores base IO addresses using u32, but in 
dw_dp_rockchip_probe() this is compared against the 64-bit 
resource_size_t res->start:

	if (res->start == plat_data_const->ctrl_ids[id]) {

If a future variant maps the DP controller above the 4GB boundary, 
the upper 32 bits of the address will be lost, causing the hardware 
controller ID lookup to fail.

> +	u32 max_link_rate;
> +	u8 pixel_mode;
> +	u32 hpd_reg[ROCKCHIP_MAX_CTRLS];
> +};
> +
>  struct rockchip_dw_dp {
>  	struct dw_dp *base;
>  	struct device *dev;
> +	const struct rockchip_dw_dp_plat_data *pdata;
> +	struct regmap *vo_grf;
>  	struct rockchip_encoder *encoder;
> +	int id;
>  };
>  
> +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 code risk triggering an SError by accessing unpowered registers? 

The commit message states that accessing the vo_grf register requires the DP 
controller power domain to be active. Since this function is called during 
initialization and the driver enables runtime PM with autosuspend, the write 
might execute against an unpowered register domain. Should this acquire a PM 
runtime reference via pm_runtime_get_sync(dp->dev) first?

> +}
> +
> +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]
Similarly, because this out-of-band hotplug notification is triggered 
asynchronously by the Type-C controller, could the DP controller be suspended 
when this executes? This seems to risk triggering the exact SError the patch 
intended to fix if a wake up isn't performed before the regmap write.

> +}
> +
[ ... ]
> @@ -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 unconditionally fail for future platforms instead of attempting 
the fallback mechanism? 

The comment above states that future platforms should add a VO GRF phandle 
in the DP controller DT node. However, this logic explicitly returns -ENODEV 
if the phy isn't one of the hardcoded compatible strings, without ever parsing 
the rockchip,vo-grf phandle from the DisplayPort node. 

> +
> +	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/20260803-synopsys-dw-dp-improvements-v9-0-2e6e46135312-ZGY8ohtN/[email protected]?part=14