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);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.Ic2c7c69...
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