Add driver for bcm2838 PCI express controller

[crowston_protonmail.com]

Summary

This adds support for the Broadcom bcm2711 PCI express controller, found
on the Raspberry Pi 4 (aka the bcm2838 SoC). The driver has only been
developed against the soldered-on VIA XHCI controller and not tested
with other end points.

Test Plan

Tested with two USB keyboard, a mouse, and various memory sticks on two 2 GB Pi 4s and a 4 GB Pi 4, with various xhci firmware revisions.

Klaus Küchemann from the mailing list also tested it on his Pis with hubs and other devices.

There are some intermittent USB controller resets, sometimes when transferring to or from a memory stick.

[andrew]

I think you can use driver inheritance to your advantage more in this driver to reduce duplicate code.

sys/arm/broadcom/bcm2835/bcm2838_pci.c

111

For a generic_pcie_fdt_driver derived driver you should use generic_pcie_fdt_softc as the base softc.

230–231

Multi line comments should be in the form

/*
 * The controller ...
 * ...
 */

263–265

Is there a reason we caan't use the fdt aalloc resource function?

349

It looks like non-IRQ types could be handled by generic_pcie_activate_resource

429–432

I think we could create a new kobj interface for the pci_host_generic driver to validate a location and to return the offset.

598

What is this magic number?

771

What are these magic numbers?

sys/dev/pci/pci_host_generic_fdt.c

123–124

After using the fdt softc in the bcm driver you can assume device_get_softc(dev); will return the correct thing. The base softc is always first in the more specific struct so the returned pointer will be correct.

[crowston_protonmail.com]

sys/arm/broadcom/bcm2835/bcm2838_pci.c

263–265

It stems from some confusion I have.

Essentially the addresses that get passed in here are from the zero offset, 0x0. e.g.., the bridge window when requested is 0x0 through 0-0xfffff.

That's not a valid range for this controller.

The fdt.c's alloc function assumes the addresses given will be physical CPU addresses (for this chip, such addresses begin at 0x600000000) and adjusts them to PCI addresses by subtracting the physical offset (in this case 0x600000000) and adding the pci offset (in this case 0xf8000000).

AFAIK, the bridge window allocation code apparently works on 32bit numbers, so it can never supply a valid CPU address range to the fdt's function.

On line 284 I just add the offset back in to move the addresses up into the PCI controller's outbound memory range.

I'd definitely be happy to have all this cleared up.

349

generic_pcie_activate_resource has no concept to remap the PCI address stored in the rman to CPU addresses. I think it assumes that PCI address == CPU address.

Come to think of it, it may be possible to dispense with storing the PCI addresses altogether, and store everything as a CPU address. In which case we could use the generic function.

429–432

That's how openbsd and Linux do it, they have a hook for PCI drivers to express the offset to the configuration of a given end point.

The only thing we have to be careful when designing such an interface, is that the function I call is not side-effect free; it is not just a calculation, it changes the state of the controller.

We would also need a concept of "is this a valid endpoint to request?", since unfortunately trying to probe the endpoint config for {bus=0, slot=0, func=1} somehow locks up the memory controller on the processor.

[andrew]

sys/arm/broadcom/bcm2835/bcm2838_pci.c

263–265

It's possible the calculation in pci_host_generic_alloc_resource is wrong. From a quick glance I can see other issues with the code and based on a look at the dts files in the tree pci_base == phys_base for everything FreeBSD supports.

My reading of pci_pci.c leads me to think we pass in PCI addresses so they should be transformed to a physical address.

349

It looks like it is remapping from pci to cpu address space by updating the rman start and end, e.g. rman_set_start(r, rman_get_start(r) - pci_base + phys_base);

429–432

It would take a bus, slot, function, register, and pointer to the offset value and a bool. I also want to split the code below into a new kobj function so could have a look at doing that in the next few days.

[mmel]

