Topic/skl rom init timeout

[zhuyingjiang]

As part of #704
make the ROM init timeout SKL platform specific, setting to 150 ms.
change the core mask, so the two cores can be enabled.
unlike DSP core of APL, CNL which must power up, reset and unstall core 0 first, the SKL DSP cores can be enabled at the same time.

[plbossart]

I don't understand why you enable 2 cores by default, and I have questions on the changes that have nothing to do with the rom timeout.

[plbossart]

why is this changed?

[zhuyingjiang]

@plbossart This is a bug, using goto will dump and call powerdown two times. so if fail, just return.

[plbossart]

@zhuyingjiang sorry, I don't see what would cause this loop to be executed twice. the flow is quite unclear to me here.

[plbossart]

this is really not related to the timeout, you need to provide patches that match the commit message

[zhuyingjiang]

@plbossart OK, will sub another PR

[plbossart]

Why would you use 2 cores by default? This makes no sense. It's not because you can that you should, it's horrible in terms of power consumption.

[lgirdwood]

iirc, this is copied exactly from SKL driver.

[ranj063]

@lgirdwood @zhuyingjiang I dont think the SKL driver does this. But even if it does, we have the means to bring up the cores when there are pipelines scheduled to run on them. I dont think it is necessary to power them all up at boot.

[zhuyingjiang]

OK, let's keep only one core enabled at first, so just remove the second commit is OK.

[zhuyingjiang]

@plbossart Updated

[plbossart]

the explanations on patch2 aren't clear to me. You know I am paranoid about error flows so I'd like to understand a bit more what problem this patch2 solves.

[plbossart]

@zhuyingjiang sorry, I don't see what would cause this loop to be executed twice. the flow is quite unclear to me here.

[zhuyingjiang]

@plbossart It is not loop twice, when cl_dsp_init_skl can't enable the core, it will return a ret which is less than 0, and will execute the following flow inside cl_dsp_init_skl to dump the registers and reset/power down the core:

hda_dsp_dump_skl(sdev, SOF_DBG_REGS | SOF_DBG_PCI | SOF_DBG_MBOX);
cl_cleanup_skl(sdev);
hda_dsp_core_reset_power_down(sdev, HDA_DSP_CORE_MASK(0));

if here is goto irq_err;, it will execute the same flow at the end of hda_dsp_cl_boot_firmware_skl, it means the dump and reset has been execute twice. So this is a bug and just return here can solve this.

[ranj063]

@plbossart It is not loop twice, when cl_dsp_init_skl can't enable the core, it will return a ret which is less than 0, and will execute the following flow inside cl_dsp_init_skl to dump the registers and reset/power down the core:

hda_dsp_dump_skl(sdev, SOF_DBG_REGS | SOF_DBG_PCI | SOF_DBG_MBOX);
cl_cleanup_skl(sdev);
hda_dsp_core_reset_power_down(sdev, HDA_DSP_CORE_MASK(0));

if here is goto irq_err;, it will execute the same flow at the end of hda_dsp_cl_boot_firmware_skl, it means the dump and reset has been execute twice. So this is a bug and just return here can solve this.

@zhuyingjiang the sequence in boot_firmware() in loader.c has recently been updated to do a proper cleanup in case of failures. Could you please take a look and align the skl loader along similar lines?

[plbossart]

Agree with @ranj063 the error flow doesn't seem quite right

[zhuyingjiang]

@plbossart @ranj063 Thanks for the comments, I am working on the S3 issue and will change this PR some later.

[zhuyingjiang]

@zhuyingjiang the sequence in boot_firmware() in loader.c has recently been updated to do a proper cleanup in case of failures. Could you please take a look and align the skl loader along similar lines?

@ranj063
As i checked the hda_dsp_cl_boot_firmware in hda-loader.c, there are two goto: "cleanup:" and "err:", I think that is for fail of get_stream_with_tag and error handle of cl_dsp_init, but in SKL, we use codeloader DMA instead of one of the stream DMA, so no get_stream_with_tag there. That means we can handle the error with one "goto". So my understand is that hda_dsp_cl_boot_firmware_skl don't need change to align with hda_dsp_cl_boot_firmware

