On 29/10/26 23:49, Tom Rini wrote: > 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?
I agree with both of your assignments. I was just trying to revive the board to boot with this patch, but we should fix the bigger problem here. Will work on it. Thanks, Beleswar > Thanks. >
