| Message ID | 20190626093851.18474-1-pawel.harlozinski@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Intel: Skylake: Recover BXT FW on DSP boot timeout error | expand |
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@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. > } > } > >
On 6/26/2019 8:13 PM, Cezary Rojewski wrote: > On 2019-06-26 11:38, Pawel Harlozinski wrote: >> When DSP boots with timeout error, reinitialize, transfer >> and boot firmware to recover audio. >> done in PATCH v2 > This is so called "recovery". Say "why" we do it, not just "what" the > sequence is. > >> Signed-off-by: Szymon Mielczarek <szymonx.mielczarek@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? In current implementation CNL handles FW init quite different way, So implementation of FW recovery for CNL have to wait at least for patch unifying firmware loading mechanism. That is already prepared - just waiting in queue with other cleanup patches to be send. > >> /* 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. This log_level change should be part of this commit, because of added recovery after boot timeout. Only recovery fail should be logged on err level. > >> 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. > done in PATCH v2 >> } >> } >> > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
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) /* 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", 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; + } } }
When DSP boots with timeout error, reinitialize, transfer and boot firmware to recover audio. Signed-off-by: Szymon Mielczarek <szymonx.mielczarek@intel.com> --- sound/soc/intel/skylake/bxt-sst.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)