From mboxrd@z Thu Jan  1 00:00:00 1970
From: Matthias Brugger <mbrugger@suse.com>
Date: Tue, 5 May 2020 16:13:01 +0200
Subject: [PATCH v2 05/10] rpi4: add a mapping for the PCIe XHCI controller
 MMIO registers (ARM 64bit)
In-Reply-To: <6f6482f9-e804-89bf-c3b8-ba04eda74a14@samsung.com>
References: <20200504124523.23484-1-s.nawrocki@samsung.com>
 <CGME20200504124545eucas1p1532613e2c1558cb043f3b32946c748ea@eucas1p1.samsung.com>
 <20200504124523.23484-6-s.nawrocki@samsung.com>
 <6c5feb04-a0ba-e746-cc87-6604df6ee810@suse.com>
 <6f6482f9-e804-89bf-c3b8-ba04eda74a14@samsung.com>
Message-ID: <5d2e4b4a-cae0-e014-7827-1f1491dc1208@suse.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de



On 05/05/2020 16:10, Marek Szyprowski wrote:
> Hi Matthias,
> 
> On 05.05.2020 16:00, Matthias Brugger wrote:
>> On 04/05/2020 14:45, Sylwester Nawrocki wrote:
>>> From: Marek Szyprowski <m.szyprowski@samsung.com>
>>>
>>> Create a non-cacheable mapping for the 0x600000000 physical memory region,
>>> where MMIO registers for the PCIe XHCI controller are instantiated by the
>>> PCIe bridge.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>> Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>> ---
>>> Changes since v1:
>>>   - none.
>>> ---
>>>   arch/arm/mach-bcm283x/init.c | 18 +++++++++++++++---
>>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-bcm283x/init.c b/arch/arm/mach-bcm283x/init.c
>>> index 4295356..6a748da 100644
>>> --- a/arch/arm/mach-bcm283x/init.c
>>> +++ b/arch/arm/mach-bcm283x/init.c
>>> @@ -11,10 +11,15 @@
>>>   #include <dm/device.h>
>>>   #include <fdt_support.h>
>>>   
>>> +#define BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS	0x600000000UL
>>> +#define BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE	0x800000UL
>>> +
>>>   #ifdef CONFIG_ARM64
>>>   #include <asm/armv8/mmu.h>
>>>   
>>> -static struct mm_region bcm283x_mem_map[] = {
>>> +#define MAX_MAP_MAX_ENTRIES (4)
>> What stands the second 'MAX' for?
> a simple copy/paste issue. I will fix it.
>>> +
>>> +static struct mm_region bcm283x_mem_map[MAX_MAP_MAX_ENTRIES] = {
>>>   	{
>>>   		.virt = 0x00000000UL,
>>>   		.phys = 0x00000000UL,
>>> @@ -34,7 +39,7 @@ static struct mm_region bcm283x_mem_map[] = {
>>>   	}
>>>   };
>>>   
>>> -static struct mm_region bcm2711_mem_map[] = {
>>> +static struct mm_region bcm2711_mem_map[MAX_MAP_MAX_ENTRIES] = {
>>>   	{
>>>   		.virt = 0x00000000UL,
>>>   		.phys = 0x00000000UL,
>>> @@ -49,6 +54,13 @@ static struct mm_region bcm2711_mem_map[] = {
>>>   			 PTE_BLOCK_NON_SHARE |
>>>   			 PTE_BLOCK_PXN | PTE_BLOCK_UXN
>>>   	}, {
>> I'd prefer a comment instead of using the BCM2711_RPI4_PCIE_XHCI_MMIO_* defines.
> 
> Those defines are also used in ARM 32bit code.
> 
>>> +		.virt = BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS> +		.phys = BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS,
>>> +		.size = BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE,
>>> +		.attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |
>>> +			 PTE_BLOCK_NON_SHARE |
>>> +			 PTE_BLOCK_PXN | PTE_BLOCK_UXN
>>> +	}, {
>>>   		/* List terminator */
>>>   		0,
>>>   	}
>>> @@ -71,7 +83,7 @@ static void _rpi_update_mem_map(struct mm_region *pd)
>>>   {
>>>   	int i;
>>>   
>>> -	for (i = 0; i < 2; i++) {
>>> +	for (i = 0; i < MAX_MAP_MAX_ENTRIES; i++) {
>> Variable mem_map points to bcm283x_mem_map which only holds two mm_region's
>> (plus list terminator). So we have an overflow here.
> Nope, I've changed the bcm283x_mem_map to be large enough, see the above 
> diff.
>>   I think we should just
>> define bcm2711_mem_map and bcm283x_mem_map with a fixed array size of 4 (see
>> comment on the define naming above).
> 
> That's exactly what I did.
> 

You are right, sorry for the noise!

Matthias