)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1002344,"name":"Adrien Charruel","email":"acharruel@nanoxplore.com","username":"acharruel","status":"NanoXplore"},"change_message_id":"a472508fcbfc5eedd8f5e3768ff98535d9042561","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"51bb4f3f_df1bbfbf","updated":"2025-03-11 13:02:06.000000000","message":"Hi,\n\nI prepared a split of this patch in two separate gerrits.\nI will push them once the opened topics are resolved here to keep a nice history of the discussion.\n\nThanks.","commit_id":"917f9ed217000b2d8395037a94c234bb4b537a6d"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"7e8ab36741e0dc598105b39e2b72a47b689a098f","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"18931938_49e4cd2c","updated":"2025-03-09 18:38:48.000000000","message":"I would prefer having this patch split in two:\n- at first a patch for decoupling MMU and Cache management, as this is a general improvement;\n- then a patch for the extra flush/invalidate you are adding.\n\nI don\u0027t understand why you flush/invalidate BEFORE a cacheable read/write! It looks incorrect to me!\nOn armv{7,8,9}{A,R} OpenOCD has to use the CPU for every memory read/write.\nA cacheable read/write through the CPU can be done directly, with no need to flush/invalidate during this process, thus benefit in speed from the cache content.\nThe original logic in OpenOCD file `aarch64.c` (but also in `cortex_a.c`) is to flush/invalidate only when we switch from cacheable to uncacheable, and it take plenty of time, very annoying during debug (on my dual core Cortex-A35 it\u0027s in the order of one minute). I\u0027m already disappointed by such delay; this patch adds flush/invalidate also at every read/write cacheable!\n\nCan you please explain me the use case that requires this additional cache flush?","commit_id":"917f9ed217000b2d8395037a94c234bb4b537a6d"},{"author":{"_account_id":1002344,"name":"Adrien Charruel","email":"acharruel@nanoxplore.com","username":"acharruel","status":"NanoXplore"},"change_message_id":"a472508fcbfc5eedd8f5e3768ff98535d9042561","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"96d34987_c0e914ea","in_reply_to":"18931938_49e4cd2c","updated":"2025-03-11 13:02:06.000000000","message":"Thank you Antonio for your review.\n\nIf you confirm that `aarch64_write_memory` and `aarch64_read_memory` are indedd cacheable accesses, then we don\u0027t need to flush the cache. Agreed.\n\nHowever we believe that it\u0027s mandatory for `aarch64_read_phys_memory` and `aarch64_write_phys_memory`. What do you think?","commit_id":"917f9ed217000b2d8395037a94c234bb4b537a6d"},{"author":{"_account_id":1002344,"name":"Adrien Charruel","email":"acharruel@nanoxplore.com","username":"acharruel","status":"NanoXplore"},"change_message_id":"406ed731ffff1a4e9c6db362f84448aa6893eaca","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c27a3892_8238131f","in_reply_to":"73a45b5e_6c78f57a","updated":"2025-03-31 08:00:06.000000000","message":"Hi Antonio,\n\nSorry I forgot to describe our usecase. It is as follows:\n\nWe have a cortex-r52 based device with eRAM, and we are trying to:\n- Load a program using GDB+OpenOCD.\n- Run the program, which activates d-cache and i-cache.\n- Stop the first program.\n- Load a second program with GDB+OpenOCD.\n- Then we are unable to run this second program because the CPU didn\u0027t flush the caches.\n\nI can confirm the issue is solves with patch #8805, cache flush before \"phys\" accesses.\n\nMaybe the mechanism to load a program is done through another bus, as you suggested in your point 2. I can\u0027t really tell for now.\n\nThank you for your support.","commit_id":"917f9ed217000b2d8395037a94c234bb4b537a6d"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"f018e2be8b763b053457eaae2a2bee557a512dd0","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"73a45b5e_6c78f57a","in_reply_to":"96d34987_c0e914ea","updated":"2025-03-15 23:27:45.000000000","message":"No, the situation is different.\nThe read/write \"phys\" are at physical address, that means no virtual memory, MMU off!\nThe read/write \"non-phys\" are at virtual address, so MMU is on to make the translation.\n\nThe cache is (somehow) independent from MMU.\n\nIn the case of a CPU that has MMU (e.g. ARMv8A) and MMU is on:\n- \"non-phys\" read/write can be issued directly through the MMU and the cache. No need to flush cache or modify the MMU;\n- \"phys\" read/write require turning off the MMU. The data in cache are tagged to their virtual address and switching the MMU off we loose the relations between virtual and physical address of the data in cache. We have to flush all the cache before turning off the MMU to guarantee the data get stored at their virtual address.\n\nIn the case of a CPU without MMU (ARMv8R) or a CPU with MMU off, we are already accessing the memory at physical addresses. The data in cache are tagged to their physical address. There is no need to flush the cache.\n\nYou have not explained what is the use case that require the additional cache flush on this patch!\nI try to guess:\n\n1) you want to write a register in a peripheral (a GPIO bit to turn on a LED on a pin). If the register is cached and the cache is write-back, the write goes in cache and only later will be written in the register. Result is that the write does not immediately change the LED state.\nIn this case the error is that the address should be non-cacheable because HW register should not be cached.\n\n2) you are accessing at the same time your memory through the CPU (with cache on) and through another bus or peripheral that do not snoop the CPU cache.\nIn this case it is \"normal\" that a write from one side is not visible on the other side, because of the cache. The code in the CPU should force some flush and invalidate to synchronize the cache with the memory and permit a correct exchange with the other actor.\nMaybe you would like to have an OpenOCD \"cache flush\" command, as it is present in other targets (e.g. `esirisc flush_caches`, cortex-a `cache l1 d flush_all`) to force some synchronization during debug for this case. But this is a specific use case, don\u0027t change the generic read/write code for this case only as it would degrade the overall performance of OpenOCD.","commit_id":"917f9ed217000b2d8395037a94c234bb4b537a6d"},{"author":{"_account_id":1002344,"name":"Adrien Charruel","email":"acharruel@nanoxplore.com","username":"acharruel","status":"NanoXplore"},"change_message_id":"dd7d28060658c6059477ffdbe2cd7fc475035d22","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"de70fb1f_a8ca5b11","in_reply_to":"c27a3892_8238131f","updated":"2025-03-31 08:01:51.000000000","message":"Hi Antonio,\n\nI just realiased today that my answer was pending in gerrit.\nI\u0027m still a little confused with gerrit UI and I didn\u0027t hit reply. Sorry for the delay and thanks for your review!","commit_id":"917f9ed217000b2d8395037a94c234bb4b537a6d"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"478ad64065342ff06083ab578a7553c05eb8d82c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"f10579b7_bde4f5a3","updated":"2025-03-15 17:35:50.000000000","message":"minor fix below.\nThanks for this code reorganization","commit_id":"372e96424e979e43a803bdd040d90bc25b20b5ee"}],"src/target/aarch64.c":[{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"7e8ab36741e0dc598105b39e2b72a47b689a098f","unresolved":true,"context_lines":[{"line_number":68,"context_line":"\tswitch (armv8-\u003earm.core_mode) {"},{"line_number":69,"context_line":"\tcase ARMV8_64_EL0T:"},{"line_number":70,"context_line":"\t\ttarget_mode \u003d ARMV8_64_EL1H;"},{"line_number":71,"context_line":"\t/* fall through */"},{"line_number":72,"context_line":"\tcase ARMV8_64_EL1T:"},{"line_number":73,"context_line":"\tcase ARMV8_64_EL1H:"},{"line_number":74,"context_line":"\t\tinstr \u003d ARMV8_MSR_GP(SYSTEM_SCTLR_EL1, 0);"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"f361812c_d23032b0","line":71,"updated":"2025-03-09 18:38:48.000000000","message":"don\u0027t change the indentation. The comment is part of the `case` block above and replaces the statement `break;`","commit_id":"917f9ed217000b2d8395037a94c234bb4b537a6d"},{"author":{"_account_id":1002344,"name":"Adrien Charruel","email":"acharruel@nanoxplore.com","username":"acharruel","status":"NanoXplore"},"change_message_id":"a472508fcbfc5eedd8f5e3768ff98535d9042561","unresolved":false,"context_lines":[{"line_number":68,"context_line":"\tswitch (armv8-\u003earm.core_mode) {"},{"line_number":69,"context_line":"\tcase ARMV8_64_EL0T:"},{"line_number":70,"context_line":"\t\ttarget_mode \u003d ARMV8_64_EL1H;"},{"line_number":71,"context_line":"\t/* fall through */"},{"line_number":72,"context_line":"\tcase ARMV8_64_EL1T:"},{"line_number":73,"context_line":"\tcase ARMV8_64_EL1H:"},{"line_number":74,"context_line":"\t\tinstr \u003d ARMV8_MSR_GP(SYSTEM_SCTLR_EL1, 0);"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"fd85503f_503a6dcb","line":71,"in_reply_to":"f361812c_d23032b0","updated":"2025-03-11 13:02:06.000000000","message":"Ack","commit_id":"917f9ed217000b2d8395037a94c234bb4b537a6d"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"7e8ab36741e0dc598105b39e2b72a47b689a098f","unresolved":true,"context_lines":[{"line_number":104,"context_line":""},{"line_number":105,"context_line":"\tretval \u003d armv8-\u003edpm.instr_write_data_r0_64(\u0026armv8-\u003edpm, instr, system_control_reg);"},{"line_number":106,"context_line":"\tif (retval !\u003d ERROR_OK)"},{"line_number":107,"context_line":"\t\treturn retval;"},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"\tif (target_mode !\u003d ARM_MODE_ANY)"},{"line_number":110,"context_line":"\t\tarmv8_dpm_modeswitch(\u0026armv8-\u003edpm, ARM_MODE_ANY);"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"8804671c_07bee246","line":107,"updated":"2025-03-09 18:38:48.000000000","message":"No, the logic here is to NOT return immediately but only AFTER setting back\n`armv8_dpm_modeswitch(\u0026armv8-\u003edpm, ARM_MODE_ANY);`\n\nActually that\u0027s not you fault. The original code in `aarch64_restore_system_control_reg()` had this `return`, but not the similar code in `aarch64_mmu_modify()`.\nLet\u0027s use this patch to fix it","commit_id":"917f9ed217000b2d8395037a94c234bb4b537a6d"},{"author":{"_account_id":1002344,"name":"Adrien Charruel","email":"acharruel@nanoxplore.com","username":"acharruel","status":"NanoXplore"},"change_message_id":"a472508fcbfc5eedd8f5e3768ff98535d9042561","unresolved":false,"context_lines":[{"line_number":104,"context_line":""},{"line_number":105,"context_line":"\tretval \u003d armv8-\u003edpm.instr_write_data_r0_64(\u0026armv8-\u003edpm, instr, system_control_reg);"},{"line_number":106,"context_line":"\tif (retval !\u003d ERROR_OK)"},{"line_number":107,"context_line":"\t\treturn retval;"},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"\tif (target_mode !\u003d ARM_MODE_ANY)"},{"line_number":110,"context_line":"\t\tarmv8_dpm_modeswitch(\u0026armv8-\u003edpm, ARM_MODE_ANY);"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"9ac51666_5be980b0","line":107,"in_reply_to":"8804671c_07bee246","updated":"2025-03-11 13:02:06.000000000","message":"Ack","commit_id":"917f9ed217000b2d8395037a94c234bb4b537a6d"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"478ad64065342ff06083ab578a7553c05eb8d82c","unresolved":true,"context_lines":[{"line_number":94,"context_line":"\t\tbreak;"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"\tdefault:"},{"line_number":97,"context_line":"\t\tLOG_ERROR(\"cannot read system control register in this mode: (%s : 0x%x)\","},{"line_number":98,"context_line":"\t\t\t\tarmv8_mode_name(armv8-\u003earm.core_mode), armv8-\u003earm.core_mode);"},{"line_number":99,"context_line":"\t\t\treturn ERROR_FAIL;"},{"line_number":100,"context_line":"\t}"}],"source_content_type":"text/x-csrc","patch_set":4,"id":"964b2023_595a6ad9","line":97,"updated":"2025-03-15 17:35:50.000000000","message":"\"read\" ? this function is named `aarch64_write_system_control_reg()`","commit_id":"372e96424e979e43a803bdd040d90bc25b20b5ee"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"478ad64065342ff06083ab578a7553c05eb8d82c","unresolved":false,"context_lines":[{"line_number":122,"context_line":"\t\tretval \u003d aarch64_write_system_control_reg(target, aarch64-\u003esystem_control_reg_curr);"},{"line_number":123,"context_line":"\t}"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"\treturn retval;"},{"line_number":126,"context_line":"}"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"/*  modify system_control_reg in order to enable or disable mmu for :"}],"source_content_type":"text/x-csrc","patch_set":4,"id":"2330d5af_57640b04","line":125,"updated":"2025-03-15 17:35:50.000000000","message":"Let\u0027s keep this patch as is, because it is just a code reorganization.\n\nBut I think we would need a new patch to rewrite this function differently to fix a potential issue.\nIf `aarch64_write_system_control_reg()` returns error, it probably means that the system register has not been updated. Assigning `aarch64-\u003esystem_control_reg_curr` doesn\u0027t look correct.\n```\n\tif (aarch64-\u003esystem_control_reg \u003d\u003d aarch64-\u003esystem_control_reg_curr)\n\t\treturn ERROR_OK;\n\t\n\tretval \u003d aarch64_write_system_control_reg(target, aarch64-\u003esystem_control_reg);\n\tif (retval \u003d\u003d ERROR_OK)\n\t\taarch64-\u003esystem_control_reg_curr \u003d aarch64-\u003esystem_control_reg;\n\treturn retval;\n}\n```\n\nSame below in `aarch64_mmu_modify()`","commit_id":"372e96424e979e43a803bdd040d90bc25b20b5ee"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"478ad64065342ff06083ab578a7553c05eb8d82c","unresolved":false,"context_lines":[{"line_number":137,"context_line":""},{"line_number":138,"context_line":"\tif (enable) {"},{"line_number":139,"context_line":"\t\t/*\tif mmu enabled at target stop and mmu not enable */"},{"line_number":140,"context_line":"\t\tif (!(system_control_reg_curr_updated \u0026 0x1U)) {"},{"line_number":141,"context_line":"\t\t\tLOG_ERROR(\"trying to enable mmu on target stopped with mmu disable\");"},{"line_number":142,"context_line":"\t\t\treturn ERROR_FAIL;"},{"line_number":143,"context_line":"\t\t}"}],"source_content_type":"text/x-csrc","patch_set":4,"id":"3b30126f_c4bc6156","line":140,"updated":"2025-03-15 17:35:50.000000000","message":"Not in this patch, but after this get merged we would need to drop these hardcoded bit values in use today for SCTLR register and use macros instead, e.g.:\n```\n#define SCTLR_M BIT(0)\n#define SCTLR_C BIT(2)\n#define SCTLR_I BIT(12)\n```","commit_id":"372e96424e979e43a803bdd040d90bc25b20b5ee"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"478ad64065342ff06083ab578a7553c05eb8d82c","unresolved":true,"context_lines":[{"line_number":141,"context_line":"\t\t\tLOG_ERROR(\"trying to enable mmu on target stopped with mmu disable\");"},{"line_number":142,"context_line":"\t\t\treturn ERROR_FAIL;"},{"line_number":143,"context_line":"\t\t}"},{"line_number":144,"context_line":"\t\tif (!(system_control_reg_curr_updated \u0026 0x1U))"},{"line_number":145,"context_line":"\t\t\tsystem_control_reg_curr_updated |\u003d 0x1U;"},{"line_number":146,"context_line":"\t} else {"},{"line_number":147,"context_line":"\t\tif ((system_control_reg_curr_updated \u0026 0x1U))"}],"source_content_type":"text/x-csrc","patch_set":4,"id":"74fff77a_8921cb18","line":144,"updated":"2025-03-15 17:35:50.000000000","message":"No need for this check, directly assign the value\n`system_control_reg_curr_updated |\u003d 0x1U;`","commit_id":"372e96424e979e43a803bdd040d90bc25b20b5ee"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"478ad64065342ff06083ab578a7553c05eb8d82c","unresolved":true,"context_lines":[{"line_number":144,"context_line":"\t\tif (!(system_control_reg_curr_updated \u0026 0x1U))"},{"line_number":145,"context_line":"\t\t\tsystem_control_reg_curr_updated |\u003d 0x1U;"},{"line_number":146,"context_line":"\t} else {"},{"line_number":147,"context_line":"\t\tif ((system_control_reg_curr_updated \u0026 0x1U))"},{"line_number":148,"context_line":"\t\t\tsystem_control_reg_curr_updated \u0026\u003d ~0x1U;"},{"line_number":149,"context_line":"\t}"},{"line_number":150,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":4,"id":"08fd0c0a_731199bb","line":147,"updated":"2025-03-15 17:35:50.000000000","message":"No need for this check, directly assign the value\n`system_control_reg_curr_updated \u0026\u003d ~0x1U;`","commit_id":"372e96424e979e43a803bdd040d90bc25b20b5ee"}]}