Re: [PATCH v4 1/3] of: address: Add parent_bus_addr to struct of_pci_range

From: Frank Li
Date: Thu Oct 10 2024 - 18:41:11 EST

On Thu, Oct 10, 2024 at 04:57:45PM -0500, Bjorn Helgaas wrote:
> 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..._....

It is "untranslated CPU address", previous patch use cpu_untranslate_addr.
Rob suggest change to parent_bus_addr.

Is it better change to "to retrieve the address at bus fabric port" instead
of *untranslated* CPU address

>
> I guess "parent_bus_addr" would be a CPU address for the bus@5f000000
> ranges, but an IA address for the pcie@5f010000 ranges?

simple said "parent_bus_addr" should be address under pcie dt node.
5f000000 should be 1:1 map.
Only 80000000 range is not 1:1 map.

>
> > 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.

Okay, reverse should be simple.

>
> > 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)
> m
> 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

Yes,

>
> 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.

Yes, it is my mistake, first length should reduce to 0x0100_0000 from
0x21000000. It works because dt convert IA to CPU, instead of CPU to
IA. for example, input IA: 0x80000000, match second one, convert to
CPU address 0x0_70000000.

>
> 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>, ...;

Yes.

>
> 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:

Yes, history reason, dwc's config space is not in ranges.

>
> CPU 0x0_7ff00000 -> IA 0x8ff00000 -> PCI 0x00000000 config
>
> which looks like it would match the "reg" property.

regname = "config"

Frank
>
> > };
> > };
> >
> > '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
> >