Re: [PATCH u-boot-marvell] arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() function again

2022-03-04 Thread Stefan Roese

On 2/23/22 13:52, Marek Behún wrote:

From: Pali Rohár 

The a3700_fdt_fix_pcie_regions() function still computes nonsense.

It computes the fixup offset from the PCI address taken from the first
row of the "ranges" array, which means that:
- PCI address must equal CPU address (otherwise the computed fix offset
   will be wrong),
- the first row must contain the lowest address.

This is the case for the default device-tree, which is why we didn't
notice it.

It also adds the fixup offset to all PCI and CPU addresses, which is
wrong.

Instead:
1) The fixup offset must be computed from the CPU address, not PCI
address.

2) The fixup offset must be computed from the row containing the lowest
CPU address, which is not necessarily contained in the first row.

3) The PCI address - the address to which the PCIe controller remaps the
address space as seen from the point of view of the PCIe device -
must be fixed by the fix offset in the same way as the CPU address
only in the special case when the CPU adn PCI addresses are the same.
Same addresses means that remapping is disabled, and thus if we
change the CPU address, we need also to change the PCI address so
that the remapping is still disabled afterwards.

Consider an example:
   The ranges entries contain:
 PCI address   CPU address
 7000  EA00
 E900  E900
 EB00  EB00

   By default CPU PCIe window is at:E800 - F000
   Consider the case when TF-A moves it to: F200 - FA00

   Until now the function would take the PCI address of the first entry:
   7000, and the new base, F200, to compute the fix offset:
   F200 - 7000 = 8200, and then add 820 to all addresses,
   resulting in
 PCI address   CPU address
 F200  6C00
 6B00  6B00
 6D00  6D00
   which is complete nonsense - none of the CPU addresses is in the
   requested window.

   Now it will take the lowest CPU address, which is in second row,
   E900, and compute the fix offset F200 - E900 = 0900,
   and then add it to all CPU addresses and those PCI addresses which
   equal to their corresponding CPU addresses, resulting in
 PCI address   CPU address
 7000  F300
 F200  F200
 F400  F400
   where all of the CPU addresses are in the needed window.

