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

[Rob Herring <robh@kernel.org>]

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