[PATCH 0/5] soundwire: qcom: stablity fixes

[PATCH 0/5] soundwire: qcom: stablity fixes

[Srinivas Kandagatla]

During x13s audio testing we hit few corner cases due to issues
in codec drivers and some obvious code bugs.

Here are the fixes for those issues, mostly the issues are around
devices loosing the sync in between runtime pm suspend resume path.

With codec fixes along with these fixes, audio on x13s is pretty stable.

Thanks,
Srini

Srinivas Kandagatla (5):
  soundwire: qcom: update status correctly with mask
  soundwire: qcom: enable runtime pm before controller is registered
  soundwire: qcom: wait for fifo to be empty before suspend
  soundwire: qcom: add software workaround for bus clash interrupt
    assertion
  soundwire: qcom: set clk stop need reset flag at runtime

 drivers/soundwire/qcom.c | 111 +++++++++++++++++++++++++--------------
 1 file changed, 73 insertions(+), 38 deletions(-)

-- 
2.21.0

[PATCH 1/5] soundwire: qcom: update status correctly with mask

[Srinivas Kandagatla]

SoundWire device status can be incorrectly updated without
proper mask, fix this by adding a mask before updating the status.

Fixes: c7d49c76d1d5 ("soundwire: qcom: add support to new interrupts")
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 335424870290..9d8ae77bad0a 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -434,7 +434,7 @@ static int qcom_swrm_get_alert_slave_dev_num(struct qcom_swrm_ctrl *ctrl)
 		status = (val >> (dev_num * SWRM_MCP_SLV_STATUS_SZ));
 
 		if ((status & SWRM_MCP_SLV_STATUS_MASK) == SDW_SLAVE_ALERT) {
-			ctrl->status[dev_num] = status;
+			ctrl->status[dev_num] = status & SWRM_MCP_SLV_STATUS_MASK;
 			return dev_num;
 		}
 	}
-- 
2.21.0

[PATCH 2/5] soundwire: qcom: enable runtime pm before controller is registered

[Srinivas Kandagatla]

Registering controller even before pm runtime is enabled will result
in pm runtime underflow warnings. Fix this by properly moving
the runtime pm enable before registering controller.

Fixes: 74e79da9fd46 ("soundwire: qcom: add runtime pm support")
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/qcom.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 9d8ae77bad0a..b2363839624c 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -1417,6 +1417,12 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 		}
 	}
 
+	pm_runtime_set_autosuspend_delay(dev, 3000);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
 	if (ret) {
 		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
@@ -1435,12 +1441,6 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
 		 ctrl->version & 0xffff);
 
