Re: [RFC PATCH] soundwire: amd: report clock-stop exit failures

[Mukunda,Vijendar]

On 23/06/26 19:33, Pengpeng Hou wrote:
> [You don't often get email from pengpeng@xxxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> amd_sdw_clock_stop_exit() requests SoundWire clock-stop exit and waits
> for AMD_SDW_CLK_RESUME_DONE, but timeout handling only leaves
> clk_stopped set and returns 0. It also logs sdw_bus_exit_clk_stop()
> failures without propagating them.
>
> Return the hardware wait error and the bus clock-stop-exit error so
> runtime resume does not report success while the manager remains in
> clock-stop state.
>
> This is sent as RFC because clock-stop exit is part of AMD SoundWire
> runtime/system PM recovery and the expected timeout policy needs
> maintainer confirmation before changing the resume return value.
>
> Fixes: 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")
> Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
> ---
>  drivers/soundwire/amd_manager.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index a3316efdf..7d82fad0d 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1206,18 +1206,20 @@ static int amd_sdw_clock_stop_exit(struct amd_sdw_manager *amd_manager)
>                 ret = readl_poll_timeout(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL, val,
>                                          (val & AMD_SDW_CLK_RESUME_DONE), ACP_DELAY_US,
>                                          AMD_SDW_TIMEOUT);
> -               if (val & AMD_SDW_CLK_RESUME_DONE) {
> -                       writel(0, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
> -                       ret = sdw_bus_exit_clk_stop(&amd_manager->bus);
> -                       if (ret < 0)
> -                               dev_err(amd_manager->dev, "bus failed to exit clock stop %d\n",
> -                                       ret);
> -                       amd_manager->clk_stopped = false;
> +               if (ret) {
> +                       dev_err(amd_manager->dev, "SDW%x clock stop exit failed\n",
> +                               amd_manager->instance);
> +                       return ret;
>                 }
> -       }
> -       if (amd_manager->clk_stopped) {
> -               dev_err(amd_manager->dev, "SDW%x clock stop exit failed\n", amd_manager->instance);
> -               return 0;
> +
> +               writel(0, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
> +               ret = sdw_bus_exit_clk_stop(&amd_manager->bus);
> +               if (ret < 0) {
> +                       dev_err(amd_manager->dev, "bus failed to exit clock stop %d\n",
> +                               ret);
> +                       return ret;
> +               }
> +               amd_manager->clk_stopped = false;

Thanks for the RFC.

The patch correctly points out two real problems in the helper today:

A CLK_RESUME_DONE timeout is logged, but the function still returns success.
clk_stopped can be cleared even when sdw_bus_exit_clk_stop() fails.
That said, on these platforms, returning 0 after a clock-stop-exit
failure was deliberate. The intent was to avoid failing manager PM
callbacks when ACP_SW_CLK_RESUME_CTRL is slow to report AMD_SDW_CLK_RESUME_DONE,
which we have seen during ACP6.3/7.x bring-up.
Because amd_resume_runtime() already propagates errors from
clock_stop_exit(), changing this helper to return -ETIMEDOUT would cause
runtime and system resume to fail on these variants, instead of
continuing with the rest of the resume sequence.

Earlier, Bossart suggested to try recovering the bus in case of failures
instead of propagating the errors to the PM framework.
This was marked as ToDo.

If we want better visibility without changing PM semantics,
improved logging on timeout while keeping return 0 may be enough.
One related note: under AMD_SDW_POWER_OFF_MODE, amd_resume_runtime()
has another CLK_RESUME_CTRL poll with the same timeout-handling pattern.

-
Vijendar



>         }
>         dev_dbg(amd_manager->dev, "SDW%x clock stop exit successful\n", amd_manager->instance);
>         return 0;
> --
> 2.50.1 (Apple Git-155)
>