[PATCH v4 0/3] PCI: dwc: opitimaze RC host pci_fixup_addr()

[PATCH v4 0/3] PCI: dwc: opitimaze RC host pci_fixup_addr()

[Frank Li]

┌─────────┐                    ┌────────────┐
 ┌─────┐    │         │ 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
                                           └────────────┘

Current dwc implimemnt, pci_fixup_addr() call back is needed when bus
fabric convert cpu address before send to PCIe controller.

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

Device tree already can descript all address translate. Some hardware
driver implement fixup function by mask some bits of cpu address. Last
pci-imx6.c are little bit better by fetch memory resource's offset to do
fixup.

static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
{
	...
	entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
	return cpu_addr - entry->offset;
}

But it is not good by using IORESOURCE_MEM to fix up io/cfg address map
although address translate is the same as IORESOURCE_MEM.

This patches to fetch untranslate range information for PCIe controller
(pcie@5f010000: ranges). So current config ATU without cpu_fixup_addr().

EP side patch:
https://lore.kernel.org/linux-pci/20240923-pcie_ep_range-v2-0-78d2ea434d9f@nxp.com/T/#mfc73ca113a69ad2c0294a2e629ecee3105b72973

The both pave the road to eliminate ugle cpu_fixup_addr() callback function.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v4:
- Improve commit message by add driver source code path.
- Link to v3: https://lore.kernel.org/r/20240930-pci_fixup_addr-v3-0-80ee70352fc7@nxp.com

Changes in v3:
- see each patch
- Link to v2: https://lore.kernel.org/r/20240926-pci_fixup_addr-v2-0-e4524541edf4@nxp.com

Changes in v2:
- see each patch
- Link to v1: https://lore.kernel.org/r/20240924-pci_fixup_addr-v1-0-57d14a91ec4f@nxp.com

---
Frank Li (3):
      of: address: Add parent_bus_addr to struct of_pci_range
      PCI: dwc: Using parent_bus_addr in of_range to eliminate cpu_addr_fixup()
      PCI: imx6: Remove cpu_addr_fixup()

 drivers/of/address.c                              |  2 ++
 drivers/pci/controller/dwc/pci-imx6.c             | 22 ++----------
 drivers/pci/controller/dwc/pcie-designware-host.c | 42 +++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h      |  8 +++++
 include/linux/of_address.h                        |  1 +
 5 files changed, 55 insertions(+), 20 deletions(-)
---
base-commit: 69940764dc1c429010d37cded159fadf1347d318
change-id: 20240924-pci_fixup_addr-a8568f9bbb34

Best regards,
---
Frank Li <Frank.Li@nxp.com>

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

[Frank Li]

Introduce field 'parent_bus_addr' in of_pci_range to retrieve untranslated
CPU address information.

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
                                           └────────────┘

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

'parent_bus_addr' in of_pci_range can indicate above diagram internal
address (IA) address information.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
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

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

[Bjorn Helgaas]

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@kernel.org>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> 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
> 

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

[Frank Li]

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@kernel.org>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > 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
> >

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

[Bjorn Helgaas]

On Thu, Oct 10, 2024 at 06:40:48PM -0400, Frank Li wrote:
> 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.

> 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

"parent_bus_addr" will hold an untranslated CPU address in some cases,
but not all.  I think it's better to use a generic term like "parent
bus addres" here because that is accurate in all cases.

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

Great, if we can omit 0x5f000000 completely that will avoid the
confusion.  I hope the actual DT doesn't have this error.

Bjorn

[PATCH v4 2/3] PCI: dwc: Using parent_bus_addr in of_range to eliminate cpu_addr_fixup()

[Frank Li]

parent_bus_addr in struct of_range can indicate address information just
ahead of PCIe controller. Most system's bus fabric use 1:1 map between
input and output address. but some hardware like i.MX8QXP doesn't use 1:1
map. See below diagram:

            ┌─────────┐                    ┌────────────┐
 ┌─────┐    │         │ 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
                                           └────────────┘

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

Term internal address (IA) here means the address just before PCIe
controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
be removed.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v3 to v4
- none

Change from v2 to v3
- %s/cpu_untranslate_addr/parent_bus_addr/g
- update diagram.
- improve commit message.

Change from v1 to v2
- update because patch1 change get untranslate address method.
- add using_dtbus_info in case break back compatibility for exited platform.
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 42 +++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h      |  8 +++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 3e41865c72904..823ff42c2e2c9 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -418,6 +418,34 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
 	}
 }
 
+static int dw_pcie_get_untranslate_addr(struct dw_pcie *pci, resource_size_t pci_addr,
+					resource_size_t *i_addr)
+{
+	struct device *dev = pci->dev;
+	struct device_node *np = dev->of_node;
+	struct of_range_parser parser;
+	struct of_range range;
+	int ret;
+
+	if (!pci->using_dtbus_info) {
+		*i_addr = pci_addr;
+		return 0;
+	}
+
+	ret = of_range_parser_init(&parser, np);
+	if (ret)
+		return ret;
+
+	for_each_of_pci_range(&parser, &range) {
+		if (pci_addr == range.bus_addr) {
+			*i_addr = range.parent_bus_addr;
+			break;
+		}
+	}
+
+	return 0;
+}
+
 int dw_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -427,6 +455,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	struct resource_entry *win;
 	struct pci_host_bridge *bridge;
 	struct resource *res;
