Change in coreboot[master]: util/ifdtool: Add support for setting flash density on IFD V2

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36101 )

Change subject: util/ifdtool: Add support for setting flash density on IFD V2
......................................................................

util/ifdtool: Add support for setting flash density on IFD V2

Change-Id: Ibc3e4c197f99f99007cb208cf6cc4ae6f56be70c
Signed-off-by: Arthur Heymans 
---
M util/ifdtool/ifdtool.c
1 file changed, 34 insertions(+), 32 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/36101/1

diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c
index 0e83c76..7747ec6 100644
--- a/util/ifdtool/ifdtool.c
+++ b/util/ifdtool/ifdtool.c
@@ -921,6 +921,7 @@
                             unsigned int density)
 {
    fcba_t *fcba = find_fcba(image, size);
+	u8 mask, chip2_offset;
    if (!fcba)
    	exit(EXIT_FAILURE);

@@ -938,11 +939,12 @@
    		exit(EXIT_FAILURE);
    	}
    	break;
+		mask = 0x7;
+		chip2_offset = 3;
    case IFD_VERSION_2:
-		/* I do not have a version 2 IFD nor do i have the docs. */
-		printf("error: Changing the chip density for IFD version 2 has not been"
-		       " implemented yet.\n");
-		exit(EXIT_FAILURE);
+		break;
+		mask = 0xf;
+		chip2_offset = 4;
    default:
    	printf("error: Unknown IFD version\n");
    	exit(EXIT_FAILURE);
@@ -952,13 +954,13 @@
    /* clear chip density for corresponding chip */
    switch (selected_chip) {
    case 1:
-		fcba->flcomp &= ~(0x7);
+		fcba->flcomp &= ~mask;
    	break;
    case 2:
-		fcba->flcomp &= ~(0x7 << 3);
+		fcba->flcomp &= ~(mask << chip2_offset);
    	break;
    default: /*both chips*/
-		fcba->flcomp &= ~(0x3F);
+		fcba->flcomp &= ~(mask | (mask << chip2_offset));
    	break;
    }

@@ -966,7 +968,7 @@
    if (selected_chip == 1 || selected_chip == 0)
    	fcba->flcomp |= (density); /* first chip */
    if (selected_chip == 2 || selected_chip == 0)
-		fcba->flcomp |= (density << 3); /* second chip */
+		fcba->flcomp |= (density << chip2_offset); /* second chip */

write_image(filename, image, size);
 }
@@ -1403,30 +1405,30 @@
 {
    printf("usage: %s [-vhdix?] <filename>\n", name);
    printf("\n"
-	       "   -d | --dump:                       dump intel firmware descriptor\n"
-	       "   -f | --layout <filename>           dump regions into a flashrom layout file\n"
-	       "   -t | --validate                    Validate that the firmware descriptor layout matches the fmap layout\n"
-	       "   -x | --extract:                    extract intel fd modules\n"
-	       "   -i | --inject <region>:<module>    inject file <module> into region <region>\n"
-	       "   -n | --newlayout <filename>        update regions using a flashrom layout file\n"
-	       "   -s | --spifreq <17|20|30|33|48|50> set the SPI frequency\n"
-	       "   -D | --density <512|1|2|4|8|16>    set chip density (512 in KByte, others in MByte)\n"
-	       "   -C | --chip <0|1|2>                select spi chip on which to operate\n"
-	       "                                      can only be used once per run:\n"
-	       "                                      0 - both chips (default), 1 - first chip, 2 - second chip\n"
-	       "   -e | --em100                       set SPI frequency to 20MHz and disable\n"
-	       "                                      Dual Output Fast Read Support\n"
-	       "   -l | --lock                        Lock firmware descriptor and ME region\n"
-	       "   -u | --unlock                      Unlock firmware descriptor and ME region\n"
-	       "   -M | --altmedisable <0|1>          Set the AltMeDisable (or HAP for skylake or newer platform)\n"
-	       "                                      bit to disable ME\n"
-	       "   -p | --platform                    Add platform-specific quirks\n"
-	       "                                      aplk - Apollo Lake\n"
-	       "                                      cnl - Cannon Lake\n"
-	       "                                      glk - Gemini Lake\n"
-	       "                                      sklkbl - Skylake/Kaby Lake\n"
-	       "   -v | --version:                    print the version\n"
-	       "   -h | --help:                       print this help\n\n"
+	       "   -d | --dump:                          dump intel firmware descriptor\n"
+	       "   -f | --layout <filename>              dump regions into a flashrom layout file\n"
+	       "   -t | --validate                       Validate that the firmware descriptor layout matches the fmap layout\n"
+	       "   -x | --extract:                       extract intel fd modules\n"
+	       "   -i | --inject <region>:<module>       inject file <module> into region <region>\n"
+	       "   -n | --newlayout <filename>           update regions using a flashrom layout file\n"
+	       "   -s | --spifreq <17|20|30|33|48|50>    set the SPI frequency\n"
+	       "   -D | --density <512|1|2|4|8|16|32|64> set chip density (512 in KByte, others in MByte)\n"
+	       "   -C | --chip <0|1|2>                   select spi chip on which to operate\n"
+	       "                                         can only be used once per run:\n"
+	       "                                         0 - both chips (default), 1 - first chip, 2 - second chip\n"
+	       "   -e | --em100                          set SPI frequency to 20MHz and disable\n"
+	       "                                         Dual Output Fast Read Support\n"
+	       "   -l | --lock                           Lock firmware descriptor and ME region\n"
+	       "   -u | --unlock                         Unlock firmware descriptor and ME region\n"
+	       "   -M | --altmedisable <0|1>             Set the AltMeDisable (or HAP for skylake or newer platform)\n"
+	       "                                         bit to disable ME\n"
+	       "   -p | --platform                       Add platform-specific quirks\n"
+	       "                                         aplk - Apollo Lake\n"
+	       "                                         cnl - Cannon Lake\n"
+	       "                                         glk - Gemini Lake\n"
+	       "                                         sklkbl - Skylake/Kaby Lake\n"
+	       "   -v | --version:                       print the version\n"
+	       "   -h | --help:                          print this help\n\n"
           "<region> is one of Descriptor, BIOS, ME, GbE, Platform\n"
           "\n");
 }

