[PATCH 1/3] arm: caches: protect dram_bank_mmu_setup access to bi_dram

[PATCH 1/3] arm: caches: protect dram_bank_mmu_setup access to bi_dram

[Patrick Delaunay]

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@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;
+
 	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) +
-- 
2.17.1

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Patrick Delaunay]

Add the new flags DCACHE_DEFAULT_OPTION to define the default
option to use according the compilation flags
CONFIG_SYS_ARM_CACHE_WRITETHROUGH or CONFIG_SYS_ARM_CACHE_WRITEALLOC.

This new compilation flag allows to simplify dram_bank_mmu_setup()
and can be used as third parameter (option=dcache option to select)
of mmu_set_region_dcache_behaviour function.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 arch/arm/include/asm/system.h |  8 ++++++++
 arch/arm/lib/cache-cp15.c     | 11 ++---------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index 81ccead112..01ea96e8ad 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -485,6 +485,14 @@ enum dcache_option {
 };
 #endif
 
+#if defined(CONFIG_SYS_ARM_CACHE_WRITETHROUGH)
+#define DCACHE_DEFAULT_OPTION	DCACHE_WRITETHROUGH
+#elif defined(CONFIG_SYS_ARM_CACHE_WRITEALLOC)
+#define DCACHE_DEFAULT_OPTION	DCACHE_WRITEALLOC
+#else
+#define DCACHE_DEFAULT_OPTION	DCACHE_WRITEBACK
+#endif
+
 /* Size of an MMU section */
 enum {
 #ifdef CONFIG_ARMV7_LPAE
diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c
index 54509f11c3..d15144188b 100644
--- a/arch/arm/lib/cache-cp15.c
+++ b/arch/arm/lib/cache-cp15.c
@@ -99,15 +99,8 @@ __weak void dram_bank_mmu_setup(int bank)
 	for (i = bd->bi_dram[bank].start >> MMU_SECTION_SHIFT;
 	     i < (bd->bi_dram[bank].start >> MMU_SECTION_SHIFT) +
 		 (bd->bi_dram[bank].size >> MMU_SECTION_SHIFT);
-	     i++) {
-#if defined(CONFIG_SYS_ARM_CACHE_WRITETHROUGH)
-		set_section_dcache(i, DCACHE_WRITETHROUGH);
-#elif defined(CONFIG_SYS_ARM_CACHE_WRITEALLOC)
-		set_section_dcache(i, DCACHE_WRITEALLOC);
-#else
-		set_section_dcache(i, DCACHE_WRITEBACK);
-#endif
-	}
+	     i++)
+		set_section_dcache(i, DCACHE_DEFAULT_OPTION);
 }
 
 /* to activate the MMU we need to set up virtual memory: use 1M areas */
-- 
2.17.1

[PATCH 3/3] arm: caches: manage phys_addr_t overflow in mmu_set_region_dcache_behaviour

[Patrick Delaunay]

Detect and solve the overflow on phys_addr_t type for start + size in
mmu_set_region_dcache_behaviour() function.

This issue occurs for example with ARM32, start = 0xC0000000 and
size = 0x40000000: start + size = 0x100000000 and end = 0x0.

Overflow is detected when end < start.
In normal case the previous behavior is still used: when start is not
aligned on MMU section, the end address is only aligned after the sum
start + size.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 arch/arm/lib/cache-cp15.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c
index d15144188b..e5a7fd0ef4 100644
--- a/arch/arm/lib/cache-cp15.c
+++ b/arch/arm/lib/cache-cp15.c
@@ -63,6 +63,11 @@ void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size,
 
 	end = ALIGN(start + size, MMU_SECTION_SIZE) >> MMU_SECTION_SHIFT;
 	start = start >> MMU_SECTION_SHIFT;
+
+	/* phys_addr_t overflow detected */
+	if (end < start)
+		end = (~(phys_addr_t)0x0 >> MMU_SECTION_SHIFT) + 1;
+
 #ifdef CONFIG_ARMV7_LPAE
 	debug("%s: start=%pa, size=%zu, option=%llx\n", __func__, &start, size,
 	      option);
-- 
2.17.1

[PATCH 1/3] arm: caches: protect dram_bank_mmu_setup access to bi_dram

[Marek Vasut]

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@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 ?

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Marek Vasut]

On 4/3/20 10:28 AM, Patrick Delaunay wrote:
> Add the new flags DCACHE_DEFAULT_OPTION to define the default
> option to use according the compilation flags
> CONFIG_SYS_ARM_CACHE_WRITETHROUGH or CONFIG_SYS_ARM_CACHE_WRITEALLOC.

Can't you unify these macros into a single Kconfig "select" statement
instead , and then just select the matching cache configuration in Kconfig ?

Or better yet, can't you extract this info from DT ?

[PATCH 3/3] arm: caches: manage phys_addr_t overflow in mmu_set_region_dcache_behaviour

[Marek Vasut]

On 4/3/20 10:28 AM, Patrick Delaunay wrote:
> Detect and solve the overflow on phys_addr_t type for start + size in
> mmu_set_region_dcache_behaviour() function.
> 
> This issue occurs for example with ARM32, start = 0xC0000000 and
> size = 0x40000000: start + size = 0x100000000 and end = 0x0.
> 
> Overflow is detected when end < start.
> In normal case the previous behavior is still used: when start is not
> aligned on MMU section, the end address is only aligned after the sum
> start + size.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
> 
>  arch/arm/lib/cache-cp15.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c
> index d15144188b..e5a7fd0ef4 100644
> --- a/arch/arm/lib/cache-cp15.c
> +++ b/arch/arm/lib/cache-cp15.c
> @@ -63,6 +63,11 @@ void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size,
>  
>  	end = ALIGN(start + size, MMU_SECTION_SIZE) >> MMU_SECTION_SHIFT;
>  	start = start >> MMU_SECTION_SHIFT;
> +
> +	/* phys_addr_t overflow detected */
> +	if (end < start)
> +		end = (~(phys_addr_t)0x0 >> MMU_SECTION_SHIFT) + 1;
> +

Or, you can divide $start and $size separately by MMU_SECTION_SIZE and
then add them up .

[PATCH 1/3] arm: caches: protect dram_bank_mmu_setup access to bi_dram

[Patrick DELAUNAY]

Dear Marek,

> From: Marek Vasut <marex@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@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

[PATCH 1/3] arm: caches: protect dram_bank_mmu_setup access to bi_dram

[Marek Vasut]

On 4/8/20 7:54 PM, Patrick DELAUNAY wrote:
> Dear Marek,
> 
>> From: Marek Vasut <marex@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@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.

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Patrick DELAUNAY]

