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

[Srinivas Kandagatla]

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