[PATCH v2 05/10] rpi4: add a mapping for the PCIe XHCI controller MMIO registers (ARM 64bit)

[Matthias Brugger]

On 05/05/2020 16:00, Matthias Brugger wrote:
> 
> 
> On 04/05/2020 14:45, Sylwester Nawrocki wrote:
>> From: Marek Szyprowski <m.szyprowski at 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 at samsung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com>
>> Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne at 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?
> 
>> +
>> +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.
> 

Ok, I see now you use the defines later on, forget my comment. It's ok as is.

Regards,
Matthias

>> +		.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. 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).
> 
> Regards,
> Matthias
> 
>>  		mem_map[i].virt = pd[i].virt;
>>  		mem_map[i].phys = pd[i].phys;
>>  		mem_map[i].size = pd[i].size;
>>