Subject: Re: PCI_MAPREG_TYPE_ROM... or the lack of it
To: Garrett D'Amore <garrett_damore@tadpole.com>
From: Garrett D'Amore <garrett_damore@tadpole.com>
List: tech-kern
Date: 02/26/2006 21:59:22
This is a multi-part message in MIME format.
--------------020205010900080109040308
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

Forgot to attach the diffs.  Sorry about that.

Garrett D'Amore wrote:
> Garrett D'Amore wrote:
>   
>> Allen Briggs wrote:
>>   
>>     
>>> On Fri, Feb 24, 2006 at 07:43:01PM -0800, Garrett D'Amore wrote:
>>>   
>>>     
>>>       
>>>> Actually, it occurs to me that there is an easier way:
>>>>
>>>> PCI_MAPREG_TYPE_ROM could be defined the same as PCI_MAPREG_TYPE_MEM,
>>>> and the PCI register mapping code could look at the offset of the register.
>>>>
>>>> PCI_MAPREG_ROM is *always* 0x30.  That's probably a better way to do it.
>>>>     
>>>>       
>>>>         
>>> I agree.  I think it should be noted in the man page that
>>> PCI_MAPREG_TYPE_ROM is only valid for PCI_MAPREG_ROM.
>>>
>>> -allen
>>>
>>>   
>>>     
>>>       
>> Okay, I'm going to take that as the "official" word then.  Expect diffs
>> early next week. :-)
>>
>>     - Garrett
>>
>>   
>>     
> My diffs are attached, please review.  These diffs add support for
> PCI_MAPREG_TYPE_ROM as an argument to pci_mapreg_map().
>
> A consequence of these changes is that now pci_configure_bus() properly
> setups up the PCI expansion ROM BAR at 0x30, whereas before it still
> wasn't getting it right (due to an incorrect check against
> PCI_CONF_ENABLE_ROM that should only have been PCI_CONF_MAP_ROM.
>
> This means that on *most* systems that use PCI_NETBSD_CONFIGURE, you
> will now see Expansion ROMs get allocated address space *and* have that
> address written to the ROM BAR, but it will *not* be enabled.
>
> pci_mapreg_map() will *enable* the expansion rom address decoder if not
> already done.  As this can have detrimental effects for some devices, it
> should only be done in target device drivers that really need this
> access to ROMs.  Note that there is *no* standard API for *disabling* an
> enabled ROM address decoder.  If you need to do that, just turn off the
> enable bit in the ROM address decoder.  (This will not normally be a
> problem, since those devices with drivers that need an expansion rom
> usually will have a separate address decoder for the expansion rom.)
>
> If none of this makes any sense, rest assured that your devices/drivers
> will continue to operate as before.
>
> These diffs do not include the necessary changes to the man pages
> (primarily to indicate that PCI_MAPREG_TYPE_ROM is only allowed when reg
> is PCI_MAPREG_ROM), but I'll do that as well, assuming that the
> technical content of these diffs is acceptable.
>
>     -- Garrett
>
>   


-- 
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecomputer.com/
Phone: 951 325-2134  Fax: 951 325-2191


--------------020205010900080109040308
Content-Type: text/plain;
 name="diff.exrom"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="diff.exrom"

Index: pci_map.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/pci_map.c,v
retrieving revision 1.14
diff -u -r1.14 pci_map.c
--- pci_map.c	11 Dec 2005 12:22:50 -0000	1.14
+++ pci_map.c	27 Feb 2006 05:05:07 -0000
@@ -111,20 +111,21 @@
 {
 	pcireg_t address, mask, address1 = 0, mask1 = 0xffffffff;
 	u_int64_t waddress, wmask;
-	int s, is64bit;
+	int s, is64bit, isrom;
 
 	is64bit = (PCI_MAPREG_MEM_TYPE(type) == PCI_MAPREG_MEM_TYPE_64BIT);
+	isrom = reg == PCI_MAPREG_ROM;
 
-	if (reg < PCI_MAPREG_START ||
+	if ((!isrom) && (reg < PCI_MAPREG_START ||
 #if 0
 	    /*
 	     * Can't do this check; some devices have mapping registers
 	     * way out in left field.
 	     */
-	    reg >= PCI_MAPREG_END ||
+		reg >= PCI_MAPREG_END ||
 #endif
-	    (reg & 3))
-		panic("pci_mem_find: bad request");
+		(reg & 3)))
+	    panic("pci_mem_find: bad request");
 
 	if (is64bit && (reg + 4) >= PCI_MAPREG_END)
 		panic("pci_mem_find: bad 64-bit request");
@@ -152,15 +153,24 @@
 	}
 	splx(s);
 
