On Wed, Dec 01, 2021 at 06:23:24PM +0100, Marek Behún wrote:
> On Wed, 1 Dec 2021 06:35:18 -0600
> Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> > On Wed, Dec 01, 2021 at 10:50:45AM +0100, Marek Behún wrote:
> > > On Tue, 30 Nov 2021 19:53:08 -0600
> > > Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > >
> > > > On Tue, Nov 30, 2021 at 11:29:37AM +0000, Lorenzo Pieralisi wrote:
> > > > > On Thu, Nov 25, 2021 at 05:01:48PM +0100, Marek Behún wrote:
> > > > > > This reverts commit 239edf686c14a9ff926dec2f350289ed7adfefe2.
> > > > > >
> > > > > > PCI Bridge which represents aardvark's PCIe Root Port has Expansion ROM
> > > > > > Base Address register at offset 0x30, but its meaning is different than
> > > > > > PCI's Expansion ROM BAR register, although the layout is the same.
> > > > > > (This is why we thought it does the same thing.)
> > > > > >
> > > > > > First: there is no ROM (or part of BootROM) in the A3720 SOC dedicated
> > > > > > for PCIe Root Port (or controller in RC mode) containing executable code
> > > > > > that would initialize the Root Port, suitable for execution in
> > > > > > bootloader (this is how Expansion ROM BAR is used on x86).
> > > > > >
> > > > > > Second: in A3720 spec the register (address D0070030) is not documented
> > > > > > at all for Root Complex mode, but similar to other BAR registers, it has
> > > > > > an "entangled partner" in register D0075920, which does address
> > > > > > translation for the BAR in D0070030:
> > > > > > - the BAR register sets the address from the view of PCIe bus
> > > > > > - the translation register sets the address from the view of the CPU
> > > > > >
> > > > > > The other BAR registers also have this entangled partner, and they
> > > > > > can be used to:
> > > > > > - in RC mode: address-checking on the receive side of the RC (they
> > > > > > can define address ranges for memory accesses from remote Endpoints
> > > > > > to the RC)
> > > > > > - in Endpoint mode: allow the remote CPU to access memory on A3720
> > > > > >
> > > > > > The Expansion ROM BAR has only the Endpoint part documented, but from
> > > > > > the similarities we think that it can also be used in RC mode in that
> > > > > > way.
> > > > > >
> > > > > > So either Expansion ROM BAR has different meaning (if the hypothesis
> > > > > > above is true), or we don't know it's meaning (since it is not
> > > > > > documented for RC mode).
> > > > > >
> > > > > > Remove the register from the emulated bridge accessing functions.
> > > > > >
> > > > > > Fixes: 239edf686c14 ("PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge")
> > > > > > Signed-off-by: Marek Behún <kabel@xxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/pci/controller/pci-aardvark.c | 9 ---------
> > > > > > 1 file changed, 9 deletions(-)
> > > > >
> > > > > Bjorn,
> > > > >
> > > > > this reverts a commit we merged the last merge window so it is
> > > > > a candidate for one of the upcoming -rcX.
> > > >
> > > > Sure, happy to apply the revert.
> > > >
> > > > What problem does the revert fix? I assume 239edf686c14 ("PCI:
> > > > aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge") broke
> > > > something, but the commit log for the revert doesn't say *what*. How
> > > > would one notice that something broke?
> > >
> > > Hello Bjorn,
> > >
> > > It doesn't break any real functionality that I know of, although it
> > > might, since the register is read pci/probe.c pci_setup_device()
> > > (pci_read_bases()).
> > >
> > > But allowing the access to the register when it has different meaning
> > > is wrong in a similar sense that a memory leak is wrong (a memory leak
> > > also does not necessarily cause real problems, if it is small, but
> > > still we should fix it).
> >
> > What is the new information that led you to conclude that 239edf686c14
> > is wrong? Apparently you originally thought the bridge had a ROM BAR,
> > but later decided it didn't, based on *something*? New observation?
> > New understanding of the spec?
>
> Hi Bjorn,
>
> The new observation is that although the register is defined in
> register list (together with it's layour), it is not documented in RC
> mode, although other BAR registers are (and their meaning is something
> different from standard BAR registers in RC mode). Combined with the
> fact that there is no ROM containing executable code in the SOC (which
> we knew even before, but thought that maybe it could be somehow also
> implemented), we concluded that this register has different meaning from
> standard Expansion ROM BAR.
Thanks, applied to for-linus for v5.16!
Bjorn
