mirror of
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
synced 2026-10-26 10:57:38 +08:00
Miquel Raynal
63489002d3
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>