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.
Details
Details
- Reviewers
kevans andrew - Commits
- rS362954: Add a driver for bcm2838 PCI express controller
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.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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 ↗ | (On Diff #72445) | For a generic_pcie_fdt_driver derived driver you should use generic_pcie_fdt_softc as the base softc. |
| 230–231 ↗ | (On Diff #72445) | Multi line comments should be in the form /* * The controller ... * ... */ |
| 263–265 ↗ | (On Diff #72445) | Is there a reason we caan't use the fdt aalloc resource function? |
| 349 ↗ | (On Diff #72445) | It looks like non-IRQ types could be handled by generic_pcie_activate_resource |
| 429–432 ↗ | (On Diff #72445) | 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 ↗ | (On Diff #72445) | What is this magic number? |
| 771 ↗ | (On Diff #72445) | What are these magic numbers? |
| sys/dev/pci/pci_host_generic_fdt.c | ||
| 123–124 ↗ | (On Diff #72445) | 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. |
| sys/arm/broadcom/bcm2835/bcm2838_pci.c | ||
|---|---|---|
| 263–265 ↗ | (On Diff #72445) | 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 ↗ | (On Diff #72445) | 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 ↗ | (On Diff #72445) | 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. |
| sys/arm/broadcom/bcm2835/bcm2838_pci.c | ||
|---|---|---|
| 263–265 ↗ | (On Diff #72445) | 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 ↗ | (On Diff #72445) | 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 ↗ | (On Diff #72445) | 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. |
Comment Actions
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.
Comment Actions
I've created D25121 to translate addresses in the base pci host generic driver. It also cleans up other memory management while there.
Comment Actions
pci_host_generic should work for any PCI controller that uses a memory mapped interface.
Comment Actions
@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?
Comment Actions
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.
Comment Actions
... 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)
Comment Actions
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.Comment Actions
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.
Comment Actions
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.