[M] Change in coreboot[main]: util/ifdtool: Add new cmdline to enable GPR0 protection

[Subrata Banik (Code Review)]

Attention is currently required from: Reka Norman.

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80235?usp=email )

Change subject: util/ifdtool: Add new cmdline to enable GPR0 protection
......................................................................

Patch Set 3:

(12 comments)

File util/ifdtool/ifdtool.c:

https://review.coreboot.org/c/coreboot/+/80235/comment/6dc86e82_6bcab88d :
PS3, Line 47: CSE_REGION_DATA_PARTITION_OFFSET
> > > This applies to BPDT v1.7 (TGL onwards) only, referring to util/cbfstool/bpdt_formats/{bpdt_1_6. […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/80235/comment/7f286198_958f95d4 :
PS3, Line 1556: uint32_t gprd
> nit: make `union gprd_info` public and use it here instead of manual bit shifting?

Acknowledged

https://review.coreboot.org/c/coreboot/+/80235/comment/4d0998ba_28bded66 :
PS3, Line 1610: fpsba->pchstrp[gpr0_offset] & 0x80000000
> nit: use `union gprd_info` here too

Acknowledged

https://review.coreboot.org/c/coreboot/+/80235/comment/99308517_5e108a39 :
PS3, Line 1624: FIT
> FITC?

Acknowledged

https://review.coreboot.org/c/coreboot/+/80235/comment/9361dc4f_9d0af6b2 :
PS3, Line 1624: FITC
> FPT?

Acknowledged

https://review.coreboot.org/c/coreboot/+/80235/comment/a9b6168f_dd17014e :
PS3, Line 1648: }
> Fail if FITC is not found? Right now it will silently continue and fill in an incorrect range.

Acknowledged

https://review.coreboot.org/c/coreboot/+/80235/comment/5ea2f6e6_db12f68b :
PS3, Line 1663: region.size <= 0xfff
> Why 0xfff (not 0)?

Acknowledged

https://review.coreboot.org/c/coreboot/+/80235/comment/1c01891d_e0cc7c35 :
PS3, Line 1712: fpsba->pchstrp[gpr0_offset] & 0x80000000
> nit: use `union gprd_info` here too

done

https://review.coreboot.org/c/coreboot/+/80235/comment/726af3cb_bd3bd9b7 :
PS3, Line 1724: union gprd_info
> nit: just `union gprd`? This is more consistent with other places where we just use the register nam […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/80235/comment/003cdadb_47702446 :
PS3, Line 1726: start:16
> I think bit 16 is actually read protect enable, referring to `union spi_bios_gpr0` in src/soc/intel/ […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/80235/comment/6c474f45_5af4addd :
PS3, Line 1733: 	/*
              : 	 * Start Address: bit 0-15 of the GPRD represents the protected region start address,
              : 	 * where bit 0-11 of the start address are assumed to be zero.
              : 	 */
> nit: maybe move these comments to the struct definition, so we don't have to duplicate them here and […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/80235/comment/41efbe1a_b678f672 :
PS3, Line 1737: 0xffff
> 0x7fff, based on my comment about read protect enable.

Acknowledged