Don't assert OE and CS simultaneously during electrical test

If you do, and a SIMM is installed in the socket, you will get an
erroneous short detected for any data pins that are currently outputting
a 0. Avoid this by testing OE and CS for 5V shorts separately.

drivers/parallel_flash.c

@@ -140,112 +140,32 @@ void ParallelFlash_EraseChips(uint8_t chipsMask)
  * @param address The start address to erase (must be aligned to a sector boundary)
  * @param length The number of bytes to erase (must be aligned to a sector boundary)
  * @param chipsMask The mask of which chips to erase
- * @param numEraseSectorGroups The number of erase sector groups we know about
- * @param eraseSectorGroups The erase sector groups
  * @return True on success, false on failure
  */
-bool ParallelFlash_EraseSectors(uint32_t address, uint32_t length, uint8_t chipsMask, uint8_t numEraseSectorGroups, ParallelFlashEraseSectorGroup const *eraseSectorGroups)
+bool ParallelFlash_EraseSectors(uint32_t address, uint32_t length, uint8_t chipsMask)
 {
-	// Choose a default sector group if we don't have the info
-	static const ParallelFlashEraseSectorGroup defaultSST39SF040Sectors[] = {
-		{0xFFFFFFFFUL, SECTOR_SIZE_SST39SF040}
-	};
-
-	static const ParallelFlashEraseSectorGroup defaultM29F160FBSectors[] = {
-		{1, 0x4000},
-		{2, 0x2000},
-		{1, 0x8000},
-		{0xFFFFFFFFUL, SECTOR_SIZE_M29F160FB5AN6E2_8}
-	};
-
-	// If we don't know the sector info (older programmer or unknown chips)
-	// then fall back to the previous hardcoded sector maps.
-	// Note that "chip type" isn't really accurate anymore; this is more about
-	// whether or not it has shifted unlock addresses. But these are the hardcoded
-	// defaults that seemed to work okay for people previously.
-	if (numEraseSectorGroups == 0)
-	{
-		switch (curChipType)
-		{
-		case ParallelFlash_SST39SF040_x4:
-		default:
-			eraseSectorGroups = defaultSST39SF040Sectors;
-			numEraseSectorGroups = sizeof(defaultSST39SF040Sectors)/sizeof(defaultSST39SF040Sectors[0]);
-			break;
-		case ParallelFlash_M29F160FB5AN6E2_x4:
-			eraseSectorGroups = defaultM29F160FBSectors;
-			numEraseSectorGroups = sizeof(defaultM29F160FBSectors)/sizeof(defaultM29F160FBSectors[0]);
-			break;
-		}
-	}
-
 	bool result = false;
 
-	// The first sector group and index in that group to erase
-	uint32_t firstSectorGroup = 0;
-	uint32_t firstSectorInGroup = 0;
-
-	// Temporary counters for matching up sector locations
-	uint32_t curSectorGroup = 0;
-	uint32_t curSectorInGroup = 0;
-
-	// Find the first sector we need to erase. Keep searching until we've
-	// 1) found it or gone past it, or
-	// 2) exhausted our list of erase sector groups
-	uint32_t curAddress = 0;
-	while (curAddress < address &&
-		   curSectorGroup < numEraseSectorGroups)
+	// Figure out our sector size
+	uint32_t sectorSize;
+	switch (curChipType)
 	{
-		curAddress += eraseSectorGroups[curSectorGroup].size;
-		curSectorInGroup++;
-		if (curSectorInGroup >= eraseSectorGroups[curSectorGroup].count)
-		{
-			curSectorGroup++;
-			curSectorInGroup = 0;
-		}
+	case ParallelFlash_SST39SF040_x4:
+	default:
+		sectorSize = SECTOR_SIZE_SST39SF040;
+		break;
+	case ParallelFlash_M29F160FB5AN6E2_x4:
+		sectorSize = SECTOR_SIZE_M29F160FB5AN6E2_8;
+		break;
 	}
 
-	// If the start address wasn't on a sector boundary, bail
-	if (curAddress != address)
+	// Make sure the area requested to be erased is on good boundaries
+	if ((address % sectorSize) ||
+		(length % sectorSize))
 	{
 		return false;
 	}
 
-	// OK, we've found our first sector to erase.
-	firstSectorGroup = curSectorGroup;
-	firstSectorInGroup = curSectorInGroup;
-
-	// Now, locate our last sector to erase.
-	uint32_t curLength = 0;
-	while (curLength < length &&
-		   curSectorGroup < numEraseSectorGroups)
-	{
-		curLength += eraseSectorGroups[curSectorGroup].size;
-
-		// If we still haven't handled the entire requested space,
-		// go to the next sector
-		if (curLength < length)
-		{
-			curSectorInGroup++;
-			if (curSectorInGroup >= eraseSectorGroups[curSectorGroup].count)
-			{
-				curSectorGroup++;
-				curSectorInGroup = 0;
-			}
-		}
-	}
-
-	// If the length wasn't on a sector boundary, bail
-	if (curLength != length)
-	{
-		return false;
-	}
-
-	// We've now verified that everything is on a sector boundary, so we can
-	// go ahead with the erase operation!
-	curSectorGroup = firstSectorGroup;
-	curSectorInGroup = firstSectorInGroup;
-
 	// We're good to go. Let's do it. The process varies based on the chip type
 	if (curChipType == ParallelFlash_SST39SF040_x4)
 	{
@@ -263,15 +183,8 @@ bool ParallelFlash_EraseSectors(uint32_t address, uint32_t length, uint8_t chips
 			// unlock sequence has to be done again after this sector is done.
 			ParallelBus_WriteCycle(address, 0x30303030UL);
 
-			// Move our counters in preparation for the next sector
-			address += eraseSectorGroups[curSectorGroup].size;
-			length -= eraseSectorGroups[curSectorGroup].size;
-			curSectorInGroup++;
-			if (curSectorInGroup >= eraseSectorGroups[curSectorGroup].count)
-			{
-				curSectorGroup++;
-				curSectorInGroup = 0;
-			}
+			address += sectorSize;
+			length -= sectorSize;
 
 			// Wait for completion of this individual erase operation before
 			// we can start a new erase operation.
@@ -289,19 +202,25 @@ bool ParallelFlash_EraseSectors(uint32_t address, uint32_t length, uint8_t chips
 		ParallelBus_WriteCycle(ParallelFlash_UnlockAddress1(), 0x80808080UL);
 		ParallelFlash_UnlockChips(chipsMask);
 
+		// Now provide as many sector addresses as needed to erase.
+		// The first address is a bit of a special case because the boot sector
+		// actually has finer granularity for sector sizes.
+		if (address == 0)
+		{
+			ParallelBus_WriteCycle(0x00000000UL, 0x30303030UL);
+			ParallelBus_WriteCycle(0x00004000UL, 0x30303030UL);
+			ParallelBus_WriteCycle(0x00006000UL, 0x30303030UL);
+			ParallelBus_WriteCycle(0x00008000UL, 0x30303030UL);
+			address += sectorSize;
+			length -= sectorSize;
+		}
+
+		// The remaining sectors can use a more generic algorithm
 		while (length)
 		{
 			ParallelBus_WriteCycle(address, 0x30303030UL);
-
-			// Move our counters in preparation for the next sector
-			address += eraseSectorGroups[curSectorGroup].size;
-			length -= eraseSectorGroups[curSectorGroup].size;
-			curSectorInGroup++;
-			if (curSectorInGroup >= eraseSectorGroups[curSectorGroup].count)
-			{
-				curSectorGroup++;
-				curSectorInGroup = 0;
-			}
+			address += sectorSize;
+			length -= sectorSize;
 		}
 
 		// Wait for completion of the entire erase operation
@@ -311,6 +230,7 @@ bool ParallelFlash_EraseSectors(uint32_t address, uint32_t length, uint8_t chips
 	}
 
 	return result;
+
 }
 
 /** Writes a buffer of data to all 4 chips simultaneously

drivers/parallel_flash.h

@@ -54,13 +54,6 @@ typedef enum ParallelFlashChipType
 	ParallelFlash_M29F160FB5AN6E2_x4,
 } ParallelFlashChipType;
 
-/// Struct representing a group of identical erase sectors
-typedef struct ParallelFlashEraseSectorGroup
-{
-	uint32_t count;
-	uint32_t size;
-} ParallelFlashEraseSectorGroup;
-
 // Tells which type of flash chip we are communicating with
 void ParallelFlash_SetChipType(ParallelFlashChipType type);
 ParallelFlashChipType ParallelFlash_ChipType(void);
@@ -76,7 +69,7 @@ void ParallelFlash_IdentifyChips(ParallelFlashChipID *chips);
 
 // Erases the chips/sectors requested
 void ParallelFlash_EraseChips(uint8_t chipsMask);
-bool ParallelFlash_EraseSectors(uint32_t address, uint32_t length, uint8_t chipsMask, uint8_t numEraseSectorGroups, ParallelFlashEraseSectorGroup const *eraseSectorGroups);
+bool ParallelFlash_EraseSectors(uint32_t address, uint32_t length, uint8_t chipsMask);
 
 // Writes a buffer to all 4 chips simultaneously (each uint32_t contains an 8-bit portion for each chip).
 // Optimized variant of this function if we know we're writing to all 4 chips simultaneously.

programmer_protocol.h

@@ -46,9 +46,7 @@ typedef enum ProgrammerCommand
     ErasePortion,
     WriteChipsAt,
     ReadChipsAt,
-	SetChipsMask,
-	SetSectorLayout,
-	GetFirmwareVersion
+    SetChipsMask
 } ProgrammerCommand;
 
 // After a command is sent, the programmer will always respond with
@@ -186,15 +184,4 @@ typedef enum ProgrammerErasePortionOfChipReply
 	ProgrammerErasePortionFinished
 } ProgrammerErasePortionOfChipReply;
 
-// -------------------------  GET FIRMWARE VERSION PROTOCOL  -------------------------
-// If the command is GetFirmwareVersion, the programmer will reply CommandReplyOK.
-// Next, it will return 4 bytes: major version, minor version, revision, and a final
-// byte where 0 means it's a normal version and 1 means it's a prerelease version.
-// Other values are reserved.
-// Finally, it will finish the response with ProgrammerGetFWVersionDone.
-typedef enum ProgrammerGetFWVersionReply
-{
-	ProgrammerGetFWVersionDone
-} ProgrammerGetFWVersionReply;
-
 #endif /* PROGRAMMER_PROTOCOL_H_ */

simm_programmer.c

@@ -29,7 +29,6 @@
 #include "led.h"
 #include "hardware.h"
 #include <stdbool.h>
-#include <string.h>
 
 /// Maximum size of an individual chip on a SIMM we read
 #define MAX_CHIP_SIZE				(2UL * 1024UL * 1024UL)
@@ -39,20 +38,8 @@
 #if ((READ_WRITE_CHUNK_SIZE_BYTES % 4) != 0)
 #error Read/write chunk size should be a multiple of 4 bytes
 #endif
-
-/// The maximum number of erase groups we deal with
-#define MAX_ERASE_SECTOR_GROUPS				10
-
-/// Version info to respond with
-#define VERSION_MAJOR						1
-#define VERSION_MINOR						5
-#define VERSION_REVISION					0
-
-/// The number of erase sector groups we know about currently.
-/// If it's zero, we don't know, so fall back to defaults.
-static uint8_t numEraseSectorGroups = 0;
-/// The erase sector groups that we will pass to the programmer
-static ParallelFlashEraseSectorGroup eraseSectorGroups[MAX_ERASE_SECTOR_GROUPS];
+/// The smallest granularity for sector erase that we support
+#define ERASE_SECTOR_SIZE_BYTES		(256UL * 1024UL)
 
 /// Internal state so we know how to interpret the next-received byte
 typedef enum ProgrammerCommandState
@@ -65,7 +52,6 @@ typedef enum ProgrammerCommandState
 	ReadingChipsReadStartPos,    //!< Reading the start position for reading data from the SIMM
 	WritingChipsReadingStartPos, //!< Reading the start position for writing data to the SIMM
 	ReadingChipsMask,            //!< Reading the bitmask of which chips should be programmed
-	ReadingSectorLayout,         //!< Reading the erase sector layout
 } ProgrammerCommandState;
 static ProgrammerCommandState curCommandState = WaitingForCommand;
 
@@ -98,7 +84,6 @@ static void SIMMProgrammer_HandleErasePortionReadPosLengthByte(uint8_t byte);
 static void SIMMProgrammer_HandleReadingChipsReadStartPosByte(uint8_t byte);
 static void SIMMProgrammer_HandleWritingChipsReadingStartPosByte(uint8_t byte);
 static void SIMMProgrammer_HandleReadingChipsMaskByte(uint8_t byte);
-static void SIMMProgrammer_HandleReadingSectorLayoutByte(uint8_t byte);
 
 /** Initializes the SIMM programmer and prepares it for USB communication.
  *
@@ -147,9 +132,6 @@ void SIMMProgrammer_Check(void)
 		case ReadingChipsMask:
 			SIMMProgrammer_HandleReadingChipsMaskByte(recvByte);
 			break;
-		case ReadingSectorLayout:
-			SIMMProgrammer_HandleReadingSectorLayoutByte(recvByte);
-			break;
 		}
 	}
 
@@ -282,18 +264,6 @@ static void SIMMProgrammer_HandleWaitingForCommandByte(uint8_t byte)
 		curCommandState = ReadingChipsMask;
 		USBCDC_SendByte(CommandReplyOK);
 		break;
-	case SetSectorLayout:
-		curCommandState = ReadingSectorLayout;
-		USBCDC_SendByte(CommandReplyOK);
-		break;
-	case GetFirmwareVersion:
-		USBCDC_SendByte(CommandReplyOK);
-		USBCDC_SendByte(VERSION_MAJOR);
-		USBCDC_SendByte(VERSION_MINOR);
-		USBCDC_SendByte(VERSION_REVISION);
-		USBCDC_SendByte(0);
-		USBCDC_SendByte(ProgrammerGetFWVersionDone);
-		break;
 	// We don't know what this command is, so reply that it was invalid.
 	default:
 		USBCDC_SendByte(CommandReplyInvalid);
@@ -553,28 +523,44 @@ static void SIMMProgrammer_HandleErasePortionReadPosLengthByte(uint8_t byte)
 
 	if (++readLengthByteIndex >= 8)
 	{
+		ParallelFlashChipType chipType = ParallelFlash_ChipType();
 		bool eraseSuccess = false;
 
-		// Ensure the position and length are a multiple of 4 so that the division by 4
-		// won't confuse anything.
-		if (((erasePosition % 4) == 0) &&
-			((eraseLength % 4) == 0))
+		// Ensure they are both within limits of sector size erasure
+		if (((erasePosition % ERASE_SECTOR_SIZE_BYTES) == 0) &&
+			((eraseLength % ERASE_SECTOR_SIZE_BYTES) == 0))
 		{
 			uint32_t boundary = eraseLength + erasePosition;
 
-			// Ensure they are within the limits of our addressable length too.
-			// We can't address more than 8 MB of data at a time.
-			if (boundary <= (8 * 1024UL * 1024UL))
+			// Ensure they are within the limits of the chip size too
+			if (chipType == ParallelFlash_SST39SF040_x4)
 			{
-				// OK! We're erasing certain sectors of a SIMM.
-				USBCDC_SendByte(ProgrammerErasePortionOK);
-				// Send the response immediately, it could take a while.
-				USBCDC_Flush();
-				if (ParallelFlash_EraseSectors(erasePosition/PARALLEL_FLASH_NUM_CHIPS,
-						eraseLength/PARALLEL_FLASH_NUM_CHIPS, chipsMask,
-						numEraseSectorGroups, eraseSectorGroups))
+				if (boundary <= (8 * 1024UL * 1024UL))
 				{
-					eraseSuccess = true;
+					// OK! We're erasing certain sectors of a SIMM.
+					USBCDC_SendByte(ProgrammerErasePortionOK);
+					// Send the response immediately, it could take a while.
+					USBCDC_Flush();
+					if (ParallelFlash_EraseSectors(erasePosition/PARALLEL_FLASH_NUM_CHIPS,
+							eraseLength/PARALLEL_FLASH_NUM_CHIPS, chipsMask))
+					{
+						eraseSuccess = true;
+					}
+				}
+			}
+			else if (chipType == ParallelFlash_M29F160FB5AN6E2_x4)
+			{
+				if (boundary <= (8 * 1024UL * 1024UL))
+				{
+					// OK! We're erasing certain sectors of a SIMM.
+					USBCDC_SendByte(ProgrammerErasePortionOK);
+					// Send the response immediately, it could take a while.
+					USBCDC_Flush();
+					if (ParallelFlash_EraseSectors(erasePosition/PARALLEL_FLASH_NUM_CHIPS,
+							eraseLength/PARALLEL_FLASH_NUM_CHIPS, chipsMask))
+					{
+						eraseSuccess = true;
+					}
 				}
 			}
 		}
@@ -660,57 +646,3 @@ static void SIMMProgrammer_HandleReadingChipsMaskByte(uint8_t byte)
 	// Done either way; now we're waiting for a command to arrive
 	curCommandState = WaitingForCommand;
 }
-
-/** Handles a received byte when we are reading in the sector layout
- *
- * @param byte The received byte, which is the first sector layout byte
- */
-static void SIMMProgrammer_HandleReadingSectorLayoutByte(uint8_t byte)
-{
-	numEraseSectorGroups = 0;
-
-	uint32_t sectorCount = byte;
-	uint32_t sectorSize = 0;
-	int byteIndex = 1;
-
-	while (1)
-	{
-		// Read in the sector size
-		for (int i = byteIndex; i < 4; i++)
-		{
-			uint32_t nextByte = (uint32_t)USBCDC_ReadByteBlocking();
-			sectorCount |= nextByte << (i * 8);
-		}
-		// From now on, we loop over 4 bytes, not 3
-		byteIndex = 0;
-
-		// If we read in a count of 0, we're done!
-		if (sectorCount == 0)
-		{
-			break;
-		}
-
-		// We have a nonzero count, so read in the size now
-		for (int i = 0; i < 4; i++)
-		{
-			uint32_t nextByte = (uint32_t)USBCDC_ReadByteBlocking();
-			sectorSize |= nextByte << (i * 8);
-		}
-
-		// If we have room to store it in the array, do it
-		if (numEraseSectorGroups < MAX_ERASE_SECTOR_GROUPS)
-		{
-			eraseSectorGroups[numEraseSectorGroups].count = sectorCount;
-			eraseSectorGroups[numEraseSectorGroups].size = sectorSize;
-			numEraseSectorGroups++;
-		}
-
-		// Now read in the next chunk of data
-		sectorCount = 0;
-		sectorSize = 0;
-	}
-
-	// We got the list. Done!
-	USBCDC_SendByte(CommandReplyOK);
-	curCommandState = WaitingForCommand;
-}