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

[Scott Wood]

On Thu, 2014-02-06 at 14:26 +0100, Alexander Graf wrote:
> On 04.02.2014, at 03:47, Scott Wood <scottwood at freescale.com> 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.

-Scott