[PATCH] libfdt: add address translation support

[PATCH] libfdt: add address translation support

[Rob Herring]

From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Add FDT based address translation. Currently, only simple bus
translations are supported, but the framework allows adding other
buses like PCI. Adding this to libfdt allows the address
translation code to be shared amongst u-boot, Linux kernel and other
projects.

This code is based on GPL only licensed code. It must first be
re-licensed for dual GPL/BSD to add to libfdt which requires approval
from the copyright holders.

This code is copied from u-boot common/fdt_support.c. The portion used
here was originally added to u-boot by Kumar Gala in 2010 and Freescale
is the copyright holder. Later changes have also only been done by
Freescale authors. The u-boot code appears to have been copied from the
Linux kernel's address translation code in drivers/of/address.c which
has no copyright. This code was moved by Grant Likely in 2010 and
originated from arch/powerpc/kernel/prom_parse.c which was written by
Ben Herrenschmidt. Who is the copyright holder on a file in the kernel
with no copyright? Does this default to the author or Linus or nobody?

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Kim Phillips <kim.phillips-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
---
 libfdt/fdt_ro.c | 203 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libfdt/libfdt.h |   4 ++
 2 files changed, 207 insertions(+)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 50007f6..3c1bde1 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -571,3 +571,206 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
 
 	return offset; /* error from fdt_next_node() */
 }
+
+/* Max address size we deal with */
+#define OF_MAX_ADDR_CELLS	4
+#define OF_CHECK_COUNTS(na, ns)	((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
+			(ns) > 0)
+
+/* Helper to read a big number; size is in cells (not bytes) */
+static uint64_t fdt_read_number(const fdt32_t *cell, int size)
+{
+	uint64_t r = 0;
+	while (size--)
+		r = (r << 32) | fdt32_to_cpu(*(cell++));
+	return r;
+}
+
+/* Callbacks for bus specific translators */
+struct of_bus {
+	void (*count_cells)(void *blob, int parentoffset,
+			    int *addrc, int *sizec);
+	uint64_t (*map)(fdt32_t *addr, const fdt32_t *range,
+				int na, int ns, int pna);
+	int (*translate)(fdt32_t *addr, uint64_t offset, int na);
+};
+
+/* Default translator (generic bus) */
+static void fdt_bus_default_count_cells(void *blob, int parentoffset,
+				 	int *addrc, int *sizec)
+{
+	const fdt32_t *prop;
+
+	if (addrc) {
+		prop = fdt_getprop(blob, parentoffset, "#address-cells", NULL);
+		if (prop)
+			*addrc = fdt32_to_cpu(*prop);
+		else
+			*addrc = 2;
+	}
+
+	if (sizec) {
+		prop = fdt_getprop(blob, parentoffset, "#size-cells", NULL);
+		if (prop)
+			*sizec = fdt32_to_cpu(*prop);
+		else
+			*sizec = 2;
+	}
+}
+
+static uint64_t fdt_bus_default_map(fdt32_t *addr, const fdt32_t *range,
+				    int na, int ns, int pna)
+{
+	uint64_t cp, s, da;
+
+	cp = fdt_read_number(range, na);
+	s  = fdt_read_number(range + na + pna, ns);
+	da = fdt_read_number(addr, na);
+
+	if (da < cp || da >= (cp + s))
+		return FDT_BAD_ADDR;
+	return da - cp;
+}
+
+static int fdt_bus_default_translate(fdt32_t *addr, uint64_t offset, int na)
+{
+	uint64_t a = fdt_read_number(addr, na);
+	memset(addr, 0, na * 4);
+	a += offset;
+	if (na > 1)
+		addr[na - 2] = cpu_to_fdt32(a >> 32);
+	addr[na - 1] = cpu_to_fdt32(a & 0xffffffffu);
+
+	return 0;
+}
+
+/* Array of bus specific translators */
+static const struct of_bus of_busses[] = {
+	/* Default */
+	{
+		.count_cells = fdt_bus_default_count_cells,
+		.map = fdt_bus_default_map,
+		.translate = fdt_bus_default_translate,
+	},
+};
+
+static int fdt_translate_one(void * blob, int parent,
+			     const struct of_bus *bus,
+			     const struct of_bus *pbus, fdt32_t *addr,
+			     int na, int ns, int pna, const char *rprop)
+{
+	const fdt32_t *ranges;
+	int rlen;
+	int rone;
+	uint64_t offset = FDT_BAD_ADDR;
+
+	/* Normally, an absence of a "ranges" property means we are
+	 * crossing a non-translatable boundary, and thus the addresses
+	 * below the current not cannot be converted to CPU physical ones.
+	 * Unfortunately, while this is very clear in the spec, it's not
+	 * what Apple understood, and they do have things like /uni-n or
+	 * /ht nodes with no "ranges" property and a lot of perfectly
+	 * useable mapped devices below them. Thus we treat the absence of
+	 * "ranges" as equivalent to an empty "ranges" property which means
+	 * a 1:1 translation at that level. It's up to the caller not to try
+	 * to translate addresses that aren't supposed to be translated in
+	 * the first place. --BenH.
+	 */
+	ranges = fdt_getprop(blob, parent, rprop, &rlen);
+	if (ranges == NULL || rlen == 0) {
+		offset = fdt_read_number(addr, na);
+		memset(addr, 0, pna * 4);
+		goto finish;
+	}
+
+	/* Now walk through the ranges */
+	rlen /= 4;
+	rone = na + pna + ns;
+	for (; rlen >= rone; rlen -= rone, ranges += rone) {
+		offset = bus->map(addr, ranges, na, ns, pna);
+		if (offset != FDT_BAD_ADDR)
+			break;
+	}
+	if (offset == FDT_BAD_ADDR)
+		return -FDT_ERR_NOTFOUND;
+
+	memcpy(addr, ranges + na, 4 * pna);
+
+ finish:
+	/* Translate it into parent bus space */
+	return pbus->translate(addr, offset, pna);
+}
+
+static uint64_t __fdt_translate_address(void *blob, int node_offset,
+					const char *rprop)
+{
+	int parent, len;
+	const struct of_bus *bus, *pbus;
+	const fdt32_t *reg;
+	fdt32_t addr[OF_MAX_ADDR_CELLS];
+	int na, ns, pna, pns;
+	uint64_t result = FDT_BAD_ADDR;
+
+	reg = fdt_getprop(blob, node_offset, "reg", &len);
+	if (!reg)
+		goto bail;
+
+	/* Get parent & match bus type */
+	parent = fdt_parent_offset(blob, node_offset);
+	if (parent < 0)
+		goto bail;
+	bus = &of_busses[0];
+
+	/* Cound address cells & copy address locally */
+	bus->count_cells(blob, parent, &na, &ns);
+	if (!OF_CHECK_COUNTS(na, ns))
+		goto bail;
+
+	memcpy(addr, reg, na * 4);
+
+	/* Translate */
+	for (;;) {
+		/* Switch to parent bus */
+		node_offset = parent;
+		parent = fdt_parent_offset(blob, node_offset);
+
+		/* If root, we have finished */
+		if (parent < 0) {
+			result = fdt_read_number(addr, na);
+			break;
+		}
+
+		/* Get new parent bus and counts */
+		pbus = &of_busses[0];
+		pbus->count_cells(blob, parent, &pna, &pns);
+		if (!OF_CHECK_COUNTS(pna, pns))
+			break;
+
+		/* Apply bus translation */
+		if (fdt_translate_one(blob, node_offset, bus, pbus,
+					addr, na, ns, pna, "ranges"))
+			break;
+
+		/* Complete the move up one level */
+		na = pna;
+		ns = pns;
+		bus = pbus;
+	}
+ bail:
+	return result;
+}
+
+/*
+ * Translate an address from the device-tree into a CPU physical address,
+ * this walks up the tree and applies the various bus mappings on the
+ * way.
+ *
+ * Note: We consider that crossing any level with #size-cells == 0 to mean
+ * that translation is impossible (that is we are not dealing with a value
+ * that can be mapped to a cpu physical address). This is not really specified
+ * that way, but this is traditionally the way IBM at least do things
+ */
+uint64_t fdt_translate_address(void *fdt, int node_offset)
+{
+	return __fdt_translate_address(fdt, node_offset, "ranges");
+}
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index c4d5a91..afbea65 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -118,6 +118,8 @@
 
 #define FDT_ERR_MAX		13
 
+#define FDT_BAD_ADDR		((uint64_t)-1)
+
 /**********************************************************************/
 /* Low-level functions (you probably don't need these)                */
 /**********************************************************************/
@@ -852,6 +854,8 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
  */
 int fdt_stringlist_contains(const char *strlist, int listlen, const char *str);
 
+uint64_t fdt_translate_address(void *blob, int node_offset);
+
 /**********************************************************************/
 /* Write-in-place functions                                           */
 /**********************************************************************/
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[Warner Losh]

On Apr 1, 2014, at 10:57 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Ben Herrenschmidt. Who is the copyright holder on a file in the kernel
> with no copyright? Does this default to the author or Linus or nobody?

