From owner-freebsd-current Sat Nov 18 21:39:43 2000 Delivered-To: freebsd-current@freebsd.org Received: from rover.village.org (rover.village.org [204.144.255.66]) by hub.freebsd.org (Postfix) with ESMTP id 2FD3537B4C5; Sat, 18 Nov 2000 21:39:37 -0800 (PST) Received: from harmony.village.org (harmony.village.org [10.0.0.6]) by rover.village.org (8.11.0/8.11.0) with ESMTP id eAJ5daQ16743; Sat, 18 Nov 2000 22:39:36 -0700 (MST) (envelope-from imp@harmony.village.org) Received: from harmony.village.org (localhost.village.org [127.0.0.1]) by harmony.village.org (8.9.3/8.8.3) with ESMTP id WAA87209; Sat, 18 Nov 2000 22:39:35 -0700 (MST) Message-Id: <200011190539.WAA87209@harmony.village.org> To: "Justin T. Gibbs" Subject: Re: Cardbus fixes Cc: jon@FreeBSD.org, current@FreeBSD.org In-reply-to: Your message of "Sat, 18 Nov 2000 22:05:01 MST." <200011190505.eAJ552428239@aslan.scsiguy.com> References: <200011190505.eAJ552428239@aslan.scsiguy.com> Date: Sat, 18 Nov 2000 22:39:35 -0700 From: Warner Losh Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG In message <200011190505.eAJ552428239@aslan.scsiguy.com> "Justin T. Gibbs" writes: : 1) When mucking with mapping registers, it is best to *not* have : io or memory space access enabled. This patch defers the setting : of these bits until after all of the mapping registers are probed. : It might be even better to defer this until a particular mapping : is activated and to disable that type of access when a new : register is activated. Agreed. : 2) The PCI spec is very explicit about how mapping registers and : the expansion ROM mapping register should be probed. This patch : makes cardbus_add_map() follow the spec. OK. : 3) The PCI spec allows a device to use the same address decoder for : expansion ROM access as is used for memory mapped register access. : This patch carefully enables and disables ROM access along with : resource (de)activiation. Makes sense. : 4) The cardbus CIS code treats the CIS_PTR as a mapping register if : it is mentioned in the CIS. I don't have a spec handy to understand : why the CIS_PTR is mentioned in the CIS, but allocating a memory range : for it is certainly bogus. My patch ignores bar #6 to prevent the : mapping. I'll have to look up the CIS_PTR spec. I'm not sure I like hardwiring things like this. : 5) The CIS code allocated duplicate resources to those already found : by cardbus_add_resources(). The fix is to pass in the bar computed : from the CIS instead of the particular resource ID for that bar, : so bus_generic_alloc_resource succeeds in finding the old resource. : It seems somewhat strange that we have to have two methods for : finding and activating the mapping registers. Isn't one method : sufficient? I'd think so, but I'm not sure. : 6) cardbus_read_exrom_cis() failed to advance correctly to higer rom : images. To effect the fix, the cis_ptr value must be provided to : the different CIS reading methods, unaltered. OK. : 7) The CIS code seems to use the wrong bit to determine rather a particular : register mapping is for I/O or memory space. From looking at the : two cards I have, it seems TPL_BAR_REG_AS should be 0x10 instead : of 0x08. Otherwise, all registers that should be I/O mapped gain : a second mapping in memory space. I'll have look this one up as well. : 8) The cardbus bridge code leaves memory space prefetching enabled. : Prefetching is only allowed if the target device indicates (through : its mapping register) that prefetching is allowable. My patch : simply disables prefetching and includes code to detect this capability : and pass an RF flag to enable it, but nothing more. OK. : 9) The pccbb code was impoperly handling the I/O and mem range limit : registers. The limit register indicates the highest valid address : in the window, not the first invalid address outside the window. OK. : One last thing that is started here is an attempt to rely more heavily : on PCI register definitions and eventually functions, to get things : done. The cardbus code duplicates a lot of functionality that is : already available in the pci code (mapping register size/type detection). I'd love to see that as well. There's a lot of duplicated code that I'd like to see factored. This is especially true with the 16bit code. : One other thing that struck me while I was looking at this was that : the resource manager should be providing the "resource pooling" : that pccbb_cardbus_auto_open() emulates. Although the cardbus : bridges we support only provide 4K granularity for memory mapped : windows, things like external rom access often can be mapped on : 2K boundaries. This could allow the resource manager to allocate : a range that doesn't appear to overlap with another allocation but : does due to the bridges constraints. I might look into adding the : concept of hierarchical resource pools to the resource manager so : that, for example, the cardbus bridges pool will always grow in : 4K increments from its parent resource pool. The parent would then : grow according to its own requirements, etc. That might be interesting. Now, I'm off to try these patches... Warner To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message