| Message ID | 20210413120210.3671536-1-ikjn@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Vignesh R |
| Headers | show |
| Series | mtd: spi-nor: macronix: Add block protection support to mx25u6435f | expand |
Hi Ikjoon, Am 2021-04-13 14:02, schrieb Ikjoon Jang: > This patch adds block protection support to Macronix mx25u6432f and > mx25u6435f. Two different chips share the same JEDEC ID while only > mx25u6423f support section protections. And two chips have slightly > different definitions of BP bits than generic (ST Micro) > implementation. What is different compared to the current implementation? Could you give an example? > So this patch defines a new spi_nor_locking_ops only for macronix > until this could be merged into a generic swp implementation. TBH, I don't really like the code duplication and I'd presume that it won't ever be merged with the generic code. You also assume that both the WPSEL and T/B bit are 0, which might not be true. Please note that both are write-once, thus should only be read. See also: https://lore.kernel.org/linux-mtd/346332bf6ab0dd92b9ffd9e126b6b97c@walle.cc/ -michael
HI Michael, thanks for the review. On Tue, Apr 13, 2021 at 8:26 PM Michael Walle <michael@walle.cc> wrote: > > Hi Ikjoon, > > Am 2021-04-13 14:02, schrieb Ikjoon Jang: > > This patch adds block protection support to Macronix mx25u6432f and > > mx25u6435f. Two different chips share the same JEDEC ID while only > > mx25u6423f support section protections. And two chips have slightly > > different definitions of BP bits than generic (ST Micro) > > implementation. > > What is different compared to the current implementation? Could you give > an example? Two chips have different definitions on BP and T/B bits. - 35f has 4 BPs without T/B, BP3 behaves like T/B bit. - 32f has 4 BPs plus T/B on CR. # MX25U6435F BPs | BP3=0 | BP3=1 --------------------- 000 | None | 1/2 001 | 1/128 | 3/4 010 | 1/64 | 7/8 011 | 1/32 | 15/16 100 | 1/16 | 31/32 101 | 1/8 | 63/64 110 | 1/4 | 127/128 111 | 1/2 | All # MX25U6432F BPs | T/B=0 | T/B=1 --------------------- 0000 | None | None 0001 | 1/128 | 1/128 0010 | 1/64 | 1/64 0011 | 1/32 | 1/32 0100 | 1/16 | 1/16 0101 | 1/8 | 1/8 0110 | 1/4 | 1/4 0111 | 1/2 | 1/2 1xxx | All | All It seems that 35f has a unique definition on bottom protections than others. Assuming there's no way to distinguish between mx25u6435f and 32f, This patch simply takes the common parts only - BP[2:0] without using T/B or BP3 at all. But the current swp implementation implies that "BP with all ones" is to be 'all' protection while in this approach it's 1/2, A hidden BP3 should be involved for 'all' protection. > > > So this patch defines a new spi_nor_locking_ops only for macronix > > until this could be merged into a generic swp implementation. > > TBH, I don't really like the code duplication and I'd presume that it > won't ever be merged with the generic code. Agree, I hope I could make a more generalized version into swp.c. Honestly I expected that I just needed to add one line of SPI_NOR_HAS_LOCK to flash_info to support mx256432f (this was the main purpose of my patch) before I read the datasheets. :) > > You also assume that both the WPSEL and T/B bit are 0, which might not > be true. Please note that both are write-once, thus should only be read. yep, that also should be considered, I'm thinking just not to support WPSEL=1 case for now. > > See also: > https://lore.kernel.org/linux-mtd/346332bf6ab0dd92b9ffd9e126b6b97c@walle.cc/ > Thanks, let me try it. > -michael
Hi, Am 2021-04-14 08:53, schrieb Ikjoon Jang: > On Tue, Apr 13, 2021 at 8:26 PM Michael Walle <michael@walle.cc> wrote: >> Am 2021-04-13 14:02, schrieb Ikjoon Jang: >> > This patch adds block protection support to Macronix mx25u6432f and >> > mx25u6435f. Two different chips share the same JEDEC ID while only >> > mx25u6423f support section protections. And two chips have slightly >> > different definitions of BP bits than generic (ST Micro) >> > implementation. >> >> What is different compared to the current implementation? Could you >> give >> an example? > > Two chips have different definitions on BP and T/B bits. > - 35f has 4 BPs without T/B, BP3 behaves like T/B bit. See below. > - 32f has 4 BPs plus T/B on CR. Ok, so this scheme looks like what we have right now, only that the TB bit is OTP and at a different place, right? > > # MX25U6435F > > BPs | BP3=0 | BP3=1 > --------------------- > 000 | None | 1/2 > 001 | 1/128 | 3/4 > 010 | 1/64 | 7/8 > 011 | 1/32 | 15/16 > 100 | 1/16 | 31/32 > 101 | 1/8 | 63/64 > 110 | 1/4 | 127/128 > 111 | 1/2 | All > > # MX25U6432F > > BPs | T/B=0 | T/B=1 > --------------------- > 0000 | None | None > 0001 | 1/128 | 1/128 > 0010 | 1/64 | 1/64 > 0011 | 1/32 | 1/32 > 0100 | 1/16 | 1/16 > 0101 | 1/8 | 1/8 > 0110 | 1/4 | 1/4 > 0111 | 1/2 | 1/2 > 1xxx | All | All > > It seems that 35f has a unique definition on bottom protections than > others. Oh my.. That looks more like an invert and the top protection is also different. That is, if you'd treat BP3 as T/B, then BP[2:0] = 111 should be "lock all", but it is rather lock half.. I just looked at the mx25u3235f back then. There it looked correct. But now it looks like the top protection scheme clips on the lower end (i.e. always starts with 1 block), where on the current supported scheme, we clip on the upper end (i.e. we start with protect all and walk backwards). > Assuming there's no way to distinguish between mx25u6435f and 32f, > This patch simply takes the common parts only - BP[2:0] > without using T/B or BP3 at all. You could look for differences in the SFDP and then distiguish them during probe and set the corresponding flags. Where the flags would need to be implemented first. I wouldn't have a problem with saying we just support top protection for the MX25U6435F but then we'd need to make sure the BP3 is set to 0. If you want to read out the SFDP, see here: https://patchwork.ozlabs.org/project/linux-mtd/list/?series=235475 > But the current swp implementation implies that "BP with all ones" > is to be 'all' protection while in this approach it's 1/2, > A hidden BP3 should be involved for 'all' protection. Yes, so the MX25U6432F needs to have the 4bit BP flag set and the MX25U6435F seems to be completely different. Doh.. >> > So this patch defines a new spi_nor_locking_ops only for macronix >> > until this could be merged into a generic swp implementation. >> >> TBH, I don't really like the code duplication and I'd presume that it >> won't ever be merged with the generic code. > > Agree, I hope I could make a more generalized version into swp.c. > > Honestly I expected that I just needed to add one line of > SPI_NOR_HAS_LOCK > to flash_info to support mx256432f (this was the main purpose of my > patch) > before I read the datasheets. :) > >> >> You also assume that both the WPSEL and T/B bit are 0, which might not >> be true. Please note that both are write-once, thus should only be >> read. > > yep, that also should be considered, > I'm thinking just not to support WPSEL=1 case for now. which is ok, but we should make sure it is not set at all. Now the question is what do we do if it is set? I'd say just don't register the locking ops and log a message. -michael
On Wed, Apr 14, 2021 at 12:53:24PM +0200, Michael Walle wrote: > Hi, > > Am 2021-04-14 08:53, schrieb Ikjoon Jang: > > On Tue, Apr 13, 2021 at 8:26 PM Michael Walle <michael@walle.cc> wrote: > > > Am 2021-04-13 14:02, schrieb Ikjoon Jang: > > > > This patch adds block protection support to Macronix mx25u6432f and > > > > mx25u6435f. Two different chips share the same JEDEC ID while only > > > > mx25u6423f support section protections. And two chips have slightly > > > > different definitions of BP bits than generic (ST Micro) > > > > implementation. > > > > > > What is different compared to the current implementation? Could you > > > give > > > an example? > > > > Two chips have different definitions on BP and T/B bits. > > - 35f has 4 BPs without T/B, BP3 behaves like T/B bit. > > See below. > > > - 32f has 4 BPs plus T/B on CR. > > Ok, so this scheme looks like what we have right now, only that the > TB bit is OTP and at a different place, right? yes, right. > > > > > # MX25U6435F > > > > BPs | BP3=0 | BP3=1 > > --------------------- > > 000 | None | 1/2 > > 001 | 1/128 | 3/4 > > 010 | 1/64 | 7/8 > > 011 | 1/32 | 15/16 > > 100 | 1/16 | 31/32 > > 101 | 1/8 | 63/64 > > 110 | 1/4 | 127/128 > > 111 | 1/2 | All > > > > # MX25U6432F > > > > BPs | T/B=0 | T/B=1 > > --------------------- > > 0000 | None | None > > 0001 | 1/128 | 1/128 > > 0010 | 1/64 | 1/64 > > 0011 | 1/32 | 1/32 > > 0100 | 1/16 | 1/16 > > 0101 | 1/8 | 1/8 > > 0110 | 1/4 | 1/4 > > 0111 | 1/2 | 1/2 > > 1xxx | All | All > > > > It seems that 35f has a unique definition on bottom protections than > > others. > > Oh my.. That looks more like an invert and the top protection is also > different. That is, if you'd treat BP3 as T/B, then BP[2:0] = 111 > should be "lock all", but it is rather lock half.. I just looked at > the mx25u3235f back then. There it looked correct. But now it looks > like the top protection scheme clips on the lower end (i.e. always > starts with 1 block), where on the current supported scheme, we clip > on the upper end (i.e. we start with protect all and walk backwards). > Yes, indeed, but it's still confusing. :) Now I'm thinking of adding a 'table' which maps between BP mask and lock_len in swp.c, instead of ilog2() calculations. > > Assuming there's no way to distinguish between mx25u6435f and 32f, > > This patch simply takes the common parts only - BP[2:0] > > without using T/B or BP3 at all. > > You could look for differences in the SFDP and then distiguish them > during probe and set the corresponding flags. Where the flags would > need to be implemented first. I wouldn't have a problem with saying > we just support top protection for the MX25U6435F but then we'd need > to make sure the BP3 is set to 0. > > If you want to read out the SFDP, see here: > https://patchwork.ozlabs.org/project/linux-mtd/list/?series=235475 you're right, those chips have different values - JESD216 and JESD216B. > > > But the current swp implementation implies that "BP with all ones" > > is to be 'all' protection while in this approach it's 1/2, > > A hidden BP3 should be involved for 'all' protection. > > Yes, so the MX25U6432F needs to have the 4bit BP flag set and the > MX25U6435F seems to be completely different. Doh.. > > > > > So this patch defines a new spi_nor_locking_ops only for macronix > > > > until this could be merged into a generic swp implementation. > > > > > > TBH, I don't really like the code duplication and I'd presume that it > > > won't ever be merged with the generic code. > > > > Agree, I hope I could make a more generalized version into swp.c. > > > > Honestly I expected that I just needed to add one line of > > SPI_NOR_HAS_LOCK > > to flash_info to support mx256432f (this was the main purpose of my > > patch) > > before I read the datasheets. :) > > > > > > > > You also assume that both the WPSEL and T/B bit are 0, which might not > > > be true. Please note that both are write-once, thus should only be > > > read. > > > > yep, that also should be considered, > > I'm thinking just not to support WPSEL=1 case for now. > > which is ok, but we should make sure it is not set at all. Now the > question is what do we do if it is set? I'd say just don't register the > locking ops and log a message. > So the my current rough plan to support macronix's HAS_LOCK is: a. respin your patches supporting macronix TB (OTP + CR) b. fix swp.c to have a mapping between BP_mask and lock_len (or similar approach) to support unlinear/assymetric protection levels of MX25U35F, instead of calling ilog2() c. Distinguish MX25U35F and MX25U32F at runtime and apply flags only when WPSEL==0 && SFDP==MX25U32F set HAS_LOCK | OTP_TB | CR_TB | HAS_4BIT_BP. (it's the only MX25U32F I can test the new features for now). I'm not so sure on b part although.. > -michael Super thanks for the review!
diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c index 42c2cf31702e..563005830e46 100644 --- a/drivers/mtd/spi-nor/macronix.c +++ b/drivers/mtd/spi-nor/macronix.c @@ -8,6 +8,195 @@ #include "core.h" +/* + * mx25u6435f/mx25u6432f common protection table: + * + * mx25u6432f has T/B bit, but mx25u6435f doesn't. + * while both chips have the same JEDEC ID, + * Also BP bits are slightly different with generic swp. + * So here we only use common part of the BPs definitions. + * + * - Upper 2^(Prot Level - 1) blocks are protected. + * - Block size is hardcoded as 64Kib. + * - Assume T/B is always 0 (top protected, factory default). + * + * BP3| BP2 | BP1 | BP0 | Prot Level + * ----------------------------------- + * 0 | 0 | 0 | 0 | NONE + * 0 | 0 | 0 | 1 | 1 + * 0 | 0 | 1 | 0 | 2 + * 0 | 0 | 1 | 1 | 3 + * 0 | 1 | 0 | 0 | 4 + * 0 | 1 | 0 | 1 | 5 + * 0 | 1 | 1 | 0 | 6 + * 0 | 1 | 1 | 1 | 7 + * .....................| differ by 35f/32f, not used + * 1 | 1 | 1 | 1 | ALL + */ + +#define MX_BP_MASK (SR_BP0 | SR_BP1 | SR_BP2 | SR_BP3) +#define MX_BP_SHIFT (SR_BP_SHIFT) + +static int mx_get_locked_len(struct spi_nor *nor, u8 sr, uint64_t *lock_len) +{ + struct mtd_info *mtd = &nor->mtd; + u8 bp; + + bp = (sr & MX_BP_MASK) >> MX_BP_SHIFT; + + if (bp == 0xf) { + /* protected all */ + *lock_len = mtd->size; + return 0; + } + + /* sorry, not yet supported */ + if (bp > 0x7) + return -EOPNOTSUPP; + + /* block size = 64Kib */ + *lock_len = bp ? (0x8000 << bp) : 0; + return 0; +} + +static int mx_set_prot_level(struct spi_nor *nor, uint64_t lock_len, u8 *sr) +{ + uint64_t new_len; + u8 new_lvl; + + if (lock_len) { + /* 64Kib block size harcoded */ + new_lvl = ilog2(lock_len) - 15; + new_len = 1ULL << (15 + new_lvl); + + if (new_len != lock_len) + return -EINVAL; + } else { + new_lvl = 0; + } + + *sr &= ~MX_BP_MASK; + *sr |= (new_lvl) << MX_BP_SHIFT; + + return 0; +} + +static int mx_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) +{ + struct mtd_info *mtd = &nor->mtd; + int ret; + uint64_t lock_len; + u8 sr; + + ret = spi_nor_read_sr(nor, nor->bouncebuf); + if (ret) + return ret; + + sr = nor->bouncebuf[0]; + + /* always 'top' protection */ + if ((ofs + len) != mtd->size) + return -EINVAL; + + ret = mx_get_locked_len(nor, sr, &lock_len); + if (ret) + return ret; + + /* already locked? */ + if (len <= lock_len) + return 0; + + ret = mx_set_prot_level(nor, len, &sr); + if (ret) + return ret; + + /* Disallow further writes if WP pin is asserted */ + sr |= SR_SRWD; + + return spi_nor_write_sr_and_check(nor, sr); +} + +static int mx_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) +{ + struct mtd_info *mtd = &nor->mtd; + int ret; + uint64_t lock_len; + u8 sr; + + if ((ofs + len) > mtd->size) + return -EINVAL; + + ret = spi_nor_read_sr(nor, nor->bouncebuf); + if (ret) + return ret; + + sr = nor->bouncebuf[0]; + + ret = mx_get_locked_len(nor, sr, &lock_len); + if (ret) + return ret; + + /* already unlocked? */ + if ((ofs + len) <= (mtd->size - lock_len)) + return 0; + + /* can't make a hole in a locked region */ + if (ofs > (mtd->size - lock_len)) + return -EINVAL; + + lock_len = mtd->size - ofs - len; + ret = mx_set_prot_level(nor, lock_len, &sr); + if (ret) + return ret; + + /* Don't protect status register if we're fully unlocked */ + if (lock_len == 0) + sr &= ~SR_SRWD; + + return spi_nor_write_sr_and_check(nor, sr); +} + +static int mx_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len) +{ + struct mtd_info *mtd = &nor->mtd; + int ret; + uint64_t lock_len; + u8 sr; + + if ((ofs + len) > mtd->size) + return -EINVAL; + + if (!len) + return 0; + + ret = spi_nor_read_sr(nor, nor->bouncebuf); + if (ret) + return ret; + + sr = nor->bouncebuf[0]; + + ret = mx_get_locked_len(nor, sr, &lock_len); + if (ret) + return ret; + + return (ofs >= (mtd->size - lock_len)) ? 1 : 0; +} + +static const struct spi_nor_locking_ops mx_locking_ops = { + .lock = mx_lock, + .unlock = mx_unlock, + .is_locked = mx_is_locked, +}; + +static void mx_default_init(struct spi_nor *nor) +{ + nor->params->locking_ops = &mx_locking_ops; +} + +static const struct spi_nor_fixups mx_locking_fixups = { + .default_init = mx_default_init, +}; + static int mx25l25635_post_bfpt_fixups(struct spi_nor *nor, const struct sfdp_parameter_header *bfpt_header, @@ -48,7 +237,9 @@ static const struct flash_info macronix_parts[] = { SPI_NOR_QUAD_READ) }, { "mx25u4035", INFO(0xc22533, 0, 64 * 1024, 8, SECT_4K) }, { "mx25u8035", INFO(0xc22534, 0, 64 * 1024, 16, SECT_4K) }, - { "mx25u6435f", INFO(0xc22537, 0, 64 * 1024, 128, SECT_4K) }, + { "mx25u6435f", INFO(0xc22537, 0, 64 * 1024, 128, + SECT_4K | SPI_NOR_HAS_LOCK) + .fixups = &mx_locking_fixups }, { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SECT_4K) }, { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) }, { "mx25r1635f", INFO(0xc22815, 0, 64 * 1024, 32,
This patch adds block protection support to Macronix mx25u6432f and mx25u6435f. Two different chips share the same JEDEC ID while only mx25u6423f support section protections. And two chips have slightly different definitions of BP bits than generic (ST Micro) implementation. So this patch defines a new spi_nor_locking_ops only for macronix until this could be merged into a generic swp implementation. Signed-off-by: Ikjoon Jang <ikjn@chromium.org> --- drivers/mtd/spi-nor/macronix.c | 193 ++++++++++++++++++++++++++++++++- 1 file changed, 192 insertions(+), 1 deletion(-)