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 ? > command approach. > > Signed-off-by: Srinivas Kandagatla <[email protected]> > --- > 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 = ∁ > - 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 = ∁ > - 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_jiffies(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{} ? > + > + 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 > + 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 -- ~Vinod
