From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Date: Wed, 20 Aug 2014 08:36:01 +0200 Subject: [U-Boot] [PATCH 03/23] fdt: Add resource parsing functions In-Reply-To: References: <1408346196-30419-1-git-send-email-thierry.reding@gmail.com> <1408346196-30419-4-git-send-email-thierry.reding@gmail.com> <20140819113549.GC19515@ulmo> <20140819131201.GB1586@ulmo> Message-ID: <20140820063600.GF13793@ulmo> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, Aug 19, 2014 at 03:28:09PM -0600, Simon Glass wrote: > Hi Thierry, > > On 19 August 2014 07:12, Thierry Reding wrote: > > On Tue, Aug 19, 2014 at 06:55:18AM -0600, Simon Glass wrote: > >> On 19 August 2014 05:35, Thierry Reding 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. > > 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. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: