)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"2b4db9e4324e81c2443c527d9fc3f65110d4b967","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"372a8c33_9addbda7","updated":"2023-05-01 05:20:48.000000000","message":"Damn, just after sending the last reply I noticed we have very similar patch in gerrit.","commit_id":"7ffe55ed5911f1e2c9bf9044fcecabe2152aa3c4"},{"author":{"_account_id":1001023,"name":"Matthijs Kooijman","email":"matthijs@stdin.nl","username":"matthijskooijman"},"change_message_id":"e21ad799f12d7ecfbee147d9f96c6f0aad8a7edc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"c7b7b201_6cba7b3c","updated":"2023-05-01 11:05:46.000000000","message":"Heh, I looked around for existing patches before I started fixing this, but I\u0027m still getting used to gerrit, so I somehow thought that there was only 1 page of pending patches, so I just looked those over instead of using the search interface. Oh well :-)\n\nIn any case, this patch is very similar (in most places byte-for-byte identical) to my https://review.openocd.org/c/openocd/+/7612 and https://review.openocd.org/c/openocd/+/7613/2 patches combined, but is a bit more complete (and I think it\u0027s actually better in some places).\n\nThese are differences that should have been in my patch as well (so should be ported over if we merge my version).\n - Adds an extra assert to check `stm32x_otp_protect` is actually called with otp memory, which is just an extra sanity check.\n - Explicitly passes `FLASH_PSIZE_8`, which I overlooked in my patch (though its value is 0, which is why my code worked without it).\n - Actually waits for the BUSY bit to be cleared after writing each byte (which I guess happens fast enough that it worked for me without waiting, but waiting is definitely better).\n \nIn addition, Will\u0027s patch moves the `stm32x_is_otp_unlocked()` check for normal OTP writes to prevent small writes to bypass the check, which is a separate (but IMHO good) change. Will is also moving around the HALTED check for reasons I do not really understand, but it does not look wrong (just unnecessary to my untrained eye). I left an inline comment about this.\n\n\nOverall, based on reviewing the code, I think this is a good patch. The only criticism I can find is that it combines a couple of changes in a single patch that could be split out, but I know I can go a bit overboard when it comes to commit cleanliness and I don\u0027t know the policies of this project, so you should probably just ignore that :-)\n\nI would be fine with using this patch instead of my two patches (the other two patches I submitted are still useful but I expect they can be applied on top of this one too).\n\nSo +1 from me.\n\nI have not actually tested this patch yet. I can give it a whirl later this week, but my testing platform is the same STM32F401 that Will has also been using, so it might not add very much.","commit_id":"7ffe55ed5911f1e2c9bf9044fcecabe2152aa3c4"},{"author":{"_account_id":1001023,"name":"Matthijs Kooijman","email":"matthijs@stdin.nl","username":"matthijskooijman"},"change_message_id":"7e6f356ff7d278bb18d8c7d52fcb20d8a192d63c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"d7048298_0fe8dba9","updated":"2023-05-19 12:36:05.000000000","message":"I\u0027ve taken this code for a test drive, and it worked well.\n\nI\u0027ve cherry-picked this patch together with my https://review.openocd.org/c/openocd/+/7614 and https://review.openocd.org/c/openocd/+/7615/1 patches on top of current git master (30b0e9af8d1e68ee051ac62dd0e27c920fb396bd), which apply without conflicts.\n\nI\u0027ve tested locking a single sector, locking already locked sectors, locking a range of sectors, trying to lock without otp enable, trying to unlock, all of which behaved as expected.\n\nI\u0027ve been using an STM32F401 chip, with variants of this commandline:\n\n    src/openocd -s tcl -f interface/stlink.cfg -f target/stm32f4x.cfg -c \u0027init; reset init; flash info 1;  stm32f2x otp 1 enable; flash protect 1 0 1 on\u0027\n\nI\u0027ll go ahead and close my two patches that duplicate this one and would recommend merging this patch instead.\n\nThere is one comment from me still unresolved, but that is mostly a question for Will, not an actual problem with the code, so no need to block merging as far as I\u0027m concerned.","commit_id":"7ffe55ed5911f1e2c9bf9044fcecabe2152aa3c4"},{"author":{"_account_id":1000687,"name":"Tomas Vanek","display_name":"Tomas Vanek","email":"vanekt@fbl.cz","username":"vanekt"},"change_message_id":"2b4db9e4324e81c2443c527d9fc3f65110d4b967","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"8569191b_05f2e066","updated":"2023-05-01 05:20:48.000000000","message":"Very similar and independent\n7612: flash/nor/stm32f2x: Fix setting OTP sector protection | https://review.openocd.org/c/openocd/+/7612\nconfirms this fix.\nThis one still applies cleanly and the new chackpatch doesn\u0027t complain.\nLet\u0027s wait for Matthijs\u0027 review.","commit_id":"7ffe55ed5911f1e2c9bf9044fcecabe2152aa3c4"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"5f7fbd8669c2e73808ee7de0d323fe3cc59c3c51","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"5374a038_34634298","updated":"2022-11-19 07:29:42.000000000","message":"unable to test it, sorry.\nAdded Tarek as reviewer","commit_id":"7ffe55ed5911f1e2c9bf9044fcecabe2152aa3c4"},{"author":{"_account_id":1002057,"name":"Will Rigby","email":"will@breakfastny.com","username":"wrigby"},"change_message_id":"7ecd37323bf1071175d8651a97cc67b1a1199522","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"7cb1a5d4_28602b48","in_reply_to":"5374a038_34634298","updated":"2022-11-21 19:57:34.000000000","message":"No worries; thanks Antonio!\n\nTarek, let me know if you see anything I should do differently. Prior to this patchset, OpenOCD would just hang indefinitely when I tried to enable the OTP protection bits.","commit_id":"7ffe55ed5911f1e2c9bf9044fcecabe2152aa3c4"},{"author":{"_account_id":1002057,"name":"Will Rigby","email":"will@breakfastny.com","username":"wrigby"},"change_message_id":"184c9cb267967a8224802f2fff4a884a19b2ba3e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"3a97d3bd_0b5c3914","in_reply_to":"8569191b_05f2e066","updated":"2023-05-01 08:43:00.000000000","message":"Sounds good. For reference, this was tested on STM32F401 MCUs. It\u0027s been awhile since I wrote it, but happy to get my dev environment dusted off if any revisions are needed.","commit_id":"7ffe55ed5911f1e2c9bf9044fcecabe2152aa3c4"}],"src/flash/nor/stm32f2x.c":[{"author":{"_account_id":1001023,"name":"Matthijs Kooijman","email":"matthijs@stdin.nl","username":"matthijskooijman"},"change_message_id":"e21ad799f12d7ecfbee147d9f96c6f0aad8a7edc","unresolved":true,"context_lines":[{"line_number":689,"context_line":"\t\tLOG_ERROR(\"Target not halted\");"},{"line_number":690,"context_line":"\t\treturn ERROR_TARGET_NOT_HALTED;"},{"line_number":691,"context_line":"\t}"},{"line_number":692,"context_line":""},{"line_number":693,"context_line":"\t/* read protection settings */"},{"line_number":694,"context_line":"\tint retval \u003d stm32x_read_options(bank);"},{"line_number":695,"context_line":"\tif (retval !\u003d ERROR_OK) {"}],"source_content_type":"text/x-csrc","patch_set":3,"id":"b82a7f06_cc427797","line":692,"updated":"2023-05-01 11:05:46.000000000","message":"Why move this halted check down? Now you need to duplicate it in stm32x_otp_protect for no obvious reason?","commit_id":"7ffe55ed5911f1e2c9bf9044fcecabe2152aa3c4"}]}