Dear Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: vendredi 3 avril 2020 23:29
> 
> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
> > Add the new flags DCACHE_DEFAULT_OPTION to define the default option
> > to use according the compilation flags
> > CONFIG_SYS_ARM_CACHE_WRITETHROUGH or
> CONFIG_SYS_ARM_CACHE_WRITEALLOC.
> 
> Can't you unify these macros into a single Kconfig "select" statement instead ,
> and then just select the matching cache configuration in Kconfig ?

Yes I will try, with 2 steps
- migrate existing CONFIG_SYS_ARM_CACHE_.... in Kconfig
- add new option CONFIG_SYS_ARM_CACHE_OPTION

> Or better yet, can't you extract this info from DT ?

I don't think.... it is called before device tree parsing

Patrick

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Marek Vasut]

On 4/8/20 8:16 PM, Patrick DELAUNAY wrote:
> Dear Marek,
> 
>> From: Marek Vasut <marex@denx.de>
>> Sent: vendredi 3 avril 2020 23:29
>>
>> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
>>> Add the new flags DCACHE_DEFAULT_OPTION to define the default option
>>> to use according the compilation flags
>>> CONFIG_SYS_ARM_CACHE_WRITETHROUGH or
>> CONFIG_SYS_ARM_CACHE_WRITEALLOC.
>>
>> Can't you unify these macros into a single Kconfig "select" statement instead ,
>> and then just select the matching cache configuration in Kconfig ?
> 
> Yes I will try, with 2 steps
> - migrate existing CONFIG_SYS_ARM_CACHE_.... in Kconfig
> - add new option CONFIG_SYS_ARM_CACHE_OPTION
> 
>> Or better yet, can't you extract this info from DT ?
> 
> I don't think.... it is called before device tree parsing
> 

The FDT access should be set up as one of the first things during
U-Boot's boot_init_f , so it should be possible.

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Patrick DELAUNAY]

Hi

> From: Marek Vasut <marex@denx.de>
> Sent: mercredi 8 avril 2020 20:18
> 
> On 4/8/20 8:16 PM, Patrick DELAUNAY wrote:
> > Dear Marek,
> >
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: vendredi 3 avril 2020 23:29
> >>
> >> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
> >>> Add the new flags DCACHE_DEFAULT_OPTION to define the default option
> >>> to use according the compilation flags
> >>> CONFIG_SYS_ARM_CACHE_WRITETHROUGH or
> >> CONFIG_SYS_ARM_CACHE_WRITEALLOC.
> >>
> >> Can't you unify these macros into a single Kconfig "select" statement
> >> instead , and then just select the matching cache configuration in Kconfig ?
> >
> > Yes I will try, with 2 steps
> > - migrate existing CONFIG_SYS_ARM_CACHE_.... in Kconfig
> > - add new option CONFIG_SYS_ARM_CACHE_OPTION
> >
> >> Or better yet, can't you extract this info from DT ?
> >
> > I don't think.... it is called before device tree parsing
> >
> 
> The FDT access should be set up as one of the first things during U-Boot's
> boot_init_f , so it should be possible.

But I try to activate de dcache to speed-up the device tree parsing.... 
So the MMU function is now called really early,  in arch init.

Or I miss something.

Patrick

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Marek Vasut]

On 4/8/20 9:07 PM, Patrick DELAUNAY wrote:
> Hi
> 
>> From: Marek Vasut <marex@denx.de>
>> Sent: mercredi 8 avril 2020 20:18
>>
>> On 4/8/20 8:16 PM, Patrick DELAUNAY wrote:
>>> Dear Marek,
>>>
>>>> From: Marek Vasut <marex@denx.de>
>>>> Sent: vendredi 3 avril 2020 23:29
>>>>
>>>> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
>>>>> Add the new flags DCACHE_DEFAULT_OPTION to define the default option
>>>>> to use according the compilation flags
>>>>> CONFIG_SYS_ARM_CACHE_WRITETHROUGH or
>>>> CONFIG_SYS_ARM_CACHE_WRITEALLOC.
>>>>
>>>> Can't you unify these macros into a single Kconfig "select" statement
>>>> instead , and then just select the matching cache configuration in Kconfig ?
>>>
>>> Yes I will try, with 2 steps
>>> - migrate existing CONFIG_SYS_ARM_CACHE_.... in Kconfig
>>> - add new option CONFIG_SYS_ARM_CACHE_OPTION
>>>
>>>> Or better yet, can't you extract this info from DT ?
>>>
>>> I don't think.... it is called before device tree parsing
>>>
>>
>> The FDT access should be set up as one of the first things during U-Boot's
>> boot_init_f , so it should be possible.
> 
> But I try to activate de dcache to speed-up the device tree parsing.... 
> So the MMU function is now called really early,  in arch init.
> 
> Or I miss something.

Ah, oops, please forget what I said.

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Patrick DELAUNAY]

Dear Marek,

> From: Uboot-stm32 <uboot-stm32-bounces@st-md-mailman.stormreply.com> On
> Behalf Of Patrick DELAUNAY
> 
> Dear Marek,
> 
> > From: Marek Vasut <marex@denx.de>
> > Sent: vendredi 3 avril 2020 23:29
> >
> > On 4/3/20 10:28 AM, Patrick Delaunay wrote:
> > > Add the new flags DCACHE_DEFAULT_OPTION to define the default option
> > > to use according the compilation flags
> > > CONFIG_SYS_ARM_CACHE_WRITETHROUGH or
> > CONFIG_SYS_ARM_CACHE_WRITEALLOC.
> >
> > Can't you unify these macros into a single Kconfig "select" statement
> > instead , and then just select the matching cache configuration in Kconfig ?
> 
> Yes I will try, with 2 steps
> - migrate existing CONFIG_SYS_ARM_CACHE_.... in Kconfig

First step done...
I will push it as a separate patchset I think.

> - add new option CONFIG_SYS_ARM_CACHE_OPTION

In fact it is to difficult to use select because each defines
DCACHE_XXX value can have several values

they are build according CONFIG_ARM64 / LPAE

But, I can't use this define in Kconfig

I try :
option	ARM_OPTION
	int "option"
	default DCACHE_WRITETHROUGHT if CONFIG_SYS_ARM_CACHE_WRITETHROUGH
	default DCACHE_ WRITEALLOC if CONFIG_SYS_ARM_CACHE_ WRITEALLOC
	default DCACHE_WRITEBACK if CONFIG_SYS_ARM_CACHE_WRITEBACK

int and hex is invalid, and string can't be use with "".

And I don't found way to build it automatically when option is activated.

Any idea ?

Regards

Patrick

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Marek Vasut]

On 4/9/20 12:01 PM, Patrick DELAUNAY wrote:
> Dear Marek,

Hi,

