[U-Boot] [RFC][PATCH] fdt: Remove fdt_fixup_memory function

[Tom Rini]

On Wed, Apr 09, 2014 at 04:26:40PM +0200, Wolfgang Denk wrote:
> Dear Tom Rini,
> 
> In message <1397047800-26221-1-git-send-email-trini at ti.com> you wrote:
> > The fdt_fixup_memory function is only used on PowerPC where we only
> > claim one memory bank in U-Boot (and then in the device tree) so we can
> > call a function that just calls fdt_fixup_memory_banks(..., 1).  Call
> > this directly for consistency with other architectures.
> 
> I understand what you mean and what you want, but I'm not really
> happy about it.

Code or comment wise?

> First, the description is not correct.  In my understanding a "bank"
> of memory is some memory device which, on the hardware level, is
> addressed using one specific chip select signal.  On PPC, we usually
> have flexible memory controllers, so e can always map all existing
> memory banks such that they for a single, contiguous region.

Right, so we have a mismatch between function name
(fdt_fixup_memory_bank) and function of the node
(Documentation/devicetree/booting-without-of.txt in the kernel is,
sadly, the best description I can find of the memory node bindings).  We
itterate over the number of "banks" passed in (1 on PowerPC,
CONFIG_NR_DRAM_BANKS on ARM, which is between 1 and 4) and do, as the
binding expects, set the reg property correctly (base, size) for each
"bank".  It would be more correct to call this "ranges" rather than
"banks", or perhaps "nr_ranges".

> It is causing me some creepes to introduce code that claims it is
> fixing memory for only one bank - I feel this is wrong.
> 
> And is dropping the (u64) not a problem?  bd->bi_memstart is just an
> "unsigned long", but fdt_fixup_memory_banks() expects a u64 ?

Oops, I don't know how I missed that.  Or rather, what the hell is up
with calling fdt_fixup_memory() in two places on PowerPC?  I just
changed the call in board/freescale/t1040qds/t1040qds.c::ft_board_setup,
which uses phy_addr_t/phy_size_t on getenv_lowmem_... and this is fine, but
arch/powerpc/cpu/mpc85xx/fdt.c::ft_cpu_setup calls bd->bi_mem* and needs
a cast.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140409/a2b99876/attachment.pgp>