LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Sven Peter" <sven@svenpeter.dev>
To: "Marc Zyngier" <maz@kernel.org>
Cc: "Alyssa Rosenzweig" <alyssa@rosenzweig.io>,
	linux-pci@vger.kernel.org, "Bjorn Helgaas" <bhelgaas@google.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Krzysztof Wilczyński" <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
Date: Mon, 16 Aug 2021 08:37:07 +0200	[thread overview]
Message-ID: <6c97e086-52f6-4f2f-8238-d9b1c24fe8da@www.fastmail.com> (raw)
In-Reply-To: <871r6u1wlz.wl-maz@kernel.org>



On Sun, Aug 15, 2021, at 18:49, Marc Zyngier wrote:
> On Sun, 15 Aug 2021 13:33:14 +0100,
> "Sven Peter" <sven@svenpeter.dev> wrote:
> > 
> > 
> > 
> > On Sun, Aug 15, 2021, at 09:42, Marc Zyngier wrote:
> > > On Sun, 15 Aug 2021 05:25:25 +0100,
> > > Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
> > [...]
> > > > +
> > > > +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.
> > 
> > > 
> > > > +
> > > > +	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 for the offer!
Unfortunately, what I've written above is almost everything I know about this
hardware. If you tell me what additional details you need to know I can see
what I'm able to figure out though and write a small summary.

Thanks,


Sven

  reply	other threads:[~2021-08-16  6:37 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-15  4:25 [RFC PATCH 0/2] Add PCI " Alyssa Rosenzweig
2021-08-15  4:25 ` [RFC PATCH 1/2] dt-bindings: PCI: Add Apple PCI controller Alyssa Rosenzweig
2021-08-15  7:09   ` Marc Zyngier
     [not found]     ` <1566004903.6140692.1629015053757@ox-webmail.xs4all.nl>
2021-08-15  9:12       ` Marc Zyngier
2021-08-16  1:34     ` Alyssa Rosenzweig
2021-08-22 18:03       ` Mark Kettenis
2021-08-15  4:25 ` [RFC PATCH 2/2] PCI: apple: Add driver for the Apple M1 Alyssa Rosenzweig
2021-08-15  7:42   ` Marc Zyngier
2021-08-15  9:19     ` Marc Zyngier
2021-08-16  1:45       ` Alyssa Rosenzweig
2021-08-15 12:33     ` Sven Peter
2021-08-15 16:49       ` Marc Zyngier
2021-08-16  6:37         ` Sven Peter [this message]
2021-08-18 11:43       ` Hector Martin
2021-08-18 14:22         ` Mark Kettenis
2021-08-16  1:31     ` Alyssa Rosenzweig
2021-08-16 21:56       ` Marc Zyngier
2021-08-17  7:34         ` Arnd Bergmann
2021-08-17  8:12           ` Marc Zyngier
2021-08-17  7:35         ` Sven Peter
2021-08-15  7:43   ` Sven Peter
2021-08-15 21:40     ` Alyssa Rosenzweig
2021-08-15 20:57   ` Rob Herring
2021-08-15 21:33     ` Alyssa Rosenzweig
     [not found]   ` <CAHp75VeKeGgUgALLztA3Q3jizF2=OkSzU9bzaPmTHO9Pad=QOQ@mail.gmail.com>
2021-08-16  3:20     ` Alyssa Rosenzweig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6c97e086-52f6-4f2f-8238-d9b1c24fe8da@www.fastmail.com \
    --to=sven@svenpeter.dev \
    --cc=alyssa@rosenzweig.io \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=kettenis@openbsd.org \
    --cc=kw@linux.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=marcan@marcan.st \
    --cc=maz@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=stan@corellium.com \
    --subject='Re: [RFC PATCH 2/2] PCI: apple: Add driver for the Apple M1' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).