OFW PCI code decuplications - involves ARM, PPC and SPARC64

[mma_semihalf.com]

Summary

Import portions of the PowerPC OF PCI implementation into new file "ofwpci.c", common for other platforms. The files ofw_pci.c and ofw_pci.h from sys/powerpc/ofw no longer exist. All required declarations are moved to sys/dev/ofw/ofwpci.h. This creates a new ofw_pci_write_ivar() function and modifies some others methods. Most functions contain existing ppc implementations in the majority unchanged. Now there is no need to have multiple identical copies of methods for various architectures.

This request was made in D2579: PCI support for Alpine platform from Annapurna Labs by @jhibbits

[andrew]

sys/powerpc/ofw/ofw_pci.c

49

Why are you adding this header?

[jhibbits]

There is a conflict in naming:

make[2]: "/home/chmeee/world/rb800/powerpc.powerpc/home/chmeee/freebsd/pristine/sys/MPC85XX/Makefile" line 5863: warning: duplicate script for target "ofw_pci.ln" ignored
make[2]: "/home/chmeee/world/rb800/powerpc.powerpc/home/chmeee/freebsd/pristine/sys/MPC85XX/Makefile" line 1769: warning: using previous script for "ofw_pci.ln" defined here
make[2]: "/home/chmeee/world/rb800/powerpc.powerpc/home/chmeee/freebsd/pristine/sys/MPC85XX/Makefile" line 5866: warning: duplicate script for target "ofw_pci.o" ignored
make[2]: "/home/chmeee/world/rb800/powerpc.powerpc/home/chmeee/freebsd/pristine/sys/MPC85XX/Makefile" line 1773: warning: using previous script for "ofw_pci.o" defined here
make[2]: "/home/chmeee/world/rb800/powerpc.powerpc/home/chmeee/freebsd/pristine/sys/MPC85XX/Makefile" line 5867: warning: duplicate script for target "ofw_pci.o" ignored
make[2]: "/home/chmeee/world/rb800/powerpc.powerpc/home/chmeee/freebsd/pristine/sys/MPC85XX/Makefile" line 1773: warning: using previous script for "ofw_pci.o" defined here

Can sys/powerpc/ofw/ofw_pci.c not just be taken nearly wholesale? If not, maybe the powerpc file should be renamed, or the new file should just be ofw_pci_subr.c. Looking at the powerpc file again, there's nothing that I can see that's powerpc-specific, so you should be able to just move the file to sys/dev/ofw as-is.

[nwhitehorn]

Why not just move the existing PowerPC code and use it on other architectures by subclassing, as is done already there? Aside from the use of bs_le_tag at line 431, which could be fixed with one line of #ifdef, the existing code is 100% machine-independent.

I'd also second the comment about machine/fdt.h. It should never be used in general, and especially not in MI code. Among many other reasons, some architectures (PowerPC) don't have it.

sys/dev/ofw/ofw_pci.h

116 ↗

