Hi Stefan, > Hi Lukasz, > > sorry, I still have some comments to (hopefully) make this > implementation a bit more "elegant". Please see below. > > On 29.04.2018 15:36, Lukasz Majewski wrote: > > Those two functions can be used to provide easy bootcount > > management. > > > > Signed-off-by: Lukasz Majewski <[email protected]> > > > > --- > > > > Changes in v3: > > - Unify those functions to also work with common/autoboot.c code > > - Add enum bootcount_context to distinguish between u-boot proper > > and SPL > > > > Changes in v2: > > - None > > > > include/bootcount.h | 50 > > ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, > > 50 insertions(+) > > > > diff --git a/include/bootcount.h b/include/bootcount.h > > index e3b3f7028e..16fc657b2a 100644 > > --- a/include/bootcount.h > > +++ b/include/bootcount.h > > @@ -11,6 +11,13 @@ > > #include <asm/io.h> > > #include <asm/byteorder.h> > > > > +enum bootcount_context { > > + SPL = 1, > > + UBOOT, > > +}; > > Perhaps better some bootcount specific values / enums, like: > > enum bootcount_context { > BOOTCOUNT_STATE_SPL = 1, > BOOTCOUNT_STATE_UBOOT, > }; > > Or even more generic, as this "boot-state" does not have to be > bootcount specific: > > enum u_boot_context { > U_BOOT_STATE_SPL = 1, > U_BOOT_STATE_U_BOOT, > }; > > Or do we already have something like this, perhaps in "gd", where > its marked, in which state we are currently running? If not, it > could be added there and could be used from the bootcounter code > and other interfaces / drivers as well. The parts pre-relocation and > post-relocation fall also into this area (for the pre- / post-reloc > we definitely have a variable / flag in gd somewhere). So perhaps: > > enum u_boot_context { > U_BOOT_STATE_SPL = 1, > U_BOOT_STATE_U_BOOT_PRE_RELOC, > U_BOOT_STATE_U_BOOT_POST_RELOC, > };
I will check if we do have such flags already defined. "gd" approach
looks better (more generic for sure) than the one from v3.
>
> > +#if defined CONFIG_SPL_BOOTCOUNT_LIMIT || defined
> > CONFIG_BOOTCOUNT_LIMIT +
> > #if !defined(CONFIG_SYS_BOOTCOUNT_LE)
> > && !defined(CONFIG_SYS_BOOTCOUNT_BE) # if __BYTE_ORDER ==
> > __LITTLE_ENDIAN # define CONFIG_SYS_BOOTCOUNT_LE
> > @@ -40,4 +47,47 @@ static inline u32 raw_bootcount_load(volatile
> > u32 *addr) return in_be32(addr);
> > }
> > #endif
> > +
> > +static inline int bootcount_error(enum bootcount_context bc)
> > +{
> > + unsigned long bootcount = bootcount_load();
> > + unsigned long bootlimit = env_get_ulong("bootlimit", 10,
> > 0); +
> > + if (bootlimit && bootcount > bootlimit) {
> > + printf("Warning: Bootlimit (%lu) exceeded.",
> > bootlimit);
> > + if (bc == UBOOT)
> > + printf(" Using altbootcmd.");
> > + printf("\n");
> > +
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static inline void bootcount_inc(enum bootcount_context bc)
> > +{
> > + unsigned long bootcount = bootcount_load();
> > +
> > + if (bc == SPL) {
> > + bootcount_store(++bootcount);
> > + return;
> > + }
> > +
> > + /* Only increment bootcount when no bootcount support in
> > SPL */ +#ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> > + bootcount++;
> > +#endif
> > + bootcount_store(bootcount);
> > + env_set_ulong("bootcount", bootcount);
> > +}
>
> Why not use the same logic / code as above (the store does not need
> to happen twice):
>
> /* Only increment bootcount when no bootcount support in SPL
> */ #ifndef CONFIG_SPL_BOOTCOUNT_LIMIT
> bootcount_store(++bootcount);
> #endif
>
> env_set_ulong("bootcount", bootcount);
>
> ?
>
> What do you think?
Good point - I will add it in v4.
Thanks for input.
>
> Thanks,
> Stefan
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: [email protected]
pgpEanBzuObx7.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
