Re: [BUG] rockpro64: PCI BAR reassignment broken by commit 9d57e61bf723 ("of/pci: Add IORESOURCE_MEM_64 to resource flags for 64-bit memory addresses")

From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: Ard Biesheuvel <ardb@kernel.org>, Peter Geis <pgwipeout@gmail.com>
Cc: Punit Agrawal <punitagrawal@gmail.com>,
	Robin Murphy <robin.murphy@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	arm-mail-list <linux-arm-kernel@lists.infradead.org>,
	Heiko Stuebner <heiko.stuebner@theobroma-systems.com>,
	Leonardo Bras <leobras.c@gmail.com>,
	Rob Herring <robh@kernel.org>, PCI <linux-pci@vger.kernel.org>
Subject: Re: [BUG] rockpro64: PCI BAR reassignment broken by commit 9d57e61bf723 ("of/pci: Add IORESOURCE_MEM_64 to resource flags for 64-bit memory addresses")
Date: Wed, 26 May 2021 15:55:33 +0200	[thread overview]
Message-ID: <9b99d520-e4b1-ae44-44eb-93c2e3d0c0cb@gmail.com> (raw)
In-Reply-To: <CAMj1kXHLCJbzRpic-kkdWh5wKTE=6fqkesYbB6XoeJELKn93tw@mail.gmail.com>

Hi Ard,

Am 25.05.21 um 19:18 schrieb Ard Biesheuvel:
> [SNIP]
>>> I seriously doubt that this is what is going on here.
>>>
>>> lspci -x will give you the bare BAR values - I suspect that those are
>>> probably fine.
>> lspci -x
>> 00:00.0 PCI bridge: Fuzhou Rockchip Electronics Co., Ltd Device 3566 (rev 01)
>> 00: 87 1d 66 35 07 05 10 40 01 00 04 06 00 00 01 00
>> 10: 00 00 00 00 00 00 00 00 00 01 ff 00 10 10 00 20
>> 20: 00 10 00 10 01 00 f1 0f 00 00 00 00 00 00 00 00
>> 30: 00 00 00 00 40 00 00 00 00 00 00 00 5f 01 02 00
>>
>> 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc.
>> [AMD/ATI] Turks PRO [Radeon HD 7570]
>> 00: 02 10 5d 67 07 00 10 20 00 00 00 03 00 00 80 00
>> 10: 0c 00 00 00 00 00 00 00
> This is a 64-bit prefetchable BAR programmed with bus address 0x0
>
>> 04 00 00 10 00 00 00 00
> This is a 64-bit non-prefetchable BAR programmed with bus address 0x1000_0000
>
> (https://en.wikipedia.org/wiki/PCI_configuration_space describes the
> meaning of the low order BAR bits)

Sorry for jumping into the middle of the discussion and to be honest I 
haven't fully read it.

This looks a bit odd since on AMD VGA hardware the non-prefetchable BAR 
is usually only 32bit, not 64bit.

But this hardware generation is rather old and I'm not sure what the BAR 
assignment for that generation was. I would need to dig up the register 
description in our archives as well.

Christian.

>
>> 20: 01 10 70 3f 00 00 00 00
> This looks odd. This looks like a 32-bit MMIO address poked into a I/O BAR.
>
>
>> 00 00 00 00 28 10 20 2b
>> 30: 00 00 02 10 50 00 00 00 00 00 00 00 5f 01 00 00
>>
>> 01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Turks
>> HDMI Audio [Radeon HD 6500/6600 / 6700M Series]
>> 00: 02 10 90 aa 06 00 10 20 00 00 03 04 00 00 80 00
>> 10: 04 00 04 10 00 00 00 00 00 00 00 00 00 00 00 00
>> 20: 00 00 00 00 00 00 00 00 00 00 00 00 28 10 90 aa
>> 30: 00 00 00 00 50 00 00 00 00 00 00 00 ff 02 00 00
>>
>>>
>>>>>> Also, if <0x82000000> (32 bit) is changed to <0x83000000> (64 bit),
>>>>>> most of the allocations for the dGPU fail due to no valid regions
>>>>>> available.
>>>>>>
>>>>> But wasn't the original problem that the resource window was 64-bit to
>>>>> begin with? Are you sure we are talking about the same problem here?
>>>> The rk3399 in the original report has a 32MB memory window in the
>>>> upper end of the 4GB range.
>>>> The rk356x has a similar layout, or it can use a 1GB window available
>>>> at <0x3 0x00000000>.
>>>> Rockchip's default windows are defined as 64bit.
>>>>
>>>> The rk3399 doesn't have enough space to reasonably define two windows,
>>>> one 32bit, one 64bit, to work around an allocation bug.
>>>> These are the defined regions in the rk3399:
>>>> ranges = <0x83000000 0x0 0xfa000000 0x0 0xfa000000 0x0 0x1e00000>,
>>>> <0x81000000 0x0 0xfbe00000 0x0 0xfbe00000 0x0 0x100000>;
>>>>
>>> All you really need is a 32-bit non-prefetchable resource window: any
>>> BAR can be allocated from that. A 64-bit BAR can carry a 32-bit number
>>> (just add zeroes at the top), and a prefetchable BAR can happily live
>>> in a non-prefetchable window, with a theoretical performance impact if
>>> the OS actually does use different memory attributes for the
>>> prefetchable window (but I don't think Linux ever handles it this way)
>> So is the IO range necessary as well or will it be automatically
>> allocated as well?
>>
> You need one I/O range and one 32-bit non-prefetchable MMIO window at
> the very least, even though the I/O range is rarely used, even by
> endpoints that expose I/O BARs.
>
> The translation is tricky to get right, and confuses some drivers, so
> it is better avoided if possible. If you do need translation, make
> sure to translate in the right direction.
>
>>>
>>>>>
>>>>>>>> I am happy to put something together once I understand the preferred way
>>>>>>>> to go about it.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Punit
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Linux-rockchip mailing list
>>>>>>> Linux-rockchip@lists.infradead.org
>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip