Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING
This is the only board using AGESA f14 with a different version string. Change it so that it matches the other AGESA f14 boards in the tree.
Change-Id: I384bd96db51457e68a320b99ecdbb2ada0dfbdd5 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/asrock/e350m1/buildOpts.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/41621/1
diff --git a/src/mainboard/asrock/e350m1/buildOpts.c b/src/mainboard/asrock/e350m1/buildOpts.c index 1e644c5..29a20e7 100644 --- a/src/mainboard/asrock/e350m1/buildOpts.c +++ b/src/mainboard/asrock/e350m1/buildOpts.c @@ -238,7 +238,7 @@
// This is the release version number of the AGESA component // This string MUST be exactly 12 characters long -#define AGESA_VERSION_STRING {'V', '0', '.', '0', '.', '0', '.', '1', ' ', ' ', ' ', ' '} +#define AGESA_VERSION_STRING {'V', '1', '.', '1', '.', '0', '.', '3', ' ', ' ', ' ', ' '}
/* MEMORY_BUS_SPEED */ #define DDR400_FREQUENCY 200 ///< DDR 400
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG@7 PS1, Line 7: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING … to 1.1.0.3
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG@10 PS1, Line 10: Change it so that it matches the other AGESA f14 boards in the tree. Change it from 0.0.0.1 to 1.1.0.3 so that it matches …
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG@7 PS1, Line 7: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING
… to 1.1.0. […]
Aren't commit summaries supposed to be short? This is already over 55 characters. Refer to the other comment as to why I don't want to add any version numbers to the commit message.
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG@10 PS1, Line 10: Change it so that it matches the other AGESA f14 boards in the tree.
Change it from 0.0.0.1 to 1.1.0. […]
What for? One might as well check the changed values in the single line this commit modifies.
This change is the first one of a rather bulky patch train that is supposed to be reproducible, so it is quite annoying to have to rebase everything just because of one commit message.
What's more, if this is to allow grepping for the version numbers, these numbers return only one or two somewhat relevant changes on gerrit, one of which was abandoned as it had been untouched for over seven years. So, I highly doubt they are useful.
Last but not least: to me, putting the numbers being changed in the commit message feels just as bad as writing a comment like this:
/* Assign the value of 3 to the variable i */ i = 3;
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
Patch Set 1: Code-Review+2
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
Patch Set 1:
Indeed, now a version matches with i.e. PC Engines APU1.
Krystian Hebel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
Patch Set 1: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG@10 PS1, Line 10: Change it so that it matches the other AGESA f14 boards in the tree.
What for? One might as well check the changed values in the single line this commit modifies. […]
The commit message is one way during review to verify the diff. A few errors where already found during review, where the values in the commit message and the diff differed, and could be fixed then.
Currently, reviewers have to open other files in the tree to verify the version number you put in.
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
Abandoned
waste of time
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG@10 PS1, Line 10: Change it so that it matches the other AGESA f14 boards in the tree.
The commit message is one way during review to verify the diff. […]
There's a follow-up that factors this out, and it *has to* be correct for it to be reproducible. If people can't be bothered to open up a few files for reviewing, then I'd rather have them NOT review ANY of my patches at all.
I've wasted way too much time and patience over this damned single-line change, so off to the bitbucket it goes.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG@10 PS1, Line 10: Change it so that it matches the other AGESA f14 boards in the tree.
There's a follow-up that factors this out, and it *has to* be correct for it to be reproducible. […]
Well, I voted +2 for it and think it is already okay. Feel free to merge it.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
Patch Set 2:
(2 comments)
Comment in line 7 was from tonight. No idea, why Gerrit didn’t publish it.
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG@7 PS1, Line 7: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING Yes, and they should also be understandable. *Fix* is just too general to get anything out of the short log. Adding the version number adds some perspective.
mb/asrock/e350m1: Set AGESA version to 1.1.0.3
mb/asrock/e350m1: Fix AGESA_VERSION_STRING to 1.1.0.3
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG@10 PS1, Line 10: Change it so that it matches the other AGESA f14 boards in the tree.
Well, I voted +2 for it and think it is already okay. Feel free to merge it.
Angel, I am sorry for the frustration. I suggested the improvement, but still voted +1, so you could have resolved the commit and merged despite the comments. I am fine to agree to disagree.
Angel Pons has restored this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING ......................................................................
Restored
Hello Mike Banon, build bot (Jenkins), Michał Żygowski, Paul Menzel, Michal Zygowski, Mike Banon, Krystian Hebel,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41621
to look at the new patch set (#3).
Change subject: mb/asrock/e350m: Do not redefine AGESA_VERSION_STRING ......................................................................
mb/asrock/e350m: Do not redefine AGESA_VERSION_STRING
This is the only AGESA f14 board which has a different version string. As it is most likely a copy-paste error, drop the redefinition of this macro from buildOpts.c and use the value defined in AGESA f14 headers.
Change-Id: I384bd96db51457e68a320b99ecdbb2ada0dfbdd5 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/asrock/e350m1/buildOpts.c 1 file changed, 0 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/41621/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m: Do not redefine AGESA_VERSION_STRING ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG@7 PS1, Line 7: mb/asrock/e350m1/buildOpts.c: Fix AGESA_VERSION_STRING
Yes, and they should also be understandable. […]
Oh, right. "fix" is too generic of a word. Now I understand why you wanted me to add the version number!
https://review.coreboot.org/c/coreboot/+/41621/1//COMMIT_MSG@10 PS1, Line 10: Change it so that it matches the other AGESA f14 boards in the tree.
Angel, I am sorry for the frustration. […]
I believe I am the one who should apologize here. That was not a proper reply to a reviewer's comment. Even if I had a point there, the vitriol in my words was completely uncalled for. I am sorry that I exploded just because of such a non-critical thing.
Aaaaanyway, after having some rest, I think I found a different approach that does not rely on "fixing" that AGESA_VERSION_NUMBER macro.
Hello Mike Banon, build bot (Jenkins), Michał Żygowski, Paul Menzel, Michal Zygowski, Mike Banon, Krystian Hebel,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41621
to look at the new patch set (#4).
Change subject: mb/asrock/e350m1: Do not redefine AGESA_VERSION_STRING ......................................................................
mb/asrock/e350m1: Do not redefine AGESA_VERSION_STRING
This is the only AGESA f14 board which has a different version string. As it is most likely a copy-paste error, drop the redefinition of this macro from buildOpts.c and use the value defined in AGESA f14 headers.
Change-Id: I384bd96db51457e68a320b99ecdbb2ada0dfbdd5 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/asrock/e350m1/buildOpts.c 1 file changed, 0 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/41621/4
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1: Do not redefine AGESA_VERSION_STRING ......................................................................
Patch Set 4: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1: Do not redefine AGESA_VERSION_STRING ......................................................................
Patch Set 4: Code-Review+1
Thank you Angel.
Angel Pons has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1: Do not redefine AGESA_VERSION_STRING ......................................................................
Removed Verified+1 by build bot (Jenkins) no-reply@coreboot.org
Angel Pons has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1: Do not redefine AGESA_VERSION_STRING ......................................................................
Removed Verified+1 by build bot (Jenkins) no-reply@coreboot.org
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41621 )
Change subject: mb/asrock/e350m1: Do not redefine AGESA_VERSION_STRING ......................................................................
mb/asrock/e350m1: Do not redefine AGESA_VERSION_STRING
This is the only AGESA f14 board which has a different version string. As it is most likely a copy-paste error, drop the redefinition of this macro from buildOpts.c and use the value defined in AGESA f14 headers.
Change-Id: I384bd96db51457e68a320b99ecdbb2ada0dfbdd5 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/41621 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Mike Banon mikebdp2@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net --- M src/mainboard/asrock/e350m1/buildOpts.c 1 file changed, 0 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Mike Banon: Looks good to me, approved
diff --git a/src/mainboard/asrock/e350m1/buildOpts.c b/src/mainboard/asrock/e350m1/buildOpts.c index 7a2cc3b..1c4d62c 100644 --- a/src/mainboard/asrock/e350m1/buildOpts.c +++ b/src/mainboard/asrock/e350m1/buildOpts.c @@ -38,11 +38,5 @@ #include "cpuLateInit.h" #include "GnbInterface.h"
-/* FIXME: This is most likely wrong */ -#undef AGESA_VERSION_STRING - // This is the release version number of the AGESA component - // This string MUST be exactly 12 characters long -#define AGESA_VERSION_STRING {'V', '0', '.', '0', '.', '0', '.', '1', ' ', ' ', ' ', ' '} - /* Instantiate all solution relevant data */ #include <PlatformInstall.h>
-
Angel Pons (Code Review) -
Krystian Hebel (Code Review)
-
Mike Banon (Code Review)
-
Paul Menzel (Code Review)