[PATCH u-boot-marvell] arm: a37xx: pci: Fix a3700_fdt_fix_pcie_regions() function again
Stefan Roese
sr at denx.de
Fri Mar 4 08:26:52 CET 2022
On 2/23/22 13:52, Marek Behún wrote:
> From: Pali Rohár <pali at 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 <pali at kernel.org>
> Signed-off-by: Marek Behún <marek.behun at nic.cz>
Reviewed-by: Stefan Roese <sr at denx.de>
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: sr at denx.de
More information about the U-Boot
mailing list