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

[Peng Fan <peng.fan@oss.nxp.com>]

On Wed, Jun 10, 2026 at 01:36:14PM +0000, sashiko-bot@kernel.org wrote:
>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.

This is expected, keep bit 3 always in hardware default state. No need
to handle GENPD_NOTIFY_PRE_OFF

Regards
Peng

>
>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