Date: Thu, 24 Oct 2013 11:49:33 -0500 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Marius Strobl <marius@alchemy.franken.de> Cc: freebsd-sparc64@freebsd.org Subject: Re: Nexus improvements Message-ID: <52694F9D.6000705@freebsd.org> In-Reply-To: <20131024155130.GA14293@alchemy.franken.de> References: <5268332A.6080107@freebsd.org> <20131024155130.GA14293@alchemy.franken.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On 10/24/13 10:51, Marius Strobl wrote: > On Wed, Oct 23, 2013 at 03:35:54PM -0500, Nathan Whitehorn wrote: >> I've been trying for the last few days to clean up some of our OF/FDT >> code, in particular related to reducing code duplication. There is a >> 12-year-old comment in sparc64/sparc64/nexus.c that "this code should >> get into dev/ofw to some extent". Most of the code has been moved now >> and the PowerPC nexus device has been replaced by the pieces of the >> sparc64 one that now do live in dev/ofw. The following (almost entirely >> negative) patch moves sparc64 to use it as well. Since it is just a copy >> of the sparc64 code, this should involve no functional changes, and I >> will commit it in a few days unless someone raises an objection. The >> patch is only compile tested for the time being, so hardware tests would >> be appreciated. >> >> Patch at: http://people.freebsd.org/~nwhitehorn/sparc64-nexus-unify.diff > Well, as is this patch breaks resource allocation in a strange way: > nexus0: <Open Firmware Nexus device> > pcib0: <Sun Host-PCIe bridge> mem 0x4000f600000-0x4000f6effff,0x4000f410000-0x4000f473fff irq 1983,1982 on nexus0 > panic: fire_attach: could not allocate register bank 0 Odd. The resource allocation code is an exact copy of the one from sparc64/nexus.c. I'll power up my SPARC machine and try to debug. > I didn't try to figure out what's going on as I'm still not very fond > of sharing that code across platforms. The comment you are referring to > is - as you are also pointing out - rather old and applied to a sparc64 > nexus(4) that was quite different than the current version and f. e. > didn't do resource management of its children based on OFW information. > In particular, "true" MI code shouldn't have #ifdefs with very MD code > like the one your patch would introduce. Similarly, the device exclude > lists shouldn't be shared across all platforms as one might want to > exclude a specific device whilst another does not. Having the whole > lists in unconditionally also just wastes memory on embedded machinery. There are ways around that. The main point here is to eliminate the duplication with dev/fdt, which has a similar list, for example, as well as reinventing half of the code in /sys/dev/ofw -- and that in buggy and incomplete ways. So in principle, I agree with you that other things could be wanted, but in practice they do not seem to be. The patch in general very deliberately keeps all resource allocation inside the MD code (you will note that the nexus device in sparc64 maintains all of that). The only exception has to do with interrupt numbering. SPARC doesn't seem to follow the Open Firmware interrupt-mapping standard as far as I can tell, which means that we can't use the usual platform-independent ofw_bus_map_irq(interrupt-parent, irq) call there to construct the interrupt vector ID. A few-line #ifdef for this single corner case did not seem so bad. Aside from resource allocation (which is maintained by the patch in MD code) and this nit with IRQs, the PowerPC nexus is at present a line-for-line copy of the sparc64 one. It seems a shame to maintain that duplication. > Also, switching sparc64 to use the current dev/ofw/ofw_nexus.c would > introduce some subtle differences compared to how things currently > work with the MD nexus(4). At a quick glance that would be: > o the interrupts RMAN range gets extended from IV_MAX - 1 to ~0, > which means no longer sanity checking for out-of-range interrupts, This could be restored, though I can't imagine it adds much. > o devices without at least memory resources get added as children, > while previously they were skipped. Does it harm anything to add these? I'll try to fix the bugs in any case, and then we can revisit the patch. -Nathan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?52694F9D.6000705>