Re: [PATCH 3/3] soundwire: qcom: add clock stop via runtime pm support

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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