| Message ID | c27b9cb1-33a1-221d-abd8-30050fe2b23b@yadro.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | mtd: spi-nor: fix options for mx66l51235f | expand |
On 08/20/2018 12:26 PM, Alexander Amelkin wrote: > 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. > 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. > Cc: Marek Vasut <marek.vasut@gmail.com> > Cc: <linux-mtd@lists.infradead.org> > Cc: <openbmc@lists.ozlabs.org> > Cc: Joel Stanley <joel@jms.id.au> > Fixes: d342b6a973af ("mtd: spi-nor: enable 4B opcodes for mx66l51235l") > Signed-off-by: Alexander Amelkin <a.amelkin@yadro.com> > Reviewed-by: Cédric Le Goater <clg@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) }, >
27.08.2018 13:33, Marek Vasut wrote: > On 08/20/2018 12:26 PM, Alexander Amelkin wrote: >> 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. >> 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.
On 08/27/2018 01:24 PM, Alexander Amelkin wrote: > 27.08.2018 13:33, Marek Vasut wrote: >> On 08/20/2018 12:26 PM, Alexander Amelkin wrote: >>> 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. >>> 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. 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. > 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. > 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.
27.08.2018 14:34, Marek Vasut wrote: > >>>> 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. > 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.
On 08/28/2018 01:29 PM, Alexander Amelkin wrote: > 27.08.2018 14:34, Marek Vasut wrote: Hi, >>>>> 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. >> 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. > 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 ? > 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. > Thanks. > Alexander. >
28.08.2018 14:43, Marek Vasut wrote: > On 08/28/2018 01:29 PM, Alexander Amelkin wrote: >> 27.08.2018 14:34, Marek Vasut wrote: >>>>>> 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. >>> 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. No, EN4B is volatile. It doesn't survive power cycling. A quote from datasheet: >>> 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 > 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. >> 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.
On 08/28/2018 05:54 PM, Alexander Amelkin wrote: > 28.08.2018 14:43, Marek Vasut wrote: >> On 08/28/2018 01:29 PM, Alexander Amelkin wrote: >>> 27.08.2018 14:34, Marek Vasut wrote: >>>>>>> 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. >>>> 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. > No, EN4B is volatile. It doesn't survive power cycling. A quote from > datasheet: >>>> 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. >>> 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? > 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. >> 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 ?
28.08.2018 19:03, Marek Vasut wrote: > >>>> 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). 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. 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. 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. > >> 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. > >>> 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? Best regards, Alexander.
On 08/29/2018 01:00 PM, Alexander Amelkin wrote: > > 28.08.2018 19:03, Marek Vasut wrote: >> >>>>> 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 > 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 > 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 ? > 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 >>> 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 ? >>>> 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.
> 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. [ ... ] > 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 ? > 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.
[ ... ] > 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. [ ... ] > 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. [ ... ] >> 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.
29.08.2018 15:05, Cédric Le Goater wrote: > >> 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) ? Not exactly. Currently, spi-nor driver for devices over 16MB in size checks whether or not the SPI_NOR_4B_OPCODES is set. If it is not, it simply issues an EN4B command to set legacy opcodes to 4-byte mode. If the option is there, the driver replaces pointers to legacy-3-byte-command-based functions with pointers to modern-4-byte-comand-based functions. My proposal is to add an option that would force EN4B always, leaving the rest as is. That is, the driver will still use 4-byte commands if they are supported, but the default mode of the chip will be switched to 4-byte as well. > 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. The aspeed-smc driver configures the Aspeed SMC controller, not the flash chip. > 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 ? If I understand correctly, SNOR_HWCAPS are defined as per JEDEC standard and are set according to chip's SFDP table. Where do you propose the information that "we don't want SPI_NOR_4B_OPCODES" to come from? My proposal is to put the option into DTS where it quite naturally belongs because it's a platform-specific configuration. > >> 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@ Sorry, I wasn't asking about openbmc@. I was asking about devicetree@ and @linux-mtd. Alexander.
[ ... ] > Not exactly. Currently, spi-nor driver for devices over 16MB in size checks > whether or not the SPI_NOR_4B_OPCODES is set. If it is not, it simply issues > an EN4B command to set legacy opcodes to 4-byte mode. If the option is there, > the driver replaces pointers to legacy-3-byte-command-based functions with pointers > to modern-4-byte-comand-based functions. yes > My proposal is to add an option that would force EN4B always, leaving the > rest as is. ok. so you want to force the call to set_4byte() in spi_nor_init(). There : ... if ((nor->addr_width == 4) && (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) && !(nor->info->flags & SPI_NOR_4B_OPCODES)) set_4byte(nor, nor->info, 1); It should be possible initiate a SPI transfer from the driver also but I am sure this is acceptable. > That is, the driver will still use 4-byte commands if they are supported, > but the default mode of the chip will be switched to 4-byte as well. ok. >> 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. > The aspeed-smc driver configures the Aspeed SMC controller, not the flash chip. Not exactly, yes, but it does configure the controller registers for a specific flash chip: timings, opcodes, hclk, dummies. >> 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 ? > > If I understand correctly, SNOR_HWCAPS are defined as per JEDEC standard > and are set according to chip's SFDP table. and according to the SPI controller. The driver performs a scan with its capabilities. So we could extend the hwcaps struct for this purpose. > Where do you propose the information that "we don't want SPI_NOR_4B_OPCODES" > to come from? The DT. > My proposal is to put the option into DTS where it quite naturally belongs > because it's a platform-specific configuration. yes but I don't think we can ask the common SPI-NOR layer to handle a platform specific property. We need a way to provide that information. >>> 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@ > Sorry, I wasn't asking about openbmc@. I was asking about devicetree@ and @linux-mtd. ok but I think we can keep openbmc@ in the loop too. Thanks, C.
29.08.2018 16:31, Cédric Le Goater wrote: > >> My proposal is to add an option that would force EN4B always, leaving the >> rest as is. > ok. so you want to force the call to set_4byte() in spi_nor_init(). Exactly! > >> My proposal is to put the option into DTS where it quite naturally belongs >> because it's a platform-specific configuration. > yes but I don't think we can ask the common SPI-NOR layer to handle a > platform specific property. We need a way to provide that information. Well, there is already a "broken-flash-reset" option that is also very platform-specific. > ok but I think we can keep openbmc@ in the loop too. Sure. Alexander.
On Mon, 27 Aug 2018 at 14:35, Marek Vasut <marek.vasut@gmail.com> wrote: > > On 08/27/2018 01:24 PM, Alexander Amelkin wrote: > > 27.08.2018 13:33, Marek Vasut wrote: > >> On 08/20/2018 12:26 PM, Alexander Amelkin wrote: > >>> 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. > > >>> 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. > > 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. > > > 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. > > 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
Hi all, Le 29/08/2018 à 14:12, Cédric Le Goater a écrit : > [ ... ] > >> 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. > > [ ... ] > >> 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. > > [ ... ] > >>> 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 > Thanks, > > C. > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ >
On 08/30/2018 04:16 PM, Cyrille Pitchen wrote: > Hi all, > > Le 29/08/2018 à 14:12, Cédric Le Goater a écrit : >> [ ... ] >> >>> 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. >> >> [ ... ] >> >>> 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. >> >> [ ... ] >> >>>> 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 :)
[ ... ] >> 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. > 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. > 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 ? > Sorry for not clarifying this in commit message. Thanks, C.
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) },
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 <marek.vasut@gmail.com> Cc: <linux-mtd@lists.infradead.org> Cc: <openbmc@lists.ozlabs.org> Cc: Joel Stanley <joel@jms.id.au> Fixes: d342b6a973af ("mtd: spi-nor: enable 4B opcodes for mx66l51235l") Signed-off-by: Alexander Amelkin <a.amelkin@yadro.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> --- drivers/mtd/spi-nor/spi-nor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)