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

[Heiko Schocher]

Hello Lukasz,

Am 22.02.2018 um 14:03 schrieb Lukasz Majewski:
> Hi Stefano,
> 
>> 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 at 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 ?
> 
> I think not - as we are now in u-boot.
> 
> (I do have patches, which add also support for bootcount in SPL - I
> will send them when Kconfig prerequisities go into mainline).
> 
>>
>> flush_dcache_range() requires that addresses are aligned with
>> CONFIG_SYS_CACHELINE_SIZE. I cannot flush a single word.
> 
> Yes. Correct.
> 
>>
>>>
>>> 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.
> 
> You mean the SRC_GPRx registers?
> 
>>
>> 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,...).
> 
> What I can say.... :/
> 
>>
>> Due to recent issues, I searched for a suitable place in IRAM.
> 
> Ok.
> 
>>
>> Anyway, this has nothing to do with the issue. If the storage with the
>> bootcounter is cached, it must be flushed.
> 
> Yes. Of course.
> 
>>
>>
>>> 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.
> 
> Ok. Maybe the reply from the NXP community is not complete...

Hmm.. I have here an imx6dl based board:

CPU:   Freescale i.MX6DL rev1.3 at 792 MHz

and I tried to put the bootcounter into the iram, but found no addr, which
is stable over a reboot from linux into U-Boot ... I must admit, that I
use the GPU may it use iram? Nevertheless, unsure place.

Tried SRC_GPRx registers, but also not stable after a "boot linux - reboot to
U-Boot" cycle.

imx6 D/QL UML says
SRC_GPRx with x = 5-8 for quad only -> so not usable for me

and x=(1-8):

"Holds argument of entry function for coreX. The SRC ensures that the register
value will persist across  system resets."

with (X=0-3)

So, the core may uses them ...

The S/DL manual says to this registers:
"Read/write general purpose bits used to store an arbitrary value.
This register is used by the ROM code and should not be used by application software."

So also on S/DL no chance to use this registers without risk ...

I have on my hw a pfuze100 and I stored the bootcounter in the 4 8bit
registers MEMA - MEMD ... as we have no battery attached to the pfuze
this works fine... as you said "Development by luck" :-D

Hmm... SVN_LGPR regsiter ... sounds good. Tried it fast here on my hw,
seems to work ... but may instability remains, UML says:

"When GPR_SL or GPR_HL bit is set, the register cannot be programmed."

Once GPR_SL bit is set, it can only cleared through System Reset ...

May a risk?

bye,
Heiko
> 
>>
>>>
>>> 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....
> 
> It would be great if Heiko could share the problem with SRC_GPRx
> registers. I would like to know the root cause for the future usage.
> 
>>
>>> However, I do
>>> prefer SNVS_LPGR.
>>
>> You're lucky you do not yet go into trouble :-)
> 
> Devlopment by luck :-)
> 
>>
>>>    
>>>>   }
>>>>   
>>>>   __weak ulong bootcount_load(void)
>>>
>>>
>>>    
>>
>> Best regards,
>> Stefano
>>
>>
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> 

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