[PATCH v3 1/5] soundwire: intel_ace2x: free master runtime on BPT open error path

[PATCH v3 1/5] soundwire: intel_ace2x: free master runtime on BPT open error path

[Syed Saba Kareem]

From: Syed Saba Kareem <Syed.SabaKareem@amd.com>

intel_ace2x_bpt_open_stream() calls sdw_slave_bpt_stream_add(), which
via sdw_stream_add_slave() -> sdw_master_rt_alloc() allocates the master
runtime, links it into bus->m_rt_list and raises bus->bpt_stream_refcount.
Several later failure paths (PDI allocation, port-config allocation and
sdw_stream_add_master()) jump to the remove_slave label, which only calls
sdw_stream_remove_slave() followed by sdw_release_stream().

sdw_stream_remove_slave() frees only the slave runtime and ports; it does
not reach sdw_master_rt_free(). The master runtime is therefore left on
bus->m_rt_list pointing at the just-freed stream, and bpt_stream_refcount
stays non-zero. Because sdw_master_rt_alloc() rejects a new BPT allocation
while bpt_stream_refcount > 0, every subsequent BPT transfer on that bus
is rejected with -EBUSY until the driver is reloaded.

Route these error paths through the remove_master label so that
sdw_stream_remove_master() frees the master runtime and drops the refcount
before the stream is released, mirroring the error-path unwind in
amd_sdw_bpt_open_stream().

Drop the now-unused remove_slave label; its sdw_stream_remove_slave() call
still runs by falling through from remove_master, and is a no-op once the
master runtime (and with it the slave runtimes) has been freed.

Fixes: 4c1ce9f37d8a ("soundwire: intel_ace2x: add BPT send_async/wait callbacks")
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
---
 drivers/soundwire/intel_ace2x.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/intel_ace2x.c b/drivers/soundwire/intel_ace2x.c
index 642b33ab5526..96940779fdec 100644
--- a/drivers/soundwire/intel_ace2x.c
+++ b/drivers/soundwire/intel_ace2x.c
@@ -105,7 +105,7 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 	if (!pdi0) {
 		dev_err(cdns->dev, "%s: sdw_cdns_alloc_pdi0 failed\n", __func__);
 		ret = -EINVAL;
-		goto remove_slave;
+		goto remove_master;
 	}
 
 	sdw_cdns_config_stream(cdns, 1, dir, pdi0);
@@ -117,7 +117,7 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 	if (!pdi1) {
 		dev_err(cdns->dev, "%s: sdw_cdns_alloc_pdi1 failed\n", __func__);
 		ret = -EINVAL;
-		goto remove_slave;
+		goto remove_master;
 	}
 
 	sdw_cdns_config_stream(cdns, 1, dir, pdi1);
@@ -136,7 +136,7 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 	pconfig = kzalloc_objs(*pconfig, 2);
 	if (!pconfig) {
 		ret =  -ENOMEM;
-		goto remove_slave;
+		goto remove_master;
 	}
 
 	for (i = 0; i < 2 /* num_pdi */; i++) {
@@ -149,7 +149,7 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 
 	if (ret < 0) {
 		dev_err(cdns->dev, "add master to stream failed:%d\n", ret);
-		goto remove_slave;
+		goto remove_master;
 	}
 
 	ret = sdw_prepare_stream(cdns->bus.bpt_stream);
@@ -293,7 +293,6 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 		dev_err(cdns->dev, "%s: remove master failed: %d\n",
 			__func__, ret1);
 
-remove_slave:
 	ret1 = sdw_stream_remove_slave(slave, cdns->bus.bpt_stream);
 	if (ret1 < 0)
 		dev_err(cdns->dev, "%s: remove slave failed: %d\n",
-- 
2.43.0

[PATCH v3 2/5] soundwire: intel_ace2x: order bpt_stream publish/clear against refcount

[Syed Saba Kareem]

From: Syed Saba Kareem <Syed.SabaKareem@amd.com>

The BPT (Bulk Payload Transport) stream pointer bus->bpt_stream is read
locklessly by the SoundWire core to tell whether a BPT transfer owns the
bus. For those readers to be safe the pointer and bus->bpt_stream_refcount
must stay consistent: an observer that sees refcount == 0 under bus_lock
must also see bpt_stream == NULL.

Make intel_ace2x maintain that ordering:

 - Publish bus->bpt_stream with WRITE_ONCE() only after the master
   runtime has been added and bpt_stream_refcount raised, and (on the
   open path) before the in-open sdw_prepare_stream().

 - Clear it with WRITE_ONCE() before sdw_stream_remove_master() drops the
   refcount on the close and error paths. Add a clear_bpt_stream label so
   paths that already published the pointer clear it, while the
   pre-publish failure paths skip the clear.

 - Snapshot the pointer into a local once (READ_ONCE()) so the
   open/close/error paths act on a single stable value instead of
   repeatedly re-reading the shared field.

This is a no-op under the current policy, where BPT and audio streams are
mutually exclusive, but establishes the ordering the core relies on once
BPT is allowed to run alongside idle audio streams.

Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
---
 drivers/soundwire/intel_ace2x.c | 63 ++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 17 deletions(-)

diff --git a/drivers/soundwire/intel_ace2x.c b/drivers/soundwire/intel_ace2x.c
index 96940779fdec..65d3630c2822 100644
--- a/drivers/soundwire/intel_ace2x.c
+++ b/drivers/soundwire/intel_ace2x.c
@@ -83,7 +83,7 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 	int len;
 	int i;
 
-	if (cdns->bus.bpt_stream) {
+	if (READ_ONCE(cdns->bus.bpt_stream)) {
 		dev_err(cdns->dev, "%s: BPT stream already exists\n", __func__);
 		return -EAGAIN;
 	}
@@ -92,8 +92,6 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 	if (!stream)
 		return -ENOMEM;
 
-	cdns->bus.bpt_stream = stream;
-
 	ret = sdw_slave_bpt_stream_add(slave, stream);
 	if (ret < 0)
 		goto release_stream;
@@ -152,9 +150,25 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 		goto remove_master;
 	}
 
-	ret = sdw_prepare_stream(cdns->bus.bpt_stream);
+	/*
+	 * Publish bus->bpt_stream now that the BPT master runtime is fully
+	 * built and bpt_stream_refcount has been raised. The increment happens
+	 * in sdw_slave_bpt_stream_add() above, via sdw_stream_add_slave() ->
+	 * sdw_master_rt_alloc(); sdw_stream_add_master() then reuses that same
+	 * runtime through sdw_master_rt_find() without incrementing it again.
+	 * Publish before sdw_prepare_stream() below: the prepare runs
+	 * sdw_program_params(), whose filter skips idle audio runtimes only
+	 * while bus->bpt_stream is set, so publishing here (rather than after
+	 * the DMA setup) keeps that filter active for the BPT prepare. Ordering
+	 * the publish after the refcount is raised keeps the pointer and
+	 * refcount consistent for lockless observers, mirroring amd_manager.c
+	 * and pairing with the READ_ONCE() in sdw_program_params().
+	 */
+	WRITE_ONCE(cdns->bus.bpt_stream, stream);
+
+	ret = sdw_prepare_stream(stream);
 	if (ret < 0)
-		goto remove_master;
+		goto clear_bpt_stream;
 
 	command = (msg->flags & SDW_MSG_FLAG_WRITE) ? 0 : 1;
 
@@ -285,22 +299,30 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 			__func__, ret1);
 
 deprepare_stream:
-	sdw_deprepare_stream(cdns->bus.bpt_stream);
+	sdw_deprepare_stream(stream);
+
+clear_bpt_stream:
+	/*
+	 * Paths that jump here published bus->bpt_stream above; clear it before
+	 * sdw_stream_remove_master() drops bpt_stream_refcount so the pointer and
+	 * refcount stay consistent for lockless observers. The pre-publish failure
+	 * paths jump to remove_master and skip this clear.
+	 */
+	WRITE_ONCE(cdns->bus.bpt_stream, NULL);
 
 remove_master:
-	ret1 = sdw_stream_remove_master(&cdns->bus, cdns->bus.bpt_stream);
+	ret1 = sdw_stream_remove_master(&cdns->bus, stream);
 	if (ret1 < 0)
 		dev_err(cdns->dev, "%s: remove master failed: %d\n",
 			__func__, ret1);
 
-	ret1 = sdw_stream_remove_slave(slave, cdns->bus.bpt_stream);
+	ret1 = sdw_stream_remove_slave(slave, stream);
 	if (ret1 < 0)
 		dev_err(cdns->dev, "%s: remove slave failed: %d\n",
 			__func__, ret1);
 
 release_stream:
-	sdw_release_stream(cdns->bus.bpt_stream);
-	cdns->bus.bpt_stream = NULL;
+	sdw_release_stream(stream);
 
 	return ret;
 }
