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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3578 bytes --]

On Thu, Aug 24, 2017 at 11:43:18PM +0200, BALATON Zoltan wrote:
> On Thu, 24 Aug 2017, David Gibson wrote:
> > On Wed, Aug 23, 2017 at 01:43:56PM +0200, 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...
> > 
> > Probably not.
> > 
> > > > > > +/* 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?
> > 
> > Yes.  Basically the CPU reset should do what real hardware does - and
> > I'd expect u-boot to be written to work with that.  The kernel, on the
> > other hand will expect whatever mappings come out of u-boot (or
> > another bootloader).  Long term, I think you want to always use the
> > hardware setup and actually run the guest firmware to set up the
> > mappings the kernel expects.
> 
> Maybe we should emulate the nvram for that and then we could patch it to
> have it boot the kernel with the parameters specified in the command line?
> This would also be needed to avoid the warning from u-boot about bad
> checksum and always going with the built in defaults and would also allow
> users to store settings like on real hardware but it's not something that
> looks high priority to me at the moment.

That all sounds reasonable.

> >  For early development where it's useful
> > to be able to boot into a kernel without firmware, faking the right
> > mappings in the board code is perfectly reasonable.
> 
> So for now I'd just go with this, because there's much more to clean up and
> improve before this becomes and issue I think.

Yup, makes sense.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]