* Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic
2020-10-01 6:34 ` Pratyush Yadav
@ 2020-10-01 14:15 ` Tudor.Ambarus
2020-10-01 22:22 ` Bert Vermeulen
2020-10-06 11:03 ` Tudor.Ambarus
2 siblings, 0 replies; 11+ messages in thread
From: Tudor.Ambarus @ 2020-10-01 14:15 UTC (permalink / raw)
To: p.yadav, bert; +Cc: richard, linux-mtd, vigneshr, linux-kernel, miquel.raynal
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic
2020-10-01 6:34 ` Pratyush Yadav
2020-10-01 14:15 ` Tudor.Ambarus
@ 2020-10-01 22:22 ` Bert Vermeulen
2020-10-02 7:50 ` David Laight
2020-10-06 23:19 ` Joel Stanley
2020-10-06 11:03 ` Tudor.Ambarus
2 siblings, 2 replies; 11+ messages in thread
From: Bert Vermeulen @ 2020-10-01 22:22 UTC (permalink / raw)
To: Pratyush Yadav
Cc: vigneshr, tudor.ambarus, richard, linux-kernel, linux-mtd, miquel.raynal
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/
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic
2020-10-01 22:22 ` Bert Vermeulen
@ 2020-10-02 7:50 ` David Laight
2020-10-04 21:12 ` Bert Vermeulen
2020-10-06 23:19 ` Joel Stanley
1 sibling, 1 reply; 11+ messages in thread
From: David Laight @ 2020-10-02 7:50 UTC (permalink / raw)
To: 'Bert Vermeulen', Pratyush Yadav
Cc: vigneshr, tudor.ambarus, richard, linux-kernel, linux-mtd, miquel.raynal
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic
2020-10-02 7:50 ` David Laight
@ 2020-10-04 21:12 ` Bert Vermeulen
2020-10-04 21:36 ` David Laight
0 siblings, 1 reply; 11+ messages in thread
From: Bert Vermeulen @ 2020-10-04 21:12 UTC (permalink / raw)
To: David Laight, Pratyush Yadav
Cc: vigneshr, tudor.ambarus, richard, linux-kernel, linux-mtd, miquel.raynal
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic
2020-10-04 21:12 ` Bert Vermeulen
@ 2020-10-04 21:36 ` David Laight
0 siblings, 0 replies; 11+ messages in thread
From: David Laight @ 2020-10-04 21:36 UTC (permalink / raw)
To: 'Bert Vermeulen', Pratyush Yadav
Cc: vigneshr, tudor.ambarus, richard, linux-kernel, linux-mtd, miquel.raynal
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic
2020-10-01 22:22 ` Bert Vermeulen
2020-10-02 7:50 ` David Laight
@ 2020-10-06 23:19 ` Joel Stanley
1 sibling, 0 replies; 11+ messages in thread
From: Joel Stanley @ 2020-10-06 23:19 UTC (permalink / raw)
To: Bert Vermeulen
Cc: Vignesh Raghavendra, Tudor Ambarus, Richard Weinberger,
Linux Kernel Mailing List, linux-mtd, Miquel Raynal,
Pratyush Yadav
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic
2020-10-01 6:34 ` Pratyush Yadav
2020-10-01 14:15 ` Tudor.Ambarus
2020-10-01 22:22 ` Bert Vermeulen
@ 2020-10-06 11:03 ` Tudor.Ambarus
2020-10-06 11:19 ` Tudor.Ambarus
2 siblings, 1 reply; 11+ messages in thread
From: Tudor.Ambarus @ 2020-10-06 11:03 UTC (permalink / raw)
To: p.yadav, bert; +Cc: richard, linux-mtd, vigneshr, linux-kernel, miquel.raynal
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic
2020-10-06 11:03 ` Tudor.Ambarus
@ 2020-10-06 11:19 ` Tudor.Ambarus
2020-10-06 11:40 ` Pratyush Yadav
0 siblings, 1 reply; 11+ messages in thread
From: Tudor.Ambarus @ 2020-10-06 11:19 UTC (permalink / raw)
To: p.yadav, bert; +Cc: richard, linux-mtd, vigneshr, linux-kernel, miquel.raynal
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic
2020-10-06 11:19 ` Tudor.Ambarus
@ 2020-10-06 11:40 ` Pratyush Yadav
0 siblings, 0 replies; 11+ messages in thread
From: Pratyush Yadav @ 2020-10-06 11:40 UTC (permalink / raw)
To: Tudor.Ambarus
Cc: vigneshr, richard, linux-kernel, linux-mtd, miquel.raynal, bert
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/
^ permalink raw reply [flat|nested] 11+ messages in thread