| Message ID | 20200403102815.1.I64599059b66bacb531db38c67273754a145dbad8@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] arm: caches: protect dram_bank_mmu_setup access to bi_dram | expand |
On 4/3/20 10:28 AM, Patrick Delaunay wrote: > Add protection in dram_bank_mmu_setup() to avoid access to bd->bi_dram > before relocation. > > This patch allow to use the generic weak function dram_bank_mmu_setup > to activate the MMU and the data cache in SPL or in U-Boot before > relocation, when bd->bi_dram is not yet initialized. > > In this cases, the MMU must be initialized explicitly with > mmu_set_region_dcache_behaviour function. > > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com> > --- > > arch/arm/lib/cache-cp15.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c > index f8d20960da..54509f11c3 100644 > --- a/arch/arm/lib/cache-cp15.c > +++ b/arch/arm/lib/cache-cp15.c > @@ -91,6 +91,10 @@ __weak void dram_bank_mmu_setup(int bank) > bd_t *bd = gd->bd; > int i; > > + /* bd->bi_dram is available only after relocation */ > + if ((gd->flags & GD_FLG_RELOC) == 0) > + return; Why not just set the bd->bi_dram correctly before this is called ?
Dear Marek, > From: Marek Vasut <marex at denx.de> > Sent: vendredi 3 avril 2020 23:27 > > On 4/3/20 10:28 AM, Patrick Delaunay wrote: > > Add protection in dram_bank_mmu_setup() to avoid access to bd->bi_dram > > before relocation. > > > > This patch allow to use the generic weak function dram_bank_mmu_setup > > to activate the MMU and the data cache in SPL or in U-Boot before > > relocation, when bd->bi_dram is not yet initialized. > > > > In this cases, the MMU must be initialized explicitly with > > mmu_set_region_dcache_behaviour function. > > > > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com> > > --- > > > > arch/arm/lib/cache-cp15.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c > > index f8d20960da..54509f11c3 100644 > > --- a/arch/arm/lib/cache-cp15.c > > +++ b/arch/arm/lib/cache-cp15.c > > @@ -91,6 +91,10 @@ __weak void dram_bank_mmu_setup(int bank) > > bd_t *bd = gd->bd; > > int i; > > > > + /* bd->bi_dram is available only after relocation */ > > + if ((gd->flags & GD_FLG_RELOC) == 0) > > + return; > > Why not just set the bd->bi_dram correctly before this is called ? Just set "bd->bi_dram" seens as a hack. For me the bd struct can be updated only in common/board_f.c after reserve_board() for U-Boot Or other spl_set_bd() called in board_init_r() for SPL. And that can cause issue if CONFIG_NR_DRAM_BANKS > 1 (even it is not the case today for STM32MP1). But if this kind of protection is not correct here I prefer come back to overidde of the weak fucntio dram_bank_mmu_setup in stm32mp arch (it is the reason this weak definition) Patrick
On 4/8/20 7:54 PM, Patrick DELAUNAY wrote: > Dear Marek, > >> From: Marek Vasut <marex at denx.de> >> Sent: vendredi 3 avril 2020 23:27 >> >> On 4/3/20 10:28 AM, Patrick Delaunay wrote: >>> Add protection in dram_bank_mmu_setup() to avoid access to bd->bi_dram >>> before relocation. >>> >>> This patch allow to use the generic weak function dram_bank_mmu_setup >>> to activate the MMU and the data cache in SPL or in U-Boot before >>> relocation, when bd->bi_dram is not yet initialized. >>> >>> In this cases, the MMU must be initialized explicitly with >>> mmu_set_region_dcache_behaviour function. >>> >>> Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com> >>> --- >>> >>> arch/arm/lib/cache-cp15.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c >>> index f8d20960da..54509f11c3 100644 >>> --- a/arch/arm/lib/cache-cp15.c >>> +++ b/arch/arm/lib/cache-cp15.c >>> @@ -91,6 +91,10 @@ __weak void dram_bank_mmu_setup(int bank) >>> bd_t *bd = gd->bd; >>> int i; >>> >>> + /* bd->bi_dram is available only after relocation */ >>> + if ((gd->flags & GD_FLG_RELOC) == 0) >>> + return; >> >> Why not just set the bd->bi_dram correctly before this is called ? > > Just set "bd->bi_dram" seens as a hack. > > For me the bd struct can be updated only in common/board_f.c > after reserve_board() for U-Boot > Or other spl_set_bd() called in board_init_r() for SPL. > > And that can cause issue if CONFIG_NR_DRAM_BANKS > 1 > (even it is not the case today for STM32MP1). > > But if this kind of protection is not correct here I prefer come back > to overidde of the weak fucntio dram_bank_mmu_setup in stm32mp arch > (it is the reason this weak definition) I'd say, let's wait for feedback from the others. I would be inclined to set bd->bi_dram, but maybe others have other opinions.
diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c index f8d20960da..54509f11c3 100644 --- a/arch/arm/lib/cache-cp15.c +++ b/arch/arm/lib/cache-cp15.c @@ -91,6 +91,10 @@ __weak void dram_bank_mmu_setup(int bank) bd_t *bd = gd->bd; int i; + /* bd->bi_dram is available only after relocation */ + if ((gd->flags & GD_FLG_RELOC) == 0) + return; + debug("%s: bank: %d\n", __func__, bank); for (i = bd->bi_dram[bank].start >> MMU_SECTION_SHIFT; i < (bd->bi_dram[bank].start >> MMU_SECTION_SHIFT) +
Add protection in dram_bank_mmu_setup() to avoid access to bd->bi_dram before relocation. This patch allow to use the generic weak function dram_bank_mmu_setup to activate the MMU and the data cache in SPL or in U-Boot before relocation, when bd->bi_dram is not yet initialized. In this cases, the MMU must be initialized explicitly with mmu_set_region_dcache_behaviour function. Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com> --- arch/arm/lib/cache-cp15.c | 4 ++++ 1 file changed, 4 insertions(+)