Hi Sergei,

On 23 June 2014 22:11, Sergei wrote:

On 06/23/2014 08:44 PM, Phil Edworthy wrote:

quoted

quoted

     I'm investigating an imprecise external abort occurring once userland is
started when I have NetMos

    Or is it MosChip now? Can't remember all their renames. :-)

Do you know of somewhere I can buy a card with this chipset in the EU? I had a
quick search but couldn't find anything.

[...]

quoted

quoted

quoted

+static void pci_write_reg(struct rcar_pcie *pcie, unsigned long val,
+			  unsigned long reg)
+{
+	writel(val, pcie->base + reg);
+}
+
+static unsigned long pci_read_reg(struct rcar_pcie *pcie, unsigned long

reg)

quoted

+{
+	return readl(pcie->base + reg);
+}

quoted

quoted

     As a side note, these functions are hardly needed, and risk collision

too...

quoted

Ben mentioned this in his review and as I said then, I found them useful

during

quoted

development, so we agreed to leave them. Since they are static, there

shouldn't

quoted

be a collision risk.

    You're risking clashes even at the source level, not even at object file
level...

Ah, yes you are correct. 

quoted

quoted

quoted

+static void rcar_pcie_setup_window(int win, struct resource *res,
+				   struct rcar_pcie *pcie)

quoted

quoted

     As a side note, 'res' parameter is hardly needed here, as the function
always gets
called with the resources contained within 'struct rcar_pcie'...

quoted

Either I would have to pass an index to the resource in,

    But you already do pass it, 'win' is the index!

quoted

or as I have done, a
pointer to the individual resource. I found it cleaner to pass the pointer.

    You're actually pass excess parameters, both the index and the pointer.

Ha, yes I didn't notice that :)

[...]

quoted

quoted

quoted

+
+	/* First resource is for IO */
+	mask = PAR_ENABLE;
+	if (res->flags & IORESOURCE_IO)
+		mask |= IO_SPACE;

quoted

quoted

     For the memory space this works OK as you're identity-mapping the
memory
ranges in your device trees. However, for the I/O space this means that it
won't work as the BARs in the PCIe devices get programmed with the PCI

bus

quoted

quoted

addresses but the PCIe window translation register is programmed with a
CPU
address which don't at all match (given your device trees) and hence one
can't
access the card's I/O mapped registers at all...

quoted

Hmm, I couldn't find any cards that supported I/O, so I wasn't able to test
this. Clearly this is an issue that needs looking into.

    Will you look into it then, or should I?

I'll look at it.

quoted

quoted

quoted

