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

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

[Frank Li]

┌─────────┐                    ┌────────────┐
 ┌─────┐    │         │ IA: 0x8ff0_0000    │            │
 │ CPU ├───►│ BUS     ├─────────────────┐  │ PCI        │
 └─────┘    │         │ IA: 0x8ff8_0000 │  │            │
  CPU Addr  │ Fabric  ├─────────────┐   │  │ Controller │
0x7000_0000 │         │             │   │  │            │
            │         │             │   │  │            │   PCI Addr
            │         │             │   └──► CfgSpace  ─┼────────────►
            │         ├─────────┐   │      │            │    0
            │         │         │   │      │            │
            └─────────┘         │   └──────► IOSpace   ─┼────────────►
                                │          │            │    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>
---
Frank Li (3):
      of: address: Add helper function to get untranslated 'ranges' information
      PCI: dwc: Using for_each_of_range_untranslate to elminate cpu_addr_fixup()
      PCI: imx6: Remove cpu_addr_fixup()

 drivers/of/address.c                              | 33 ++++++++++++++++-------
 drivers/pci/controller/dwc/pci-imx6.c             | 21 +--------------
 drivers/pci/controller/dwc/pcie-designware-host.c | 33 +++++++++++++++++++++++
 include/linux/of_address.h                        |  9 ++++++-
 4 files changed, 65 insertions(+), 31 deletions(-)
---
base-commit: 69940764dc1c429010d37cded159fadf1347d318
change-id: 20240924-pci_fixup_addr-a8568f9bbb34

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

[PATCH 1/3] of: address: Add helper function to get untranslated 'ranges' information

[Frank Li]

Introduce `for_each_of_range_untranslate()` to retrieve untranslated CPU
address information, similar to `of_property_read_reg()`. This is required
for hardware like i.MX8QXP to configure the PCIe controller ATU and
eliminate the need for workaround address fixups in drivers. Currently,
many drivers use hardcoded CPU addresses for fixups, but this information
is already described in the Device Tree. With correct hardware
descriptions, such fixups can be removed.

            ┌─────────┐                    ┌────────────┐
 ┌─────┐    │         │ IA: 0x8ff0_0000    │            │
 │ CPU ├───►│ BUS     ├─────────────────┐  │ PCI        │
 └─────┘    │         │ IA: 0x8ff8_0000 │  │            │
  CPU Addr  │ Fabric  ├─────────────┐   │  │ Controller │
0x7000_0000 │         │             │   │  │            │
            │         │             │   │  │            │   PCI Addr
            │         │             │   └──► CfgSpace  ─┼────────────►
            │         ├─────────┐   │      │            │    0
            │         │         │   │      │            │
            └─────────┘         │   └──────► IOSpace   ─┼────────────►
                                │          │            │    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>;

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

Currently all function related 'range' return CPU address. THe new help
function for_each_of_range_untranslate() can get above diagram IA address
informaiton.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/of/address.c       | 33 +++++++++++++++++++++++----------
 include/linux/of_address.h |  9 ++++++++-
 2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 286f0c161e332..09c73936e573f 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -787,8 +787,9 @@ int of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
 EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
 #define of_dma_range_parser_init of_pci_dma_range_parser_init
 
-struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
-						struct of_pci_range *range)
+struct of_pci_range *of_pci_range_parser_one_common(struct of_pci_range_parser *parser,
+						    struct of_pci_range *range,
+						    bool translate)
 {
 	int na = parser->na;
 	int ns = parser->ns;
@@ -806,11 +807,13 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
 	range->bus_addr = of_read_number(parser->range + busflag_na, na - busflag_na);
 
 	if (parser->dma)
-		range->cpu_addr = of_translate_dma_address(parser->node,
-				parser->range + na);
+		range->cpu_addr = translate ? of_translate_dma_address(parser->node,
+						parser->range + na) :
+					      of_read_number(parser->range + na, parser->pna);
 	else
-		range->cpu_addr = of_translate_address(parser->node,
-				parser->range + na);
+		range->cpu_addr = translate ? of_translate_address(parser->node,
+						parser->range + na) :
+					      of_read_number(parser->range + na, parser->pna);
 	range->size = of_read_number(parser->range + parser->pna + na, ns);
 
 	parser->range += np;
@@ -823,11 +826,13 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
 		flags = parser->bus->get_flags(parser->range);
 		bus_addr = of_read_number(parser->range + busflag_na, na - busflag_na);
 		if (parser->dma)
-			cpu_addr = of_translate_dma_address(parser->node,
-					parser->range + na);
+			cpu_addr = translate ? of_translate_dma_address(parser->node,
+						parser->range + na) :
+					       of_read_number(parser->range + np, np);
 		else
-			cpu_addr = of_translate_address(parser->node,
-					parser->range + na);
+			cpu_addr = translate ? of_translate_address(parser->node,
+						parser->range + na) :
+					       of_read_number(parser->range + np, np);
 		size = of_read_number(parser->range + parser->pna + na, ns);
 
 		if (flags != range->flags)
@@ -842,6 +847,14 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
 
 	return range;
 }
