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

[Syed Saba Kareem <syed.sabakareem@amd.com>]

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