flash: stop caching protection state
authorØyvind Harboe <oyvind.harboe@zylin.com>
Wed, 5 May 2010 13:08:34 +0000 (15:08 +0200)
committerØyvind Harboe <oyvind.harboe@zylin.com>
Wed, 5 May 2010 13:24:25 +0000 (15:24 +0200)
There are a million reasons why cached protection state might
be stale: power cycling of target, reset, code executing on
the target, etc.

The "flash protect_check" command is now gone. This is *always*
executed when running a "flash info".

As a bonus for more a more robust approach, lots of code could
be deleted.

Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
doc/openocd.texi
src/flash/nor/cfi.c
src/flash/nor/core.c
src/flash/nor/core.h
src/flash/nor/tcl.c
src/target/target.c

index d311c8eee4c9589175dd9450313356df807ea90d..a4c4de2922dc62b516108fbd19e3b69349fc04a7 100644 (file)
@@ -4129,9 +4129,8 @@ The @var{num} parameter is a value shown by @command{flash banks}.
 @deffn Command {flash info} num
 Print info about flash bank @var{num}
 The @var{num} parameter is a value shown by @command{flash banks}.
-The information includes per-sector protect status, which may be
-incorrect (outdated) unless you first issue a
-@command{flash protect_check num} command.
+This command will first query the hardware, it does not print cached
+and possibly stale information.
 @end deffn
 
 @anchor{flash protect}
@@ -4144,14 +4143,6 @@ specifies "to the end of the flash bank".
 The @var{num} parameter is a value shown by @command{flash banks}.
 @end deffn
 
-@deffn Command {flash protect_check} num
-Check protection state of sectors in flash bank @var{num}.
-The @var{num} parameter is a value shown by @command{flash banks}.
-@comment @option{flash erase_sector} using the same syntax.
-This updates the protection information displayed by @option{flash info}.
-(Code execution may have invalidated any state records kept by OpenOCD.)
-@end deffn
-
 @anchor{Flash Driver List}
 @section Flash Driver List
 As noted above, the @command{flash bank} command requires a driver name,
