[PATCH v2 5/9] pci: pcie_dw_rockchip: Hide BARs of the root complex

[Jonas Karlman]

Hi Kever,
On 2023-06-29 12:25, Kever Yang wrote:
> Hi Jonas,
> 
>      Sorry for reply late for this patch.
> 
>      I have check internally again for this BAR issue, the BAR alloce 
> fail for RC would not affect other process, and it always works on 
> vendor U-Boot tree.

The issue is not that the RC BAR allocation fails and that it affect
other PCI devices BAR allocation after that, the issue is that with
updated ranges the RC BAR allocation is successful and the entire memory
region gets allocated and that in turn affect other PCI devices from a
successful probe.

There are multiple parts in play here:

1. Two BARs of 1 GiB each is reported for RC on RK3568, 0xC0000000 is read
   back instead of 0x0000000.
   As seen by "PCI Autoconfig: BAR X, Mem, size=0x40000000" below.

2. With the ranges from "rockchip: rk356x: Update PCIe config, IO and
   memory regions", now also merged in linux, the first BAR for RC does
   allocate the entire memory region.
   As seen by "With a memory region of the entire 1 GiB this leads to".

   PCI Autoconfig: BAR 0, Mem, size=0x40000000, address=0x40000000 bus_lower=0x80000000

3. Any other BAR processed will fail with "No room in resource" and the
   PCI autoconfig fails and no device is discovered.
   As seen by "With a memory region of the entire 1 GiB this leads to".

   PCI Autoconfig: BAR 0, Mem64, size=0x4000, No room in resource, avail start=80000000 / size=40000000, need=4000
   PCI: Failed autoconfig bar 10

On downstream vendor U-Boot this is not an issue because the start
address is 0x0 instead of 0x40000000. U-Boot skips first 0x1000 of the
range when the start address is 0x0, meaning the available range is less
then 1 GiB, and the BAR allocation for RC fails.

As seen by "With a memory region less than 1 GiB this was not a real
issue" this also happen until memory ranges are adjusted in device tree.

  PCI Autoconfig: BAR 0, Mem, size=0x40000000, No room in resource, avail start=1000 / size=3ef00000, need=40000000

The issue with RC allocating the entire 1 GiB region would probably
affect vendor U-Boot if it would use 0x40000000 as start address instead
of 0x0.

There has been reports on linux ML that the use of 0x0 as start address
caused issues for some PCI devices and that the use of 0x40000000 as
start address fixed such issue.

> 
>      And if we have to handle this, we can disable the BAR instead of 
> this workaround.

Please provide details on how we can configure the HW to not report any
BARs when in RC mode.

Using a very similar approach that other pci drivers have used, e.g. the
Intel FPGA PCIe host controller driver, is the only way that I found
that worked consistently.
See https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_intel_fpga.c#L87-103

Regards,
Jonas