+	int index;
 	int ret;
 
 	raw_spin_lock_init(&pp->lock);
@@ -440,6 +469,13 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 		pp->cfg0_size = resource_size(res);
 		pp->cfg0_base = res->start;
 
+		if (pci->using_dtbus_info) {
+			index = of_property_match_string(np, "reg-names", "config");
+			if (index < 0)
+				return -EINVAL;
+			of_property_read_reg(np, index, &pp->cfg0_base, NULL);
+		}
+
 		pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res);
 		if (IS_ERR(pp->va_cfg0_base))
 			return PTR_ERR(pp->va_cfg0_base);
@@ -462,6 +498,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 		pp->io_base = pci_pio_to_address(win->res->start);
 	}
 
+	if (dw_pcie_get_untranslate_addr(pci, pp->io_bus_addr, &pp->io_base))
+		return -ENODEV;
+
 	/* Set default bus ops */
 	bridge->ops = &dw_pcie_ops;
 	bridge->child_ops = &dw_child_pcie_ops;
@@ -733,6 +772,9 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 		atu.cpu_addr = entry->res->start;
 		atu.pci_addr = entry->res->start - entry->offset;
 
+		if (dw_pcie_get_untranslate_addr(pci, atu.pci_addr, &atu.cpu_addr))
+			return -EINVAL;
+
 		/* Adjust iATU size if MSG TLP region was allocated before */
 		if (pp->msg_res && pp->msg_res->parent == entry->res)
 			atu.size = resource_size(entry->res) -
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index c189781524fb8..e22d32b5a5f19 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -464,6 +464,14 @@ struct dw_pcie {
 	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
 	struct gpio_desc		*pe_rst;
 	bool			suspended;
+	/*
+	 * Use device tree 'ranges' property of bus node instead using
+	 * cpu_addr_fixup(). Some old platform dts 'ranges' in bus node may not
+	 * reflect real hardware's behavior. In case break these platform back
+	 * compatibility, add below flags. Set it true if dts already correct
+	 * indicate bus fabric address convert.
+	 */
+	bool			using_dtbus_info;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)

-- 
2.34.1

[PATCH v4 3/3] PCI: imx6: Remove cpu_addr_fixup()

[Frank Li]

Remove cpu_addr_fixup() because dwc common driver already handle address
translate.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v2 to v4
- none
Change from v1 to v2
- set using_dtbus_info true
---
 drivers/pci/controller/dwc/pci-imx6.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 1e58c24137e7f..94f3411352bf0 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -82,7 +82,6 @@ enum imx_pcie_variants {
 #define IMX_PCIE_FLAG_HAS_PHY_RESET		BIT(5)
 #define IMX_PCIE_FLAG_HAS_SERDES		BIT(6)
 #define IMX_PCIE_FLAG_SUPPORT_64BIT		BIT(7)
-#define IMX_PCIE_FLAG_CPU_ADDR_FIXUP		BIT(8)
 
 #define imx_check_flag(pci, val)	(pci->drvdata->flags & val)
 
@@ -1015,22 +1014,6 @@ static void imx_pcie_host_exit(struct dw_pcie_rp *pp)
 		regulator_disable(imx_pcie->vpcie);
 }
 
-static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
-{
-	struct imx_pcie *imx_pcie = to_imx_pcie(pcie);
-	struct dw_pcie_rp *pp = &pcie->pp;
-	struct resource_entry *entry;
-
-	if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_CPU_ADDR_FIXUP))
-		return cpu_addr;
-
-	entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
-	if (!entry)
-		return cpu_addr;
-
-	return cpu_addr - entry->offset;
-}
-
 static const struct dw_pcie_host_ops imx_pcie_host_ops = {
 	.init = imx_pcie_host_init,
 	.deinit = imx_pcie_host_exit,
@@ -1039,7 +1022,6 @@ static const struct dw_pcie_host_ops imx_pcie_host_ops = {
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.start_link = imx_pcie_start_link,
 	.stop_link = imx_pcie_stop_link,
-	.cpu_addr_fixup = imx_pcie_cpu_addr_fixup,
 };
 
 static void imx_pcie_ep_init(struct dw_pcie_ep *ep)
@@ -1459,6 +1441,7 @@ static int imx_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	pci->using_dtbus_info = true;
 	if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
 		ret = imx_add_pcie_ep(imx_pcie, pdev);
 		if (ret < 0)
@@ -1598,8 +1581,7 @@ static const struct imx_pcie_drvdata drvdata[] = {
 	},
 	[IMX8Q] = {
 		.variant = IMX8Q,
-		.flags = IMX_PCIE_FLAG_HAS_PHYDRV |
-			 IMX_PCIE_FLAG_CPU_ADDR_FIXUP,
+		.flags = IMX_PCIE_FLAG_HAS_PHYDRV,
 		.clk_names = imx8q_clks,
 		.clks_cnt = ARRAY_SIZE(imx8q_clks),
 	},

-- 
2.34.1