From: Marek Vasut <marex@denx.de>
To: Austin Boyle <boyle.austin@gmail.com>
Cc: Gerlando Falauto <gerlando.falauto@keymile.com>,
Brian Norris <computersforpeace@gmail.com>,
linux-mtd@lists.infradead.org,
David Woodhouse <dwmw2@infradead.org>,
linux-kernel@vger.kernel.org,
Artem Bityutskiy <artem.bityutskiy@linux.intel.com>,
Angus Clark <angus.clark@st.com>
Subject: Re: [PATCH v2] mtd: m25p80: Calculate flash block protect bits based on number of sectors
Date: Sun, 13 Apr 2014 19:24:03 +0200 [thread overview]
Message-ID: <201404131924.03967.marex@denx.de> (raw)
In-Reply-To: <alpine.DEB.2.02.1404062115570.19515@austin-Satellite-L510>
On Sunday, April 06, 2014 at 01:19:59 PM, Austin Boyle wrote:
[...]
> @@ -752,24 +773,18 @@ static int m25p80_lock(struct mtd_info *mtd, loff_t
> ofs, uint64_t len)
>
> status_old = read_sr(flash);
>
> - if (offset < flash->mtd.size-(flash->mtd.size/2))
> - status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0;
> - else if (offset < flash->mtd.size-(flash->mtd.size/4))
> - status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
> - else if (offset < flash->mtd.size-(flash->mtd.size/8))
> - status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
> - else if (offset < flash->mtd.size-(flash->mtd.size/16))
> - status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
> - else if (offset < flash->mtd.size-(flash->mtd.size/32))
> - status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
> - else if (offset < flash->mtd.size-(flash->mtd.size/64))
> - status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
> - else
> - status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
> + for (lock_bits = 1; lock_bits < 7; lock_bits++) {
> + protected_area_start = flash->mtd.size -
> + get_protected_area(flash, lock_bits);
> + if (offset >= protected_area_start)
> + break;
> + }
> +
> + status_new = (status_old & ~SR_BP_BIT_MASK) |
> + ((lock_bits << SR_BP_BIT_OFFSET) & SR_BP_BIT_MASK);
This portion of code looks like a duplicate of ...
> /* Only modify protection if it will not unlock other areas */
> - if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) >
> - (status_old&(SR_BP2|SR_BP1|SR_BP0))) {
> + if ((status_new & SR_BP_BIT_MASK) > (status_old & SR_BP_BIT_MASK)) {
> write_enable(flash);
> if (write_sr(flash, status_new) < 0) {
> res = 1;
> @@ -786,6 +801,8 @@ static int m25p80_unlock(struct mtd_info *mtd, loff_t
> ofs, uint64_t len) struct m25p *flash = mtd_to_m25p(mtd);
> uint32_t offset = ofs;
> uint8_t status_old, status_new;
> + uint8_t lock_bits;
> + uint32_t protected_area_start;
> int res = 0;
>
> mutex_lock(&flash->lock);
> @@ -797,24 +814,19 @@ static int m25p80_unlock(struct mtd_info *mtd, loff_t
> ofs, uint64_t len)
>
> status_old = read_sr(flash);
>
> - if (offset+len > flash->mtd.size-(flash->mtd.size/64))
> - status_new = status_old & ~(SR_BP2|SR_BP1|SR_BP0);
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/32))
> - status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/16))
> - status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/8))
> - status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/4))
> - status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
> - else if (offset+len > flash->mtd.size-(flash->mtd.size/2))
> - status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
> - else
> - status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
> + for (lock_bits = 1; lock_bits < 7; lock_bits++) {
> + protected_area_start = flash->mtd.size -
> + get_protected_area(flash, lock_bits);
> + if (offset+len >= protected_area_start)
> + break;
> + }
> + lock_bits--;
> +
> + status_new = (status_old & ~SR_BP_BIT_MASK) |
> + ((lock_bits << SR_BP_BIT_OFFSET) & SR_BP_BIT_MASK);
... this here (if you don't consider the lock_bits--) . Why don't we move this
into a separate function to avoid the duplication?
> /* Only modify protection if it will not lock other areas */
> - if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) <
> - (status_old&(SR_BP2|SR_BP1|SR_BP0))) {
> + if ((status_new & SR_BP_BIT_MASK) < (status_old & SR_BP_BIT_MASK)) {
> write_enable(flash);
> if (write_sr(flash, status_new) < 0) {
> res = 1;
> @@ -826,6 +838,39 @@ err: mutex_unlock(&flash->lock);
> return res;
> }
>
> +static int m25p80_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t
> len) +{
> + struct m25p *flash = mtd_to_m25p(mtd);
> + uint32_t offset = ofs;
> + uint8_t status;
> + uint8_t lock_bits;
> + uint32_t protected_area_start;
> + int res;
> +
> + mutex_lock(&flash->lock);
> + /* Wait until finished previous command */
> + if (wait_till_ready(flash)) {
> + mutex_unlock(&flash->lock);
> + return -EBUSY;
> + }
> + status = read_sr(flash);
> + mutex_unlock(&flash->lock);
> +
> + lock_bits = ((status & SR_BP_BIT_MASK) >> SR_BP_BIT_OFFSET);
Excessive () , the outer ones are not needed.
> +
> + protected_area_start = flash->mtd.size -
> + get_protected_area(flash, lock_bits);
You might want to introduce some "get_protected_area_size()" here, since this is
the third time I see this call.
Otherwise nice, thanks for cleaning this mess up, it really helps !
next prev parent reply other threads:[~2014-04-13 22:02 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-04-06 11:19 [PATCH v2] mtd: m25p80: Calculate flash block protect bits based on number of sectors Austin Boyle
2014-04-13 17:24 ` Marek Vasut [this message]
2014-04-16 12:26 ` Austin Boyle
2014-04-16 12:30 ` Marek Vasut
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201404131924.03967.marex@denx.de \
--to=marex@denx.de \
--cc=angus.clark@st.com \
--cc=artem.bityutskiy@linux.intel.com \
--cc=boyle.austin@gmail.com \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=gerlando.falauto@keymile.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox