| Message ID | 20200323092430.1466234-2-tudor.ambarus@microchip.com |
|---|---|
| State | Superseded |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: Add SR 4bit block protection support | expand |
Hi, Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > Fix the gap for the SR block protection, the BP bits were set with > a +1 value than actually needed. This patch does not change the > behavior of the locking operations, just fixes the protected areas. So instead of rounding up, it does round down now? > > On a 16Mbit flash with 64KByte erase sector, the following changed > for the lock operation: > > Number of blocks | BP2:0 before | BP2:0 now | > 1 | 010b | 001b | > 2 | 110b | 010b | > 3 | 110b | 010b | > 4 | 100b | 011b | > 5 | 100b | 011b | > 6 | 100b | 011b | > 7 | 100b | 011b | > 8 | 101b | 100b | > 9 | 101b | 100b | > ... | ... | ... | > > For the lock operation, if one requests to lock an area that is not > matching the upper boundary of a BP protected area, we round down > the total length and lock less than the user requested, in order to > not lock more than the user actually requested. I don't know if that is really what a user really want. Because you'd end up with regions which the user believes are locked but are not. IMHO if you'd have to make a choice I'd prefer to have the remainder locked. Not the other way around. Esp. since the user explicitly express to have a region locked. -michael > For the unlock operation, read the number of blocks column as > "locked all but number of blocks value". On a 16Mbit flash with > 64KByte erase sector, the following changed for the lock operation: > > Number of blocks | BP2:0 before | BP2:0 now | > 1 | 111b | 101b | > ... | ... | ... | > 15 | 111b | 101b | > 16 | 110b | 101b | > 17 | 110b | 100b | > ... | ... | ... | > 24 | 101b | 100b | > 25 | 101b | 011b | > 26 | 101b | 011b | > 27 | 101b | 011b | > 28 | 100b | 011b | > 29 | 100b | 010b | > 30 | 011b | 010b | > 31 | 010b | 001b | > 32 | 000b | 000b | > > For the unlock operation, if one requests to unlock an area that is > not matching the upper boundary of a BP protected area, we round up > the total length and unlock more than the user actually requested. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 877557dbda7f..36660068bc04 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1654,13 +1654,13 @@ static int spi_nor_sr_lock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > /* > * Need smallest pow such that: > * > - * 1 / (2^pow) <= (len / size) > + * 1 / ((2^pow) - 1) <= (len / size) > * > * so (assuming power-of-2 size) we do: > * > - * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) > + * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1 > */ > - pow = ilog2(mtd->size) - ilog2(lock_len); > + pow = ilog2(mtd->size) - ilog2(lock_len) + 1; > val = mask - (pow << SR_BP_SHIFT); > if (val & ~mask) > return -EINVAL; > @@ -1739,13 +1739,13 @@ static int spi_nor_sr_unlock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > /* > * Need largest pow such that: > * > - * 1 / (2^pow) >= (len / size) > + * 1 / ((2^pow) - 1) >= (len / size) > * > * so (assuming power-of-2 size) we do: > * > - * pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) > + * pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) + 1 > */ > - pow = ilog2(mtd->size) - order_base_2(lock_len); > + pow = ilog2(mtd->size) - order_base_2(lock_len) + 1; > if (lock_len == 0) { > val = 0; /* fully unlocked */ > } else {
On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi, > > Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > > > Fix the gap for the SR block protection, the BP bits were set with > > a +1 value than actually needed. This patch does not change the > > behavior of the locking operations, just fixes the protected areas. > > So instead of rounding up, it does round down now? No. Why do you say that it rounds up? The behavior is not changed, the patch merely fix the protected area, which was wrong before. The round down is present before this patch. > > > On a 16Mbit flash with 64KByte erase sector, the following changed > > for the lock operation: > > > > Number of blocks | BP2:0 before | BP2:0 now | > > > > 1 | 010b | 001b | > > 2 | 110b | 010b | > > 3 | 110b | 010b | > > 4 | 100b | 011b | > > 5 | 100b | 011b | > > 6 | 100b | 011b | > > 7 | 100b | 011b | > > 8 | 101b | 100b | > > 9 | 101b | 100b | > > > > ... | ... | ... | > > > > For the lock operation, if one requests to lock an area that is not > > matching the upper boundary of a BP protected area, we round down > > the total length and lock less than the user requested, in order to > > not lock more than the user actually requested. > > I don't know if that is really what a user really want. Because you'd > end up with regions which the user believes are locked but are not. True. I'm thinking of how we can improve this, but it's not in the scope of this patch set, because the behavior is not changed. > IMHO if you'd have to make a choice I'd prefer to have the remainder > locked. Not the other way around. Esp. since the user explicitly > express to have a region locked. > We can still talk about this. Please notice that the formula that we want to introduce does the same thing as described in this commit message: for unaligned locks, it round down the length, and for unaligned unlocks it rounds up the length. I'm waiting for a reply. ta
Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the >> content is safe >> >> Hi, >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> >> > >> > Fix the gap for the SR block protection, the BP bits were set with >> > a +1 value than actually needed. This patch does not change the >> > behavior of the locking operations, just fixes the protected areas. >> >> So instead of rounding up, it does round down now? > > No. Why do you say that it rounds up? The behavior is not changed, the > patch > merely fix the protected area, which was wrong before. The round down > is > present before this patch. TBH I don't understand what this patch should do. Could you give an example? >> >> > On a 16Mbit flash with 64KByte erase sector, the following changed >> > for the lock operation: 16MBit is a bad example, because it is broken anyway, isn't it? We use a 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. a 50/50 split. I haven't seen any issued. Shouldn't it be then completely locked according this the following example? >> > >> > Number of blocks | BP2:0 before | BP2:0 now | >> > >> > 1 | 010b | 001b | >> > 2 | 110b | 010b | >> > 3 | 110b | 010b | >> > 4 | 100b | 011b | >> > 5 | 100b | 011b | >> > 6 | 100b | 011b | >> > 7 | 100b | 011b | >> > 8 | 101b | 100b | >> > 9 | 101b | 100b | >> > >> > ... | ... | ... | >> > >> > For the lock operation, if one requests to lock an area that is not >> > matching the upper boundary of a BP protected area, we round down >> > the total length and lock less than the user requested, in order to >> > not lock more than the user actually requested. >> >> I don't know if that is really what a user really want. Because you'd >> end up with regions which the user believes are locked but are not. > > True. I'm thinking of how we can improve this, but it's not in the > scope of > this patch set, because the behavior is not changed. ok, agreed, > >> IMHO if you'd have to make a choice I'd prefer to have the remainder >> locked. Not the other way around. Esp. since the user explicitly >> express to have a region locked. >> > > We can still talk about this. Please notice that the formula that we > want to > introduce does the same thing as described in this commit message: for > unaligned locks, it round down the length, and for unaligned unlocks it > rounds > up the length. ok -michael > > I'm waiting for a reply. > ta
On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: > > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> the > >> content is safe > >> > >> Hi, > >> > >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> > >> > > >> > Fix the gap for the SR block protection, the BP bits were set with > >> > a +1 value than actually needed. This patch does not change the > >> > behavior of the locking operations, just fixes the protected areas. > >> > >> So instead of rounding up, it does round down now? > > > > No. Why do you say that it rounds up? The behavior is not changed, the > > patch > > merely fix the protected area, which was wrong before. The round down > > is > > present before this patch. > > TBH I don't understand what this patch should do. Could you give an > example? sure, let me try to be more explicit. > > >> > On a 16Mbit flash with 64KByte erase sector, the following changed > > >> > for the lock operation: > 16MBit is a bad example, because it is broken anyway, isn't it? We use a it's not. > 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. a > 50/50 split. I haven't seen any issued. Shouldn't it be then completely > locked according this the following example? I don't follow. The table from below was generated for the S25FL116K 16 Mbit flash. BTW, one has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the locking. When you have a 4k sector erase, the locking is simply wrong, but this is another topic. > > >> > Number of blocks | BP2:0 before | BP2:0 now | > >> > > >> > 1 | 010b | 001b | - number of blocks is how many blocks you want to lock. One would do for one block: flash_lock /dev/mtd 0 1 i.e. lock a single erase block starting from offset 0. - "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0 1" before this patch - "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1" using this patch So before this patch, the lock operation was bad, because it locked 2 blocks instead of one. > >> > 2 | 110b | 010b | - lock 2 erase blocks starting from offset 0. Results before this patch, and after this patch. Continue the logic on the following lines. oops there's a typo in column 2, sorry. The value in column 2 should have been 011b. So before this patch, when one requested to lock 2 block starting from offset 0, we would obtain 4 blocks locked, and he should have obtained just 2. The scope of this patch is to first fix the locking ops, so that we can introduce a more generic formula that gives the same results as before introducing it. Without this patch, the new formula will silently fix the bug that is described here. > >> > 3 | 110b | 010b | ^ typo s/110b/011b rest of the examples are good. Cheers, ta > >> > 4 | 100b | 011b | > >> > 5 | 100b | 011b | > >> > 6 | 100b | 011b | > >> > 7 | 100b | 011b | > >> > 8 | 101b | 100b | > >> > 9 | 101b | 100b | > >> > > >> > ... | ... | ... | > >> > > >> > For the lock operation, if one requests to lock an area that is not > >> > matching the upper boundary of a BP protected area, we round down > >> > the total length and lock less than the user requested, in order to > >> > not lock more than the user actually requested.
Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com: > On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the >> content is safe >> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: >> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> >> the >> >> content is safe >> >> >> >> Hi, >> >> >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: >> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> >> >> > >> >> > Fix the gap for the SR block protection, the BP bits were set with >> >> > a +1 value than actually needed. This patch does not change the >> >> > behavior of the locking operations, just fixes the protected areas. >> >> >> >> So instead of rounding up, it does round down now? >> > >> > No. Why do you say that it rounds up? The behavior is not changed, the >> > patch >> > merely fix the protected area, which was wrong before. The round down >> > is >> > present before this patch. >> >> TBH I don't understand what this patch should do. Could you give an >> example? > > sure, let me try to be more explicit. > >> >> >> > On a 16Mbit flash with 64KByte erase sector, the following changed >> >> >> > for the lock operation: >> 16MBit is a bad example, because it is broken anyway, isn't it? We use >> a > > it's not. If I'm not mistaken this falls into the same category like the new 4bits BP flashes, because there are more slots free than needed. Ie. the last one "protect all" is either 110b or 111b and thus don't work with the old formula. This was actually my reason why there is no new formula for the 4 bits BP flashes; but the current one is not working with flashes <32Mbit. See the old long thread. > >> 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. >> a >> 50/50 split. I haven't seen any issued. Shouldn't it be then >> completely >> locked according this the following example? > > I don't follow. We've successfully used the "flash_lock 0 0x200" (with 4k sectors) on our boards to lock the first half of our 4MiB flash. > The table from below was generated for the S25FL116K 16 Mbit flash. > BTW, one > has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the > locking. > When you have a 4k sector erase, the locking is simply wrong, but this > is > another topic. it should work with that too if you convert the number to the smaller sectors, ie multiply by 16; But yeah the cli tool has a broken interface. It should accept both offset and length in bytes; not one one in bytes and one in sectors, where the latter also changes with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. >> >> >> > Number of blocks | BP2:0 before | BP2:0 now | >> >> > >> >> > 1 | 010b | 001b | > > - number of blocks is how many blocks you want to lock. One would do > for one > block: > flash_lock /dev/mtd 0 1 > i.e. lock a single erase block starting from offset 0. > > - "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0 > 1" > before this patch Without your patch applied it works like expected: [ 1.914329] spi-nor spi0.0: w25q32dw (4096 Kbytes) # flash_lock -l /dev/mtd1 0 1 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg a4 A4 is 1010_0100, ie BP[2:0] = 001b and TB=1 # flash_lock -u /dev/mtd1 0 64 # flash_lock -l /dev/mtd1 0 32 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg b8 With this patch applied: # flash_lock -u /dev/mtd1 0 64 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg 00 # flash_lock -l /dev/mtd1 0 1 flash_lock: error!: could not lock device: /dev/mtd1 error 22 (Invalid argument) # flash_lock -l /dev/mtd1 0 2 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg a4 which is wrong, isn't it? -michael > > - "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1" > using > this patch > > So before this patch, the lock operation was bad, because it locked 2 > blocks > instead of one. > >> >> > 2 | 110b | 010b | > > - lock 2 erase blocks starting from offset 0. Results before this > patch, and > after this patch. Continue the logic on the following lines. > > oops there's a typo in column 2, sorry. The value in column 2 should > have been > 011b. > > So before this patch, when one requested to lock 2 block starting from > offset > 0, we would obtain 4 blocks locked, and he should have obtained just 2. > > The scope of this patch is to first fix the locking ops, so that we can > introduce a more generic formula that gives the same results as before > introducing it. Without this patch, the new formula will silently fix > the bug > that is described here. > >> >> > 3 | 110b | 010b | > ^ typo s/110b/011b > > rest of the examples are good. > > Cheers, > ta > >> >> > 4 | 100b | 011b | >> >> > 5 | 100b | 011b | >> >> > 6 | 100b | 011b | >> >> > 7 | 100b | 011b | >> >> > 8 | 101b | 100b | >> >> > 9 | 101b | 100b | >> >> > >> >> > ... | ... | ... | >> >> > >> >> > For the lock operation, if one requests to lock an area that is not >> >> > matching the upper boundary of a BP protected area, we round down >> >> > the total length and lock less than the user requested, in order to >> >> > not lock more than the user actually requested.
On Monday, March 23, 2020 11:14:05 PM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com: > > On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> the > >> content is safe > >> > >> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: > >> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: > >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> >> the > >> >> content is safe > >> >> > >> >> Hi, > >> >> > >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > >> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> > >> >> > > >> >> > Fix the gap for the SR block protection, the BP bits were set with > >> >> > a +1 value than actually needed. This patch does not change the > >> >> > behavior of the locking operations, just fixes the protected areas. > >> >> > >> >> So instead of rounding up, it does round down now? > >> > > >> > No. Why do you say that it rounds up? The behavior is not changed, the > >> > patch > >> > merely fix the protected area, which was wrong before. The round down > >> > is > >> > present before this patch. > >> > >> TBH I don't understand what this patch should do. Could you give an > >> example? > > > > sure, let me try to be more explicit. > > > >> >> > On a 16Mbit flash with 64KByte erase sector, the following changed > >> > >> >> > for the lock operation: > >> 16MBit is a bad example, because it is broken anyway, isn't it? We use > >> a > > > > it's not. > > If I'm not mistaken this falls into the same category like the new 4bits > BP > flashes, because there are more slots free than needed. Ie. the last one > "protect all" is either 110b or 111b and thus don't work with the old > formula. This was actually my reason why there is no new formula for the > 4 bits BP flashes; but the current one is not working with flashes > <32Mbit. > See the old long thread. > > >> 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. > >> a > >> 50/50 split. I haven't seen any issued. Shouldn't it be then > >> completely > >> locked according this the following example? > > > > I don't follow. > > We've successfully used the "flash_lock 0 0x200" (with 4k sectors) on > our > boards to lock the first half of our 4MiB flash. > > > The table from below was generated for the S25FL116K 16 Mbit flash. > > BTW, one > > has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the > > locking. > > When you have a 4k sector erase, the locking is simply wrong, but this > > is > > another topic. > > it should work with that too if you convert the number to the smaller > sectors, > ie multiply by 16; But yeah the cli tool has a broken interface. It > should > accept both offset and length in bytes; not one one in bytes and one in > sectors, > where the latter also changes with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. > > >> >> > Number of blocks | BP2:0 before | BP2:0 now | > >> >> > > >> >> > 1 | 010b | 001b | > > > > - number of blocks is how many blocks you want to lock. One would do > > for one > > > > block: > > flash_lock /dev/mtd 0 1 > > > > i.e. lock a single erase block starting from offset 0. > > > > - "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0 > > 1" > > before this patch > > Without your patch applied it works like expected: > > [ 1.914329] spi-nor spi0.0: w25q32dw (4096 Kbytes) > # flash_lock -l /dev/mtd1 0 1 > # cat > /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg > a4 > > A4 is 1010_0100, ie BP[2:0] = 001b and TB=1 > what happens if you request flash_lock -l /dev/mtd1 0 3? > # flash_lock -u /dev/mtd1 0 64 > # flash_lock -l /dev/mtd1 0 32 > # cat > /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg > b8 > > > With this patch applied: > > # flash_lock -u /dev/mtd1 0 64 > # cat > /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg > 00 > # flash_lock -l /dev/mtd1 0 1 > flash_lock: error!: could not lock device: /dev/mtd1 > > error 22 (Invalid argument) I'm wondering what was the reason for the -EINVAL. > # flash_lock -l /dev/mtd1 0 2 > # cat > /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg > a4 > > which is wrong, isn't it? > Looks so. You should have obtained, 0xa8, right? Will recheck tomorrow morning. Thanks for testing this! I don't have a 32Mbit flash ... Cheers, ta > > > - "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1" > > using > > this patch > > > > So before this patch, the lock operation was bad, because it locked 2 > > blocks > > instead of one. > > > >> >> > 2 | 110b | 010b | > > > > - lock 2 erase blocks starting from offset 0. Results before this > > patch, and > > after this patch. Continue the logic on the following lines. > > > > oops there's a typo in column 2, sorry. The value in column 2 should > > have been > > 011b. > > > > So before this patch, when one requested to lock 2 block starting from > > offset > > 0, we would obtain 4 blocks locked, and he should have obtained just 2. > > > > The scope of this patch is to first fix the locking ops, so that we can > > introduce a more generic formula that gives the same results as before > > introducing it. Without this patch, the new formula will silently fix > > the bug > > that is described here. > > > >> >> > 3 | 110b | 010b | > > > > ^ typo s/110b/011b > > > > rest of the examples are good. > > > > Cheers, > > ta > > > >> >> > 4 | 100b | 011b | > >> >> > 5 | 100b | 011b | > >> >> > 6 | 100b | 011b | > >> >> > 7 | 100b | 011b | > >> >> > 8 | 101b | 100b | > >> >> > 9 | 101b | 100b | > >> >> > > >> >> > ... | ... | ... | > >> >> > > >> >> > For the lock operation, if one requests to lock an area that is not > >> >> > matching the upper boundary of a BP protected area, we round down > >> >> > the total length and lock less than the user requested, in order to > >> >> > not lock more than the user actually requested.
On Monday, March 23, 2020 11:30:44 PM EET Tudor Ambarus wrote: > > With this patch applied: > > > > # flash_lock -u /dev/mtd1 0 64 > > # cat > > /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg > > 00 > > # flash_lock -l /dev/mtd1 0 1 > > flash_lock: error!: could not lock device: /dev/mtd1 > > > > error 22 (Invalid argument) > > I'm wondering what was the reason for the -EINVAL. Probably because the BP bits would have value zero.
Am 2020-03-23 22:30, schrieb Tudor.Ambarus@microchip.com: > On Monday, March 23, 2020 11:14:05 PM EET Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the >> content is safe >> Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com: >> > On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote: >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> >> the >> >> content is safe >> >> >> >> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: >> >> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: >> >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> >> >> the >> >> >> content is safe >> >> >> >> >> >> Hi, >> >> >> >> >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: >> >> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> >> >> >> > >> >> >> > Fix the gap for the SR block protection, the BP bits were set with >> >> >> > a +1 value than actually needed. This patch does not change the >> >> >> > behavior of the locking operations, just fixes the protected areas. >> >> >> >> >> >> So instead of rounding up, it does round down now? >> >> > >> >> > No. Why do you say that it rounds up? The behavior is not changed, the >> >> > patch >> >> > merely fix the protected area, which was wrong before. The round down >> >> > is >> >> > present before this patch. >> >> >> >> TBH I don't understand what this patch should do. Could you give an >> >> example? >> > >> > sure, let me try to be more explicit. >> > >> >> >> > On a 16Mbit flash with 64KByte erase sector, the following changed >> >> >> >> >> > for the lock operation: >> >> 16MBit is a bad example, because it is broken anyway, isn't it? We use >> >> a >> > >> > it's not. >> >> If I'm not mistaken this falls into the same category like the new >> 4bits >> BP >> flashes, because there are more slots free than needed. Ie. the last >> one >> "protect all" is either 110b or 111b and thus don't work with the old >> formula. This was actually my reason why there is no new formula for >> the >> 4 bits BP flashes; but the current one is not working with flashes >> <32Mbit. >> See the old long thread. >> >> >> 32Mbit flash where 2MB are locked and the second 2MB are unlocked. Eg. >> >> a >> >> 50/50 split. I haven't seen any issued. Shouldn't it be then >> >> completely >> >> locked according this the following example? >> > >> > I don't follow. >> >> We've successfully used the "flash_lock 0 0x200" (with 4k sectors) on >> our >> boards to lock the first half of our 4MiB flash. >> >> > The table from below was generated for the S25FL116K 16 Mbit flash. >> > BTW, one >> > has to disable CONFIG_MTD_SPI_NOR_USE_4K_SECTORS in order to test the >> > locking. >> > When you have a 4k sector erase, the locking is simply wrong, but this >> > is >> > another topic. >> >> it should work with that too if you convert the number to the smaller >> sectors, >> ie multiply by 16; But yeah the cli tool has a broken interface. It >> should >> accept both offset and length in bytes; not one one in bytes and one >> in >> sectors, >> where the latter also changes with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. >> >> >> >> > Number of blocks | BP2:0 before | BP2:0 now | >> >> >> > >> >> >> > 1 | 010b | 001b | >> > >> > - number of blocks is how many blocks you want to lock. One would do >> > for one >> > >> > block: >> > flash_lock /dev/mtd 0 1 >> > >> > i.e. lock a single erase block starting from offset 0. >> > >> > - "BP0:2 before" is the result of the operation "flash_lock /dev/mtd 0 >> > 1" >> > before this patch >> >> Without your patch applied it works like expected: >> >> [ 1.914329] spi-nor spi0.0: w25q32dw (4096 Kbytes) >> # flash_lock -l /dev/mtd1 0 1 >> # cat >> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg >> a4 >> >> A4 is 1010_0100, ie BP[2:0] = 001b and TB=1 >> > > what happens if you request flash_lock -l /dev/mtd1 0 3? with your patch applied: # flash_lock -u /dev/mtd1 0 64 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg 00 # flash_lock -l /dev/mtd1 0 3 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg a4 without it: # flash_lock -u /dev/mtd1 0 64 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg 00 # flash_lock -l /dev/mtd1 0 3 # cat /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg a8 >> # flash_lock -u /dev/mtd1 0 64 >> # flash_lock -l /dev/mtd1 0 32 >> # cat >> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg >> b8 >> >> >> With this patch applied: >> >> # flash_lock -u /dev/mtd1 0 64 >> # cat >> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg >> 00 >> # flash_lock -l /dev/mtd1 0 1 >> flash_lock: error!: could not lock device: /dev/mtd1 >> >> error 22 (Invalid argument) > > I'm wondering what was the reason for the -EINVAL. > >> # flash_lock -l /dev/mtd1 0 2 >> # cat >> /sys/devices/platform/soc/20c0000.spi/spi_master/spi0/spi0.0/status_reg >> a4 >> >> which is wrong, isn't it? >> > Looks so. You should have obtained, 0xa8, right? correct, BP should be 010b for the first two sectors. > Will recheck tomorrow > morning. > > Thanks for testing this! I don't have a 32Mbit flash ... You should be able to reproduce it with every flash >=32Mbit which has 3 BP bits. -michael > > Cheers, > ta >> >> > - "BP0:2 now" is the result of the operation "flash_lock /dev/mtd 0 1" >> > using >> > this patch >> > >> > So before this patch, the lock operation was bad, because it locked 2 >> > blocks >> > instead of one. >> > >> >> >> > 2 | 110b | 010b | >> > >> > - lock 2 erase blocks starting from offset 0. Results before this >> > patch, and >> > after this patch. Continue the logic on the following lines. >> > >> > oops there's a typo in column 2, sorry. The value in column 2 should >> > have been >> > 011b. >> > >> > So before this patch, when one requested to lock 2 block starting from >> > offset >> > 0, we would obtain 4 blocks locked, and he should have obtained just 2. >> > >> > The scope of this patch is to first fix the locking ops, so that we can >> > introduce a more generic formula that gives the same results as before >> > introducing it. Without this patch, the new formula will silently fix >> > the bug >> > that is described here. >> > >> >> >> > 3 | 110b | 010b | >> > >> > ^ typo s/110b/011b >> > >> > rest of the examples are good. >> > >> > Cheers, >> > ta >> > >> >> >> > 4 | 100b | 011b | >> >> >> > 5 | 100b | 011b | >> >> >> > 6 | 100b | 011b | >> >> >> > 7 | 100b | 011b | >> >> >> > 8 | 101b | 100b | >> >> >> > 9 | 101b | 100b | >> >> >> > >> >> >> > ... | ... | ... | >> >> >> > >> >> >> > For the lock operation, if one requests to lock an area that is not >> >> >> > matching the upper boundary of a BP protected area, we round down >> >> >> > the total length and lock less than the user requested, in order to >> >> >> > not lock more than the user actually requested.
Hi, Tudor, On Mon, 2020-03-23 at 09:24 +0000, Tudor.Ambarus@microchip.com wrote: > From: Tudor Ambarus <tudor.ambarus@microchip.com> > > Fix the gap for the SR block protection, the BP bits were set with > a +1 value than actually needed. This patch does not change the > behavior of the locking operations, just fixes the protected areas. > > On a 16Mbit flash with 64KByte erase sector, the following changed > for the lock operation: > > Number of blocks | BP2:0 before | BP2:0 now | > 1 | 010b | 001b | > 2 | 110b | 010b | > 3 | 110b | 010b | > 4 | 100b | 011b | > 5 | 100b | 011b | > 6 | 100b | 011b | > 7 | 100b | 011b | > 8 | 101b | 100b | > 9 | 101b | 100b | > ... | ... | ... | > > For the lock operation, if one requests to lock an area that is not > matching the upper boundary of a BP protected area, we round down > the total length and lock less than the user requested, in order to > not lock more than the user actually requested. > > For the unlock operation, read the number of blocks column as > "locked all but number of blocks value". On a 16Mbit flash with > 64KByte erase sector, the following changed for the lock operation: > > Number of blocks | BP2:0 before | BP2:0 now | > 1 | 111b | 101b | > ... | ... | ... | > 15 | 111b | 101b | > 16 | 110b | 101b | > 17 | 110b | 100b | > ... | ... | ... | > 24 | 101b | 100b | > 25 | 101b | 011b | > 26 | 101b | 011b | > 27 | 101b | 011b | > 28 | 100b | 011b | > 29 | 100b | 010b | > 30 | 011b | 010b | > 31 | 010b | 001b | > 32 | 000b | 000b | > > For the unlock operation, if one requests to unlock an area that is > not matching the upper boundary of a BP protected area, we round up > the total length and unlock more than the user actually requested. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 877557dbda7f..36660068bc04 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1654,13 +1654,13 @@ static int spi_nor_sr_lock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > /* > * Need smallest pow such that: > * > - * 1 / (2^pow) <= (len / size) > + * 1 / ((2^pow) - 1) <= (len / size) > * > * so (assuming power-of-2 size) we do: > * > - * pow = ceil(log2(size / len)) = log2(size) - > floor(log2(len)) > + * pow = ceil(log2(size / len)) = log2(size) - > floor(log2(len)) + 1 > */ > - pow = ilog2(mtd->size) - ilog2(lock_len); > + pow = ilog2(mtd->size) - ilog2(lock_len) + 1; > val = mask - (pow << SR_BP_SHIFT); > if (val & ~mask) > return -EINVAL; (1) - if bp slot is insufficient. (2) - if bp slot is sufficient. if (bp_slots_needed > bp_slots) // (1) min_prot_length = sector_size << (bp_slots_needed - bp_slots); else // (2) min_prot_length = sector_size; I think that current mainline implementation is only valid for flashes in case (1). (for BP0-2 : 1/64, 1/32 ...) Isn't it? This is current implementaion. pow = ilog2(mtd->size) - order_base_2(lock_len) val = mask - (pow << SR_BP_SHIFT); 1/64 6 = 110b -> 001b 1/32 5 = 101b -> 010b 1/16 4 = 100b -> 011b 1/8 3 = 011b -> 100b 1/4 2 = 010b -> 101b 1/2 1 = 001b -> 110b ALL 0 = 000b -> 111b It is handling case (1) admirably. In other cases, however, the situation would be different. A 16Mbit flash with 64KByte erase sector(which you mentioned on this patch) should get 32 erase sectors and seems that it's smallest protected portion is 1/32. So supporting the flash, it needs some modification as you did. pow = ilog2(mtd->size) - order_base_2(lock_len) + 1 val = mask - (pow << SR_BP_SHIFT); 1/64 6 = 110b 111b -> 000b (execption case) 1/32 5 = 101b 110b -> 001b 1/16 4 = 100b 101b -> 010b 1/8 3 = 011b 100b -> 011b 1/4 2 = 010b 011b -> 100b 1/2 1 = 001b 010b -> 101b ALL 0 = 000b 001b -> 110b It would works for the flash, but it will conflicts with flashes in case (1) what has already been applied on mainline. And there are too various flashes that has different protected portions to handle with the above. Btw, the description on this patch is exactly what I had been looking for before and seems it's very useful. Thanks, > @@ -1739,13 +1739,13 @@ static int spi_nor_sr_unlock(struct spi_nor > *nor, loff_t ofs, uint64_t len) > /* > * Need largest pow such that: > * > - * 1 / (2^pow) >= (len / size) > + * 1 / ((2^pow) - 1) >= (len / size) > * > * so (assuming power-of-2 size) we do: > * > - * pow = floor(log2(size / len)) = log2(size) - > ceil(log2(len)) > + * pow = floor(log2(size / len)) = log2(size) - > ceil(log2(len)) + 1 > */ > - pow = ilog2(mtd->size) - order_base_2(lock_len); > + pow = ilog2(mtd->size) - order_base_2(lock_len) + 1; > if (lock_len == 0) { > val = 0; /* fully unlocked */ > } else {
On Tuesday, March 24, 2020 12:35:30 AM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > Am 2020-03-23 22:30, schrieb Tudor.Ambarus@microchip.com: > > On Monday, March 23, 2020 11:14:05 PM EET Michael Walle wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> the > >> content is safe > >> > >> Am 2020-03-23 21:26, schrieb Tudor.Ambarus@microchip.com: > >> > On Monday, March 23, 2020 9:54:38 PM EET Michael Walle wrote: > >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> >> the > >> >> content is safe > >> >> > >> >> Am 2020-03-23 20:20, schrieb Tudor.Ambarus@microchip.com: > >> >> > On Monday, March 23, 2020 8:27:13 PM EET Michael Walle wrote: > >> >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you > >> >> >> know > >> >> >> the > >> >> >> content is safe > >> >> >> > >> >> >> Hi, > >> >> >> > >> >> >> Am 2020-03-23 10:24, schrieb Tudor.Ambarus@microchip.com: > >> >> >> > From: Tudor Ambarus <tudor.ambarus@microchip.com> > >> >> >> > > >> >> >> > Fix the gap for the SR block protection, the BP bits were set > >> >> >> > with > >> >> >> > a +1 value than actually needed. This patch does not change the > >> >> >> > behavior of the locking operations, just fixes the protected > >> >> >> > areas. > >> >> >> > >> >> >> So instead of rounding up, it does round down now? > >> >> > > >> >> > No. Why do you say that it rounds up? The behavior is not changed, > >> >> > the > >> >> > patch > >> >> > merely fix the protected area, which was wrong before. The round > >> >> > down > >> >> > is > >> >> > present before this patch. > >> >> > >> >> TBH I don't understand what this patch should do. Could you give an > >> >> example? > >> > > >> > sure, let me try to be more explicit. > >> > > >> >> >> > On a 16Mbit flash with 64KByte erase sector, the following > >> >> >> > changed > >> >> > >> >> >> > for the lock operation: > >> >> 16MBit is a bad example, because it is broken anyway, isn't it? We use > >> >> a > >> > > >> > it's not. > >> > >> If I'm not mistaken this falls into the same category like the new > >> 4bits > >> BP > >> flashes, because there are more slots free than needed. Ie. the last > >> one > >> "protect all" is either 110b or 111b and thus don't work with the old > >> formula. This was actually my reason why there is no new formula for > >> the > >> 4 bits BP flashes; but the current one is not working with flashes > >> <32Mbit. > >> See the old long thread. You're right, I was trying to fix a dead horse. 16Mbit BP0:2 flashes are fixed with the generic formula. I'm going to respin without this patch. Thanks!
On Monday, March 23, 2020 11:24:33 AM EET Tudor Ambarus - M18064 wrote: > For the unlock operation, read the number of blocks column as > "locked all but number of blocks value". On a 16Mbit flash with > 64KByte erase sector, the following changed for the lock operation: ^ typo s/lock/unlock
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 877557dbda7f..36660068bc04 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1654,13 +1654,13 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, uint64_t len) /* * Need smallest pow such that: * - * 1 / (2^pow) <= (len / size) + * 1 / ((2^pow) - 1) <= (len / size) * * so (assuming power-of-2 size) we do: * - * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + * pow = ceil(log2(size / len)) = log2(size) - floor(log2(len)) + 1 */ - pow = ilog2(mtd->size) - ilog2(lock_len); + pow = ilog2(mtd->size) - ilog2(lock_len) + 1; val = mask - (pow << SR_BP_SHIFT); if (val & ~mask) return -EINVAL; @@ -1739,13 +1739,13 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len) /* * Need largest pow such that: * - * 1 / (2^pow) >= (len / size) + * 1 / ((2^pow) - 1) >= (len / size) * * so (assuming power-of-2 size) we do: * - * pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) + * pow = floor(log2(size / len)) = log2(size) - ceil(log2(len)) + 1 */ - pow = ilog2(mtd->size) - order_base_2(lock_len); + pow = ilog2(mtd->size) - order_base_2(lock_len) + 1; if (lock_len == 0) { val = 0; /* fully unlocked */ } else {