Re: [PATCH v1 2/2] soundwire: qcom: add EE-aware register layout and cpu selection

[Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>]

On 6/9/2026 2:23 PM, Srinivas Kandagatla wrote:
> 
> 
> On 6/8/26 6:53 PM, Mohammad Rafi Shaik wrote:
>> Some Qualcomm SoundWire masters expose interrupt, FIFO and status
>> registers in EE-specific register windows on v2.0 and newer hardware.
>>
>> Add support for selecting the SoundWire execution environment from DT
>> and use it to program the correct register window for the active EE.
> 
> So this is integration details, which can be derived dynamically based
> on version or compatible.
> 
Yes, agreed.

This can be configured dynamically based on the compatible string.

I will try to add SoC-specific compatible entries and configure the EE 
accordingly.

> 
>> The driver now reads the EE value from the new
>> qcom,swr-master-ee-val property, with qcom,ee as a fallback for
>> backward compatibility.
> 
>>
>> For v2.0+ hardware, the IRQ/FIFO/status register layout is adjusted by
> Which exact version, do we have minor or step values for this?
> 
>> the EE window stride so the driver programs the correct bank for the
>> selected EE. The interrupt enable path is also updated to always use
>> the selected EE window.
>>
>> This change allows SoundWire interrupt routing and register accesses to
>> work correctly on platforms where the master is not mapped to the
>> default EE1 window.
>>
>> In Shikra, the soundwire execution environment is set to 0 unlike other
>> Qualcomm boards.
>>
> 
> Can we get this integration details dynamically.
>> Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
>> ---
>>   drivers/soundwire/qcom.c | 78 +++++++++++++++++++++++++++++++++-------
>>   1 file changed, 65 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index 3d8f5a81e..f4b8ff77b 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -26,6 +26,7 @@
>>   #define SWRM_COMP_STATUS					0x014
>>   #define SWRM_LINK_MANAGER_EE					0x018
>>   #define SWRM_EE_CPU						1
>> +#define SWRM_MAX_EE						1
> 
> Can we not make the EE_CPU dynamic and everything should fall in place?
> isn't?
> 

yes right, I will implement dynamic configuration based on the 
compatible string.

> Is the register layout changed?
> 

Layout is same logically, but access differs per EE via stride-based 
windows.

