| Message ID | 20230209131336.18252-4-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: qcom: stablity fixes | expand |
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?
>>>>> 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?
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;
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(+)