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 <mich...@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!
