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 | ||
---|---|---|
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.
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. |
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 ↗ | (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. |
sys/arm64/cavium/thunder_pcie.c | ||
---|---|---|
465 | Removed it completely, since these values are being setup by parent nexus_activate_resource. |