In the absence of a formal, written conveyance of the Copyright, it belongs
to the author(s) of that file.

Warner


--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

On Tue, Apr 01, 2014 at 12:59:43PM -0600, Warner Losh wrote:
> 
> On Apr 1, 2014, at 10:57 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > Ben Herrenschmidt. Who is the copyright holder on a file in the kernel
> > with no copyright? Does this default to the author or Linus or nobody?
> 
> In the absence of a formal, written conveyance of the Copyright, it belongs
> to the author(s) of that file.

It might depend on jurisdiction, but I don't believe presence or
absence of a copyright notice actually changes who holds the
copyright.  It just makes it easier or harder to figure out who htat
is.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] libfdt: add address translation support

[Warner Losh]

On Apr 1, 2014, at 11:43 PM, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:

> On Tue, Apr 01, 2014 at 12:59:43PM -0600, Warner Losh wrote:
>> 
>> On Apr 1, 2014, at 10:57 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> 
>>> Ben Herrenschmidt. Who is the copyright holder on a file in the kernel
>>> with no copyright? Does this default to the author or Linus or nobody?
>> 
>> In the absence of a formal, written conveyance of the Copyright, it belongs
>> to the author(s) of that file.
> 
> It might depend on jurisdiction, but I don't believe presence or
> absence of a copyright notice actually changes who holds the
> copyright.  It just makes it easier or harder to figure out who htat
> is.

Pedantically correct. In the US copyright can only be transferred in writing.
Most other countries are similar, but there is still variation despite the attempts
to make it completely uniform. But the important bit is that there’s no “default”
author in copyright law if one fails to document the author. It doesn’t go to
Torvalds or The Linux Foundation or the Easter Bunny. The original author
is the person who wrote it, or if it was a work for hire or such a contract exists
then the employer of the person who wrote it. Just to add more wrinkles to
the mix.

Warner

--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]

On Wed, Apr 02, 2014 at 08:53:54AM -0600, Warner Losh wrote:
> 
> On Apr 1, 2014, at 11:43 PM, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> > On Tue, Apr 01, 2014 at 12:59:43PM -0600, Warner Losh wrote:
> >> 
> >> On Apr 1, 2014, at 10:57 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> 
> >>> Ben Herrenschmidt. Who is the copyright holder on a file in the kernel
> >>> with no copyright? Does this default to the author or Linus or nobody?
> >> 
> >> In the absence of a formal, written conveyance of the Copyright, it belongs
> >> to the author(s) of that file.
> > 
> > It might depend on jurisdiction, but I don't believe presence or
> > absence of a copyright notice actually changes who holds the
> > copyright.  It just makes it easier or harder to figure out who htat
> > is.
> 
> Pedantically correct. In the US copyright can only be transferred in writing.
> Most other countries are similar, but there is still variation despite the attempts
> to make it completely uniform. But the important bit is that there’s no “default”
> author in copyright law if one fails to document the author. It doesn’t go to
> Torvalds or The Linux Foundation or the Easter Bunny. The original author
> is the person who wrote it, or if it was a work for hire or such a contract exists
> then the employer of the person who wrote it. Just to add more wrinkles to
> the mix.

Right, we are in agreement.

I'm sorry, I misread your initial comment as referring to a copyright
notice, rather than a separate conveyance of copyright.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] libfdt: add address translation support

[Scott Wood]

On Tue, 2014-04-01 at 11:57 -0500, Rob Herring wrote:
> From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> Add FDT based address translation. Currently, only simple bus
> translations are supported, but the framework allows adding other
> buses like PCI. Adding this to libfdt allows the address
> translation code to be shared amongst u-boot, Linux kernel and other
> projects.
> 
> This code is based on GPL only licensed code. It must first be
> re-licensed for dual GPL/BSD to add to libfdt which requires approval
> from the copyright holders.
> 
> This code is copied from u-boot common/fdt_support.c. The portion used
> here was originally added to u-boot by Kumar Gala in 2010 and Freescale
> is the copyright holder. Later changes have also only been done by
> Freescale authors. The u-boot code appears to have been copied from the
> Linux kernel's address translation code in drivers/of/address.c which
> has no copyright. This code was moved by Grant Likely in 2010 and
> originated from arch/powerpc/kernel/prom_parse.c which was written by
> Ben Herrenschmidt. Who is the copyright holder on a file in the kernel
> with no copyright? Does this default to the author or Linus or nobody?

ACK for the Freescale contribution (on copyright issues, not technical -- see below).

FWIW, if you have trouble tracking everyone down for this code, there's
address translation code in arch/powerpc/boot/devtree.c that has a
cleaner history (I wrote it from scratch, and there's been only minor
modifications since then, by Mark Greer).

> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Kim Phillips <kim.phillips-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> ---
>  libfdt/fdt_ro.c | 203 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libfdt/libfdt.h |   4 ++
>  2 files changed, 207 insertions(+)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 50007f6..3c1bde1 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -571,3 +571,206 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
>  
>  	return offset; /* error from fdt_next_node() */
>  }
> +
> +/* Max address size we deal with */
> +#define OF_MAX_ADDR_CELLS	4
> +#define OF_CHECK_COUNTS(na, ns)	((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
> +			(ns) > 0)
> +
> +/* Helper to read a big number; size is in cells (not bytes) */
> +static uint64_t fdt_read_number(const fdt32_t *cell, int size)
> +{
> +	uint64_t r = 0;
> +	while (size--)
> +		r = (r << 32) | fdt32_to_cpu(*(cell++));
> +	return r;
> +}
> +
> +/* Callbacks for bus specific translators */
> +struct of_bus {
> +	void (*count_cells)(void *blob, int parentoffset,
> +			    int *addrc, int *sizec);
> +	uint64_t (*map)(fdt32_t *addr, const fdt32_t *range,
> +				int na, int ns, int pna);
> +	int (*translate)(fdt32_t *addr, uint64_t offset, int na);
> +};
> +
> +/* Default translator (generic bus) */
> +static void fdt_bus_default_count_cells(void *blob, int parentoffset,
> +				 	int *addrc, int *sizec)
> +{
> +	const fdt32_t *prop;
> +
> +	if (addrc) {
> +		prop = fdt_getprop(blob, parentoffset, "#address-cells", NULL);
> +		if (prop)
> +			*addrc = fdt32_to_cpu(*prop);
> +		else
> +			*addrc = 2;
> +	}
> +
> +	if (sizec) {
> +		prop = fdt_getprop(blob, parentoffset, "#size-cells", NULL);
> +		if (prop)
> +			*sizec = fdt32_to_cpu(*prop);
> +		else
> +			*sizec = 2;
> +	}
> +}

#size-cells should default to 1 as per IEEE1275 (ePAPR agrees, though
it calls a missing #size-cells non-compliant).  This is also what
kernel code currently seems to do (see OF_ROOT_NODE_SIZE_CELLS_DEFAULT).

