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 <srinivas.kandagatla@xxxxxxxxxx>
> ---
> 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
