Add PCIe driver for ThunderX armv8 system.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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. |
Comment Actions
Also, have a look through the comments in D2386. It's the review to add generic ecam support.
| 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? |
| 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. |
Comment Actions
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.
Comment Actions
Cosmetic changes: aligned with the current driver version to ease any further merge of external pcie driver.
| 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. |
| 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. |
| 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. |
| sys/arm64/cavium/thunder_pcie.c | ||
|---|---|---|
| 464 ↗ | (On Diff #7595) | Removed it completely, since these values are being setup by parent nexus_activate_resource. |