Re: [PATCH 5/6] soundwire: qcom: update register read/write routine

[Srinivas Kandagatla]

On 01/02/2021 14:26, Vinod Koul wrote:
On 29-01-21, 17:32, Srinivas Kandagatla wrote:
In the existing code every soundwire register read and register write
are kinda blocked. Each of these are using a special command id that
generates interrupt after it successfully finishes. This is really
overhead, limiting and not really necessary unless we are doing
something special.

We can simply read/write the fifo that should also give exactly
what we need! This will also allow to read/write registers in
interrupt context, which was not possible with the special

Okay but then why use a mutex ?

Read and writes can come from two places,
1> from SoundWire Core itself which takes care of locking
2> from interrupt handler in the driver itself, which is why we need an 
additional locking!

Having said that I did have look at the current state of driver 
interrupt handler, and it does not do any soundwire register read/writes 
from interrupt handler so we can remove the lock for now!


command approach.

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

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 83df15d83935..d61b204dc284 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -78,13 +78,15 @@
  #define SWRM_SPECIAL_CMD_ID   0xF
  #define MAX_FREQ_NUM          1
  #define TIMEOUT_MS            (2 * HZ)
-#define QCOM_SWRM_MAX_RD_LEN   0xf
+#define QCOM_SWRM_MAX_RD_LEN   0x1
  #define QCOM_SDW_MAX_PORTS    14
  #define DEFAULT_CLK_FREQ      9600000
  #define SWRM_MAX_DAIS         0xF
  #define SWR_INVALID_PARAM 0xFF
  #define SWR_HSTOP_MAX_VAL 0xF
  #define SWR_HSTART_MIN_VAL 0x0
