[flashrom-gerrit] [L] Change in flashrom[main]: cli_client: Add rpmc command support

[Matti Finder (Code Review)]

Attention is currently required from: Anastasia Klimchuk, Peter Marheine.

Matti Finder has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email )

Change subject: cli_client: Add rpmc command support
......................................................................

Patch Set 5:

(8 comments)

This change is ready for review.

Patchset:

PS4:
> Oh one more thing I forgot to mention sorry... […]

1) I think that makes sense I have created a new change at ID 84934 for the first commit and left this one as the documenation and cli_client change.

2) Done.

3) Yes I would like to become a maintainer for the rpmc feature :)

Commit Message:

https://review.coreboot.org/c/flashrom/+/84840/comment/f74ea504_d66478d3?usp... :
PS4, Line 21: usefull
> usefull -> useful (typo) […]

Done

File cli_classic.c:

https://review.coreboot.org/c/flashrom/+/84840/comment/4753fab4_29821d26?usp... :
PS4, Line 147: #if CONFIG_RPMC_ENABLED == 1
             : 	       "RPMC COMMANDS\n"
             : 	       "      --get-rpmc-status             read the extended status\n"
             : 	       "      --write-root-key              write the root key register\n"
             : 	       "      --update-hmac-key             update the hmac key register\n"
             : 	       "      --increment-counter <previous>\n"
             : 	       "                                    increment rpmc counter\n"
             : 	       "      --get-counter                 get rpmc counter\n"
             : 	       "RPMC OPTIONS\n"
             : 	       "      --counter-address <number>    counter address (default: 0)\n"
             : 	       "      --rpmc-root-key <keyfile>     rpmc root key file\n"
             : 	       "      --key-data <keydata>          hex number to use as key data (default: 0)\n"
             : #endif // CONFIG_RPMC_ENABLED
> These new commands need to also be documented on the manpage. […]

Done

https://review.coreboot.org/c/flashrom/+/84840/comment/8fc2ba09_c9679c38?usp... :
PS4, Line 1323: 	if (options.rpmc_read_data)
              : 		ret = rpmc_read_data(fill_flash);
> I think having code in a separate file rpmc.c is better, you don't need to move it. Keep rpmc. […]

Ok then

File include/flash.h:

https://review.coreboot.org/c/flashrom/+/84840/comment/7edbebcc_d5824ca9?usp... :
PS4, Line 553: implement
> I know it's a small thing, but if you start with uppercase it would look better.

This comment doesn't really make sense in the final commit, since it was more of an todo for me. I have removed it.

https://review.coreboot.org/c/flashrom/+/84840/comment/fcf66055_c25ccc4d?usp... :
PS4, Line 575: 		/*
             : 		 * all times in microsecond (us)
             : 		 */
> Also small comment, but this can be 1-line comment, and starting with uppercase […]

Done

https://review.coreboot.org/c/flashrom/+/84840/comment/68c5445c_2d8c4f65?usp... :
PS4, Line 581: rpmc_ctx
> I understand this goes into a flashchip definition? and you mentioned you tested on one chip? It wou […]

This is sadly not as easily possible, since the chip reports itself with the same id as the W25Q128.V. So adding this feature would require changing the polling function to basically just be the same as the one for sfdp-capable chips.

File meson.build:

https://review.coreboot.org/c/flashrom/+/84840/comment/a486222c_9d85dce1?usp... :
PS4, Line 177: if libcrypto.found()
             :     add_project_arguments('-DCONFIG_RPMC_ENABLED=1', language : 'c')
> This ignores the meson option you added. […]

Done