>> From: Uboot-stm32 <uboot-stm32-bounces@st-md-mailman.stormreply.com> On
>> Behalf Of Patrick DELAUNAY
>>
>> Dear Marek,
>>
>>> From: Marek Vasut <marex@denx.de>
>>> Sent: vendredi 3 avril 2020 23:29
>>>
>>> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
>>>> Add the new flags DCACHE_DEFAULT_OPTION to define the default option
>>>> to use according the compilation flags
>>>> CONFIG_SYS_ARM_CACHE_WRITETHROUGH or
>>> CONFIG_SYS_ARM_CACHE_WRITEALLOC.
>>>
>>> Can't you unify these macros into a single Kconfig "select" statement
>>> instead , and then just select the matching cache configuration in Kconfig ?
>>
>> Yes I will try, with 2 steps
>> - migrate existing CONFIG_SYS_ARM_CACHE_.... in Kconfig
> 
> First step done...
> I will push it as a separate patchset I think.
> 
>> - add new option CONFIG_SYS_ARM_CACHE_OPTION
> 
> In fact it is to difficult to use select because each defines
> DCACHE_XXX value can have several values
> 
> they are build according CONFIG_ARM64 / LPAE
> 
> But, I can't use this define in Kconfig
> 
> I try :
> option	ARM_OPTION
> 	int "option"
> 	default DCACHE_WRITETHROUGHT if CONFIG_SYS_ARM_CACHE_WRITETHROUGH
> 	default DCACHE_ WRITEALLOC if CONFIG_SYS_ARM_CACHE_ WRITEALLOC
> 	default DCACHE_WRITEBACK if CONFIG_SYS_ARM_CACHE_WRITEBACK
> 
> int and hex is invalid, and string can't be use with "".
> 
> And I don't found way to build it automatically when option is activated.
> 
> Any idea ?

Maybe you can have a select in the Kconfig to set some differently named
option, e.g.

DCACHE_MODE_WRITE{THROUGH,ALLOC,BACK}

and then an ifdef in some header file, e.g.

#ifdef CONFIG_DCACHE_MODE_WRITETHROUGH
#define ARM_CACHE_MODE DCACHE_WRITETHROUGH
...

And then use ARM_CACHE_MODE where you need a value and
CONFIG_DCACHE_MODE{...} where you need a config option check.

Does this work ?

[PATCH 3/3] arm: caches: manage phys_addr_t overflow in mmu_set_region_dcache_behaviour

[Patrick DELAUNAY]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: vendredi 3 avril 2020 23:31
> To: Patrick DELAUNAY <patrick.delaunay@st.com>; u-boot at lists.denx.de
> Cc: Simon Glass <sjg@chromium.org>; Alexey Brodkin
> <abrodkin@synopsys.com>; Lokesh Vutla <lokeshvutla@ti.com>; Tom Rini
> <trini@konsulko.com>; Trevor Woerner <trevor@toganlabs.com>; U-Boot STM32
> <uboot-stm32@st-md-mailman.stormreply.com>
> Subject: Re: [PATCH 3/3] arm: caches: manage phys_addr_t overflow in
> mmu_set_region_dcache_behaviour
> Importance: High
> 
> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
> > Detect and solve the overflow on phys_addr_t type for start + size in
> > mmu_set_region_dcache_behaviour() function.
> >
> > This issue occurs for example with ARM32, start = 0xC0000000 and size
> > = 0x40000000: start + size = 0x100000000 and end = 0x0.
> >
> > Overflow is detected when end < start.
> > In normal case the previous behavior is still used: when start is not
> > aligned on MMU section, the end address is only aligned after the sum
> > start + size.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > ---
> >
> >  arch/arm/lib/cache-cp15.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c
> > index d15144188b..e5a7fd0ef4 100644
> > --- a/arch/arm/lib/cache-cp15.c
> > +++ b/arch/arm/lib/cache-cp15.c
> > @@ -63,6 +63,11 @@ void mmu_set_region_dcache_behaviour(phys_addr_t
> > start, size_t size,
> >
> >  	end = ALIGN(start + size, MMU_SECTION_SIZE) >>
> MMU_SECTION_SHIFT;
> >  	start = start >> MMU_SECTION_SHIFT;
> > +
> > +	/* phys_addr_t overflow detected */
> > +	if (end < start)
> > +		end = (~(phys_addr_t)0x0 >> MMU_SECTION_SHIFT) + 1;
> > +
> 
> Or, you can divide $start and $size separately by MMU_SECTION_SIZE and then
> add them up .

It was my first idea but that change the function behavior,
because today start and size can be not aligned on MMU_SECTION aligned.

I think it is strange, but I preferred to don't change this part.

Example with shift = 21 and 2MB section size: 0x200000

Start = 0x1000000
Size = 0x1000000

End = 0x2000000 

=> after alignment start = 0x0, end = 0x1

But if we align the start and size before addition as proposed, the final result change 

Start = 0x1000000 => 0
Size = 0x1000000 => 0

End = 0x0

I prefer don't modify this current (strange) behavior to avoid regression.

But if it is acceptable (because the caller MUST always use start and size MMU_SECTION aligned),
I will change the proposal

Patrick

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Patrick DELAUNAY]

Dear Marek,

> Sent: jeudi 9 avril 2020 12:21
> To: Patrick DELAUNAY <patrick.delaunay@st.com>; u-boot at lists.denx.de
> 
> On 4/9/20 12:01 PM, Patrick DELAUNAY wrote:
> > Dear Marek,
> 
> Hi,
> 
> >> From: Uboot-stm32 <uboot-stm32-bounces@st-md-mailman.stormreply.com>
> >> On Behalf Of Patrick DELAUNAY
> >>
> >> Dear Marek,
> >>
> >>> From: Marek Vasut <marex@denx.de>
> >>> Sent: vendredi 3 avril 2020 23:29
> >>>
> >>> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
> >>>> Add the new flags DCACHE_DEFAULT_OPTION to define the default
> >>>> option to use according the compilation flags
> >>>> CONFIG_SYS_ARM_CACHE_WRITETHROUGH or
> >>> CONFIG_SYS_ARM_CACHE_WRITEALLOC.
> >>>
> >>> Can't you unify these macros into a single Kconfig "select"
> >>> statement instead , and then just select the matching cache configuration in
> Kconfig ?
> >>
> >> Yes I will try, with 2 steps
> >> - migrate existing CONFIG_SYS_ARM_CACHE_.... in Kconfig
> >
> > First step done...
> > I will push it as a separate patchset I think.
> >
> >> - add new option CONFIG_SYS_ARM_CACHE_OPTION
> >
> > In fact it is to difficult to use select because each defines
> > DCACHE_XXX value can have several values
> >
> > they are build according CONFIG_ARM64 / LPAE
> >
> > But, I can't use this define in Kconfig
> >
> > I try :
> > option	ARM_OPTION
> > 	int "option"
> > 	default DCACHE_WRITETHROUGHT if
> CONFIG_SYS_ARM_CACHE_WRITETHROUGH
> > 	default DCACHE_ WRITEALLOC if CONFIG_SYS_ARM_CACHE_
> WRITEALLOC
> > 	default DCACHE_WRITEBACK if
> CONFIG_SYS_ARM_CACHE_WRITEBACK
> >
> > int and hex is invalid, and string can't be use with "".
> >
> > And I don't found way to build it automatically when option is activated.
> >
> > Any idea ?
> 
> Maybe you can have a select in the Kconfig to set some differently named option,
> e.g.
> 
> DCACHE_MODE_WRITE{THROUGH,ALLOC,BACK}
> 
> and then an ifdef in some header file, e.g.
> 
> #ifdef CONFIG_DCACHE_MODE_WRITETHROUGH
> #define ARM_CACHE_MODE DCACHE_WRITETHROUGH ...
> 
> And then use ARM_CACHE_MODE where you need a value and
> CONFIG_DCACHE_MODE{...} where you need a config option check.
> 
> Does this work ?