(On Diff #12165)

The indentation here is really weird.

[jpa-semihalf.com]

As for the fdt header I looks it is remaining not cleaned up. Shall not appear at this stage.
Moving it as is would conflict with what we expected from its functionality, and would need to copy&paste&modify the code, just in another directory. Slightly extending the functionality gives what is needed in ARM code, not really conflicting PPC flow.

[nwhitehorn]

Could you elaborate on what is different for ARM? I see only a one-line change that would be required.

[jpa-semihalf.com]

In our code it would be convenient and elegant to have some cell attributes kept aside and use at a later stage - this is done in ofw_pci_nranges(). Moving the files to have everything shared is a good idea, but some changes are needed while the transition. It could be done in two runs (move the code, and then update for our needs) but taking the changes are slight I would rather merge them into one commit.

[nwhitehorn]

If it's just the config range that you need, it's a 5-line diff to this existing code to store that in the softc. Do you need more than that? I'd very much prefer to keep this code centralized than exploded into a dozen utility functions that are used in identical ways on four platforms.

[jpa-semihalf.com]

As previously suggested this diff includes move from ppc ofw code into sys ofw, which is a desired transition regardless on additional changes. It also includes changes in _nranges() function which is desired in our platform code. That implied some minor changes in header. Could you please comment starting from this revison?

[mmel]

sys/dev/ofw/ofw_pci.c

145 ↗

(On Diff #12356)

This part is (probably) PPC specific. On ARM, 'reg' property of PCI node contains standard address range(s) of PCI(e) core.
See: https://svnweb.freebsd.org/base/head/sys/gnu/dts/arm/tegra124.dtsi?revision=279385&view=markup#l17

403 ↗

(On Diff #12356)

And what support of prefetchable/ non-prefetchable memory and NEW_PCIB ?

489 ↗

(On Diff #12356)

imho, resource start and end must be adjusted here (rman_set_start(), rman_set_end()).

494 ↗

(On Diff #12356)

Is not BUS_ACTIVATE_RESOURCE() right method instead direct call to pmap_mapdev() and following rman_* calls?

[jpa-semihalf.com]

sys/dev/ofw/ofw_pci.c

145 ↗

(On Diff #12356)

Do you think this would be sufficient?

#ifdef __powerpc__
	if (OF_getencprop(node, "reg", (pcell_t *)&sc->sc_pcir,
	    sizeof(sc->sc_pcir)) == -1) {
		error = ENXIO;
		goto out;
	}
#endif

403 ↗

(On Diff #12356)

As for this and the other comments I would rather keep it unchanged as this is taken directly from a correct code base, and any changes should be performed on top of it. The same applies to the changes we introduced - they will be patched as a separate diff.

[nwhitehorn]

Thanks! A couple comments inline. Mostly looks good to me at this point.

sys/dev/ofw/ofw_pci.c

145 ↗

(On Diff #12356)

It was actually specific to a single PPC driver, but was harmless on others since it otherwise amounted only to a test that the node has a reg property. I've moved the code out to that driver in the PPC version in r294277.

489 ↗

(On Diff #12356)

No, that's deliberately not done. Resources are supposed to reflect child, not parent, bus ranges.

494 ↗

(On Diff #12356)

Nexus uses a different rman and bus range, so it wouldn't work here. On big-endian systems, this code also has to do something with the bus tag to make accesses LE anyway, so most of this code would still be needed.

[jpa-semihalf.com]

This diff just ports the ofw_pci stuff, taking into consideration the recent changes in PPC files (r294277). This shall be a good starting point for further modifications - I hope I omitted nothing from the discussion conclusions.

[mmel]

sys/dev/ofw/ofw_pci.c

489 ↗

(On Diff #12356)

My bad. Thanks for clarification.

494 ↗

(On Diff #12356)

I still have significant problem with this piece. By my opinion:
a) calling pmap_mapdev() directly is horrible hack.

• pmap_mapdev() is not part of KAPI, so it cannot be used in MI code.
• We cannot expect that all 3 types of different PCI resources can be mapped with same memory attributes.

b) nobody fills the resource tag (in non ppc case), so this code cannot work at all.

• on arm, and aarch64, the proper bus tag is not accessible from MI code

Simply, exact method of mapping (and unmapping) of PCI resources is platform specific. I think that we can move it to new, per platform, methods. Something like ofw_map_pci_resource( struct resource *res, vm_offset_t start);

[nwhitehorn]

sys/dev/ofw/ofw_pci.c

495 ↗

(On Diff #12463)

Yes, this part is intrinsically MD. Since there is no, for example, rman_set_physical() analog to rman_set_virtual(), there does not appear to be any way around that.

There is a slight complication that this code is not quite correct anyway because the code here assumes that the PCI bus's parent is the CPU's memory space and no further mapping by the PCI adapter's parent is required. I'm not sure how to solve that.

I believe I would prefer #ifdef here to MD functions in ofw_machdep to preserve the encapsulation. If different types of memory resource (IO ports, 64-bit areas, 32-bit areas, whatever) need different handling, we will have to continually change the API of that MD function and it will quickly become more complicated than a bit of #ifdef in a place that has more information.

[imp]

sys/dev/ofw/ofw_pci.c

495 ↗

(On Diff #12463)

how would rman_set_physical be different than rman_set_bushandle?

[nwhitehorn]

sys/dev/ofw/ofw_pci.c

495 ↗

(On Diff #12463)

bus handle isn't always a physical address (on sparc64, for example). This code doesn't work on sparc64 anyway, but I don't think that is any more portable than using pmap_mapdev(), and neither does it solve the nesting issue.

My instinct right now is to commit this code as-is and deal with issues as they become apparent. For the platforms that this supports now (PPC, ARM, MIPS) it should work fine in all cases we have now. Overengineering it to support cases we may or may not run into in the future doesn't seem like a good use of time. There should be a large block comment here that reflects this discussion, though.

[mmel]

sys/dev/ofw/ofw_pci.c

495 ↗

(On Diff #12463)

Unfortunately, i cannot agree.
As i wrote above, the code doesn't work (at least on ARM) because nobody sets resource tag.
And, at least at ARM, each active resource must have tag set....

[jpa-semihalf.com]

sys/dev/ofw/ofw_pci.c

495 ↗

(On Diff #12463)

To my knowledge no ARM uses it (this is configures by nexus by means of generic calls).

The resource tag indeed must be set in order to ensure generic correctness of this function. Please note that there is one issue that must be clarified at first - endianess, so the bs_le_tag will differ from one arch to another (bs_be_tag?). Can we come to a valid conclusion? This fix is a blocker for a couple of oncoming commits and once we took care of it we want to get it done.

[mmel]

sys/dev/ofw/ofw_pci.c

495 ↗

(On Diff #12463)

The resource tag (indirectly) is used by any of bus_space_* function.
see:

sc->sc_bst = rman_get_bustag(sc->sc_mem_res);
sc->sc_bsh = rman_get_bushandle(sc->sc_mem_res);
x = bus_space_read_4(sc->sc_bst, sc->sc_bsh, offs);

or
by bus_read/write()

Imo, we can follow the 'bus_get_dma_tag()' by implementing new bus_get_bus_tag() method.

[nwhitehorn]

sys/dev/ofw/ofw_pci.c

495 ↗

(On Diff #12463)

This will fail on PowerPC, where the PCI bus tag must *not* match the parent bus tag (which is big-endian). That's why it is set here in the first place. There should just be a short #ifdef block by architecture here that does the right thing with an #else #error clause at the end.

[mmel]

sys/dev/ofw/ofw_pci.c

495 ↗

(On Diff #12463)

Seem that we are slightly disconnected.
More descriptive:

1. in bus_subr.x duplicate bus_get_dma_tag machinery to bus_get_bus_tag()

2. In this file:

#ifdef __powerpc__
DEVMETHOD(bus_get_bus_tag,        ofw_pci_bus_get_bus_tag),
#endif


static int
ofw_pci_activate_resource(device_t bus, device_t child, int type, int rid,
    struct resource *res)
{
	 bus_space_handle_t handle;
	 bus_space_tag_t tag;

	 int rv;
...
	if (bootverbose)
		printf("ofw_pci mapdev: start %zx, len %ld\n", start,
		    rman_get_size(res));

 	tag = BUS_GET_BUS_TAG(child, child);
	if (tag == NULL)
		return (ENOMEM);
	rman_set_bustag(res, tag);
	rv = bus_space_map(rman_set_bustag(res), start, rman_get_size(res), 0,
	    &handle);
	if (rv != 0)
		return (ENOMEM);
	rman_set_bushandle(res, handle);
	rman_set_virtual(res, handle); /* XXX  for powerpc only ? */
	return (rman_activate_resource(res));
}

#ifdef __powerpc__
static bus_tag_t 
ofw_pci_bus_get_bus_tag(device_t bus, device_t child)
{

	return (&bs_le_tag)
}
#endif

4. in all platforms nexus.c, implement BUS_GET_BUS_TAG method that return per platform specific default tag.

[jpa-semihalf.com]

Add implementation of new bus_get_bus_tag(). This is required for archs where child bus tag differs from parent's one (another endianess).
There are added methods in some platforms nexus.c files which return platform specific default tag and ofw_pci_bus_get_bus_tag() in dev/ofw/ofw_pci.c for PowerPC which return correct tag.

[jpa-semihalf.com]

This comment was removed by jpa-semihalf.com.

[jpa-semihalf.com]

Anyone would like to share a comment on this? There are a few pending commits blocked by this one...

[jhibbits]

Looks fine to me. Didn't compile or test it, though. Hoping you did that (tinderbox build at least).

[mmel]

Also, please, split this patch to two commits, one for bus_get_bus_tag and other for ofw_pci.

sys/arm/arm/nexus.c

272 ↗

(On Diff #12954)

return ((bus_space_tag_t)0);

The '1' returned from bus_activate_resource() isn't probably correct, but this can be fixed later.

[zbb]

This patch breaks both PowerPC and Sparc64.
Needs to be revised.

[zbb]

Fix PowerPC and Sparc64

[mma_semihalf.com]

Fix failures for sparc64 and powerpc architectures. OF PCI implementation from sparc64/ofw is now extracted. Anything that was machine-specific was moved to machine-specific file -
sparc64_ofw_pci.c (renamed from ofw_pci.c). The code which was identical or largely common went into general dev/ofw_pci.c file. Structure ofw_pci_softc is now the same for every architecture.

[marius]

sys/conf/files.sparc64

50 ↗

(On Diff #13933)

You do a lot of gratuitous file and function renaming in the sparc64 MD code so it's hard to tell, but as far as I can determine from this diff, apart from - all very small if not tiny - ofw_pci_alloc_resource(), ofw_pci_get_{dma_tag,node}() and ofw_pci_read_ivar(), there is no advantage for sparc64 in including dev/ofw/ofw_pci.c. But at the same time, that would add quite a few unused and non-applicable functions to the sparc64 kernel. So please, just leave things as they are for sparc64. That also saves me a lot of time to test whether all 6 supported host-PCI-bridges with all their machine-specific quirks still work as they did before.

sys/dev/ofw/ofw_pci.h

92 ↗

(On Diff #13933)

So far, dev/ofw/ofw_pci.h only contains macros and structures based on the IEEE 1275 PCI binding specification (actually, just see the header of this file), but no FreeBSD- or machine-specific bits or prototypes. Please keep dev/ofw/ofw_pci.h to the existing scheme. There's a relevant precedence here, which is dev/ofw/ofw_bus.h vs. dev/ofw/ofwbus.c. So please, add all of this to a new file dev/ofw/ofwpci.h and rename dev/ofw/ofw_pci.c to dev/ofw/ofwpci.c .

103 ↗

(On Diff #13933)

Actually, device_type being "pci" for bus nodes is part of the IEEE 1275 PCI binding specification so that can stay. But "pciex" may or may not be sparc64-specific.

105 ↗

(On Diff #13933)

I believe all these MSI related bits are specific to the host-PCIe-bridges found in sun4u and sun4v machines so should not be part of any MI file.

211 ↗

(On Diff #13933)

These sparc64_ofw_pci_*_resource() and all of the ofw_pci_*_common() stuff below is sparc64 MD. So even if it were a good idea to use dev/ofw/ofwpci.c on sparc64, these prototypes shouldn't be in any MI location.

[jan_semihalf.com]

sys/dev/ofw/ofw_pci.h

92 ↗

(On Diff #13933)

This value is based on IEEE 1275 PCI binding specification (rev 2.1 from August 29, 1998) - in 2.1 Address Spaces it's defined 4 types of spaces (2.1.1-2.1.4).
Is a comment where this value comes from enough or do we really need to move number of PCI spaces to different file?

[mma_semihalf.com]

Undochanges for sparc64, now things for sparc are as before.
Keep dev/ofw/ofw_pci.h to the existed scheme.
Move all of required declarations from dev/ofw/ofw_pci.h to new file dev/ofw/ofwpci.h.
Rename dev/ofw/ofw_pci.c file to dev/ofw/ofwpci.c. For now this file is not including to the sparc64 kernel, it uses only sparc64-specific ofw files.

This request was made by @marius

[mma_semihalf.com]

Someone else would like to share a comment on this?
@marius, is it correct now in your view?

[jhibbits]

sys/dev/ofw/ofwpci.c

69

This needs to be rman_res_t, not u_long, for all the resources.

77

Same here.

[jhibbits]

I think something got messed up with this patch. I see the following fatal trap on my RouterBoard (PowerPC, mpc85xx):

pcib1: <Freescale Integrated PCI/PCI-E Controller> mem 0xe0009000-0xe0009fff irq 25 on ofwbus0
nexus mapdev: start e0009000, len 4096
PCI: reading firmware bus numbers for secbus = 1 (bus/sec/sub) = (0/0/0)
PCI: translate firmware bus numbers for secbus 1 (0/0/0) -> (0/1/1)
pci1: <OFW PCI bus> on pcib1
pci1: domain=1, physical bus=0
found-> vendor=0x1957, dev=0x0033, revid=0x11

domain=1, bus=0, slot=0, func=0
class=0b-20-00, hdrtype=0x01, mfdev=0
cmdreg=0x0104, statreg=0x0010, cachelnsz=0 (dwords)
lattimer=0x00 (0 ns), mingnt=0x00 (0 ns), maxlat=0x00 (0 ns)
powerspec 2  supports D0 D1 D2 D3  current D0
map[10]: type Memory, range 32, base 0, size 20, memory disabled

pcib2 at device 0.0 on pci1

fatal kernel trap:

exception       = 0x300 (data storage interrupt)
virtual address = 0x64657668
esr             = 0x0
srr0            = 0xc034a068
srr1            = 0x21000
lr              = 0xc02b0130
curthread       = 0xc070e1b0
       pid = 0, comm = swapper

[ thread pid 0 tid 100000 ]
Stopped at strlen+0x20: lwz r11, r8, 0x0
db> bt
Tracing pid 0 tid 100000 td 0xc070e1b0
0xc5008310: at kvprintf+0x9c0
0xc50083d0: at vsnprintf+0x44
0xc50083f0: at kassert_panic+0x84
0xc5008440: at mtx_lock_flags+0xdc
0xc5008460: at rman_reserve_resource_bound+0x128
0xc5008500: at rman_reserve_resource+0x34
0xc5008520: at ofw_pci_write_ivar+0x64c
0xc5008570: at bus_generic_alloc_resource+0xe0
0xc50085b0: at pci_alloc_multi_resource+0x15c
0xc5008630: at pci_alloc_resource+0x134
0xc5008670: at bus_alloc_resource+0xe0
0xc50086b0: at pcib_alloc_resource+0x774
0xc5008740: at pcib_attach_common+0xba4
0xc5008790: at pcib_attach+0x30
0xc50087b0: at device_attach+0x3d8
0xc5008800: at device_probe_and_attach+0x68
0xc5008820: at bus_generic_attach+0x2c
0xc5008840: at ofw_mem_regions+0xb50
0xc50088b0: at device_attach+0x3d8
0xc5008900: at device_probe_and_attach+0x68
0xc5008920: at bus_generic_attach+0x2c
0xc5008940: at ofw_pci_attach+0x64
0xc5008960: at mpc85xx_attach+0x1950
0xc50089b0: at device_attach+0x3d8
0xc5008a00: at device_probe_and_attach+0x68
0xc5008a20: at bus_generic_new_pass+0x120
0xc5008a50: at bus_generic_new_pass+0x10c
0xc5008a80: at bus_generic_new_pass+0x10c
0xc5008ab0: at bus_set_pass+0xd0
0xc5008ae0: at root_bus_configure+0x1c
0xc5008af0: at enable_vec+0x230
0xc5008b00: at mi_startup+0x178
0xc5008b50: at start+0x1b4

It looks as though it's using a string itself as an address. I haven't looked any further than this yet.

[mma_semihalf.com]

Add few corrections and fix issue for PowerPC

[jhibbits]

This one builds and runs fine on my RB800.