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

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

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.