OK -- so I separated the actual port code from the external memory controller code. I think this makes more sense.

It does add some complexity to the code. I may be going through a chain of calls just to turn the CS pin on, for instance. Hopefully I'm not going too crazy with this.

Anyway, this means that I can control the ports from a SIMM electrical test routine using the same types of functions that the actual programming  controlling code would use, without having to duplicate a bunch of port definitions and bit manipulation. I made sure to add all the functions I can think of needing to the ports module. We'll see if I got them all!

.cproject

@@ -53,6 +53,10 @@
 							<tool id="de.innot.avreclipse.tool.avrdude.app.debug.403293383" name="AVRDude" superClass="de.innot.avreclipse.tool.avrdude.app.debug"/>
 						</toolChain>
 					</folderInfo>
+					<sourceEntries>
+						<entry flags="VALUE_WORKSPACE_PATH|RESOLVED" kind="sourcePath" name="tests"/>
+						<entry excluding="tests" flags="VALUE_WORKSPACE_PATH|RESOLVED" kind="sourcePath" name=""/>
+					</sourceEntries>
 				</configuration>
 			</storageModule>
 			<storageModule moduleId="scannerConfiguration">
@@ -416,6 +420,10 @@
 							<tool id="de.innot.avreclipse.tool.avrdude.app.release.1179659974" name="AVRDude" superClass="de.innot.avreclipse.tool.avrdude.app.release"/>
 						</toolChain>
 					</folderInfo>
+					<sourceEntries>
+						<entry flags="VALUE_WORKSPACE_PATH|RESOLVED" kind="sourcePath" name="tests"/>
+						<entry excluding="tests" flags="VALUE_WORKSPACE_PATH|RESOLVED" kind="sourcePath" name=""/>
+					</sourceEntries>
 				</configuration>
 			</storageModule>
 			<storageModule moduleId="scannerConfiguration">

.project

@@ -31,7 +31,7 @@
 				</dictionary>
 				<dictionary>
 					<key>org.eclipse.cdt.make.core.buildLocation</key>
-					<value>${workspace_loc:/SIMMProgrammer/Debug}</value>
+					<value>${workspace_loc:/SIMMProgrammer/Release}</value>
 				</dictionary>
 				<dictionary>
 					<key>org.eclipse.cdt.make.core.cleanBuildTarget</key>

external_mem.c

@@ -6,72 +6,55 @@
  */
 
 #include "external_mem.h"
-#include "mcp23s17.h"
+#include "ports.h"
 #include <avr/io.h>
 
-// SIMM control signals on port B
+#define NUM_ADDRESS_LINES	20
-#define SIMM_WE		(1 << 6)
-#define SIMM_OE		(1 << 5)
-#define SIMM_CS		(1 << 4)
-
-static bool ExternalMem_Inited = false;
 