> 
> 
> Thanks,
> 
> - Kever
> 
> On 2023/5/18 06:53, Jonas Karlman wrote:
>> PCI Autoconfig read the Root Complex BARs and try to claim the entire
>> 1 GiB memory region on RK3568, leaving no space for any attached device.
>>
>> With a memory region less than 1 GiB this was not a real issue:
>>
>>    PCI Autoconfig: Bus Memory region: [0-3eefffff],
>>    PCI Autoconfig: Bus I/O region: [3ef00000-3effffff],
>>    PCI Autoconfig: Found P2P bridge, device 0
>>    PCI Autoconfig: BAR 0, Mem, size=0x40000000, No room in resource, avail start=1000 / size=3ef00000, need=40000000
>>    PCI: Failed autoconfig bar 10
>>    PCI Autoconfig: BAR 1, Mem, size=0x40000000, No room in resource, avail start=1000 / size=3ef00000, need=40000000
>>    PCI: Failed autoconfig bar 14
>>    PCI Autoconfig: ROM, size=0x10000, address=0x10000 bus_lower=0x20000
>>
>>    PCI Autoconfig: BAR 0, Mem64, size=0x4000, address=0x100000 bus_lower=0x104000
>>
>> With a memory region of the entire 1 GiB this leads to:
>>
>>    PCI Autoconfig: Bus Memory region: [40000000-7fffffff],
>>    PCI Autoconfig: Bus I/O region: [f0100000-f01fffff],
>>    PCI Autoconfig: Found P2P bridge, device 0
>>    PCI Autoconfig: BAR 0, Mem, size=0x40000000, address=0x40000000 bus_lower=0x80000000
>>    PCI Autoconfig: BAR 1, Mem, size=0x40000000, No room in resource, avail start=80000000 / size=40000000, need=40000000
>>    PCI: Failed autoconfig bar 14
>>    PCI Autoconfig: ROM, size=0x10000, No room in resource, avail start=80000000 / size=40000000, need=10000
>>
>>    PCI Autoconfig: BAR 0, Mem64, size=0x4000, No room in resource, avail start=80000000 / size=40000000, need=4000
>>    PCI: Failed autoconfig bar 10
>>
>> After this change with a memory region of the entire 1 GiB:
>>
>>    PCI Autoconfig: Bus Memory region: [40000000-7fffffff],
>>    PCI Autoconfig: Bus I/O region: [f0100000-f01fffff],
>>    PCI Autoconfig: Found P2P bridge, device 0
>>    PCI Autoconfig: ROM, size=0x10000, address=0x40000000 bus_lower=0x40010000
>>
>>    PCI Autoconfig: BAR 0, Mem64, size=0x4000, address=0x40100000 bus_lower=0x40104000
>>
>> Return an invalid value during config read of Root Complex BARs during
>> autoconfig to work around such issue.
>>
>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>> ---
>> v2:
>> - Update commit message
>>
>>   drivers/pci/pcie_dw_rockchip.c | 28 +++++++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pcie_dw_rockchip.c b/drivers/pci/pcie_dw_rockchip.c
>> index 82a8b9c96e2b..f56773c2e58c 100644
>> --- a/drivers/pci/pcie_dw_rockchip.c
>> +++ b/drivers/pci/pcie_dw_rockchip.c
>> @@ -146,6 +146,32 @@ static inline void rk_pcie_writel_apb(struct rk_pcie *rk_pcie, u32 reg,
>>   	__rk_pcie_write_apb(rk_pcie, rk_pcie->apb_base, reg, 0x4, val);
>>   }
>>   
>> +/**
>> + * The BARs of bridge should be hidden during enumeration to avoid
>> + * allocation of the entire memory region by PCIe core on RK3568.
>> + */
>> +static bool rk_pcie_hide_rc_bar(struct pcie_dw *pcie, pci_dev_t bdf,
>> +				uint offset)
>> +{
>> +	int bus = PCI_BUS(bdf) - pcie->first_busno;
>> +
>> +	return bus == 0 && PCI_DEV(bdf) == 0 && PCI_FUNC(bdf) == 0 &&
>> +	       offset >= PCI_BASE_ADDRESS_0 && offset <= PCI_BASE_ADDRESS_1;
>> +}
>> +
>> +static int rk_pcie_read_config(const struct udevice *bus, pci_dev_t bdf,
>> +			       uint offset, ulong *valuep,
>> +			       enum pci_size_t size)
>> +{
>> +	struct pcie_dw *pcie = dev_get_priv(bus);
>> +	int ret = pcie_dw_read_config(bus, bdf, offset, valuep, size);
>> +
>> +	if (!ret && rk_pcie_hide_rc_bar(pcie, bdf, offset))
>> +		*valuep = pci_get_ff(size);
>> +
>> +	return ret;
>> +}
>> +
>>   /**
>>    * rk_pcie_configure() - Configure link capabilities and speed
>>    *
>> @@ -476,7 +502,7 @@ rockchip_pcie_probe_err_init_port:
>>   }
>>   
>>   static const struct dm_pci_ops rockchip_pcie_ops = {
>> -	.read_config	= pcie_dw_read_config,
>> +	.read_config	= rk_pcie_read_config,
>>   	.write_config	= pcie_dw_write_config,
>>   };
>>