[alsa-devel] [PATCH] ASoC: Intel: Skylake: Recover BXT FW on DSP boot timeout error

[Cezary Rojewski]

On 2019-06-26 11:38, Pawel Harlozinski wrote:
> When DSP boots with timeout error, reinitialize, transfer
> and boot firmware to recover audio.
> 

This is so called "recovery". Say "why" we do it, not just "what" the 
sequence is.

> Signed-off-by: Szymon Mielczarek <szymonx.mielczarek at intel.com>
> ---
>   sound/soc/intel/skylake/bxt-sst.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c
> index 440bca7afbf1..a2844bdca1b8 100644
> --- a/sound/soc/intel/skylake/bxt-sst.c
> +++ b/sound/soc/intel/skylake/bxt-sst.c
> @@ -455,13 +455,18 @@ static int bxt_set_dsp_D0(struct sst_dsp *ctx, unsigned int core_id)

What about its neighbour, cnl_set_dsp_D0?

>   	/* If core 1 was turned on for booting core 0, turn it off */
>   		skl_dsp_core_power_down(ctx, SKL_DSP_CORE_MASK(1));
>   		if (ret == 0) {
> -			dev_err(ctx->dev, "%s: DSP boot timeout\n", __func__);
> -			dev_err(ctx->dev, "Error code=0x%x: FW status=0x%x\n",
> +			dev_warn(ctx->dev,
> +				"DSP boot timeout: Error code=0x%x: FW status=0x%x\n",

Log change (--log_level) without mention in commit msg. Maybe split this 
into separate commit? The entire log-polish thingy could be a theme for 
a patchset as this is not the only place where such changes are welcome.

>   				sst_dsp_shim_read(ctx, BXT_ADSP_ERROR_CODE),
>   				sst_dsp_shim_read(ctx, BXT_ADSP_FW_STATUS));
> -			dev_err(ctx->dev, "Failed to set core0 to D0 state\n");
> -			ret = -EIO;
> -			goto err;
> +
> +			ret = bxt_sst_init_fw(skl->dev, skl);
> +			dev_warn(ctx->dev, "Reload fw status: %d\n", ret);
> +			if (ret < 0) {
> +				dev_err(ctx->dev, "Failed to set core0 to D0 state\n");
> +				ret = -EIO;
> +				goto err;
> +			}

Double message is unnecessary, routine is verbose enough. Either leave 
"always report stuff" and resign from mentioning anything at all on 
failure -or- do it only on failure explicitly.

>   		}
>   	}
>   
>