From: Paul Walmsley <paul.walmsley@sifive.com>
To: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
Cc: palmer@sifive.com, bbrezillon@kernel.org, richard@nod.at,
tudor.ambarus@microchip.com, linux-kernel@vger.kernel.org,
marek.vasut@gmail.com, linux-mtd@lists.infradead.org,
paul.walmsley@sifive.com, linux-riscv@lists.infradead.org,
computersforpeace@gmail.com, dwmw2@infradead.org
Subject: Re: [PATCH v2 3/3] mtd: spi-nor: add locking support for is25xxxxx device
Date: Tue, 30 Apr 2019 10:13:16 -0700 (PDT) [thread overview]
Message-ID: <alpine.DEB.2.21.9999.1904301004060.7063@viisi.sifive.com> (raw)
In-Reply-To: <1556474956-27786-4-git-send-email-sagar.kadam@sifive.com>
On Sun, 28 Apr 2019, Sagar Shrikant Kadam wrote:
> The locking scheme for ISSI devices is based on stm_lock mechanism.
> The is25xxxxx devices have 4 bits for selecting the range of blocks to
> be locked for write.
>
> The current implementation, blocks entire 512 blocks of flash memory.
>
> Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
> ---
> drivers/mtd/spi-nor/spi-nor.c | 60 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 81c7b3e..2dba7e9 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1459,6 +1459,65 @@ static int macronix_quad_enable(struct spi_nor *nor)
>
> return 0;
> }
> +/**
The above sequence indicates a kerneldoc-style comment, but the format of
the comment is not in kerneldoc format. Please fix this comment to
conform with
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/kernel-docs.rst
> + * Lock a region of the flash.Implementation is based on stm_lock
> + * Supports the block protection bits BP{0,1,2,3} in the status register
> + * Returns negative on errors, 0 on success.
> + */
> +static int issi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
Almost all of this function is copied and pasted from stm_lock(). Please
don't do this: it adds bloat, makes the code hard to maintain, and
increases the risk that fixes will only target one function rather than
both. Instead please pull the common code out into a separate static
function.
> + struct mtd_info *mtd = &nor->mtd;
> + int status_old, status_new;
> + u8 mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
> + u8 shift = ffs(mask) - 1, pow, val = 0;
> + loff_t lock_len;
> + bool use_top = true;
> +
> + status_old = read_sr(nor);
> +
> + if (status_old < 0)
> + return status_old;
> +
> + /* lock_len: length of region that should end up locked */
> + if (use_top)
> + lock_len = mtd->size - ofs;
> + else
> + lock_len = ofs + len;
> +
> + /*
> + * Need smallest pow such that:
> + *
> + * 1 / (2^pow) <= (len / size)
> + *
> + * so (assuming power-of-2 size) we do:
> + *
> + * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
> + */
> + pow = ilog2(mtd->size) - ilog2(lock_len);
> + val = mask - (pow << shift);
> +
> + if (val & ~mask)
> + return -EINVAL;
> +
> + /* Don't "lock" with no region! */
> + if (!(val & mask))
> + return -EINVAL;
> +
> + status_new = (status_old & ~mask & ~SR_TB) | val;
> +
> + /* Disallow further writes if WP pin is asserted */
> + status_new |= SR_SRWD;
> +
> + /* Don't bother if they're the same */
> + if (status_new == status_old)
> + return 0;
> +
> + /* Only modify protection if it will not unlock other areas */
> + if ((status_new & mask) < (status_old & mask))
> + return -EINVAL;
> +
> + return write_sr_and_check(nor, status_new, mask);
> +}
>
> /**
> * issi_unlock() - clear BP[0123] write-protection.
> @@ -4121,6 +4180,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> /* NOR protection support for ISSI chips */
> if (JEDEC_MFR(info) == SNOR_MFR_ISSI ||
> info->flags & SPI_NOR_HAS_LOCK) {
> + nor->flash_lock = issi_lock;
> nor->flash_unlock = issi_unlock;
>
> }
> --
> 1.9.1
>
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2019-04-30 17:13 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-28 18:09 [PATCH v2 0/3] add support for is25wp256 spi-nor device Sagar Shrikant Kadam
2019-04-28 18:09 ` [PATCH v2 1/3] mtd: spi-nor: add support for is25wp256 Sagar Shrikant Kadam
2019-04-30 17:03 ` Paul Walmsley
2019-05-02 9:50 ` Sagar Kadam
2019-04-28 18:09 ` [PATCH v2 2/3] mtd: spi-nor: add support to unlock flash device Sagar Shrikant Kadam
2019-04-30 17:19 ` Paul Walmsley
2019-05-02 9:56 ` Sagar Kadam
2019-04-28 18:09 ` [PATCH v2 3/3] mtd: spi-nor: add locking support for is25xxxxx device Sagar Shrikant Kadam
2019-04-30 17:13 ` Paul Walmsley [this message]
2019-04-30 17:19 ` Paul Walmsley
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=alpine.DEB.2.21.9999.1904301004060.7063@viisi.sifive.com \
--to=paul.walmsley@sifive.com \
--cc=bbrezillon@kernel.org \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-riscv@lists.infradead.org \
--cc=marek.vasut@gmail.com \
--cc=palmer@sifive.com \
--cc=richard@nod.at \
--cc=sagar.kadam@sifive.com \
--cc=tudor.ambarus@microchip.com \
/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;
as well as URLs for NNTP newsgroup(s).