IDE driver and dumper bugfixes, minor corrections

CMakeLists.txt

@@ -27,6 +27,7 @@ add_executable(
 	src/gpu.cpp
 	src/gpufont.cpp
 	src/ide.cpp
+	src/ideglue.cpp
 	src/io.cpp
 	src/main.cpp
 	src/pad.cpp

src/app/app.cpp

@@ -72,10 +72,7 @@ bool App::_cartDetectWorker(void) {
 	_unloadCartData();
 
 #ifdef ENABLE_DUMMY_DRIVER
-	if (
+	if (_resourceProvider->loadStruct(_dump, "data/test.573")) {
-		_resourceProvider->loadStruct(_dump, "data/test.573")
-		== sizeof(cart::Dump)
-	) {
 		LOG("using dummy cart driver");
 		_driver = new cart::DummyDriver(_dump);
 	} else {
@@ -242,9 +239,10 @@ bool App::_cartDumpWorker(void) {
 	else
 		__builtin_strcpy(path, "unknown.573");
 
-	LOG("saving dump as %s", path);
+	size_t length = _dump.getDumpLength();
+	LOG("saving %s, length=%d", path, length);
 
-	if (_fileProvider->saveStruct(_dump, path) != sizeof(cart::Dump)) {
+	if (_fileProvider->saveData(&_dump, length, path) != length) {
 		_errorScreen.setMessage(
 			_cartInfoScreen, WSTR("App.cartDumpWorker.error")
 		);
@@ -374,6 +372,11 @@ struct DumpRegion {
 	uint32_t       inputs;
 };
 
+enum DumpBank {
+	BANK_NONE_8BIT  = -1,
+	BANK_NONE_16BIT = -2
+};
+
 static constexpr int _NUM_DUMP_REGIONS = 5;
 
 static const DumpRegion _DUMP_REGIONS[_NUM_DUMP_REGIONS]{
@@ -382,14 +385,14 @@ static const DumpRegion _DUMP_REGIONS[_NUM_DUMP_REGIONS]{
 		.path   = "dump_%d/bios.bin",
 		.ptr    = reinterpret_cast<const uint16_t *>(DEV2_BASE),
 		.length = 0x80000,
-		.bank   = -1,
+		.bank   = BANK_NONE_16BIT,
 		.inputs = 0
 	}, {
 		.prompt = "App.romDumpWorker.dumpRTC"_h,
 		.path   = "dump_%d/rtc.bin",
 		.ptr    = reinterpret_cast<const uint16_t *>(DEV0_BASE | 0x620000),
 		.length = 0x2000,
-		.bank   = -1,
+		.bank   = BANK_NONE_8BIT,
 		.inputs = 0
 	}, {
 		.prompt = "App.romDumpWorker.dumpFlash"_h,
@@ -446,7 +449,6 @@ bool App::_romDumpWorker(void) {
 		if (region.inputs && !(inputs & region.inputs))
 			continue;
 
-		_workerStatus.update(i, _NUM_DUMP_REGIONS, WSTRH(region.prompt));
 		snprintf(path, sizeof(path), region.path, index);
 
 		auto _file = _fileProvider->openFile(
@@ -456,24 +458,38 @@ bool App::_romDumpWorker(void) {
 		if (!_file)
 			goto _writeError;
 
-		// Read data from the source and write it to the file one 8 KB chunk at
+		// The buffer has to be 8 KB to match the size of RTC RAM.
-		// a time.
+		uint8_t buffer[0x2000];
-		uint16_t buffer[0x1000];
 
-		const uint16_t *ptr   = region.ptr;
+		const uint16_t *ptr  = region.ptr;
-		size_t         length = region.length;
+		int            count = region.length / sizeof(buffer);
-		int            bank   = region.bank;
+		int            bank  = region.bank;
 
-		io::setFlashBank(bank++);
+		if (bank >= 0)
+			io::setFlashBank(bank++);
 
-		for (; length; length -= sizeof(buffer)) {
+		for (int j = 0; j < count; j++) {
-			uint16_t *output = buffer;
+			_workerStatus.update(j, count, WSTRH(region.prompt));
 
-			for (int i = sizeof(buffer); i; i -= 2)
+			// The RTC is an 8-bit device connected to a 16-bit bus, i.e. each
-				*(output++) = *(ptr++);
+			// byte must be read as a 16-bit value and then the upper 8 bits
+			// must be discarded.
+			if (bank == BANK_NONE_8BIT) {
+				uint8_t *output = buffer;
 
-			// TODO TODO
+				for (size_t k = sizeof(buffer); k; k--)
-			if (ptr >= reinterpret_cast<const void *>(DEV0_BASE | 0x400000)) {
+					*(output++) = static_cast<uint8_t>(*(ptr++));
+			} else {
+				uint16_t *output = reinterpret_cast<uint16_t *>(buffer);
+
+				for (size_t k = sizeof(buffer); k; k -= 2)
+					*(output++) = *(ptr++);
+			}
+
+			if (
+				(bank >= 0) &&
+				(ptr  >= reinterpret_cast<const void *>(DEV0_BASE | 0x400000))
+			) {
 				ptr = region.ptr;
 				io::setFlashBank(bank++);
 			}

src/cart.cpp

@@ -102,7 +102,7 @@ void IdentifierSet::updateTraceID(TraceIDType type, int param) {
 				traceID.data[2] = checksum >> 8;
 			}
 
-			LOG("prefix=0x82, checksum=%04x", checksum);
+			LOG("prefix=0x82, checksum=0x%04x", checksum);
 			break;
 	}
 

src/ide.cpp

@@ -3,7 +3,6 @@
 #include <stdint.h>
 #include "ps1/registers.h"
 #include "ps1/system.h"
-#include "vendor/diskio.h"
 #include "ide.hpp"
 #include "io.hpp"
 #include "util.hpp"
@@ -29,7 +28,7 @@ static constexpr int _DMA_TIMEOUT          = 10000;
 
 /* Utilities */
 
-void _copyString(char *output, const uint16_t *input, size_t length) {
+static void _copyString(char *output, const uint16_t *input, size_t length) {
 	// The strings in the identification block are byte-swapped and padded with
 	// spaces. To make them printable, any span of consecutive space characters
 	// at the end is replaced with null bytes.
@@ -56,6 +55,33 @@ void _copyString(char *output, const uint16_t *input, size_t length) {
 	}
 }
 
+bool IdentifyBlock::validateChecksum(void) const {
+	if ((checksum & 0xff) != 0xa5)
+		return true;
+
+	uint8_t value = (util::sum(
+		reinterpret_cast<const uint8_t *>(&deviceFlags), ATA_SECTOR_SIZE - 1
+	) & 0xff) ^ 0xff;
+
+	if (value != (checksum >> 8)) {
+		LOG("mismatch, exp=0x%02x, got=0x%02x", value, checksum >> 8);
+		return false;
+	}
+
+	return true;
+}
+
+int IdentifyBlock::getHighestPIOMode(void) const {
+	if (timingValidityFlags & (1 << 1)) {
+		if (pioModeFlags & (1 << 1))
+			return 4;
+		if (pioModeFlags & (1 << 0))
+			return 3;
+	}
+
+	return 1;
+}
+
 /* Device class */
 
 Device devices[2]{ (DEVICE_PRIMARY), (DEVICE_SECONDARY) };
@@ -154,6 +180,7 @@ DeviceError Device::_transferDMA(void *data, size_t length, bool write) {
 	uint32_t flags = DMA_CHCR_MODE_BURST | DMA_CHCR_ENABLE | DMA_CHCR_TRIGGER;
 	flags         |= write ? DMA_CHCR_WRITE : DMA_CHCR_READ;
 
+	// TODO: is this actually needed?
 	BIU_DEV0_ADDR = reinterpret_cast<uint32_t>(SYS573_IDE_CS0_BASE) & 0x1fffffff;
 
 	DMA_MADR(DMA_PIO) = reinterpret_cast<uint32_t>(data);
@@ -199,6 +226,8 @@ DeviceError Device::enumerate(void) {
 		if (error)
 			return error;
 
+		if (!block.validateChecksum())
+			return CHECKSUM_MISMATCH;
 		if (
 			(block.deviceFlags & IDENTIFY_DEV_ATAPI_TYPE_BITMASK)
 			== IDENTIFY_DEV_ATAPI_TYPE_CDROM
@@ -218,6 +247,8 @@ DeviceError Device::enumerate(void) {
 		if (error)
 			return error;
 
+		if (!block.validateChecksum())
+			return CHECKSUM_MISMATCH;
 		if (block.commandSetFlags[1] & (1 << 10)) {
 			flags   |= DEVICE_HAS_LBA48;
 			capacity = block.getSectorCountExt();
@@ -232,17 +263,10 @@ DeviceError Device::enumerate(void) {
 	_copyString(revision, block.revision, sizeof(revision));
 	_copyString(serialNumber, block.serialNumber, sizeof(serialNumber));
 
-	LOG("%s: %s", (flags & DEVICE_SECONDARY) ? "secondary" : "primary", model);
+	LOG("%s=%s", (flags & DEVICE_SECONDARY) ? "sec" : "pri", model);
 
 	// Find out the fastest PIO transfer mode supported and enable it.
-	int mode = 1;
+	int mode = block.getHighestPIOMode();
-
-	if (block.timingValidityFlags & (1 << 1)) {
-		if (block.pioModeFlags & (1 << 1))
-			mode = 4;
-		else if (block.pioModeFlags & (1 << 0))
-			mode = 3;
-	}
 
 	_write(CS0_FEATURES, FEATURE_TRANSFER_MODE);
 	_write(CS0_COUNT,    (1 << 3) | mode);
@@ -250,13 +274,13 @@ DeviceError Device::enumerate(void) {
 	if (error)
 		return error;
 
-	LOG("done, stat=0x%02x", _read(CS0_STATUS));
+	LOG("done, stat=0x%02x, mode=PIO%d", _read(CS0_STATUS), mode);
 	flags |= DEVICE_READY;
 	return NO_ERROR;
 }
 
 DeviceError Device::_ideReadWrite(
-	uint32_t ptr, uint64_t lba, size_t count, bool write
+	uintptr_t ptr, uint64_t lba, size_t count, bool write
 ) {
 	if (flags & DEVICE_ATAPI)
 		return UNSUPPORTED_OP;
@@ -280,11 +304,17 @@ DeviceError Device::_ideReadWrite(
 		if (error)
 			return error;
 
-		error = _transferPIO(
+		// Data must be transferred one sector at a time as the drive may
-			reinterpret_cast<void *>(ptr), length * ATA_SECTOR_SIZE, write
+		// deassert DRQ between sectors.
-		);
+		for (size_t i = length; i; i--) {
-		if (error)
+			error = _transferDMA(
-			return error;
+				reinterpret_cast<void *>(ptr), ATA_SECTOR_SIZE, write
+			);
+			if (error)
+				return error;
+
+			ptr += ATA_SECTOR_SIZE;
+		}
 
 		error = _waitForStatus(
 			CS0_STATUS_BSY | CS0_STATUS_DRDY, CS0_STATUS_DRDY, _STATUS_TIMEOUT
@@ -292,14 +322,13 @@ DeviceError Device::_ideReadWrite(
 		if (error)
 			return error;
 
-		ptr   += length;
 		count -= length;
 	}
 
 	return NO_ERROR;
 }
 
-DeviceError Device::flushCache(void) {
+DeviceError Device::ideFlushCache(void) {
 	if (!(flags & DEVICE_HAS_FLUSH))
 		return NO_ERROR;
 		//return UNSUPPORTED_OP;
@@ -310,84 +339,4 @@ DeviceError Device::flushCache(void) {
 	);
 }
 
-/* FatFs API glue */
-
-extern "C" DSTATUS disk_initialize(uint8_t drive) {
-	if (devices[drive].enumerate())
-		return RES_NOTRDY;
-
-	return disk_status(drive);
-}
-
-extern "C" DSTATUS disk_status(uint8_t drive) {
-	auto     &dev  = devices[drive];
-	uint32_t flags = 0;
-
-	if (!(dev.flags & DEVICE_READY))
-		flags |= STA_NOINIT;
-	if (!dev.capacity)
-		flags |= STA_NODISK;
-	if (dev.flags & DEVICE_READ_ONLY)
-		flags |= STA_PROTECT;
-
-	return flags;
-}
-
-extern "C" DRESULT disk_read(
-	uint8_t drive, uint8_t *data, LBA_t lba, size_t count
-) {
-	auto &dev = devices[drive];
-
-	if (!(dev.flags & DEVICE_READY))
-		return RES_NOTRDY;
-	if (dev.ideRead(data, lba, count))
-		return RES_ERROR;
-
-	return RES_OK;
-}
-
-extern "C" DRESULT disk_write(
-	uint8_t drive, const uint8_t *data, LBA_t lba, size_t count
-) {
-	auto &dev = devices[drive];
-
-	if (!(dev.flags & DEVICE_READY))
-		return RES_NOTRDY;
-	if (dev.flags & DEVICE_READ_ONLY)
-		return RES_WRPRT;
-	if (dev.ideWrite(data, lba, count))
-		return RES_ERROR;
-
-	return RES_OK;
-}
-
-extern "C" DRESULT disk_ioctl(uint8_t drive, uint8_t cmd, void *data) {
-	auto &dev = devices[drive];
-
-	if (!(dev.flags & DEVICE_READY))
-		return RES_NOTRDY;
-
-	switch (cmd) {
-		case CTRL_SYNC:
-			return dev.flushCache() ? RES_ERROR : RES_OK;
-
-		case GET_SECTOR_COUNT:
-			__builtin_memcpy(data, &dev.capacity, sizeof(LBA_t));
-			return RES_OK;
-
-		case GET_SECTOR_SIZE:
-		//case GET_BLOCK_SIZE:
-			*reinterpret_cast<uint16_t *>(data) = (dev.flags & DEVICE_ATAPI)
-				? ATAPI_SECTOR_SIZE : ATA_SECTOR_SIZE;
-			return RES_OK;
-
-		default:
-			return RES_PARERR;
-	}
-}
-
-extern "C" uint32_t get_fattime(void) {
-	return io::getRTCTime();
-}
-
 }

src/ide.hpp

@@ -220,26 +220,30 @@ public:
 	uint16_t _reserved99[49];
 	uint16_t checksum;              // 255
 
-	inline uint32_t getSectorCount(void) {
+	inline uint32_t getSectorCount(void) const {
 		return sectorCount[0] | (sectorCount[1] << 16);
 	}
-	inline uint64_t getSectorCountExt(void) {
+	inline uint64_t getSectorCountExt(void) const {
 		return 0
 			| (uint64_t(sectorCountExt[0]) <<  0)
 			| (uint64_t(sectorCountExt[1]) << 16)
 			| (uint64_t(sectorCountExt[2]) << 32)
 			| (uint64_t(sectorCountExt[3]) << 48);
 	}
+
+	bool validateChecksum(void) const;
+	int getHighestPIOMode(void) const;
 };
 
 /* Device class */
 
 enum DeviceError {
-	NO_ERROR        = 0,
+	NO_ERROR          = 0,
-	UNSUPPORTED_OP  = 1,
+	UNSUPPORTED_OP    = 1,
-	STATUS_TIMEOUT  = 2,
+	STATUS_TIMEOUT    = 2,
-	DRIVE_ERROR     = 3,
+	DRIVE_ERROR       = 3,
-	INCOMPLETE_DATA = 4
+	INCOMPLETE_DATA   = 4,
+	CHECKSUM_MISMATCH = 5
 };
 
 enum DeviceFlag {
@@ -284,7 +288,7 @@ private:
 	DeviceError _transferDMA(void *data, size_t length, bool write = false);
 
 	DeviceError _ideReadWrite(
-		uint32_t ptr, uint64_t lba, size_t count, bool write
+		uintptr_t ptr, uint64_t lba, size_t count, bool write
 	);
 
 public:
@@ -296,6 +300,9 @@ public:
 	inline Device(uint32_t flags)
 	: flags(flags), capacity(0) {}
 
+	inline size_t getSectorSize(void) const {
+		return (flags & DEVICE_ATAPI) ? ATAPI_SECTOR_SIZE : ATA_SECTOR_SIZE;
+	}
 	inline DeviceError ideRead(void *data, uint64_t lba, size_t count) {
 		return _ideReadWrite(reinterpret_cast<uint32_t>(data), lba, count, false);
 	}
@@ -304,7 +311,7 @@ public:
 	}
 
 	DeviceError enumerate(void);
-	DeviceError flushCache(void);
+	DeviceError ideFlushCache(void);
 };
 
 extern Device devices[2];

src/ideglue.cpp

@@ -0,0 +1,85 @@
+
+#include <stddef.h>
+#include <stdint.h>
+#include "vendor/diskio.h"
+#include "ide.hpp"
+#include "io.hpp"
+
+/* FatFs library API glue */
+
+extern "C" DSTATUS disk_initialize(uint8_t drive) {
+	if (ide::devices[drive].enumerate())
+		return RES_NOTRDY;
+
+	return disk_status(drive);
+}
+
+extern "C" DSTATUS disk_status(uint8_t drive) {
+	auto     &dev  = ide::devices[drive];
+	uint32_t flags = 0;
+
+	if (!(dev.flags & ide::DEVICE_READY))
+		flags |= STA_NOINIT;
+	if (!dev.capacity)
+		flags |= STA_NODISK;
+	if (dev.flags & ide::DEVICE_READ_ONLY)
+		flags |= STA_PROTECT;
+
+	return flags;
+}
+
+extern "C" DRESULT disk_read(
+	uint8_t drive, uint8_t *data, LBA_t lba, size_t count
+) {
+	auto &dev = ide::devices[drive];
+
+	if (!(dev.flags & ide::DEVICE_READY))
+		return RES_NOTRDY;
+	if (dev.ideRead(data, lba, count))
+		return RES_ERROR;
+
+	return RES_OK;
+}
+
+extern "C" DRESULT disk_write(
+	uint8_t drive, const uint8_t *data, LBA_t lba, size_t count
+) {
+	auto &dev = ide::devices[drive];
+
+	if (!(dev.flags & ide::DEVICE_READY))
+		return RES_NOTRDY;
+	if (dev.flags & ide::DEVICE_READ_ONLY)
+		return RES_WRPRT;
+	if (dev.ideWrite(data, lba, count))
+		return RES_ERROR;
+
+	return RES_OK;
+}
+
+extern "C" DRESULT disk_ioctl(uint8_t drive, uint8_t cmd, void *data) {
+	auto &dev = ide::devices[drive];
+
+	if (!(dev.flags & ide::DEVICE_READY))
+		return RES_NOTRDY;
+
+	switch (cmd) {
+		case CTRL_SYNC:
+			return dev.ideFlushCache() ? RES_ERROR : RES_OK;
+
+		case GET_SECTOR_COUNT:
+			__builtin_memcpy(data, &dev.capacity, sizeof(LBA_t));
+			return RES_OK;
+
+		case GET_SECTOR_SIZE:
+		//case GET_BLOCK_SIZE:
+			*reinterpret_cast<uint16_t *>(data) = dev.getSectorSize();
+			return RES_OK;
+
+		default:
+			return RES_PARERR;
+	}
+}
+
+extern "C" uint32_t get_fattime(void) {
+	return io::getRTCTime();
+}

src/main.cpp

@@ -120,11 +120,13 @@ _fileInitDone:
 			goto _resourceInitDone;
 	}
 
-	zipFile = fileProvider.openFile(resPath, file::READ);
+	if (fileProvider.fileExists(resPath)) {
+		zipFile = fileProvider.openFile(resPath, file::READ);
 
-	if (zipFile) {
+		if (zipFile) {
-		if (resourceProvider.init(zipFile))
+			if (resourceProvider.init(zipFile))
-			goto _resourceInitDone;
+				goto _resourceInitDone;
+		}
 	}
 
 	resourceProvider.init(_resources, _resourcesSize);

src/uibase.hpp

@@ -152,6 +152,7 @@ public:
 	void *screenData; // Opaque, can be accessed by screens
 
 	inline void tick(void) {
+		//buttons.update();
 		time++;
 	}
 	inline void setBackgroundLayer(Layer &layer) {

src/util.cpp

@@ -163,7 +163,7 @@ size_t traceIDToString(char *output, const uint8_t *input) {
 
 // This encoding is similar to standard base45, but with some problematic
 // characters (' ', '$', '%', '*') excluded.
-static const char _BASE41_CHARSET[]{ "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ+-./:"};
+static const char _BASE41_CHARSET[]{ "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ+-./:" };
 
 size_t encodeBase41(char *output, const uint8_t *input, size_t length) {
 	size_t outLength = 0;

src/util.hpp

@@ -56,7 +56,7 @@ static inline uint32_t swapEndian(uint32_t value) {
 	return value;
 }
 
-template<typename T, typename X> inline void assertAligned(X *ptr) {
+template<typename T, typename X> static inline void assertAligned(X *ptr) {
 	assert(!(reinterpret_cast<uintptr_t>(ptr) % alignof(T)));
 }