+#define SWR_BROADCAST_CMD_ID    0x0F
+#define MAX_FIFO_RD_FAIL_RETRY 3
  
  struct qcom_swrm_port_config {
        u8 si;
@@ -104,11 +106,13 @@ struct qcom_swrm_ctrl {
        struct regmap *regmap;
        void __iomem *mmio;
        struct completion *comp;
+       struct completion broadcast;
        struct work_struct slave_work;
        /* read/write lock */
        spinlock_t comp_lock;
        /* Port alloc/free lock */
        struct mutex port_lock;
+       struct mutex io_lock;
        struct clk *hclk;
        u8 wr_cmd_id;
        u8 rd_cmd_id;
@@ -122,6 +126,8 @@ struct qcom_swrm_ctrl {
        int rows_index;
        unsigned long dout_port_mask;
        unsigned long din_port_mask;
+       u8 rcmd_id;
+       u8 wcmd_id;
        struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
        struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
        enum sdw_slave_status status[SDW_MAX_DEVICES];
@@ -200,75 +206,111 @@ static int qcom_swrm_cpu_reg_write(struct qcom_swrm_ctrl 
*ctrl, int reg,
        return SDW_CMD_OK;
  }
  
-static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
-                                    u8 dev_addr, u16 reg_addr)
+static u32 swrm_get_packed_reg_val(u8 *cmd_id, u8 cmd_data,
+                                u8 dev_addr, u16 reg_addr)
  {
-       DECLARE_COMPLETION_ONSTACK(comp);
-       unsigned long flags;
        u32 val;
-       int ret;
-
-       spin_lock_irqsave(&ctrl->comp_lock, flags);
-       ctrl->comp = &comp;
-       spin_unlock_irqrestore(&ctrl->comp_lock, flags);
-       val = SWRM_REG_VAL_PACK(cmd_data, dev_addr,
-                               SWRM_SPECIAL_CMD_ID, reg_addr);
-       ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
-       if (ret)
-               goto err;
-
-       ret = wait_for_completion_timeout(ctrl->comp,
-                                         msecs_to_jiffies(TIMEOUT_MS));
+       u8 id = *cmd_id;
  
-	if (!ret)
-               ret = SDW_CMD_IGNORED;
-       else
-               ret = SDW_CMD_OK;
-err:
-       spin_lock_irqsave(&ctrl->comp_lock, flags);
-       ctrl->comp = NULL;
-       spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+       if (id != SWR_BROADCAST_CMD_ID) {
+               if (id < 14)
+                       id += 1;
+               else
+                       id = 0;
+               *cmd_id = id;
+       }
+       val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, id, reg_addr);
  
-	return ret;
+       return val;
  }
  
-static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
-                                    u8 dev_addr, u16 reg_addr,
-                                    u32 len, u8 *rval)
+
+static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *swrm, u8 cmd_data,
+                                    u8 dev_addr, u16 reg_addr)
  {
-       int i, ret;
+
        u32 val;
-       DECLARE_COMPLETION_ONSTACK(comp);
-       unsigned long flags;
+       int ret = 0;
+       u8 cmd_id = 0x0;
+
+       mutex_lock(&swrm->io_lock);
+       if (dev_addr == SDW_BROADCAST_DEV_NUM) {
+               cmd_id = SWR_BROADCAST_CMD_ID;
+               val = swrm_get_packed_reg_val(&cmd_id, cmd_data,
+                                             dev_addr, reg_addr);
+       } else {
+               val = swrm_get_packed_reg_val(&swrm->wcmd_id, cmd_data,
+                                             dev_addr, reg_addr);
+       }
  
-	spin_lock_irqsave(&ctrl->comp_lock, flags);
-       ctrl->comp = &comp;
-       spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+       swrm->reg_write(swrm, SWRM_CMD_FIFO_WR_CMD, val);
  
-	val = SWRM_REG_VAL_PACK(len, dev_addr, SWRM_SPECIAL_CMD_ID, reg_addr);
-       ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
-       if (ret)
-               goto err;
+       /* version 1.3 or less */
+       if (swrm->version_major == 1 && swrm->version_minor <= 3)
+               usleep_range(150, 155);
  
-	ret = wait_for_completion_timeout(ctrl->comp,
-                                         msecs_to_jif
--srini
fies(TIMEOUT_MS));
+       if (cmd_id == SWR_BROADCAST_CMD_ID) {
+               /*
+                * sleep for 10ms for MSM soundwire variant to allow broadcast
+                * command to complete.
+                */
+               ret = wait_for_completion_timeout(&swrm->broadcast, (2 * 
HZ/10));
+               if (!ret)
+                       ret = SDW_CMD_IGNORED;
+               else
+                       ret = SDW_CMD_OK;
  
-	if (!ret) {
-               ret = SDW_CMD_IGNORED;
-               goto err;
        } else {
                ret = SDW_CMD_OK;
        }
+       mutex_unlock(&swrm->io_lock);
+       return ret;
+}
  
-	for (i = 0; i < len; i++) {
-               ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR, &val);
-               rval[i] = val & 0xFF;
+static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
+                                    u8 dev_addr, u16 reg_addr,
+                                    u32 len, u8 *rval)
+{
+       u32 val;
+       u32 retry_attempt = 0;
+       u32 cmd_data;
+       int ret = SDW_CMD_OK;
+
+       mutex_lock(&swrm->io_lock);
+       val = swrm_get_packed_reg_val(&swrm->rcmd_id, len, dev_addr, reg_addr);
+
+       /* wait for FIFO RD to complete to avoid overflow */
+       usleep_range(100, 105);
+       swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
+       /* wait for FIFO RD CMD complete to avoid overflow */
+       usleep_range(250, 255);
+
+retry_read:

do while{} ?

Sure!


+
+       swrm->reg_read(swrm, SWRM_CMD_FIFO_RD_FIFO_ADDR, &cmd_data);
+       rval[0] = cmd_data & 0xFF;
+
+       if ((((cmd_data) & 0xF00) >> 8) != swrm->rcmd_id) {
+               if (retry_attempt < MAX_FIFO_RD_FAIL_RETRY) {
+                       /* wait 500 us before retry on fifo read failure */
+                       usleep_range(500, 505);
+                       if (retry_attempt == (MAX_FIFO_RD_FAIL_RETRY - 1)) {

why not do this at the end if retry fails, that will make code look
neater

I agree, will clean this one in next version!


+                               swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
+                               swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, 
val);
+                       }
+                       retry_attempt++;
+                       goto retry_read;
+               } else {
+                       dev_err(swrm->dev,
+                                       "failed to read fifo: reg: 0x%x, \
+                                       rcmd_id: 0x%x, dev_num: 0x%x, cmd_data: 
0x%x\n",
+                                       reg_addr, swrm->rcmd_id,
+                                       dev_addr, cmd_data);

Do you want to log retry as err..?

+                       ret = SDW_CMD_IGNORED;
+               }
        }
  
-err:
-       spin_lock_irqsave(&ctrl->comp_lock, flags);
-       ctrl->comp = NULL;
-       spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+       mutex_unlock(&swrm->io_lock);
  
  	return ret;
  }
@@ -949,6 +991,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
        dev_set_drvdata(&pdev->dev, ctrl);
        spin_lock_init(&ctrl->comp_lock);
        mutex_init(&ctrl->port_lock);
+       mutex_init(&ctrl->io_lock);
+       init_completion(&ctrl->broadcast);
        INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
  
  	ctrl->bus.ops = &qcom_swrm_ops;
--
2.21.0