Piotr Kleinschmidt has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: src/southbridge/amd/pi/hudson/sata.c: set SATA in AHCI mode ......................................................................
src/southbridge/amd/pi/hudson/sata.c: set SATA in AHCI mode
The attempt to install pfSense ended up in a SATA driver error. Changing SATA mode from IDE to AHCI solved that issue.
Change-Id: I1b0322392712d797dd5a8931150c8d0ff1b60940 Signed-off-by: Piotr Kleinschmidt piotr.kleins@gmail.com --- M src/southbridge/amd/pi/hudson/sata.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/35891/1
diff --git a/src/southbridge/amd/pi/hudson/sata.c b/src/southbridge/amd/pi/hudson/sata.c index 08e967d..8e58390 100644 --- a/src/southbridge/amd/pi/hudson/sata.c +++ b/src/southbridge/amd/pi/hudson/sata.c @@ -32,6 +32,9 @@ #define UNLOCK_BIT (1<<0) #define SATA_CAPABILITIES_REG 0xFC #define CFG_CAP_SPM (1<<12) + #define SATA_REV_SUBCLASS_REG 0x08 + #define SUBCLASS_AHCI_MODE 0x60000 + #define SATA_PROGRAMIF_AHCI 0x100
volatile u32 *ahci_ptr = (u32*)(pci_read_config32(dev, AHCI_BASE_ADDRESS_REG) & 0xFFFFFF00); @@ -45,6 +48,11 @@ /* set the SATA AHCI mode to allow port expanders */ *(ahci_ptr + BYTE_TO_DWORD_OFFSET(SATA_CAPABILITIES_REG)) |= CFG_CAP_SPM;
+ /* enable AHCI mode */ + temp = pci_read_config32(dev, SATA_REV_SUBCLASS_REG); + temp = (temp & 0xFF0070FF) | SUBCLASS_AHCI_MODE | SATA_PROGRAMIF_AHCI; + pci_write_config32(dev, SATA_REV_SUBCLASS_REG, temp); + /* lock the write-protect */ temp = pci_read_config32(dev, MISC_CONTROL_REG); temp &= ~UNLOCK_BIT;
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: src/southbridge/amd/pi/hudson/sata.c: set SATA in AHCI mode ......................................................................
Patch Set 1:
(3 comments)
I am surprised such a problem should surface that late.
https://review.coreboot.org/c/coreboot/+/35891/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35891/1//COMMIT_MSG@7 PS1, Line 7: src/southbridge/amd/pi/hudson/sata.c: set SATA in AHCI mode The prefix does not need to be the path.
sb/amd/pi/hudson: Default SATA to AHCI
https://review.coreboot.org/c/coreboot/+/35891/1//COMMIT_MSG@10 PS1, Line 10: Changing SATA mode from IDE to AHCI solved that issue. Shouldn’t pfSense be able to deal with IDE mode?
https://review.coreboot.org/c/coreboot/+/35891/1/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/sata.c:
https://review.coreboot.org/c/coreboot/+/35891/1/src/southbridge/amd/pi/huds... PS1, Line 51: /* enable AHCI mode */ Can you look at other boards, on how to make this build or run time configurable?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: src/southbridge/amd/pi/hudson/sata.c: set SATA in AHCI mode ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35891/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35891/1//COMMIT_MSG@10 PS1, Line 10: Changing SATA mode from IDE to AHCI solved that issue.
Shouldn’t pfSense be able to deal with IDE mode?
Of course it deals with IDE mode, however, the pfSense software seems not to be robust. See: https://github.com/pcengines/apu2-documentation/blob/master/docs/debug/pfsen...
https://review.coreboot.org/c/coreboot/+/35891/1/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/sata.c:
https://review.coreboot.org/c/coreboot/+/35891/1/src/southbridge/amd/pi/huds... PS1, Line 51: /* enable AHCI mode */
Can you look at other boards, on how to make this build or run time configurable?
Firstly, we do not use CMOS runtime configuration for this board.
Secondly, there is CONFIG_HUDSON_SATA_MODE which is supposed to set the desired mode in AGESA blob, however, our experiments showed it doesn't work.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: src/southbridge/amd/pi/hudson/sata.c: set SATA in AHCI mode ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35891/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35891/1//COMMIT_MSG@10 PS1, Line 10: Changing SATA mode from IDE to AHCI solved that issue.
Of course it deals with IDE mode, however, the pfSense software seems not to be robust. […]
So the Linux kernel does not have a problem, and it is a …BSD issue?
Please improve the commit message accordingly. (Also with the information from your other comments.)
https://review.coreboot.org/c/coreboot/+/35891/1/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/sata.c:
https://review.coreboot.org/c/coreboot/+/35891/1/src/southbridge/amd/pi/huds... PS1, Line 51: /* enable AHCI mode */
Firstly, we do not use CMOS runtime configuration for this board. […]
ad 1) The commit deals with the south bridge, and not a specific board, doesn’t it? (The commit message also fails to mention that.)
ad 2) This should be fixed then.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: src/southbridge/amd/pi/hudson/sata.c: set SATA in AHCI mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35891/1/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/sata.c:
https://review.coreboot.org/c/coreboot/+/35891/1/src/southbridge/amd/pi/huds... PS1, Line 51: /* enable AHCI mode */
ad 1) The commit deals with the south bridge, and not a specific board, doesn’t it? (The commit mess […]
ad 1) Ack. Piotr please be more verbose in the commit message, that the problem is present on apu2 and relates only to pfSense/BSD. ad 2) There is very little we can do here. Fixing closed source blob is far from possible. Assuming AGESA even doing something with SATA controller, coreboot does it in its own way in this file. So the easiest and safest option is to apply fixes here.
I understand that the commit message should be more descriptive and it caused the small confusion here.
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35891
to look at the new patch set (#2).
Change subject: sb/amd/pi/hudson: Default SATA to AHCI ......................................................................
sb/amd/pi/hudson: Default SATA to AHCI
The attempt to install pfSense on hard disk on PC Engines apu2 board ended up in a SATA driver error. The problem is related only to BSD and didn't occur with Linux kernel. More information is available here: https://github.com/pcengines/apu2-documentation/blob/master/docs/debug/pfsen...
Changing SATA mode from IDE to AHCI solved the problem.
Change-Id: I1b0322392712d797dd5a8931150c8d0ff1b60940 Signed-off-by: Piotr Kleinschmidt piotr.kleins@gmail.com --- M src/southbridge/amd/pi/hudson/sata.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/35891/2
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: sb/amd/pi/hudson: Default SATA to AHCI ......................................................................
Patch Set 2:
Set that patch as WIP due to performing additional tests. Setting AHCI mode is possible also via CONFIG_HUDSON_SATA_MODE. Need to check if it works and doesn't trigger described pfSense bug.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: mb/pcengines/apu2: Change default SATA mode to AHCI ......................................................................
Patch Set 3:
This change is ready for review.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: mb/pcengines/apu2: Change default SATA mode to AHCI ......................................................................
Patch Set 3:
As AHCI is nowadays more common than IDE, why not make this the chipset default?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: mb/pcengines/apu2: Change default SATA mode to AHCI ......................................................................
Patch Set 3:
Patch Set 3:
As AHCI is nowadays more common than IDE, why not make this the chipset default?
Right, why not. It also speeds up the interface a little bit. But touching something that was unchanged for years leaves an uneasy feel of breaking something. I can give a try and change it globally for chipset, also hoping for more opinions on that change.
Michał Żygowski has uploaded a new patch set (#4) to the change originally created by Piotr Kleinschmidt. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: sb/amd/{agesa,pi}/hudson/Kconfig: Change default SATA mode to AHCI ......................................................................
sb/amd/{agesa,pi}/hudson/Kconfig: Change default SATA mode to AHCI
The attempt to install pfSense on hard disk on PC Engines apu2 board ended up in a SATA driver error. The problem is related only to BSD and didn't occur with Linux kernel. Changing SATA mode from IDE to AHCI solved the problem.
Additionally AHCI is faster than IDE so it speeds up the installation. Since AHCI works perfectly with SeaBIOS, Linux and BSD, make it a default choice for all Hudson southbridges.
Change-Id: I1b0322392712d797dd5a8931150c8d0ff1b60940 Signed-off-by: Piotr Kleinschmidt piotr.kleins@gmail.com Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com --- M src/southbridge/amd/agesa/hudson/Kconfig M src/southbridge/amd/pi/hudson/Kconfig 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/35891/4
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: sb/amd/{agesa,pi}/hudson/Kconfig: Change default SATA mode to AHCI ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35891/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35891/1//COMMIT_MSG@7 PS1, Line 7: src/southbridge/amd/pi/hudson/sata.c: set SATA in AHCI mode
The prefix does not need to be the path. […]
Done
https://review.coreboot.org/c/coreboot/+/35891/1//COMMIT_MSG@10 PS1, Line 10: Changing SATA mode from IDE to AHCI solved that issue.
So the Linux kernel does not have a problem, and it is a …BSD issue? […]
Done
https://review.coreboot.org/c/coreboot/+/35891/1/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/sata.c:
https://review.coreboot.org/c/coreboot/+/35891/1/src/southbridge/amd/pi/huds... PS1, Line 51: /* enable AHCI mode */
ad 1) Ack. […]
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: sb/amd/{agesa,pi}/hudson/Kconfig: Change default SATA mode to AHCI ......................................................................
Patch Set 4: Code-Review+2
SeaBIOS (and Grub) use PIO instead of DMA with IDE mode, so AHCI is indeed a better default IMO. The speed difference is easily a order of magnitude faster with AHCI (on ICH7 at least).
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: sb/amd/{agesa,pi}/hudson/Kconfig: Change default SATA mode to AHCI ......................................................................
Patch Set 4:
Patch Set 4: Code-Review+2
SeaBIOS (and Grub) use PIO instead of DMA with IDE mode, so AHCI is indeed a better default IMO. The speed difference is easily a order of magnitude faster with AHCI (on ICH7 at least).
IIRC SeaBIOS needs explicit: CONFIG_ATA_DMA=y CONFIG_ATA_PIO32=y passed as a SeaBIOS config file in order to use PIO. Maybe should I add a patch for default PIO in SeaBIOS Makefile?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: sb/amd/{agesa,pi}/hudson/Kconfig: Change default SATA mode to AHCI ......................................................................
Patch Set 4: Code-Review+1
SeaBIOS (and Grub) use PIO instead of DMA with IDE mode, so AHCI is indeed a better default IMO. The speed difference is easily a order of magnitude faster with AHCI (on ICH7 at least).
IIRC SeaBIOS needs explicit: CONFIG_ATA_DMA=y CONFIG_ATA_PIO32=y passed as a SeaBIOS config file in order to use PIO. Maybe should I add a patch for default PIO in SeaBIOS Makefile?
AFAIK, it's not the default because it breaks compatibility with actual PATA drives. So it would have to be decided per board.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: sb/amd/{agesa,pi}/hudson/Kconfig: Change default SATA mode to AHCI ......................................................................
Patch Set 4: Code-Review+1
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: sb/amd/{agesa,pi}/hudson/Kconfig: Change default SATA mode to AHCI ......................................................................
Patch Set 4: Code-Review+1
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35891 )
Change subject: sb/amd/{agesa,pi}/hudson/Kconfig: Change default SATA mode to AHCI ......................................................................
sb/amd/{agesa,pi}/hudson/Kconfig: Change default SATA mode to AHCI
The attempt to install pfSense on hard disk on PC Engines apu2 board ended up in a SATA driver error. The problem is related only to BSD and didn't occur with Linux kernel. Changing SATA mode from IDE to AHCI solved the problem.
Additionally AHCI is faster than IDE so it speeds up the installation. Since AHCI works perfectly with SeaBIOS, Linux and BSD, make it a default choice for all Hudson southbridges.
Change-Id: I1b0322392712d797dd5a8931150c8d0ff1b60940 Signed-off-by: Piotr Kleinschmidt piotr.kleins@gmail.com Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35891 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/southbridge/amd/agesa/hudson/Kconfig M src/southbridge/amd/pi/hudson/Kconfig 2 files changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, but someone else must approve Nico Huber: Looks good to me, but someone else must approve Paul Menzel: Looks good to me, but someone else must approve Arthur Heymans: Looks good to me, approved
diff --git a/src/southbridge/amd/agesa/hudson/Kconfig b/src/southbridge/amd/agesa/hudson/Kconfig index e56a493..96857b0 100644 --- a/src/southbridge/amd/agesa/hudson/Kconfig +++ b/src/southbridge/amd/agesa/hudson/Kconfig @@ -90,7 +90,7 @@
config HUDSON_SATA_MODE int "SATA Mode" - default 0 + default 2 range 0 6 help Select the mode in which SATA should be driven. NATIVE AHCI, or RAID. diff --git a/src/southbridge/amd/pi/hudson/Kconfig b/src/southbridge/amd/pi/hudson/Kconfig index ea37e3e..4884b73 100644 --- a/src/southbridge/amd/pi/hudson/Kconfig +++ b/src/southbridge/amd/pi/hudson/Kconfig @@ -102,7 +102,7 @@
config HUDSON_SATA_MODE int "SATA Mode" - default 0 + default 2 range 0 6 help Select the mode in which SATA should be driven. NATIVE AHCI, or RAID.
-
Arthur Heymans (Code Review) -
Kyösti Mälkki (Code Review)
-
Michał Żygowski (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Piotr Kleinschmidt (Code Review)