Page MenuHomeFreeBSD

PCIe support for ThunderX
ClosedPublic

Authored by wma_semihalf.com on Jul 9 2015, 7:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 6, 1:17 AM
Unknown Object (File)
Sat, Apr 6, 1:14 AM
Unknown Object (File)
Sat, Apr 6, 12:28 AM
Unknown Object (File)
Fri, Apr 5, 11:29 PM
Unknown Object (File)
Fri, Apr 5, 9:26 PM
Unknown Object (File)
Mar 16 2024, 9:28 AM
Unknown Object (File)
Mar 16 2024, 9:28 AM
Unknown Object (File)
Mar 16 2024, 9:28 AM

Details

Summary

Add PCIe driver for ThunderX armv8 system.

Diff Detail

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

Event Timeline

wma_semihalf.com retitled this revision from to PCIe support for ThunderX.
wma_semihalf.com updated this object.
wma_semihalf.com edited the test plan for this revision. (Show Details)
wma_semihalf.com added reviewers: zbb, andrew, emaste.
wma_semihalf.com set the repository for this revision to rS FreeBSD src repository - subversion.

In general there are too many magic numbers, and comments explaining what's happening and why would be useful in a few places.

sys/arm64/cavium/thunder_pcie.c
2

Just 2014?

119–120

A comment explaining what these are for would be useful.

124–127

And these

189–190

What are these for?

231

Why do you need to use fdtbus_bs_tag here? It should only be used directly in the early boot code.

326

What is this magic number?

339

Why 0x3?

343

Magic 32?

355

0x3?

364

Why a delay of 1000?

367

Should be (regval & PEM_LINK_LT) != 0

385–409

Why not the following, but with a comment on why 3 is magic?

sli = sc->pem / 3;
sli_group = sc->pem % 3;
if (sli > 1)
  return (ENXIO);
425

What is a PEM number?

426–427

Magic numbers

431

retval != 0

514

Should be:

/*
 * This ...
766

Magic

778

Magic

810

Magic

932

Why does this address need to be translated?

982–1013

It feels like you could extract the ecam value from the data.

1087–1097

Magic

sys/arm64/include/fdt.h
1

Why do you need this file? We have been working on removing the need for it on other architectures.

sys/conf/files.arm64
50

Can you add a thunderx_pci option? It would allow for users to build a custom kernel with support for just the hardware they need.

Also, have a look through the comments in D2386. It's the review to add generic ecam support.

wma_semihalf.com edited edge metadata.

Reduced the diff to internal Root Complex only.

sys/conf/files.arm64
50

So what is our current path? I thought we include all possible drivers to make a GENERIC kernel work on any arm64 hardware. Was there any change in priorities I'm not aware of?

sys/conf/files.arm64
50

Yes, GENERIC kernel should be able to run on any arm64 hardware. But users should also be able to compile a custom kernel without drivers they don't want. So in this case thunderx_pci would become an option, and GENERIC would have that option enabled.

andrew requested changes to this revision.Jul 27 2015, 3:09 PM
andrew edited edge metadata.

Do not use any fdt_* functions here, they should only be used in the early boot code, i.e. nothing later than configuring the uart before we get to machine independent code.

You should use ofw_* or OF_* functions. If you are missing something we can add it.

This revision now requires changes to proceed.Jul 27 2015, 3:09 PM
wma_semihalf.com edited edge metadata.

Removed all fdt-related stuff and cleaned up a little.

wma_semihalf.com edited edge metadata.

Cosmetic changes: aligned with the current driver version to ease any further merge of external pcie driver.

Marking all issues as done.

sys/arm64/cavium/thunder_pcie.c
264

Why is the I/O space ignored?

310–312

Should this be a KASSERT?

346–348

KASSERT?

sys/conf/options.arm64
4 ↗(On Diff #7494)

Is this used?

sys/arm64/cavium/thunder_pcie.c
264

It's not supported by internal PCIe controller. Will add a comment.

310–312

No, because KASSERTs can be disabled. These values are used to calculate an offset in #315 and I don't want to allow any access exceed ECAM space. That could corrupt PCIe registers and stay unnoticed for a long time.

sys/conf/options.arm64
4 ↗(On Diff #7494)

Will remove.

sys/arm64/cavium/thunder_pcie.c
310–312

I'd be a little worried that returning here might also mask a problem and thus leave it unnoticed.

sys/arm64/cavium/thunder_pcie.c
310–312

I'll add device_printf here and below. I guess it will be the best compromise. The PCI spec tells to return 0xff...ff whenever the request is invalid so we be in-sync with it, and the user still could notice that something is not right.

sys/arm64/cavium/thunder_pcie.c
310–312

The x86 code has returned -1 for this condition for a long time without a printf. I think this is fine as-is.

(See sys/amd64/pci/pci_cfgreg.c pcireg_cfgread() and pciereg_cfgread())

464

Please do not set bustag/handle in bus_alloc_resource() routines, but do it in a bus_activate_resource() method for this driver instead.

wma_semihalf.com marked 4 inline comments as done.
wma_semihalf.com added inline comments.
sys/arm64/cavium/thunder_pcie.c
465

Removed it completely, since these values are being setup by parent nexus_activate_resource.

jhb edited edge metadata.
andrew edited edge metadata.
This revision is now accepted and ready to land.Aug 3 2015, 9:32 PM
This revision was automatically updated to reflect the committed changes.