Re: [PATCH] mtd: spi-nor: macronix: Add block protection support to mx25u6435f

From: Ikjoon Jang
Date: Thu Apr 15 2021 - 04:28:16 EST


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@xxxxxxxx> 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!