| Message ID | 1436992006-32644-1-git-send-email-yorksun@freescale.com |
|---|---|
| State | RFC |
| Delegated to: | Simon Glass |
| Headers | show |
Hello York, On Wed, 15 Jul 2015 13:26:46 -0700, York Sun <yorksun@freescale.com> wrote: > fdt_addr_t and fdt_size_t can be either 64-bit or 32-bit, depending > on the architecture. Change the type to phys_addr_t and phys_size_t. If the only reason for changing the type is to avoid warnings in debug() then I don't think this is the right approach (see my message re PRIu64). If, on the other hand, there is reason to believe the fdt_addr_t and fdt_size_t are wrongly defined and should be typedef'd or ~define's as phys_*_t, then the rationale for this should be exposed. Amicalement,
Hi, On 16 July 2015 at 01:01, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Hello York, > > On Wed, 15 Jul 2015 13:26:46 -0700, York Sun <yorksun@freescale.com> > wrote: >> fdt_addr_t and fdt_size_t can be either 64-bit or 32-bit, depending >> on the architecture. Change the type to phys_addr_t and phys_size_t. > > If the only reason for changing the type is to avoid warnings in > debug() then I don't think this is the right approach (see my message > re PRIu64). > > If, on the other hand, there is reason to believe the fdt_addr_t and > fdt_size_t are wrongly defined and should be typedef'd or ~define's as > phys_*_t, then the rationale for this should be exposed. > > Amicalement, > -- > Albert. For LBA we do this in ide.h: #ifdef CONFIG_SYS_64BIT_LBA typedef uint64_t lbaint_t; #define LBAF "%llx" #define LBAFU "%llu" #else typedef ulong lbaint_t; #define LBAF "%lx" #define LBAFU "%lu" #endif How about something similar for this case? It would be nice to avoid promoting everything to 64-bit given that very few platforms are actually 64-bit. Regards, Simon
On 07/16/2015 09:40 AM, Simon Glass wrote: > Hi, > > On 16 July 2015 at 01:01, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: >> Hello York, >> >> On Wed, 15 Jul 2015 13:26:46 -0700, York Sun <yorksun@freescale.com> >> wrote: >>> fdt_addr_t and fdt_size_t can be either 64-bit or 32-bit, depending >>> on the architecture. Change the type to phys_addr_t and phys_size_t. >> >> If the only reason for changing the type is to avoid warnings in >> debug() then I don't think this is the right approach (see my message >> re PRIu64). >> >> If, on the other hand, there is reason to believe the fdt_addr_t and >> fdt_size_t are wrongly defined and should be typedef'd or ~define's as >> phys_*_t, then the rationale for this should be exposed. >> >> Amicalement, >> -- >> Albert. > > For LBA we do this in ide.h: > > #ifdef CONFIG_SYS_64BIT_LBA > typedef uint64_t lbaint_t; > #define LBAF "%llx" > #define LBAFU "%llu" > #else > typedef ulong lbaint_t; > #define LBAF "%lx" > #define LBAFU "%lu" > #endif > > How about something similar for this case? It would be nice to avoid > promoting everything to 64-bit given that very few platforms are > actually 64-bit. > Simon, Do you think fdt_addr_t should be phys_addr_t? According to the comments you put in file, "A typedef for a physical address". If yes, I can change the commit message. On the other hand, if we want to keep the current typedef, we can use Albert's PRIu64 technique, we then don't have to define the format string. York
Hi York, On 16 July 2015 at 11:00, York Sun <yorksun@freescale.com> wrote: > > > On 07/16/2015 09:40 AM, Simon Glass wrote: >> Hi, >> >> On 16 July 2015 at 01:01, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: >>> Hello York, >>> >>> On Wed, 15 Jul 2015 13:26:46 -0700, York Sun <yorksun@freescale.com> >>> wrote: >>>> fdt_addr_t and fdt_size_t can be either 64-bit or 32-bit, depending >>>> on the architecture. Change the type to phys_addr_t and phys_size_t. >>> >>> If the only reason for changing the type is to avoid warnings in >>> debug() then I don't think this is the right approach (see my message >>> re PRIu64). >>> >>> If, on the other hand, there is reason to believe the fdt_addr_t and >>> fdt_size_t are wrongly defined and should be typedef'd or ~define's as >>> phys_*_t, then the rationale for this should be exposed. >>> >>> Amicalement, >>> -- >>> Albert. >> >> For LBA we do this in ide.h: >> >> #ifdef CONFIG_SYS_64BIT_LBA >> typedef uint64_t lbaint_t; >> #define LBAF "%llx" >> #define LBAFU "%llu" >> #else >> typedef ulong lbaint_t; >> #define LBAF "%lx" >> #define LBAFU "%lu" >> #endif >> >> How about something similar for this case? It would be nice to avoid >> promoting everything to 64-bit given that very few platforms are >> actually 64-bit. >> > > Simon, > > Do you think fdt_addr_t should be phys_addr_t? According to the comments you put > in file, "A typedef for a physical address". If yes, I can change the commit > message. Sounds good to me. > > On the other hand, if we want to keep the current typedef, we can use Albert's > PRIu64 technique, we then don't have to define the format string. The only reason for a difference typedef is the big endian nature of FDT data. So I think your patch is fine on that front. Regards, Simon
diff --git a/include/fdtdec.h b/include/fdtdec.h index 2323603..c61734c 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -14,6 +14,7 @@ * changes to support FDT are minimized. */ +#include <linux/types.h> #include <libfdt.h> #include <pci.h> @@ -21,15 +22,13 @@ * A typedef for a physical address. Note that fdt data is always big * endian even on a litle endian machine. */ +typedef phys_addr_t fdt_addr_t; +typedef phys_size_t fdt_size_t; #ifdef CONFIG_PHYS_64BIT -typedef u64 fdt_addr_t; -typedef u64 fdt_size_t; #define FDT_ADDR_T_NONE (-1ULL) #define fdt_addr_to_cpu(reg) be64_to_cpu(reg) #define fdt_size_to_cpu(reg) be64_to_cpu(reg) #else -typedef u32 fdt_addr_t; -typedef u32 fdt_size_t; #define FDT_ADDR_T_NONE (-1U) #define fdt_addr_to_cpu(reg) be32_to_cpu(reg) #define fdt_size_to_cpu(reg) be32_to_cpu(reg) diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 9877849..1f22fab 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -102,8 +102,8 @@ fdt_addr_t fdtdec_get_addr_size(const void *blob, int node, size = (fdt_size_t *)((char *)cell + sizeof(fdt_addr_t)); *sizep = fdt_size_to_cpu(*size); - debug("addr=%08lx, size=%08x\n", - (ulong)addr, *sizep); + debug("addr=%pa, size=%zx\n", + &addr, *sizep); } else { debug("%08lx\n", (ulong)addr); }
fdt_addr_t and fdt_size_t can be either 64-bit or 32-bit, depending on the architecture. Change the type to phys_addr_t and phys_size_t. Signed-off-by: York Sun <yorksun@freescale.com> --- include/fdtdec.h | 7 +++---- lib/fdtdec.c | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-)