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/[email protected]/
-michael
HI Michael, thanks for the review.
On Tue, Apr 13, 2021 at 8:26 PM Michael Walle <[email protected]> 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/[email protected]/
>
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 <[email protected]> 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 <[email protected]> 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!