[ranj063]

@zhuyingjiang the skl loader needs a bit of rework. here're my comments:

1. Your argument that there's no need to dump regs if cl_dsp_init_skl() fails is correct. But we still need to indicate fw load failure and return the return value.
2. Secondly, looking into cl_copy_fw_skl(), it looks like this funcions performs cl_cleanup_skl() in case it is successful. But when it fails, there doesnt seem to be a need to perform the clean up at all. So I would remove the cleanup_skl() call in hda_dsp_cl_boot_firmware_skl() after the irq_err:
3. In the function cl_skl_cldma_copy_to_buf(), although the logic is correct, it looks like we will never hit the "return bytes". bytes_left will never be 0 to begin with. So I'd rather just have a break in the else {} block and return 0 at the end.

[zhuyingjiang]

@zhuyingjiang the skl loader needs a bit of rework. here're my comments:

1. Your argument that there's no need to dump regs if cl_dsp_init_skl() fails is correct. But we still need to indicate fw load failure and return the return value.
2. Secondly, looking into cl_copy_fw_skl(), it looks like this funcions performs cl_cleanup_skl() in case it is successful. But when it fails, there doesnt seem to be a need to perform the clean up at all. So I would remove the cleanup_skl() call in hda_dsp_cl_boot_firmware_skl() after the irq_err:
3. In the function cl_skl_cldma_copy_to_buf(), although the logic is correct, it looks like we will never hit the "return bytes". bytes_left will never be 0 to begin with. So I'd rather just have a break in the else {} block and return 0 at the end.

@ranj063 seems what you mention is not related to the "ROM init timeout", Should that go with another refine PR? I heard that it will lost track if the title message and the code are not related.

[ranj063]

@ranj063 seems what you mention is not related to the "ROM init timeout", Should that go with another refine PR? I heard that it will lost track if the title message and the code are not related.

Yes,please.

[zhuyingjiang]

OK, I removed the "skl fix a bug of error handle" commit and keep the commit for "skl make ROM init timeout skl specific" and "skl fix some comments error". @plbossart @ranj063 Could you approve and merge the kept ROM init part? I will create a issue and a follow PR to refine the skl loader code.

[ranj063]

@zhuyingjiang i just looked back into my PR and i've not touched the HDA_DSP_INIT_TIMEOUT but only removed the HDA_ROM_INIT_TIMEOUT. Although it looks like we should be using the rom init timeout here, I wonder why we were using HDA_DSP_INIT_TIMEOUT before.

How did you test this? I am confused about whether this is correct or not.

[ranj063]

besides the register poll method has been changed recently. Can you please fix the conflicts?

[zhuyingjiang]

@ranj063 As I have mentioned in #704

Yes, SKL still need this timeout. As I test, the HDA_ROM_INIT_TIMEOUT which is 70ms is some short, So I use the already defined HDA_DSP_INIT_TIMEOUT 500ms, If this can be platform specific, 150 ms can work for SKL.

[zhuyingjiang]

besides the register poll method has been changed recently. Can you please fix the conflicts?

Done.

[ranj063]

@ranj063 As I have mentioned in #704

Yes, SKL still need this timeout. As I test, the HDA_ROM_INIT_TIMEOUT which is 70ms is some short, So I use the already defined HDA_DSP_INIT_TIMEOUT 500ms, If this can be platform specific, 150 ms can work for SKL.

@zhuyingjiang sorry I missed the previous comment. Thanks

[zhuyingjiang]

@plbossart Could you approve and merge this? I will create a issue and follow a PR to do the refine of skl code loader.

[plbossart]

@zhuyingjiang I will merge this one but this is the last one. I don't want anything related to SKL unless we have a real plan to support this platform.

[zhuyingjiang]

@plbossart OK, thanks!