> +static uint64_t fdt_bus_default_map(fdt32_t *addr, const fdt32_t *range,
> +				    int na, int ns, int pna)
> +{
> +	uint64_t cp, s, da;
> +
> +	cp = fdt_read_number(range, na);
> +	s  = fdt_read_number(range + na + pna, ns);
> +	da = fdt_read_number(addr, na);
> +
> +	if (da < cp || da >= (cp + s))
> +		return FDT_BAD_ADDR;
> +	return da - cp;
> +}
> +
> +static int fdt_bus_default_translate(fdt32_t *addr, uint64_t offset, int na)
> +{
> +	uint64_t a = fdt_read_number(addr, na);
> +	memset(addr, 0, na * 4);
> +	a += offset;
> +	if (na > 1)
> +		addr[na - 2] = cpu_to_fdt32(a >> 32);
> +	addr[na - 1] = cpu_to_fdt32(a & 0xffffffffu);
> +
> +	return 0;
> +}
> +
> +/* Array of bus specific translators */
> +static const struct of_bus of_busses[] = {
> +	/* Default */
> +	{
> +		.count_cells = fdt_bus_default_count_cells,
> +		.map = fdt_bus_default_map,
> +		.translate = fdt_bus_default_translate,
> +	},
> +};
> +
> +static int fdt_translate_one(void * blob, int parent,
> +			     const struct of_bus *bus,
> +			     const struct of_bus *pbus, fdt32_t *addr,
> +			     int na, int ns, int pna, const char *rprop)
> +{
> +	const fdt32_t *ranges;
> +	int rlen;
> +	int rone;
> +	uint64_t offset = FDT_BAD_ADDR;
> +
> +	/* Normally, an absence of a "ranges" property means we are
> +	 * crossing a non-translatable boundary, and thus the addresses
> +	 * below the current not cannot be converted to CPU physical ones.
> +	 * Unfortunately, while this is very clear in the spec, it's not
> +	 * what Apple understood, and they do have things like /uni-n or
> +	 * /ht nodes with no "ranges" property and a lot of perfectly
> +	 * useable mapped devices below them. Thus we treat the absence of
> +	 * "ranges" as equivalent to an empty "ranges" property which means
> +	 * a 1:1 translation at that level. It's up to the caller not to try
> +	 * to translate addresses that aren't supposed to be translated in
> +	 * the first place. --BenH.
> +	 */

I don't think libfdt should be copying a non-compliant hack that was
meant to deal with broken Apple firmware that doesn't use flat trees.

-Scott

> +	ranges = fdt_getprop(blob, parent, rprop, &rlen);
> +	if (ranges == NULL || rlen == 0) {
> +		offset = fdt_read_number(addr, na);
> +		memset(addr, 0, pna * 4);
> +		goto finish;
> +	}
> +
> +	/* Now walk through the ranges */
> +	rlen /= 4;
> +	rone = na + pna + ns;
> +	for (; rlen >= rone; rlen -= rone, ranges += rone) {
> +		offset = bus->map(addr, ranges, na, ns, pna);
> +		if (offset != FDT_BAD_ADDR)
> +			break;
> +	}
> +	if (offset == FDT_BAD_ADDR)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	memcpy(addr, ranges + na, 4 * pna);
> +
> + finish:
> +	/* Translate it into parent bus space */
> +	return pbus->translate(addr, offset, pna);
> +}
> +
> +static uint64_t __fdt_translate_address(void *blob, int node_offset,
> +					const char *rprop)
> +{
> +	int parent, len;
> +	const struct of_bus *bus, *pbus;
> +	const fdt32_t *reg;
> +	fdt32_t addr[OF_MAX_ADDR_CELLS];
> +	int na, ns, pna, pns;
> +	uint64_t result = FDT_BAD_ADDR;
> +
> +	reg = fdt_getprop(blob, node_offset, "reg", &len);
> +	if (!reg)
> +		goto bail;
> +
> +	/* Get parent & match bus type */
> +	parent = fdt_parent_offset(blob, node_offset);
> +	if (parent < 0)
> +		goto bail;
> +	bus = &of_busses[0];
> +
> +	/* Cound address cells & copy address locally */
> +	bus->count_cells(blob, parent, &na, &ns);
> +	if (!OF_CHECK_COUNTS(na, ns))
> +		goto bail;
> +
> +	memcpy(addr, reg, na * 4);
> +
> +	/* Translate */
> +	for (;;) {
> +		/* Switch to parent bus */
> +		node_offset = parent;
> +		parent = fdt_parent_offset(blob, node_offset);
> +
> +		/* If root, we have finished */
> +		if (parent < 0) {
> +			result = fdt_read_number(addr, na);
> +			break;
> +		}
> +
> +		/* Get new parent bus and counts */
> +		pbus = &of_busses[0];
> +		pbus->count_cells(blob, parent, &pna, &pns);
> +		if (!OF_CHECK_COUNTS(pna, pns))
> +			break;
> +
> +		/* Apply bus translation */
> +		if (fdt_translate_one(blob, node_offset, bus, pbus,
> +					addr, na, ns, pna, "ranges"))
> +			break;
> +
> +		/* Complete the move up one level */
> +		na = pna;
> +		ns = pns;
> +		bus = pbus;
> +	}
> + bail:
> +	return result;
> +}
> +
> +/*
> + * Translate an address from the device-tree into a CPU physical address,
> + * this walks up the tree and applies the various bus mappings on the
> + * way.
> + *
> + * Note: We consider that crossing any level with #size-cells == 0 to mean
> + * that translation is impossible (that is we are not dealing with a value
> + * that can be mapped to a cpu physical address). This is not really specified
> + * that way, but this is traditionally the way IBM at least do things
> + */
> +uint64_t fdt_translate_address(void *fdt, int node_offset)
> +{
> +	return __fdt_translate_address(fdt, node_offset, "ranges");
> +}
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index c4d5a91..afbea65 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -118,6 +118,8 @@
>  
>  #define FDT_ERR_MAX		13
>  
> +#define FDT_BAD_ADDR		((uint64_t)-1)
> +
>  /**********************************************************************/
>  /* Low-level functions (you probably don't need these)                */
>  /**********************************************************************/
> @@ -852,6 +854,8 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
>   */
>  int fdt_stringlist_contains(const char *strlist, int listlen, const char *str);
>  
> +uint64_t fdt_translate_address(void *blob, int node_offset);
> +
>  /**********************************************************************/
>  /* Write-in-place functions                                           */
>  /**********************************************************************/



--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[Kim Phillips]

On Tue, 1 Apr 2014 18:02:38 -0500
Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> On Tue, 2014-04-01 at 11:57 -0500, Rob Herring wrote:
> > From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > 
> > Add FDT based address translation. Currently, only simple bus
> > translations are supported, but the framework allows adding other
> > buses like PCI. Adding this to libfdt allows the address
> > translation code to be shared amongst u-boot, Linux kernel and other
> > projects.
> > 
> > This code is based on GPL only licensed code. It must first be
> > re-licensed for dual GPL/BSD to add to libfdt which requires approval
> > from the copyright holders.
> > 
> > This code is copied from u-boot common/fdt_support.c. The portion used
> > here was originally added to u-boot by Kumar Gala in 2010 and Freescale
> > is the copyright holder. Later changes have also only been done by
> > Freescale authors. The u-boot code appears to have been copied from the
> > Linux kernel's address translation code in drivers/of/address.c which
> > has no copyright. This code was moved by Grant Likely in 2010 and
> > originated from arch/powerpc/kernel/prom_parse.c which was written by
> > Ben Herrenschmidt. Who is the copyright holder on a file in the kernel
> > with no copyright? Does this default to the author or Linus or nobody?
> 
> ACK for the Freescale contribution (on copyright issues, not technical -- see below).

FWIW, ack from me too.

> +/* Default translator (generic bus) */
> +static void fdt_bus_default_count_cells(void *blob, int parentoffset,
> +				 	int *addrc, int *sizec)

space before tab in indent (git am).

Also, thanks for not introducing new sparse warnings :)

Kim

--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[Rob Herring]

On Wed, Apr 2, 2014 at 2:24 PM, Kim Phillips <kim.phillips-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> On Tue, 1 Apr 2014 18:02:38 -0500
> Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>
>> On Tue, 2014-04-01 at 11:57 -0500, Rob Herring wrote:
>> > From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> >
>> > Add FDT based address translation. Currently, only simple bus
>> > translations are supported, but the framework allows adding other
>> > buses like PCI. Adding this to libfdt allows the address
>> > translation code to be shared amongst u-boot, Linux kernel and other
>> > projects.
>> >
>> > This code is based on GPL only licensed code. It must first be
>> > re-licensed for dual GPL/BSD to add to libfdt which requires approval
>> > from the copyright holders.
>> >
>> > This code is copied from u-boot common/fdt_support.c. The portion used
>> > here was originally added to u-boot by Kumar Gala in 2010 and Freescale
>> > is the copyright holder. Later changes have also only been done by
>> > Freescale authors. The u-boot code appears to have been copied from the
>> > Linux kernel's address translation code in drivers/of/address.c which
>> > has no copyright. This code was moved by Grant Likely in 2010 and
>> > originated from arch/powerpc/kernel/prom_parse.c which was written by
>> > Ben Herrenschmidt. Who is the copyright holder on a file in the kernel
>> > with no copyright? Does this default to the author or Linus or nobody?
>>
>> ACK for the Freescale contribution (on copyright issues, not technical -- see below).
>
> FWIW, ack from me too.

Thanks.

>> +/* Default translator (generic bus) */
>> +static void fdt_bus_default_count_cells(void *blob, int parentoffset,
>> +                                     int *addrc, int *sizec)
>
> space before tab in indent (git am).
>
> Also, thanks for not introducing new sparse warnings :)

I believe that's mainly because someone already did on the u-boot source. :)

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[Rob Herring]

Ben,

On Tue, Apr 1, 2014 at 6:02 PM, Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> On Tue, 2014-04-01 at 11:57 -0500, Rob Herring wrote:
>> From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>
>> Add FDT based address translation. Currently, only simple bus
>> translations are supported, but the framework allows adding other
>> buses like PCI. Adding this to libfdt allows the address
>> translation code to be shared amongst u-boot, Linux kernel and other
>> projects.
>>
>> This code is based on GPL only licensed code. It must first be
>> re-licensed for dual GPL/BSD to add to libfdt which requires approval
>> from the copyright holders.
>>
>> This code is copied from u-boot common/fdt_support.c. The portion used
>> here was originally added to u-boot by Kumar Gala in 2010 and Freescale
>> is the copyright holder. Later changes have also only been done by
>> Freescale authors. The u-boot code appears to have been copied from the
>> Linux kernel's address translation code in drivers/of/address.c which
>> has no copyright. This code was moved by Grant Likely in 2010 and
>> originated from arch/powerpc/kernel/prom_parse.c which was written by
>> Ben Herrenschmidt. Who is the copyright holder on a file in the kernel
>> with no copyright? Does this default to the author or Linus or nobody?
>
> ACK for the Freescale contribution (on copyright issues, not technical -- see below).

