linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Pali Rohár" <pali@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Rob Herring" <robh@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Russell King" <rmk+kernel@armlinux.org.uk>,
	"Marek Behún" <kabel@kernel.org>,
	"Remi Pommarel" <repk@triplefau.lt>, Xogium <contact@xogium.me>,
	"Tomasz Maciej Nowak" <tmn505@gmail.com>,
	"Marc Zyngier" <maz@kernel.org>,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 06/42] PCI: aardvark: Fix reporting CRS Software Visibility on emulated bridge
Date: Fri, 7 May 2021 17:25:42 +0200	[thread overview]
Message-ID: <20210507152542.sd54lk7bk56qapf3@pali> (raw)
In-Reply-To: <20210507130307.GA1448097@bjorn-Precision-5520>

On Friday 07 May 2021 08:03:07 Bjorn Helgaas wrote:
> On Thu, May 06, 2021 at 05:31:17PM +0200, Pali Rohár wrote:
> > CRS Software Visibility is supported and always enabled by PIO.
> > Correctly report this information via emulated root bridge.
> 
> Maybe spell out "Configuration Request Retry Status (CRS) Software
> Visibility" once.
> 
> I'm guessing the aardvark hardware spec is proprietary,

Seems so. I have never seen aardvark hardware or software spec. I have
already asked Marvell more times if they can provide me documentation,
but I have not received anything (yet).

I have access to A3720 documentation which is available on Marvell
Customer portal and it contains section about PCIe on A3720. It is
incomplete and mostly buggy.

So most code is written by trial & error method and simply observe how
this hardware behaves. Lot of things I can just guess how should work.
See e.g. long description in patch 7/42 Fix link training.

> but can you at
> least include a reference to the section that says CRSVIS is
> supported and CRSSVE is enabled?

