[PATCH v5 3/6] arm: provide a function for boards init code to modify MMU virtual-physical map

Marek Szyprowski
2020-08-03T12:32:50Z

Hi Patrick,

On 23.07.2020 18:32, Patrick DELAUNAY wrote:
> Hi Marek,
>
>> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Tom Rini
>> Sent: vendredi 10 juillet 2020 22:22
>>
>> On Wed, Jun 03, 2020 at 02:43:42PM +0200, Marek Szyprowski wrote:
>>
>>> Provide function for setting arbitrary virtual-physical MMU mapping
>>> and cache settings for the given region.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Reviewed-by: Tom Rini <trini@konsulko.com>
>> Applied to u-boot/master, thanks!
> For information, this patch break the SPL boot on stm32mp1 platform in master branch.
>
> It is linked to commit 7e8471cae5c6614c54b9cfae2746d7299bd47a0c
> ("arm: stm32mp: activate data cache in SPL and before relocation")
>
> For the lines:
>
> + mmu_set_region_dcache_behaviour(STM32_SYSRAM_BASE,
> + STM32_SYSRAM_SIZE,
> + DCACHE_DEFAULT_OPTION);
>
> In this patch, the STM32MP15x activate the DACACHE on SYSRAM with:
>
> #define STM32_SYSRAM_BASE 0x2FFC0000
> #define STM32_SYSRAM_SIZE SZ_256K
>
> Even is this address is not MMU_SECTION_SIZE aligned, the configuration of the MMU
> section was correctly aligned in set_section_dcache
>
> - value |= ((u32)section << MMU_SECTION_SHIFT);
>
> With caller :
>
> void mmu_set_region_dcache_behaviour (
> .....
> /* div by 2 before start + size to avoid phys_addr_t overflow */
> end = ALIGN((start / 2) + (size / 2), MMU_SECTION_SIZE / 2)
> >> (MMU_SECTION_SHIFT - 1);
> start = start >> MMU_SECTION_SHIFT;
> ....
> - for (upto = start; upto < end; upto++)
> - set_section_dcache(upto, option);
>
>
> But today it it no more the case when the start address is not MMU_SIZE aligned.
>
> void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size,
> enum dcache_option option)
> {
> mmu_set_region_dcache_behaviour_phys(start, start, size, option);
> }
>
> 'start' parameter is directly used in 'phys' address in the next function:
>
> void mmu_set_region_dcache_behaviour_phys(phys_addr_t start, phys_addr_t phys,
> size_t size, enum dcache_option option)
> {
> .....
> for (upto = start; upto < end; upto++, phys += MMU_SECTION_SIZE)
> set_section_phys(upto, phys, option);
>
>
> and then
>
> static void set_section_phys(int section, phys_addr_t phys,
> enum dcache_option option)
> {
> ....
>
> /* Add the page offset */
> value |= phys;
>
>
> So today I can solve my issue, if I align the section in my code:
>
> if (IS_ENABLED(CONFIG_SPL_BUILD))
> - mmu_set_region_dcache_behaviour(STM32_SYSRAM_BASE,
> - STM32_SYSRAM_SIZE,
> - DCACHE_DEFAULT_OPTION);
> + mmu_set_region_dcache_behaviour(
> + ALIGN(STM32_SYSRAM_BASE, MMU_SECTION_SIZE),
> + round_up(STM32_SYSRAM_SIZE, MMU_SECTION_SIZE),
> + DCACHE_DEFAULT_OPTION);
> else
>
> But I just concerned that this lib behavior is really unexpected: need to provided MMU_SIZE aligned
> start address when mmu_set_region_dcache_behaviour is called.
>
> Do you think we need to restore the previous behavior of the cp15 function mmu_set_region_dcache_behaviour() ?
> => internally aligned DACHE region on MMU_SECTION_SIZE when parameters, start or size, are not aligned.
>
> See also explanation for other use case, and start / end / size usage
> http://patchwork.ozlabs.org/project/uboot/patch/20200424201957.v2.3.Ic2c7c6923035711a4c653d52ae7c0f57ca6f9210 at changeid/

I'm sorry for breaking your setup. However IMHO the caller should ensure
that the parameters are correctly aligned. The old behavior modified
cache parameters on the larger area of the memory than the called
wanted, what might lead to some side-effects. I would prefer aligning
parameters in the callers and maybe add some comment to the function
description that it assumes that the parameters are properly aligned.
Adding a check for the proper alignment is a bit useless, because
usually this function is called so early, that no message can be printed.

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland