Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
User-Agent: Cyrus-JMAP/3.5.0-alpha0-1118-g75eff666e5-fm-20210816.002-g75eff666
Mime-Version: 1.0
Message-Id: <836a1a93-5031-4218-bc0b-17fc38f93931@www.fastmail.com>
In-Reply-To: <87tujpyrxu.wl-maz@kernel.org>
References: <20210815042525.36878-1-alyssa@rosenzweig.io>
 <20210815042525.36878-3-alyssa@rosenzweig.io> <87a6lj17d1.wl-maz@kernel.org>
 <YRm//JU0otojw+rV@sunset> <87tujpyrxu.wl-maz@kernel.org>
Date:   Tue, 17 Aug 2021 09:35:43 +0200
From:   "Sven Peter" <sven@svenpeter.dev>
To:     "Marc Zyngier" <maz@kernel.org>,
        "Alyssa Rosenzweig" <alyssa@rosenzweig.io>
Cc:     linux-pci@vger.kernel.org, "Bjorn Helgaas" <bhelgaas@google.com>,
        "Rob Herring" <robh+dt@kernel.org>,
        "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
        =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        "Stan Skowronek" <stan@corellium.com>,
        "Mark Kettenis" <kettenis@openbsd.org>,
        "Hector Martin" <marcan@marcan.st>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] PCI: apple: Add driver for the Apple M1
Content-Type: text/plain
Precedence: bulk


On Mon, Aug 16, 2021, at 23:56, Marc Zyngier wrote:
> [...]
> 
> > > > +	writel(0xfb512fff, port + PORT_INTMSKSET);
> > > 
> > > Magic. What is this for?
> > 
> > Sven's explanation is the most likely. This magic value comes from
> > Corellium via Mark; I assume it's written by macOS.
> 
> I really wish there was no magic values whatsoever, and I've resisted
> posting the PCIe support patch myself for this very reason. Frankly,
> this stuff doesn't give me the warm feeling that we know what we're
> doing.

I'm pretty sure we can get rid of most magic since we have register names for
almost everything we need and since m1n1 does the really obscure black magic
involving the PHY layer and those tunables thanks to Mark.

As I mentioned earlier, all bits missing in 0xfb512fff are those used in
the writel one line below. This line only keeps a set of interrupts unmasked
and the next one acks exactly this set (which isn't correct, but that's what
this code does).
There only unknown interrupt here is BIT(26) but this whole sequence is like a
cargo cult anyway right now since nothing checks for these interrupts.

> 
> > 
> > > > +	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 reordered the code to have the configuration after this happen
> > before the START command as suggested (this works), and then removed
> > the poll entirely (this also works?). It's possible the poll here
> > was just a debug leftover in the original code.
> 
> What happens if the core PCI code probes the ports without the link
> being up yet?

We pretty much rely on everything being slow enough that the ports aren't probed
if there's no readl_poll_timeout and no waiting for the link up interrupt.
Now it doesn't take long for the link to be up after the LTSSM has been
started, but it's still a few cycles and this is not a good idea.
This needs to wait for the interrupt.

I don't know yet what the first usleep_range is used for, but I'm willing to
bet it's either waiting for another interrupt to fire or sometimes the link doesn't
come up the first time and you just have to try again and the usleep prevents that.
(I'm less inclined to bet on this one, but: this might be required for the first
port with the WiFi/bluetooth radios which will never come up unless power has been
enabled by talking to another co-processor first. That usleep_range might be a hack
so that this code always runs after power has been enabled.)

That same LTSSM retry flow is used for Thunderbolt hotplugging fwiw:
Wait for the NHI layer to notify you, start the link training, wait for the
link up interrupt (or the link down or error interrupt and just try link training
again a few times), rescan the port.

> 
> > It's possible it's needed in the original but not needed in the
> > interrupt-driven common code (if the link doesn't come up yet,
> > nothing happens, so we don't have to block on it ourselves..)
> > 
> > It's also possible I've introduced a race that we happen to win every time.
> > 
> > Without specs, it's exceedingly hard to know which it is...
> 
> Indeed, and I hate this "finger in the air" approach. Specially when
> you need to trust your data to it.
> 
> > The poll isn't what we want at any rate, so I've removed the poll in
> > v2. But we may want extra interrupt handling code for v3.
> 
> Indeed. I need to rework the MSI patch anyway after the discussion
> with Rob, and I'll see what I can do for the rest of the event stuff.

Again, thanks a lot for this! As I said in the other mail, if you need
any specific information about this hardware just let me know.
I won't be able to give you an accurate spec but I can try to figure out
most details you need.

Thanks,


Sven