Why you choose to use pci_host_generic as base class? By my best knowledge pci_host_generic is driver for ACPI/ECAM compatible PCI root port and it have nothing with Broadcom iProc PCIe controller. I think that it should be derived from ofw_pci_driver. Please see pci_dw.c for template...
(Don't kill me immediately, this change is not that horrible as it looks on first view).

Also, I'm near sure that handling with MSI interrupts are wrong/incomplete. The binding (devicetree/bindings/pci/brcmstb-pcie.txt)indicates that there is internal MSI controller connected to single interrupt line of GIC. If is this true, you should implement it.

[andrew]

I've created D25121 to translate addresses in the base pci host generic driver. It also cleans up other memory management while there.

[andrew]

In D25068#553228, @mmel wrote:

Why you choose to use pci_host_generic as base class? By my best knowledge pci_host_generic is driver for ACPI/ECAM compatible PCI root port and it have nothing with Broadcom iProc PCIe controller. I think that it should be derived from ofw_pci_driver. Please see pci_dw.c for template...
(Don't kill me immediately, this change is not that horrible as it looks on first view).

pci_host_generic should work for any PCI controller that uses a memory mapped interface.

[crowston_protonmail.com]

@andrew Now that I have an 8 GB Pi4 I realise I need to change the dma mapping to ensure that DMA is only performed in the lower 3 GB of the physical address space (this is a hardware limitation of the controller). The bus_dma_tag_create() call is right in the guts of pci_host_generic_core_attach(). Any thoughts on the best way to thread this through?

[maciphone2_googlemail.com]

In D25068#553366, @andrew wrote:

I've created D25121 to translate addresses in the base pci host generic driver. It also cleans up other memory management while there.

you both already know that D25068 is not yet compatible with D25121, right?

[crowston_protonmail.com]

Updated per comments and to handle changes made in pci_host_generic.c.

[crowston_protonmail.com]

In D25068#553228, @mmel wrote:

Why you choose to use pci_host_generic as base class?

I emulated the other arm and DTB-based drivers which used pci_host_generic_fdt. (Sorry for confusion in initially using pci_host_generic in the softc.)

Also, I'm near sure that handling with MSI interrupts are wrong/incomplete. The binding (devicetree/bindings/pci/brcmstb-pcie.txt)indicates that there is internal MSI controller connected to single interrupt line of GIC. If is this true, you should implement it.

It may be wrong and incomplete, but it works. (i.e., I can see interrupts are being fired, and either on the older hardware revisions or with the companion patch, I can see that USB works as expected). I have no documentation for an alternative operating mode for the MSI controller.

[maciphone2_googlemail.com]

... companion patch, ....

yes, this driver should land together with D25261, which significantly improves the undesired controller resets, while that reset-issue (very)rarely still appears(specially after the now working reboot-feature)

[crowston_protonmail.com]

Any volunteers for a committer?

[andrew]

In D25068#563120, @crowston_protonmail.com wrote:

Any volunteers for a committer?

I just tried building for armv7 in preperation for committing, however I got a few errors:

/usr/home/andrew/freebsd/repo/head-svn/sys/arm/broadcom/bcm2835/bcm2838_pci.c:497:17: error: implicit conversion from 'long long' to 'bus_addr_t' (aka 'unsigned long') changes value from 68719476732 to 4294967292 [-Werror,-Wconstant-conversion]
        sc->msi_addr = 0xffffffffc;
                     ~ ^~~~~~~~~~~
/usr/home/andrew/freebsd/repo/head-svn/sys/arm/broadcom/bcm2835/bcm2838_pci.c:544:56: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
        bcm_pcib_set_reg(sc, REG_MSI_ADDR_HIGH, (sc->msi_addr >> 32));
                                                              ^  ~~
/usr/home/andrew/freebsd/repo/head-svn/sys/arm/broadcom/bcm2835/bcm2838_pci.c:595:21: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
        return ((phys_base >> 0x20) & 0xff);
                           ^  ~~~~
/usr/home/andrew/freebsd/repo/head-svn/sys/arm/broadcom/bcm2835/bcm2838_pci.c:602:34: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
        return (((phys_base + size - 1) >> 0x20) & 0xff);
                                        ^  ~~~~
/usr/home/andrew/freebsd/repo/head-svn/sys/arm/broadcom/bcm2835/bcm2838_pci.c:686:60: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
        bcm_pcib_set_reg(sc, REG_BRIDGE_BUS_WINDOW_HIGH, pci_base >> 32);
                                                                  ^  ~~
5 errors generated.

[crowston_protonmail.com]

• bcm2838_pci.c: fix some types for armv7.

[crowston_protonmail.com]

My bad, I didn't test this on a 32 bit platform. I fixed some types to use the right type (pci_addr_t instead of bus_addr_t) and checked it builds on armv7 and aarch64.

In D25068#563917, @andrew wrote:

I just tried building for armv7 in preperation for committing, however I got a few errors

[crowston_protonmail.com]

• bcm2838_pci.c: fix one type.

[markmi_dsl-only.net]

This code leads to an OverDrive 1000 getting the following error.

module_register: cannot register simplebus/pcib from kernel; already loaded from kernel
Module simplebus/pcib failed to register: 17

This is via both of the following attempting a linker_file_register_modules for the
simplebus/pcib combination:

/usr/src/sys/dev/pci/pci_host_generic_fdt.c:DRIVER_MODULE(pcib, simplebus, generic_pcie_fdt_driver,
    generic_pcie_fdt_devclass, 0, 0);

/usr/src/sys/arm/broadcom/bcm2835/bcm2838_pci.c:DRIVER_MODULE(pcib, simplebus, bcm_pcib_driver, bcm_pcib_devclass, 0, 0);

This ends up with EEXIST (17) results for the 2nd in execution order. That message
is from linker_file_register_modules's code:

error = module_register(moddata, lf);
if (error) {
        printf("Module %s failed to register: %d\n",
            moddata->name, error);
        if (first_error == 0)
                first_error = error;
}

That in turn leads to linker_load_file doing linker_file_unload in:

error = linker_file_register_modules(lf);
if (error == EEXIST) {
        linker_file_unload(lf, LINKER_UNLOAD_FORCE);
        return (error);
}

Example net result consequences: no EtherNet and no USB where in
prior FreeBSD head versions both were present and working for the
OverDrive 1000.