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

[Alexander Graf]

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

> On Thu, 2014-02-06 at 14:26 +0100, Alexander Graf wrote:
>> 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?
> 
> It would remove U-Boot complexity (unlike the LAW stuff where we just
> skip it) because we wouldn't need to care about QEMU's default settings.
> It should be easier to do than LAW support, and more useful (e.g. Linux
> currently programs this as well, we just get lucky that it misuses the
> device tree as configuration information).
> 
>>>> +{
>>>> +  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.
> 
> The more that line of thought is applied, the uglier the codebase we'll
> end up with. :-)
> 
>> There seems to be an fdt helper framework that's only targeted at a few ARM
>> devices - not sure what to make of that.
> 
> Make use of whatever parts you can, and extend it with the missing bits
> you need.  There's also common/fdt_support.c which is definitely not
> just used by ARM.
> 
>>> +   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 :).
> 
> It still indicates where the proper place for code like this is. :-)
> 
>>>> +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?
> 
> Yes.
> 
> And maybe fix that align == -2 bug while you're at it. :-)
> 
>>>> 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?
> 
> I guess you can't use that when you don't know the bus address, but
> still this is the wrong place to implement it.  If getting PCI range
> info from the device tree is something we want to do, then it should be
> a new common code helper.

The more I think about this the less of an idea I have how to do any generic 
API for this at all. And I'm not convinced anything generic is going to help 
anyone. Do a git grep on fdt_translate_address over the code base and you will 
find a _single_ caller.

So even if we come up with something now, nobody's going to use it.


Alex