+// Allow this to be initialized more than once.
+// In case we mess with the port settings,
+// re-initializing ExternalMem should reset everything
+// to sensible defaults.
 void ExternalMem_Init(void)
 {
-	// If it has already been initialized, no need to do it again.
+	// Initialize the ports connected to address/data/control lines
-	if (ExternalMem_Inited)
+	Ports_Init();
-	{
-		return;
-	}
 
-	// This module depends on the MCP23S17
+	// Disable all pull-ups, on both the address and data lines. They aren't needed
-	MCP23S17_Init();
+	// for normal operation.
+	Ports_AddressPullups_RMW(0, (1UL << (NUM_ADDRESS_LINES - 1)) - 1);
+	Ports_DataPullups_RMW(0, 0xFFFFFFFFUL);
 
-	// Configure address lines as outputs
+	// Configure all address lines as outputs
-	DDRA = 0xFF; // A0-A7
+	Ports_SetAddressDDR((1UL << (NUM_ADDRESS_LINES - 1)) - 1);
-	DDRC = 0xFF; // A8-A15
-	DDRD |= 0x73; // A16-A20
 
-	// Sensible defaults for address and data lines
+	// Sensible defaults for address and data lines:
-	ExternalMem_SetAddress(0);
+	// Write out address zero
-	ExternalMem_SetDataAsInput();
+	Ports_SetAddressOut(0);
 
-	// Control lines
+	// Set all data lines as inputs (with no pullups! we turned them off)
-	DDRB |= SIMM_WE | SIMM_OE | SIMM_CS;
+	Ports_SetDataDDR(0);
 
-	// Default all the control lines to high (de-asserted)
+	// Control lines (I'm cheating and manipulating the bits directly here)
-	PORTB |= SIMM_WE | SIMM_OE | SIMM_CS;
+	Ports_SetCSDDR(1);
+	Ports_SetOEDDR(1);
+	Ports_SetWEDDR(1);
 
-	// All done!
+	// Default all control lines to high (de-asserted)
-	ExternalMem_Inited = true;
+	ExternalMem_DeassertCS();
+	ExternalMem_DeassertOE();
+	ExternalMem_DeassertWE();
 }
 
 void ExternalMem_SetAddress(uint32_t address)
 {
-	PORTA = (address & 0xFF); // A0-A7
+	Ports_SetAddressOut(address);
-	PORTC = ((address >> 8) & 0xFF); // A8-A15
-
-	// A16-A20 are special because they are split up...(We use PORTD pins 0, 1, 4, 5, 6)
-	uint8_t tmp = (address >> 16) & 0xFF;
-	tmp = (tmp & 0x03) | ((tmp & 0x1C) << 2);
-
-	// Now, turn off the pins we have to turn off, and turn on the pins we have to turn on
-	// (without affecting other pins [2, 3, and 7] that we aren't supposed to touch)
-	PORTD &= (0x8C | tmp); // This should turn off all '0' bits in tmp.
-	PORTD |= tmp; // This should turn on all '1' bits in tmp
 }
 
 void ExternalMem_SetData(uint32_t data)
 {
-	// Set as outputs
+	Ports_SetDataDDR(0xFFFFFFFFUL);
-	MCP23S17_SetDDR(0xFFFF); // D0-D15
+	Ports_SetDataOut(data);
-	DDRE = 0xFF; // D16-D23
-	DDRF = 0xFF; // D24-D31
-
-	// Set the actual outputted values
-	MCP23S17_SetPins(data & 0xFFFF); // D0-D15
-	PORTE = ((data >> 16) & 0xFF); // D16-D23
-	PORTF = ((data >> 24) & 0xFF); // D24-D31
 }
 
 void ExternalMem_SetAddressAndData(uint32_t address, uint32_t data)
@@ -82,44 +65,40 @@ void ExternalMem_SetAddressAndData(uint32_t address, uint32_t data)
 
 void ExternalMem_SetDataAsInput(void)
 {
-	MCP23S17_SetDDR(0x0000); // D0-D15
+	Ports_SetDataDDR(0);
-	DDRE = 0; // D16-D23
-	DDRF = 0; // D24-D31
 }
 
 uint32_t ExternalMem_ReadData(void)
 {
-	return ((uint32_t)MCP23S17_ReadPins()) |
+	return Ports_ReadData();
-			(((uint32_t)PORTE) << 16) |
-			(((uint32_t)PORTF) << 24);
 }
 
 void ExternalMem_AssertCS(void)
 {
-	PORTB &= ~SIMM_CS;
+	Ports_SetCSOut(0);
 }
 
 void ExternalMem_DeassertCS(void)
 {
-	PORTB |= SIMM_CS;
+	Ports_SetCSOut(1);
 }
 
 void ExternalMem_AssertWE(void)
 {
-	PORTB &= ~SIMM_WE;
+	Ports_SetWEOut(0);
 }
 
 void ExternalMem_DeassertWE(void)
 {
-	PORTB |= SIMM_WE;
+	Ports_SetWEOut(1);
 }
 
 void ExternalMem_AssertOE(void)
 {
-	PORTB &= ~SIMM_OE;
+	Ports_SetOEOut(0);
 }
 
 void ExternalMem_DeassertOE(void)
 {
-	PORTB |= SIMM_OE;
+	Ports_SetOEOut(1);
 }

mcp23s17.c

@@ -113,6 +113,22 @@ void MCP23S17_SetPullups(uint16_t pullups)
 	MCP23S17_WriteBothRegs(MCP23S17_GPPUA, pullups);
 }
 
