Change in ...coreboot[master]: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only wit...
Hello Mike Banon,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/30749
to review the following change.
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware
Currently, by default AGESA's XHCI controller is enabled without XHCI firmware being added if USE_BLOBS isn't selected. With this faulty set of default values at least AMD Lenovo G505S laptop has the following problem: both USB 3.0 ports are not working at all, even at 2.0 mode. To avoid it we should disable XHCI controller by default and allow enabling it only with a firmware provided.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I9f2b88d65f4f300ba1a28db09fb41d6bac6252b6 --- M src/southbridge/amd/agesa/hudson/Kconfig 1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/30749/1
diff --git a/src/southbridge/amd/agesa/hudson/Kconfig b/src/southbridge/amd/agesa/hudson/Kconfig index b80f734..0c6ac3e 100644 --- a/src/southbridge/amd/agesa/hudson/Kconfig +++ b/src/southbridge/amd/agesa/hudson/Kconfig @@ -41,7 +41,8 @@
config HUDSON_XHCI_ENABLE bool "Enable Hudson XHCI Controller" - default y + default n + select HUDSON_XHCI_FWM help The XHCI controller must be enabled and the XHCI firmware must be added in order to have USB 3.0 support configured @@ -50,7 +51,7 @@ XHCI controller is not enabled by coreboot.
config HUDSON_XHCI_FWM - bool "Add xhci firmware" + bool "Add XHCI firmware" default y if USE_BLOBS help Add Hudson 2/3/4 XHCI Firmware to support the onboard USB 3.0
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/30749/1/src/southbridge/amd/agesa/hudson/Kco... File src/southbridge/amd/agesa/hudson/Kconfig:
https://review.coreboot.org/#/c/30749/1/src/southbridge/amd/agesa/hudson/Kco... PS1, Line 45: select HUDSON_XHCI_FWM
Out of 4 there are 3 valid combinations of HUDSON_XHCI_ENABLE + HUDSON_XHCI_FWM : […]
Again, y + n may have unfortunate results, but is *not* invalid. Somebody could still want to add the blob later to their `coreboot.rom` (and have coreboot enable xHCI).
If you'd do what I suggested, you would have sane defaults, either n + n or y + y, depending on USE_BLOBS. That's the best we can do, I guess.
Hello Kyösti Mälkki, Felix Held, Mike Banon, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30749
to look at the new patch set (#3).
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware
Currently, by default AGESA's XHCI controller is enabled without XHCI firmware being added if USE_BLOBS isn't selected. With this faulty set of default values at least AMD Lenovo G505S laptop has the following problem: both USB 3.0 ports are not working at all, even at 2.0 mode. To avoid it we should disable XHCI controller by default and allow enabling it only with a firmware provided.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I9f2b88d65f4f300ba1a28db09fb41d6bac6252b6 --- M src/southbridge/amd/agesa/hudson/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/30749/3
Hello Kyösti Mälkki, Felix Held, Mike Banon, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30749
to look at the new patch set (#4).
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware
Currently, by default AGESA's XHCI controller is enabled without XHCI firmware being added if USE_BLOBS isn't selected. With this faulty set of default values at least AMD Lenovo G505S laptop has the following problem: both USB 3.0 ports are not working at all, even at 2.0 mode. To avoid it we should disable XHCI controller by default and allow enabling it only with a firmware provided.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I9f2b88d65f4f300ba1a28db09fb41d6bac6252b6 --- M src/southbridge/amd/agesa/hudson/Kconfig 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/30749/4
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
Sorry for delay and thank you very much for your suggestions. After some experimenting hopefully I got the perfect Kconfig: now it will allow adding the xHCI firmware only if xHCI controller has been enabled
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
I'm wondering if you already know if USB3 works at all ( with! ) xhci firmware, on g505s.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
Patch Set 4:
I'm wondering if you already know if USB3 works at all ( with! ) xhci firmware, on g505s.
Recently my friend Ivan told me that USB3 works at G505S, but only if you 1) change at its' Kconfig from select SOUTHBRIDGE_AMD_AGESA_HUDSON to select SOUTHBRIDGE_AMD_AGESA_BOLTON - because G505S FCH is Bolton-M3, not Hudson 2) and also add the correct xHCI firmware (e.g. available at https://github.com/informer2016/shared_devfiles/blob/master/0.10.0_1022_7814... ) which has been extracted using Felix Held's fch_xhci_rom_dumper - https://github.com/felixheld/fch_xhci_rom_dumper - from G505S while it has been running the proprietary UEFI.
I am really sorry I missed your CR:30899 patch (AGESA/binaryPI: Drop invalid AMD_AGESA_BOLTON) which removed SOUTHBRIDGE_AMD_AGESA_BOLTON and been merged recently :( So I'm not sure if USB3 xHCI will work at the latest coreboot revision because it's not just a different blob but there seems to be a pinout difference between Hudson and Bolton which may affect the results.
I haven't tested by myself but will test at pre-CR:30899 version as soon as possible - and, if USB3 indeed works at G505S if these conditions are satisfied - maybe I would submit a Bolton restoration patch which would be either the same or maybe would be slightly different compared to what CR:30899 has reverted (I will highlight the differences if there would be any). Also, at the same time I could test your CR:21624 patch if it is ready for testing at G505S hardware
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
Recently my friend Ivan told me that USB3 works at G505S, but only if you
- change at its' Kconfig from select SOUTHBRIDGE_AMD_AGESA_HUDSON to select SOUTHBRIDGE_AMD_AGESA_BOLTON - because G505S FCH is Bolton-M3, not Hudson
The different XHCI controller is the main difference between Hudson and Bolton.
- and also add the correct xHCI firmware (e.g. available at https://github.com/informer2016/shared_devfiles/blob/master/0.10.0_1022_7814... ) which has been extracted using Felix Held's fch_xhci_rom_dumper - https://github.com/felixheld/fch_xhci_rom_dumper - from G505S while it has been running the proprietary UEFI.
Huh, fch_xhci_rom_dumper is completely unrelated and not needed here, since it dumps the rom part of the firmware and not the runtime-loaded part. The right XHCI blob for bolton is in the blobs repo.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
I diffed KaveriPI xHCI source (that actually refers to bolton) against (scrubbed) TrinityPI xHCI source we have under vendorcode. Just by judging the number of added register modifications on bolton side, I do not expect bolton hardware to work reliably with vendorcode/fam15tn/.
In my opinion even the xHCI firmware loader in TrinityPI, for Hudson, seems fishy
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
Patch Set 4:
I diffed KaveriPI xHCI source (that actually refers to bolton) against (scrubbed) TrinityPI xHCI source we have under vendorcode. Just by judging the number of added register modifications on bolton side, I do not expect bolton hardware to work reliably with vendorcode/fam15tn/.
In my opinion even the xHCI firmware loader in TrinityPI, for Hudson, seems fishy
Ah, ok
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
- and also add the correct xHCI firmware (e.g. available at https://github.com/informer2016/shared_devfiles/blob/master/0.10.0_1022_7814... ) which has been extracted using Felix Held's fch_xhci_rom_dumper - https://github.com/felixheld/fch_xhci_rom_dumper - from G505S while it has been running the proprietary UEFI.
Huh, fch_xhci_rom_dumper is completely unrelated and not needed here, since it dumps the rom part of the firmware and not the runtime-loaded part. The right XHCI blob for bolton is in the blobs repo.
Sorry I misunderstood him, usb 3.0 worked for Ivan at g505s with this bolton/xhci.bin from the blobs repo, and this another blob (obtained with your fch_xhci_rom_dumper) wasn't tested by him at all! So I just tested it and (maybe as expected because you said it is wrong blob) my laptop didn't boot at all - black screen :P Then I tried building coreboot with bolton/xhci.bin and now it boots, however I see the following messages at Linux kernel log:
xhci_hcd 0000:00:10.0: xHCI Host Controller xhci_hcd 0000:00:10.0: new USB bus registered, assigned bus number 8 xhci_hcd 0000:00:10.0: can't setup: -110 xhci_hcd 0000:00:10.0: USB bus 8 deregistered xhci_hcd 0000:00:10.0: init 0000:00:10.0 fail, -110 xhci_hcd: probe of 0000:00:10.0 failed with error -110
So those "blue" USB ports are working but maybe at 2.0 mode only, don't know why I can't reproduce the successful results. Tested with Linux kernel 4.15.0 if it matters
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
Felix, you said that "fch_xhci_rom_dumper" is an incorrect way of obtaining this xhci binary. Please could you tell what I should do to correctly obtain such xhci blob by myself in case I would want it? Just extract as-is from vendor's UEFI? I am curious because when I need to use blobs I prefer to use those which I could obtain by myself. Also, this bolton/xhci.bin Bolton blob is for "AMD Embedded Bald Eagle" - it seems to be another bolton, and G505S bolton-m3 may be not 100% compatible with this one
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
Patch Set 4:
Felix, you said that "fch_xhci_rom_dumper" is an incorrect way of obtaining this xhci binary. Please could you tell what I should do to correctly obtain such xhci blob by myself in case I would want it? Just extract as-is from vendor's UEFI? I am curious because when I need to use blobs I prefer to use those which I could obtain by myself. Also, this bolton/xhci.bin Bolton blob is for "AMD Embedded Bald Eagle" - it seems to be another bolton, and G505S bolton-m3 may be not 100% compatible with this one
In my g505s extracted rom, I can find blobs matching the ones we have in 3rdparty/:
At 0x0010000 is blobs/hudson/imc.bin At 0x0021000 is blobs/hudson/xhci.rom (probably left in by mistake, not pointed to) At 0x0321000 is blobs/bolton/xhci.rom
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
Find below comments for Bolton support in leaked KaveriPI tree. At least the following register modifications are missing in vendocode/fam15tn/hudson (which is pre-Bolton but still has two XHCI controllers).
Per vendorcode, 0.14:0 [08] revision >= 0x15 is identified as Bolton where following is done. My lenovo/g505s reports 0x16 here. Now, most of those below do say 'enhance' instead of 'fix' so using hudson vendorcode for bolton hardware may be quite functional.
Be aware, if you come across those leaked KaveriPI or CarrizoPI trees, some of the firmware binaries there suffered CRLF conversions and are corrupted. AFAICS, bolton blob in 3rdparty/blobs matches the one my lenovo/g505s was originally shipped with.
// RPR 8.27 Enhance D3Cold ccu sequencing // RPR 8.28 Set HCIVERSION to 1.0 // RPR 8.29 xHCI 1.0 Sub-Features Supported // RRG 8.30 XHC USB2 Loopback RX SE0 // RRG 8.31 XHC U2IF_Enabled_Quiettermination off // RRG 8.32 XHC S0 BLM Reset Mode // RRG 8.33 Enhance XHC Ent_Flag // RRG 8.34 Enhance TRB Pointer when both MSE and SKIP TRB IOC evt opened // RRG 8.35 LPM Broadcast disable // RRG 8.37 Enhance XHC FS/LS connect // RRG 8.38 Enhance XHC ISOCH td_cmp // RRG 8.39 LPM Clock 5us Select // RRG 8.40 Enhance DPP ERR as XactErr // RRG 8.41 Enhance U2IF PME Enable // RRG 8.42 U2IF S3 Disconnect detection // RRG 8.43 Stream Error Handling // RRG 8.44 FLA Deassert // RRG 8.45 LPM Ctrl Improve // 393674 Enable fix for control transfer error
//8.60 Enhance LPM Host initial L1 Exit
// RRG 8.46 Enhance resume after disconnect // RRG 8.47 Enhance SS HD Detected on Plug-in during S3 // RRG 8.48 Frame Babble Reporting // xHCI Debug Capability // RRG 8.49 DCP Halt RSTSM OFF // RRG 8.50 Enable DCP DPH check // RRG 8.51 DCP LTSSM Inactive to Rxdetect // RRG 8.52 Enhance DCP EP State // RRG 8.53 DCP Remote Wakeup Capable // RRG 8.58 Enable ERDY send once DBC detectes HIT/HOT // RRG 8.59 Block HIT/HOT until service interval done // RRG 8.54 Enhance SS HS detected during S3 // RRG 8.55 Enhance U1 timer (shorten U1 exit response time) // RRG 8.56 Enhance HW LPM U2Entry state // RRG 8.57 Enhance SSIF PME // RPR 8.62 Enable FW enhancement on XHC clock control when memory power saving is disabled // RPR 8.63 U2IF Remote Wake Select // RPR 8.64 HS Data Toggle Error Handling // A1 // UBTS 444589 PIL failures when reset device command hit link td points to invaild trb // UBTS 437021 PME not generated by U2IF logic during S3 shutdown // UBTS 432864 When a USB3 device is disconnected, and connected again, xHC cannot detect it. // UBTS 430710 USB3 port enters compliance state when debug port enabled (0: enable) // UBTS 430715 [DbC] Compatibility issue with 3rd party (I) chip as Host and AMD as Target // UBTS 430708 The second configure endpoint command haven't been executed completely when the port was in L1 // UBTS 428048 [LPM] LPM 2ports interrupt in is ERROR after host initiate L1exit respond a nak in HS // UBTS 428045 LPM packet with incorrect address // UBTS 443139 Buffer overrun during S4 loop // UBTS 419582 LPM arbitration between USB ports // UBTS 429765 USB3.0 ACPI Indirect registers get changed after reboot // UBTS 439658 Black Magic USB 3.0 device when enabled/disabled under windows causes the driver to register disconnect // UBTS SS RX terminations blocked unexpectedly
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
Patch Set 4:
In my g505s extracted rom, I can find blobs matching the ones we have in 3rdparty/:
At 0x0010000 is blobs/hudson/imc.bin At 0x0021000 is blobs/hudson/xhci.rom (probably left in by mistake, not pointed to) At 0x0321000 is blobs/bolton/xhci.rom
Thank you very much for sharing this info, I tried extracting them and indeed they match 1:1
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Patch Set 4:
Patch Set 4:
Find below comments for Bolton support in leaked KaveriPI tree. At least the following register modifications are missing in vendocode/fam15tn/hudson (which is pre-Bolton but still has two XHCI controllers).
Per vendorcode, 0.14:0 [08] revision >= 0x15 is identified as Bolton where following is done. My lenovo/g505s reports 0x16 here. Now, most of those below do say 'enhance' instead of 'fix' so using hudson vendorcode for bolton hardware may be quite functional.
Be aware, if you come across those leaked KaveriPI or CarrizoPI trees, some of the firmware binaries there suffered CRLF conversions and are corrupted. AFAICS, bolton blob in 3rdparty/blobs matches the one my lenovo/g505s was originally shipped with.
Interesting findings, guess that's why our Bolton xHCI support is buggy even with this blob - these additional changes are needed. To be honest xHCI is a low priority to me, I lived fine without it for a long time and right now I'm working on other more important patches...
mikeb mikeb has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/30749 )
Change subject: sb/amd/agesa/hudson/Kconfig: Disable XHCI by default, enable only with firmware ......................................................................
Abandoned
Superseeded by CB:31216 which has been merged already
-
Felix Held (Code Review) -
Kyösti Mälkki (Code Review)
-
mikeb mikeb (Code Review)
-
Nico Huber (Code Review)