Date: Sat, 18 Nov 2000 22:39:35 -0700 From: Warner Losh <imp@village.org> To: "Justin T. Gibbs" <gibbs@scsiguy.com> Cc: jon@FreeBSD.org, current@FreeBSD.org Subject: Re: Cardbus fixes Message-ID: <200011190539.WAA87209@harmony.village.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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200011190539.WAA87209>