hw/pflash_cfi02: Fix lazy reset of ROMD mode

When checking pfl->rom_mode for when to lazily reenter ROMD mode, the value was check was the opposite of what it should have been. This prevent the part from returning to ROMD mode after a write was made to the CFI rom region.

Signed-off-by Jordan Justen <[email protected]>

---
 hw/pflash_cfi02.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

hw/pflash_cfi02.c +1 -1

index 30c8aa4..370c5ee 100644
@@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
 
     DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
     ret = -1;
-    if (pfl->rom_mode) {
+    if (!pfl->rom_mode) {
         /* Lazy reset of to ROMD mode */
         if (pfl->wcycle == 0)
             pflash_register_memory(pfl, 1);

Signature · 1 line

1.7.1

When checking pfl->rom_mode for when to lazily reenter ROMD mode, the value was check was the opposite of what it should have been. This prevent the part from returning to ROMD mode after a write was made to the CFI rom region.

Signed-off-by Jordan Justen <[email protected]>

---
 hw/pflash_cfi02.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

hw/pflash_cfi02.c +0 -0

index 30c8aa4..370c5ee 100644

Quoted 16 lines · Jordan Justen

When checking pfl->rom_mode for when to lazily reenter ROMD mode, the value was check was the opposite of what it should have been. This prevent the part from returning to ROMD mode after a write was made to the CFI rom region.

Signed-off-by Jordan Justen <[email protected]>

---
 hw/pflash_cfi02.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

hw/pflash_cfi02.c +0 -0

index 30c8aa4..370c5ee 100644
@@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
 

hw/pflash_cfi02.c static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset, · line 118

Quoted 7 lines

hw/pflash_cfi02.c +1 -1

     DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
     ret = -1;
-    if (pfl->rom_mode) {
+    if (!pfl->rom_mode) {
         /* Lazy reset of to ROMD mode */
         if (pfl->wcycle == 0)
             pflash_register_memory(pfl, 1);

Indeed, that block looks weird to its author today as well. But inverting the logic completely defeats the purpose of lazy mode switching (will likely file a patch to remove the block). We should instead switch back using the timer.

Commit 5145b3d1cc revealed a bug in the lazy ROMD switch-back logic, but resolved it by breaking that feature. This approach addresses the issue by switching back to ROMD after a certain amount of read accesses without further unlock sequences.

Quoted 4 lines · Jan Kiszka

Commit 5145b3d1cc revealed a bug in the lazy ROMD switch-back logic, but resolved it by breaking that feature. This approach addresses the issue by switching back to ROMD after a certain amount of read accesses without further unlock sequences.

Without this change, the code will stay in flash mode until a single read occurs. The code sequence you are wanting to support using will issue a read before trying to unlock again?

Actually, I suppose it will want to verify the written data before moving on, so this does make sense.

Is the overhead of switching the modes significant enough to justify the lazy switch-back? (If so, maybe I'll look at this for cfi01 too.)

Quoted 17 lines · Jan Kiszka

Quoted 16 lines · Jordan Justen

When checking pfl->rom_mode for when to lazily reenter ROMD mode, the value was check was the opposite of what it should have been. This prevent the part from returning to ROMD mode after a write was made to the CFI rom region.

Signed-off-by Jordan Justen <[email protected]>

---
 hw/pflash_cfi02.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

hw/pflash_cfi02.c +0 -0

index 30c8aa4..370c5ee 100644
@@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
 

Quoted 12 lines

hw/pflash_cfi02.c static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset, · line 118

Quoted 7 lines

hw/pflash_cfi02.c +1 -1

     DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
     ret = -1;
-    if (pfl->rom_mode) {
+    if (!pfl->rom_mode) {
         /* Lazy reset of to ROMD mode */
         if (pfl->wcycle == 0)
             pflash_register_memory(pfl, 1);

Indeed, that block looks weird to its author today as well. But inverting the logic completely defeats the purpose of lazy mode switching (will likely file a patch to remove the block). We should instead switch back using the timer.

That was wishful thinking. We were actually lacking a switch-back on read, something that the old twisted logic was papering over. Patch below should resolve that more gracefully, at least it does so here.

Jan

------8<------

Commit 5145b3d1cc revealed a bug in the lazy ROMD switch-back logic, but resolved it by breaking that feature. This approach addresses the issue by switching back to ROMD after a certain amount of read accesses without further unlock sequences.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/pflash_cfi02.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

hw/pflash_cfi02.c +7 -4

index 370c5ee..14bbc34 100644
@@ -50,6 +50,8 @@ do {                                               \
 #define DPRINTF(fmt, ...) do { } while (0)
 #endif
 
+#define PFLASH_LAZY_ROMD_THRESHOLD 42
+
 struct pflash_t {
     BlockDriverState *bs;
     target_phys_addr_t base;

@@ -70,6 +72,7 @@ struct pflash_t {
     ram_addr_t off;
     int fl_mem;
     int rom_mode;
+    int read_counter; /* used for lazy switch-back to rom mode */
     void *storage;
 };
 

@@ -112,10 +115,10 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
 
     DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
     ret = -1;
-    if (!pfl->rom_mode) {
-        /* Lazy reset of to ROMD mode */
-        if (pfl->wcycle == 0)
-            pflash_register_memory(pfl, 1);
+    /* Lazy reset to ROMD mode after a certain amount of read accesses */
+    if (!pfl->rom_mode && pfl->wcycle == 0 &&
+        ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
+        pflash_register_memory(pfl, 1);
     }
     offset &= pfl->chip_len - 1;
     boff = offset & 0xFF;

Quoted 87 lines · Jan Kiszka

Quoted 17 lines · Jan Kiszka

Quoted 16 lines · Jordan Justen

When checking pfl->rom_mode for when to lazily reenter ROMD mode, the value was check was the opposite of what it should have been. This prevent the part from returning to ROMD mode after a write was made to the CFI rom region.

Signed-off-by Jordan Justen <[email protected]>

---
 hw/pflash_cfi02.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

hw/pflash_cfi02.c +0 -0

index 30c8aa4..370c5ee 100644
@@ -112,7 +112,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
 

Quoted 12 lines

hw/pflash_cfi02.c static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset, · line 118

Quoted 7 lines

hw/pflash_cfi02.c +1 -1

     DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
     ret = -1;
-    if (pfl->rom_mode) {
+    if (!pfl->rom_mode) {
         /* Lazy reset of to ROMD mode */
         if (pfl->wcycle == 0)
             pflash_register_memory(pfl, 1);

Indeed, that block looks weird to its author today as well. But inverting the logic completely defeats the purpose of lazy mode switching (will likely file a patch to remove the block). We should instead switch back using the timer.

That was wishful thinking. We were actually lacking a switch-back on read, something that the old twisted logic was papering over. Patch below should resolve that more gracefully, at least it does so here.

Jan

------8<------

Commit 5145b3d1cc revealed a bug in the lazy ROMD switch-back logic, but resolved it by breaking that feature. This approach addresses the issue by switching back to ROMD after a certain amount of read accesses without further unlock sequences.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/pflash_cfi02.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

hw/pflash_cfi02.c +7 -4

index 370c5ee..14bbc34 100644
@@ -50,6 +50,8 @@ do {                                               \
 #define DPRINTF(fmt, ...) do { } while (0)
 #endif
 
+#define PFLASH_LAZY_ROMD_THRESHOLD 42
+
 struct pflash_t {
     BlockDriverState *bs;
     target_phys_addr_t base;

@@ -70,6 +72,7 @@ struct pflash_t {
     ram_addr_t off;
     int fl_mem;
     int rom_mode;
+    int read_counter; /* used for lazy switch-back to rom mode */
     void *storage;
 };
 

@@ -112,10 +115,10 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
 
     DPRINTF("%s: offset " TARGET_FMT_plx "\n", __func__, offset);
     ret = -1;
-    if (!pfl->rom_mode) {
-        /* Lazy reset of to ROMD mode */
-        if (pfl->wcycle == 0)
-            pflash_register_memory(pfl, 1);
+    /* Lazy reset to ROMD mode after a certain amount of read accesses */
+    if (!pfl->rom_mode && pfl->wcycle == 0 &&
+        ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
+        pflash_register_memory(pfl, 1);
     }
     offset &= pfl->chip_len - 1;
     boff = offset & 0xFF;

@@ -254,6 +257,7 @@ static void pflash_write (pflash_t *pfl, target_phys_addr_t offset,
         /* Set the device in I/O access mode if required */

hw/pflash_cfi02.c static void pflash_write (pflash_t *pfl, target_phys_addr_t offset, · line 263

Quoted 6 lines

hw/pflash_cfi02.c +1 -0

         if (pfl->rom_mode)
             pflash_register_memory(pfl, 0);
+        pfl->read_counter = 0;
         /* We're in read mode */
     check_unlock0:
         if (boff == 0x55 && cmd == 0x98) {

Please apply unless concerns remain.

Indeed, that block looks weird to its author today as well. But inverting the logic completely defeats the purpose of lazy mode switching (will likely file a patch to remove the block).

We should
instead switch back using the timer.

So the question is: Did you actually see a problem that the flash was stuck in write mode, or did you just stumble over this strange code? In the former case, please explain the sequence or provide a trace.