Change in coreboot[master]: {northbridge,soc,southbridge}: Don't use both of _ADR and _HID
Name of user not set #1002701 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37532 )
Change subject: {northbridge,soc,southbridge}: Don't use both of _ADR and _HID ......................................................................
{northbridge,soc,southbridge}: Don't use both of _ADR and _HID
ACPI Version 6.3 Section 6.1: "A device object must contain either an _HID object or an _ADR object, but should not contain both."
Change-Id: I48fb3e9e48893336416eb90f80957e5bd21d1711 Signed-off-by: Jorge Fernandez jorgefm@cirsa.com --- M src/northbridge/amd/pi/00660F01/acpi/northbridge.asl 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/37532/1
diff --git a/src/northbridge/amd/pi/00660F01/acpi/northbridge.asl b/src/northbridge/amd/pi/00660F01/acpi/northbridge.asl index d54f985..4a48aaf 100644 --- a/src/northbridge/amd/pi/00660F01/acpi/northbridge.asl +++ b/src/northbridge/amd/pi/00660F01/acpi/northbridge.asl @@ -16,7 +16,7 @@ /* Note: Only need HID on Primary Bus */ External (TOM1) External (TOM2) -Name(_HID, EISAID("PNP0A08")) /* PCI Express Root Bridge */ +/* Name(_HID, EISAID("PNP0A08")) // PCI Express Root Bridge */ Name(_CID, EISAID("PNP0A03")) /* PCI Root Bridge */ Name(_ADR, 0x00180000) /* Dev# = BSP Dev#, Func# = 0 */
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37532 )
Change subject: {northbridge,soc,southbridge}: Don't use both of _ADR and _HID ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37532/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37532/1//COMMIT_MSG@7 PS1, Line 7: {northbridge,soc,southbridge}: Don't use both of _ADR and _HID Looks like this change only changes one file on a specific northbridge.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37532 )
Change subject: {northbridge,soc,southbridge}: Don't use both of _ADR and _HID ......................................................................
Patch Set 1:
(1 comment)
If you rebase on the latest code, I think this commit should go away.
https://review.coreboot.org/c/coreboot/+/37532/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37532/1//COMMIT_MSG@7 PS1, Line 7: {northbridge,soc,southbridge}: Don't use both of _ADR and _HID
Looks like this change only changes one file on a specific northbridge.
I believe he used CB:36318 but only kept one file.
Name of user not set #1002701 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37532 )
Change subject: {northbridge,soc,southbridge}: Don't use both of _ADR and _HID ......................................................................
Patch Set 1:
Patch Set 1:
(1 comment)
If you rebase on the latest code, I think this commit should go away.
Yes, I get the comment and code from that commit... Should I redo the comment or leave it because is already in the latest code?
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37532
to look at the new patch set (#2).
Change subject: northbridge: Don't use both of _ADR and _HID ......................................................................
northbridge: Don't use both of _ADR and _HID
ACPI Version 6.3 Section 6.1: "A device object must contain either an _HID object or an _ADR object, but should not contain both."
Change-Id: I48fb3e9e48893336416eb90f80957e5bd21d1711 Signed-off-by: Jorge Fernandez jorgefm@cirsa.com --- M src/northbridge/amd/pi/00660F01/acpi/northbridge.asl 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/37532/2
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37532 )
Change subject: northbridge: Don't use both of _ADR and _HID ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37532/2/src/northbridge/amd/pi/0066... File src/northbridge/amd/pi/00660F01/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/37532/2/src/northbridge/amd/pi/0066... PS2, Line 19: /* Name(_HID, EISAID("PNP0A08")) // PCI Express Root Bridge */ Why not just removing this line?
Name of user not set #1002701 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37532 )
Change subject: northbridge: Don't use both of _ADR and _HID ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
(1 comment)
For my it's OK, I was only using an already present patch.
https://review.coreboot.org/c/coreboot/+/37532/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37532/1//COMMIT_MSG@7 PS1, Line 7: {northbridge,soc,southbridge}: Don't use both of _ADR and _HID
I believe he used CB:36318 but only kept one file.
Yes, the code and comment is from that commit. Should I redo the comment?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37532 )
Change subject: northbridge: Don't use both of _ADR and _HID ......................................................................
Patch Set 2:
Patch Set 1:
Patch Set 1:
(1 comment)
If you rebase on the latest code, I think this commit should go away.
Yes, I get the comment and code from that commit... Should I redo the comment or leave it because is already in the latest code?
What I'm saying is that it looked like your starting point is out of date, and this particular change has already gone in. If you run "git fetch && git rebase origin/master", then you'll no longer need this commit at all. OTOH if I'm misreading it, then you should change the commit message to match what this commit actually does.
Jorge Fernandez has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37532 )
Change subject: northbridge: Don't use both of _ADR and _HID ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 1:
Patch Set 1:
(1 comment)
If you rebase on the latest code, I think this commit should go away.
Yes, I get the comment and code from that commit... Should I redo the comment or leave it because is already in the latest code?
What I'm saying is that it looked like your starting point is out of date, and this particular change has already gone in. If you run "git fetch && git rebase origin/master", then you'll no longer need this commit at all. OTOH if I'm misreading it, then you should change the commit message to match what this commit actually does.
Sorry, I'm in an accelerated git and gerrit course and a lot of things are new for me. I was pointed to start my bettong port in an old commit from Apr (f77f2c79c2bb898c123ffe89a0bd1acb5362afc5) because now the board is disabled. The commit I'm redoing (01787608670adec26fcea48173e18395e51c790e) with this patch is from Oct. I don't know how to import this new commit in my old branch... I guess is a rebase. I'm still learning to be able to contribute.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37532 )
Change subject: northbridge: Don't use both of _ADR and _HID ......................................................................
Patch Set 2:
I was pointed to start my bettong port in an old commit from Apr (f77f2c79c2bb898c123ffe89a0bd1acb5362afc5) because now the board is disabled.
Oh, I see now. Sorry for my confusion. What I would do, in a a case like that, is to change the commit message here to reflect that it won't be merged onto master. That way anyone reviewing can see what you're doing and understand the reasons. I would probably write the commit message something like:
[NOT FOR MERGE] northbridge: Don't use both _ADR and _HID
This patch duplicates removal of _HID as in CB:37532 and allows older source to build and boot on Bettong.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37532
to look at the new patch set (#3).
Change subject: [NOT FOR MERGE] northbridge: Don't use both _ADR and _HID ......................................................................
[NOT FOR MERGE] northbridge: Don't use both _ADR and _HID
This patch duplicates removal of _HID as in CB:37532 and allows older source to build and boot on Bettong.
Change-Id: I48fb3e9e48893336416eb90f80957e5bd21d1711 Signed-off-by: Jorge Fernandez jorgefm@cirsa.com --- M src/northbridge/amd/pi/00660F01/acpi/northbridge.asl 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/37532/3
Jorge Fernandez has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37532 )
Change subject: [NOT FOR MERGE] northbridge: Don't use both _ADR and _HID ......................................................................
Patch Set 3:
Patch Set 2:
I was pointed to start my bettong port in an old commit from Apr (f77f2c79c2bb898c123ffe89a0bd1acb5362afc5) because now the board is disabled.
Oh, I see now. Sorry for my confusion. What I would do, in a a case like that, is to change the commit message here to reflect that it won't be merged onto master. That way anyone reviewing can see what you're doing and understand the reasons. I would probably write the commit message something like:
[NOT FOR MERGE] northbridge: Don't use both _ADR and _HID
This patch duplicates removal of _HID as in CB:37532 and allows older source to build and boot on Bettong.
Good thought. But I hesitate because I'm doing all this commits to be merged because to push a board_status report it seems all commits should be merged, if I got it right. Or maybe if the commits are in Gerrit they can be used to build the same coreboot.rom as myself and the board_status report I report is valid if they come from known commit in Gerrit...
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37532 )
Change subject: [NOT FOR MERGE] northbridge: Don't use both _ADR and _HID ......................................................................
Patch Set 3:
Good thought. But I hesitate because I'm doing all this commits to be merged because to push a board_status report it seems all commits should be merged, if I got it right. Or maybe if the commits are in Gerrit they can be used to build the same coreboot.rom as myself and the board_status report I report is valid if they come from known commit in Gerrit...
Yes, for the board status to be the most meaningful, it should be in code that's merged. IIRC it's not prevented pushing unmerged, however someone trying to reproduce it won't appreciate what you did. There's no good way to communicate a revision to a patchset from Gerrit. Also in Gerrit the commit ancestry can be confusing if patches get modified out of order -- perhaps you've noticed that already.
...commits to be merged...
By the way, if it helps, don't think of your changes as being technically merged at completion. Consider them cherry-picked instead, and in this one's case for the master branch. Any change always becomes the very latest in the git log once we +2 and click submit, then anyone who uses git fetch will have a copy of your patch. The change in source for this commit would technically get "merged" out of existence because it's already in the source. And if it's cherry-picked, it should cause a message of "The previous cherry-pick is now empty..." (Hmm, it seems like Gerrit would want to flag that like it does a merge conflict...)
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37532?usp=email )
Change subject: [NOT FOR MERGE] northbridge: Don't use both _ADR and _HID ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Frans Hendriks (Code Review)
-
Jorge Fernandez (Code Review)
-
Marshall Dawson (Code Review)
-
Martin L Roth (Code Review)
-
Name of user not set (Code Review)