Change in coreboot[master]: New PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver.
Name of user not set #1002701 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37551 )
Change subject: New PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver. ......................................................................
New PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver.
The 1022:7904 FCH SATA Controller is found in the AMD Bettong board. Support is added to configure properly the SATA device in initialization.
SATA Controller has different device IDs for different drivers. They are renamed accordingly.
Change-Id: Icdede188c82bfe8964c32fe81bdf6a3a6a17096c Signed-off-by: Jorge Fernandez jorgefm@cirsa.com --- M src/include/device/pci_ids.h M src/soc/amd/common/block/sata/sata.c M src/southbridge/amd/pi/hudson/sata.c 3 files changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/37551/1
diff --git a/src/include/device/pci_ids.h b/src/include/device/pci_ids.h index 1276994..853e910 100644 --- a/src/include/device/pci_ids.h +++ b/src/include/device/pci_ids.h @@ -447,8 +447,8 @@
#define PCI_DEVICE_ID_AMD_CZ_HDA 0x157A #define PCI_DEVICE_ID_AMD_CZ_LPC 0x790E -#define PCI_DEVICE_ID_AMD_CZ_SATA 0x7900 -#define PCI_DEVICE_ID_AMD_CZ_SATA_AHCI 0x7901 +#define PCI_DEVICE_ID_AMD_CZ_SATA_IDE 0x7900 +#define PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_MS 0x7901 #define PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_AMD 0x7904 #define PCI_DEVICE_ID_AMD_CZ_USB_0 0x7907 #define PCI_DEVICE_ID_AMD_CZ_USB_1 0x7908 diff --git a/src/soc/amd/common/block/sata/sata.c b/src/soc/amd/common/block/sata/sata.c index 1cd342e..b276809 100644 --- a/src/soc/amd/common/block/sata/sata.c +++ b/src/soc/amd/common/block/sata/sata.c @@ -28,8 +28,8 @@ };
static const unsigned short pci_device_ids[] = { - PCI_DEVICE_ID_AMD_CZ_SATA, - PCI_DEVICE_ID_AMD_CZ_SATA_AHCI, + PCI_DEVICE_ID_AMD_CZ_SATA_IDE, + PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_MS, PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_AMD, 0 }; diff --git a/src/southbridge/amd/pi/hudson/sata.c b/src/southbridge/amd/pi/hudson/sata.c index 77365ef..5846a45 100644 --- a/src/southbridge/amd/pi/hudson/sata.c +++ b/src/southbridge/amd/pi/hudson/sata.c @@ -68,8 +68,8 @@ static const unsigned short pci_device_ids[] = { PCI_DEVICE_ID_AMD_SB900_SATA, PCI_DEVICE_ID_AMD_SB900_SATA_AHCI, - PCI_DEVICE_ID_AMD_CZ_SATA, - PCI_DEVICE_ID_AMD_CZ_SATA_AHCI, + PCI_DEVICE_ID_AMD_CZ_SATA_IDE, + PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_MS, PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_AMD, 0 };
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37551 )
Change subject: New PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver. ......................................................................
Patch Set 1:
(1 comment)
What does MS stand for?
Should this be squashed in the other commit, or put before it?
https://review.coreboot.org/c/coreboot/+/37551/1/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/37551/1/src/include/device/pci_ids.... PS1, Line 451: #define PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_MS 0x7901 Why MS?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37551 )
Change subject: New PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver. ......................................................................
Patch Set 1:
(2 comments)
I would recommend squashing with CB:37546
https://review.coreboot.org/c/coreboot/+/37551/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37551/1//COMMIT_MSG@9 PS1, Line 9: The 1022:7904 FCH This doesn't seem to match what your change is doing.
https://review.coreboot.org/c/coreboot/+/37551/1/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/37551/1/src/include/device/pci_ids.... PS1, Line 451: #define PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_MS 0x7901
Why MS?
"Why MS?" That's how AMD referred to the different IDs in the newer documentation. At this point, I can't recall the exact reasoning. Looking at a Hudson3 RRG for clues, 7801 was called "for ACHI controller" and 7804 was "for AMD AHCI driver". I infer that uses the standard Windows AHCI driver and 7804 requires and AMD driver to be loaded. There may potentially be additional information in AMD's SW stack on devhub.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37551
to look at the new patch set (#2).
Change subject: New PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver. ......................................................................
New PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver.
SATA Controller has different device IDs for different drivers. They are renamed accordingly. This new device ID is found in the AMD Bettong board. Support is added to configure properly the SATA device in initialization.
Change-Id: Icdede188c82bfe8964c32fe81bdf6a3a6a17096c Signed-off-by: Jorge Fernandez jorgefm@cirsa.com --- M src/include/device/pci_ids.h M src/soc/amd/common/block/sata/sata.c M src/southbridge/amd/pi/hudson/sata.c 3 files changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/37551/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37551
to look at the new patch set (#3).
Change subject: amd/hudson: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver. ......................................................................
amd/hudson: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver.
SATA Controller has different device IDs for different drivers. They are renamed accordingly. This new device ID is found in the AMD Bettong board. Support is added to configure properly the SATA device in initialization.
Change-Id: Icdede188c82bfe8964c32fe81bdf6a3a6a17096c Signed-off-by: Jorge Fernandez jorgefm@cirsa.com --- M src/include/device/pci_ids.h M src/soc/amd/common/block/sata/sata.c M src/southbridge/amd/pi/hudson/sata.c 3 files changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/37551/3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37551 )
Change subject: amd/hudson: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver. ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37551/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37551/3//COMMIT_MSG@7 PS3, Line 7: amd/hudson: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver. Please remove the dot/period at the end of the git commit message summary.
https://review.coreboot.org/c/coreboot/+/37551/1/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/37551/1/src/include/device/pci_ids.... PS1, Line 451: #define PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_MS 0x7901
"Why MS?" That's how AMD referred to the different IDs in the newer documentation. […]
So Microsoft? I’d prefer AHCI in the code here, and add a comment, that it’s MS in the documentation.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37551
to look at the new patch set (#4).
Change subject: amd/hudson: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver ......................................................................
amd/hudson: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver
SATA Controller has different device IDs for different drivers. They are renamed accordingly. This new device ID is found in the AMD Bettong board. Support is added to configure properly the SATA device in initialization.
Change-Id: Icdede188c82bfe8964c32fe81bdf6a3a6a17096c Signed-off-by: Jorge Fernandez jorgefm@cirsa.com --- M src/include/device/pci_ids.h M src/soc/amd/common/block/sata/sata.c M src/southbridge/amd/pi/hudson/sata.c 3 files changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/37551/4
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37551 )
Change subject: amd/hudson: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37551/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37551/4//COMMIT_MSG@7 PS4, Line 7: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver Nit: at this point, you're no longer adding, but renaming.
https://review.coreboot.org/c/coreboot/+/37551/4//COMMIT_MSG@10 PS4, Line 10: This new device ID is found in the AMD Bettong board. : Support is added to configure properly the SATA device in initialization. This text looks a little stale too.
https://review.coreboot.org/c/coreboot/+/37551/1/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/37551/1/src/include/device/pci_ids.... PS1, Line 451: #define PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_MS 0x7901
So Microsoft? I’d prefer AHCI in the code here, and add a comment, that it’s MS in the documentation […]
Paul, I infer Microsoft. I think I prefer simply ..._SATA_AHCI too, and leaving AHCI_AMD below as-is.
Jorge Fernandez has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37551 )
Change subject: amd/hudson: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver ......................................................................
Patch Set 4:
Patch Set 4:
(3 comments)
Sorry, how can I generate the changes? I have to push a new commit? I can modify the comment editing it from the webpage but I don't know how to modify the pci_ids.h file...
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37551 )
Change subject: amd/hudson: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
(3 comments)
Sorry, how can I generate the changes? I have to push a new commit? I can modify the comment editing it from the webpage but I don't know how to modify the pci_ids.h file...
The online editor seems only somewhat useful and the file editor is kind of clunky, so it might be better to push updates to your changes, especially if you've been asked to "squash" patches. In case you're not familiar with that process, I looked around a little and found this near the top of Google results https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-hi... Until your changes are approved and merged, you're free to rewrite your on history. Just keep the Change-Id portion of your commit message, as that's how Gerrit tracks which change you're repushing.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37551 )
Change subject: amd/hudson: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
(3 comments)
Sorry, how can I generate the changes? I have to push a new commit? I can modify the comment editing it from the webpage but I don't know how to modify the pci_ids.h file...
The online editor seems only somewhat useful and the file editor is kind of clunky, so it might be better to push updates to your changes, especially if you've been asked to "squash" patches. In case you're not familiar with that process, I looked around a little and found this near the top of Google results https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-hi... Until your changes are approved and merged, you're free to rewrite your on history. Just keep the Change-Id portion of your commit message, as that's how Gerrit tracks which change you're repushing.
If the commit you want to edit is at the top, you can also use `git commit --amend`.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37551
to look at the new patch set (#5).
Change subject: amd/hudson: Rename PCI IDs for FCH SATA Controller ......................................................................
amd/hudson: Rename PCI IDs for FCH SATA Controller
SATA Controller has different device IDs for different drivers. They are renamed accordingly.
Change-Id: Icdede188c82bfe8964c32fe81bdf6a3a6a17096c Signed-off-by: Jorge Fernandez jorgefm@cirsa.com --- M src/include/device/pci_ids.h M src/soc/amd/common/block/sata/sata.c M src/southbridge/amd/pi/hudson/sata.c 3 files changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/37551/5
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37551
to look at the new patch set (#6).
Change subject: amd/hudson: Rename PCI IDs for FCH SATA Controller ......................................................................
amd/hudson: Rename PCI IDs for FCH SATA Controller
SATA Controller has different device IDs for different drivers. They are renamed accordingly.
Change-Id: Icdede188c82bfe8964c32fe81bdf6a3a6a17096c Signed-off-by: Jorge Fernandez jorgefm@cirsa.com --- M src/include/device/pci_ids.h M src/soc/amd/common/block/sata/sata.c M src/southbridge/amd/pi/hudson/sata.c 3 files changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/37551/6
Jorge Fernandez has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37551 )
Change subject: amd/hudson: Rename PCI IDs for FCH SATA Controller ......................................................................
Patch Set 6:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
(3 comments)
Sorry, how can I generate the changes? I have to push a new commit? I can modify the comment editing it from the webpage but I don't know how to modify the pci_ids.h file...
The online editor seems only somewhat useful and the file editor is kind of clunky, so it might be better to push updates to your changes, especially if you've been asked to "squash" patches. In case you're not familiar with that process, I looked around a little and found this near the top of Google results https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-hi... Until your changes are approved and merged, you're free to rewrite your on history. Just keep the Change-Id portion of your commit message, as that's how Gerrit tracks which change you're repushing.
If the commit you want to edit is at the top, you can also use `git commit --amend`.
Is it correct now? I've done a rebase with squash... Why the merge conflict is still active?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37551 )
Change subject: amd/hudson: Rename PCI IDs for FCH SATA Controller ......................................................................
Patch Set 6:
(1 comment)
Is it correct now? I've done a rebase with squash... Why the merge conflict is still active?
The "merge conflict" means this patch cannot be submitted on its own. Git/Gerrit will attempt to be a little conservative in making the determination, opting for human interaction instead. There are two typical reasons why this may be. (a) A patch is based on work not yet merged, and this looks like the case here. This change makes a modification to the same line of code modified in CB:37546. "Merge conflict" often causes unnecessary confusion in this scenario, however it's typical to push patch stacks with the intent of them being submitted in order. (b) Another reason you may encounter the "merge conflict" is when the file itself has been modified in the history. That is, if your work is based on an older version of the repo, then you would need to rebase on something newer and fix up any problems that Git can't solve on its own.
By the way, you'll also need to mark all unresolved inline comments as Done before it can be merged. The easiest way I know to find these is to click in the area just above the filename list, i.e. "Files Base Patchset 6". Click on Base or the patchset number and you'll see where something is still unresolved.
https://review.coreboot.org/c/coreboot/+/37551/6/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/37551/6/src/include/device/pci_ids.... PS6, Line 450: #define PCI_DEVICE_ID_AMD_CZ_SATA_IDE 0x7900 Can this change be squashed with https://review.coreboot.org/c/coreboot/+/37546/1/src/include/device/pci_ids....
Jorge Fernandez has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37551 )
Change subject: amd/hudson: Rename PCI IDs for FCH SATA Controller ......................................................................
Patch Set 6:
(5 comments)
I don't understand. In this commit I'm renaming from PCI_DEVICE_ID_AMD_CZ_SATA to PCI_DEVICE_ID_AMD_CZ_SATA_IDE as the message says. The commit you say to be squashed with defines a new PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_AMD... Isn't it better to have two different commits for different things?
https://review.coreboot.org/c/coreboot/+/37551/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37551/1//COMMIT_MSG@9 PS1, Line 9: The 1022:7904 FCH
This doesn't seem to match what your change is doing.
Done
https://review.coreboot.org/c/coreboot/+/37551/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37551/3//COMMIT_MSG@7 PS3, Line 7: amd/hudson: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver.
Please remove the dot/period at the end of the git commit message summary.
Done
https://review.coreboot.org/c/coreboot/+/37551/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37551/4//COMMIT_MSG@7 PS4, Line 7: Add PCI ID for FCH SATA Controller (AHCI Mode) for AMD driver
Nit: at this point, you're no longer adding, but renaming.
Done
https://review.coreboot.org/c/coreboot/+/37551/4//COMMIT_MSG@10 PS4, Line 10: This new device ID is found in the AMD Bettong board. : Support is added to configure properly the SATA device in initialization.
This text looks a little stale too.
Done
https://review.coreboot.org/c/coreboot/+/37551/1/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/37551/1/src/include/device/pci_ids.... PS1, Line 451: #define PCI_DEVICE_ID_AMD_CZ_SATA_AHCI_MS 0x7901
Paul, I infer Microsoft. I think I prefer simply ... […]
Done
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37551?usp=email )
Change subject: amd/hudson: Rename PCI IDs for FCH SATA Controller ......................................................................
Abandoned
-
Jorge Fernandez (Code Review) -
Marshall Dawson (Code Review)
-
Name of user not set (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)