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? > > 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 > >