Hi Thierry, On 20 August 2014 00:36, Thierry Reding <thierry.red...@gmail.com> wrote: > On Tue, Aug 19, 2014 at 03:28:09PM -0600, Simon Glass wrote: >> Hi Thierry, >> >> On 19 August 2014 07:12, Thierry Reding <thierry.red...@gmail.com> wrote: >> > On Tue, Aug 19, 2014 at 06:55:18AM -0600, Simon Glass wrote: >> >> On 19 August 2014 05:35, Thierry Reding <thierry.red...@gmail.com> wrote: >> > [...] >> >> > The Linux kernel handles this by wrapping it in an of_read_number() >> >> > helper and always returning u64, like this: >> >> > >> >> > static inline u64 of_read_number(const __be32 *cell, int size) >> >> > { >> >> > u64 r = 0; >> >> > >> >> > while (size--) >> >> > r = (r << 32) | be32_to_cpu(*(cell++)); >> >> > >> >> > return r; >> >> > } >> >> > >> >> > It obviously only works for size in { 0, 1, 2 }, but I can add a helper >> >> > like that if you want. >> >> >> >> >> >> That looks good. It works for the cases we need and it's obvious later >> >> where the logic is if we want to extend it. >> > >> > This is what I have for U-Boot currently. >> > >> > static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int >> > cells) >> > { >> > fdt_addr_t addr = 0; >> > >> > while (cells--) >> > /* do the shift in two steps to avoid warning on 32-bit */ >> > addr = (addr << 16) << 16 | fdt32_to_cpu(*ptr++); >> >> Odd warning! Does 32UL help? > > The exact warning that I get is this: > > warning: left shift count >= width of type > > So the problem is that since addr is of type fdt_addr_t which is 32-bit, > we're effectively shifting all bits out of the variable. The compiler > will generate the same assembly code whether or not I use the single 32- > bit shift or two 16-bit shifts, but using the latter the warning is > gone. That's on both 32-bit and 64-bit ARM.
Well if fdt_addr_t is 32-bit then you are don't going to get a better result with your change - the warning is correct. So I think you need to have an #ifdef for the case where CONFIG_PHYS_64BIT is not defined and deal with it separate. Yes would could just move to 64-bit everwhere, but that will bloat the code I suspect (as we can always do it later anyway). > >> > Alternatively perhaps something like this could be done: >> > >> > static u64 fdtdec_get_number(const fdt32_t *ptr, unsigned int >> > cells) >> > { >> > /* the above */ >> > } >> > >> > static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned >> > int cells) >> > { >> > return fdtdec_get_number(ptr, cells); >> > } >> > >> > static fdt_size_t fdtdec_get_size(const fdt32_t *ptr, unsigned int >> > cells) >> > { >> > return fdtdec_get_number(ptr, cells); >> > } >> > >> > Again, I'm not sure it's really worth the trouble since fdt_addr_t and >> > fdt_size_t are both always either u32 or u64. >> >> Yes, although I suppose ultimately these might be 64-bit always, >> Perhaps we should merge the types? > > That's one other possibility. On Linux there's one common type for > these: > > typedef phys_addr_t resource_size_t; > > where phys_addr_t is defined as follows: > > #ifdef CONFIG_PHYS_ADDR_T_64BIT > typedef u64 phys_addr_t; > #else > typedef u32 phys_addr_t; > #endif > > Perhaps we should simply copy that. I take it CONFIG_PHYS_64BIT is > U-Boot's equivalent of CONFIG_PHYS_ADDR_T_64BIT? It doesn't seem to be > documented anywhere but its usage would indicate that it is. I don't > think U-Boot supports things like LPAE, so there's probably no need to > control this more fine-grainedly than with a single CONFIG_PHYS_64BIT > setting. U-Boot does have LPAE on x86 at least. I think using resource_size_t is a good approach. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot