On Wed, Sep 20, 2023 at 07:03:34PM -0600, Simon Glass wrote:
> Hi Tom,
>
> On Wed, 30 Aug 2023 at 15:39, Tom Rini <tr...@konsulko.com> wrote:
> >
> > On Wed, Aug 30, 2023 at 12:04:40PM -0600, Simon Glass wrote:
> > > Use an accessor in the header file to avoid this.
> > >
> > > Signed-off-by: Simon Glass <s...@chromium.org>
> > > ---
> > >
> > > common/spl/spl.c | 9 +++++----
> > > include/asm-generic/global_data.h | 7 +++++++
> > > 2 files changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > > index f0a90c280da..f5cef81000c 100644
> > > --- a/common/spl/spl.c
> > > +++ b/common/spl/spl.c
> > > @@ -876,10 +876,11 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
> > > } else {
> > > debug("Unsupported OS image.. Jumping nevertheless..\n");
> > > }
> > > -#if CONFIG_VAL(SYS_MALLOC_F_LEN) && !defined(CONFIG_SPL_SYS_MALLOC_SIZE)
> > > - debug("SPL malloc() used 0x%lx bytes (%ld KB)\n", gd->malloc_ptr,
> > > - gd->malloc_ptr / 1024);
> > > -#endif
> > > + if (IS_ENABLED(CONFIG_SYS_MALLOC_F) &&
> > > + !IS_ENABLED(CONFIG_SPL_SYS_MALLOC_SIZE))
> > > + debug("SPL malloc() used 0x%lx bytes (%ld KB)\n",
> > > + gd_malloc_ptr(), gd_malloc_ptr() / 1024);This is not more readable. But I guess my comment was unclear as you're mixing changes here. gd_malloc_ptr() rather than gd->malloc_ptr is a wash, to me. I think one could argue it's not a helpful abstraction. but it's so that you can avoid CONFIG_VAL here, and say "if (...)" not "#if ..." here. > > > + > > > bootstage_mark_name(get_bootstage_id(false), "end phase"); > > > #ifdef CONFIG_BOOTSTAGE_STASH > > > ret = bootstage_stash((void *)CONFIG_BOOTSTAGE_STASH_ADDR, > > > diff --git a/include/asm-generic/global_data.h > > > b/include/asm-generic/global_data.h > > > index 8fc205ded1a..edf9ff6823f 100644 > > > --- a/include/asm-generic/global_data.h > > > +++ b/include/asm-generic/global_data.h > > > @@ -573,6 +573,13 @@ static_assert(sizeof(struct global_data) == GD_SIZE); > > > #define gd_malloc_start() 0 > > > #define gd_set_malloc_start(val) > > > #endif > > > + > > > +#if CONFIG_VAL(SYS_MALLOC_F_LEN) > > > +#define gd_malloc_ptr() gd->malloc_ptr > > > +#else > > > +#define gd_malloc_ptr() 0L > > > +#endif > > > + > > > /** > > > * enum gd_flags - global data flags > > > * > > > > This is another case where readability is not improved. I also have a > > bad feeling that changing that exact area had some unintended > > consequences from the compiler, that totally should not have happened. > > But maybe that was something in a similar code section instead. > > The improvement is in the C file...here we have an accessor in the > header file as has been done elsewhere. > > Do you have any more details on the problem you mention? I will align > the accessor to the struct member which should resolve it. It's unfortunately one of those cases to do a global before/after build and see if anything does or does not get tripped up. As I believe I did use CONFIG_VAL there and not a check on CONFIG_SYS_MALLOC_F itself for a reason, at the time. -- Tom
signature.asc
Description: PGP signature
