HI Michael, thanks for the review. On Tue, Apr 13, 2021 at 8:26 PM Michael Walle <mich...@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/346332bf6ab0dd92b9ffd9e126b6b...@walle.cc/ > Thanks, let me try it. > -michael