On Thu, Oct 29, 2026 at 01:07:08PM -0500, Andrew Davis wrote: > On 10/29/26 12:49 PM, Tom Rini wrote: > > On Thu, Oct 29, 2026 at 11:09:14PM +0530, Beleswar Padhi wrote: > > > > > Currently, the control status register address is obtained from the > > > ctrl global pointer. ctrl points to a fixed SRAM scratch slot > > > (OMAP_SRAM_SCRATCH_SYS_CTRL), which holds (*ctrl) pointing to the > > > omap5_ctrl/dra7xx_ctrl struct at DDR address 0x8086XXXX in the > > > pre-relocation U-Boot image. > > > > > > Since the SRAM slot is outside the U-Boot image, (*ctrl) is not updated > > > during relocation and becomes stale. The kernel section from the > > > fitImage is loaded to address 0x80008000. So, a larger kernel image > > > (~8.4 MB) overwrites the value at the address (0x8086XXXX) pointed by > > > the (*ctrl) pointer. This corrupts the (*ctrl)->control_status value > > > and CPU goes into a data abort when it tries to access the data at > > > addr=(*ctrl)->control_status > > > > > > Before relocation: > > > +-------------+ +--------------+ +--------------------+ > > > | ctrl | --> | *ctrl | --> | **ctrl | > > > | .data | | SRAM scratch | | .rodata | > > > | 0x8088XXXX | | 0x4037BC20 | | 0x8086XXXX | > > > +-------------+ +--------------+ +--------------------+ > > > > > > After relocation: > > > +-------------+ +--------------+ +--------------------+ > > > | ctrl | --> | *ctrl | --> | **ctrl | > > > | .data | | SRAM scratch | | old .rodata | > > > | relocated | | 0x4037BC20 | | 0x8086XXXX (stale) | > > > | | | (unchanged) | | | > > > +-------------+ +--------------+ +--------------------+ > > > > So this brings up a similar question as v1. It's never going to be safe > > for SRAM to be pointing at some pre-relocation address in DDR. That's a > > problem, full stop. Going to the patch: > > > > [snip] > > > diff --git a/arch/arm/mach-omap2/sysinfo-common.c > > > b/arch/arm/mach-omap2/sysinfo-common.c > > > index 49bc3a634fb..1b7bca3ee37 100644 > > > --- a/arch/arm/mach-omap2/sysinfo-common.c > > > +++ b/arch/arm/mach-omap2/sysinfo-common.c > > > @@ -22,6 +22,9 @@ u32 get_device_type(void) > > > */ > > > return (readl(OMAP34XX_CTRL_BASE + 0x2f0) & DEVICE_TYPE_MASK) >> > > > DEVICE_TYPE_SHIFT; > > > +#elif defined(CONFIG_OMAP54XX) > > > + return (readl(OMAP54XX_CTRL_BASE + 0x134) & DEVICE_TYPE_MASK) >> > > > + DEVICE_TYPE_SHIFT; > > > #else > > > return (readl((*ctrl)->control_status) & DEVICE_TYPE_MASK) >> > > > DEVICE_TYPE_SHIFT; > > > > There's not enough context to see what chips are in that final "else" > > clause, but they're all likely in a fragile and breakable state, and > > that needs to be fixed. Is there still a reason we're shadowing that > > information anymore, rather than always reading it? And, while we're in > > here, what other fragile things do we have SRAM pointing to > > pre-relocation-DRAM in? Thanks! > > > > I had tried to root cause this sometime previously, it starts here[0] > where we have different ctrl register layouts based on the device/revision > and we call hw_data_init() early to setup these pointers to the right > layout early. This part is probably fine and saves doing the lookup > every time we do a register access. > > The issue is then that we use this double pointer with one layer > being stored in an SRAM scrachpad area. My best guess (this is > code from ~2013 so I don't have full history) is that this was > done so the pointers would survive SPL->U-Boot and not need looked > up again. That part is the issue, the sane thing to do would be to > just re-fetch these tables, which AM43xx does do[2] which seems to > be why it doesn't seem to need this work around. And the commit > that adds that for AM43xx[2] seems to confirm it was done for this > reason. > > Best solution IMHO would be to get rid of the odd double pointers > and just make sure the now single pointer ctrl tables are initialized > at the start of both boot stages.
Sounds like a good plan to me, Beleswar can you take care of that? Thanks. -- Tom
signature.asc
Description: PGP signature
