[PATCH v2] pmdomain: imx8m-blk-ctrl: Serialize power on/off across sibling domains

[PATCH v2] pmdomain: imx8m-blk-ctrl: Serialize power on/off across sibling domains

[Ming Qian]

On i.MX8MP the VPU blk-ctrl exposes G1, G2 and VC8000E as three separate
genpds, each serialized only by its own genpd lock, so their power_on and
power_off callbacks can run concurrently on SMP.

The sequences are not independent: they share the VPUMIX bus domain, the
VPU_NOC and the ADB400 handshake. On power up the GPC cannot ack-verify
the ADB400 handshake - the ack only completes once blk-ctrl sets the bus
clk-en bit - so it just waits a fixed delay instead of polling hskack. A
sibling transition landing inside another domain's reset window disturbs
that shared clock and handshake timing, the victim's reset does not take
effect, and its block registers read back all zeros: the decoder times
out or the encoder fails its format check.

Serialize the blk-ctrl reset sequence with a per-blk-ctrl mutex; the
driver deliberately avoids a genpd hierarchy, so the genpd core gives no
cross-sibling serialization.

Fixes: a1a5f15f7f6c ("soc: imx: imx8m-blk-ctrl: add i.MX8MP VPU blk ctrl")
Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
Problem:
On i.MX8MP, running the VC8000E encoder and the G1/G2 decoders
concurrently rarely and non-deterministically leaves a VPU stuck in
reset: its block registers read back all zeros. A decoder then times out
or the encoder fails its format check.

Root cause:
The VPU blk-ctrl exposes G1, G2 and VC8000E as three separate genpds,
serialized only by the per-genpd lock, so on SMP their power_on/power_off
callbacks can run concurrently. The sequences share the VPUMIX bus
domain, the VPU_NOC and the ADB400 handshake. On power up the GPC does
not ack-verify the ADB400 handshake - the ack only completes once
blk-ctrl sets the bus clk-en bit - so it just waits a fixed delay. A
sibling transition landing inside another domain's reset window disturbs
that shared clock and handshake timing, the victim's reset fails to take
effect, and its block is left in reset with registers reading zero.

Fix:
Serialize the blk-ctrl reset sequence with a per-blk-ctrl mutex, so a
sibling domain cannot run its sequence while another is inside its reset
window. The driver deliberately avoids a genpd hierarchy, so the genpd
core provides no cross-sibling serialization.

Test:
i.MX8MP, Android 6.18 kernel, concurrent H.264 decode and encode. Without
this patch the failure reproduces within one to two hours. With it the
same test ran overnight, over 14 hours, without a single occurrence.
---
Changes in v2:
- Replace guard(mutex) with explicit mutex_lock()/mutex_unlock():
  power_on() already unwinds errors with goto, and cleanup.h asks not to
  mix goto and scope-based cleanup in one function (sashiko-bot).
- Shorten the commit message to the essentials and move the detailed
  hardware analysis into this cover letter (Frank Li).
- Link to v1: https://patch.msgid.link/20260915-imx8mp-blk-ctrl-v1-1-b3b4e6e7e676@oss.nxp.com

To: Ulf Hansson <ulfh@kernel.org>
To: Frank Li <Frank.Li@nxp.com>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Pengutronix Kernel Team <kernel@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
To: Shawn Guo <shawnguo@kernel.org>
To: Peng Fan <peng.fan@nxp.com>
Cc: linux-pm@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/pmdomain/imx/imx8m-blk-ctrl.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
index 479789009c7f..270f43229fe7 100644
--- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
+++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
@@ -15,6 +15,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/clk.h>
+#include <linux/mutex.h>
 
 #include <dt-bindings/power/imx8mm-power.h>
 #include <dt-bindings/power/imx8mn-power.h>
