PCI driver for Gem5 simulator (pretends to be ECAM generic)

[br]

Summary

based on thunder_pcie. Thunder-specific logic stripped, IRQ and legacy IOPORT resources allocation added

Tested with ATA (Gem5 simulator)

[jhb]

sys/dev/pci/pci-host-generic.c

444 ↗

(On Diff #5055)

This doesn't seem right. The alloc_resource routine uses a local rman for memory, I/O, and IRQs. You should be using rman_release_resource() for all all of those. Ideally you would have a helper routine:

static struct rman *
generic_pcie_rman(int type)
{
   switch (type) {
       case SYS_RES_IOPORT:
           return (&sc->io_rman);
   ....
}

And then in release_resource do something like:

struct rman *rm;

rm = generic_pcie_rman(type);
if (rm != NULL) {
    KASSERT(rman_is_region_manager(res, rm), ("rman mismatch"));
    rman_release_resource(res);
}
return (bus_generic_release_resource(dev, child, type, rid, res));

(I prefer using bus_generic_* over BUS_* directly)

You can then use the rman lookup routine in alloc_resource as well.

487 ↗

(On Diff #5055)

Please only set bustag/bushandle in activate_resource(). It needs to work that way for NEW_PCIB. Also, I would add an adjust_resource method. It can be fairly short:

adjust_resource(...)
{
   rm = generic_pcie_rman(rman_get_type(res));
   if (rm != NULL)
       return (rman_adjust_resource(res, ...));
   return (bus_generic_adjust_resource(...));
}

547 ↗

(On Diff #5055)

If you want a parent device to set the bus tag/handle for memory, then this probably warrants
a comment. Most of the arm upper layers don't work properly for that either since they try to only set bus tag/handle in alloc and not activate. If you don't want to pass this up, you should be using rman_activate_resource() directly instead.

Regardless I think you to assert that resources all use a local rman (similar to the KASSERT I suggested for release). You should also be using the rman methods to activate/deactivate on SYS_RES_IRQ.

Also, I don't see a deactivate resource method. It should undo the effects of activate (deactivate in rman, etc.).

Also, in general in activate routines you should to the rman activate first since that checks for !RF_SHAREABLE violations, etc. and only do MD work like pmap_mapdev if rman_activate_resource succeeds.

561 ↗

(On Diff #5055)

I would just use bus_generic_setup_intr() and bus_generic_teardown_intr() directly in the DEVMETHODS table and remove these.

586 ↗

(On Diff #5055)

This doesn't appear to actually do anything?

[br]

excellent review, thanks, I updated patch

[jhb]

Just one more suggestion, but this looks really good, thanks!

sys/dev/pci/pci-host-generic.c

604 ↗

(On Diff #5175)

My only suggestion here would be to do rman_deactivate_resource() first. It will fail if the resource isn't actually active so you won't risk calling pmap_unmapdev() on an invalid value (i.e. if someone allocates a resource without RF_ACTIVE and then calls bus_deactivate_resource() directly. This is a bug in their driver certainly, but it is good to handle that regardless.)

[andrew]

In general we are trying to stop use the fdt_* functions.

The places you are calling an arm_* function it's most likely someone didn't understand how it should have been implemented and should be fixed.

sys/arm64/include/fdt.h

1 ↗

(On Diff #5322)

The file we just can't kill, fdtbus_bs_tag should only be used in early devices, i.e. the uart, and only when you know you are booting with FDT enabled.

sys/dev/pci/pci-host-generic.c

254 ↗

(On Diff #5322)

Don't use fdt_* functions, just use OF_getencprop here and fail if the address or size is missing.

257 ↗

(On Diff #5322)

Use OF_getencprop(OF_parent(node) for this

566 ↗

(On Diff #5322)

This doesn't look correct, shouldn't it call into the parent for this?

617 ↗

(On Diff #5322)

This should be calling into the parent for this. The nexus driver should be fixed to handle this method.

626 ↗

(On Diff #5322)

And here, and below.

663 ↗

(On Diff #5322)

Could you group these, e.g. device, then bus, then pcib.

[• ian]

sys/arm64/include/fdt.h

1 ↗

(On Diff #5322)

I think part of the problem here is the name. This really has nothing to do with fdt in any way, it's just the arm global root tag that all other bus_space tags inherit from. We've always had one, but in the pre-fdt days each soc had its own and they had varying names. They were all identical except on a couple strange xscale variants that had different high/low addr constraints.

[jhb]

sys/dev/pci/pci-host-generic.c

566 ↗

(On Diff #5322)

Quite possibly (I'll defer to you on this one). You will need to ensure the parent has a proper activate_resource method. (Most of sys/arm does not, but I haven't looked at arm64's nexus.)

[jhb]

Modulo Andrew's remaining comments, I think this looks fine from my end.

[andrew]

sys/dev/pci/pci-host-generic.c

566 ↗

(On Diff #5322)

If not it should be fixed.

[br]

eliminate all the fdt_* calls

sys/arm64/include/fdt.h

2 ↗

(On Diff #6064)

so how to deal with it ? it is the last thing I have to fix

[br]

use memmap_bus instead of fdtbus_bs_tag

[br]

call to bus for resource activation, so no need to use memmap_bus

[br]

sys/dev/pci/pci-host-generic.c

567 ↗

(On Diff #6066)

fixed

[br]

add nexus msi-related functions and use it

[br]

sys/dev/pci/pci-host-generic.c

618 ↗

(On Diff #6069)

ok fixed

[br]

remove msi-related functions for now