>>   #define SWRM_FRM_GEN_ENABLED					BIT(0)
>>   #define SWRM_VERSION_1_3_0					0x01030000
>>   #define SWRM_VERSION_1_5_1					0x01050001
>> @@ -118,6 +119,7 @@
>>   #define SWRM_V2_0_CLK_CTRL					0x5060
>>   #define SWRM_V2_0_CLK_CTRL_CLK_START				BIT(0)
>>   #define SWRM_V2_0_LINK_STATUS					0x5064
>> +#define SWRM_V2_REG_EE_STRIDE					0x1000
>>   
>>   #define SWRM_DP_PORT_CTRL_EN_CHAN_SHFT				0x18
>>   #define SWRM_DP_PORT_CTRL_OFFSET2_SHFT				0x10
>> @@ -202,6 +204,7 @@ struct qcom_swrm_ctrl {
>>   	struct mutex port_lock;
>>   	struct clk *hclk;
>>   	int irq;
>> +	u32 ee;
>>   	unsigned int version;
>>   	int wake_irq;
>>   	int num_din_ports;
>> @@ -222,6 +225,7 @@ struct qcom_swrm_ctrl {
>>   	u32 slave_status;
>>   	u32 wr_fifo_depth;
>>   	bool clock_stop_not_supported;
>> +	unsigned int reg_layout_local[SWRM_OFFSET_DP_SAMPLECTRL2_BANK + 1];
>>   };
>>   
>>   struct qcom_swrm_data {
>> @@ -328,6 +332,36 @@ static const struct qcom_swrm_data swrm_v3_0_data = {
>>   };
>>   #define to_qcom_sdw(b)	container_of(b, struct qcom_swrm_ctrl, bus)
>>   
>> +static void qcom_swrm_set_ee_register_layout(struct qcom_swrm_ctrl *ctrl,
>> +					     const struct qcom_swrm_data *data)
>> +{
>> +	int ee_offset;
>> +
>> +	memcpy(ctrl->reg_layout_local, data->reg_layout,
>> +	       sizeof(ctrl->reg_layout_local));
>> +	ctrl->reg_layout = ctrl->reg_layout_local;
>> +
>> +	if (ctrl->version < SWRM_VERSION_2_0_0)
>> +		return;
>> +
>> +	/*
>> +	 * Current register constants map EE1. For EE0, use the EE register
>> +	 * window stride to access status/IRQ/FIFO registers.
>> +	 */
>> +	ee_offset = ((int)ctrl->ee - SWRM_EE_CPU) * SWRM_V2_REG_EE_STRIDE;
>> +	if (!ee_offset)
>> +		return;
>> +
>> +	ctrl->reg_layout_local[SWRM_REG_FRAME_GEN_ENABLED] += ee_offset;
>> +	ctrl->reg_layout_local[SWRM_REG_INTERRUPT_STATUS] += ee_offset;
>> +	ctrl->reg_layout_local[SWRM_REG_INTERRUPT_CLEAR] += ee_offset;
>> +	ctrl->reg_layout_local[SWRM_REG_INTERRUPT_CPU_EN] += ee_offset;
>> +	ctrl->reg_layout_local[SWRM_REG_CMD_FIFO_WR_CMD] += ee_offset;
>> +	ctrl->reg_layout_local[SWRM_REG_CMD_FIFO_RD_CMD] += ee_offset;
>> +	ctrl->reg_layout_local[SWRM_REG_CMD_FIFO_STATUS] += ee_offset;
>> +	ctrl->reg_layout_local[SWRM_REG_CMD_FIFO_RD_FIFO_ADDR] += ee_offset;
> 
> Why not make these registers take the ee value rather then doing this way?
> 

Will review and address in the next revision.

Thanks,
Rafi.

> 
>> +}
>> +
>>   static int qcom_swrm_ahb_reg_read(struct qcom_swrm_ctrl *ctrl, int reg,
>>   				  u32 *val)
>>   {
>> @@ -904,12 +938,13 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>>   	ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
>>   
>>   	if (ctrl->version == SWRM_VERSION_1_7_0) {
>> -		ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, SWRM_EE_CPU);
>> +		ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, ctrl->ee);
>>   		ctrl->reg_write(ctrl, SWRM_MCP_BUS_CTRL,
>> -				SWRM_MCP_BUS_CLK_START << SWRM_EE_CPU);
>> +				SWRM_MCP_BUS_CLK_START << ctrl->ee);
>>   	} else if (ctrl->version >= SWRM_VERSION_2_0_0) {
>> -		ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, SWRM_EE_CPU);
>> -		ctrl->reg_write(ctrl, SWRM_V2_0_CLK_CTRL,
>> +		ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, ctrl->ee);
>> +		ctrl->reg_write(ctrl, SWRM_V2_0_CLK_CTRL +
>> +				((int)ctrl->ee - SWRM_EE_CPU) * SWRM_V2_REG_EE_STRIDE,
>>   				SWRM_V2_0_CLK_CTRL_CLK_START);
>>   	} else {
>>   		ctrl->reg_write(ctrl, SWRM_MCP_BUS_CTRL, SWRM_MCP_BUS_CLK_START);
>> @@ -935,11 +970,9 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>>   	ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CLEAR],
>>   			0xFFFFFFFF);
>>   
>> -	/* enable CPU IRQs */
>> -	if (ctrl->mmio) {
>> -		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
>> -				SWRM_INTERRUPT_STATUS_RMSK);
>> -	}
>> +	/* enable CPU IRQs for the selected EE window */
>> +	ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
>> +			SWRM_INTERRUPT_STATUS_RMSK);
>>   
>>   	/* Set IRQ to PULSE */
>>   	ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
>> @@ -1545,7 +1578,22 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>>   		return -ENOMEM;
>>   
>>   	data = of_device_get_match_data(dev);
>> +	ctrl->ee = SWRM_EE_CPU;
>> +	ret = of_property_read_u32(dev->of_node, "qcom,swr-master-ee-val", &ctrl->ee);
>> +	if (ret)
>> +		ret = of_property_read_u32(dev->of_node, "qcom,ee", &ctrl->ee);
>> +	if (ret)
>> +		ctrl->ee = SWRM_EE_CPU;
>> +	if (ctrl->ee > SWRM_MAX_EE) {
>> +		dev_warn(dev, "invalid SoundWire EE %u, using EE%u\n",
>> +			 ctrl->ee, SWRM_EE_CPU);
>> +		ctrl->ee = SWRM_EE_CPU;
>> +	}
>>   	ctrl->max_reg = data->max_reg;
>> +	/*
>> +	 * Defer EE register window selection until HW version is known.
>> +	 * For v2.0+ the IRQ/FIFO window is EE-banked.
>> +	 */
>>   	ctrl->reg_layout = data->reg_layout;
>>   	ctrl->rows_index = sdw_find_row_index(data->default_rows);
>>   	ctrl->cols_index = sdw_find_col_index(data->default_cols);
>> @@ -1623,6 +1671,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>>   	prop->default_row = data->default_rows;
>>   
>>   	ctrl->reg_read(ctrl, SWRM_COMP_HW_VERSION, &ctrl->version);
>> +	qcom_swrm_set_ee_register_layout(ctrl, data);
>>   
>>   	ret = devm_request_threaded_irq(dev, ctrl->irq, NULL,
>>   					qcom_swrm_irq_handler,
>> @@ -1733,16 +1782,19 @@ static int __maybe_unused swrm_runtime_resume(struct device *dev)
>>   		reset_control_reset(ctrl->audio_cgcr);
>>   
>>   		if (ctrl->version == SWRM_VERSION_1_7_0) {
>> -			ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, SWRM_EE_CPU);
>> +			ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, ctrl->ee);
>>   			ctrl->reg_write(ctrl, SWRM_MCP_BUS_CTRL,
>> -					SWRM_MCP_BUS_CLK_START << SWRM_EE_CPU);
>> +					SWRM_MCP_BUS_CLK_START << ctrl->ee);
>>   		} else if (ctrl->version >= SWRM_VERSION_2_0_0) {
>> -			ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, SWRM_EE_CPU);
>> -			ctrl->reg_write(ctrl, SWRM_V2_0_CLK_CTRL,
>> +			ctrl->reg_write(ctrl, SWRM_LINK_MANAGER_EE, ctrl->ee);
>> +			ctrl->reg_write(ctrl, SWRM_V2_0_CLK_CTRL +
>> +					((int)ctrl->ee - SWRM_EE_CPU) *
>> +					SWRM_V2_REG_EE_STRIDE,
>>   					SWRM_V2_0_CLK_CTRL_CLK_START);
>>   		} else {
>>   			ctrl->reg_write(ctrl, SWRM_MCP_BUS_CTRL, SWRM_MCP_BUS_CLK_START);
>>   		}
>> +
>>   		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CLEAR],
>>   			SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET);
>>   
>