@@ -34,6 +35,12 @@ struct imx8m_blk_ctrl {
 	struct regmap *regmap;
 	struct imx8m_blk_ctrl_domain *domains;
 	struct genpd_onecell_data onecell_data;
+	/*
+	 * Serializes the blk-ctrl reset/clock sequence across sibling domains;
+	 * their transitions interact through the shared VPUMIX bus domain,
+	 * VPU_NOC and the not-ack-verified ADB400 handshake (ERR050531).
+	 */
+	struct mutex power_lock;
 };
 
 struct imx8m_blk_ctrl_domain_data {
@@ -98,12 +105,14 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
 	struct imx8m_blk_ctrl *bc = domain->bc;
 	int ret;
 
+	mutex_lock(&bc->power_lock);
+
 	/* make sure bus domain is awake */
 	ret = pm_runtime_get_sync(bc->bus_power_dev);
 	if (ret < 0) {
 		pm_runtime_put_noidle(bc->bus_power_dev);
 		dev_err(bc->dev, "failed to power up bus domain\n");
-		return ret;
+		goto unlock;
 	}
 
 	/* put devices into reset */
@@ -148,12 +157,16 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
 	/* disable upstream clocks */
 	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
 
+	mutex_unlock(&bc->power_lock);
+
 	return 0;
 
 clk_disable:
 	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
 bus_put:
 	pm_runtime_put(bc->bus_power_dev);
+unlock:
+	mutex_unlock(&bc->power_lock);
 
 	return ret;
 }
@@ -164,6 +177,8 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
 	const struct imx8m_blk_ctrl_domain_data *data = domain->data;
 	struct imx8m_blk_ctrl *bc = domain->bc;
 
+	mutex_lock(&bc->power_lock);
+
 	/* put devices into reset and disable clocks */
 	if (data->mipi_phy_rst_mask)
 		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
@@ -177,6 +192,8 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
 	/* allow bus domain to suspend */
 	pm_runtime_put(bc->bus_power_dev);
 
+	mutex_unlock(&bc->power_lock);
+
 	return 0;
 }
 
@@ -202,6 +219,10 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev)
 
 	bc->dev = dev;
 
+	ret = devm_mutex_init(dev, &bc->power_lock);
+	if (ret)
+		return ret;
+
 	bc_data = of_device_get_match_data(dev);
 
 	base = devm_platform_ioremap_resource(pdev, 0);

---
base-commit: 27953c044974baf7e24dee3e9342fe0103dea80c
change-id: 20260911-imx8mp-blk-ctrl-c46f26783073

Re: [PATCH v2] pmdomain: imx8m-blk-ctrl: Serialize power on/off across sibling domains

[Frank Li]

