Re: [U-Boot] [PATCH] arm: cache: always flush cache line size for page table

[Stefan Agner]

On 2016-08-03 18:22, Simon Glass wrote:
> Hi,
> 
> On 3 August 2016 at 10:32, Fabio Estevam  wrote:
>> On Tue, Aug 2, 2016 at 4:07 AM, Stefan Agner  wrote:
>>> From: Stefan Agner 
>>>
>>> The page table is maintained by the CPU, hence it is safe to always
>>> align cache flush to a whole cache line size. This allows to use
>>> mmu_page_table_flush for a single page table, e.g. when configure
>>> only small regions through mmu_set_region_dcache_behaviour.
>>>
>>> Signed-off-by: Stefan Agner 
>>
>> Tested-by: Fabio Estevam 
> 
> I'm OK with this, or a change in mmu_set_region_dcache_behaviour() to
> align he addresses.

Ok will move to mmu_set_region_dcache_behaviour as Marek seems to prefer
that solution.

--
Stefan

> 
> Reviewed-by: Simon Glass 
> 
> Regards,
> Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] arm: cache: always flush cache line size for page table

[Simon Glass]

Hi,

On 3 August 2016 at 10:32, Fabio Estevam  wrote:
> On Tue, Aug 2, 2016 at 4:07 AM, Stefan Agner  wrote:
>> From: Stefan Agner 
>>
>> The page table is maintained by the CPU, hence it is safe to always
>> align cache flush to a whole cache line size. This allows to use
>> mmu_page_table_flush for a single page table, e.g. when configure
>> only small regions through mmu_set_region_dcache_behaviour.
>>
>> Signed-off-by: Stefan Agner 
>
> Tested-by: Fabio Estevam 

I'm OK with this, or a change in mmu_set_region_dcache_behaviour() to
align he addresses.

Reviewed-by: Simon Glass 

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] arm: cache: always flush cache line size for page table

[Fabio Estevam]

On Tue, Aug 2, 2016 at 4:07 AM, Stefan Agner  wrote:
> From: Stefan Agner 
>
> The page table is maintained by the CPU, hence it is safe to always
> align cache flush to a whole cache line size. This allows to use
> mmu_page_table_flush for a single page table, e.g. when configure
> only small regions through mmu_set_region_dcache_behaviour.
>
> Signed-off-by: Stefan Agner 

Tested-by: Fabio Estevam 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] arm: cache: always flush cache line size for page table

[Fabio Estevam]

Hi Stefan,

On Tue, Aug 2, 2016 at 4:07 AM, Stefan Agner  wrote:
> From: Stefan Agner 
>
> The page table is maintained by the CPU, hence it is safe to always
> align cache flush to a whole cache line size. This allows to use
> mmu_page_table_flush for a single page table, e.g. when configure
> only small regions through mmu_set_region_dcache_behaviour.
>
> Signed-off-by: Stefan Agner 
> ---
> This avoids two messages observed on a i.MX 7 based system:
> CACHE: Misaligned operation at range [9fff, 9fff0004]
> CACHE: Misaligned operation at range [9fff0024, 9fff0028]
>
> Those were caused by two calls to mmu_set_region_dcache_behaviour
> in arch/arm/imx-common/cache.c (enable_caches).
>
> Not sure if this is the right way to fix this... Also, we could
> do the alignment in mmu_set_region_dcache_behaviour.

I am also getting similar warnings on a mx6ul pico board:

U-Boot 2016.09-rc1-00245-gad6a303 (Aug 03 2016 - 10:31:52 -0300)

CPU:   Freescale i.MX6UL rev1.0 at 396 MHz
Reset cause: WDOG
Board: PICO-IMX6UL-EMMC
I2C:   ready
DRAM:  256 MiB
CACHE: Misaligned operation at range [8fff, 8fff0004]
CACHE: Misaligned operation at range [8fff0024, 8fff0028]
PMIC: PFUZE3000 DEV_ID=0x30 REV_ID=0x11
MMC:   FSL_SDHC: 0
*** Warning - bad CRC, using default environment

In:serial
Out:   serial
Err:   serial
Net:   FEC
Hit any key to stop autoboot:  0

Applying your patch makes these cache warnings go away.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] arm: cache: always flush cache line size for page table

[Stefan Agner]

On 2016-08-02 10:55, Marek Vasut wrote:
> On 08/02/2016 07:01 PM, Stefan Agner wrote:
>> On 2016-08-02 08:56, Marek Vasut wrote:
>>> On 08/02/2016 05:47 PM, Stefan Agner wrote:
 On 2016-08-02 02:38, Marek Vasut wrote:
> On 08/02/2016 09:07 AM, Stefan Agner wrote:
>> From: Stefan Agner 
>>
>> The page table is maintained by the CPU, hence it is safe to always
>> align cache flush to a whole cache line size. This allows to use
>> mmu_page_table_flush for a single page table, e.g. when configure
>> only small regions through mmu_set_region_dcache_behaviour.
>>
>> Signed-off-by: Stefan Agner 
>> ---
>> This avoids two messages observed on a i.MX 7 based system:
>> CACHE: Misaligned operation at range [9fff, 9fff0004]
>> CACHE: Misaligned operation at range [9fff0024, 9fff0028]
>>
>> Those were caused by two calls to mmu_set_region_dcache_behaviour
>> in arch/arm/imx-common/cache.c (enable_caches).
>>
>> Not sure if this is the right way to fix this... Also, we could
>> do the alignment in mmu_set_region_dcache_behaviour.
>
> This should be fixed on the driver level indeed, not in cache_v7.c

 Fixing it in enable_caches in arch/arm/imx-common/cache.c is definitely
 unpractical...

 So I guess by driver level you mean in
 arch/arm/lib/cache-cp15.c:mmu_set_region_dcache_behaviour
 correct?

 It has the potential to code duplication in case other users of
 mmu_page_table_flush need to flush page tables less than cache line
 size...
>>>
>>> Isn't the function supposed to flush the whole MMU table ? Or is the
>>> idea here to really flush separate entries ?
>>
>> It has a start/stop argument, so I guess it is supposed to flush
>> separate
>> entries...
> 
> The cache ops also have start/stop argument, but they explicitly cannot
> be used on non-cache-aligned addresses, so the start/stop argument does
> not imply anything.

mmu_set_region_dcache_behaviour and mmu_page_table_flush have been added
by
Simon in one commit, and since mmu_set_region_dcache_behaviour uses it
to
flush only parts of the page table I assume it was ment to be used to
flush
(a range of) separate entries...

In the end it really depends on how we define the semantics of those two
functions... However, we need to take care of alignment in one of those
two,
it is almost impossible on the caller site of
mmu_set_region_dcache_behaviour.

Opinions?

--
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] arm: cache: always flush cache line size for page table

[Marek Vasut]

On 08/02/2016 07:01 PM, Stefan Agner wrote:
> On 2016-08-02 08:56, Marek Vasut wrote:
>> On 08/02/2016 05:47 PM, Stefan Agner wrote:
>>> On 2016-08-02 02:38, Marek Vasut wrote:
 On 08/02/2016 09:07 AM, Stefan Agner wrote:
> From: Stefan Agner 
>
> The page table is maintained by the CPU, hence it is safe to always
> align cache flush to a whole cache line size. This allows to use
> mmu_page_table_flush for a single page table, e.g. when configure
> only small regions through mmu_set_region_dcache_behaviour.
>
> Signed-off-by: Stefan Agner 
> ---
> This avoids two messages observed on a i.MX 7 based system:
> CACHE: Misaligned operation at range [9fff, 9fff0004]
> CACHE: Misaligned operation at range [9fff0024, 9fff0028]
>
> Those were caused by two calls to mmu_set_region_dcache_behaviour
> in arch/arm/imx-common/cache.c (enable_caches).
>
> Not sure if this is the right way to fix this... Also, we could
> do the alignment in mmu_set_region_dcache_behaviour.

 This should be fixed on the driver level indeed, not in cache_v7.c
>>>
>>> Fixing it in enable_caches in arch/arm/imx-common/cache.c is definitely
>>> unpractical...
>>>
>>> So I guess by driver level you mean in
>>> arch/arm/lib/cache-cp15.c:mmu_set_region_dcache_behaviour
>>> correct?
>>>
>>> It has the potential to code duplication in case other users of
>>> mmu_page_table_flush need to flush page tables less than cache line
>>> size...
>>
>> Isn't the function supposed to flush the whole MMU table ? Or is the
>> idea here to really flush separate entries ?
> 
> It has a start/stop argument, so I guess it is supposed to flush
> separate
> entries...

The cache ops also have start/stop argument, but they explicitly cannot
be used on non-cache-aligned addresses, so the start/stop argument does
not imply anything.

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] arm: cache: always flush cache line size for page table

[Stefan Agner]

On 2016-08-02 08:56, Marek Vasut wrote:
> On 08/02/2016 05:47 PM, Stefan Agner wrote:
>> On 2016-08-02 02:38, Marek Vasut wrote:
>>> On 08/02/2016 09:07 AM, Stefan Agner wrote:
 From: Stefan Agner 

 The page table is maintained by the CPU, hence it is safe to always
 align cache flush to a whole cache line size. This allows to use
 mmu_page_table_flush for a single page table, e.g. when configure
 only small regions through mmu_set_region_dcache_behaviour.

 Signed-off-by: Stefan Agner 
 ---
 This avoids two messages observed on a i.MX 7 based system:
 CACHE: Misaligned operation at range [9fff, 9fff0004]
 CACHE: Misaligned operation at range [9fff0024, 9fff0028]

 Those were caused by two calls to mmu_set_region_dcache_behaviour
 in arch/arm/imx-common/cache.c (enable_caches).

 Not sure if this is the right way to fix this... Also, we could
 do the alignment in mmu_set_region_dcache_behaviour.
