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

["Simon Trimmer" <simont@opensource.cirrus.com>]

Hi Syed,
I'll look to try this out this week!

The code has changed a bit since I last looked at it properly, I had a query in amd_sdw_bpt_wait() and I think this may be missing bra_block_alignment support that recently entered the tree in  https://lore.kernel.org/all/20260728124639.1484973-3-yung-chuan.liao@linux.intel.com/

commit c0840f8be5d59071096b8e6b42693a0d912b7cba
Author: Richard Fitzgerald <rf@opensource.cirrus.com>
Date:   Tue Jul 28 20:46:36 2026 +0800

    soundwire: Add bra_block_alignment property support

    Add a property to struct sdw_slave_prop equivalent to the Disco
    property "mipi-sdw-bra-mode-block-alignment".

    The SoundWire Disco specification defines this as:

    "The data payload size for this BRA Mode shall be an integer
    multiple of the value of this Property."

    Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
    Co-developed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
    Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
    Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
    Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>
    Link: https://patch.msgid.link/20260728124639.1484973-3-yung-chuan.liao@linux.intel.com
    Signed-off-by: Vinod Koul <vkoul@kernel.org>

Thanks,
-Simon

On 9/9/26 13:56, Syed Saba Kareem wrote:
> ...
> - 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.

Query about handling of contiguous small sections

> ...
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> .....
> +static int amd_sdw_bpt_wait(struct sdw_bus *bus,
> +			    struct sdw_slave *slave,
> +			    struct sdw_bpt_msg *msg)
> +{
> ...
> +	 * 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)) {

Have we dropped handling of when a contiguous transfer is less than bytes_per_frame like in the non-contiguous case?

> +		/*
> +		 * All sections are contiguous in peripheral address space.
> +		 * A single BRA call covers the entire firmware image.
> +		 */
> +		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)
> ...