Page MenuHomeFreeBSD

PCIe support for ThunderX
ClosedPublic

Authored by wma_semihalf.com on Jul 9 2015, 7:59 AM.

Details

Summary

Add PCIe driver for ThunderX armv8 system.

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

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.
andrew edited edge metadata.Jul 9 2015, 12:24 PM

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

Just 2014?

119–120 ↗(On Diff #6804)

A comment explaining what these are for would be useful.

124–127 ↗(On Diff #6804)

And these

189–190 ↗(On Diff #6804)

What are these for?

231 ↗(On Diff #6804)

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

326 ↗(On Diff #6804)

What is this magic number?

339 ↗(On Diff #6804)

Why 0x3?

343 ↗(On Diff #6804)

Magic 32?

355 ↗(On Diff #6804)

0x3?

364 ↗(On Diff #6804)

Why a delay of 1000?

367 ↗(On Diff #6804)

Should be (regval & PEM_LINK_LT) != 0

385–409 ↗(On Diff #6804)

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

What is a PEM number?

426–427 ↗(On Diff #6804)

Magic numbers

431 ↗(On Diff #6804)

retval != 0

514 ↗(On Diff #6804)

Should be:

/*
 * This ...
766 ↗(On Diff #6804)

Magic

778 ↗(On Diff #6804)

Magic

810 ↗(On Diff #6804)

Magic

932 ↗(On Diff #6804)

Why does this address need to be translated?

982–1013 ↗(On Diff #6804)

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

1087–1097 ↗(On Diff #6804)

Magic

sys/arm64/include/fdt.h
1 ↗(On Diff #6804)

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

sys/conf/files.arm64
50 ↗(On Diff #6804)

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

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?

emaste added inline comments.Jul 24 2015, 2:15 PM
sys/conf/files.arm64
52 ↗(On Diff #7257)

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.

wma_semihalf.com marked 25 inline comments as done.Jul 30 2015, 5:33 AM

Marking all issues as done.

andrew added inline comments.Jul 30 2015, 10:39 AM
sys/arm64/cavium/thunder_pcie.c
263 ↗(On Diff #7494)

Why is the I/O space ignored?

309–311 ↗(On Diff #7494)

Should this be a KASSERT?

345–347 ↗(On Diff #7494)

KASSERT?

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

Is this used?

andrew added a reviewer: jhb.Jul 30 2015, 10:39 AM
sys/arm64/cavium/thunder_pcie.c
263 ↗(On Diff #7494)

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

309–311 ↗(On Diff #7494)

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.

emaste added inline comments.Jul 30 2015, 6:41 PM
sys/arm64/cavium/thunder_pcie.c
309–311 ↗(On Diff #7494)

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
309–311 ↗(On Diff #7494)

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.

emaste added a subscriber: arm64.Jul 31 2015, 3:59 PM
jhb added inline comments.Aug 1 2015, 4:55 PM
sys/arm64/cavium/thunder_pcie.c
309–311 ↗(On Diff #7494)

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())

463 ↗(On Diff #7494)

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

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

jhb accepted this revision.Aug 3 2015, 9:25 PM
jhb edited edge metadata.
andrew accepted this revision.Aug 3 2015, 9:32 PM
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.