Skip to content

Fw boot refine and DSP init retry fixes #507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
plbossart merged 2 commits into from
Jan 9, 2019
Merged

Fw boot refine and DSP init retry fixes #507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a31b2b
ASoC: SOF: optimize dsp register polling
ranj063 Jan 4, 2019
53d6049
ASoC: SOF: hda-loader: reduce timeout for ROM init and refine FW load…
keyonjie Jan 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 86 additions & 68 deletions sound/soc/sof/intel/hda-loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,25 +78,12 @@ static int cl_stream_prepare(struct snd_sof_dev *sdev, unsigned int format,
* reset/stall and then turn it off
*/
static int cl_dsp_init(struct snd_sof_dev *sdev, const void *fwdata,
u32 fwsize)
u32 fwsize, int stream_tag)
{
int tag, ret, i;
u32 hipcie;
struct sof_intel_hda_dev *hda =
(struct sof_intel_hda_dev *)sdev->pdata->hw_pdata;
struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
Copy link
Copy Markdown
Member

@plbossart plbossart Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverse xmas tree please, we will get that feedback when upstreaming

Copy link
Copy Markdown
Author

@keyonjie keyonjie Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let me do it as already at here.

const struct sof_intel_dsp_desc *chip = hda->desc;

/* prepare DMA for code loader stream */
tag = cl_stream_prepare(sdev, 0x40, fwsize, &sdev->dmab,
SNDRV_PCM_STREAM_PLAYBACK);

if (tag <= 0) {
dev_err(sdev->dev, "error: dma prepare for fw loading err: %x\n",
tag);
return tag;
}

memcpy(sdev->dmab.area, fwdata, fwsize);
int ret, i;
u32 hipcie;

/* step 1: power up corex */
ret = hda_dsp_core_power_up(sdev, chip->cores_mask);
Expand All @@ -108,7 +95,7 @@ static int cl_dsp_init(struct snd_sof_dev *sdev, const void *fwdata,
/* step 2: purge FW request */
snd_sof_dsp_write(sdev, HDA_DSP_BAR, chip->ipc_req,
chip->ipc_req_mask | (HDA_DSP_IPC_PURGE_FW |
((tag - 1) << 9)));
((stream_tag - 1) << 9)));

/* step 3: unset core 0 reset state & unstall/run core 0 */
ret = hda_dsp_core_run(sdev, HDA_DSP_CORE_MASK(0));
Expand Down Expand Up @@ -164,20 +151,15 @@ static int cl_dsp_init(struct snd_sof_dev *sdev, const void *fwdata,
ret = snd_sof_dsp_register_poll(sdev, HDA_DSP_BAR,
HDA_DSP_SRAM_REG_ROM_STATUS,
HDA_DSP_ROM_STS_MASK, HDA_DSP_ROM_INIT,
HDA_DSP_INIT_TIMEOUT);
if (ret >= 0)
goto out;

ret = -EIO;
HDA_ROM_INIT_TIMEOUT);
if (!ret)
return 0;

err:
hda_dsp_dump(sdev, SOF_DBG_REGS | SOF_DBG_PCI | SOF_DBG_MBOX);
hda_dsp_core_reset_power_down(sdev, HDA_DSP_CORE_MASK(0) |
HDA_DSP_CORE_MASK(1));
return ret;
Copy link
Copy Markdown
Member

@plbossart plbossart Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the error sequence doesn't seem to deal with more than 2 cores?

Copy link
Copy Markdown
Author

@keyonjie keyonjie Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, it doesn't, @ranj063 mentioned to do that so I thought it already done, let me change it now.

hda_dsp_core_reset_power_down(sdev, chip->cores_mask);

out:
return tag;
return ret;
}

static int cl_trigger(struct snd_sof_dev *sdev,
Expand Down Expand Up @@ -219,8 +201,8 @@ static int cl_cleanup(struct snd_sof_dev *sdev, struct snd_dma_buffer *dmab,

ret = hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_DISABLE, 0);

/* TODO: spin lock ?*/
hstream->opened = 0;
hda_dsp_stream_put(sdev, SNDRV_PCM_STREAM_PLAYBACK,
hstream->stream_tag);
hstream->running = 0;
hstream->substream = NULL;

Expand Down Expand Up @@ -280,12 +262,6 @@ static int cl_copy_fw(struct snd_sof_dev *sdev, int tag)
return ret;
}

ret = cl_cleanup(sdev, &sdev->dmab, stream);
if (ret < 0) {
dev_err(sdev->dev, "error: Code loader DSP cleanup failed\n");
return ret;
}

return status;
}

