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

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(+)

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;

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;

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?

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?