+// Determines the output values of output pins without reading any input pins
+uint16_t MCP23S17_GetOutputs(void)
+{
+	return MCP23S17_ReadBothRegs(MCP23S17_OLATA);
+}
+
+uint16_t MCP23S17_GetDDR(void)
+{
+	return MCP23S17_ReadBothRegs(MCP23S17_IODIRA);
+}
+
+uint16_t MCP23S17_GetPullups(void)
+{
+	return MCP23S17_ReadBothRegs(MCP23S17_GPPUA);
+}
+
 void MCP23S17_WriteBothRegs(uint8_t addrA, uint16_t value)
 {
 	// addrA should contain the address of the "A" register.

mcp23s17.h

@@ -15,5 +15,8 @@ void MCP23S17_SetDDR(uint16_t ddr);
 void MCP23S17_SetPins(uint16_t data);
 uint16_t MCP23S17_ReadPins(void);
 void MCP23S17_SetPullups(uint16_t pullups);
+uint16_t MCP23S17_GetOutputs(void);
+uint16_t MCP23S17_GetDDR(void);
+uint16_t MCP23S17_GetPullups(void);
 
 #endif /* MCP23S17_H_ */

ports.c

@@ -0,0 +1,338 @@
+/*
+ * ports.c
+ *
+ *  Created on: Nov 26, 2011
+ *      Author: Doug
+ */
+
+#include "ports.h"
+#include "mcp23s17.h"
+#include <avr/io.h>
+
+// SIMM control signals on port B
+#define SIMM_WE		(1 << 6)
+#define SIMM_OE		(1 << 5)
+#define SIMM_CS		(1 << 4)
+
+void Ports_Init(void)
+{
+	// This module depends on the MPC23S17
+	MCP23S17_Init();
+}
+
+void Ports_SetAddressOut(uint32_t data)
+{
+	// NOTE: If any of PORTA or PORTC or PORTD pins 0, 1, 4, 5, or 6 are set as
+	// inputs, this function might mess with their pull-up resistors.
+	// Only use it under normal operation when all the address pins are being
+	// used as outputs
+
+	PORTA = (data & 0xFF); // A0-A7
+	PORTC = ((data >> 8) & 0xFF); // A8-A15
+
+	// A16-A20 are special because they are split up...(We use PORTD pins 0, 1, 4, 5, 6)
+	uint8_t tmp = (data >> 16) & 0xFF;
+	tmp = (tmp & 0x03) | ((tmp & 0x1C) << 2);
+
+	// Now, turn off the pins we have to turn off, and turn on the pins we have to turn on
+	// (without affecting other pins [2, 3, and 7] that we aren't supposed to touch)
+	PORTD &= (0x8C | tmp); // This should turn off all '0' bits in tmp.
+	PORTD |= tmp; // This should turn on all '1' bits in tmp
+}
+
+void Ports_AddressOut_RMW(uint32_t data, uint32_t modifyMask)
+{
+	uint32_t modifiedDataOn = data & modifyMask;
+	uint32_t modifiedDataOff = data | ~modifyMask;
+
+	// Turn on/off requested bits in the PORT register.
+	PORTA |= ((modifiedDataOn >> 0) & 0xFF);
+	PORTA &= ((modifiedDataOff >> 0) & 0xFF);
+	PORTC |= ((modifiedDataOn >> 8) & 0xFF);
+	PORTC &= ((modifiedDataOff >> 8) & 0xFF);
+
+	// A16-A20 are special because they are split up...(We use PORTD pins 0, 1, 4, 5, 6)
+	uint8_t tmp = (modifiedDataOn >> 16) & 0xFF;
+	tmp = (tmp & 0x03) | ((tmp & 0x1C) << 2);
+
+	PORTD |= tmp;
+	PORTD &= (0x8C | tmp);
+}
+
+void Ports_SetDataOut(uint32_t data)
+{
+	// NOTE: If any pins of PORTE or PORTF are set as inputs, this
+	// function might mess with their pull-up resistors.
+	// Only use it under normal operation when all the address pins are being
+	// used as outputs
+
+	// Set the actual outputted values
+	MCP23S17_SetPins(data & 0xFFFF); // D0-D15
+	PORTE = ((data >> 16) & 0xFF); // D16-D23
+	PORTF = ((data >> 24) & 0xFF); // D24-D31
+}
+
+void Ports_DataOut_RMW(uint32_t data, uint32_t modifyMask)
+{
+	uint32_t modifiedDataOn = data & modifyMask;
+	uint32_t modifiedDataOff = data | ~modifyMask;
+
+	// Read what's in it first...
+	uint16_t outputLatches = MCP23S17_GetOutputs();
+	outputLatches |= (modifiedDataOn) & 0xFFFF;
+	outputLatches &= modifiedDataOff & 0xFFFF;
+	MCP23S17_SetPins(outputLatches);
+
+	// Turn on/off requested bits in the PORT register.
+	PORTE |= ((modifiedDataOn >> 16) & 0xFF);
+	PORTE &= ((modifiedDataOff >> 16) & 0xFF);
+	PORTF |= ((modifiedDataOn >> 24) & 0xFF);
+	PORTF &= ((modifiedDataOff >> 24) & 0xFF);
+}
+
+void Ports_SetCSOut(bool data)
+{
+	if (data)
+	{
+		PORTB |= SIMM_CS;
+	}
+	else
+	{
+		PORTB &= ~SIMM_CS;
+	}
+}
+
+void Ports_SetOEOut(bool data)
+{
+	if (data)
+	{
+		PORTB |= SIMM_OE;
+	}
+	else
+	{
+		PORTB &= ~SIMM_OE;
+	}
+}
+
+void Ports_SetWEOut(bool data)
+{
+	if (data)
+	{
+		PORTB |= SIMM_WE;
+	}
+	else
+	{
+		PORTB &= ~SIMM_WE;
+	}
+}
+
+void Ports_SetAddressDDR(uint32_t ddr)
+{
+	PORTA = (ddr & 0xFF); // A0-A7
+	PORTC = ((ddr >> 8) & 0xFF); // A8-A15
+
+	// A16-A20 are special because they are split up...(We use PORTD pins 0, 1, 4, 5, 6)
+	uint8_t tmp = (ddr >> 16) & 0xFF;
+	tmp = (tmp & 0x03) | ((tmp & 0x1C) << 2);
+
+	// Now, turn off the DDR bits we have to turn off,
+	// and turn on the DDR bits we have to turn on
+	// (without affecting other bits [2, 3, and 7]
+	// that we aren't supposed to touch)
+	DDRD &= (0x8C | tmp); // This should turn off all '0' bits in tmp.
+	DDRD |= tmp; // This should turn on all '1' bits in tmp
+}
+
+void Ports_AddressDDR_RMW(uint32_t ddr, uint32_t modifyMask)
+{
+	uint32_t modifiedDataOn = ddr & modifyMask;
+	uint32_t modifiedDataOff = ddr | ~modifyMask;
+
+	// Turn on/off requested bits in the DDR register.
+	DDRA |= ((modifiedDataOn >> 0) & 0xFF);
+	DDRA &= ((modifiedDataOff >> 0) & 0xFF);
+	DDRC |= ((modifiedDataOn >> 8) & 0xFF);
+	DDRC &= ((modifiedDataOff >> 8) & 0xFF);
+
+	// A16-A20 are special because they are split up...(We use PORTD pins 0, 1, 4, 5, 6)
+	uint8_t tmp = (modifiedDataOn >> 16) & 0xFF;
+	tmp = (tmp & 0x03) | ((tmp & 0x1C) << 2);
+
+	DDRD |= tmp;
+	DDRD &= (0x8C | tmp);
+}
+
+void Ports_SetDataDDR(uint32_t ddr)
+{
+	MCP23S17_SetDDR(ddr & 0xFFFF); // D0-D15
+	DDRE = ((ddr >> 16) & 0xFF); // D16-D23
+	DDRF = ((ddr >> 24) & 0xFF); // D24-D31
+}
+
+void Ports_DataDDR_RMW(uint32_t ddr, uint32_t modifyMask)
+{
+	uint32_t modifiedDataOn = ddr & modifyMask;
+	uint32_t modifiedDataOff = ddr | ~modifyMask;
+
+	// If we can get away with it, don't bother reading back...
+	if ((modifyMask & 0xFFFF) == 0xFFFF)
+	{
+		MCP23S17_SetDDR(modifiedDataOn & 0xFFFF);
+	}
+	else // Otherwise, we have to read what's in it first...(unless I decide to keep a local cached copy)
+	{
+		uint16_t outputLatches = MCP23S17_GetDDR();
+		outputLatches |= (modifiedDataOn) & 0xFFFF;
+		outputLatches &= modifiedDataOff & 0xFFFF;
+		MCP23S17_SetDDR(outputLatches);
+	}
+
+	// Turn on/off requested bits in the DDR register.
+	DDRE |= ((modifiedDataOn >> 16) & 0xFF);
+	DDRE &= ((modifiedDataOff >> 16) & 0xFF);
+	DDRF |= ((modifiedDataOn >> 24) & 0xFF);
+	DDRF &= ((modifiedDataOff >> 24) & 0xFF);
+}
+
+void Ports_SetCSDDR(bool ddr)
+{
+	if (ddr)
+	{
+		DDRB |= SIMM_CS;
+	}
+	else
+	{
+		DDRB &= ~SIMM_CS;
+	}
+}
+
+void Ports_SetOEDDR(bool ddr)
+{
+	if (ddr)
+	{
+		DDRB |= SIMM_OE;
+	}
+	else
+	{
+		DDRB &= ~SIMM_OE;
+	}
+}
+
+void Ports_SetWEDDR(bool ddr)
+{
+	if (ddr)
+	{
+		DDRB |= SIMM_WE;
+	}
+	else
+	{
+		DDRB &= ~SIMM_WE;
+	}
+}
+
+void Ports_AddressPullups_RMW(uint32_t pullups, uint32_t modifyMask)
+{
+	// Pull-ups are set by writing to the data register when in input mode.
+	// MAKE SURE THE PINS ARE SET AS INPUTS FIRST!
+	Ports_AddressOut_RMW(pullups, modifyMask);
+}
+
+void Ports_DataPullups_RMW(uint32_t pullups, uint32_t modifyMask)
+{
+	// Pull-ups here are a little more tricky because the MCP23S17 has
+	// separate registers for pull-up enable.
+	uint32_t modifiedDataOn = pullups & modifyMask;
+	uint32_t modifiedDataOff = pullups | ~modifyMask;
+
+	// If we can get away with it, don't bother reading back...
+	if ((modifyMask & 0xFFFF) == 0xFFFF)
+	{
+		MCP23S17_SetPullups(modifiedDataOn & 0xFFFF);
+	}
+	else // Otherwise, we have to read what's in it first...(unless I decide to keep a local cached copy)
+	{
+		uint16_t outputLatches = MCP23S17_GetPullups();
+		outputLatches |= (modifiedDataOn) & 0xFFFF;
+		outputLatches &= modifiedDataOff & 0xFFFF;
+		MCP23S17_SetPullups(outputLatches);
+	}
+
+	// Turn on/off requested bits in the PORT register for the other 16 bits.
+	PORTE |= ((modifiedDataOn >> 16) & 0xFF);
+	PORTE &= ((modifiedDataOff >> 16) & 0xFF);
+	PORTF |= ((modifiedDataOn >> 24) & 0xFF);
+	PORTF &= ((modifiedDataOff >> 24) & 0xFF);
+}
+
+void Ports_SetCSPullup(bool pullup)
+{
+	if (pullup)
+	{
+		PORTB |= SIMM_CS;
+	}
+	else
+	{
+		PORTB &= ~SIMM_CS;
+	}
+}
+
+void Ports_SetOEPullup(bool pullup)
+{
+	if (pullup)
+	{
+		PORTB |= SIMM_OE;
+	}
+	else
+	{
+		PORTB &= ~SIMM_OE;
+	}
+}
+
+void Ports_SetWEPullup(bool pullup)
+{
+	if (pullup)
+	{
+		PORTB |= SIMM_WE;
+	}
+	else
+	{
+		PORTB &= ~SIMM_WE;
+	}
+}
+
+
+uint32_t Ports_ReadAddress(void)
+{
+	uint32_t result = PINA;
+	result |= (((uint32_t)PINC) << 8);
+	uint8_t tmp = (PIND & 0x03) | ((PIND & 0x70) >> 2);
+	result |= (((uint32_t)tmp) << 16);
+
+	return result;
+}
+
+uint32_t Ports_ReadData(void)
+{
+	uint32_t result = (uint32_t)MCP23S17_ReadPins();
+
+	// Turn on/off requested bits in the PORT register.
+	result |= (((uint32_t)PINE) << 16);
+	result |= (((uint32_t)PINF) << 24);
+
+	return result;
+}
+
+bool Ports_ReadCSInData(void)
+{
+	return (PINB & SIMM_CS) != 0;
+}
+
+bool Ports_ReadOEInData(void)
+{
+	return (PINB & SIMM_OE) != 0;
+}
+
+bool Ports_ReadWEInData(void)
+{
+	return (PINB & SIMM_WE) != 0;
+}

ports.h

@@ -0,0 +1,60 @@
+/*
+ * ports.h
+ *
+ *  Created on: Nov 26, 2011
+ *      Author: Doug
+ */
+
+#ifndef PORTS_H_
+#define PORTS_H_
+
+#include <stdbool.h>
+#include <stdint.h>
+
+// Under normal operation, we will only be using the address pins as outputs.
+// The data pins will switch back and forth between all inputs and all outputs.
+// The CS/OE/WE pins will always be outputs.
+// The pullups will all be turned off.
+// So you should use Ports_SetAddressOut(),
+// Ports_SetDataOut(),
+// Ports_SetAddressDDR() [once at the beginning]
+// Ports_SetDataDDR(),
+// and Ports_ReadDataInData
+//
+// The reason I have implemented all this functionality is to give me complete
+// control over all the pins for other use cases, such as a SIMM electrical test.
+// By playing with pull-ups and inputs and outputs, I should be able to detect
+// many shorted output/input scenarios. So even though these functions are overkill,
+// they will be useful for diagnostics.
+
+void Ports_Init(void);
+
+void Ports_SetAddressOut(uint32_t data);
+void Ports_AddressOut_RMW(uint32_t data, uint32_t modifyMask);
+void Ports_SetDataOut(uint32_t data);
+void Ports_DataOut_RMW(uint32_t data, uint32_t modifyMask);
+void Ports_SetCSOut(bool data);
+void Ports_SetOEOut(bool data);
+void Ports_SetWEOut(bool data);
+
+void Ports_SetAddressDDR(uint32_t ddr);
+void Ports_AddressDDR_RMW(uint32_t ddr, uint32_t modifyMask);
+void Ports_SetDataDDR(uint32_t ddr);
+void Ports_DataDDR_RMW(uint32_t ddr, uint32_t modifyMask);
+void Ports_SetCSDDR(bool ddr);
+void Ports_SetOEDDR(bool ddr);
+void Ports_SetWEDDR(bool ddr);
+
+void Ports_AddressPullups_RMW(uint32_t pullups, uint32_t modifyMask);
+void Ports_DataPullups_RMW(uint32_t pullups, uint32_t modifyMask);
+void Ports_SetCSPullup(bool pullup);
+void Ports_SetOEPullup(bool pullup);
+void Ports_SetWEPullup(bool pullup);
+
+uint32_t Ports_ReadAddress(void);
+uint32_t Ports_ReadData(void);
+bool Ports_ReadCS(void);
+bool Ports_ReadOE(void);
+bool Ports_ReadWE(void);
+
+#endif /* PORTS_H_ */

tests/simm_electrical_test.c

@@ -0,0 +1,16 @@
+/*
+ * simm_electrical_test.c
+ *
+ *  Created on: Nov 26, 2011
+ *      Author: Doug
+ */
+
+#include "simm_electrical_test.h"
+
+int SIMMElectricalTest_Run(void)
+{
+	// Returns number of errors found
+	int numErrors = 0;
+
+	return numErrors;
+}

tests/simm_electrical_test.h

@@ -0,0 +1,12 @@
+/*
+ * simm_electrical_test.h
+ *
+ *  Created on: Nov 26, 2011
+ *      Author: Doug
+ */
+
+#ifndef SIMM_ELECTRICAL_TEST_H_
+#define SIMM_ELECTRICAL_TEST_H_
+
+
+#endif /* SIMM_ELECTRICAL_TEST_H_ */