Can you please comment on the re-licensing of this code.

Rob

> FWIW, if you have trouble tracking everyone down for this code, there's
> address translation code in arch/powerpc/boot/devtree.c that has a
> cleaner history (I wrote it from scratch, and there's been only minor
> modifications since then, by Mark Greer).
>
>> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> Cc: Kim Phillips <kim.phillips-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>> ---
>>  libfdt/fdt_ro.c | 203 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  libfdt/libfdt.h |   4 ++
>>  2 files changed, 207 insertions(+)
>>
>> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
>> index 50007f6..3c1bde1 100644
>> --- a/libfdt/fdt_ro.c
>> +++ b/libfdt/fdt_ro.c
>> @@ -571,3 +571,206 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
>>
>>       return offset; /* error from fdt_next_node() */
>>  }
>> +
>> +/* Max address size we deal with */
>> +#define OF_MAX_ADDR_CELLS    4
>> +#define OF_CHECK_COUNTS(na, ns)      ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
>> +                     (ns) > 0)
>> +
>> +/* Helper to read a big number; size is in cells (not bytes) */
>> +static uint64_t fdt_read_number(const fdt32_t *cell, int size)
>> +{
>> +     uint64_t r = 0;
>> +     while (size--)
>> +             r = (r << 32) | fdt32_to_cpu(*(cell++));
>> +     return r;
>> +}
>> +
>> +/* Callbacks for bus specific translators */
>> +struct of_bus {
>> +     void (*count_cells)(void *blob, int parentoffset,
>> +                         int *addrc, int *sizec);
>> +     uint64_t (*map)(fdt32_t *addr, const fdt32_t *range,
>> +                             int na, int ns, int pna);
>> +     int (*translate)(fdt32_t *addr, uint64_t offset, int na);
>> +};
>> +
>> +/* Default translator (generic bus) */
>> +static void fdt_bus_default_count_cells(void *blob, int parentoffset,
>> +                                     int *addrc, int *sizec)
>> +{
>> +     const fdt32_t *prop;
>> +
>> +     if (addrc) {
>> +             prop = fdt_getprop(blob, parentoffset, "#address-cells", NULL);
>> +             if (prop)
>> +                     *addrc = fdt32_to_cpu(*prop);
>> +             else
>> +                     *addrc = 2;
>> +     }
>> +
>> +     if (sizec) {
>> +             prop = fdt_getprop(blob, parentoffset, "#size-cells", NULL);
>> +             if (prop)
>> +                     *sizec = fdt32_to_cpu(*prop);
>> +             else
>> +                     *sizec = 2;
>> +     }
>> +}
>
> #size-cells should default to 1 as per IEEE1275 (ePAPR agrees, though
> it calls a missing #size-cells non-compliant).  This is also what
> kernel code currently seems to do (see OF_ROOT_NODE_SIZE_CELLS_DEFAULT).
>
>> +static uint64_t fdt_bus_default_map(fdt32_t *addr, const fdt32_t *range,
>> +                                 int na, int ns, int pna)
>> +{
>> +     uint64_t cp, s, da;
>> +
>> +     cp = fdt_read_number(range, na);
>> +     s  = fdt_read_number(range + na + pna, ns);
>> +     da = fdt_read_number(addr, na);
>> +
>> +     if (da < cp || da >= (cp + s))
>> +             return FDT_BAD_ADDR;
>> +     return da - cp;
>> +}
>> +
>> +static int fdt_bus_default_translate(fdt32_t *addr, uint64_t offset, int na)
>> +{
>> +     uint64_t a = fdt_read_number(addr, na);
>> +     memset(addr, 0, na * 4);
>> +     a += offset;
>> +     if (na > 1)
>> +             addr[na - 2] = cpu_to_fdt32(a >> 32);
>> +     addr[na - 1] = cpu_to_fdt32(a & 0xffffffffu);
>> +
>> +     return 0;
>> +}
>> +
>> +/* Array of bus specific translators */
>> +static const struct of_bus of_busses[] = {
>> +     /* Default */
>> +     {
>> +             .count_cells = fdt_bus_default_count_cells,
>> +             .map = fdt_bus_default_map,
>> +             .translate = fdt_bus_default_translate,
>> +     },
>> +};
>> +
>> +static int fdt_translate_one(void * blob, int parent,
>> +                          const struct of_bus *bus,
>> +                          const struct of_bus *pbus, fdt32_t *addr,
>> +                          int na, int ns, int pna, const char *rprop)
>> +{
>> +     const fdt32_t *ranges;
>> +     int rlen;
>> +     int rone;
>> +     uint64_t offset = FDT_BAD_ADDR;
>> +
>> +     /* Normally, an absence of a "ranges" property means we are
>> +      * crossing a non-translatable boundary, and thus the addresses
>> +      * below the current not cannot be converted to CPU physical ones.
>> +      * Unfortunately, while this is very clear in the spec, it's not
>> +      * what Apple understood, and they do have things like /uni-n or
>> +      * /ht nodes with no "ranges" property and a lot of perfectly
>> +      * useable mapped devices below them. Thus we treat the absence of
>> +      * "ranges" as equivalent to an empty "ranges" property which means
>> +      * a 1:1 translation at that level. It's up to the caller not to try
>> +      * to translate addresses that aren't supposed to be translated in
>> +      * the first place. --BenH.
>> +      */
>
> I don't think libfdt should be copying a non-compliant hack that was
> meant to deal with broken Apple firmware that doesn't use flat trees.
>
> -Scott
>
>> +     ranges = fdt_getprop(blob, parent, rprop, &rlen);
>> +     if (ranges == NULL || rlen == 0) {
>> +             offset = fdt_read_number(addr, na);
>> +             memset(addr, 0, pna * 4);
>> +             goto finish;
>> +     }
>> +
>> +     /* Now walk through the ranges */
>> +     rlen /= 4;
>> +     rone = na + pna + ns;
>> +     for (; rlen >= rone; rlen -= rone, ranges += rone) {
>> +             offset = bus->map(addr, ranges, na, ns, pna);
>> +             if (offset != FDT_BAD_ADDR)
>> +                     break;
>> +     }
>> +     if (offset == FDT_BAD_ADDR)
>> +             return -FDT_ERR_NOTFOUND;
>> +
>> +     memcpy(addr, ranges + na, 4 * pna);
>> +
>> + finish:
>> +     /* Translate it into parent bus space */
>> +     return pbus->translate(addr, offset, pna);
>> +}
>> +
>> +static uint64_t __fdt_translate_address(void *blob, int node_offset,
>> +                                     const char *rprop)
>> +{
>> +     int parent, len;
>> +     const struct of_bus *bus, *pbus;
>> +     const fdt32_t *reg;
>> +     fdt32_t addr[OF_MAX_ADDR_CELLS];
>> +     int na, ns, pna, pns;
>> +     uint64_t result = FDT_BAD_ADDR;
>> +
>> +     reg = fdt_getprop(blob, node_offset, "reg", &len);
>> +     if (!reg)
>> +             goto bail;
>> +
>> +     /* Get parent & match bus type */
>> +     parent = fdt_parent_offset(blob, node_offset);
>> +     if (parent < 0)
>> +             goto bail;
>> +     bus = &of_busses[0];
>> +
>> +     /* Cound address cells & copy address locally */
>> +     bus->count_cells(blob, parent, &na, &ns);
>> +     if (!OF_CHECK_COUNTS(na, ns))
>> +             goto bail;
>> +
>> +     memcpy(addr, reg, na * 4);
>> +
>> +     /* Translate */
>> +     for (;;) {
>> +             /* Switch to parent bus */
>> +             node_offset = parent;
>> +             parent = fdt_parent_offset(blob, node_offset);
>> +
>> +             /* If root, we have finished */
>> +             if (parent < 0) {
>> +                     result = fdt_read_number(addr, na);
>> +                     break;
>> +             }
>> +
>> +             /* Get new parent bus and counts */
>> +             pbus = &of_busses[0];
>> +             pbus->count_cells(blob, parent, &pna, &pns);
>> +             if (!OF_CHECK_COUNTS(pna, pns))
>> +                     break;
>> +
>> +             /* Apply bus translation */
>> +             if (fdt_translate_one(blob, node_offset, bus, pbus,
>> +                                     addr, na, ns, pna, "ranges"))
>> +                     break;
>> +
>> +             /* Complete the move up one level */
>> +             na = pna;
>> +             ns = pns;
>> +             bus = pbus;
>> +     }
>> + bail:
>> +     return result;
>> +}
>> +
>> +/*
>> + * Translate an address from the device-tree into a CPU physical address,
>> + * this walks up the tree and applies the various bus mappings on the
>> + * way.
>> + *
>> + * Note: We consider that crossing any level with #size-cells == 0 to mean
>> + * that translation is impossible (that is we are not dealing with a value
>> + * that can be mapped to a cpu physical address). This is not really specified
>> + * that way, but this is traditionally the way IBM at least do things
>> + */
>> +uint64_t fdt_translate_address(void *fdt, int node_offset)
>> +{
>> +     return __fdt_translate_address(fdt, node_offset, "ranges");
>> +}
>> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
>> index c4d5a91..afbea65 100644
>> --- a/libfdt/libfdt.h
>> +++ b/libfdt/libfdt.h
>> @@ -118,6 +118,8 @@
>>
>>  #define FDT_ERR_MAX          13
>>
>> +#define FDT_BAD_ADDR         ((uint64_t)-1)
>> +
>>  /**********************************************************************/
>>  /* Low-level functions (you probably don't need these)                */
>>  /**********************************************************************/
>> @@ -852,6 +854,8 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
>>   */
>>  int fdt_stringlist_contains(const char *strlist, int listlen, const char *str);
>>
>> +uint64_t fdt_translate_address(void *blob, int node_offset);
>> +
>>  /**********************************************************************/
>>  /* Write-in-place functions                                           */
>>  /**********************************************************************/
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[Rob Herring]