[Arthur Heymans (Code Review)]

Hello Stefan Reinauer, build bot (Jenkins),

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

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

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

Change subject: util/ifdtool: Add support for setting flash density on IFD V2
......................................................................

util/ifdtool: Add support for setting flash density on IFD V2

Change-Id: Ibc3e4c197f99f99007cb208cf6cc4ae6f56be70c
Signed-off-by: Arthur Heymans 
---
M util/ifdtool/ifdtool.c
1 file changed, 34 insertions(+), 32 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/36101/2

[Arthur Heymans (Code Review)]

Hello Stefan Reinauer, build bot (Jenkins),

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

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

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

Change subject: util/ifdtool: Add support for setting flash density on IFD V2
......................................................................

util/ifdtool: Add support for setting flash density on IFD V2

Change-Id: Ibc3e4c197f99f99007cb208cf6cc4ae6f56be70c
Signed-off-by: Arthur Heymans 
---
M util/ifdtool/ifdtool.c
1 file changed, 34 insertions(+), 32 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/36101/3

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36101 )

Change subject: util/ifdtool: Add support for setting flash density on IFD V2
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/36101/3/util/ifdtool/ifdtool.c 
File util/ifdtool/ifdtool.c:

https://review.coreboot.org/c/coreboot/+/36101/3/util/ifdtool/ifdtool.c@942 
PS3, Line 942: 		chip2_offset = 3;
This is not true for all "V1" descriptors btw. The component
width change to 4 bits with 8 series.

The V1/V2 disctinction in ifdtool seems even more arbitrary
than the changes Intel makes to the descriptor oO If anyone
has the time: It would be a good idea to extract the IFD
code from flashrom into a common library.

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36101 )

Change subject: util/ifdtool: Add support for setting flash density on IFD V2
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36101/3/util/ifdtool/ifdtool.c 
File util/ifdtool/ifdtool.c:

https://review.coreboot.org/c/coreboot/+/36101/3/util/ifdtool/ifdtool.c@942 
PS3, Line 942: 		chip2_offset = 3;
> If anyone has the time: It would be a good idea to extract the IFD code from flashrom into a common library.

Could you put that in Documentation/contributing/project_ideas.md?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36101 )

Change subject: util/ifdtool: Add support for setting flash density on IFD V2
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36101/3/util/ifdtool/ifdtool.c 
File util/ifdtool/ifdtool.c:

https://review.coreboot.org/c/coreboot/+/36101/3/util/ifdtool/ifdtool.c@942 
PS3, Line 942: 		chip2_offset = 3;
Done (wasn't meant as something to do here anyway, behaviour for V1 isn't changed).
> Could you put that in Documentation/contributing/project_ideas.md?

Is that the right place? I'm confused. I've put latest ideas into tickets.

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36101 )

Change subject: util/ifdtool: Add support for setting flash density on IFD V2
......................................................................

util/ifdtool: Add support for setting flash density on IFD V2

Change-Id: Ibc3e4c197f99f99007cb208cf6cc4ae6f56be70c
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36101
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M util/ifdtool/ifdtool.c
1 file changed, 34 insertions(+), 32 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c
index 24e20e1..0e8f76d 100644
--- a/util/ifdtool/ifdtool.c
+++ b/util/ifdtool/ifdtool.c
@@ -921,6 +921,7 @@
                             unsigned int density)
 {
    fcba_t *fcba = find_fcba(image, size);
+	uint8_t mask, chip2_offset;
    if (!fcba)
    	exit(EXIT_FAILURE);

@@ -937,12 +938,13 @@
    		printf("error: Selected density not supported in IFD version 1.\n");
    		exit(EXIT_FAILURE);
    	}
+		mask = 0x7;
+		chip2_offset = 3;
    	break;
    case IFD_VERSION_2:
-		/* I do not have a version 2 IFD nor do i have the docs. */
-		printf("error: Changing the chip density for IFD version 2 has not been"
-		       " implemented yet.\n");
-		exit(EXIT_FAILURE);
+		mask = 0xf;
+		chip2_offset = 4;
+		break;
    default:
    	printf("error: Unknown IFD version\n");
    	exit(EXIT_FAILURE);
@@ -952,13 +954,13 @@
    /* clear chip density for corresponding chip */
    switch (selected_chip) {
    case 1:
-		fcba->flcomp &= ~(0x7);
+		fcba->flcomp &= ~mask;
    	break;
    case 2:
-		fcba->flcomp &= ~(0x7 << 3);
+		fcba->flcomp &= ~(mask << chip2_offset);
    	break;
    default: /*both chips*/
-		fcba->flcomp &= ~(0x3F);
+		fcba->flcomp &= ~(mask | (mask << chip2_offset));
    	break;
    }

@@ -966,7 +968,7 @@
    if (selected_chip == 1 || selected_chip == 0)
    	fcba->flcomp |= (density); /* first chip */
    if (selected_chip == 2 || selected_chip == 0)
-		fcba->flcomp |= (density << 3); /* second chip */
+		fcba->flcomp |= (density << chip2_offset); /* second chip */

write_image(filename, image, size);
 }
@@ -1403,30 +1405,30 @@
 {
    printf("usage: %s [-vhdix?] <filename>\n", name);
    printf("\n"
-	       "   -d | --dump:                       dump intel firmware descriptor\n"
-	       "   -f | --layout <filename>           dump regions into a flashrom layout file\n"
-	       "   -t | --validate                    Validate that the firmware descriptor layout matches the fmap layout\n"
-	       "   -x | --extract:                    extract intel fd modules\n"
-	       "   -i | --inject <region>:<module>    inject file <module> into region <region>\n"
-	       "   -n | --newlayout <filename>        update regions using a flashrom layout file\n"
-	       "   -s | --spifreq <17|20|30|33|48|50> set the SPI frequency\n"
-	       "   -D | --density <512|1|2|4|8|16>    set chip density (512 in KByte, others in MByte)\n"
-	       "   -C | --chip <0|1|2>                select spi chip on which to operate\n"
-	       "                                      can only be used once per run:\n"
-	       "                                      0 - both chips (default), 1 - first chip, 2 - second chip\n"
-	       "   -e | --em100                       set SPI frequency to 20MHz and disable\n"
-	       "                                      Dual Output Fast Read Support\n"
-	       "   -l | --lock                        Lock firmware descriptor and ME region\n"
-	       "   -u | --unlock                      Unlock firmware descriptor and ME region\n"
-	       "   -M | --altmedisable <0|1>          Set the AltMeDisable (or HAP for skylake or newer platform)\n"
-	       "                                      bit to disable ME\n"
-	       "   -p | --platform                    Add platform-specific quirks\n"
-	       "                                      aplk - Apollo Lake\n"
-	       "                                      cnl - Cannon Lake\n"
-	       "                                      glk - Gemini Lake\n"
-	       "                                      sklkbl - Skylake/Kaby Lake\n"
-	       "   -v | --version:                    print the version\n"
-	       "   -h | --help:                       print this help\n\n"
+	       "   -d | --dump:                          dump intel firmware descriptor\n"
+	       "   -f | --layout <filename>              dump regions into a flashrom layout file\n"
+	       "   -t | --validate                       Validate that the firmware descriptor layout matches the fmap layout\n"
+	       "   -x | --extract:                       extract intel fd modules\n"
+	       "   -i | --inject <region>:<module>       inject file <module> into region <region>\n"
+	       "   -n | --newlayout <filename>           update regions using a flashrom layout file\n"
+	       "   -s | --spifreq <17|20|30|33|48|50>    set the SPI frequency\n"
+	       "   -D | --density <512|1|2|4|8|16|32|64> set chip density (512 in KByte, others in MByte)\n"
+	       "   -C | --chip <0|1|2>                   select spi chip on which to operate\n"
+	       "                                         can only be used once per run:\n"
+	       "                                         0 - both chips (default), 1 - first chip, 2 - second chip\n"
+	       "   -e | --em100                          set SPI frequency to 20MHz and disable\n"
+	       "                                         Dual Output Fast Read Support\n"
+	       "   -l | --lock                           Lock firmware descriptor and ME region\n"
+	       "   -u | --unlock                         Unlock firmware descriptor and ME region\n"
+	       "   -M | --altmedisable <0|1>             Set the AltMeDisable (or HAP for skylake or newer platform)\n"
+	       "                                         bit to disable ME\n"
+	       "   -p | --platform                       Add platform-specific quirks\n"
+	       "                                         aplk - Apollo Lake\n"
+	       "                                         cnl - Cannon Lake\n"
+	       "                                         glk - Gemini Lake\n"
+	       "                                         sklkbl - Skylake/Kaby Lake\n"
+	       "   -v | --version:                       print the version\n"
+	       "   -h | --help:                          print this help\n\n"
           "<region> is one of Descriptor, BIOS, ME, GbE, Platform\n"
           "\n");
 }