Expand All @@ -305,7 +281,10 @@ int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
{
struct snd_sof_pdata *plat_data = dev_get_platdata(sdev->dev);
struct firmware stripped_firmware;
int ret, tag, i;
struct hdac_bus *bus = sof_to_bus(sdev);
struct hdac_ext_stream *stream = NULL;
struct hdac_stream *s;
int ret, ret1, tag, i;

stripped_firmware.data = plat_data->fw->data;
stripped_firmware.size = plat_data->fw->size;
Expand All @@ -314,44 +293,83 @@ int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
init_waitqueue_head(&sdev->boot_wait);
sdev->boot_complete = false;

/* try attempting fw boot a few times before giving up */
/* prepare DMA for code loader stream */
tag = cl_stream_prepare(sdev, 0x40, stripped_firmware.size,
&sdev->dmab, SNDRV_PCM_STREAM_PLAYBACK);

if (tag < 0) {
dev_err(sdev->dev, "error: dma prepare for fw loading err: %x\n",
tag);
return tag;
Copy link
Copy Markdown
Member

@plbossart plbossart Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very odd. You could return 0 as an error but the calling layers in core.c expects errors to be <0.
Again mixing tags and return values doesn't seem to be very wise.

Copy link
Copy Markdown
Author

@keyonjie keyonjie Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to change too much in this small series, shall we keep cl_stream_prepare() what it is now, and refine it later if needed?

Today, it returns allocated stream_tag(>=1) at success and minus value(<0) at fail.

Copy link
Copy Markdown
Member

@plbossart plbossart Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so zero is what then?

Copy link
Copy Markdown
Author

@keyonjie keyonjie Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so zero is what then?

There won't be zero returned, so will you feel better if I change it like this:

--- if (tag <= 0) {
+++ if (tag < 0) {

Copy link
Copy Markdown
Author

@keyonjie keyonjie Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart changed.

}

memcpy(sdev->dmab.area, stripped_firmware.data,
stripped_firmware.size);

/* try ROM init a few times before giving up */
for (i = 0; i < HDA_FW_BOOT_ATTEMPTS; i++) {
tag = cl_dsp_init(sdev, stripped_firmware.data,
stripped_firmware.size);

if (tag <= 0) {
dev_err(sdev->dev, "error: Error code=0x%x: FW status=0x%x\n",
snd_sof_dsp_read(sdev, HDA_DSP_BAR,
HDA_DSP_SRAM_REG_ROM_ERROR),
snd_sof_dsp_read(sdev, HDA_DSP_BAR,
HDA_DSP_SRAM_REG_ROM_STATUS));
dev_err(sdev->dev, "error: iteration %d of Core En/ROM load fail:%d\n",
i, tag);
ret = tag;
continue;
}
ret = cl_dsp_init(sdev, stripped_firmware.data,
stripped_firmware.size, tag);

/* at this point DSP ROM has been initialized and
* should be ready for code loading and firmware boot
*/
ret = cl_copy_fw(sdev, tag);
if (ret < 0) {
dev_err(sdev->dev, "error: iteration %d of load fw failed err: %d\n",
i, ret);
continue;
}
/* don't retry anymore if successful */
if (!ret)
break;

dev_err(sdev->dev, "error: Error code=0x%x: FW status=0x%x\n",
snd_sof_dsp_read(sdev, HDA_DSP_BAR,
HDA_DSP_SRAM_REG_ROM_ERROR),
snd_sof_dsp_read(sdev, HDA_DSP_BAR,
HDA_DSP_SRAM_REG_ROM_STATUS));
dev_err(sdev->dev, "error: iteration %d of Core En/ROM load failed: %d\n",
i, ret);
}

if (i == HDA_FW_BOOT_ATTEMPTS) {
dev_err(sdev->dev, "error: dsp init failed after %d attempts with err: %d\n",
i, ret);

hda_dsp_dump(sdev, SOF_DBG_REGS | SOF_DBG_PCI | SOF_DBG_MBOX);

/* disable DSP */
snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR,
SOF_HDA_REG_PP_PPCTL,
SOF_HDA_PPCTL_GPROCEN, 0);
goto done;
}

/* at this point DSP ROM has been initialized and
* should be ready for code loading and firmware boot
*/
ret = cl_copy_fw(sdev, tag);
if (ret < 0)
dev_err(sdev->dev, "error: load fw failed ret: %d\n", ret);
else
dev_dbg(sdev->dev, "Firmware download successful, booting...\n");
return ret;

done:
/* get stream with tag for code loader stream cleanup */
list_for_each_entry(s, &bus->stream_list, list) {
if (s->direction == SNDRV_PCM_STREAM_PLAYBACK &&
s->stream_tag == tag) {
stream = stream_to_hdac_ext_stream(s);
break;
}
}

hda_dsp_dump(sdev, SOF_DBG_REGS | SOF_DBG_PCI | SOF_DBG_MBOX);
if (!stream) {
dev_err(sdev->dev,
"error: could not get stream with stream tag%d\n",
tag);
return -ENODEV;
}

/* cleanup code loader stream */
ret1 = cl_cleanup(sdev, &sdev->dmab, stream);
if (ret1 < 0) {
dev_err(sdev->dev, "error: Code loader DSP cleanup failed\n");
return ret1;
}

/* disable DSP */
snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL,
SOF_HDA_PPCTL_GPROCEN, 0);
dev_err(sdev->dev, "error: load fw failed after %d attempts with err: %d\n",
HDA_FW_BOOT_ATTEMPTS, ret);
return ret;
}

