[PATCH] Revert "arm: bootm: Disable LMB reservation for command line and board info on arm64"

[Marek Vasut]

On 8/2/21 11:37 AM, Jan Kiszka wrote:
> On 02.08.21 02:54, Marek Vasut wrote:
>> On 7/29/21 6:58 PM, Tom Rini wrote:
>>
>> [...]
>>
>>>>> so when did rcar3 introduce something there that shouldn't be
>>>>> reserved?  And you had phrased this to me on IRC as about reserving
>>>>> spot
>>>>> for ATAGS, and that not being needed of course on arm64.  But that's
>>>>> not
>>>>> what's going on.  Perhaps the answer is that rcar3 needs to introduce a
>>>>> board_lmb_reserve to free the normal arch one and provide whatever more
>>>>> narrow scope it needs.
>>>>
>>>> Based on the commit message 2359fa7a878 ("arm: bootm: Disable LMB
>>>> reservation for command line and board info on arm64") , this is
>>>> about ATAGS
>>>> and we really don't need to reserve those on arm64.
>>>
>>> Commit 2359fa7a878 disables the entire arch_lmb_reserve function on
>>> aarch64, yes.  I assumed when we had talked that it was a small area
>>> being set aside and perhaps mis-recalled that ATAGS tended to live at
>>> DDR_BASE + 0x800 or so.
>>
>> That arch_lmb_reserve() is responsible for reserving architecture
>> specific memory. On arm32 it is ATAGS, on arm64 it is nothing as far as
>> I can tell (and see below regarding the TLB).
>>
>>> This reservation is not at that spot, and a lot
>>> more than that.
>>
>> Can you please elaborate on this "lot more" part ? Because as much as I
>> studied the reservation code, the "lot more" was ATAGS on arm32 and
>> nothing on arm64.
> 
> See my commit log.

This is not particularly useful answer, considering the commit log says:
"lot of crucial things", "Possibly more", "likely also on other boards" 
and other opaque statements. But really, the problem so far happens on 
one K3 board.

I am asking for specifics, what is it that should be reserved by the 
arch_lmb_reserve() ? I believe it is only ATAGS on arm32 and nothing on 
arm64.

>>> That commit is also only present in v2021.10-rc1 so
>>> it's not a great idea for another project to depend on it and can also
>>> revert their changes until someone is able to analyze the entire
>>> situation again.
>>
>> We are in rc1 so we still have enough time to analyze this problem, yes.
>>
>>>> I didn't hit the tlb reservation issue Jan has in my tests on rcar3,
>>>> so I
>>>> suspect that is the real problem here, i.e. we need some more fine
>>>> grained
>>>> lmb reservation on arm64 ?
>>>
>>> Yes, some further analysis is needed here.  But due to a lack of time I
>>> suspect the best answer for now is for rcar3 to add board_lmb_reserve()
>>> and lmb_free what's not needed there.
>>
>> So, no.
>>
>> The question I would like answered is whether we even really have to
>> reserve the TLB area with LMB at all. Linux will set up the TLB again,
>> so I think we do not have to.
> 
> See my commit log.

The commit log does not explain why do you think the TLB should be 
reserved, sorry.

I explain above why I think it should not be reserved.

>> And I _think_ the problem is isolated to K3 due to the non-standard TLB
>> and cache init it does in arch/arm/mach-k3/common.c spl_enable_dcache()
>> in SPL. The code sets up TLB close to the end of DRAM and enables dcache
>> early in SPL, and the end of DRAM is incidentally also where U-Boot is
>> relocated shortly after and at that point U-Boot possibly overwrites or
>> corrupts that TLB content. This hypothesis can be easily tested on K3,
>> just comment out the content of spl_enable_dcache() and see if that
>> fixes things.
> 
> This is not only TLB, see my commit log. Specifically the clash with the
> relocated U-Boot code causes the crash on the k3. But that location does
> not look like it is k3-specfic. Other boards are just lucky if the work.

Please have a look at arch/arm/mach-k3/common.c spl_enable_dcache() , 
that code sets up the TLB in a location which is overwritten by the 
relocated U-Boot. This is specific to K3 and no other platform does that.

>> It would also be interesting to know what the debug() print in
>> spl_enable_dcache() prints on the K3 from Jan.
> 
> The information is already available, I shared bdinfo.

spl_enable_dcache() is called in SPL, it is not part of bdinfo.