On Tue, Apr 15, 2014 at 9:03 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Ben,
>
> On Tue, Apr 1, 2014 at 6:02 PM, Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> On Tue, 2014-04-01 at 11:57 -0500, Rob Herring wrote:
>>> From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>
>>> Add FDT based address translation. Currently, only simple bus
>>> translations are supported, but the framework allows adding other
>>> buses like PCI. Adding this to libfdt allows the address
>>> translation code to be shared amongst u-boot, Linux kernel and other
>>> projects.
>>>
>>> This code is based on GPL only licensed code. It must first be
>>> re-licensed for dual GPL/BSD to add to libfdt which requires approval
>>> from the copyright holders.
>>>
>>> This code is copied from u-boot common/fdt_support.c. The portion used
>>> here was originally added to u-boot by Kumar Gala in 2010 and Freescale
>>> is the copyright holder. Later changes have also only been done by
>>> Freescale authors. The u-boot code appears to have been copied from the
>>> Linux kernel's address translation code in drivers/of/address.c which
>>> has no copyright. This code was moved by Grant Likely in 2010 and
>>> originated from arch/powerpc/kernel/prom_parse.c which was written by
>>> Ben Herrenschmidt. Who is the copyright holder on a file in the kernel
>>> with no copyright? Does this default to the author or Linus or nobody?
>>
>> ACK for the Freescale contribution (on copyright issues, not technical -- see below).
>
> Can you please comment on the re-licensing of this code.
>

Ping.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[Grant Likely]

On Tue, Apr 1, 2014 at 5:57 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> Add FDT based address translation. Currently, only simple bus
> translations are supported, but the framework allows adding other
> buses like PCI. Adding this to libfdt allows the address
> translation code to be shared amongst u-boot, Linux kernel and other
> projects.
>
> This code is based on GPL only licensed code. It must first be
> re-licensed for dual GPL/BSD to add to libfdt which requires approval
> from the copyright holders.
>
> This code is copied from u-boot common/fdt_support.c. The portion used
> here was originally added to u-boot by Kumar Gala in 2010 and Freescale
> is the copyright holder. Later changes have also only been done by
> Freescale authors. The u-boot code appears to have been copied from the
> Linux kernel's address translation code in drivers/of/address.c which
> has no copyright. This code was moved by Grant Likely in 2010 and
> originated from arch/powerpc/kernel/prom_parse.c which was written by
> Ben Herrenschmidt.

Okay, so this makes things a bit more complicated. You need to compose
a list of every commit that has touched this file in *both* the u-boot
and kernel trees, and identify all of the copyright holders based on
who submitted the patches. Once you have that list, then you need to
get an Ack from every single one of them. If anyone does not respond,
or does not give agreement, then we need to look at their change and
determine whether or not we consider it to be a significant enough
change that it would warrant copyright protection.

g.


> Who is the copyright holder on a file in the kernel
> with no copyright? Does this default to the author or Linus or nobody?
>
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Kim Phillips <kim.phillips-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> ---
>  libfdt/fdt_ro.c | 203 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libfdt/libfdt.h |   4 ++
>  2 files changed, 207 insertions(+)
>
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 50007f6..3c1bde1 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -571,3 +571,206 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
>
>         return offset; /* error from fdt_next_node() */
>  }
> +
> +/* Max address size we deal with */
> +#define OF_MAX_ADDR_CELLS      4
> +#define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
> +                       (ns) > 0)
> +
> +/* Helper to read a big number; size is in cells (not bytes) */
> +static uint64_t fdt_read_number(const fdt32_t *cell, int size)
> +{
> +       uint64_t r = 0;
> +       while (size--)
> +               r = (r << 32) | fdt32_to_cpu(*(cell++));
> +       return r;
> +}
> +
> +/* Callbacks for bus specific translators */
> +struct of_bus {
> +       void (*count_cells)(void *blob, int parentoffset,
> +                           int *addrc, int *sizec);
> +       uint64_t (*map)(fdt32_t *addr, const fdt32_t *range,
> +                               int na, int ns, int pna);
> +       int (*translate)(fdt32_t *addr, uint64_t offset, int na);
> +};
> +
> +/* Default translator (generic bus) */
> +static void fdt_bus_default_count_cells(void *blob, int parentoffset,
> +                                       int *addrc, int *sizec)
> +{
> +       const fdt32_t *prop;
> +
> +       if (addrc) {
> +               prop = fdt_getprop(blob, parentoffset, "#address-cells", NULL);
> +               if (prop)
> +                       *addrc = fdt32_to_cpu(*prop);
> +               else
> +                       *addrc = 2;
> +       }
> +
> +       if (sizec) {
> +               prop = fdt_getprop(blob, parentoffset, "#size-cells", NULL);
> +               if (prop)
> +                       *sizec = fdt32_to_cpu(*prop);
> +               else
> +                       *sizec = 2;
> +       }
> +}
> +
> +static uint64_t fdt_bus_default_map(fdt32_t *addr, const fdt32_t *range,
> +                                   int na, int ns, int pna)
> +{
> +       uint64_t cp, s, da;
> +
> +       cp = fdt_read_number(range, na);
> +       s  = fdt_read_number(range + na + pna, ns);
> +       da = fdt_read_number(addr, na);
> +
> +       if (da < cp || da >= (cp + s))
> +               return FDT_BAD_ADDR;
> +       return da - cp;
> +}
> +
> +static int fdt_bus_default_translate(fdt32_t *addr, uint64_t offset, int na)
> +{
> +       uint64_t a = fdt_read_number(addr, na);
> +       memset(addr, 0, na * 4);
> +       a += offset;
> +       if (na > 1)
> +               addr[na - 2] = cpu_to_fdt32(a >> 32);
> +       addr[na - 1] = cpu_to_fdt32(a & 0xffffffffu);
> +
> +       return 0;
> +}
> +
> +/* Array of bus specific translators */
> +static const struct of_bus of_busses[] = {
> +       /* Default */
> +       {
> +               .count_cells = fdt_bus_default_count_cells,
> +               .map = fdt_bus_default_map,
> +               .translate = fdt_bus_default_translate,
> +       },
> +};
> +
> +static int fdt_translate_one(void * blob, int parent,
> +                            const struct of_bus *bus,
> +                            const struct of_bus *pbus, fdt32_t *addr,
> +                            int na, int ns, int pna, const char *rprop)
> +{
> +       const fdt32_t *ranges;
> +       int rlen;
> +       int rone;
> +       uint64_t offset = FDT_BAD_ADDR;
> +
> +       /* Normally, an absence of a "ranges" property means we are
> +        * crossing a non-translatable boundary, and thus the addresses
> +        * below the current not cannot be converted to CPU physical ones.
> +        * Unfortunately, while this is very clear in the spec, it's not
> +        * what Apple understood, and they do have things like /uni-n or
> +        * /ht nodes with no "ranges" property and a lot of perfectly
> +        * useable mapped devices below them. Thus we treat the absence of
> +        * "ranges" as equivalent to an empty "ranges" property which means
> +        * a 1:1 translation at that level. It's up to the caller not to try
> +        * to translate addresses that aren't supposed to be translated in
> +        * the first place. --BenH.
> +        */
> +       ranges = fdt_getprop(blob, parent, rprop, &rlen);
> +       if (ranges == NULL || rlen == 0) {
> +               offset = fdt_read_number(addr, na);
> +               memset(addr, 0, pna * 4);
> +               goto finish;
> +       }
> +
> +       /* Now walk through the ranges */
> +       rlen /= 4;
> +       rone = na + pna + ns;
> +       for (; rlen >= rone; rlen -= rone, ranges += rone) {
> +               offset = bus->map(addr, ranges, na, ns, pna);
> +               if (offset != FDT_BAD_ADDR)
> +                       break;
> +       }
> +       if (offset == FDT_BAD_ADDR)
> +               return -FDT_ERR_NOTFOUND;
> +
> +       memcpy(addr, ranges + na, 4 * pna);
> +
> + finish:
> +       /* Translate it into parent bus space */
> +       return pbus->translate(addr, offset, pna);
> +}
> +
> +static uint64_t __fdt_translate_address(void *blob, int node_offset,
> +                                       const char *rprop)
> +{
> +       int parent, len;
> +       const struct of_bus *bus, *pbus;
> +       const fdt32_t *reg;
> +       fdt32_t addr[OF_MAX_ADDR_CELLS];
> +       int na, ns, pna, pns;
> +       uint64_t result = FDT_BAD_ADDR;
> +
> +       reg = fdt_getprop(blob, node_offset, "reg", &len);
> +       if (!reg)
> +               goto bail;
> +
> +       /* Get parent & match bus type */
> +       parent = fdt_parent_offset(blob, node_offset);
> +       if (parent < 0)
> +               goto bail;
> +       bus = &of_busses[0];
> +
> +       /* Cound address cells & copy address locally */
> +       bus->count_cells(blob, parent, &na, &ns);
> +       if (!OF_CHECK_COUNTS(na, ns))
> +               goto bail;
> +
> +       memcpy(addr, reg, na * 4);
> +
> +       /* Translate */
> +       for (;;) {
> +               /* Switch to parent bus */
> +               node_offset = parent;
> +               parent = fdt_parent_offset(blob, node_offset);
> +
> +               /* If root, we have finished */
> +               if (parent < 0) {
> +                       result = fdt_read_number(addr, na);
> +                       break;
> +               }
> +
> +               /* Get new parent bus and counts */
> +               pbus = &of_busses[0];
> +               pbus->count_cells(blob, parent, &pna, &pns);
> +               if (!OF_CHECK_COUNTS(pna, pns))
> +                       break;
> +
> +               /* Apply bus translation */
> +               if (fdt_translate_one(blob, node_offset, bus, pbus,
> +                                       addr, na, ns, pna, "ranges"))
> +                       break;
> +
> +               /* Complete the move up one level */
> +               na = pna;
> +               ns = pns;
> +               bus = pbus;
> +       }
> + bail:
> +       return result;
> +}
> +
> +/*
> + * Translate an address from the device-tree into a CPU physical address,
> + * this walks up the tree and applies the various bus mappings on the
> + * way.
> + *
> + * Note: We consider that crossing any level with #size-cells == 0 to mean
> + * that translation is impossible (that is we are not dealing with a value
> + * that can be mapped to a cpu physical address). This is not really specified
> + * that way, but this is traditionally the way IBM at least do things
> + */
> +uint64_t fdt_translate_address(void *fdt, int node_offset)
> +{
> +       return __fdt_translate_address(fdt, node_offset, "ranges");
> +}
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index c4d5a91..afbea65 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -118,6 +118,8 @@
>
>  #define FDT_ERR_MAX            13
>
> +#define FDT_BAD_ADDR           ((uint64_t)-1)
> +
>  /**********************************************************************/
>  /* Low-level functions (you probably don't need these)                */
>  /**********************************************************************/
> @@ -852,6 +854,8 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
>   */
>  int fdt_stringlist_contains(const char *strlist, int listlen, const char *str);
>
> +uint64_t fdt_translate_address(void *blob, int node_offset);
> +
>  /**********************************************************************/
>  /* Write-in-place functions                                           */
>  /**********************************************************************/
> --
> 1.8.3.2
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[Rob Herring]

On Tue, Apr 15, 2014 at 4:57 PM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Apr 1, 2014 at 5:57 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>
>> Add FDT based address translation. Currently, only simple bus
>> translations are supported, but the framework allows adding other
>> buses like PCI. Adding this to libfdt allows the address
>> translation code to be shared amongst u-boot, Linux kernel and other
>> projects.
>>
>> This code is based on GPL only licensed code. It must first be
>> re-licensed for dual GPL/BSD to add to libfdt which requires approval
>> from the copyright holders.
>>
>> This code is copied from u-boot common/fdt_support.c. The portion used
>> here was originally added to u-boot by Kumar Gala in 2010 and Freescale
>> is the copyright holder. Later changes have also only been done by
>> Freescale authors. The u-boot code appears to have been copied from the
>> Linux kernel's address translation code in drivers/of/address.c which
>> has no copyright. This code was moved by Grant Likely in 2010 and
>> originated from arch/powerpc/kernel/prom_parse.c which was written by
>> Ben Herrenschmidt.
>
> Okay, so this makes things a bit more complicated. You need to compose
> a list of every commit that has touched this file in *both* the u-boot
> and kernel trees, and identify all of the copyright holders based on
> who submitted the patches. Once you have that list, then you need to
> get an Ack from every single one of them. If anyone does not respond,
> or does not give agreement, then we need to look at their change and
> determine whether or not we consider it to be a significant enough
> change that it would warrant copyright protection.

Fortunately, it is not that complicated. The u-boot version was done
almost exactly the same time as the PPC version was moved by you (June
2010). The next commit that was not you was Oct 2010. Therefore, any
changes to drivers/of/address.c can be ignored (I'm assuming you will
ack this :)). So we only need to look at the powerpc version in
arch/powerpc/kernel/prom_parse.c. Ignoring lines other than the
default address translation code, there are only 3 committers: Ben,
Jeremy Kerr, and Stephen Rothwell. The overwhelming majority of the
code is from the original commit by Ben (hence why I only considered
him to begin with).

Here are the commits from Jeremy and Stephen. I would characterize as
trivial fixes, but I've copied both of them here.

Stephen:
e2eb639 [POWERPC] Rename get_property to of_get_property: arch/powerpc
9213fee [POWERPC] Rename prom_n_size_cells to of_n_size_cells
a8bda5d [POWERPC] Rename prom_n_addr_cells to of_n_addr_cells

Jeremy:
a7f67bd [POWERPC] Constify & voidify get_property()
b5a1a9a [POWERPC] Use const qualifiers for prom parsing utilites

Here is the git blame I used for the analysis:
git blame dbbdee9^ arch/powerpc/kernel/prom_parse.c


For the u-boot side, I don't think the situation is any different from
we originally discussed. I've verified the copyrights are in agreement
with the history. The file has relevant copyrights (although missing
source attribution), and all changes of the relevant code are/were
employees of the copyright holder Freescale.

Rob

>
> g.
>
>
>> Who is the copyright holder on a file in the kernel
>> with no copyright? Does this default to the author or Linus or nobody?
>>
>> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Scott Wood <scottwood-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> Cc: Kim Phillips <kim.phillips-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> Cc: Kumar Gala <galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>> ---
>>  libfdt/fdt_ro.c | 203 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  libfdt/libfdt.h |   4 ++
>>  2 files changed, 207 insertions(+)
>>
>> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
>> index 50007f6..3c1bde1 100644
>> --- a/libfdt/fdt_ro.c
>> +++ b/libfdt/fdt_ro.c
>> @@ -571,3 +571,206 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
>>
>>         return offset; /* error from fdt_next_node() */
>>  }
>> +
>> +/* Max address size we deal with */
>> +#define OF_MAX_ADDR_CELLS      4
>> +#define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS && \
>> +                       (ns) > 0)
>> +
>> +/* Helper to read a big number; size is in cells (not bytes) */
>> +static uint64_t fdt_read_number(const fdt32_t *cell, int size)
>> +{
>> +       uint64_t r = 0;
>> +       while (size--)
>> +               r = (r << 32) | fdt32_to_cpu(*(cell++));
>> +       return r;
>> +}
>> +
>> +/* Callbacks for bus specific translators */
>> +struct of_bus {
>> +       void (*count_cells)(void *blob, int parentoffset,
>> +                           int *addrc, int *sizec);
>> +       uint64_t (*map)(fdt32_t *addr, const fdt32_t *range,
>> +                               int na, int ns, int pna);
>> +       int (*translate)(fdt32_t *addr, uint64_t offset, int na);
>> +};
>> +
>> +/* Default translator (generic bus) */
>> +static void fdt_bus_default_count_cells(void *blob, int parentoffset,
>> +                                       int *addrc, int *sizec)
>> +{
>> +       const fdt32_t *prop;
>> +
>> +       if (addrc) {
>> +               prop = fdt_getprop(blob, parentoffset, "#address-cells", NULL);
>> +               if (prop)
>> +                       *addrc = fdt32_to_cpu(*prop);
>> +               else
>> +                       *addrc = 2;
>> +       }
>> +
>> +       if (sizec) {
>> +               prop = fdt_getprop(blob, parentoffset, "#size-cells", NULL);
>> +               if (prop)
>> +                       *sizec = fdt32_to_cpu(*prop);
>> +               else
>> +                       *sizec = 2;
>> +       }
>> +}
>> +
>> +static uint64_t fdt_bus_default_map(fdt32_t *addr, const fdt32_t *range,
>> +                                   int na, int ns, int pna)
>> +{
>> +       uint64_t cp, s, da;
>> +
>> +       cp = fdt_read_number(range, na);
>> +       s  = fdt_read_number(range + na + pna, ns);
>> +       da = fdt_read_number(addr, na);
>> +
>> +       if (da < cp || da >= (cp + s))
>> +               return FDT_BAD_ADDR;
>> +       return da - cp;
>> +}
>> +
>> +static int fdt_bus_default_translate(fdt32_t *addr, uint64_t offset, int na)
>> +{
>> +       uint64_t a = fdt_read_number(addr, na);
>> +       memset(addr, 0, na * 4);
>> +       a += offset;
>> +       if (na > 1)
>> +               addr[na - 2] = cpu_to_fdt32(a >> 32);
>> +       addr[na - 1] = cpu_to_fdt32(a & 0xffffffffu);
>> +
>> +       return 0;
>> +}
>> +
>> +/* Array of bus specific translators */
>> +static const struct of_bus of_busses[] = {
>> +       /* Default */
>> +       {
>> +               .count_cells = fdt_bus_default_count_cells,
>> +               .map = fdt_bus_default_map,
>> +               .translate = fdt_bus_default_translate,
>> +       },
>> +};
>> +
>> +static int fdt_translate_one(void * blob, int parent,
>> +                            const struct of_bus *bus,
>> +                            const struct of_bus *pbus, fdt32_t *addr,
>> +                            int na, int ns, int pna, const char *rprop)
>> +{
>> +       const fdt32_t *ranges;
>> +       int rlen;
>> +       int rone;
>> +       uint64_t offset = FDT_BAD_ADDR;
>> +
>> +       /* Normally, an absence of a "ranges" property means we are
>> +        * crossing a non-translatable boundary, and thus the addresses
>> +        * below the current not cannot be converted to CPU physical ones.
>> +        * Unfortunately, while this is very clear in the spec, it's not
>> +        * what Apple understood, and they do have things like /uni-n or
>> +        * /ht nodes with no "ranges" property and a lot of perfectly
>> +        * useable mapped devices below them. Thus we treat the absence of
>> +        * "ranges" as equivalent to an empty "ranges" property which means
>> +        * a 1:1 translation at that level. It's up to the caller not to try
>> +        * to translate addresses that aren't supposed to be translated in
>> +        * the first place. --BenH.
>> +        */
>> +       ranges = fdt_getprop(blob, parent, rprop, &rlen);
>> +       if (ranges == NULL || rlen == 0) {
>> +               offset = fdt_read_number(addr, na);
>> +               memset(addr, 0, pna * 4);
>> +               goto finish;
>> +       }
>> +
>> +       /* Now walk through the ranges */
>> +       rlen /= 4;
>> +       rone = na + pna + ns;
>> +       for (; rlen >= rone; rlen -= rone, ranges += rone) {
>> +               offset = bus->map(addr, ranges, na, ns, pna);
>> +               if (offset != FDT_BAD_ADDR)
>> +                       break;
>> +       }
>> +       if (offset == FDT_BAD_ADDR)
>> +               return -FDT_ERR_NOTFOUND;
>> +
>> +       memcpy(addr, ranges + na, 4 * pna);
>> +
>> + finish:
>> +       /* Translate it into parent bus space */
>> +       return pbus->translate(addr, offset, pna);
>> +}
>> +
>> +static uint64_t __fdt_translate_address(void *blob, int node_offset,
>> +                                       const char *rprop)
>> +{
>> +       int parent, len;
>> +       const struct of_bus *bus, *pbus;
>> +       const fdt32_t *reg;
>> +       fdt32_t addr[OF_MAX_ADDR_CELLS];
>> +       int na, ns, pna, pns;
>> +       uint64_t result = FDT_BAD_ADDR;
>> +
>> +       reg = fdt_getprop(blob, node_offset, "reg", &len);
>> +       if (!reg)
>> +               goto bail;
>> +
>> +       /* Get parent & match bus type */
>> +       parent = fdt_parent_offset(blob, node_offset);
>> +       if (parent < 0)
>> +               goto bail;
>> +       bus = &of_busses[0];
>> +
>> +       /* Cound address cells & copy address locally */
>> +       bus->count_cells(blob, parent, &na, &ns);
>> +       if (!OF_CHECK_COUNTS(na, ns))
>> +               goto bail;
>> +
>> +       memcpy(addr, reg, na * 4);
>> +
>> +       /* Translate */
>> +       for (;;) {
>> +               /* Switch to parent bus */
>> +               node_offset = parent;
>> +               parent = fdt_parent_offset(blob, node_offset);
>> +
>> +               /* If root, we have finished */
>> +               if (parent < 0) {
>> +                       result = fdt_read_number(addr, na);
>> +                       break;
>> +               }
>> +
>> +               /* Get new parent bus and counts */
>> +               pbus = &of_busses[0];
>> +               pbus->count_cells(blob, parent, &pna, &pns);
>> +               if (!OF_CHECK_COUNTS(pna, pns))
>> +                       break;
>> +
>> +               /* Apply bus translation */
>> +               if (fdt_translate_one(blob, node_offset, bus, pbus,
>> +                                       addr, na, ns, pna, "ranges"))
>> +                       break;
>> +
>> +               /* Complete the move up one level */
>> +               na = pna;
>> +               ns = pns;
>> +               bus = pbus;
>> +       }
>> + bail:
>> +       return result;
>> +}
>> +
>> +/*
>> + * Translate an address from the device-tree into a CPU physical address,
>> + * this walks up the tree and applies the various bus mappings on the
>> + * way.
>> + *
>> + * Note: We consider that crossing any level with #size-cells == 0 to mean
>> + * that translation is impossible (that is we are not dealing with a value
>> + * that can be mapped to a cpu physical address). This is not really specified
>> + * that way, but this is traditionally the way IBM at least do things
>> + */
>> +uint64_t fdt_translate_address(void *fdt, int node_offset)
>> +{
>> +       return __fdt_translate_address(fdt, node_offset, "ranges");
>> +}
>> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
>> index c4d5a91..afbea65 100644
>> --- a/libfdt/libfdt.h
>> +++ b/libfdt/libfdt.h
>> @@ -118,6 +118,8 @@
>>
>>  #define FDT_ERR_MAX            13
>>
>> +#define FDT_BAD_ADDR           ((uint64_t)-1)
>> +
>>  /**********************************************************************/
>>  /* Low-level functions (you probably don't need these)                */
>>  /**********************************************************************/
>> @@ -852,6 +854,8 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
>>   */
>>  int fdt_stringlist_contains(const char *strlist, int listlen, const char *str);
>>
>> +uint64_t fdt_translate_address(void *blob, int node_offset);
>> +
>>  /**********************************************************************/
>>  /* Write-in-place functions                                           */
>>  /**********************************************************************/
>> --
>> 1.8.3.2
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2388 bytes --]

On Tue, Apr 15, 2014 at 10:57:16PM +0100, Grant Likely wrote:
> On Tue, Apr 1, 2014 at 5:57 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > From: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >
> > Add FDT based address translation. Currently, only simple bus
> > translations are supported, but the framework allows adding other
> > buses like PCI. Adding this to libfdt allows the address
> > translation code to be shared amongst u-boot, Linux kernel and other
> > projects.
> >
> > This code is based on GPL only licensed code. It must first be
> > re-licensed for dual GPL/BSD to add to libfdt which requires approval
> > from the copyright holders.
> >
> > This code is copied from u-boot common/fdt_support.c. The portion used
> > here was originally added to u-boot by Kumar Gala in 2010 and Freescale
> > is the copyright holder. Later changes have also only been done by
> > Freescale authors. The u-boot code appears to have been copied from the
> > Linux kernel's address translation code in drivers/of/address.c which
> > has no copyright. This code was moved by Grant Likely in 2010 and
> > originated from arch/powerpc/kernel/prom_parse.c which was written by
> > Ben Herrenschmidt.
> 
> Okay, so this makes things a bit more complicated. You need to compose
> a list of every commit that has touched this file in *both* the u-boot
> and kernel trees, and identify all of the copyright holders based on
> who submitted the patches. Once you have that list, then you need to
> get an Ack from every single one of them. If anyone does not respond,
> or does not give agreement, then we need to look at their change and
> determine whether or not we consider it to be a significant enough
> change that it would warrant copyright protection.

Actually, don't bother.

I'm not all that impressed by this translation code - in particular
the per-bus-type hooks don't make sense to me.  AFAIK the
interpretation of ranges is not bus specific. I think it will also
fail in some cases with #address-cells > 2, which is unfortunate.

I'm about to post my own implementation of address translation.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] libfdt: add address translation support

[Benjamin Herrenschmidt]

On Mon, 2014-05-12 at 16:00 +1000, David Gibson wrote:
> I'm not all that impressed by this translation code - in particular
> the per-bus-type hooks don't make sense to me.  AFAIK the
> interpretation of ranges is not bus specific. I think it will also
> fail in some cases with #address-cells > 2, which is unfortunate.
> 
> I'm about to post my own implementation of address translation.

Well, I came up with the original code which did per-bus type hooks.
There are cases where it is needed.

Take PCI. The top word can contain completely unrelated stuff such
as the "prefetchable" attribute.

