From: "Chocron, Jonathan" <jonnyc@amazon.com>
To: "andrew.murray@arm.com" <andrew.murray@arm.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"jingoohan1@gmail.com" <jingoohan1@gmail.com>,
"Woodhouse, David" <dwmw@amazon.co.uk>,
"Hanoch, Uri" <hanochu@amazon.com>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
"gustavo.pimentel@synopsys.com" <gustavo.pimentel@synopsys.com>,
"Wasserstrom, Barak" <barakw@amazon.com>,
"Saidi, Ali" <alisaidi@amazon.com>,
"mark.rutland@arm.com" <mark.rutland@arm.com>,
"Hawa, Hanna" <hhhawa@amazon.com>,
"Shenhar, Talel" <talel@amazon.com>,
"Krupnik, Ronen" <ronenk@amazon.com>,
"bhelgaas@google.com" <bhelgaas@google.com>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"benh@kernel.crashing.org" <benh@kernel.crashing.org>,
"Chocron, Jonathan" <jonnyc@amazon.com>
Subject: Re: [PATCH v4 7/7] PCI: dwc: Add validation that PCIe core is set to correct mode
Date: Wed, 4 Sep 2019 13:40:42 +0000 [thread overview]
Message-ID: <dad666b23010c120b153ed59d18148a69b46cd88.camel@amazon.com> (raw)
In-Reply-To: <20190827094827.GJ14582@e119886-lin.cambridge.arm.com>
On Tue, 2019-08-27 at 10:48 +0100, Andrew Murray wrote:
> On Thu, Aug 22, 2019 at 04:30:09PM +0000, Chocron, Jonathan wrote:
> > On Thu, 2019-08-22 at 12:13 +0100, Andrew Murray wrote:
> > > On Wed, Aug 21, 2019 at 06:47:45PM +0300, Jonathan Chocron wrote:
> > > > Some PCIe controllers can be set to either Host or EP according
> > > > to
> > > > some
> > > > early boot FW. To make sure there is no discrepancy (e.g. FW
> > > > configured
> > > > the port to EP mode while the DT specifies it as a host bridge
> > > > or
> > > > vice
> > > > versa), a check has been added for each mode.
> > > >
> > > > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > > > Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > > > ---
> > > > drivers/pci/controller/dwc/pcie-designware-ep.c | 8 ++++++++
> > > > drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
> > > > 2 files changed, 16 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > index 2bf5a35c0570..00e59a134b93 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > @@ -531,6 +531,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > > > int ret;
> > > > u32 reg;
> > > > void *addr;
> > > > + u8 hdr_type;
> > > > unsigned int nbars;
> > > > unsigned int offset;
> > > > struct pci_epc *epc;
> > > > @@ -543,6 +544,13 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > > > return -EINVAL;
> > > > }
> > > >
> > > > + hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
> > > > + if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
> > > > + dev_err(pci->dev, "PCIe controller is not set
> > > > to EP
> > > > mode (hdr_type:0x%x)!\n",
> > > > + hdr_type);
> > > > + return -EIO;
> > > > + }
> > > > +
> > > > ret = of_property_read_u32(np, "num-ib-windows", &ep-
> > > > > num_ib_windows);
> > > >
> > > > if (ret < 0) {
> > > > dev_err(dev, "Unable to read *num-ib-windows*
> > > > property\n");
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > index f93252d0da5b..d2ca748e4c85 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > @@ -323,6 +323,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > > struct pci_bus *child;
> > > > struct pci_host_bridge *bridge;
> > > > struct resource *cfg_res;
> > > > + u8 hdr_type;
> > > > int ret;
> > > >
> > > > raw_spin_lock_init(&pci->pp.lock);
> > > > @@ -396,6 +397,13 @@ int dw_pcie_host_init(struct pcie_port
> > > > *pp)
> > > > }
> > > > }
> > > >
> > > > + hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
> > >
> > > Do we know if it's always safe to read these registers at this
> > > point
> > > in time?
> > >
> > > Later in dw_pcie_host_init we call pp->ops->host_init - looking
> > > at
> > > the
> > > implementations of .host_init I can see:
> > >
> > > - resets being performed (qcom_ep_reset_assert,
> > > artpec6_pcie_assert_core_reset, imx6_pcie_assert_core_reset)
> > > - changes to config space registers (ks_pcie_init_id,
> > > dw_pcie_setup_rc)
> > > including setting PCI_CLASS_DEVICE
> > > - and clocks being enabled (qcom_pcie_init_1_0_0)
> > >
> >
> > Good point! This indeed might break host drivers which actually
> > setup
> > the rc in the kernel, and don't depend on early boot FW. So the
> > validation should probably be moved to after pp->ops->host_init()
> > (and
> > similarly after ep->ops->ep_init() for the ep driver), right?
>
> Yes I think so.
>
Done as part of v5.
>
> >
> > > I'm not sure if your changes would cause anything to break for
> > > these
> > > other
> > > controllers (or future controllers) as I couldn't see any other
> > > reads
> > > to the
> > > config.
> > >
> > > Given that we are reading config space should dw_pcie_rd_own_conf
> > > be
> > > used?
> >
> > The config space of the DW core is located at the beginning of the
> > DBI
> > regspace. Furthermore, this would break the "symmetry" between the
> > host
> > and ep validations (since the ep has no notion of reading from
> > config
> > space nor a .read callback in struct dw_pcie_ep_ops).
>
> This is true, though given how different the two drivers are - this
> is only
> really 'nice to have'.
>
>
> > I agree that
> > there is some sort of overlap between the dw_pcie_read{/write}_dbi
> > dw_pcie_rd{/wr}_own_conf APIs, when accessing the host mode config
> > space, but I assume that any host driver which supplies a callback
> > for
> > .rd_own_conf() must supply an equivalent .read_dbi() one as well.
> >
> > > (For example kirin_pcie_rd_own_conf does something special).
> > >
> >
> > They specifically define an equivalent kirin_pcie_read_dbi().
>
> This may also be true. However given that the dwc framework gives
> host drivers
> the ability to provide their own rd_own_conf callback - it's very
> possible
> that these drivers can use the callback to handle quirks - now or in
> the
> future. Potentially a quirk that your direct reading won't handle.
>
> Looking at the existing drivers which provide a .rd_own_conf:
>
> pci-exynos.c:
> - Uses dw_pcie_read to directly read from registers, sandwich'd
> between writes
> to set/clear PCIE_ELBI_SLV_ARMISC. This driver provides a
> .read_dbi which
> means dw_pcie_readb_dbi probably does the same thing as
> rd_own_conf.
>
> pci-menson.c:
> - Uses dw_pcie_read to directly read from registers, it then applies
> a read
> quirk for the PCI_CLASS_DEVICE register. This driver doesn't
> provide a
> .read_dbi - thus dw_pcie_readb_dbi probably does the same thing as
> rd_own_conf provided you don't read the PCI_CLASS_DEVICE register.
>
> pcie-hisi.c:
> - Uses dw_pcie_readl_dbi to read from registers, but the controller
> only
> supports 32bit aligned reads - it doesn't provide a .read_dbi.
> This means
> that if you used dw_pcie_readb_dbi it'd probably break because you
> can't
> read 1 byte at the offset where HEADER_TYPE is.
>
> pcie-histb.c, pcie-kirin.c: looks like pci-exynos.c
>
> It looks like we're going to have to break symmetry...
>
Done as part of v5.
Thanks for your insightful review.
> Thanks,
>
> Andrew Murray
>
> >
> > > Thanks,
> > >
> > > Andrew Murray
> > >
> > > > + if (hdr_type != PCI_HEADER_TYPE_BRIDGE) {
> > > > + dev_err(pci->dev, "PCIe controller is not set
> > > > to bridge
> > > > type (hdr_type: 0x%x)!\n",
> > > > + hdr_type);
> > > > + return -EIO;
> > > > + }
> > > > +
> > > > pp->mem_base = pp->mem->start;
> > > >
> > > > if (!pp->va_cfg0_base) {
> > > > --
> > > > 2.17.1
> > > >
next prev parent reply other threads:[~2019-09-04 13:40 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-21 15:35 [PATCH v4 0/7] Amazon's Annapurna Labs DT-based PCIe host controller driver Jonathan Chocron
2019-08-21 15:35 ` [PATCH v4 1/7] PCI: Add Amazon's Annapurna Labs vendor ID Jonathan Chocron
2019-08-21 15:35 ` [PATCH v4 2/7] PCI: Add ACS quirk for Amazon Annapurna Labs root ports Jonathan Chocron
2019-08-22 8:31 ` Andrew Murray
2019-09-04 13:33 ` Chocron, Jonathan
2019-08-21 15:35 ` [PATCH v4 3/7] PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root Port Jonathan Chocron
2019-08-22 11:41 ` Andrew Murray
2019-08-22 14:36 ` Chocron, Jonathan
2019-08-22 15:07 ` Andrew Murray
2019-09-04 13:36 ` Chocron, Jonathan
2019-09-04 14:08 ` Andrew Murray
2019-08-22 15:08 ` Andrew Murray
2019-08-21 15:35 ` [PATCH v4 4/7] PCI: Add quirk to disable MSI-X support " Jonathan Chocron
2019-08-22 7:54 ` Andrew Murray
2019-08-21 15:47 ` [PATCH v4 5/7] dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge binding Jonathan Chocron
2019-08-22 11:15 ` Andrew Murray
2019-08-27 18:53 ` Rob Herring
2019-08-21 15:47 ` [PATCH v4 6/7] PCI: dwc: al: Add support for DW based driver type Jonathan Chocron
2019-08-22 10:00 ` Andrew Murray
2019-08-22 15:23 ` Chocron, Jonathan
2019-08-21 15:47 ` [PATCH v4 7/7] PCI: dwc: Add validation that PCIe core is set to correct mode Jonathan Chocron
2019-08-22 11:13 ` Andrew Murray
2019-08-22 16:30 ` Chocron, Jonathan
2019-08-27 9:48 ` Andrew Murray
2019-09-04 13:40 ` Chocron, Jonathan [this message]
2019-09-02 9:58 ` [PATCH v4 0/7] Amazon's Annapurna Labs DT-based PCIe host controller driver Lorenzo Pieralisi
2019-09-04 13:41 ` Chocron, Jonathan
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=dad666b23010c120b153ed59d18148a69b46cd88.camel@amazon.com \
--to=jonnyc@amazon.com \
--cc=alisaidi@amazon.com \
--cc=andrew.murray@arm.com \
--cc=barakw@amazon.com \
--cc=benh@kernel.crashing.org \
--cc=bhelgaas@google.com \
--cc=devicetree@vger.kernel.org \
--cc=dwmw@amazon.co.uk \
--cc=gustavo.pimentel@synopsys.com \
--cc=hanochu@amazon.com \
--cc=hhhawa@amazon.com \
--cc=jingoohan1@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=mark.rutland@arm.com \
--cc=robh+dt@kernel.org \
--cc=ronenk@amazon.com \
--cc=talel@amazon.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).