Page MenuHomeFreeBSD

Add support for ThunderX2 PCIe
ClosedPublic

Authored by pdk_semihalf.com on Apr 20 2018, 9:29 AM.

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
andrew added a reviewer: jhb.Apr 20 2018, 2:42 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.

mw added a comment.Apr 24 2018, 1:00 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.

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

jhb added inline comments.Apr 24 2018, 5:58 PM
sys/dev/acpica/acpi.c
2096 ↗(On Diff #41814)

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

2105 ↗(On Diff #41814)

Why is this moved?

sys/dev/pci/pci_host_generic_acpi.c
171 ↗(On Diff #41814)

Are these encoded as Producer resources instead of Consumer resources?

sys/dev/acpica/acpi.c
2096 ↗(On Diff #41814)

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 ↗(On Diff #41814)

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 ↗(On Diff #41814)

They are encoded as Producer resources.

sys/dev/acpica/acpi.c
2096 ↗(On Diff #41814)

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.

emaste added inline comments.Jun 12 2018, 2:07 AM
sys/dev/pci/pci_host_generic_acpi.c
7–8 ↗(On Diff #42789)

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

442 ↗(On Diff #42789)

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 ↗(On Diff #42789)

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

442 ↗(On Diff #42789)

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

andrew added inline comments.Jun 19 2018, 4:58 PM
sys/arm64/include/cpu.h
95 ↗(On Diff #43643)

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
126–131 ↗(On Diff #43643)

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

447–449 ↗(On Diff #43643)

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

Remove unused declarations.

andrew added inline comments.Jun 22 2018, 12:14 PM
sys/dev/pci/pci_host_generic.c
229 ↗(On Diff #44205)

Why 0x80?

sys/dev/pci/pci_host_generic_acpi.c
213 ↗(On Diff #44205)

Why is this only needed for memory?

sys/dev/pci/pci_host_generic.c
229 ↗(On Diff #44205)

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
213 ↗(On Diff #44205)

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?

Any further comment @andrew?

Removed unnecessary changes

This revision was not accepted when it landed; it landed in state Needs Review.Jul 9 2018, 8:55 AM
Closed by commit rS336129: ARM64: Add support for ThunderX2 PCIe (authored by wma, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.