Re: [Qemu-ppc] [PATCH v2 5/6] PPC 85xx: Find PCI host controllers on ppce500 from device tree

[Scott Wood]

On Fri, 2014-01-31 at 12:16 +0100, Alexander Graf wrote:
> The definition of our ppce500 PV machine is that every address is dynamically
> determined through device tree bindings.
> 
> So don't hardcode where PCI devices are in our physical memory layout but 
> instead
> read them dynamically from the device tree we get passed on boot.

Would it be difficult to make the QEMU emulation properly implement
access windows?

> Signed-off-by: Alexander Graf <address@hidden>
> ---
>  board/freescale/qemu-ppce500/qemu-ppce500.c |  193 
> ++++++++++++++++++++++++---
>  board/freescale/qemu-ppce500/tlb.c          |   13 --
>  include/configs/qemu-ppce500.h              |   12 --
>  3 files changed, 175 insertions(+), 43 deletions(-)
> 
> diff --git a/board/freescale/qemu-ppce500/qemu-ppce500.c 
> b/board/freescale/qemu-ppce500/qemu-ppce500.c
> index 6491ae9..5d4dd64 100644
> --- a/board/freescale/qemu-ppce500/qemu-ppce500.c
> +++ b/board/freescale/qemu-ppce500/qemu-ppce500.c
> @@ -19,7 +19,51 @@
>  #include <malloc.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
> -static struct pci_controller pci1_hose;
> +
> +static uint64_t myfdt_readcells(const void *fdt, int node, const char 
> *property,
> +                             int num, int off, uint64_t defval)

"my" fdt?

> +{
> +     int len;
> +     const uint32_t *prop;
> +
> +     prop = fdt_getprop(fdt, node, property, &len);
> +
> +     if (!prop)
> +             return defval;
> +
> +     if (len < ((off + num) * sizeof(uint32_t)))
> +             panic("Invalid fdt");
> +
> +     prop += off;
> +
> +     switch (num) {
> +     case 1:
> +             return *prop;
> +     case 2:
> +             return *(const uint64_t *)prop;
> +     }
> +

What about this function is specific to qemu-ppce500?

+       panic("Invalid cell size");
+}

s/cell size/cell count/

> +static uint32_t myfdt_one_cell(const void *fdt, int node, const char 
> *property,
> +                            uint32_t defval)
> +{
> +     return myfdt_readcells(fdt, node, property, 1, 0, defval);
> +}

This looks a lot like fdt_getprop_u32_default(), except for "int node"
versus path.

> +static void map_tlb1_io(ulong virt_addr, uint64_t phys_addr, uint64_t size)
> +{
> +     unsigned int max_cam, tsize_mask;
> +     int i;
> +
> +     if ((mfspr(SPRN_MMUCFG) & MMUCFG_MAVN) == MMUCFG_MAVN_V1) {
> +             /* Convert (4^max) kB to (2^max) bytes */
> +             max_cam = ((mfspr(SPRN_TLB1CFG) >> 16) & 0xf) * 2 + 10;
> +             tsize_mask = ~1U;
> +     } else {
> +             /* Convert (2^max) kB to (2^max) bytes */
> +             max_cam = __ilog2(mfspr(SPRN_TLB1PS)) + 10;
> +             tsize_mask = ~0U;
> +     }
> +
> +     for (i = 0; size && i < 8; i++) {
> +             int tlb_index = find_free_tlbcam();
> +             u32 camsize = __ilog2_u64(size) & tsize_mask;
> +             u32 align = __ilog2(virt_addr) & tsize_mask;
> +             unsigned int tlb_size;
> +
> +             if (tlb_index == -1)
> +                     break;
> +
> +             if (align == -2) align = max_cam;

-2?  Besides align being unsigned, if this is meant to handle the case
where virt_addr is zero, that's undefined for __ilog2() (don't rely on
it being implemented with cntlzw), and you're not handling the MMUv2
case.

Plus, whitespace.

> +             if (camsize > align)
> +                     camsize = align;
> +
> +             if (camsize > max_cam)
> +                     camsize = max_cam;

What about min_cam?

> +     }
> +
> +}

Whitespace.