-	pm_runtime_set_autosuspend_delay(dev, 3000);
-	pm_runtime_use_autosuspend(dev);
-	pm_runtime_mark_last_busy(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-
 	/* Clk stop is not supported on WSA Soundwire masters */
 	if (ctrl->version <= 0x01030000) {
 		ctrl->clock_stop_not_supported = true;
-- 
2.21.0

[PATCH 3/5] soundwire: qcom: wait for fifo to be empty before suspend

[Srinivas Kandagatla]

Wait for Fifo to be empty before going to suspend or before bank
switch happens. Just to make sure that all the reads/writes are done.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/qcom.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index b2363839624c..465b2a2ef0d5 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -325,6 +325,32 @@ static int swrm_wait_for_wr_fifo_avail(struct qcom_swrm_ctrl *swrm)
 	return 0;
 }
 
+static bool swrm_wait_for_wr_fifo_done(struct qcom_swrm_ctrl *swrm)
+{
+	u32 fifo_outstanding_cmds, value;
+	int fifo_retry_count = SWR_OVERFLOW_RETRY_COUNT;
+
+	/* Check for fifo overflow during write */
+	swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
+	fifo_outstanding_cmds = FIELD_GET(SWRM_WR_CMD_FIFO_CNT_MASK, value);
+
+	if (fifo_outstanding_cmds) {
+		while (fifo_retry_count) {
+			usleep_range(500, 510);
+			swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
+			fifo_outstanding_cmds = FIELD_GET(SWRM_WR_CMD_FIFO_CNT_MASK, value);
+			fifo_retry_count--;
+			if (fifo_outstanding_cmds == 0)
+				return true;
+		}
+	} else {
+		return true;
+	}
+
+
+	return false;
+}
+
 static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *swrm, u8 cmd_data,
 				     u8 dev_addr, u16 reg_addr)
 {
@@ -356,6 +382,7 @@ static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *swrm, u8 cmd_data,
 		usleep_range(150, 155);
 
 	if (cmd_id == SWR_BROADCAST_CMD_ID) {
+		swrm_wait_for_wr_fifo_done(swrm);
 		/*
 		 * sleep for 10ms for MSM soundwire variant to allow broadcast
 		 * command to complete.
@@ -1122,6 +1149,7 @@ static void qcom_swrm_shutdown(struct snd_pcm_substream *substream,
 {
 	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
 
+	swrm_wait_for_wr_fifo_done(ctrl);
 	sdw_release_stream(ctrl->sruntime[dai->id]);
 	ctrl->sruntime[dai->id] = NULL;
 	pm_runtime_mark_last_busy(ctrl->dev);
@@ -1558,6 +1586,7 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev)
 	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev);
 	int ret;
 
+	swrm_wait_for_wr_fifo_done(ctrl);
 	if (!ctrl->clock_stop_not_supported) {
 		/* Mask bus clash interrupt */
 		ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
-- 
2.21.0

[PATCH 4/5] soundwire: qcom: add software workaround for bus clash interrupt assertion

[Srinivas Kandagatla]

Sometimes Hard reset does not clear some of the registers,
this sometimes results in firing a bus clash interrupt.
Add workaround for this during power up sequence, as
suggested by hardware manual.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/qcom.c | 55 ++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 465b2a2ef0d5..74e38c0d651b 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -697,6 +697,26 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 	return ret;
 }
 
+static bool swrm_wait_for_frame_gen_enabled(struct qcom_swrm_ctrl *swrm)
+{
+	int retry = SWRM_LINK_STATUS_RETRY_CNT;
+	int comp_sts;
+
+	do {
+		swrm->reg_read(swrm, SWRM_COMP_STATUS, &comp_sts);
+
+		if (comp_sts & SWRM_FRM_GEN_ENABLED)
+			return true;
+
+		usleep_range(500, 510);
+	} while (retry--);
+
+	dev_err(swrm->dev, "%s: link status %s\n", __func__,
+		comp_sts & SWRM_FRM_GEN_ENABLED ? "connected" : "disconnected");
+
+	return false;
+}
+
 static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
 {
 	u32 val;
@@ -741,16 +761,27 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
 				SWRM_RD_WR_CMD_RETRIES);
 	}
 
+	/* COMP Enable */
+	ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR, SWRM_COMP_CFG_ENABLE_MSK);
+
 	/* Set IRQ to PULSE */
 	ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
-			SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK |
-			SWRM_COMP_CFG_ENABLE_MSK);
+			SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK);
+
+	ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, 0xFFFFFFFF);
 
 	/* enable CPU IRQs */
 	if (ctrl->mmio) {
 		ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN,
 				SWRM_INTERRUPT_STATUS_RMSK);
 	}
+
+	/* Set IRQ to PULSE */
+	ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
+			SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK |
+			SWRM_COMP_CFG_ENABLE_MSK);
+
+	swrm_wait_for_frame_gen_enabled(ctrl);
 	ctrl->slave_status = 0;
 	ctrl->reg_read(ctrl, SWRM_COMP_PARAMS, &val);
 	ctrl->rd_fifo_depth = FIELD_GET(SWRM_COMP_PARAMS_RD_FIFO_DEPTH, val);
@@ -1504,26 +1535,6 @@ static int qcom_swrm_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static bool swrm_wait_for_frame_gen_enabled(struct qcom_swrm_ctrl *swrm)
-{
-	int retry = SWRM_LINK_STATUS_RETRY_CNT;
-	int comp_sts;
-
-	do {
-		swrm->reg_read(swrm, SWRM_COMP_STATUS, &comp_sts);
-
-		if (comp_sts & SWRM_FRM_GEN_ENABLED)
-			return true;
-
-		usleep_range(500, 510);
-	} while (retry--);
-
-	dev_err(swrm->dev, "%s: link status not %s\n", __func__,
-		comp_sts & SWRM_FRM_GEN_ENABLED ? "connected" : "disconnected");
-
-	return false;
-}
-
 static int __maybe_unused swrm_runtime_resume(struct device *dev)
 {
 	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev);
-- 
2.21.0

[PATCH 5/5] soundwire: qcom: set clk stop need reset flag at runtime

[Srinivas Kandagatla]

