Page MenuHomeFreeBSD

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

Authored by br on Apr 28 2015, 1:02 PM.

Details

Summary

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

Tested with ATA (Gem5 simulator)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

br updated this revision to Diff 5055.Apr 28 2015, 1:02 PM
br retitled this revision from to PCI driver for Gem5 simulator (pretends to be ECAM generic).
br updated this object.
br edited the test plan for this revision. (Show Details)
br added a reviewer: ARM.
andrew added a reviewer: jhb.Apr 28 2015, 1:15 PM
jhb added inline comments.May 1 2015, 1:04 PM
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 updated this revision to Diff 5175.May 4 2015, 2:33 PM
br marked 5 inline comments as done.May 4 2015, 2:36 PM

excellent review, thanks, I updated patch

jhb edited edge metadata.May 8 2015, 2:19 PM

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.)

br updated this revision to Diff 5322.May 11 2015, 10:27 AM
br edited edge metadata.
br marked an inline comment as done.May 11 2015, 10:28 AM
andrew added a subscriber: andrew.May 11 2015, 11:06 AM

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 added a subscriber: ian.May 11 2015, 2:32 PM
ian added inline comments.
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 added inline comments.May 12 2015, 12:18 PM
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 added a comment.May 25 2015, 12:40 PM

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

andrew added inline comments.May 25 2015, 12:54 PM
sys/dev/pci/pci-host-generic.c
566 ↗(On Diff #5322)

If not it should be fixed.

br updated this revision to Diff 5676.May 25 2015, 12:57 PM
br updated this revision to Diff 5677.May 25 2015, 1:00 PM
ian removed a subscriber: ian.May 25 2015, 5:05 PM
br updated this revision to Diff 6064.Jun 9 2015, 4:06 PM

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 updated this revision to Diff 6065.Jun 9 2015, 4:31 PM

use memmap_bus instead of fdtbus_bs_tag

br updated this revision to Diff 6066.Jun 9 2015, 4:51 PM

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

br added inline comments.Jun 9 2015, 4:52 PM
sys/dev/pci/pci-host-generic.c
567 ↗(On Diff #6066)

fixed

br updated this revision to Diff 6069.Jun 9 2015, 5:29 PM

add nexus msi-related functions and use it

br added inline comments.Jun 9 2015, 5:31 PM
sys/dev/pci/pci-host-generic.c
618 ↗(On Diff #6069)

ok fixed

br updated this revision to Diff 6134.Jun 12 2015, 12:39 PM
br marked 8 inline comments as done.

remove msi-related functions for now

This revision was automatically updated to reflect the committed changes.