mtd: spi-nor: Fix 3-or-4 address byte mode logic

Flash chips that announce BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 capability get an addr_width of 3. This breaks when the flash chip is actually larger than 16MB, since that requires a 4-byte address. The MX25L25635F does exactly this, breaking anything over 16MB.

spi-nor only enables 4-byte opcodes or 4-byte address mode if addr_width is 4, so no 4-byte mode is ever enabled. The > 16MB check in spi_nor_set_addr_width() only works if addr_width wasn't already set by the SFDP, which it was.

It could be fixed in a post_bfpt fixup for the MX25L25635F, but setting addr_width to 4 when BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 is found fixes the problem for all such cases.

Signed-off-by Bert Vermeulen <[email protected]>

---
 drivers/mtd/spi-nor/sfdp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mtd/spi-nor/sfdp.c +0 -0

index e2a43d39eb5f..6fedc425bcf7 100644
@@ -456,10 +456,10 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 	/* Number of address bytes. */

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Hi,

Quoted 5 lines · Bert Vermeulen

Flash chips that announce BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 capability get an addr_width of 3. This breaks when the flash chip is actually larger than 16MB, since that requires a 4-byte address. The MX25L25635F does exactly this, breaking anything over 16MB.

Quoted 8 lines

spi-nor only enables 4-byte opcodes or 4-byte address mode if addr_width is 4, so no 4-byte mode is ever enabled. The > 16MB check in spi_nor_set_addr_width() only works if addr_width wasn't already set by the SFDP, which it was.

It could be fixed in a post_bfpt fixup for the MX25L25635F, but setting addr_width to 4 when BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 is found fixes the problem for all such cases.

JESD216D.01 says: "01b: 3- or 4-Byte addressing (e.g., defaults to 3-Byte mode; enters 4-Byte mode on command)"

So using an address width of 4 here is not necessarily the right thing to do. This change would break SMPT parsing for all flashes that use 3-byte addressing by default because SMPT parsing can involve register reads/writes. One such device is the Cypress S28HS flash. In fact, this was what prompted me to write the patch [0].

Before that patch, how did MX25L25635F decide to use 4-byte addressing? Coming out of BFPT parsing addr_width would still be 0. My guess is that it would go into spi_nor_set_addr_width() with addr_width still 0 and then the check for (nor->mtd.size > 0x1000000) would set it to 4. Do I guess correctly?

In that case maybe we can do a better job of deciding what gets priority in the if-else chain. For example, giving addr_width from nor->info precedence over the one configured by SFDP can solve this problem. Then all you have to do is set the addr_width in the info struct, which is certainly easier than adding a fixup hook. There may be a more elegant solution to this but I haven't given it much thought.

So using an address width of 4 here is not necessarily the right thing to do. This change would break SMPT parsing for all flashes that use 3-byte addressing by default because SMPT parsing can involve register reads/writes. One such device is the Cypress S28HS flash. In fact, this was what prompted me to write the patch [0].

Before that patch, how did MX25L25635F decide to use 4-byte addressing?

Coming out of BFPT parsing addr_width would still be 0. My guess is that it would go into spi_nor_set_addr_width() with addr_width still 0 and then the check for (nor->mtd.size > 0x1000000) would set it to 4. Do I guess correctly?

In that case maybe we can do a better job of deciding what gets priority in the if-else chain. For example, giving addr_width from nor->info precedence over the one configured by SFDP can solve this problem. Then all you have to do is set the addr_width in the info struct, which is certainly easier than adding a fixup hook. There may be a more elegant solution to this but I haven't given it much thought.

Sent: 01 October 2020 23:23

Quoted 6 lines · Pratyush Yadav

So using an address width of 4 here is not necessarily the right thing to do. This change would break SMPT parsing for all flashes that use 3-byte addressing by default because SMPT parsing can involve register reads/writes. One such device is the Cypress S28HS flash. In fact, this was what prompted me to write the patch [0].

Quoted 1 line

Before that patch, how did MX25L25635F decide to use 4-byte addressing?

The SoCs I'm dealing with have an SPI_ADDR_SEL pin, indicating whether it should be in 3 or 4-byte mode. The vendor's hacked-up U-Boot sets the mode accordingly, as does their BSP. It seems to me like a misfeature, and I want to just ignore it and do reasonable JEDEC things, but I have the problem that the flash chip can be in 4-byte mode by the time it gets to my spi-nor driver.

From: Bert Vermeulen

Quoted 6 lines

The SoCs I'm dealing with have an SPI_ADDR_SEL pin, indicating whether it should be in 3 or 4-byte mode. The vendor's hacked-up U-Boot sets the mode accordingly, as does their BSP. It seems to me like a misfeature, and I want to just ignore it and do reasonable JEDEC things, but I have the problem that the flash chip can be in 4-byte mode by the time it gets to my spi-nor driver.

If these are the devices I think they are, can't you read the non-volatile config word (bit 0) to find out whether the device expects a 3 or 4 byte address and how many 'idle' clocks there are before the read data?