Expand Down
1 change: 1 addition & 0 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@
#define HDA_DSP_RESET_TIMEOUT 50
#define HDA_DSP_BASEFW_TIMEOUT 3000
#define HDA_DSP_INIT_TIMEOUT 500
#define HDA_ROM_INIT_TIMEOUT 70
#define HDA_DSP_CTRL_RESET_TIMEOUT 100
#define HDA_DSP_WAIT_TIMEOUT 500 /* 500 msec */

Expand Down
43 changes: 23 additions & 20 deletions sound/soc/sof/ops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,37 +142,40 @@ void snd_sof_dsp_update_bits_forced(struct snd_sof_dev *sdev, u32 bar,
EXPORT_SYMBOL(snd_sof_dsp_update_bits_forced);
Comment thread
plbossart marked this conversation as resolved.
Outdated

int snd_sof_dsp_register_poll(struct snd_sof_dev *sdev, u32 bar, u32 offset,
u32 mask, u32 target, u32 timeout)
u32 mask, u32 target, u32 timeout_ms)
{
int time, ret;
u32 reg;
unsigned long tout_jiff;
int k = 0, s = 500;

/*
* we will poll for couple of ms using mdelay, if not successful
* then go to longer sleep using usleep_range
* Split the loop into 2 sleep stages with varying resolution.
* To do it more accurately, the range of wakeups are:
* Phase 1(first 5ms): min sleep 0.5ms; max sleep 1ms.
* Phase 2(beyond 5ms): min sleep 5ms; max sleep 10ms.
*/

/* check if set state successful */
for (time = 5; time > 0; time--) {
if ((snd_sof_dsp_read(sdev, bar, offset) & mask) == target)
tout_jiff = jiffies + msecs_to_jiffies(timeout_ms);
do {
reg = snd_sof_dsp_read(sdev, bar, offset);
if ((reg & mask) == target)
break;
msleep(20);
}

if (!time) {
/* sleeping in 10ms steps so adjust timeout value */
timeout /= 10;
/* Phase 2 after 5ms(500us * 10) */
if (++k > 10)
s = 5000;
Copy link
Copy Markdown
Member

@plbossart plbossart Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make those constants device-specific, e.g. with fields that are set in sdev? I don't see any reason why every hardware on the planet would need to use those constants?

Copy link
Copy Markdown
Author

@keyonjie keyonjie Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is for register polling, it poll until we get some specific value or timeout, we sleep some time between every 2 polling to save CPU bandwidth, we will get the target value within different time polling for different devices/registers, no any document staged this for specific devices, here it is a quite generic algorithm about the trade-off between responsiveness(about register update) and bandwidth occupying.

I think the logic here is:

  1. for those registers which its value is changed to target one rapidly, let's get it ASAP, that is phase 1(0.5~1.0 ms sleep only).
  2. after tried enough(e.g. 10 here) times with failing, we deem that it might need wait more time, let's take it easy, slow down our cadence.

We can define macros for those numbers(e.g. PHASE1_TRY=10, PHASE1_MIN/MAX, PHASE2_MIN/MAX, ...), but I don't think we can find any theoretical basis about what numbers are ideal ones(let alone to specific devices), if we won't change them frequently, I think using direct numbers(not macros) is fine.

@plbossart what's your opinion?

Copy link
Copy Markdown
Member

@plbossart plbossart Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested constant fields that are provided in the SOC-dependent structures, not generic macros which indeed are not better than magic values.


for (time = timeout; time > 0; time--) {
if ((snd_sof_dsp_read(sdev, bar, offset) & mask) == target)
break;
usleep_range(s, 2 * s);
} while (time_before(jiffies, tout_jiff));

usleep_range(5000, 10000);
}
}
if ((reg & mask) == target) {
dev_dbg(sdev->dev, "FW Poll Status: reg=%#x successful\n", reg);

ret = time ? 0 : -ETIME;
return 0;
}

return ret;
dev_dbg(sdev->dev, "FW Poll Status: reg=%#x timedout\n", reg);
return -ETIME;
}
EXPORT_SYMBOL(snd_sof_dsp_register_poll);

Expand Down