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

[Alexander Graf]

On 06.02.2014, at 23:55, Scott Wood <address@hidden> wrote:

> On Thu, 2014-02-06 at 13:48 +0100, Alexander Graf wrote:
>> 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.
> 
> It's a paravirt target...

So? It should still look "normal" to someone who runs it.

> 
>>>> +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.
> 
> I'd give it its own CONFIG symbol to be explicitly located in the header
> file.

It's only a temporary map that is alive for a few dozen instructions. I think a 
new CONFIG symbol is excessive here :).

> 
>>>> +  /* 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?
> 
> No, you map CCSR as a whole, rather than the UART, PCI controller regs,
> etc.  If you want to that, ideally the size of CCSR should come from the
> device tree as well as the address.

Ah, ok. I can certainly try and pull the size out of the device tree.


Alex