Add PCIe driver for ThunderX armv8 system.
Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
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 | ||
|---|---|---|
| 3 | Just 2014? | |
| 120–121 | A comment explaining what these are for would be useful. | |
| 125–128 | And these | |
| 190–191 | What are these for? | |
| 232 | Why do you need to use fdtbus_bs_tag here? It should only be used directly in the early boot code. | |
| 327 | What is this magic number? | |
| 340 | Why 0x3? | |
| 344 | Magic 32? | |
| 356 | 0x3? | |
| 365 | Why a delay of 1000? | |
| 368 | Should be (regval & PEM_LINK_LT) != 0 | |
| 386–410 | 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); | |
| 426 | What is a PEM number? | |
| 427–428 | Magic numbers | |
| 432 | retval != 0 | |
| 515 | Should be: /* * This ... | |
| 767 | Magic | |
| 779 | Magic | |
| 811 | Magic | |
| 933 | Why does this address need to be translated? | |
| 983–1014 | It feels like you could extract the ecam value from the data. | |
| 1088–1098 | 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 | ||
| 52 | 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.
| sys/conf/files.arm64 | ||
|---|---|---|
| 52 | 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 | ||
|---|---|---|
| 52 | 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. | |
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.
Cosmetic changes: aligned with the current driver version to ease any further merge of external pcie driver.
| 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 | 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. | |
| sys/arm64/cavium/thunder_pcie.c | ||
|---|---|---|
| 465 | Removed it completely, since these values are being setup by parent nexus_activate_resource. | |