index 92b553b54bfeb56b52648047f6a180433cfe863f..4ef41b9ad72f6dcf437400841a10e28877225405 100644 (file)
@@ -852,6 +852,17 @@ static int cfi_intel_protect(struct flash_bank *bank, int set, int first, int la
         */
        if ((!set) && (!(pri_ext->feature_support & 0x20)))
        {
+               /* FIX!!! this code path is broken!!!
+                *
+                * The correct approach is:
+                *
+                * 1. read out current protection status
+                *
+                * 2. override read out protection status w/unprotected.
+                *
+                * 3. re-protect what should be protected.
+                *
+                */
                for (i = 0; i < bank->num_sectors; i++)
                {
                        if (bank->sectors[i].is_protected == 1)
index e07ca1f649d9f3ed3a73f2d7ac9e8c1f97c42e9b..936f07ca62caa0990dafccfdb4f7829d87d23641 100644 (file)
@@ -54,74 +54,27 @@ int flash_driver_erase(struct flash_bank *bank, int first, int last)
 int flash_driver_protect(struct flash_bank *bank, int set, int first, int last)
 {
        int retval;
-       bool updated = false;
-
-       /* NOTE: "first == last" means (un?)protect just that sector.
-        code including Lower level ddrivers may rely on this "first <= last"
-        * invariant.
-       */
 
        /* callers may not supply illegal parameters ... */
        if (first < 0 || first > last || last >= bank->num_sectors)
+       {
+               LOG_ERROR("illegal sector range");
                return ERROR_FAIL;
+       }
 
        /* force "set" to 0/1 */
        set = !!set;
 
-       /*
-        * Filter out what trivial nonsense we can, so drivers don't have to.
+       /* DANGER!
         *
-        * Don't tell drivers to change to the current state...  it's needless,
-        * and reducing the amount of work to be done (potentially to nothing)
-        * speeds at least some things up.
-        */
-scan:
-       for (int i = first; i <= last; i++) {
-               struct flash_sector *sector = bank->sectors + i;
-
-               /* Only filter requests to protect the already-protected, or
-                * to unprotect the already-unprotected.  Changing from the
-                * unknown state (-1) to a known one is unwise but allowed;
-                * protection status is best checked first.
-                */
-               if (sector->is_protected != set)
-                       continue;
-
-               /* Shrink this range of sectors from the start; don't overrun
-                * the end.  Also shrink from the end; don't overun the start.
-                *
-                * REVISIT we could handle discontiguous regions by issuing
-                * more than one driver request.  How much would that matter?
-                */
-               if (i == first && i != last) {
-                       updated = true;
-                       first++;
-               } else if (i == last && i != first) {
-                       updated = true;
-                       last--;
-               }
-       }
-
-       /* updating the range affects the tests in the scan loop above; so
-        * re-scan, to make sure we didn't miss anything.
-        */
-       if (updated) {
-               updated = false;
-               goto scan;
-       }
-
-       /* Single sector, already protected?  Nothing to do!
-        * We may have trimmed our parameters into this degenerate case.
+        * We must not use any cached information about protection state!!!!
         *
-        * FIXME repeating the "is_protected==set" test is a giveaway that
-        * this fast-exit belongs earlier, in the trim-it-down loop; mve.
-        * */
-       if (first == last && bank->sectors[first].is_protected == set)
-               return ERROR_OK;
-
-
-       /* Note that we don't pass illegal parameters to drivers; any
-        * trimming just turns one valid range into another one.
+        * There are a million things that could change the protect state:
+        *
+        * the target could have reset, power cycled, been hot plugged,
+        * the application could have run, etc.
+        *
+        * Drivers only receive valid sector range.
         */
        retval = bank->driver->protect(bank, set, first, last);
        if (retval != ERROR_OK)
@@ -754,34 +707,3 @@ int flash_write(struct target *target, struct image *image,
 {
        return flash_write_unlock(target, image, written, erase, false);
 }
-
-/**
- * Invalidates cached flash state which a target can change as it runs.
- *
- * @param target The target being resumed
- *
- * OpenOCD caches some flash state for brief periods.  For example, a sector
- * that is protected must be unprotected before OpenOCD tries to write it,
- * Also, a sector that's not erased must be erased before it's written.
- *
- * As a rule, OpenOCD and target firmware can both modify the flash, so when
- * a target starts running, OpenOCD needs to invalidate its cached state.
- */
-void nor_resume(struct target *target)
-{
-       struct flash_bank *bank;
-
-       for (bank = flash_banks; bank; bank = bank->next) {
-               int i;
-
-               if (bank->target != target)
-                       continue;
-
-               for (i = 0; i < bank->num_sectors; i++) {
-                       struct flash_sector *sector = bank->sectors + i;
-
-                       sector->is_erased = -1;
-                       sector->is_protected = -1;
-               }
-       }
-}
index b152677447912006f83b0b2f8896e2d8f8c49ae8..1dfd721be568fe29110c6650132c662f744379fc 100644 (file)
@@ -53,6 +53,10 @@ struct flash_sector
         * Indication of protection status: 0 = unprotected/unlocked,
         * 1 = protected/locked, other = unknown.  Set by
         * @c flash_driver_s::protect_check.
+        *
+        * This information must be considered stale immediately.
+        * A million things could make it stale: power cycle,
+        * reset of target, code running on target, etc.
         */
        int is_protected;
 };
@@ -124,9 +128,6 @@ int flash_unlock_address_range(struct target *target, uint32_t addr,
 int flash_write(struct target *target,
                struct image *image, uint32_t *written, int erase);
 
-/* invalidate cached state (targets may modify their own flash) */
-void nor_resume(struct target *target);
-
 /**
  * Forces targets to re-examine their erase/protection state.
  * This routine must be called when the system may modify the status.
index 947fd046bc012aba7e3e03ccb055e21d2adf1f7e..17c6e91037e20e1951ed548cd2ffb801b926df13 100644 (file)
@@ -70,6 +70,11 @@ COMMAND_HANDLER(handle_flash_info_command)
                if ((retval = p->driver->auto_probe(p)) != ERROR_OK)
                        return retval;
 
+               /* We must query the hardware to avoid printing stale information! */
+               retval = p->driver->protect_check(p);
+               if (retval != ERROR_OK)
+                       return retval;
+
                command_print(CMD_CTX,
                              "#%" PRIi32 " : %s at 0x%8.8" PRIx32 ", size 0x%8.8" PRIx32 ", buswidth %i, chipwidth %i",
                              i,
@@ -266,32 +271,6 @@ COMMAND_HANDLER(handle_flash_erase_address_command)
        return retval;
 }
 
-COMMAND_HANDLER(handle_flash_protect_check_command)
-{
-       if (CMD_ARGC != 1)
-               return ERROR_COMMAND_SYNTAX_ERROR;
-
-       struct flash_bank *p;
-       int retval = CALL_COMMAND_HANDLER(flash_command_get_bank, 0, &p);
-       if (ERROR_OK != retval)
-               return retval;
-
-       if ((retval = p->driver->protect_check(p)) == ERROR_OK)
-       {
-               command_print(CMD_CTX, "successfully checked protect state");
-       }
-       else if (retval == ERROR_FLASH_OPERATION_FAILED)
-       {
-               command_print(CMD_CTX, "checking protection state failed (possibly unsupported) by flash #%s at 0x%8.8" PRIx32, CMD_ARGV[0], p->base);
-       }
-       else
-       {
-               command_print(CMD_CTX, "unknown error when checking protection state of flash bank '#%s' at 0x%8.8" PRIx32, CMD_ARGV[0], p->base);
-       }
-
-       return ERROR_OK;
-}
-
 static int flash_check_sector_parameters(struct command_context *cmd_ctx,
                uint32_t first, uint32_t last, uint32_t num_sectors)
 {
@@ -706,14 +685,6 @@ static const struct command_registration flash_exec_command_handlers[] = {
                .help = "Check erase state of all blocks in a "
                        "flash bank.",
        },
-       {
-               .name = "protect_check",
-               .handler = handle_flash_protect_check_command,
-               .mode = COMMAND_EXEC,
-               .usage = "bank_id",
-               .help = "Check protection state of all blocks in a "
-                       "flash bank.",
-       },
        {
                .name = "erase_sector",
                .handler = handle_flash_erase_command,
index d17bb7445fa119b579a28ee15e9bedfbb2f35a38..37e515a647a467881d61b9f44650417d52b810f8 100644 (file)
@@ -474,19 +474,6 @@ int target_resume(struct target *target, int current, uint32_t address, int hand
        if ((retval = target->type->resume(target, current, address, handle_breakpoints, debug_execution)) != ERROR_OK)
                return retval;
 
-       /* Invalidate any cached protect/erase/... flash status, since
-        * almost all targets will now be able modify the flash by
-        * themselves.  We want flash drivers and infrastructure to
-        * be able to rely on (non-invalidated) cached state.
-        *
-        * For now we require that algorithms provided by OpenOCD are
-        * used only by code which properly maintains that  cached state.
-        * state
-        *
-        * REVISIT do the same for NAND ; maybe other flash flavors too...
-        */
-               if (!target->running_alg)
-               nor_resume(target);
        return retval;
 }