Re: [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 08/02/2023 12:05, Ayan Kumar Halder wrote:
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 6c9712ab7b..d0f19d5cfc 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -934,8 +934,9 @@ bail:
>   }
>   
>   /* dt_device_address - Translate device tree address and return it */
> -int dt_device_get_address(const struct dt_device_node *dev, unsigned int index,
> -                          u64 *addr, u64 *size)
> +static int dt_device_get_address(const struct dt_device_node *dev,
> +                                 unsigned int index,
> +                                 u64 *addr, u64 *size)
>   {
>       const __be32 *addrp;
>       unsigned int flags;
> @@ -955,6 +956,37 @@ int dt_device_get_address(const struct dt_device_node 
> *dev, unsigned int index,
>       return 0;
>   }
>   
> +int dt_device_get_paddr(const struct dt_device_node *dev, unsigned int index,
> +                        paddr_t *addr, paddr_t *size)
> +{
> +    u64 dt_addr = 0, dt_size = 0;
Please use uint64_t for new code.

> +    int ret;
> +
> +    ret = dt_device_get_address(dev, index, &dt_addr, &dt_size);
> +
> +    if ( addr )
> +    {
> +        if ( (dt_addr >> (PADDR_SHIFT - 1)) > 1 )
> +        {
> +            printk("Error: Physical address greater than max width 
> supported\n");
I would print the width, address and the node name to help debugging.

> +            return -EINVAL;
NIT: -EINVAL tends to be quite overloaded. How about using -ERANGE?

> +        }
> +
> +        *addr = dt_addr;
> +    }
> +
> +    if ( size )
> +    {
> +        if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
> +        {
> +            printk("Error: Physical size greater than max width supported\n");
Same here for the message but with s/address/size/.


> +            return -EINVAL;
Same here for the errno.

The rest looks fine so long Stefano's comment are addressed.

> +        }
> +        *size = dt_size;
> +    }
> +
> +    return ret;
> +}
Cheers,

--
Julien Grall