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

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

[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.

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(-)

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;
-- 
2.17.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

[Pratyush Yadav]

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

-- 
Regards,
Pratyush Yadav
Texas Instruments India

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

[Tudor.Ambarus]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

[Bert Vermeulen]

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?


-- 
Bert Vermeulen
bert@biot.com

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

[David Laight]

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)
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

[Bert Vermeulen]

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.


-- 
Bert Vermeulen
bert@biot.com

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

[David Laight]

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)
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

[Tudor.Ambarus]

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 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

[Tudor.Ambarus]

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.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

[Pratyush Yadav]

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.

-- 
Regards,
Pratyush Yadav
Texas Instruments India

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

[Joel Stanley]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/