Fixes: 4a82fca8e330 ("arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() 
function")
Signed-off-by: Pali Rohár 
Signed-off-by: Marek Behún 


Applied to u-boot-marvell/master

Thanks,
Stefan


---
  arch/arm/mach-mvebu/armada3700/cpu.c | 81 +++-
  1 file changed, 55 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c 
b/arch/arm/mach-mvebu/armada3700/cpu.c
index 23492f49da..52b5109b73 100644
--- a/arch/arm/mach-mvebu/armada3700/cpu.c
+++ b/arch/arm/mach-mvebu/armada3700/cpu.c
@@ -316,8 +316,8 @@ static int fdt_setprop_inplace_u32_partial(void *blob, int 
node,
  
  int a3700_fdt_fix_pcie_regions(void *blob)

  {
-   int acells, pacells, scells;
-   u32 base, fix_offset;
+   u32 base, lowest_cpu_addr, fix_offset;
+   int pci_cells, cpu_cells, size_cells;
const u32 *ranges;
int node, pnode;
int ret, i, len;
@@ -331,51 +331,80 @@ int a3700_fdt_fix_pcie_regions(void *blob)
return node;
  
  	ranges = fdt_getprop(blob, node, "ranges", );

-   if (!ranges || len % sizeof(u32))
-   return -ENOENT;
+   if (!ranges || !len || len % sizeof(u32))
+   return -EINVAL;
  
  	/*

 * The "ranges" property is an array of
-* {}
+*   {}
+* where number of PCI address cells and size cells is stored in the
+* "#address-cells" and "#size-cells" properties of the same node
+* containing the "ranges" property and number of CPU address cells
+* is stored in the parent's "#address-cells" property.
 *
-* All 3 elements can span a diffent number of cells. Fetch their sizes.
+* All 3 elements can span a diffent number of cells. Fetch them.
 */
pnode = fdt_parent_offset(blob, node);
-   acells = fdt_address_cells(blob, node);
-   pacells = fdt_address_cells(blob, pnode);
-   scells = fdt_size_cells(blob, node);
+   pci_cells = fdt_address_cells(blob, node);
+   cpu_cells = fdt_address_cells(blob, pnode);
+   size_cells = fdt_size_cells(blob, node);
  
-	/* Child PCI addresses always use 3 cells */

-   if (acells != 3)
-   return -ENOENT;
+   /* PCI addresses always use 3 cells */
+   if (pci_cells != 3)
+   return -EINVAL;
+
+   /* CPU addresses on Armada 37xx always use 2 cells */
+   if (cpu_cells != 2)
+   return -EINVAL;
+
+   for (i = 0; i < len / sizeof(u32);
+i += pci_cells + cpu_cells + size_cells) {
+ 

Re: [PATCH u-boot-marvell] arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() function again

2022-03-03 Thread Stefan Roese

On 2/23/22 13:52, Marek Behún wrote:

From: Pali Rohár 

The a3700_fdt_fix_pcie_regions() function still computes nonsense.

It computes the fixup offset from the PCI address taken from the first
row of the "ranges" array, which means that:
- PCI address must equal CPU address (otherwise the computed fix offset
   will be wrong),
- the first row must contain the lowest address.

This is the case for the default device-tree, which is why we didn't
notice it.

It also adds the fixup offset to all PCI and CPU addresses, which is
wrong.

Instead:
1) The fixup offset must be computed from the CPU address, not PCI
address.

2) The fixup offset must be computed from the row containing the lowest
CPU address, which is not necessarily contained in the first row.

3) The PCI address - the address to which the PCIe controller remaps the
address space as seen from the point of view of the PCIe device -
must be fixed by the fix offset in the same way as the CPU address
only in the special case when the CPU adn PCI addresses are the same.
Same addresses means that remapping is disabled, and thus if we
change the CPU address, we need also to change the PCI address so
that the remapping is still disabled afterwards.

Consider an example:
   The ranges entries contain:
 PCI address   CPU address
 7000  EA00
 E900  E900
 EB00  EB00

   By default CPU PCIe window is at:E800 - F000
   Consider the case when TF-A moves it to: F200 - FA00

   Until now the function would take the PCI address of the first entry:
   7000, and the new base, F200, to compute the fix offset:
   F200 - 7000 = 8200, and then add 820 to all addresses,
   resulting in
 PCI address   CPU address
 F200  6C00
 6B00  6B00
 6D00  6D00
   which is complete nonsense - none of the CPU addresses is in the
   requested window.

   Now it will take the lowest CPU address, which is in second row,
   E900, and compute the fix offset F200 - E900 = 0900,
   and then add it to all CPU addresses and those PCI addresses which
   equal to their corresponding CPU addresses, resulting in
 PCI address   CPU address
 7000  F300
 F200  F200
 F400  F400
   where all of the CPU addresses are in the needed window.

Fixes: 4a82fca8e330 ("arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() 
function")
Signed-off-by: Pali Rohár 
Signed-off-by: Marek Behún 


Reviewed-by: Stefan Roese 

Thanks,
Stefan


---
  arch/arm/mach-mvebu/armada3700/cpu.c | 81 +++-
  1 file changed, 55 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c 
b/arch/arm/mach-mvebu/armada3700/cpu.c
index 23492f49da..52b5109b73 100644
--- a/arch/arm/mach-mvebu/armada3700/cpu.c
+++ b/arch/arm/mach-mvebu/armada3700/cpu.c
@@ -316,8 +316,8 @@ static int fdt_setprop_inplace_u32_partial(void *blob, int 
node,
  
  int a3700_fdt_fix_pcie_regions(void *blob)

  {
-   int acells, pacells, scells;
-   u32 base, fix_offset;
+   u32 base, lowest_cpu_addr, fix_offset;
+   int pci_cells, cpu_cells, size_cells;
const u32 *ranges;
int node, pnode;
int ret, i, len;
@@ -331,51 +331,80 @@ int a3700_fdt_fix_pcie_regions(void *blob)
return node;
  
  	ranges = fdt_getprop(blob, node, "ranges", );

-   if (!ranges || len % sizeof(u32))
-   return -ENOENT;
+   if (!ranges || !len || len % sizeof(u32))
+   return -EINVAL;
  
  	/*

 * The "ranges" property is an array of
-* {}
+*   {}
+* where number of PCI address cells and size cells is stored in the
+* "#address-cells" and "#size-cells" properties of the same node
+* containing the "ranges" property and number of CPU address cells
+* is stored in the parent's "#address-cells" property.
 *
-* All 3 elements can span a diffent number of cells. Fetch their sizes.
+* All 3 elements can span a diffent number of cells. Fetch them.
 */
pnode = fdt_parent_offset(blob, node);
-   acells = fdt_address_cells(blob, node);
-   pacells = fdt_address_cells(blob, pnode);
-   scells = fdt_size_cells(blob, node);
+   pci_cells = fdt_address_cells(blob, node);
+   cpu_cells = fdt_address_cells(blob, pnode);
+   size_cells = fdt_size_cells(blob, node);
  
-	/* Child PCI addresses always use 3 cells */

-   if (acells != 3)
-   return -ENOENT;
+   /* PCI addresses always use 3 cells */
+   if (pci_cells != 3)
+   return -EINVAL;
+
+   /* CPU addresses on Armada 37xx always use 2 cells */
+   if (cpu_cells != 2)
+   return -EINVAL;
+
+   for (i = 0; i < len / sizeof(u32);
+i += pci_cells + cpu_cells + size_cells) {
+   

[PATCH u-boot-marvell] arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() function again

2022-02-23 Thread Marek Behún
From: Pali Rohár 

The a3700_fdt_fix_pcie_regions() function still computes nonsense.

It computes the fixup offset from the PCI address taken from the first
row of the "ranges" array, which means that:
- PCI address must equal CPU address (otherwise the computed fix offset
  will be wrong),
- the first row must contain the lowest address.

This is the case for the default device-tree, which is why we didn't
notice it.

It also adds the fixup offset to all PCI and CPU addresses, which is
wrong.

Instead:
1) The fixup offset must be computed from the CPU address, not PCI
   address.

2) The fixup offset must be computed from the row containing the lowest
   CPU address, which is not necessarily contained in the first row.

3) The PCI address - the address to which the PCIe controller remaps the
   address space as seen from the point of view of the PCIe device -
   must be fixed by the fix offset in the same way as the CPU address
   only in the special case when the CPU adn PCI addresses are the same.
   Same addresses means that remapping is disabled, and thus if we
   change the CPU address, we need also to change the PCI address so
   that the remapping is still disabled afterwards.

Consider an example:
  The ranges entries contain:
PCI address   CPU address
7000  EA00
E900  E900
EB00  EB00

  By default CPU PCIe window is at:E800 - F000
  Consider the case when TF-A moves it to: F200 - FA00

  Until now the function would take the PCI address of the first entry:
  7000, and the new base, F200, to compute the fix offset:
  F200 - 7000 = 8200, and then add 820 to all addresses,
  resulting in
PCI address   CPU address
F200  6C00
6B00  6B00
6D00  6D00
  which is complete nonsense - none of the CPU addresses is in the
  requested window.

  Now it will take the lowest CPU address, which is in second row,
  E900, and compute the fix offset F200 - E900 = 0900,
  and then add it to all CPU addresses and those PCI addresses which
  equal to their corresponding CPU addresses, resulting in
PCI address   CPU address
7000  F300
F200  F200
F400  F400
  where all of the CPU addresses are in the needed window.

Fixes: 4a82fca8e330 ("arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() 
function")
Signed-off-by: Pali Rohár 
Signed-off-by: Marek Behún 
---
 arch/arm/mach-mvebu/armada3700/cpu.c | 81 +++-
 1 file changed, 55 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-mvebu/armada3700/cpu.c 
b/arch/arm/mach-mvebu/armada3700/cpu.c
index 23492f49da..52b5109b73 100644
--- a/arch/arm/mach-mvebu/armada3700/cpu.c
+++ b/arch/arm/mach-mvebu/armada3700/cpu.c
@@ -316,8 +316,8 @@ static int fdt_setprop_inplace_u32_partial(void *blob, int 
node,
 
 int a3700_fdt_fix_pcie_regions(void *blob)
 {
-   int acells, pacells, scells;
-   u32 base, fix_offset;
+   u32 base, lowest_cpu_addr, fix_offset;
+   int pci_cells, cpu_cells, size_cells;
const u32 *ranges;
int node, pnode;
int ret, i, len;
@@ -331,51 +331,80 @@ int a3700_fdt_fix_pcie_regions(void *blob)
return node;
 
ranges = fdt_getprop(blob, node, "ranges", );
-   if (!ranges || len % sizeof(u32))
-   return -ENOENT;
+   if (!ranges || !len || len % sizeof(u32))
+   return -EINVAL;
 
/*
 * The "ranges" property is an array of
-* {}
+*   {}
+* where number of PCI address cells and size cells is stored in the
+* "#address-cells" and "#size-cells" properties of the same node
+* containing the "ranges" property and number of CPU address cells
+* is stored in the parent's "#address-cells" property.
 *
-* All 3 elements can span a diffent number of cells. Fetch their sizes.
+* All 3 elements can span a diffent number of cells. Fetch them.
 */
pnode = fdt_parent_offset(blob, node);
-   acells = fdt_address_cells(blob, node);
-   pacells = fdt_address_cells(blob, pnode);
-   scells = fdt_size_cells(blob, node);
+   pci_cells = fdt_address_cells(blob, node);
+   cpu_cells = fdt_address_cells(blob, pnode);
+   size_cells = fdt_size_cells(blob, node);
 
-   /* Child PCI addresses always use 3 cells */
-   if (acells != 3)
-   return -ENOENT;
+   /* PCI addresses always use 3 cells */
+   if (pci_cells != 3)
+   return -EINVAL;
+
+   /* CPU addresses on Armada 37xx always use 2 cells */
+   if (cpu_cells != 2)
+   return -EINVAL;
+
+   for (i = 0; i < len / sizeof(u32);
+i += pci_cells + cpu_cells + size_cells) {
+   /*
+* Parent CPU addresses on Armada 37xx are always 32-bit, so
+* check that