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

2020-05-05 Thread Matthias Brugger 



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 
>>>
>>> Create a non-cacheable mapping for the 0x6 physical memory region,
>>> where MMIO registers for the PCIe XHCI controller are instantiated by the
>>> PCIe bridge.
>>>
>>> Signed-off-by: Marek Szyprowski 
>>> Signed-off-by: Sylwester Nawrocki 
>>> Reviewed-by: Nicolas Saenz Julienne 
>>> ---
>>> 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 
>>>   #include 
>>>   
>>> +#define BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS   0x6UL
>>> +#define BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE   0x80UL
>>> +
>>>   #ifdef CONFIG_ARM64
>>>   #include 
>>>   
>>> -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 = 0xUL,
>>> .phys = 0xUL,
>>> @@ -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 = 0xUL,
>>> .phys = 0xUL,
>>> @@ -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

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

2020-05-05 Thread Marek Szyprowski 
Hi Matthias,

On 05.05.2020 16:00, Matthias Brugger wrote:
> On 04/05/2020 14:45, Sylwester Nawrocki wrote:
>> From: Marek Szyprowski 
>>
>> Create a non-cacheable mapping for the 0x6 physical memory region,
>> where MMIO registers for the PCIe XHCI controller are instantiated by the
>> PCIe bridge.
>>
>> Signed-off-by: Marek Szyprowski 
>> Signed-off-by: Sylwester Nawrocki 
>> Reviewed-by: Nicolas Saenz Julienne 
>> ---
>> 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 
>>   #include 
>>   
>> +#define BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS0x6UL
>> +#define BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE0x80UL
>> +
>>   #ifdef CONFIG_ARM64
>>   #include 
>>   
>> -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 = 0xUL,
>>  .phys = 0xUL,
>> @@ -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 = 0xUL,
>>  .phys = 0xUL,
>> @@ -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
-- 
Marek Szyprowski, PhD
Samsung R Institute Poland

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

2020-05-05 Thread Matthias Brugger 



On 05/05/2020 16:00, Matthias Brugger wrote:
> 
> 
> On 04/05/2020 14:45, Sylwester Nawrocki wrote:
>> From: Marek Szyprowski 
>>
>> Create a non-cacheable mapping for the 0x6 physical memory region,
>> where MMIO registers for the PCIe XHCI controller are instantiated by the
>> PCIe bridge.
>>
>> Signed-off-by: Marek Szyprowski 
>> Signed-off-by: Sylwester Nawrocki 
>> Reviewed-by: Nicolas Saenz Julienne 
>> ---
>> 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 
>>  #include 
>>  
>> +#define BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS0x6UL
>> +#define BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE0x80UL
>> +
>>  #ifdef CONFIG_ARM64
>>  #include 
>>  
>> -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 = 0xUL,
>>  .phys = 0xUL,
>> @@ -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 = 0xUL,
>>  .phys = 0xUL,
>> @@ -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;
>>

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

2020-05-05 Thread Matthias Brugger 



On 04/05/2020 14:45, Sylwester Nawrocki wrote:
> From: Marek Szyprowski 
> 
> Create a non-cacheable mapping for the 0x6 physical memory region,
> where MMIO registers for the PCIe XHCI controller are instantiated by the
> PCIe bridge.
> 
> Signed-off-by: Marek Szyprowski 
> Signed-off-by: Sylwester Nawrocki 
> Reviewed-by: Nicolas Saenz Julienne 
> ---
> 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 
>  #include 
>  
> +#define BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS 0x6UL
> +#define BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE 0x80UL
> +
>  #ifdef CONFIG_ARM64
>  #include 
>  
> -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 = 0xUL,
>   .phys = 0xUL,
> @@ -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 = 0xUL,
>   .phys = 0xUL,
> @@ -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;
>

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

2020-05-04 Thread Sylwester Nawrocki 
From: Marek Szyprowski 

Create a non-cacheable mapping for the 0x6 physical memory region,
where MMIO registers for the PCIe XHCI controller are instantiated by the
PCIe bridge.

Signed-off-by: Marek Szyprowski 
Signed-off-by: Sylwester Nawrocki 
Reviewed-by: Nicolas Saenz Julienne 
---
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 
 #include 
 
+#define BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS   0x6UL
+#define BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE   0x80UL
+
 #ifdef CONFIG_ARM64
 #include 
 
-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 = 0xUL,
.phys = 0xUL,
@@ -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 = 0xUL,
.phys = 0xUL,
@@ -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;
-- 
2.7.4