Re: [PATCH u-boot-marvell 06/10] pci: pci_mvebu: Do not allow setting ROM BAR on PCI Bridge

On 11/11/21 16:35, Marek Behún wrote:

From: Pali Rohár pali@kernel.org

The PCI Bridge which represents mvebu PCIe Root Port has Expansion ROM Base Address register at offset 0x30 but its meaning is different that of PCI's Expansion ROM BAR register, although the address format of the register is the same.

In reality, this device does not have any configurable PCI BARs. So ensure that write operation into BARs (including Expansion ROM BAR) is a noop and registers always contain zero address which indicates that BARs are unsupported.

Fixes: a7b61ab58d5d ("pci: pci_mvebu: Properly configure and use PCI Bridge (PCIe Root Port)") Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

---

drivers/pci/pci_mvebu.c | 55 +++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c index b545e62689..701a17dfb7 100644 --- a/drivers/pci/pci_mvebu.c +++ b/drivers/pci/pci_mvebu.c @@ -93,7 +93,7 @@ struct mvebu_pcie { unsigned int mem_attr; unsigned int io_target; unsigned int io_attr;

• u32 cfgcache[(0x34 - 0x10) / 4];

• u32 cfgcache[(0x3c - 0x10) / 4]; };

  /*

@@ -189,20 +189,20 @@ static int mvebu_pcie_read_config(const struct udevice *bus, pci_dev_t bdf, }

/*

• * mvebu has different internal registers mapped into PCI config space
  

• * in range 0x10-0x34 for PCI bridge, so do not access PCI config space
  

• * for this range and instead read content from driver virtual cfgcache
  

• * The configuration space of the PCI Bridge on primary (first) bus is
  

• * of Type 0 but the BAR registers (including ROM BAR) don't have the
  

• * same meaning as in the PCIe specification. Therefore do not access
  

• * BAR registers and non-common registers (those which have different
  

• * meaning for Type 0 and Type 1 config space) of the PCI Bridge and
  

• * instead read their content from driver virtual cfgcache[].
  

  */

• if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) {

• if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) ||

• 			   (offset >= 0x38 && offset < 0x3c))) {
  

  data = pcie->cfgcache[(offset - 0x10) / 4]; debug("(addr,size,val)=(0x%04x, %d, 0x%08x) from cfgcache\n", offset, size, data); *valuep = pci_conv_32_to_size(data, offset, size); return 0;

• } else if (busno == pcie->first_busno &&

•    (offset & ~3) == PCI_ROM_ADDRESS1) {
  

• /* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */
  

• offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF;
  

  }

  /*

@@ -269,17 +269,21 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf, }

/*

• * mvebu has different internal registers mapped into PCI config space
  

• * in range 0x10-0x34 for PCI bridge, so do not access PCI config space
  

• * for this range and instead write content to driver virtual cfgcache
  

• * As explained in mvebu_pcie_read_config(), PCI Bridge Type 1 specific
  

• * config registers are not available, so we write their content only
  

• * into driver virtual cfgcache[].
  

• * And as explained in mvebu_pcie_probe(), mvebu has its own specific
  

• * way for configuring primary and secondary bus numbers.
  

  */

• if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) {

• if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) ||

• 			   (offset >= 0x38 && offset < 0x3c))) {
  

  debug("Writing to cfgcache only\n"); data = pcie->cfgcache[(offset - 0x10) / 4]; data = pci_conv_size_to_32(data, value, offset, size); /* mvebu PCI bridge does not have configurable bars */ if ((offset & ~3) == PCI_BASE_ADDRESS_0 ||

•     (offset & ~3) == PCI_BASE_ADDRESS_1)
  

•     (offset & ~3) == PCI_BASE_ADDRESS_1 ||
  

•     (offset & ~3) == PCI_ROM_ADDRESS1)
  data = 0x0;
  

  pcie->cfgcache[(offset - 0x10) / 4] = data; /* mvebu has its own way how to set PCI primary bus number */

@@ -297,10 +301,6 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf, pcie->sec_busno); } return 0;

• } else if (busno == pcie->first_busno &&

•    (offset & ~3) == PCI_ROM_ADDRESS1) {
  

• /* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */
  

• offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF;
  

  }

  /*

@@ -424,13 +424,20 @@ static int mvebu_pcie_probe(struct udevice *dev) * U-Boot cannot recognize as P2P Bridge. * * Note that this mvebu PCI Bridge does not have compliant Type 1

• * Configuration Space. Header Type is reported as Type 0 and in
  

• * range 0x10-0x34 it has aliased internal mvebu registers 0x10-0x34
  

• * (e.g. PCIE_BAR_LO_OFF) and register 0x38 is reserved.
  

• * Configuration Space. Header Type is reported as Type 0 and it
  

• * has format of Type 0 config space.
  

• * Driver for this range redirects access to virtual cfgcache[] buffer
  

• * which avoids changing internal mvebu registers. And changes Header
  

• * Type response value to Type 1.
  

• * Moreover Type 0 BAR registers (ranges 0x10 - 0x28 and 0x30 - 0x34)
  

• * have the same format in Marvell's specification as in PCIe
  

• * specification, but their meaning is totally different and they do
  

• * different things: they are aliased into internal mvebu registers
  

• * (e.g. PCIE_BAR_LO_OFF) and these should not be changed or
  

• * reconfigured by pci device drivers.
  

• *
  

• * So our driver converts Type 0 config space to Type 1 and reports
  

• * Header Type as Type 1. Access to BAR registers and to non-existent
  

• * Type 1 registers is redirected to the virtual cfgcache[] buffer,
  

• * which avoids changing unrelated registers.
  

  */ reg = readl(pcie->base + PCIE_DEV_REV_OFF); reg &= ~0xffffff00;

Viele Grüße, Stefan Roese