mtd: spi-nor: Refactor Read Status/Write Status support

SPI NOR has a lot of history. Additions over additions, the subtleties
of the JESD216 specification, their implementations by the manufacturers
and the hardware mistakes have generated a gigantic maze, let's try to
understand what is really needed.

The specification explains the QE (Quad Enable) bitfield as describing
where the bit to enable the Quad capability is, but also how to set
it. Unfortunately, the specification is not precise about what opcodes
are supported exactly in all the cases. There is an introduction that
basically states:
- Opcode 0x05 reads SR1
- Opcode 0x35 reads SR2
- Opcode 0x01 writes SR1, and then SR2 if another byte is written

Then the bitfield, among indicating the location of the QE bit, may
indicate:
- Reading SR1 and SR2 in one operation is not possible (loops over the
  content of SR1)
- Reading SR2 directly is possible
- Only writing SR1 zeroes SR2.

One problem with the current implementation, is that it only focuses on
the QE bit. A quad_enable function was created for each case, even
though in practice, the logic was always the same: read, modify, write,
read back and verify. One problem comes when other features need to play
with the status registers, like software block protection or OTP: you
never know how to properly handle the QE bit, nor where it is, nor how
to read/write the Status registers. This lead to
approximations/guessing (in swp.c, otp.c and obviously in legacy
controller drivers like atmel.c) but also to the implementation of a
gazillon of helpers for reading/writing/checking the status registers.

In addition, I believe some design decisions had a negative impact over
the years.

- All possible situations had to be flagged by the core. This is likely
  wrong, because we no longer know why we need specific quirks. It was
  ineherent to the state of the SPI NOR core before the great cleanup
  that had happened the past few years. I believe this creates confusion
  in the core today, and we should push this to vendor fixups
  instead. As an example, in 2023 Hsin-Yi was facing an issue because
  his chip was setting the wrong QER value, leading to RDCR being
  prevented, thus falling into a condition blindly setting a random QE
  bit (I strongy believe it is done like that for wrong reasons). His
  chip actually had RDCR support! The correct fix should have been to
  mark the capability in a device fixup instead of handling this in the
  core.

  Link: https://lore.kernel.org/lkml/CAJMQK-hR0eaO0b4Vd0U8_KAndLyZapqdHjVLAoe42rWi9rdLkA@mail.gmail.com/

- SFDP parsing is over cautious. I believe
  BFPT_DWORD15_QER_SR2_BIT1_NO_RD is abusive (nothing states that RDCR is
  not supported), and BFPT_DWORD15_QER_SR2_BIT1 is also out of
  specification when forcing 16-bit Status writes.

- SNOR_F_HAS_16BIT_SR is only imposing 16bit Status writes, whereas
  reads can still be 8-bit wide.

- The usage of helpers verifying the writes was also spread for IMHO no
  really good reason. Why shouldn't we trust spi operations when it
  comes to Status Registers? We do not read back our page reads, so why
  status registers should be treated with so much care, if it's not
  because we are unsure of what is being done? My proposal includes a
  check when it comes to the QE bit (done once) but we don't need these
  checks otherwise. If the QE bit was written properly, there are high
  chances that the other register accesses will just be fine, no?

Asde from my main quest, I also observed no good reason to ask the
read/write status register callers to use nor->bouncebuf while the
low-level helpers could do it themselves (we are talking about one or
two bytes being copied).

So after these observations, my proposal is the following:
- Create private low level helpers that just read or write a status
  register. They are flexible, we can give the opcode (which varies
  based on the SFDP QER field) and the length (1 or 2).
- Create public generic accessors which will be used to read/write sr1
  and/or sr2. This is where all the cleverness shall be. The helpers use
  the available opcodes for a given chip in order to fullfill the
  request.
- Provide a single generic ->quad_enable() hook which generically does
  all the steps mentioned above (read, modify, write, read back and
  verify).
- Create a list of opcodes for all 6 possible situations:
  {read, write} {sr1, sr2, sr1 and sr2}. These opcodes are
  filled/cleared based on the QER field. An opcode set to 0 indicates
  the absence of support (there is no 0x00 opcode in SPI NOR).

I tried my best to analyze the current behavior and to mimic it as much
as possible, but this is a risky cleanup. However, if we go for this, it
will be *much* easier in the future to handle all kind of chip
variations. We won't be limited to a couple of flags anymore, but rather
we'll be able to just disable a read or write capability using a single
line.

Known deviations:
- The Atmel manufacturer driver drives non SFDP chips which by default
  were receiving the HAS_16B_WR flag, forcing SR2 writes all the times,
  but it was deliberately making single bytes writes to SR1 for its
  locking operations. The flag is likely wrong for them, so I removed
  it.
- I am proposing on purpose a single quad_enable helper. It should match
  all the cases, with 1 identified difference which I believe is
  harmless: in the SR2_BIT7 case, the helper would make an extra SR1
  read/write which was not done before. This entry explicitly supports
  reading and writing the two registers. Note: no manufacturer has been
  identified to actually use that entry yet.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Michael Walle <mwalle@kernel.org>

0 files changed, 0 insertions, 0 deletions