On Sun, 15 Aug 2021 13:33:14 +0100,
"Sven Peter" [off-list ref] wrote:

On Sun, Aug 15, 2021, at 09:42, Marc Zyngier wrote:

quoted

On Sun, 15 Aug 2021 05:25:25 +0100,
Alyssa Rosenzweig [off-list ref] wrote:

[...]

quoted

quoted

+
+static int apple_pcie_setup_port(struct apple_pcie *pcie, unsigned int i)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(pcie->dev);
+	void __iomem *port;
+	struct gpio_desc *reset;
+	uint32_t stat;
+	int ret;
+
+	port = devm_of_iomap(pcie->dev, to_of_node(fwnode), i + 3, NULL);
+
+	if (IS_ERR(port))
+		return -ENODEV;
+
+	reset = devm_gpiod_get_index(pcie->dev, "reset", i, 0);
+	if (IS_ERR(reset))
+		return PTR_ERR(reset);
+
+	gpiod_direction_output(reset, 0);
+
+	rmwl(0, PORT_APPCLK_EN, port + PORT_APPCLK);
+
+	ret = apple_pcie_setup_refclk(pcie->rc, port, i);
+	if (ret < 0)
+		return ret;
+
+	rmwl(0, PORT_PERST_OFF, port + PORT_PERST);
+	gpiod_set_value(reset, 1);
+
+	ret = readl_poll_timeout(port + PORT_STATUS, stat,
+				 stat & PORT_STATUS_READY, 100, 250000);
+	if (ret < 0) {
+		dev_err(pcie->dev, "port %u ready wait timeout\n", i);
+		return ret;
+	}
+
+	rmwl(PORT_REFCLK_CGDIS, 0, port + PORT_REFCLK);
+	rmwl(PORT_APPCLK_CGDIS, 0, port + PORT_APPCLK);
+
+	ret = readl_poll_timeout(port + PORT_LINKSTS, stat,
+				 !(stat & PORT_LINKSTS_BUSY), 100, 250000);
+	if (ret < 0) {
+		dev_err(pcie->dev, "port %u link not busy timeout\n", i);
+		return ret;
+	}
+
+	writel(0xfb512fff, port + PORT_INTMSKSET);

Magic. What is this for?

The magic comes from the original Corellium driver. It first masks everything
except for the interrupts in the next line, then acks the interrupts it keeps
enabled and then probably wants to wait for PORT_INT_LINK_UP (or any of the
other interrupts which seem to indicate various error conditions) to fire but
instead polls for PORT_LINKSTS_UP.

quoted

quoted

+
+	writel(PORT_INT_LINK_UP | PORT_INT_LINK_DOWN | PORT_INT_AF_TIMEOUT |
+	       PORT_INT_REQADDR_GT32 | PORT_INT_MSI_ERR |
+	       PORT_INT_MSI_BAD_DATA | PORT_INT_CPL_ABORT |
+	       PORT_INT_CPL_TIMEOUT | (1 << 26), port + PORT_INTSTAT);
+
+	usleep_range(5000, 10000);
+
+	rmwl(0, PORT_LTSSMCTL_START, port + PORT_LTSSMCTL);
+
+	ret = readl_poll_timeout(port + PORT_LINKSTS, stat,
+				 stat & PORT_LINKSTS_UP, 100, 500000);
+	if (ret < 0) {
+		dev_err(pcie->dev, "port %u link up wait timeout\n", i);
+		return ret;
+	}

I have the strong feeling that a lot of things in the above is to get
an interrupt when the port reports an event. Why the polling then?

I'm pretty sure this is true. The same registers are also used to setup
and handle legacy interrupts.

My current understanding is that PORT_INTSTAT is used to retrieve the fired
interrupts and ack them, and PORT_INTMSK are the masked interrupts.
And then PORT_INTMSKSET and PORT_INTMSKCLR can be used to manipulate
individual bits of PORT_INTMSK with a single store.

So this really should be modelled as an interrupt controller. If
someone comes up with a bit of a spec (though the bit assignment is
relatively clear), I can update the interrupt code to handle
that. After all, I moan enough at people writing horrible PCI driver
code, I might as well write an example myself and point them to it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.