Message ID | 20200930235611.6355-1-bert@biot.com |
---|---|
State | Changes Requested |
Delegated to: | Ambarus Tudor |
Headers | show |
Series | mtd: spi-nor: Fix 3-or-4 address byte mode logic | expand |
Hi, On 01/10/20 01:56AM, Bert Vermeulen wrote: > 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. 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 from my side, NACK! > > Signed-off-by: Bert Vermeulen <bert@biot.com> > --- > drivers/mtd/spi-nor/sfdp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c > index e2a43d39eb5f..6fedc425bcf7 100644 > --- a/drivers/mtd/spi-nor/sfdp.c > +++ b/drivers/mtd/spi-nor/sfdp.c > @@ -456,10 +456,10 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, > /* Number of address bytes. */ > switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) { > case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY: > - case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: > nor->addr_width = 3; > break; > > + case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: > case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: > nor->addr_width = 4; > break; [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f9acd7fa80be6ee14aecdc54429f2a48e56224e8
On 10/1/20 9:34 AM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi, > > On 01/10/20 01:56AM, Bert Vermeulen wrote: >> 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. > > 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. > I do agree with Pratyush that we should follow the SFDP standard and don't change it. So the change is not acceptable. The standard is the "law". If manufacturers mess with it, and fill bits without respecting the standard, then we have to fix it via the post sfdp fixup hook. SFDP is above nor->info flags, don't change the order of the ifs. Cheers, ta
On 10/1/20 8:34 AM, Pratyush Yadav wrote: > 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. > 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. > 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 @@ -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?
From: Bert Vermeulen > Sent: 01 October 2020 23:23 > > On 10/1/20 8:34 AM, Pratyush Yadav wrote: > > 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. 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? A device that requires 3 bytes of address can be set to a read delay of 12 cycles (rather than the usual 10) so that 'hardware' reads (typically from address 0) can transparently support devices that require 3 or 4 bytes addresses. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 10/2/20 9:50 AM, David Laight wrote: > From: Bert Vermeulen >> 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? 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.
From: Bert Vermeulen > Sent: 04 October 2020 22:12 > > On 10/2/20 9:50 AM, David Laight wrote: > > From: Bert Vermeulen > >> 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? > > 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. How do they manage to let you do read/write without 'read control'. Actually I can imagine... The problem we had was getting the IO pins to link up to user logic on an Altera Cyclone-V fpga. Then it was just a 'SMOP' to get reads and write converted to nibble 'bit-bang' with the chipselect and output enable (IIRC) controlled by the register address. I doubt any 'standard' interface is as efficient. I think I found a hardware bug in the chips we are using. There seemed to be a timing window in which the 'read status' command (after a write/erase) was completely ignored by the device. Everything looked write on a scope - but the data line wasn't driven. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 10/1/20 9:34 AM, Pratyush Yadav wrote: > 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()? addr width, dummy and opcode are discovered when reading sfdp, we should be fine. If READ SFDP requirements have changed for octal ddr, then we'll have to handle that separately. Cheers, ta
On 10/6/20 2:03 PM, Tudor Ambarus - M18064 wrote: > On 10/1/20 9:34 AM, Pratyush Yadav wrote: >> 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()? 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.
On 06/10/20 11:19AM, Tudor.Ambarus@microchip.com wrote: > On 10/6/20 2:03 PM, Tudor Ambarus - M18064 wrote: > > On 10/1/20 9:34 AM, Pratyush Yadav wrote: > >> 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()? > > 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. Yes it will but that is not the problem I was trying to solve. The problem is simply that nor->addr_width is 0 without the BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 case that I added, since BFPT parsing won't touch it at all. And so SMPT_CMD_ADDRESS_LEN_USE_CURRENT results in the command using an op.addr.nbytes == 0 for the register read even though op.addr.val is set correctly. This means the controller skips the address phase and the register read fails. Defaulting to 3 for the BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 case means op.addr.nbytes is correctly set to 3 and register read works correctly and SMPT parsing correctly detects the current configuration. If the address width is set to 4 by the bootloader then we have the same problem in some ways as the 8D boot problem where we have no way of easily detecting which mode is being used. I did not try to solve that problem with this change. > 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. Yes, the U-Boot xSPI series I sent does this somewhat. It issues a soft reset before handing control over to the kernel, so the kernel sees the flash in PoR state. This also helps when U-Boot uses the flash in 8D mode.
On Thu, 1 Oct 2020 at 22:23, Bert Vermeulen <bert@biot.com> wrote: > > On 10/1/20 8:34 AM, Pratyush Yadav wrote: > > 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. > > > 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. > > > 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. Thanks for starting this conversation Bert. I had intended on mentioning this broke our systems but didn't get to it. It broke a few different Aspeed platforms where the flashes are >= 32MB. We are running with a revert of the 3_OR_4 patch in OpenBMC kernels: https://github.com/openbmc/linux/commit/ee41b2b489259f01585e49327377f62b76a24748 > > Since Tudor doesn't want the order of sfdp->info changed, how about > something like this instead? > > +++ b/drivers/mtd/spi-nor/core.c > @@ -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? I tested this change you proposed and it fixed the issue for me. Cheers, Joel > > > -- > Bert Vermeulen > bert@biot.com
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c index e2a43d39eb5f..6fedc425bcf7 100644 --- a/drivers/mtd/spi-nor/sfdp.c +++ b/drivers/mtd/spi-nor/sfdp.c @@ -456,10 +456,10 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, /* Number of address bytes. */ switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) { case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY: - case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: nor->addr_width = 3; break; + case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4: case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: nor->addr_width = 4; break;
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 <bert@biot.com> --- drivers/mtd/spi-nor/sfdp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)