Re: [PATCH pci-fixes 2/2] Revert "PCI: aardvark: Fix support for PCI_ROM_ADDRESS1 on emulated bridge"

[Bjorn Helgaas <helgaas@kernel.org>]

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@kernel.org>
> > ---
> >  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?

> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index baa62cdcaab4..e3001b3b3293 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -32,7 +32,6 @@
> >  #define PCIE_CORE_DEV_ID_REG					0x0
> >  #define PCIE_CORE_CMD_STATUS_REG				0x4
> >  #define PCIE_CORE_DEV_REV_REG					0x8
> > -#define PCIE_CORE_EXP_ROM_BAR_REG				0x30
> >  #define PCIE_CORE_PCIEXP_CAP					0xc0
> >  #define PCIE_CORE_ERR_CAPCTL_REG				0x118
> >  #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX			BIT(5)
> > @@ -774,10 +773,6 @@ advk_pci_bridge_emul_base_conf_read(struct pci_bridge_emul *bridge,
> >  		*value = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
> >  		return PCI_BRIDGE_EMUL_HANDLED;
> >  
> > -	case PCI_ROM_ADDRESS1:
> > -		*value = advk_readl(pcie, PCIE_CORE_EXP_ROM_BAR_REG);
> > -		return PCI_BRIDGE_EMUL_HANDLED;
> > -
> >  	case PCI_INTERRUPT_LINE: {
> >  		/*
> >  		 * From the whole 32bit register we support reading from HW only
> > @@ -810,10 +805,6 @@ advk_pci_bridge_emul_base_conf_write(struct pci_bridge_emul *bridge,
> >  		advk_writel(pcie, new, PCIE_CORE_CMD_STATUS_REG);
> >  		break;
> >  
> > -	case PCI_ROM_ADDRESS1:
> > -		advk_writel(pcie, new, PCIE_CORE_EXP_ROM_BAR_REG);
> > -		break;
> > -
> >  	case PCI_INTERRUPT_LINE:
> >  		if (mask & (PCI_BRIDGE_CTL_BUS_RESET << 16)) {
> >  			u32 val = advk_readl(pcie, PCIE_CORE_CTRL1_REG);
> > -- 
> > 2.32.0
> >