Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linux: Backport MTD patch to support memory size above 128Mib #620

Closed
wants to merge 3 commits into from

Conversation

@corny corny force-pushed the digineo:mtd-patch branch from 682e57a to 6ed295d Dec 17, 2016

@NeoRaider

This comment has been minimized.

Copy link
Member

commented Dec 17, 2016

I think it would make sense to move these patches to target/linux/generic/patches-4.4, where the other MTD patches are.

They could also need a refresh:

Applying /home/neoraider/Devel/LEDE/source/target/linux/ramips/patches-4.4/0400-mtd-spi-nor-rename-SPINOR-macros.patch using plaintext:
patching file drivers/mtd/devices/serial_flash_cmds.h
patching file drivers/mtd/devices/st_spi_fsm.c
patching file drivers/mtd/spi-nor/spi-nor.c
Hunk #1 succeeded at 1349 (offset -141 lines).
patching file include/linux/mtd/spi-nor.h
Hunk #1 succeeded at 42 (offset -1 lines).
Hunk #2 succeeded at 59 (offset -1 lines).

Applying /home/neoraider/Devel/LEDE/source/target/linux/ramips/patches-4.4/0401-mtd-spi-nor-add-a-stateless-method-to-support-memory-size-above-128Mib.patch using plaintext:
patching file drivers/mtd/spi-nor/spi-nor.c
Hunk #1 succeeded at 68 (offset -2 lines).
Hunk #2 succeeded at 185 (offset -7 lines).
Hunk #3 succeeded at 1398 (offset -141 lines).

See https://lede-project.org/docs/guide-developer/use-patches-with-buildsystem for a guide to the handling of patch files in LEDE.

@corny corny force-pushed the digineo:mtd-patch branch 2 times, most recently from 4f3cfe2 to 151e9d1 Dec 17, 2016

@corny

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2016

ok, done

@NeoRaider

This comment has been minimized.

Copy link
Member

commented Dec 18, 2016

It seems one of the patches was marked as "Superseded" in upstream patchwork: http://patchwork.ozlabs.org/patch/704050/ , but I can't find a new version...

I don't feel qualified to review this myself, so I'd like to wait for some version the patches to hit http://git.infradead.org/linux-mtd.git before merging them (unless someone else from the LEDE team wants to step up and take them).

@NeoRaider

This comment has been minimized.

Copy link
Member

commented Dec 18, 2016

Another thing: The commit message is missing a rationale. It should explain what issues on what devices these patches fix.

@dmke

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2016

These patches are related to a soft reset issue, described in this Gist (see also #464).

It basically moves from a stateful MTD addressing mode to a stateless one.

Alternatives to this are either modifying the bootloader to try 3-byte addressing if it detects a failed 4-byte read, or resetting the memory just before the soft reset (depends on the actual memory chip, for Winbond W25Q256 this equals to writing 0x66 0x99).

@dmke

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2016

There has been an update:

@corny corny force-pushed the digineo:mtd-patch branch 2 times, most recently from 6759795 to f437b25 Dec 20, 2016

@corny

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2016

I updated my pull request to use most recent patches.

@corny corny force-pushed the digineo:mtd-patch branch from f437b25 to 926d135 Dec 20, 2016

@corny

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2017

Is still anything missing?

@mkresin

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2017

No there isn't really something missing. The problem is that the most recent patches (v3) are marked as superseded in patchwork but a v4 wasn't send yet.

Edit:

A v4 was send today but due to issues with the linux-mtd mailing list, the patch didn't made it into linux-mtd patchwork. Fortunately the patch was send to linux-kernel and linux-spi as well and can be found in the linux-spi patchwork:

For sure, it is an annoying issue, but maybe we should wait for a final version (the one that gets merged in the end) instead of updating the patches in LEDE over and over again.

@corny

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2017

Ok, I agree to wait for the final version that gets merged.

@mkresin

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2017

The final versions of the patches are now accepted in the linux-mtd patchwork and are part of https://github.com/spi-nor/linux/commits/next (which will be pulled into linux-mtd.git at some point):

Please take care to refresh all patches touching the same file. For each target the generic patches applied first and the target specific ones afterwards. That is why these patches could have an impact on targets patching the same file as well.

The usual way to refresh all patches is to select a target via make menuconfig and run make target/linux/{clean,refresh}. Since it is a lot of work, KanjiMonster wrote a script to automate this: https://gist.github.com/KanjiMonster/dfcd1af3190aa7b2941f1bdcb119f25e.

Use the script the following ./refresh_all_targets_patches.sh 4.4 4.4.42 and fix the conflicts.

@corny corny force-pushed the digineo:mtd-patch branch from 926d135 to be97453 Jan 19, 2017

@corny

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2017

I updated the Patches, but they don't work for me. The device hangs instead of rebooting. Has anyone else affected devices that can be tested?

#define SPI_NOR_QUAD_READ 0x40 /* Flash supports Quad Read */
#define USE_FSR 0x80 /* use flag status register */
#define SPI_NOR_HAS_LOCK 0x100 /* Flash supports lock/unlock via SR */
+#define SPI_NOR_4B_OPCODES BIT(11) /*
+ * Use dedicated 4byte address op codes
+ * to support memory size above 128Mib.
+ */
};

