Fix the issue that reset from u boot proper gets stuck in spl #91
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,4 +17,3 @@ obj-$(CONFIG_SC59X_64) += sc59x_64.o | |
| obj-$(CONFIG_SC59X_64) += sc59x_64-spl.o | ||
|
|
||
| obj-$(CONFIG_XPL_BUILD) += spl.o | ||
|
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.
Member
There was a problem hiding this comment.
plus, code continues toward reset regardless of whether the flash removal actually succeeded. the return value of 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:
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.
Collaborator
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.
Author
There was a problem hiding this comment. So we close this issue with won't fix?
Author
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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| // SPDX-License-Identifier: GPL-2.0-or-later | ||
| /* | ||
| * (C) Copyright 2026 - Analog Devices, Inc. | ||
| */ | ||
|
|
||
| #include <dm.h> | ||
| #include <sysreset.h> | ||
| #include <asm/io.h> | ||
|
|
||
| struct sc5xx_sysreset_priv { | ||
| void __iomem *rcu; | ||
| }; | ||
|
|
||
| static int sc5xx_sysreset_request(struct udevice *dev, enum sysreset_t type) | ||
| { | ||
| 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. |
||
| } | ||
|
|
||
| static int sc5xx_sysreset_probe(struct udevice *dev) | ||
| { | ||
| struct sc5xx_sysreset_priv *priv = dev_get_priv(dev); | ||
|
|
||
| priv->rcu = dev_remap_addr(dev); | ||
| if (!priv->rcu) | ||
| return -EINVAL; | ||
| return 0; | ||
| } | ||
|
|
||
| static const struct udevice_id sc5xx_sysreset_ids[] = { | ||
| { .compatible = "adi,reset-controller" }, | ||
| { } | ||
| }; | ||
|
|
||
| static struct sysreset_ops sc5xx_sysreset_ops = { | ||
| .request = sc5xx_sysreset_request, | ||
| }; | ||
|
|
||
| U_BOOT_DRIVER(sysreset_sc5xx) = { | ||
| .name = "sc5xx_sysreset", | ||
| .id = UCLASS_SYSRESET, | ||
| .of_match = sc5xx_sysreset_ids, | ||
| .ops = &sc5xx_sysreset_ops, | ||
| .probe = sc5xx_sysreset_probe, | ||
| .priv_auto = sizeof(struct sc5xx_sysreset_priv), | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qconfigremovedCONFIG_CMD_REMOTEPROCsince it can not be reachable within the current config. there is noCONFIG_SYSCON, whichCONFIG_REMOTEPROC_ADI_SC5XXdepends on. AlsoCONFIG_CMD_REMOTEPROCdepends onREMOTEPROCwhich is selected byCONFIG_REMOTEPROC_ADI_SC5XX.Just as an extra note,
qconfigacutally build the configuration for the selected defconfigs. and then runsmake savedefconfig. andsavedefconfigactually 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This runs occasionally in upstream. As I wrote above, the reason these are removed is because you removed
SYSCONmanually, before I asked you to runqconfig.This 340fa41 is the last commit just before I asked you to run qconfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.