On Thu, Sep 17, 2026 at 04:07:54PM +0900, Ming Qian wrote:
> On i.MX8MP the VPU blk-ctrl exposes G1, G2 and VC8000E as three separate
> genpds, each serialized only by its own genpd lock, so their power_on and
> power_off callbacks can run concurrently on SMP.
>
> The sequences are not independent: they share the VPUMIX bus domain, the
> VPU_NOC and the ADB400 handshake. On power up the GPC cannot ack-verify
> the ADB400 handshake - the ack only completes once blk-ctrl sets the bus
> clk-en bit - so it just waits a fixed delay instead of polling hskack. A
> sibling transition landing inside another domain's reset window disturbs
> that shared clock and handshake timing, the victim's reset does not take
> effect, and its block registers read back all zeros: the decoder times
> out or the encoder fails its format check.
>
> Serialize the blk-ctrl reset sequence with a per-blk-ctrl mutex; the
> driver deliberately avoids a genpd hierarchy, so the genpd core gives no
> cross-sibling serialization.
>
> Fixes: a1a5f15f7f6c ("soc: imx: imx8m-blk-ctrl: add i.MX8MP VPU blk ctrl")
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> ---
> Problem:
> On i.MX8MP, running the VC8000E encoder and the G1/G2 decoders
> concurrently rarely and non-deterministically leaves a VPU stuck in
> reset: its block registers read back all zeros. A decoder then times out
> or the encoder fails its format check.
>
> Root cause:
> The VPU blk-ctrl exposes G1, G2 and VC8000E as three separate genpds,
> serialized only by the per-genpd lock, so on SMP their power_on/power_off
> callbacks can run concurrently. The sequences share the VPUMIX bus
> domain, the VPU_NOC and the ADB400 handshake. On power up the GPC does
> not ack-verify the ADB400 handshake - the ack only completes once
> blk-ctrl sets the bus clk-en bit - so it just waits a fixed delay. A
> sibling transition landing inside another domain's reset window disturbs
> that shared clock and handshake timing, the victim's reset fails to take
> effect, and its block is left in reset with registers reading zero.
>
> Fix:
> Serialize the blk-ctrl reset sequence with a per-blk-ctrl mutex, so a
> sibling domain cannot run its sequence while another is inside its reset
> window. The driver deliberately avoids a genpd hierarchy, so the genpd
> core provides no cross-sibling serialization.
>
> Test:
> i.MX8MP, Android 6.18 kernel, concurrent H.264 decode and encode. Without
> this patch the failure reproduces within one to two hours. With it the
> same test ran overnight, over 14 hours, without a single occurrence.
> ---
> Changes in v2:
> - Replace guard(mutex) with explicit mutex_lock()/mutex_unlock():
>   power_on() already unwinds errors with goto, and cleanup.h asks not to
>   mix goto and scope-based cleanup in one function (sashiko-bot).
> - Shorten the commit message to the essentials and move the detailed
>   hardware analysis into this cover letter (Frank Li).
> - Link to v1: https://patch.msgid.link/20260915-imx8mp-blk-ctrl-v1-1-b3b4e6e7e676@oss.nxp.com
>
> To: Ulf Hansson <ulfh@kernel.org>
> To: Frank Li <Frank.Li@nxp.com>
> To: Sascha Hauer <s.hauer@pengutronix.de>
> To: Pengutronix Kernel Team <kernel@pengutronix.de>
> To: Fabio Estevam <festevam@gmail.com>
> To: Shawn Guo <shawnguo@kernel.org>
> To: Peng Fan <peng.fan@nxp.com>
> Cc: linux-pm@vger.kernel.org
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/pmdomain/imx/imx8m-blk-ctrl.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> index 479789009c7f..270f43229fe7 100644
> --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> @@ -15,6 +15,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/clk.h>
> +#include <linux/mutex.h>
>
>  #include <dt-bindings/power/imx8mm-power.h>
>  #include <dt-bindings/power/imx8mn-power.h>
> @@ -34,6 +35,12 @@ struct imx8m_blk_ctrl {
>  	struct regmap *regmap;
>  	struct imx8m_blk_ctrl_domain *domains;
>  	struct genpd_onecell_data onecell_data;
> +	/*
> +	 * Serializes the blk-ctrl reset/clock sequence across sibling domains;
> +	 * their transitions interact through the shared VPUMIX bus domain,
> +	 * VPU_NOC and the not-ack-verified ADB400 handshake (ERR050531).
> +	 */
> +	struct mutex power_lock;
>  };
>
>  struct imx8m_blk_ctrl_domain_data {
> @@ -98,12 +105,14 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>  	struct imx8m_blk_ctrl *bc = domain->bc;
>  	int ret;
>
> +	mutex_lock(&bc->power_lock);
> +
>  	/* make sure bus domain is awake */
>  	ret = pm_runtime_get_sync(bc->bus_power_dev);
>  	if (ret < 0) {
>  		pm_runtime_put_noidle(bc->bus_power_dev);
>  		dev_err(bc->dev, "failed to power up bus domain\n");
> -		return ret;
> +		goto unlock;

can you use auto cleanup guard()

Frank

>  	}
>
>  	/* put devices into reset */
> @@ -148,12 +157,16 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
>  	/* disable upstream clocks */
>  	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
>
> +	mutex_unlock(&bc->power_lock);
> +
>  	return 0;
>
>  clk_disable:
>  	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
>  bus_put:
>  	pm_runtime_put(bc->bus_power_dev);
> +unlock:
> +	mutex_unlock(&bc->power_lock);
>
>  	return ret;
>  }
> @@ -164,6 +177,8 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
>  	const struct imx8m_blk_ctrl_domain_data *data = domain->data;
>  	struct imx8m_blk_ctrl *bc = domain->bc;
>
> +	mutex_lock(&bc->power_lock);
> +
>  	/* put devices into reset and disable clocks */
>  	if (data->mipi_phy_rst_mask)
>  		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> @@ -177,6 +192,8 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
>  	/* allow bus domain to suspend */
>  	pm_runtime_put(bc->bus_power_dev);
>
> +	mutex_unlock(&bc->power_lock);
> +
>  	return 0;
>  }
>
> @@ -202,6 +219,10 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev)
>
>  	bc->dev = dev;
>
> +	ret = devm_mutex_init(dev, &bc->power_lock);
> +	if (ret)
> +		return ret;
> +
>  	bc_data = of_device_get_match_data(dev);
>
>  	base = devm_platform_ioremap_resource(pdev, 0);
>
> ---
> base-commit: 27953c044974baf7e24dee3e9342fe0103dea80c
> change-id: 20260911-imx8mp-blk-ctrl-c46f26783073
>
>