I try with string and default (as select is allowed on for bolean or trisate),
And I failed :-<

I don't found a way to have the de-stringficate the KConfig option
to generated the correct define

config SYS_ARM_CACHE_POLICY
	string "Name of the ARM data write cache policy"
	default WRITEBACK if SYS_ARM_CACHE_WRITEBACK
	default WRITETHROUGH if SYS_ARM_CACHE_WRITEBACK
	default WRITEALLOC if SYS_ARM_CACHE_WRITEALLOC

#define DCACHE_DEFAULT_OPTION	DCACHE_ ## CONFIG_SYS_ARM_CACHE_POLICY

=> error: ?DCACHE_CONFIG_SYS_ARM_CACHE_POLICY? undeclared (first use in this function); did you mean ?CONFIG_SYS_ARM_CACHE_POLICY??

#define DCACHE_OPTION(s)	DCACHE_ ## CONFIG_SYS_ARM_CACHE_POLICY

#define DCACHE_DEFAULT_OPTION	DCACHE_OPTION(CONFIG_SYS_ARM_CACHE_POLICY)

arch/arm/include/asm/system.h:488:26: error: ?DCACHE_? undeclared (first use in this function); did you mean ?DCACHE_OFF??
arch/arm/lib/cache-cp15.c:99:25: error: expected ?)? before string constant

The stringification is possible but not the inverse operation (remove the quote)...

In my .config, CONFIG_SYS_ARM_CACHE_POLICY="WRITEBACK"

Regards

Patrick

[PATCH 3/3] arm: caches: manage phys_addr_t overflow in mmu_set_region_dcache_behaviour

[Marek Vasut]

On 4/9/20 4:18 PM, Patrick DELAUNAY wrote:
> 
> 
>> -----Original Message-----
>> From: Marek Vasut <marex@denx.de>
>> Sent: vendredi 3 avril 2020 23:31
>> To: Patrick DELAUNAY <patrick.delaunay@st.com>; u-boot at lists.denx.de
>> Cc: Simon Glass <sjg@chromium.org>; Alexey Brodkin
>> <abrodkin@synopsys.com>; Lokesh Vutla <lokeshvutla@ti.com>; Tom Rini
>> <trini@konsulko.com>; Trevor Woerner <trevor@toganlabs.com>; U-Boot STM32
>> <uboot-stm32@st-md-mailman.stormreply.com>
>> Subject: Re: [PATCH 3/3] arm: caches: manage phys_addr_t overflow in
>> mmu_set_region_dcache_behaviour
>> Importance: High
>>
>> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
>>> Detect and solve the overflow on phys_addr_t type for start + size in
>>> mmu_set_region_dcache_behaviour() function.
>>>
>>> This issue occurs for example with ARM32, start = 0xC0000000 and size
>>> = 0x40000000: start + size = 0x100000000 and end = 0x0.
>>>
>>> Overflow is detected when end < start.
>>> In normal case the previous behavior is still used: when start is not
>>> aligned on MMU section, the end address is only aligned after the sum
>>> start + size.
>>>
>>> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
>>> ---
>>>
>>>  arch/arm/lib/cache-cp15.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c
>>> index d15144188b..e5a7fd0ef4 100644
>>> --- a/arch/arm/lib/cache-cp15.c
>>> +++ b/arch/arm/lib/cache-cp15.c
>>> @@ -63,6 +63,11 @@ void mmu_set_region_dcache_behaviour(phys_addr_t
>>> start, size_t size,
>>>
>>>  	end = ALIGN(start + size, MMU_SECTION_SIZE) >>
>> MMU_SECTION_SHIFT;
>>>  	start = start >> MMU_SECTION_SHIFT;
>>> +
>>> +	/* phys_addr_t overflow detected */
>>> +	if (end < start)
>>> +		end = (~(phys_addr_t)0x0 >> MMU_SECTION_SHIFT) + 1;
>>> +
>>
>> Or, you can divide $start and $size separately by MMU_SECTION_SIZE and then
>> add them up .
> 
> It was my first idea but that change the function behavior,
> because today start and size can be not aligned on MMU_SECTION aligned.
> 
> I think it is strange, but I preferred to don't change this part.
> 
> Example with shift = 21 and 2MB section size: 0x200000
> 
> Start = 0x1000000
> Size = 0x1000000
> 
> End = 0x2000000 
> 
> => after alignment start = 0x0, end = 0x1
> 
> But if we align the start and size before addition as proposed, the final result change 
> 
> Start = 0x1000000 => 0
> Size = 0x1000000 => 0
> 
> End = 0x0
> 
> I prefer don't modify this current (strange) behavior to avoid regression.
> 
> But if it is acceptable (because the caller MUST always use start and size MMU_SECTION aligned),
> I will change the proposal

The minimum page size is 4k, right ? Then divide both by 4k and then by
the rest of MMU_SECTION_SHIFT.

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Marek Vasut]

On 4/9/20 8:06 PM, Patrick DELAUNAY wrote:
> Dear Marek,

Hi,

