Re: [PATCH v4 1/2] pmdomain: imx: Fix i.MX8MP power notifier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `imx8mp_vpu_power_notifier` fails to ungate the NoC clock (bit 3) and omits `GENPD_NOTIFY_PRE_OFF` handling, which contradicts the commit message and breaks the ADB handshake logic during power transitions.
--

commit b6702343f03cbdda69190bd48f95c1b0b1fe3617
Author: Peng Fan <peng.fan@nxp.com>

pmdomain: imx: Fix i.MX8MP power notifier

Using imx8mm_vpu_power_notifier() for i.MX8MP is wrong, as it ungates
the VPU clocks to provide the ADB clock, which is necessary on i.MX8MM,
but on i.MX8MP there is a separate gate (bit 3) for the NoC. So add
imx8mp_vpu_power_notifier() for i.MX8MP.

> diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> index 19e992d2ee3b8..e13a47eeed75d 100644
> --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
[ ... ]
> @@ -514,9 +514,34 @@ static const struct imx8m_blk_ctrl_domain_data imx8mp_vpu_blk_ctl_domain_data[]
>  	},
>  };
>  
> +static int imx8mp_vpu_power_notifier(struct notifier_block *nb,
> +				     unsigned long action, void *data)
> +{
> +	struct imx8m_blk_ctrl *bc = container_of(nb, struct imx8m_blk_ctrl,
> +						 power_nb);
> +
> +	if (action == GENPD_NOTIFY_ON) {

[Severity: High]
The commit message states there is a separate gate (bit 3) for the NoC on
i.MX8MP, but does this implementation actually ungate it?

It looks like the NoC clock (bit 3) is never ungated here. Also, there is no
handling for the GENPD_NOTIFY_PRE_OFF action.

During both power up and power down transitions, the GPC driver waits for an
ADB handshake. Without explicitly enabling the NoC clock, won't the hardware
state machine lack the necessary clock to respond to the GPC, potentially
stalling the handshake and deadlocking the system during domain transitions?

> +		/*
> +		 * On power up we have no software backchannel to the GPC to
> +		 * wait for the ADB handshake to happen, so we just delay for a
> +		 * bit. On power down the GPC driver waits for the handshake.
> +		 */
> +
> +		udelay(5);
> +
> +		/* set "fuse" bits to enable the VPUs */
> +		regmap_set_bits(bc->regmap, 0x8, 0xffffffff);
> +		regmap_set_bits(bc->regmap, 0xc, 0xffffffff);
> +		regmap_set_bits(bc->regmap, 0x10, 0xffffffff);
> +		regmap_set_bits(bc->regmap, 0x14, 0xffffffff);
> +	}
> +
> +	return NOTIFY_OK;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-b4-imx8mp-vc8000e-pm-v4-1-v4-0-ea58ce929c84@nxp.com?part=1