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

[Alexander Graf]

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

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

What are access windows? You mean BAR region offsets? Not too hard I suppose, 
but it adds complexity which we were trying to avoid, no?

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

Yeah, didn't want to clash with the fdt namespace.

> 
>> +{
>> +    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?

Nothing. But the less common code I touch the less I can break. There seems to 
be an fdt helper framework that's only targeted at a few ARM devices - not sure 
what to make of that.

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

Well, I use it inside of a loop where I don't have the 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.

I merely copied this from tlb.c's setup_ddr_tlbs_phys() and adjusted it 
slightly to let me choose the target virt address.

Would you prefer if I generalize setup_ddr_tlbs_phys() inside tlb.c and export 
that function there?

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

Mind to show exactly how?


Alex