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

[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...

> 
> > 
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180222/9814b1b6/attachment.sig>