>> Sent: jeudi 9 avril 2020 12:21
>> To: Patrick DELAUNAY <patrick.delaunay@st.com>; u-boot at lists.denx.de
>>
>> On 4/9/20 12:01 PM, Patrick DELAUNAY wrote:
>>> Dear Marek,
>>
>> Hi,
>>
>>>> From: Uboot-stm32 <uboot-stm32-bounces@st-md-mailman.stormreply.com>
>>>> On Behalf Of Patrick DELAUNAY
>>>>
>>>> Dear Marek,
>>>>
>>>>> From: Marek Vasut <marex@denx.de>
>>>>> Sent: vendredi 3 avril 2020 23:29
>>>>>
>>>>> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
>>>>>> Add the new flags DCACHE_DEFAULT_OPTION to define the default
>>>>>> option to use according the compilation flags
>>>>>> CONFIG_SYS_ARM_CACHE_WRITETHROUGH or
>>>>> CONFIG_SYS_ARM_CACHE_WRITEALLOC.
>>>>>
>>>>> Can't you unify these macros into a single Kconfig "select"
>>>>> statement instead , and then just select the matching cache configuration in
>> Kconfig ?
>>>>
>>>> Yes I will try, with 2 steps
>>>> - migrate existing CONFIG_SYS_ARM_CACHE_.... in Kconfig
>>>
>>> First step done...
>>> I will push it as a separate patchset I think.
>>>
>>>> - add new option CONFIG_SYS_ARM_CACHE_OPTION
>>>
>>> In fact it is to difficult to use select because each defines
>>> DCACHE_XXX value can have several values
>>>
>>> they are build according CONFIG_ARM64 / LPAE
>>>
>>> But, I can't use this define in Kconfig
>>>
>>> I try :
>>> option	ARM_OPTION
>>> 	int "option"
>>> 	default DCACHE_WRITETHROUGHT if
>> CONFIG_SYS_ARM_CACHE_WRITETHROUGH
>>> 	default DCACHE_ WRITEALLOC if CONFIG_SYS_ARM_CACHE_
>> WRITEALLOC
>>> 	default DCACHE_WRITEBACK if
>> CONFIG_SYS_ARM_CACHE_WRITEBACK
>>>
>>> int and hex is invalid, and string can't be use with "".
>>>
>>> And I don't found way to build it automatically when option is activated.
>>>
>>> Any idea ?
>>
>> Maybe you can have a select in the Kconfig to set some differently named option,
>> e.g.
>>
>> DCACHE_MODE_WRITE{THROUGH,ALLOC,BACK}
>>
>> and then an ifdef in some header file, e.g.
>>
>> #ifdef CONFIG_DCACHE_MODE_WRITETHROUGH
>> #define ARM_CACHE_MODE DCACHE_WRITETHROUGH ...
>>
>> And then use ARM_CACHE_MODE where you need a value and
>> CONFIG_DCACHE_MODE{...} where you need a config option check.
>>
>> Does this work ?
> 
> I try with string and default (as select is allowed on for bolean or trisate),
> And I failed :-<
> 
> I don't found a way to have the de-stringficate the KConfig option
> to generated the correct define

The result is a boolean , isn't it ? One out of N configs ends up being
defined and the rest are not defined.

> config SYS_ARM_CACHE_POLICY
> 	string "Name of the ARM data write cache policy"
> 	default WRITEBACK if SYS_ARM_CACHE_WRITEBACK
> 	default WRITETHROUGH if SYS_ARM_CACHE_WRITEBACK
> 	default WRITEALLOC if SYS_ARM_CACHE_WRITEALLOC
> 
> #define DCACHE_DEFAULT_OPTION	DCACHE_ ## CONFIG_SYS_ARM_CACHE_POLICY
> 
> => error: ?DCACHE_CONFIG_SYS_ARM_CACHE_POLICY? undeclared (first use in this function); did you mean ?CONFIG_SYS_ARM_CACHE_POLICY??
> 
> #define DCACHE_OPTION(s)	DCACHE_ ## CONFIG_SYS_ARM_CACHE_POLICY
> 
> #define DCACHE_DEFAULT_OPTION	DCACHE_OPTION(CONFIG_SYS_ARM_CACHE_POLICY)
> 
> arch/arm/include/asm/system.h:488:26: error: ?DCACHE_? undeclared (first use in this function); did you mean ?DCACHE_OFF??
> arch/arm/lib/cache-cp15.c:99:25: error: expected ?)? before string constant
> 
> The stringification is possible but not the inverse operation (remove the quote)...
> 
> In my .config, CONFIG_SYS_ARM_CACHE_POLICY="WRITEBACK"

What about this:

choice
prompt "Cache policy"
 default CACHE_WRITEBACK

config CACHE_WRITEBACK
 bool "Writeback"

config ...

endchoice

and then in some header

#ifdef CONFIG_CACHE_WRITEBACK
#define CONFIF_SYS_ARM_CACHE_WRITEBACK
#else
...

Would that work ?

But I feel I might really be missing the question here.

[PATCH 3/3] arm: caches: manage phys_addr_t overflow in mmu_set_region_dcache_behaviour

[Patrick DELAUNAY]

Dear Marek

> From: Marek Vasut <marex@denx.de>
> Sent: vendredi 10 avril 2020 10:06
> 
> On 4/9/20 4:18 PM, Patrick DELAUNAY wrote:
> >
> >
> >> -----Original Message-----
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: vendredi 3 avril 2020 23:31
> >> To: Patrick DELAUNAY <patrick.delaunay@st.com>; u-boot at lists.denx.de
> >> Cc: Simon Glass <sjg@chromium.org>; Alexey Brodkin
> >> <abrodkin@synopsys.com>; Lokesh Vutla <lokeshvutla@ti.com>; Tom Rini
> >> <trini@konsulko.com>; Trevor Woerner <trevor@toganlabs.com>; U-Boot
> >> STM32 <uboot-stm32@st-md-mailman.stormreply.com>
> >> Subject: Re: [PATCH 3/3] arm: caches: manage phys_addr_t overflow in
> >> mmu_set_region_dcache_behaviour
> >> Importance: High
> >>
> >> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
> >>> Detect and solve the overflow on phys_addr_t type for start + size
> >>> in
> >>> mmu_set_region_dcache_behaviour() function.
> >>>
> >>> This issue occurs for example with ARM32, start = 0xC0000000 and
> >>> size = 0x40000000: start + size = 0x100000000 and end = 0x0.
> >>>
> >>> Overflow is detected when end < start.
> >>> In normal case the previous behavior is still used: when start is
> >>> not aligned on MMU section, the end address is only aligned after
> >>> the sum start + size.
> >>>
> >>> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> >>> ---
> >>>
> >>>  arch/arm/lib/cache-cp15.c | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c
> >>> index d15144188b..e5a7fd0ef4 100644
> >>> --- a/arch/arm/lib/cache-cp15.c
> >>> +++ b/arch/arm/lib/cache-cp15.c
> >>> @@ -63,6 +63,11 @@ void mmu_set_region_dcache_behaviour(phys_addr_t
> >>> start, size_t size,
> >>>
> >>>  	end = ALIGN(start + size, MMU_SECTION_SIZE) >>
> >> MMU_SECTION_SHIFT;
> >>>  	start = start >> MMU_SECTION_SHIFT;
> >>> +
> >>> +	/* phys_addr_t overflow detected */
> >>> +	if (end < start)
> >>> +		end = (~(phys_addr_t)0x0 >> MMU_SECTION_SHIFT) + 1;
> >>> +
> >>
> >> Or, you can divide $start and $size separately by MMU_SECTION_SIZE
> >> and then add them up .
> >
> > It was my first idea but that change the function behavior, because
> > today start and size can be not aligned on MMU_SECTION aligned.
> >
> > I think it is strange, but I preferred to don't change this part.
> >
> > Example with shift = 21 and 2MB section size: 0x200000
> >
> > Start = 0x1000000
> > Size = 0x1000000
> >
> > End = 0x2000000
> >
> > => after alignment start = 0x0, end = 0x1
> >
> > But if we align the start and size before addition as proposed, the
> > final result change
> >
> > Start = 0x1000000 => 0
> > Size = 0x1000000 => 0
> >
> > End = 0x0
> >
> > I prefer don't modify this current (strange) behavior to avoid regression.
> >
> > But if it is acceptable (because the caller MUST always use start and
> > size MMU_SECTION aligned), I will change the proposal
> 
> The minimum page size is 4k, right ? Then divide both by 4k and then by the rest
> of MMU_SECTION_SHIFT.

Yes, good idea...
I am waiting possible other feedbacks

but I think ii ts candidate to integrate V2.

Patrick

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Patrick DELAUNAY]

