[keyonjie]

We are using the lowest 2 digits of the ROM status register to denote
the status code, so the mask for it should be 0xff, here correct it.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

[keyonjie]

@ClarexZhou can you help try if this PR can fix #969 please?

@xiulipan as discussed, may be helpful for your boot failure also.

[xiulipan]

@keyonjie
Some comments issue:
lowest 2 digits => lowest 2 bytes

And maybe you can mentioned the error we meet here to treat 0x600021 as 0x500001 when use 0xf as mask.

Other looks fine

[kv2019i]

@xiulipan wrote:

Some comments issue:
lowest 2 digits => lowest 2 bytes

Hmm, the mask (0xff) in patch is for one byte still, which is right, description or patch? @keyonjie ?

[keyonjie]

@xiulipan wrote:

Some comments issue:
lowest 2 digits => lowest 2 bytes

Hmm, the mask (0xff) in patch is for one byte still, which is right, description or patch? @keyonjie ?

yes, it is 8 bits = 2 digits(in hex) = 1 bytes, let me use "bits" to make it clearer.

[RanderWang]

LGTM

[kv2019i]

Thanks, looks good now!

[plbossart]

the commit message can be improved.

"Without this change, we treated status like 0x6000021 (0x1 as the lowest
4 bits) as HDA_DSP_ROM_INIT (0x1 as the lowest 4 bits)"

This value 0x6000021 is not listed as a valid code in the defines, so this looks like a work-around without fixing the root cause. Or you have to explain what this code is and what using the full 8 bits will do.

While we are at it, it's very odd to have a mix of hex and decimal values as error codes. this is ugly and should be fixed.

Also you have a grammar error, it's "downloading too early", not "too earlier"

[kv2019i]

@plbossart @keyonjie wrote:

"Without this change, we treated status like 0x6000021 (0x1 as the lowest
4 bits) as HDA_DSP_ROM_INIT (0x1 as the lowest 4 bits)"

This value 0x6000021 is not listed as a valid code in the defines, so this looks like a work-around
without fixing the root cause. Or you have to explain what this code is and what using the full 8 bits
will do.
[...]
While we are at it, it's very odd to have a mix of hex and decimal values as error codes. this is
ugly and should be fixed.

I was trying earlier today to find a good reference for this when reviewing. Probably this is best:
https://thesofproject.github.io/latest/architectures/dsp/intel/cavs-boot/apollolake/apl-boot-rom.html

ROM_STATUS has values larger than 0xf, so the patch seems correct. And if this indeed helps with #969 , this is a very nice find (given the difficult of reproducing the problem).

But yeah, some background on 0x6000021 would be good. I believe ROM uses similar approach as FW_STATUS to use upper 24bit to report state while processing is ongoing (documented in above link). @keyonjie can you confirm this?

[ranj063]

I am also a bit confused about the commit message. I agree that 0xff is the right status mask but then the INIT_DONE status is only 0x1. So do you mean to say that 0x21 isnt the right status and should be ignored until the 0x01 is read?

[keyonjie]

I am also a bit confused about the commit message. I agree that 0xff is the right status mask but then the INIT_DONE status is only 0x1. So do you mean to say that 0x21 isnt the right status and should be ignored until the 0x01 is read?

yes.

[keyonjie]

@plbossart @keyonjie wrote:

"Without this change, we treated status like 0x6000021 (0x1 as the lowest
4 bits) as HDA_DSP_ROM_INIT (0x1 as the lowest 4 bits)"
This value 0x6000021 is not listed as a valid code in the defines, so this looks like a work-around
without fixing the root cause. Or you have to explain what this code is and what using the full 8 bits
will do.
[...]
While we are at it, it's very odd to have a mix of hex and decimal values as error codes. this is
ugly and should be fixed.

I was trying earlier today to find a good reference for this when reviewing. Probably this is best:
https://thesofproject.github.io/latest/architectures/dsp/intel/cavs-boot/apollolake/apl-boot-rom.html

ROM_STATUS has values larger than 0xf, so the patch seems correct. And if this indeed helps with #969 , this is a very nice find (given the difficult of reproducing the problem).

But yeah, some background on 0x6000021 would be good. I believe ROM uses similar approach as FW_STATUS to use upper 24bit to report state while processing is ongoing (documented in above link). @keyonjie can you confirm this?

Yes, the above link you provided is for APL, for 1.8, I believe they have extended the state that's why we are seeing 0x21 on CNL/WHL/TGL. Maybe a new page in the document for 1.8 will be helpful, @jajanusz can you help on that?

union fw_status_reg
{
        int32_t full;
        struct Bits
        {
                uint32_t state      : 24;
                uint32_t wait_state : 4;
                uint32_t module     : 3;
                uint32_t running    : 1;
        } bits;
};

[ranj063]

@keyonjie how about something like this:
"ROM status is represented by the 8 LSB bits in the status register. So the status mask should be 0xff instead of 0xf. Without this change, status such as 0x6000021 (0x1 as the lowest 4 bits) could be treated as as HDA_DSP_ROM_INIT (0x1 as the lowest 4 bits) and FW loading could end up getting started too soon resulting in failures."

[keyonjie]

the commit message can be improved.

"Without this change, we treated status like 0x6000021 (0x1 as the lowest
4 bits) as HDA_DSP_ROM_INIT (0x1 as the lowest 4 bits)"

This value 0x6000021 is not listed as a valid code in the defines, so this looks like a work-around without fixing the root cause. Or you have to explain what this code is and what using the full 8 bits will do.