@@ -309,6 +331,7 @@ static void intel_ace2x_bpt_close_stream(struct sdw_intel *sdw, struct sdw_slave
 					 struct sdw_bpt_msg *msg)
 {
 	struct sdw_cdns *cdns = &sdw->cdns;
+	struct sdw_stream_runtime *stream = READ_ONCE(cdns->bus.bpt_stream);
 	int ret;
 
 	ret = hda_sdw_bpt_close(cdns->dev->parent /* PCI device */, sdw->instance,
@@ -319,23 +342,29 @@ static void intel_ace2x_bpt_close_stream(struct sdw_intel *sdw, struct sdw_slave
 		dev_err(cdns->dev, "%s:  hda_sdw_bpt_close failed: ret %d\n",
 			__func__, ret);
 
-	ret = sdw_deprepare_stream(cdns->bus.bpt_stream);
+	ret = sdw_deprepare_stream(stream);
 	if (ret < 0)
 		dev_err(cdns->dev, "%s: sdw_deprepare_stream failed: ret %d\n",
 			__func__, ret);
 
-	ret = sdw_stream_remove_master(&cdns->bus, cdns->bus.bpt_stream);
+	/*
+	 * Clear bus->bpt_stream before sdw_stream_remove_master() drops
+	 * bpt_stream_refcount, so the pointer is never visible while the
+	 * refcount reads zero (mirrors the open path and amd_manager.c).
+	 */
+	WRITE_ONCE(cdns->bus.bpt_stream, NULL);
+
+	ret = sdw_stream_remove_master(&cdns->bus, stream);
 	if (ret < 0)
 		dev_err(cdns->dev, "%s: remove master failed: %d\n",
 			__func__, ret);
 
-	ret = sdw_stream_remove_slave(slave, cdns->bus.bpt_stream);
+	ret = sdw_stream_remove_slave(slave, stream);
 	if (ret < 0)
 		dev_err(cdns->dev, "%s: remove slave failed: %d\n",
 			__func__, ret);
 
-	sdw_release_stream(cdns->bus.bpt_stream);
-	cdns->bus.bpt_stream = NULL;
+	sdw_release_stream(stream);
 }
 
 #define INTEL_BPT_MSG_BYTE_MIN 16
@@ -374,7 +403,7 @@ static int intel_ace2x_bpt_send_async(struct sdw_intel *sdw, struct sdw_slave *s
 		return ret;
 	}
 
-	ret = sdw_enable_stream(cdns->bus.bpt_stream);
+	ret = sdw_enable_stream(READ_ONCE(cdns->bus.bpt_stream));
 	if (ret < 0) {
 		dev_err(cdns->dev, "%s: sdw_stream_enable failed: %d\n",
 			__func__, ret);
@@ -397,7 +426,7 @@ static int intel_ace2x_bpt_wait(struct sdw_intel *sdw, struct sdw_slave *slave,
 	if (ret < 0)
 		dev_err(cdns->dev, "%s: hda_sdw_bpt_wait failed: %d\n", __func__, ret);
 
-	ret = sdw_disable_stream(cdns->bus.bpt_stream);
+	ret = sdw_disable_stream(READ_ONCE(cdns->bus.bpt_stream));
 	if (ret < 0) {
 		dev_err(cdns->dev, "%s: sdw_stream_enable failed: %d\n",
 			__func__, ret);
-- 
2.43.0

[PATCH v3 3/5] soundwire: stream: allow flagged BPT firmware download while streams are idle

[Syed Saba Kareem]

From: Syed Saba Kareem <Syed.SabaKareem@amd.com>

sdw_master_rt_alloc() rejected a BPT (Bulk Payload Transport) stream
allocation whenever any audio stream was allocated on the bus
(bus->stream_refcount > 0). On a power-off-mode platform, an amplifier
that was left DISABLED across system suspend still holds an allocated
but idle stream runtime, yet it must re-download its firmware over BPT
on resume before that stream can be re-enabled. The blanket refcount
check made the resume-time BPT transfer fail with -EBUSY.

Rather than key this off the audio stream state - which would relax the
policy for every caller and every scenario, not just a controlled
firmware download - let the manager explicitly flag the transfer. Add
bus->bpt_fw_download, which a BPT-capable manager sets around a firmware
download that it knows may coexist with allocated-but-idle audio streams
and for which it guarantees no audio stream is made active on the bus for
the duration.

sdw_master_rt_alloc() now rejects a BPT allocation when:

 - another BPT transfer is already allocated, or
 - an audio stream is actively using the bus (PREPARED/ENABLED), or
 - an audio stream is merely allocated but idle and the manager has not
   set bus->bpt_fw_download.

Add sdw_bus_has_active_stream() for the active-stream test; it returns
true only for streams in the PREPARED or ENABLED state. When
bpt_fw_download is clear (the default, and the only possibility for
managers that do not opt in) the three checks together reproduce the
original strict policy exactly: any allocated stream, active or idle,
still blocks BPT. Only a manager that sets the flag can allocate BPT
alongside idle audio streams, and even then an actively streaming stream
still blocks it.

The audio path is still protected while a flagged download runs:
sdw_program_params() skips master runtimes other than the active BPT
stream while bus->bpt_stream is set, so BPT preparation does not rewrite
the transport/port parameters of idle audio runtimes or deliver BPT bus
parameters to their peripherals via sdw_notify_config(). The bus-wide
SDW_SCP_BUSCLOCK_SCALE programming is intentionally left unfiltered, as
every attached peripheral must track the actual bus clock. The filter
keys off bus->bpt_stream, which managers publish with WRITE_ONCE() only
after raising bpt_stream_refcount and clear before dropping it;
sdw_program_params() reads it with READ_ONCE(). So an audio path that
sees refcount == 0 under bus_lock also sees bpt_stream == NULL and
programs its own parameters instead of being skipped.

BPT and active audio are mutually exclusive on the bus: a flagged
download is only started while all audio streams are idle, and the
manager that sets bpt_fw_download guarantees that no audio stream is
made active (PREPARED/ENABLED) on the bus for the transfer's duration.
Serialising the two is the manager's/codec's responsibility (for
example, the codec completes its firmware download before starting its
own stream). In practice the flagged download runs in the codec's
power-off-mode resume path, before its stream - left DISABLED across
suspend - is re-enabled, and while userspace tasks are still frozen for
system resume, so no userspace PCM operation (prepare, enable or hw_free)
can race the download window. Because of that guarantee, the only
sdw_program_params() passes while bus->bpt_stream is set are the BPT
stream's own prepare/enable/disable/deprepare passes; the filter above
keeps those passes from reprogramming or re-notifying the idle audio
runtimes that remain allocated on the bus. It is not a mechanism for
running audio traffic concurrently with a download.

While at it, make the allocation-time rejection messages state the
actual reason instead of printing the now-misleading stream_refcount.

Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
---
 drivers/soundwire/stream.c    | 97 +++++++++++++++++++++++++++++++++--
 include/linux/soundwire/sdw.h | 14 +++++
 2 files changed, 108 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index fb5fdafe1999..5162c9cd327f 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -674,9 +674,11 @@ static int sdw_notify_config(struct sdw_master_runtime *m_rt)
 static int sdw_program_params(struct sdw_bus *bus, bool prepare)
 {
 	struct sdw_master_runtime *m_rt;
+	struct sdw_stream_runtime *bpt;
 	struct sdw_slave *slave;
 	int ret = 0;
 	u32 addr1;
+	bool bpt_seen = false;
 
 	/* Check if all Peripherals comply with SDCA */
 	list_for_each_entry(slave, &bus->slaves, node) {
@@ -719,7 +721,21 @@ static int sdw_program_params(struct sdw_bus *bus, bool prepare)
 	}
 
 manager_runtime:
+	/*
+	 * Read bus->bpt_stream once so the whole programming pass uses a
+	 * consistent snapshot. While a BPT transfer owns the bus, only its own
+	 * runtime may be (re)programmed. Any audio runtimes still allocated
+	 * during a flagged resume-time download are idle by contract; skip them
+	 * so BPT preparation does not rewrite their transport/port parameters or
+	 * deliver BPT bus parameters to their Slaves via sdw_notify_config().
+	 */
+	bpt = READ_ONCE(bus->bpt_stream);
 	list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
+		if (bpt) {
+			if (m_rt->stream != bpt)
+				continue;
+			bpt_seen = true;
+		}
 
 		/*
 		 * this loop walks through all master runtimes for a
@@ -756,6 +772,24 @@ static int sdw_program_params(struct sdw_bus *bus, bool prepare)
 		}
 	}
 
+	/*
+	 * bpt_stream is published only while its runtime is on m_rt_list: the
+	 * manager adds the runtime before prepare and clears bpt_stream before
+	 * removing it at teardown, so a non-NULL bpt must always be matched in
+	 * the loop above. If it was not, a BPT teardown left a dangling pointer
+	 * on an error path (or a manager violated the exclusivity contract):
+	 * every audio runtime was filtered out and nothing was programmed.
+	 * Fail loudly rather than return success for Slave registers that were
+	 * never written, which would let the stream state machine advance to
+	 * the bank switch on stale hardware.
+	 */
+	if (bpt && !bpt_seen) {
+		dev_err(bus->dev,
+			"BPT stream set but its runtime is absent; skipped programming\n");
+		WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
+
 	return ret;
 }
 
@@ -1234,6 +1268,33 @@ static struct sdw_master_runtime
 	return NULL;
 }
 
+/*
+ * sdw_bus_has_active_stream() - check for an audio stream actively using the bus
+ *
+ * Returns true if any master runtime on @bus has a stream in the PREPARED or
+ * ENABLED state, i.e. one that is reserving or moving data over the bus. BPT
+ * and active audio are mutually exclusive, so a BPT transfer must not be
+ * started while this returns true. Allocated-but-idle streams
+ * (ALLOCATED/CONFIGURED/DISABLED/DEPREPARED) are not reported here; whether
+ * they permit BPT is decided by the caller and gated on the
+ * bus->bpt_fw_download resume flag.
+ *
+ * Must be called with bus_lock held.
+ */
+static bool sdw_bus_has_active_stream(struct sdw_bus *bus)
+{
+	struct sdw_master_runtime *m_rt;
+
+	list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
+		if (m_rt->stream &&
+		    (m_rt->stream->state == SDW_STREAM_PREPARED ||
+		     m_rt->stream->state == SDW_STREAM_ENABLED))
+			return true;
+	}
+
+	return false;
+}
+
 /**
  * sdw_master_rt_alloc() - Allocates a Master runtime handle
  *
@@ -1250,9 +1311,39 @@ static struct sdw_master_runtime
 	struct list_head *insert_after;
 
 	if (stream->type == SDW_STREAM_BPT) {
-		if (bus->stream_refcount > 0 || bus->bpt_stream_refcount > 0) {
-			dev_err(bus->dev, "%s: %d/%d audio/BPT stream already allocated\n",
-				__func__, bus->stream_refcount, bus->bpt_stream_refcount);
+		/*
+		 * BPT and audio are mutually exclusive on the bus: BPT needs
+		 * exclusive bandwidth, so it must never run while another BPT
+		 * transfer is allocated or while an audio stream is actively using
+		 * the bus (PREPARED/ENABLED).
+		 *
+		 * The one exception is resume-time firmware download, flagged by the
+		 * manager via bus->bpt_fw_download: on a power-off-mode platform the
+		 * codec loses power across system suspend and must re-download its
+		 * firmware over BPT before its stream (left DISABLED across suspend)
+		 * can be re-enabled. In that window the manager guarantees no audio
+		 * stream is made active, so there is no concurrent audio and no
+		 * bandwidth to share; only then may BPT proceed while an idle
+		 * allocated stream exists. Without the flag such a stream blocks BPT.
+		 * This is not a mechanism for running audio concurrently with a
+		 * download.
+		 */
+		if (bus->bpt_stream_refcount > 0) {
+			dev_err(bus->dev,
+				"%s: BPT rejected: another BPT transfer active\n",
+				__func__);
+			return ERR_PTR(-EBUSY);
+		}
+		if (sdw_bus_has_active_stream(bus)) {
+			dev_err(bus->dev,
+				"%s: BPT rejected: audio stream active\n",
+				__func__);
+			return ERR_PTR(-EBUSY);
+		}
+		if (bus->stream_refcount > 0 && !READ_ONCE(bus->bpt_fw_download)) {
+			dev_err(bus->dev,
+				"%s: BPT rejected: audio stream allocated\n",
+				__func__);
 			return ERR_PTR(-EBUSY);
 		}
 	} else {
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index f710e5932b4b..9a3904f42078 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -1005,6 +1005,19 @@ struct sdw_stream_runtime {
  * @bpt_stream_refcount: number of BTP streams currently using this bus (should
  * be zero or one, multiple streams per link is not supported).
  * @bpt_stream: pointer stored to handle BTP streams.
+ * @bpt_fw_download: set by a BPT-capable manager to flag a resume-time firmware
+ * download (BPT/BRA). BPT and active audio are mutually exclusive on the bus;
+ * this flag marks the one narrow exception -- a power-off-mode resume where the
+ * codec must re-download firmware over BPT before its stream (left DISABLED
+ * across suspend) is re-enabled. The manager guarantees no audio stream is made
+ * active on the bus for the duration, so sdw_master_rt_alloc() permits the BPT
+ * allocation even when idle audio streams are still allocated; an actively
+ * streaming audio stream (PREPARED/ENABLED) still blocks BPT. It is not a
+ * mechanism for running audio concurrently with a download. Written with
+ * WRITE_ONCE() by the manager before it enters the stream allocation path and
+ * cleared (also WRITE_ONCE()) after the transfer; read with READ_ONCE() in
+ * sdw_master_rt_alloc() under bus_lock. Single-BPT exclusivity
+ * (bpt_stream_refcount) means no concurrent writer races the lock-protected read.
  * @ops: Master callback ops
  * @port_ops: Master port callback ops
  * @prop: Master properties
@@ -1045,6 +1058,7 @@ struct sdw_bus {
 	int stream_refcount;
 	int bpt_stream_refcount;
 	struct sdw_stream_runtime *bpt_stream;
+	bool bpt_fw_download;
 	const struct sdw_master_ops *ops;
 	const struct sdw_master_port_ops *port_ops;
 	struct sdw_master_prop prop;
-- 
2.43.0

Re: [PATCH v3 3/5] soundwire: stream: allow flagged BPT firmware download while streams are idle

[Pierre-Louis Bossart]

> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index f710e5932b4b..9a3904f42078 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -1005,6 +1005,19 @@ struct sdw_stream_runtime {
>   * @bpt_stream_refcount: number of BTP streams currently using this bus (should
>   * be zero or one, multiple streams per link is not supported).
>   * @bpt_stream: pointer stored to handle BTP streams.
> + * @bpt_fw_download: set by a BPT-capable manager to flag a resume-time firmware
> + * download (BPT/BRA). BPT and active audio are mutually exclusive on the bus;
> + * this flag marks the one narrow exception -- a power-off-mode resume where the
> + * codec must re-download firmware over BPT before its stream (left DISABLED
> + * across suspend) is re-enabled. The manager guarantees no audio stream is made
> + * active on the bus for the duration, so sdw_master_rt_alloc() permits the BPT
> + * allocation even when idle audio streams are still allocated; an actively
> + * streaming audio stream (PREPARED/ENABLED) still blocks BPT. It is not a
> + * mechanism for running audio concurrently with a download. Written with
> + * WRITE_ONCE() by the manager before it enters the stream allocation path and
> + * cleared (also WRITE_ONCE()) after the transfer; read with READ_ONCE() in
> + * sdw_master_rt_alloc() under bus_lock. Single-BPT exclusivity
> + * (bpt_stream_refcount) means no concurrent writer races the lock-protected read.
>   * @ops: Master callback ops
>   * @port_ops: Master port callback ops
>   * @prop: Master properties
> @@ -1045,6 +1058,7 @@ struct sdw_bus {
>  	int stream_refcount;
>  	int bpt_stream_refcount;
>  	struct sdw_stream_runtime *bpt_stream;
> +	bool bpt_fw_download;

nit-pick: I would add this one line above to leave pointers and
structures grouped, this would be nicer in terms of alignment.

>  	const struct sdw_master_ops *ops;
>  	const struct sdw_master_port_ops *port_ops;
>  	struct sdw_master_prop prop;

Re: [PATCH v3 3/5] soundwire: stream: allow flagged BPT firmware download while streams are idle

[Syed Saba Kareem]

On 10/5/26 16:56, Pierre-Louis Bossart wrote:
>> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
>> index f710e5932b4b..9a3904f42078 100644
>> --- a/include/linux/soundwire/sdw.h
>> +++ b/include/linux/soundwire/sdw.h
>> @@ -1005,6 +1005,19 @@ struct sdw_stream_runtime {
>>    * @bpt_stream_refcount: number of BTP streams currently using this bus (should
>>    * be zero or one, multiple streams per link is not supported).
>>    * @bpt_stream: pointer stored to handle BTP streams.
>> + * @bpt_fw_download: set by a BPT-capable manager to flag a resume-time firmware
>> + * download (BPT/BRA). BPT and active audio are mutually exclusive on the bus;
>> + * this flag marks the one narrow exception -- a power-off-mode resume where the
>> + * codec must re-download firmware over BPT before its stream (left DISABLED
>> + * across suspend) is re-enabled. The manager guarantees no audio stream is made
>> + * active on the bus for the duration, so sdw_master_rt_alloc() permits the BPT
>> + * allocation even when idle audio streams are still allocated; an actively
>> + * streaming audio stream (PREPARED/ENABLED) still blocks BPT. It is not a
>> + * mechanism for running audio concurrently with a download. Written with
>> + * WRITE_ONCE() by the manager before it enters the stream allocation path and
>> + * cleared (also WRITE_ONCE()) after the transfer; read with READ_ONCE() in
>> + * sdw_master_rt_alloc() under bus_lock. Single-BPT exclusivity
>> + * (bpt_stream_refcount) means no concurrent writer races the lock-protected read.
>>    * @ops: Master callback ops
>>    * @port_ops: Master port callback ops
>>    * @prop: Master properties
>> @@ -1045,6 +1058,7 @@ struct sdw_bus {
>>   	int stream_refcount;
>>   	int bpt_stream_refcount;
>>   	struct sdw_stream_runtime *bpt_stream;
>> +	bool bpt_fw_download;
> nit-pick: I would add this one line above to leave pointers and
> structures grouped, this would be nicer in terms of alignment.
Good point, will refactor in v4 — bpt_fw_download now sits with the 
scalar fields above the bpt_stream pointer, and I moved its kernel-doc 
entry to match the new field order.
>>   	const struct sdw_master_ops *ops;
>>   	const struct sdw_master_port_ops *port_ops;
>>   	struct sdw_master_prop prop;

[PATCH v3 4/5] soundwire: amd: Add BRA/BPT firmware download support

[Syed Saba Kareem]

From: Syed Saba Kareem <Syed.SabaKareem@amd.com>

Add Bulk Register Access (BRA) / Bulk Payload Transport (BPT) support
for AMD SoundWire platforms. This enables high-speed firmware download
to SoundWire peripherals via DP0, using the ACP BRA DMA engine.

Key design points:

- Uses the SoundWire stream framework (sdw_prepare_stream,
  sdw_enable_stream, sdw_disable_stream, sdw_deprepare_stream)
  for all DP0 port programming and bank switches. No manual DP0
  register writes or bank mirrors are needed.

- BRA transport parameters (hstart, hstop, SampleInterval,
  BytesPerFrame) are computed dynamically from the current bus
  frame shape, not hardcoded.

- The ACP BPT DMA engine is triggered by the bank switch
  performed inside sdw_enable_stream(), and stopped by the
  bank switch in sdw_disable_stream().

- Non-contiguous firmware sections are handled by iterating
  per-section: large sections use BRA DMA, small sections
  (< one BRA frame) fall back to sdw_nwrite/sdw_nread.

- BPT stream m_rt entries are skipped in audio compute_params
  to prevent BPT transport parameters from corrupting audio
  port block offset calculations.

- DP0 port_params, xport_params, and port_enable callbacks
  return early for BPT streams since the ACP BRA descriptor
  registers handle DP0 configuration independently.

- bus->bpt_stream is published with WRITE_ONCE() only after the stream
  runtime is added and bpt_stream_refcount is raised under bus_lock, and
  is cleared before the runtime is removed and the stream is freed, so
  the lockless DP0 port callbacks never observe a half-initialised or
  freed stream pointer.

- bus->bpt_fw_download is set around the transfer so sdw_master_rt_alloc()
  admits the BPT allocation while audio streams on the same bus are
  allocated but idle (for example an amplifier left DISABLED across a
  power-off-mode suspend that must re-download firmware on resume). It is
  cleared on the close and open error paths. An audio stream that is
  actively using the bus still blocks the transfer.

- On an aborted or timed-out transfer the ACP BPT DMA engine is disarmed
  (PORT_EN=0) before the sdw_disable_stream() bank switch, so the bank
  switch cannot re-trigger a DMA write into the buffer that is freed once
  the transfer returns.

- A per-manager bpt_lock serialises concurrent BPT transfers
  from multiple slave probes. pm_runtime keeps the bus clock
  active during transfers.

- PTE-based ACP ATU mapping provides DMA scatter-gather for the
  firmware buffer. The ATU maps up to 512 4KB pages (2 MB per
  transfer); each transfer is additionally bounded by the SoundWire
  BPT limit of 1 MB (SDW_BPT_MSG_MAX_BYTES), which the driver enforces.

Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
---
 drivers/soundwire/amd_init.c      |    1 +
 drivers/soundwire/amd_manager.c   | 1388 ++++++++++++++++++++++++++++-
 drivers/soundwire/amd_manager.h   |   17 +-
 include/linux/soundwire/sdw_amd.h |   35 +
 sound/soc/amd/ps/acp63.h          |    3 +
 sound/soc/amd/ps/pci-ps.c         |    3 +
 sound/soc/sof/amd/acp.c           |    3 +
 sound/soc/sof/amd/acp.h           |    4 +
 8 files changed, 1449 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
index 8e419ddfa516..00f4a87da7cb 100644
--- a/drivers/soundwire/amd_init.c
+++ b/drivers/soundwire/amd_init.c
@@ -120,6 +120,7 @@ static struct sdw_amd_ctx *sdw_amd_probe_controller(struct sdw_amd_res *res)
 
 		sdw_pdata[index].instance = index;
 		sdw_pdata[index].acp_sdw_lock = res->acp_lock;
+		sdw_pdata[index].acp_bra_lock = res->acp_bra_lock;
 		sdw_pdata[index].acp_rev = res->acp_rev;
 		pdevinfo[index].name = "amd_sdw_manager";
 		pdevinfo[index].id = index;
diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index a3316efdf8ac..078e13b01ac9 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -9,6 +9,7 @@
 #include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/io.h>
+#include <linux/dma-mapping.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -16,6 +17,7 @@
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_registers.h>
 #include <linux/pm_runtime.h>
+#include <linux/sched.h>
 #include <linux/wait.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -23,15 +25,53 @@
 #include "amd_init.h"
 #include "amd_manager.h"
 
+/* ATU register offsets */
+#define ACPAXI2AXI_ATU_PAGE_SIZE_GRP_1		0x0000C00
+#define ACPAXI2AXI_ATU_BASE_ADDR_GRP_1		0x0000C04
+#define ACPAXI2AXI_ATU_CTRL			0x0000C40
+#define ACP_SCRATCH_REG_0			0x0010000
+
 #define DRV_NAME "amd_sdw_manager"
 
 #define to_amd_sdw(b)	container_of(b, struct amd_sdw_manager, bus)
 
+#define AMD_BPT_MSG_BYTE_MIN 16
+
+/*
+ * BRA PTE/ATU Configuration Constants
+ *
+ * BRA DMA uses ATU GRP_1 with 4KB pages. GRP_1 PTE table base sits at
+ * ACP_SCRATCH_REG_0 (offset 0x03800000 from MMIO base). The AXI window
+ * for GRP_1 starts at ACP_BRA_MEM_WINDOW_START (0x4000000). PTE entries
+ * are 8 bytes each and start at scratch offset ACP_BRA_PTE_OFFSET (0x0).
+ * ATU_PAGE_SIZE = 0x0 means DISABLED; set ACP_BRA_PAGE_SIZE_4K_ENABLE (0x2)
+ * to enable 4KB page translation.
+ */
+#define ACP_BRA_SRAM_GRP1_BASE		0x03800000
+#define ACP_BRA_PAGE_SIZE_4K_ENABLE	0x2
+#define ACP_BRA_PTE_OFFSET		0x0
+#define ACP_BRA_MEM_WINDOW_START	0x4000000
+#define ACP_BRA_ATU_PTE_ENTRY_SIZE	8
+#define ACP_BRA_MAX_PTE_ENTRIES		512
+struct amd_bra_params {
+	u32 sample_interval;
+	u32 bytes_per_frame;
+	u8 hstart;
+	u8 hstop;
+	u8 word_length;
+	u8 dev_addr;
+	bool write_mode;
+	u32 peripheral_first_byte_addr;
+	u32 dma_base_addr;
+	u32 transfer_length;
+};
+
 static int amd_sdw_clk_init_ctrl(struct amd_sdw_manager *amd_manager)
 {
 	struct sdw_bus *bus = &amd_manager->bus;
 	struct sdw_master_prop *prop = &bus->prop;
-	u32 divider;
+	u32 val;
+	int divider;
 
 	dev_dbg(amd_manager->dev, "mclk %d max %d row %d col %d frame_rate:%d\n",
 		prop->mclk_freq, prop->max_clk_freq, prop->default_row,
@@ -44,16 +84,28 @@ static int amd_sdw_clk_init_ctrl(struct amd_sdw_manager *amd_manager)
 	}
 
 	/* Set clock divider */
+	dev_dbg(amd_manager->dev, "bus params curr_dr_freq: %d\n",
+		bus->params.curr_dr_freq);
 	divider = (prop->mclk_freq / bus->params.curr_dr_freq);
+
 	writel(divider, amd_manager->mmio + ACP_SW_CLK_FREQUENCY_CTRL);
+	val = readl(amd_manager->mmio + ACP_SW_CLK_FREQUENCY_CTRL);
+	dev_dbg(amd_manager->dev, "ACP_SW_CLK_FREQUENCY_CTRL:0x%x\n", val);
 
 	/* Set frame shape base on the actual bus frequency. */
 	prop->default_col = bus->params.curr_dr_freq /
 			    prop->default_frame_rate / prop->default_row;
+
+	dev_dbg(amd_manager->dev, "default_frame_rate:%d default_row: %d default_col: %d\n",
+		prop->default_frame_rate, prop->default_row, prop->default_col);
 	amd_manager->cols_index = sdw_find_col_index(prop->default_col);
 	amd_manager->rows_index = sdw_find_row_index(prop->default_row);
 	bus->params.col = prop->default_col;
 	bus->params.row = prop->default_row;
+	dev_dbg(amd_manager->dev, "rows_index: %d cols_index: %d\n",
+		amd_manager->rows_index, amd_manager->cols_index);
+	dev_dbg(amd_manager->dev, "params.col:0x%x params.row:0x%x\n",
+		bus->params.col, bus->params.row);
 	return 0;
 }
 
@@ -267,7 +319,8 @@ static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
 	}
 
 	if (sts & AMD_SDW_IMM_RES_VALID) {
-		dev_err(amd_manager->dev, "SDW%x manager is in bad state\n", amd_manager->instance);
+		dev_warn(amd_manager->dev, "SDW%x stale IMM response cleared\n",
+			 amd_manager->instance);
 		writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
 	}
 	writel(upper_data, amd_manager->mmio + ACP_SW_IMM_CMD_UPPER_WORD);
@@ -465,12 +518,53 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
 	return slave_stat;
 }
 
+/*
+ * amd_sdw_bra_sample_interval() - Derive the BRA SampleInterval
+ *
+ * The ACP BRA hardware descriptor and the peripheral DP0 must be
+ * programmed with an identical SampleInterval, otherwise the frame
+ * layout seen by the two ends diverges and the transfer misaligns.
+ * Both amd_sdw_compute_params() (peripheral DP0) and
+ * amd_sdw_calculate_bra_params() (ACP BRA descriptor) use this helper
+ * so the value can never diverge.
+ *
+ * BlockCount algorithm (col_width = hstop - hstart + 1):
+ *  - col_width >= 8: BlockCount = 1, SI = nc
+ *  - otherwise:      smallest BlockCount in [2..8] such that
+ *                    (BlockCount * col_width >= 8) && (nr % BlockCount == 0),
+ *                    SI = nc * BlockCount
+ *
+ * Returns 0 if no valid BlockCount can be found.
+ */
+static u32 amd_sdw_bra_sample_interval(u32 nr, u32 nc, u8 hstart, u8 hstop)
+{
+	u8 col_width = hstop - hstart + 1;
+	u32 block_count;
+
+	if (col_width >= 8)
+		return nc;		/* BlockCount = 1 */
+
+	/*
+	 * col_width < 8 (including col_width == 1, which needs BlockCount = 8):
+	 * pick the smallest BlockCount that both reaches a full 8-bit sample
+	 * and divides the row count, so the frame is not fractionally blocked.
+	 */
+	for (block_count = 2; block_count <= 8; block_count++) {
+		if ((block_count * col_width >= 8) &&
+		    (nr % block_count == 0))
+			return nc * block_count;
+	}
+
+	return 0;
+}
+
 static int amd_sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime *stream)
 {
 	struct amd_sdw_manager *amd_manager = to_amd_sdw(bus);
 	struct sdw_transport_data t_data = {0};
 	struct sdw_master_runtime *m_rt;
 	struct sdw_port_runtime *p_rt;
+	struct sdw_slave_runtime *s_rt;
 	struct sdw_bus_params *b_params = &bus->params;
 	int port_bo, hstart, hstop, sample_int;
 	unsigned int rate, bps, channels;
@@ -478,6 +572,60 @@ static int amd_sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime
 	static unsigned int next_offset[AMD_SDW_MAX_MANAGER_COUNT] = {1};
 	unsigned int inst_id = amd_manager->instance;
 
+	/*
+	 * BPT stream: compute DP0 transport/port params so the SoundWire stream
+	 * framework can program peripheral DP0 registers in sdw_prepare_stream().
+	 */
+	if (stream->type == SDW_STREAM_BPT) {
+		u32 nc = bus->params.col;
+		u8 bpt_hstart = amd_manager->bra_hstart;
+		u8 bpt_hstop = amd_manager->bra_hstop;
+		u32 bpt_si = amd_sdw_bra_sample_interval(bus->params.row, nc,
+							 bpt_hstart, bpt_hstop);
+
+		if (!bpt_si || bpt_si > bus->params.row * nc) {
+			dev_err(bus->dev,
+				"BPT: cannot derive SI: NR=%u NC=%u hstart=%u hstop=%u\n",
+				bus->params.row, nc, bpt_hstart, bpt_hstop);
+			return -EINVAL;
+		}
+
+		dev_dbg(bus->dev, "BPT compute: NC=%u hstart=%u hstop=%u SI=%u\n",
+			nc, bpt_hstart, bpt_hstop, bpt_si);
+
+		list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
+			if (m_rt->stream != stream)
+				continue;
+
+			list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
+				sdw_fill_xport_params(&p_rt->transport_params,
+						      p_rt->num, false,
+						      SDW_BLK_GRP_CNT_1, bpt_si,
+						      0, 0, bpt_hstart, bpt_hstop,
+						      SDW_BLK_PKG_PER_PORT, 0);
+				sdw_fill_port_params(&p_rt->port_params,
+						     p_rt->num, 8,
+						     SDW_PORT_FLOW_MODE_ISOCH,
+						     SDW_PORT_DATA_MODE_NORMAL);
+			}
+
+			list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
+				list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
+					sdw_fill_xport_params(&p_rt->transport_params,
+							      p_rt->num, false,
+							      SDW_BLK_GRP_CNT_1, bpt_si,
+							      0, 0, bpt_hstart, bpt_hstop,
+							      SDW_BLK_PKG_PER_PORT, 0);
+					sdw_fill_port_params(&p_rt->port_params,
+							     p_rt->num, 8,
+							     SDW_PORT_FLOW_MODE_ISOCH,
+							     SDW_PORT_DATA_MODE_NORMAL);
+				}
+			}
+		}
+		return 0;
+	}
+
 	port_bo = 0;
 	hstart = 1;
 	hstop = bus->params.col - 1;
@@ -485,6 +633,14 @@ static int amd_sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime
 	t_data.hstart = hstart;
 
 	list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
+		/*
+		 * Skip BPT stream entries when computing audio params.
+		 * BPT may have stream params (rate/bps) that don't match the
+		 * assumptions below and can lead to division by zero.
+		 */
+		if (m_rt->stream->type == SDW_STREAM_BPT)
+			continue;
+
 		rate = m_rt->stream->params.rate;
 		bps = m_rt->stream->params.bps;
 		channels = m_rt->stream->params.ch_count;
@@ -557,6 +713,19 @@ static int amd_sdw_port_params(struct sdw_bus *bus, struct sdw_port_params *p_pa
 	struct amd_sdw_manager *amd_manager = to_amd_sdw(bus);
 	u32 frame_fmt_reg, dpn_frame_fmt;
 
+	/*
+	 * BPT uses the dedicated ACP BRA descriptor registers, not the DPn
+	 * audio port registers, so the manager side of DP0 must not be
+	 * programmed while a BPT transfer owns the bus. Manager port 0 is also
+	 * an audio port (AUDIO0/AUDIO1 TX), but an audio port-0 op cannot race
+	 * a BPT transfer: BPT holds the bus across the whole prepare->transfer
+	 * sequence and the core disallows an active audio stream while BPT
+	 * runs, so a num == 0 op observed here is always the BPT stream. Allow
+	 * DPn (n > 0) audio port ops to proceed even while BPT is active.
+	 */
+	if (READ_ONCE(bus->bpt_stream) && p_params->num == 0)
+		return 0;
+
 	dev_dbg(amd_manager->dev, "p_params->num:0x%x\n", p_params->num);
 	switch (amd_manager->acp_rev) {
 	case ACP63_PCI_REV_ID:
@@ -601,6 +770,19 @@ static int amd_sdw_transport_params(struct sdw_bus *bus,
 	u32 frame_fmt_reg, sample_int_reg, hctrl_dp0_reg;
 	u32 offset_reg, lane_ctrl_ch_en_reg;
 
+	/*
+	 * BPT uses the dedicated ACP BRA descriptor registers, not the DPn
+	 * audio port registers, so the manager side of DP0 must not be
+	 * programmed while a BPT transfer owns the bus. Manager port 0 is also
+	 * an audio port (AUDIO0/AUDIO1 TX), but an audio port-0 op cannot race
+	 * a BPT transfer: BPT holds the bus across the whole prepare->transfer
+	 * sequence and the core disallows an active audio stream while BPT
+	 * runs, so a port_num == 0 op observed here is always the BPT stream.
+	 * Allow DPn (n > 0) audio port ops to proceed even while BPT is active.
+	 */
+	if (READ_ONCE(bus->bpt_stream) && params->port_num == 0)
+		return 0;
+
 	switch (amd_manager->acp_rev) {
 	case ACP63_PCI_REV_ID:
 		switch (amd_manager->instance) {
@@ -673,6 +855,13 @@ static int amd_sdw_port_enable(struct sdw_bus *bus,
 	u32 dpn_ch_enable;
 	u32 lane_ctrl_ch_en_reg;
 
+	/*
+	 * BPT port enable/disable is handled in execute_bra_transfer; ignore
+	 * DP0 ops only. Allow DPn audio port ops even while BPT is active.
+	 */
+	if (READ_ONCE(bus->bpt_stream) && enable_ch->port_num == 0)
+		return 0;
+
 	switch (amd_manager->acp_rev) {
 	case ACP63_PCI_REV_ID:
 		switch (amd_manager->instance) {
@@ -710,6 +899,1071 @@ static int amd_sdw_port_enable(struct sdw_bus *bus,
 	return 0;
 }
 
+static int amd_sdw_calculate_bra_params(struct amd_sdw_manager *amd_manager,
+					struct amd_bra_params *params,
+					u8 peripheral_addr)
+{
+	struct sdw_bus *bus = &amd_manager->bus;
+	u32 nr = bus->params.row;  /* current rows - NOT enlarged */
+	u32 nc = bus->params.col;  /* current cols */
+	u8 hstart = amd_manager->bra_hstart;
+	u8 hstop  = amd_manager->bra_hstop;
+	u8 col_width = hstop - hstart + 1;
+	u32 sample_interval;
+	u32 bits_per_frame;
+	u32 bpf;
+
+	params->hstart      = hstart;
+	params->hstop       = hstop;
+	params->word_length = 8;  /* BRA is always byte-oriented: WL=8 */
+
+	/*
+	 * Derive SampleInterval via the shared helper so the ACP BRA
+	 * descriptor and the peripheral DP0 (programmed in
+	 * amd_sdw_compute_params()) always agree.
+	 */
+	sample_interval = amd_sdw_bra_sample_interval(nr, nc, hstart, hstop);
+
+	if (!sample_interval || sample_interval > nr * nc) {
+		dev_err(amd_manager->dev,
+			"BPT: cannot derive SI: col_width=%u NR=%u NC=%u\n",
+			col_width, nr, nc);
+		return -EINVAL;
+	}
+
+	bits_per_frame = ((nr * nc) / sample_interval) * params->word_length;
+	bpf = bits_per_frame / 8;
+	if (bpf <= 10) {
+		dev_err(amd_manager->dev,
+			"BPT: frame too small: %u bytes (NR=%u NC=%u SI=%u)\n",
+			bpf, nr, nc, sample_interval);
+		return -EINVAL;
+	}
+	bpf -= 10;  /* subtract BRA protocol overhead */
+	if (bpf > 511)
+		bpf = 511;
+
+	params->sample_interval = sample_interval;
+	params->bytes_per_frame = bpf;
+	params->dev_addr        = peripheral_addr;
+
+	dev_dbg(amd_manager->dev,
+		"BPT calc_params: NR=%u NC=%u col_width=%u WL=%u SI=%u BPF=%u hstart=%u hstop=%u dev=%u\n",
+		nr, nc, col_width, params->word_length, sample_interval,
+		bpf, hstart, hstop, peripheral_addr);
+	return 0;
+}
+
+/*
+ * Latched when a BRA transfer leaves the ACP DMA engine wedged: it failed to
+ * confirm it stopped and may still be bus-mastering through the ACP-global ATU
+ * PTEs. Once set, amd_sdw_bra_configure_pte() refuses to reprogram those shared
+ * PTEs, so a still-active engine on one manager instance cannot be redirected
+ * into another instance's buffer. The flag lives in the shared ACP device
+ * context (acp_bra_wedged, reached through amd_manager->acp_bra_wedged) so it
+ * is scoped to one ACP controller and does not disable BRA on an independent
+ * ACP instance. Written and read only under acp_bra_lock and never cleared:
+ * recovering the engine requires a device reset / driver reload.
+ */
+static u32 amd_sdw_bra_configure_pte(struct amd_sdw_manager *amd_manager,
+				     dma_addr_t dma_addr, size_t size)
+{
+	u32 num_pages = (u32)(PAGE_ALIGN(size) >> PAGE_SHIFT);
+	u32 low, high, val;
+	u16 page_idx;
+	dma_addr_t addr = dma_addr;
+
+	if (*amd_manager->acp_bra_wedged) {
+		dev_err(amd_manager->dev,
+			"BRA engine previously wedged; refusing to reprogram shared ATU PTEs\n");
+		return 0;
+	}
+
+	if (num_pages > ACP_BRA_MAX_PTE_ENTRIES) {
+		dev_err(amd_manager->dev,
+			"BRA buffer too large: %u pages (max %u)\n",
+			num_pages, ACP_BRA_MAX_PTE_ENTRIES);
+		return 0;
+	}
+
+	/*
+	 * Program ATU GRP_1 with 4KB pages. PTE entries start at scratch offset
+	 * ACP_BRA_PTE_OFFSET (0x0); DMA AXI base = ACP_BRA_MEM_WINDOW_START.
+	 *
+	 * The ATU and scratch registers are ACP-global, so they must be
+	 * accessed via acp_mmio (the shared ACP base) rather than mmio
+	 * (which carries the per-instance SDW_MANAGER_REG_OFFSET). Otherwise
+	 * SoundWire instance 1 would program the wrong physical addresses.
+	 */
+	writel(ACP_BRA_SRAM_GRP1_BASE | BIT(31),
+	       amd_manager->acp_mmio + ACPAXI2AXI_ATU_BASE_ADDR_GRP_1);
+	writel(ACP_BRA_PAGE_SIZE_4K_ENABLE,
+	       amd_manager->acp_mmio + ACPAXI2AXI_ATU_PAGE_SIZE_GRP_1);
+
+	val = ACP_BRA_PTE_OFFSET;
+	for (page_idx = 0; page_idx < num_pages; page_idx++) {
+		low  = lower_32_bits(addr);
+		high = upper_32_bits(addr) | BIT(31);
+		writel(low,  amd_manager->acp_mmio + ACP_SCRATCH_REG_0 + val);
+		writel(high, amd_manager->acp_mmio + ACP_SCRATCH_REG_0 + val + 4);
+		val += ACP_BRA_ATU_PTE_ENTRY_SIZE;
+		addr += PAGE_SIZE;
+	}
+
+	/* Flush ATU cache to ensure PTE update takes effect */
+	writel(0x1, amd_manager->acp_mmio + ACPAXI2AXI_ATU_CTRL);
+
+	dev_dbg(amd_manager->dev,
+		"BRA PTE: phys=0x%llx pages=%u ACP=0x%08x\n",
+		(u64)dma_addr, num_pages, ACP_BRA_MEM_WINDOW_START);
+
+	return ACP_BRA_MEM_WINDOW_START;
+}
+
+static void amd_sdw_bra_deconfigure_pte(struct amd_sdw_manager *amd_manager,
+					size_t size)
+{
+	u32 num_pages = (u32)(PAGE_ALIGN(size) >> PAGE_SHIFT);
+	u32 val;
+	u16 page_idx;
+
+	if (num_pages > ACP_BRA_MAX_PTE_ENTRIES)
+		num_pages = ACP_BRA_MAX_PTE_ENTRIES;
+
+	/* Clear all BRA PTE entries at scratch[ACP_BRA_PTE_OFFSET..] */
+	val = ACP_BRA_PTE_OFFSET;
+	for (page_idx = 0; page_idx < num_pages; page_idx++) {
+		writel(0, amd_manager->acp_mmio + ACP_SCRATCH_REG_0 + val);
+		writel(0, amd_manager->acp_mmio + ACP_SCRATCH_REG_0 + val + 4);
+		val += ACP_BRA_ATU_PTE_ENTRY_SIZE;
+	}
+
+	/* Flush ATU cache */
+	writel(0x1, amd_manager->acp_mmio + ACPAXI2AXI_ATU_CTRL);
+}
+
+static int amd_sdw_config_bra_descriptor(struct amd_sdw_manager *amd_manager,
+					 struct amd_bra_params *params)
+{
+	u32 frame_format, hctrl;
+	u32 int_mask;
+
+	if (params->dev_addr > 15 ||
+	    params->sample_interval == 0 || params->sample_interval > 65536 ||
+	    params->hstart > 15 || params->hstop > 15 ||
+	    params->word_length == 0 || params->word_length > 64 ||
+	    params->bytes_per_frame > 511)
+		return -EINVAL;
+
+	/*
+	 * ACP BPT_PORT_HCTRL: encode as (hstart << 4) | hstop.
+	 * The ACP BRA engine starts reading from column hstart (including
+	 * the BRA frame header).  No offset adjustment is needed.
+	 */
+	hctrl = (u32)((params->hstart << 4) | params->hstop);
+
+	frame_format =
+		((u32)(params->word_length - 1) << 2)  |  /* [7:2]  WordLength-1       */
+		(params->write_mode ? BIT(10) : 0U)    |  /* [10]   Write/Read         */
+		((u32)params->bytes_per_frame << 11)   |  /* [19:11] BytesPerFrame     */
+		((u32)params->dev_addr << 20);		   /* [23:20] DeviceAddr        */
+
+	/* Mask BRA error interrupts while programming descriptor */
+	int_mask = readl(amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
+	int_mask &= ~(u32)AMD_SDW_BPT_ERR_INTR_MASK;
+	writel(int_mask, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
+
+	writel(frame_format,                  amd_manager->mmio + ACP_SW_BPT_PORT_FRAME_FORMAT);
+	writel(params->sample_interval - 1,   amd_manager->mmio + ACP_SW_BPT_PORT_SAMPLEINTERVAL);
+	writel(hctrl,                         amd_manager->mmio + ACP_SW_BPT_PORT_HCTRL);
+	writel(0,                             amd_manager->mmio + ACP_SW_BPT_PORT_OFFSET);
+	writel(BIT(3),                        amd_manager->mmio + ACP_SW_BPT_PORT_CHANNEL_ENABLE);
+	writel(params->peripheral_first_byte_addr,
+	       amd_manager->mmio + ACP_SW_BPT_PORT_FIRST_BYTE_ADDR);
+	writel(params->dma_base_addr,         amd_manager->mmio + ACP_SW_BRA_BASE_ADDRESS);
+	writel(params->transfer_length,       amd_manager->mmio + ACP_SW_BRA_TRANSFER_SIZE);
+
+	int_mask |= AMD_SDW_BPT_ERR_INTR_MASK;
+	writel(int_mask, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
+
+	dev_dbg(amd_manager->dev,
+		"BPT config_desc: FF=0x%08x SI=0x%x HC=0x%02x CE=0x%02x periph=0x%08x dma=0x%08x xfer=%u\n",
+		frame_format, params->sample_interval - 1, hctrl, (u32)BIT(3),
+		params->peripheral_first_byte_addr, params->dma_base_addr,
+		params->transfer_length);
+	return 0;
+}
+
+static void amd_sdw_deconfig_bra_descriptor(struct amd_sdw_manager *amd_manager)
+{
+	writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_FRAME_FORMAT);
+	writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_SAMPLEINTERVAL);
+	writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_HCTRL);
+	writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_OFFSET);
+	writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_CHANNEL_ENABLE);
+	writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_FIRST_BYTE_ADDR);
+	writel(0, amd_manager->mmio + ACP_SW_BRA_BASE_ADDRESS);
+	writel(0, amd_manager->mmio + ACP_SW_BRA_TRANSFER_SIZE);
+}
+
+static int amd_sdw_execute_bra_transfer(struct amd_sdw_manager *amd_manager,
+					struct sdw_slave *slave,
+					bool *dma_unsafe)
+{
+	struct sdw_bus *bus = &amd_manager->bus;
+	u32 i2s_err_offset;
+	u32 saved_intr_mask;
+	u32 reg_addr, len;
+	u32 val;
+	int ret, ret_disable;
+
+	/* Read descriptor regs before enabling the DMA engine. */
+	reg_addr = readl(amd_manager->mmio + ACP_SW_BPT_PORT_FIRST_BYTE_ADDR);
+	len = readl(amd_manager->mmio + ACP_SW_BRA_TRANSFER_SIZE);
+
+	i2s_err_offset = (amd_manager->instance == 0) ?
+			 ACP_SW_I2S_ERROR_REASON : ACP_P1_SW_I2S_ERROR_REASON;
+
+	/*
+	 * Save and disable the error interrupt mask for manual error
+	 * checking.  acp_bra_lock is held across the whole BPT sequence by
+	 * amd_sdw_bpt_wait(), which serialises this shared-register
+	 * read-modify-write against the other manager instance.
+	 */
+	saved_intr_mask = readl(amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
+	writel(0, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
+	writel(0, amd_manager->acp_mmio + i2s_err_offset);
+	writel(0, amd_manager->mmio + ACP_SW_ERROR_REASON1);
+
+	/* Arm the ACP BPT DMA engine */
+	writel(1, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
+
+	/*
+	 * Use the framework's sdw_enable_stream() to write CHANNELEN and
+	 * perform a bank switch.  The ACP BPT hardware uses the bank switch
+	 * as the trigger to start the DMA transfer.  The framework manages
+	 * bank state consistently, eliminating the need for a manual bank
+	 * switch or DP0 bank mirror.
+	 */
+	dev_dbg(amd_manager->dev,
+		"BPT: pre-enable: curr_bank=%u next_bank=%u BPT_EN_STATUS=0x%x stream_state=%d\n",
+		bus->params.curr_bank, bus->params.next_bank,
+		readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
+		bus->bpt_stream->state);
+
+	ret = sdw_enable_stream(bus->bpt_stream);
+	if (ret < 0) {
+		dev_err(amd_manager->dev,
+			"BPT: sdw_enable_stream failed: %d\n", ret);
+		/*
+		 * Disarm the engine and wait for the port to quiesce before
+		 * returning: the caller (amd_sdw_bra_transfer()) then deconfigures
+		 * the BRA descriptor unconditionally, zeroing BASE_ADDRESS and
+		 * TRANSFER_SIZE, so the port must be idle first -- mirrors the
+		 * disable path below.
+		 */
+		writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
+		ret_disable = readl_poll_timeout(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS,
+						 val, !val, ACP_DELAY_US, AMD_SDW_TIMEOUT);
+		if (ret_disable < 0) {
+			dev_err(amd_manager->dev, "BPT: PORT_EN disable timeout\n");
+			*dma_unsafe = true;
+		}
+		goto restore_intr;
+	}
+
+	dev_dbg(amd_manager->dev,
+		"BPT: DMA started: curr_bank=%u next_bank=%u BPT_EN_STATUS=0x%x stream_state=%d\n",
+		bus->params.curr_bank, bus->params.next_bank,
+		readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
+		bus->bpt_stream->state);
+
+	/* Poll DMA_BUSY until transfer completes */
+	{
+		unsigned long timeout;
+
+		timeout = jiffies + msecs_to_jiffies(BRA_DMA_TIMEOUT_MS);
+		do {
+			val = readl(amd_manager->mmio + ACP_SW_BRA_DMA_BUSY);
+			/*
+			 * Side-effectful read: reading ACP_SW_BRA_CURRENT_TRANSFER_SIZE
+			 * advances the BRA DMA engine to the next frame.  The value is
+			 * intentionally discarded (hence the (void) cast); removing this
+			 * read stalls multi-frame transfers, which then time out.
+			 */
+			(void)readl(amd_manager->mmio + ACP_SW_BRA_CURRENT_TRANSFER_SIZE);
+			if (!(val & 0x01))
+				break;
+			if (time_after(jiffies, timeout)) {
+				/*
+				 * Re-sample the busy bit before declaring a
+				 * timeout. cond_resched() below can park this
+				 * thread for longer than BRA_DMA_TIMEOUT_MS, in
+				 * which window the DMA may have completed; without
+				 * this final read a finished transfer would be
+				 * reported as a false timeout.
+				 */
+				val = readl(amd_manager->mmio + ACP_SW_BRA_DMA_BUSY);
+				if (!(val & 0x01))
+					break;
+				dev_err(amd_manager->dev,
+					"BPT: DMA timeout: periph=0x%08x len=%u EN_STATUS=0x%x RESP=0x%x I2S_ERR=0x%08x\n",
+					reg_addr, len,
+					readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
+					readl(amd_manager->mmio + ACP_SW_BRA_RESP),
+					readl(amd_manager->acp_mmio + i2s_err_offset));
+				ret = -ETIMEDOUT;
+				break;
+			}
+			/*
+			 * Runs in process context. Yield the CPU so a
+			 * multi-frame transfer cannot busy-spin for up to
+			 * BRA_DMA_TIMEOUT_MS and trip the soft lockup
+			 * watchdog on non-preemptible kernels. cond_resched()
+			 * keeps the poll cadence tight -- reading
+			 * ACP_SW_BRA_CURRENT_TRANSFER_SIZE every iteration is
+			 * required to advance the DMA engine between frames,
+			 * so usleep_range() (which would space out the reads)
+			 * is deliberately not used here.
+			 */
+			cond_resched();
+		} while (1);
+	}
+
+	/* Check for I2S/BRA errors */
+	val = readl(amd_manager->acp_mmio + i2s_err_offset);
+	if (val & AMD_SDW_BRA_I2S_ERROR_MASK) {
+		dev_err(amd_manager->dev,
+			"BPT: BRA failed I2S_ERROR_REASON=0x%08x (NAK=%u Clash=%u HdrResp=%u FtrResp=%u CRC=%u DMA=%u Cmd=%u)\n",
+			val,
+			!!(val & BIT(18)), !!(val & BIT(19)),
+			!!(val & BIT(26)), !!(val & BIT(27)),
+			!!(val & BIT(28)), !!(val & BIT(29)), !!(val & BIT(30)));
+		writel(0, amd_manager->acp_mmio + i2s_err_offset);
+		if (!ret)
+			ret = -EIO;
+	} else if (!ret) {
+		dev_dbg(amd_manager->dev,
+			"BPT: DMA done: periph=0x%08x len=%u\n", reg_addr, len);
+	}
+
+	/*
+	 * On an aborted or timed-out transfer the ACP BPT DMA engine can still
+	 * be armed (PORT_EN=1, DMA_BUSY=1). sdw_disable_stream() below performs
+	 * a bank switch, which is the BPT DMA start trigger, so disarm the engine
+	 * first -- otherwise the bank switch could (re)start a DMA write into
+	 * dma_buf, which amd_sdw_bpt_wait() frees as soon as this transfer
+	 * returns. On the success path the DMA has already completed, so this
+	 * early clear is a no-op.
+	 */
+	if (ret < 0)
+		writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
+
+	/*
+	 * Disable the stream via framework (writes CHANNELEN=0 + bank switch).
+	 * This cleanly stops the port and keeps bank state consistent.
+	 * Propagate a disable failure: it leaves the stream in
+	 * SDW_STREAM_ENABLED, and sdw_enable_stream() returns 0 early for an
+	 * already-ENABLED stream without the bank switch that triggers the BRA
+	 * DMA, so the next section would be silently skipped rather than
+	 * transferred.  Report the failure to the caller so it stops instead of
+	 * chaining a section whose DMA never starts.
+	 */
+	ret_disable = sdw_disable_stream(bus->bpt_stream);
+	if (ret_disable < 0) {
+		dev_err(amd_manager->dev, "BPT: sdw_disable_stream failed: %d\n",
+			ret_disable);
+		if (!ret)
+			ret = ret_disable;
+	}
+
+	dev_dbg(amd_manager->dev,
+		"BPT: post-disable: curr_bank=%u next_bank=%u stream_state=%d\n",
+		bus->params.curr_bank, bus->params.next_bank,
+		bus->bpt_stream->state);
+
+	/*
+	 * Disarm the ACP BPT DMA engine and wait for the port to quiesce.
+	 * Like the manager enable/disable sequence (ACP_SW_EN paired with
+	 * ACP_SW_EN_STATUS), ACP_SW_BPT_PORT_EN_STATUS reflects the real port
+	 * state, so polling it here guarantees a non-contiguous transfer's next
+	 * section cannot re-arm PORT_EN before the current one has torn down.
+	 */
+	writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
+	ret_disable = readl_poll_timeout(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS,
+					 val, !val, ACP_DELAY_US, AMD_SDW_TIMEOUT);
+	if (ret_disable < 0) {
+		dev_err(amd_manager->dev, "BPT: PORT_EN disable timeout\n");
+		if (!ret)
+			ret = ret_disable;
+		*dma_unsafe = true;
+	}
+	writel(0, amd_manager->acp_mmio + i2s_err_offset);
+	writel(0, amd_manager->mmio + ACP_SW_ERROR_REASON1);
+	/*
+	 * Clearing the immediate-command response-valid status races with the
+	 * immediate-command path (amd_sdw_send_cmd_get_resp()), which polls and
+	 * clears the same ACP_SW_IMM_CMD_STS bit under bus->msg_lock. The BPT DMA
+	 * poll loop above drops all locks, so a concurrent slave enumeration or
+	 * register access (amd_sdw_work / IRQ thread / codec regmap) can be
+	 * mid-command here. Take msg_lock so this teardown clear cannot erase a
+	 * response the command path has not yet consumed, which would make that
+	 * command time out. This msg_lock nests inside acp_bra_lock and bpt_lock,
+	 * both already held by the enclosing amd_sdw_bpt_wait(), preserving the
+	 * bpt_lock -> acp_bra_lock -> msg_lock order. do_bank_switch() inside
+	 * sdw_enable_stream()/sdw_disable_stream() only takes msg_lock for
+	 * multi-link managers, so this single-link manager establishes that order
+	 * via the explicit msg_lock here and in the non-contiguous
+	 * sdw_nwrite_no_pm()/sdw_nread_no_pm() fallback.
+	 */
+	mutex_lock(&bus->msg_lock);
+	writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
+	mutex_unlock(&bus->msg_lock);
+
+restore_intr:
+	writel(saved_intr_mask, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
+	return ret;
+}
+
+/**
+ * amd_sdw_bra_transfer() - Execute a single-shot BRA DMA transfer
+ * @amd_manager: AMD SoundWire manager
+ * @slave: SoundWire slave device
+ * @reg_addr: Peripheral register start address
+ * @acp_base_addr: Pre-configured ACP system address for the DMA buffer
+ * @len: Total number of bytes to transfer
+ * @write: true for write, false for read
+ * @dma_unsafe: output flag, set to true when the BRA DMA engine could not be
+ *              confirmed stopped after the transfer; the caller must then leak
+ *              the DMA buffer instead of freeing it
+ *
+ * Programs the BRA descriptor once with the full transfer length and triggers
+ * a single DMA operation.  The ACP BRA hardware autonomously slices the
+ * buffer into BPF-sized BRA frames, auto-incrementing PERIPHERAL_FIRST_BYTE_ADDR
+ * and DMA_BASE_ADDRESS between frames.  The last frame may be shorter than BPF;
+ * the hardware handles partial final frames correctly.
+ *
+ * The poll loop in amd_sdw_execute_bra_transfer() reads
+ * ACP_SW_BRA_CURRENT_TRANSFER_SIZE in every iteration, which is required
+ * to advance the DMA engine between frames.
+ */
+static int amd_sdw_bra_transfer(struct amd_sdw_manager *amd_manager,
+				struct sdw_slave *slave, u32 reg_addr,
+				u32 acp_base_addr, size_t len, bool write,
+				bool *dma_unsafe)
+{
+	struct amd_bra_params params = {0};
+	int ret;
+
+	ret = amd_sdw_calculate_bra_params(amd_manager, &params,
+					   (u8)slave->dev_num);
+	if (ret < 0)
+		return ret;
+
+	params.peripheral_first_byte_addr = reg_addr;
+	params.dma_base_addr = acp_base_addr;
+	params.transfer_length = (u32)len;
+	params.write_mode = write;
+
+	ret = amd_sdw_config_bra_descriptor(amd_manager, &params);
+	if (ret < 0)
+		return ret;
+
+	ret = amd_sdw_execute_bra_transfer(amd_manager, slave, dma_unsafe);
+	/*
+	 * If the engine could not be confirmed stopped it may still be
+	 * bus-mastering through the BRA descriptor. Deconfiguring would zero
+	 * ACP_SW_BRA_BASE_ADDRESS and redirect that live DMA to physical
+	 * address 0, so leave the descriptor pointing at the (now leaked)
+	 * buffer and skip deconfigure in the wedged case.
+	 */
+	if (!*dma_unsafe)
+		amd_sdw_deconfig_bra_descriptor(amd_manager);
+
+	return ret;
+}
+
+/**
+ * amd_sdw_bpt_open_stream() - Allocate and prepare BPT stream
+ * @amd_manager: AMD SoundWire manager
+ * @slave: SoundWire slave device
+ * @msg: BPT message with transfer direction
+ *
+ * Allocates a SoundWire BPT stream and adds slave and master runtime
+ * entries so that transport column params are visible to the port ops
+ * callbacks. DP0 is programmed through the SoundWire stream framework in
+ * amd_sdw_bpt_wait() immediately before the DMA transfer.
+ */
+static int amd_sdw_bpt_open_stream(struct amd_sdw_manager *amd_manager,
+				   struct sdw_slave *slave,
+				   struct sdw_bpt_msg *msg)
+{
+	struct sdw_bus *bus = &amd_manager->bus;
+	struct sdw_stream_config sconfig = {0};
+	struct sdw_port_config pconfig = {0};
+	struct sdw_stream_runtime *stream;
+	int ret;
+
+	stream = sdw_alloc_stream("BPT", SDW_STREAM_BPT);
+	if (!stream)
+		return -ENOMEM;
+
+	/*
+	 * BPT has no PCM sample rate, but sdw_prepare_stream() still validates
+	 * the stream rate against the bus clock: _sdw_prepare_stream() rejects a
+	 * rate that does not evenly divide max_clk_freq ("Async mode not
+	 * supported"). Use the bus frame rate, which the driver derives as
+	 * curr_dr_freq / (rows * cols) and therefore divides the SoundWire clock
+	 * by construction; this passes on any valid clock instead of only when
+	 * max_clk_freq happens to be a multiple of 48 kHz. The DP0 sample interval
+	 * used for the transfer is computed from the BRA frame geometry in
+	 * amd_sdw_compute_params(), not from this rate.
+	 */
+	sconfig.frame_rate = bus->prop.default_frame_rate;
+	sconfig.ch_count = 1;
+	sconfig.bps = 8;
+	sconfig.direction = (msg->flags & SDW_MSG_FLAG_WRITE) ?
+			    SDW_DATA_DIR_TX : SDW_DATA_DIR_RX;
+	sconfig.type = SDW_STREAM_BPT;
+
+	pconfig.num = 0;
+	pconfig.ch_mask = BIT(0);
+
+	/*
+	 * Flag this BPT transfer as a firmware download that may run while
+	 * audio streams on this bus are allocated but idle. sdw_stream_add_slave()
+	 * allocates the master runtime, and sdw_master_rt_alloc() consults
+	 * bus->bpt_fw_download to permit that allocation even when idle audio
+	 * streams are still allocated. The peripheral does not start its own
+	 * audio stream until the download has completed, so no stream is made
+	 * active on the bus for the duration.
+	 */
+	WRITE_ONCE(bus->bpt_fw_download, true);
+
+	ret = sdw_stream_add_slave(slave, &sconfig, &pconfig, 1, stream);
+	if (ret < 0) {
+		dev_err(amd_manager->dev,
+			"add slave to BPT stream failed: %d\n", ret);
+		goto remove_rt;
+	}
+
+	ret = sdw_stream_add_master(bus, &sconfig, &pconfig, 1, stream);
+	if (ret < 0) {
+		dev_err(amd_manager->dev,
+			"add master to BPT stream failed: %d\n", ret);
+		goto remove_rt;
+	}
+
+	/*
+	 * Publish bus->bpt_stream only after the runtime is added and
+	 * bus->bpt_stream_refcount has been raised under bus_lock. The refcount
+	 * is raised by sdw_stream_add_slave() above (via sdw_master_rt_alloc());
+	 * sdw_stream_add_master() then reuses that runtime through
+	 * sdw_master_rt_find() without incrementing it again.
+	 * sdw_program_params() keys off bpt_stream; publishing it last keeps
+	 * it consistent with the refcount, so an audio path that observes
+	 * refcount == 0 under bus_lock also sees bpt_stream == NULL and
+	 * programs its own parameters instead of being skipped.
+	 */
+	WRITE_ONCE(bus->bpt_stream, stream);
+
+	return 0;
+
+remove_rt:
+	/*
+	 * sdw_stream_add_slave() allocates the master runtime and raises
+	 * bus->bpt_stream_refcount. If it or the following sdw_stream_add_master()
+	 * fails, sdw_stream_remove_slave() frees only the slave runtime and
+	 * sdw_release_stream() only frees the stream, leaving the master runtime on
+	 * bus->m_rt_list dangling at the freed stream with bpt_stream_refcount stuck
+	 * non-zero -- which rejects every future BPT transfer with -EBUSY. Mirror
+	 * the amd_sdw_bpt_close_stream() teardown and remove the slave then the
+	 * master runtime (dropping the refcount) before releasing the stream; both
+	 * removes are no-ops when nothing was allocated.
+	 */
+	WRITE_ONCE(bus->bpt_fw_download, false);
+	sdw_stream_remove_slave(slave, stream);
+	sdw_stream_remove_master(bus, stream);
+	sdw_release_stream(stream);
+	return ret;
+}
+
+/**
+ * amd_sdw_bpt_close_stream() - Deprepare and release BPT stream
+ * @amd_manager: AMD SoundWire manager
+ * @slave: SoundWire slave device
+ *
+ * Deprepares slave DP0 and removes the stream/master/slave entries.
+ * Hardware teardown (CHANNELEN=0, bank-switch, PORT_EN=0) was handled by
+ * execute_bra_transfer()'s cleanup path.
+ */
+static void amd_sdw_bpt_close_stream(struct amd_sdw_manager *amd_manager,
+				     struct sdw_slave *slave)
+{
+	struct sdw_bus *bus = &amd_manager->bus;
+	struct sdw_stream_runtime *stream = READ_ONCE(bus->bpt_stream);
+	int ret;
+
+	if (!stream)
+		return;
+
+	/*
+	 * Deprepare DP0 via SoundWire framework to leave the peripheral in a
+	 * clean state for subsequent audio streams. Only deprepare when the
+	 * stream actually reached a depreparable state: on early-failure paths
+	 * (e.g. DMA buffer alloc failed before sdw_prepare_stream()) the stream
+	 * is still SDW_STREAM_CONFIGURED and sdw_deprepare_stream() would reject
+	 * it with -EINVAL. In that case DP0 was never programmed, so there is
+	 * nothing to clean up.
+	 */
+	dev_dbg(amd_manager->dev,
+		"BPT: pre-deprepare: curr_bank=%u next_bank=%u stream_state=%d\n",
+		bus->params.curr_bank, bus->params.next_bank,
+		stream->state);
+
+	/*
+	 * A failed sdw_disable_stream() during the transfer can leave the
+	 * stream in SDW_STREAM_ENABLED. Disable it first so it reaches a
+	 * depreparable state and the peripheral DP0 is not left active.
+	 */
+	if (stream->state == SDW_STREAM_ENABLED) {
+		ret = sdw_disable_stream(stream);
+		if (ret < 0)
+			dev_err(amd_manager->dev,
+				"BPT: sdw_disable_stream (cleanup) failed: %d\n", ret);
+	}
+
+	if (stream->state == SDW_STREAM_PREPARED ||
+	    stream->state == SDW_STREAM_DISABLED) {
+		ret = sdw_deprepare_stream(stream);
+		if (ret < 0)
+			dev_err(amd_manager->dev,
+				"BPT: sdw_deprepare_stream failed: %d\n", ret);
+		else
+			dev_dbg(amd_manager->dev,
+				"BPT: deprepared: curr_bank=%u next_bank=%u stream_state=%d\n",
+				bus->params.curr_bank, bus->params.next_bank,
+				stream->state);
+	}
+
+	if (stream->state == SDW_STREAM_ENABLED)
+		dev_warn(amd_manager->dev,
+			 "BPT: stream still ENABLED after cleanup; DP0 may remain active, peripheral may need re-enumeration\n");
+
+	/*
+	 * Clear bus->bpt_stream while bpt_stream_refcount is still raised so
+	 * the two remain consistent for lockless observers.
+	 * sdw_stream_remove_master() drops the refcount under bus_lock, so
+	 * clearing bpt_stream first means any audio path that observes
+	 * refcount == 0 also sees bpt_stream == NULL and programs its own
+	 * parameters rather than skipping them in sdw_program_params().
+	 */
+	WRITE_ONCE(bus->bpt_stream, NULL);
+	WRITE_ONCE(bus->bpt_fw_download, false);
+
+	ret = sdw_stream_remove_slave(slave, stream);
+	if (ret < 0)
+		dev_err(amd_manager->dev,
+			"remove slave from BPT stream failed: %d\n", ret);
+
+	ret = sdw_stream_remove_master(bus, stream);
+	if (ret < 0)
+		dev_err(amd_manager->dev,
+			"remove master from BPT stream failed: %d\n", ret);
+
+	sdw_release_stream(stream);
+}
+
+/**
+ * amd_sdw_bpt_send_async() - Validate a BPT message before transfer
+ * @bus: SoundWire bus
+ * @slave: SoundWire slave device
+ * @msg: BPT message with transfer sections
+ *
+ * For AMD the BRA engine transfer is synchronous, so the entire transfer
+ * (stream open/prepare, DMA, poll and teardown) is performed in
+ * amd_sdw_bpt_wait().  This callback only validates the message so a bad
+ * request fails fast.  Deliberately, no lock, runtime-PM reference or
+ * stream is taken here: nothing is held across the send_async()/wait()
+ * boundary, so a caller that never reaches bpt_wait() cannot leak the BPT
+ * lock or a runtime-PM reference.
+ */
+static int amd_sdw_bpt_send_async(struct sdw_bus *bus,
+				  struct sdw_slave *slave,
+				  struct sdw_bpt_msg *msg)
+{
+	struct amd_sdw_manager *amd_manager = to_amd_sdw(bus);
+	size_t total_len = 0;
+	int i;
+
+	for (i = 0; i < msg->sections; i++) {
+		if (msg->sec[i].len > SDW_BPT_MSG_MAX_BYTES - total_len) {
+			dev_err(amd_manager->dev,
+				"BPT msg length exceeds maximum %d bytes\n",
+				SDW_BPT_MSG_MAX_BYTES);
+			return -EINVAL;
+		}
+		total_len += msg->sec[i].len;
+	}
+
+	if (total_len < AMD_BPT_MSG_BYTE_MIN) {
+		dev_err(amd_manager->dev,
+			"BPT msg length %zu < minimum %d bytes\n",
+			total_len, AMD_BPT_MSG_BYTE_MIN);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static bool amd_sdw_sections_are_contiguous(struct sdw_bpt_msg *msg)
+{
+	int i;
+
+	for (i = 1; i < msg->sections; i++) {
+		if (msg->sec[i].addr != msg->sec[i - 1].addr + msg->sec[i - 1].len)
+			return false;
+	}
+	return true;
+}
+
+/**
+ * amd_sdw_bpt_wait() - Execute a BPT transfer and release all resources
+ * @bus: SoundWire bus
+ * @slave: SoundWire slave device
+ * @msg: BPT message with transfer sections
+ *
+ * Performs the whole BPT transfer for AMD: it serialises against other BPT
+ * transfers, holds a runtime-PM reference, opens/prepares the BPT stream,
+ * allocates the DMA buffer, configures the PTE mapping, runs the BRA DMA
+ * transfer and finally tears everything down.  Acquiring and releasing the
+ * BPT lock and the runtime-PM reference within this single function
+ * guarantees they cannot leak across the send_async()/wait() boundary.
+ */
+static int amd_sdw_bpt_wait(struct sdw_bus *bus,
+			    struct sdw_slave *slave,
+			    struct sdw_bpt_msg *msg)
+{
+	struct amd_sdw_manager *amd_manager = to_amd_sdw(bus);
+	bool is_write = (msg->flags & SDW_MSG_FLAG_WRITE);
+	struct amd_bra_params prep_params = {0};
+	u32 acp_sys_addr;
+	dma_addr_t dma_addr = 0;
+	u8 *dma_buf = NULL;
+	size_t offset = 0;
+	size_t total_len = 0;
+	int ret = 0;
+	int i;
+	bool dma_unsafe = false;
+
+	for (i = 0; i < msg->sections; i++) {
+		if (msg->sec[i].len > SDW_BPT_MSG_MAX_BYTES - total_len) {
+			dev_err(amd_manager->dev,
+				"BPT msg length exceeds maximum %d bytes\n",
+				SDW_BPT_MSG_MAX_BYTES);
+			return -EINVAL;
+		}
+		total_len += msg->sec[i].len;
+	}
+
+	if (total_len < AMD_BPT_MSG_BYTE_MIN) {
+		dev_err(amd_manager->dev,
+			"BPT msg length %zu < minimum %d bytes\n",
+			total_len, AMD_BPT_MSG_BYTE_MIN);
+		return -EINVAL;
+	}
+
+	/*
+	 * Take the runtime-PM reference that keeps the bus clock alive before
+	 * acquiring bpt_lock, not after.  When the manager is runtime
+	 * suspended, pm_runtime_get_sync() runs amd_resume_runtime()
+	 * synchronously in this task, and that callback acquires bpt_lock to
+	 * clear bpt_disabled.  Taking the reference while already holding
+	 * bpt_lock would therefore deadlock against ourselves.
+	 */
+	ret = pm_runtime_get_sync(amd_manager->dev);
+	/*
+	 * -EACCES only occurs when runtime PM is disabled, which for this
+	 * device happens across the system suspend/resume window.  amd_suspend()
+	 * sets bpt_disabled before the clock is stopped and amd_resume_runtime()
+	 * clears it only once the clock is back, so the bpt_disabled check below
+	 * rejects the transfer with -ESHUTDOWN before any BRA access whenever the
+	 * clock could be down.  A merely runtime-suspended manager (clock stopped,
+	 * PM still enabled) is resumed by the get above and returns success, not
+	 * -EACCES; tolerating -EACCES therefore never drives the BRA engine on a
+	 * stopped clock.
+	 */
+	if (ret < 0 && ret != -EACCES) {
+		pm_runtime_put_noidle(amd_manager->dev);
+		dev_err(amd_manager->dev, "BPT: pm_runtime_get failed: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * The ACP BRA engine is a single shared resource.  Serialise all BPT
+	 * transfers so concurrent slave probes do not race over the hardware.
+	 */
+	mutex_lock(&amd_manager->bpt_lock);
+
+	/*
+	 * Refuse the transfer if the bus is being (or has been) clock-stopped
+	 * for system suspend.  Driving the BRA/command channel while the clock
+	 * is stopping wedges the channel and leaves the peripheral unable to
+	 * re-enumerate on resume; the codec retries once it re-attaches.
+	 */
+	if (amd_manager->bpt_disabled) {
+		mutex_unlock(&amd_manager->bpt_lock);
+		pm_runtime_mark_last_busy(amd_manager->dev);
+		pm_runtime_put_autosuspend(amd_manager->dev);
+		return -ESHUTDOWN;
+	}
+
+	/* BRA always uses all available data columns (1..NC-1). */
+	amd_manager->bra_hstart = 1;
+	amd_manager->bra_hstop  = bus->params.col - 1;
+
+	/*
+	 * Open the BPT stream so the framework stream state machine tracks the
+	 * transfer.  Slave DP0 is prepared below before bra_transfer().
+	 */
+	ret = amd_sdw_bpt_open_stream(amd_manager, slave, msg);
+	if (ret < 0)
+		goto close_stream;
+
+	/* Allocate single DMA buffer for entire BPT message */
+	dma_buf = dma_alloc_coherent(amd_manager->dev->parent, total_len,
+				     &dma_addr, GFP_KERNEL);
+	if (!dma_buf) {
+		ret = -ENOMEM;
+		goto close_stream;
+	}
+
+	/*
+	 * The ATU GRP_1 and scratch PTE registers programmed here are
+	 * ACP-global and shared by both SoundWire manager instances, as is the
+	 * BRA DMA engine that reads through them.  bpt_lock is per-manager and
+	 * does not serialise across instances, so hold the ACP-wide
+	 * acp_bra_lock across the whole configure -> transfer -> deconfigure
+	 * sequence to stop the other instance reprogramming the shared PTEs
+	 * mid-transfer.
+	 *
+	 * A dedicated lock (not acp_sdw_lock) is used so that a transfer, which
+	 * can last up to BRA_DMA_TIMEOUT_MS, does not block the brief
+	 * ACP_EXTERNAL_INTR_CNTL updates done under acp_sdw_lock by
+	 * amd_enable_sdw_interrupts()/amd_disable_sdw_interrupts() and the PDM
+	 * interrupt helpers, which touch a different ACP-global register.
+	 */
+	mutex_lock(amd_manager->acp_bra_lock);
+	acp_sys_addr = amd_sdw_bra_configure_pte(amd_manager, dma_addr, total_len);
+	if (!acp_sys_addr) {
+		ret = -ENOMEM;
+		mutex_unlock(amd_manager->acp_bra_lock);
+		goto free_dma;
+	}
+
+	/* For writes, copy all section data into the DMA buffer */
+	if (is_write) {
+		for (i = 0; i < msg->sections; i++) {
+			memcpy(dma_buf + offset, msg->sec[i].buf, msg->sec[i].len);
+			offset += msg->sec[i].len;
+		}
+	}
+
+	dev_dbg(amd_manager->dev,
+		"BPT %s start: dev_num=%d sections=%d total_len=%zu dma_addr=0x%llx\n",
+		is_write ? "write" : "read",
+		msg->dev_num, msg->sections, total_len, (unsigned long long)dma_addr);
+
+	/*
+	 * Calculate BRA parameters for this device. We use bytes_per_frame to
+	 * decide whether a section is too small for BRA DMA and must fallback
+	 * to register read/write commands.
+	 */
+	ret = amd_sdw_calculate_bra_params(amd_manager, &prep_params,
+					   (u8)slave->dev_num);
+	if (ret < 0) {
+		dev_err(amd_manager->dev,
+			"BPT: failed to calc params: %d\n", ret);
+		goto deconfigure_pte;
+	}
+
+	/*
+	 * Prepare DP0 via SoundWire framework so the core programs the
+	 * peripheral DP0 transport/port registers and issues PREPARECTRL.
+	 * This is invoked from the BPT transfer context (firmware callback)
+	 * and not from update_status(), so it is safe w.r.t. sdw_dev_lock.
+	 */
+	ret = sdw_prepare_stream(bus->bpt_stream);
+	if (ret < 0) {
+		dev_err(amd_manager->dev,
+			"BPT: sdw_prepare_stream failed: %d\n", ret);
+		goto deconfigure_pte;
+	}
+	dev_dbg(amd_manager->dev,
+		"BPT: stream prepared, curr_bank=%u next_bank=%u state=%d\n",
+		bus->params.curr_bank, bus->params.next_bank,
+		bus->bpt_stream->state);
+
+	if (amd_sdw_sections_are_contiguous(msg)) {
+		/*
+		 * All sections are contiguous in peripheral address space, so a
+		 * single BRA call can cover the entire firmware image -- unless the
+		 * whole image is smaller than one BRA frame.  The DMA engine never
+		 * starts for a sub-frame payload, so fall back to regular SDW
+		 * register commands in that case, mirroring the per-section
+		 * fallback used for non-contiguous transfers below.
+		 */
+		if (total_len < prep_params.bytes_per_frame) {
+			if (is_write)
+				ret = sdw_nwrite_no_pm(slave, msg->sec[0].addr,
+						       total_len, dma_buf);
+			else
+				ret = sdw_nread_no_pm(slave, msg->sec[0].addr,
+						      total_len, dma_buf);
+		} else {
+			ret = amd_sdw_bra_transfer(amd_manager, slave,
+						   msg->sec[0].addr,
+						   acp_sys_addr,
+						   total_len, is_write,
+						   &dma_unsafe);
+		}
+		if (ret < 0) {
+			dev_err(amd_manager->dev,
+				"BPT contiguous transfer failed: addr=0x%x len=%zu ret=%d\n",
+				msg->sec[0].addr, total_len, ret);
+			/*
+			 * Skip the read-back copy below so a failed read
+			 * cannot return stale DMA buffer contents to the
+			 * caller as if the transfer had succeeded.
+			 */
+			goto deconfigure_pte;
+		}
+	} else {
+		/*
+		 * Non-contiguous sections: each section targets a different
+		 * peripheral address range.  The ACP BRA DMA engine is
+		 * triggered by sdw_enable_stream() (bank switch + CHANNELEN), so
+		 * each section needs its own full config -> activate ->
+		 * run_dma -> deactivate -> deconfig cycle.
+		 *
+		 * Sections smaller than one BRA frame (bytes_per_frame)
+		 * cannot be transferred via DMA because the engine never
+		 * starts for sub-frame payloads.  Use regular SDW register
+		 * read/write commands for those tiny sections instead.
+		 */
+		offset = 0;
+		for (i = 0; i < msg->sections; i++) {
+			if (i < 3 || i == msg->sections - 1)
+				dev_dbg(amd_manager->dev,
+					"BPT nc sec[%d/%d]: periph=0x%08x len=%u acp=0x%08x\n",
+					i, msg->sections, msg->sec[i].addr,
+					msg->sec[i].len,
+					acp_sys_addr + (u32)offset);
+			if (msg->sec[i].len < prep_params.bytes_per_frame) {
+				/*
+				 * Section too small for BRA DMA -- use
+				 * regular SDW byte-level commands instead.
+				 */
+				if (is_write)
+					ret = sdw_nwrite_no_pm(slave,
+							       msg->sec[i].addr,
+							       msg->sec[i].len,
+							       dma_buf + offset);
+				else
+					ret = sdw_nread_no_pm(slave,
+							      msg->sec[i].addr,
+							      msg->sec[i].len,
+							      dma_buf + offset);
+				if (ret < 0)
+					dev_err(amd_manager->dev,
+						"BPT reg %s failed: sec=%d/%d addr=0x%x len=%u ret=%d\n",
+						is_write ? "write" : "read",
+						i, msg->sections,
+						msg->sec[i].addr,
+						msg->sec[i].len, ret);
+				else
+					dev_dbg(amd_manager->dev,
+						"BPT sec[%d/%d]: used reg %s for %u bytes at 0x%08x\n",
+						i, msg->sections,
+						is_write ? "write" : "read",
+						msg->sec[i].len,
+						msg->sec[i].addr);
+			} else {
+				ret = amd_sdw_bra_transfer(amd_manager, slave,
+							   msg->sec[i].addr,
+							   acp_sys_addr + (u32)offset,
+							   msg->sec[i].len, is_write,
+							   &dma_unsafe);
+				if (ret < 0)
+					dev_err(amd_manager->dev,
+						"BPT failed: sec=%d/%d addr=0x%x len=%u ret=%d\n",
+						i, msg->sections, msg->sec[i].addr,
+						msg->sec[i].len, ret);
+			}
+			if (ret < 0)
+				break;
+			offset += msg->sec[i].len;
+		}
+		if (ret < 0)
+			goto deconfigure_pte;
+	}
+
+	offset = 0;
+	/* For reads, copy all section data out of the DMA buffer */
+	if (!is_write) {
+		for (i = 0; i < msg->sections; i++) {
+			memcpy(msg->sec[i].buf, dma_buf + offset, msg->sec[i].len);
+			offset += msg->sec[i].len;
+		}
+	}
+
+deconfigure_pte:
+	if (dma_unsafe) {
+		/*
+		 * PORT_EN_STATUS never cleared: the ACP BPT DMA engine did not
+		 * confirm it stopped and may still be bus-mastering through the
+		 * ATU into dma_buf.  PORT_EN=0 is the only stop control the
+		 * hardware exposes -- the audio DMA path relies on the same
+		 * enable-clear-then-poll-status primitive -- so there is no
+		 * stronger barrier to force it off here.  Leave the ATU PTEs
+		 * pointing at dma_buf and do NOT free the buffer: returning
+		 * these pages to the allocator while a wedged engine can still
+		 * write would corrupt memory reused for another purpose.
+		 * Deliberately leak the buffer instead -- the only memory-safe
+		 * option, matching the guarantee the managed-buffer audio path
+		 * gets from the PCM core (buffer never reused while the engine
+		 * may be active).
+		 */
+		dev_err(amd_manager->dev,
+			"BPT: engine did not quiesce; leaking %zu bytes to avoid corruption\n",
+			total_len);
+		/*
+		 * Latch the wedged state under acp_bra_lock before dropping it,
+		 * so neither instance can reprogram the shared ATU PTEs while this
+		 * engine may still be mastering into the leaked buffer.
+		 */
+		*amd_manager->acp_bra_wedged = true;
+		mutex_unlock(amd_manager->acp_bra_lock);
+		goto close_stream;
+	}
+	amd_sdw_bra_deconfigure_pte(amd_manager, total_len);
+	mutex_unlock(amd_manager->acp_bra_lock);
+free_dma:
+	dma_free_coherent(amd_manager->dev->parent, total_len, dma_buf, dma_addr);
+close_stream:
+	amd_sdw_bpt_close_stream(amd_manager, slave);
+	/*
+	 * Release in the reverse of the acquire order documented at the top:
+	 * drop bpt_lock before releasing the runtime-PM reference (matching the
+	 * bpt_disabled early-exit path above), so the lock is never held across
+	 * the PM put.
+	 */
+	mutex_unlock(&amd_manager->bpt_lock);
+	pm_runtime_mark_last_busy(amd_manager->dev);
+	pm_runtime_put_autosuspend(amd_manager->dev);
+	return ret;
+}
+
 static int sdw_master_read_amd_prop(struct sdw_bus *bus)
 {
 	struct amd_sdw_manager *amd_manager = to_amd_sdw(bus);
@@ -761,6 +2015,20 @@ static const struct sdw_master_ops amd_sdw_ops = {
 	.read_prop = amd_prop_read,
 	.xfer_msg = amd_sdw_xfer_msg,
 	.read_ping_status = amd_sdw_read_ping_status,
+	.bpt_send_async = amd_sdw_bpt_send_async,
+	.bpt_wait = amd_sdw_bpt_wait,
+};
+
+/*
+ * BRA/BPT was validated only on ACP70 and later.  ACP63 uses a different
+ * BPT register layout (see the per-revision port/transport-params tables),
+ * so it must not advertise the BPT callbacks -- doing so would let a BPT
+ * transfer program the wrong registers and corrupt audio DMA state.
+ */
+static const struct sdw_master_ops amd_sdw_ops_no_bpt = {
+	.read_prop = amd_prop_read,
+	.xfer_msg = amd_sdw_xfer_msg,
+	.read_ping_status = amd_sdw_read_ping_status,
 };
 
 static int amd_sdw_hw_params(struct snd_pcm_substream *substream,
@@ -1012,6 +2280,16 @@ static void amd_sdw_irq_thread(struct work_struct *work)
 
 	dev_dbg(amd_manager->dev, "[SDW%d] SDW INT: 0to7=0x%x, 8to11=0x%x\n",
 		amd_manager->instance, status_change_0to7, status_change_8to11);
+
+	/*
+	 * Clear non-slave-status bits before processing.
+	 * Bit 18 (BRA DMA completion) and bit 17 (command response)
+	 * are informational -- not tied to slave state changes.
+	 * Leaving them set can confuse the slave status update path.
+	 */
+	status_change_8to11 &= ~(AMD_SDW_BRA_DMA_COMPLETION_STAT |
+				   AMD_SDW_CMD_RESP_INTR_STAT);
+
 	if (status_change_8to11 & AMD_SDW_WAKE_STAT_MASK)
 		return amd_sdw_process_wake_event(amd_manager);
 
@@ -1081,11 +2359,16 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)
 	amd_manager->mmio = amd_manager->acp_mmio +
 			    (amd_manager->instance * SDW_MANAGER_REG_OFFSET);
 	amd_manager->acp_sdw_lock = pdata->acp_sdw_lock;
+	amd_manager->acp_bra_lock = pdata->acp_bra_lock;
+	amd_manager->acp_bra_wedged = pdata->acp_bra_wedged;
 	amd_manager->acp_rev = pdata->acp_rev;
 	amd_manager->cols_index = sdw_find_col_index(AMD_SDW_DEFAULT_COLUMNS);
 	amd_manager->rows_index = sdw_find_row_index(AMD_SDW_DEFAULT_ROWS);
 	amd_manager->dev = dev;
-	amd_manager->bus.ops = &amd_sdw_ops;
+	if (amd_manager->acp_rev >= ACP70_PCI_REV_ID)
+		amd_manager->bus.ops = &amd_sdw_ops;
+	else
+		amd_manager->bus.ops = &amd_sdw_ops_no_bpt;
 	amd_manager->bus.port_ops = &amd_sdw_port_ops;
 	amd_manager->bus.compute_params = &amd_sdw_compute_params;
 	amd_manager->bus.clk_stop_timeout = 200;
@@ -1132,6 +2415,10 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)
 	prop = &amd_manager->bus.prop;
 	prop->mclk_freq = AMD_SDW_BUS_BASE_FREQ;
 
+	ret = devm_mutex_init(dev, &amd_manager->bpt_lock);
+	if (ret)
+		return ret;
+
 	ret = sdw_bus_master_add(&amd_manager->bus, dev, dev->fwnode);
 	if (ret) {
 		dev_err(dev, "Failed to register SoundWire manager(%d)\n", ret);
@@ -1154,9 +2441,32 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
 	struct amd_sdw_manager *amd_manager = dev_get_drvdata(&pdev->dev);
 	int ret;
 
+	/*
+	 * A BPT firmware transfer runs in the codec's firmware-download
+	 * context (amd_sdw_bpt_wait()) and holds bpt_lock for its entire
+	 * duration.  Latch bpt_disabled under the lock (mirroring amd_suspend())
+	 * before tearing anything down: draining alone is not enough, because as
+	 * soon as bpt_lock is released a task already blocked on it could start a
+	 * new transfer that then races sdw_bus_master_delete() into a
+	 * use-after-free.  Setting bpt_disabled makes any such transfer bail out
+	 * with -ESHUTDOWN instead, and the drain below waits for a transfer that
+	 * is already in flight to finish, so sdw_bus_master_delete() cannot free
+	 * bus structures still in use by amd_sdw_bpt_wait().
+	 */
+	mutex_lock(&amd_manager->bpt_lock);
+	amd_manager->bpt_disabled = true;
+	mutex_unlock(&amd_manager->bpt_lock);
+
 	pm_runtime_disable(&pdev->dev);
-	cancel_work_sync(&amd_manager->amd_sdw_work);
+	/*
+	 * Disable interrupts first so the ACP ISR can no longer schedule
+	 * amd_sdw_irq_thread, then drain the IRQ bottom-half before the
+	 * slave-status work: amd_sdw_irq_thread may schedule amd_sdw_work, so
+	 * it must be cancelled first (mirrors the suspend path ordering).
+	 */
 	amd_disable_sdw_interrupts(amd_manager);
+	cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
+	cancel_work_sync(&amd_manager->amd_sdw_work);
 	sdw_bus_master_delete(&amd_manager->bus);
 	ret = amd_disable_sdw_manager(amd_manager);
 	if (ret)
@@ -1290,6 +2600,22 @@ static int __maybe_unused amd_suspend(struct device *dev)
 		return 0;
 	}
 
+	/*
+	 * Close the window between a BPT transfer and the clock stop below.
+	 * A codec's async firmware download runs in a workqueue
+	 * (amd_sdw_bpt_wait()) and its runtime-PM reference does not block
+	 * system-suspend clock stop.  Taking bpt_lock drains any transfer
+	 * already in progress (it completes on the still-live bus), and
+	 * setting bpt_disabled under the lock makes every subsequent transfer
+	 * bail with -ESHUTDOWN instead of racing amd_sdw_clock_stop().  A
+	 * plain drain is not enough: a re-enumeration-triggered download can
+	 * start a new BPT after the drain but before the clock stop.
+	 * amd_resume_runtime() clears the flag when the bus comes back.
+	 */
+	mutex_lock(&amd_manager->bpt_lock);
+	amd_manager->bpt_disabled = true;
+	mutex_unlock(&amd_manager->bpt_lock);
+
 	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
 		cancel_work_sync(&amd_manager->amd_sdw_work);
 		amd_sdw_wake_enable(amd_manager, false);
@@ -1340,6 +2666,26 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 			bus->link_id);
 		return 0;
 	}
+	/*
+	 * A BPT (firmware) transfer holds bpt_lock for its whole duration and
+	 * keeps a runtime-PM reference across it (pm_runtime_get_sync() in
+	 * amd_sdw_bpt_wait() until the matching put), so usage_count stays > 0
+	 * and this callback cannot be entered while a transfer is in flight; the
+	 * trylock below is a belt-and-braces guard for that invariant.
+	 *
+	 * Unlike the system-suspend path, runtime PM is still enabled here, so
+	 * there is no need to latch bpt_disabled to close the window between this
+	 * unlock and amd_sdw_clock_stop(): a BPT that starts in that window calls
+	 * pm_runtime_get_sync() first, and because the device is RPM_SUSPENDING
+	 * the PM core makes that get wait for this suspend to complete and then
+	 * resume the manager - restoring the clock - before it returns, so the
+	 * transfer never drives the BRA engine on a stopped clock. (The
+	 * system-suspend path must latch bpt_disabled because there runtime PM is
+	 * disabled and pm_runtime_get_sync() returns -EACCES instead of resuming.)
+	 */
+	if (!mutex_trylock(&amd_manager->bpt_lock))
+		return -EBUSY;
+	mutex_unlock(&amd_manager->bpt_lock);
 	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
 		amd_sdw_wake_enable(amd_manager, true);
 		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
@@ -1397,6 +2743,15 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 		ret = amd_sdw_clock_stop_exit(amd_manager);
 		if (ret)
 			return ret;
+		/*
+		 * The bus clock is live again as soon as clock_stop_exit()
+		 * succeeds, so re-allow BPT here.  Doing it before the host-wake
+		 * step below means a spurious host_wake_enable() error cannot
+		 * leave BPT wedged at -ESHUTDOWN on an otherwise running clock.
+		 */
+		mutex_lock(&amd_manager->bpt_lock);
+		amd_manager->bpt_disabled = false;
+		mutex_unlock(&amd_manager->bpt_lock);
 		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
 			ret = amd_sdw_host_wake_enable(amd_manager, false);
 			if (ret)
@@ -1432,11 +2787,36 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 			return ret;
 		amd_sdw_set_frameshape(amd_manager);
 	}
+
+	/*
+	 * Re-allow BPT transfers now that the bus clock is restored.  amd_suspend()
+	 * latches bpt_disabled under bpt_lock before stopping the clock; this
+	 * callback (which serves both runtime and system resume) clears it once the
+	 * clock-restore steps above have succeeded, before the BPT-independent
+	 * set_device_state() call below.  A BPT that tolerated a
+	 * pm_runtime_get_sync() -EACCES during the system-resume window still sees
+	 * bpt_disabled and bails out with -ESHUTDOWN instead of driving the BRA
+	 * engine on a not-yet-running clock.
+	 *
+	 * These paths differ in when the flag is cleared.  POWER_OFF_MODE clears it
+	 * only here, so a clock-restore step that fails returns above with
+	 * bpt_disabled still set and a failed resume correctly leaves BPT blocked.
+	 * CLK_STOP_MODE instead clears it right after clock_stop_exit() succeeds --
+	 * the clock is already live there -- so a subsequent host_wake_enable()
+	 * failure returns with BPT already re-allowed, which is intentional: blocking
+	 * BPT on a running clock would be needlessly conservative.  For the same
+	 * reason a set_device_state() failure below also leaves BPT enabled.
+	 */
+	mutex_lock(&amd_manager->bpt_lock);
+	amd_manager->bpt_disabled = false;
+	mutex_unlock(&amd_manager->bpt_lock);
+
 	if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
 		ret = amd_sdw_set_device_state(amd_manager, AMD_SDW_DEVICE_STATE_D0);
 		if (ret)
 			return ret;
 	}
+
 	return 0;
 }
 
diff --git a/drivers/soundwire/amd_manager.h b/drivers/soundwire/amd_manager.h
index 88cf8a426a0c..1e355a597520 100644
--- a/drivers/soundwire/amd_manager.h
+++ b/drivers/soundwire/amd_manager.h
@@ -96,6 +96,17 @@
 #define ACP_SW_CLK_RESUME_DELAY_CNTR			0x0003184
 #define ACP_SW_BUS_RESET_CTRL				0x0003188
 #define ACP_SW_PRBS_ERR_STATUS				0x000318c
+#define ACP_SW_ERROR_REASON1					0x00031cc
+/*
+ * ACP_SW_I2S_ERROR_REASON is in the codec/I2S address space (not the SDW
+ * controller space) and must be accessed via acp_mmio (not mmio).
+ * Instance 0 and instance 1 have separate registers.
+ * bit 18: NAK response  bit 19: bus clash
+ * bits 26-30: BRA errors (header/footer/CRC/DMA/command response)
+ */
+#define ACP_SW_I2S_ERROR_REASON				0x000018b4
+#define ACP_P1_SW_I2S_ERROR_REASON			0x00001a50
+#define AMD_SDW_BRA_I2S_ERROR_MASK			0x7ffc0000
 #define ACP_SW_IMM_CMD_UPPER_WORD			0x0003230
 #define ACP_SW_IMM_CMD_LOWER_QWORD			0x0003234
 #define ACP_SW_IMM_RESP_UPPER_WORD			0x0003238
@@ -105,7 +116,6 @@
 #define ACP_SW_BRA_TRANSFER_SIZE			0x0003248
 #define ACP_SW_BRA_DMA_BUSY				0x000324c
 #define ACP_SW_BRA_RESP					0x0003250
-#define ACP_SW_BRA_RESP_FRAME_ADDR			0x0003254
 #define ACP_SW_BRA_CURRENT_TRANSFER_SIZE		0x0003258
 #define ACP_SW_STATE_CHANGE_STATUS_0TO7			0x000325c
 #define ACP_SW_STATE_CHANGE_STATUS_8TO11		0x0003260
@@ -117,6 +127,7 @@
 
 #define ACP_DELAY_US					10
 #define AMD_SDW_TIMEOUT					1000
+#define BRA_DMA_TIMEOUT_MS				1000
 #define AMD_SDW_DEFAULT_CLK_FREQ			12000000
 
 #define AMD_SDW_MCP_RESP_ACK				BIT(0)
@@ -154,6 +165,8 @@
 #define AMD_SDW_IRQ_MASK_0TO7		0x77777777
 #define AMD_SDW_IRQ_MASK_8TO11		0x000c7777
 #define AMD_SDW_IRQ_ERROR_MASK		0xff
+/* BRA DMA error interrupt bits [5:7] in ACP_SW_ERROR_INTR_MASK */
+#define AMD_SDW_BPT_ERR_INTR_MASK	(BIT(5) | BIT(6) | BIT(7))
 #define AMD_SDW_MAX_FREQ_NUM		1
 #define AMD_ACP63_SDW0_MAX_TX_PORTS		3
 #define AMD_ACP63_SDW0_MAX_RX_PORTS		3
@@ -189,6 +202,8 @@
 #define AMD_SDW0_PAD_KEEPER_DISABLE_MASK		0x1e
 #define AMD_SDW1_PAD_KEEPER_DISABLE_MASK		0xf
 #define AMD_SDW_PREQ_INTR_STAT				BIT(19)
+#define AMD_SDW_BRA_DMA_COMPLETION_STAT			BIT(18)
+#define AMD_SDW_CMD_RESP_INTR_STAT			BIT(17)
 #define AMD_SDW_CLK_STOP_DONE				1
 #define AMD_SDW_CLK_RESUME_REQ				2
 #define AMD_SDW_CLK_RESUME_DONE				3
diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
index 470360a2723c..993984cb4a22 100644
--- a/include/linux/soundwire/sdw_amd.h
+++ b/include/linux/soundwire/sdw_amd.h
@@ -37,6 +37,10 @@ struct acp_sdw_pdata {
 	u32 acp_rev;
 	/* mutex to protect acp common register access */
 	struct mutex *acp_sdw_lock;
+	/* mutex to protect the SoundWire BRA transfer vs the other manager instance */
+	struct mutex *acp_bra_lock;
+	/* latched when a BRA transfer leaves the ACP DMA engine wedged */
+	bool *acp_bra_wedged;
 };
 
 /**
@@ -63,6 +67,7 @@ struct sdw_amd_dai_runtime {
  * @amd_sdw_irq_thread: SoundWire manager irq workqueue
  * @amd_sdw_work: peripheral status work queue
  * @acp_sdw_lock: mutex to protect acp share register access
+ * @acp_bra_lock: mutex to protect the SoundWire BRA transfer vs the other manager instance
  * @status: peripheral devices status array
  * @num_din_ports: number of input ports
  * @num_dout_ports: number of output ports
@@ -89,6 +94,10 @@ struct amd_sdw_manager {
 	struct work_struct amd_sdw_work;
 	/* mutex to protect acp common register access */
 	struct mutex *acp_sdw_lock;
+	/* mutex to protect the SoundWire BRA transfer vs the other manager instance */
+	struct mutex *acp_bra_lock;
+	/* latched when a BRA transfer leaves the ACP DMA engine wedged */
+	bool *acp_bra_wedged;
 
 	enum sdw_slave_status status[SDW_MAX_DEVICES + 1];
 
@@ -108,6 +117,27 @@ struct amd_sdw_manager {
 	bool clk_stopped;
 
 	struct sdw_amd_dai_runtime **dai_runtime_array;
+
+	/* Transport columns allocated for the active BPT transfer */
+	u8 bra_hstart;
+	u8 bra_hstop;
+
+	/*
+	 * Serialises concurrent BPT requests from multiple slaves on the
+	 * same bus.  Acquired at the start of amd_sdw_bpt_wait() and released
+	 * before it returns, so that only one slave can use the single ACP BRA
+	 * engine at a time.  amd_sdw_bpt_send_async() deliberately takes no
+	 * lock; the whole transfer is serialised in amd_sdw_bpt_wait().
+	 */
+	struct mutex bpt_lock;
+
+	/*
+	 * Set true while the bus clock is stopped for system suspend so
+	 * that amd_sdw_bpt_wait() refuses new BPT transfers: starting one
+	 * on a clock-stopped bus wedges the command channel and leaves the
+	 * peripheral unable to re-enumerate on resume.  Guarded by bpt_lock.
+	 */
+	bool bpt_disabled;
 };
 
 /**
@@ -153,6 +183,7 @@ struct sdw_amd_ctx {
  * @parent: parent device
  * @dev: device implementing hwparams and free callbacks
  * @acp_lock: mutex protecting acp common registers access
+ * @acp_bra_lock: mutex protecting the SoundWire BRA transfer vs the other manager instance
  */
 struct sdw_amd_res {
 	u32 acp_rev;
@@ -166,6 +197,10 @@ struct sdw_amd_res {
 	struct device *dev;
 	/* use to protect acp common registers access */
 	struct mutex *acp_lock;
+	/* use to protect the SoundWire BRA transfer vs the other manager instance */
+	struct mutex *acp_bra_lock;
+	/* latched when a BRA transfer leaves the ACP DMA engine wedged */
+	bool *acp_bra_wedged;
 };
 
 int sdw_amd_probe(struct sdw_amd_res *res, struct sdw_amd_ctx **ctx);
diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
index 62cb6bef17ab..5ebc32d91ace 100644
--- a/sound/soc/amd/ps/acp63.h
+++ b/sound/soc/amd/ps/acp63.h
@@ -322,6 +322,7 @@ struct acp_hw_ops {
  * sdw_dma_dev: platform device for SoundWire DMA controller
  * @mach_dev: platform device for machine driver to support ACP PDM/SoundWire configuration
  * @acp_lock: used to protect acp common registers
+ * @acp_bra_lock: protect the SoundWire BRA transfer against the other manager instance
  * @info: SoundWire AMD information found in ACPI tables
  * @sdw: SoundWire context for all SoundWire manager instances
  * @machine: ACPI machines for SoundWire interface
@@ -356,6 +357,8 @@ struct acp63_dev_data {
 	struct platform_device *sdw_dma_dev;
 	struct platform_device *mach_dev;
 	struct mutex acp_lock; /* protect shared registers */
+	struct mutex acp_bra_lock; /* protect BRA transfer vs the other manager instance */
+	bool acp_bra_wedged; /* latched when a BRA transfer leaves the DMA engine wedged */
 	struct sdw_amd_acpi_info info;
 	/* sdw context allocated by SoundWire driver */
 	struct sdw_amd_ctx *sdw;
diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
index 729f9aaba69e..530bea2f740c 100644
--- a/sound/soc/amd/ps/pci-ps.c
+++ b/sound/soc/amd/ps/pci-ps.c
@@ -287,6 +287,8 @@ static int amd_sdw_probe(struct device *dev)
 	sdw_res.parent = dev;
 	sdw_res.dev = dev;
 	sdw_res.acp_lock = &acp_data->acp_lock;
+	sdw_res.acp_bra_lock = &acp_data->acp_bra_lock;
+	sdw_res.acp_bra_wedged = &acp_data->acp_bra_wedged;
 	sdw_res.count = acp_data->info.count;
 	sdw_res.mmio_base = acp_data->acp63_base;
 	sdw_res.acp_rev = acp_data->acp_rev;
@@ -630,6 +632,7 @@ static int snd_acp63_probe(struct pci_dev *pci,
 	pci_set_master(pci);
 	pci_set_drvdata(pci, adata);
 	mutex_init(&adata->acp_lock);
+	mutex_init(&adata->acp_bra_lock);
 	ret = acp_hw_init_ops(adata, pci);
 	if (ret) {
 		dev_err(&pci->dev, "ACP hw ops init failed\n");
diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
index f89ad86260b4..134bab578f17 100644
--- a/sound/soc/sof/amd/acp.c
+++ b/sound/soc/sof/amd/acp.c
@@ -889,6 +889,8 @@ static int amd_sof_sdw_probe(struct snd_sof_dev *sdev)
 	sdw_res.parent = sdev->dev;
 	sdw_res.dev = sdev->dev;
 	sdw_res.acp_lock = &acp_data->acp_lock;
+	sdw_res.acp_bra_lock = &acp_data->acp_bra_lock;
+	sdw_res.acp_bra_wedged = &acp_data->acp_bra_wedged;
 	sdw_res.count = acp_data->info.count;
 	sdw_res.link_mask = acp_data->info.link_mask;
 	sdw_res.mmio_base = sdev->bar[ACP_DSP_BAR];
@@ -968,6 +970,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
 	adata->reg_range = chip->reg_end_addr - chip->reg_start_addr;
 	adata->pci_rev = pci->revision;
 	mutex_init(&adata->acp_lock);
+	mutex_init(&adata->acp_bra_lock);
 	sdev->pdata->hw_pdata = adata;
 
 	ret = acp_init(sdev);
diff --git a/sound/soc/sof/amd/acp.h b/sound/soc/sof/amd/acp.h
index 1cd9904c2908..093366c540bf 100644
--- a/sound/soc/sof/amd/acp.h
+++ b/sound/soc/sof/amd/acp.h
@@ -245,6 +245,10 @@ struct acp_dev_data {
 	struct platform_device *dmic_dev;
 	/* mutex lock to protect ACP common registers access */
 	struct mutex acp_lock;
+	/* protect the SoundWire BRA transfer vs the other manager instance */
+	struct mutex acp_bra_lock;
+	/* latched when a BRA transfer leaves the ACP DMA engine wedged */
+	bool acp_bra_wedged;
 	/* ACPI information stored between scan and probe steps */
 	struct sdw_amd_acpi_info info;
 	/* sdw context allocated by SoundWire driver */
-- 
2.43.0

Re: [PATCH v3 4/5] soundwire: amd: Add BRA/BPT firmware download support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soundwire: uninitialized pointer dereference of acp_bra_wedged
--

commit bce4ebb14d16cc86a99aa8e0be380048484eab35
Author: Syed Saba Kareem <Syed.SabaKareem@amd.com>

soundwire: amd: Add BRA/BPT firmware download support

This commit adds Bulk Register Access (BRA) and Bulk Payload Transport
(BPT) support for AMD SoundWire platforms. This enables high-speed
firmware download to SoundWire peripherals via DP0, using the ACP BRA
DMA engine.

> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
> index 8e419ddfa5161..00f4a87da7cbc 100644
> --- a/drivers/soundwire/amd_init.c
> +++ b/drivers/soundwire/amd_init.c
> @@ -120,6 +120,7 @@ static struct sdw_amd_ctx *sdw_amd_probe_controller(struct sdw_amd_res *res)
>  
>  		sdw_pdata[index].instance = index;
>  		sdw_pdata[index].acp_sdw_lock = res->acp_lock;
> +		sdw_pdata[index].acp_bra_lock = res->acp_bra_lock;
>  		sdw_pdata[index].acp_rev = res->acp_rev;
>  		pdevinfo[index].name = "amd_sdw_manager";
>  		pdevinfo[index].id = index;

[Severity: High]
Does this code miss initializing the acp_bra_wedged field in sdw_pdata?

Since sdw_pdata is allocated on the stack and not zero-initialized, this leaves
the acp_bra_wedged pointer containing stack garbage.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index a3316efdf8ac2..078e13b01ac97 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
[ ... ]
> +static u32 amd_sdw_bra_configure_pte(struct amd_sdw_manager *amd_manager,
> +				     dma_addr_t dma_addr, size_t size)
> +{
> +	u32 num_pages = (u32)(PAGE_ALIGN(size) >> PAGE_SHIFT);
> +	u32 low, high, val;
> +	u16 page_idx;
> +	dma_addr_t addr = dma_addr;
> +
> +	if (*amd_manager->acp_bra_wedged) {

[Severity: High]
Can this dereference an uninitialized pointer and cause a kernel oops or panic?

When a BRA firmware download transfer is requested via amd_sdw_bpt_wait(), it
calls amd_sdw_bra_configure_pte() where *amd_manager->acp_bra_wedged is
dereferenced. 

Because the field wasn't initialized in sdw_amd_probe_controller(), this will
dereference an invalid stack address.

> +		dev_err(amd_manager->dev,
> +			"BRA engine previously wedged; refusing to reprogram shared ATU PTEs\n");
> +		return 0;
> +	}
[ ... ]
> @@ -1081,11 +2359,16 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)
>  	amd_manager->mmio = amd_manager->acp_mmio +
>  			    (amd_manager->instance * SDW_MANAGER_REG_OFFSET);
>  	amd_manager->acp_sdw_lock = pdata->acp_sdw_lock;
> +	amd_manager->acp_bra_lock = pdata->acp_bra_lock;
> +	amd_manager->acp_bra_wedged = pdata->acp_bra_wedged;

[Severity: High]
This assigns the uninitialized stack garbage from pdata to
amd_manager->acp_bra_wedged. 

Should sdw_amd_probe_controller() be updated to properly initialize 
sdw_pdata[index].acp_bra_wedged before copying it here?

>  	amd_manager->acp_rev = pdata->acp_rev;
>  	amd_manager->cols_index = sdw_find_col_index(AMD_SDW_DEFAULT_COLUMNS);
>  	amd_manager->rows_index = sdw_find_row_index(AMD_SDW_DEFAULT_ROWS);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005091620.1390916-2-syed.sabakareem@amd.com?part=4

Re: [PATCH v3 4/5] soundwire: amd: Add BRA/BPT firmware download support

[Pierre-Louis Bossart]

general comment: this patch is quite complex, with code needed for the
BTP side as well as the DMA side, only interleaved. Is there a way to
have a 'cleaner' split between DMA functionality on the ACP side and BPT
on the SoundWire bus side?

Likewise the parts dealing with contiguous and non-contiguous parts of
the firmware should be handled at a higher level using DMA/BTP support
lower-level routines.

> +static int amd_sdw_execute_bra_transfer(struct amd_sdw_manager *amd_manager,
> +					struct sdw_slave *slave,
> +					bool *dma_unsafe)
> +{
> +	struct sdw_bus *bus = &amd_manager->bus;
> +	u32 i2s_err_offset;
> +	u32 saved_intr_mask;
> +	u32 reg_addr, len;
> +	u32 val;
> +	int ret, ret_disable;
> +
> +	/* Read descriptor regs before enabling the DMA engine. */
> +	reg_addr = readl(amd_manager->mmio + ACP_SW_BPT_PORT_FIRST_BYTE_ADDR);
> +	len = readl(amd_manager->mmio + ACP_SW_BRA_TRANSFER_SIZE);
> +
> +	i2s_err_offset = (amd_manager->instance == 0) ?
> +			 ACP_SW_I2S_ERROR_REASON : ACP_P1_SW_I2S_ERROR_REASON;
> +
> +	/*
> +	 * Save and disable the error interrupt mask for manual error
> +	 * checking.  acp_bra_lock is held across the whole BPT sequence by
> +	 * amd_sdw_bpt_wait(), which serialises this shared-register
> +	 * read-modify-write against the other manager instance.
> +	 */
> +	saved_intr_mask = readl(amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
> +	writel(0, amd_manager->acp_mmio + i2s_err_offset);
> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_REASON1);
> +
> +	/* Arm the ACP BPT DMA engine */
> +	writel(1, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
> +
> +	/*
> +	 * Use the framework's sdw_enable_stream() to write CHANNELEN and
> +	 * perform a bank switch.  The ACP BPT hardware uses the bank switch
> +	 * as the trigger to start the DMA transfer.  The framework manages

do you mean to say
a) the DMA transfer was armed and started ealier, and the bank switch
unblocks it, ob
b) the DMA starts fetching data from memory when the bank switch happens?

the latter case would be quite racy and dependent on the time needed to
access memory..

> +	 * bank state consistently, eliminating the need for a manual bank
> +	 * switch or DP0 bank mirror.
> +	 */
> +	dev_dbg(amd_manager->dev,
> +		"BPT: pre-enable: curr_bank=%u next_bank=%u BPT_EN_STATUS=0x%x stream_state=%d\n",
> +		bus->params.curr_bank, bus->params.next_bank,
> +		readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
> +		bus->bpt_stream->state);
> +
> +	ret = sdw_enable_stream(bus->bpt_stream);
> +	if (ret < 0) {
> +		dev_err(amd_manager->dev,
> +			"BPT: sdw_enable_stream failed: %d\n", ret);
> +		/*
> +		 * Disarm the engine and wait for the port to quiesce before
> +		 * returning: the caller (amd_sdw_bra_transfer()) then deconfigures
> +		 * the BRA descriptor unconditionally, zeroing BASE_ADDRESS and
> +		 * TRANSFER_SIZE, so the port must be idle first -- mirrors the
> +		 * disable path below.
> +		 */
> +		writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
> +		ret_disable = readl_poll_timeout(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS,
> +						 val, !val, ACP_DELAY_US, AMD_SDW_TIMEOUT);
> +		if (ret_disable < 0) {
> +			dev_err(amd_manager->dev, "BPT: PORT_EN disable timeout\n");
> +			*dma_unsafe = true;
> +		}
> +		goto restore_intr;
> +	}
> +
> +	dev_dbg(amd_manager->dev,
> +		"BPT: DMA started: curr_bank=%u next_bank=%u BPT_EN_STATUS=0x%x stream_state=%d\n",
> +		bus->params.curr_bank, bus->params.next_bank,
> +		readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
> +		bus->bpt_stream->state);
> +
> +	/* Poll DMA_BUSY until transfer completes */
> +	{
> +		unsigned long timeout;
> +
> +		timeout = jiffies + msecs_to_jiffies(BRA_DMA_TIMEOUT_MS);
> +		do {
> +			val = readl(amd_manager->mmio + ACP_SW_BRA_DMA_BUSY);
> +			/*
> +			 * Side-effectful read: reading ACP_SW_BRA_CURRENT_TRANSFER_SIZE
> +			 * advances the BRA DMA engine to the next frame.  The value is
> +			 * intentionally discarded (hence the (void) cast); removing this
> +			 * read stalls multi-frame transfers, which then time out.
> +			 */
> +			(void)readl(amd_manager->mmio + ACP_SW_BRA_CURRENT_TRANSFER_SIZE);
> +			if (!(val & 0x01))
> +				break;
> +			if (time_after(jiffies, timeout)) {
> +				/*
> +				 * Re-sample the busy bit before declaring a
> +				 * timeout. cond_resched() below can park this
> +				 * thread for longer than BRA_DMA_TIMEOUT_MS, in
> +				 * which window the DMA may have completed; without
> +				 * this final read a finished transfer would be
> +				 * reported as a false timeout.
> +				 */
> +				val = readl(amd_manager->mmio + ACP_SW_BRA_DMA_BUSY);
> +				if (!(val & 0x01))
> +					break;
> +				dev_err(amd_manager->dev,
> +					"BPT: DMA timeout: periph=0x%08x len=%u EN_STATUS=0x%x RESP=0x%x I2S_ERR=0x%08x\n",
> +					reg_addr, len,
> +					readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
> +					readl(amd_manager->mmio + ACP_SW_BRA_RESP),
> +					readl(amd_manager->acp_mmio + i2s_err_offset));
> +				ret = -ETIMEDOUT;
> +				break;
> +			}
> +			/*
> +			 * Runs in process context. Yield the CPU so a
> +			 * multi-frame transfer cannot busy-spin for up to
> +			 * BRA_DMA_TIMEOUT_MS and trip the soft lockup
> +			 * watchdog on non-preemptible kernels. cond_resched()
> +			 * keeps the poll cadence tight -- reading
> +			 * ACP_SW_BRA_CURRENT_TRANSFER_SIZE every iteration is
> +			 * required to advance the DMA engine between frames,
> +			 * so usleep_range() (which would space out the reads)
> +			 * is deliberately not used here.
> +			 */
> +			cond_resched();
> +		} while (1);
> +	}
> +
> +	/* Check for I2S/BRA errors */
> +	val = readl(amd_manager->acp_mmio + i2s_err_offset);
> +	if (val & AMD_SDW_BRA_I2S_ERROR_MASK) {
> +		dev_err(amd_manager->dev,
> +			"BPT: BRA failed I2S_ERROR_REASON=0x%08x (NAK=%u Clash=%u HdrResp=%u FtrResp=%u CRC=%u DMA=%u Cmd=%u)\n",
> +			val,
> +			!!(val & BIT(18)), !!(val & BIT(19)),
> +			!!(val & BIT(26)), !!(val & BIT(27)),
> +			!!(val & BIT(28)), !!(val & BIT(29)), !!(val & BIT(30)));
> +		writel(0, amd_manager->acp_mmio + i2s_err_offset);
> +		if (!ret)
> +			ret = -EIO;
> +	} else if (!ret) {
> +		dev_dbg(amd_manager->dev,
> +			"BPT: DMA done: periph=0x%08x len=%u\n", reg_addr, len);
> +	}
> +
> +	/*
> +	 * On an aborted or timed-out transfer the ACP BPT DMA engine can still
> +	 * be armed (PORT_EN=1, DMA_BUSY=1). sdw_disable_stream() below performs
> +	 * a bank switch, which is the BPT DMA start trigger, so disarm the engine
> +	 * first -- otherwise the bank switch could (re)start a DMA write into
> +	 * dma_buf, which amd_sdw_bpt_wait() frees as soon as this transfer
> +	 * returns. On the success path the DMA has already completed, so this
> +	 * early clear is a no-op.
> +	 */
> +	if (ret < 0)
> +		writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
> +
> +	/*
> +	 * Disable the stream via framework (writes CHANNELEN=0 + bank switch).
> +	 * This cleanly stops the port and keeps bank state consistent.
> +	 * Propagate a disable failure: it leaves the stream in
> +	 * SDW_STREAM_ENABLED, and sdw_enable_stream() returns 0 early for an
> +	 * already-ENABLED stream without the bank switch that triggers the BRA
> +	 * DMA, so the next section would be silently skipped rather than
> +	 * transferred.  Report the failure to the caller so it stops instead of
> +	 * chaining a section whose DMA never starts.
> +	 */
> +	ret_disable = sdw_disable_stream(bus->bpt_stream);
> +	if (ret_disable < 0) {
> +		dev_err(amd_manager->dev, "BPT: sdw_disable_stream failed: %d\n",
> +			ret_disable);
> +		if (!ret)
> +			ret = ret_disable;
> +	}
> +
> +	dev_dbg(amd_manager->dev,
> +		"BPT: post-disable: curr_bank=%u next_bank=%u stream_state=%d\n",
> +		bus->params.curr_bank, bus->params.next_bank,
> +		bus->bpt_stream->state);
> +
> +	/*
> +	 * Disarm the ACP BPT DMA engine and wait for the port to quiesce.
> +	 * Like the manager enable/disable sequence (ACP_SW_EN paired with
> +	 * ACP_SW_EN_STATUS), ACP_SW_BPT_PORT_EN_STATUS reflects the real port
> +	 * state, so polling it here guarantees a non-contiguous transfer's next
> +	 * section cannot re-arm PORT_EN before the current one has torn down.
> +	 */
> +	writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
> +	ret_disable = readl_poll_timeout(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS,
> +					 val, !val, ACP_DELAY_US, AMD_SDW_TIMEOUT);
> +	if (ret_disable < 0) {
> +		dev_err(amd_manager->dev, "BPT: PORT_EN disable timeout\n");
> +		if (!ret)
> +			ret = ret_disable;
> +		*dma_unsafe = true;
> +	}
> +	writel(0, amd_manager->acp_mmio + i2s_err_offset);
> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_REASON1);
> +	/*
> +	 * Clearing the immediate-command response-valid status races with the
> +	 * immediate-command path (amd_sdw_send_cmd_get_resp()), which polls and
> +	 * clears the same ACP_SW_IMM_CMD_STS bit under bus->msg_lock. The BPT DMA
> +	 * poll loop above drops all locks, so a concurrent slave enumeration or
> +	 * register access (amd_sdw_work / IRQ thread / codec regmap) can be
> +	 * mid-command here. Take msg_lock so this teardown clear cannot erase a
> +	 * response the command path has not yet consumed, which would make that
> +	 * command time out. This msg_lock nests inside acp_bra_lock and bpt_lock,
> +	 * both already held by the enclosing amd_sdw_bpt_wait(), preserving the
> +	 * bpt_lock -> acp_bra_lock -> msg_lock order. do_bank_switch() inside
> +	 * sdw_enable_stream()/sdw_disable_stream() only takes msg_lock for
> +	 * multi-link managers, so this single-link manager establishes that order
> +	 * via the explicit msg_lock here and in the non-contiguous
> +	 * sdw_nwrite_no_pm()/sdw_nread_no_pm() fallback.
> +	 */
> +	mutex_lock(&bus->msg_lock);
> +	writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
> +	mutex_unlock(&bus->msg_lock);
> +
> +restore_intr:
> +	writel(saved_intr_mask, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
> +	return ret;
> +}

[...]

> +static int amd_sdw_bpt_wait(struct sdw_bus *bus,
> +			    struct sdw_slave *slave,
> +			    struct sdw_bpt_msg *msg)
> +{
> +	struct amd_sdw_manager *amd_manager = to_amd_sdw(bus);
> +	bool is_write = (msg->flags & SDW_MSG_FLAG_WRITE);
> +	struct amd_bra_params prep_params = {0};
> +	u32 acp_sys_addr;
> +	dma_addr_t dma_addr = 0;
> +	u8 *dma_buf = NULL;
> +	size_t offset = 0;
> +	size_t total_len = 0;
> +	int ret = 0;
> +	int i;
> +	bool dma_unsafe = false;
> +
> +	for (i = 0; i < msg->sections; i++) {
> +		if (msg->sec[i].len > SDW_BPT_MSG_MAX_BYTES - total_len) {
> +			dev_err(amd_manager->dev,
> +				"BPT msg length exceeds maximum %d bytes\n",
> +				SDW_BPT_MSG_MAX_BYTES);
> +			return -EINVAL;
> +		}
> +		total_len += msg->sec[i].len;
> +	}
> +
> +	if (total_len < AMD_BPT_MSG_BYTE_MIN) {
> +		dev_err(amd_manager->dev,
> +			"BPT msg length %zu < minimum %d bytes\n",
> +			total_len, AMD_BPT_MSG_BYTE_MIN);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Take the runtime-PM reference that keeps the bus clock alive before
> +	 * acquiring bpt_lock, not after.  When the manager is runtime
> +	 * suspended, pm_runtime_get_sync() runs amd_resume_runtime()
> +	 * synchronously in this task, and that callback acquires bpt_lock to
> +	 * clear bpt_disabled.  Taking the reference while already holding
> +	 * bpt_lock would therefore deadlock against ourselves.
> +	 */
> +	ret = pm_runtime_get_sync(amd_manager->dev);
> +	/*
> +	 * -EACCES only occurs when runtime PM is disabled, which for this
> +	 * device happens across the system suspend/resume window.  amd_suspend()
> +	 * sets bpt_disabled before the clock is stopped and amd_resume_runtime()
> +	 * clears it only once the clock is back, so the bpt_disabled check below
> +	 * rejects the transfer with -ESHUTDOWN before any BRA access whenever the
> +	 * clock could be down.  A merely runtime-suspended manager (clock stopped,
> +	 * PM still enabled) is resumed by the get above and returns success, not
> +	 * -EACCES; tolerating -EACCES therefore never drives the BRA engine on a
> +	 * stopped clock.
> +	 */
> +	if (ret < 0 && ret != -EACCES) {
> +		pm_runtime_put_noidle(amd_manager->dev);
> +		dev_err(amd_manager->dev, "BPT: pm_runtime_get failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * The ACP BRA engine is a single shared resource.  Serialise all BPT
> +	 * transfers so concurrent slave probes do not race over the hardware.
> +	 */
> +	mutex_lock(&amd_manager->bpt_lock);
> +
> +	/*
> +	 * Refuse the transfer if the bus is being (or has been) clock-stopped
> +	 * for system suspend.  Driving the BRA/command channel while the clock
> +	 * is stopping wedges the channel and leaves the peripheral unable to
> +	 * re-enumerate on resume; the codec retries once it re-attaches.
> +	 */
> +	if (amd_manager->bpt_disabled) {
> +		mutex_unlock(&amd_manager->bpt_lock);
> +		pm_runtime_mark_last_busy(amd_manager->dev);
> +		pm_runtime_put_autosuspend(amd_manager->dev);
> +		return -ESHUTDOWN;
> +	}
> +
> +	/* BRA always uses all available data columns (1..NC-1). */
> +	amd_manager->bra_hstart = 1;
> +	amd_manager->bra_hstop  = bus->params.col - 1;
> +
> +	/*
> +	 * Open the BPT stream so the framework stream state machine tracks the
> +	 * transfer.  Slave DP0 is prepared below before bra_transfer().
> +	 */
> +	ret = amd_sdw_bpt_open_stream(amd_manager, slave, msg);
> +	if (ret < 0)
> +		goto close_stream;
> +
> +	/* Allocate single DMA buffer for entire BPT message */
> +	dma_buf = dma_alloc_coherent(amd_manager->dev->parent, total_len,
> +				     &dma_addr, GFP_KERNEL);
> +	if (!dma_buf) {
> +		ret = -ENOMEM;
> +		goto close_stream;
> +	}
> +
> +	/*
> +	 * The ATU GRP_1 and scratch PTE registers programmed here are
> +	 * ACP-global and shared by both SoundWire manager instances, as is the
> +	 * BRA DMA engine that reads through them.  bpt_lock is per-manager and
> +	 * does not serialise across instances, so hold the ACP-wide
> +	 * acp_bra_lock across the whole configure -> transfer -> deconfigure
> +	 * sequence to stop the other instance reprogramming the shared PTEs
> +	 * mid-transfer.

In that case, what is the point of having a per-instance btp_lock as well?

It seems from the comment that only *one* BTP transfer can take place
across all manager instances, which defeats the purpose of a
per-instance lock, no?
What I am missing?

Re: [PATCH v3 4/5] soundwire: amd: Add BRA/BPT firmware download support

[Syed Saba Kareem]

[-- Attachment #1: Type: text/plain, Size: 18075 bytes --]


On 10/5/26 17:34, Pierre-Louis Bossart wrote:
> general comment: this patch is quite complex, with code needed for the
> BTP side as well as the DMA side, only interleaved. Is there a way to
> have a 'cleaner' split between DMA functionality on the ACP side and BPT
> on the SoundWire bus side?
>
> Likewise the parts dealing with contiguous and non-contiguous parts of
> the firmware should be handled at a higher level using DMA/BTP support
> lower-level routines.

Agreed, I'll restructure this into three layers for v5: ACP BRA /DMA/ 
primitives (configure descriptor, arm, poll/wait, disarm, deconfigure) 
with no SoundWire-stream knowledge; SoundWire /BPT/ primitives 
(open/close stream, enable/disable);
and a higher-level orchestrator with a dispatcher for the contiguous 
(single-buffer) vs non-contiguous (per-section loop, with the 
small-section sdw_nwrite/nread fallback) cases.

One hardware detail I'll preserve and document along the way: the 
SoundWire bank switch is itself the BRA DMA start trigger,and correct 
teardown requires disarming the DMA before the disable-path bank switch.
So the orchestrator still sequences an ACP call and a SoundWire call in 
a fixed order — the layering makes each side independently readable and 
testable while keeping that ordering contract explicit.

>> +static int amd_sdw_execute_bra_transfer(struct amd_sdw_manager *amd_manager,
>> +					struct sdw_slave *slave,
>> +					bool *dma_unsafe)
>> +{
>> +	struct sdw_bus *bus = &amd_manager->bus;
>> +	u32 i2s_err_offset;
>> +	u32 saved_intr_mask;
>> +	u32 reg_addr, len;
>> +	u32 val;
>> +	int ret, ret_disable;
>> +
>> +	/* Read descriptor regs before enabling the DMA engine. */
>> +	reg_addr = readl(amd_manager->mmio + ACP_SW_BPT_PORT_FIRST_BYTE_ADDR);
>> +	len = readl(amd_manager->mmio + ACP_SW_BRA_TRANSFER_SIZE);
>> +
>> +	i2s_err_offset = (amd_manager->instance == 0) ?
>> +			 ACP_SW_I2S_ERROR_REASON : ACP_P1_SW_I2S_ERROR_REASON;
>> +
>> +	/*
>> +	 * Save and disable the error interrupt mask for manual error
>> +	 * checking.  acp_bra_lock is held across the whole BPT sequence by
>> +	 * amd_sdw_bpt_wait(), which serialises this shared-register
>> +	 * read-modify-write against the other manager instance.
>> +	 */
>> +	saved_intr_mask = readl(amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
>> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
>> +	writel(0, amd_manager->acp_mmio + i2s_err_offset);
>> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_REASON1);
>> +
>> +	/* Arm the ACP BPT DMA engine */
>> +	writel(1, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
>> +
>> +	/*
>> +	 * Use the framework's sdw_enable_stream() to write CHANNELEN and
>> +	 * perform a bank switch.  The ACP BPT hardware uses the bank switch
>> +	 * as the trigger to start the DMA transfer.  The framework manages
> do you mean to say
> a) the DMA transfer was armed and started ealier, and the bank switch
> unblocks it, ob
> b) the DMA starts fetching data from memory when the bank switch happens?
>
> the latter case would be quite racy and dependent on the time needed to
> access memory..
It's (a). The BRA descriptor (BASE_ADDRESS/TRANSFER_SIZE/FRAME_FORMAT) 
is fully programmed in amd_sdw_config_bra_descriptor(),
and the engine is armed with ACP_SW_BPT_PORT_EN=1, both before 
sdw_enable_stream().
The bank switch is only the trigger for the already-armed engine — it 
doesn't start a cold engine, so there's no memory-latency race on the 
switch.
Once triggered the engine runs autonomously frame by frame, and any 
stall/underrun surfaces as a BRA/I2S error (ACP_SW_I2S_ERROR_REASON / 
ACP_SW_BRA_RESP), not silent corruption.
This is also why, on an aborted or timed-out transfer, the teardown 
clears PORT_EN before the sdw_disable_stream() bank switch — otherwise 
that switch
could re-trigger the armed engine into a buffer we're about to free.
I'll reword the comment to say the engine is armed here and the bank 
switch only triggers it.
>> +	 * bank state consistently, eliminating the need for a manual bank
>> +	 * switch or DP0 bank mirror.
>> +	 */
>> +	dev_dbg(amd_manager->dev,
>> +		"BPT: pre-enable: curr_bank=%u next_bank=%u BPT_EN_STATUS=0x%x stream_state=%d\n",
>> +		bus->params.curr_bank, bus->params.next_bank,
>> +		readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
>> +		bus->bpt_stream->state);
>> +
>> +	ret = sdw_enable_stream(bus->bpt_stream);
>> +	if (ret < 0) {
>> +		dev_err(amd_manager->dev,
>> +			"BPT: sdw_enable_stream failed: %d\n", ret);
>> +		/*
>> +		 * Disarm the engine and wait for the port to quiesce before
>> +		 * returning: the caller (amd_sdw_bra_transfer()) then deconfigures
>> +		 * the BRA descriptor unconditionally, zeroing BASE_ADDRESS and
>> +		 * TRANSFER_SIZE, so the port must be idle first -- mirrors the
>> +		 * disable path below.
>> +		 */
>> +		writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
>> +		ret_disable = readl_poll_timeout(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS,
>> +						 val, !val, ACP_DELAY_US, AMD_SDW_TIMEOUT);
>> +		if (ret_disable < 0) {
>> +			dev_err(amd_manager->dev, "BPT: PORT_EN disable timeout\n");
>> +			*dma_unsafe = true;
>> +		}
>> +		goto restore_intr;
>> +	}
>> +
>> +	dev_dbg(amd_manager->dev,
>> +		"BPT: DMA started: curr_bank=%u next_bank=%u BPT_EN_STATUS=0x%x stream_state=%d\n",
>> +		bus->params.curr_bank, bus->params.next_bank,
>> +		readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
>> +		bus->bpt_stream->state);
>> +
>> +	/* Poll DMA_BUSY until transfer completes */
>> +	{
>> +		unsigned long timeout;
>> +
>> +		timeout = jiffies + msecs_to_jiffies(BRA_DMA_TIMEOUT_MS);
>> +		do {
>> +			val = readl(amd_manager->mmio + ACP_SW_BRA_DMA_BUSY);
>> +			/*
>> +			 * Side-effectful read: reading ACP_SW_BRA_CURRENT_TRANSFER_SIZE
>> +			 * advances the BRA DMA engine to the next frame.  The value is
>> +			 * intentionally discarded (hence the (void) cast); removing this
>> +			 * read stalls multi-frame transfers, which then time out.
>> +			 */
>> +			(void)readl(amd_manager->mmio + ACP_SW_BRA_CURRENT_TRANSFER_SIZE);
>> +			if (!(val & 0x01))
>> +				break;
>> +			if (time_after(jiffies, timeout)) {
>> +				/*
>> +				 * Re-sample the busy bit before declaring a
>> +				 * timeout. cond_resched() below can park this
>> +				 * thread for longer than BRA_DMA_TIMEOUT_MS, in
>> +				 * which window the DMA may have completed; without
>> +				 * this final read a finished transfer would be
>> +				 * reported as a false timeout.
>> +				 */
>> +				val = readl(amd_manager->mmio + ACP_SW_BRA_DMA_BUSY);
>> +				if (!(val & 0x01))
>> +					break;
>> +				dev_err(amd_manager->dev,
>> +					"BPT: DMA timeout: periph=0x%08x len=%u EN_STATUS=0x%x RESP=0x%x I2S_ERR=0x%08x\n",
>> +					reg_addr, len,
>> +					readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
>> +					readl(amd_manager->mmio + ACP_SW_BRA_RESP),
>> +					readl(amd_manager->acp_mmio + i2s_err_offset));
>> +				ret = -ETIMEDOUT;
>> +				break;
>> +			}
>> +			/*
>> +			 * Runs in process context. Yield the CPU so a
>> +			 * multi-frame transfer cannot busy-spin for up to
>> +			 * BRA_DMA_TIMEOUT_MS and trip the soft lockup
>> +			 * watchdog on non-preemptible kernels. cond_resched()
>> +			 * keeps the poll cadence tight -- reading
>> +			 * ACP_SW_BRA_CURRENT_TRANSFER_SIZE every iteration is
>> +			 * required to advance the DMA engine between frames,
>> +			 * so usleep_range() (which would space out the reads)
>> +			 * is deliberately not used here.
>> +			 */
>> +			cond_resched();
>> +		} while (1);
>> +	}
>> +
>> +	/* Check for I2S/BRA errors */
>> +	val = readl(amd_manager->acp_mmio + i2s_err_offset);
>> +	if (val & AMD_SDW_BRA_I2S_ERROR_MASK) {
>> +		dev_err(amd_manager->dev,
>> +			"BPT: BRA failed I2S_ERROR_REASON=0x%08x (NAK=%u Clash=%u HdrResp=%u FtrResp=%u CRC=%u DMA=%u Cmd=%u)\n",
>> +			val,
>> +			!!(val & BIT(18)), !!(val & BIT(19)),
>> +			!!(val & BIT(26)), !!(val & BIT(27)),
>> +			!!(val & BIT(28)), !!(val & BIT(29)), !!(val & BIT(30)));
>> +		writel(0, amd_manager->acp_mmio + i2s_err_offset);
>> +		if (!ret)
>> +			ret = -EIO;
>> +	} else if (!ret) {
>> +		dev_dbg(amd_manager->dev,
>> +			"BPT: DMA done: periph=0x%08x len=%u\n", reg_addr, len);
>> +	}
>> +
>> +	/*
>> +	 * On an aborted or timed-out transfer the ACP BPT DMA engine can still
>> +	 * be armed (PORT_EN=1, DMA_BUSY=1). sdw_disable_stream() below performs
>> +	 * a bank switch, which is the BPT DMA start trigger, so disarm the engine
>> +	 * first -- otherwise the bank switch could (re)start a DMA write into
>> +	 * dma_buf, which amd_sdw_bpt_wait() frees as soon as this transfer
>> +	 * returns. On the success path the DMA has already completed, so this
>> +	 * early clear is a no-op.
>> +	 */
>> +	if (ret < 0)
>> +		writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
>> +
>> +	/*
>> +	 * Disable the stream via framework (writes CHANNELEN=0 + bank switch).
>> +	 * This cleanly stops the port and keeps bank state consistent.
>> +	 * Propagate a disable failure: it leaves the stream in
>> +	 * SDW_STREAM_ENABLED, and sdw_enable_stream() returns 0 early for an
>> +	 * already-ENABLED stream without the bank switch that triggers the BRA
>> +	 * DMA, so the next section would be silently skipped rather than
>> +	 * transferred.  Report the failure to the caller so it stops instead of
>> +	 * chaining a section whose DMA never starts.
>> +	 */
>> +	ret_disable = sdw_disable_stream(bus->bpt_stream);
>> +	if (ret_disable < 0) {
>> +		dev_err(amd_manager->dev, "BPT: sdw_disable_stream failed: %d\n",
>> +			ret_disable);
>> +		if (!ret)
>> +			ret = ret_disable;
>> +	}
>> +
>> +	dev_dbg(amd_manager->dev,
>> +		"BPT: post-disable: curr_bank=%u next_bank=%u stream_state=%d\n",
>> +		bus->params.curr_bank, bus->params.next_bank,
>> +		bus->bpt_stream->state);
>> +
>> +	/*
>> +	 * Disarm the ACP BPT DMA engine and wait for the port to quiesce.
>> +	 * Like the manager enable/disable sequence (ACP_SW_EN paired with
>> +	 * ACP_SW_EN_STATUS), ACP_SW_BPT_PORT_EN_STATUS reflects the real port
>> +	 * state, so polling it here guarantees a non-contiguous transfer's next
>> +	 * section cannot re-arm PORT_EN before the current one has torn down.
>> +	 */
>> +	writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
>> +	ret_disable = readl_poll_timeout(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS,
>> +					 val, !val, ACP_DELAY_US, AMD_SDW_TIMEOUT);
>> +	if (ret_disable < 0) {
>> +		dev_err(amd_manager->dev, "BPT: PORT_EN disable timeout\n");
>> +		if (!ret)
>> +			ret = ret_disable;
>> +		*dma_unsafe = true;
>> +	}
>> +	writel(0, amd_manager->acp_mmio + i2s_err_offset);
>> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_REASON1);
>> +	/*
>> +	 * Clearing the immediate-command response-valid status races with the
>> +	 * immediate-command path (amd_sdw_send_cmd_get_resp()), which polls and
>> +	 * clears the same ACP_SW_IMM_CMD_STS bit under bus->msg_lock. The BPT DMA
>> +	 * poll loop above drops all locks, so a concurrent slave enumeration or
>> +	 * register access (amd_sdw_work / IRQ thread / codec regmap) can be
>> +	 * mid-command here. Take msg_lock so this teardown clear cannot erase a
>> +	 * response the command path has not yet consumed, which would make that
>> +	 * command time out. This msg_lock nests inside acp_bra_lock and bpt_lock,
>> +	 * both already held by the enclosing amd_sdw_bpt_wait(), preserving the
>> +	 * bpt_lock -> acp_bra_lock -> msg_lock order. do_bank_switch() inside
>> +	 * sdw_enable_stream()/sdw_disable_stream() only takes msg_lock for
>> +	 * multi-link managers, so this single-link manager establishes that order
>> +	 * via the explicit msg_lock here and in the non-contiguous
>> +	 * sdw_nwrite_no_pm()/sdw_nread_no_pm() fallback.
>> +	 */
>> +	mutex_lock(&bus->msg_lock);
>> +	writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>> +	mutex_unlock(&bus->msg_lock);
>> +
>> +restore_intr:
>> +	writel(saved_intr_mask, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
>> +	return ret;
>> +}
> [...]
>
>> +static int amd_sdw_bpt_wait(struct sdw_bus *bus,
>> +			    struct sdw_slave *slave,
>> +			    struct sdw_bpt_msg *msg)
>> +{
>> +	struct amd_sdw_manager *amd_manager = to_amd_sdw(bus);
>> +	bool is_write = (msg->flags & SDW_MSG_FLAG_WRITE);
>> +	struct amd_bra_params prep_params = {0};
>> +	u32 acp_sys_addr;
>> +	dma_addr_t dma_addr = 0;
>> +	u8 *dma_buf = NULL;
>> +	size_t offset = 0;
>> +	size_t total_len = 0;
>> +	int ret = 0;
>> +	int i;
>> +	bool dma_unsafe = false;
>> +
>> +	for (i = 0; i < msg->sections; i++) {
>> +		if (msg->sec[i].len > SDW_BPT_MSG_MAX_BYTES - total_len) {
>> +			dev_err(amd_manager->dev,
>> +				"BPT msg length exceeds maximum %d bytes\n",
>> +				SDW_BPT_MSG_MAX_BYTES);
>> +			return -EINVAL;
>> +		}
>> +		total_len += msg->sec[i].len;
>> +	}
>> +
>> +	if (total_len < AMD_BPT_MSG_BYTE_MIN) {
>> +		dev_err(amd_manager->dev,
>> +			"BPT msg length %zu < minimum %d bytes\n",
>> +			total_len, AMD_BPT_MSG_BYTE_MIN);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Take the runtime-PM reference that keeps the bus clock alive before
>> +	 * acquiring bpt_lock, not after.  When the manager is runtime
>> +	 * suspended, pm_runtime_get_sync() runs amd_resume_runtime()
>> +	 * synchronously in this task, and that callback acquires bpt_lock to
>> +	 * clear bpt_disabled.  Taking the reference while already holding
>> +	 * bpt_lock would therefore deadlock against ourselves.
>> +	 */
>> +	ret = pm_runtime_get_sync(amd_manager->dev);
>> +	/*
>> +	 * -EACCES only occurs when runtime PM is disabled, which for this
>> +	 * device happens across the system suspend/resume window.  amd_suspend()
>> +	 * sets bpt_disabled before the clock is stopped and amd_resume_runtime()
>> +	 * clears it only once the clock is back, so the bpt_disabled check below
>> +	 * rejects the transfer with -ESHUTDOWN before any BRA access whenever the
>> +	 * clock could be down.  A merely runtime-suspended manager (clock stopped,
>> +	 * PM still enabled) is resumed by the get above and returns success, not
>> +	 * -EACCES; tolerating -EACCES therefore never drives the BRA engine on a
>> +	 * stopped clock.
>> +	 */
>> +	if (ret < 0 && ret != -EACCES) {
>> +		pm_runtime_put_noidle(amd_manager->dev);
>> +		dev_err(amd_manager->dev, "BPT: pm_runtime_get failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * The ACP BRA engine is a single shared resource.  Serialise all BPT
>> +	 * transfers so concurrent slave probes do not race over the hardware.
>> +	 */
>> +	mutex_lock(&amd_manager->bpt_lock);
>> +
>> +	/*
>> +	 * Refuse the transfer if the bus is being (or has been) clock-stopped
>> +	 * for system suspend.  Driving the BRA/command channel while the clock
>> +	 * is stopping wedges the channel and leaves the peripheral unable to
>> +	 * re-enumerate on resume; the codec retries once it re-attaches.
>> +	 */
>> +	if (amd_manager->bpt_disabled) {
>> +		mutex_unlock(&amd_manager->bpt_lock);
>> +		pm_runtime_mark_last_busy(amd_manager->dev);
>> +		pm_runtime_put_autosuspend(amd_manager->dev);
>> +		return -ESHUTDOWN;
>> +	}
>> +
>> +	/* BRA always uses all available data columns (1..NC-1). */
>> +	amd_manager->bra_hstart = 1;
>> +	amd_manager->bra_hstop  = bus->params.col - 1;
>> +
>> +	/*
>> +	 * Open the BPT stream so the framework stream state machine tracks the
>> +	 * transfer.  Slave DP0 is prepared below before bra_transfer().
>> +	 */
>> +	ret = amd_sdw_bpt_open_stream(amd_manager, slave, msg);
>> +	if (ret < 0)
>> +		goto close_stream;
>> +
>> +	/* Allocate single DMA buffer for entire BPT message */
>> +	dma_buf = dma_alloc_coherent(amd_manager->dev->parent, total_len,
>> +				     &dma_addr, GFP_KERNEL);
>> +	if (!dma_buf) {
>> +		ret = -ENOMEM;
>> +		goto close_stream;
>> +	}
>> +
>> +	/*
>> +	 * The ATU GRP_1 and scratch PTE registers programmed here are
>> +	 * ACP-global and shared by both SoundWire manager instances, as is the
>> +	 * BRA DMA engine that reads through them.  bpt_lock is per-manager and
>> +	 * does not serialise across instances, so hold the ACP-wide
>> +	 * acp_bra_lock across the whole configure -> transfer -> deconfigure
>> +	 * sequence to stop the other instance reprogramming the shared PTEs
>> +	 * mid-transfer.
> In that case, what is the point of having a per-instance btp_lock as well?
>
> It seems from the comment that only *one* BTP transfer can take place
> across all manager instances, which defeats the purpose of a
> per-instance lock, no?
> What I am missing?

You're right that the transfer itself is serialized ACP-wide: that's 
acp_bra_lock, held only across configure → transfer → deconfigure, 
because the ATU PTEs and
  the BRA DMA engine are single resources shared by both links. bpt_lock 
is per-link and has a different scope rather than a different transfer 
window:

  * It guards bpt_disabled, which is per-link
    suspend/clock-stop/runtime-PM state — each link's
    amd_suspend()/clock-stop
    sets it and amd_resume_runtime() clears it under bpt_lock. A
    per-link flag doesn't belong under an ACP-global lock.
  * It serializes multiple BPT requests to the same link and lets that
    link's suspend path drain an in-flight transfer, without reaching
    for the global lock.
  * It keeps acp_bra_lock's hold time minimal: the per-link
    setup/teardown (open_stream, dma_alloc, the PM get/put, the
    bpt_disabled check) runs
    under bpt_lock but outside acp_bra_lock, so the other link contends
    for the global lock only during the actual shared-hardware window,
    not the whole sequence.

Folding everything onto acp_bra_lock would mean the global lock has to 
cover each link's full transfer path including its runtime-PM callbacks,
  i.e. one link's PM transitions would serialize against the other 
link's transfer — a scope mismatch and a longer global hold time.
  So the two are intentionally different scopes: bpt_lock = per-link 
lifecycle/PM, acp_bra_lock = ACP-global shared hardware. Today the 
comment only documents acp_bra_lock; I'll expand it to cover both.

[-- Attachment #2: Type: text/html, Size: 19646 bytes --]

Re: [PATCH v3 4/5] soundwire: amd: Add BRA/BPT firmware download support

[Pierre-Louis Bossart]

>> general comment: this patch is quite complex, with code needed for the
>> BTP side as well as the DMA side, only interleaved. Is there a way to
>> have a 'cleaner' split between DMA functionality on the ACP side and BPT
>> on the SoundWire bus side?
>>
>> Likewise the parts dealing with contiguous and non-contiguous parts of
>> the firmware should be handled at a higher level using DMA/BTP support
>> lower-level routines.
> 
> Agreed, I'll restructure this into three layers for v5: ACP BRA /DMA/
> primitives (configure descriptor, arm, poll/wait, disarm, deconfigure)
> with no SoundWire-stream knowledge; SoundWire /BPT/ primitives (open/
> close stream, enable/disable); 
> and a higher-level orchestrator with a dispatcher for the contiguous
> (single-buffer) vs non-contiguous (per-section loop, with the small-
> section sdw_nwrite/nread fallback) cases.

Sounds good.

> One hardware detail I'll preserve and document along the way: the
> SoundWire bank switch is itself the BRA DMA start trigger,and correct
> teardown requires disarming the DMA before the disable-path bank switch. 

That sounds fine, provided that hardware pushes a invalid BPT frame on
the link in the time window between the disarming and bank switch.
Otherwise the peripherals might receive garbage BTP data, no?
> So the orchestrator still sequences an ACP call and a SoundWire call in
> a fixed order — the layering makes each side independently readable and
> testable while keeping that ordering contract explicit.
> 
>>> +static int amd_sdw_execute_bra_transfer(struct amd_sdw_manager *amd_manager,
>>> +					struct sdw_slave *slave,
>>> +					bool *dma_unsafe)
>>> +{
>>> +	struct sdw_bus *bus = &amd_manager->bus;
>>> +	u32 i2s_err_offset;
>>> +	u32 saved_intr_mask;
>>> +	u32 reg_addr, len;
>>> +	u32 val;
>>> +	int ret, ret_disable;
>>> +
>>> +	/* Read descriptor regs before enabling the DMA engine. */
>>> +	reg_addr = readl(amd_manager->mmio + ACP_SW_BPT_PORT_FIRST_BYTE_ADDR);
>>> +	len = readl(amd_manager->mmio + ACP_SW_BRA_TRANSFER_SIZE);
>>> +
>>> +	i2s_err_offset = (amd_manager->instance == 0) ?
>>> +			 ACP_SW_I2S_ERROR_REASON : ACP_P1_SW_I2S_ERROR_REASON;
>>> +
>>> +	/*
>>> +	 * Save and disable the error interrupt mask for manual error
>>> +	 * checking.  acp_bra_lock is held across the whole BPT sequence by
>>> +	 * amd_sdw_bpt_wait(), which serialises this shared-register
>>> +	 * read-modify-write against the other manager instance.
>>> +	 */
>>> +	saved_intr_mask = readl(amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
>>> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
>>> +	writel(0, amd_manager->acp_mmio + i2s_err_offset);
>>> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_REASON1);
>>> +
>>> +	/* Arm the ACP BPT DMA engine */
>>> +	writel(1, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
>>> +
>>> +	/*
>>> +	 * Use the framework's sdw_enable_stream() to write CHANNELEN and
>>> +	 * perform a bank switch.  The ACP BPT hardware uses the bank switch
>>> +	 * as the trigger to start the DMA transfer.  The framework manages
>> do you mean to say
>> a) the DMA transfer was armed and started ealier, and the bank switch
>> unblocks it, ob
>> b) the DMA starts fetching data from memory when the bank switch happens?
>>
>> the latter case would be quite racy and dependent on the time needed to
>> access memory..
> It's (a). The BRA descriptor (BASE_ADDRESS/TRANSFER_SIZE/FRAME_FORMAT)
> is fully programmed in amd_sdw_config_bra_descriptor(), 
> and the engine is armed with ACP_SW_BPT_PORT_EN=1, both before
> sdw_enable_stream(). 
> The bank switch is only the trigger for the already-armed engine — it
> doesn't start a cold engine, so there's no memory-latency race on the
> switch. 

ok

> Once triggered the engine runs autonomously frame by frame, and any
> stall/underrun surfaces as a BRA/I2S error (ACP_SW_I2S_ERROR_REASON /
> ACP_SW_BRA_RESP), not silent corruption. 

but then if I follow your explanations above, if you stop the DMA first
don't you get a systematic xrun error before the bank switch happens?
> This is also why, on an aborted or timed-out transfer, the teardown
> clears PORT_EN before the sdw_disable_stream() bank switch — otherwise
> that switch 
> could re-trigger the armed engine into a buffer we're about to free. 
> I'll reword the comment to say the engine is armed here and the bank
> switch only triggers it.

ok

[...]

>>> +	/*
>>> +	 * The ATU GRP_1 and scratch PTE registers programmed here are
>>> +	 * ACP-global and shared by both SoundWire manager instances, as is the
>>> +	 * BRA DMA engine that reads through them.  bpt_lock is per-manager and
>>> +	 * does not serialise across instances, so hold the ACP-wide
>>> +	 * acp_bra_lock across the whole configure -> transfer -> deconfigure
>>> +	 * sequence to stop the other instance reprogramming the shared PTEs
>>> +	 * mid-transfer.
>> In that case, what is the point of having a per-instance btp_lock as well?
>>
>> It seems from the comment that only *one* BTP transfer can take place
>> across all manager instances, which defeats the purpose of a
>> per-instance lock, no?
>> What I am missing?
> 
> You're right that the transfer itself is serialized ACP-wide: that's
> acp_bra_lock, held only across configure → transfer → deconfigure,
> because the ATU PTEs and
>  the BRA DMA engine are single resources shared by both links. bpt_lock
> is per-link and has a different scope rather than a different transfer
> window:
> 
>   * It guards bpt_disabled, which is per-link suspend/clock-stop/
>     runtime-PM state — each link's amd_suspend()/clock-stop 
>     sets it and amd_resume_runtime() clears it under bpt_lock. A per-
>     link flag doesn't belong under an ACP-global lock.
>   * It serializes multiple BPT requests to the same link and lets that
>     link's suspend path drain an in-flight transfer, without reaching
>     for the global lock.
>   * It keeps acp_bra_lock's hold time minimal: the per-link setup/
>     teardown (open_stream, dma_alloc, the PM get/put, the bpt_disabled
>     check) runs 
>     under bpt_lock but outside acp_bra_lock, so the other link contends
>     for the global lock only during the actual shared-hardware window,
>     not the whole sequence.
> 
> Folding everything onto acp_bra_lock would mean the global lock has to
> cover each link's full transfer path including its runtime-PM callbacks,
>  i.e. one link's PM transitions would serialize against the other link's
> transfer — a scope mismatch and a longer global hold time.
>  So the two are intentionally different scopes: bpt_lock = per-link
> lifecycle/PM, acp_bra_lock = ACP-global shared hardware. Today the
> comment only documents acp_bra_lock; I'll expand it to cover both.

The explanation make sense... but I am not fully clear on why you'd need
to protect the BPT state for link-based pm_runtime. Presumably a BPT
transfer would happen with a device reference held to prevent it from
suspending? Something's not right if you need to drain a BPT transfer in
a pm_runtime suspend, that suspend shouldn't happen in the first place...

Re: [PATCH v3 4/5] soundwire: amd: Add BRA/BPT firmware download support

[Syed Saba Kareem]

On 10/7/26 21:45, Pierre-Louis Bossart wrote:
>>> general comment: this patch is quite complex, with code needed for the
>>> BTP side as well as the DMA side, only interleaved. Is there a way to
>>> have a 'cleaner' split between DMA functionality on the ACP side and BPT
>>> on the SoundWire bus side?
>>>
>>> Likewise the parts dealing with contiguous and non-contiguous parts of
>>> the firmware should be handled at a higher level using DMA/BTP support
>>> lower-level routines.
>> Agreed, I'll restructure this into three layers for v5: ACP BRA /DMA/
>> primitives (configure descriptor, arm, poll/wait, disarm, deconfigure)
>> with no SoundWire-stream knowledge; SoundWire /BPT/ primitives (open/
>> close stream, enable/disable);
>> and a higher-level orchestrator with a dispatcher for the contiguous
>> (single-buffer) vs non-contiguous (per-section loop, with the small-
>> section sdw_nwrite/nread fallback) cases.
> Sounds good.
>
>> One hardware detail I'll preserve and document along the way: the
>> SoundWire bank switch is itself the BRA DMA start trigger,and correct
>> teardown requires disarming the DMA before the disable-path bank switch.
> That sounds fine, provided that hardware pushes a invalid BPT frame on
> the link in the time window between the disarming and bank switch.
> Otherwise the peripherals might receive garbage BTP data, no?
You're right to flag this. The early disarm only runs on the error path 
— it's gated on the transfer having already timed out or failed (if (ret 
< 0) before sdw_disable_stream()),
so on a normal completion it's skipped entirely and there's no 
disarm-before-switch window on a good transfer (the ordering is in Q2).
On the error path the transfer is already being failed back to the 
caller and the firmware download is retried/aborted, so its result is 
never relied upon.
I also keep the error interrupt masked (ACP_SW_ERROR_INTR_MASK) and the 
error-reason registers cleared across the teardown, so the expected 
teardown error isn't mis-reported as a fresh failure.
If it helps, I'm happy to add an explicit note in the comment that the 
error-path frame is discarded by the retry rather than relied upon.
>> So the orchestrator still sequences an ACP call and a SoundWire call in
>> a fixed order — the layering makes each side independently readable and
>> testable while keeping that ordering contract explicit.
>>
>>>> +static int amd_sdw_execute_bra_transfer(struct amd_sdw_manager *amd_manager,
>>>> +					struct sdw_slave *slave,
>>>> +					bool *dma_unsafe)
>>>> +{
>>>> +	struct sdw_bus *bus = &amd_manager->bus;
>>>> +	u32 i2s_err_offset;
>>>> +	u32 saved_intr_mask;
>>>> +	u32 reg_addr, len;
>>>> +	u32 val;
>>>> +	int ret, ret_disable;
>>>> +
>>>> +	/* Read descriptor regs before enabling the DMA engine. */
>>>> +	reg_addr = readl(amd_manager->mmio + ACP_SW_BPT_PORT_FIRST_BYTE_ADDR);
>>>> +	len = readl(amd_manager->mmio + ACP_SW_BRA_TRANSFER_SIZE);
>>>> +
>>>> +	i2s_err_offset = (amd_manager->instance == 0) ?
>>>> +			 ACP_SW_I2S_ERROR_REASON : ACP_P1_SW_I2S_ERROR_REASON;
>>>> +
>>>> +	/*
>>>> +	 * Save and disable the error interrupt mask for manual error
>>>> +	 * checking.  acp_bra_lock is held across the whole BPT sequence by
>>>> +	 * amd_sdw_bpt_wait(), which serialises this shared-register
>>>> +	 * read-modify-write against the other manager instance.
>>>> +	 */
>>>> +	saved_intr_mask = readl(amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
>>>> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
>>>> +	writel(0, amd_manager->acp_mmio + i2s_err_offset);
>>>> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_REASON1);
>>>> +
>>>> +	/* Arm the ACP BPT DMA engine */
>>>> +	writel(1, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
>>>> +
>>>> +	/*
>>>> +	 * Use the framework's sdw_enable_stream() to write CHANNELEN and
>>>> +	 * perform a bank switch.  The ACP BPT hardware uses the bank switch
>>>> +	 * as the trigger to start the DMA transfer.  The framework manages
>>> do you mean to say
>>> a) the DMA transfer was armed and started ealier, and the bank switch
>>> unblocks it, ob
>>> b) the DMA starts fetching data from memory when the bank switch happens?
>>>
>>> the latter case would be quite racy and dependent on the time needed to
>>> access memory..
>> It's (a). The BRA descriptor (BASE_ADDRESS/TRANSFER_SIZE/FRAME_FORMAT)
>> is fully programmed in amd_sdw_config_bra_descriptor(),
>> and the engine is armed with ACP_SW_BPT_PORT_EN=1, both before
>> sdw_enable_stream().
>> The bank switch is only the trigger for the already-armed engine — it
>> doesn't start a cold engine, so there's no memory-latency race on the
>> switch.
> ok
>
>> Once triggered the engine runs autonomously frame by frame, and any
>> stall/underrun surfaces as a BRA/I2S error (ACP_SW_I2S_ERROR_REASON /
>> ACP_SW_BRA_RESP), not silent corruption.
> but then if I follow your explanations above, if you stop the DMA first
> don't you get a systematic xrun error before the bank switch happens?
That would be true if the engine were still running, but on the normal 
path it has already finished before we stop it. The order is:

 1. arm (ACP_SW_BPT_PORT_EN=1, armed but idle)
 2. sdw_enable_stream() — the bank switch triggers the armed engine
 3. poll ACP_SW_BRA_DMA_BUSY to zero (transfer completes)
 4. ret == 0, so the early PORT_EN=0 is skipped
 5. sdw_disable_stream() — bank switch, CHANNELEN=0
 6. PORT_EN=0 on an already-idle engine (a no-op)

So there's no "stop a running engine, then switch" on the normal path, 
and hence no xrun. The early disarm only runs on an already-failed 
transfer, where the xrun is expected and is masked/cleared as above.
>> This is also why, on an aborted or timed-out transfer, the teardown
>> clears PORT_EN before the sdw_disable_stream() bank switch — otherwise
>> that switch
>> could re-trigger the armed engine into a buffer we're about to free.
>> I'll reword the comment to say the engine is armed here and the bank
>> switch only triggers it.
> ok
>
> [...]
>
>>>> +	/*
>>>> +	 * The ATU GRP_1 and scratch PTE registers programmed here are
>>>> +	 * ACP-global and shared by both SoundWire manager instances, as is the
>>>> +	 * BRA DMA engine that reads through them.  bpt_lock is per-manager and
>>>> +	 * does not serialise across instances, so hold the ACP-wide
>>>> +	 * acp_bra_lock across the whole configure -> transfer -> deconfigure
>>>> +	 * sequence to stop the other instance reprogramming the shared PTEs
>>>> +	 * mid-transfer.
>>> In that case, what is the point of having a per-instance btp_lock as well?
>>>
>>> It seems from the comment that only *one* BTP transfer can take place
>>> across all manager instances, which defeats the purpose of a
>>> per-instance lock, no?
>>> What I am missing?
>> You're right that the transfer itself is serialized ACP-wide: that's
>> acp_bra_lock, held only across configure → transfer → deconfigure,
>> because the ATU PTEs and
>>   the BRA DMA engine are single resources shared by both links. bpt_lock
>> is per-link and has a different scope rather than a different transfer
>> window:
>>
>>    * It guards bpt_disabled, which is per-link suspend/clock-stop/
>>      runtime-PM state — each link's amd_suspend()/clock-stop
>>      sets it and amd_resume_runtime() clears it under bpt_lock. A per-
>>      link flag doesn't belong under an ACP-global lock.
>>    * It serializes multiple BPT requests to the same link and lets that
>>      link's suspend path drain an in-flight transfer, without reaching
>>      for the global lock.
>>    * It keeps acp_bra_lock's hold time minimal: the per-link setup/
>>      teardown (open_stream, dma_alloc, the PM get/put, the bpt_disabled
>>      check) runs
>>      under bpt_lock but outside acp_bra_lock, so the other link contends
>>      for the global lock only during the actual shared-hardware window,
>>      not the whole sequence.
>>
>> Folding everything onto acp_bra_lock would mean the global lock has to
>> cover each link's full transfer path including its runtime-PM callbacks,
>>   i.e. one link's PM transitions would serialize against the other link's
>> transfer — a scope mismatch and a longer global hold time.
>>   So the two are intentionally different scopes: bpt_lock = per-link
>> lifecycle/PM, acp_bra_lock = ACP-global shared hardware. Today the
>> comment only documents acp_bra_lock; I'll expand it to cover both.
> The explanation make sense... but I am not fully clear on why you'd need
> to protect the BPT state for link-based pm_runtime. Presumably a BPT
> transfer would happen with a device reference held to prevent it from
> suspending? Something's not right if you need to drain a BPT transfer in
> a pm_runtime suspend, that suspend shouldn't happen in the first place...
You're right, and thanks for catching it — I described this poorly 
earlier. A BPT transfer takes pm_runtime_get_sync() in 
amd_sdw_bpt_wait() and holds the reference until the matching put after 
the transfer,
so usage_count stays > 0 and runtime suspend can't be entered while a 
transfer is in flight. amd_suspend_runtime() doesn't touch bpt_disabled 
at all — there's no draining of a runtime-PM suspend.
The mutex_trylock(&bpt_lock) there is only belt-and-braces for that 
invariant.
bpt_disabled exists only for the case the held reference can't cover: 
the system-sleep path. There runtime PM is disabled, so a BPT request's 
pm_runtime_get_sync() returns -EACCES instead of resuming the device,
and it would otherwise proceed with the bus clock stopped. That's a real 
scenario for us — the power-off-mode resume-time firmware re-download 
runs while userspace is still frozen.
  So amd_suspend() latches bpt_disabled under bpt_lock before 
amd_sdw_clock_stop(), any such request bails out with -ESHUTDOWN, and 
amd_resume_runtime() clears it once the clock is back.
amd_sdw_manager_remove() latches it for the same reason against 
sdw_bus_master_delete().
So the scope is really: bpt_lock = per-link serialization of BPT 
requests against that link's system suspend/resume clock-stop and driver 
removal (not runtime PM); acp_bra_lock = ACP-global serialization
of the shared BRA engine/ATU across the two links. I'll fix the comment 
that currently implies bpt_lock covers runtime-PM state — thanks for 
pointing it out.

Re: [PATCH v3 4/5] soundwire: amd: Add BRA/BPT firmware download support

[Syed Saba Kareem]

[-- Attachment #1: Type: text/plain, Size: 18065 bytes --]


On 10/5/26 17:34, Pierre-Louis Bossart wrote:
> general comment: this patch is quite complex, with code needed for the
> BTP side as well as the DMA side, only interleaved. Is there a way to
> have a 'cleaner' split between DMA functionality on the ACP side and BPT
> on the SoundWire bus side?
>
> Likewise the parts dealing with contiguous and non-contiguous parts of
> the firmware should be handled at a higher level using DMA/BTP support
> lower-level routines.

Agreed, I'll restructure this into three layers for v5: ACP BRA DMA 
primitives (configure descriptor, arm, poll/wait, disarm, deconfigure) 
with no SoundWire-stream knowledge;
SoundWire BPT primitives (open/close stream, enable/disable); and a 
higher-level orchestrator with a dispatcher for the contiguous 
(single-buffer) vs non-contiguous (per-section loop,
with the small-section sdw_nwrite/nread fallback) cases.

One hardware detail I'll preserve and document along the way: the 
SoundWire bank switch is itself the BRA DMA start trigger, and
correct teardown requires disarming the DMA before the disable-path bank 
switch.
So the orchestrator still sequences an ACP call and a SoundWire call in 
a fixed order — the layering makes each side independently readable and 
testable while keeping that ordering contract explicit.

>> +static int amd_sdw_execute_bra_transfer(struct amd_sdw_manager *amd_manager,
>> +					struct sdw_slave *slave,
>> +					bool *dma_unsafe)
>> +{
>> +	struct sdw_bus *bus = &amd_manager->bus;
>> +	u32 i2s_err_offset;
>> +	u32 saved_intr_mask;
>> +	u32 reg_addr, len;
>> +	u32 val;
>> +	int ret, ret_disable;
>> +
>> +	/* Read descriptor regs before enabling the DMA engine. */
>> +	reg_addr = readl(amd_manager->mmio + ACP_SW_BPT_PORT_FIRST_BYTE_ADDR);
>> +	len = readl(amd_manager->mmio + ACP_SW_BRA_TRANSFER_SIZE);
>> +
>> +	i2s_err_offset = (amd_manager->instance == 0) ?
>> +			 ACP_SW_I2S_ERROR_REASON : ACP_P1_SW_I2S_ERROR_REASON;
>> +
>> +	/*
>> +	 * Save and disable the error interrupt mask for manual error
>> +	 * checking.  acp_bra_lock is held across the whole BPT sequence by
>> +	 * amd_sdw_bpt_wait(), which serialises this shared-register
>> +	 * read-modify-write against the other manager instance.
>> +	 */
>> +	saved_intr_mask = readl(amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
>> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
>> +	writel(0, amd_manager->acp_mmio + i2s_err_offset);
>> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_REASON1);
>> +
>> +	/* Arm the ACP BPT DMA engine */
>> +	writel(1, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
>> +
>> +	/*
>> +	 * Use the framework's sdw_enable_stream() to write CHANNELEN and
>> +	 * perform a bank switch.  The ACP BPT hardware uses the bank switch
>> +	 * as the trigger to start the DMA transfer.  The framework manages
> do you mean to say
> a) the DMA transfer was armed and started ealier, and the bank switch
> unblocks it, ob
> b) the DMA starts fetching data from memory when the bank switch happens?
>
> the latter case would be quite racy and dependent on the time needed to
> access memory..
>
It's (a). The BRA descriptor (BASE_ADDRESS/TRANSFER_SIZE/FRAME_FORMAT) 
is fully programmed in amd_sdw_config_bra_descriptor(),
and the engine is armed with ACP_SW_BPT_PORT_EN=1, both before 
sdw_enable_stream().
The bank switch is only the trigger for the already-armed engine — it 
doesn't start a cold engine, so there's no memory-latency race on the 
switch.
Once triggered the engine runs autonomously frame by frame, and any 
stall/underrun surfaces as a BRA/I2S error (ACP_SW_I2S_ERROR_REASON / 
ACP_SW_BRA_RESP), not silent corruption.
This is also why, on an aborted or timed-out transfer, the teardown 
clears PORT_EN before the sdw_disable_stream() bank switch — otherwise 
that switch could
re-trigger the armed engine into a buffer we're about to free. I'll 
reword the comment to say the engine is armed here and the bank switch 
only triggers it.
>> +	 * bank state consistently, eliminating the need for a manual bank
>> +	 * switch or DP0 bank mirror.
>> +	 */
>> +	dev_dbg(amd_manager->dev,
>> +		"BPT: pre-enable: curr_bank=%u next_bank=%u BPT_EN_STATUS=0x%x stream_state=%d\n",
>> +		bus->params.curr_bank, bus->params.next_bank,
>> +		readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
>> +		bus->bpt_stream->state);
>> +
>> +	ret = sdw_enable_stream(bus->bpt_stream);
>> +	if (ret < 0) {
>> +		dev_err(amd_manager->dev,
>> +			"BPT: sdw_enable_stream failed: %d\n", ret);
>> +		/*
>> +		 * Disarm the engine and wait for the port to quiesce before
>> +		 * returning: the caller (amd_sdw_bra_transfer()) then deconfigures
>> +		 * the BRA descriptor unconditionally, zeroing BASE_ADDRESS and
>> +		 * TRANSFER_SIZE, so the port must be idle first -- mirrors the
>> +		 * disable path below.
>> +		 */
>> +		writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
>> +		ret_disable = readl_poll_timeout(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS,
>> +						 val, !val, ACP_DELAY_US, AMD_SDW_TIMEOUT);
>> +		if (ret_disable < 0) {
>> +			dev_err(amd_manager->dev, "BPT: PORT_EN disable timeout\n");
>> +			*dma_unsafe = true;
>> +		}
>> +		goto restore_intr;
>> +	}
>> +
>> +	dev_dbg(amd_manager->dev,
>> +		"BPT: DMA started: curr_bank=%u next_bank=%u BPT_EN_STATUS=0x%x stream_state=%d\n",
>> +		bus->params.curr_bank, bus->params.next_bank,
>> +		readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
>> +		bus->bpt_stream->state);
>> +
>> +	/* Poll DMA_BUSY until transfer completes */
>> +	{
>> +		unsigned long timeout;
>> +
>> +		timeout = jiffies + msecs_to_jiffies(BRA_DMA_TIMEOUT_MS);
>> +		do {
>> +			val = readl(amd_manager->mmio + ACP_SW_BRA_DMA_BUSY);
>> +			/*
>> +			 * Side-effectful read: reading ACP_SW_BRA_CURRENT_TRANSFER_SIZE
>> +			 * advances the BRA DMA engine to the next frame.  The value is
>> +			 * intentionally discarded (hence the (void) cast); removing this
>> +			 * read stalls multi-frame transfers, which then time out.
>> +			 */
>> +			(void)readl(amd_manager->mmio + ACP_SW_BRA_CURRENT_TRANSFER_SIZE);
>> +			if (!(val & 0x01))
>> +				break;
>> +			if (time_after(jiffies, timeout)) {
>> +				/*
>> +				 * Re-sample the busy bit before declaring a
>> +				 * timeout. cond_resched() below can park this
>> +				 * thread for longer than BRA_DMA_TIMEOUT_MS, in
>> +				 * which window the DMA may have completed; without
>> +				 * this final read a finished transfer would be
>> +				 * reported as a false timeout.
>> +				 */
>> +				val = readl(amd_manager->mmio + ACP_SW_BRA_DMA_BUSY);
>> +				if (!(val & 0x01))
>> +					break;
>> +				dev_err(amd_manager->dev,
>> +					"BPT: DMA timeout: periph=0x%08x len=%u EN_STATUS=0x%x RESP=0x%x I2S_ERR=0x%08x\n",
>> +					reg_addr, len,
>> +					readl(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS),
>> +					readl(amd_manager->mmio + ACP_SW_BRA_RESP),
>> +					readl(amd_manager->acp_mmio + i2s_err_offset));
>> +				ret = -ETIMEDOUT;
>> +				break;
>> +			}
>> +			/*
>> +			 * Runs in process context. Yield the CPU so a
>> +			 * multi-frame transfer cannot busy-spin for up to
>> +			 * BRA_DMA_TIMEOUT_MS and trip the soft lockup
>> +			 * watchdog on non-preemptible kernels. cond_resched()
>> +			 * keeps the poll cadence tight -- reading
>> +			 * ACP_SW_BRA_CURRENT_TRANSFER_SIZE every iteration is
>> +			 * required to advance the DMA engine between frames,
>> +			 * so usleep_range() (which would space out the reads)
>> +			 * is deliberately not used here.
>> +			 */
>> +			cond_resched();
>> +		} while (1);
>> +	}
>> +
>> +	/* Check for I2S/BRA errors */
>> +	val = readl(amd_manager->acp_mmio + i2s_err_offset);
>> +	if (val & AMD_SDW_BRA_I2S_ERROR_MASK) {
>> +		dev_err(amd_manager->dev,
>> +			"BPT: BRA failed I2S_ERROR_REASON=0x%08x (NAK=%u Clash=%u HdrResp=%u FtrResp=%u CRC=%u DMA=%u Cmd=%u)\n",
>> +			val,
>> +			!!(val & BIT(18)), !!(val & BIT(19)),
>> +			!!(val & BIT(26)), !!(val & BIT(27)),
>> +			!!(val & BIT(28)), !!(val & BIT(29)), !!(val & BIT(30)));
>> +		writel(0, amd_manager->acp_mmio + i2s_err_offset);
>> +		if (!ret)
>> +			ret = -EIO;
>> +	} else if (!ret) {
>> +		dev_dbg(amd_manager->dev,
>> +			"BPT: DMA done: periph=0x%08x len=%u\n", reg_addr, len);
>> +	}
>> +
>> +	/*
>> +	 * On an aborted or timed-out transfer the ACP BPT DMA engine can still
>> +	 * be armed (PORT_EN=1, DMA_BUSY=1). sdw_disable_stream() below performs
>> +	 * a bank switch, which is the BPT DMA start trigger, so disarm the engine
>> +	 * first -- otherwise the bank switch could (re)start a DMA write into
>> +	 * dma_buf, which amd_sdw_bpt_wait() frees as soon as this transfer
>> +	 * returns. On the success path the DMA has already completed, so this
>> +	 * early clear is a no-op.
>> +	 */
>> +	if (ret < 0)
>> +		writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
>> +
>> +	/*
>> +	 * Disable the stream via framework (writes CHANNELEN=0 + bank switch).
>> +	 * This cleanly stops the port and keeps bank state consistent.
>> +	 * Propagate a disable failure: it leaves the stream in
>> +	 * SDW_STREAM_ENABLED, and sdw_enable_stream() returns 0 early for an
>> +	 * already-ENABLED stream without the bank switch that triggers the BRA
>> +	 * DMA, so the next section would be silently skipped rather than
>> +	 * transferred.  Report the failure to the caller so it stops instead of
>> +	 * chaining a section whose DMA never starts.
>> +	 */
>> +	ret_disable = sdw_disable_stream(bus->bpt_stream);
>> +	if (ret_disable < 0) {
>> +		dev_err(amd_manager->dev, "BPT: sdw_disable_stream failed: %d\n",
>> +			ret_disable);
>> +		if (!ret)
>> +			ret = ret_disable;
>> +	}
>> +
>> +	dev_dbg(amd_manager->dev,
>> +		"BPT: post-disable: curr_bank=%u next_bank=%u stream_state=%d\n",
>> +		bus->params.curr_bank, bus->params.next_bank,
>> +		bus->bpt_stream->state);
>> +
>> +	/*
>> +	 * Disarm the ACP BPT DMA engine and wait for the port to quiesce.
>> +	 * Like the manager enable/disable sequence (ACP_SW_EN paired with
>> +	 * ACP_SW_EN_STATUS), ACP_SW_BPT_PORT_EN_STATUS reflects the real port
>> +	 * state, so polling it here guarantees a non-contiguous transfer's next
>> +	 * section cannot re-arm PORT_EN before the current one has torn down.
>> +	 */
>> +	writel(0, amd_manager->mmio + ACP_SW_BPT_PORT_EN);
>> +	ret_disable = readl_poll_timeout(amd_manager->mmio + ACP_SW_BPT_PORT_EN_STATUS,
>> +					 val, !val, ACP_DELAY_US, AMD_SDW_TIMEOUT);
>> +	if (ret_disable < 0) {
>> +		dev_err(amd_manager->dev, "BPT: PORT_EN disable timeout\n");
>> +		if (!ret)
>> +			ret = ret_disable;
>> +		*dma_unsafe = true;
>> +	}
>> +	writel(0, amd_manager->acp_mmio + i2s_err_offset);
>> +	writel(0, amd_manager->mmio + ACP_SW_ERROR_REASON1);
>> +	/*
>> +	 * Clearing the immediate-command response-valid status races with the
>> +	 * immediate-command path (amd_sdw_send_cmd_get_resp()), which polls and
>> +	 * clears the same ACP_SW_IMM_CMD_STS bit under bus->msg_lock. The BPT DMA
>> +	 * poll loop above drops all locks, so a concurrent slave enumeration or
>> +	 * register access (amd_sdw_work / IRQ thread / codec regmap) can be
>> +	 * mid-command here. Take msg_lock so this teardown clear cannot erase a
>> +	 * response the command path has not yet consumed, which would make that
>> +	 * command time out. This msg_lock nests inside acp_bra_lock and bpt_lock,
>> +	 * both already held by the enclosing amd_sdw_bpt_wait(), preserving the
>> +	 * bpt_lock -> acp_bra_lock -> msg_lock order. do_bank_switch() inside
>> +	 * sdw_enable_stream()/sdw_disable_stream() only takes msg_lock for
>> +	 * multi-link managers, so this single-link manager establishes that order
>> +	 * via the explicit msg_lock here and in the non-contiguous
>> +	 * sdw_nwrite_no_pm()/sdw_nread_no_pm() fallback.
>> +	 */
>> +	mutex_lock(&bus->msg_lock);
>> +	writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + ACP_SW_IMM_CMD_STS);
>> +	mutex_unlock(&bus->msg_lock);
>> +
>> +restore_intr:
>> +	writel(saved_intr_mask, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
>> +	return ret;
>> +}
> [...]
>
>> +static int amd_sdw_bpt_wait(struct sdw_bus *bus,
>> +			    struct sdw_slave *slave,
>> +			    struct sdw_bpt_msg *msg)
>> +{
>> +	struct amd_sdw_manager *amd_manager = to_amd_sdw(bus);
>> +	bool is_write = (msg->flags & SDW_MSG_FLAG_WRITE);
>> +	struct amd_bra_params prep_params = {0};
>> +	u32 acp_sys_addr;
>> +	dma_addr_t dma_addr = 0;
>> +	u8 *dma_buf = NULL;
>> +	size_t offset = 0;
>> +	size_t total_len = 0;
>> +	int ret = 0;
>> +	int i;
>> +	bool dma_unsafe = false;
>> +
>> +	for (i = 0; i < msg->sections; i++) {
>> +		if (msg->sec[i].len > SDW_BPT_MSG_MAX_BYTES - total_len) {
>> +			dev_err(amd_manager->dev,
>> +				"BPT msg length exceeds maximum %d bytes\n",
>> +				SDW_BPT_MSG_MAX_BYTES);
>> +			return -EINVAL;
>> +		}
>> +		total_len += msg->sec[i].len;
>> +	}
>> +
>> +	if (total_len < AMD_BPT_MSG_BYTE_MIN) {
>> +		dev_err(amd_manager->dev,
>> +			"BPT msg length %zu < minimum %d bytes\n",
>> +			total_len, AMD_BPT_MSG_BYTE_MIN);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Take the runtime-PM reference that keeps the bus clock alive before
>> +	 * acquiring bpt_lock, not after.  When the manager is runtime
>> +	 * suspended, pm_runtime_get_sync() runs amd_resume_runtime()
>> +	 * synchronously in this task, and that callback acquires bpt_lock to
>> +	 * clear bpt_disabled.  Taking the reference while already holding
>> +	 * bpt_lock would therefore deadlock against ourselves.
>> +	 */
>> +	ret = pm_runtime_get_sync(amd_manager->dev);
>> +	/*
>> +	 * -EACCES only occurs when runtime PM is disabled, which for this
>> +	 * device happens across the system suspend/resume window.  amd_suspend()
>> +	 * sets bpt_disabled before the clock is stopped and amd_resume_runtime()
>> +	 * clears it only once the clock is back, so the bpt_disabled check below
>> +	 * rejects the transfer with -ESHUTDOWN before any BRA access whenever the
>> +	 * clock could be down.  A merely runtime-suspended manager (clock stopped,
>> +	 * PM still enabled) is resumed by the get above and returns success, not
>> +	 * -EACCES; tolerating -EACCES therefore never drives the BRA engine on a
>> +	 * stopped clock.
>> +	 */
>> +	if (ret < 0 && ret != -EACCES) {
>> +		pm_runtime_put_noidle(amd_manager->dev);
>> +		dev_err(amd_manager->dev, "BPT: pm_runtime_get failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * The ACP BRA engine is a single shared resource.  Serialise all BPT
>> +	 * transfers so concurrent slave probes do not race over the hardware.
>> +	 */
>> +	mutex_lock(&amd_manager->bpt_lock);
>> +
>> +	/*
>> +	 * Refuse the transfer if the bus is being (or has been) clock-stopped
>> +	 * for system suspend.  Driving the BRA/command channel while the clock
>> +	 * is stopping wedges the channel and leaves the peripheral unable to
>> +	 * re-enumerate on resume; the codec retries once it re-attaches.
>> +	 */
>> +	if (amd_manager->bpt_disabled) {
>> +		mutex_unlock(&amd_manager->bpt_lock);
>> +		pm_runtime_mark_last_busy(amd_manager->dev);
>> +		pm_runtime_put_autosuspend(amd_manager->dev);
>> +		return -ESHUTDOWN;
>> +	}
>> +
>> +	/* BRA always uses all available data columns (1..NC-1). */
>> +	amd_manager->bra_hstart = 1;
>> +	amd_manager->bra_hstop  = bus->params.col - 1;
>> +
>> +	/*
>> +	 * Open the BPT stream so the framework stream state machine tracks the
>> +	 * transfer.  Slave DP0 is prepared below before bra_transfer().
>> +	 */
>> +	ret = amd_sdw_bpt_open_stream(amd_manager, slave, msg);
>> +	if (ret < 0)
>> +		goto close_stream;
>> +
>> +	/* Allocate single DMA buffer for entire BPT message */
>> +	dma_buf = dma_alloc_coherent(amd_manager->dev->parent, total_len,
>> +				     &dma_addr, GFP_KERNEL);
>> +	if (!dma_buf) {
>> +		ret = -ENOMEM;
>> +		goto close_stream;
>> +	}
>> +
>> +	/*
>> +	 * The ATU GRP_1 and scratch PTE registers programmed here are
>> +	 * ACP-global and shared by both SoundWire manager instances, as is the
>> +	 * BRA DMA engine that reads through them.  bpt_lock is per-manager and
>> +	 * does not serialise across instances, so hold the ACP-wide
>> +	 * acp_bra_lock across the whole configure -> transfer -> deconfigure
>> +	 * sequence to stop the other instance reprogramming the shared PTEs
>> +	 * mid-transfer.
> In that case, what is the point of having a per-instance btp_lock as well?
>
> It seems from the comment that only *one* BTP transfer can take place
> across all manager instances, which defeats the purpose of a
> per-instance lock, no?
> What I am missing?

You're right that the transfer itself is serialized ACP-wide: that's 
acp_bra_lock, held only across configure → transfer → deconfigure, 
because the ATU PTEs and
  the BRA DMA engine are single resources shared by both links. bpt_lock 
is per-link and has a different scope rather than a different transfer 
window:

  * It guards bpt_disabled, which is per-link
    suspend/clock-stop/runtime-PM state — each link's
    amd_suspend()/clock-stop sets it and amd_resume_runtime() clears it
    under bpt_lock. A per-link flag doesn't belong under an ACP-global lock.
  * It serializes multiple BPT requests to the same link and lets that
    link's suspend path drain an in-flight transfer, without reaching
    for the global lock.
  * It keeps acp_bra_lock's hold time minimal: the per-link
    setup/teardown (open_stream, dma_alloc, the PM get/put, the
    bpt_disabled check) runs under bpt_lock but outside acp_bra_lock, so
    the other link contends for the global lock only during the actual
    shared-hardware window, not the whole sequence.

Folding everything onto acp_bra_lock would mean the global lock has to 
cover each link's full transfer path including its runtime-PM callbacks, 
i.e. one link's PM transitions
  would serialize against the other link's transfer — a scope mismatch 
and a longer global hold time. So the two are intentionally different 
scopes: bpt_lock = per-link lifecycle/PM, acp_bra_lock = ACP-global 
shared hardware.
Today the comment only documents acp_bra_lock; I'll expand it to cover both.

[-- Attachment #2: Type: text/html, Size: 19146 bytes --]

[PATCH v3 5/5] soundwire: amd: honor peripheral BRA block alignment

[Syed Saba Kareem]

From: Syed Saba Kareem <Syed.SabaKareem@amd.com>

The MIPI DisCo property "mipi-sdw-bra-mode-block-alignment" (parsed into
sdw_slave_prop.bra_block_alignment) states that the BRA data payload must
be an integer multiple of the advertised value. Some peripherals only
ingest the BRA payload in fixed-size blocks and mis-assemble a trailing
partial block otherwise.

The ACP BRA descriptor programs BytesPerFrame (frame_format[19:11]), which
is the number of data payload bytes the engine packs into each BRA frame.
Derive it as before, then, when the peripheral advertises a non-zero
alignment, round bytes_per_frame down to the nearest multiple of it; the
engine simply uses more frames to cover the same transfer_length. This
mirrors the Cadence/Intel handling in sdw_cdns_bpt_find_buffer_sizes().

To read the property, amd_sdw_calculate_bra_params() now takes the
sdw_slave directly instead of just its device number, and derives
dev_addr from it.

Peripherals that do not advertise the property leave bra_block_alignment
at 0, so the new path is skipped and behavior is unchanged.

Signed-off-by: Syed Saba Kareem <Syed.SabaKareem@amd.com>
---
 drivers/soundwire/amd_manager.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 078e13b01ac9..9da60e8681f0 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -901,7 +901,7 @@ static int amd_sdw_port_enable(struct sdw_bus *bus,
 
 static int amd_sdw_calculate_bra_params(struct amd_sdw_manager *amd_manager,
 					struct amd_bra_params *params,
-					u8 peripheral_addr)
+					struct sdw_slave *slave)
 {
 	struct sdw_bus *bus = &amd_manager->bus;
 	u32 nr = bus->params.row;  /* current rows - NOT enlarged */
@@ -912,6 +912,7 @@ static int amd_sdw_calculate_bra_params(struct amd_sdw_manager *amd_manager,
 	u32 sample_interval;
 	u32 bits_per_frame;
 	u32 bpf;
+	u32 block_alignment = slave->prop.bra_block_alignment;
 
 	params->hstart      = hstart;
 	params->hstop       = hstop;
@@ -943,14 +944,33 @@ static int amd_sdw_calculate_bra_params(struct amd_sdw_manager *amd_manager,
 	if (bpf > 511)
 		bpf = 511;
 
+	/*
+	 * If the peripheral advertises the DisCo BRA block-alignment property
+	 * (mipi-sdw-bra-mode-block-alignment), the data payload carried in each
+	 * BRA frame must be an integer multiple of that value. Round
+	 * bytes_per_frame down to the nearest multiple; the ACP BRA engine then
+	 * packs an aligned payload per frame and simply uses more frames to
+	 * cover the same transfer_length. When the property is absent (0) the
+	 * value is left unchanged.
+	 */
+	if (block_alignment) {
+		if (bpf < block_alignment) {
+			dev_err(amd_manager->dev,
+				"BPT: bytes_per_frame %u smaller than block alignment %u\n",
+				bpf, block_alignment);
+			return -EINVAL;
+		}
+		bpf -= bpf % block_alignment;
+	}
+
 	params->sample_interval = sample_interval;
 	params->bytes_per_frame = bpf;
-	params->dev_addr        = peripheral_addr;
+	params->dev_addr        = (u8)slave->dev_num;
 
 	dev_dbg(amd_manager->dev,
 		"BPT calc_params: NR=%u NC=%u col_width=%u WL=%u SI=%u BPF=%u hstart=%u hstop=%u dev=%u\n",
 		nr, nc, col_width, params->word_length, sample_interval,
-		bpf, hstart, hstop, peripheral_addr);
+		bpf, hstart, hstop, slave->dev_num);
 	return 0;
 }
 
@@ -1356,7 +1376,7 @@ static int amd_sdw_bra_transfer(struct amd_sdw_manager *amd_manager,
 	int ret;
 
 	ret = amd_sdw_calculate_bra_params(amd_manager, &params,
-					   (u8)slave->dev_num);
+					   slave);
 	if (ret < 0)
 		return ret;
 
@@ -1780,7 +1800,7 @@ static int amd_sdw_bpt_wait(struct sdw_bus *bus,
 	 * to register read/write commands.
 	 */
 	ret = amd_sdw_calculate_bra_params(amd_manager, &prep_params,
-					   (u8)slave->dev_num);
+					   slave);
 	if (ret < 0) {
 		dev_err(amd_manager->dev,
 			"BPT: failed to calc params: %d\n", ret);
-- 
2.43.0