+EXPORT_SYMBOL_GPL(of_pci_range_parser_one_common);
+
+struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
+					     struct of_pci_range *range)
+{
+	return of_pci_range_parser_one_common(parser, range, true);
+}
+
 EXPORT_SYMBOL_GPL(of_pci_range_parser_one);
 
 static u64 of_translate_ioport(struct device_node *dev, const __be32 *in_addr,
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 26a19daf0d092..692aae853217a 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -32,8 +32,11 @@ struct of_pci_range {
 #define of_range of_pci_range
 
 #define for_each_of_pci_range(parser, range) \
-	for (; of_pci_range_parser_one(parser, range);)
+	for (; of_pci_range_parser_one_common(parser, range, true);)
+#define for_each_of_pci_range_untranslate(parser, range) \
+	for (; of_pci_range_parser_one_common(parser, range, false);)
 #define for_each_of_range for_each_of_pci_range
+#define for_each_of_range_untranslate for_each_of_pci_range_untranslate
 
 /*
  * of_range_count - Get the number of "ranges" or "dma-ranges" entries
@@ -78,6 +81,10 @@ extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
 			struct device_node *node);
 extern int of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
 			struct device_node *node);
+extern struct of_pci_range *of_pci_range_parser_one_common(
+					struct of_pci_range_parser *parser,
+					struct of_pci_range *range,
+					bool translate);
 extern struct of_pci_range *of_pci_range_parser_one(
 					struct of_pci_range_parser *parser,
 					struct of_pci_range *range);

-- 
2.34.1

Re: [PATCH 1/3] of: address: Add helper function to get untranslated 'ranges' information

[Rob Herring]

On Tue, Sep 24, 2024 at 05:54:19PM -0400, Frank Li wrote:
> Introduce `for_each_of_range_untranslate()` to retrieve untranslated CPU
> address information, similar to `of_property_read_reg()`. This is required
> for hardware like i.MX8QXP to configure the PCIe controller ATU and
> eliminate the need for workaround address fixups in drivers. Currently,
> many drivers use hardcoded CPU addresses for fixups, but this information
> is already described in the Device Tree. With correct hardware
> descriptions, such fixups can be removed.

Just to be clear, you don't have to change the DT to add the 
intermediate bus? If you do, then that's an ABI issue.

> 
>             ┌─────────┐                    ┌────────────┐
>  ┌─────┐    │         │ IA: 0x8ff0_0000    │            │
>  │ CPU ├───►│ BUS     ├─────────────────┐  │ PCI        │
>  └─────┘    │         │ IA: 0x8ff8_0000 │  │            │
>   CPU Addr  │ Fabric  ├─────────────┐   │  │ Controller │
> 0x7000_0000 │         │             │   │  │            │
>             │         │             │   │  │            │   PCI Addr
>             │         │             │   └──► CfgSpace  ─┼────────────►
>             │         ├─────────┐   │      │            │    0
>             │         │         │   │      │            │
>             └─────────┘         │   └──────► IOSpace   ─┼────────────►
>                                 │          │            │    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>;
> 
>         pcieb: 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>;
> 	...
> 	};
> };
> 
> Currently all function related 'range' return CPU address. THe new help
> function for_each_of_range_untranslate() can get above diagram IA address
> informaiton.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/of/address.c       | 33 +++++++++++++++++++++++----------
>  include/linux/of_address.h |  9 ++++++++-
>  2 files changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 286f0c161e332..09c73936e573f 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -787,8 +787,9 @@ int of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
>  EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
>  #define of_dma_range_parser_init of_pci_dma_range_parser_init
>  
> -struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
> -						struct of_pci_range *range)
> +struct of_pci_range *of_pci_range_parser_one_common(struct of_pci_range_parser *parser,
> +						    struct of_pci_range *range,
> +						    bool translate)
>  {
>  	int na = parser->na;
>  	int ns = parser->ns;
> @@ -806,11 +807,13 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>  	range->bus_addr = of_read_number(parser->range + busflag_na, na - busflag_na);
>  
>  	if (parser->dma)
> -		range->cpu_addr = of_translate_dma_address(parser->node,
> -				parser->range + na);
> +		range->cpu_addr = translate ? of_translate_dma_address(parser->node,
> +						parser->range + na) :
> +					      of_read_number(parser->range + na, parser->pna);
>  	else
> -		range->cpu_addr = of_translate_address(parser->node,
> -				parser->range + na);
> +		range->cpu_addr = translate ? of_translate_address(parser->node,
> +						parser->range + na) :
> +					      of_read_number(parser->range + na, parser->pna);
>  	range->size = of_read_number(parser->range + parser->pna + na, ns);
>  
>  	parser->range += np;
> @@ -823,11 +826,13 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>  		flags = parser->bus->get_flags(parser->range);
>  		bus_addr = of_read_number(parser->range + busflag_na, na - busflag_na);
>  		if (parser->dma)
> -			cpu_addr = of_translate_dma_address(parser->node,
> -					parser->range + na);
> +			cpu_addr = translate ? of_translate_dma_address(parser->node,
> +						parser->range + na) :
> +					       of_read_number(parser->range + np, np);
>  		else
> -			cpu_addr = of_translate_address(parser->node,
> -					parser->range + na);
> +			cpu_addr = translate ? of_translate_address(parser->node,
> +						parser->range + na) :
> +					       of_read_number(parser->range + np, np);
>  		size = of_read_number(parser->range + parser->pna + na, ns);
>  
>  		if (flags != range->flags)
> @@ -842,6 +847,14 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>  
>  	return range;
>  }
> +EXPORT_SYMBOL_GPL(of_pci_range_parser_one_common);
> +
> +struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
> +					     struct of_pci_range *range)
> +{
> +	return of_pci_range_parser_one_common(parser, range, true);
> +}
> +
>  EXPORT_SYMBOL_GPL(of_pci_range_parser_one);
>  
>  static u64 of_translate_ioport(struct device_node *dev, const __be32 *in_addr,
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index 26a19daf0d092..692aae853217a 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -32,8 +32,11 @@ struct of_pci_range {
>  #define of_range of_pci_range
>  
>  #define for_each_of_pci_range(parser, range) \
> -	for (; of_pci_range_parser_one(parser, range);)
> +	for (; of_pci_range_parser_one_common(parser, range, true);)
> +#define for_each_of_pci_range_untranslate(parser, range) \
> +	for (; of_pci_range_parser_one_common(parser, range, false);)
>  #define for_each_of_range for_each_of_pci_range
> +#define for_each_of_range_untranslate for_each_of_pci_range_untranslate

You may want both the translated and untranslated address, so I would 
just add the untranslated address to the of_pci_range struct and return 
both with the existing iterator.

Rob

Re: [PATCH 1/3] of: address: Add helper function to get untranslated 'ranges' information

[Frank Li]

On Wed, Sep 25, 2024 at 09:26:10PM -0500, Rob Herring wrote:
> On Tue, Sep 24, 2024 at 05:54:19PM -0400, Frank Li wrote:
> > Introduce `for_each_of_range_untranslate()` to retrieve untranslated CPU
> > address information, similar to `of_property_read_reg()`. This is required
> > for hardware like i.MX8QXP to configure the PCIe controller ATU and
> > eliminate the need for workaround address fixups in drivers. Currently,
> > many drivers use hardcoded CPU addresses for fixups, but this information
> > is already described in the Device Tree. With correct hardware
> > descriptions, such fixups can be removed.
>
> Just to be clear, you don't have to change the DT to add the
> intermediate bus? If you do, then that's an ABI issue.

imx8q's pci dts is still not upstreamed. We are working on it. I think use
this way is more reasonable.

>
> >
> >             ┌─────────┐                    ┌────────────┐
> >  ┌─────┐    │         │ IA: 0x8ff0_0000    │            │
> >  │ CPU ├───►│ BUS     ├─────────────────┐  │ PCI        │
> >  └─────┘    │         │ IA: 0x8ff8_0000 │  │            │
> >   CPU Addr  │ Fabric  ├─────────────┐   │  │ Controller │
> > 0x7000_0000 │         │             │   │  │            │
> >             │         │             │   │  │            │   PCI Addr
> >             │         │             │   └──► CfgSpace  ─┼────────────►
> >             │         ├─────────┐   │      │            │    0
> >             │         │         │   │      │            │
> >             └─────────┘         │   └──────► IOSpace   ─┼────────────►
> >                                 │          │            │    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>;
> >
> >         pcieb: 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>;
> > 	...
> > 	};
> > };
> >
> > Currently all function related 'range' return CPU address. THe new help
> > function for_each_of_range_untranslate() can get above diagram IA address
> > informaiton.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/of/address.c       | 33 +++++++++++++++++++++++----------
> >  include/linux/of_address.h |  9 ++++++++-
> >  2 files changed, 31 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 286f0c161e332..09c73936e573f 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -787,8 +787,9 @@ int of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
> >  EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init);
> >  #define of_dma_range_parser_init of_pci_dma_range_parser_init
> >
> > -struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
> > -						struct of_pci_range *range)
> > +struct of_pci_range *of_pci_range_parser_one_common(struct of_pci_range_parser *parser,
> > +						    struct of_pci_range *range,
> > +						    bool translate)
> >  {
> >  	int na = parser->na;
> >  	int ns = parser->ns;
> > @@ -806,11 +807,13 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
> >  	range->bus_addr = of_read_number(parser->range + busflag_na, na - busflag_na);
> >
> >  	if (parser->dma)
> > -		range->cpu_addr = of_translate_dma_address(parser->node,
> > -				parser->range + na);
> > +		range->cpu_addr = translate ? of_translate_dma_address(parser->node,
> > +						parser->range + na) :
> > +					      of_read_number(parser->range + na, parser->pna);
> >  	else
> > -		range->cpu_addr = of_translate_address(parser->node,
> > -				parser->range + na);
> > +		range->cpu_addr = translate ? of_translate_address(parser->node,
> > +						parser->range + na) :
> > +					      of_read_number(parser->range + na, parser->pna);
> >  	range->size = of_read_number(parser->range + parser->pna + na, ns);
> >
> >  	parser->range += np;
> > @@ -823,11 +826,13 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
> >  		flags = parser->bus->get_flags(parser->range);
> >  		bus_addr = of_read_number(parser->range + busflag_na, na - busflag_na);
> >  		if (parser->dma)
> > -			cpu_addr = of_translate_dma_address(parser->node,
> > -					parser->range + na);
> > +			cpu_addr = translate ? of_translate_dma_address(parser->node,
> > +						parser->range + na) :
> > +					       of_read_number(parser->range + np, np);
> >  		else
> > -			cpu_addr = of_translate_address(parser->node,
> > -					parser->range + na);
> > +			cpu_addr = translate ? of_translate_address(parser->node,
> > +						parser->range + na) :
> > +					       of_read_number(parser->range + np, np);
> >  		size = of_read_number(parser->range + parser->pna + na, ns);
> >
> >  		if (flags != range->flags)
> > @@ -842,6 +847,14 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
> >
> >  	return range;
> >  }
> > +EXPORT_SYMBOL_GPL(of_pci_range_parser_one_common);
> > +
> > +struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
> > +					     struct of_pci_range *range)
> > +{
> > +	return of_pci_range_parser_one_common(parser, range, true);
> > +}
> > +
> >  EXPORT_SYMBOL_GPL(of_pci_range_parser_one);
> >
> >  static u64 of_translate_ioport(struct device_node *dev, const __be32 *in_addr,
> > diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> > index 26a19daf0d092..692aae853217a 100644
> > --- a/include/linux/of_address.h
> > +++ b/include/linux/of_address.h
> > @@ -32,8 +32,11 @@ struct of_pci_range {
> >  #define of_range of_pci_range
> >
> >  #define for_each_of_pci_range(parser, range) \
> > -	for (; of_pci_range_parser_one(parser, range);)
> > +	for (; of_pci_range_parser_one_common(parser, range, true);)
> > +#define for_each_of_pci_range_untranslate(parser, range) \
> > +	for (; of_pci_range_parser_one_common(parser, range, false);)
> >  #define for_each_of_range for_each_of_pci_range
> > +#define for_each_of_range_untranslate for_each_of_pci_range_untranslate
>
> You may want both the translated and untranslated address, so I would
> just add the untranslated address to the of_pci_range struct and return
> both with the existing iterator.

Yes, let me try it.

Frank

>
> Rob

[PATCH 2/3] PCI: dwc: Using for_each_of_range_untranslate to elminate cpu_addr_fixup()

[Frank Li]

for_each_of_range_untranslate() can get address informaiton just ahead of
PCIe controller. Most system it is the same as CPU address, but some
hardware like i.MX8QXP convert it to difference address by bus fabric. See
below diagram:

                ┌─────────┐                    ┌────────────┐
     ┌─────┐    │         │ IA: 0x8ff0_0000    │            │
     │ CPU ├───►│ BUS     ├─────────────────┐  │ PCI        │
     └─────┘    │         │ IA: 0x8ff8_0000 │  │            │
      CPU Addr  │ Fabric  ├─────────────┐   │  │ Controller │
    0x7000_0000 │         │             │   │  │            │
                │         │             │   │  │            │   PCI Addr
                │         │             │   └──► CfgSpace  ─┼────────────►
                │         ├─────────┐   │      │            │    0
                │         │         │   │      │            │
                └─────────┘         │   └──────► IOSpace   ─┼────────────►
                                    │          │            │    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 'IA' here means the address just before PCIe controle. After ATU use
this IA instead CPU address, cpu_addr_fixup() can be removed.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 33 +++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 3e41865c72904..68c679a2e1737 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -418,6 +418,27 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
 	}
 }
 
+static int dw_pcie_get_untranslate_addr(struct device_node *np, resource_size_t pci_addr,
+					resource_size_t *i_addr)
+{
+	struct of_range_parser parser;
+	struct of_range range;
+	int ret;
+
+	ret = of_range_parser_init(&parser, np);
+	if (ret)
+		return ret;
+
+	for_each_of_pci_range_untranslate(&parser, &range) {
+		if (pci_addr == range.bus_addr) {
+			*i_addr = range.cpu_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 +448,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 +462,11 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 		pp->cfg0_size = resource_size(res);
 		pp->cfg0_base = res->start;
 
+		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 +489,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(np, 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 +763,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->dev->of_node, 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) -

-- 
2.34.1

[PATCH 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>
---
 drivers/pci/controller/dwc/pci-imx6.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 1e58c24137e7f..76174b3a0388c 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)
@@ -1598,8 +1580,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

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

[Hongxing Zhu]

> -----Original Message-----
> From: Frank Li <frank.li@nxp.com>
> Sent: 2024年9月25日 5:54
> To: Rob Herring <robh@kernel.org>; Saravana Kannan
> <saravanak@google.com>; Jingoo Han <jingoohan1@gmail.com>; Manivannan
> Sadhasivam <manivannan.sadhasivam@linaro.org>; Lorenzo Pieralisi
> <lpieralisi@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas
> <bhelgaas@google.com>; Hongxing Zhu <hongxing.zhu@nxp.com>; Lucas
> Stach <l.stach@pengutronix.de>; Shawn Guo <shawnguo@kernel.org>; Sascha
> Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>
> Cc: devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: [PATCH 3/3] PCI: imx6: Remove cpu_addr_fixup()
> 
> Remove cpu_addr_fixup() because dwc common driver already handle address
> translate.
handle/s/handles 
translate/s/translation

Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>

Best Regards
Richard Zhu
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 21 +--------------------
>  1 file changed, 1 insertion(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 1e58c24137e7f..76174b3a0388c 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) @@ -1598,8 +1580,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