Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

soc/intel/meteorlake: Set UsbTcPortEn based on tcss_port[x] (ported from alderlake) #230

Closed
wants to merge 6 commits into from
Closed

soc/intel/meteorlake: Set UsbTcPortEn based on tcss_port[x] (ported from alderlake) #230

wants to merge 6 commits into from

Conversation

tobiasfunke1
Copy link
Contributor

Looks like for the Darter Pro 10 (Meteor Lake) there is the same bug with the usb3 solved in #227 for Raptor Lake and Alder Lake.

So I just ported the code to Meteor Lake, but I was not be able to test it yet. Can someone confirm that the patch is working?

Seams to be related to system76/firmware-open#511 and system76/firmware-open#497 but both using Raptor Lake so it seams to be fixed after the patch. Here is the related PR system76/firmware-open#556.

jackpot51
jackpot51 previously approved these changes Jul 10, 2024
@jackpot51 jackpot51 requested review from a team and crawfxrd July 10, 2024 18:22
@jackpot51
Copy link
Member

The code looks fine. I've asked for QA to check this out, and Tim who wrote the patch for ADL

@crawfxrd
Copy link
Member

The ADL patch is from https://review.coreboot.org/c/coreboot/+/82058.

There is also https://review.coreboot.org/c/coreboot/+/80500 for FSP-M.

Copy link
Member

@leviport leviport left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've confirmed that the USB-C ports are running at 2.0 speeds on darp10 and darp10-b. After flashing this patch, however, I'm not seeing the speeds increase to expected speeds.

For testing, I'm using an NVMe in a USB-C caddy, and using the "Benchmark" utility built into Gnome Disks. On darp10 & darp10-b, it averages ~45 MB/s. The same drive on the USB-C ports on lemp13 hits roughly 1 GB/s.

@leviport
Copy link
Member

I recall the USB-C ports running at 2.0 speeds on lemp13 during product development as well, but I'm having trouble finding which commit fixed that. This one looks like it might be promising? 828cf02

@tobiasfunke1
Copy link
Contributor Author

tobiasfunke1 commented Jul 17, 2024

@leviport Thanks for the confirmation of the problem and trying the suggested patch. Since the patch is not working and you mentioned a commit that may work, can you check if it fixes the issue? The commit you mentioned was ported form Alder Lake c6b65c1.

Should we create an issue for that problem now?

@leviport
Copy link
Member

I was pretty sure that 828cf02 was already part of darp10's firmware (also a meteor lake board), but I can double-check that today.

I don't think we need a new issue if this PR is already tracking the issue. If this has to be closed for some reason, we can make an issue so that the bug remains tracked.

@leviport
Copy link
Member

I rebuilt and flashed darp10-b firmware with the latest system76 branch of coreboot checked out, and the USB-C ports are still benchmarking at 2.0 speeds. So despite that commit fixing lemp13, it seems something different needs to be done for darp10.

@theguy147
Copy link

theguy147 commented Jul 18, 2024

This is a snippet from the lemp13/overridetree.cb

	device domain 0 on
		subsystemid 0x1558 0x2624 inherit

		device ref tbt_pcie_rp0 on end
		device ref tcss_xhci on
			register "tcss_ports[0]" = "TCSS_PORT_DEFAULT(OC_SKIP)"
			#TODO: TCP1 is used as USB Type-A
			register "tcss_ports[1]" = "TCSS_PORT_DEFAULT(OC_SKIP)"
			#TODO: TCP2 is used as HDMI
			#TODO: TCP3 goes to redriver, then mux, then USB Type-C
			register "tcss_ports[3]" = "TCSS_PORT_DEFAULT(OC_SKIP)"

In that snippet various tcss_ports are set. In comparison, in the darp10/overridetree.cb no tcss_ports are set at all. Maybe this is the problem with this patch not helping as they arent even set/enabled in the device specific configuration files?

Sorry, if this is completely off as I do not yet understand it all :)

EDIT: Is there some public documentation for the motherboard used in the darp10 so we can register the correct tcss_ports in the darp10/overridetree.cb file and see if that would fix the issue?

@tobiasfunke1
Copy link
Contributor Author

Hi @theguy147, I looked into it and this makes sense to me.

As I can see it, when @jackpot51 added the lemp13 in bcd3446, he registered the tcss_ports. Those were not registered for the darp10 in 49122b0 by @crawfxrd.

Can someone please confirm, that this makes sense and test if this patch works now?

@leviport leviport requested a review from a team July 23, 2024 14:18
@leviport
Copy link
Member

I'll give it a re-test soon!

@leviport
Copy link
Member

Now I'm getting ~1GB/s speeds on the back (thunderbolt) USB-C port, but I'm still seeing ~45MB/s on the front (non-TB) port. An improvement!

Lemp13 also has two USB-C's, one of which is TB. However, both USB-C's on lemp13 support USB 3.x speeds. I wonder what else is required to enable full speed on the front USB-C port as well?

@theguy147
Copy link

