[U-Boot] [PATCH] board_f: fix noncached reservation calculation

[Stephen Warren]

On 8/27/19 6:01 PM, Vikas MANOCHA wrote:
> Stephen Warren wrote at Tuesday, August 27, 2019 3:50 PM
>> On 8/27/19 4:10 PM, Vikas MANOCHA wrote:
>>> Stephen Warren wrote at Tuesday, August 27, 2019 10:55 AM
>>>> The current code in reserve_noncached() has two issues:
>>>>
>>>> 1) The first update of gd->start_addr_sp always rounds down to a
>>>> section start. However, the equivalent calculation in
>>>> cache.c:noncached_init() always first rounds up to a section start, then
>>>> subtracts a section size.
>>>> These two calculations differ if the initial value is already rounded
>>>> to section alignment.
>>>
>>> It shouldn't cause any issue, first one round down to section size.
>>> Second
>>> one(cache.c: noncached_init()) rounds up, so needs section size
>>> subtraction.
>>
>> Here's an example where it fails, based on code before my patch:
>>
>> Assume that MMU section size is 2, and that mem_malloc_start and
>> gd->start_addr_sp are both 1000M on entry to the functions, and the
>> noncached region is 1 (what Jetson TX1 uses). The example uses values
>> assumed to be multiples of 1M to make the numbers easier to read.
>>
>> noncached_init:
>>
>> // mem_malloc_start = 1000
>> end = ALIGN(mem_malloc_start, MMU_SECTION_SIZE) -
>> MMU_SECTION_SIZE; // end = 1000 - 2 = 998 // was already aligned, so 1000
>> not 1002 size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY,
>> MMU_SECTION_SIZE); // size = 2 start = end - size; // start = 998 - 2 = 996 //
>> region is 996...998
> 
> Thanks for this example, it definitely seems a bug.  Just that we are fixing it by adding this gap in the reserve_noncached() also.
> Better would be to fix this subtraction of MMU_SECTION_SIZE by aligning down "end" location, like:
> 
> end = ALIGN_DOWN(mem_malloc_start, MMU_SECTION_SIZE); // end = 1000
> size = ALIGN(CONFIG_SYS_NONCACHED_MEMORY, MMU_SECTION_SIZE); // size = 2
> start = end -size; // start = 998

That would change yet another piece of code that's been stable for a
while. It's late in the U-Boot release cycle, so I think we should be
conservative, and not change any more code than necessary. Changing lots
of extra code would run the risk of introducing more regressions. I'd
rather (a) apply the original change I posted, which adjusts only the
code that caused the regression, or (b) revert the patch that caused the
regression.

If you want to adjust the code in noncached_init, we can do that
immediately after the release, to give maximum time for any regressions
to be debugged and fixed before the next release.