Hi Abdellatif, On Mon, 10 Jul 2023 at 08:49, Abdellatif El Khlifi <[email protected]> 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 > > <[email protected]> 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 > > > > <[email protected]> wrote: > > > > > > > > > > sets the log formatting according to the platform (64-bit vs 32-bit) > > > > > > > > > > Signed-off-by: Abdellatif El Khlifi <[email protected]> > > > > > Cc: Simon Glass <[email protected]> > > > > > --- > > > > > 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
