| Message ID | 20210129173248.5941-6-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | soundwire: qcom: various improvements | expand |
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@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; > + 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
On 29/01/2021 19:33, Pierre-Louis Bossart wrote: > > > On 1/29/21 11:32 AM, 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 > > what does 'kinda blocked' mean? I meant read/writes are waiting for completion interrupt! > >> 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 >> command approach. > > This is really unclear, sorry. If read/writes are waiting for an interrupt, it becomes difficult to read or write to any registers from same interrupt handler! > >> + if (id != SWR_BROADCAST_CMD_ID) { >> + if (id < 14) >> + id += 1; >> + else >> + id = 0; > > that is really odd. if id=13 (group2) then id becomes 14 (master > address). A comment is really needed here. This is magic value for each fifo read or write, so that we can verify that them by comparing with this magic value! This has nothing to do with device number! > >> + if (cmd_id == SWR_BROADCAST_CMD_ID) { >> + /* >> + * sleep for 10ms for MSM soundwire variant to allow broadcast >> + * command to complete. > > that's also super-odd. There is nothing in SoundWire that makes any > difference between a regular and a broadcast command. they all complete > in the same time (a frame). >> + */ >> + ret = wait_for_completion_timeout(&swrm->broadcast, (2 * >> HZ/10)); > > is this 10ms really or dependent on CONFIG_HZ? > >> + if (!ret) >> + ret = SDW_CMD_IGNORED; >> + else >> + ret = SDW_CMD_OK; > > no CMD_FAILED support? Qcom controllers does not provide that information if the command is ignored or failed by any means! That was the behavior from the starting of this driver. > >> +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: >> + >> + 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)) { >> + 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); >> + ret = SDW_CMD_IGNORED; >> + } >> } > > the flow seems complicated with multiple tests and goto? Can this be > simplified? I will try to simplify this in next version!
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; >> + 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_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 >
On 2/1/21 9:50 AM, Srinivas Kandagatla wrote: > > > On 29/01/2021 19:33, Pierre-Louis Bossart wrote: >> >> >> On 1/29/21 11:32 AM, 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 >> >> what does 'kinda blocked' mean? > > I meant read/writes are waiting for completion interrupt! > >> >>> 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 >>> command approach. >> >> This is really unclear, sorry. > > If read/writes are waiting for an interrupt, it becomes difficult to > read or write to any registers from same interrupt handler! Well, yes, you need to handle the complete() at a lower level than the code that initiates the transactions otherwise you self-deadlock. IIRC in the Intel initial code, the complete was in the handler and the register IOs in the thread. > > >> >>> + if (id != SWR_BROADCAST_CMD_ID) { >>> + if (id < 14) >>> + id += 1; >>> + else >>> + id = 0; >> >> that is really odd. if id=13 (group2) then id becomes 14 (master >> address). A comment is really needed here. > > This is magic value for each fifo read or write, so that we can verify > that them by comparing with this magic value! > > This has nothing to do with device number! You should probably add a comment here then, or use a #define instead of the 14 which threw me off. > >> >>> + if (cmd_id == SWR_BROADCAST_CMD_ID) { >>> + /* >>> + * sleep for 10ms for MSM soundwire variant to allow broadcast >>> + * command to complete. >> >> that's also super-odd. There is nothing in SoundWire that makes any >> difference between a regular and a broadcast command. they all >> complete in the same time (a frame). >>> + */ >>> + ret = wait_for_completion_timeout(&swrm->broadcast, (2 * >>> HZ/10)); >> >> is this 10ms really or dependent on CONFIG_HZ? comment missed? >> >>> + if (!ret) >>> + ret = SDW_CMD_IGNORED; >>> + else >>> + ret = SDW_CMD_OK; >> >> no CMD_FAILED support? > > Qcom controllers does not provide that information if the command is > ignored or failed by any means! > > That was the behavior from the starting of this driver. ah yes, now I remember this.
On 01/02/2021 16:42, Pierre-Louis Bossart wrote: > > > On 2/1/21 9:50 AM, Srinivas Kandagatla wrote: >> >> >> On 29/01/2021 19:33, Pierre-Louis Bossart wrote: >>> >>> >>> On 1/29/21 11:32 AM, 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 >>> >>> what does 'kinda blocked' mean? >> >> I meant read/writes are waiting for completion interrupt! >> >>> >>>> 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 >>>> command approach. >>> >>> This is really unclear, sorry. >> >> If read/writes are waiting for an interrupt, it becomes difficult to >> read or write to any registers from same interrupt handler! > > Well, yes, you need to handle the complete() at a lower level than the > code that initiates the transactions otherwise you self-deadlock. > > IIRC in the Intel initial code, the complete was in the handler and the > register IOs in the thread. > Yes, we did the same in previous version of the code, however with this patch reading/writing fifo directly without need of completion should remove that need of another thread! >> >> >>> >>>> + if (id != SWR_BROADCAST_CMD_ID) { >>>> + if (id < 14) >>>> + id += 1; >>>> + else >>>> + id = 0; >>> >>> that is really odd. if id=13 (group2) then id becomes 14 (master >>> address). A comment is really needed here. >> >> This is magic value for each fifo read or write, so that we can verify >> that them by comparing with this magic value! >> >> This has nothing to do with device number! > > You should probably add a comment here then, or use a #define instead of > the 14 which threw me off. I agree! > >> >>> >>>> + if (cmd_id == SWR_BROADCAST_CMD_ID) { >>>> + /* >>>> + * sleep for 10ms for MSM soundwire variant to allow broadcast >>>> + * command to complete. >>> >>> that's also super-odd. There is nothing in SoundWire that makes any >>> difference between a regular and a broadcast command. they all >>> complete in the same time (a frame). >>>> + */ >>>> + ret = wait_for_completion_timeout(&swrm->broadcast, (2 * >>>> HZ/10)); >>> >>> is this 10ms really or dependent on CONFIG_HZ? > > comment missed? Not intentionally :-) I should probably to use msecs_to_jiffies here to keep it inline with the comment! --srini > >>> >>>> + if (!ret) >>>> + ret = SDW_CMD_IGNORED; >>>> + else >>>> + ret = SDW_CMD_OK; >>> >>> no CMD_FAILED support? >> >> Qcom controllers does not provide that information if the command is >> ignored or failed by any means! >> >> That was the behavior from the starting of this driver. > > ah yes, now I remember this.
>>>>> 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 >>>>> command approach. >>>> >>>> This is really unclear, sorry. >>> >>> If read/writes are waiting for an interrupt, it becomes difficult to >>> read or write to any registers from same interrupt handler! >> >> Well, yes, you need to handle the complete() at a lower level than the >> code that initiates the transactions otherwise you self-deadlock. >> >> IIRC in the Intel initial code, the complete was in the handler and >> the register IOs in the thread. >> > Yes, we did the same in previous version of the code, however with this > patch reading/writing fifo directly without need of completion should > remove that need of another thread! Right, but you'll also write-off some command/control efficiency by either sleeping too much before checking the status, or sleeping too little and reading status from a transaction that's not finished.
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: + + 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)) { + 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); + 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;
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 command approach. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/soundwire/qcom.c | 148 +++++++++++++++++++++++++-------------- 1 file changed, 96 insertions(+), 52 deletions(-)