Re: [PATCH v3 3/7] of: address: Add parent pointer to the __of_translate_address args

[Robin Murphy <robin.murphy@xxxxxxx>]

On 11/02/2019 15:02, Maxime Ripard wrote:
> The __of_translate_address function is used to translate the device tree
> addresses to physical addresses using the various ranges property to create
> the offset.
>
> However, it's shared between the CPU addresses (based on the ranges
> property) and the DMA addresses (based on dma-ranges). Since we're going to
> add support for a DMA parent node that is not the DT parent node, we need
> to change the logic a bit to have an optional parent node that we should
> use.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> ---
>   drivers/of/address.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 2270373b30ab..4c5dc21c71ca 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -569,10 +569,10 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>    * relative to that node.
>    */
>   static u64 __of_translate_address(struct device_node *dev,
> +				  struct device_node *parent,
>   				  const __be32 *in_addr, const char *rprop,
>   				  struct device_node **host)
>   {
> -	struct device_node *parent = NULL;
>   	struct of_bus *bus, *pbus;
>   	__be32 addr[OF_MAX_ADDR_CELLS];
>   	int na, ns, pna, pns;
> @@ -583,11 +583,14 @@ static u64 __of_translate_address(struct device_node *dev,
>   	/* Increase refcount at current level */
>   	of_node_get(dev);
>   
> -	*host = NULL;
> -	/* Get parent & match bus type */
> -	parent = of_get_parent(dev);
> -	if (parent == NULL)
> -		goto bail;
> +	if (!parent) {

I would have suggested having the callers make the of_get_parent() call 
themselves, but having pulled up the code for a look at the whole 
function, there's a bigger issue at play. The parent traversal happens 
within the main translation loop as well, and topologies could quite 
feasibly exist where that makes a difference, e.g.:

	icc {
		#interconnect-cells = <0>;
		dma-ranges = <...>;
	};

	subsystem {
		interconnects = <&icc>;
		interconnect-names = "dma";

		intermediate-bus {
			dma-ranges;

			device {
				...
			};
		};
	};

or:

	icc2 {
		#interconnect-cells = <0>;
		dma-ranges = <...>;
	};

	icc1 {
		#interconnect-cells = <0>;
		dma-ranges = <...>;
		interconnects = <&icc2>;
		interconnect-names = "dma";
	};

	device {
		interconnects = <&icc1>;
		interconnect-names = "dma";
		...
	};

So I guess the answer is to switch this over to using a callback and 
have the callers pass of_get_parent/of_get_dma_parent in as appropriate.

Robin.

> +		*host = NULL;
> +		/* Get parent & match bus type */
> +		parent = of_get_parent(dev);
> +		if (parent == NULL)
> +			goto bail;
> +	}
> +
>   	bus = of_match_bus(parent);
>   
>   	/* Count address cells & copy address locally */
> @@ -665,7 +668,7 @@ u64 of_translate_address(struct device_node *dev, const __be32 *in_addr)
>   	struct device_node *host;
>   	u64 ret;
>   
> -	ret = __of_translate_address(dev, in_addr, "ranges", &host);
> +	ret = __of_translate_address(dev, NULL, in_addr, "ranges", &host);
>   	if (host) {
>   		of_node_put(host);
>   		return OF_BAD_ADDR;
> @@ -680,7 +683,7 @@ u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr)
>   	struct device_node *host;
>   	u64 ret;
>   
> -	ret = __of_translate_address(dev, in_addr, "dma-ranges", &host);
> +	ret = __of_translate_address(dev, NULL, in_addr, "dma-ranges", &host);
>   
>   	if (host) {
>   		of_node_put(host);
> @@ -736,7 +739,7 @@ static u64 of_translate_ioport(struct device_node *dev, const __be32 *in_addr,
>   	unsigned long port;
>   	struct device_node *host;
>   
> -	taddr = __of_translate_address(dev, in_addr, "ranges", &host);
> +	taddr = __of_translate_address(dev, NULL, in_addr, "ranges", &host);
>   	if (host) {
>   		/* host-specific port access */
>   		port = logic_pio_trans_hwaddr(&host->fwnode, taddr, size);