David,
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
Execution is more linear and without goto statement.
Moreover, it splits the first-last interval and should improve performance.
Best Regards,
Antonio Borneo
On Wed, Mar 10, 2010 at 10:31 AM, David Brownell <davi...@pacbell.net> wrote:
> On Tuesday 09 March 2010, David Brownell wrote:
>
> And tl clarify a bit:
>
>
>> On Monday 08 March 2010, Antonio Borneo wrote:
>> > Cannot protect or unprotect single sector in cfi flash.
>> > When first==last the procedure fails.
>> >
>> > Signed-off-by: Antonio Borneo <borneo.anto...@gmail.com>
>> > ---
>> > src/flash/nor/core.c | 4 ++--
>> > 1 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/src/flash/nor/core.c b/src/flash/nor/core.c
>> > index 767006d..8b581b0 100644
>> > --- a/src/flash/nor/core.c
>> > +++ b/src/flash/nor/core.c
>> > @@ -73,7 +73,7 @@ int flash_driver_protect(struct flash_bank *bank, int
>> > set, int first, int last)
>> > * speeds at least some things up.
>> > */
>> > scan:
>> > - for (int i = first; i < last; i++) {
>> > + for (int i = first; i <= last; i++) {
>>
>> OK
>
> ... because "first == last" is an OK loop entry state;
> it was explicitly allowed in the procedure parameter
> checks earlier, indicating that "single sector" case.
>
>
>>
>> > struct flash_sector *sector = bank->sectors + i;
>> >
>> > /* Only filter requests to protect the already-protected, or
>> > @@ -108,7 +108,7 @@ scan:
>> > }
>> >
>> > /* Single sector, already protected? Nothing to do! */
>> > - if (first == last)
>> > + if (first > last)
>>
>> ... not. the loop previously maintained the "first <= last" invariant,
>> and should still have done so. "first > last" is clearly an error case.
>
> ... "clearly" since the procedure parameter checks rejected that error.
> Also,
>
> - a comment in the loop pointed out the importance of not overrunning
> the end
>
> - the comment up top says "first == last" is the "single sector" case
>
> In short, this would add three internal inconsistencies...
>
>>
>>
>> > return ERROR_OK;
>> >
>> >
>> > --
>> > 1.5.2.2
> _______________________________________________
> Openocd-development mailing list
> Openocd-development@lists.berlios.de
> https://lists.berlios.de/mailman/listinfo/openocd-development
>
From 7896dd208161d0e01157bc4fc957dc75a911e7fd Mon Sep 17 00:00:00 2001
From: Antonio Borneo <borneo.anto...@gmail.com>
Date: Wed, 10 Mar 2010 11:14:13 +0800
Subject: [PATCH] CFI CORE: rewrite flash_driver_protect()
Simpler code to improve maintenance and readability.
Increased performance by skipping sectors that don't need to
modify protection.
Signed-off-by: Antonio Borneo <borneo.anto...@gmail.com>
---
src/flash/nor/core.c | 68 ++++++++++++++++++-------------------------------
1 files changed, 25 insertions(+), 43 deletions(-)
diff --git a/src/flash/nor/core.c b/src/flash/nor/core.c
index 8b581b0..d4260c6 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 ... */
@@ -71,54 +68,39 @@ int flash_driver_protect(struct flash_bank *bank, int set, int first, int last)
* 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.
+ *
+ * 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.
*/
-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)
+ for (; first <= last ; first++) {
+ int next;
+ int retval;
+
+ /* Skip sectors don't need change protection */
+ if (bank->sectors[first].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--;
+ for (next = first + 1; next <= last ; next++) {
+ /* Exit when next sector doesn't need change protection */
+ if (bank->sectors[next].is_protected == set)
+ break;
}
- }
-
- /* 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, next - 1);
+ if (retval != ERROR_OK)
+ {
+ LOG_ERROR("failed setting protection for areas %d to %d (%d)", first, next - 1, retval);
+ return retval;
+ }
- 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);
+ /* Reiterate from next chunk.
+ * Sector 'next' already tested and no need change */
+ first = next;
}
- return retval;
+ return ERROR_OK;
}
int flash_driver_write(struct flash_bank *bank,
--
1.5.2.2
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
