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

[Stefan Roese <sr@denx.de>]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de