There is nothing :-(

> What is PIO?  I assume this is something other than "programmed I/O"?

I'm not sure what PIO abbreviation means. As I saw that sometimes people
refers to PCIe I/O, sometimes (generic) programmed I/O and sometimes
Posted I/O.

I was probably not accurate in all commit messages as some of these
patches I have written months ago and some just few days ago.

I will try to explain what PIO means in this driver and context of A3720
PCIe. I understand it as a way how to send PCIe messages/packets from
A3720 PCIe controller without need to map PCIe address space. A3720 HW
provides registers into which you fill information what kind of PCIe
message you want to send (e.g. config read/write or memory read/write
transactions, INTx or Error or Slot Power or Vendor message) and then by
another register you trigger sending of this message and waiting until
register indicates that operation finished.

> I'd like the commit log to say something about the effect of this
> change, i.e., why are we doing it?
> 
> For one thing, I expect lspci will now show "RootCtl: ... CRSVisible+"
> and "RootCap: CRSVisible+".

Exactly.

> With PCI_EXP_RTCAP_CRSVIS set, pci_enable_crs() should now try to set
> PCI_EXP_RTCTL_CRSSVE (which I think is a no-op since
> advk_pci_bridge_emul_pcie_conf_write() doesn't do anything with
> PCI_EXP_RTCTL_CRSSVE).

aardvark hook for emul bridge does nothing for PCI_EXP_RTCTL_CRSSVE.

> So AFAICT this has zero effect on the kernel.  Possibly we *should*
> base some kernel behavior on whether PCI_EXP_RTCTL_CRSSVE is set, but
> I don't think we do today.

I understood CRSSVE bit that when it is enabled then root complex starts
returning CRS when trying to read device+vendor id from device which is
not ready yet. And if CRSSVE is not enabled then root complex should
re-issue config request until completes with non-CRS status and do not
return CRS.

And because I have not found any way how to reconfigure aardvark to auto
re-issue config request (which is when CRSSVE is not enabled) started by
PIO, I included this patch which sets CRSSVE bit in emulated bridge to
match expected behavior.

If my understanding is incorrect here, please let me know and I update
and fix this driver.

> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <kabel@kernel.org>
> > Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
> > Cc: stable@vger.kernel.org
> 
> Again, I think this just adds functionality and doesn't fix something
> that used to be broken.
> 
> Per [1], patches for the stable kernel should be for serious issues
> like an oops, hang, data corruption, etc.  I know stable kernel
> maintainers pick up all sorts of other stuff, but that's up to them.
> I try to limit stable tags to reduce the risk of regressing.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?id=v5.11

Ok, no problem. I can omit either CC, Fixes or both tags.

> > ---
> >  drivers/pci/controller/pci-aardvark.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index 3f3c72927afb..e297ec9ec390 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -578,6 +578,8 @@ advk_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
> >  	case PCI_EXP_RTCTL: {
> >  		u32 val = advk_readl(pcie, PCIE_ISR0_MASK_REG);
> >  		*value = (val & PCIE_MSG_PM_PME_MASK) ? 0 : PCI_EXP_RTCTL_PMEIE;
> > +		*value |= PCI_EXP_RTCTL_CRSSVE;
> > +		*value |= PCI_EXP_RTCAP_CRSVIS << 16;
> >  		return PCI_BRIDGE_EMUL_HANDLED;
> >  	}
> >  
> > @@ -659,6 +661,7 @@ static struct pci_bridge_emul_ops advk_pci_bridge_emul_ops = {
> >  static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> >  {
> >  	struct pci_bridge_emul *bridge = &pcie->bridge;
> > +	int ret;
> >  
> >  	bridge->conf.vendor =
> >  		cpu_to_le16(advk_readl(pcie, PCIE_CORE_DEV_ID_REG) & 0xffff);
> > @@ -682,7 +685,16 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> >  	bridge->data = pcie;
> >  	bridge->ops = &advk_pci_bridge_emul_ops;
> >  
> > -	return pci_bridge_emul_init(bridge, 0);
> > +	/* PCIe config space can be initialized after pci_bridge_emul_init() */
> > +	ret = pci_bridge_emul_init(bridge, 0);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Completion Retry Status is supported and always enabled by PIO */
> 
> "CRS Software Visibility", not "Completion Retry Status".
> 
> The CRSSVE bit is *supposed* to be RW, per spec.  Is it RO on this
> hardware?

As I wrote above, I did not find a way how to turn off CRS.

A3720 spec does not mention any register which could map to PCIe Root
Capabilities Register.

> > +	bridge->pcie_conf.rootctl = cpu_to_le16(PCI_EXP_RTCTL_CRSSVE);
> > +	bridge->pcie_conf.rootcap = cpu_to_le16(PCI_EXP_RTCAP_CRSVIS);
> > +
> > +	return 0;
> >  }
> >  
> >  static bool advk_pcie_valid_device(struct advk_pcie *pcie, struct pci_bus *bus,
> > -- 
> > 2.20.1
> > 

  reply	other threads:[~2021-05-07 15:25 UTC|newest]

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-06 15:31 [PATCH 00/42] PCI: aardvark: Various driver fixes Pali Rohár
2021-05-06 15:31 ` [PATCH 01/42] PCI: aardvark: Fix kernel panic during PIO transfer Pali Rohár
2021-05-19  8:06   ` Pali Rohár
2021-05-06 15:31 ` [PATCH 02/42] PCI: aardvark: Fix checking for PIO Non-posted Request Pali Rohár
2021-05-06 15:31 ` [PATCH 03/42] PCI: aardvark: Fix checking for PIO status Pali Rohár
2021-05-06 15:31 ` [PATCH 04/42] PCI: aardvark: Increase polling delay to 1.5s while waiting for PIO response Pali Rohár
2021-05-06 15:31 ` [PATCH 05/42] PCI: pci-bridge-emul: Add PCIe Root Capabilities Register Pali Rohár
2021-05-06 23:10   ` Bjorn Helgaas
2021-05-07 14:40     ` Pali Rohár
2021-05-07 16:41       ` Bjorn Helgaas
2021-05-06 15:31 ` [PATCH 06/42] PCI: aardvark: Fix reporting CRS Software Visibility on emulated bridge Pali Rohár
2021-05-07 13:03   ` Bjorn Helgaas
2021-05-07 15:25     ` Pali Rohár [this message]
2021-05-07 15:33     ` Pali Rohár
2021-05-06 15:31 ` [PATCH 07/42] PCI: aardvark: Fix link training Pali Rohár
2021-05-06 15:31 ` [PATCH 08/42] PCI: Add PCI_EXP_DEVCTL_PAYLOAD_* macros Pali Rohár
2021-05-06 15:31 ` [PATCH 09/42] PCI: aardvark: Fix PCIe Max Payload Size setting Pali Rohár
2021-05-06 15:31 ` [PATCH 10/42] PCI: aardvark: Implement workaround for the readback value of VEND_ID Pali Rohár
2021-05-06 15:31 ` [PATCH 11/42] PCI: aardvark: Do not touch status bits of masked interrupts in interrupt handler Pali Rohár
2021-05-06 15:31 ` [PATCH 12/42] PCI: aardvark: Check for virq mapping when processing INTx IRQ Pali Rohár
2021-05-07  9:15   ` Marc Zyngier
2021-06-04 16:24     ` Pali Rohár
2021-06-04 16:29       ` Marc Zyngier
2021-05-06 15:31 ` [PATCH 13/42] PCI: aardvark: Remove irq_mask_ack callback for INTx interrupts Pali Rohár
2021-05-07  9:16   ` Marc Zyngier
2021-05-06 15:31 ` [PATCH 14/42] PCI: aardvark: Don't mask irq when mapping Pali Rohár
2021-05-07  9:20   ` Marc Zyngier
2021-05-07  9:27     ` Pali Rohár
2021-05-06 15:31 ` [PATCH 15/42] PCI: aardvark: Change name of INTx irq_chip to advk-INT Pali Rohár
2021-05-07  9:08   ` Marc Zyngier
2021-05-24 14:36     ` Marek Behún
2021-05-24 15:14       ` Marc Zyngier
2021-05-06 15:31 ` [PATCH 16/42] PCI: aardvark: Remove unneeded goto Pali Rohár
2021-05-06 15:31 ` [PATCH 17/42] PCI: aardvark: Fix support for MSI interrupts Pali Rohár
2021-05-07 10:16   ` Marc Zyngier
2021-05-07 14:44     ` Pali Rohár
2021-05-07 16:24       ` Marc Zyngier
2021-06-04 16:02         ` Pali Rohár
2021-06-04 16:22           ` Marc Zyngier
2021-05-06 15:31 ` [PATCH 18/42] PCI: aardvark: Correctly clear and unmask all " Pali Rohár
2021-05-07 10:19   ` Marc Zyngier
2021-05-07 10:21     ` Pali Rohár
2021-05-06 15:31 ` [PATCH 19/42] PCI: aardvark: Fix setting MSI address Pali Rohár
2021-05-07 10:25   ` Marc Zyngier
2021-05-06 15:31 ` [PATCH 20/42] PCI: aardvark: Add support for more than 32 MSI interrupts Pali Rohár
2021-07-02 21:35   ` Pali Rohár
2021-05-06 15:31 ` [PATCH 21/42] PCI: aardvark: Add support for masking " Pali Rohár
2021-05-06 15:31 ` [PATCH 22/42] PCI: aardvark: Enable MSI-X support Pali Rohár
2021-05-06 15:31 ` [PATCH 23/42] PCI: aardvark: Fix support for ERR interrupt on emulated bridge Pali Rohár
2021-05-06 15:31 ` [PATCH 24/42] PCI: aardvark: Fix support for PME " Pali Rohár
2021-05-06 15:31 ` [PATCH 25/42] PCI: aardvark: Fix support for PME requester " Pali Rohár
2021-05-06 15:31 ` [PATCH 26/42] PCI: aardvark: Fix support for bus mastering and PCI_COMMAND " Pali Rohár
2021-05-06 15:31 ` [PATCH 27/42] PCI: aardvark: Disable bus mastering and mask all interrupts when unbinding driver Pali Rohár
2021-05-06 15:31 ` [PATCH 28/42] PCI: aardvark: Free config space for emulated root bridge when unbinding driver to fix memory leak Pali Rohár
2021-05-06 15:31 ` [PATCH 29/42] PCI: aardvark: Reset PCIe card and disable PHY when unbinding driver Pali Rohár
2021-05-06 15:31 ` [PATCH 30/42] PCI: aardvark: Rewrite irq code to chained irq handler Pali Rohár
2021-05-06 15:31 ` [PATCH 31/42] PCI: aardvark: Use separate INTA interrupt for emulated root bridge Pali Rohár
2021-05-06 15:31 ` [PATCH 32/42] PCI: pci-bridge-emul: Add description for class_revision field Pali Rohár
2021-05-06 15:31 ` [PATCH 33/42] PCI: pci-bridge-emul: Add definitions for missing capabilities registers Pali Rohár
2021-05-06 15:31 ` [PATCH 34/42] PCI: aardvark: Add support for DEVCAP2, DEVCTL2, LNKCAP2 and LNKCTL2 registers on emulated bridge Pali Rohár
2021-05-06 15:31 ` [PATCH 35/42] PCI: aardvark: Add support for PCI_BRIDGE_CTL_BUS_RESET " Pali Rohár
2021-05-06 15:31 ` [PATCH 36/42] PCI: aardvark: Replace custom PCIE_CORE_ERR_CAPCTL_* macros by linux/pci_regs.h macros Pali Rohár
2021-05-06 15:31 ` [PATCH 37/42] PCI: aardvark: Replace custom PCIE_CORE_INT_* macros by linux PCI_INTERRUPT_* values Pali Rohár
2021-05-06 15:31 ` [PATCH 38/42] PCI: aardvark: Cleanup some register macros Pali Rohár
2021-05-06 15:31 ` [PATCH 39/42] PCI: aardvark: Add comments for OB_WIN_ENABLE and ADDR_WIN_DISABLE Pali Rohár
2021-05-06 15:31 ` [PATCH 40/42] PCI: pci-bridge-emul: re-arrange register tests Pali Rohár
2021-05-06 15:31 ` [PATCH 41/42] PCI: pci-bridge-emul: add support for PCIe extended capabilities Pali Rohár
2021-05-06 15:31 ` [PATCH 42/42] PCI: aardvark: Add support for Advanced Error Reporting registers on emulated bridge Pali Rohár
2021-06-03 15:16 ` [PATCH 00/42] PCI: aardvark: Various driver fixes Lorenzo Pieralisi
2021-06-03 17:02   ` Pali Rohár
2021-06-03 18:02   ` Simon Glass
2021-06-03 18:18     ` Pali Rohár
2021-06-04 14:05     ` 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=20210507152542.sd54lk7bk56qapf3@pali \
    --to=pali@kernel.org \
    --cc=bhelgaas@google.com \
    --cc=contact@xogium.me \
    --cc=helgaas@kernel.org \
    --cc=kabel@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=maz@kernel.org \
    --cc=repk@triplefau.lt \
    --cc=rmk+kernel@armlinux.org.uk \
    --cc=robh@kernel.org \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=tmn505@gmail.com \
    --subject='Re: [PATCH 06/42] PCI: aardvark: Fix reporting CRS Software Visibility on emulated bridge' \
    /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).