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);