From 746685047888dbaf96aa86ad80e579c36a35bef8 Mon Sep 17 00:00:00 2001 From: Nav Date: Wed, 29 Jan 2025 23:48:32 +0000 Subject: [PATCH] - Implemented program memory erasure routine in WchRiscV target driver - Moved away from relying on WCH-Link debug tool command for erasing program memory, due to a bug that I couldn't fix - Small tweaks to programming method selection in WCH-Link driver - Refactored flash peripheral registers in WchRiscV target driver --- .../Protocols/WchLink/WchLinkInterface.cpp | 6 ++ .../Wch/WchLinkDebugInterface.cpp | 56 ++++++++----- src/Targets/DynamicRegisterValue.cpp | 7 ++ src/Targets/DynamicRegisterValue.hpp | 2 + .../Flash/FlashControlRegisterFields.hpp | 14 ++++ .../Flash/FlashStatusRegisterFields.hpp | 14 ++++ src/Targets/RiscV/Wch/WchRiscV.cpp | 83 +++++++++++++++++-- src/Targets/RiscV/Wch/WchRiscV.hpp | 13 ++- .../RiscV/Wch/CH32V003.xml | 2 +- .../RiscV/Wch/CH32X035.xml | 2 +- 10 files changed, 166 insertions(+), 33 deletions(-) create mode 100644 src/Targets/RiscV/Wch/Peripherals/Flash/FlashControlRegisterFields.hpp create mode 100644 src/Targets/RiscV/Wch/Peripherals/Flash/FlashStatusRegisterFields.hpp diff --git a/src/DebugToolDrivers/Wch/Protocols/WchLink/WchLinkInterface.cpp b/src/DebugToolDrivers/Wch/Protocols/WchLink/WchLinkInterface.cpp index e40ec049..bb6ba845 100644 --- a/src/DebugToolDrivers/Wch/Protocols/WchLink/WchLinkInterface.cpp +++ b/src/DebugToolDrivers/Wch/Protocols/WchLink/WchLinkInterface.cpp @@ -241,6 +241,12 @@ namespace DebugToolDrivers::Wch::Protocols::WchLink this->sendCommandAndWaitForResponse(Commands::EndProgrammingSession{}); } + /* + * We don't actually use this anywhere, as the WCH-Link's erase function is buggy. For more, see the comment in + * the WchLinkDebugInterface::eraseMemory() member function. + * + * TODO: Consider removing this after v2.0.0 + */ void WchLinkInterface::eraseProgramMemory() { this->sendCommandAndWaitForResponse(Commands::EraseProgramMemory{}); } diff --git a/src/DebugToolDrivers/Wch/WchLinkDebugInterface.cpp b/src/DebugToolDrivers/Wch/WchLinkDebugInterface.cpp index 746d80d7..bd2dbb46 100644 --- a/src/DebugToolDrivers/Wch/WchLinkDebugInterface.cpp +++ b/src/DebugToolDrivers/Wch/WchLinkDebugInterface.cpp @@ -234,13 +234,12 @@ namespace DebugToolDrivers::Wch * the target. But the partial block write is faster and more suitable for writing buffers that are * smaller than 64 bytes, such as when we're inserting software breakpoints. */ - if ( - buffer.size() <= WchLinkInterface::MAX_PARTIAL_BLOCK_WRITE_SIZE - || !this->fullBlockWriteCompatible(addressSpaceDescriptor, memorySegmentDescriptor, startAddress) + buffer.size() > (WchLinkInterface::MAX_PARTIAL_BLOCK_WRITE_SIZE * 3) + && this->fullBlockWriteCompatible(addressSpaceDescriptor, memorySegmentDescriptor, startAddress) ) { - Logger::debug("Using partial block write method"); - return this->writeProgramMemoryPartialBlock( + Logger::debug("Using full block write method"); + return this->writeProgramMemoryFullBlock( addressSpaceDescriptor, memorySegmentDescriptor, startAddress, @@ -248,8 +247,8 @@ namespace DebugToolDrivers::Wch ); } - Logger::debug("Using full block write method"); - return this->writeProgramMemoryFullBlock( + Logger::debug("Using partial block write method"); + return this->writeProgramMemoryPartialBlock( addressSpaceDescriptor, memorySegmentDescriptor, startAddress, @@ -269,21 +268,40 @@ namespace DebugToolDrivers::Wch const TargetAddressSpaceDescriptor& addressSpaceDescriptor, const TargetMemorySegmentDescriptor& memorySegmentDescriptor ) { - if (memorySegmentDescriptor == this->mainProgramSegmentDescriptor) { - return this->wchLinkInterface.eraseProgramMemory(); - } - - Logger::debug("Ignoring erase operation on `" + memorySegmentDescriptor.key + "` segment - not supported"); + /* + * WCH-Link tools provide an erase function that erases the entire user section (main program segment). + * However, this function doesn't always work. Sometimes, it silently fails and leaves the target in a bad + * state, causing further issues (like program memory corruption). + * + * I spent a long time trying to fix this, but all attempts failed. I then decided to cease all use of this + * function and just implement the erase procedure myself, in the target driver. + * + * See the WchRiscV::eraseMainFlashSegment() member function for more. + * + * For future reference, my notes on this issue: + * + * - I discovered the issue with a WCH-LinkE, FW version 2.9, connected to a CH32V003. I'm not sure if other + * targets are affected + * + * - The issue occurs rarely - a very specific sequence of events must take place for the issue to occur: + * 1. Perform exactly one partial block write, to the main program segment, at address 0x08000100, with + * exactly 64 bytes of data + * 2. Reset the target + * 3. Erase the target via the erase function provided by the WCH-Link tool + * The erase operation will silently fail, and the target will be left in a bad state, causing the subsequent + * full block write to corrupt the target's program memory + * + * - The reset is what causes the erase operation to fail. But I have no idea why. I've inspected the relevant + * registers, at the relevant times, but have found nothing significant that could explain this + * + * - Subsequent resets do not fix the issue, but another full block write does. My guess is that the first full + * block write (which corrupted program memory) corrected the target state, cleaning the mess made by the + * failed erase operation + */ + throw TargetOperationFailure{"Not supported"}; } void WchLinkDebugInterface::enableProgrammingMode() { - /* - * Nothing to do here - * - * We cannot prepare the WCH-Link tool for a programming session here, as the preparation process differs - * across the two types of flash write commands (full and partial block write). We don't know which command - * we'll be utilising at this point. - */ } void WchLinkDebugInterface::disableProgrammingMode() { diff --git a/src/Targets/DynamicRegisterValue.cpp b/src/Targets/DynamicRegisterValue.cpp index c28324eb..ff97006c 100644 --- a/src/Targets/DynamicRegisterValue.cpp +++ b/src/Targets/DynamicRegisterValue.cpp @@ -31,6 +31,13 @@ namespace Targets assert(registerDescriptor.size <= 8); } + DynamicRegisterValue& DynamicRegisterValue::operator = (const DynamicRegisterValue& other) { + assert(other.registerDescriptor == this->registerDescriptor); + this->value = other.value; + + return *this; + } + void DynamicRegisterValue::setBitField( const TargetBitFieldDescriptor& bitFieldDescriptor, std::uint64_t inputValue diff --git a/src/Targets/DynamicRegisterValue.hpp b/src/Targets/DynamicRegisterValue.hpp index 7dfc6e65..8c954309 100644 --- a/src/Targets/DynamicRegisterValue.hpp +++ b/src/Targets/DynamicRegisterValue.hpp @@ -19,6 +19,8 @@ namespace Targets DynamicRegisterValue(const TargetRegisterDescriptor& registerDescriptor, TargetMemoryBufferSpan value); DynamicRegisterValue(const TargetRegisterDescriptor& registerDescriptor, std::uint64_t value); + DynamicRegisterValue& operator = (const DynamicRegisterValue& other); + void setBitField(const TargetBitFieldDescriptor& bitFieldDescriptor, std::uint64_t inputValue); void setBitField(const std::string& bitFieldKey, std::uint64_t inputValue); diff --git a/src/Targets/RiscV/Wch/Peripherals/Flash/FlashControlRegisterFields.hpp b/src/Targets/RiscV/Wch/Peripherals/Flash/FlashControlRegisterFields.hpp new file mode 100644 index 00000000..8eb27cdb --- /dev/null +++ b/src/Targets/RiscV/Wch/Peripherals/Flash/FlashControlRegisterFields.hpp @@ -0,0 +1,14 @@ +#pragma once + +#include "src/Targets/TargetBitFieldDescriptor.hpp" +#include "src/Targets/RiscV/Wch/TargetDescriptionFile.hpp" + +namespace Targets::RiscV::Wch::Peripherals::Flash +{ + struct FlashControlRegisterFields + { + const TargetBitFieldDescriptor& locked; + const TargetBitFieldDescriptor& startErase; + const TargetBitFieldDescriptor& mainSegmentErase; + }; +} diff --git a/src/Targets/RiscV/Wch/Peripherals/Flash/FlashStatusRegisterFields.hpp b/src/Targets/RiscV/Wch/Peripherals/Flash/FlashStatusRegisterFields.hpp new file mode 100644 index 00000000..07d6c1fb --- /dev/null +++ b/src/Targets/RiscV/Wch/Peripherals/Flash/FlashStatusRegisterFields.hpp @@ -0,0 +1,14 @@ +#pragma once + +#include "src/Targets/TargetBitFieldDescriptor.hpp" +#include "src/Targets/RiscV/Wch/TargetDescriptionFile.hpp" + +namespace Targets::RiscV::Wch::Peripherals::Flash +{ + struct FlashStatusRegisterFields + { + const TargetBitFieldDescriptor& busy; + const TargetBitFieldDescriptor& bootLock; + const TargetBitFieldDescriptor& bootMode; + }; +} diff --git a/src/Targets/RiscV/Wch/WchRiscV.cpp b/src/Targets/RiscV/Wch/WchRiscV.cpp index f4ead5d7..cb4949a9 100644 --- a/src/Targets/RiscV/Wch/WchRiscV.cpp +++ b/src/Targets/RiscV/Wch/WchRiscV.cpp @@ -2,6 +2,8 @@ #include #include +#include +#include #include "src/Targets/DynamicRegisterValue.hpp" @@ -12,6 +14,7 @@ #include "src/Services/StringService.hpp" #include "src/Logger/Logger.hpp" +#include "src/TargetController/Exceptions/TargetOperationFailure.hpp" namespace Targets::RiscV::Wch { @@ -36,8 +39,21 @@ namespace Targets::RiscV::Wch , flashKeyRegisterDescriptor(this->flashPeripheralDescriptor.getRegisterDescriptor("flash", "keyr")) , flashBootKeyRegisterDescriptor(this->flashPeripheralDescriptor.getRegisterDescriptor("flash", "boot_modekeyr")) , flashStatusRegisterDescriptor(this->flashPeripheralDescriptor.getRegisterDescriptor("flash", "statr")) - , flashStatusBootLockFieldDescriptor(this->flashStatusRegisterDescriptor.getBitFieldDescriptor("boot_lock")) - , flashStatusBootModeFieldDescriptor(this->flashStatusRegisterDescriptor.getBitFieldDescriptor("boot_mode")) + , flashControlRegisterDescriptor(this->flashPeripheralDescriptor.getRegisterDescriptor("flash", "ctrl")) + , flashControlRegisterFields( + Peripherals::Flash::FlashControlRegisterFields{ + .locked = this->flashControlRegisterDescriptor.getBitFieldDescriptor("lock"), + .startErase = this->flashControlRegisterDescriptor.getBitFieldDescriptor("strt"), + .mainSegmentErase = this->flashControlRegisterDescriptor.getBitFieldDescriptor("mer"), + } + ) + , flashStatusRegisterFields( + Peripherals::Flash::FlashStatusRegisterFields{ + .busy = this->flashStatusRegisterDescriptor.getBitFieldDescriptor("bsy"), + .bootLock = this->flashStatusRegisterDescriptor.getBitFieldDescriptor("boot_lock"), + .bootMode = this->flashStatusRegisterDescriptor.getBitFieldDescriptor("boot_mode"), + } + ) , rccPeripheralDescriptor(this->targetDescriptionFile.getTargetPeripheralDescriptor("rcc")) , portPeripheralClockEnableRegisterDescriptor( this->rccPeripheralDescriptor.getRegisterDescriptor("rcc", "apb2pcenr") @@ -321,11 +337,17 @@ namespace Targets::RiscV::Wch const TargetAddressSpaceDescriptor& addressSpaceDescriptor, const TargetMemorySegmentDescriptor& memorySegmentDescriptor ) { - if (memorySegmentDescriptor == this->mappedSegmentDescriptor) { - return RiscV::eraseMemory(addressSpaceDescriptor, this->selectedProgramSegmentDescriptor); + if ( + memorySegmentDescriptor == this->mainProgramSegmentDescriptor + || ( + memorySegmentDescriptor == this->mappedSegmentDescriptor + && this->selectedProgramSegmentDescriptor == this->mainProgramSegmentDescriptor + ) + ) { + return this->eraseMainFlashSegment(); } - RiscV::eraseMemory(addressSpaceDescriptor, memorySegmentDescriptor); + Logger::debug("Ignoring erase operation on `" + memorySegmentDescriptor.key + "` segment - not supported"); } TargetMemoryAddress WchRiscV::getProgramCounter() { @@ -659,11 +681,11 @@ namespace Targets::RiscV::Wch auto statusRegister = this->readRegisterDynamicValue(this->flashStatusRegisterDescriptor); - if (statusRegister.bitFieldAs(this->flashStatusBootLockFieldDescriptor)) { + if (statusRegister.bitFieldAs(this->flashStatusRegisterFields.bootLock)) { throw Exceptions::Exception{"Failed to unlock boot mode field"}; } - statusRegister.setBitField(this->flashStatusBootModeFieldDescriptor, true); + statusRegister.setBitField(this->flashStatusRegisterFields.bootMode, true); this->writeRegister(statusRegister); this->reset(); @@ -675,16 +697,59 @@ namespace Targets::RiscV::Wch auto statusRegister = this->readRegisterDynamicValue(this->flashStatusRegisterDescriptor); - if (statusRegister.bitFieldAs(this->flashStatusBootLockFieldDescriptor)) { + if (statusRegister.bitFieldAs(this->flashStatusRegisterFields.bootLock)) { throw Exceptions::Exception{"Failed to unlock boot mode field"}; } - statusRegister.setBitField(this->flashStatusBootModeFieldDescriptor, false); + statusRegister.setBitField(this->flashStatusRegisterFields.bootMode, false); this->writeRegister(statusRegister); this->reset(); } + void WchRiscV::eraseMainFlashSegment() { + static constexpr auto ERASE_RESPONSE_DELAY = std::chrono::microseconds{10}; + static constexpr auto ERASE_TIMEOUT = std::chrono::milliseconds{100}; + + this->unlockFlash(); + + auto statusRegister = this->readRegisterDynamicValue(this->flashStatusRegisterDescriptor); + if (statusRegister.bitFieldAs(this->flashStatusRegisterFields.busy)) { + throw Exceptions::TargetOperationFailure{"Flash peripheral is unavailable"}; + } + + auto controlRegister = this->readRegisterDynamicValue(this->flashControlRegisterDescriptor); + if (controlRegister.bitFieldAs(this->flashControlRegisterFields.locked)) { + throw Exceptions::TargetOperationFailure{"Failed to unlock flash"}; + } + + // It seems we have to write these bit fields individually. Otherwise, the target misbehaves + controlRegister.setBitField(this->flashControlRegisterFields.mainSegmentErase, true); + this->writeRegister(controlRegister); + controlRegister.setBitField(this->flashControlRegisterFields.startErase, true); + this->writeRegister(controlRegister); + + statusRegister = this->readRegisterDynamicValue(this->flashStatusRegisterDescriptor); + for ( + auto attempts = 0; + statusRegister.bitFieldAs(this->flashStatusRegisterFields.busy) + && (ERASE_RESPONSE_DELAY * attempts) <= ERASE_TIMEOUT; + ++attempts + ) { + std::this_thread::sleep_for(ERASE_RESPONSE_DELAY); + statusRegister = this->readRegisterDynamicValue(this->flashStatusRegisterDescriptor); + } + + controlRegister = this->readRegisterDynamicValue(this->flashControlRegisterDescriptor); + controlRegister.setBitField(this->flashControlRegisterFields.mainSegmentErase, false); + controlRegister.setBitField(this->flashControlRegisterFields.startErase, false); + this->writeRegister(controlRegister); + + if (statusRegister.bitFieldAs(this->flashStatusRegisterFields.busy)) { + throw Exceptions::TargetOperationFailure{"Erase operation timed out"}; + } + } + std::map WchRiscV::generateGpioPadDescriptorMapping( const TargetRegisterDescriptor& portPeripheralClockEnableRegisterDescriptor, const std::vector& portPeripheralDescriptors, diff --git a/src/Targets/RiscV/Wch/WchRiscV.hpp b/src/Targets/RiscV/Wch/WchRiscV.hpp index 6c57eba2..0180e43a 100644 --- a/src/Targets/RiscV/Wch/WchRiscV.hpp +++ b/src/Targets/RiscV/Wch/WchRiscV.hpp @@ -12,11 +12,16 @@ #include "WchRiscVTargetConfig.hpp" #include "TargetDescriptionFile.hpp" + #include "GpioPadDescriptor.hpp" +#include "Peripherals/Flash/FlashControlRegisterFields.hpp" +#include "Peripherals/Flash/FlashStatusRegisterFields.hpp" namespace Targets::RiscV::Wch { - class WchRiscV: public ::Targets::RiscV::RiscV + class WchRiscV + : public ::Targets::RiscV::RiscV + , public DeltaProgrammingInterface { public: WchRiscV(const TargetConfig& targetConfig, TargetDescriptionFile&& targetDescriptionFile); @@ -88,9 +93,10 @@ namespace Targets::RiscV::Wch const TargetRegisterDescriptor& flashKeyRegisterDescriptor; const TargetRegisterDescriptor& flashBootKeyRegisterDescriptor; const TargetRegisterDescriptor& flashStatusRegisterDescriptor; + const TargetRegisterDescriptor& flashControlRegisterDescriptor; - const TargetBitFieldDescriptor& flashStatusBootLockFieldDescriptor; - const TargetBitFieldDescriptor& flashStatusBootModeFieldDescriptor; + const Peripherals::Flash::FlashControlRegisterFields flashControlRegisterFields; + const Peripherals::Flash::FlashStatusRegisterFields flashStatusRegisterFields; const TargetPeripheralDescriptor rccPeripheralDescriptor; const TargetRegisterDescriptor& portPeripheralClockEnableRegisterDescriptor; @@ -109,6 +115,7 @@ namespace Targets::RiscV::Wch void unlockBootModeBitField(); void enableBootMode(); void enableUserMode(); + void eraseMainFlashSegment(); static std::map generateGpioPadDescriptorMapping( const TargetRegisterDescriptor& portPeripheralClockEnableRegisterDescriptor, diff --git a/src/Targets/TargetDescriptionFiles/RiscV/Wch/CH32V003.xml b/src/Targets/TargetDescriptionFiles/RiscV/Wch/CH32V003.xml index 3dc50e40..b8f91b03 100644 --- a/src/Targets/TargetDescriptionFiles/RiscV/Wch/CH32V003.xml +++ b/src/Targets/TargetDescriptionFiles/RiscV/Wch/CH32V003.xml @@ -1932,7 +1932,7 @@ - + diff --git a/src/Targets/TargetDescriptionFiles/RiscV/Wch/CH32X035.xml b/src/Targets/TargetDescriptionFiles/RiscV/Wch/CH32X035.xml index c2c5668e..5deab84c 100644 --- a/src/Targets/TargetDescriptionFiles/RiscV/Wch/CH32X035.xml +++ b/src/Targets/TargetDescriptionFiles/RiscV/Wch/CH32X035.xml @@ -3227,7 +3227,7 @@ - +