On 2/23/22 13:52, Marek Behún wrote:
From: Pali Rohár <p...@kernel.org>
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
70000000 EA000000
E9000000 E9000000
EB000000 EB000000
By default CPU PCIe window is at: E8000000 - F0000000
Consider the case when TF-A moves it to: F2000000 - FA000000
Until now the function would take the PCI address of the first entry:
70000000, and the new base, F2000000, to compute the fix offset:
F2000000 - 70000000 = 82000000, and then add 8200000 to all addresses,
resulting in
PCI address CPU address
F2000000 6C000000
6B000000 6B000000
6D000000 6D000000
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,
E9000000, and compute the fix offset F2000000 - E9000000 = 09000000,
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
70000000 F3000000
F2000000 F2000000
F4000000 F4000000
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 <p...@kernel.org>
Signed-off-by: Marek Behún <marek.be...@nic.cz>
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", &len);
- if (!ranges || len % sizeof(u32))
- return -ENOENT;
+ if (!ranges || !len || len % sizeof(u32))
+ return -EINVAL;
/*
* The "ranges" property is an array of
- * { <child address> <parent address> <size in child address space> }
+ * { <PCI address> <CPU address> <size in PCI address space> }
+ * 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 the high word is zero.
+ */
+ if (fdt32_to_cpu(ranges[i + pci_cells]))
+ return -EINVAL;
+
+ if (i == 0 ||
+ fdt32_to_cpu(ranges[i + pci_cells + 1]) < lowest_cpu_addr)
+ lowest_cpu_addr = fdt32_to_cpu(ranges[i + pci_cells +
1]);
+ }
- /* Calculate fixup offset from first child address (in last cell) */
- fix_offset = base - fdt32_to_cpu(ranges[2]);
+ /* Calculate fixup offset from the lowest (first) CPU address */
+ fix_offset = base - lowest_cpu_addr;
- /* If fixup offset is zero then there is nothing to fix */
+ /* If fixup offset is zero there is nothing to fix */
if (!fix_offset)
return 0;
/*
- * Fix address (last cell) of each child address and each parent
- * address
+ * Fix each CPU address and corresponding PCI address if PCI address
+ * is not already remapped (has the same value)
*/
- for (i = 0; i < len / sizeof(u32); i += acells + pacells + scells) {
+ for (i = 0; i < len / sizeof(u32);
+ i += pci_cells + cpu_cells + size_cells) {
+ u32 cpu_addr;
+ u64 pci_addr;
int idx;
- /* fix child address */
- idx = i + acells - 1;
+ /* Fix CPU address */
+ idx = i + pci_cells + cpu_cells - 1;
+ cpu_addr = fdt32_to_cpu(ranges[idx]);
ret = fdt_setprop_inplace_u32_partial(blob, node, "ranges", idx,
- fdt32_to_cpu(ranges[idx])
+
- fix_offset);
+ cpu_addr + fix_offset);
if (ret)
return ret;
- /* fix parent address */
- idx = i + acells + pacells - 1;
+ /* Fix PCI address only if it isn't remapped (is same as CPU) */
+ idx = i + pci_cells - 1;
+ pci_addr = ((u64)fdt32_to_cpu(ranges[idx - 1]) << 32) |
+ fdt32_to_cpu(ranges[idx]);
+ if (cpu_addr != pci_addr)
+ continue;
+
ret = fdt_setprop_inplace_u32_partial(blob, node, "ranges", idx,
- fdt32_to_cpu(ranges[idx])
+
- fix_offset);
+ cpu_addr + fix_offset);
if (ret)
return ret;
}
Viele Grüße,
Stefan Roese
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: s...@denx.de