Using the same config for the darp10 as for the lemp13 was just a guess and there might actually be differences such as a different port than TCP2 being used for HDMI and probably different ACPI config. For the exact config override it would be best to know the specifics of the motherboard in the darp10 and I guess this will be easy for you guys at system76 as you have them. Otherwise I might find some time later today or tomorrow to open up my darp10 and try to see if I can correctly trace the ports or something.

@leviport
Copy link
Member

We do have schematics available: https://github.com/system76/firmware-open/blob/master/docs/schematics.md

That should make things much easier than following traces manually, at least.

@theguy147
Copy link

theguy147 commented Jul 24, 2024

Thanks @leviport

Looking through the schematics, this should be the correct tcss_xhci config:

		device ref tcss_xhci on
			register "tcss_ports[0]" = "TCSS_PORT_DEFAULT(OC_SKIP)"
			register "tcss_ports[1]" = "TCSS_PORT_DEFAULT(OC_SKIP)"
			# TCP2 is used as HDMI
			# TCP3 is not used
			chip drivers/usb/acpi
				device ref tcss_root_hub on
					chip drivers/usb/acpi
						register "desc" = ""TBT Type-C""
						register "type" = "UPC_TYPE_C_USB2_SS_SWITCH"
						device ref tcss_usb3_port0 on end
					end
					chip drivers/usb/acpi
						register "desc" = ""USB Type-C""
						register "type" = "UPC_TYPE_C_USB2_SS_SWITCH"
						device ref tcss_usb3_port1 on end
					end
				end
			end
		end

The updated ACPI config (specifically for tcss_usb3_port1 ) should probably also fix the issue for the other USB-C port.

@tobiasfunke1 If you agree, could you update this PR accordingly?

update the ACPI config for the type c ports
@tobiasfunke1
Copy link
Contributor Author

Thanks to @theguy147 for going trough the schematics of the darp10.

I have updated the PR with the new ACPI configuration, @leviport can you please check if it still works and if the non-TB port works now?

@leviport
Copy link
Member

Yep, I'll give it a try today!

@tobiasfunke1
Copy link
Contributor Author

There is a Dasharo coreboot meteorlake branch, where the devicetree.cb for the clevo mainboard looks like this https://github.com/Dasharo/coreboot/blob/mtl-h/src/mainboard/clevo/mtl-h/devicetree.cb#L105-L124.

This confirms that we are on the right way. :)

@leviport
Copy link
Member

Sorry to report back with bad news, but that pesky front USB-C port is still benchmarking around ~45MB/s with the latest update. The rear (TB) port is still looking good at over ~1GB/s though.

@theguy147
Copy link

@tobiasfunke1 Could you apply the following changes which solved the issue on my machine?

diff --git a/src/mainboard/system76/mtl/variants/darp10/ramstage.c b/src/mainboard/system76/mtl/variants/darp10/ramstage.c
index 276279484f..bc6d94d2f3 100644
--- a/src/mainboard/system76/mtl/variants/darp10/ramstage.c
+++ b/src/mainboard/system76/mtl/variants/darp10/ramstage.c
@@ -4,6 +4,13 @@

 void mainboard_silicon_init_params(FSP_S_CONFIG *params)
 {
+	// Enable TCP1 USB-A conversion
+	// BIT 0:3 is mapping to PCH XHCI USB2 port
+	// BIT 4:5 is reserved
+	// BIT 6 is orientational
+	// BIT 7 is enable
+	params->EnableTcssCovTypeA[1] = 0x82;
+
 	// XXX: Enabling C10 reporting causes system to constantly enter and
 	// exit opportunistic suspend when idle.
 	params->PchEspiHostC10ReportEnable = 0;

As the front (non-TBT) USB-C port is connected to USB2 port 2 and to TCP1 only via an Re-Driver and a MUX, this patch allows the MUX to do its thing and correctly patch the USB through for faster speeds than USB2 :)

@tobiasfunke1
Copy link
Contributor Author

Seems to be legit, there is a similar initialization for the lemp13: https://github.com/system76/coreboot/blob/system76/src/mainboard/system76/mtl/variants/lemp13/ramstage.c

I will add it to the patch.

Add enabling of TCP1 USB-A conversion in mainboard silicon initialization
@tobiasfunke1
Copy link
Contributor Author

Done, @leviport can you please confirm that the patch works for you too?

@leviport
Copy link
Member

Will do!

Copy link
Member

@leviport leviport left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix confirmed! Benchmark speeds on both USB-C ports are faster than 2.0 specs. Both ports also still support PD charging. I double-checked TB and DP on the back port, and both are still working nicely. Thanks for all the help on this fix!

@leviport leviport requested review from a team and removed request for crawfxrd and jackpot51 July 29, 2024 17:44
@crawfxrd
Copy link
Member

Still no way to manually mark things as merged on GitHub.

Applied as:

  • a1b808e ("mb/system76/darp10: Add TCSS configs")
  • 7877cab ("soc/intel/meteorlake: Set UsbTcPortEn based on tcss_port[x]")

@crawfxrd crawfxrd closed this Jul 30, 2024
@tobiasfunke1 tobiasfunke1 deleted the tobiasfunke1-meteorlake-usb3 branch July 30, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants