Re: fdt: translate memory addresses

[Mark Kettenis]

> Date: Tue, 5 Apr 2016 18:11:03 +0200
> From: Patrick Wildt 
> 
> Updated to include feedback.
> 
> ok?

Looks good to me; ok kettenis@

> diff --git sys/dev/ofw/fdt.c sys/dev/ofw/fdt.c
> index 4cebbb4..09ddcfc 100644
> --- sys/dev/ofw/fdt.c
> +++ sys/dev/ofw/fdt.c
> @@ -34,6 +34,7 @@ void*skip_node(void *);
>  void *fdt_parent_node_recurse(void *, void *);
>  int   fdt_node_property_int(void *, char *, int *);
>  int   fdt_node_property_ints(void *, char *, int *, int);
> +int   fdt_translate_memory_address(void *, struct fdt_memory *);
>  #ifdef DEBUG
>  void  fdt_print_node_recurse(void *, int);
>  #endif
> @@ -390,6 +391,105 @@ fdt_parent_node(void *node)
>  }
>  
>  /*
> + * Translate memory address depending on parent's range.
> + *
> + * Ranges are a way of mapping one address to another.  This ranges attribute
> + * is set on a node's parent.  This means if a node does not have a parent,
> + * there's nothing to translate.  If it does have a parent and the parent 
> does
> + * not have a ranges attribute, there's nothing to translate either.
> + *
> + * If the parent has a ranges attribute and the attribute is not empty, the
> + * node's memory address has to be in one of the given ranges.  This range is
> + * then used to translate the memory address.
> + *
> + * If the parent has a ranges attribute, but the attribute is empty, there's
> + * nothing to translate.  But it's not a translation barrier.  It can be 
> treated
> + * as a simple 1:1 mapping.
> + *
> + * Translation does not end here.  We need to check if the parent's parent 
> also
> + * has a ranges attribute and ask the same questions again.
> + */
> +int
> +fdt_translate_memory_address(void *node, struct fdt_memory *mem)
> +{
> + void *parent;
> + int pac, psc, ac, sc, ret, rlen, rone, *range;
> + uint64_t from, to, size;
> +
> + /* No parent, no translation. */
> + parent = fdt_parent_node(node);
> + if (parent == NULL)
> + return 0;
> +
> + /* Extract ranges property from node. */
> + rlen = fdt_node_property(node, "ranges", (char **)) / sizeof(int);
> +
> + /* No ranges means translation barrier. Translation stops here. */
> + if (range == NULL)
> + return 0;
> +
> + /* Empty ranges means 1:1 mapping. Continue translation on parent. */
> + if (rlen <= 0)
> + return fdt_translate_memory_address(parent, mem);
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide addresses here. */
> + ret = fdt_node_property_int(parent, "#address-cells", );
> + if (ret != 1 || pac <= 0 || pac > 2)
> + return EINVAL;
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide sizes here. */
> + ret = fdt_node_property_int(parent, "#size-cells", );
> + if (ret != 1 || psc <= 0 || psc > 2)
> + return EINVAL;
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide addresses here. */
> + ret = fdt_node_property_int(node, "#address-cells", );
> + if (ret <= 0)
> + ac = pac;
> + else if (ret > 1 || ac <= 0 || ac > 2)
> + return EINVAL;
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide sizes here. */
> + ret = fdt_node_property_int(node, "#size-cells", );
> + if (ret <= 0)
> + sc = psc;
> + else if (ret > 1 || sc <= 0 || sc > 2)
> + return EINVAL;
> +
> + /* Must have at least one range. */
> + rone = pac + ac + sc;
> + if (rlen < rone)
> + return ESRCH;
> +
> + /* For each range. */
> + for (; rlen >= rone; rlen -= rone, range += rone) {
> + /* Extract from and size, so we can see if we fit. */
> + from = betoh32(range[0]);
> + if (ac == 2)
> + from = (from << 32) + betoh32(range[1]);
> + size = betoh32(range[ac + pac]);
> + if (sc == 2)
> + size = (size << 32) + betoh32(range[ac + pac + 1]);
> +
> + /* Try next, if we're not in the range. */
> + if (mem->addr < from || (mem->addr + mem->size) > (from + size))
> + continue;
> +
> + /* All good, extract to address and translate. */
> + to = betoh32(range[ac]);
> + if (pac == 2)
> + to = (to << 32) + betoh32(range[ac + 1]);
> +
> + mem->addr -= from;
> + mem->addr += to;
> + return fdt_translate_memory_address(parent, mem);
> + }
> +
> + /* To be successful, we must have returned in the for-loop. */
> + return ESRCH;
> +}
> +
> +/*
>   * Parse the memory address and size of a node.
>   */
>  int
> @@ -429,10 +529,7 @@ fdt_get_memory_address(void *node, int idx, struct 
> fdt_memory *mem)
>   if (sc == 2)
>   mem->size = (mem->size << 32) + betoh32(in[off + ac + 1]);
>  
> - /* TODO: translate memory address in ranges */
> - //return 

