[U-Boot] [PATCH] fdt: __of_translate_address(): check parent's 'ranges' before translate

[Lukasz Majewski]

Hi Przemyslaw,

> The present implementation of __of_translate_address() taken
> from the Linux, is designed for translate bus/child address
> mappings by using 'ranges' property - and it doesn't allow
> for checking an address for a device's node with zero size-cells.
> 
> The 'size-cells > 0' is required for bus/child address mapping,
> but is not required for non-memory mapped address, e.g.: I2C chip.
> Then when we need only raw 'reg' property's value.
> 
> Since the I2C device address goes to a single-cell reg property,
> support for that case is welcome, but currently calling dev_get_addr()
> for I2C device will return 'FDT_ADDR_T_NONE', and print the warning:
> 
> warning:
> __of_translate_address: Bad cell count for 'some-dev'
> 
> The fix:
> The proper result by this commit, is achieved by skipping
> the condition check: 'size-cells > 0', when the parent doesn't
> provide the 'ranges' property and allows to return 1:1 address
> translation, if caller want's just the reg property's value.
> 
> No additional argument is needed, the 'ranges' property existence
> decides, what type of translation is done when calling dev_get_addr().
> And this should be, what the compatible driver expects.
> 
> Now, by this commit the function __of_translate_address() can be used
> for the both reg property use-cases:
> 
> Case 1: (ranges)
> ----------------
> some-bus {
> 	address-cells = <1>;
> 	size-cells = <1>;
> 	ranges = <0x0 0x10000000 0x1000>;
> 	reg = <0x10000000 0x1000>;
> 
> 	child1 {
> 		reg = <0xa00 0x100>;
> 	};
> 
> 	child2 {
> 		reg = <0xb00 0x100>;
> 	};
> };
> 
> Return values (CONFIG_OF_TRANSLATE=y):
>  - dev_get_addr(some-bus) - retrurns: 0x10000000 - correct
>  - dev_get_addr(child1)   - retrurns: 0x10000a00 - correct
>  - dev_get_addr(child2)   - retrurns: 0x10000b00 - correct
> This works as previous - this commit have no impact on this case.
> 
> Case 2: (no ranges - e.g. I2C bus childs) - fixed
> -------------------------------------------------
> I2C-bus {
> 	address-cells = <1>;
> 	size-cells = <0>;
> 	reg = <0x10000000>;
> 
> 	chip1 {
> 		reg = <0xa00>;
> 	};
> 
> 	chip2 {
> 		reg = <0xb00>;
> 	};
> };
> 
> Return values (CONFIG_OF_TRANSLATE=y):
>  - dev_get_addr(I2C-bus) - retrurns: 0x10000000 - correct
>  - dev_get_addr(chip1)   - retrurns: 0xa00      - correct
>  - dev_get_addr(chip2)   - retrurns: 0xb00      - correct
> 
> This is fixed, since the previously returned value for chip1 and chip2
> was: 0xffffffff, which means: 'FDT_ADDR_T_NONE'.
> 
> Signed-off-by: Przemyslaw Marczak <p.marczak at samsung.com>
> Cc: Stefan Roese <sr at denx.de>
> Cc: Tom Rini <trini at konsulko.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Stephen Warren <swarren at nvidia.com>
> Cc: Stephen Warren <swarren at wwwdotorg.org>
> ---
>  common/fdt_support.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 66464db..1f472ca 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -952,8 +952,9 @@ void fdt_del_node_and_alias(void *blob, const
> char *alias) /* Max address size we deal with */
>  #define OF_MAX_ADDR_CELLS	4
>  #define OF_BAD_ADDR	FDT_ADDR_T_NONE
> -#define OF_CHECK_COUNTS(na, ns)	((na) > 0 && (na) <=
> OF_MAX_ADDR_CELLS && \
> -			(ns) > 0)
> +#define OF_CHECK_COUNTS(na, ns, skip_ns) \
> +			((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
> +			(skip_ns || (ns) > 0))
>  
>  /* Debug utility */
>  #ifdef DEBUG
> @@ -1104,11 +1105,12 @@ static int of_translate_one(void * blob, int
> parent, struct of_bus *bus, static u64 __of_translate_address(void
> *blob, int node_offset, const fdt32_t *in_addr, const char *rprop)
>  {
> -	int parent;
> -	struct of_bus *bus, *pbus;
> +	const fdt32_t *pranges;
>  	fdt32_t addr[OF_MAX_ADDR_CELLS];
> -	int na, ns, pna, pns;
> +	int na, ns, pna, pns, parent;
> +	struct of_bus *bus, *pbus;
>  	u64 result = OF_BAD_ADDR;
> +	bool skip_ns_check;
>  
>  	debug("OF: ** translation for device %s **\n",
>  		fdt_get_name(blob, node_offset, NULL));
> @@ -1119,9 +1121,16 @@ static u64 __of_translate_address(void *blob,
> int node_offset, const fdt32_t *in goto bail;
>  	bus = &of_busses[0];
>  
> -	/* Cound address cells & copy address locally */
> +	/* Chek 'ns' only if parent provides 'ranges' property */
> +	pranges = fdt_getprop(blob, parent, rprop, NULL);
> +	if (pranges)
> +		skip_ns_check = false;
> +	else
> +		skip_ns_check = true;
> +
> +	/* Count address cells & copy address locally */
>  	bus->count_cells(blob, parent, &na, &ns);
> -	if (!OF_CHECK_COUNTS(na, ns)) {
> +	if (!OF_CHECK_COUNTS(na, ns, skip_ns_check)) {
>  		printf("%s: Bad cell count for %s\n", __FUNCTION__,
>  		       fdt_get_name(blob, node_offset, NULL));
>  		goto bail;
> @@ -1148,7 +1157,7 @@ static u64 __of_translate_address(void *blob,
> int node_offset, const fdt32_t *in /* Get new parent bus and counts */
>  		pbus = &of_busses[0];
>  		pbus->count_cells(blob, parent, &pna, &pns);
> -		if (!OF_CHECK_COUNTS(pna, pns)) {
> +		if (!OF_CHECK_COUNTS(pna, pns, skip_ns_check)) {
>  			printf("%s: Bad cell count for %s\n",
> __FUNCTION__, fdt_get_name(blob, node_offset, NULL));
>  			break;

Reviewed-by: Lukasz Majewski <l.majewski at samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group