>>>
>>> This should be fixed on the driver level indeed, not in cache_v7.c
>>
>> Fixing it in enable_caches in arch/arm/imx-common/cache.c is definitely
>> unpractical...
>>
>> So I guess by driver level you mean in
>> arch/arm/lib/cache-cp15.c:mmu_set_region_dcache_behaviour
>> correct?
>>
>> It has the potential to code duplication in case other users of
>> mmu_page_table_flush need to flush page tables less than cache line
>> size...
> 
> Isn't the function supposed to flush the whole MMU table ? Or is the
> idea here to really flush separate entries ?

It has a start/stop argument, so I guess it is supposed to flush
separate
entries...

--
Stefan

> 
>> I felt that mmu_page_table_flush is a convenience function and should
>> take care of that issue.
>>
>> --
>> Stefan
>>
>>
>>
>>>
 --
 Stefan

  arch/arm/cpu/armv7/cache_v7.c | 7 +++
  1 file changed, 7 insertions(+)

 diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
 index 52f1856..71787fc 100644
 --- a/arch/arm/cpu/armv7/cache_v7.c
 +++ b/arch/arm/cpu/armv7/cache_v7.c
 @@ -147,6 +147,13 @@ void arm_init_before_mmu(void)

  void mmu_page_table_flush(unsigned long start, unsigned long stop)
  {
 +  /*
 +   * Make sure range is cache line aligned
 +   * Only CPU maintains page tables, hence it is save to always
 +   * flush the complete cache line...
 +   */
 +  start &= ~(CONFIG_SYS_CACHELINE_SIZE - 1);
 +  stop = ALIGN(stop, CONFIG_SYS_CACHELINE_SIZE);
flush_dcache_range(start, stop);
v7_inval_tlb();
  }

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] arm: cache: always flush cache line size for page table

[Marek Vasut]

On 08/02/2016 05:47 PM, Stefan Agner wrote:
> On 2016-08-02 02:38, Marek Vasut wrote:
>> On 08/02/2016 09:07 AM, Stefan Agner wrote:
>>> From: Stefan Agner 
>>>
>>> The page table is maintained by the CPU, hence it is safe to always
>>> align cache flush to a whole cache line size. This allows to use
>>> mmu_page_table_flush for a single page table, e.g. when configure
>>> only small regions through mmu_set_region_dcache_behaviour.
>>>
>>> Signed-off-by: Stefan Agner 
>>> ---
>>> This avoids two messages observed on a i.MX 7 based system:
>>> CACHE: Misaligned operation at range [9fff, 9fff0004]
>>> CACHE: Misaligned operation at range [9fff0024, 9fff0028]
>>>
>>> Those were caused by two calls to mmu_set_region_dcache_behaviour
>>> in arch/arm/imx-common/cache.c (enable_caches).
>>>
>>> Not sure if this is the right way to fix this... Also, we could
>>> do the alignment in mmu_set_region_dcache_behaviour.
>>
>> This should be fixed on the driver level indeed, not in cache_v7.c
> 
> Fixing it in enable_caches in arch/arm/imx-common/cache.c is definitely
> unpractical...
> 
> So I guess by driver level you mean in 
> arch/arm/lib/cache-cp15.c:mmu_set_region_dcache_behaviour
> correct?
> 
> It has the potential to code duplication in case other users of
> mmu_page_table_flush need to flush page tables less than cache line
> size...

Isn't the function supposed to flush the whole MMU table ? Or is the
idea here to really flush separate entries ?

> I felt that mmu_page_table_flush is a convenience function and should
> take care of that issue.
> 
> --
> Stefan
> 
> 
> 
>>
>>> --
>>> Stefan
>>>
>>>  arch/arm/cpu/armv7/cache_v7.c | 7 +++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
>>> index 52f1856..71787fc 100644
>>> --- a/arch/arm/cpu/armv7/cache_v7.c
>>> +++ b/arch/arm/cpu/armv7/cache_v7.c
>>> @@ -147,6 +147,13 @@ void arm_init_before_mmu(void)
>>>
>>>  void mmu_page_table_flush(unsigned long start, unsigned long stop)
>>>  {
>>> +   /*
>>> +* Make sure range is cache line aligned
>>> +* Only CPU maintains page tables, hence it is save to always
>>> +* flush the complete cache line...
>>> +*/
>>> +   start &= ~(CONFIG_SYS_CACHELINE_SIZE - 1);
>>> +   stop = ALIGN(stop, CONFIG_SYS_CACHELINE_SIZE);
>>> flush_dcache_range(start, stop);
>>> v7_inval_tlb();
>>>  }
>>>


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] arm: cache: always flush cache line size for page table

