Fix the issue that reset from u boot proper gets stuck in spl - #91
Conversation
Currently 4-byte adressing mode function is not exported, but since we plan to use it outside of the SPI NOR core we need to export it. While we are here, rename it to align the naming with the rest of exported functions. Signed-off-by: Robert Marko <robert.marko@sartura.hr> Signed-off-by: Jie Zhang <jie.zhang@analog.com>
Extend spi_flash_std_remove() to exit 4-byte addressing mode before the device is torn down. This ensures that on board reset the flash is left in 3-byte mode, which is required by most SPL environments. The exit is skipped for Spansion parts and for devices that use 4-byte opcodes (SPI_NOR_4B_OPCODES) or octal DTR mode, as those don't rely on the addressing mode register. Signed-off-by: Robert Marko <robert.marko@sartura.hr> Signed-off-by: Jie Zhang <jie.zhang@analog.com>
Before triggering a system reset, remove all active SPI flash devices via device_remove(). This causes the driver's .remove callback to run, which exits 4-byte address mode. This is necessary for flash devices that don't use 4-byte opcodes (SPI_NOR_4B_OPCODES), since the SPL accesses flash using 3-byte addressing. Using device_remove() leverages existing DM infrastructure: it already guards against unprobed devices and properly tears down device state, with no need for a separate on_reset hook in struct driver. Signed-off-by: Jie Zhang <jie.zhang@analog.com>
IS25LP01G and similar ISSI parts have the same issue as Winbond W25Q256FV: the EX4B command alone does not reliably exit 4-byte address mode because the Extended Address Register EXTADD bit (bit 7) remains set, causing subsequent 3-byte-address reads to access the wrong 16 MiB region. Extend the post-EX4B clear-EAR sequence to also cover ISSI manufacturers. Signed-off-by: Jie Zhang <jie.zhang@analog.com>
Add a DM-based system reset driver for Analog Devices SC5xx SoCs that writes the SYSRST bit in the Reset Control Unit (RCU) CTL register to trigger a system reset via UCLASS_SYSRESET. The driver binds to the existing "adi,reset-controller" DT node, keeping U-Boot DTS consistent with the Linux kernel. Signed-off-by: Jie Zhang <jie.zhang@analog.com>
Enable the "adi,reset-controller" RCU node (status = "okay") in sc58x and sc59x dtsi files so the sysreset driver can probe it. Add the missing RCU node for sc57x, which was not present in U-Boot DTS. Signed-off-by: Jie Zhang <jie.zhang@analog.com>
Remove rcu.c, which registered the "adi,reset-controller" node as UCLASS_SYSCON but was never queried by any driver. The node is now handled by the sysreset driver. Guard the bare reset_cpu() in soc.c with #if !CONFIG_IS_ENABLED(SYSRESET) to avoid a duplicate symbol when the DM-based sysreset driver is active. Signed-off-by: Jie Zhang <jie.zhang@analog.com>
dec9d38 to
340fa41
Compare
|
I will check it in detail but I just wanted to let you know that it would be good if you run qconfig on the configuration files that you edited. It formats configs and check for any problems. |
Enable CONFIG_SYSRESET and CONFIG_SYSRESET_SC5XX in all SC5xx defconfigs to activate the DM-based sysreset driver. Signed-off-by: Jie Zhang <jie.zhang@analog.com>
340fa41 to
bff8d79
Compare
Now I see. defconfigs updated. Thanks. |
vasbimpikasadi
left a comment
vasbimpikasadi
left a comment
There was a problem hiding this comment.
approach looks good. clean separation between the generic flash fix and the sc5xx sysreset driver. two things to address: explain the REMOTEPROC removal, and flag that the sysreset-uclass.c change is platform-wide. once those are sorted this is good to go.
| CONFIG_CMD_MMC=y | ||
| CONFIG_CMD_MTD=y | ||
| # CONFIG_CMD_PINMUX is not set | ||
| CONFIG_CMD_REMOTEPROC=y |
There was a problem hiding this comment.
why are we removing REMOTEPROC and CMD_REMOTEPROC here? thats unrelated to the reset fix. if qconfig flagged it because nothing selects it anymore thats fine but call it out in the commit message, otherwise it looks liek an accident.
There was a problem hiding this comment.
They are removed by qconfig. Honestly I don't know why they can be removed or why they were not removed yet. Maybe I put them back in this PR and remove them in another PR?
There was a problem hiding this comment.
qconfig removed CONFIG_CMD_REMOTEPROC since it can not be reachable within the current config. there is no CONFIG_SYSCON, which CONFIG_REMOTEPROC_ADI_SC5XX depends on. Also CONFIG_CMD_REMOTEPROC depends on REMOTEPROC which is selected by CONFIG_REMOTEPROC_ADI_SC5XX.
Just as an extra note, qconfig acutally build the configuration for the selected defconfigs. and then runs make savedefconfig. and savedefconfig actually removes the one that are unneccesary/non-reachable.
For your other PR, this information can be provided within the commit body, but mostly just saying syncing the configurations is enough.
See as an example: https://lore.kernel.org/u-boot/20260310115938.2902653-1-ozndrgt@gmail.com/
There was a problem hiding this comment.
So it seems running qconfig is not a hard requirement. At least before this PR, it has not been run. Otherwise REMOTEPROC and CMD_REMOTEPROC should have been removed already. If running qconfig is required for all future PRs which touch defconfig files, I'd suggest use a separate PR to fix the differences from qconfig.
There was a problem hiding this comment.
This runs occasionally in upstream. As I wrote above, the reason these are removed is because you removed SYSCON manually, before I asked you to run qconfig.
This 340fa41 is the last commit just before I asked you to run qconfig.
There was a problem hiding this comment.
Is this a bug in u-boot config system? If A depends on B and A is enabled but B is not, make config should report an error that A cannot be enabled since B is not enabled.
There was a problem hiding this comment.
I do not think that this is a bug. A defconfig is not the final resolved config, it is only a set of requested options for that specific board. When dependencies are not satisfied, Kconfig resolves the final .config and savedefconfig drops options that are no longer reachable.
This is exactly why tools like qconfig exist, and why U-Boot provides ways to check and sync config files. They help catch cases where defconfig entries no longer match the resolved configuration.
There was a problem hiding this comment.
But to get a resolved config, the config system drops a user requested config without telling user. This is dangerous. The config system should report the dependency cannot be satisfied instead.
| struct sc5xx_sysreset_priv *priv = dev_get_priv(dev); | ||
|
|
||
| writel(readl(priv->rcu) | 1, priv->rcu); | ||
| return -EINPROGRESS; |
There was a problem hiding this comment.
minor: other sysreset drivers return -EPROTONOSUPPORT for types they cant handle (like SYSRESET_POWER_OFF). doesnt matter much in practice since the uclass retries with the next driver, but might be worth a one-liner switch to be consistent. up to you if you want to bother.
There was a problem hiding this comment.
this is in the generic sysreset uclass so it hits every platform not just sc5xx. thats probably the right thing but worth flagging since its a behavioral change for all boards.
|
cc @gmalysa |
There was a problem hiding this comment.
sysreset-uclass is a broad reset thing. As I know all sysreset devices goes in there and for each of them it calls sysreset_request until one succeed. And in each trial we remove SPI flash. the problem is that with this change, every such trial now removes SPI flash device or devices before we even know whether the current sysreset driver will actually perform the reset.
plus, code continues toward reset regardless of whether the flash removal actually succeeded. the return value of device_remove is ignored
We can find a better approach to fix it I think. This one is like a SC5xx-specific workaround, not a generic sysreset concern. This logic/fix should live in the SC5xx reset driver itself or idk somewhere else.
There was a problem hiding this comment.
I agree with Ozan. I think the 3-byte vs. 4-byte default addressing issue is primarily a board design issue where our reset out line does not also trigger a flash reset (but on other boards from different mfgs I suspect this is the case, or the device is reset by the respective bootroms prior to access, etc.). I'm not sure what upstream would prefer but I think there are two ways to deal with it:
- Define sysreset handler for the RCU as you've done, as a driver. Then, create another one for the board that is higher priority, which will call exit 4-byte mode. Uboot doesn't have reset priorities the way Linux does though, so this is brittle at best.
- Suggest/introduce
board_resetas a function hidden behind a Kconfig with an empty weak default if selected (much like otherboard_functions). Modifysysreset_walkto start with a call toboard_resetif enabled. This would provide deterministic sequencing and ensure exit 4-byte mode happens only once. There is a small existingboard_resetthat is tied to specific arch or cpu choices used by xtensa, m68k, and one powerpc cpu. You could unify those in the process and migrate that out of the arch-specific code.
I think based on the way uboot traditionally does things, removing all/any drivers before boot or reboot automatically is unlikely to be accepted. I would prefer to do option 2, as it is a bit cleaner in my mind and has a hierarchy between things the driver requires (spi flash does not itself require removal), things the cpu arch requires (arm64 doesn't require it), things our SoC requires (the sc5xx chips do not strictly require that it be reset they only require that it be in the right mode at boot), and things our board requires (it does not trigger a chip reset in another way so in software we need to manually reset it first).
This raises another question: what happens if there is a watchdog reset? That cannot be defended against in software unless the chip is always kept in 3 byte mode outside of a transaction. It seems the actual best solution is option 3, a new revision of the SOM-CRR which wires appropriate board-level resets and associated software configuration to ensure the RCU drives the reset out line when any reset event happens, but this is probably hard to arrange.
There was a problem hiding this comment.
Thanks for taking a look @gmalysa!
I'm tempted to say that since we'll want to remove U-Boot Proper (except possibly for a special debug image) that this won't be an issue if we only use SPL. Additionally, it seems like all boards where 4-byte addressing should be used should have the boot ROM configured correctly using the OTP. And on top of that, as Greg suggests, this may be impacted by board design. Those combined with the fact that this issue has likely persisted for a very long time without it being reported mean that any fix should be very simple.
Let's discuss further on the call on Thursday.
There was a problem hiding this comment.
So we close this issue with won't fix?
There was a problem hiding this comment.
btw, I agree that this is a hardware issue. The real fix should be in hardware. But this is a quite common issue. You can find the same issue happened to boards from other vendors. Linux has done the same thing since 2017. I chose the fix in this PR so it can benefit all boards.
The return value from device_remove does not matter when doing reset. User already wants to reset the system, we can expect that something bad already happened. We just do what we can to help the reset happen. Trying to remove SPI flash multiple times should also be OK. It does no harm.
|
@jiez please address the checkpatch warnings/errors: https://github.com/analogdevicesinc/u-boot/actions/runs/25389944663/job/74461408683?pr=91 |
|
Apparently the boot ROM can be configured with OTP to use 4-byte addressing. All the more reason to have a userspace program that can easily configure the OTP. |
|
If you have the source from the OP-TEE OTP lib that should be relatively easy to migrate into a kernel driver and a sysfs wrapper could be added there, but there will be significant hurdles for releasing the OTP code as open source. If OP-TEE is enabled then the existing Linux OTP CLI application allows userspace OTP programming, although it may be missing the specific field needed for 4-byte addressing (easy to add, it is just a whitelisted constant to identify the field). Using the ROM API is not (easily) an option after booting into Linux because it uses physical addresses internally for poking at hardware registers and it may assume that buffer pointers given are physical addresses as well. You could also copy the old i.MX 6/7 approach and include a u-boot command for programming OTP (which they call fuses) because we could more readily run with the MMU turned off at that point. |
|
See #96 for another way to fix the issue. |
|
@jiez It would be nice to see if we can generate a warning message when booting if the mode is changed by U-Boot stating that reset is not possible in U-Boot and suggesting to configure the boot ROM using the OTP |
But it is possible. |
Detecting addressing mode of SPI flash chip is chip specific. When we can detect flash addressing mode, we can pass appropriate command to adi_rom_Boot() so it will boot using the correct addressing mode. See #96 for how this idea is implemented. What do you think? |
6 participants
The root cause of this reset issue:
When the SPI NOR flash on SOM board is probed, the flash will be changed from 3 byte address mode to 4 byte address mode because of the size of the flash chip. Doing a system reset does not reset the flash. So the flash is still in 4 byte address mode. Thus BOOTROM cannot load U-Boot proper using 3 byte address mode.
Since we have changed to not save env to SPI flash, the flash is not probed by default. So reset is OK. But reset will still hang after "sf probe" command.
To fix this issue, we need to change the flash back to 3 byte address mode before reset command. Someone had done some work two years ago for a Qualcomm board. I updated and simplified his patchset, and added a sysreset driver for SC5xx.
When review, please focus on my changes. I will resubmit Robert's patchset to upstream for review.
https://lore.kernel.org/u-boot/20240507100431.206670-1-robert.marko@sartura.hr/