Hi Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: vendredi 10 avril 2020 10:14
> 
> On 4/9/20 8:06 PM, Patrick DELAUNAY wrote:
> > Dear Marek,
> 
> Hi,
> 
> >> Sent: jeudi 9 avril 2020 12:21
> >> To: Patrick DELAUNAY <patrick.delaunay@st.com>; u-boot at lists.denx.de
> >>
> >> On 4/9/20 12:01 PM, Patrick DELAUNAY wrote:
> >>> Dear Marek,
> >>
> >> Hi,
> >>
> >>>> From: Uboot-stm32
> >>>> <uboot-stm32-bounces@st-md-mailman.stormreply.com>
> >>>> On Behalf Of Patrick DELAUNAY
> >>>>
> >>>> Dear Marek,
> >>>>
> >>>>> From: Marek Vasut <marex@denx.de>
> >>>>> Sent: vendredi 3 avril 2020 23:29
> >>>>>
> >>>>> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
> >>>>>> Add the new flags DCACHE_DEFAULT_OPTION to define the default
> >>>>>> option to use according the compilation flags
> >>>>>> CONFIG_SYS_ARM_CACHE_WRITETHROUGH or
> >>>>> CONFIG_SYS_ARM_CACHE_WRITEALLOC.
> >>>>>
> >>>>> Can't you unify these macros into a single Kconfig "select"
> >>>>> statement instead , and then just select the matching cache
> >>>>> configuration in
> >> Kconfig ?
> >>>>
> >>>> Yes I will try, with 2 steps
> >>>> - migrate existing CONFIG_SYS_ARM_CACHE_.... in Kconfig
> >>>
> >>> First step done...
> >>> I will push it as a separate patchset I think.
> >>>
> >>>> - add new option CONFIG_SYS_ARM_CACHE_OPTION
> >>>
> >>> In fact it is to difficult to use select because each defines
> >>> DCACHE_XXX value can have several values
> >>>
> >>> they are build according CONFIG_ARM64 / LPAE
> >>>
> >>> But, I can't use this define in Kconfig
> >>>
> >>> I try :
> >>> option	ARM_OPTION
> >>> 	int "option"
> >>> 	default DCACHE_WRITETHROUGHT if
> >> CONFIG_SYS_ARM_CACHE_WRITETHROUGH
> >>> 	default DCACHE_ WRITEALLOC if CONFIG_SYS_ARM_CACHE_
> >> WRITEALLOC
> >>> 	default DCACHE_WRITEBACK if
> >> CONFIG_SYS_ARM_CACHE_WRITEBACK
> >>>
> >>> int and hex is invalid, and string can't be use with "".
> >>>
> >>> And I don't found way to build it automatically when option is activated.
> >>>
> >>> Any idea ?
> >>
> >> Maybe you can have a select in the Kconfig to set some differently
> >> named option, e.g.
> >>
> >> DCACHE_MODE_WRITE{THROUGH,ALLOC,BACK}
> >>
> >> and then an ifdef in some header file, e.g.
> >>
> >> #ifdef CONFIG_DCACHE_MODE_WRITETHROUGH #define
> ARM_CACHE_MODE
> >> DCACHE_WRITETHROUGH ...
> >>
> >> And then use ARM_CACHE_MODE where you need a value and
> >> CONFIG_DCACHE_MODE{...} where you need a config option check.
> >>
> >> Does this work ?
> >
> > I try with string and default (as select is allowed on for bolean or
> > trisate), And I failed :-<
> >
> > I don't found a way to have the de-stringficate the KConfig option to
> > generated the correct define
> 
> The result is a boolean , isn't it ? One out of N configs ends up being defined and
> the rest are not defined.
> 
> > config SYS_ARM_CACHE_POLICY
> > 	string "Name of the ARM data write cache policy"
> > 	default WRITEBACK if SYS_ARM_CACHE_WRITEBACK
> > 	default WRITETHROUGH if SYS_ARM_CACHE_WRITEBACK
> > 	default WRITEALLOC if SYS_ARM_CACHE_WRITEALLOC
> >
> > #define DCACHE_DEFAULT_OPTION	DCACHE_ ##
> CONFIG_SYS_ARM_CACHE_POLICY
> >
> > => error: ?DCACHE_CONFIG_SYS_ARM_CACHE_POLICY? undeclared (first
> use in this function); did you mean ?CONFIG_SYS_ARM_CACHE_POLICY??
> >
> > #define DCACHE_OPTION(s)	DCACHE_ ##
> CONFIG_SYS_ARM_CACHE_POLICY
> >
> > #define DCACHE_DEFAULT_OPTION
> 	DCACHE_OPTION(CONFIG_SYS_ARM_CACHE_POLICY)
> >
> > arch/arm/include/asm/system.h:488:26: error: ?DCACHE_? undeclared (first use
> in this function); did you mean ?DCACHE_OFF??
> > arch/arm/lib/cache-cp15.c:99:25: error: expected ?)? before string
> > constant
> >
> > The stringification is possible but not the inverse operation (remove the quote)...
> >
> > In my .config, CONFIG_SYS_ARM_CACHE_POLICY="WRITEBACK"
> 
> What about this:
> 
> choice
> prompt "Cache policy"
>  default CACHE_WRITEBACK
> 
> config CACHE_WRITEBACK
>  bool "Writeback"
> 
> config ...
> 
> endchoice
> 
> and then in some header
> 
> #ifdef CONFIG_CACHE_WRITEBACK
> #define CONFIF_SYS_ARM_CACHE_WRITEBACK
> #else
> ...
> 
> Would that work ?

Yes, it can work it seems complicated

I push v2 for CONFIG_SYS_ARM_CACHE_* migration in Kconfig

My proposal becomes:

