Re: [Qemu-ppc] [PATCH 3/3] PPC 85xx: Add qemu-ppce500 machine

[Scott Wood]

On Fri, 2014-01-24 at 15:19 +0100, Alexander Graf wrote:
> On 24.01.2014, at 02:39, Scott Wood <address@hidden> wrote:
> 
> > On Fri, 2014-01-24 at 02:25 +0100, Alexander Graf wrote:
> >> On 24.01.2014, at 01:49, Scott Wood <address@hidden> wrote:
> >> 
> >>> On Thu, 2014-01-23 at 14:08 +0100, Alexander Graf wrote:
> >>>> which is the only place where we actually use $ramdisk_addr. If a user
> >>>> wants to specify the initrd within U-Boot he can do that easily because
> >>>> he would specify his own boot command anyway, right?
> >>> 
> >>> Oh.  I was confusing it with the existing $ramdiskaddr.  So I'm changing
> >>> my objection to the fact that it's confusing. :-)
> >>> 
> >>> Likewise $fdt_addr versus $fdtaddr.
> >> 
> >> Mind to explain what the difference is between $fdtaddr, $fdt_addr (pxe 
> >> boot?) and $fdt_addr_r? Which one should I set?
> > 
> > I don't know what the "_r" is supposed to mean, but the underscore seems
> > to just be something that varies from board to board.  Our boards use
> > $fdtaddr and until now I didn't realize other boards do it differently.
> 
> Bah - I'll just set all of them.

Ugh, no.

It looks like these are documented in README (and our use of fdtaddr is
nonstandard, though the README does state that they're just
suggestions).  fdt_addr is supposed to be the address in flash, and
fdt_addr_r is supposed to be the address in RAM.
 
> > What sort of messing would you need to do to cpu_init_early_f()?  What
> > part of the normal workflow breaks under QEMU?
> 
> #ifndef CONFIG_FSL_CORENET
> #if (defined(CONFIG_SYS_RAMBOOT) || defined(CONFIG_SPL)) && \
>         !defined(CONFIG_SYS_INIT_L2_ADDR)
> phys_size_t initdram(int board_type)
> {
> #if defined(CONFIG_SPD_EEPROM) || defined(CONFIG_DDR_SPD)
>         return fsl_ddr_sdram_size();
> #else
>         return (phys_size_t)CONFIG_SYS_SDRAM_SIZE * 1024 * 1024;
> #endif
> }
> #else /* CONFIG_SYS_RAMBOOT */
> phys_size_t initdram(int board_type)
> {
> [...]
>         dram_size = setup_ddr_tlbs(dram_size / 0x100000);
> [...]
> 
> So because we set CONFIG_SYS_RAMBOOT we never call setup_ddr_tlbs() but
> instead have to do it ourselves in fsl_ddr_sdram_size(). I'll move the
> TLB fixup there.

That looks like a bug in RAMBOOT handling, not specific to the QEMU
target.

You shouldn't be calling fsl_ddr_sdram_size() at all -- there's no
emulated SPD EEPROM.

For that matter, I don't see much in cpu.c that we really want to use
for the QEMU target.

-Scott