Re: [Qemu-ppc] [PATCH 15/15] ppc: Add aCube Sam460ex board

[David Gibson]

On Wed, Aug 23, 2017 at 02:47:42PM +0200, BALATON Zoltan wrote:
> On Wed, 23 Aug 2017, François Revol wrote:
> > Le 23/08/2017 à 13:12, BALATON Zoltan a écrit :
> > > > What's the connection with mips_malta?
> > > 
> > > The board's firmware wants to see SPD EEPROMs of the connected memory
> > > while initialising the memory controller. This is why we need to
> > > implement SDRAM controller, I2C and SPD EEPROMs. MIPS malta board had
> > > already SPD EEPROM implementation so this is based on that. The comment
> > > just indicates where this code comes from.
> > 
> > Indeed, and I copy-pasted from elsewhere for this.
> > 
> > > > > +        fprintf(stderr, "qemu: Error registering flash memory.\n");
> > > > 
> > > > Use error_report() instead, please.
> > 
> > I guess this didn't exist back when I started writing it...
> 
> No problem, I can take care of these.
> 
> > > > > +/* Create reset TLB entries for BookE, mapping only the flash
> > > > > memory.  */
> > > > > +static void mmubooke_create_initial_mapping_uboot(CPUPPCState *env)
> > > > > +{
> > > > > +    ppcemb_tlb_t *tlb = &env->tlb.tlbe[0];
> > > > > +
> > > > > +    /* on reset the flash is mapped by a shadow TLB,
> > > > > +     * but since we don't implement them we need to use
> > > > > +     * the same values U-Boot will use to avoid a fault.
> > > > > +     */
> > > > 
> > > > Usually the reset state of the MMU is handled in the cpu code rather
> > > > than the board code.  Is there a specific reason you need it in the
> > > > board code here?
> > > 
> > > I'm not sure, probably lack of a better place. The ppc440_bamboo board
> > > this is based on has it the same way in the board code. Maybe this could
> > > be cleaned up when someone wants to QOMify the SoC models sometimes.
> > 
> > Thing is, the code allows both booting with U-Boot and with a kernel
> > directly, and the MMU mapping differ in those cases.
> > 
> > Maybe the CPU reset should use the U-Boot setup and the kernel boot
> > would just overwrite it?
> > 
> > > > > +        env->nip = bi->entry;
> > > > > +
> > > > > +        /* Create a mapping for the kernel.  */
> > > > > +        mmubooke_create_initial_mapping(env, 0, 0);
> > > > > +        env->gpr[6] = tswap32(EPAPR_MAGIC);
> > > > 
> > > > I'm pretty sure the tswap can't be right here.  env->gpr is in host
> > > > native order and I'd expect the constant to be as well.
> > > 
> > > I know nothing about this, maybe Francois remembers why it's there. But
> > > booting linux with -kernel works so it's probably either correct or does
> > > not matter.
> > 
> > Absolutely no idea. It seems to be there from the first commit in my own
> > history here.
> > 
> > I don't recall testing booting linux at all though.
> > Linux does check the magic, so it'd be weird if it booted:
> > 
> > https://github.com/torvalds/linux/blob/master/arch/powerpc/boot/epapr.c
> 
> Is this code used on Sam460 at all? Is U-Boot ePAPR compliant
> firmware?

It can be, depends on the build, I think.

> Isn't that only needed on OpenFirmware?

No, not at all.  True OF will generally *not* be ePAPR compliant -
ePAPR was explicitly built as a much simpler interface that doesn't
require an OF implementation.  It uses some concepts from OF,
specifically the contents of the device tree, but other than that it's
not related.

> 
> > But maybe it got added later than the version you tested?
> 
> I've tried some of these:
> http://www.supertuxkart-amiga.de/amiga/sam.html#downloads
> which also have kernel 4.5 so that's fairly recent. These kernels are
> "u-boot legacy uImage" so maybe they don't need ePAPR magic? Are there some
> docs on what the kernel expects on this board or it has to be dug out from
> U-Boot?

If it's a legacy uImage I suspect it's not using ePAPR.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature