[coreboot-gerrit] [M] Change in coreboot[master]: soc/intel/cmn/tom: Cache TOM region early

[Subrata Banik (Code Review)]

Attention is currently required from: Tarun Tuli, Jamie Ryu, Paul Menzel, Kapil Porwal, Arthur Heymans.

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73272 )

Change subject: soc/intel/cmn/tom: Cache TOM region early
......................................................................

Patch Set 4:

(2 comments)

File src/soc/intel/common/basecode/tom/tom.c:

https://review.coreboot.org/c/coreboot/+/73272/comment/982ec42c_ead76038 
PS4, Line 20: #defin
> > > Assert that it does not overlap with other cmos options.
> >
> > looking at the other files which uses the CMOS offset for storing details previously. Unable to find something which resembles with ur feedback.
> >
> > Can you please share more details about what/how u wish to do this check of non-overlapping assets ?
>
> soc/intel/xeon_sp/cpx/romstage.c has a good example

Are you referring to below code?

```
#if CONFIG(USE_OPTION_TABLE)
#include "option_table.h"
#if CMOS_VSTART_mrc_status != CMOS_OFFSET_MRC_STATUS * 8
#error "CMOS start for CPX-SP MRC status byte is not correct, check your cmos.layout"
#endif
#if CMOS_VLEN_mrc_status != 8
#error "CMOS length for CPX-SP MRC status byte is not correct, check your cmos.layout"
#endif
#endif
```

Then I believe it's for platform uses `USE_OPTION_TABLE` and I don't see this config being used on latest Intel platform hence, covered that in line 12-13 and Kconfig depends as well.

https://review.coreboot.org/c/coreboot/+/73272/comment/d97c26b6_39347651 
PS4, Line 113: tom
> > > Does TOM always have a 16M granularity?
> >
> > Atleast on the client side, I'm seeing the same in last 5 generations.
>
> Ok. Maybe add this as a comment?
> > > Also on xeon_sp 16M does not cut it. Maybe use mtrr_ctx like on AMD code to cache the region below TOM as WB? See void early_cache_setup(void) for that.
> >
> > I believe I'm seeing hang with WB and mostly while postcar is tries to tear-down the CAR.
>
> Right. https://review.coreboot.org/c/coreboot/+/37196 should help.
> > i hope u r suggesting like this ?
> > set_var_mtrr(&mtrr_ctx.ctx, 0, tom, MTRR_TYPE_WRBACK);
>
> yes

I'm seeing continuous reboot when tries to configure MTRR as above (before even jumping into FSP)

```
[DEBUG]  0x0000000000000005: PHYBASE4: Address = 0x0000000000000000, WP
[DEBUG]  0x00003fff80000800: PHYMASK4: Length  = 0x0000000080000000, Valid
```