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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

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