WSA Soundwire controller needs an full reset if clock stop support
is not available in slave devices. WSA881x does not support clock stop
however WSA883x supports clock stop.

Make setting this flag at runtime to address above issue.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/qcom.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 74e38c0d651b..0224a5a866de 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -536,10 +536,14 @@ static int qcom_swrm_enumerate(struct sdw_bus *bus)
 
 		sdw_extract_slave_id(bus, addr, &id);
 		found = false;
+		ctrl->clock_stop_not_supported = false;
 		/* Now compare with entries */
 		list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
 			if (sdw_compare_devid(slave, id) == 0) {
 				qcom_swrm_set_slave_dev_num(bus, slave, i);
+				if (!slave->prop.simple_clk_stop_capable)
+					ctrl->clock_stop_not_supported = true;
+
 				found = true;
 				break;
 			}
@@ -1500,15 +1504,6 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
 		 ctrl->version & 0xffff);
 
-	/* Clk stop is not supported on WSA Soundwire masters */
-	if (ctrl->version <= 0x01030000) {
-		ctrl->clock_stop_not_supported = true;
-	} else {
-		ctrl->reg_read(ctrl, SWRM_COMP_MASTER_ID, &val);
-		if (val == MASTER_ID_WSA)
-			ctrl->clock_stop_not_supported = true;
-	}
-
 #ifdef CONFIG_DEBUG_FS
 	ctrl->debugfs = debugfs_create_dir("qualcomm-sdw", ctrl->bus.debugfs);
 	debugfs_create_file("qualcomm-registers", 0400, ctrl->debugfs, ctrl,
-- 
2.21.0

Re: [PATCH 2/5] soundwire: qcom: enable runtime pm before controller is registered

[Pierre-Louis Bossart]

On 2/9/23 07:13, Srinivas Kandagatla wrote:
> Registering controller even before pm runtime is enabled will result
> in pm runtime underflow warnings. Fix this by properly moving
> the runtime pm enable before registering controller.

That seems very odd. The Intel code configures the pm_runtime stuff
*after* the call to sdw_bus_master_add(), and we've not seen any
underflow warnings? We even configure pm_runtime after starting the bus.
Likewise for peripherals the pm_runtime part is enabled after the device
is initialized.

Not following the problem and suggested solution.

> Fixes: 74e79da9fd46 ("soundwire: qcom: add runtime pm support")
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/soundwire/qcom.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 9d8ae77bad0a..b2363839624c 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -1417,6 +1417,12 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	pm_runtime_set_autosuspend_delay(dev, 3000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
>  	ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
>  	if (ret) {
>  		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
> @@ -1435,12 +1441,6 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
>  		 ctrl->version & 0xffff);
>  
> -	pm_runtime_set_autosuspend_delay(dev, 3000);
> -	pm_runtime_use_autosuspend(dev);
> -	pm_runtime_mark_last_busy(dev);
> -	pm_runtime_set_active(dev);
> -	pm_runtime_enable(dev);
> -
>  	/* Clk stop is not supported on WSA Soundwire masters */
>  	if (ctrl->version <= 0x01030000) {
>  		ctrl->clock_stop_not_supported = true;

Re: [PATCH 3/5] soundwire: qcom: wait for fifo to be empty before suspend

[Pierre-Louis Bossart]

On 2/9/23 07:13, Srinivas Kandagatla wrote:
> Wait for Fifo to be empty before going to suspend or before bank
> switch happens. Just to make sure that all the reads/writes are done.

For the suspend case that seems like a valid approach, but for bank
switch don't we already have a bus->msg_lock mutex that will prevent the
bank switch command from being sent before the other commands are handled?

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/soundwire/qcom.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index b2363839624c..465b2a2ef0d5 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -325,6 +325,32 @@ static int swrm_wait_for_wr_fifo_avail(struct qcom_swrm_ctrl *swrm)
>  	return 0;
>  }
>  
> +static bool swrm_wait_for_wr_fifo_done(struct qcom_swrm_ctrl *swrm)
> +{
> +	u32 fifo_outstanding_cmds, value;
> +	int fifo_retry_count = SWR_OVERFLOW_RETRY_COUNT;
> +
> +	/* Check for fifo overflow during write */
> +	swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
> +	fifo_outstanding_cmds = FIELD_GET(SWRM_WR_CMD_FIFO_CNT_MASK, value);
> +
> +	if (fifo_outstanding_cmds) {
> +		while (fifo_retry_count) {
> +			usleep_range(500, 510);
> +			swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
> +			fifo_outstanding_cmds = FIELD_GET(SWRM_WR_CMD_FIFO_CNT_MASK, value);
> +			fifo_retry_count--;
> +			if (fifo_outstanding_cmds == 0)
> +				return true;
> +		}
> +	} else {
> +		return true;
> +	}
> +
> +
> +	return false;
> +}
> +
>  static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *swrm, u8 cmd_data,
>  				     u8 dev_addr, u16 reg_addr)
>  {
> @@ -356,6 +382,7 @@ static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *swrm, u8 cmd_data,
>  		usleep_range(150, 155);
>  
>  	if (cmd_id == SWR_BROADCAST_CMD_ID) {
> +		swrm_wait_for_wr_fifo_done(swrm);
>  		/*
>  		 * sleep for 10ms for MSM soundwire variant to allow broadcast
>  		 * command to complete.
> @@ -1122,6 +1149,7 @@ static void qcom_swrm_shutdown(struct snd_pcm_substream *substream,
>  {
>  	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
>  
> +	swrm_wait_for_wr_fifo_done(ctrl);
>  	sdw_release_stream(ctrl->sruntime[dai->id]);
>  	ctrl->sruntime[dai->id] = NULL;
>  	pm_runtime_mark_last_busy(ctrl->dev);
> @@ -1558,6 +1586,7 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev)
>  	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev);
>  	int ret;
>  
> +	swrm_wait_for_wr_fifo_done(ctrl);
>  	if (!ctrl->clock_stop_not_supported) {
>  		/* Mask bus clash interrupt */
>  		ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;

Re: [PATCH 5/5] soundwire: qcom: set clk stop need reset flag at runtime

[Pierre-Louis Bossart]

On 2/9/23 07:13, Srinivas Kandagatla wrote:
> WSA Soundwire controller needs an full reset if clock stop support
> is not available in slave devices. WSA881x does not support clock stop
> however WSA883x supports clock stop.
> 
> Make setting this flag at runtime to address above issue.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/soundwire/qcom.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 74e38c0d651b..0224a5a866de 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -536,10 +536,14 @@ static int qcom_swrm_enumerate(struct sdw_bus *bus)
>  
>  		sdw_extract_slave_id(bus, addr, &id);
>  		found = false;
> +		ctrl->clock_stop_not_supported = false;
>  		/* Now compare with entries */
>  		list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
>  			if (sdw_compare_devid(slave, id) == 0) {
>  				qcom_swrm_set_slave_dev_num(bus, slave, i);
> +				if (!slave->prop.simple_clk_stop_capable)
> +					ctrl->clock_stop_not_supported = true;

IIRC the 'simple_clk_stop_capable' for a peripheral refers to the
Simplified_SCSP_SM

see Figure 35 "Slave Clock Stop Prepare State Machine (SCSP_SM)"

In addition, there's a requirement that all peripherals shall support
ClockStopMode0. Only ClockStopMode1 is optional, and that case is
handled with a different property:

 * @clk_stop_mode1: Clock-Stop Mode 1 is supported

I think you overloaded one concept with another, or used the wrong property?

> +
>  				found = true;
>  				break;
>  			}
> @@ -1500,15 +1504,6 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
>  		 ctrl->version & 0xffff);
>  
> -	/* Clk stop is not supported on WSA Soundwire masters */
> -	if (ctrl->version <= 0x01030000) {
> -		ctrl->clock_stop_not_supported = true;
> -	} else {
> -		ctrl->reg_read(ctrl, SWRM_COMP_MASTER_ID, &val);
> -		if (val == MASTER_ID_WSA)
> -			ctrl->clock_stop_not_supported = true;
> -	}
> -
>  #ifdef CONFIG_DEBUG_FS
>  	ctrl->debugfs = debugfs_create_dir("qualcomm-sdw", ctrl->bus.debugfs);
>  	debugfs_create_file("qualcomm-registers", 0400, ctrl->debugfs, ctrl,

Re: [PATCH 2/5] soundwire: qcom: enable runtime pm before controller is registered

[Srinivas Kandagatla]

On 09/02/2023 15:21, Pierre-Louis Bossart wrote:
> 
> 
> On 2/9/23 07:13, Srinivas Kandagatla wrote:
>> Registering controller even before pm runtime is enabled will result
>> in pm runtime underflow warnings. Fix this by properly moving
>> the runtime pm enable before registering controller.
> 
> That seems very odd. The Intel code configures the pm_runtime stuff
> *after* the call to sdw_bus_master_add(), and we've not seen any
> underflow warnings? We even configure pm_runtime after starting the bus.
> Likewise for peripherals the pm_runtime part is enabled after the device
> is initialized.
> 
This was very random during bootup, Let me see if I can collect a back 
trace after reverting this patch..

--srini

> Not following the problem and suggested solution.
> 
>> Fixes: 74e79da9fd46 ("soundwire: qcom: add runtime pm support")
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/soundwire/qcom.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index 9d8ae77bad0a..b2363839624c 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -1417,6 +1417,12 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>   
>> +	pm_runtime_set_autosuspend_delay(dev, 3000);
>> +	pm_runtime_use_autosuspend(dev);
>> +	pm_runtime_mark_last_busy(dev);
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
>> +
>>   	ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
>>   	if (ret) {
>>   		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
>> @@ -1435,12 +1441,6 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>>   		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
>>   		 ctrl->version & 0xffff);
>>   
>> -	pm_runtime_set_autosuspend_delay(dev, 3000);
>> -	pm_runtime_use_autosuspend(dev);
>> -	pm_runtime_mark_last_busy(dev);
>> -	pm_runtime_set_active(dev);
>> -	pm_runtime_enable(dev);
>> -
>>   	/* Clk stop is not supported on WSA Soundwire masters */
>>   	if (ctrl->version <= 0x01030000) {
>>   		ctrl->clock_stop_not_supported = true;

Re: [PATCH 3/5] soundwire: qcom: wait for fifo to be empty before suspend

[Srinivas Kandagatla]

On 09/02/2023 15:23, Pierre-Louis Bossart wrote:
> 
> 
> On 2/9/23 07:13, Srinivas Kandagatla wrote:
>> Wait for Fifo to be empty before going to suspend or before bank
>> switch happens. Just to make sure that all the reads/writes are done.
> 
> For the suspend case that seems like a valid approach, but for bank
> switch don't we already have a bus->msg_lock mutex that will prevent the
> bank switch command from being sent before the other commands are handled?

All read/writes are fifo based, so writes could be still pending.

--srini
> 
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/soundwire/qcom.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index b2363839624c..465b2a2ef0d5 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -325,6 +325,32 @@ static int swrm_wait_for_wr_fifo_avail(struct qcom_swrm_ctrl *swrm)
>>   	return 0;
>>   }
>>   
>> +static bool swrm_wait_for_wr_fifo_done(struct qcom_swrm_ctrl *swrm)
>> +{
>> +	u32 fifo_outstanding_cmds, value;
>> +	int fifo_retry_count = SWR_OVERFLOW_RETRY_COUNT;
>> +
>> +	/* Check for fifo overflow during write */
>> +	swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
>> +	fifo_outstanding_cmds = FIELD_GET(SWRM_WR_CMD_FIFO_CNT_MASK, value);
>> +
>> +	if (fifo_outstanding_cmds) {
>> +		while (fifo_retry_count) {
>> +			usleep_range(500, 510);
>> +			swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, &value);
>> +			fifo_outstanding_cmds = FIELD_GET(SWRM_WR_CMD_FIFO_CNT_MASK, value);
>> +			fifo_retry_count--;
>> +			if (fifo_outstanding_cmds == 0)
>> +				return true;
>> +		}
>> +	} else {
>> +		return true;
>> +	}
>> +
>> +
>> +	return false;
>> +}
>> +
>>   static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *swrm, u8 cmd_data,
>>   				     u8 dev_addr, u16 reg_addr)
>>   {
>> @@ -356,6 +382,7 @@ static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *swrm, u8 cmd_data,
>>   		usleep_range(150, 155);
>>   
>>   	if (cmd_id == SWR_BROADCAST_CMD_ID) {
>> +		swrm_wait_for_wr_fifo_done(swrm);
>>   		/*
>>   		 * sleep for 10ms for MSM soundwire variant to allow broadcast
>>   		 * command to complete.
>> @@ -1122,6 +1149,7 @@ static void qcom_swrm_shutdown(struct snd_pcm_substream *substream,
>>   {
>>   	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
>>   
>> +	swrm_wait_for_wr_fifo_done(ctrl);
>>   	sdw_release_stream(ctrl->sruntime[dai->id]);
>>   	ctrl->sruntime[dai->id] = NULL;
>>   	pm_runtime_mark_last_busy(ctrl->dev);
>> @@ -1558,6 +1586,7 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev)
>>   	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev);
>>   	int ret;
>>   
>> +	swrm_wait_for_wr_fifo_done(ctrl);
>>   	if (!ctrl->clock_stop_not_supported) {
>>   		/* Mask bus clash interrupt */
>>   		ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;

Re: [PATCH 3/5] soundwire: qcom: wait for fifo to be empty before suspend

[Pierre-Louis Bossart]

On 2/9/23 09:52, Srinivas Kandagatla wrote:
> 
> 
> On 09/02/2023 15:23, Pierre-Louis Bossart wrote:
>>
>>
>> On 2/9/23 07:13, Srinivas Kandagatla wrote:
>>> Wait for Fifo to be empty before going to suspend or before bank
>>> switch happens. Just to make sure that all the reads/writes are done.
>>
>> For the suspend case that seems like a valid approach, but for bank
>> switch don't we already have a bus->msg_lock mutex that will prevent the
>> bank switch command from being sent before the other commands are
>> handled?
> 
> All read/writes are fifo based, so writes could be still pending.

I am not following. The bank switch happens with this function, where a
mutex is taken.

int sdw_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
{
	int ret;

	mutex_lock(&bus->msg_lock);

	ret = sdw_transfer_unlocked(bus, msg);

	mutex_unlock(&bus->msg_lock);

	return ret;
}

The transfer_unlocked is synchronous and waits for the command response
to be available.

In other words, there's both a mutual exclusion and a synchronous
behavior, so not sure how commands *before* the bank switch could be
pending?

Re: [PATCH 3/5] soundwire: qcom: wait for fifo to be empty before suspend

[Srinivas Kandagatla]

On 09/02/2023 16:33, Pierre-Louis Bossart wrote:
> 
> 
> On 2/9/23 09:52, Srinivas Kandagatla wrote:
>>
>>
>> On 09/02/2023 15:23, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 2/9/23 07:13, Srinivas Kandagatla wrote:
>>>> Wait for Fifo to be empty before going to suspend or before bank
>>>> switch happens. Just to make sure that all the reads/writes are done.
>>>
>>> For the suspend case that seems like a valid approach, but for bank
>>> switch don't we already have a bus->msg_lock mutex that will prevent the
>>> bank switch command from being sent before the other commands are
>>> handled?
>>
>> All read/writes are fifo based, so writes could be still pending.
> 
> I am not following. The bank switch happens with this function, where a
> mutex is taken.
> 
> int sdw_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
> {
> 	int ret;
> 
> 	mutex_lock(&bus->msg_lock);
> 
> 	ret = sdw_transfer_unlocked(bus, msg);

Qualcomm controller uses fifo to read/write, so return after writing to 
fifo might not always indicate that write is completed.

Qcom Soundwire controller do not have any synchronous interrupt 
mechanism to indicate write complete.

--srini


> 
> 	mutex_unlock(&bus->msg_lock);
> 
> 	return ret;
> }
> 
> The transfer_unlocked is synchronous and waits for the command response
> to be available.
> 
> In other words, there's both a mutual exclusion and a synchronous
> behavior, so not sure how commands *before* the bank switch could be
> pending?

Re: [PATCH 3/5] soundwire: qcom: wait for fifo to be empty before suspend

[Pierre-Louis Bossart]

>>>>> Wait for Fifo to be empty before going to suspend or before bank
>>>>> switch happens. Just to make sure that all the reads/writes are done.
>>>>
>>>> For the suspend case that seems like a valid approach, but for bank
>>>> switch don't we already have a bus->msg_lock mutex that will prevent
>>>> the
>>>> bank switch command from being sent before the other commands are
>>>> handled?
>>>
>>> All read/writes are fifo based, so writes could be still pending.
>>
>> I am not following. The bank switch happens with this function, where a
>> mutex is taken.
>>
>> int sdw_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
>> {
>>     int ret;
>>
>>     mutex_lock(&bus->msg_lock);
>>
>>     ret = sdw_transfer_unlocked(bus, msg);
> 
> Qualcomm controller uses fifo to read/write, so return after writing to
> fifo might not always indicate that write is completed.
> 
> Qcom Soundwire controller do not have any synchronous interrupt
> mechanism to indicate write complete.

Ack, I forgot that one. Might be worth a comment or reworded commit message?