Re: [PATCH v2] pmdomain: imx8m-blk-ctrl: Serialize power on/off across sibling domains

[Ming Qian(OSS)]

Hi Frank,

On Thu, Sep 17, 2026 at 11:42:22AM -0500, Frank Li wrote:
> On Thu, Sep 17, 2026 at 04:07:54PM +0900, Ming Qian wrote:
> > On i.MX8MP the VPU blk-ctrl exposes G1, G2 and VC8000E as three separate
> > genpds, each serialized only by its own genpd lock, so their power_on and
> > power_off callbacks can run concurrently on SMP.
> >
> > The sequences are not independent: they share the VPUMIX bus domain, the
> > VPU_NOC and the ADB400 handshake. On power up the GPC cannot ack-verify
> > the ADB400 handshake - the ack only completes once blk-ctrl sets the bus
> > clk-en bit - so it just waits a fixed delay instead of polling hskack. A
> > sibling transition landing inside another domain's reset window disturbs
> > that shared clock and handshake timing, the victim's reset does not take
> > effect, and its block registers read back all zeros: the decoder times
> > out or the encoder fails its format check.
> >
> > Serialize the blk-ctrl reset sequence with a per-blk-ctrl mutex; the
> > driver deliberately avoids a genpd hierarchy, so the genpd core gives no
> > cross-sibling serialization.
> >
> > Fixes: a1a5f15f7f6c ("soc: imx: imx8m-blk-ctrl: add i.MX8MP VPU blk ctrl")
> > Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> > ---
> > Problem:
> > On i.MX8MP, running the VC8000E encoder and the G1/G2 decoders
> > concurrently rarely and non-deterministically leaves a VPU stuck in
> > reset: its block registers read back all zeros. A decoder then times out
> > or the encoder fails its format check.
> >
> > Root cause:
> > The VPU blk-ctrl exposes G1, G2 and VC8000E as three separate genpds,
> > serialized only by the per-genpd lock, so on SMP their power_on/power_off
> > callbacks can run concurrently. The sequences share the VPUMIX bus
> > domain, the VPU_NOC and the ADB400 handshake. On power up the GPC does
> > not ack-verify the ADB400 handshake - the ack only completes once
> > blk-ctrl sets the bus clk-en bit - so it just waits a fixed delay. A
> > sibling transition landing inside another domain's reset window disturbs
> > that shared clock and handshake timing, the victim's reset fails to take
> > effect, and its block is left in reset with registers reading zero.
> >
> > Fix:
> > Serialize the blk-ctrl reset sequence with a per-blk-ctrl mutex, so a
> > sibling domain cannot run its sequence while another is inside its reset
> > window. The driver deliberately avoids a genpd hierarchy, so the genpd
> > core provides no cross-sibling serialization.
> >
> > Test:
> > i.MX8MP, Android 6.18 kernel, concurrent H.264 decode and encode. Without
> > this patch the failure reproduces within one to two hours. With it the
> > same test ran overnight, over 14 hours, without a single occurrence.
> > ---
> > Changes in v2:
> > - Replace guard(mutex) with explicit mutex_lock()/mutex_unlock():
> >   power_on() already unwinds errors with goto, and cleanup.h asks not to
> >   mix goto and scope-based cleanup in one function (sashiko-bot).
> > - Shorten the commit message to the essentials and move the detailed
> >   hardware analysis into this cover letter (Frank Li).
> > - Link to v1: https://patch.msgid.link/20260915-imx8mp-blk-ctrl-v1-1-b3b4e6e7e676@oss.nxp.com
> >
> > To: Ulf Hansson <ulfh@kernel.org>
> > To: Frank Li <Frank.Li@nxp.com>
> > To: Sascha Hauer <s.hauer@pengutronix.de>
> > To: Pengutronix Kernel Team <kernel@pengutronix.de>
> > To: Fabio Estevam <festevam@gmail.com>
> > To: Shawn Guo <shawnguo@kernel.org>
> > To: Peng Fan <peng.fan@nxp.com>
> > Cc: linux-pm@vger.kernel.org
> > Cc: imx@lists.linux.dev
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  drivers/pmdomain/imx/imx8m-blk-ctrl.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> > index 479789009c7f..270f43229fe7 100644
> > --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> > +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regmap.h>
> >  #include <linux/clk.h>
> > +#include <linux/mutex.h>
> >
> >  #include <dt-bindings/power/imx8mm-power.h>
> >  #include <dt-bindings/power/imx8mn-power.h>
> > @@ -34,6 +35,12 @@ struct imx8m_blk_ctrl {
> >  	struct regmap *regmap;
> >  	struct imx8m_blk_ctrl_domain *domains;
> >  	struct genpd_onecell_data onecell_data;
> > +	/*
> > +	 * Serializes the blk-ctrl reset/clock sequence across sibling domains;
> > +	 * their transitions interact through the shared VPUMIX bus domain,
> > +	 * VPU_NOC and the not-ack-verified ADB400 handshake (ERR050531).
> > +	 */
> > +	struct mutex power_lock;
> >  };
> >
> >  struct imx8m_blk_ctrl_domain_data {
> > @@ -98,12 +105,14 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> >  	struct imx8m_blk_ctrl *bc = domain->bc;
> >  	int ret;
> >
> > +	mutex_lock(&bc->power_lock);
> > +
> >  	/* make sure bus domain is awake */
> >  	ret = pm_runtime_get_sync(bc->bus_power_dev);
> >  	if (ret < 0) {
> >  		pm_runtime_put_noidle(bc->bus_power_dev);
> >  		dev_err(bc->dev, "failed to power up bus domain\n");
> > -		return ret;
> > +		goto unlock;
> 
> can you use auto cleanup guard()
> 
> Frank
> 

v1 did exactly that, and sashiko-bot flagged it, because
imx8m_blk_ctrl_power_on() unwinds its errors with goto:

  https://patch.msgid.link/20260915102028.2CFFD1F000FF@smtp.kernel.org

According to the cleanup subsystem guidelines (include/linux/cleanup.h),
using goto and scope-based cleanup helpers shouldn't be mixed in the same
function:

   * Lastly, given that the benefit of cleanup helpers is removal of
   * "goto", and that the "goto" statement can jump between scopes, the
   * expectation is that usage of "goto" and cleanup helpers is never
   * mixed in the same function. I.e. for a given routine, convert all
   * resources that need a "goto" cleanup to scope-based cleanup, or
   * convert none of them.

So v2 changed imx8m_blk_ctrl_power_on() back from guard() to
mutex_lock(), and imx8m_blk_ctrl_power_off() follows the same style for
consistency.

Regards,
Ming

> >  	}
> >
> >  	/* put devices into reset */
> > @@ -148,12 +157,16 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> >  	/* disable upstream clocks */
> >  	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> >
> > +	mutex_unlock(&bc->power_lock);
> > +
> >  	return 0;
> >
> >  clk_disable:
> >  	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> >  bus_put:
> >  	pm_runtime_put(bc->bus_power_dev);
> > +unlock:
> > +	mutex_unlock(&bc->power_lock);
> >
> >  	return ret;
> >  }
> > @@ -164,6 +177,8 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> >  	const struct imx8m_blk_ctrl_domain_data *data = domain->data;
> >  	struct imx8m_blk_ctrl *bc = domain->bc;
> >
> > +	mutex_lock(&bc->power_lock);
> > +
> >  	/* put devices into reset and disable clocks */
> >  	if (data->mipi_phy_rst_mask)
> >  		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > @@ -177,6 +192,8 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> >  	/* allow bus domain to suspend */
> >  	pm_runtime_put(bc->bus_power_dev);
> >
> > +	mutex_unlock(&bc->power_lock);
> > +
> >  	return 0;
> >  }
> >
> > @@ -202,6 +219,10 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev)
> >
> >  	bc->dev = dev;
> >
> > +	ret = devm_mutex_init(dev, &bc->power_lock);
> > +	if (ret)
> > +		return ret;
> > +
> >  	bc_data = of_device_get_match_data(dev);
> >
> >  	base = devm_platform_ioremap_resource(pdev, 0);
> >
> > ---
> > base-commit: 27953c044974baf7e24dee3e9342fe0103dea80c
> > change-id: 20260911-imx8mp-blk-ctrl-c46f26783073
> >
> >