+#if defined(CONFIG_SYS_ARM_CACHE_WRITETHROUGH)
+#define DCACHE_DEFAULT_OPTION	DCACHE_WRITETHROUGH
+#elif defined(CONFIG_SYS_ARM_CACHE_WRITEALLOC)
+#define DCACHE_DEFAULT_OPTION	DCACHE_WRITEALLOC
+#elif defined(CONFIG_SYS_ARM_CACHE_WRITEBACK)
+#define DCACHE_DEFAULT_OPTION	DCACHE_WRITEBACK
+#endif
+

I think it is is more clear solution.


I can use macro magic to build DCACHE_DEFAULT_OPTION 

+#if defined(CONFIG_SYS_ARM_CACHE_WRITETHROUGH)
+#define ARM_CACHE_POLICY	WRITETHROUGH
+#elif defined(CONFIG_SYS_ARM_CACHE_WRITEALLOC)
+#define ARM_CACHE_POLICY	WRITEALLOC
+#elif defined(CONFIG_SYS_ARM_CACHE_WRITEBACK)
+#define ARM_CACHE_POLICY	WRITEBACK
+#endif
 
+#define _DCACHE_OPTION(policy)	DCACHE_ ## policy
+#define DCACHE_OPTION(policy)	_DCACHE_OPTION(policy)
+#define DCACHE_DEFAULT_OPTION	DCACHE_OPTION(ARM_CACHE_POLICY)
+

> 
> But I feel I might really be missing the question here.

I think I will push V2 in one week (I am out of office next week) to wait other feedback.

It could be more clear with an updated patchset.

Regards

[PATCH 3/3] arm: caches: manage phys_addr_t overflow in mmu_set_region_dcache_behaviour

[Marek Vasut]

