linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-pci <linux-pci@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	Marcin Wojtas <mw@semihalf.com>,
	Leif Lindholm <leif.lindholm@linaro.org>,
	Graeme Gregory <graeme.gregory@linaro.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Jingoo Han <jingoohan1@gmail.com>,
	Joao Pinto <Joao.Pinto@synopsys.com>,
	Rob Herring <robh@kernel.org>, Will Deacon <will.deacon@arm.com>
Subject: Re: [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode
Date: Sat, 7 Oct 2017 00:10:23 +0100	[thread overview]
Message-ID: <CAKv+Gu9mcTfpDGse-QpiGDwzR7j-zV+kYQ7Ysmmi61-2a_R7Ew@mail.gmail.com> (raw)
In-Reply-To: <20171006224503.GH25517@bhelgaas-glaptop.roam.corp.google.com>

On 6 October 2017 at 23:45, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Oct 06, 2017 at 03:52:03PM +0100, Ard Biesheuvel wrote:
>> On 28 September 2017 at 16:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> > On 26 September 2017 at 10:32, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >> [+cc Will]
>> >>
>> >> On Mon, Aug 28, 2017 at 07:04:36PM +0100, Ard Biesheuvel wrote:
>> >>> Some implementations of the Synopsys Designware PCIe controller implement
>> >>> a so-called ECAM shift mode, which allows a static memory window to be
>> >>> configured that covers the configuration space of the entire bus range.
>> >>>
>> >>> If the firmware performs all the low level configuration that is required
>> >>> to expose this controller in a fully ECAM compatible manner, we can
>> >>> simply describe it as "pci-host-ecam-generic" and be done with it.
>> >>> However, it appears that in some cases (one of which is the Armada 80x0),
>> >>> the IP is synthesized with an ATU window size that does not allow the
>> >>> first bus to be mapped in a way that prevents the device on the
>> >>> downstream port from appearing more than once.
>> >>>
>> >>> So implement a driver that relies on the firmware to perform all low
>> >>> level initialization, and drives the controller in ECAM mode, but
>> >>> overrides the config space accessors to take the above quirk into
>> >>> account.
>> >>>
>> >>> Note that, unlike most drivers for this IP, this driver does not expose
>> >>> a fake bridge device at B/D/F 00:00.0. There is no point in doing so,
>> >>> given that this is not a true bridge, and does not require any windows
>> >>> to be configured in order for the downstream device to operate correctly.
>> >>> Omitting it also prevents the PCI resource allocation routines from
>> >>> handing out BAR space to it unnecessarily.
>> >>
>> >> This is a tangent, but does this mean the other drivers do not need to
>> >> expose a fake 00:00.0 device either?
>> >>
>> >
>> > To be honest, I am not so sure anymore. I am seeing some issues in
>> > ASPM code making the assumption that any device which is not a root
>> > port has a parent. If this is mandated by the spec, I guess there
>> > isn't a whole lot we can do except expose a fake root port on b/d/f
>> > 0/0/0. This used to work fine, though, and I have to confirm whether
>> > the issues I am seeing currently are due to different hardware or
>> > changes in the software.
>> >
>>
>> OK, so the issue was new because I hadn't tried using a PCIe switch
>> before, and you have already queued the fix to make the ASPM code deal
>> with that.
>>
>> So I think it /would/ be better for the other drivers to not bother
>> mocking up the root port, and simply expose the downstream device as
>> B/D/F 0/0/0 (assuming the bus range starts at 0).
>>
>> It really looks like Altera, Aardvark, Sigma, etc are all in the same
>> boat here, and need to
>> a) filter type 0 config TLPs to avoid the downstream device to appear
>> 32 times, and
>> b) mangle config space accesses to the 'root port' to hide BARs that
>> have different meanings in this context (the size of the inbound
>> window), in order to prevent the PCI resource allocation routines to
>> waste huge amounts of BAR space on them.
>
> I don't know what to do about this.  Obviously it's not your problem
> to clean this up.
>
> I don't know anything about the topology of those systems.  If the
> ASPM thing was the only issue, we can probably fix that.  Of course,
> that means no device connected to the link from those RCs could use
> ASPM (maybe that's the case anyway with the mocked-up root ports).
>

That may be the answer, although I am not sure. The limited Synopsys
documentation I have access to does list 'L0s and L1 ASPM support;
software L1 and L2 support' as a feature, and what they expose as the
root port is actually a set of PCIe config registers that are in the
MMIO region of the controller (but cannot be remapped statically in
the ECAM space). But the same MMIO region has a BAR to configure the
inbound window, and has bridge BARs that can be programmed but don't
actually affect what gets passed onto the link.

The almost-ECAM mode is much more appealing for the use cases I am
involved with, and losing ASPM is no big deal, so I'd rather not use
the dwc/ drivers if I can avoid it.

>> >>> +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
>> >>> +                                int size, u32 *val)
>> >>> +{
>> >>> +     struct pci_config_window *cfg = bus->sysdata;
>> >>> +
>> >>> +     /*
>> >>> +      * The Synopsys dw PCIe controller in RC mode will not filter type 0
>> >>> +      * config TLPs sent to devices 1 and up on its downstream port,
>> >>> +      * resulting in devices appearing multiple times on bus 0 unless we
>> >>> +      * filter them here.
>> >>> +      */
>> >>> +     if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0) {
>> >>
>> >> Trivial, but maybe you could factor out this test?  We already have
>> >> these functions that do basically the same thing and it'd be nice to
>> >> use a similar pattern (altera and dw also check for the link being up,
>> >> which seems racy and possibly bogus to me):
>> >>
>> >>   altera_pcie_valid_device()
>> >>   dw_pcie_valid_device()
>> >>   rockchip_pcie_valid_device()
>> >>
>>
>> This is rather difficult to factor out, I'm afraid:
>>
>> static bool altera_pcie_valid_device(struct altera_pcie *pcie,
>>                                      struct pci_bus *bus, int dev)
>>
>> static int dw_pcie_valid_device(struct pcie_port *pp, struct pci_bus *bus,
>>                                 int dev)
>>
>> static int rockchip_pcie_valid_device(struct rockchip_pcie *rockchip,
>>                                       struct pci_bus *bus, int dev)
>>
>> They all use different struct types to describe the RC, and the fact
>> that they model a root port means the type0 TLP filter should be
>> applied to bus 1 not bus 0.
>> So I agree there is some similarity between these, but not as much
>> with the driver I am proposing.
>
> Sorry, I didn't mean to factor all these out into a single routine; I
> just meant maybe we could add a pci_dw_valid_device() with a structure
> similar to those I mentioned.
>
>> >> The fact that altera and rockchip do essentially the same thing as dw
>> >> here suggests that this pattern is not limited to DesignWare.
>> >>
>>
>> No.
>> >> These other functions also do something similar, though not structured
>> >> the same way:
>> >>
>> >>   hisi_pcie_rd_conf()
>> >>   advk_pcie_rd_conf()
>> >>   thunder_pem_bridge_read()
>> >>   rcar_pcie_config_access()
>> >>   gapspci_config_access()
>> >>
>> >
>> > I can look into that.
>> >
>>
>> I think only hisi_pcie_rd_conf() comes close to what I need to do in
>> this driver, and this is not surprising given that it uses Synopsys IP
>> as well. I would like to replace that entirely with this driver at
>> some point, but for now I'd like to proceed with Marvell Armada 8k and
>> Socionext Synquacer only, given that those are the only ones I can
>> actually test myself.
>
> Sorry again, more unclear communication on my part.  I don't think we
> can easily factor these things out; I was really just making the
> observation that these all look pretty similar and it would be good to
> make this new driver look as similar to the existing ones as possible.
>

OK, fair enough. I took Will's advice and extended the
pci-host-generic driver instead, but I'm happy to do another round if
necessary.

  reply	other threads:[~2017-10-06 23:10 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-28 18:04 [PATCH v3 0/2] pci: add support for firmware initialized designware RCs Ard Biesheuvel
2017-08-28 18:04 ` [PATCH v3 1/2] pci: designware: add driver for DWC controller in ECAM shift mode Ard Biesheuvel
2017-09-26 17:32   ` Bjorn Helgaas
2017-09-28  9:03     ` Will Deacon
2017-09-28 15:57       ` Ard Biesheuvel
2017-09-28 16:00         ` Will Deacon
2017-09-28 16:04           ` Ard Biesheuvel
2017-09-28 15:51     ` Ard Biesheuvel
2017-09-28 17:48       ` Bjorn Helgaas
2017-09-28 18:33         ` Ard Biesheuvel
2017-09-29  3:29         ` Jingoo Han
2017-10-06 14:52       ` Ard Biesheuvel
2017-10-06 22:45         ` Bjorn Helgaas
2017-10-06 23:10           ` Ard Biesheuvel [this message]
2017-08-28 18:04 ` [PATCH v3 2/2] dt-bindings: designware: add binding for Designware PCIe in ECAM mode Ard Biesheuvel
2017-08-31 14:23   ` Rob Herring
2017-08-29 15:40 ` [PATCH v3 0/2] pci: add support for firmware initialized designware RCs Marcin Wojtas

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=CAKv+Gu9mcTfpDGse-QpiGDwzR7j-zV+kYQ7Ysmmi61-2a_R7Ew@mail.gmail.com \
    --to=ard.biesheuvel@linaro.org \
    --cc=Joao.Pinto@synopsys.com \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=graeme.gregory@linaro.org \
    --cc=helgaas@kernel.org \
    --cc=jingoohan1@gmail.com \
    --cc=leif.lindholm@linaro.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=mw@semihalf.com \
    --cc=robh@kernel.org \
    --cc=will.deacon@arm.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).