From: "Pali Rohár" <pali@kernel.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: "Bjorn Helgaas" <bhelgaas@google.com>,
"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
"Marek Behún" <kabel@kernel.org>,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RESEND PATCH 2/3] PCI: aardvark: Fix checking for PIO status
Date: Fri, 25 Jun 2021 13:21:32 +0200 [thread overview]
Message-ID: <20210625112132.r7p7gqcyajpnnvjp@pali> (raw)
In-Reply-To: <20210625110429.GA17337@lpieralisi>
On Friday 25 June 2021 12:04:29 Lorenzo Pieralisi wrote:
> On Thu, Jun 24, 2021 at 11:33:44PM +0200, Pali Rohár wrote:
>
> [...]
>
> > -static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > +static int advk_pcie_check_pio_status(struct advk_pcie *pcie, u32 *val)
> > {
> > struct device *dev = &pcie->pdev->dev;
> > u32 reg;
> > @@ -472,15 +476,50 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> > PIO_COMPLETION_STATUS_SHIFT;
> >
> > - if (!status)
> > - return;
> > -
> > + /*
> > + * According to HW spec, the PIO status check sequence as below:
> > + * 1) even if COMPLETION_STATUS(bit9:7) indicates successful,
> > + * it still needs to check Error Status(bit11), only when this bit
> > + * indicates no error happen, the operation is successful.
> > + * 2) value Unsupported Request(1) of COMPLETION_STATUS(bit9:7) only
> > + * means a PIO write error, and for PIO read it is successful with
> > + * a read value of 0xFFFFFFFF.
> > + * 3) value Completion Retry Status(CRS) of COMPLETION_STATUS(bit9:7)
> > + * only means a PIO write error, and for PIO read it is successful
> > + * with a read value of 0xFFFF0001.
> > + * 4) value Completer Abort (CA) of COMPLETION_STATUS(bit9:7) means
> > + * error for both PIO read and PIO write operation.
> > + * 5) other errors are indicated as 'unknown'.
> > + */
> > switch (status) {
> > + case PIO_COMPLETION_STATUS_OK:
> > + if (reg & PIO_ERR_STATUS) {
> > + strcomp_status = "COMP_ERR";
> > + break;
> > + }
> > + /* Get the read result */
> > + if (val)
> > + *val = advk_readl(pcie, PIO_RD_DATA);
> > + /* No error */
> > + strcomp_status = NULL;
> > + break;
> > case PIO_COMPLETION_STATUS_UR:
> > - strcomp_status = "UR";
> > + if (val) {
> > + /* For reading, UR is not an error status */
> > + *val = CFG_RD_UR_VAL;
> > + strcomp_status = NULL;
> > + } else {
> > + strcomp_status = "UR";
> > + }
> > break;
> > case PIO_COMPLETION_STATUS_CRS:
> > - strcomp_status = "CRS";
> > + if (val) {
> > + /* For reading, CRS is not an error status */
> > + *val = CFG_RD_CRS_VAL;
>
> Need Bjorn's input on this.
Ok.
> I don't think this is what is expected from
> from a root complex according to the PCI specifications (depending on
> whether CSR software visibility is supported or not).
This patch / logic was written and reviewed by Marvell people as is
mentioned in commit description. But I was not able to get any feedback
from them about aardvark, so I have not put them into recipients of this
patch...
> Here we are fabricating a CRS completion value for all PCI config read
> transactions that are hitting a CRS completion status (and that's not
> the expected behaviour according to the PCI specifications and I don't
> think that's correct).
I see what what you mean. I think that for PCI_VENDOR_ID read request it
is correct. But question is what we should return for other read
requests.
> > + strcomp_status = NULL;
> > + } else {
> > + strcomp_status = "CRS";
> > + }
> > break;
> > case PIO_COMPLETION_STATUS_CA:
> > strcomp_status = "CA";
> > @@ -490,6 +529,9 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> > break;
> > }
> >
> > + if (!strcomp_status)
> > + return 0;
> > +
> > if (reg & PIO_NON_POSTED_REQ)
> > str_posted = "Non-posted";
> > else
> > @@ -497,6 +539,8 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> >
> > dev_err(dev, "%s PIO Response Status: %s, %#x @ %#x\n",
> > str_posted, strcomp_status, reg, advk_readl(pcie, PIO_ADDR_LS));
> > +
> > + return -EFAULT;
> > }
> >
> > static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> > @@ -703,8 +747,17 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > size, val);
> >
> > if (advk_pcie_pio_is_running(pcie)) {
> > - *val = 0xffffffff;
> > - return PCIBIOS_SET_FAILED;
> > + /*
> > + * For PCI_VENDOR_ID register, return Completion Retry Status
> > + * so caller tries to issue the request again insted of failing
> > + */
> > + if (where == PCI_VENDOR_ID) {
> > + *val = CFG_RD_CRS_VAL;
> > + return PCIBIOS_SUCCESSFUL;
>
> Mmmm..here we are faking a CRS completion value to coerce the kernel
> into believing a CRS completion was received (which is not necessarily
> true) ?
This part of patch was written by me. I chose to return "fake CRS" to
let kernel / software to issue a new PCI_VENDOR_ID read request again
after timeout. After some timeout previous PIO transfer should complete
and therefore advk_pcie_pio_is_running returns false.
> if advk_pcie_pio_is_running(pcie) == true, is that an HW error ?
No. It indicates that software (kernel) was impatient for previous
config read / write request and did not wait for previous completion. So
at the time when kernel tried to issue a new (this) config read request,
previous one was still running (advk_pcie_pio_is_running returned true)
and therefore driver was not able to issue a new config read request.
In patch 3/3 I increased wait timeout so this situation when
advk_pcie_pio_is_running returns true should not happen. Or rather to
say, I was not able to reproduce it anymore.
> Lorenzo
>
> > + } else {
> > + *val = 0xffffffff;
> > + return PCIBIOS_SET_FAILED;
> > + }
> > }
> >
> > /* Program the control register */
> > @@ -729,15 +782,27 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> > advk_writel(pcie, 1, PIO_START);
> >
> > ret = advk_pcie_wait_pio(pcie);
> > + if (ret < 0) {
> > + /*
> > + * For PCI_VENDOR_ID register, return Completion Retry Status
> > + * so caller tries to issue the request again instead of failing
> > + */
> > + if (where == PCI_VENDOR_ID) {
> > + *val = CFG_RD_CRS_VAL;
> > + return PCIBIOS_SUCCESSFUL;
> > + } else {
> > + *val = 0xffffffff;
> > + return PCIBIOS_SET_FAILED;
> > + }
> > + }
> > +
> > + /* Check PIO status and get the read result */
> > + ret = advk_pcie_check_pio_status(pcie, val);
> > if (ret < 0) {
> > *val = 0xffffffff;
> > return PCIBIOS_SET_FAILED;
> > }
> >
> > - advk_pcie_check_pio_status(pcie);
> > -
> > - /* Get the read result */
> > - *val = advk_readl(pcie, PIO_RD_DATA);
> > if (size == 1)
> > *val = (*val >> (8 * (where & 3))) & 0xff;
> > else if (size == 2)
> > @@ -801,7 +866,9 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> > if (ret < 0)
> > return PCIBIOS_SET_FAILED;
> >
> > - advk_pcie_check_pio_status(pcie);
> > + ret = advk_pcie_check_pio_status(pcie, NULL);
> > + if (ret < 0)
> > + return PCIBIOS_SET_FAILED;
> >
> > return PCIBIOS_SUCCESSFUL;
> > }
> > --
> > 2.20.1
> >
next prev parent reply other threads:[~2021-06-25 11:21 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-24 21:33 [RESEND PATCH 0/3] PCI: aardvark: PIO fixes Pali Rohár
2021-06-24 21:33 ` [RESEND PATCH 1/3] PCI: aardvark: Fix checking for PIO Non-posted Request Pali Rohár
2021-06-24 21:33 ` [RESEND PATCH 2/3] PCI: aardvark: Fix checking for PIO status Pali Rohár
2021-06-25 11:04 ` Lorenzo Pieralisi
2021-06-25 11:21 ` Pali Rohár [this message]
2021-07-19 23:12 ` Bjorn Helgaas
2021-07-20 14:49 ` Pali Rohár
2021-07-20 16:34 ` Bjorn Helgaas
2021-07-20 18:11 ` Pali Rohár
2021-07-20 18:30 ` Bjorn Helgaas
2021-07-21 21:05 ` Bjorn Helgaas
2021-06-24 21:33 ` [RESEND PATCH 3/3] PCI: aardvark: Increase polling delay to 1.5s while waiting for PIO response Pali Rohár
2021-06-25 11:44 ` [RESEND PATCH 0/3] PCI: aardvark: PIO fixes Lorenzo Pieralisi
2021-07-22 14:40 ` [PATCH v2 0/4] " Pali Rohár
2021-07-22 14:40 ` [PATCH v2 1/4] PCI: aardvark: Fix checking for PIO status Pali Rohár
2021-07-22 14:40 ` [PATCH v2 2/4] PCI: aardvark: Increase polling delay to 1.5s while waiting for PIO response Pali Rohár
2021-07-22 14:40 ` [PATCH v2 3/4] PCI: pci-bridge-emul: Add PCIe Root Capabilities Register Pali Rohár
2021-07-22 14:40 ` [PATCH v2 4/4] PCI: aardvark: Fix reporting CRS value Pali Rohár
2021-08-05 9:54 ` [PATCH v2 0/4] PCI: aardvark: PIO fixes Lorenzo Pieralisi
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=20210625112132.r7p7gqcyajpnnvjp@pali \
--to=pali@kernel.org \
--cc=bhelgaas@google.com \
--cc=kabel@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=thomas.petazzoni@bootlin.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).