Re: [PATCH 2/2] soundwire: amd: refactor bandwidth calculaiton logic

[Mario Limonciello <superm1@kernel.org>]

On 12/22/25 1:48 AM, Vijendar Mukunda wrote:
> For ACP6.3/7.0/7.1/7.2 platforms, amd SoundWire manager doesn't have
> banked registers concept. For bandwidth calculation, need to use static
> mapping for block offset calculation based on master port request.
> Refactor bandwidth calculation logic to support 6Mhz bus clock frequency
> with frame size as 50 x 10, 125 x 2 and 12Mhz bus clock frequency with
> frame size as 50 x 10 based on static port block offset logic.
> 
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> ---

You have a typo in the subject "calculaiton"

One more minor comment below. With those fixed you can add to next version:

Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>

>   drivers/soundwire/amd_manager.c   | 52 ++++++++++++++++++++++++++++---
>   include/linux/soundwire/sdw_amd.h |  4 +++
>   2 files changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index ee3c37a5a48b..4e4fd4c31019 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -480,27 +480,66 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
>   
>   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_bus_params *b_params = &bus->params;
>   	int port_bo, hstart, hstop, sample_int;
> -	unsigned int rate, bps;
> +	unsigned int rate, bps, channels;
> +	int stream_slot_size, max_slots;
> +	static int next_offset[AMD_SDW_MAX_MANAGER_COUNT] = {0};
> +	unsigned int inst_id = amd_manager->instance;
>   
>   	port_bo = 0;
>   	hstart = 1;
>   	hstop = bus->params.col - 1;
>   	t_data.hstop = hstop;
>   	t_data.hstart = hstart;
> +	if (next_offset[inst_id] == 0)
> +		next_offset[inst_id] = 1;

This seems like pointless check no?
You initialized

 > +	static int next_offset[AMD_SDW_MAX_MANAGER_COUNT] = {0};

So all offsets will be 0.  Instead this can just be:

	next_offset[inst_id] = 1;

>   
>   	list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
>   		rate = m_rt->stream->params.rate;
>   		bps = m_rt->stream->params.bps;
> +		channels = m_rt->stream->params.ch_count;
>   		sample_int = (bus->params.curr_dr_freq / rate);
> +
> +		/* Compute slots required for this stream dynamically */
> +		stream_slot_size = bps * channels;
> +
>   		list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
> -			port_bo = (p_rt->num * 64) + 1;
> -			dev_dbg(bus->dev, "p_rt->num=%d hstart=%d hstop=%d port_bo=%d\n",
> -				p_rt->num, hstart, hstop, port_bo);
> +			if (p_rt->num >= amd_manager->max_ports) {
> +				dev_err(bus->dev, "Port %d exceeds max ports %d\n",
> +					p_rt->num, amd_manager->max_ports);
> +				return -EINVAL;
> +			}
> +
> +			/* Static mapping logic */
> +			if (!amd_manager->port_offset_map[p_rt->num]) {
> +				if (bus->params.curr_dr_freq == 12000000) {
> +					max_slots = bus->params.row * (bus->params.col - 1);
> +					if (next_offset[inst_id] + stream_slot_size <=
> +					    (max_slots - 1)) {
> +						amd_manager->port_offset_map[p_rt->num] =
> +									next_offset[inst_id];
> +						next_offset[inst_id] += stream_slot_size;
> +					} else {
> +						dev_err(bus->dev,
> +							"No space for port %d\n", p_rt->num);
> +						return -ENOMEM;
> +					}
> +				} else {
> +					amd_manager->port_offset_map[p_rt->num] =
> +									(p_rt->num * 64) + 1;
> +				}
> +			}
> +			port_bo = amd_manager->port_offset_map[p_rt->num];
> +			dev_dbg(bus->dev,
> +				"Port=%d hstart=%d hstop=%d port_bo=%d slots=%d max_ports=%d\n",
> +				p_rt->num, hstart, hstop, port_bo, stream_slot_size,
> +				amd_manager->max_ports);
> +
>   			sdw_fill_xport_params(&p_rt->transport_params, p_rt->num,
>   					      false, SDW_BLK_GRP_CNT_1, sample_int,
>   					      port_bo, port_bo >> 8, hstart, hstop,
> @@ -1093,6 +1132,11 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)
>   	default:
>   		return -EINVAL;
>   	}
> +	amd_manager->max_ports = amd_manager->num_dout_ports + amd_manager->num_din_ports;
> +	amd_manager->port_offset_map = devm_kcalloc(dev, amd_manager->max_ports,
> +						    sizeof(int), GFP_KERNEL);
> +	if (!amd_manager->port_offset_map)
> +		return -ENOMEM;
>   
>   	prop = &amd_manager->bus.prop;
>   	prop->mclk_freq = AMD_SDW_BUS_BASE_FREQ;
> diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
> index fe31773d5210..470360a2723c 100644
> --- a/include/linux/soundwire/sdw_amd.h
> +++ b/include/linux/soundwire/sdw_amd.h
> @@ -66,8 +66,10 @@ struct sdw_amd_dai_runtime {
>    * @status: peripheral devices status array
>    * @num_din_ports: number of input ports
>    * @num_dout_ports: number of output ports
> + * @max_ports: total number of input ports and output ports
>    * @cols_index: Column index in frame shape
>    * @rows_index: Rows index in frame shape
> + * @port_offset_map: dynamic array to map port block offset
>    * @instance: SoundWire manager instance
>    * @quirks: SoundWire manager quirks
>    * @wake_en_mask: wake enable mask per SoundWire manager
> @@ -92,10 +94,12 @@ struct amd_sdw_manager {
>   
>   	int num_din_ports;
>   	int num_dout_ports;
> +	int max_ports;
>   
>   	int cols_index;
>   	int rows_index;
>   
> +	int *port_offset_map;
>   	u32 instance;
>   	u32 quirks;
>   	u32 wake_en_mask;