[Stefan Agner]

On 2016-08-02 02:38, Marek Vasut wrote:
> On 08/02/2016 09:07 AM, Stefan Agner wrote:
>> From: Stefan Agner 
>>
>> The page table is maintained by the CPU, hence it is safe to always
>> align cache flush to a whole cache line size. This allows to use
>> mmu_page_table_flush for a single page table, e.g. when configure
>> only small regions through mmu_set_region_dcache_behaviour.
>>
>> Signed-off-by: Stefan Agner 
>> ---
>> This avoids two messages observed on a i.MX 7 based system:
>> CACHE: Misaligned operation at range [9fff, 9fff0004]
>> CACHE: Misaligned operation at range [9fff0024, 9fff0028]
>>
>> Those were caused by two calls to mmu_set_region_dcache_behaviour
>> in arch/arm/imx-common/cache.c (enable_caches).
>>
>> Not sure if this is the right way to fix this... Also, we could
>> do the alignment in mmu_set_region_dcache_behaviour.
> 
> This should be fixed on the driver level indeed, not in cache_v7.c

Fixing it in enable_caches in arch/arm/imx-common/cache.c is definitely
unpractical...

So I guess by driver level you mean in 
arch/arm/lib/cache-cp15.c:mmu_set_region_dcache_behaviour
correct?

It has the potential to code duplication in case other users of
mmu_page_table_flush need to flush page tables less than cache line
size...

I felt that mmu_page_table_flush is a convenience function and should
take care of that issue.

--
Stefan



> 
>> --
>> Stefan
>>
>>  arch/arm/cpu/armv7/cache_v7.c | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
>> index 52f1856..71787fc 100644
>> --- a/arch/arm/cpu/armv7/cache_v7.c
>> +++ b/arch/arm/cpu/armv7/cache_v7.c
>> @@ -147,6 +147,13 @@ void arm_init_before_mmu(void)
>>
>>  void mmu_page_table_flush(unsigned long start, unsigned long stop)
>>  {
>> +/*
>> + * Make sure range is cache line aligned
>> + * Only CPU maintains page tables, hence it is save to always
>> + * flush the complete cache line...
>> + */
>> +start &= ~(CONFIG_SYS_CACHELINE_SIZE - 1);
>> +stop = ALIGN(stop, CONFIG_SYS_CACHELINE_SIZE);
>>  flush_dcache_range(start, stop);
>>  v7_inval_tlb();
>>  }
>>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] arm: cache: always flush cache line size for page table

[Marek Vasut]

On 08/02/2016 09:07 AM, Stefan Agner wrote:
> From: Stefan Agner 
> 
> The page table is maintained by the CPU, hence it is safe to always
> align cache flush to a whole cache line size. This allows to use
> mmu_page_table_flush for a single page table, e.g. when configure
> only small regions through mmu_set_region_dcache_behaviour.
> 
> Signed-off-by: Stefan Agner 
> ---
> This avoids two messages observed on a i.MX 7 based system:
> CACHE: Misaligned operation at range [9fff, 9fff0004]
> CACHE: Misaligned operation at range [9fff0024, 9fff0028]
> 
> Those were caused by two calls to mmu_set_region_dcache_behaviour
> in arch/arm/imx-common/cache.c (enable_caches).
> 
> Not sure if this is the right way to fix this... Also, we could
> do the alignment in mmu_set_region_dcache_behaviour.

This should be fixed on the driver level indeed, not in cache_v7.c

> --
> Stefan
> 
>  arch/arm/cpu/armv7/cache_v7.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> index 52f1856..71787fc 100644
> --- a/arch/arm/cpu/armv7/cache_v7.c
> +++ b/arch/arm/cpu/armv7/cache_v7.c
> @@ -147,6 +147,13 @@ void arm_init_before_mmu(void)
>  
>  void mmu_page_table_flush(unsigned long start, unsigned long stop)
>  {
> + /*
> +  * Make sure range is cache line aligned
> +  * Only CPU maintains page tables, hence it is save to always
> +  * flush the complete cache line...
> +  */
> + start &= ~(CONFIG_SYS_CACHELINE_SIZE - 1);
> + stop = ALIGN(stop, CONFIG_SYS_CACHELINE_SIZE);
>   flush_dcache_range(start, stop);
>   v7_inval_tlb();
>  }
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot