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?
I need to be able to explain why we should merge this after the merge
window, and I'm having a hard time extracting that from the commit
log.
Bjorn