Re: fdt: translate memory addresses

[Patrick Wildt]

On Sun, Apr 03, 2016 at 06:56:52PM +0200, Mark Kettenis wrote:
> > Date: Sun, 3 Apr 2016 16:57:10 +0200
> > From: Patrick Wildt 
> > 
> > Hi,
> > 
> > now we're able to get a node's memory address.  Though, a device tree
> > may implement so called ranges.  Those ranges are used to translate from
> > one address space to another.
> > 
> > This is used on a few machines, for instance on the raspberry pi:
> > 
> > / {
> >#address-cells = <0x1>;
> >#size-cells = <0x1>;
> >interrupt-parent = <0x1>;
> >compatible = "brcm,bcm2710", "brcm,bcm2709";
> >model = "Raspberry Pi 3 Model B";
> > [...]
> >soc {
> >compatible = "simple-bus";
> >#address-cells = <0x1>;
> >#size-cells = <0x1>;
> >ranges = <0x7e00 0x3f00 0x100>;
> > [...]
> >interrupt-controller@7e00b200 {
> >compatible = "brcm,bcm2708-armctrl-ic";
> >reg = <0x7e00b200 0x200>;
> >interrupt-controller;
> >#interrupt-cells = <0x2>;
> >linux,phandle = <0x1>;
> >phandle = <0x1>;
> >};
> > 
> > Even though the node's reg is set to 0x7e00b200, the actual address is
> > 0x3f00b200.  To get to that address, we need to check the parent's
> > ranges attribute.
> > 
> > Since I last posted this diff in another thread I have added an
> > explanatory comment before the function and improved a few comments
> > inline.
> > 
> > ok?
> 
> Is the 
> 
> > +   if (node == NULL || mem == NULL)
> 
> check really necessary?
> 
> Also I wonder if it would make sense to return an errno value upon
> failure instead of 1.  That makes it immediately obvious that the
> function returns 0 upon success.
> 
> > diff --git sys/dev/ofw/fdt.c sys/dev/ofw/fdt.c
> > index c430a1e..8990afc 100644
> > --- sys/dev/ofw/fdt.c
> > +++ sys/dev/ofw/fdt.c
> > @@ -34,6 +34,7 @@ void  *skip_node(void *);
> >  void   *fdt_parent_node_recurse(void *, void *);
> >  int fdt_node_property_int(void *, char *, int *);
> >  int fdt_node_property_ints(void *, char *, int *, int);
> > +int fdt_translate_memory_address(void *, struct fdt_memory *);
> >  #ifdef DEBUG
> >  voidfdt_print_node_recurse(void *, int);
> >  #endif
> > @@ -390,6 +391,108 @@ fdt_parent_node(void *node)
> >  }
> >  
> >  /*
> > + * Translate memory address depending on parent's range.
> > + *
> > + * Ranges are a way of mapping one address to another.  This ranges 
> > attribute
> > + * is set on a node's parent.  This means if a node does not have a parent,
> > + * there's nothing to translate.  If it does have a parent and the parent 
> > does
> > + * not have a ranges attribute, there's nothing to translate either.
> > + *
> > + * If the parent has a ranges attribute and the attribute is not empty, the
> > + * node's memory address has to be in one of the given ranges.  This range 
> > is
> > + * then used to translate the memory address.
> > + *
> > + * If the parent has a ranges attribute, but the attribute is empty, 
> > there's
> > + * nothing to translate.  But it's not a translation barrier.  It can be 
> > treated
> > + * as a simple 1:1 mapping.
> > + *
> > + * Translation does not end here.  We need to check if the parent's parent 
> > also
> > + * has a ranges attribute and ask the same questions again.
> > + */
> > +int
> > +fdt_translate_memory_address(void *node, struct fdt_memory *mem)
> > +{
> > +   void *parent;
> > +   int pac, psc, ac, sc, ret, rlen, rone, *range;
> > +   uint64_t from, to, size;
> > +
> > +   if (node == NULL || mem == NULL)
> > +   return 1;
> > +
> > +   /* No parent, no translation. */
> > +   parent = fdt_parent_node(node);
> > +   if (parent == NULL)
> > +   return 0;
> > +
> > +   /* Extract ranges property from node. */
> > +   rlen = fdt_node_property(node, "ranges", (char **)) / sizeof(int);
> > +
> > +   /* No ranges means translation barrier. Translation stops here. */
> > +   if (range == NULL)
> > +   return 0;
> > +
> > +   /* Empty ranges means 1:1 mapping. Continue translation on parent. */
> > +   if (rlen <= 0)
> > +   return fdt_translate_memory_address(parent, mem);
> > +
> > +   /* We only support 32-bit (1), and 64-bit (2) wide addresses here. */
> > +   ret = fdt_node_property_int(parent, "#address-cells", );
> > +   if (ret != 1 || pac <= 0 || pac > 2)
> > +   return 1;
> > +
> > +   /* We only support 32-bit (1), and 64-bit (2) wide sizes here. */
> > +   ret = fdt_node_property_int(parent, "#size-cells", );
> > +   if (ret != 1 || psc <= 0 || psc > 2)
> > +   return 1;
> > +
> > +   /* We only support 32-bit (1), and 64-bit (2) wide addresses here. */
> > +   ret = fdt_node_property_int(node, "#address-cells", );
> > +   if (ret <= 0)
> > +   ac = pac;
> > +   else if 

Re: fdt: translate memory addresses

[Patrick Wildt]

On Sun, Apr 03, 2016 at 06:56:52PM +0200, Mark Kettenis wrote:
> > Date: Sun, 3 Apr 2016 16:57:10 +0200
> > From: Patrick Wildt 
> > 
> > Hi,
> > 
> > now we're able to get a node's memory address.  Though, a device tree
> > may implement so called ranges.  Those ranges are used to translate from
> > one address space to another.
> > 
> > This is used on a few machines, for instance on the raspberry pi:
> > 
> > / {
> >#address-cells = <0x1>;
> >#size-cells = <0x1>;
> >interrupt-parent = <0x1>;
> >compatible = "brcm,bcm2710", "brcm,bcm2709";
> >model = "Raspberry Pi 3 Model B";
> > [...]
> >soc {
> >compatible = "simple-bus";
> >#address-cells = <0x1>;
> >#size-cells = <0x1>;
> >ranges = <0x7e00 0x3f00 0x100>;
> > [...]
> >interrupt-controller@7e00b200 {
> >compatible = "brcm,bcm2708-armctrl-ic";
> >reg = <0x7e00b200 0x200>;
> >interrupt-controller;
> >#interrupt-cells = <0x2>;
> >linux,phandle = <0x1>;
> >phandle = <0x1>;
> >};
> > 
> > Even though the node's reg is set to 0x7e00b200, the actual address is
> > 0x3f00b200.  To get to that address, we need to check the parent's
> > ranges attribute.
> > 
> > Since I last posted this diff in another thread I have added an
> > explanatory comment before the function and improved a few comments
> > inline.
> > 
> > ok?
> 
> Is the 
> 
> > +   if (node == NULL || mem == NULL)
> 
> check really necessary?

Actually, no, it's not.  It will never be called with either of
those set to NULL.  I will remove that check.

> 
> Also I wonder if it would make sense to return an errno value upon
> failure instead of 1.  That makes it immediately obvious that the
> function returns 0 upon success.

Sure, sounds good.  Do you have one in mind that would match here?
Maybe EINVAL? ENOTSUP?

> 
> > diff --git sys/dev/ofw/fdt.c sys/dev/ofw/fdt.c
> > index c430a1e..8990afc 100644
> > --- sys/dev/ofw/fdt.c
> > +++ sys/dev/ofw/fdt.c
> > @@ -34,6 +34,7 @@ void  *skip_node(void *);
> >  void   *fdt_parent_node_recurse(void *, void *);
> >  int fdt_node_property_int(void *, char *, int *);
> >  int fdt_node_property_ints(void *, char *, int *, int);
> > +int fdt_translate_memory_address(void *, struct fdt_memory *);
> >  #ifdef DEBUG
> >  voidfdt_print_node_recurse(void *, int);
> >  #endif
> > @@ -390,6 +391,108 @@ fdt_parent_node(void *node)
> >  }
> >  
> >  /*
> > + * Translate memory address depending on parent's range.
> > + *
> > + * Ranges are a way of mapping one address to another.  This ranges 
> > attribute
> > + * is set on a node's parent.  This means if a node does not have a parent,
> > + * there's nothing to translate.  If it does have a parent and the parent 
> > does
> > + * not have a ranges attribute, there's nothing to translate either.
> > + *
> > + * If the parent has a ranges attribute and the attribute is not empty, the
> > + * node's memory address has to be in one of the given ranges.  This range 
> > is
> > + * then used to translate the memory address.
> > + *
> > + * If the parent has a ranges attribute, but the attribute is empty, 
> > there's
> > + * nothing to translate.  But it's not a translation barrier.  It can be 
> > treated
> > + * as a simple 1:1 mapping.
> > + *
> > + * Translation does not end here.  We need to check if the parent's parent 
> > also
> > + * has a ranges attribute and ask the same questions again.
> > + */
> > +int
> > +fdt_translate_memory_address(void *node, struct fdt_memory *mem)
> > +{
> > +   void *parent;
> > +   int pac, psc, ac, sc, ret, rlen, rone, *range;
> > +   uint64_t from, to, size;
> > +
> > +   if (node == NULL || mem == NULL)
> > +   return 1;
> > +
> > +   /* No parent, no translation. */
> > +   parent = fdt_parent_node(node);
> > +   if (parent == NULL)
> > +   return 0;
> > +
> > +   /* Extract ranges property from node. */
> > +   rlen = fdt_node_property(node, "ranges", (char **)) / sizeof(int);
> > +
> > +   /* No ranges means translation barrier. Translation stops here. */
> > +   if (range == NULL)
> > +   return 0;
> > +
> > +   /* Empty ranges means 1:1 mapping. Continue translation on parent. */
> > +   if (rlen <= 0)
> > +   return fdt_translate_memory_address(parent, mem);
> > +
> > +   /* We only support 32-bit (1), and 64-bit (2) wide addresses here. */
> > +   ret = fdt_node_property_int(parent, "#address-cells", );
> > +   if (ret != 1 || pac <= 0 || pac > 2)
> > +   return 1;
> > +
> > +   /* We only support 32-bit (1), and 64-bit (2) wide sizes here. */
> > +   ret = fdt_node_property_int(parent, "#size-cells", );
> > +   if (ret != 1 || psc <= 0 || psc > 2)
> > +   return 1;
> > +
> > 

Re: fdt: translate memory addresses

[Mark Kettenis]

> Date: Sun, 3 Apr 2016 16:57:10 +0200
> From: Patrick Wildt 
> 
> Hi,
> 
> now we're able to get a node's memory address.  Though, a device tree
> may implement so called ranges.  Those ranges are used to translate from
> one address space to another.
> 
> This is used on a few machines, for instance on the raspberry pi:
> 
> / {
>#address-cells = <0x1>;
>#size-cells = <0x1>;
>interrupt-parent = <0x1>;
>compatible = "brcm,bcm2710", "brcm,bcm2709";
>model = "Raspberry Pi 3 Model B";
> [...]
>soc {
>compatible = "simple-bus";
>#address-cells = <0x1>;
>#size-cells = <0x1>;
>ranges = <0x7e00 0x3f00 0x100>;
> [...]
>interrupt-controller@7e00b200 {
>compatible = "brcm,bcm2708-armctrl-ic";
>reg = <0x7e00b200 0x200>;
>interrupt-controller;
>#interrupt-cells = <0x2>;
>linux,phandle = <0x1>;
>phandle = <0x1>;
>};
> 
> Even though the node's reg is set to 0x7e00b200, the actual address is
> 0x3f00b200.  To get to that address, we need to check the parent's
> ranges attribute.
> 
> Since I last posted this diff in another thread I have added an
> explanatory comment before the function and improved a few comments
> inline.
> 
> ok?

Is the 

> + if (node == NULL || mem == NULL)

check really necessary?

Also I wonder if it would make sense to return an errno value upon
failure instead of 1.  That makes it immediately obvious that the
function returns 0 upon success.

> diff --git sys/dev/ofw/fdt.c sys/dev/ofw/fdt.c
> index c430a1e..8990afc 100644
> --- sys/dev/ofw/fdt.c
> +++ sys/dev/ofw/fdt.c
> @@ -34,6 +34,7 @@ void*skip_node(void *);
>  void *fdt_parent_node_recurse(void *, void *);
>  int   fdt_node_property_int(void *, char *, int *);
>  int   fdt_node_property_ints(void *, char *, int *, int);
> +int   fdt_translate_memory_address(void *, struct fdt_memory *);
>  #ifdef DEBUG
>  void  fdt_print_node_recurse(void *, int);
>  #endif
> @@ -390,6 +391,108 @@ fdt_parent_node(void *node)
>  }
>  
>  /*
> + * Translate memory address depending on parent's range.
> + *
> + * Ranges are a way of mapping one address to another.  This ranges attribute
> + * is set on a node's parent.  This means if a node does not have a parent,
> + * there's nothing to translate.  If it does have a parent and the parent 
> does
> + * not have a ranges attribute, there's nothing to translate either.
> + *
> + * If the parent has a ranges attribute and the attribute is not empty, the
> + * node's memory address has to be in one of the given ranges.  This range is
> + * then used to translate the memory address.
> + *
> + * If the parent has a ranges attribute, but the attribute is empty, there's
> + * nothing to translate.  But it's not a translation barrier.  It can be 
> treated
> + * as a simple 1:1 mapping.
> + *
> + * Translation does not end here.  We need to check if the parent's parent 
> also
> + * has a ranges attribute and ask the same questions again.
> + */
> +int
> +fdt_translate_memory_address(void *node, struct fdt_memory *mem)
> +{
> + void *parent;
> + int pac, psc, ac, sc, ret, rlen, rone, *range;
> + uint64_t from, to, size;
> +
> + if (node == NULL || mem == NULL)
> + return 1;
> +
> + /* No parent, no translation. */
> + parent = fdt_parent_node(node);
> + if (parent == NULL)
> + return 0;
> +
> + /* Extract ranges property from node. */
> + rlen = fdt_node_property(node, "ranges", (char **)) / sizeof(int);
> +
> + /* No ranges means translation barrier. Translation stops here. */
> + if (range == NULL)
> + return 0;
> +
> + /* Empty ranges means 1:1 mapping. Continue translation on parent. */
> + if (rlen <= 0)
> + return fdt_translate_memory_address(parent, mem);
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide addresses here. */
> + ret = fdt_node_property_int(parent, "#address-cells", );
> + if (ret != 1 || pac <= 0 || pac > 2)
> + return 1;
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide sizes here. */
> + ret = fdt_node_property_int(parent, "#size-cells", );
> + if (ret != 1 || psc <= 0 || psc > 2)
> + return 1;
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide addresses here. */
> + ret = fdt_node_property_int(node, "#address-cells", );
> + if (ret <= 0)
> + ac = pac;
> + else if (ret > 1 || ac <= 0 || ac > 2)
> + return 1;
> +
> + /* We only support 32-bit (1), and 64-bit (2) wide sizes here. */
> + ret = fdt_node_property_int(node, "#size-cells", );
> + if (ret <= 0)
> + sc = psc;
> + else if (ret > 1 || sc <= 0 || sc > 2)
> +