| Message ID | 20210226170250.9067-4-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: qcom: add Clock Stop and Auto Enumeration support | expand |
> + pm_runtime_get_sync(ctrl->dev); if this fails you've got to do a put_noidle(). we use this for Intel: ret = pm_runtime_get_sync(cdns->dev); if (ret < 0 && ret != -EACCES) { dev_err_ratelimited(cdns->dev, "pm_runtime_get_sync failed in %s, ret %d\n", __func__, ret); pm_runtime_put_noidle(cdns->dev); return ret; } > + pm_runtime_mark_last_busy(ctrl->dev); > + pm_runtime_put_autosuspend(ctrl->dev); > > complete(&ctrl->enumeration); > return 0; > @@ -421,6 +427,7 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id) > u8 devnum = 0; > int ret = IRQ_HANDLED; > > + clk_prepare_enable(swrm->hclk); > swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts); > intr_sts_masked = intr_sts & swrm->intr_mask; > > @@ -529,6 +536,7 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id) > intr_sts_masked = intr_sts & swrm->intr_mask; > } while (intr_sts_masked); > > + clk_disable_unprepare(swrm->hclk); > return ret; > } > > @@ -587,6 +595,8 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus, > struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); > int ret, i, len; > > + pm_runtime_get_sync(ctrl->dev); > + > if (msg->flags == SDW_MSG_FLAG_READ) { > for (i = 0; i < msg->len;) { > if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN) > @@ -598,7 +608,7 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus, > msg->addr + i, len, > &msg->buf[i]); > if (ret) > - return ret; > + goto done; > > i = i + len; > } > @@ -607,12 +617,20 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus, > ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i], > msg->dev_num, > msg->addr + i); > - if (ret) > - return SDW_CMD_IGNORED; > + if (ret) { > + ret = SDW_CMD_IGNORED; > + goto done; > + } > } > } > > + pm_runtime_put_autosuspend(ctrl->dev); > + pm_runtime_mark_last_busy(ctrl->dev); wrong order, you've got to mark_last_busy before the put(). > return SDW_CMD_OK; > +done: > + pm_runtime_mark_last_busy(ctrl->dev); > + pm_runtime_put_autosuspend(ctrl->dev); > + return ret; this looks weird. why not reuse the same flow and return ret in all cases? > } > > static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus) > @@ -620,13 +638,19 @@ static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus) > u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank); > struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); > u32 val; > + int ret; > > + pm_runtime_get_sync(ctrl->dev); > ctrl->reg_read(ctrl, reg, &val); > > u32p_replace_bits(&val, ctrl->cols_index, SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK); > u32p_replace_bits(&val, ctrl->rows_index, SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK); > > - return ctrl->reg_write(ctrl, reg, val); > + ret = ctrl->reg_write(ctrl, reg, val); > + pm_runtime_mark_last_busy(ctrl->dev); > + pm_runtime_put_autosuspend(ctrl->dev); > + > + return ret; > } > > static int qcom_swrm_port_params(struct sdw_bus *bus, > @@ -634,13 +658,18 @@ static int qcom_swrm_port_params(struct sdw_bus *bus, > unsigned int bank) > { > struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); > + int ret = 0; > + pm_runtime_get_sync(ctrl->dev); > > if (p_params->bps != SWR_INVALID_PARAM) > - return ctrl->reg_write(ctrl, > + ret = ctrl->reg_write(ctrl, > SWRM_DP_BLOCK_CTRL_1(p_params->num), > p_params->bps - 1); > + pm_runtime_mark_last_busy(ctrl->dev); > + pm_runtime_put_autosuspend(ctrl->dev); it feels like all you pm_runtime_get/put() should be moved to your register read/write operations? > > - return 0; > + > + return ret; > } > > static int qcom_swrm_transport_params(struct sdw_bus *bus, > @@ -651,6 +680,7 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus, > u32 value; > int reg = SWRM_DP_PORT_CTRL_BANK((params->port_num), bank); > int ret; > + pm_runtime_get_sync(ctrl->dev); > > value = params->offset1 << SWRM_DP_PORT_CTRL_OFFSET1_SHFT; > value |= params->offset2 << SWRM_DP_PORT_CTRL_OFFSET2_SHFT; > @@ -685,6 +715,9 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus, > reg = SWRM_DP_BLOCK_CTRL3_BANK(params->port_num, bank); > ret = ctrl->reg_write(ctrl, reg, params->blk_pkg_mode); > } > + pm_runtime_mark_last_busy(ctrl->dev); > + pm_runtime_put_autosuspend(ctrl->dev); > + > > return ret; > } > @@ -696,6 +729,9 @@ static int qcom_swrm_port_enable(struct sdw_bus *bus, > u32 reg = SWRM_DP_PORT_CTRL_BANK(enable_ch->port_num, bank); > struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); > u32 val; > + int ret; > + > + pm_runtime_get_sync(ctrl->dev); > > ctrl->reg_read(ctrl, reg, &val); > > @@ -704,7 +740,11 @@ static int qcom_swrm_port_enable(struct sdw_bus *bus, > else > val &= ~(0xff << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT); > > - return ctrl->reg_write(ctrl, reg, val); > + ret = ctrl->reg_write(ctrl, reg, val); > + pm_runtime_mark_last_busy(ctrl->dev); > + pm_runtime_put_autosuspend(ctrl->dev); > + > + return ret; > } > > static const struct sdw_master_port_ops qcom_swrm_port_ops = { > @@ -1194,6 +1234,13 @@ static int qcom_swrm_probe(struct platform_device *pdev) > (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff, > ctrl->version & 0xffff); > > + pm_runtime_set_autosuspend_delay(dev, 30000); 30s? that sounds very very long for an audio device. > + pm_runtime_use_autosuspend(dev); > + pm_runtime_mark_last_busy(dev); > + > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + > return 0; > > err_master_add: > @@ -1214,6 +1261,47 @@ static int qcom_swrm_remove(struct platform_device *pdev) > return 0; > } > > +static int swrm_runtime_resume(struct device *dev) > +{ > + struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev); > + > + reinit_completion(&ctrl->enumeration); > + clk_prepare_enable(ctrl->hclk); > + ctrl->reg_write(ctrl, SWRM_COMP_SW_RESET, 0x01); > + qcom_swrm_get_device_status(ctrl); > + sdw_handle_slave_status(&ctrl->bus, ctrl->status); > + qcom_swrm_init(ctrl); > + wait_for_completion_timeout(&ctrl->enumeration, > + msecs_to_jiffies(TIMEOUT_MS)); > + usleep_range(100, 105); > + > + pm_runtime_mark_last_busy(dev); Humm, what 'clock stop' are we talking about here? In SoundWire 1.x devices, you can stop the BUS clock and not have to redo any enumeration on resume, devices are required to save their context. You have to also follow the pattern of preparing and broadcasting the CLOCK STOP NOW message. It looks like you are stopping something else, and completely resetting the hardware. It's fine, it's just a reset but not clock stop support as defined in the SoundWire spec. > + > + return 0; > +} > + > +static int __maybe_unused swrm_runtime_suspend(struct device *dev) > +{ > + struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev); > + > + /* Mask bus clash interrupt */ > + ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET; > + ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR, ctrl->intr_mask); > + ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN, ctrl->intr_mask); > + /* clock stop sequence */ > + qcom_swrm_cmd_fifo_wr_cmd(ctrl, 0x2, 0xF, SDW_SCP_CTRL); Humm, this looks like writing in SCP_CTRL::ClockStopNow, so why is enumeration required on restart? If you take down the bus and reset everything, you don't need to do this sequence. a hardware reset will do... > + > + clk_disable_unprepare(ctrl->hclk); > + > + usleep_range(100, 105); > + > + return 0; > +} > + > +static const struct dev_pm_ops swrm_dev_pm_ops = { > + SET_RUNTIME_PM_OPS(swrm_runtime_suspend, swrm_runtime_resume, NULL) > +}; > + > static const struct of_device_id qcom_swrm_of_match[] = { > { .compatible = "qcom,soundwire-v1.3.0", .data = &swrm_v1_3_data }, > { .compatible = "qcom,soundwire-v1.5.1", .data = &swrm_v1_5_data }, > @@ -1228,6 +1316,7 @@ static struct platform_driver qcom_swrm_driver = { > .driver = { > .name = "qcom-soundwire", > .of_match_table = qcom_swrm_of_match, > + .pm = &swrm_dev_pm_ops, > } > }; > module_platform_driver(qcom_swrm_driver); >
Thanks Pierre for reviewing this in detail! On 26/02/2021 17:41, Pierre-Louis Bossart wrote: ... >> return 0; >> err_master_add: >> @@ -1214,6 +1261,47 @@ static int qcom_swrm_remove(struct >> platform_device *pdev) >> return 0; >> } >> +static int swrm_runtime_resume(struct device *dev) >> +{ >> + struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev); >> + >> + reinit_completion(&ctrl->enumeration); >> + clk_prepare_enable(ctrl->hclk); >> + ctrl->reg_write(ctrl, SWRM_COMP_SW_RESET, 0x01); >> + qcom_swrm_get_device_status(ctrl); >> + sdw_handle_slave_status(&ctrl->bus, ctrl->status); >> + qcom_swrm_init(ctrl); >> + wait_for_completion_timeout(&ctrl->enumeration, >> + msecs_to_jiffies(TIMEOUT_MS)); >> + usleep_range(100, 105); >> + >> + pm_runtime_mark_last_busy(dev); > > Humm, what 'clock stop' are we talking about here? > > In SoundWire 1.x devices, you can stop the BUS clock and not have to > redo any enumeration on resume, devices are required to save their > context. You have to also follow the pattern of preparing and > broadcasting the CLOCK STOP NOW message. > > It looks like you are stopping something else, and completely resetting > the hardware. It's fine, it's just a reset but not clock stop support as > defined in the SoundWire spec. > This is clock stop that Soundwire Spec refers to. However I think I messed up this patch! :-) >> + >> + return 0; >> +} >> + >> +static int __maybe_unused swrm_runtime_suspend(struct device *dev) >> +{ >> + struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev); >> + >> + /* Mask bus clash interrupt */ >> + ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET; >> + ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR, ctrl->intr_mask); >> + ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN, ctrl->intr_mask); >> + /* clock stop sequence */ >> + qcom_swrm_cmd_fifo_wr_cmd(ctrl, 0x2, 0xF, SDW_SCP_CTRL); > > Humm, this looks like writing in SCP_CTRL::ClockStopNow, so why is > enumeration required on restart? > One of the controller instance needed a full reset so there is a mix of code for both clock stop and reset here! Am working on cleaning up this in a better way! I will also address the runtime pm comments that you have noticed in next version! --srini > If you take down the bus and reset everything, you don't need to do this > sequence. a hardware reset will do... > >> + >> + clk_disable_unprepare(ctrl->hclk); >> + >> + usleep_range(100, 105); >> + >> + return 0; >> +} >> + >> +static const struct dev_pm_ops swrm_dev_pm_ops = { >> + SET_RUNTIME_PM_OPS(swrm_runtime_suspend, swrm_runtime_resume, NULL) >> +}; >> + >> static const struct of_device_id qcom_swrm_of_match[] = { >> { .compatible = "qcom,soundwire-v1.3.0", .data = &swrm_v1_3_data }, >> { .compatible = "qcom,soundwire-v1.5.1", .data = &swrm_v1_5_data }, >> @@ -1228,6 +1316,7 @@ static struct platform_driver qcom_swrm_driver = { >> .driver = { >> .name = "qcom-soundwire", >> .of_match_table = qcom_swrm_of_match, >> + .pm = &swrm_dev_pm_ops, >> } >> }; >> module_platform_driver(qcom_swrm_driver); >>
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index d90eba6a1e88..0cf8c5c724e2 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -10,6 +10,7 @@ #include <linux/of.h> #include <linux/of_irq.h> #include <linux/of_device.h> +#include <linux/pm_runtime.h> #include <linux/regmap.h> #include <linux/slab.h> #include <linux/slimbus.h> @@ -19,6 +20,8 @@ #include <sound/soc.h> #include "bus.h" +#define SWRM_COMP_SW_RESET (0x008) +#define SWRM_COMP_STATUS (0x014) #define SWRM_COMP_HW_VERSION 0x00 #define SWRM_COMP_CFG_ADDR 0x04 #define SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK BIT(1) @@ -408,6 +411,9 @@ static int qcom_swrm_enumerate(struct sdw_bus *bus) } } } + pm_runtime_get_sync(ctrl->dev); + pm_runtime_mark_last_busy(ctrl->dev); + pm_runtime_put_autosuspend(ctrl->dev); complete(&ctrl->enumeration); return 0; @@ -421,6 +427,7 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id) u8 devnum = 0; int ret = IRQ_HANDLED; + clk_prepare_enable(swrm->hclk); swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts); intr_sts_masked = intr_sts & swrm->intr_mask; @@ -529,6 +536,7 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id) intr_sts_masked = intr_sts & swrm->intr_mask; } while (intr_sts_masked); + clk_disable_unprepare(swrm->hclk); return ret; } @@ -587,6 +595,8 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus, struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); int ret, i, len; + pm_runtime_get_sync(ctrl->dev); + if (msg->flags == SDW_MSG_FLAG_READ) { for (i = 0; i < msg->len;) { if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN) @@ -598,7 +608,7 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus, msg->addr + i, len, &msg->buf[i]); if (ret) - return ret; + goto done; i = i + len; } @@ -607,12 +617,20 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus, ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i], msg->dev_num, msg->addr + i); - if (ret) - return SDW_CMD_IGNORED; + if (ret) { + ret = SDW_CMD_IGNORED; + goto done; + } } } + pm_runtime_put_autosuspend(ctrl->dev); + pm_runtime_mark_last_busy(ctrl->dev); return SDW_CMD_OK; +done: + pm_runtime_mark_last_busy(ctrl->dev); + pm_runtime_put_autosuspend(ctrl->dev); + return ret; } static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus) @@ -620,13 +638,19 @@ static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus) u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank); struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); u32 val; + int ret; + pm_runtime_get_sync(ctrl->dev); ctrl->reg_read(ctrl, reg, &val); u32p_replace_bits(&val, ctrl->cols_index, SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK); u32p_replace_bits(&val, ctrl->rows_index, SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK); - return ctrl->reg_write(ctrl, reg, val); + ret = ctrl->reg_write(ctrl, reg, val); + pm_runtime_mark_last_busy(ctrl->dev); + pm_runtime_put_autosuspend(ctrl->dev); + + return ret; } static int qcom_swrm_port_params(struct sdw_bus *bus, @@ -634,13 +658,18 @@ static int qcom_swrm_port_params(struct sdw_bus *bus, unsigned int bank) { struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); + int ret = 0; + pm_runtime_get_sync(ctrl->dev); if (p_params->bps != SWR_INVALID_PARAM) - return ctrl->reg_write(ctrl, + ret = ctrl->reg_write(ctrl, SWRM_DP_BLOCK_CTRL_1(p_params->num), p_params->bps - 1); + pm_runtime_mark_last_busy(ctrl->dev); + pm_runtime_put_autosuspend(ctrl->dev); - return 0; + + return ret; } static int qcom_swrm_transport_params(struct sdw_bus *bus, @@ -651,6 +680,7 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus, u32 value; int reg = SWRM_DP_PORT_CTRL_BANK((params->port_num), bank); int ret; + pm_runtime_get_sync(ctrl->dev); value = params->offset1 << SWRM_DP_PORT_CTRL_OFFSET1_SHFT; value |= params->offset2 << SWRM_DP_PORT_CTRL_OFFSET2_SHFT; @@ -685,6 +715,9 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus, reg = SWRM_DP_BLOCK_CTRL3_BANK(params->port_num, bank); ret = ctrl->reg_write(ctrl, reg, params->blk_pkg_mode); } + pm_runtime_mark_last_busy(ctrl->dev); + pm_runtime_put_autosuspend(ctrl->dev); + return ret; } @@ -696,6 +729,9 @@ static int qcom_swrm_port_enable(struct sdw_bus *bus, u32 reg = SWRM_DP_PORT_CTRL_BANK(enable_ch->port_num, bank); struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); u32 val; + int ret; + + pm_runtime_get_sync(ctrl->dev); ctrl->reg_read(ctrl, reg, &val); @@ -704,7 +740,11 @@ static int qcom_swrm_port_enable(struct sdw_bus *bus, else val &= ~(0xff << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT); - return ctrl->reg_write(ctrl, reg, val); + ret = ctrl->reg_write(ctrl, reg, val); + pm_runtime_mark_last_busy(ctrl->dev); + pm_runtime_put_autosuspend(ctrl->dev); + + return ret; } static const struct sdw_master_port_ops qcom_swrm_port_ops = { @@ -1194,6 +1234,13 @@ static int qcom_swrm_probe(struct platform_device *pdev) (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff, ctrl->version & 0xffff); + pm_runtime_set_autosuspend_delay(dev, 30000); + pm_runtime_use_autosuspend(dev); + pm_runtime_mark_last_busy(dev); + + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + return 0; err_master_add: @@ -1214,6 +1261,47 @@ static int qcom_swrm_remove(struct platform_device *pdev) return 0; } +static int swrm_runtime_resume(struct device *dev) +{ + struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev); + + reinit_completion(&ctrl->enumeration); + clk_prepare_enable(ctrl->hclk); + ctrl->reg_write(ctrl, SWRM_COMP_SW_RESET, 0x01); + qcom_swrm_get_device_status(ctrl); + sdw_handle_slave_status(&ctrl->bus, ctrl->status); + qcom_swrm_init(ctrl); + wait_for_completion_timeout(&ctrl->enumeration, + msecs_to_jiffies(TIMEOUT_MS)); + usleep_range(100, 105); + + pm_runtime_mark_last_busy(dev); + + return 0; +} + +static int __maybe_unused swrm_runtime_suspend(struct device *dev) +{ + struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev); + + /* Mask bus clash interrupt */ + ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET; + ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR, ctrl->intr_mask); + ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN, ctrl->intr_mask); + /* clock stop sequence */ + qcom_swrm_cmd_fifo_wr_cmd(ctrl, 0x2, 0xF, SDW_SCP_CTRL); + + clk_disable_unprepare(ctrl->hclk); + + usleep_range(100, 105); + + return 0; +} + +static const struct dev_pm_ops swrm_dev_pm_ops = { + SET_RUNTIME_PM_OPS(swrm_runtime_suspend, swrm_runtime_resume, NULL) +}; + static const struct of_device_id qcom_swrm_of_match[] = { { .compatible = "qcom,soundwire-v1.3.0", .data = &swrm_v1_3_data }, { .compatible = "qcom,soundwire-v1.5.1", .data = &swrm_v1_5_data }, @@ -1228,6 +1316,7 @@ static struct platform_driver qcom_swrm_driver = { .driver = { .name = "qcom-soundwire", .of_match_table = qcom_swrm_of_match, + .pm = &swrm_dev_pm_ops, } }; module_platform_driver(qcom_swrm_driver);
Add Clock stop feature support using runtime PM. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/soundwire/qcom.c | 103 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 96 insertions(+), 7 deletions(-)