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=email :
PS4, Line 21: usefull
> usefull -> useful (typo) […]
Done


File cli_classic.c:

https://review.coreboot.org/c/flashrom/+/84840/comment/4753fab4_29821d26?usp=email :
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=email :
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=email :
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=email :
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=email :
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=email :
PS4, Line 177: if libcrypto.found()
             :     add_project_arguments('-DCONFIG_RPMC_ENABLED=1', language : 'c')
> This ignores the meson option you added. […]
Done



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/84840?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1
Gerrit-Change-Number: 84840
Gerrit-PatchSet: 5
Gerrit-Owner: Matti Finder <matti.finder@gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm@chromium.org>
Gerrit-Reviewer: Peter Marheine <pmarheine@chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm@chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine@chromium.org>
Gerrit-Comment-Date: Wed, 30 Oct 2024 17:18:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm@chromium.org>
Comment-In-Reply-To: Peter Marheine <pmarheine@chromium.org>
Comment-In-Reply-To: Matti Finder <matti.finder@gmail.com>