Re: [PATCH v4 1/3] of: address: Add parent_bus_addr to struct of_pci_range
From: Bjorn Helgaas
Date: Thu Oct 10 2024 - 17:57:57 EST
On Tue, Oct 08, 2024 at 03:53:58PM -0400, Frank Li wrote:
> Introduce field 'parent_bus_addr' in of_pci_range to retrieve untranslated
> CPU address information.
s/in of_pci_range/in struct of_pci_range/
s/CPU address information/parent bus information/ ?
This patch adds "parent_bus_addr" (not "cpu_addr", which already
exists), and if I understand the example below correctly, it says
parent_bus_addr will be 0x8..._.... (an internal address), not a CPU
address, which would be 0x7..._....
I guess "parent_bus_addr" would be a CPU address for the bus@5f000000
ranges, but an IA address for the pcie@5f010000 ranges?
> Refer to the diagram below to understand that the bus fabric in some
> systems (like i.MX8QXP) does not use a 1:1 address map between input and
> output.
>
> Currently, many controller drivers use .cpu_addr_fixup() callback hardcodes
> that translation in the code, e.g., "cpu_addr & CDNS_PLAT_CPU_TO_BUS_ADDR"
> (drivers/pci/controller/cadence/pcie-cadence-plat.c),
> "cpu_addr + BUS_IATU_OFFSET"(drivers/pci/controller/dwc/pcie-intel-gw.c),
> etc, even though those translations *should* be described via DT.
>
> The .cpu_addr_fixup() can be eliminated if DT correct reflect hardware
> behavior and driver use 'parent_bus_addr' in of_pci_range.
>
> ┌─────────┐ ┌────────────┐
> ┌─────┐ │ │ IA: 0x8ff0_0000 │ │
> │ CPU ├───►│ ┌────►├─────────────────┐ │ PCI │
> └─────┘ │ │ │ IA: 0x8ff8_0000 │ │ │
> CPU Addr │ │ ┌─►├─────────────┐ │ │ Controller │
> 0x7ff0_0000─┼───┘ │ │ │ │ │ │
> │ │ │ │ │ │ │ PCI Addr
> 0x7ff8_0000─┼──────┘ │ │ └──► CfgSpace ─┼────────────►
> │ │ │ │ │ 0
> 0x7000_0000─┼────────►├─────────┐ │ │ │
> └─────────┘ │ └──────► IOSpace ─┼────────────►
> BUS Fabric │ │ │ 0
> │ │ │
> └──────────► MemSpace ─┼────────────►
> IA: 0x8000_0000 │ │ 0x8000_0000
> └────────────┘
Thanks for this diagram. I think it would be nice if the ranges were
in address order, e.g.,
0x7000_0000
0x7ff0_0000
0x7ff8_0000
(or the reverse). But it's a little confusing that 0x7ff8_0000 is in
the middle because that's the highest address range of the picture.
> bus@5f000000 {
> compatible = "simple-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0x5f000000 0x0 0x5f000000 0x21000000>,
> <0x80000000 0x0 0x70000000 0x10000000>;
>
> pcie@5f010000 {
> compatible = "fsl,imx8q-pcie";
> reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> reg-names = "dbi", "config";
> #address-cells = <3>;
> #size-cells = <2>;
> device_type = "pci";
> bus-range = <0x00 0xff>;
> ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
I'm still learning to interpret "ranges", so bear with me and help me
out a bit.
IIUC, "ranges" consists of (child-bus-address, parent-bus-address,
length) triplets. child-bus-address requires #address-cells of THIS
node, parent-bus-address requires #address-cells of the PARENT, and
length requires #size-cells of THIS node.
I guess bus@5f000000 "ranges" describes the translation from CPU to IA
addresses, so the triplet format would be:
(1-cell IA child addr, 2-cell CPU parent addr, 1-cell IA length)
and this "ranges":
ranges = <0x5f000000 0x0 0x5f000000 0x21000000>,
<0x80000000 0x0 0x70000000 0x10000000>;
means:
(IA 0x5f000000, CPU 0x0 0x5f000000, length 0x21000000)
(IA 0x80000000, CPU 0x0 0x70000000, length 0x10000000)
which would mean:
CPU 0x0_5f000000-0x0_7fffffff -> IA 0x5f000000-0x7fffffff
CPU 0x0_70000000-0x0_7fffffff -> IA 0x80000000-0x8fffffff
I must be misunderstanding something because this would mean CPU addr
0x70000000 would translate to IA addr 0x70000000 via the first range
and to IA addr 0x80000000 via the second range, which doesn't make
sense.
0x0_5f000000 doesn't appear in the diagram. If it's not relevant, can
you just omit it from the bus@5f000000 "ranges" and just say something
like this?
ranges = <0x80000000 0x0 0x70000000 0x10000000>, ...;
Then pcie@5f010000 describes the translations from IA to PCI bus
address? These triplets would be:
(3-cell PCI child addr, 1-cell IA parent addr, 2-cell PCI length)
ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
<0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
which would mean:
(IA 0x8ff80000, PCI 0x81000000 0 0x00000000, length 0 0x00010000)
(IA 0x80000000, PCI 0x82000000 0 0x80000000, length 0 0x0ff00000)
IA 0x8ff80000-0x8ff8ffff -> PCI 0x0_00000000-0x0_0x0008ffff (I/O)
IA 0x80000000-0x8fefffff -> PCI 0x0_80000000-0x0_0x8fefffff (32-bit mem)
The diagram shows the address translations for all three address
spaces (config, I/O, memory). If we ignore the 0x5f000000 range, the
mem and I/O paths through the diagram make sense to me:
CPU 0x0_7ff80000 -> IA 0x8ff80000 -> PCI 0x00000000 I/O
CPU 0x0_70000000 -> IA 0x80000000 -> PCI 0x0_80000000 mem
I guess config space handled separately from "ranges"? The diagram
suggests that it's something like this:
CPU 0x0_7ff00000 -> IA 0x8ff00000 -> PCI 0x00000000 config
which looks like it would match the "reg" property.
> };
> };
>
> 'parent_bus_addr' in of_pci_range can indicate above diagram internal
> address (IA) address information.
>
> Reviewed-by: Rob Herring (Arm) <robh@xxxxxxxxxx>
> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> ---
> Change from v3 to v4
> - improve commit message by driver source code path.
>
> Change from v2 to v3
> - cpu_untranslate_addr -> parent_bus_addr
> - Add Rob's review tag
> I changed commit message base on Bjorn, if you have concern about review
> added tag, let me know.
>
> Change from v1 to v2
> - add parent_bus_addr in of_pci_range, instead adding new API.
> ---
> drivers/of/address.c | 2 ++
> include/linux/of_address.h | 1 +
> 2 files changed, 3 insertions(+)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 286f0c161e332..1a0229ee4e0b2 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -811,6 +811,8 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
> else
> range->cpu_addr = of_translate_address(parser->node,
> parser->range + na);
> +
> + range->parent_bus_addr = of_read_number(parser->range + na, parser->pna);
> range->size = of_read_number(parser->range + parser->pna + na, ns);
>
> parser->range += np;
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index 26a19daf0d092..13dd79186d02c 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -26,6 +26,7 @@ struct of_pci_range {
> u64 bus_addr;
> };
> u64 cpu_addr;
> + u64 parent_bus_addr;
> u64 size;
> u32 flags;
> };
>
> --
> 2.34.1
>