diff mbox series

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

Message ID 20200504124523.23484-6-s.nawrocki@samsung.com
State Superseded
Delegated to: Matthias Brugger
Headers show
Series USB host support for Raspberry Pi 4 board | expand

Commit Message

Sylwester Nawrocki May 4, 2020, 12:45 p.m. UTC 
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(-)

Comments

Matthias Brugger May 5, 2020, 2 p.m. UTC | #1 
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?

> +
> +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.

> +		.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;
>
Matthias Brugger May 5, 2020, 2:07 p.m. UTC | #2 
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?
> 
>> +
>> +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;
>>
Marek Szyprowski May 5, 2020, 2:10 p.m. UTC | #3 
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.

Best regards
Matthias Brugger May 5, 2020, 2:13 p.m. UTC | #4 
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
diff mbox series

Patch

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