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.kandagatla@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 = ∁
- 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;elseid = 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_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;elseret = SDW_CMD_OK;
- if (!ret) {
ret = SDW_CMD_IGNORED;goto err;
- 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