-	if (PCI_MAPREG_TYPE(address) != PCI_MAPREG_TYPE_MEM) {
-		printf("pci_mem_find: expected type mem, found i/o\n");
-		return (1);
-	}
-	if (PCI_MAPREG_MEM_TYPE(address) != PCI_MAPREG_MEM_TYPE(type)) {
-		printf("pci_mem_find: expected mem type %08x, found %08x\n",
-		    PCI_MAPREG_MEM_TYPE(type),
-		    PCI_MAPREG_MEM_TYPE(address));
-		return (1);
+	if (!isrom) {
+		/*
+		 * roms should have an enable bit instead of a memory
+		 * type decoder bit.  For normal BARs, make sure that
+		 * the address decoder type matches what we asked for.
+		 */
+		if (PCI_MAPREG_TYPE(address) != PCI_MAPREG_TYPE_MEM) {
+			printf("pci_mem_find: expected type mem, found i/o\n");
+			return (1);
+		}
+		if (PCI_MAPREG_MEM_TYPE(address) !=
+		    PCI_MAPREG_MEM_TYPE(type)) {
+			printf("pci_mem_find: "
+			    "expected mem type %08x, found %08x\n",
+			    PCI_MAPREG_MEM_TYPE(type),
+			    PCI_MAPREG_MEM_TYPE(address));
+			return (1);
+		}
 	}
 
 	waddress = (u_int64_t)address1 << 32UL | address;
@@ -208,7 +218,7 @@
 			*sizep = PCI_MAPREG_MEM64_SIZE(wmask);
 	}
 	if (flagsp != 0)
-		*flagsp = PCI_MAPREG_MEM_PREFETCHABLE(address) ?
+		*flagsp = (isrom || PCI_MAPREG_MEM_PREFETCHABLE(address)) ?
 		    BUS_SPACE_MAP_PREFETCHABLE : 0;
 
 	return (0);
@@ -287,6 +297,17 @@
 		tag = pa->pa_memt;
 	}
 
+	if (reg == PCI_MAPREG_ROM) {
+		pcireg_t 	mask;
+		int		s;
+		/* we have to enable the ROM address decoder... */
+		s = splhigh();
+		mask = pci_conf_read(pa->pa_pc, pa->pa_tag, reg);
+		mask |= PCI_MAPREG_ROM_ENABLE;
+		pci_conf_write(pa->pa_pc, pa->pa_tag, reg, mask);
+		splx(s);
+	}
+
 	if (bus_space_map(tag, base, size, busflags | flags, &handle))
 		return (1);
 
Index: pciconf.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/pciconf.c,v
retrieving revision 1.28
diff -u -r1.28 pciconf.c
--- pciconf.c	10 Feb 2006 20:52:57 -0000	1.28
+++ pciconf.c	27 Feb 2006 05:05:07 -0000
@@ -808,7 +808,7 @@
 	for (pm=pb->pcimemwin; pm < &pb->pcimemwin[pb->nmemwin] ; pm++) {
 		if (pm->reg == PCI_MAPREG_ROM && pm->address != -1) {
 			pd = pm->dev;
-			if (!(pd->enable & PCI_CONF_ENABLE_ROM))
+			if (!(pd->enable & PCI_CONF_MAP_ROM))
 				continue;
 			if (pci_conf_debug) {
 				print_tag(pd->pc, pd->tag);
@@ -817,7 +817,10 @@
 				    PRIx64 " (reg %x)\n", pm->size,
 				    pm->address, pm->reg);
 			}
-			base = (pcireg_t) (pm->address | PCI_MAPREG_ROM_ENABLE);
+			base = (pcireg_t) pm->address;
+			if (pd->enable & PCI_CONF_ENABLE_ROM)
+				base |= PCI_MAPREG_ROM_ENABLE;
+
 			pci_conf_write(pd->pc, pd->tag, pm->reg, base);
 		}
 	}
Index: pcireg.h
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/pcireg.h,v
retrieving revision 1.47
diff -u -r1.47 pcireg.h
--- pcireg.h	11 Dec 2005 12:22:50 -0000	1.47
+++ pcireg.h	27 Feb 2006 05:05:07 -0000
@@ -352,6 +352,7 @@
 #define	PCI_MAPREG_TYPE_MASK			0x00000001
 
 #define	PCI_MAPREG_TYPE_MEM			0x00000000
+#define	PCI_MAPREG_TYPE_ROM			0x00000000
 #define	PCI_MAPREG_TYPE_IO			0x00000001
 #define	PCI_MAPREG_ROM_ENABLE			0x00000001
 

--------------020205010900080109040308--