Re: [PATCH v2] pmdomain: imx8m-blk-ctrl: Serialize power on/off across sibling domains

[Frank Li]

On Fri, Sep 18, 2026 at 11:08:06AM +0900, Ming Qian(OSS) wrote:
> Hi Frank,
>
> On Thu, Sep 17, 2026 at 11:42:22AM -0500, Frank Li wrote:
> > On Thu, Sep 17, 2026 at 04:07:54PM +0900, Ming Qian wrote:
> > > On i.MX8MP the VPU blk-ctrl exposes G1, G2 and VC8000E as three separate
> > > genpds, each serialized only by its own genpd lock, so their power_on and
> > > power_off callbacks can run concurrently on SMP.
> > >
> > > The sequences are not independent: they share the VPUMIX bus domain, the
> > > VPU_NOC and the ADB400 handshake. On power up the GPC cannot ack-verify
> > > the ADB400 handshake - the ack only completes once blk-ctrl sets the bus
> > > clk-en bit - so it just waits a fixed delay instead of polling hskack. A
> > > sibling transition landing inside another domain's reset window disturbs
> > > that shared clock and handshake timing, the victim's reset does not take
> > > effect, and its block registers read back all zeros: the decoder times
> > > out or the encoder fails its format check.
> > >
> > > Serialize the blk-ctrl reset sequence with a per-blk-ctrl mutex; the
> > > driver deliberately avoids a genpd hierarchy, so the genpd core gives no
> > > cross-sibling serialization.
> > >
> > > Fixes: a1a5f15f7f6c ("soc: imx: imx8m-blk-ctrl: add i.MX8MP VPU blk ctrl")
> > > Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> > > ---
> > > Problem:
> > > On i.MX8MP, running the VC8000E encoder and the G1/G2 decoders
> > > concurrently rarely and non-deterministically leaves a VPU stuck in
> > > reset: its block registers read back all zeros. A decoder then times out
> > > or the encoder fails its format check.
> > >
> > > Root cause:
> > > The VPU blk-ctrl exposes G1, G2 and VC8000E as three separate genpds,
> > > serialized only by the per-genpd lock, so on SMP their power_on/power_off
> > > callbacks can run concurrently. The sequences share the VPUMIX bus
> > > domain, the VPU_NOC and the ADB400 handshake. On power up the GPC does
> > > not ack-verify the ADB400 handshake - the ack only completes once
> > > blk-ctrl sets the bus clk-en bit - so it just waits a fixed delay. A
> > > sibling transition landing inside another domain's reset window disturbs
> > > that shared clock and handshake timing, the victim's reset fails to take
> > > effect, and its block is left in reset with registers reading zero.
> > >
> > > Fix:
> > > Serialize the blk-ctrl reset sequence with a per-blk-ctrl mutex, so a
> > > sibling domain cannot run its sequence while another is inside its reset
> > > window. The driver deliberately avoids a genpd hierarchy, so the genpd
> > > core provides no cross-sibling serialization.
> > >
> > > Test:
> > > i.MX8MP, Android 6.18 kernel, concurrent H.264 decode and encode. Without
> > > this patch the failure reproduces within one to two hours. With it the
> > > same test ran overnight, over 14 hours, without a single occurrence.
> > > ---
> > > Changes in v2:
> > > - Replace guard(mutex) with explicit mutex_lock()/mutex_unlock():
> > >   power_on() already unwinds errors with goto, and cleanup.h asks not to
> > >   mix goto and scope-based cleanup in one function (sashiko-bot).
> > > - Shorten the commit message to the essentials and move the detailed
> > >   hardware analysis into this cover letter (Frank Li).
> > > - Link to v1: https://patch.msgid.link/20260915-imx8mp-blk-ctrl-v1-1-b3b4e6e7e676@oss.nxp.com
> > >
> > > To: Ulf Hansson <ulfh@kernel.org>
> > > To: Frank Li <Frank.Li@nxp.com>
> > > To: Sascha Hauer <s.hauer@pengutronix.de>
> > > To: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > To: Fabio Estevam <festevam@gmail.com>
> > > To: Shawn Guo <shawnguo@kernel.org>
> > > To: Peng Fan <peng.fan@nxp.com>
> > > Cc: linux-pm@vger.kernel.org
> > > Cc: imx@lists.linux.dev
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Cc: linux-kernel@vger.kernel.org
> > > ---
> > >  drivers/pmdomain/imx/imx8m-blk-ctrl.c | 23 ++++++++++++++++++++++-
> > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> > > index 479789009c7f..270f43229fe7 100644
> > > --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> > > +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c
> > > @@ -15,6 +15,7 @@
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/clk.h>
> > > +#include <linux/mutex.h>
> > >
> > >  #include <dt-bindings/power/imx8mm-power.h>
> > >  #include <dt-bindings/power/imx8mn-power.h>
> > > @@ -34,6 +35,12 @@ struct imx8m_blk_ctrl {
> > >  	struct regmap *regmap;
> > >  	struct imx8m_blk_ctrl_domain *domains;
> > >  	struct genpd_onecell_data onecell_data;
> > > +	/*
> > > +	 * Serializes the blk-ctrl reset/clock sequence across sibling domains;
> > > +	 * their transitions interact through the shared VPUMIX bus domain,
> > > +	 * VPU_NOC and the not-ack-verified ADB400 handshake (ERR050531).
> > > +	 */
> > > +	struct mutex power_lock;
> > >  };
> > >
> > >  struct imx8m_blk_ctrl_domain_data {
> > > @@ -98,12 +105,14 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > >  	struct imx8m_blk_ctrl *bc = domain->bc;
> > >  	int ret;
> > >
> > > +	mutex_lock(&bc->power_lock);
> > > +
> > >  	/* make sure bus domain is awake */
> > >  	ret = pm_runtime_get_sync(bc->bus_power_dev);
> > >  	if (ret < 0) {
> > >  		pm_runtime_put_noidle(bc->bus_power_dev);
> > >  		dev_err(bc->dev, "failed to power up bus domain\n");
> > > -		return ret;
> > > +		goto unlock;
> >
> > can you use auto cleanup guard()
> >
> > Frank
> >
>
> v1 did exactly that, and sashiko-bot flagged it, because
> imx8m_blk_ctrl_power_on() unwinds its errors with goto:
>
>   https://patch.msgid.link/20260915102028.2CFFD1F000FF@smtp.kernel.org
>
> According to the cleanup subsystem guidelines (include/linux/cleanup.h),
> using goto and scope-based cleanup helpers shouldn't be mixed in the same
> function:
>
>    * Lastly, given that the benefit of cleanup helpers is removal of
>    * "goto", and that the "goto" statement can jump between scopes, the
>    * expectation is that usage of "goto" and cleanup helpers is never
>    * mixed in the same function. I.e. for a given routine, convert all
>    * resources that need a "goto" cleanup to scope-based cleanup, or
>    * convert none of them.
>
> So v2 changed imx8m_blk_ctrl_power_on() back from guard() to
> mutex_lock(), and imx8m_blk_ctrl_power_off() follows the same style for
> consistency.

include/linux/cleanup.h, the description is not exactly correct. The major
means is avoid goto back to cleanup scope, which will cause scope's cause.

err:

guard()
goto err

sashiko mark feedback as low. I think it is fine by use guard, if your goto
to do tear down work.

Frank

>
> Regards,
> Ming
>
> > >  	}
> > >
> > >  	/* put devices into reset */
> > > @@ -148,12 +157,16 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > >  	/* disable upstream clocks */
> > >  	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > >
> > > +	mutex_unlock(&bc->power_lock);
> > > +
> > >  	return 0;
> > >
> > >  clk_disable:
> > >  	clk_bulk_disable_unprepare(data->num_clks, domain->clks);
> > >  bus_put:
> > >  	pm_runtime_put(bc->bus_power_dev);
> > > +unlock:
> > > +	mutex_unlock(&bc->power_lock);
> > >
> > >  	return ret;
> > >  }
> > > @@ -164,6 +177,8 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > >  	const struct imx8m_blk_ctrl_domain_data *data = domain->data;
> > >  	struct imx8m_blk_ctrl *bc = domain->bc;
> > >
> > > +	mutex_lock(&bc->power_lock);
> > > +
> > >  	/* put devices into reset and disable clocks */
> > >  	if (data->mipi_phy_rst_mask)
> > >  		regmap_clear_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask);
> > > @@ -177,6 +192,8 @@ static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > >  	/* allow bus domain to suspend */
> > >  	pm_runtime_put(bc->bus_power_dev);
> > >
> > > +	mutex_unlock(&bc->power_lock);
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -202,6 +219,10 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev)
> > >
> > >  	bc->dev = dev;
> > >
> > > +	ret = devm_mutex_init(dev, &bc->power_lock);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	bc_data = of_device_get_match_data(dev);
> > >
> > >  	base = devm_platform_ioremap_resource(pdev, 0);
> > >
> > > ---
> > > base-commit: 27953c044974baf7e24dee3e9342fe0103dea80c
> > > change-id: 20260911-imx8mp-blk-ctrl-c46f26783073
> > >
> > >