> +
>  void pci_init_board(void)
>  {
> -     struct fsl_pci_info pci_info;
> +     struct pci_controller *pci_hoses;
>       const void *fdt = get_fdt();
>       int pci_node;
> +     int pci_num = 0;
> +     int pci_count;
> +     const char *compat = "fsl,mpc8540-pci";
> +     ulong map_addr;
>  
>       puts("\n");
>  
> -     pci_node = fdt_path_offset(fdt, "/pci");
> -     if (pci_node < 0) {
> +     /* Start MMIO and PIO range maps above RAM */
> +     map_addr = CONFIG_MAX_MEM_MAPPED;
> +
> +     /* Count and allocate PCI buses */
> +     pci_count = myfdt_count_compatibles(fdt, compat);
> +
> +     if (pci_count) {
> +             pci_hoses = malloc(sizeof(struct pci_controller) * pci_count);
> +     } else {
>               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);
> -
> -     fsl_pci_init_port(&pci_info, &pci1_hose, 0);
> +     /* Spawn PCI buses based on device tree */
> +     pci_node = fdt_node_offset_by_compatible(fdt, -1, compat);
> +     while (pci_node != -FDT_ERR_NOTFOUND) {
> +             struct fsl_pci_info pci_info = { };
> +             uint64_t phys_addr;
> +             int pnode = fdt_parent_offset(fdt, pci_node);
> +             int paddress_cells;
> +             int address_cells;
> +             int size_cells;
> +             int off = 0;
> +
> +             paddress_cells = myfdt_one_cell(fdt, pnode, "#address-cells", 
> 1);
> +             address_cells = myfdt_one_cell(fdt, pci_node, "#address-cells", 
> 1);
> +             size_cells = myfdt_one_cell(fdt, pci_node, "#size-cells", 1);
> +
> +             pci_info.regs = myfdt_readcells(fdt, pci_node, "reg",
> +                                             paddress_cells, 0, 0);
> +
> +             /* MMIO range */
> +             off += address_cells;
> +             phys_addr = myfdt_readcells(fdt, pci_node, "ranges",
> +                                         paddress_cells, off, 0);
> +             off += paddress_cells;
> +             pci_info.mem_size = myfdt_readcells(fdt, pci_node, "ranges",
> +                     size_cells, off, 0);
> +             off += size_cells;
> +
> +             /* Align virtual region */
> +             map_addr += pci_info.mem_size - 1;
> +             map_addr &= ~(pci_info.mem_size - 1);
> +             /* Map virtual memory for MMIO range */
> +             map_tlb1_io(map_addr, phys_addr, pci_info.mem_size);
> +             pci_info.mem_bus = phys_addr;
> +             pci_info.mem_phys = phys_addr;
> +             map_addr += pci_info.mem_size;
> +
> +             /* PIO range */
> +             off += address_cells;
> +             pci_info.io_phys = myfdt_readcells(fdt, pci_node, "ranges",
> +                     paddress_cells, off, 0);
> +             off += paddress_cells;
> +             pci_info.io_size = myfdt_readcells(fdt, pci_node, "ranges",
> +                     size_cells, off, 0);
> +
> +             /* Align virtual region */
> +             map_addr += pci_info.io_size - 1;
> +             map_addr &= ~(pci_info.io_size - 1);
> +             /* Map virtual memory for MMIO range */
> +             map_tlb1_io(map_addr, pci_info.io_phys, pci_info.io_size);
> +             pci_info.io_bus = map_addr;
> +             map_addr += pci_info.io_size;
> +
> +             /* Instantiate */
> +             pci_info.pci_num = pci_num + 1;
> +
> +             fsl_setup_hose(&pci_hoses[pci_num], pci_info.regs);
> +             printf("PCI: 32 bit, 66 MHz, async, host, base address %lx\n",
> +                     pci_info.regs);
> +
> +             fsl_pci_init_port(&pci_info, &pci_hoses[pci_num], pci_num);
> +
> +             /* Jump to next PCI node */
> +             pci_node = fdt_node_offset_by_compatible(fdt, pci_node, compat);
> +             pci_num++;
> +     }
>  
>       puts("\n");
>  }

How about using fdt_translate_address() or other existing functionality?

-Scott