[PATCH v14 05/11] log: select physical address formatting in a generic way

[Simon Glass]

Hi Abdellatif,

On Mon, 10 Jul 2023 at 08:49, Abdellatif El Khlifi
<abdellatif.elkhlifi at arm.com> wrote:
>
> Hi Simon,
>
> On Mon, Jul 10, 2023 at 08:17:41AM -0600, Simon Glass wrote:
> > Hi Abdellatif,
> >
> > On Mon, 10 Jul 2023 at 06:15, Abdellatif El Khlifi
> > <abdellatif.elkhlifi at arm.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Fri, Jul 07, 2023 at 06:34:14PM +0100, Simon Glass wrote:
> > > > Hi Abdellatif,
> > > >
> > > > On Fri, 7 Jul 2023 at 15:44, Abdellatif El Khlifi
> > > > <abdellatif.elkhlifi at arm.com> wrote:
> > > > >
> > > > > sets the log formatting according to the platform (64-bit vs 32-bit)
> > > > >
> > > > > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi at arm.com>
> > > > > Cc: Simon Glass <sjg at chromium.org>
> > > > > ---
> > > > >  include/log.h | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/include/log.h b/include/log.h
> > > > > index 3bab40b617..689cef905b 100644
> > > > > --- a/include/log.h
> > > > > +++ b/include/log.h
> > > > > @@ -686,4 +686,12 @@ static inline int log_get_default_format(void)
> > > > >                (IS_ENABLED(CONFIG_LOGF_FUNC) ? BIT(LOGF_FUNC) : 0);
> > > > >  }
> > > > >
> > > > > +/* Select the right physical address formatting according to the platform */
> > > > > +#ifdef CONFIG_PHYS_64BIT
> > > > > +#define PhysAddrLength "ll"
> > > > > +#else
> > > > > +#define PhysAddrLength ""
> > > >
> > > > Shouldn't this be "l" ? We normally use ulong for addresses.
> > > >
> > >
> > > The "ll" is needed by many architectures who declare "phys_addr_t" as "unsigned long long"
> > >
> > > Example of architetures doing that: arm, powerpc, ...
> > >
> > > Also mips and sandbox use "u64" for "phys_addr_t" , ( "u64" is an "unsigned long long").
> >
> > Yes, the ll looks right. I was referring to the "" line.
> >
> > >
> > > Note: When using an "l" for arm, the compiler shows this warning:
> > >
> > > cmd/armffa.c:174:18: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘phys_addr_t’ {aka ‘long long unsign
> > > ed int’} [8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat=-Wformat=8;;]
> > >   174 |         log_info("device %s, addr " PHYS_ADDR_LN ", driver %s, ops " PHYS_ADDR_LN "\n",
> > >       |                  ^~~~~~~~~~~~~~~~~~
> > >   175 |                  dev->name,
> > >   176 |                  map_to_sysmem(dev),
> > >       |                  ~~~~~~~~~~~~~~~~~~
> > >       |                  |
> > >       |                  phys_addr_t {aka long long unsigned int}
> >
> > Hmm I was hoping that we could use ll for 64-bit and l for 32-bit and
> > that this could be a generic header for all archs.
>
> The "" was there for 32-bit architecures (e.g: sandbox, mips) who declare "phys_addr_t" as u32 or "unsigned int"
>
> For example, when using sandbox  with "l" in place of "", there is a compiler warning:
>
> cmd/armffa.c:174:11: warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 8 has type ‘phys_addr_t’ {aka ‘unsigned int’} -
> Wformat=]
>   174 |  log_info("device %s, addr " PHYS_ADDR_LN ", driver %s, ops " PHYS_ADDR_LN "\n",
>
> >
> > Can we get your series in as is and deal with this separately. I was
> > intending for this to be a follow-up patch.
>
> Sounds good to me. I'll move this commit out of the FF-A patchset and define PHYS_ADDR_LN in armffa.c to be used only there.
>
> Is that ok ?

It's fine with me.

Regards,
Simon