Change in coreboot[master]: soc/intel/tgl: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Jamie Ryu (Code Review)]

Jamie Ryu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tgl: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

soc/intel/tgl: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

This is a W/A to avoid a communication issue with CSE Lite over Heci
interface. This will help to avoid boot failures with CSE Lite until
the permanent fix is available.

BUG=b:158643194
TEST=build and boot volteer with serial and non-serial image

Change-Id: Ib136a2154b36c63c7147bbcfbf1ca7beac3a5685
Signed-off-by: Jamie Ryu 
---
M src/soc/intel/common/block/cse/cse_lite.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/42790/1

diff --git a/src/soc/intel/common/block/cse/cse_lite.c b/src/soc/intel/common/block/cse/cse_lite.c
index 3077c7f..a0b1e10 100644
--- a/src/soc/intel/common/block/cse/cse_lite.c
+++ b/src/soc/intel/common/block/cse/cse_lite.c
@@ -361,4 +361,4 @@
    }
 }

-BOOT_STATE_INIT_ENTRY(BS_PRE_DEVICE, BS_ON_ENTRY, cse_fw_sync, NULL);
+BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_ENTRY, cse_fw_sync, NULL);

[Jamie Ryu (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42790

to look at the new patch set (#2).

Change subject: soc/intel/tgl: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

soc/intel/tgl: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

This is a W/A to avoid a communication issue with CSE Lite over Heci
interface. This will help to avoid boot failures with CSE Lite until
the permanent fix is available.

BUG=b:159884143
TEST=build and boot volteer with serial and non-serial image

Change-Id: Ib136a2154b36c63c7147bbcfbf1ca7beac3a5685
Signed-off-by: Jamie Ryu 
---
M src/soc/intel/common/block/cse/cse_lite.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/42790/2

[Sridhar Siricilla (Code Review)]

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tgl: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42790/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/42790/2/src/soc/intel/common/block/... 
PS2, Line 364: BS_DEV_INIT_CHIPS
This will affect CML/JSL platforms. Without evaluating , we cann't have this change.

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Jamie Ryu (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42790

to look at the new patch set (#4).

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

This is a W/A to avoid a communication issue with CSE Lite over Heci
interface. This will help to avoid boot failures with CSE Lite until
the permanent fix is available.

BUG=b:159884143
TEST=build and boot volteer with serial and non-serial image

Change-Id: Ib136a2154b36c63c7147bbcfbf1ca7beac3a5685
Signed-off-by: Jamie Ryu 
---
M src/soc/intel/common/block/cse/cse_lite.c
1 file changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/42790/4

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Jamie Ryu (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Duncan Laurie, Angel Pons, Nick Vaccaro, Raj Astekar, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42790

to look at the new patch set (#5).

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

This is a W/A to avoid a communication issue with CSE Lite over Heci
interface. This will help to avoid boot failures with CSE Lite until
the permanent fix is available.

BUG=b:159884143
TEST=build and boot volteer with serial and non-serial image

Change-Id: Ib136a2154b36c63c7147bbcfbf1ca7beac3a5685
Signed-off-by: Jamie Ryu 
---
M src/soc/intel/common/block/cse/cse_lite.c
1 file changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/42790/5

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

Patch Set 5: Code-Review-1

Is there a reason this can't go in soc/intel/tigerlake/me?

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

Patch Set 5:

what about a Kconfig for which ramstage stage to run at?

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

Patch Set 5:
> Patch Set 5: Code-Review-1
>
> Is there a reason this can't go in soc/intel/tigerlake/me?

Ideally calling the sync in BS_DEV_INIT_CHIPS should have been common for all SoCs, and hence there was no choice provided to the SoC code to choose where the sync was called.

To address the the TGL anomaly we can
1. Add a SoC specific switch like this patch
2. Create couple Kconfig options, by which SoC can select which stage to call the sync
   One to specify the stage and the other for entry or exit
   Will require some plumbing to handle the stage and entry exit options
3. Or provide full freedom to SoC code to call this function wherever it wants
   this will require to make sure it calls the cse_fw_sync function apart from selecting the Kconfig
   SoC code is responsible for keeping in mind the valid scope/stage of calling this function

Any simpler approach
Since this is a more of a workaround until we root cause and fix the issue, option 1 seems like a simpler one to choose.
Tim, Let me know your thoughts

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

Patch Set 5: -Code-Review

Ok, I had missed some of the background here. Thank for your analysis Rizwan. A straight-up revert when this is not needed would be great.

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

Patch Set 5:

(1 comment)
> Patch Set 5: -Code-Review
>
> Ok, I had missed some of the background here. Thank for your analysis Rizwan. A straight-up revert when this is not needed would be great.

Yes, we can just revert when 159884143 is fixed

https://review.coreboot.org/c/coreboot/+/42790/5/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/42790/5/src/soc/intel/common/block/... 
PS5, Line 364: #if CONFIG(SOC_INTEL_TIGERLAKE)
             : BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_ENTRY, cse_fw_sync, NULL);
             : #else
Although mute, please add a TODO to indicate that this should be removed once the issue is fixed in the CSE firmware i.e when we have a true fix for b:159884143

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42790/5/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/42790/5/src/soc/intel/common/block/... 
PS5, Line 364: #if CONFIG(SOC_INTEL_TIGERLAKE)
             : BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_ENTRY, cse_fw_sync, NULL);
             : #else
> Although mute, please add a TODO to indicate that this should be removed once the issue is fixed in  […]

I added a note to the bug that this CL needs to be reverted as part of the bug closing process.

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Jamie Ryu (Code Review)]

Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

Patch Set 6: Code-Review+1

I am working to root cause the issue and this patch will be reverted when new change is added to fix 159884143.

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/42790/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/42790/2/src/soc/intel/common/block/... 
PS2, Line 364: BS_DEV_INIT_CHIPS
> This will affect CML/JSL platforms. Without evaluating , we cann't have this change.

Ack

https://review.coreboot.org/c/coreboot/+/42790/5/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/42790/5/src/soc/intel/common/block/... 
PS5, Line 364: #if CONFIG(SOC_INTEL_TIGERLAKE)
             : BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_ENTRY, cse_fw_sync, NULL);
             : #else
> I added a note to the bug that this CL needs to be reverted as part of the bug closing process.

Ack

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

This is a W/A to avoid a communication issue with CSE Lite over Heci
interface. This will help to avoid boot failures with CSE Lite until
the permanent fix is available.

BUG=b:159884143
TEST=build and boot volteer with serial and non-serial image

Change-Id: Ib136a2154b36c63c7147bbcfbf1ca7beac3a5685
Signed-off-by: Jamie Ryu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42790
Reviewed-by: Nick Vaccaro 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/common/block/cse/cse_lite.c
1 file changed, 4 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nick Vaccaro: Looks good to me, approved
  Jamie Ryu: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/common/block/cse/cse_lite.c b/src/soc/intel/common/block/cse/cse_lite.c
index 3077c7f..8e43e35 100644
--- a/src/soc/intel/common/block/cse/cse_lite.c
+++ b/src/soc/intel/common/block/cse/cse_lite.c
@@ -361,4 +361,8 @@
    }
 }

+#if CONFIG(SOC_INTEL_TIGERLAKE)
+BOOT_STATE_INIT_ENTRY(BS_DEV_INIT_CHIPS, BS_ON_ENTRY, cse_fw_sync, NULL);
+#else
 BOOT_STATE_INIT_ENTRY(BS_PRE_DEVICE, BS_ON_ENTRY, cse_fw_sync, NULL);
+#endif

Change in coreboot[master]: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42790 )

Change subject: soc/intel/tigerlake: Switch to CSE Lite RW at BS_DEV_INIT_CHIPS entry
......................................................................

Patch Set 7: Code-Review+2