As I just replied to @kv2019i, the 0x21 is new extended in cAVS 1.8, that's why we don't see it the document for APL.

Ideally we should use 24 bits according the definition of fw_status_reg, I chose 8 bits only as what ROM used until now has not exceed 8 bits yet.

union fw_status_reg
{
        int32_t full;
        struct Bits
        {
                uint32_t state      : 24;
                uint32_t wait_state : 4;
                uint32_t module     : 3;
                uint32_t running    : 1;
        } bits;
};

While we are at it, it's very odd to have a mix of hex and decimal values as error codes. this is ugly and should be fixed.

I think they are defining state macros in hex and error code in decimal in ROM header file.

Also you have a grammar error, it's "downloading too early", not "too earlier"

Thanks, change it now.

[keyonjie]

@keyonjie how about something like this:
"ROM status is represented by the 8 LSB bits in the status register. So the status mask should be 0xff instead of 0xf. Without this change, status such as 0x6000021 (0x1 as the lowest 4 bits) could be treated as as HDA_DSP_ROM_INIT (0x1 as the lowest 4 bits) and FW loading could end up getting started too soon resulting in failures."

Thanks, but this still not addressing @plbossart's comment about what 0x6000021 is, and why it is the 8 LSB bits.

I am considering to drop this description about 0x6000021, and change the mask to lowest 24 bits, any comment? @ranj063 @plbossart @kv2019i

[ranj063]

I am considering to drop this description about 0x6000021, and change the mask to lowest 24 bits, any comment? @ranj063 @plbossart @kv2019i

@keyonjie yes, I think it is the right thing to do.

[xiulipan]

LGTM.

[ranj063]

@keyonjie pointing to the APL ROM documentation will make people wonder if it applies to all platforms. Can you please clarify it in the commit message?

[keyonjie]

@keyonjie pointing to the APL ROM documentation will make people wonder if it applies to all platforms. Can you please clarify it in the commit message?

@ranj063 The 24 bits mask apply to all platforms.

[ClarexZhou]

Error #969 not occurred with #1089 after run 2k interactions s3 without playback stress test on both ICL HD-A RVP and CFL HD-A notebook.
Kernel: topic/sof-dev: 04b9bfe + PR#1089

[jajanusz]

Yes, the above link you provided is for APL, for 1.8, I believe they have extended the state that's why we are seeing 0x21 on CNL/WHL/TGL. Maybe a new page in the document for 1.8 will be helpful, @jajanusz can you help on that?

What do you mean by extended the state?
The fact that you see f.e. something like this for cnl rom code:

uint32_t state      : 28;
uint32_t module     : 3;
uint32_t running    : 1;

vs what is in docs:

uint32_t state      : 24;
uint32_t wait_state : 4;
uint32_t module     : 3;
uint32_t running    : 1;

doesn't change the fact that it works as it is in docs, these 4 bits (noted as wait_state) are handled in the same way anyway.

It depends on how you look at state overall, usually 24 later bits are changing less often as it is some bigger part f.e. CSE verification, while wait_state will change often meantime, f.e. for every ipc (wait/busy/done/etc)

[keyonjie]

Yes, the above link you provided is for APL, for 1.8, I believe they have extended the state that's why we are seeing 0x21 on CNL/WHL/TGL. Maybe a new page in the document for 1.8 will be helpful, @jajanusz can you help on that?

What do you mean by extended the state?

I meant new 0x21(xx_IPC_RESET_PHASE_1) added for 1.8 only.

The fact that you see f.e. something like this for cnl rom code:

uint32_t state      : 28;
uint32_t module     : 3;
uint32_t running    : 1;

vs what is in docs:

uint32_t state      : 24;
uint32_t wait_state : 4;
uint32_t module     : 3;
uint32_t running    : 1;

doesn't change the fact that it works as it is in docs, these 4 bits (noted as wait_state) are handled in the same way anyway.

It depends on how you look at state overall, usually 24 later bits are changing less often as it is some bigger part f.e. CSE verification, while wait_state will change often meantime, f.e. for every ipc (wait/busy/done/etc)

Thanks for explaining @jajanusz , yes, that's true, previously we only used the LSB 4 bits of the state, here we are correcting it to use the whole LSB 24 bits.

[kv2019i]

uint32_t state      : 24;
uint32_t wait_state : 4;
uint32_t module     : 3;
uint32_t running    : 1;

A pedantic note @jajanusz @keyonjie : bitfields in C are highly compiler and platform dependent, so in documenting binary interfaces (seen by multiple CPUs), better to simply use plain ol' bitmasks and document endianess.

Here also mismatch between use of "state" and "status" is confusing.

Anyways, patch looks good!

[jajanusz]

uint32_t state      : 24;
uint32_t wait_state : 4;
uint32_t module     : 3;
uint32_t running    : 1;

A pedantic note @jajanusz @keyonjie : bitfields in C are highly compiler and platform dependent, so in documenting binary interfaces (seen by multiple CPUs), better to simply use plain ol' bitmasks and document endianess.

Here also mismatch between use of "state" and "status" is confusing.

Anyways, patch looks good!

We don't use them in FW code, these bitfields are from rom code that we don't develop ;)

[plbossart]

nit-pick below

[plbossart]

GENMASK(23, 0)?
this will be an upstream comment so might as well fix it now.

[keyonjie]

yes, changing it now.

[kv2019i]

A pedantic note @jajanusz @keyonjie : bitfields in C are highly compiler and platform dependent, so >
We don't use them in FW code, these bitfields are from rom code that we don't develop ;)

@jajanusz Ok good, just checking :)

[plbossart]

Thanks @keyonjie