Page MenuHomeFreeBSD

Add support for ThunderX2 PCIe
ClosedPublic

Authored by pdk_semihalf.com on Apr 20 2018, 9:29 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 4:31 PM
Unknown Object (File)
Mon, Mar 25, 5:39 AM
Unknown Object (File)
Feb 21 2024, 12:19 AM
Unknown Object (File)
Feb 20 2024, 1:50 AM
Unknown Object (File)
Jan 19 2024, 12:56 PM
Unknown Object (File)
Jan 19 2024, 8:30 AM
Unknown Object (File)
Dec 31 2023, 2:36 AM
Unknown Object (File)
Dec 31 2023, 2:36 AM
Subscribers

Details

Summary

This patch contains seperate driver for ThunderX2 PCIe, which contains necesseary fixups (eg. AHCI BAR fixup)

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

andrew requested changes to this revision.Apr 20 2018, 2:41 PM

You should restructure the pci_host_generic_acpi code to handle this so you don't need to copy and paste so much of it in this driver, or even better handle ThunderX2 in the existing driver with quirks.

sys/arm64/cavium/thunderx2_pcie_acpi.c
2 ↗(On Diff #41680)

There is code below that looks like it was copied from an existing driver, however you didn't include the copyright.

243 ↗(On Diff #41680)

Why?

This revision now requires changes to proceed.Apr 20 2018, 2:41 PM

It is good idea to provide support using quirks, however pci_host_generic_acpi still needs some resource allocation changes. I have limited ability to check if it doesn't break another devices, so from my point of view it is safe to provide separate driver.
Of course I will follow your advices, but I'm depending on you to perform review very carefully.

sys/arm64/cavium/thunderx2_pcie_acpi.c
2 ↗(On Diff #41680)

Just an oversight

243 ↗(On Diff #41680)

There is no SATA connectors on board, it requires bar fixing too.

It is good idea to provide support using quirks, however pci_host_generic_acpi still needs some resource allocation changes. I have limited ability to check if it doesn't break another devices, so from my point of view it is safe to provide separate driver.

There are no other devices, it only works on ThunderX2.

It is good idea to provide support using quirks, however pci_host_generic_acpi still needs some resource allocation changes. I have limited ability to check if it doesn't break another devices, so from my point of view it is safe to provide separate driver.

There are no other devices, it only works on ThunderX2.

Hi, if we limit usecase to the the explicit quirks and enable proper host ecam generic operation, we can test solution on Marvell Armada 7k/8k as well (it works without quirks, when booting with ACPI).

This is not final version!

I've uploaded this so I can get some feedback from you

Changes:

  • pci_host_generic_acpi adapted to work with ThunderX2
  • Added quirk mechanism

It works on ThunderX2 but I think it needs to replace memory regions adding (in attach) with something more flexible

sys/dev/acpica/acpi.c
2096

It is still not clear to me why this code is here rather than in the attach routine of the relevant bridge driver.

2105

Why is this moved?

sys/dev/pci/pci_host_generic_acpi.c
171

Are these encoded as Producer resources instead of Consumer resources?

sys/dev/acpica/acpi.c
2096

I haven't written this code, but pci_host_generic (for fdt and acpi) depends on resource which is set by this code. I think one wanted to keep it symetric with fdt. I think it is possible to move it to attach routine.

2105

Because acpi_enable_pcie always overwrite first resource (rid 0), so if we call acpi_parse_resources earlier we lose 1 resource.

sys/dev/pci/pci_host_generic_acpi.c
171

They are encoded as Producer resources.

sys/dev/acpica/acpi.c
2096

I've checked and it can't be moved to attach routine because in attach we can't edit our resources.

Changed approach we add resources to rman. Clean unnecessary changes.

Do you have any objections to this patch?

Remove unused functions and declarations.

sys/dev/pci/pci_host_generic_acpi.c
7–8

The previous statement is the standard one for Foundation-sponsored work, however I am not aware of the history of this file off hand.

418

do you know if these are temporary quirks for pre-prod systems only?

Restore previous statement about Foundation sponsorship

sys/dev/pci/pci_host_generic_acpi.c
7–8

You are right. I should have kept this. Fixed.

418

These quirks are necessary for current ThunderX2 revision. I have no information if there will be some future revisions with these things fixed.

sys/arm64/include/cpu.h
95

Can you split this out into a new review, and include the needed updates to identcpu.c? I have moved some of these macros around so you'll also need to rebase.

sys/dev/pci/pci_host_generic_acpi.c
119–124

Can you split the quirks out into a new review? It is mostly separate from the other changes.

422–424

Is there an erratum for this? I don't see anything in Linux, or my copy of the ThunderX2 known issues document.

No functional changes. This is the previous patch with quirks and macros moved, as andrew suggested

sys/dev/pci/pci_host_generic.c
229

Why 0x80?

sys/dev/pci/pci_host_generic_acpi.c
269–277

Why is this only needed for memory?

sys/dev/pci/pci_host_generic.c
229

Every 0x80 we have _BBN number of root complex. I can create analogue function in pci_host_generic_acpi, and obtain value directly from acpi

sys/dev/pci/pci_host_generic_acpi.c
269–277

In pci_host_generic_acpi_parse_resource we add memory regions to rman. Here we are allocating memory from it.

Added obtaining ecam and bus number from ACPI. This allows us to remove ThunderX2 quirk connected with ecam.

Do you have still objections to this patch?

This revision was not accepted when it landed; it landed in state Needs Review.Jul 9 2018, 8:55 AM
This revision was automatically updated to reflect the committed changes.