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

[Alexander Graf]

On 04.02.2014, at 03:19, Scott Wood <address@hidden> wrote:

> On Fri, 2014-01-31 at 12:16 +0100, Alexander Graf wrote:
>> +void pci_init_board(void)
>> +{
>> +    struct fsl_pci_info pci_info;
>> +    const void *fdt = get_fdt();
>> +    int pci_node;
>> +
>> +    puts("\n");
>> +
>> +    pci_node = fdt_path_offset(fdt, "/pci");
>> +    if (pci_node < 0) {
>> +            printf("PCI: disabled\n\n");
>> +            return;
>> +    }
>> +
>> +    SET_STD_PCI_INFO(pci_info, 1);
>> +
>> +    fsl_setup_hose(&pci1_hose, pci_info.regs);
>> +    printf("PCI: 32 bit, 66 MHz, async, host, base address %lx\n",
>> +            pci_info.regs);
> 
> Why hardcode these things in a message?  Just don't print anything if
> you don't have the info.

To make it look more akin to a real e500 board. But I'll change it.

> Is QEMU's PCI emulation 32-bit-only?
> 
>> +void init_tlbs_dynamic(void)
>> +{
>> +    unsigned long fdt_tlb = (unsigned long)get_fdt() & ~0xffffful;
>> +    u32 mas0, mas1, mas2, mas3, mas7;
>> +    phys_size_t ram_size;
>> +
>> +    /*
>> +     * Create a temporary AS=1 map for the fdt
>> +     *
>> +     * We use ESEL=0 here to overwrite the previous AS=0 map for ourselves
>> +     * which was only 4k big. This way we don't have to clear any other 
>> maps.
>> +     */
>> +    mas0 = MAS0_TLBSEL(1) | MAS0_ESEL(0);
>> +    mas1 = MAS1_VALID | MAS1_TID(0) | MAS1_TS | MAS1_TSIZE(BOOKE_PAGESZ_1M);
>> +    mas2 = FSL_BOOKE_MAS2(fdt_tlb, 0);
>> +    mas3 = FSL_BOOKE_MAS3(fdt_tlb, 0, MAS3_SW|MAS3_SR|MAS3_SX);
>> +    mas7 = FSL_BOOKE_MAS7(fdt_tlb);
>> +
>> +    write_tlb(mas0, mas1, mas2, mas3, mas7);
> 
> How do you know you're not creating an overlapping TLB entry?  You
> should map this to a fixed virtual address that you know is safe.

Ok. I'll map it behind CCSRBAR.

> 
>> +    /* Create a dynamic AS=0 CCSRBAR mapping */
>> +    mas0 = MAS0_TLBSEL(1) | MAS0_ESEL(13);
>> +    mas1 = MAS1_VALID | MAS1_TID(0) | MAS1_TSIZE(BOOKE_PAGESZ_1M);
>> +    mas2 = FSL_BOOKE_MAS2(CONFIG_SYS_CCSRBAR, MAS2_I|MAS2_G);
>> +    mas3 = FSL_BOOKE_MAS3(CONFIG_SYS_CCSRBAR_PHYS, 0, MAS3_SW|MAS3_SR);
>> +    mas7 = FSL_BOOKE_MAS7(CONFIG_SYS_CCSRBAR_PHYS);
> 
> CCSRBAR may be 1M or 16M (assuming qemu-ppce500 sticks with a CCSR-ish
> layout at all).  Really we should be creating explicit maps for
> everything we find in the device tree that we care about.

I don't understand that part. We do create explicit maps for everything we care 
about, no?

> 
>> +#define CONFIG_SYS_CCSRBAR          0xe0000000
>> +#define CONFIG_SYS_CCSRBAR_PHYS_LOW CONFIG_SYS_CCSRBAR
> 
> Set CONFIG_SYS_CCSR_DO_NOT_RELOCATE instead of
> CONFIG_SYS_CCSRBAR_PHYS_LOW.

Ok.

> 
>> +/* Get RAM size from device tree */
>> +#define CONFIG_DDR_SPD
> 
> That's not what CONFIG_DDR_SPD means.

I've changed the #ifdef I care about to cover CONFIG_QEMU_E500 as well and 
removed this define now.


Alex