Improve ThunderX PEM driver to work on pass2 revision

[wma]

Summary

Things changed:
* do not allocate 4GB of SLI space, because it's the waste of
  system resources. Allocate only small portions when needed.
* provide own implementation of activate_resource which performs
  address translation between PCI bus and host PA address space.

[wma]

sys/arm64/cavium/thunder_pcie_pem.c

412 ↗

(On Diff #13345)

Argh... will add error check here and below.

[jhb]

sys/arm64/cavium/thunder_pcie_pem.c

246 ↗

(On Diff #13345)

Instead as I mentioned in another review/mail, your resources you hand out below should be in PCI space, not CPU space (rman's can be in any space). You would then not need to read the BAR but just assume the rman_get_start() value is in the PCI space.

I think the problem you are running into here is that you are counting on nexus to do the actual bus space tag/handle allocation. This is where bus_map_resource() would come in handy as it would let you allocate a single resource for the PCI window from your parent and use bus_map_resource() of the appropriate sub-range of _that_ resource from the nexus to determine the tag/handle to use for the associated PCI resource.

[jhb]

sys/arm64/cavium/thunder_pcie_pem.c

246 ↗

(On Diff #13345)

For now, instead of waiting for bus_map_resource(), let's fix this by explicitly setting the tag/handle (duplicating what is in the nexus) after you fix the rmans to operate on the PCI space (instead of the CPU space). We can later clean that up to remove the duplication with nexus once bus_map_resource() is in place. However, I think that will resolve several of the patches in flight and will let you use the existing PCI bus driver as-is.

In summary, change the memory rman in this driver to use PCI addresses (not PHYS). When this driver activates a resource, you know that the rman_get_start() is a PCI address, so convert it to a phys address and use that phys address to generate the appropriate bus space tag/handle in the activate_resource method by duplicating the current logic in nexus for that. This should let you remove the need to access BARs directly from this driver and remove the need for your changes to the PCI bus driver.

[wma]

Fixed

[jhb]

sys/arm64/cavium/thunder_pcie_pem.c

152 ↗

(On Diff #13491)

Eventually you will also want a deactivate resource method, but you can possibly get by without it for now if the parent nexus happens to do the right thing. In general I don't like depending on that and feel that if a bus driver hands out resources from an rman it should directly handle all requests related to resources from that rman (so it should use rman_adjust_resource to implement bus_adjust_resource, etc.).

Also, you will want to add a bus_adjust_resource() method at some point to support NEW_PCIB. You just need to call rman_adjust_resource() for MEMORY / IOPORT resources using your rman. The PCI-PCI bridge driver uses this when it needs to resize it's I/O windows.

490 ↗

(On Diff #13491)

Hmm, do you really expect to get requests for resources that aren't in your PCI space? If not, that will let you DTRT and handle wildcard requests (like when a BAR doesn't have any resources assigned). Instead, you would just do this:

switch (type) {
   ...
};

if (bootverbose) {
   device_printf(dev, "%s: start=%#lx, end = %#lx, count = %#lx\n", start, end, count);

res = rman_reserve_resource(rm, start, end, count, flags, child);
if (res == NULL)
    goto fail;

That will only allocate resources in the PCI range (since those are the only ranges the rman's manage) so that a "default" range of 0 and ~0 will pick a free range from the rman that satisfies the count and alignment.

However, the question is if you can have any downstream resources that aren't PCI ranges that require translation. If every transaction across this bridge always translates (i.e. the bridge only accepts requests for addresses in its "window" and then translates those addresses when issuing the PCI request "down" to its children) then this is true and the simpler code I wrote should work fine. It will also fully handle devices whose BARs aren't assigned by the firmware with the existing PCI bus driver.

653 ↗

(On Diff #13491)

This always uses the memory rman. That's probably not what you want as you are putting the I/O range in your memory rman. The current code should cause all IOPORT resources to always fail to allocate since the I/O rman is empty. I think the original code here is closer, you just want to use 'pci_base' instead of 'phys_base' in the old code I think?

[wma]

Rebased against HEAD.

[jhb]

Does this work well in your testing? Having the thunder_pem_rman() method will make your adjust_resource method very trivial. Likely it should be something like:

{
     rm = thunder_pem_rman(type);
     if (rm == NULL)
        return (bus_generic_adjust_resource(...));
     if (!rman_is_region_manager(r, rm))
        /* This means a child device has a memory or I/O resource not from you which shouldn't happen. */
       return (EINVAL);
     return (rman_adjust_resource(r, start, end));
}

I think the diff looks good, I just want to verify that this now allows you to allocate BARs for devices when EFI doesn't do so.

[wma]

Yes, it works well in case EFI does not provide BAR configuration.
Also, added adjust resource.

[wma]

sys/arm64/cavium/thunder_pcie_pem.c

311 ↗

(On Diff #13882)

"you" -> "PEM driver"

[wma]

I guess that all nits are now resolved. I commit this change tomorrow if no further issues are reported. Thanks.

[jhb]

Thanks for being patient on fixing this "right". For fun you should try NEW_PCIB out now on the ThunderX board to see if it boots ok. For more adventure there are tunables for the PCI bus and PCI-PCI bridge driver to force it to ignore firmware-assigned windows and buses and allocate resources from scratch (hw.pci.clear_bars and hw.pci.clear_pcib) that would be worth trying with NEW_PCIB if it works initially.