On Wed, Mar 10, 2010 at 5:26 PM, David Brownell <[email protected]> wrote:
> On Tuesday 09 March 2010, Antonio Borneo wrote:
>> I'm not sure I fully catch your points.
>> Anyway, I agree that code got even more confused and that comments are
>> not consistent with code anymore.
>>
>> Why not changing the whole procedure in a cleaner way?
>> I've put in attachment my proposal
>
> Could you reduce the gratuitous changes which just hide
> the content of your patch? Like shuffling comments, Such
> things just make patches hard to review.
Of course, in attachment.
>> Execution is more linear and without goto statement.
>
> YOu talk as if a well structured "goto" is a problem in itself.
> That's not true ... but if that bothers you, all you had to
> do was turn that into a "do { ... } while(updated)" loop.
>
> (And note that the reason for that additional loop was to handle
> changes affecting the core loop criteria ... a quick read
> didn't show you still addressing those issues.)
In the original code the "for" loop iterates across the whole
sectors[] array, while only the "first" and the "last" are really
important.
And every time "first" or "last" is changed, the array is scanned again.
Moreover, with original "for" boundaries, the condition "i == last" is
never true inside loop's body.
All such issues convinced me that code needs a deep review.
My first patch was just aimed at a quick fix without too much concern
about consistency between code and comments, looking for a better
code.
Apologise for increasing confusion.
>> Moreover, it splits the first-last interval and should improve performance.
>
> I don't follow what you mean by this. Do you mean it addresses
> the REVISIT comment by issuing multiple driver requests if it works
> out that there are discontiguous regions? That's not necessarily
> going to be faster... and such a change would have been more clearly
> done as a separate patch.
Yes, the idea is to include also the REVISIT comment in the final patch.
But also remove all the useless iterations on the array
Antonio
diff --git a/src/flash/nor/core.c b/src/flash/nor/core.c
index 8b581b0..6408fd4 100644
--- a/src/flash/nor/core.c
+++ b/src/flash/nor/core.c
@@ -53,9 +53,6 @@ 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 protect just that sector */
/* callers may not supply illegal parameters ... */
@@ -65,60 +62,33 @@ int flash_driver_protect(struct flash_bank *bank, int set, int first, int last)
/* force "set" to 0/1 */
set = !!set;
- /*
- * Filter out what trivial nonsense we can, so drivers don't have to.
- *
- * 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) {
- updated = true;
- first++;
- } else if (i == last) {
- 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! */
- if (first > last)
- return ERROR_OK;
-
-
- retval = bank->driver->protect(bank, set, first, last);
- if (retval != ERROR_OK)
- {
- LOG_ERROR("failed setting protection for areas %d to %d (%d)", first, last, retval);
- }
-
- return retval;
+ for (; first <= last ; first++) {
+ int next;
+ int retval;
+
+ /* Skip, until first sector that needs change protection */
+ if (bank->sectors[first].is_protected == set)
+ continue;
+
+ /* Search next sector that doesn't need change protection */
+ for (next = first + 1; next <= last ; next++) {
+ if (bank->sectors[next].is_protected == set)
+ break;
+ }
+
+ retval = bank->driver->protect(bank, set, first, next - 1);
+ if (retval != ERROR_OK)
+ {
+ LOG_ERROR("failed setting protection for areas %d to %d (%d)", first, next - 1, retval);
+ return retval;
+ }
+
+ /* Reiterate from next chunk.
+ * Sector 'next' already tested and doesn't need change */
+ first = next;
+ }
+
+ return ERROR_OK;
}
int flash_driver_write(struct flash_bank *bank,
_______________________________________________
Openocd-development mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/openocd-development