This comment has been minimized.

Copy link
@naobsd

naobsd Jan 19, 2017

Contributor

this hunk and 2nd URI in commit log seem to be v3...
(code should be logically same)

This comment has been minimized.

Copy link
@corny

corny Jan 27, 2017

Author Contributor

Yes, I put the wrong URI into my commit, but this hunk is equal for v3 and v4.

This comment has been minimized.

Copy link
@naobsd

naobsd Jan 28, 2017

Contributor

sure. sorry!

@cstratton

This comment has been minimized.

Copy link

commented Jan 21, 2017

In the case of something like a W25Q256 on an MT7688, you can't fix it in the bootloader, as the actual issue is that the hardcoded (state machine or bootrom) boot behavior of the CPU will on reset try to read the chip in whichever 3 or 4 byte mode is selected by the hosting board's mode strappings of assorted pins. If that doesn't match the current mode of the chip, the bootloader transfers U-Boot (or whatever) offset by one byte, and boot fails - typically you see endless SPI access with a scope, while a logic analyzer will show the address size mismatch - meanwhile the system appears dead with nothing on the console or any LEDs, and even JTAG can get in an odd state.

Essentially, whatever addressing mode the bootmode strapping selects (or whatever mode the flash chip's power-on-behavior register is programmed to) is the mode that chip has to stay in for all time (during both U-Boot and Linux), otherwise reset (software, hardware, or watchdog) will result in a hang if it occurs at any time when the flash chip has been set to a mode other than the power on expectation.

corny added 3 commits Dec 17, 2016
linux: Backport MTD patch to support memory size above 128Mib
These patches are related to a soft reset issue, described in #464.
It basically moves from a stateful MTD addressing mode to a stateless one.
Affected devices are Newifi-D1 and Digineo AC1200 Pro.

I omited the spi-bcm-qspi.c since it does not exist in the used kernel.

source:
- http://lists.infradead.org/pipermail/linux-mtd/2016-December/070890.html
- http://lists.infradead.org/pipermail/linux-mtd/2016-December/070891.html

Signed-off-by: Julian Kornberger <jk+github@digineo.de>

@corny corny force-pushed the digineo:mtd-patch branch 2 times, most recently from fee6c13 to 028f7fd Jan 27, 2017

@corny

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2017

I tried with SPI_NOR_4B_OPCODES = BIT(10) and SPI_NOR_4B_OPCODES = BIT(11) without success.

@nbd168

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2017

What's the status of this? Does the backport resolve these issues? If so, then please squash the update commits into the main one so it can be applied properly.
Please also port port this to 4.9

@corny

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2017

I received a mail from @CyrillePitchen but did not investigate further yet:

Hi Julian,

what is the exact part number of the SPI memory embedded on your ZBT-wg3526
board? Which manufacturer? Spansion, Macronix, Micron, Winbond, SST, other?

Do you have the kernel boot logs? Is the SPI memory probed correctly?
Did you put the SPI_NOR_4B_OPCODES info->flags in the relevant entry of the
spi_nor_ids[] array? This is needed unless your board embeds a Spansion memory.

Without the SPI_NOR_4B_OPCODES flag, it should work just as before the
patch: making the SPI nor memory enter its statefull 4-byte address mode.

With the flag, only the (Fast) Read, Page Program and Sector Erase op codes
are substituted. So if the SPI memory fully supports the 4-byte address
instruction set, there is no reason it doesn't work.

You may try to add some traces in the spi-nor.c driver to see what happens
and where it blocks if spi_nor_scan() fails.

Best regards,

Cyrille

I'm going to take another look at it next week.

@cstratton

This comment has been minimized.

Copy link

commented Feb 12, 2017

@TeutonJon78

This comment has been minimized.

Copy link

commented Feb 13, 2017

The Archer C2600 v1.1 also has an affected NOR flash chip that is affected by the addressing size issue.

It has the mx25u25635f chip (so same family as the aforementined chip).

@pepe2k pepe2k added the kernel label Mar 27, 2017

@mkresin

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2017

The best explanation for 3-byte and 4-byte thinghy I found so far can be read in the commit message introducing stateless 4-byte mode: http://lists.infradead.org/pipermail/linux-mtd/2013-March/046136.html

Summary:

  • 3-byte address mode can only be used to address the first 16 MByte of the flash
  • 4-byte address mode has to be used to address the flash behind this boundary
  • there are two 4-byte modes:
    • stateless: flash is in 3-byte address mode and special (4-byte aware) op codes are used to read, program and erase the flash chip
    • statefull: 4-byte addresses are expected

The issue you are seeing is related to the "dumb boot sequence" performed by some SoCs, which expect the flash in 3-byte mode while reading the bootloader. For the SoC the 3-byte mode is fine since only the bootloader has to be read from flash and the bootloader is usually within the first 16MByte of the flash. But the kernel (and bootloader if kernel is behind the 16MB boundary) needs to enable the 4-byte mode to have access to the whole available flash.

Cyrilles patches only allows to enable stateless 4-byte on a per flash chip base. It is done by adding SPI_NOR_4B_OPCODES to the spi_nor_ids array for flash chips supporting stateless 4-byte mode. As long as this flag isn't present, the stateless mode is only used for spansion flash chips by default. Long story short, add this flag to get it working.

The MX25U25635F used in the Archer C2600 v1.1 supports all required stateless 4-byte op codes according to page 18 - Read/Write Aray Commands (4 Byte Address Command Set). A patch for enabling stateless 4b support has been committed upstream and was backported to LEDE.

But SPI_NOR_4B_OPCODES will not work for all flash chips. The Winbond W25Q256FV used in the Digineo AC1200 Pro/ZBT-WG3526 32MB for example, supports the stateless 4-byte op codes only for reading and not for program or erase (see datasheet page 25 - Instruction Set Table 1. For these chips we still need that clumsy "reset flash chip" or "switch chip to 3-byte mode" on shutdown. But even this would not fix the reboot bug entirely. If for example the watchdog resets the board or a reboot as a result of a kernel panic is done, the flash chip would be still in 4-byte statefull mode and the system hangs on reboot. The only proper solution for these flash chips is hardware design fix, by attaching the flash reset pin to the SoC reset pin or similar. For unknown reasons it isn't done by all OEM/ODM.

As I wrote in PR#464 the/a chip reset should not be done unconditionally as it can be seen here, here and here, since it isn't required for all flash chips. It should be done conditionally. One idea I had was a device tree property like reset-on-shutdown or a new spi_nor_ids array flag. If possible the workaround should be accepted upstream or upstream should at least signalise it is the best we can do in software.

@mkresin

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2017

Beside the fact that there are still three commits instead of one, the PR as it is now causes build errors on bcm53xx, brcm63xx and layerscape. Cyrilles patches were backported to kernel 4.9 in LEDE with https://git.lede-project.org/174ce4c56d742712696e546eecee3d89e05c909a.

I would suggest to close this PR, since a lot targets got kernel 4.9 support in the meantime. And at least from my point of view it doesn't make much sense to backport these commits to the lede-17.01 branch and risk some kind of breakage.

@cstratton

This comment has been minimized.

Copy link

commented Apr 19, 2017

@pepe2k

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

@mkresin

I would suggest to close this PR, since a lot targets got kernel 4.9 support in the meantime. And at least from my point of view it doesn't make much sense to backport these commits to the lede-17.01 branch and risk some kind of breakage.

If there are no more comments I also would like to see this closed.
Your explanation is perfect and, in my opinion, fully explains the problem.

Cheers,
Piotr

@mkresin

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2017

I'm going to close this one due to inactivity. Please reopen the PR if you have time to address the outlined issues.

@mkresin mkresin closed this Apr 28, 2017

@corny

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2017

@corny corny deleted the digineo:mtd-patch branch Jul 21, 2017

@cstratton

This comment has been minimized.

Copy link

commented Jul 21, 2017

zorun pushed a commit to zorun/source that referenced this pull request Aug 27, 2017
Baptiste Jonglez
kernel: mtd: Backport SPI NOR support for stateless 4 bytes opcodes
This is needed for an upcoming fix on devices that need to keep the NOR
flash in 3-bytes mode.  Unfortunately, these devices are scattered across
several targets (FS#179, FS#570), so it seems best to backport this to the
generic target.

A lot of patches are refreshed, but most are trivial.

Interesting refresh on layerscape:

- 1105-mtd-spi-nor-add-DDR-quad-read-support.patch: this patch introduces
  a new opcode SPINOR_OP_READ4_1_4_4_D.  Refresh it to change the name of
  the opcode to SPINOR_OP_READ_1_4_4_D_4B, and integrate it in the new
  opcode translation code introduced by this backport.

Also of interest, rename a few occurrences of SPINOR_OP_READ4_* introduced
in the following patches:

- brcm63xx: 345-MIPS-BCM63XX-fixup-mapped-SPI-flash-access-on-boot.patch
- layerscape: 1101-mtd-spi-nor-fsl-quadspi-extend-support-for-some-spec.patch
- layerscape: 1104-mtd-fsl-quadspi-Add-quad-mode-for-flash-n25q128.patc
- layerscape: 1106-mtd-fsl-quadspi-add-DDR-quad-read-for-Spansion.patch

This is a respin of the first attempt at lede-project#620

Build-tested on all targets, run-tested on Archer C2600 v1.1 (ipq806x).
@zorun

This comment has been minimized.

Copy link

commented Aug 27, 2017

Thanks for the work, I have spent time to re-spin this for lede-17.01, see #1320

zorun pushed a commit to zorun/source that referenced this pull request Aug 27, 2017
Baptiste Jonglez
kernel: mtd: Backport SPI NOR support for stateless 4 bytes opcodes
This is needed for an upcoming fix on devices that need to keep the NOR
flash in 3-bytes mode.  Unfortunately, these devices are scattered across
several targets (FS#179, FS#570), so it seems best to backport this to the
generic target.

A lot of patches are refreshed, but most are trivial.

Interesting refresh on layerscape:

- 1105-mtd-spi-nor-add-DDR-quad-read-support.patch: this patch introduces
  a new opcode SPINOR_OP_READ4_1_4_4_D.  Refresh it to change the name of
  the opcode to SPINOR_OP_READ_1_4_4_D_4B, and integrate it in the new
  opcode translation code introduced by this backport.

Also of interest, rename a few occurrences of SPINOR_OP_READ4_* introduced
in the following patches:

- brcm63xx: 345-MIPS-BCM63XX-fixup-mapped-SPI-flash-access-on-boot.patch
- layerscape: 1101-mtd-spi-nor-fsl-quadspi-extend-support-for-some-spec.patch
- layerscape: 1104-mtd-fsl-quadspi-Add-quad-mode-for-flash-n25q128.patc
- layerscape: 1106-mtd-fsl-quadspi-add-DDR-quad-read-for-Spansion.patch

This is a respin of the first attempt at lede-project#620

Build-tested on all targets, run-tested on Archer C2600 v1.1 (ipq806x).

Signed-off-by: Baptiste Jonglez <git@bitsofnetworks.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.