PCI support for Alpine platform from Annapurna Labs

[jpa-semihalf.com]

Summary

This is a continuation of https://reviews.freebsd.org/D2340 and follows Andrew's suggestion to extract code related to PCI support into a separate review.

D7571 now overtakes these changes.

[andrew]

This is from my first pass over the driver.

sys/arm/annapurna/alpine/alpine_pci.c

248

When would not be defined?

285–308

This code looks similar to sys/powerpc/ofw/ofw_pci.c ofw_pci_nranges but using fdt_* in place of the correct OF_* functions. It would be nice to not hve to do the same parsing each time.

415

Why the else given the return above?

423

(bus)?

430

What is magic about 0xff?

442

Extra newline.

451

Should these be under bootverbose?

480

0xff?

497

No need for the brace, and is this check needed? When would ofw_bus_search_compatible match something that's not a pci device?

528

This doesn't look how we would normally structure this, it would be something like:

struct al_pcie_atu_region io_atu_region;

if (sc->sc_type == AL_PCI_TYPE_INTERNAL)
  return (0);

io_atu_region.enable = AL_TRUE;
io_atu_region.direction = AL_PCIE_ATU_DIR_OUTBOUND;
...

548

Should this be under bootverbose?

597

We have setbit, clrbit, isset, and isclr in sys/param.h for this sort of thing.

680

What's special about that value and why does it need to be written before the value can be read?

734

Shouldn't this be a bus_size_t?

848

What's special about 5?

963

Why 12?

980

You can combine v and tmp on one line, and mps and reg on another.

1099

(hdrtype & PCIM_MFDEV) != 0

1181

This looks wrong, why are you parsing the fdt data in the clikd class? It should be being provided by the parent, then you use bus_alloc_resource or similar to get the resource.

1222

Don't use fdtbus_bs_tag in this driver, it should come from the parent. fdtbus_bs_tag should only be used by the early console.

1241

Doesn't this leave the hardware partially configuredon failure?

1392

Why 0xffffffff?

1471

Unneeded

[emaste]

sys/arm/annapurna/alpine/alpine_pci.c

451

Does this represent a hardware error, or a normal operating condition e.g. with nothing installed in a PCIe slot?

548

Yes, in my opinion.

[jpa-semihalf.com]

sys/arm/annapurna/alpine/alpine_pci.c

248

Its an options flag for MSI, instead of wired one.

415

I don't understand, could you elaborate?

680

I think this is the way to learn the size (PCI 3.0 spec, sec 6.2.5.1)

[jpa-semihalf.com]

sys/arm/annapurna/alpine/alpine_pci.c

451

The 'funny' thing is that the al_pcie_link_status never returns values < 0. But right, I must verify whether presence != link down.

[andrew]

sys/arm/annapurna/alpine/alpine_pci.c

248

So is it needed or is it always defined?

415

You have:

if (bus == 0) {
  ...
  return (0);
} else {
  ...
}

The else case isn't needed as there is no way to get to past the if case. You could rewrite it as:

if (bus == 0) {
  ...
  return (0);
}
...

[emaste]

sys/arm/annapurna/alpine/alpine_pci.c

415

See e.g. this discussion from LLVM's coding standards doc:
http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
We don't have this explicitly documented in FreeBSD, but in general prefer early returns.

[jpa-semihalf.com]

sys/arm/annapurna/alpine/alpine_pci.c

248

Actually, it is MSIx that will be the default option. If you opt for making it always on, I am OK about that.

415

I am sorry, I looked at it but didn't see it. I will also remove the double test against (slot >0).

[jhb]

My main comment here is that this seems to duplicate a lot of work of the PCI bus driver itself. With NEW_PCIB, the PCI bus is actually far more capable of handling things like assigning resources for bridges and the devices behind them, etc. I'm not sure how much would have to be updated in layers above this driver to allow that to work, but it might reduce a lot of the duplication.

sys/arm/annapurna/alpine/alpine_pci.c

1328

Do not set bustag/handle in alloc routines. Please provide proper activate/deactivate routines instead (NEW_PCIB requires this). You can see my suggestions on how to do this properly (arm does this wrong an awful lot) in D2386. You should also add an adjust_resource routine.

[jpa-semihalf.com]

sys/arm/annapurna/alpine/alpine_pci.c

451

Ok, I will remove the check, as the hal code returns the status up/down.

1241

In general that would be a clean solution. However, if pci fails, the board actually needs rebooting.

[jpa-semihalf.com]

This is an updated version of previous drop, and addresses comments from previous review.
Additionally, it adds MSI-x support, because during discussion it turned out that this is a cleaner way.

[jpa-semihalf.com]

Filled missing options that were added.

[jhibbits]

A lot of this looks like duplicated code from powerpc/ofw. At some point, this duplicated code should be merged into sys/dev/ofw, so all fdt/ofw platforms benefit from updates and bugfixes.

sys/arm/annapurna/alpine/alpine_pci.c

1545

This function is identical to ofw_pci_route_interrupt() in sys/powerpc/ofw/ofw_pci.c. Can you dedup these (and other identical functions)? Or, at least file a bug to do it, since it doesn't make sense to have multiple identical copies of functions spread across nearly identical files for various architectures/platforms.

[jpa-semihalf.com]

This is an update version, with fixes applied + uses the common code to be found in D4879: OFW PCI code decuplications - involves ARM, PPC and SPARC64

[jpa-semihalf.com]

It looks things went forward - the D4879 got into mainline. There are no dependencies left, may I ask you for your review, then?

[jpa-semihalf.com]

This is yet another version, which takes into consideration other Alpine platforms.

[jpa-semihalf.com]

Anyone?

[jhibbits]

My comments have been addressed, but someone more familiar should review it.

[jhb]

In general this driver is still doing a lot of work to assign resources for BARs, PCI-PCI bridge windows, etc. that it shouldn't have to do. Using the unit number of the bridge as the base bus number also seems quite wrong if you ever have more than one bridge since the second bridge would have a unit number (device_get_unit()) of '1'.