On 4/10/20 3:24 PM, Patrick DELAUNAY wrote:
> Dear Marek
> 
>> From: Marek Vasut <marex@denx.de>
>> Sent: vendredi 10 avril 2020 10:06
>>
>> On 4/9/20 4:18 PM, Patrick DELAUNAY wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Marek Vasut <marex@denx.de>
>>>> Sent: vendredi 3 avril 2020 23:31
>>>> To: Patrick DELAUNAY <patrick.delaunay@st.com>; u-boot at lists.denx.de
>>>> Cc: Simon Glass <sjg@chromium.org>; Alexey Brodkin
>>>> <abrodkin@synopsys.com>; Lokesh Vutla <lokeshvutla@ti.com>; Tom Rini
>>>> <trini@konsulko.com>; Trevor Woerner <trevor@toganlabs.com>; U-Boot
>>>> STM32 <uboot-stm32@st-md-mailman.stormreply.com>
>>>> Subject: Re: [PATCH 3/3] arm: caches: manage phys_addr_t overflow in
>>>> mmu_set_region_dcache_behaviour
>>>> Importance: High
>>>>
>>>> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
>>>>> Detect and solve the overflow on phys_addr_t type for start + size
>>>>> in
>>>>> mmu_set_region_dcache_behaviour() function.
>>>>>
>>>>> This issue occurs for example with ARM32, start = 0xC0000000 and
>>>>> size = 0x40000000: start + size = 0x100000000 and end = 0x0.
>>>>>
>>>>> Overflow is detected when end < start.
>>>>> In normal case the previous behavior is still used: when start is
>>>>> not aligned on MMU section, the end address is only aligned after
>>>>> the sum start + size.
>>>>>
>>>>> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
>>>>> ---
>>>>>
>>>>>  arch/arm/lib/cache-cp15.c | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c
>>>>> index d15144188b..e5a7fd0ef4 100644
>>>>> --- a/arch/arm/lib/cache-cp15.c
>>>>> +++ b/arch/arm/lib/cache-cp15.c
>>>>> @@ -63,6 +63,11 @@ void mmu_set_region_dcache_behaviour(phys_addr_t
>>>>> start, size_t size,
>>>>>
>>>>>  	end = ALIGN(start + size, MMU_SECTION_SIZE) >>
>>>> MMU_SECTION_SHIFT;
>>>>>  	start = start >> MMU_SECTION_SHIFT;
>>>>> +
>>>>> +	/* phys_addr_t overflow detected */
>>>>> +	if (end < start)
>>>>> +		end = (~(phys_addr_t)0x0 >> MMU_SECTION_SHIFT) + 1;
>>>>> +
>>>>
>>>> Or, you can divide $start and $size separately by MMU_SECTION_SIZE
>>>> and then add them up .
>>>
>>> It was my first idea but that change the function behavior, because
>>> today start and size can be not aligned on MMU_SECTION aligned.
>>>
>>> I think it is strange, but I preferred to don't change this part.
>>>
>>> Example with shift = 21 and 2MB section size: 0x200000
>>>
>>> Start = 0x1000000
>>> Size = 0x1000000
>>>
>>> End = 0x2000000
>>>
>>> => after alignment start = 0x0, end = 0x1
>>>
>>> But if we align the start and size before addition as proposed, the
>>> final result change
>>>
>>> Start = 0x1000000 => 0
>>> Size = 0x1000000 => 0
>>>
>>> End = 0x0
>>>
>>> I prefer don't modify this current (strange) behavior to avoid regression.
>>>
>>> But if it is acceptable (because the caller MUST always use start and
>>> size MMU_SECTION aligned), I will change the proposal
>>
>> The minimum page size is 4k, right ? Then divide both by 4k and then by the rest
>> of MMU_SECTION_SHIFT.
> 
> Yes, good idea...
> I am waiting possible other feedbacks
> 
> but I think ii ts candidate to integrate V2.

It's much better than dealing with overflows, esp. if you're shifting by
power-of-two anyway. And using 4k should also take care of LPAE 36bit
address space.

[PATCH 2/3] arm: caches: add DCACHE_DEFAULT_OPTION

[Marek Vasut]

On 4/10/20 4:58 PM, Patrick DELAUNAY wrote:
> Hi Marek,
> 
>> From: Marek Vasut <marex@denx.de>
>> Sent: vendredi 10 avril 2020 10:14
>>
>> On 4/9/20 8:06 PM, Patrick DELAUNAY wrote:
>>> Dear Marek,
>>
>> Hi,
>>
>>>> Sent: jeudi 9 avril 2020 12:21
>>>> To: Patrick DELAUNAY <patrick.delaunay@st.com>; u-boot at lists.denx.de
>>>>
>>>> On 4/9/20 12:01 PM, Patrick DELAUNAY wrote:
>>>>> Dear Marek,
>>>>
>>>> Hi,
>>>>
>>>>>> From: Uboot-stm32
>>>>>> <uboot-stm32-bounces@st-md-mailman.stormreply.com>
>>>>>> On Behalf Of Patrick DELAUNAY
>>>>>>
>>>>>> Dear Marek,
>>>>>>
>>>>>>> From: Marek Vasut <marex@denx.de>
>>>>>>> Sent: vendredi 3 avril 2020 23:29
>>>>>>>
>>>>>>> On 4/3/20 10:28 AM, Patrick Delaunay wrote:
>>>>>>>> Add the new flags DCACHE_DEFAULT_OPTION to define the default
>>>>>>>> option to use according the compilation flags
>>>>>>>> CONFIG_SYS_ARM_CACHE_WRITETHROUGH or
>>>>>>> CONFIG_SYS_ARM_CACHE_WRITEALLOC.
>>>>>>>
>>>>>>> Can't you unify these macros into a single Kconfig "select"
>>>>>>> statement instead , and then just select the matching cache
>>>>>>> configuration in
>>>> Kconfig ?
>>>>>>
>>>>>> Yes I will try, with 2 steps
>>>>>> - migrate existing CONFIG_SYS_ARM_CACHE_.... in Kconfig
>>>>>
>>>>> First step done...
>>>>> I will push it as a separate patchset I think.
>>>>>
>>>>>> - add new option CONFIG_SYS_ARM_CACHE_OPTION
>>>>>
>>>>> In fact it is to difficult to use select because each defines
>>>>> DCACHE_XXX value can have several values
>>>>>
>>>>> they are build according CONFIG_ARM64 / LPAE
>>>>>
>>>>> But, I can't use this define in Kconfig
>>>>>
>>>>> I try :
>>>>> option	ARM_OPTION
>>>>> 	int "option"
>>>>> 	default DCACHE_WRITETHROUGHT if
>>>> CONFIG_SYS_ARM_CACHE_WRITETHROUGH
>>>>> 	default DCACHE_ WRITEALLOC if CONFIG_SYS_ARM_CACHE_
>>>> WRITEALLOC
>>>>> 	default DCACHE_WRITEBACK if
>>>> CONFIG_SYS_ARM_CACHE_WRITEBACK
>>>>>
>>>>> int and hex is invalid, and string can't be use with "".
>>>>>
>>>>> And I don't found way to build it automatically when option is activated.
>>>>>
>>>>> Any idea ?
>>>>
>>>> Maybe you can have a select in the Kconfig to set some differently
>>>> named option, e.g.
>>>>
>>>> DCACHE_MODE_WRITE{THROUGH,ALLOC,BACK}
>>>>
>>>> and then an ifdef in some header file, e.g.
>>>>
>>>> #ifdef CONFIG_DCACHE_MODE_WRITETHROUGH #define
>> ARM_CACHE_MODE
>>>> DCACHE_WRITETHROUGH ...
>>>>
>>>> And then use ARM_CACHE_MODE where you need a value and
>>>> CONFIG_DCACHE_MODE{...} where you need a config option check.
>>>>
>>>> Does this work ?
>>>
>>> I try with string and default (as select is allowed on for bolean or
>>> trisate), And I failed :-<
>>>
>>> I don't found a way to have the de-stringficate the KConfig option to
>>> generated the correct define
>>
>> The result is a boolean , isn't it ? One out of N configs ends up being defined and
>> the rest are not defined.
>>
>>> config SYS_ARM_CACHE_POLICY
>>> 	string "Name of the ARM data write cache policy"
>>> 	default WRITEBACK if SYS_ARM_CACHE_WRITEBACK
>>> 	default WRITETHROUGH if SYS_ARM_CACHE_WRITEBACK
>>> 	default WRITEALLOC if SYS_ARM_CACHE_WRITEALLOC
>>>
>>> #define DCACHE_DEFAULT_OPTION	DCACHE_ ##
>> CONFIG_SYS_ARM_CACHE_POLICY
>>>
>>> => error: ?DCACHE_CONFIG_SYS_ARM_CACHE_POLICY? undeclared (first
>> use in this function); did you mean ?CONFIG_SYS_ARM_CACHE_POLICY??
>>>
>>> #define DCACHE_OPTION(s)	DCACHE_ ##
>> CONFIG_SYS_ARM_CACHE_POLICY
>>>
>>> #define DCACHE_DEFAULT_OPTION
>> 	DCACHE_OPTION(CONFIG_SYS_ARM_CACHE_POLICY)
>>>
>>> arch/arm/include/asm/system.h:488:26: error: ?DCACHE_? undeclared (first use
>> in this function); did you mean ?DCACHE_OFF??
>>> arch/arm/lib/cache-cp15.c:99:25: error: expected ?)? before string
>>> constant
>>>
>>> The stringification is possible but not the inverse operation (remove the quote)...
>>>
>>> In my .config, CONFIG_SYS_ARM_CACHE_POLICY="WRITEBACK"
>>
>> What about this:
>>
>> choice
>> prompt "Cache policy"
>>  default CACHE_WRITEBACK
>>
>> config CACHE_WRITEBACK
>>  bool "Writeback"
>>
>> config ...
>>
>> endchoice
>>
>> and then in some header
>>
>> #ifdef CONFIG_CACHE_WRITEBACK
>> #define CONFIF_SYS_ARM_CACHE_WRITEBACK
>> #else
>> ...
>>
>> Would that work ?
> 
> Yes, it can work it seems complicated
> 
> I push v2 for CONFIG_SYS_ARM_CACHE_* migration in Kconfig
> 
> My proposal becomes:
> 
> +#if defined(CONFIG_SYS_ARM_CACHE_WRITETHROUGH)
> +#define DCACHE_DEFAULT_OPTION	DCACHE_WRITETHROUGH
> +#elif defined(CONFIG_SYS_ARM_CACHE_WRITEALLOC)
> +#define DCACHE_DEFAULT_OPTION	DCACHE_WRITEALLOC
> +#elif defined(CONFIG_SYS_ARM_CACHE_WRITEBACK)
> +#define DCACHE_DEFAULT_OPTION	DCACHE_WRITEBACK
> +#endif
> +
> 
> I think it is is more clear solution.
> 
> 
> I can use macro magic to build DCACHE_DEFAULT_OPTION 
> 
> +#if defined(CONFIG_SYS_ARM_CACHE_WRITETHROUGH)
> +#define ARM_CACHE_POLICY	WRITETHROUGH
> +#elif defined(CONFIG_SYS_ARM_CACHE_WRITEALLOC)
> +#define ARM_CACHE_POLICY	WRITEALLOC
> +#elif defined(CONFIG_SYS_ARM_CACHE_WRITEBACK)
> +#define ARM_CACHE_POLICY	WRITEBACK
> +#endif
>  
> +#define _DCACHE_OPTION(policy)	DCACHE_ ## policy
> +#define DCACHE_OPTION(policy)	_DCACHE_OPTION(policy)
> +#define DCACHE_DEFAULT_OPTION	DCACHE_OPTION(ARM_CACHE_POLICY)
> +
> 
>>
>> But I feel I might really be missing the question here.
> 
> I think I will push V2 in one week (I am out of office next week) to wait other feedback.
> 
> It could be more clear with an updated patchset.

That's a good idea, let's see.