It's perfectly legal to put a prefetchable BAR under a non-prefetchable
bridge window.

However a translation scheme that doesn't know to know that bit
will fail.

It's not far fetched, it happens on our machines today.

And that's just one of the problems I had back then...

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[Benjamin Herrenschmidt]

On Mon, 2014-05-12 at 16:10 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2014-05-12 at 16:00 +1000, David Gibson wrote:
> > I'm not all that impressed by this translation code - in particular
> > the per-bus-type hooks don't make sense to me.  AFAIK the
> > interpretation of ranges is not bus specific. I think it will also
> > fail in some cases with #address-cells > 2, which is unfortunate.
> > 
> > I'm about to post my own implementation of address translation.
> 
> Well, I came up with the original code which did per-bus type hooks.
> There are cases where it is needed.
> 
> Take PCI. The top word can contain completely unrelated stuff such
> as the "prefetchable" attribute.
> 
> It's perfectly legal to put a prefetchable BAR under a non-prefetchable
> bridge window.
> 
> However a translation scheme that doesn't know to know that bit
> will fail.

Oh and the other way around with IO vs. memory btw.

> It's not far fetched, it happens on our machines today.
> 
> And that's just one of the problems I had back then...

Note that I'm just stating the problem :-) There may be a better way to
address it. For example, for the one above specifically, well, we could
have a per-bus-type list of masks to apply before translation (bits to
ignore).

I wish in fact OF had done that to begin with like we have
interrupt-map-mask, maybe we should introduce that concept to limit the
damage to existing well known cases like PCI and have anything new just
tell us what to use for the compare.

In fact, thinking more, the above mask trick might not fully work for
PCI, we have to check the exact values, because I think the same field
somewhat use an enumeration for 32-bit MEM, 64-bit MEM and IO ... oh
well.

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2229 bytes --]

On Mon, May 12, 2014 at 04:14:12PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2014-05-12 at 16:10 +1000, Benjamin Herrenschmidt wrote:
> > On Mon, 2014-05-12 at 16:00 +1000, David Gibson wrote:
> > > I'm not all that impressed by this translation code - in particular
> > > the per-bus-type hooks don't make sense to me.  AFAIK the
> > > interpretation of ranges is not bus specific. I think it will also
> > > fail in some cases with #address-cells > 2, which is unfortunate.
> > > 
> > > I'm about to post my own implementation of address translation.
> > 
> > Well, I came up with the original code which did per-bus type hooks.
> > There are cases where it is needed.
> > 
> > Take PCI. The top word can contain completely unrelated stuff such
> > as the "prefetchable" attribute.
> > 
> > It's perfectly legal to put a prefetchable BAR under a non-prefetchable
> > bridge window.
> > 
> > However a translation scheme that doesn't know to know that bit
> > will fail.
> 
> Oh and the other way around with IO vs. memory btw.
> 
> > It's not far fetched, it happens on our machines today.
> > 
> > And that's just one of the problems I had back then...
> 
> Note that I'm just stating the problem :-) There may be a better way to
> address it. For example, for the one above specifically, well, we could
> have a per-bus-type list of masks to apply before translation (bits to
> ignore).
> 
> I wish in fact OF had done that to begin with like we have
> interrupt-map-mask, maybe we should introduce that concept to limit the
> damage to existing well known cases like PCI and have anything new just
> tell us what to use for the compare.
> 
> In fact, thinking more, the above mask trick might not fully work for
> PCI, we have to check the exact values, because I think the same field
> somewhat use an enumeration for 32-bit MEM, 64-bit MEM and IO ... oh
> well.

Hm, yeah.  At this point I'm thinking a per-bus "quirks" mechanism
around the basic universal translation logic.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] libfdt: add address translation support

[Rob Herring]

On Mon, May 12, 2014 at 1:10 AM, Benjamin Herrenschmidt
<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> On Mon, 2014-05-12 at 16:00 +1000, David Gibson wrote:
>> I'm not all that impressed by this translation code - in particular
>> the per-bus-type hooks don't make sense to me.  AFAIK the
>> interpretation of ranges is not bus specific. I think it will also
>> fail in some cases with #address-cells > 2, which is unfortunate.
>>
>> I'm about to post my own implementation of address translation.
>
> Well, I came up with the original code which did per-bus type hooks.
> There are cases where it is needed.
>
> Take PCI. The top word can contain completely unrelated stuff such
> as the "prefetchable" attribute.
>
> It's perfectly legal to put a prefetchable BAR under a non-prefetchable
> bridge window.
>
> However a translation scheme that doesn't know to know that bit
> will fail.
>
> It's not far fetched, it happens on our machines today.
>
> And that's just one of the problems I had back then...

But what you wrote was for unflattened tree. For early FDT uses, do we
really need to worry about PCI or other special cases? The current FDT
address translation code in arch/powerpc/boot/devtree.c (yes, now we
have 3 implementations) does not for example.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[Benjamin Herrenschmidt]

On Mon, 2014-05-12 at 14:27 -0500, Rob Herring wrote:
> But what you wrote was for unflattened tree. For early FDT uses, do we
> really need to worry about PCI or other special cases? The current FDT
> address translation code in arch/powerpc/boot/devtree.c (yes, now we
> have 3 implementations) does not for example.

This is specific to a bootloader case yes and it's ... fishy.

PCI on ARM is here now. PCI is everywhere (well, pseudo-PCI but that's
the basic issue here) on Intel embedded.

Some embedded systems will probably want to enumerate from the firmware
and Linux will need to translate. For example because the UART or some
critical GPIO that needs to be handled early on is behind a pseudo-PCI
interface of some sort.

I think we need to handle these cases rather than fail in obscure ways.

Cheers,
Ben.

 

--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

On Tue, May 13, 2014 at 08:01:05AM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2014-05-12 at 14:27 -0500, Rob Herring wrote:
> > But what you wrote was for unflattened tree. For early FDT uses, do we
> > really need to worry about PCI or other special cases? The current FDT
> > address translation code in arch/powerpc/boot/devtree.c (yes, now we
> > have 3 implementations) does not for example.
> 
> This is specific to a bootloader case yes and it's ... fishy.
> 
> PCI on ARM is here now. PCI is everywhere (well, pseudo-PCI but that's
> the basic issue here) on Intel embedded.
> 
> Some embedded systems will probably want to enumerate from the firmware
> and Linux will need to translate. For example because the UART or some
> critical GPIO that needs to be handled early on is behind a pseudo-PCI
> interface of some sort.
> 
> I think we need to handle these cases rather than fail in obscure ways.

Yeah, I think PCI is way to common to just ignore.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] libfdt: add address translation support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

On Mon, May 12, 2014 at 04:10:02PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2014-05-12 at 16:00 +1000, David Gibson wrote:
> > I'm not all that impressed by this translation code - in particular
> > the per-bus-type hooks don't make sense to me.  AFAIK the
> > interpretation of ranges is not bus specific. I think it will also
> > fail in some cases with #address-cells > 2, which is unfortunate.
> > 
> > I'm about to post my own implementation of address translation.
> 
> Well, I came up with the original code which did per-bus type hooks.
> There are cases where it is needed.
> 
> Take PCI. The top word can contain completely unrelated stuff such
> as the "prefetchable" attribute.
> 
> It's perfectly legal to put a prefetchable BAR under a non-prefetchable
> bridge window.
> 
> However a translation scheme that doesn't know to know that bit
> will fail.
> 
> It's not far fetched, it happens on our machines today.
> 
> And that's just one of the problems I had back then...

Ah.  Bother.

I supposed it's arguable that a PCI ranges property really should have
windows for both prefetchable and non-prefetchable areas, but since
that's not done in practice, it's pretty much moot.

Ok, I'll need to rethink.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] libfdt: add address translation support

[Benjamin Herrenschmidt]

On Wed, 2014-05-14 at 14:18 +1000, David Gibson wrote:
> I supposed it's arguable that a PCI ranges property really should have
> windows for both prefetchable and non-prefetchable areas, but since
> that's not done in practice, it's pretty much moot.

They don't have to, only if there's a relevant difference, ie it's
perfectly legit for a firmware to assign prefetchable BARs in a
non-prefetchable region.

Now granted, those are somewhat special cases so maybe we should
do just that ... special case PCI and try to not continue that mess
with new bindings.

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfdt: add address translation support

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

On Wed, May 14, 2014 at 03:23:58PM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2014-05-14 at 14:18 +1000, David Gibson wrote:
> > I supposed it's arguable that a PCI ranges property really should have
> > windows for both prefetchable and non-prefetchable areas, but since
> > that's not done in practice, it's pretty much moot.
> 
> They don't have to, only if there's a relevant difference, ie it's
> perfectly legit for a firmware to assign prefetchable BARs in a
> non-prefetchable region.

In what sense is the window region non-prefetchable?  Isn't that a
property only of the final destination of the bus cycle?

> Now granted, those are somewhat special cases so maybe we should
> do just that ... special case PCI and try to not continue that mess
> with new bindings.

Yeah, I think that's the way to go.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]