Sent: 04 October 2020 22:12

Quoted 8 lines · David Laight

From: Bert Vermeulen

Quoted 6 lines

The SoCs I'm dealing with have an SPI_ADDR_SEL pin, indicating whether it should be in 3 or 4-byte mode. The vendor's hacked-up U-Boot sets the mode accordingly, as does their BSP. It seems to me like a misfeature, and I want to just ignore it and do reasonable JEDEC things, but I have the problem that the flash chip can be in 4-byte mode by the time it gets to my spi-nor driver.

Quoted 4 lines

If these are the devices I think they are, can't you read the non-volatile config word (bit 0) to find out whether the device expects a 3 or 4 byte address and how many 'idle' clocks there are before the read data?

I'm working with Realtek RTL838x/RTL839x SoCs. Reading it out is a pretty convoluted procedure involving different I/O registers depending on the SoC model.

Quoted 7 lines · Pratyush Yadav

So using an address width of 4 here is not necessarily the right thing to do. This change would break SMPT parsing for all flashes that use 3-byte addressing by default because SMPT parsing can involve register reads/writes. One such device is the Cypress S28HS flash. In fact, this was what prompted me to write the patch [0].

Before that patch, how did MX25L25635F decide to use 4-byte addressing?

The SoCs I'm dealing with have an SPI_ADDR_SEL pin, indicating whether it should be in 3 or 4-byte mode. The vendor's hacked-up U-Boot sets the mode accordingly, as does their BSP. It seems to me like a misfeature, and I want to just ignore it and do reasonable JEDEC things, but I have the problem that the flash chip can be in 4-byte mode by the time it gets to my spi-nor driver.

Quoted 4 lines

Coming out of BFPT parsing addr_width would still be 0. My guess is that it would go into spi_nor_set_addr_width() with addr_width still 0 and then the check for (nor->mtd.size > 0x1000000) would set it to 4. Do I guess correctly?

No, it comes out of that with addr_width=3 because the chip publishes 3_OR_4 and hence gets 3, even if that's nonsensical for a 32MB chip to publish.

Certainly that's the problem, I just want to solve it in a more general case than just a fixup for this chip.

Quoted 6 lines

In that case maybe we can do a better job of deciding what gets priority in the if-else chain. For example, giving addr_width from nor->info precedence over the one configured by SFDP can solve this problem. Then all you have to do is set the addr_width in the info struct, which is certainly easier than adding a fixup hook. There may be a more elegant solution to this but I haven't given it much thought.

Since Tudor doesn't want the order of sfdp->info changed, how about something like this instead?

+++ b/drivers/mtd/spi-nor/core.c

(quoted diff) +5 -3

@@ -3028,13 +3028,15 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
         /* already configured from SFDP */
     } else if (nor->info->addr_width) {
         nor->addr_width = nor->info->addr_width;
-   } else if (nor->mtd.size > 0x1000000) {
-       /* enable 4-byte addressing if the device exceeds 16MiB */
-       nor->addr_width = 4;
     } else {
         nor->addr_width = 3;
     }
 
+   if (nor->addr_width == 3 && nor->mtd.size > 0x1000000) {
+       /* enable 4-byte addressing if the device exceeds 16MiB */
+       nor->addr_width = 4;
+   }
+
 

Still fixes the general case, but I'm not sure what the SMPT parsing problem is -- would this still trigger it?

--
Bert Vermeulen
[email protected]

So using an address width of 4 here is not necessarily the right thing to do. This change would break SMPT parsing for all flashes that use 3-byte addressing by default because SMPT parsing can involve register reads/writes. One such device is the Cypress S28HS flash. In fact, this was what prompted me to write the patch [0].

Quoted 5 lines · Pratyush Yadav

So using an address width of 4 here is not necessarily the right thing to do. This change would break SMPT parsing for all flashes that use 3-byte addressing by default because SMPT parsing can involve register reads/writes. One such device is the Cypress S28HS flash. In fact, this was what prompted me to write the patch [0].

Do you refer to spi_nor_get_map_in_use()?

Quoted 7 lines · On 10/6/20 2:03 PM, Tudor Ambarus - M18064 wrote:

Quoted 5 lines · Pratyush Yadav

So using an address width of 4 here is not necessarily the right thing to do. This change would break SMPT parsing for all flashes that use 3-byte addressing by default because SMPT parsing can involve register reads/writes. One such device is the Cypress S28HS flash. In fact, this was what prompted me to write the patch [0].

Quoted 1 line

Do you refer to spi_nor_get_map_in_use()?

oh, I see. If addr width is set via the SMPT_CMD_ADDRESS_LEN_USE_CURRENT, case, and if the flash comes in 4 byte address mode from a bootloader, then setting addr_width to 3 in case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4, will break the reading of the map.

If the Address Mode bit is volatile, maybe we can reset the flash to its power on state immediately after identification. For the NV bits, we have the same recurring problem.