[U-Boot] [PATCH] bootcount: flush after storing the bootcounter

[Stefano Babic <sbabic@denx.de>]

Hi Lukasz,

On 22/02/2018 12:52, Lukasz Majewski wrote:
> Hi Stefano,
> 
>> If the bootcounter address is in a cached memory,
>> a flush of dcache must occur after updateing the bootcounter.
>>
>> Issue found on i.MX6 where bootcounter is put into the internal
>> (cached) IRAM.
>>
>> Signed-off-by: Stefano Babic <sbabic@denx.de>
>> ---
>>  drivers/bootcount/bootcount.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/bootcount/bootcount.c
>> b/drivers/bootcount/bootcount.c index d5ce450..48594a6 100644
>> --- a/drivers/bootcount/bootcount.c
>> +++ b/drivers/bootcount/bootcount.c
>> @@ -59,6 +59,9 @@ __weak void bootcount_store(ulong a)
>>  	raw_bootcount_store(reg, a);
>>  	raw_bootcount_store(reg + 4, BOOTCOUNT_MAGIC);
>>  #endif /* defined(CONFIG_SYS_BOOTCOUNT_SINGLEWORD */
>> +	flush_dcache_range(CONFIG_SYS_BOOTCOUNT_ADDR,
>> +				CONFIG_SYS_BOOTCOUNT_ADDR +
>> +				CONFIG_SYS_CACHELINE_SIZE);
> 
> Is it safe to flush the whole cache line?

Is there is some drawbacks ?

flush_dcache_range() requires that addresses are aligned with
CONFIG_SYS_CACHELINE_SIZE. I cannot flush a single word.

> 
> For iMX6Q I do use SNVS_LPGR register (0x020CC068).

It is a long story...

> It will preserve
> its content after reset caused by WDT (also reset command in u-boot).
> I also do use the SINGLEWORD to store "magic" and boot count in a
> single 32 bit number.
> 

I used it in the past - Heiko found issues with recent kernel versions
because kernel is using it. That means, after a rebootwe get what the
kernel has written in it, not the bootcounter anymore.

It looks like, too, that SNVS behavior changes between i.MX6 variants.
Even in manual, there are discordances (on i.MX6Q seems can be used, on
DL seems to be reserved,...).

Due to recent issues, I searched for a suitable place in IRAM.

Anyway, this has nothing to do with the issue. If the storage with the
bootcounter is cached, it must be flushed.


> You may also want to consider using SRC_GPRx registers:
> https://community.nxp.com/message/985790?commentID=985790&et=watches.email.thread#comment-985790

See above, waiting for Heiko's comment,too.

> 
> As it shall be safe to use them for bootcount scenario.

Rather, it looks like it is not safe. Or it was safe, it is not. Or it
depends on i.MX6 revisions....

> However, I do
> prefer SNVS_LPGR.

You're lucky you do not yet go into trouble :-)

> 
>>  }
>>  
>>  __weak ulong bootcount_load(void)
> 
> 
> 

Best regards,
Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================