Discussion:
[PATCH] mtd: spi-nor: fix options for mx66l51235f
Alexander Amelkin
2018-08-20 10:26:51 UTC
Permalink
Currently in spi-nor driver there is a line for mx66l51235l.
According to Macronix site, there is no such part number. The chip
detected as such is actually mx66l51235f. Hence, this commit renames
the chip.

According to the datasheet for mx66l51235f, "The device default is in
24-bit address mode" (section 9-10). Having option SPI_NOR_4B_OPCODES
makes the code act as if the device was already in 4B mode and didn't
need the EN4B command. That prevents this chip from functioning on
systems where the boot loader left the chip in 3B mode (e.g. if the
chip wasn't used during the boot process).

Hence, this commit removes the SPI_NOR_4B_OPCODES option for
mx66l51235f (added previously by commit d342b6a973af).

Cc: Marek Vasut <***@gmail.com>
Cc: <linux-***@lists.infradead.org>
Cc: <***@lists.ozlabs.org>
Cc: Joel Stanley <***@jms.id.au>
Fixes: d342b6a973af ("mtd: spi-nor: enable 4B opcodes for mx66l51235l")
Signed-off-by: Alexander Amelkin <***@yadro.com>
Reviewed-by: Cédric Le Goater <***@kaod.org>
---
drivers/mtd/spi-nor/spi-nor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f028277..c5ef85e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1091,7 +1091,7 @@ static const struct flash_info spi_nor_ids[] = {
{ "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
{ "mx25u25635f", INFO(0xc22539, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_4B_OPCODES) },
{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
- { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
+ { "mx66l51235f", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
{ "mx66u51235f", INFO(0xc2253a, 0, 64 * 1024, 1024, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
{ "mx66l1g45g", INFO(0xc2201b, 0, 64 * 1024, 2048, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
{ "mx66l1g55g", INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) },
--
2.7.4
Marek Vasut
2018-08-27 10:33:21 UTC
Permalink
Post by Alexander Amelkin
Currently in spi-nor driver there is a line for mx66l51235l.
According to Macronix site, there is no such part number. The chip
detected as such is actually mx66l51235f. Hence, this commit renames
the chip.
I wonder if this could pose a problem, since this might interfere with
the DT being an ABI. But I guess fixing the name is OK.
Post by Alexander Amelkin
According to the datasheet for mx66l51235f, "The device default is in
24-bit address mode" (section 9-10). Having option SPI_NOR_4B_OPCODES
makes the code act as if the device was already in 4B mode and didn't
need the EN4B command. That prevents this chip from functioning on
systems where the boot loader left the chip in 3B mode (e.g. if the
chip wasn't used during the boot process).
Hence, this commit removes the SPI_NOR_4B_OPCODES option for
mx66l51235f (added previously by commit d342b6a973af).
Could it be there are two variants of the chip, one which supports the
4B opcodes and one which does not ? Wouldn't be the first time I saw
this. If this chip supports the SFDP tables, you can check those.
Post by Alexander Amelkin
Fixes: d342b6a973af ("mtd: spi-nor: enable 4B opcodes for mx66l51235l")
---
drivers/mtd/spi-nor/spi-nor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f028277..c5ef85e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1091,7 +1091,7 @@ static const struct flash_info spi_nor_ids[] = {
{ "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
{ "mx25u25635f", INFO(0xc22539, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_4B_OPCODES) },
{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
- { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
+ { "mx66l51235f", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
{ "mx66u51235f", INFO(0xc2253a, 0, 64 * 1024, 1024, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
{ "mx66l1g45g", INFO(0xc2201b, 0, 64 * 1024, 2048, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
{ "mx66l1g55g", INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) },
--
Best regards,
Marek Vasut
Alexander Amelkin
2018-08-27 11:24:40 UTC
Permalink
Post by Marek Vasut
Post by Alexander Amelkin
Currently in spi-nor driver there is a line for mx66l51235l.
According to Macronix site, there is no such part number. The chip
detected as such is actually mx66l51235f. Hence, this commit renames
the chip.
I wonder if this could pose a problem, since this might interfere with
the DT being an ABI. But I guess fixing the name is OK.
Well, as far as I understand, in the DT you'll only have "compatible=jedec,spi-nor" for most cases.
Anyway, I did `grep -r mx66l41235 arch/*/boot/dts` and found nothing, so I guess it's safe to rename.
Post by Marek Vasut
Post by Alexander Amelkin
According to the datasheet for mx66l51235f, "The device default is in
24-bit address mode" (section 9-10). Having option SPI_NOR_4B_OPCODES
makes the code act as if the device was already in 4B mode and didn't
need the EN4B command. That prevents this chip from functioning on
systems where the boot loader left the chip in 3B mode (e.g. if the
chip wasn't used during the boot process).
Hence, this commit removes the SPI_NOR_4B_OPCODES option for
mx66l51235f (added previously by commit d342b6a973af).
Could it be there are two variants of the chip, one which supports the
4B opcodes and one which does not ? Wouldn't be the first time I saw
this. If this chip supports the SFDP tables, you can check those.
I was unable to find another variant of the chip. There is only one specified in the datasheet:
http://www.macronix.com/Lists/Datasheet/Attachments/7401/MX66L51235F,%203V,%20512Mb,%20v1.1.pdf
and it says that the device supports both 3B and 4B modes, but defaults to 3B (24-bit) mode.

Most probably, Roman Yeryomin, the author of commit d342b6a973af, was fighting post-effects of u-boot or some other boot loader that set his chip to 4B mode before jumping to Linux kernel.
It is hard to say for sure, though, as neither his patch itself, nor his post to the mailing list contained any rationale for the change.

It is for sure though that for OpenPOWER systems using this chip as PNOR (BIOS flash in x86 terms), commit d342b6a973af broke host booting.

With best regards,
Alexander.
Marek Vasut
2018-08-27 11:34:54 UTC
Permalink
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
Currently in spi-nor driver there is a line for mx66l51235l.
According to Macronix site, there is no such part number. The chip
detected as such is actually mx66l51235f. Hence, this commit renames
the chip.
I wonder if this could pose a problem, since this might interfere with
the DT being an ABI. But I guess fixing the name is OK.
Well, as far as I understand, in the DT you'll only have "compatible=jedec,spi-nor" for most cases.
Anyway, I did `grep -r mx66l41235 arch/*/boot/dts` and found nothing, so I guess it's safe to rename.
The argument can go in the direction of out-of-tree DTs . I think it's
OK to fix it too though.
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
According to the datasheet for mx66l51235f, "The device default is in
24-bit address mode" (section 9-10). Having option SPI_NOR_4B_OPCODES
makes the code act as if the device was already in 4B mode and didn't
need the EN4B command. That prevents this chip from functioning on
systems where the boot loader left the chip in 3B mode (e.g. if the
chip wasn't used during the boot process).
Hence, this commit removes the SPI_NOR_4B_OPCODES option for
mx66l51235f (added previously by commit d342b6a973af).
Could it be there are two variants of the chip, one which supports the
4B opcodes and one which does not ? Wouldn't be the first time I saw
this. If this chip supports the SFDP tables, you can check those.
http://www.macronix.com/Lists/Datasheet/Attachments/7401/MX66L51235F,%203V,%20512Mb,%20v1.1.pdf
and it says that the device supports both 3B and 4B modes, but defaults to 3B (24-bit) mode.
So keep the 4B part in. Linux will just not reconfigure the device to 4B
mode using register write, but will instead issue the 4B opcode
directly, without any stateful change.
Post by Alexander Amelkin
Most probably, Roman Yeryomin, the author of commit d342b6a973af, was fighting post-effects of u-boot or some other boot loader that set his chip to 4B mode before jumping to Linux kernel.
It's the other way around, U-Boot keeps the flash in 3B mode.
Post by Alexander Amelkin
It is hard to say for sure, though, as neither his patch itself, nor his post to the mailing list contained any rationale for the change.
It is for sure though that for OpenPOWER systems using this chip as PNOR (BIOS flash in x86 terms), commit d342b6a973af broke host booting.
I CCed him, so maybe he can clarify.

But this is weird. If you reboot the system, isn't the SPI NOR
power-cycled and reset into defined default state ? If not, your
hardware is severely broken and you should fix your reset routing.
--
Best regards,
Marek Vasut
Alexander Amelkin
2018-08-28 11:29:13 UTC
Permalink
Post by Marek Vasut
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
According to the datasheet for mx66l51235f, "The device default is in
24-bit address mode" (section 9-10). Having option SPI_NOR_4B_OPCODES
makes the code act as if the device was already in 4B mode and didn't
need the EN4B command. That prevents this chip from functioning on
systems where the boot loader left the chip in 3B mode (e.g. if the
chip wasn't used during the boot process).
Hence, this commit removes the SPI_NOR_4B_OPCODES option for
mx66l51235f (added previously by commit d342b6a973af).
Could it be there are two variants of the chip, one which supports the
4B opcodes and one which does not ? Wouldn't be the first time I saw
this. If this chip supports the SFDP tables, you can check those.
http://www.macronix.com/Lists/Datasheet/Attachments/7401/MX66L51235F,%203V,%20512Mb,%20v1.1.pdf
and it says that the device supports both 3B and 4B modes, but defaults to 3B (24-bit) mode.
So keep the 4B part in. Linux will just not reconfigure the device to 4B
mode using register write, but will instead issue the 4B opcode
directly, without any stateful change.
Marek, thank you for asking all these questions. They made me conduct a deeper investigation than just finding the commit that broke it and reverting it. It appears that OpenPOWER P8's SBE code (the thing that P8 CPU runs first from its built-in memory) expects the PNOR flash chip to be in EN4B mode. It reads the rest of OpenPOWER Firmware using a usual READ (0x03) command with 4 bytes of address. Always. That's why, I believe, Roman's commit broke host booting for us.

Probably it would be best not to merge this patch into Linux kernel. I'll discuss it with colleagues from OpenPOWER community and we'll probably fix it on SBE side.

As for renaming the chip, I can provide a patch just for that. Or we can leave it as is.

Thanks.
Alexander.
Marek Vasut
2018-08-28 11:43:17 UTC
Permalink
Hi,
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
According to the datasheet for mx66l51235f, "The device default is in
24-bit address mode" (section 9-10). Having option SPI_NOR_4B_OPCODES
makes the code act as if the device was already in 4B mode and didn't
need the EN4B command. That prevents this chip from functioning on
systems where the boot loader left the chip in 3B mode (e.g. if the
chip wasn't used during the boot process).
Hence, this commit removes the SPI_NOR_4B_OPCODES option for
mx66l51235f (added previously by commit d342b6a973af).
Could it be there are two variants of the chip, one which supports the
4B opcodes and one which does not ? Wouldn't be the first time I saw
this. If this chip supports the SFDP tables, you can check those.
http://www.macronix.com/Lists/Datasheet/Attachments/7401/MX66L51235F,%203V,%20512Mb,%20v1.1.pdf
and it says that the device supports both 3B and 4B modes, but defaults to 3B (24-bit) mode.
So keep the 4B part in. Linux will just not reconfigure the device to 4B
mode using register write, but will instead issue the 4B opcode
directly, without any stateful change.
Marek, thank you for asking all these questions. They made me conduct a deeper investigation than just finding the commit that broke it and reverting it. It appears that OpenPOWER P8's SBE code (the thing that P8 CPU runs first from its built-in memory) expects the PNOR flash chip to be in EN4B mode. It reads the rest of OpenPOWER Firmware using a usual READ (0x03) command with 4 bytes of address. Always. That's why, I believe, Roman's commit broke host booting for us.
Uh oh, I seem to remember some flashes had this weird quirk where they
were in EN4B mode by default, and if U-Boot/Linux reset the flash into
"sane default" state (3B addressing etc), some systems failed to boot.
Furthermore, this EN4B is non-volatile bit on this flash, right ? That
means it is retained across power-cycles, which sucks even more.
Post by Alexander Amelkin
Probably it would be best not to merge this patch into Linux kernel. I'll discuss it with colleagues from OpenPOWER community and we'll probably fix it on SBE side.
Can you fix the firmware ? If so, that'd be amazing.
But still, there can be a software which does rogue transmission on the
SPI bus and flips the flash into 3B mode before reset, so the system
will not boot anyway. Do you have a way to deal with that somehow ?
Post by Alexander Amelkin
As for renaming the chip, I can provide a patch just for that. Or we can leave it as is.
A patch would be nice, yes.
Post by Alexander Amelkin
Thanks.
Alexander.
--
Best regards,
Marek Vasut
Alexander Amelkin
2018-08-28 15:54:12 UTC
Permalink
Post by Marek Vasut
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
According to the datasheet for mx66l51235f, "The device default is in
24-bit address mode" (section 9-10). Having option SPI_NOR_4B_OPCODES
makes the code act as if the device was already in 4B mode and didn't
need the EN4B command. That prevents this chip from functioning on
systems where the boot loader left the chip in 3B mode (e.g. if the
chip wasn't used during the boot process).
Hence, this commit removes the SPI_NOR_4B_OPCODES option for
mx66l51235f (added previously by commit d342b6a973af).
Could it be there are two variants of the chip, one which supports the
4B opcodes and one which does not ? Wouldn't be the first time I saw
this. If this chip supports the SFDP tables, you can check those.
http://www.macronix.com/Lists/Datasheet/Attachments/7401/MX66L51235F,%203V,%20512Mb,%20v1.1.pdf
and it says that the device supports both 3B and 4B modes, but defaults to 3B (24-bit) mode.
So keep the 4B part in. Linux will just not reconfigure the device to 4B
mode using register write, but will instead issue the 4B opcode
directly, without any stateful change.
Marek, thank you for asking all these questions. They made me conduct a deeper investigation than just finding the commit that broke it and reverting it. It appears that OpenPOWER P8's SBE code (the thing that P8 CPU runs first from its built-in memory) expects the PNOR flash chip to be in EN4B mode. It reads the rest of OpenPOWER Firmware using a usual READ (0x03) command with 4 bytes of address. Always. That's why, I believe, Roman's commit broke host booting for us.
Uh oh, I seem to remember some flashes had this weird quirk where they
were in EN4B mode by default, and if U-Boot/Linux reset the flash into
"sane default" state (3B addressing etc), some systems failed to boot.
Furthermore, this EN4B is non-volatile bit on this flash, right ? That
means it is retained across power-cycles, which sucks even more.
Post by Alexander Amelkin
Post by Marek Vasut
The 4BYTE bit may be cleared by power-off or writing EX4B instruction to reset the state to be "0".
Probably it would be best not to merge this patch into Linux kernel. I'll discuss it with colleagues from OpenPOWER community and we'll probably fix it on SBE side.
Can you fix the firmware ? If so, that'd be amazing.
It turns out that it is impossible to fix that on firmware side mainly because the firmware does not send any opcodes over SPI at all. It relies on the SPI flash controller to do the address mapping so the firmware accesses the chip as a usual memory address range. The flash controller is located in our case inside Aspeed AST2400 BMC SoC and does not support 4B opcodes. It expects the chip to respond to generic READ and WRITE opcodes with (configurable) either 3 or 4 address byte cycles. So the only way to make it work is put the chip in EN4B mode when BMC boots. As this is quite a specific case, I guess you won't be willing to remove the SPI_NOR_4B_OPCODES option globally. We'll have to keep the patch local to openbmc project
Post by Marek Vasut
But still, there can be a software which does rogue transmission on the
SPI bus and flips the flash into 3B mode before reset, so the system
will not boot anyway. Do you have a way to deal with that somehow ?
Sure. As I said, there is no way to turn on the host without BMC knowing/controlling it.
The BMC ensures the chip is in the right mode. That's actually the reason why this patch appeared in the first place.
Also, there is no rogue software on BMC.
Post by Marek Vasut
Post by Alexander Amelkin
As for renaming the chip, I can provide a patch just for that. Or we can leave it as is.
A patch would be nice, yes.
Will do a bit later.

Best regards,
Alexander.
Marek Vasut
2018-08-28 16:03:58 UTC
Permalink
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
According to the datasheet for mx66l51235f, "The device
default is in 24-bit address mode" (section 9-10). Having
option SPI_NOR_4B_OPCODES makes the code act as if the
device was already in 4B mode and didn't need the EN4B
command. That prevents this chip from functioning on
systems where the boot loader left the chip in 3B mode
(e.g. if the chip wasn't used during the boot process).
Hence, this commit removes the SPI_NOR_4B_OPCODES option
for mx66l51235f (added previously by commit
d342b6a973af).
Could it be there are two variants of the chip, one which
supports the 4B opcodes and one which does not ? Wouldn't
be the first time I saw this. If this chip supports the
SFDP tables, you can check those.
I was unable to find another variant of the chip. There is
http://www.macronix.com/Lists/Datasheet/Attachments/7401/MX66L51235F,%203V,%20512Mb,%20v1.1.pdf
and it says that the device supports both 3B and 4B modes, but defaults
to 3B (24-bit) mode.
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
Post by Marek Vasut
So keep the 4B part in. Linux will just not reconfigure the
device to 4B mode using register write, but will instead issue
the 4B opcode directly, without any stateful change.
Marek, thank you for asking all these questions. They made me
conduct a deeper investigation than just finding the commit that
broke it and reverting it. It appears that OpenPOWER P8's SBE
code (the thing that P8 CPU runs first from its built-in memory)
expects the PNOR flash chip to be in EN4B mode. It reads the rest
of OpenPOWER Firmware using a usual READ (0x03) command with 4
bytes of address. Always. That's why, I believe, Roman's commit
broke host booting for us.
Uh oh, I seem to remember some flashes had this weird quirk where
they were in EN4B mode by default, and if U-Boot/Linux reset the
flash into "sane default" state (3B addressing etc), some systems
failed to boot. Furthermore, this EN4B is non-volatile bit on this
flash, right ? That means it is retained across power-cycles, which
sucks even more.
Post by Alexander Amelkin
Post by Marek Vasut
The 4BYTE bit may be cleared by power-off or writing EX4B
instruction to reset the state to be "0".
OK, that's good. I probably thought of different flash.
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
Probably it would be best not to merge this patch into Linux
kernel. I'll discuss it with colleagues from OpenPOWER community
and we'll probably fix it on SBE side.
Can you fix the firmware ? If so, that'd be amazing.
It turns out that it is impossible to fix that on firmware side
mainly because the firmware does not send any opcodes over SPI at
all. It relies on the SPI flash controller to do the address mapping
so the firmware accesses the chip as a usual memory address range.
The flash controller is located in our case inside Aspeed AST2400 BMC
SoC and does not support 4B opcodes. It expects the chip to respond
to generic READ and WRITE opcodes with (configurable) either 3 or 4
address byte cycles. So the only way to make it work is put the chip
in EN4B mode when BMC boots.
And who does that ? Or is the EN4B set by default and U-Boot/Linux
_clears_ it , thus making the board unbootable ?

Doesn't it simply mean your board is missing SPI NOR hardware reset line?
Post by Alexander Amelkin
As this is quite a specific case, I
guess you won't be willing to remove the SPI_NOR_4B_OPCODES option
globally. We'll have to keep the patch local to openbmc project
I am more worried about this being actually unfixable in reliable way.
Post by Alexander Amelkin
Post by Marek Vasut
But still, there can be a software which does rogue transmission on
the SPI bus and flips the flash into 3B mode before reset, so the
system will not boot anyway. Do you have a way to deal with that
somehow ?
Sure. As I said, there is no way to turn on the host without BMC
knowing/controlling it. The BMC ensures the chip is in the right
mode. That's actually the reason why this patch appeared in the first
place. Also, there is no rogue software on BMC.
OK, but if the BMC puts the chip in a defined state, how come the system
will refuse to (re)boot if Linux interferes with the chip's volatile
register settings ?
--
Best regards,
Marek Vasut
Alexander Amelkin
2018-08-29 11:00:39 UTC
Permalink
This post might be inappropriate. Click to display it.
Marek Vasut
2018-08-29 11:46:50 UTC
Permalink
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
Probably it would be best not to merge this patch into Linux
kernel. I'll discuss it with colleagues from OpenPOWER
community and we'll probably fix it on SBE side.
Can you fix the firmware ? If so, that'd be amazing.
It turns out that it is impossible to fix that on firmware side
mainly because the firmware does not send any opcodes over SPI
at all. It relies on the SPI flash controller to do the address
mapping so the firmware accesses the chip as a usual memory
address range. The flash controller is located in our case inside
Aspeed AST2400 BMC SoC and does not support 4B opcodes. It
expects the chip to respond to generic READ and WRITE opcodes
with (configurable) either 3 or 4 address byte cycles. So the
only way to make it work is put the chip in EN4B mode when BMC
boots.
And who does that ? Or is the EN4B set by default and U-Boot/Linux
_clears_ it , thus making the board unbootable ?
Doesn't it simply mean your board is missing SPI NOR hardware reset line?
No, you're probably just missing it. In servers the BMC is always on
when PSUs are connected to mains. The flash chip in question is the
one used to boot the host (OpenPOWER P8), not the BMC (ASpeed
AST2400).
OK
Post by Alexander Amelkin
U-boot on AST2400 has nothing to do with the chip. When BMC
boots, it loads a driver for the host's chip (because it is connected
to the BMC SoC for the BMC to be able to upgrade the host's
firmware), and the driver initializes the chip to the right mode. The
BMC reads host firmware version info, etc. from the chip. It may also
update the firmware or parts of it. Then it powers up the host and
the host reads the chip via the memory window mapped on LPC bus by
the BMC.
OK
Post by Alexander Amelkin
The BMC hardware (AST2400 in our case) translates memory
transactions into SPI Flash commands. Unfortunately for us, AST2400
(unlike newer models) only supports generic 3-byte commands (READ,
READ FAST, PP, etc.), but has a 'hack' that allows for using 4-byte
addresses with those commands. Thus if an over-16MB flash chip is
connected (and the firmware image itself is over 16MB), and thus
AST2400 is configured in 4-byte address cycle mode, it is crucial
that the flash chip is in EN4B mode as well. That's what was provided
by the spi-nor driver before Roman Yeryomin's commit and by this
patch that reverts that commit. When OpenBMC project upgraded from
kernel 4.13 to 4.17, it sucked in the commit made in version 4.15 and
thus broke the mechanism I described above.
Ah, so the whole problem is, the ASpeed 2400 SPI NOR controller does
memory-mapped read from the flash and that has some limitations when the
flash is over 16 MiB in size -- it sends 3 byte standard opcodes with 4
byte address ?
Post by Alexander Amelkin
The flash chip in question does have a hardware reset line, but it
does not matter in this case at all. Resetting the chip will bring it
into EX4B (3-byte) mode, and that's not what is needed. Basically,
that's exactly what is happening now (without this patch).
By the way, the newer AST2500 BMC chip has support for 4-byte
commands and most probably does not have this problem.
I see
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
As this is quite a specific case, I guess you won't be willing to
remove the SPI_NOR_4B_OPCODES option globally. We'll have to keep
the patch local to openbmc project
I am more worried about this being actually unfixable in reliable way.
Well, configuring the chip for EN4B mode before powering on the host
is quite reliable. There is no-one to interfere with this setup
in-between and even after the host is booted, as there is no driver
in Linux for AST2400-based SPI controller on LPC bus (as seen from
the host). There is only driver for that peripheral as seen from
BMC.
To simplify the situation, if the ASpeed 2400 SPI NOR controller tried
reading the SPI flash past 16 MiB, it would also require that the flash
is in EN4B mode, yes ?
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
Post by Marek Vasut
But still, there can be a software which does rogue
transmission on the SPI bus and flips the flash into 3B mode
before reset, so the system will not boot anyway. Do you have a
way to deal with that somehow ?
Sure. As I said, there is no way to turn on the host without BMC
knowing/controlling it. The BMC ensures the chip is in the right
mode. That's actually the reason why this patch appeared in the
first place. Also, there is no rogue software on BMC.
OK, but if the BMC puts the chip in a defined state, how come the
system will refuse to (re)boot if Linux interferes with the chip's
volatile register settings ?
Because it was BMC's Linux that got broken, not host's. The commit
fetched into BMC's linux while upgrading 4.13 to 4.17 resulted in
putting the chip in a wrong state.
I start thinking about maybe adding an option to "jedec,spi-nor" DTS
binding like "enable-4byte-mode", so that we could get rid of our
local patch and just specify that option in the devicetree. The
option would force EN4B on the chip if it supports that mode. What do
you think? How do I do it if you approve? Send patches separately in
here and in the devicetree mailing list or cross-post both patches to
both lists?
That's an option. But what I think this is really about is a fact that
we completely lack any way to negotiate limitations between the SPI NOR
and the controller.
--
Best regards,
Marek Vasut
Cédric Le Goater
2018-08-29 12:12:31 UTC
Permalink
[ ... ]
Post by Marek Vasut
Ah, so the whole problem is, the ASpeed 2400 SPI NOR controller does
memory-mapped read from the flash and that has some limitations when the
flash is over 16 MiB in size -- it sends 3 byte standard opcodes with 4
byte address ?
yes.

[ ... ]
Post by Marek Vasut
To simplify the situation, if the ASpeed 2400 SPI NOR controller tried
reading the SPI flash past 16 MiB, it would also require that the flash
is in EN4B mode, yes ?
yes.

[ ... ]
Post by Marek Vasut
Post by Alexander Amelkin
I start thinking about maybe adding an option to "jedec,spi-nor" DTS
binding like "enable-4byte-mode", so that we could get rid of our
local patch and just specify that option in the devicetree. The
option would force EN4B on the chip if it supports that mode. What do
you think? How do I do it if you approve? Send patches separately in
here and in the devicetree mailing list or cross-post both patches to
both lists?
That's an option. But what I think this is really about is a fact that
we completely lack any way to negotiate limitations between the SPI NOR
and the controller.
what about the hwcaps in spi_nor_scan() ?

Thanks,

C.
Cyrille Pitchen
2018-08-30 14:16:01 UTC
Permalink
Hi all,
Post by Cédric Le Goater
[ ... ]
Post by Marek Vasut
Ah, so the whole problem is, the ASpeed 2400 SPI NOR controller does
memory-mapped read from the flash and that has some limitations when the
flash is over 16 MiB in size -- it sends 3 byte standard opcodes with 4
byte address ?
yes.
[ ... ]
Post by Marek Vasut
To simplify the situation, if the ASpeed 2400 SPI NOR controller tried
reading the SPI flash past 16 MiB, it would also require that the flash
is in EN4B mode, yes ?
yes.
[ ... ]
Post by Marek Vasut
Post by Alexander Amelkin
I start thinking about maybe adding an option to "jedec,spi-nor" DTS
binding like "enable-4byte-mode", so that we could get rid of our
local patch and just specify that option in the devicetree. The
option would force EN4B on the chip if it supports that mode. What do
you think? How do I do it if you approve? Send patches separately in
here and in the devicetree mailing list or cross-post both patches to
both lists?
That's an option. But what I think this is really about is a fact that
we completely lack any way to negotiate limitations between the SPI NOR
and the controller.
what about the hwcaps in spi_nor_scan() ?
Maybe only a matter of taste but I agree with Cedric: a new hwcaps sounds
better than a new DT property. There are already different "compatible"
strings to make the difference between AST2400 and AST2500, so we can
add the new hwcap only to AST2400 entries. Then no need to update any
existing device trees, at least in this case.

Best regards,

Cyille
Post by Cédric Le Goater
Thanks,
C.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Marek Vasut
2018-08-30 14:17:12 UTC
Permalink
Post by Cyrille Pitchen
Hi all,
Post by Cédric Le Goater
[ ... ]
Post by Marek Vasut
Ah, so the whole problem is, the ASpeed 2400 SPI NOR controller does
memory-mapped read from the flash and that has some limitations when the
flash is over 16 MiB in size -- it sends 3 byte standard opcodes with 4
byte address ?
yes.
[ ... ]
Post by Marek Vasut
To simplify the situation, if the ASpeed 2400 SPI NOR controller tried
reading the SPI flash past 16 MiB, it would also require that the flash
is in EN4B mode, yes ?
yes.
[ ... ]
Post by Marek Vasut
Post by Alexander Amelkin
I start thinking about maybe adding an option to "jedec,spi-nor" DTS
binding like "enable-4byte-mode", so that we could get rid of our
local patch and just specify that option in the devicetree. The
option would force EN4B on the chip if it supports that mode. What do
you think? How do I do it if you approve? Send patches separately in
here and in the devicetree mailing list or cross-post both patches to
both lists?
That's an option. But what I think this is really about is a fact that
we completely lack any way to negotiate limitations between the SPI NOR
and the controller.
what about the hwcaps in spi_nor_scan() ?
Maybe only a matter of taste but I agree with Cedric: a new hwcaps sounds
better than a new DT property. There are already different "compatible"
strings to make the difference between AST2400 and AST2500, so we can
add the new hwcap only to AST2400 entries. Then no need to update any
existing device trees, at least in this case.
Good idea :)
--
Best regards,
Marek Vasut
Cédric Le Goater
2018-08-29 12:05:46 UTC
Permalink
Post by Alexander Amelkin
The flash chip in question is the one used to boot the host (OpenPOWER P8),
not the BMC (ASpeed AST2400). U-boot on AST2400 has nothing to do with
the chip.
yes. The Aspeed SoCs has multiple SMC Controllers.

On the AST2400, the first SMC controller (called FMC) is for the
BMC Firmware and supports SPI, NOR, NAND flash devices. The second
is a SPI only controller for the host Firmware. In the OpenBMC
terminology, the flash device holding the host Firmware is referred
as the "PNOR" ...

On the AST2500, the FMC controller supports SPI and NOR flash devices.
There are two other controllers supporting only SPI, the first is used
for the host Firmware.


OpenPOWER platforms using the AST2500 don't have the problem because
the flash content is accessed differently from the host.

[ ... ]
Post by Alexander Amelkin
I start thinking about maybe adding an option to "jedec,spi-nor" DTS
binding like "enable-4byte-mode", so that we could get rid of our local
patch and just specify that option in the devicetree. The option would
force EN4B on the chip if it supports that mode. What do you think?
Are you proposing to add an extra property that would be handled at
the spi-nor level to choose which set of SPI commands should be the
default read_opcode ? either the ones requiring EN4B (SPINOR_OP_READ*)
or the others which would not (SPINOR_OP_READ*_4B) ?

I think it would be better to introduce the property in the aspeed-smc
driver because this is very specific usage of the Aspeed SoC done
by a platform.

and/or, maybe, introduce a new SNOR_HWCAPS to inform spi_nor_scan()
that some driver we doesn't want the SPI_NOR_4B_OPCODES ?
Post by Alexander Amelkin
How do I do it if you approve? Send patches separately in here and in > the devicetree mailing list or cross-post both patches to both lists?
You need to send a patch to both mailing lists, openbmc@ and linux-mtd@

Thanks,

C.
Roman Yeryomin
2018-08-29 21:16:57 UTC
Permalink
Post by Marek Vasut
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
Currently in spi-nor driver there is a line for mx66l51235l.
According to Macronix site, there is no such part number. The chip
detected as such is actually mx66l51235f. Hence, this commit renames
the chip.
I wonder if this could pose a problem, since this might interfere with
the DT being an ABI. But I guess fixing the name is OK.
Well, as far as I understand, in the DT you'll only have "compatible=jedec,spi-nor" for most cases.
Anyway, I did `grep -r mx66l41235 arch/*/boot/dts` and found nothing, so I guess it's safe to rename.
The argument can go in the direction of out-of-tree DTs . I think it's
OK to fix it too though.
Post by Alexander Amelkin
Post by Marek Vasut
Post by Alexander Amelkin
According to the datasheet for mx66l51235f, "The device default is in
24-bit address mode" (section 9-10). Having option SPI_NOR_4B_OPCODES
makes the code act as if the device was already in 4B mode and didn't
need the EN4B command. That prevents this chip from functioning on
systems where the boot loader left the chip in 3B mode (e.g. if the
chip wasn't used during the boot process).
Hence, this commit removes the SPI_NOR_4B_OPCODES option for
mx66l51235f (added previously by commit d342b6a973af).
Could it be there are two variants of the chip, one which supports the
4B opcodes and one which does not ? Wouldn't be the first time I saw
this. If this chip supports the SFDP tables, you can check those.
http://www.macronix.com/Lists/Datasheet/Attachments/7401/MX66L51235F,%203V,%20512Mb,%20v1.1.pdf
and it says that the device supports both 3B and 4B modes, but defaults to 3B (24-bit) mode.
So keep the 4B part in. Linux will just not reconfigure the device to 4B
mode using register write, but will instead issue the 4B opcode
directly, without any stateful change.
Post by Alexander Amelkin
Most probably, Roman Yeryomin, the author of commit d342b6a973af, was fighting post-effects of u-boot or some other boot loader that set his chip to 4B mode before jumping to Linux kernel.
It's the other way around, U-Boot keeps the flash in 3B mode.
Yep, usually (well I've never seen it doing differently) u-boot
doesn't alter access mode for the flash, since Linux kernel usually is
close to beginning and reachable using 3B mode.
Post by Marek Vasut
Post by Alexander Amelkin
It is hard to say for sure, though, as neither his patch itself, nor his post to the mailing list contained any rationale for the change.
It is for sure though that for OpenPOWER systems using this chip as PNOR (BIOS flash in x86 terms), commit d342b6a973af broke host booting.
I CCed him, so maybe he can clarify.
But this is weird. If you reboot the system, isn't the SPI NOR
power-cycled and reset into defined default state ? If not, your
hardware is severely broken and you should fix your reset routing.
I guess it was clarified below already but will say it again: EN4B is
not reset on soft reboot, there is no power cycle. So the system won't
boot.
There was a quirk for shutdown, which would EX4B. But it was clumsy
and probably never sent upstream. SPI_NOR_4B_OPCODES is much more
elegant.
Sorry for not clarifying this in commit message.

Regards,
Roman
Cédric Le Goater
2018-08-31 08:04:21 UTC
Permalink
[ ... ]
Post by Roman Yeryomin
Post by Marek Vasut
I CCed him, so maybe he can clarify.
But this is weird. If you reboot the system, isn't the SPI NOR
power-cycled and reset into defined default state ? If not, your
hardware is severely broken and you should fix your reset routing.
I guess it was clarified below already but will say it again: EN4B is
not reset on soft reboot, there is no power cycle. So the system won't
boot.
I am working on an Aspeed SoC SPI driver for U-Boot and I am seeing the
exact same problem with a W25Q256 when Linux reboots. These chips are
even more perfidious because they have a non-volatile bit making the
chip operate in 4Byte address mode at power-up.
Post by Roman Yeryomin
There was a quirk for shutdown, which would EX4B. But it was clumsy
and probably never sent upstream.
But the problem is supporting older Linux versions not using the
4B_OPCODES. So adding an initial EX4B in the U-Boot SPI layer seems
quite safe and not that ugly. Or add 4Byte support.

This is a discussion for U-Boot.
Post by Roman Yeryomin
SPI_NOR_4B_OPCODES is much more elegant.
I agree that the SPI_NOR_4B_OPCODES are a good practice because they
don't alter the chip state and let software do the appropriate
configuration.

Is removing EN4B/EX4B part of the long-term plan for the Linux SPI-NOR
layer ?
Post by Roman Yeryomin
Sorry for not clarifying this in commit message.
Thanks,

C.

Loading...