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 <lu...@denx.de>
> >
> > ---
> >
> > 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: w...@denx.de
pgpEanBzuObx7.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