+
+	pci_write_reg(pcie, mask, PCIEPTCTLR(win));
+}
+
+static int rcar_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+	struct rcar_pcie *pcie = sys_to_pcie(sys);
+	struct resource *res;
+	int i;
+
+	pcie->root_bus_nr = -1;
+
+	/* Setup PCI resources */
+	for (i = 0; i < PCI_MAX_RESOURCES; i++) {
+
+		res = &pcie->res[i];
+		if (!res->flags)
+			continue;
+
+		rcar_pcie_setup_window(i, res, pcie);
+
+		if (res->flags & IORESOURCE_IO)
+			pci_ioremap_io(nr * SZ_64K, res->start);

quoted

quoted

    I'm not sure why are you not calling pci_add_resource() for I/O space...

    Sorry, did you reply to that?

I used the tegra driver to inform on what I should do for this. This doesn't
call pci_add_resource() for I/O space either. However, I also see that other
drivers do call this. I think the simplest thing is for me to get a card that
supports I/O space and properly test it.

quoted

quoted

Also, this sets up only 64 KiB of I/O ports while your device tree describes
I/O space 1 MiB is size.

quoted

This driver should be able to cope with multiple host controllers, so each
allocated 64KiB for I/O. 64KiB is all you need for I/O, but the R-Car PCIe
hardware has a 1MiB region (the smallest one) that can only be used for

one

quoted

type of PCIe access.

[...]

quoted

quoted

quoted

+
+	/* Finish initialization - establish a PCI Express link */
+	pci_write_reg(pcie, CFINIT, PCIETCTLR);
+
+	/* This will timeout if we don't have a link. */
+	err = rcar_pcie_wait_for_dl(pcie);
+	if (err)
+		return err;
+
+	/* Enable INTx interrupts */
+	rcar_rmw32(pcie, PCIEINTXR, 0, 0xF << 8);
+
+	/* Enable slave Bus Mastering */
+	rcar_rmw32(pcie, RCONF(PCI_STATUS), PCI_STATUS_DEVSEL_MASK,
+		PCI_COMMAND_IO | PCI_COMMAND_MEMORY |

PCI_COMMAND_MASTER |

quoted

+		PCI_STATUS_CAP_LIST | PCI_STATUS_DEVSEL_FAST);

quoted

quoted

     Hmm, you're mixing up PCI control/status registers' bits here; they're
two 16-bit registers! So you're writing to 3 reserved LSBs of the PCI status
register...

    ... and therefore not writing these bits to the PCI command (not control,
sorry) register. Perhaps because of that PCI-PCI bridge remains inactive...

quoted

The mask arg should make sure that we don't write to reserved bits.

However,

    Look at rcar_rmw32() again -- it doesn't really do that.

Yeah, that's why I said it should, not does. It only clears those bits in the
register's current value.

[...]

quoted

quoted

quoted

+static int rcar_pcie_probe(struct platform_device *pdev)
+{
+	struct rcar_pcie *pcie;
+	unsigned int data;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	const struct of_device_id *of_id;
+	int err, win = 0;
+	int (*hw_init_fn)(struct rcar_pcie *);
+
+	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->dev = &pdev->dev;
+	platform_set_drvdata(pdev, pcie);
+
+	/* Get the bus range */
+	if (of_pci_parse_bus_range(pdev->dev.of_node, &pcie->busn)) {
+		dev_err(&pdev->dev, "failed to parse bus-range

property\n");

quoted

+		return -EINVAL;
+	}
+
+	if (of_pci_range_parser_init(&parser, pdev->dev.of_node)) {
+		dev_err(&pdev->dev, "missing ranges property\n");
+		return -EINVAL;
+	}
+
+	err = rcar_pcie_get_resources(pdev, pcie);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to request resources: %d\n",

err);

quoted

+		return err;
+	}
+
+	for_each_of_pci_range(&parser, &range) {
+		of_pci_range_to_resource(&range, pdev->dev.of_node,
+						&pcie->res[win++]);

quoted

quoted

     This function call is probably no good here as it fetches into the 'start'
field of a 'struct resource' a CPU address instead of a PCI address...

quoted

No, the ranges describe the CPU addresses of the PCI memory and I/O

regions, so

quoted

this is correct.

    The problem actually is that you need to remember both CPU and PCI
addresses, so 'struct of_pci_range' looks more fitting here...

Right, I see... this is rather a mess with all the host drivers!

quoted

quoted

quoted

+
+		if (win > PCI_MAX_RESOURCES)
+			break;
+	}
+
+	 err = rcar_pcie_parse_map_dma_ranges(pcie, pdev->dev.of_node);
+	 if (err)
+		return err;
+
+	of_id = of_match_device(rcar_pcie_of_match, pcie->dev);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+	hw_init_fn = of_id->data;
+
+	/* Failure to get a link might just be that no cards are inserted */
+	err = hw_init_fn(pcie);
+	if (err) {
+		dev_info(&pdev->dev, "PCIe link down\n");
+		return 0;

quoted

quoted

     Not quite sure why you exit normally here without enabling the

hardware.

quoted

quoted

I think the internal bridge should be visible regardless of whether link is
detected or not...

quoted

Why would you want to see the bridge when you can do nothing with it?

Aren't

    Because it's the way PCI works. You have the built-in devices always
present and seen on a PCI bus. :-)

quoted

you are just wasting resources?

    I think it's rather you who are wasting resources. ;-) Why not just fail
the probe when you have no link?

Ah, so we currently have a half-way house... not failing the probe, but not
enabling the HW. Either all or nothing would be preferable.

Thanks
Phil