[PATCH 01/14] fdt: Tidy up a few fdtdec problems

[PATCH 01/14] fdt: Tidy up a few fdtdec problems

[Simon Glass]

This fixes three trivial issues in fdtdec.c:
1. fdtdec_get_is_enabled() doesn't really need a default value
2. The fdt must be word-aligned, since otherwise it will fail on ARM
3. The compat_names[] array is missing its first element

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 include/fdtdec.h |    8 ++++----
 lib/fdtdec.c     |    7 ++++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index d871cdd..9018181 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -112,14 +112,14 @@ s32 fdtdec_get_int(const void *blob, int node, const char *prop_name,
  * Checks whether a node is enabled.
  * This looks for a 'status' property. If this exists, then returns 1 if
  * the status is 'ok' and 0 otherwise. If there is no status property,
- * it returns the default value.
+ * it returns 1 on the assumption that anything mentioned should be enabled
+ * by default.
  *
  * @param blob	FDT blob
  * @param node	node to examine
- * @param default_val	default value to return if no 'status' property exists
- * @return integer value 0/1, if found, or default_val if not
+ * @return integer value 0 (not enabled) or 1 (enabled)
  */
-int fdtdec_get_is_enabled(const void *blob, int node, int default_val);
+int fdtdec_get_is_enabled(const void *blob, int node);
 
 /**
  * Checks whether we have a valid fdt available to control U-Boot, and panic
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 0f87163..d1321a8 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -33,6 +33,7 @@ DECLARE_GLOBAL_DATA_PTR;
  */
 #define COMPAT(id, name) name
 static const char * const compat_names[COMPAT_COUNT] = {
+	COMPAT(UNKNOWN, "<none>"),
 };
 
 /**
@@ -84,14 +85,14 @@ s32 fdtdec_get_int(const void *blob, int node, const char *prop_name,
 	return default_val;
 }
 
-int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
+int fdtdec_get_is_enabled(const void *blob, int node)
 {
 	const char *cell;
 
 	cell = fdt_getprop(blob, node, "status", NULL);
 	if (cell)
 		return 0 == strcmp(cell, "ok");
-	return default_val;
+	return 1;
 }
 
 enum fdt_compat_id fd_dec_lookup(const void *blob, int node)
@@ -140,7 +141,7 @@ int fdtdec_next_alias(const void *blob, const char *name,
 int fdtdec_check_fdt(void)
 {
 	/* We must have an fdt */
-	if (fdt_check_header(gd->fdt_blob))
+	if (((uintptr_t)gd->fdt_blob & 3) || fdt_check_header(gd->fdt_blob))
 		panic("No valid fdt found - please append one to U-Boot\n"
 			"binary or define CONFIG_OF_EMBED\n");
 	return 0;
-- 
1.7.3.1

[U-Boot] [PATCH 01/14] fdt: Tidy up a few fdtdec problems

[Simon Glass]

This fixes three trivial issues in fdtdec.c:
1. fdtdec_get_is_enabled() doesn't really need a default value
2. The fdt must be word-aligned, since otherwise it will fail on ARM
3. The compat_names[] array is missing its first element

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 include/fdtdec.h |    8 ++++----
 lib/fdtdec.c     |    7 ++++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index d871cdd..9018181 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -112,14 +112,14 @@ s32 fdtdec_get_int(const void *blob, int node, const char *prop_name,
  * Checks whether a node is enabled.
  * This looks for a 'status' property. If this exists, then returns 1 if
  * the status is 'ok' and 0 otherwise. If there is no status property,
- * it returns the default value.
+ * it returns 1 on the assumption that anything mentioned should be enabled
+ * by default.
  *
  * @param blob	FDT blob
  * @param node	node to examine
- * @param default_val	default value to return if no 'status' property exists
- * @return integer value 0/1, if found, or default_val if not
+ * @return integer value 0 (not enabled) or 1 (enabled)
  */
-int fdtdec_get_is_enabled(const void *blob, int node, int default_val);
+int fdtdec_get_is_enabled(const void *blob, int node);
 
 /**
  * Checks whether we have a valid fdt available to control U-Boot, and panic
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 0f87163..d1321a8 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -33,6 +33,7 @@ DECLARE_GLOBAL_DATA_PTR;
  */
 #define COMPAT(id, name) name
 static const char * const compat_names[COMPAT_COUNT] = {
+	COMPAT(UNKNOWN, "<none>"),
 };
 
 /**
@@ -84,14 +85,14 @@ s32 fdtdec_get_int(const void *blob, int node, const char *prop_name,
 	return default_val;
 }
 
-int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
+int fdtdec_get_is_enabled(const void *blob, int node)
 {
 	const char *cell;
 
 	cell = fdt_getprop(blob, node, "status", NULL);
 	if (cell)
 		return 0 == strcmp(cell, "ok");
-	return default_val;
+	return 1;
 }
 
 enum fdt_compat_id fd_dec_lookup(const void *blob, int node)
@@ -140,7 +141,7 @@ int fdtdec_next_alias(const void *blob, const char *name,
 int fdtdec_check_fdt(void)
 {
 	/* We must have an fdt */
-	if (fdt_check_header(gd->fdt_blob))
+	if (((uintptr_t)gd->fdt_blob & 3) || fdt_check_header(gd->fdt_blob))
 		panic("No valid fdt found - please append one to U-Boot\n"
 			"binary or define CONFIG_OF_EMBED\n");
 	return 0;
-- 
1.7.3.1

[PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Simon Glass]

Add a function to lookup a property which is a phandle in a node, and
another to read a fixed-length integer array from an fdt property.
Also add a function to read boolean properties.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 include/fdtdec.h |   40 ++++++++++++++++++++++++++
 lib/fdtdec.c     |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+), 0 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 9018181..9ecb6ab 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -126,3 +126,43 @@ int fdtdec_get_is_enabled(const void *blob, int node);
  * if not.
  */
 int fdtdec_check_fdt(void);
+
+/**
+ * Look up a phandle and follow it to its node. Then return the offset
+ * of that node.
+ *
+ * @param blob		FDT blob
+ * @param node		node to examine
+ * @param prop_name	name of property to find
+ * @return node offset if found, -ve error code on error
+ */
+int fdtdec_lookup_phandle(const void *blob, int node, const char *prop_name);
+
+/**
+ * Look up a property in a node and return its contents in an integer
+ * array of given length. The property must have at least enough data for
+ * the array (4*count bytes). It may have more, but this will be ignored.
+ *
+ * @param blob		FDT blob
+ * @param node		node to examine
+ * @param prop_name	name of property to find
+ * @param array		array to fill with data
+ * @param count		number of array elements
+ * @return 0 if ok, or -FDT_ERR_NOTFOUND if the property is not found,
+ *		or -FDT_ERR_BADLAYOUT if not enough data
+ */
+int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
+		int *array, int count);
+
+/**
+ * Look up a boolean property in a node and return it.
+ *
+ * A boolean properly is true if present in the device tree and false if not
+ * present, or present with a 0 value.
+ *
+ * @param blob	FDT blob
+ * @param node	node to examine
+ * @param prop_name	name of property to find
+ * @return 1 if the properly is present; 0 if it isn't present or is 0
+ */
+int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index d1321a8..613547a 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -146,3 +146,85 @@ int fdtdec_check_fdt(void)
 			"binary or define CONFIG_OF_EMBED\n");
 	return 0;
 }
+
+int fdtdec_lookup_phandle(const void *blob, int node, const char *prop_name)
+{
+	const u32 *phandle;
+	int lookup;
+
+	phandle = fdt_getprop(blob, node, prop_name, NULL);
+	if (!phandle)
+		return -FDT_ERR_NOTFOUND;
+
+	lookup = fdt_node_offset_by_phandle(blob, fdt32_to_cpu(*phandle));
+	return lookup;
+}
+
+/**
+ * Look up a property in a node and check that it has a minimum length.
+ *
+ * @param blob		FDT blob
+ * @param node		node to examine
+ * @param prop_name	name of property to find
+ * @param min_len	minimum property length in bytes
+ * @param err		0 if ok, or -FDT_ERR_NOTFOUND if the property is not
+			found, or -FDT_ERR_BADLAYOUT if not enough data
+ * @return pointer to cell, which is only valid if err == 0
+ */
+static const void *get_prop_len(const void *blob, int node,
+		const char *prop_name, int min_len, int *err)
+{
+	const void *cell;
+	int len;
+
+	debug("%s: %s\n", __func__, prop_name);
+	cell = fdt_getprop(blob, node, prop_name, &len);
+	if (!cell)
+		*err = -FDT_ERR_NOTFOUND;
+	else if (len < min_len)
+		*err = -FDT_ERR_BADLAYOUT;
+	else
+		*err = 0;
+	return cell;
+}
+
+int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
+		int *array, int count)
+{
+	const s32 *cell;
+	int i, err = 0;
+
+	debug("%s: %s\n", __func__, prop_name);
+	cell = get_prop_len(blob, node, prop_name, sizeof(s32) * count, &err);
+	if (!err) {
+		for (i = 0; i < count; i++)
+			array[i] = fdt32_to_cpu(cell[i]);
+	}
+	return err;
+}
+
+/**
+ * Look up a boolean property in a node and return it.
+ *
+ * A boolean properly is true if present in the device tree and false if not
+ * present, or present with a 0 value.
+ *
+ * @param blob	FDT blob
+ * @param node	node to examine
+ * @param prop_name	name of property to find
+ * @return 1 if the properly is present; 0 if it isn't present or is 0
+ */
+int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
+{
+	const s32 *cell;
+	int len;
+
+	debug("%s: %s\n", __func__, prop_name);
+	cell = fdt_getprop(blob, node, prop_name, &len);
+	if (!cell)
+		return 0;
+	if (len >= sizeof(u32) && *cell == 0)
+		return 0;
+
+	return 1;
+}
-- 
1.7.3.1

[U-Boot] [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Simon Glass]

Add a function to lookup a property which is a phandle in a node, and
another to read a fixed-length integer array from an fdt property.
Also add a function to read boolean properties.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 include/fdtdec.h |   40 ++++++++++++++++++++++++++
 lib/fdtdec.c     |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+), 0 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 9018181..9ecb6ab 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -126,3 +126,43 @@ int fdtdec_get_is_enabled(const void *blob, int node);
  * if not.
  */
 int fdtdec_check_fdt(void);
+
+/**
+ * Look up a phandle and follow it to its node. Then return the offset
+ * of that node.
+ *
+ * @param blob		FDT blob
+ * @param node		node to examine
+ * @param prop_name	name of property to find
+ * @return node offset if found, -ve error code on error
+ */
+int fdtdec_lookup_phandle(const void *blob, int node, const char *prop_name);
+
+/**
+ * Look up a property in a node and return its contents in an integer
+ * array of given length. The property must have at least enough data for
+ * the array (4*count bytes). It may have more, but this will be ignored.
+ *
+ * @param blob		FDT blob
+ * @param node		node to examine
+ * @param prop_name	name of property to find
+ * @param array		array to fill with data
+ * @param count		number of array elements
+ * @return 0 if ok, or -FDT_ERR_NOTFOUND if the property is not found,
+ *		or -FDT_ERR_BADLAYOUT if not enough data
+ */
+int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
+		int *array, int count);
+
+/**
+ * Look up a boolean property in a node and return it.
+ *
+ * A boolean properly is true if present in the device tree and false if not
+ * present, or present with a 0 value.
+ *
+ * @param blob	FDT blob
+ * @param node	node to examine
+ * @param prop_name	name of property to find
+ * @return 1 if the properly is present; 0 if it isn't present or is 0
+ */
+int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index d1321a8..613547a 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -146,3 +146,85 @@ int fdtdec_check_fdt(void)
 			"binary or define CONFIG_OF_EMBED\n");
 	return 0;
 }
+
+int fdtdec_lookup_phandle(const void *blob, int node, const char *prop_name)
+{
+	const u32 *phandle;
+	int lookup;
+
+	phandle = fdt_getprop(blob, node, prop_name, NULL);
+	if (!phandle)
+		return -FDT_ERR_NOTFOUND;
+
+	lookup = fdt_node_offset_by_phandle(blob, fdt32_to_cpu(*phandle));
+	return lookup;
+}
+
+/**
+ * Look up a property in a node and check that it has a minimum length.
+ *
+ * @param blob		FDT blob
+ * @param node		node to examine
+ * @param prop_name	name of property to find
+ * @param min_len	minimum property length in bytes
+ * @param err		0 if ok, or -FDT_ERR_NOTFOUND if the property is not
+			found, or -FDT_ERR_BADLAYOUT if not enough data
+ * @return pointer to cell, which is only valid if err == 0
+ */
+static const void *get_prop_len(const void *blob, int node,
+		const char *prop_name, int min_len, int *err)
+{
+	const void *cell;
+	int len;
+
+	debug("%s: %s\n", __func__, prop_name);
+	cell = fdt_getprop(blob, node, prop_name, &len);
+	if (!cell)
+		*err = -FDT_ERR_NOTFOUND;
+	else if (len < min_len)
+		*err = -FDT_ERR_BADLAYOUT;
+	else
+		*err = 0;
+	return cell;
+}
+
+int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
+		int *array, int count)
+{
+	const s32 *cell;
+	int i, err = 0;
+
+	debug("%s: %s\n", __func__, prop_name);
+	cell = get_prop_len(blob, node, prop_name, sizeof(s32) * count, &err);
+	if (!err) {
+		for (i = 0; i < count; i++)
+			array[i] = fdt32_to_cpu(cell[i]);
+	}
+	return err;
+}
+
+/**
+ * Look up a boolean property in a node and return it.
+ *
+ * A boolean properly is true if present in the device tree and false if not
+ * present, or present with a 0 value.
+ *
+ * @param blob	FDT blob
+ * @param node	node to examine
+ * @param prop_name	name of property to find
+ * @return 1 if the properly is present; 0 if it isn't present or is 0
+ */
+int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
+{
+	const s32 *cell;
+	int len;
+
+	debug("%s: %s\n", __func__, prop_name);
+	cell = fdt_getprop(blob, node, prop_name, &len);
+	if (!cell)
+		return 0;
+	if (len >= sizeof(u32) && *cell == 0)
+		return 0;
+
+	return 1;
+}
-- 
1.7.3.1

[PATCH 03/14] arm: fdt: Ensure that an embedded fdt is word-aligned

[Simon Glass]

By putting the fdt blob into a distinctive area we can ensure that it appears
at the start of the data section and is word-aligned.

Note: It does not seem to be possible to get objcopy to honour its
--section-alignment flag, which would otherwise provide an easier fix
for this problem.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/u-boot.lds |    5 +++++
 dts/Makefile                  |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/cpu/armv7/u-boot.lds b/arch/arm/cpu/armv7/u-boot.lds
index 40ecf78..793e51b 100644
--- a/arch/arm/cpu/armv7/u-boot.lds
+++ b/arch/arm/cpu/armv7/u-boot.lds
@@ -43,6 +43,11 @@ SECTIONS
 
 	. = ALIGN(4);
 	.data : {
+		/*
+		 * Sadly objcopy seems to ignore --section-alignment.
+		 * Put any embedded device tree first so it is aligned.
+		 */
+		*(.dts.data)
 		*(.data)
 	}
 
diff --git a/dts/Makefile b/dts/Makefile
index 5792afd..83547d4 100644
--- a/dts/Makefile
+++ b/dts/Makefile
@@ -79,7 +79,7 @@ $(obj)dt.o: $(DT_BIN)
 	\
 	cd $(dir ${DT_BIN}) && \
 	$(OBJCOPY) -I binary -O $${oformat} -B $${oarch} \
-		$(notdir ${DT_BIN}) $@
+		--prefix-sections=.dts $(notdir ${DT_BIN}) $@
 	rm $(DT_BIN)
 
 OBJS-$(CONFIG_OF_EMBED)	:= dt.o
-- 
1.7.3.1

[U-Boot] [PATCH 03/14] arm: fdt: Ensure that an embedded fdt is word-aligned

[Simon Glass]

By putting the fdt blob into a distinctive area we can ensure that it appears
at the start of the data section and is word-aligned.

Note: It does not seem to be possible to get objcopy to honour its
--section-alignment flag, which would otherwise provide an easier fix
for this problem.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/u-boot.lds |    5 +++++
 dts/Makefile                  |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/cpu/armv7/u-boot.lds b/arch/arm/cpu/armv7/u-boot.lds
index 40ecf78..793e51b 100644
--- a/arch/arm/cpu/armv7/u-boot.lds
+++ b/arch/arm/cpu/armv7/u-boot.lds
@@ -43,6 +43,11 @@ SECTIONS
 
 	. = ALIGN(4);
 	.data : {
+		/*
+		 * Sadly objcopy seems to ignore --section-alignment.
+		 * Put any embedded device tree first so it is aligned.
+		 */
+		*(.dts.data)
 		*(.data)
 	}
 
diff --git a/dts/Makefile b/dts/Makefile
index 5792afd..83547d4 100644
--- a/dts/Makefile
+++ b/dts/Makefile
@@ -79,7 +79,7 @@ $(obj)dt.o: $(DT_BIN)
 	\
 	cd $(dir ${DT_BIN}) && \
 	$(OBJCOPY) -I binary -O $${oformat} -B $${oarch} \
-		$(notdir ${DT_BIN}) $@
+		--prefix-sections=.dts $(notdir ${DT_BIN}) $@
 	rm $(DT_BIN)
 
 OBJS-$(CONFIG_OF_EMBED)	:= dt.o
-- 
1.7.3.1

[PATCH 04/14] arm: fdt: Add skeleton device tree file

[Simon Glass]

This was taken from commit 1ea6b8f at:
git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 arch/arm/dts/skeleton.dtsi |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/dts/skeleton.dtsi

diff --git a/arch/arm/dts/skeleton.dtsi b/arch/arm/dts/skeleton.dtsi
new file mode 100644
index 0000000..b41d241
--- /dev/null
+++ b/arch/arm/dts/skeleton.dtsi
@@ -0,0 +1,13 @@
+/*
+ * Skeleton device tree; the bare minimum needed to boot; just include and
+ * add a compatible value.  The bootloader will typically populate the memory
+ * node.
+ */
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	chosen { };
+	aliases { };
+	memory { device_type = "memory"; reg = <0 0>; };
+};
-- 
1.7.3.1

[U-Boot] [PATCH 04/14] arm: fdt: Add skeleton device tree file

[Simon Glass]

This was taken from commit 1ea6b8f at:
git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/dts/skeleton.dtsi |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/dts/skeleton.dtsi

diff --git a/arch/arm/dts/skeleton.dtsi b/arch/arm/dts/skeleton.dtsi
new file mode 100644
index 0000000..b41d241
--- /dev/null
+++ b/arch/arm/dts/skeleton.dtsi
@@ -0,0 +1,13 @@
+/*
+ * Skeleton device tree; the bare minimum needed to boot; just include and
+ * add a compatible value.  The bootloader will typically populate the memory
+ * node.
+ */
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	chosen { };
+	aliases { };
+	memory { device_type = "memory"; reg = <0 0>; };
+};
-- 
1.7.3.1

[PATCH 05/14] tegra: fdt: Add Tegra2x device tree file

[Simon Glass]

This was taken from commit 1ea6b8f at:
git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git

config.mk is updated to provide this file to boards through the
built-in mechanism:

/include/ ARCH_CPU_DTS

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 arch/arm/cpu/armv7/tegra2/config.mk |    2 +
 arch/arm/dts/tegra20.dtsi           |  147 +++++++++++++++++++++++++++++++++++
 2 files changed, 149 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/dts/tegra20.dtsi

diff --git a/arch/arm/cpu/armv7/tegra2/config.mk b/arch/arm/cpu/armv7/tegra2/config.mk
index 2303dba..fe9ef5b 100644
--- a/arch/arm/cpu/armv7/tegra2/config.mk
+++ b/arch/arm/cpu/armv7/tegra2/config.mk
@@ -31,3 +31,5 @@ CFLAGS_arch/arm/lib/board.o += -march=armv4t
 endif
 
 USE_PRIVATE_LIBGCC = yes
+
+CONFIG_ARCH_DEVICE_TREE := tegra20
diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
new file mode 100644
index 0000000..65d7e6a
--- /dev/null
+++ b/arch/arm/dts/tegra20.dtsi
@@ -0,0 +1,147 @@
+/include/ "skeleton.dtsi"
+
+/ {
+	compatible = "nvidia,tegra20";
+	interrupt-parent = <&intc>;
+
+	intc: interrupt-controller@50041000 {
+		compatible = "nvidia,tegra20-gic";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = < 0x50041000 0x1000 >,
+		      < 0x50040100 0x0100 >;
+	};
+
+	i2c@7000c000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2c";
+		reg = <0x7000C000 0x100>;
+		interrupts = < 70 >;
+	};
+
+	i2c@7000c400 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2c";
+		reg = <0x7000C400 0x100>;
+		interrupts = < 116 >;
+	};
+
+	i2c@7000c500 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2c";
+		reg = <0x7000C500 0x100>;
+		interrupts = < 124 >;
+	};
+
+	i2c@7000d000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2c";
+		reg = <0x7000D000 0x200>;
+		interrupts = < 85 >;
+	};
+
+	i2s@70002800 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2s";
+		reg = <0x70002800 0x200>;
+		interrupts = < 45 >;
+		dma-channel = < 2 >;
+	};
+
+	i2s@70002a00 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2s";
+		reg = <0x70002a00 0x200>;
+		interrupts = < 35 >;
+		dma-channel = < 1 >;
+	};
+
+	das@70000c00 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-das";
+		reg = <0x70000c00 0x80>;
+	};
+
+	gpio: gpio@6000d000 {
+		compatible = "nvidia,tegra20-gpio";
+		reg = < 0x6000d000 0x1000 >;
+		interrupts = < 64 65 66 67 87 119 121 >;
+		#gpio-cells = <2>;
+		gpio-controller;
+	};
+
+	pinmux: pinmux@70000000 {
+		compatible = "nvidia,tegra20-pinmux";
+		reg = < 0x70000014 0x10    /* Tri-state registers */
+			0x70000080 0x20    /* Mux registers */
+			0x700000a0 0x14    /* Pull-up/down registers */
+			0x70000868 0xa8 >; /* Pad control registers */
+	};
+
+	serial@70006000 {
+		compatible = "nvidia,tegra20-uart";
+		reg = <0x70006000 0x40>;
+		reg-shift = <2>;
+		interrupts = < 68 >;
+	};
+
+	serial@70006040 {
+		compatible = "nvidia,tegra20-uart";
+		reg = <0x70006040 0x40>;
+		reg-shift = <2>;
+		interrupts = < 69 >;
+	};
+
+	serial@70006200 {
+		compatible = "nvidia,tegra20-uart";
+		reg = <0x70006200 0x100>;
+		reg-shift = <2>;
+		interrupts = < 78 >;
+	};
+
+	serial@70006300 {
+		compatible = "nvidia,tegra20-uart";
+		reg = <0x70006300 0x100>;
+		reg-shift = <2>;
+		interrupts = < 122 >;
+	};
+
+	serial@70006400 {
+		compatible = "nvidia,tegra20-uart";
+		reg = <0x70006400 0x100>;
+		reg-shift = <2>;
+		interrupts = < 123 >;
+	};
+
+	sdhci@c8000000 {
+		compatible = "nvidia,tegra20-sdhci";
+		reg = <0xc8000000 0x200>;
+		interrupts = < 46 >;
+	};
+
+	sdhci@c8000200 {
+		compatible = "nvidia,tegra20-sdhci";
+		reg = <0xc8000200 0x200>;
+		interrupts = < 47 >;
+	};
+
+	sdhci@c8000400 {
+		compatible = "nvidia,tegra20-sdhci";
+		reg = <0xc8000400 0x200>;
+		interrupts = < 51 >;
+	};
+
+	sdhci@c8000600 {
+		compatible = "nvidia,tegra20-sdhci";
+		reg = <0xc8000600 0x200>;
+		interrupts = < 63 >;
+	};
+};
+
-- 
1.7.3.1

[U-Boot] [PATCH 05/14] tegra: fdt: Add Tegra2x device tree file

[Simon Glass]

This was taken from commit 1ea6b8f at:
git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git

config.mk is updated to provide this file to boards through the
built-in mechanism:

/include/ ARCH_CPU_DTS

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/tegra2/config.mk |    2 +
 arch/arm/dts/tegra20.dtsi           |  147 +++++++++++++++++++++++++++++++++++
 2 files changed, 149 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/dts/tegra20.dtsi

diff --git a/arch/arm/cpu/armv7/tegra2/config.mk b/arch/arm/cpu/armv7/tegra2/config.mk
index 2303dba..fe9ef5b 100644
--- a/arch/arm/cpu/armv7/tegra2/config.mk
+++ b/arch/arm/cpu/armv7/tegra2/config.mk
@@ -31,3 +31,5 @@ CFLAGS_arch/arm/lib/board.o += -march=armv4t
 endif
 
 USE_PRIVATE_LIBGCC = yes
+
+CONFIG_ARCH_DEVICE_TREE := tegra20
diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
new file mode 100644
index 0000000..65d7e6a
--- /dev/null
+++ b/arch/arm/dts/tegra20.dtsi
@@ -0,0 +1,147 @@
+/include/ "skeleton.dtsi"
+
+/ {
+	compatible = "nvidia,tegra20";
+	interrupt-parent = <&intc>;
+
+	intc: interrupt-controller at 50041000 {
+		compatible = "nvidia,tegra20-gic";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = < 0x50041000 0x1000 >,
+		      < 0x50040100 0x0100 >;
+	};
+
+	i2c at 7000c000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2c";
+		reg = <0x7000C000 0x100>;
+		interrupts = < 70 >;
+	};
+
+	i2c at 7000c400 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2c";
+		reg = <0x7000C400 0x100>;
+		interrupts = < 116 >;
+	};
+
+	i2c at 7000c500 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2c";
+		reg = <0x7000C500 0x100>;
+		interrupts = < 124 >;
+	};
+
+	i2c at 7000d000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2c";
+		reg = <0x7000D000 0x200>;
+		interrupts = < 85 >;
+	};
+
+	i2s at 70002800 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2s";
+		reg = <0x70002800 0x200>;
+		interrupts = < 45 >;
+		dma-channel = < 2 >;
+	};
+
+	i2s at 70002a00 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-i2s";
+		reg = <0x70002a00 0x200>;
+		interrupts = < 35 >;
+		dma-channel = < 1 >;
+	};
+
+	das at 70000c00 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-das";
+		reg = <0x70000c00 0x80>;
+	};
+
+	gpio: gpio at 6000d000 {
+		compatible = "nvidia,tegra20-gpio";
+		reg = < 0x6000d000 0x1000 >;
+		interrupts = < 64 65 66 67 87 119 121 >;
+		#gpio-cells = <2>;
+		gpio-controller;
+	};
+
+	pinmux: pinmux at 70000000 {
+		compatible = "nvidia,tegra20-pinmux";
+		reg = < 0x70000014 0x10    /* Tri-state registers */
+			0x70000080 0x20    /* Mux registers */
+			0x700000a0 0x14    /* Pull-up/down registers */
+			0x70000868 0xa8 >; /* Pad control registers */
+	};
+
+	serial at 70006000 {
+		compatible = "nvidia,tegra20-uart";
+		reg = <0x70006000 0x40>;
+		reg-shift = <2>;
+		interrupts = < 68 >;
+	};
+
+	serial at 70006040 {
+		compatible = "nvidia,tegra20-uart";
+		reg = <0x70006040 0x40>;
+		reg-shift = <2>;
+		interrupts = < 69 >;
+	};
+
+	serial at 70006200 {
+		compatible = "nvidia,tegra20-uart";
+		reg = <0x70006200 0x100>;
+		reg-shift = <2>;
+		interrupts = < 78 >;
+	};
+
+	serial at 70006300 {
+		compatible = "nvidia,tegra20-uart";
+		reg = <0x70006300 0x100>;
+		reg-shift = <2>;
+		interrupts = < 122 >;
+	};
+
+	serial at 70006400 {
+		compatible = "nvidia,tegra20-uart";
+		reg = <0x70006400 0x100>;
+		reg-shift = <2>;
+		interrupts = < 123 >;
+	};
+
+	sdhci at c8000000 {
+		compatible = "nvidia,tegra20-sdhci";
+		reg = <0xc8000000 0x200>;
+		interrupts = < 46 >;
+	};
+
+	sdhci at c8000200 {
+		compatible = "nvidia,tegra20-sdhci";
+		reg = <0xc8000200 0x200>;
+		interrupts = < 47 >;
+	};
+
+	sdhci at c8000400 {
+		compatible = "nvidia,tegra20-sdhci";
+		reg = <0xc8000400 0x200>;
+		interrupts = < 51 >;
+	};
+
+	sdhci at c8000600 {
+		compatible = "nvidia,tegra20-sdhci";
+		reg = <0xc8000600 0x200>;
+		interrupts = < 63 >;
+	};
+};
+
-- 
1.7.3.1

[PATCH 06/14] tegra: fdt: Add device tree file for Tegra2 Seaboard

[Simon Glass]

This was taken from commit 1ea6b8f at:
git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 board/nvidia/dts/tegra2-seaboard.dts |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)
 create mode 100644 board/nvidia/dts/tegra2-seaboard.dts

diff --git a/board/nvidia/dts/tegra2-seaboard.dts b/board/nvidia/dts/tegra2-seaboard.dts
new file mode 100644
index 0000000..7d0869a
--- /dev/null
+++ b/board/nvidia/dts/tegra2-seaboard.dts
@@ -0,0 +1,32 @@
+/dts-v1/;
+
+/memreserve/ 0x1c000000 0x04000000;
+/include/ ARCH_CPU_DTS
+
+/ {
+	model = "NVIDIA Seaboard";
+	compatible = "nvidia,seaboard", "nvidia,tegra20";
+
+	chosen {
+		bootargs = "vmalloc=192M video=tegrafb console=ttyS0,115200n8 root=/dev/mmcblk1p3 rw rootwait";
+	};
+
+	memory {
+		device_type = "memory";
+		reg = < 0x00000000 0x40000000 >;
+	};
+
+	serial@70006300 {
+		clock-frequency = < 216000000 >;
+	};
+
+	sdhci@c8000400 {
+		cd-gpios = <&gpio 69 0>; /* gpio PI5 */
+		wp-gpios = <&gpio 57 0>; /* gpio PH1 */
+		power-gpios = <&gpio 70 0>; /* gpio PI6 */
+	};
+
+	sdhci@c8000600 {
+		support-8bit;
+	};
+};
-- 
1.7.3.1

[U-Boot] [PATCH 06/14] tegra: fdt: Add device tree file for Tegra2 Seaboard

[Simon Glass]

This was taken from commit 1ea6b8f at:
git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 board/nvidia/dts/tegra2-seaboard.dts |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)
 create mode 100644 board/nvidia/dts/tegra2-seaboard.dts

diff --git a/board/nvidia/dts/tegra2-seaboard.dts b/board/nvidia/dts/tegra2-seaboard.dts
new file mode 100644
index 0000000..7d0869a
--- /dev/null
+++ b/board/nvidia/dts/tegra2-seaboard.dts
@@ -0,0 +1,32 @@
+/dts-v1/;
+
+/memreserve/ 0x1c000000 0x04000000;
+/include/ ARCH_CPU_DTS
+
+/ {
+	model = "NVIDIA Seaboard";
+	compatible = "nvidia,seaboard", "nvidia,tegra20";
+
+	chosen {
+		bootargs = "vmalloc=192M video=tegrafb console=ttyS0,115200n8 root=/dev/mmcblk1p3 rw rootwait";
+	};
+
+	memory {
+		device_type = "memory";
+		reg = < 0x00000000 0x40000000 >;
+	};
+
+	serial at 70006300 {
+		clock-frequency = < 216000000 >;
+	};
+
+	sdhci at c8000400 {
+		cd-gpios = <&gpio 69 0>; /* gpio PI5 */
+		wp-gpios = <&gpio 57 0>; /* gpio PH1 */
+		power-gpios = <&gpio 70 0>; /* gpio PI6 */
+	};
+
+	sdhci at c8000600 {
+		support-8bit;
+	};
+};
-- 
1.7.3.1

[PATCH 07/14] tegra: fdt: Add initial device tree definitions for USB ports

[Simon Glass]

This is not available in kernel-land yet, but this initial
set of definitions provides the info we need.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 arch/arm/dts/tegra20.dtsi |   80 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index 65d7e6a..28d23da 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -143,5 +143,85 @@
 		reg = <0xc8000600 0x200>;
 		interrupts = < 63 >;
 	};
+
+/* This table has USB timing parameters for each Oscillator frequency we
+ * support. There are four sets of values:
+ *
+ * 1. PLLU configuration information (reference clock is osc/clk_m and
+ * PLLU-FOs are fixed at 12MHz/60MHz/480MHz).
+ *
+ *  Reference frequency     13.0MHz      19.2MHz      12.0MHz      26.0MHz
+ *  ----------------------------------------------------------------------
+ *      DIVN                960 (0x3c0)  200 (0c8)    960 (3c0h)   960 (3c0)
+ *      DIVM                13 (0d)      4 (04)       12 (0c)      26 (1a)
+ * Filter frequency (MHz)   1            4.8          6            2
+ * CPCON                    1100b        0011b        1100b        1100b
+ * LFCON0                   0            0            0            0
+ *
+ * 2. PLL CONFIGURATION & PARAMETERS for different clock generators:
+ *
+ * Reference frequency     13.0MHz         19.2MHz         12.0MHz     26.0MHz
+ * ---------------------------------------------------------------------------
+ * PLLU_ENABLE_DLY_COUNT   02 (0x02)       03 (03)         02 (02)     04 (04)
+ * PLLU_STABLE_COUNT       51 (33)         75 (4B)         47 (2F)    102 (66)
+ * PLL_ACTIVE_DLY_COUNT    05 (05)         06 (06)         04 (04)     09 (09)
+ * XTAL_FREQ_COUNT        127 (7F)        187 (BB)        118 (76)    254 (FE)
+ *
+ * 3. Debounce values IdDig, Avalid, Bvalid, VbusValid, VbusWakeUp, and
+ * SessEnd. Each of these signals have their own debouncer and for each of
+ * those one out of two debouncing times can be chosen (BIAS_DEBOUNCE_A or
+ * BIAS_DEBOUNCE_B).
+ *
+ * The values of DEBOUNCE_A and DEBOUNCE_B are calculated as follows:
+ *    0xffff -> No debouncing at all
+ *    <n> ms = <n> *1000 / (1/19.2MHz) / 4
+ *
+ * So to program a 1 ms debounce for BIAS_DEBOUNCE_A, we have:
+ * BIAS_DEBOUNCE_A[15:0] = 1000 * 19.2 / 4  = 4800 = 0x12c0
+ *
+ * We need to use only DebounceA for BOOTROM. We don't need the DebounceB
+ * values, so we can keep those to default.
+ *
+ * a4. The 20 microsecond delay after bias cell operation.
+ *  Reference frequency     13.0MHz      19.2MHz      12.0MHz      26.0MHz
+ */
+	usbparams@0 {
+		compatible = "nvidia,tegra20-usbparams";
+		osc-frequency = <13000000>;
+		/* DivN, DivM, DivP, CPCON, LFCON, Delays      Debounce, Bias */
+		params = <0x3c0 0x0d 0x00 0xc 0  0x02 0x33 0x05 0x7f  0x7ef4 5>;
+	};
+
+	usbparams@1 {
+		compatible = "nvidia,tegra20-usbparams";
+		osc-frequency = <19200000>;
+		params = <0x0c8 0x04 0x00 0x3 0  0x03 0x4b 0x06 0xbb  0xbb80 7>;
+	};
+
+	usbparams@2 {
+		compatible = "nvidia,tegra20-usbparams";
+		osc-frequency = <12000000>;
+		params = <0x3c0 0x0c 0x00 0xc 0  0x02 0x2f 0x04 0x76  0x7530 5>;
+	};
+
+	usbparams@3 {
+		compatible = "nvidia,tegra20-usbparams";
+		osc-frequency = <26000000>;
+		params = <0x3c0 0x1a 0x00 0xc 0  0x04 0x66 0x09 0xfe  0xfde8 9>;
+	};
+
+	usb@0xc5008000 {
+		compatible = "nvidia,tegra20-usb";
+		reg = <0xc5008000 0x8000>;
+		periph-id = <59>;	// PERIPH_ID_USB3
+		status = "disabled";
+	};
+
+	usb@0xc5000000 {
+		compatible = "nvidia,tegra20-usb";
+		reg = <0xc5000000 0x8000>;
+		periph-id = <22>;	// PERIPH_ID_USBD
+		status = "disabled";
+	};
 };
 
-- 
1.7.3.1

[U-Boot] [PATCH 07/14] tegra: fdt: Add initial device tree definitions for USB ports

[Simon Glass]

This is not available in kernel-land yet, but this initial
set of definitions provides the info we need.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/dts/tegra20.dtsi |   80 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index 65d7e6a..28d23da 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -143,5 +143,85 @@
 		reg = <0xc8000600 0x200>;
 		interrupts = < 63 >;
 	};
+
+/* This table has USB timing parameters for each Oscillator frequency we
+ * support. There are four sets of values:
+ *
+ * 1. PLLU configuration information (reference clock is osc/clk_m and
+ * PLLU-FOs are fixed at 12MHz/60MHz/480MHz).
+ *
+ *  Reference frequency     13.0MHz      19.2MHz      12.0MHz      26.0MHz
+ *  ----------------------------------------------------------------------
+ *      DIVN                960 (0x3c0)  200 (0c8)    960 (3c0h)   960 (3c0)
+ *      DIVM                13 (0d)      4 (04)       12 (0c)      26 (1a)
+ * Filter frequency (MHz)   1            4.8          6            2
+ * CPCON                    1100b        0011b        1100b        1100b
+ * LFCON0                   0            0            0            0
+ *
+ * 2. PLL CONFIGURATION & PARAMETERS for different clock generators:
+ *
+ * Reference frequency     13.0MHz         19.2MHz         12.0MHz     26.0MHz
+ * ---------------------------------------------------------------------------
+ * PLLU_ENABLE_DLY_COUNT   02 (0x02)       03 (03)         02 (02)     04 (04)
+ * PLLU_STABLE_COUNT       51 (33)         75 (4B)         47 (2F)    102 (66)
+ * PLL_ACTIVE_DLY_COUNT    05 (05)         06 (06)         04 (04)     09 (09)
+ * XTAL_FREQ_COUNT        127 (7F)        187 (BB)        118 (76)    254 (FE)
+ *
+ * 3. Debounce values IdDig, Avalid, Bvalid, VbusValid, VbusWakeUp, and
+ * SessEnd. Each of these signals have their own debouncer and for each of
+ * those one out of two debouncing times can be chosen (BIAS_DEBOUNCE_A or
+ * BIAS_DEBOUNCE_B).
+ *
+ * The values of DEBOUNCE_A and DEBOUNCE_B are calculated as follows:
+ *    0xffff -> No debouncing at all
+ *    <n> ms = <n> *1000 / (1/19.2MHz) / 4
+ *
+ * So to program a 1 ms debounce for BIAS_DEBOUNCE_A, we have:
+ * BIAS_DEBOUNCE_A[15:0] = 1000 * 19.2 / 4  = 4800 = 0x12c0
+ *
+ * We need to use only DebounceA for BOOTROM. We don't need the DebounceB
+ * values, so we can keep those to default.
+ *
+ * a4. The 20 microsecond delay after bias cell operation.
+ *  Reference frequency     13.0MHz      19.2MHz      12.0MHz      26.0MHz
+ */
+	usbparams at 0 {
+		compatible = "nvidia,tegra20-usbparams";
+		osc-frequency = <13000000>;
+		/* DivN, DivM, DivP, CPCON, LFCON, Delays      Debounce, Bias */
+		params = <0x3c0 0x0d 0x00 0xc 0  0x02 0x33 0x05 0x7f  0x7ef4 5>;
+	};
+
+	usbparams at 1 {
+		compatible = "nvidia,tegra20-usbparams";
+		osc-frequency = <19200000>;
+		params = <0x0c8 0x04 0x00 0x3 0  0x03 0x4b 0x06 0xbb  0xbb80 7>;
+	};
+
+	usbparams at 2 {
+		compatible = "nvidia,tegra20-usbparams";
+		osc-frequency = <12000000>;
+		params = <0x3c0 0x0c 0x00 0xc 0  0x02 0x2f 0x04 0x76  0x7530 5>;
+	};
+
+	usbparams at 3 {
+		compatible = "nvidia,tegra20-usbparams";
+		osc-frequency = <26000000>;
+		params = <0x3c0 0x1a 0x00 0xc 0  0x04 0x66 0x09 0xfe  0xfde8 9>;
+	};
+
+	usb at 0xc5008000 {
+		compatible = "nvidia,tegra20-usb";
+		reg = <0xc5008000 0x8000>;
+		periph-id = <59>;	// PERIPH_ID_USB3
+		status = "disabled";
+	};
+
+	usb at 0xc5000000 {
+		compatible = "nvidia,tegra20-usb";
+		reg = <0xc5000000 0x8000>;
+		periph-id = <22>;	// PERIPH_ID_USBD
+		status = "disabled";
+	};
 };
 
-- 
1.7.3.1

[U-Boot] [PATCH 08/14] tegra: usb: Add USB definitions for Tegra2 Seaboard

[Simon Glass]

We set up two USB ports, one of which can be host or device.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 board/nvidia/dts/tegra2-seaboard.dts |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/board/nvidia/dts/tegra2-seaboard.dts b/board/nvidia/dts/tegra2-seaboard.dts
index 7d0869a..395d3fa 100644
--- a/board/nvidia/dts/tegra2-seaboard.dts
+++ b/board/nvidia/dts/tegra2-seaboard.dts
@@ -11,6 +11,12 @@
 		bootargs = "vmalloc=192M video=tegrafb console=ttyS0,115200n8 root=/dev/mmcblk1p3 rw rootwait";
 	};
 
+        aliases {
+		/* This defines the order of our USB ports */
+                usb0 = "/usb at 0xc5008000";
+                usb1 = "/usb at 0xc5000000";
+        };
+
 	memory {
 		device_type = "memory";
 		reg = < 0x00000000 0x40000000 >;
@@ -29,4 +35,19 @@
 	sdhci at c8000600 {
 		support-8bit;
 	};
+
+	usb at 0xc5000000 {
+		status = "ok";
+		support-host-mode;
+	};
+
+	usbphy: usbphy at 0 {
+		compatible = "smsc,usb3315";
+		status = "ok";
+	};
+
+	usb at 0xc5008000 {
+		status = "ok";
+		utmi = <&usbphy>;
+	};
 };
-- 
1.7.3.1

[U-Boot] [PATCH 09/14] tegra: usb: Add support for data alignment and txfifo threshold

[Simon Glass]

CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of data for
USB packets (e.g. 4 means word-aligned). This is required for Tegra
to operate.

CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the txfilltuning
field in the EHCI controller on reset.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 README                      |    7 +++++++
 drivers/usb/host/ehci-hcd.c |   39 +++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/ehci.h     |    6 +++++-
 3 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/README b/README
index 07f1d11..d3289d2 100644
--- a/README
+++ b/README
@@ -1096,6 +1096,13 @@ The following options need to be configured:
 				May be defined to allow interrupt polling
 				instead of using asynchronous interrupts
 
+		CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of
+		data for USB packets (e.g. 4 means word-aligned). This is
+		required for Tegra to operate.
+
+		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
+		txfilltuning field in the EHCI controller on reset.
+
 - USB Device:
 		Define the below if you wish to use the USB console.
 		Once firmware is rebuilt from a serial console issue the
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 2197119..d3eeefe 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -247,6 +247,13 @@ static int ehci_reset(void)
 #endif
 		ehci_writel(reg_ptr, tmp);
 	}
+
+#ifdef CONFIG_USB_EHCI_TXFIFO_THRESH
+	cmd = ehci_readl(&hcor->or_txfilltuning);
+	cmd &= ~TXFIFO_THRESH(0x3f);
+	cmd |= TXFIFO_THRESH(CONFIG_USB_EHCI_TXFIFO_THRESH);
+	ehci_writel(&hcor->or_txfilltuning, cmd);
+#endif
 out:
 	return ret;
 }
@@ -322,6 +329,27 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 	int timeout;
 	int ret = 0;
 
+#ifdef CONFIG_USB_EHCI_DATA_ALIGN
+	/* In case ehci host requires alignment for buffers */
+	void *align_buf = NULL;
+	void *orig_buf = buffer;
+	int unaligned = ((int)buffer & (CONFIG_USB_EHCI_DATA_ALIGN - 1)) != 0;
+
+	if (unaligned) {
+		align_buf = malloc(length + CONFIG_USB_EHCI_DATA_ALIGN);
+		if (!align_buf)
+			return -1;
+		if ((int)align_buf & (CONFIG_USB_EHCI_DATA_ALIGN - 1))
+			buffer = (void *)((int)align_buf +
+				CONFIG_USB_EHCI_DATA_ALIGN -
+				((int)align_buf &
+					(CONFIG_USB_EHCI_DATA_ALIGN - 1)));
+		else
+			buffer = align_buf;
+		if (usb_pipeout(pipe))
+			memcpy(buffer, orig_buf, length);
+	}
+#endif
 	debug("dev=%p, pipe=%lx, buffer=%p, length=%d, req=%p\n", dev, pipe,
 	      buffer, length, req);
 	if (req != NULL)
@@ -513,9 +541,20 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 		      ehci_readl(&hcor->or_portsc[1]));
 	}
 
+#ifdef CONFIG_USB_EHCI_DATA_ALIGN
+	if (unaligned) {
+		if (usb_pipein(pipe) && dev->act_len)
+			memcpy(orig_buf, buffer, length);
+		free(align_buf);
+	}
+#endif
 	return (dev->status != USB_ST_NOT_PROC) ? 0 : -1;
 
 fail:
+#ifdef CONFIG_USB_EHCI_DATA_ALIGN
+	if (unaligned)
+		free(align_buf);
+#endif
 	td = (void *)hc32_to_cpu(qh->qh_overlay.qt_next);
 	while (td != (void *)QT_NEXT_TERMINATE) {
 		qh->qh_overlay.qt_next = td->qt_next;
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 3d0ad0c..cc00ce4 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -80,7 +80,11 @@ struct ehci_hcor {
 	uint32_t or_ctrldssegment;
 	uint32_t or_periodiclistbase;
 	uint32_t or_asynclistaddr;
-	uint32_t _reserved_[9];
+	uint32_t _reserved_0_;
+	uint32_t or_burstsize;
+	uint32_t or_txfilltuning;
+#define TXFIFO_THRESH(p)		((p & 0x3f) << 16)
+	uint32_t _reserved_1_[6];
 	uint32_t or_configflag;
 #define FLAG_CF		(1 << 0)	/* true:  we'll support "high speed" */
 	uint32_t or_portsc[CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS];
-- 
1.7.3.1

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

This adds basic support for the Tegra2 USB controller. Board files should
call board_usb_init() to set things up.

Configuration through FDT and CONFIG is supported. For FDT, the device
tree aliases set the order of the port, like this fragment:

        aliases {
		/* This defines the order of our USB ports */
                usb0 = "/usb at 0xc5008000";
                usb1 = "/usb at 0xc5000000";
        };

For CONFIG-based configuration, where CONFIG_OF_CONTROL is not defined,
USB ports rely on CONFIG_TEGRA2_USBx macros to select the ordering.
For example:

	#define CONFIG_TEGRA_USB0	TEGRA_USB3_BASE
	#define CONFIG_TEGRA_USB1	TEGRA_USB1_BASE

We record the order that the ports are configured and use that to select
ports by number.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/tegra2/Makefile        |    4 +-
 arch/arm/cpu/armv7/tegra2/usb.c           |  494 +++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-tegra2/tegra2.h |    2 +
 arch/arm/include/asm/arch-tegra2/usb.h    |  255 +++++++++++++++
 drivers/usb/host/Makefile                 |    1 +
 drivers/usb/host/ehci-tegra.c             |   63 ++++
 include/fdtdec.h                          |    1 +
 lib/fdtdec.c                              |    1 +
 8 files changed, 820 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/tegra2/usb.c
 create mode 100644 arch/arm/include/asm/arch-tegra2/usb.h
 create mode 100644 drivers/usb/host/ehci-tegra.c

diff --git a/arch/arm/cpu/armv7/tegra2/Makefile b/arch/arm/cpu/armv7/tegra2/Makefile
index 955c3b6..70e7abd 100644
--- a/arch/arm/cpu/armv7/tegra2/Makefile
+++ b/arch/arm/cpu/armv7/tegra2/Makefile
@@ -33,8 +33,10 @@ include $(TOPDIR)/config.mk
 LIB	=  $(obj)lib$(SOC).o
 
 SOBJS	:= lowlevel_init.o
-COBJS	:= ap20.o board.o clock.o pinmux.o sys_info.o timer.o
+COBJS-y	:= ap20.o board.o clock.o pinmux.o sys_info.o timer.o
+COBJS-$(CONFIG_USB_EHCI_TEGRA) += usb.o
 
+COBJS	:= $(COBJS-y)
 SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS) $(SOBJS))
 
diff --git a/arch/arm/cpu/armv7/tegra2/usb.c b/arch/arm/cpu/armv7/tegra2/usb.c
new file mode 100644
index 0000000..311c0ca
--- /dev/null
+++ b/arch/arm/cpu/armv7/tegra2/usb.c
@@ -0,0 +1,494 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * (C) Copyright 2010,2011 NVIDIA Corporation <www.nvidia.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm-generic/gpio.h>
+#include <asm/arch/tegra2.h>
+#include <asm/arch/clk_rst.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/gpio.h>
+#include <asm/arch/pinmux.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/arch/uart.h>
+#include <asm/arch/usb.h>
+#include <libfdt.h>
+#include <fdtdec.h>
+
+enum {
+	USB_PORTS_MAX	= 4,			/* Maximum ports we allow */
+};
+
+struct usb_port {
+	struct usb_ctlr *reg;
+};
+
+static struct usb_port port[USB_PORTS_MAX];	/* List of valid USB ports */
+static unsigned port_count;			/* Number of available ports */
+static int port_current;			/* Current port (-1 = none) */
+
+/* Record which controller can switch from host to device mode */
+static struct usb_ctlr *host_dev_ctlr;
+
+/* Parameters we need for USB */
+enum {
+	PARAM_DIVN,                     /* PLL FEEDBACK DIVIDer */
+	PARAM_DIVM,                     /* PLL INPUT DIVIDER */
+	PARAM_DIVP,                     /* POST DIVIDER (2^N) */
+	PARAM_CPCON,                    /* BASE PLLC CHARGE Pump setup ctrl */
+	PARAM_LFCON,                    /* BASE PLLC LOOP FILter setup ctrl */
+	PARAM_ENABLE_DELAY_COUNT,       /* PLL-U Enable Delay Count */
+	PARAM_STABLE_COUNT,             /* PLL-U STABLE count */
+	PARAM_ACTIVE_DELAY_COUNT,       /* PLL-U Active delay count */
+	PARAM_XTAL_FREQ_COUNT,          /* PLL-U XTAL frequency count */
+	PARAM_DEBOUNCE_A_TIME,          /* 10MS DELAY for BIAS_DEBOUNCE_A */
+	PARAM_BIAS_TIME,                /* 20US DELAY AFter bias cell op */
+
+	PARAM_COUNT
+};
+
+/* Information about USB */
+struct fdt_usb {
+	struct usb_ctlr *reg;   /* address of registers in physical memory */
+	int params[PARAM_COUNT]; /* timing parameters */
+	int host_mode;          /* 1 if port has host mode, else 0 */
+	int utmi;               /* 1 if port has external tranceiver, else 0 */
+	int enabled;            /* 1 to enable, 0 to disable */
+	enum periph_id periph_id;/* peripheral id */
+};
+
+#ifndef CONFIG_OF_CONTROL
+/*
+ * This table has USB timing parameters for each Oscillator frequency we
+ * support. There are four sets of values:
+ *
+ * 1. PLLU configuration information (reference clock is osc/clk_m and
+ * PLLU-FOs are fixed at 12MHz/60MHz/480MHz).
+ *
+ *  Reference frequency     13.0MHz      19.2MHz      12.0MHz      26.0MHz
+ *  ----------------------------------------------------------------------
+ *      DIVN                960 (0x3c0)  200 (0c8)    960 (3c0h)   960 (3c0)
+ *      DIVM                13 (0d)      4 (04)       12 (0c)      26 (1a)
+ * Filter frequency (MHz)   1            4.8          6            2
+ * CPCON                    1100b        0011b        1100b        1100b
+ * LFCON0                   0            0            0            0
+ *
+ * 2. PLL CONFIGURATION & PARAMETERS for different clock generators:
+ *
+ * Reference frequency     13.0MHz         19.2MHz         12.0MHz     26.0MHz
+ * ---------------------------------------------------------------------------
+ * PLLU_ENABLE_DLY_COUNT   02 (0x02)       03 (03)         02 (02)     04 (04)
+ * PLLU_STABLE_COUNT       51 (33)         75 (4B)         47 (2F)    102 (66)
+ * PLL_ACTIVE_DLY_COUNT    05 (05)         06 (06)         04 (04)     09 (09)
+ * XTAL_FREQ_COUNT        127 (7F)        187 (BB)        118 (76)    254 (FE)
+ *
+ * 3. Debounce values IdDig, Avalid, Bvalid, VbusValid, VbusWakeUp, and
+ * SessEnd. Each of these signals have their own debouncer and for each of
+ * those one out of two debouncing times can be chosen (BIAS_DEBOUNCE_A or
+ * BIAS_DEBOUNCE_B).
+ *
+ * The values of DEBOUNCE_A and DEBOUNCE_B are calculated as follows:
+ *    0xffff -> No debouncing at all
+ *    <n> ms = <n> *1000 / (1/19.2MHz) / 4
+ *
+ * So to program a 1 ms debounce for BIAS_DEBOUNCE_A, we have:
+ * BIAS_DEBOUNCE_A[15:0] = 1000 * 19.2 / 4  = 4800 = 0x12c0
+ *
+ * We need to use only DebounceA for BOOTROM. We don't need the DebounceB
+ * values, so we can keep those to default.
+ *
+ * 4. The 20 microsecond delay after bias cell operation.
+ */
+static const int usb_pll[CLOCK_OSC_FREQ_COUNT][PARAM_COUNT] = {
+	/* DivN, DivM, DivP, CPCON, LFCON, Delays             Debounce, Bias */
+	{ 0x3C0, 0x0D, 0x00, 0xC,   0,  0x02, 0x33, 0x05, 0x7F, 0x7EF4, 5 },
+	{ 0x0C8, 0x04, 0x00, 0x3,   0,  0x03, 0x4B, 0x06, 0xBB, 0xBB80, 7 },
+	{ 0x3C0, 0x0C, 0x00, 0xC,   0,  0x02, 0x2F, 0x04, 0x76, 0x7530, 5 },
+	{ 0x3C0, 0x1A, 0x00, 0xC,   0,  0x04, 0x66, 0x09, 0xFE, 0xFDE8, 9 }
+};
+#endif
+
+/* UTMIP Idle Wait Delay */
+static const u8 utmip_idle_wait_delay = 17;
+
+/* UTMIP Elastic limit */
+static const u8 utmip_elastic_limit = 16;
+
+/* UTMIP High Speed Sync Start Delay */
+static const u8 utmip_hs_sync_start_delay = 9;
+
+/* Put the port into host mode (this only works for USB1) */
+static void set_host_mode(struct usb_ctlr *usbctlr)
+{
+	/* Check whether remote host from USB1 is driving VBus */
+	if (readl(&usbctlr->phy_vbus_sensors) & VBUS_VLD_STS)
+		return;
+
+	/*
+	 * If not driving, we set GPIO USB1_VBus_En. Seaboard platform uses
+	 * PAD SLXK (GPIO D.00) as USB1_VBus_En Config as GPIO
+	 */
+	gpio_direction_output(GPIO_PD0, 1);
+
+	/* Z_SLXK = 0, normal, not tristate */
+	pinmux_tristate_disable(PINGRP_SLXK);
+}
+
+/* Put our ports into host mode */
+void usb_set_host_mode(void)
+{
+	if (host_dev_ctlr)
+		set_host_mode(host_dev_ctlr);
+}
+
+void usbf_reset_controller(enum periph_id id, struct usb_ctlr *usbctlr)
+{
+	/* Reset the USB controller with 2us delay */
+	reset_periph(id, 2);
+
+	/*
+	 * Set USB1_NO_LEGACY_MODE to 1, Registers are accessible under
+	 * base address
+	 */
+	if (id == PERIPH_ID_USBD)
+		setbits_le32(&usbctlr->usb1_legacy_ctrl, USB1_NO_LEGACY_MODE);
+
+	/* Put UTMIP1/3 in reset */
+	setbits_le32(&usbctlr->susp_ctrl, UTMIP_RESET);
+
+	/* Set USB3 to use UTMIP PHY */
+	if (id == PERIPH_ID_USB3)
+		setbits_le32(&usbctlr->susp_ctrl, UTMIP_PHY_ENB);
+
+	/*
+	 * TODO: where do we take the USB1 out of reset? The old code would
+	 * take USB3 out of reset, but not USB1. This code doesn't do either.
+	 */
+}
+
+/* set up the USB controller with the parameters provided */
+static void init_usb_controller(enum periph_id id, struct usb_ctlr *usbctlr,
+		const int *params)
+{
+	u32 val;
+	int loop_count;
+
+	clock_enable(id);
+
+	/* Reset the usb controller */
+	usbf_reset_controller(id, usbctlr);
+
+	/* Stop crystal clock by setting UTMIP_PHY_XTAL_CLOCKEN low */
+	clrbits_le32(&usbctlr->utmip_misc_cfg1, UTMIP_PHY_XTAL_CLOCKEN);
+
+	/* Follow the crystal clock disable by >100ns delay */
+	udelay(1);
+
+	/*
+	 * To Use the A Session Valid for cable detection logic, VBUS_WAKEUP
+	 * mux must be switched to actually use a_sess_vld threshold.
+	 */
+	if (id == PERIPH_ID_USBD) {
+		clrsetbits_le32(&usbctlr->usb1_legacy_ctrl,
+			VBUS_SENSE_CTL_MASK, VBUS_SENSE_CTL_A_SESS_VLD);
+	}
+
+	/*
+	 * PLL Delay CONFIGURATION settings. The following parameters control
+	 * the bring up of the plls.
+	 */
+	val = readl(&usbctlr->utmip_misc_cfg1);
+	clrsetbits_le32(&val, UTMIP_PLLU_STABLE_COUNT_MASK,
+		params[PARAM_STABLE_COUNT] << UTMIP_PLLU_STABLE_COUNT_SHIFT);
+	clrsetbits_le32(&val, UTMIP_PLL_ACTIVE_DLY_COUNT_MASK,
+		params[PARAM_ACTIVE_DELAY_COUNT] <<
+			UTMIP_PLL_ACTIVE_DLY_COUNT_SHIFT);
+	writel(val, &usbctlr->utmip_misc_cfg1);
+
+	/* Set PLL enable delay count and crystal frequency count */
+	val = readl(&usbctlr->utmip_pll_cfg1);
+	clrsetbits_le32(&val, UTMIP_PLLU_ENABLE_DLY_COUNT_MASK,
+		params[PARAM_ENABLE_DELAY_COUNT] <<
+			UTMIP_PLLU_ENABLE_DLY_COUNT_SHIFT);
+	clrsetbits_le32(&val, UTMIP_XTAL_FREQ_COUNT_MASK,
+		params[PARAM_XTAL_FREQ_COUNT] <<
+			UTMIP_XTAL_FREQ_COUNT_SHIFT);
+	writel(val, &usbctlr->utmip_pll_cfg1);
+
+	/* Setting the tracking length time */
+	clrsetbits_le32(&usbctlr->utmip_bias_cfg1,
+		UTMIP_BIAS_PDTRK_COUNT_MASK,
+		params[PARAM_BIAS_TIME] << UTMIP_BIAS_PDTRK_COUNT_SHIFT);
+
+	/* Program debounce time for VBUS to become valid */
+	clrsetbits_le32(&usbctlr->utmip_debounce_cfg0,
+		UTMIP_DEBOUNCE_CFG0_MASK,
+		params[PARAM_DEBOUNCE_A_TIME] << UTMIP_DEBOUNCE_CFG0_SHIFT);
+
+	setbits_le32(&usbctlr->utmip_tx_cfg0, UTMIP_FS_PREAMBLE_J);
+
+	/* Disable battery charge enabling bit */
+	setbits_le32(&usbctlr->utmip_bat_chrg_cfg0, UTMIP_PD_CHRG);
+
+	clrbits_le32(&usbctlr->utmip_xcvr_cfg0, UTMIP_XCVR_LSBIAS_SE);
+	setbits_le32(&usbctlr->utmip_spare_cfg0, FUSE_SETUP_SEL);
+
+	/*
+	 * Configure the UTMIP_IDLE_WAIT and UTMIP_ELASTIC_LIMIT
+	 * Setting these fields, together with default values of the
+	 * other fields, results in programming the registers below as
+	 * follows:
+	 *         UTMIP_HSRX_CFG0 = 0x9168c000
+	 *         UTMIP_HSRX_CFG1 = 0x13
+	 */
+
+	/* Set PLL enable delay count and Crystal frequency count */
+	val = readl(&usbctlr->utmip_hsrx_cfg0);
+	clrsetbits_le32(&val, UTMIP_IDLE_WAIT_MASK,
+		utmip_idle_wait_delay << UTMIP_IDLE_WAIT_SHIFT);
+	clrsetbits_le32(&val, UTMIP_ELASTIC_LIMIT_MASK,
+		utmip_elastic_limit << UTMIP_ELASTIC_LIMIT_SHIFT);
+	writel(val, &usbctlr->utmip_hsrx_cfg0);
+
+	/* Configure the UTMIP_HS_SYNC_START_DLY */
+	clrsetbits_le32(&usbctlr->utmip_hsrx_cfg1,
+		UTMIP_HS_SYNC_START_DLY_MASK,
+		utmip_hs_sync_start_delay << UTMIP_HS_SYNC_START_DLY_SHIFT);
+
+	/* Preceed the crystal clock disable by >100ns delay. */
+	udelay(1);
+
+	/* Resuscitate crystal clock by setting UTMIP_PHY_XTAL_CLOCKEN */
+	setbits_le32(&usbctlr->utmip_misc_cfg1, UTMIP_PHY_XTAL_CLOCKEN);
+
+	/* Finished the per-controller init. */
+
+	/* De-assert UTMIP_RESET to bring out of reset. */
+	clrbits_le32(&usbctlr->susp_ctrl, UTMIP_RESET);
+
+	/* Wait for the phy clock to become valid in 100 ms */
+	for (loop_count = 100000; loop_count != 0; loop_count--) {
+		if (readl(&usbctlr->susp_ctrl) & USB_PHY_CLK_VALID)
+			break;
+		udelay(1);
+	}
+}
+
+static void power_up_port(struct usb_ctlr *usbctlr)
+{
+	/* Deassert power down state */
+	clrbits_le32(&usbctlr->utmip_xcvr_cfg0, UTMIP_FORCE_PD_POWERDOWN |
+		UTMIP_FORCE_PD2_POWERDOWN | UTMIP_FORCE_PDZI_POWERDOWN);
+	clrbits_le32(&usbctlr->utmip_xcvr_cfg1, UTMIP_FORCE_PDDISC_POWERDOWN |
+		UTMIP_FORCE_PDCHRP_POWERDOWN | UTMIP_FORCE_PDDR_POWERDOWN);
+}
+
+static void config_clock(const int params[])
+{
+	clock_start_pll(CLOCK_ID_USB,
+		params[PARAM_DIVM], params[PARAM_DIVN], params[PARAM_DIVP],
+		params[PARAM_CPCON], params[PARAM_LFCON]);
+}
+
+/**
+ * Add a new USB port to the list of available ports
+ *
+ * @param id		peripheral id of port (PERIPH_ID_USB3, for example)
+ * @param usbctlr	register address of controller
+ * @param params	timing parameters
+ * @param utmi		1 if it has an external UTMI transceiver
+ * @return 0 if ok, -1 if error (too many ports)
+ */
+static int add_port(enum periph_id id, struct usb_ctlr *usbctlr,
+		const int params[], int utmi)
+{
+	if (port_count == USB_PORTS_MAX) {
+		debug("tegrausb: Cannot register more than %d ports\n",
+		      USB_PORTS_MAX);
+		return -1;
+	}
+	init_usb_controller(id, usbctlr, params);
+	if (utmi) {
+		/* Disable ICUSB FS/LS transceiver */
+		clrbits_le32(&usbctlr->icusb_ctrl, IC_ENB1);
+
+		/* Select UTMI parallel interface */
+		clrsetbits_le32(&usbctlr->port_sc1, PTS_MASK,
+				PTS_UTMI << PTS_SHIFT);
+		clrbits_le32(&usbctlr->port_sc1, STS);
+		power_up_port(usbctlr);
+	}
+	port[port_count++].reg = usbctlr;
+	return 0;
+}
+
+#ifndef CONFIG_OF_CONTROL
+static int probe_port(struct usb_ctlr *usbctlr, const int params[])
+{
+	enum periph_id id;
+	int utmi = 0;
+
+	/*
+	 * Get the periph id. Port 1 has an internal transceiver, port 3 is
+	 * external
+	 */
+	switch ((u32)usbctlr) {
+	case TEGRA_USB1_BASE:
+		id = PERIPH_ID_USBD;
+		break;
+
+	case TEGRA_USB3_BASE:
+		id = PERIPH_ID_USB3;
+		utmi = 1;
+		break;
+
+	default:
+		printf("tegrausb: probe_port: no such port %p\n", usbctlr);
+		return -1;
+	}
+
+	return add_port(id, usbctlr, params, utmi);
+}
+#endif
+
+int tegrausb_start_port(unsigned portnum, u32 *hccr, u32 *hcor)
+{
+	struct usb_ctlr *usbctlr;
+
+	if (portnum >= port_count)
+		return -1;
+	tegrausb_stop_port();
+
+	usbctlr = port[portnum].reg;
+	*hccr = (u32)&usbctlr->cap_length;
+	*hcor = (u32)&usbctlr->usb_cmd;
+	port_current = portnum;
+	return 0;
+}
+
+int tegrausb_stop_port(void)
+{
+	struct usb_ctlr *usbctlr;
+
+	if (port_current == -1)
+		return -1;
+
+	usbctlr = port[port_current].reg;
+
+	/* Stop controller */
+	writel(0, &usbctlr->usb_cmd);
+	udelay(1000);
+
+	/* Initiate controller reset */
+	writel(2, &usbctlr->usb_cmd);
+	udelay(1000);
+	port_current = -1;
+	return 0;
+}
+
+#ifdef CONFIG_OF_CONTROL
+int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
+		   struct fdt_usb *config)
+{
+	int clk_node = 0, rate;
+
+	/* Find the parameters for our oscillator frequency */
+	do {
+		clk_node = fdt_node_offset_by_compatible(blob, clk_node,
+					"nvidia,tegra20-usbparams");
+		if (clk_node < 0) {
+			debug("Cannot find USB params for clock %u",
+			      osc_frequency_mhz);
+			return -FDT_ERR_NOTFOUND;
+		}
+		rate = fdtdec_get_int(blob, clk_node, "osc-frequency", 0);
+	} while (rate != osc_frequency_mhz);
+
+	config->reg = (struct usb_ctlr *)fdtdec_get_addr(blob, node, "reg");
+	config->host_mode = fdtdec_get_bool(blob, node, "support-host-mode");
+	config->utmi = fdtdec_lookup_phandle(blob, node, "utmi") >= 0;
+	config->enabled = fdtdec_get_is_enabled(blob, node);
+	config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
+	if (config->periph_id == -1)
+		return -FDT_ERR_NOTFOUND;
+
+	return fdtdec_get_int_array(blob, clk_node, "params", config->params,
+			PARAM_COUNT);
+}
+#endif
+
+int board_usb_init(const void *blob)
+{
+#ifdef CONFIG_OF_CONTROL
+	struct fdt_usb config;
+	int clk_done = 0;
+	int node, upto = 0;
+	unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
+
+	do {
+		node = fdtdec_next_alias(blob, "usb",
+					 COMPAT_NVIDIA_TEGRA20_USB, &upto);
+		if (node < 0)
+			break;
+		if (fdt_decode_usb(blob, node, osc_freq, &config))
+			return -1;
+		if (!config.enabled)
+			continue;
+
+		/* The first port we find gets to set the clocks */
+		if (!clk_done) {
+			config_clock(config.params);
+			clk_done = 1;
+		}
+		if (config.host_mode) {
+			/* Only one host-dev port is supported */
+			if (host_dev_ctlr)
+				return -1;
+			host_dev_ctlr = config.reg;
+		}
+		if (add_port(config.periph_id, config.reg, config.params,
+			    config.utmi))
+			return -1;
+	} while (node);
+#else
+	enum clock_osc_freq freq;
+	const int *params;
+
+	/* Get the Oscillator frequency */
+	freq = clock_get_osc_freq();
+
+	/* Enable PLL U for USB */
+	params = &usb_pll[freq][0];
+	config_clock(params);
+
+	/* Set up our two ports */
+#ifdef CONFIG_TEGRA_USB1_HOST
+	host_dev_ctlr = (struct usb_ctlr *)TEGRA_USB1_BASE;
+#endif
+	probe_port((struct usb_ctlr *)CONFIG_TEGRA_USB0, params);
+	probe_port((struct usb_ctlr *)CONFIG_TEGRA_USB1, params);
+#endif /* CONFIG_OF_CONTROL */
+	usb_set_host_mode();
+	port_current = -1;
+	return 0;
+}
diff --git a/arch/arm/include/asm/arch-tegra2/tegra2.h b/arch/arm/include/asm/arch-tegra2/tegra2.h
index 8941443..baae2eb 100644
--- a/arch/arm/include/asm/arch-tegra2/tegra2.h
+++ b/arch/arm/include/asm/arch-tegra2/tegra2.h
@@ -41,6 +41,8 @@
 #define TEGRA2_SPI_BASE		(NV_PA_APB_MISC_BASE + 0xC380)
 #define NV_PA_PMC_BASE		0x7000E400
 #define NV_PA_CSITE_BASE	0x70040000
+#define TEGRA_USB1_BASE		0xC5000000
+#define TEGRA_USB3_BASE		0xC5008000
 
 #define TEGRA2_SDRC_CS0		NV_PA_SDRAM_BASE
 #define LOW_LEVEL_SRAM_STACK	0x4000FFFC
diff --git a/arch/arm/include/asm/arch-tegra2/usb.h b/arch/arm/include/asm/arch-tegra2/usb.h
new file mode 100644
index 0000000..0a3056d
--- /dev/null
+++ b/arch/arm/include/asm/arch-tegra2/usb.h
@@ -0,0 +1,255 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#ifndef _TEGRA_USB_H_
+#define _TEGRA_USB_H_
+
+
+/* USB Controller (USBx_CONTROLLER_) regs */
+struct usb_ctlr {
+	/* 0x000 */
+	uint id;
+	uint reserved0;
+	uint host;
+	uint device;
+
+	/* 0x010 */
+	uint txbuf;
+	uint rxbuf;
+	uint reserved1[2];
+
+	/* 0x020 */
+	uint reserved2[56];
+
+	/* 0x100 */
+	u16 cap_length;
+	u16 hci_version;
+	uint hcs_params;
+	uint hcc_params;
+	uint reserved3[5];
+
+	/* 0x120 */
+	uint dci_version;
+	uint dcc_params;
+	uint reserved4[6];
+
+	/* 0x140 */
+	uint usb_cmd;
+	uint usb_sts;
+	uint usb_intr;
+	uint frindex;
+
+	/* 0x150 */
+	uint reserved5;
+	uint periodic_list_base;
+	uint async_list_addr;
+	uint async_tt_sts;
+
+	/* 0x160 */
+	uint burst_size;
+	uint tx_fill_tuning;
+	uint reserved6;   /* is this port_sc1 on some controllers? */
+	uint icusb_ctrl;
+
+	/* 0x170 */
+	uint ulpi_viewport;
+	uint reserved7;
+	uint endpt_nak;
+	uint endpt_nak_enable;
+
+	/* 0x180 */
+	uint reserved;
+	uint port_sc1;
+	uint reserved8[6];
+
+	/* 0x1a0 */
+	uint reserved9;
+	uint otgsc;
+	uint usb_mode;
+	uint endpt_setup_stat;
+
+	/* 0x1b0 */
+	uint reserved10[20];
+
+	/* 0x200 */
+	uint reserved11[0x80];
+
+	/* 0x400 */
+	uint susp_ctrl;
+	uint phy_vbus_sensors;
+	uint phy_vbus_wakeup_id;
+	uint phy_alt_vbus_sys;
+
+	/* 0x410 */
+	uint usb1_legacy_ctrl;
+	uint reserved12[3];
+
+	/* 0x420 */
+	uint reserved13[56];
+
+	/* 0x500 */
+	uint reserved14[64 * 3];
+
+	/* 0x800 */
+	uint utmip_pll_cfg0;
+	uint utmip_pll_cfg1;
+	uint utmip_xcvr_cfg0;
+	uint utmip_bias_cfg0;
+
+	/* 0x810 */
+	uint utmip_hsrx_cfg0;
+	uint utmip_hsrx_cfg1;
+	uint utmip_fslsrx_cfg0;
+	uint utmip_fslsrx_cfg1;
+
+	/* 0x820 */
+	uint utmip_tx_cfg0;
+	uint utmip_misc_cfg0;
+	uint utmip_misc_cfg1;
+	uint utmip_debounce_cfg0;
+
+	/* 0x830 */
+	uint utmip_bat_chrg_cfg0;
+	uint utmip_spare_cfg0;
+	uint utmip_xcvr_cfg1;
+	uint utmip_bias_cfg1;
+};
+
+
+/* USB1_LEGACY_CTRL */
+#define USB1_NO_LEGACY_MODE		1
+
+#define VBUS_SENSE_CTL_SHIFT			1
+#define VBUS_SENSE_CTL_MASK			(3 << VBUS_SENSE_CTL_SHIFT)
+#define VBUS_SENSE_CTL_VBUS_WAKEUP		0
+#define VBUS_SENSE_CTL_AB_SESS_VLD_OR_VBUS_WAKEUP	1
+#define VBUS_SENSE_CTL_AB_SESS_VLD		2
+#define VBUS_SENSE_CTL_A_SESS_VLD		3
+
+/* USBx_IF_USB_SUSP_CTRL_0 */
+#define UTMIP_PHY_ENB			        (1 << 12)
+#define UTMIP_RESET			        (1 << 11)
+#define USB_PHY_CLK_VALID			(1 << 7)
+
+/* USBx_UTMIP_MISC_CFG1 */
+#define UTMIP_PLLU_STABLE_COUNT_SHIFT		6
+#define UTMIP_PLLU_STABLE_COUNT_MASK		\
+				(0xfff << UTMIP_PLLU_STABLE_COUNT_SHIFT)
+#define UTMIP_PLL_ACTIVE_DLY_COUNT_SHIFT	18
+#define UTMIP_PLL_ACTIVE_DLY_COUNT_MASK		\
+				(0x1f << UTMIP_PLL_ACTIVE_DLY_COUNT_SHIFT)
+#define UTMIP_PHY_XTAL_CLOCKEN			(1 << 30)
+
+/* USBx_UTMIP_PLL_CFG1_0 */
+#define UTMIP_PLLU_ENABLE_DLY_COUNT_SHIFT	27
+#define UTMIP_PLLU_ENABLE_DLY_COUNT_MASK	\
+				(0xf << UTMIP_PLLU_ENABLE_DLY_COUNT_SHIFT)
+#define UTMIP_XTAL_FREQ_COUNT_SHIFT		0
+#define UTMIP_XTAL_FREQ_COUNT_MASK		0xfff
+
+/* USBx_UTMIP_BIAS_CFG1_0 */
+#define UTMIP_BIAS_PDTRK_COUNT_SHIFT		3
+#define UTMIP_BIAS_PDTRK_COUNT_MASK		\
+				(0x1f << UTMIP_BIAS_PDTRK_COUNT_SHIFT)
+#define UTMIP_BIAS_PDTRK_COUNT_SHIFT		3
+#define UTMIP_BIAS_PDTRK_COUNT_MASK		\
+				(0x1f << UTMIP_BIAS_PDTRK_COUNT_SHIFT)
+
+#define UTMIP_DEBOUNCE_CFG0_SHIFT		0
+#define UTMIP_DEBOUNCE_CFG0_MASK		0xffff
+
+/* USBx_UTMIP_TX_CFG0_0 */
+#define UTMIP_FS_PREAMBLE_J			(1 << 19)
+
+/* USBx_UTMIP_BAT_CHRG_CFG0_0 */
+#define UTMIP_PD_CHRG				1
+
+/* USBx_UTMIP_XCVR_CFG0_0 */
+#define UTMIP_XCVR_LSBIAS_SE			(1 << 21)
+
+/* USBx_UTMIP_SPARE_CFG0_0 */
+#define FUSE_SETUP_SEL				(1 << 3)
+
+/* USBx_UTMIP_HSRX_CFG0_0 */
+#define UTMIP_IDLE_WAIT_SHIFT			15
+#define UTMIP_IDLE_WAIT_MASK			(0x1f << UTMIP_IDLE_WAIT_SHIFT)
+#define UTMIP_ELASTIC_LIMIT_SHIFT		10
+#define UTMIP_ELASTIC_LIMIT_MASK		\
+				(0x1f << UTMIP_ELASTIC_LIMIT_SHIFT)
+
+/* USBx_UTMIP_HSRX_CFG0_1 */
+#define UTMIP_HS_SYNC_START_DLY_SHIFT		1
+#define UTMIP_HS_SYNC_START_DLY_MASK		\
+				(0xf << UTMIP_HS_SYNC_START_DLY_SHIFT)
+
+/* USBx_CONTROLLER_2_USB2D_ICUSB_CTRL_0 */
+#define IC_ENB1					(1 << 3)
+
+/* SB2_CONTROLLER_2_USB2D_PORTSC1_0 */
+#define PTS_SHIFT				30
+#define PTS_MASK				(3 << PTS_SHIFT)
+#define PTS_UTMI	0
+#define PTS_RESERVED	1
+#define PTS_ULP		2
+#define PTS_ICUSB_SER	3
+
+#define STS					(1 << 29)
+
+/* USBx_UTMIP_XCVR_CFG0_0 */
+#define UTMIP_FORCE_PD_POWERDOWN		(1 << 14)
+#define UTMIP_FORCE_PD2_POWERDOWN		(1 << 16)
+#define UTMIP_FORCE_PDZI_POWERDOWN		(1 << 18)
+
+/* USBx_UTMIP_XCVR_CFG1_0 */
+#define UTMIP_FORCE_PDDISC_POWERDOWN		(1 << 0)
+#define UTMIP_FORCE_PDCHRP_POWERDOWN		(1 << 2)
+#define UTMIP_FORCE_PDDR_POWERDOWN		(1 << 4)
+
+/* USB3_IF_USB_PHY_VBUS_SENSORS_0 */
+#define VBUS_VLD_STS			(1 << 26)
+
+
+/* Change the USB host port into host mode */
+void usb_set_host_mode(void);
+
+/* Setup USB on the board */
+int board_usb_init(const void *blob);
+
+/**
+ * Start up the given port number (ports are numbered from 0 on each board).
+ * This returns values for the appropriate hccr and hcor addresses to use for
+ * USB EHCI operations.
+ *
+ * @param portnum	port number to start
+ * @param hccr		returns start address of EHCI HCCR registers
+ * @param hcor		returns start address of EHCI HCOR registers
+ * @return 0 if ok, -1 on error (generally invalid port number)
+ */
+int tegrausb_start_port(unsigned portnum, u32 *hccr, u32 *hcor);
+
+/**
+ * Stop the current port
+ *
+ * @return 0 if ok, -1 if no port was active
+ */
+int tegrausb_stop_port(void);
+
+#endif	/* _TEGRA_USB_H_ */
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 09abb75..39134ff 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -46,6 +46,7 @@ COBJS-$(CONFIG_USB_EHCI_PPC4XX) += ehci-ppc4xx.o
 COBJS-$(CONFIG_USB_EHCI_IXP4XX) += ehci-ixp.o
 COBJS-$(CONFIG_USB_EHCI_KIRKWOOD) += ehci-kirkwood.o
 COBJS-$(CONFIG_USB_EHCI_PCI) += ehci-pci.o
+COBJS-$(CONFIG_USB_EHCI_TEGRA) += ehci-tegra.o
 COBJS-$(CONFIG_USB_EHCI_VCT) += ehci-vct.o
 
 COBJS	:= $(COBJS-y)
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
new file mode 100644
index 0000000..3a48c26
--- /dev/null
+++ b/drivers/usb/host/ehci-tegra.c
@@ -0,0 +1,63 @@
+/*
+ * Copyright (c) 2009 NVIDIA Corporation
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <usb.h>
+
+#include "ehci.h"
+#include "ehci-core.h"
+
+#include <asm/errno.h>
+#include <asm/arch/usb.h>
+
+
+/*
+ * Create the appropriate control structures to manage
+ * a new EHCI host controller.
+ */
+int ehci_hcd_init(void)
+{
+	u32 our_hccr, our_hcor;
+
+	/*
+	 * Select the first port, as we don't have a way of selecting others
+	 * yet
+	 */
+	if (tegrausb_start_port(0, &our_hccr, &our_hcor))
+		return -1;
+
+	hccr = (struct ehci_hccr *)our_hccr;
+	hcor = (struct ehci_hcor *)our_hcor;
+
+	return 0;
+}
+
+/*
+ * Destroy the appropriate control structures corresponding
+ * the the EHCI host controller.
+ */
+int ehci_hcd_stop(void)
+{
+	usb_set_host_mode();
+	tegrausb_stop_port();
+	return 0;
+}
diff --git a/include/fdtdec.h b/include/fdtdec.h
index 9ecb6ab..f1734c4 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -57,6 +57,7 @@ struct fdt_memory {
  */
 enum fdt_compat_id {
 	COMPAT_UNKNOWN,
+	COMPAT_NVIDIA_TEGRA20_USB,	/* Tegra2 USB port */
 
 	COMPAT_COUNT,
 };
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 613547a..0213f08 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -34,6 +34,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define COMPAT(id, name) name
 static const char * const compat_names[COMPAT_COUNT] = {
 	COMPAT(UNKNOWN, "<none>"),
+	COMPAT(NVIDIA_TEGRA20_USB, "nvidia,tegra20-usb"),
 };
 
 /**
-- 
1.7.3.1

[U-Boot] [PATCH 11/14] tegra: usb: Add USB support to nvidia boards

[Simon Glass]

This adds basic USB support for port 0. The other port is not supported
yet.

Tegra2 (SeaBoard) # usb start
(Re)start USB...
USB:   Register 10011 NbrPorts 1
USB EHCI 1.00
scanning bus for devices... 5 USB Device(s) found
       scanning bus for storage devices... 1 Storage Device(s) found
Tegra2 (SeaBoard) # ext2load usb 0:3 10000000 /boot/vmlinuz
Loading file "/boot/vmlinuz" from usb device 0:3 (ROOT-A)
2932976 bytes read

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 board/nvidia/common/board.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
index f39fb24..a1fa326 100644
--- a/board/nvidia/common/board.c
+++ b/board/nvidia/common/board.c
@@ -32,6 +32,7 @@
 #include <asm/arch/pinmux.h>
 #include <asm/arch/uart.h>
 #include <spi.h>
+#include <asm/arch/usb.h>
 #include "board.h"
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -125,6 +126,10 @@ int board_init(void)
 	/* boot param addr */
 	gd->bd->bi_boot_params = (NV_PA_SDRAM_BASE + 0x100);
 
+#ifdef CONFIG_USB_EHCI_TEGRA
+	board_usb_init(gd->fdt_blob);
+#endif
+
 	return 0;
 }
 
-- 
1.7.3.1

[U-Boot] [PATCH 12/14] tegra: usb: Add common USB defines for tegra2 boards

[Simon Glass]

All Tegra2 boards should include tegra2-common. This adds the required
USB config to that file.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 include/configs/tegra2-common.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/include/configs/tegra2-common.h b/include/configs/tegra2-common.h
index e6f385f..6549d00 100644
--- a/include/configs/tegra2-common.h
+++ b/include/configs/tegra2-common.h
@@ -84,6 +84,20 @@
 #define CONFIG_SYS_BAUDRATE_TABLE	{4800, 9600, 19200, 38400, 57600,\
 					115200}
 
+/*
+ * USB Host. Tegra2 requires USB buffers to be aligned to a word boundary
+ */
+#define CONFIG_USB_EHCI_DATA_ALIGN	4
+
+/*
+ * This parameter affects a TXFILLTUNING field that controls how much data is
+ * sent to the latency fifo before it is sent to the wire. Without this
+ * parameter, the default (2) causes occasional Data Buffer Errors in OUT
+ * packets depending on the buffer address and size.
+ */
+#define CONFIG_USB_EHCI_TXFIFO_THRESH	10
+
+#define CONFIG_EHCI_IS_TDI
 /* include default commands */
 #include <config_cmd_default.h>
 
-- 
1.7.3.1

[U-Boot] [PATCH 13/14] tegra: usb: Enable USB on Seaboard

[Simon Glass]

Seaboard has a top port which is USB host or device, and a side port which
is host only.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 include/configs/seaboard.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h
index 261f952..df1b44a 100644
--- a/include/configs/seaboard.h
+++ b/include/configs/seaboard.h
@@ -72,4 +72,18 @@
 
 #define CONFIG_ENV_SECT_SIZE    CONFIG_ENV_SIZE
 #define CONFIG_ENV_OFFSET       (CONFIG_SPI_FLASH_SIZE - CONFIG_ENV_SECT_SIZE)
+
+/* USB Host support */
+#define CONFIG_USB_EHCI
+#define CONFIG_USB_EHCI_TEGRA
+#define CONFIG_USB_STORAGE
+#define CONFIG_CMD_USB
+
+/* Select the order in which U-Boot sees USB ports */
+#define CONFIG_TEGRA_USB0	TEGRA_USB3_BASE
+#define CONFIG_TEGRA_USB1	TEGRA_USB1_BASE
+
+/* Put USB1 in host mode */
+#define CONFIG_TEGRA_USB1_HOST
+
 #endif /* __CONFIG_H */
-- 
1.7.3.1

[PATCH 14/14] tegra: fdt: Enable FDT support for Seaboard

[Simon Glass]

This switches Seaboard over to use FDT for run-time config instead of
CONFIG options. USB is the only user at present.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 include/configs/seaboard.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h
index df1b44a..03e9824 100644
--- a/include/configs/seaboard.h
+++ b/include/configs/seaboard.h
@@ -27,6 +27,11 @@
 #include <asm/sizes.h>
 #include "tegra2-common.h"
 
+/* Enable fdt support for Seaboard. Flash the image in u-boot-dtb.bin */
+#define CONFIG_DEFAULT_DEVICE_TREE	tegra2-seaboard
+#define CONFIG_OF_CONTROL
+#define CONFIG_OF_SEPARATE
+
 /* High-level configuration options */
 #define TEGRA2_SYSMEM		"mem=384M@0M nvmem=128M@384M mem=512M@512M"
 #define V_PROMPT		"Tegra2 (SeaBoard) # "
@@ -79,6 +84,8 @@
 #define CONFIG_USB_STORAGE
 #define CONFIG_CMD_USB
 
+#ifndef CONFIG_OF_CONTROL
+
 /* Select the order in which U-Boot sees USB ports */
 #define CONFIG_TEGRA_USB0	TEGRA_USB3_BASE
 #define CONFIG_TEGRA_USB1	TEGRA_USB1_BASE
@@ -86,4 +93,6 @@
 /* Put USB1 in host mode */
 #define CONFIG_TEGRA_USB1_HOST
 
+#endif
+
 #endif /* __CONFIG_H */
-- 
1.7.3.1

[U-Boot] [PATCH 14/14] tegra: fdt: Enable FDT support for Seaboard

[Simon Glass]

This switches Seaboard over to use FDT for run-time config instead of
CONFIG options. USB is the only user at present.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 include/configs/seaboard.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h
index df1b44a..03e9824 100644
--- a/include/configs/seaboard.h
+++ b/include/configs/seaboard.h
@@ -27,6 +27,11 @@
 #include <asm/sizes.h>
 #include "tegra2-common.h"
 
+/* Enable fdt support for Seaboard. Flash the image in u-boot-dtb.bin */
+#define CONFIG_DEFAULT_DEVICE_TREE	tegra2-seaboard
+#define CONFIG_OF_CONTROL
+#define CONFIG_OF_SEPARATE
+
 /* High-level configuration options */
 #define TEGRA2_SYSMEM		"mem=384M at 0M nvmem=128M at 384M mem=512M at 512M"
 #define V_PROMPT		"Tegra2 (SeaBoard) # "
@@ -79,6 +84,8 @@
 #define CONFIG_USB_STORAGE
 #define CONFIG_CMD_USB
 
+#ifndef CONFIG_OF_CONTROL
+
 /* Select the order in which U-Boot sees USB ports */
 #define CONFIG_TEGRA_USB0	TEGRA_USB3_BASE
 #define CONFIG_TEGRA_USB1	TEGRA_USB1_BASE
@@ -86,4 +93,6 @@
 /* Put USB1 in host mode */
 #define CONFIG_TEGRA_USB1_HOST
 
+#endif
+
 #endif /* __CONFIG_H */
-- 
1.7.3.1

Re: [PATCH 01/14] fdt: Tidy up a few fdtdec problems

[Stephen Warren]

On 11/23/2011 08:54 PM, Simon Glass wrote:
> This fixes three trivial issues in fdtdec.c:
> 1. fdtdec_get_is_enabled() doesn't really need a default value
> 2. The fdt must be word-aligned, since otherwise it will fail on ARM
> 3. The compat_names[] array is missing its first element

> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
...
>  #define COMPAT(id, name) name
>  static const char * const compat_names[COMPAT_COUNT] = {
> +	COMPAT(UNKNOWN, "<none>"),
>  };

Could you educate me on why that change is necessary? Maybe explain this
in the commit description?

> -int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
> +int fdtdec_get_is_enabled(const void *blob, int node)
>  {
>  	const char *cell;
>  
>  	cell = fdt_getprop(blob, node, "status", NULL);
>  	if (cell)
>  		return 0 == strcmp(cell, "ok");
> -	return default_val;
> +	return 1;
>  }

Not that this patch changes this, but isn't "okay" also a legal
"enabled" value, and perhaps even the recommended value? See
http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-July/006389.html.

-- 
nvpublic

[U-Boot] [PATCH 01/14] fdt: Tidy up a few fdtdec problems

[Stephen Warren]

On 11/23/2011 08:54 PM, Simon Glass wrote:
> This fixes three trivial issues in fdtdec.c:
> 1. fdtdec_get_is_enabled() doesn't really need a default value
> 2. The fdt must be word-aligned, since otherwise it will fail on ARM
> 3. The compat_names[] array is missing its first element

> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
...
>  #define COMPAT(id, name) name
>  static const char * const compat_names[COMPAT_COUNT] = {
> +	COMPAT(UNKNOWN, "<none>"),
>  };

Could you educate me on why that change is necessary? Maybe explain this
in the commit description?

> -int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
> +int fdtdec_get_is_enabled(const void *blob, int node)
>  {
>  	const char *cell;
>  
>  	cell = fdt_getprop(blob, node, "status", NULL);
>  	if (cell)
>  		return 0 == strcmp(cell, "ok");
> -	return default_val;
> +	return 1;
>  }

Not that this patch changes this, but isn't "okay" also a legal
"enabled" value, and perhaps even the recommended value? See
http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-July/006389.html.

-- 
nvpublic

Re: [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Stephen Warren]

On 11/23/2011 08:54 PM, Simon Glass wrote:
> Add a function to lookup a property which is a phandle in a node, and
> another to read a fixed-length integer array from an fdt property.
> Also add a function to read boolean properties.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Looking at the U-Boot custodians web page, you need to send the core DT
changes (well, probably anything DT related) to Jerry Van Baren.

> +/**
> + * Look up a property in a node and return its contents in an integer
> + * array of given length. The property must have at least enough data for
> + * the array (4*count bytes). It may have more, but this will be ignored.
> + *
> + * @param blob		FDT blob
> + * @param node		node to examine
> + * @param prop_name	name of property to find
> + * @param array		array to fill with data
> + * @param count		number of array elements
> + * @return 0 if ok, or -FDT_ERR_NOTFOUND if the property is not found,
> + *		or -FDT_ERR_BADLAYOUT if not enough data
> + */
> +int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
> +		int *array, int count);

The kernel's equivalent of this function retrieves an array of U32s. Is
one version more correct than the other?

> +/**
> + * Look up a boolean property in a node and return it.
> + *
> + * A boolean properly is true if present in the device tree and false if not
> + * present, or present with a 0 value.
> + *
> + * @param blob	FDT blob
> + * @param node	node to examine
> + * @param prop_name	name of property to find
> + * @return 1 if the properly is present; 0 if it isn't present or is 0
> + */
> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name);

Does U-Boot allow use of the "bool" type here?


> +/**
> + * Look up a property in a node and check that it has a minimum length.
> + *
> + * @param blob		FDT blob
> + * @param node		node to examine
> + * @param prop_name	name of property to find
> + * @param min_len	minimum property length in bytes
> + * @param err		0 if ok, or -FDT_ERR_NOTFOUND if the property is not
> +			found, or -FDT_ERR_BADLAYOUT if not enough data
> + * @return pointer to cell, which is only valid if err == 0
> + */
> +static const void *get_prop_len(const void *blob, int node,
> +		const char *prop_name, int min_len, int *err)

Based on the function name, I'd expect it to return the length of the
property; perhaps get_prop_check_min_len?

> +/**
> + * Look up a boolean property in a node and return it.
> + *
> + * A boolean properly is true if present in the device tree and false if not
> + * present, or present with a 0 value.
> + *
> + * @param blob	FDT blob
> + * @param node	node to examine
> + * @param prop_name	name of property to find
> + * @return 1 if the properly is present; 0 if it isn't present or is 0
> + */
> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
> +{
> +	const s32 *cell;
> +	int len;
> +
> +	debug("%s: %s\n", __func__, prop_name);
> +	cell = fdt_getprop(blob, node, prop_name, &len);
> +	if (!cell)
> +		return 0;
> +	if (len >= sizeof(u32) && *cell == 0)
> +		return 0;
> +
> +	return 1;
> +}

In the kernel, I believe that property existence is all that's usually
checked. Is that wrong? Did the definition of a boolean property's value
in the function description above come from the specification? If a
property had a length of 0/1/2/3 with a zero value, it seems very odd to
treat that as true.

-- 
nvpublic

[U-Boot] [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Stephen Warren]

On 11/23/2011 08:54 PM, Simon Glass wrote:
> Add a function to lookup a property which is a phandle in a node, and
> another to read a fixed-length integer array from an fdt property.
> Also add a function to read boolean properties.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Looking at the U-Boot custodians web page, you need to send the core DT
changes (well, probably anything DT related) to Jerry Van Baren.

> +/**
> + * Look up a property in a node and return its contents in an integer
> + * array of given length. The property must have at least enough data for
> + * the array (4*count bytes). It may have more, but this will be ignored.
> + *
> + * @param blob		FDT blob
> + * @param node		node to examine
> + * @param prop_name	name of property to find
> + * @param array		array to fill with data
> + * @param count		number of array elements
> + * @return 0 if ok, or -FDT_ERR_NOTFOUND if the property is not found,
> + *		or -FDT_ERR_BADLAYOUT if not enough data
> + */
> +int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
> +		int *array, int count);

The kernel's equivalent of this function retrieves an array of U32s. Is
one version more correct than the other?

> +/**
> + * Look up a boolean property in a node and return it.
> + *
> + * A boolean properly is true if present in the device tree and false if not
> + * present, or present with a 0 value.
> + *
> + * @param blob	FDT blob
> + * @param node	node to examine
> + * @param prop_name	name of property to find
> + * @return 1 if the properly is present; 0 if it isn't present or is 0
> + */
> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name);

Does U-Boot allow use of the "bool" type here?


> +/**
> + * Look up a property in a node and check that it has a minimum length.
> + *
> + * @param blob		FDT blob
> + * @param node		node to examine
> + * @param prop_name	name of property to find
> + * @param min_len	minimum property length in bytes
> + * @param err		0 if ok, or -FDT_ERR_NOTFOUND if the property is not
> +			found, or -FDT_ERR_BADLAYOUT if not enough data
> + * @return pointer to cell, which is only valid if err == 0
> + */
> +static const void *get_prop_len(const void *blob, int node,
> +		const char *prop_name, int min_len, int *err)

Based on the function name, I'd expect it to return the length of the
property; perhaps get_prop_check_min_len?

> +/**
> + * Look up a boolean property in a node and return it.
> + *
> + * A boolean properly is true if present in the device tree and false if not
> + * present, or present with a 0 value.
> + *
> + * @param blob	FDT blob
> + * @param node	node to examine
> + * @param prop_name	name of property to find
> + * @return 1 if the properly is present; 0 if it isn't present or is 0
> + */
> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
> +{
> +	const s32 *cell;
> +	int len;
> +
> +	debug("%s: %s\n", __func__, prop_name);
> +	cell = fdt_getprop(blob, node, prop_name, &len);
> +	if (!cell)
> +		return 0;
> +	if (len >= sizeof(u32) && *cell == 0)
> +		return 0;
> +
> +	return 1;
> +}

In the kernel, I believe that property existence is all that's usually
checked. Is that wrong? Did the definition of a boolean property's value
in the function description above come from the specification? If a
property had a length of 0/1/2/3 with a zero value, it seems very odd to
treat that as true.

-- 
nvpublic

Re: [PATCH 05/14] tegra: fdt: Add Tegra2x device tree file

[Stephen Warren]

On 11/23/2011 08:54 PM, Simon Glass wrote:
> This was taken from commit 1ea6b8f at:
> git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git

That's not the latest version in linux-next. Also, this doesn't include
quite a few changes that have been sent to the mailing lists but not yet
applied.

In particular, linux-next now includes a minimal USB binding. Should we
just use this in U-Boot for now? We should get review on the kernel
lists before bringing in this more advanced USB binding in U-Boot, and
perhaps even add the binding into the kernel at the same time?

Patches have been posted to:
* Convert to the finalized ARM GIC binding.
* Disable devices in the per-board .dts files that aren't used on those
boards.
* Various other cleanups in order to make the .dts files match the
kernel's non-DT board files.
* Perhaps more that I forget.

I suppose those could be applied to U-Boot as and when they are applied
to the kernel.

-- 
nvpublic

[U-Boot] [PATCH 05/14] tegra: fdt: Add Tegra2x device tree file

[Stephen Warren]

On 11/23/2011 08:54 PM, Simon Glass wrote:
> This was taken from commit 1ea6b8f at:
> git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git

That's not the latest version in linux-next. Also, this doesn't include
quite a few changes that have been sent to the mailing lists but not yet
applied.

In particular, linux-next now includes a minimal USB binding. Should we
just use this in U-Boot for now? We should get review on the kernel
lists before bringing in this more advanced USB binding in U-Boot, and
perhaps even add the binding into the kernel at the same time?

Patches have been posted to:
* Convert to the finalized ARM GIC binding.
* Disable devices in the per-board .dts files that aren't used on those
boards.
* Various other cleanups in order to make the .dts files match the
kernel's non-DT board files.
* Perhaps more that I forget.

I suppose those could be applied to U-Boot as and when they are applied
to the kernel.

-- 
nvpublic

[U-Boot] [PATCH 09/14] tegra: usb: Add support for data alignment and txfifo threshold

[Stephen Warren]

On 11/23/2011 08:54 PM, Simon Glass wrote:
> CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of data for
> USB packets (e.g. 4 means word-aligned). This is required for Tegra
> to operate.
> 
> CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the txfilltuning
> field in the EHCI controller on reset.

Shouldn't that be two separate patches?

> diff --git a/README b/README
...
> +		CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of
> +		data for USB packets (e.g. 4 means word-aligned). This is
> +		required for Tegra to operate.
> +
> +		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
> +		txfilltuning field in the EHCI controller on reset.
> +

> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
...
> @@ -322,6 +329,27 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
>  	int timeout;
>  	int ret = 0;
>  
> +#ifdef CONFIG_USB_EHCI_DATA_ALIGN
> +	/* In case ehci host requires alignment for buffers */
> +	void *align_buf = NULL;
> +	void *orig_buf = buffer;
> +	int unaligned = ((int)buffer & (CONFIG_USB_EHCI_DATA_ALIGN - 1)) != 0;

This uses "!= 0".

> +
> +	if (unaligned) {
> +		align_buf = malloc(length + CONFIG_USB_EHCI_DATA_ALIGN);
> +		if (!align_buf)
> +			return -1;
> +		if ((int)align_buf & (CONFIG_USB_EHCI_DATA_ALIGN - 1))

This doesn't use "!= 0". Probably drop the "!= 0" above?

> +			buffer = (void *)((int)align_buf +
> +				CONFIG_USB_EHCI_DATA_ALIGN -
> +				((int)align_buf &
> +					(CONFIG_USB_EHCI_DATA_ALIGN - 1)));
> +		else
> +			buffer = align_buf;

Why not jsut do the following unconditionally; it seems much simpler
even if very marginally less efficient:

buffer = (align_buf + CONFIG_USB_EHCI_DATA_ALIGN - 1) &
		~(CONFIG_USB_EHCI_DATA_ALIGN - 1);

IIRC, in the kernel, we had to use this technique because arbitrary
callers could have allocated "buffer" however they pleased. I assume the
same is true in U-Boot; there isn't some way that this alignment
restriction could be implemented in some core USB buffer allocation
routine instead, and thus avoid all the copying? Do you know how often
unaligned buffers actually occur in practice, and hence how much of a
performance impact the copying will have?

-- 
nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren]

On 11/23/2011 08:54 PM, Simon Glass wrote:
> This adds basic support for the Tegra2 USB controller. Board files should
> call board_usb_init() to set things up.

Just a very brief review:

> +/* Put the port into host mode (this only works for USB1) */
> +static void set_host_mode(struct usb_ctlr *usbctlr)
> +{
> +       /* Check whether remote host from USB1 is driving VBus */
> +       if (readl(&usbctlr->phy_vbus_sensors) & VBUS_VLD_STS)
> +               return;
> +
> +       /*
> +        * If not driving, we set GPIO USB1_VBus_En. Seaboard platform uses
> +        * PAD SLXK (GPIO D.00) as USB1_VBus_En Config as GPIO
> +        */
> +       gpio_direction_output(GPIO_PD0, 1);

The GPIO to use here needs to be parameterized; there's no reason to
believe it'll be the same on all boards (or even that boards will have
any such GPIO).

> +/* Put our ports into host mode */
> +void usb_set_host_mode(void)
> +{
> +       if (host_dev_ctlr)
> +               set_host_mode(host_dev_ctlr);
> +}

Why not just call set_host_mode() directly from board_usb_init()?

> +#ifndef CONFIG_OF_CONTROL
> +static int probe_port(struct usb_ctlr *usbctlr, const int params[])
> +{
> +       enum periph_id id;
> +       int utmi = 0;
> +
> +       /*
> +        * Get the periph id. Port 1 has an internal transceiver, port 3 is
> +        * external
> +        */
> +       switch ((u32)usbctlr) {
> +       case TEGRA_USB1_BASE:
> +               id = PERIPH_ID_USBD;
> +               break;
> +
> +       case TEGRA_USB3_BASE:
> +               id = PERIPH_ID_USB3;
> +               utmi = 1;
> +               break;
> +
> +       default:
> +               printf("tegrausb: probe_port: no such port %p\n", usbctlr);
> +               return -1;
> +       }

What about the other port (USB2)?

> +#ifdef CONFIG_OF_CONTROL
> +int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
> +                  struct fdt_usb *config)
> +{
> +       int clk_node = 0, rate;
> +
> +       /* Find the parameters for our oscillator frequency */
> +       do {
> +               clk_node = fdt_node_offset_by_compatible(blob, clk_node,
> +                                       "nvidia,tegra20-usbparams");
> +               if (clk_node < 0) {
> +                       debug("Cannot find USB params for clock %u",
> +                             osc_frequency_mhz);
> +                       return -FDT_ERR_NOTFOUND;
> +               }
> +               rate = fdtdec_get_int(blob, clk_node, "osc-frequency", 0);
> +       } while (rate != osc_frequency_mhz);
> +
> +       config->reg = (struct usb_ctlr *)fdtdec_get_addr(blob, node, "reg");
> +       config->host_mode = fdtdec_get_bool(blob, node, "support-host-mode");

Property "support-host-mode" isn't something that's supported by the
kernel binding, and needs discussion/ack there.

> +       config->utmi = fdtdec_lookup_phandle(blob, node, "utmi") >= 0;

In the kernel DT binding, this is property "phy_type" with legal values
"utmi" or "ulpi."

> +       config->enabled = fdtdec_get_is_enabled(blob, node);
> +       config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);

periph-id is a U-Boot specific concept, not HW description. The DT
shouldn't contain that value.

> +       if (config->periph_id == -1)
> +               return -FDT_ERR_NOTFOUND;
> +
> +       return fdtdec_get_int_array(blob, clk_node, "params", config->params,
> +                       PARAM_COUNT);

Property "params" (which should probably be named something better
anyway) isn't something that's supported by the kernel binding, and
needs discussion/ack there. Instead, I suggest following the kernel's
approach for now - don't specify these PHY parameters in the binding,
but rather just apply the defaults, which are apparently suitable for
all the boards supported by the mainline kernel at least.


> +int board_usb_init(const void *blob)
> +{
> +#ifdef CONFIG_OF_CONTROL
> +       struct fdt_usb config;
> +       int clk_done = 0;
> +       int node, upto = 0;
> +       unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
> +
> +       do {
> +               node = fdtdec_next_alias(blob, "usb",
> +                                        COMPAT_NVIDIA_TEGRA20_USB, &upto);

Why only initialize USB controllers with aliases? Surely this should
enumerate all nodes with a specific compatible flag?

> +               /* The first port we find gets to set the clocks */

Ick.

> +       /* Set up our two ports */
> +#ifdef CONFIG_TEGRA_USB1_HOST
> +       host_dev_ctlr = (struct usb_ctlr *)TEGRA_USB1_BASE;
> +#endif

That port is always the host/device switchable controller. Why not
always make that assignment? The issue here is probably that
set_host_mode() isn't suitable for all boards. The solution seems to be
to fix that.

-- 
nvpublic

Re: [PATCH 01/14] fdt: Tidy up a few fdtdec problems

[David Gibson]

On Mon, Nov 28, 2011 at 11:33:22AM -0700, Stephen Warren wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
> > This fixes three trivial issues in fdtdec.c:
> > 1. fdtdec_get_is_enabled() doesn't really need a default value
> > 2. The fdt must be word-aligned, since otherwise it will fail on ARM
> > 3. The compat_names[] array is missing its first element
> 
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> ...
> >  #define COMPAT(id, name) name
> >  static const char * const compat_names[COMPAT_COUNT] = {
> > +	COMPAT(UNKNOWN, "<none>"),
> >  };
> 
> Could you educate me on why that change is necessary? Maybe explain this
> in the commit description?
> 
> > -int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
> > +int fdtdec_get_is_enabled(const void *blob, int node)
> >  {
> >  	const char *cell;
> >  
> >  	cell = fdt_getprop(blob, node, "status", NULL);
> >  	if (cell)
> >  		return 0 == strcmp(cell, "ok");
> > -	return default_val;
> > +	return 1;
> >  }
> 
> Not that this patch changes this, but isn't "okay" also a legal
> "enabled" value, and perhaps even the recommended value? See
> http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-July/006389.html.

Yes.  "okay", not "ok" is the standard value for the status property.

-- 
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

[U-Boot] [PATCH 01/14] fdt: Tidy up a few fdtdec problems

[David Gibson]

On Mon, Nov 28, 2011 at 11:33:22AM -0700, Stephen Warren wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
> > This fixes three trivial issues in fdtdec.c:
> > 1. fdtdec_get_is_enabled() doesn't really need a default value
> > 2. The fdt must be word-aligned, since otherwise it will fail on ARM
> > 3. The compat_names[] array is missing its first element
> 
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> ...
> >  #define COMPAT(id, name) name
> >  static const char * const compat_names[COMPAT_COUNT] = {
> > +	COMPAT(UNKNOWN, "<none>"),
> >  };
> 
> Could you educate me on why that change is necessary? Maybe explain this
> in the commit description?
> 
> > -int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
> > +int fdtdec_get_is_enabled(const void *blob, int node)
> >  {
> >  	const char *cell;
> >  
> >  	cell = fdt_getprop(blob, node, "status", NULL);
> >  	if (cell)
> >  		return 0 == strcmp(cell, "ok");
> > -	return default_val;
> > +	return 1;
> >  }
> 
> Not that this patch changes this, but isn't "okay" also a legal
> "enabled" value, and perhaps even the recommended value? See
> http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-July/006389.html.

Yes.  "okay", not "ok" is the standard value for the status property.

-- 
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

Re: [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[David Gibson]

On Mon, Nov 28, 2011 at 11:41:32AM -0700, Stephen Warren wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
> > Add a function to lookup a property which is a phandle in a node, and
> > another to read a fixed-length integer array from an fdt property.
> > Also add a function to read boolean properties.
> > 
> > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> Looking at the U-Boot custodians web page, you need to send the core DT
> changes (well, probably anything DT related) to Jerry Van Baren.
> 
> > +/**
> > + * Look up a property in a node and return its contents in an integer
> > + * array of given length. The property must have at least enough data for
> > + * the array (4*count bytes). It may have more, but this will be ignored.
> > + *
> > + * @param blob		FDT blob
> > + * @param node		node to examine
> > + * @param prop_name	name of property to find
> > + * @param array		array to fill with data
> > + * @param count		number of array elements
> > + * @return 0 if ok, or -FDT_ERR_NOTFOUND if the property is not found,
> > + *		or -FDT_ERR_BADLAYOUT if not enough data
> > + */
> > +int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
> > +		int *array, int count);
> 
> The kernel's equivalent of this function retrieves an array of U32s. Is
> one version more correct than the other?

Using u32 is a better idea.  The property formats are all defined in
terms of fixed width elements, so using a vague width type like int to
interact with it is a bad idea.

-- 
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

[U-Boot] [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[David Gibson]

On Mon, Nov 28, 2011 at 11:41:32AM -0700, Stephen Warren wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
> > Add a function to lookup a property which is a phandle in a node, and
> > another to read a fixed-length integer array from an fdt property.
> > Also add a function to read boolean properties.
> > 
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> 
> Looking at the U-Boot custodians web page, you need to send the core DT
> changes (well, probably anything DT related) to Jerry Van Baren.
> 
> > +/**
> > + * Look up a property in a node and return its contents in an integer
> > + * array of given length. The property must have at least enough data for
> > + * the array (4*count bytes). It may have more, but this will be ignored.
> > + *
> > + * @param blob		FDT blob
> > + * @param node		node to examine
> > + * @param prop_name	name of property to find
> > + * @param array		array to fill with data
> > + * @param count		number of array elements
> > + * @return 0 if ok, or -FDT_ERR_NOTFOUND if the property is not found,
> > + *		or -FDT_ERR_BADLAYOUT if not enough data
> > + */
> > +int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
> > +		int *array, int count);
> 
> The kernel's equivalent of this function retrieves an array of U32s. Is
> one version more correct than the other?

Using u32 is a better idea.  The property formats are all defined in
terms of fixed width elements, so using a vague width type like int to
interact with it is a bad idea.

-- 
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

Re: [PATCH 01/14] fdt: Tidy up a few fdtdec problems

[Simon Glass]

Hi Stephen,

On Mon, Nov 28, 2011 at 10:33 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> This fixes three trivial issues in fdtdec.c:
>> 1. fdtdec_get_is_enabled() doesn't really need a default value
>> 2. The fdt must be word-aligned, since otherwise it will fail on ARM
>> 3. The compat_names[] array is missing its first element
>
>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> ...
>>  #define COMPAT(id, name) name
>>  static const char * const compat_names[COMPAT_COUNT] = {
>> +     COMPAT(UNKNOWN, "<none>"),
>>  };
>
> Could you educate me on why that change is necessary? Maybe explain this
> in the commit description?

Yes I will update the description. The first element of the array is
supposed to be an invalid entry (see enum fdt_compat_id).

>
>> -int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
>> +int fdtdec_get_is_enabled(const void *blob, int node)
>>  {
>>       const char *cell;
>>
>>       cell = fdt_getprop(blob, node, "status", NULL);
>>       if (cell)
>>               return 0 == strcmp(cell, "ok");
>> -     return default_val;
>> +     return 1;
>>  }
>
> Not that this patch changes this, but isn't "okay" also a legal
> "enabled" value, and perhaps even the recommended value? See
> http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-July/006389.html.

Yes, I will change this, thanks.

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH 01/14] fdt: Tidy up a few fdtdec problems

[Simon Glass]

Hi Stephen,

On Mon, Nov 28, 2011 at 10:33 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> This fixes three trivial issues in fdtdec.c:
>> 1. fdtdec_get_is_enabled() doesn't really need a default value
>> 2. The fdt must be word-aligned, since otherwise it will fail on ARM
>> 3. The compat_names[] array is missing its first element
>
>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> ...
>> ?#define COMPAT(id, name) name
>> ?static const char * const compat_names[COMPAT_COUNT] = {
>> + ? ? COMPAT(UNKNOWN, "<none>"),
>> ?};
>
> Could you educate me on why that change is necessary? Maybe explain this
> in the commit description?

Yes I will update the description. The first element of the array is
supposed to be an invalid entry (see enum fdt_compat_id).

>
>> -int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
>> +int fdtdec_get_is_enabled(const void *blob, int node)
>> ?{
>> ? ? ? const char *cell;
>>
>> ? ? ? cell = fdt_getprop(blob, node, "status", NULL);
>> ? ? ? if (cell)
>> ? ? ? ? ? ? ? return 0 == strcmp(cell, "ok");
>> - ? ? return default_val;
>> + ? ? return 1;
>> ?}
>
> Not that this patch changes this, but isn't "okay" also a legal
> "enabled" value, and perhaps even the recommended value? See
> http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-July/006389.html.

Yes, I will change this, thanks.

Regards,
Simon

>
> --
> nvpublic
>

Re: [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Simon Glass]

Hi Stephen,

On Mon, Nov 28, 2011 at 10:41 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> Add a function to lookup a property which is a phandle in a node, and
>> another to read a fixed-length integer array from an fdt property.
>> Also add a function to read boolean properties.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Looking at the U-Boot custodians web page, you need to send the core DT
> changes (well, probably anything DT related) to Jerry Van Baren.

Yes the tag was there but not picked up as I didn't have Mike's alias
file in my tree. Will fix.

>
>> +/**
>> + * Look up a property in a node and return its contents in an integer
>> + * array of given length. The property must have at least enough data for
>> + * the array (4*count bytes). It may have more, but this will be ignored.
>> + *
>> + * @param blob               FDT blob
>> + * @param node               node to examine
>> + * @param prop_name  name of property to find
>> + * @param array              array to fill with data
>> + * @param count              number of array elements
>> + * @return 0 if ok, or -FDT_ERR_NOTFOUND if the property is not found,
>> + *           or -FDT_ERR_BADLAYOUT if not enough data
>> + */
>> +int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
>> +             int *array, int count);
>
> The kernel's equivalent of this function retrieves an array of U32s. Is
> one version more correct than the other?

I would prefer to have signed, but I will change it to use u32 *.

>
>> +/**
>> + * Look up a boolean property in a node and return it.
>> + *
>> + * A boolean properly is true if present in the device tree and false if not
>> + * present, or present with a 0 value.
>> + *
>> + * @param blob       FDT blob
>> + * @param node       node to examine
>> + * @param prop_name  name of property to find
>> + * @return 1 if the properly is present; 0 if it isn't present or is 0
>> + */
>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
>
> Does U-Boot allow use of the "bool" type here?

Which bool type? It is returning an int.

>
>
>> +/**
>> + * Look up a property in a node and check that it has a minimum length.
>> + *
>> + * @param blob               FDT blob
>> + * @param node               node to examine
>> + * @param prop_name  name of property to find
>> + * @param min_len    minimum property length in bytes
>> + * @param err                0 if ok, or -FDT_ERR_NOTFOUND if the property is not
>> +                     found, or -FDT_ERR_BADLAYOUT if not enough data
>> + * @return pointer to cell, which is only valid if err == 0
>> + */
>> +static const void *get_prop_len(const void *blob, int node,
>> +             const char *prop_name, int min_len, int *err)
>
> Based on the function name, I'd expect it to return the length of the
> property; perhaps get_prop_check_min_len?

Changed, thanks.
>
>> +/**
>> + * Look up a boolean property in a node and return it.
>> + *
>> + * A boolean properly is true if present in the device tree and false if not
>> + * present, or present with a 0 value.
>> + *
>> + * @param blob       FDT blob
>> + * @param node       node to examine
>> + * @param prop_name  name of property to find
>> + * @return 1 if the properly is present; 0 if it isn't present or is 0
>> + */
>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
>> +{
>> +     const s32 *cell;
>> +     int len;
>> +
>> +     debug("%s: %s\n", __func__, prop_name);
>> +     cell = fdt_getprop(blob, node, prop_name, &len);
>> +     if (!cell)
>> +             return 0;
>> +     if (len >= sizeof(u32) && *cell == 0)
>> +             return 0;
>> +
>> +     return 1;
>> +}
>
> In the kernel, I believe that property existence is all that's usually
> checked. Is that wrong? Did the definition of a boolean property's value
> in the function description above come from the specification? If a
> property had a length of 0/1/2/3 with a zero value, it seems very odd to
> treat that as true.

It is useful to be able to set the value to 0 or 1 (with fdtget/put),
rather than remove or add the property. A value with a length of less
than one cell is considered illegal here.

The basic idea is that the presence of the property means that it is
'true'. If it happens to have a value, then we allow that to specify
'false' if it is zero.

Thoughts?

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Simon Glass]

Hi Stephen,

On Mon, Nov 28, 2011 at 10:41 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> Add a function to lookup a property which is a phandle in a node, and
>> another to read a fixed-length integer array from an fdt property.
>> Also add a function to read boolean properties.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Looking at the U-Boot custodians web page, you need to send the core DT
> changes (well, probably anything DT related) to Jerry Van Baren.

Yes the tag was there but not picked up as I didn't have Mike's alias
file in my tree. Will fix.

>
>> +/**
>> + * Look up a property in a node and return its contents in an integer
>> + * array of given length. The property must have at least enough data for
>> + * the array (4*count bytes). It may have more, but this will be ignored.
>> + *
>> + * @param blob ? ? ? ? ? ? ? FDT blob
>> + * @param node ? ? ? ? ? ? ? node to examine
>> + * @param prop_name ?name of property to find
>> + * @param array ? ? ? ? ? ? ?array to fill with data
>> + * @param count ? ? ? ? ? ? ?number of array elements
>> + * @return 0 if ok, or -FDT_ERR_NOTFOUND if the property is not found,
>> + * ? ? ? ? ? or -FDT_ERR_BADLAYOUT if not enough data
>> + */
>> +int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
>> + ? ? ? ? ? ? int *array, int count);
>
> The kernel's equivalent of this function retrieves an array of U32s. Is
> one version more correct than the other?

I would prefer to have signed, but I will change it to use u32 *.

>
>> +/**
>> + * Look up a boolean property in a node and return it.
>> + *
>> + * A boolean properly is true if present in the device tree and false if not
>> + * present, or present with a 0 value.
>> + *
>> + * @param blob ? ? ? FDT blob
>> + * @param node ? ? ? node to examine
>> + * @param prop_name ?name of property to find
>> + * @return 1 if the properly is present; 0 if it isn't present or is 0
>> + */
>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
>
> Does U-Boot allow use of the "bool" type here?

Which bool type? It is returning an int.

>
>
>> +/**
>> + * Look up a property in a node and check that it has a minimum length.
>> + *
>> + * @param blob ? ? ? ? ? ? ? FDT blob
>> + * @param node ? ? ? ? ? ? ? node to examine
>> + * @param prop_name ?name of property to find
>> + * @param min_len ? ?minimum property length in bytes
>> + * @param err ? ? ? ? ? ? ? ?0 if ok, or -FDT_ERR_NOTFOUND if the property is not
>> + ? ? ? ? ? ? ? ? ? ? found, or -FDT_ERR_BADLAYOUT if not enough data
>> + * @return pointer to cell, which is only valid if err == 0
>> + */
>> +static const void *get_prop_len(const void *blob, int node,
>> + ? ? ? ? ? ? const char *prop_name, int min_len, int *err)
>
> Based on the function name, I'd expect it to return the length of the
> property; perhaps get_prop_check_min_len?

Changed, thanks.
>
>> +/**
>> + * Look up a boolean property in a node and return it.
>> + *
>> + * A boolean properly is true if present in the device tree and false if not
>> + * present, or present with a 0 value.
>> + *
>> + * @param blob ? ? ? FDT blob
>> + * @param node ? ? ? node to examine
>> + * @param prop_name ?name of property to find
>> + * @return 1 if the properly is present; 0 if it isn't present or is 0
>> + */
>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
>> +{
>> + ? ? const s32 *cell;
>> + ? ? int len;
>> +
>> + ? ? debug("%s: %s\n", __func__, prop_name);
>> + ? ? cell = fdt_getprop(blob, node, prop_name, &len);
>> + ? ? if (!cell)
>> + ? ? ? ? ? ? return 0;
>> + ? ? if (len >= sizeof(u32) && *cell == 0)
>> + ? ? ? ? ? ? return 0;
>> +
>> + ? ? return 1;
>> +}
>
> In the kernel, I believe that property existence is all that's usually
> checked. Is that wrong? Did the definition of a boolean property's value
> in the function description above come from the specification? If a
> property had a length of 0/1/2/3 with a zero value, it seems very odd to
> treat that as true.

It is useful to be able to set the value to 0 or 1 (with fdtget/put),
rather than remove or add the property. A value with a length of less
than one cell is considered illegal here.

The basic idea is that the presence of the property means that it is
'true'. If it happens to have a value, then we allow that to specify
'false' if it is zero.

Thoughts?

Regards,
Simon

>
> --
> nvpublic
>

Re: [PATCH 05/14] tegra: fdt: Add Tegra2x device tree file

[Simon Glass]

Hi Stephen,

On Mon, Nov 28, 2011 at 10:56 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> This was taken from commit 1ea6b8f at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git
>
> That's not the latest version in linux-next. Also, this doesn't include
> quite a few changes that have been sent to the mailing lists but not yet
> applied.

OK I see a newer version in 'next' that now has USB so have picked
that up. I picked 'master' originally.

>
> In particular, linux-next now includes a minimal USB binding. Should we
> just use this in U-Boot for now? We should get review on the kernel
> lists before bringing in this more advanced USB binding in U-Boot, and
> perhaps even add the binding into the kernel at the same time?

I copied my email to the device-tree mailing list. Hopefully that is
enough to get a review there. It feels wrong to send U-Boot patches to
the kernel list...?

>
> Patches have been posted to:
> * Convert to the finalized ARM GIC binding.
> * Disable devices in the per-board .dts files that aren't used on those
> boards.
> * Various other cleanups in order to make the .dts files match the
> kernel's non-DT board files.
> * Perhaps more that I forget.

Well I would prefer to pick these up when they are actually applied!

>
> I suppose those could be applied to U-Boot as and when they are applied
> to the kernel.

Yes I think so.

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH 05/14] tegra: fdt: Add Tegra2x device tree file

[Simon Glass]

Hi Stephen,

On Mon, Nov 28, 2011 at 10:56 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> This was taken from commit 1ea6b8f at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git
>
> That's not the latest version in linux-next. Also, this doesn't include
> quite a few changes that have been sent to the mailing lists but not yet
> applied.

OK I see a newer version in 'next' that now has USB so have picked
that up. I picked 'master' originally.

>
> In particular, linux-next now includes a minimal USB binding. Should we
> just use this in U-Boot for now? We should get review on the kernel
> lists before bringing in this more advanced USB binding in U-Boot, and
> perhaps even add the binding into the kernel at the same time?

I copied my email to the device-tree mailing list. Hopefully that is
enough to get a review there. It feels wrong to send U-Boot patches to
the kernel list...?

>
> Patches have been posted to:
> * Convert to the finalized ARM GIC binding.
> * Disable devices in the per-board .dts files that aren't used on those
> boards.
> * Various other cleanups in order to make the .dts files match the
> kernel's non-DT board files.
> * Perhaps more that I forget.

Well I would prefer to pick these up when they are actually applied!

>
> I suppose those could be applied to U-Boot as and when they are applied
> to the kernel.

Yes I think so.

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH 09/14] tegra: usb: Add support for data alignment and txfifo threshold

[Simon Glass]

Hi Stephen,

On Mon, Nov 28, 2011 at 11:05 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of data for
>> USB packets (e.g. 4 means word-aligned). This is required for Tegra
>> to operate.
>>
>> CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the txfilltuning
>> field in the EHCI controller on reset.

Sorry I am getting very little time for this this week. Hope to update
the series tomorrow.

>
> Shouldn't that be two separate patches?

Yes, will split.

>
>> diff --git a/README b/README
> ...
>> + ? ? ? ? ? ? CONFIG_USB_EHCI_DATA_ALIGN sets the required alignment of
>> + ? ? ? ? ? ? data for USB packets (e.g. 4 means word-aligned). This is
>> + ? ? ? ? ? ? required for Tegra to operate.
>> +
>> + ? ? ? ? ? ? CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
>> + ? ? ? ? ? ? txfilltuning field in the EHCI controller on reset.
>> +
>
>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> ...
>> @@ -322,6 +329,27 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
>> ? ? ? int timeout;
>> ? ? ? int ret = 0;
>>
>> +#ifdef CONFIG_USB_EHCI_DATA_ALIGN
>> + ? ? /* In case ehci host requires alignment for buffers */
>> + ? ? void *align_buf = NULL;
>> + ? ? void *orig_buf = buffer;
>> + ? ? int unaligned = ((int)buffer & (CONFIG_USB_EHCI_DATA_ALIGN - 1)) != 0;
>
> This uses "!= 0".
>
>> +
>> + ? ? if (unaligned) {
>> + ? ? ? ? ? ? align_buf = malloc(length + CONFIG_USB_EHCI_DATA_ALIGN);
>> + ? ? ? ? ? ? if (!align_buf)
>> + ? ? ? ? ? ? ? ? ? ? return -1;
>> + ? ? ? ? ? ? if ((int)align_buf & (CONFIG_USB_EHCI_DATA_ALIGN - 1))
>
> This doesn't use "!= 0". Probably drop the "!= 0" above?

Dropped.

>
>> + ? ? ? ? ? ? ? ? ? ? buffer = (void *)((int)align_buf +
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? CONFIG_USB_EHCI_DATA_ALIGN -
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((int)align_buf &
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (CONFIG_USB_EHCI_DATA_ALIGN - 1)));
>> + ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? buffer = align_buf;
>
> Why not jsut do the following unconditionally; it seems much simpler
> even if very marginally less efficient:
>
> buffer = (align_buf + CONFIG_USB_EHCI_DATA_ALIGN - 1) &
> ? ? ? ? ? ? ? ?~(CONFIG_USB_EHCI_DATA_ALIGN - 1);

OK done (adding casts back in).

>
> IIRC, in the kernel, we had to use this technique because arbitrary
> callers could have allocated "buffer" however they pleased. I assume the
> same is true in U-Boot; there isn't some way that this alignment
> restriction could be implemented in some core USB buffer allocation
> routine instead, and thus avoid all the copying? Do you know how often
> unaligned buffers actually occur in practice, and hence how much of a
> performance impact the copying will have?

No I don't but the same exercise has happened with MMC, so things will
be better now than they were. We might be able to turn this into an
assert perhaps in the future.

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> This adds basic support for the Tegra2 USB controller. Board files should
>> call board_usb_init() to set things up.
>
> Just a very brief review:

Thank you.

I will tidy up the code changes later, but just wanted to pick up on a
few general points.

>
>> +/* Put the port into host mode (this only works for USB1) */
>> +static void set_host_mode(struct usb_ctlr *usbctlr)
>> +{
>> + ? ? ? /* Check whether remote host from USB1 is driving VBus */
>> + ? ? ? if (readl(&usbctlr->phy_vbus_sensors) & VBUS_VLD_STS)
>> + ? ? ? ? ? ? ? return;
>> +
>> + ? ? ? /*
>> + ? ? ? ?* If not driving, we set GPIO USB1_VBus_En. Seaboard platform uses
>> + ? ? ? ?* PAD SLXK (GPIO D.00) as USB1_VBus_En Config as GPIO
>> + ? ? ? ?*/
>> + ? ? ? gpio_direction_output(GPIO_PD0, 1);
>
> The GPIO to use here needs to be parameterized; there's no reason to
> believe it'll be the same on all boards (or even that boards will have
> any such GPIO).
>
>> +/* Put our ports into host mode */
>> +void usb_set_host_mode(void)
>> +{
>> + ? ? ? if (host_dev_ctlr)
>> + ? ? ? ? ? ? ? set_host_mode(host_dev_ctlr);
>> +}
>
> Why not just call set_host_mode() directly from board_usb_init()?
>
>> +#ifndef CONFIG_OF_CONTROL
>> +static int probe_port(struct usb_ctlr *usbctlr, const int params[])
>> +{
>> + ? ? ? enum periph_id id;
>> + ? ? ? int utmi = 0;
>> +
>> + ? ? ? /*
>> + ? ? ? ?* Get the periph id. Port 1 has an internal transceiver, port 3 is
>> + ? ? ? ?* external
>> + ? ? ? ?*/
>> + ? ? ? switch ((u32)usbctlr) {
>> + ? ? ? case TEGRA_USB1_BASE:
>> + ? ? ? ? ? ? ? id = PERIPH_ID_USBD;
>> + ? ? ? ? ? ? ? break;
>> +
>> + ? ? ? case TEGRA_USB3_BASE:
>> + ? ? ? ? ? ? ? id = PERIPH_ID_USB3;
>> + ? ? ? ? ? ? ? utmi = 1;
>> + ? ? ? ? ? ? ? break;
>> +
>> + ? ? ? default:
>> + ? ? ? ? ? ? ? printf("tegrausb: probe_port: no such port %p\n", usbctlr);
>> + ? ? ? ? ? ? ? return -1;
>> + ? ? ? }
>
> What about the other port (USB2)?

Not yet supported. I don't have a board that brings it out.

>
>> +#ifdef CONFIG_OF_CONTROL
>> +int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
>> + ? ? ? ? ? ? ? ? ?struct fdt_usb *config)
>> +{
>> + ? ? ? int clk_node = 0, rate;
>> +
>> + ? ? ? /* Find the parameters for our oscillator frequency */
>> + ? ? ? do {
>> + ? ? ? ? ? ? ? clk_node = fdt_node_offset_by_compatible(blob, clk_node,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "nvidia,tegra20-usbparams");
>> + ? ? ? ? ? ? ? if (clk_node < 0) {
>> + ? ? ? ? ? ? ? ? ? ? ? debug("Cannot find USB params for clock %u",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? osc_frequency_mhz);
>> + ? ? ? ? ? ? ? ? ? ? ? return -FDT_ERR_NOTFOUND;
>> + ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? rate = fdtdec_get_int(blob, clk_node, "osc-frequency", 0);
>> + ? ? ? } while (rate != osc_frequency_mhz);
>> +
>> + ? ? ? config->reg = (struct usb_ctlr *)fdtdec_get_addr(blob, node, "reg");
>> + ? ? ? config->host_mode = fdtdec_get_bool(blob, node, "support-host-mode");
>
> Property "support-host-mode" isn't something that's supported by the
> kernel binding, and needs discussion/ack there.

Do you mean device-tree list? I did send the patch there. Does the
kernel have another way of knowing that this port is special?

>
>> + ? ? ? config->utmi = fdtdec_lookup_phandle(blob, node, "utmi") >= 0;
>
> In the kernel DT binding, this is property "phy_type" with legal values
> "utmi" or "ulpi."
>
>> + ? ? ? config->enabled = fdtdec_get_is_enabled(blob, node);
>> + ? ? ? config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>
> periph-id is a U-Boot specific concept, not HW description. The DT
> shouldn't contain that value.

It is actually the bit position of the peripheral in the clock
registers, so arguably a hardware description. U-Boot uses this to
efficiently manage peripheral clocks, reset, pinmux, etc.

How does the kernel figure out the clock register (etc.) to use with a
particular peripheral? Also bear in mind that the intent with U-Boot
is to be a lot more lightweight with these things.

>
>> + ? ? ? if (config->periph_id == -1)
>> + ? ? ? ? ? ? ? return -FDT_ERR_NOTFOUND;
>> +
>> + ? ? ? return fdtdec_get_int_array(blob, clk_node, "params", config->params,
>> + ? ? ? ? ? ? ? ? ? ? ? PARAM_COUNT);
>
> Property "params" (which should probably be named something better
> anyway) isn't something that's supported by the kernel binding, and
> needs discussion/ack there. Instead, I suggest following the kernel's
> approach for now - don't specify these PHY parameters in the binding,
> but rather just apply the defaults, which are apparently suitable for
> all the boards supported by the mainline kernel at least.

My actual intent was that the device tree would provide options for
each oscillator frequency and the board would simply select which it
is using.  This does not fit well with out the device tree works
though - we end up having everything in there and available at
run-time, even useless data. Anyway, the oscillator frequency is
detected at run-time, so I decided to put these into the SOC
description.

Since you say these values are fixed for all boards we might as well
put them back into the code.

>
>
>> +int board_usb_init(const void *blob)
>> +{
>> +#ifdef CONFIG_OF_CONTROL
>> + ? ? ? struct fdt_usb config;
>> + ? ? ? int clk_done = 0;
>> + ? ? ? int node, upto = 0;
>> + ? ? ? unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>> +
>> + ? ? ? do {
>> + ? ? ? ? ? ? ? node = fdtdec_next_alias(blob, "usb",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?COMPAT_NVIDIA_TEGRA20_USB, &upto);
>
> Why only initialize USB controllers with aliases? Surely this should
> enumerate all nodes with a specific compatible flag?

The aliases are (I thought) the official way that device trees specify
device ordering. No we do not enumerate things in U-Boot - there is no
device model as such. We can do this on Tegra, but still need to know
the order to use (i.e. which is port 0).

Regards,
Simon

>
>> + ? ? ? ? ? ? ? /* The first port we find gets to set the clocks */
>
> Ick.
>
>> + ? ? ? /* Set up our two ports */
>> +#ifdef CONFIG_TEGRA_USB1_HOST
>> + ? ? ? host_dev_ctlr = (struct usb_ctlr *)TEGRA_USB1_BASE;
>> +#endif
>
> That port is always the host/device switchable controller. Why not
> always make that assignment? The issue here is probably that
> set_host_mode() isn't suitable for all boards. The solution seems to be
> to fix that.
>
> --
> nvpublic
>

Re: [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Jerry Van Baren]

On 11/28/2011 01:41 PM, Stephen Warren wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> Add a function to lookup a property which is a phandle in a node, and
>> another to read a fixed-length integer array from an fdt property.
>> Also add a function to read boolean properties.
>>
>> Signed-off-by: Simon Glass<sjg@chromium.org>
>
> Looking at the U-Boot custodians web page, you need to send the core DT
> changes (well, probably anything DT related) to Jerry Van Baren.

Yeah, I saw the patch go by.  I did not recognize the file it patched 
and looked at the history:

commit b5220bc6ed6e6a197adf4926958dca1df4b420b0
Author: Simon Glass <sjg@chromium.org>
Date:   Mon Oct 24 19:15:32 2011 +0000

     fdt: add decode helper library

     This library provides useful functions to drivers which want to use
     the fdt to control their operation. Functions are provided to:
     :
     :

and the copyright is "Copyright (c) 2011 The Chromium OS Authors."

FDT helper functions have been accumulating in common/fdt_support.c 
rather than a separate file.  Simon, what is the history of 
lib/fdtdec.c?  Is it a shared file from the linux kernel?  If it is 
u-boot specific, it would probably be better to add the functions to 
fdt_support.c.

In the same vein, I also have not looked at the functions provided by 
fdtdec.c to see if there is any overlap with existing fdt_support.c 
functions (a quick look says not).

Best regards,
gvb

[snip]

[U-Boot] [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Jerry Van Baren]

On 11/28/2011 01:41 PM, Stephen Warren wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> Add a function to lookup a property which is a phandle in a node, and
>> another to read a fixed-length integer array from an fdt property.
>> Also add a function to read boolean properties.
>>
>> Signed-off-by: Simon Glass<sjg@chromium.org>
>
> Looking at the U-Boot custodians web page, you need to send the core DT
> changes (well, probably anything DT related) to Jerry Van Baren.

Yeah, I saw the patch go by.  I did not recognize the file it patched 
and looked at the history:

commit b5220bc6ed6e6a197adf4926958dca1df4b420b0
Author: Simon Glass <sjg@chromium.org>
Date:   Mon Oct 24 19:15:32 2011 +0000

     fdt: add decode helper library

     This library provides useful functions to drivers which want to use
     the fdt to control their operation. Functions are provided to:
     :
     :

and the copyright is "Copyright (c) 2011 The Chromium OS Authors."

FDT helper functions have been accumulating in common/fdt_support.c 
rather than a separate file.  Simon, what is the history of 
lib/fdtdec.c?  Is it a shared file from the linux kernel?  If it is 
u-boot specific, it would probably be better to add the functions to 
fdt_support.c.

In the same vein, I also have not looked at the functions provided by 
fdtdec.c to see if there is any overlap with existing fdt_support.c 
functions (a quick look says not).

Best regards,
gvb

[snip]

Re: [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Simon Glass]

Hi Jerry,

On Thu, Dec 1, 2011 at 7:33 PM, Jerry Van Baren <gvb.uboot@gmail.com> wrote:
> On 11/28/2011 01:41 PM, Stephen Warren wrote:
>>
>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>
>>> Add a function to lookup a property which is a phandle in a node, and
>>> another to read a fixed-length integer array from an fdt property.
>>> Also add a function to read boolean properties.
>>>
>>> Signed-off-by: Simon Glass<sjg@chromium.org>
>>
>> Looking at the U-Boot custodians web page, you need to send the core DT
>> changes (well, probably anything DT related) to Jerry Van Baren.
>
> Yeah, I saw the patch go by.  I did not recognize the file it patched and
> looked at the history:
>
> commit b5220bc6ed6e6a197adf4926958dca1df4b420b0
> Author: Simon Glass <sjg@chromium.org>
> Date:   Mon Oct 24 19:15:32 2011 +0000
>
>    fdt: add decode helper library
>
>    This library provides useful functions to drivers which want to use
>    the fdt to control their operation. Functions are provided to:
>    :
>    :
>
> and the copyright is "Copyright (c) 2011 The Chromium OS Authors."
>
> FDT helper functions have been accumulating in common/fdt_support.c rather
> than a separate file.  Simon, what is the history of lib/fdtdec.c?  Is it a
> shared file from the linux kernel?  If it is u-boot specific, it would
> probably be better to add the functions to fdt_support.c.

There are sort-of two FDT strands within U-Boot. The main one is
support for putting together an FDT to pass to the kernel (cmd_fdt,
fdt_support), and the other is for U-Boot's own use (run-time
configuration of U-Boot, fdtdec). They both use libfdt.

I regard fdt_support as part of the former, and fdtdec (decode-only
helper functions) as part of the latter. At present when you turn on
CONFIG_OF_LIBFDT you get everything, but we could provide finer
granularity for platforms which only want to decode an FDT for
run-time config and don't want to mess with it. Since fdt_decode is
about 6KB of code that might be useful.

>
> In the same vein, I also have not looked at the functions provided by
> fdtdec.c to see if there is any overlap with existing fdt_support.c
> functions (a quick look says not).

No, they are pretty low-level. An argument could be made for them to
go into libfdt once they are stable, but we are certainly not there
yet. I have quite a few patches which add more functions for
extracting data.

Regards,
Simon

>
> Best regards,
> gvb
>
> [snip]
>

[U-Boot] [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Simon Glass]

Hi Jerry,

On Thu, Dec 1, 2011 at 7:33 PM, Jerry Van Baren <gvb.uboot@gmail.com> wrote:
> On 11/28/2011 01:41 PM, Stephen Warren wrote:
>>
>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>
>>> Add a function to lookup a property which is a phandle in a node, and
>>> another to read a fixed-length integer array from an fdt property.
>>> Also add a function to read boolean properties.
>>>
>>> Signed-off-by: Simon Glass<sjg@chromium.org>
>>
>> Looking at the U-Boot custodians web page, you need to send the core DT
>> changes (well, probably anything DT related) to Jerry Van Baren.
>
> Yeah, I saw the patch go by. ?I did not recognize the file it patched and
> looked at the history:
>
> commit b5220bc6ed6e6a197adf4926958dca1df4b420b0
> Author: Simon Glass <sjg@chromium.org>
> Date: ? Mon Oct 24 19:15:32 2011 +0000
>
> ? ?fdt: add decode helper library
>
> ? ?This library provides useful functions to drivers which want to use
> ? ?the fdt to control their operation. Functions are provided to:
> ? ?:
> ? ?:
>
> and the copyright is "Copyright (c) 2011 The Chromium OS Authors."
>
> FDT helper functions have been accumulating in common/fdt_support.c rather
> than a separate file. ?Simon, what is the history of lib/fdtdec.c? ?Is it a
> shared file from the linux kernel? ?If it is u-boot specific, it would
> probably be better to add the functions to fdt_support.c.

There are sort-of two FDT strands within U-Boot. The main one is
support for putting together an FDT to pass to the kernel (cmd_fdt,
fdt_support), and the other is for U-Boot's own use (run-time
configuration of U-Boot, fdtdec). They both use libfdt.

I regard fdt_support as part of the former, and fdtdec (decode-only
helper functions) as part of the latter. At present when you turn on
CONFIG_OF_LIBFDT you get everything, but we could provide finer
granularity for platforms which only want to decode an FDT for
run-time config and don't want to mess with it. Since fdt_decode is
about 6KB of code that might be useful.

>
> In the same vein, I also have not looked at the functions provided by
> fdtdec.c to see if there is any overlap with existing fdt_support.c
> functions (a quick look says not).

No, they are pretty low-level. An argument could be made for them to
go into libfdt once they are stable, but we are certainly not there
yet. I have quite a few patches which add more functions for
extracting data.

Regards,
Simon

>
> Best regards,
> gvb
>
> [snip]
>

Re: [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Stephen Warren]

On 12/01/2011 06:01 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Nov 28, 2011 at 10:41 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>> Add a function to lookup a property which is a phandle in a node, and
>>> another to read a fixed-length integer array from an fdt property.
>>> Also add a function to read boolean properties.
>>>
>>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

>>> +/**
>>> + * Look up a boolean property in a node and return it.
>>> + *
>>> + * A boolean properly is true if present in the device tree and false if not
>>> + * present, or present with a 0 value.
>>> + *
>>> + * @param blob       FDT blob
>>> + * @param node       node to examine
>>> + * @param prop_name  name of property to find
>>> + * @return 1 if the properly is present; 0 if it isn't present or is 0
>>> + */
>>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
>>
>> Does U-Boot allow use of the "bool" type here?
> 
> Which bool type? It is returning an int.

I was asking if the return type could be changed to bool.

>>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
>>> +{
>>> +     const s32 *cell;
>>> +     int len;
>>> +
>>> +     debug("%s: %s\n", __func__, prop_name);
>>> +     cell = fdt_getprop(blob, node, prop_name, &len);
>>> +     if (!cell)
>>> +             return 0;
>>> +     if (len >= sizeof(u32) && *cell == 0)
>>> +             return 0;
>>> +
>>> +     return 1;
>>> +}
>>
>> In the kernel, I believe that property existence is all that's usually
>> checked. Is that wrong? Did the definition of a boolean property's value
>> in the function description above come from the specification? If a
>> property had a length of 0/1/2/3 with a zero value, it seems very odd to
>> treat that as true.
> 
> It is useful to be able to set the value to 0 or 1 (with fdtget/put),
> rather than remove or add the property. A value with a length of less
> than one cell is considered illegal here.
> 
> The basic idea is that the presence of the property means that it is
> 'true'. If it happens to have a value, then we allow that to specify
> 'false' if it is zero.

Well, it's more up to standard device tree practice, not me. I've
certainly sent patches that used a property with 0/1 value as a bool
and received review feedback from DT experts that DT represents bools
as present/absent properties, both with no value, so I assume zero
length.

-- 
nvpublic

[U-Boot] [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Stephen Warren]

On 12/01/2011 06:01 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Nov 28, 2011 at 10:41 AM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>> Add a function to lookup a property which is a phandle in a node, and
>>> another to read a fixed-length integer array from an fdt property.
>>> Also add a function to read boolean properties.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>

>>> +/**
>>> + * Look up a boolean property in a node and return it.
>>> + *
>>> + * A boolean properly is true if present in the device tree and false if not
>>> + * present, or present with a 0 value.
>>> + *
>>> + * @param blob       FDT blob
>>> + * @param node       node to examine
>>> + * @param prop_name  name of property to find
>>> + * @return 1 if the properly is present; 0 if it isn't present or is 0
>>> + */
>>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
>>
>> Does U-Boot allow use of the "bool" type here?
> 
> Which bool type? It is returning an int.

I was asking if the return type could be changed to bool.

>>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
>>> +{
>>> +     const s32 *cell;
>>> +     int len;
>>> +
>>> +     debug("%s: %s\n", __func__, prop_name);
>>> +     cell = fdt_getprop(blob, node, prop_name, &len);
>>> +     if (!cell)
>>> +             return 0;
>>> +     if (len >= sizeof(u32) && *cell == 0)
>>> +             return 0;
>>> +
>>> +     return 1;
>>> +}
>>
>> In the kernel, I believe that property existence is all that's usually
>> checked. Is that wrong? Did the definition of a boolean property's value
>> in the function description above come from the specification? If a
>> property had a length of 0/1/2/3 with a zero value, it seems very odd to
>> treat that as true.
> 
> It is useful to be able to set the value to 0 or 1 (with fdtget/put),
> rather than remove or add the property. A value with a length of less
> than one cell is considered illegal here.
> 
> The basic idea is that the presence of the property means that it is
> 'true'. If it happens to have a value, then we allow that to specify
> 'false' if it is zero.

Well, it's more up to standard device tree practice, not me. I've
certainly sent patches that used a property with 0/1 value as a bool
and received review feedback from DT experts that DT represents bools
as present/absent properties, both with no value, so I assume zero
length.

-- 
nvpublic

Re: [PATCH 05/14] tegra: fdt: Add Tegra2x device tree file

[Stephen Warren]

On 12/01/2011 06:24 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Nov 28, 2011 at 10:56 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>> This was taken from commit 1ea6b8f at:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git
>>
...
>> In particular, linux-next now includes a minimal USB binding. Should we
>> just use this in U-Boot for now? We should get review on the kernel
>> lists before bringing in this more advanced USB binding in U-Boot, and
>> perhaps even add the binding into the kernel at the same time?
> 
> I copied my email to the device-tree mailing list. Hopefully that is
> enough to get a review there. It feels wrong to send U-Boot patches to
> the kernel list...?

I think the kernel and U-boot need to co-ordinate on the DT bindings.
Posting at least to linux-tegra, the Tegra maintainers, and perhaps even
the kernel's domain-specific list (i.e. linux-usb) seems appropriate to
me. DT is a cross-functional thing, and really needs cross-functional
discussion and review. devicetree-discuss will cover the DT experts, but
probably not the CPU and subsystem experts.

-- 
nvpublic

[U-Boot] [PATCH 05/14] tegra: fdt: Add Tegra2x device tree file

[Stephen Warren]

On 12/01/2011 06:24 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Nov 28, 2011 at 10:56 AM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>> This was taken from commit 1ea6b8f at:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git
>>
...
>> In particular, linux-next now includes a minimal USB binding. Should we
>> just use this in U-Boot for now? We should get review on the kernel
>> lists before bringing in this more advanced USB binding in U-Boot, and
>> perhaps even add the binding into the kernel at the same time?
> 
> I copied my email to the device-tree mailing list. Hopefully that is
> enough to get a review there. It feels wrong to send U-Boot patches to
> the kernel list...?

I think the kernel and U-boot need to co-ordinate on the DT bindings.
Posting at least to linux-tegra, the Tegra maintainers, and perhaps even
the kernel's domain-specific list (i.e. linux-usb) seems appropriate to
me. DT is a cross-functional thing, and really needs cross-functional
discussion and review. devicetree-discuss will cover the DT experts, but
probably not the CPU and subsystem experts.

-- 
nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren]

On 12/01/2011 06:51 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>> This adds basic support for the Tegra2 USB controller. Board files should
>>> call board_usb_init() to set things up.

>>> +#ifdef CONFIG_OF_CONTROL
>>> +int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
>>> +                  struct fdt_usb *config)
>>> +{
>>> +       int clk_node = 0, rate;
>>> +
>>> +       /* Find the parameters for our oscillator frequency */
>>> +       do {
>>> +               clk_node = fdt_node_offset_by_compatible(blob, clk_node,
>>> +                                       "nvidia,tegra20-usbparams");
>>> +               if (clk_node < 0) {
>>> +                       debug("Cannot find USB params for clock %u",
>>> +                             osc_frequency_mhz);
>>> +                       return -FDT_ERR_NOTFOUND;
>>> +               }
>>> +               rate = fdtdec_get_int(blob, clk_node, "osc-frequency", 0);
>>> +       } while (rate != osc_frequency_mhz);
>>> +
>>> +       config->reg = (struct usb_ctlr *)fdtdec_get_addr(blob, node, "reg");
>>> +       config->host_mode = fdtdec_get_bool(blob, node, "support-host-mode");
>>
>> Property "support-host-mode" isn't something that's supported by the
>> kernel binding, and needs discussion/ack there.
> 
> Do you mean device-tree list? I did send the patch there.

linux-tegra and perhaps linux-usb too.

> Does the kernel have another way of knowing that this port is special?

When booted without DT, the platform data defines mode: device, host, OTG.

The current DT bindings for Tegra USB are the minimum required to get
host mode working, and don't include any representation of this, and so
host mode is assumed.

>>> +       config->utmi = fdtdec_lookup_phandle(blob, node, "utmi") >= 0;
>>
>> In the kernel DT binding, this is property "phy_type" with legal values
>> "utmi" or "ulpi."
>>
>>> +       config->enabled = fdtdec_get_is_enabled(blob, node);
>>> +       config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>>
>> periph-id is a U-Boot specific concept, not HW description. The DT
>> shouldn't contain that value.
> 
> It is actually the bit position of the peripheral in the clock
> registers, so arguably a hardware description. U-Boot uses this to
> efficiently manage peripheral clocks, reset, pinmux, etc.
> 
> How does the kernel figure out the clock register (etc.) to use with a
> particular peripheral? Also bear in mind that the intent with U-Boot
> is to be a lot more lightweight with these things.

The DT binding has to be identical though; U-Boot implementation details
aren't supposed to affect the content of the DT.

Clock bindings are an area of active development. I haven't been
following the progress, but I imagine that the clock controller will
define a node per clock, and the devices that consume the clock will
refer to that node using a phandle. It's then up to the clock controller
driver to extract whatever information it needs from the clock node and
map that to an internal periph-id. It's plausible that a legitimate part
of the clock binding itself is such a periph-id field, but that should
be defined by the clock controller binding, not the peripheral binding.

>>> +       if (config->periph_id == -1)
>>> +               return -FDT_ERR_NOTFOUND;
>>> +
>>> +       return fdtdec_get_int_array(blob, clk_node, "params", config->params,
>>> +                       PARAM_COUNT);
>>
>> Property "params" (which should probably be named something better
>> anyway) isn't something that's supported by the kernel binding, and
>> needs discussion/ack there. Instead, I suggest following the kernel's
>> approach for now - don't specify these PHY parameters in the binding,
>> but rather just apply the defaults, which are apparently suitable for
>> all the boards supported by the mainline kernel at least.
> 
> My actual intent was that the device tree would provide options for
> each oscillator frequency and the board would simply select which it
> is using.  This does not fit well with out the device tree works
> though - we end up having everything in there and available at
> run-time, even useless data. Anyway, the oscillator frequency is
> detected at run-time, so I decided to put these into the SOC
> description.
> 
> Since you say these values are fixed for all boards we might as well
> put them back into the code.

The values aren't necessarily fixed for /all/ boards, they just /happen/
to be identical for all boards currently supported in the mainline
kernel. I imagine that as more boards are supported, we'll see different
sets of values in use.

>>> +int board_usb_init(const void *blob)
>>> +{
>>> +#ifdef CONFIG_OF_CONTROL
>>> +       struct fdt_usb config;
>>> +       int clk_done = 0;
>>> +       int node, upto = 0;
>>> +       unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>>> +
>>> +       do {
>>> +               node = fdtdec_next_alias(blob, "usb",
>>> +                                        COMPAT_NVIDIA_TEGRA20_USB, &upto);
>>
>> Why only initialize USB controllers with aliases? Surely this should
>> enumerate all nodes with a specific compatible flag?
> 
> The aliases are (I thought) the official way that device trees specify
> device ordering. No we do not enumerate things in U-Boot - there is no
> device model as such. We can do this on Tegra, but still need to know
> the order to use (i.e. which is port 0).

I don't believe the kernel uses the alias for anything at all right now.
Instead, it enumerates all nodes that match a certain compatible flag,
and instantiates a device for each one it has a driver for. I believe
this mode of operation is pretty implicit in DT itself; it's something
U-Boot should do too.

If U-boot were to operate solely based on the aliases node, it wouldn't
work at all for the .dts files currently in the kernel since there are
no aliases, and if there were aliases, might end up instantiating a
subset of devices if some devices weren't mentioned in aliases.

I believe what aliases is meant for is that once you've enumerated all
the devices, then check the aliases node in order to override the
default naming (or set up alternative names). Still, you'd have to check
with a DT expert here, since I've never done anything with the aliases node.

-- 
nvpublic

Re: [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Simon Glass]

Hi Stephen,

On Fri, Dec 2, 2011 at 7:55 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/01/2011 06:01 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Nov 28, 2011 at 10:41 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>> Add a function to lookup a property which is a phandle in a node, and
>>>> another to read a fixed-length integer array from an fdt property.
>>>> Also add a function to read boolean properties.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
>>>> +/**
>>>> + * Look up a boolean property in a node and return it.
>>>> + *
>>>> + * A boolean properly is true if present in the device tree and false if not
>>>> + * present, or present with a 0 value.
>>>> + *
>>>> + * @param blob       FDT blob
>>>> + * @param node       node to examine
>>>> + * @param prop_name  name of property to find
>>>> + * @return 1 if the properly is present; 0 if it isn't present or is 0
>>>> + */
>>>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
>>>
>>> Does U-Boot allow use of the "bool" type here?
>>
>> Which bool type? It is returning an int.
>
> I was asking if the return type could be changed to bool.

Sorry I misunderstood -  no U-Boot does not have a bool type yet.

>
>>>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
>>>> +{
>>>> +     const s32 *cell;
>>>> +     int len;
>>>> +
>>>> +     debug("%s: %s\n", __func__, prop_name);
>>>> +     cell = fdt_getprop(blob, node, prop_name, &len);
>>>> +     if (!cell)
>>>> +             return 0;
>>>> +     if (len >= sizeof(u32) && *cell == 0)
>>>> +             return 0;
>>>> +
>>>> +     return 1;
>>>> +}
>>>
>>> In the kernel, I believe that property existence is all that's usually
>>> checked. Is that wrong? Did the definition of a boolean property's value
>>> in the function description above come from the specification? If a
>>> property had a length of 0/1/2/3 with a zero value, it seems very odd to
>>> treat that as true.
>>
>> It is useful to be able to set the value to 0 or 1 (with fdtget/put),
>> rather than remove or add the property. A value with a length of less
>> than one cell is considered illegal here.
>>
>> The basic idea is that the presence of the property means that it is
>> 'true'. If it happens to have a value, then we allow that to specify
>> 'false' if it is zero.
>
> Well, it's more up to standard device tree practice, not me. I've
> certainly sent patches that used a property with 0/1 value as a bool
> and received review feedback from DT experts that DT represents bools
> as present/absent properties, both with no value, so I assume zero
> length.

Yes that is the preferred way. However, it is useful to be able to
define a meaning when the property does have a value. The value may
come into the fdt later after compilation. At that point it is easier
to change a value rather than make something present/absent. For one
thing it can be done without adjust the size of the fdt blob.

Perhaps the solution is to adjust fdtput to support boolean values
explicitly (in that it would add or remove the property based on a
command-line 0/1 value).

Still I think it is useful to support a zero value meaning false when
it is provided.

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Simon Glass]

Hi Stephen,

On Fri, Dec 2, 2011 at 7:55 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/01/2011 06:01 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Nov 28, 2011 at 10:41 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>> Add a function to lookup a property which is a phandle in a node, and
>>>> another to read a fixed-length integer array from an fdt property.
>>>> Also add a function to read boolean properties.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
>>>> +/**
>>>> + * Look up a boolean property in a node and return it.
>>>> + *
>>>> + * A boolean properly is true if present in the device tree and false if not
>>>> + * present, or present with a 0 value.
>>>> + *
>>>> + * @param blob ? ? ? FDT blob
>>>> + * @param node ? ? ? node to examine
>>>> + * @param prop_name ?name of property to find
>>>> + * @return 1 if the properly is present; 0 if it isn't present or is 0
>>>> + */
>>>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
>>>
>>> Does U-Boot allow use of the "bool" type here?
>>
>> Which bool type? It is returning an int.
>
> I was asking if the return type could be changed to bool.

Sorry I misunderstood -  no U-Boot does not have a bool type yet.

>
>>>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
>>>> +{
>>>> + ? ? const s32 *cell;
>>>> + ? ? int len;
>>>> +
>>>> + ? ? debug("%s: %s\n", __func__, prop_name);
>>>> + ? ? cell = fdt_getprop(blob, node, prop_name, &len);
>>>> + ? ? if (!cell)
>>>> + ? ? ? ? ? ? return 0;
>>>> + ? ? if (len >= sizeof(u32) && *cell == 0)
>>>> + ? ? ? ? ? ? return 0;
>>>> +
>>>> + ? ? return 1;
>>>> +}
>>>
>>> In the kernel, I believe that property existence is all that's usually
>>> checked. Is that wrong? Did the definition of a boolean property's value
>>> in the function description above come from the specification? If a
>>> property had a length of 0/1/2/3 with a zero value, it seems very odd to
>>> treat that as true.
>>
>> It is useful to be able to set the value to 0 or 1 (with fdtget/put),
>> rather than remove or add the property. A value with a length of less
>> than one cell is considered illegal here.
>>
>> The basic idea is that the presence of the property means that it is
>> 'true'. If it happens to have a value, then we allow that to specify
>> 'false' if it is zero.
>
> Well, it's more up to standard device tree practice, not me. I've
> certainly sent patches that used a property with 0/1 value as a bool
> and received review feedback from DT experts that DT represents bools
> as present/absent properties, both with no value, so I assume zero
> length.

Yes that is the preferred way. However, it is useful to be able to
define a meaning when the property does have a value. The value may
come into the fdt later after compilation. At that point it is easier
to change a value rather than make something present/absent. For one
thing it can be done without adjust the size of the fdt blob.

Perhaps the solution is to adjust fdtput to support boolean values
explicitly (in that it would add or remove the property based on a
command-line 0/1 value).

Still I think it is useful to support a zero value meaning false when
it is provided.

Regards,
Simon

>
> --
> nvpublic
>

Re: [PATCH 05/14] tegra: fdt: Add Tegra2x device tree file

[Simon Glass]

Hi Stephen,

On Fri, Dec 2, 2011 at 7:58 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/01/2011 06:24 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Nov 28, 2011 at 10:56 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>> This was taken from commit 1ea6b8f at:
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git
>>>
> ...
>>> In particular, linux-next now includes a minimal USB binding. Should we
>>> just use this in U-Boot for now? We should get review on the kernel
>>> lists before bringing in this more advanced USB binding in U-Boot, and
>>> perhaps even add the binding into the kernel at the same time?
>>
>> I copied my email to the device-tree mailing list. Hopefully that is
>> enough to get a review there. It feels wrong to send U-Boot patches to
>> the kernel list...?
>
> I think the kernel and U-boot need to co-ordinate on the DT bindings.
> Posting at least to linux-tegra, the Tegra maintainers, and perhaps even
> the kernel's domain-specific list (i.e. linux-usb) seems appropriate to
> me. DT is a cross-functional thing, and really needs cross-functional
> discussion and review. devicetree-discuss will cover the DT experts, but
> probably not the CPU and subsystem experts.

I worry about the implication that I am blazing a trail here. I would
prefer to take the bindings as set down by the kernel and make them
work within U-Boot. The reasoning here is that Linux has more
demanding requirements, and more flexibility, so it is unlikely that
U-Boot would need more features in its fdt. The one exception might be
due to efficiency. If it takes 10ms and 50KB of code to figure out the
system state from the fdt then U-Boot people might get upset, so I do
want to make sure the bindings can be efficiently parsed by U-Boot
(hence my peripheral id approach).

Where those bindings don't exist yet, I would prefer to use a
place-holder until they are set, then change the U-Boot code later.
That process can take many months and we don't want to hold back
actual functionality in U-Boot just because we haven't finalized the
fdt definitions for a particular peripheral.

I will certainly widen my distribution list for fdt patches in the
hope that resolution can be reached then and there, but it will be
hard for kernel people to agree a binding until they have written /
modified the driver. IMO it would probably be a good idea for people
to subscribe to device-tree-discuss if they are interested in fdt
things, kernel or U-Boot or other. LKML already gets a huge amount of
traffic.

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH 05/14] tegra: fdt: Add Tegra2x device tree file

[Simon Glass]

Hi Stephen,

On Fri, Dec 2, 2011 at 7:58 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/01/2011 06:24 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Nov 28, 2011 at 10:56 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>> This was taken from commit 1ea6b8f at:
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/olof/tegra.git
>>>
> ...
>>> In particular, linux-next now includes a minimal USB binding. Should we
>>> just use this in U-Boot for now? We should get review on the kernel
>>> lists before bringing in this more advanced USB binding in U-Boot, and
>>> perhaps even add the binding into the kernel at the same time?
>>
>> I copied my email to the device-tree mailing list. Hopefully that is
>> enough to get a review there. It feels wrong to send U-Boot patches to
>> the kernel list...?
>
> I think the kernel and U-boot need to co-ordinate on the DT bindings.
> Posting at least to linux-tegra, the Tegra maintainers, and perhaps even
> the kernel's domain-specific list (i.e. linux-usb) seems appropriate to
> me. DT is a cross-functional thing, and really needs cross-functional
> discussion and review. devicetree-discuss will cover the DT experts, but
> probably not the CPU and subsystem experts.

I worry about the implication that I am blazing a trail here. I would
prefer to take the bindings as set down by the kernel and make them
work within U-Boot. The reasoning here is that Linux has more
demanding requirements, and more flexibility, so it is unlikely that
U-Boot would need more features in its fdt. The one exception might be
due to efficiency. If it takes 10ms and 50KB of code to figure out the
system state from the fdt then U-Boot people might get upset, so I do
want to make sure the bindings can be efficiently parsed by U-Boot
(hence my peripheral id approach).

Where those bindings don't exist yet, I would prefer to use a
place-holder until they are set, then change the U-Boot code later.
That process can take many months and we don't want to hold back
actual functionality in U-Boot just because we haven't finalized the
fdt definitions for a particular peripheral.

I will certainly widen my distribution list for fdt patches in the
hope that resolution can be reached then and there, but it will be
hard for kernel people to agree a binding until they have written /
modified the driver. IMO it would probably be a good idea for people
to subscribe to device-tree-discuss if they are interested in fdt
things, kernel or U-Boot or other. LKML already gets a huge amount of
traffic.

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Fri, Dec 2, 2011 at 8:10 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/01/2011 06:51 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>> This adds basic support for the Tegra2 USB controller. Board files should
>>>> call board_usb_init() to set things up.
>
>>>> +#ifdef CONFIG_OF_CONTROL
>>>> +int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
>>>> + ? ? ? ? ? ? ? ? ?struct fdt_usb *config)
>>>> +{
>>>> + ? ? ? int clk_node = 0, rate;
>>>> +
>>>> + ? ? ? /* Find the parameters for our oscillator frequency */
>>>> + ? ? ? do {
>>>> + ? ? ? ? ? ? ? clk_node = fdt_node_offset_by_compatible(blob, clk_node,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "nvidia,tegra20-usbparams");
>>>> + ? ? ? ? ? ? ? if (clk_node < 0) {
>>>> + ? ? ? ? ? ? ? ? ? ? ? debug("Cannot find USB params for clock %u",
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? osc_frequency_mhz);
>>>> + ? ? ? ? ? ? ? ? ? ? ? return -FDT_ERR_NOTFOUND;
>>>> + ? ? ? ? ? ? ? }
>>>> + ? ? ? ? ? ? ? rate = fdtdec_get_int(blob, clk_node, "osc-frequency", 0);
>>>> + ? ? ? } while (rate != osc_frequency_mhz);
>>>> +
>>>> + ? ? ? config->reg = (struct usb_ctlr *)fdtdec_get_addr(blob, node, "reg");
>>>> + ? ? ? config->host_mode = fdtdec_get_bool(blob, node, "support-host-mode");
>>>
>>> Property "support-host-mode" isn't something that's supported by the
>>> kernel binding, and needs discussion/ack there.
>>
>> Do you mean device-tree list? I did send the patch there.
>
> linux-tegra and perhaps linux-usb too.

OK

>
>> Does the kernel have another way of knowing that this port is special?
>
> When booted without DT, the platform data defines mode: device, host, OTG.
>
> The current DT bindings for Tegra USB are the minimum required to get
> host mode working, and don't include any representation of this, and so
> host mode is assumed.

OK, I will take a closer look, but may keep this in anticipation of
the kernel catching up.

>
>>>> + ? ? ? config->utmi = fdtdec_lookup_phandle(blob, node, "utmi") >= 0;
>>>
>>> In the kernel DT binding, this is property "phy_type" with legal values
>>> "utmi" or "ulpi."
>>>
>>>> + ? ? ? config->enabled = fdtdec_get_is_enabled(blob, node);
>>>> + ? ? ? config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>>>
>>> periph-id is a U-Boot specific concept, not HW description. The DT
>>> shouldn't contain that value.
>>
>> It is actually the bit position of the peripheral in the clock
>> registers, so arguably a hardware description. U-Boot uses this to
>> efficiently manage peripheral clocks, reset, pinmux, etc.
>>
>> How does the kernel figure out the clock register (etc.) to use with a
>> particular peripheral? Also bear in mind that the intent with U-Boot
>> is to be a lot more lightweight with these things.
>
> The DT binding has to be identical though; U-Boot implementation details
> aren't supposed to affect the content of the DT.
>
> Clock bindings are an area of active development. I haven't been
> following the progress, but I imagine that the clock controller will
> define a node per clock, and the devices that consume the clock will
> refer to that node using a phandle. It's then up to the clock controller
> driver to extract whatever information it needs from the clock node and
> map that to an internal periph-id. It's plausible that a legitimate part
> of the clock binding itself is such a periph-id field, but that should
> be defined by the clock controller binding, not the peripheral binding.

OK, well this is an example of where I would like to run with what we
have, and adjust it later when things are finalized in the kernel.

I'm not sure about your analysis here actually. The peripherals have a
selectable source clock and their own divider from that clock, plus
they have bits for enabling their internal clock and reset. The
registers for all of these can sort-of be indexed through the
peripheral ID. I think with this model you would need to have a
separate clock node for every peripheral, with the peripheral node
pointing back to that. Perhaps that is what you mean. It means that
every peripheral has its own node and then a clock node. It probably
won't be too slow to decode.

>
>>>> + ? ? ? if (config->periph_id == -1)
>>>> + ? ? ? ? ? ? ? return -FDT_ERR_NOTFOUND;
>>>> +
>>>> + ? ? ? return fdtdec_get_int_array(blob, clk_node, "params", config->params,
>>>> + ? ? ? ? ? ? ? ? ? ? ? PARAM_COUNT);
>>>
>>> Property "params" (which should probably be named something better
>>> anyway) isn't something that's supported by the kernel binding, and
>>> needs discussion/ack there. Instead, I suggest following the kernel's
>>> approach for now - don't specify these PHY parameters in the binding,
>>> but rather just apply the defaults, which are apparently suitable for
>>> all the boards supported by the mainline kernel at least.
>>
>> My actual intent was that the device tree would provide options for
>> each oscillator frequency and the board would simply select which it
>> is using. ?This does not fit well with out the device tree works
>> though - we end up having everything in there and available at
>> run-time, even useless data. Anyway, the oscillator frequency is
>> detected at run-time, so I decided to put these into the SOC
>> description.
>>
>> Since you say these values are fixed for all boards we might as well
>> put them back into the code.
>
> The values aren't necessarily fixed for /all/ boards, they just /happen/
> to be identical for all boards currently supported in the mainline
> kernel. I imagine that as more boards are supported, we'll see different
> sets of values in use.

OK so perhaps I should keep my bindings here...

>
>>>> +int board_usb_init(const void *blob)
>>>> +{
>>>> +#ifdef CONFIG_OF_CONTROL
>>>> + ? ? ? struct fdt_usb config;
>>>> + ? ? ? int clk_done = 0;
>>>> + ? ? ? int node, upto = 0;
>>>> + ? ? ? unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>>>> +
>>>> + ? ? ? do {
>>>> + ? ? ? ? ? ? ? node = fdtdec_next_alias(blob, "usb",
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?COMPAT_NVIDIA_TEGRA20_USB, &upto);
>>>
>>> Why only initialize USB controllers with aliases? Surely this should
>>> enumerate all nodes with a specific compatible flag?
>>
>> The aliases are (I thought) the official way that device trees specify
>> device ordering. No we do not enumerate things in U-Boot - there is no
>> device model as such. We can do this on Tegra, but still need to know
>> the order to use (i.e. which is port 0).
>
> I don't believe the kernel uses the alias for anything at all right now.
> Instead, it enumerates all nodes that match a certain compatible flag,
> and instantiates a device for each one it has a driver for. I believe
> this mode of operation is pretty implicit in DT itself; it's something
> U-Boot should do too.

It does this at present with USB. But we want to enumerate the ports
and know which is port 0, which is port 1, etc. How does the kernel do
that?

>
> If U-boot were to operate solely based on the aliases node, it wouldn't
> work at all for the .dts files currently in the kernel since there are
> no aliases, and if there were aliases, might end up instantiating a
> subset of devices if some devices weren't mentioned in aliases.
>
> I believe what aliases is meant for is that once you've enumerated all
> the devices, then check the aliases node in order to override the
> default naming (or set up alternative names). Still, you'd have to check
> with a DT expert here, since I've never done anything with the aliases node.

I did go around this loop some time ago and my understanding was that
aliases were the correct way to enumerate ports (I originally just had
an ID in the port which is easier to decode).

Regards,
Simon

>
> --
> nvpublic
>

Re: [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Jerry Van Baren]

On 12/01/2011 11:58 PM, Simon Glass wrote:
> Hi Jerry,
>
> On Thu, Dec 1, 2011 at 7:33 PM, Jerry Van Baren<gvb.uboot@gmail.com>  wrote:

[snip]

>> FDT helper functions have been accumulating in common/fdt_support.c rather
>> than a separate file.  Simon, what is the history of lib/fdtdec.c?  Is it a
>> shared file from the linux kernel?  If it is u-boot specific, it would
>> probably be better to add the functions to fdt_support.c.
>
> There are sort-of two FDT strands within U-Boot. The main one is
> support for putting together an FDT to pass to the kernel (cmd_fdt,
> fdt_support), and the other is for U-Boot's own use (run-time
> configuration of U-Boot, fdtdec). They both use libfdt.
>
> I regard fdt_support as part of the former, and fdtdec (decode-only
> helper functions) as part of the latter. At present when you turn on
> CONFIG_OF_LIBFDT you get everything, but we could provide finer
> granularity for platforms which only want to decode an FDT for
> run-time config and don't want to mess with it. Since fdt_decode is
> about 6KB of code that might be useful.

Ahh, I see.  I have not been closely tracking the u-boot config (fdtdec) 
improvements, so I did not recognize it as being part of that effort. 
That makes sense.

Thanks,
gvb

[snip]

[U-Boot] [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Jerry Van Baren]

On 12/01/2011 11:58 PM, Simon Glass wrote:
> Hi Jerry,
>
> On Thu, Dec 1, 2011 at 7:33 PM, Jerry Van Baren<gvb.uboot@gmail.com>  wrote:

[snip]

>> FDT helper functions have been accumulating in common/fdt_support.c rather
>> than a separate file.  Simon, what is the history of lib/fdtdec.c?  Is it a
>> shared file from the linux kernel?  If it is u-boot specific, it would
>> probably be better to add the functions to fdt_support.c.
>
> There are sort-of two FDT strands within U-Boot. The main one is
> support for putting together an FDT to pass to the kernel (cmd_fdt,
> fdt_support), and the other is for U-Boot's own use (run-time
> configuration of U-Boot, fdtdec). They both use libfdt.
>
> I regard fdt_support as part of the former, and fdtdec (decode-only
> helper functions) as part of the latter. At present when you turn on
> CONFIG_OF_LIBFDT you get everything, but we could provide finer
> granularity for platforms which only want to decode an FDT for
> run-time config and don't want to mess with it. Since fdt_decode is
> about 6KB of code that might be useful.

Ahh, I see.  I have not been closely tracking the u-boot config (fdtdec) 
improvements, so I did not recognize it as being part of that effort. 
That makes sense.

Thanks,
gvb

[snip]

Re: [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Simon Glass]

Hi Jerry,

On Fri, Dec 2, 2011 at 9:22 AM, Jerry Van Baren <gvb.uboot@gmail.com> wrote:
> On 12/01/2011 11:58 PM, Simon Glass wrote:
>>
>> Hi Jerry,
>>
>> On Thu, Dec 1, 2011 at 7:33 PM, Jerry Van Baren<gvb.uboot@gmail.com>
>>  wrote:
>
> [snip]
>
>>> FDT helper functions have been accumulating in common/fdt_support.c
>>> rather
>>> than a separate file.  Simon, what is the history of lib/fdtdec.c?  Is it
>>> a
>>> shared file from the linux kernel?  If it is u-boot specific, it would
>>> probably be better to add the functions to fdt_support.c.
>>
>> There are sort-of two FDT strands within U-Boot. The main one is
>> support for putting together an FDT to pass to the kernel (cmd_fdt,
>> fdt_support), and the other is for U-Boot's own use (run-time
>> configuration of U-Boot, fdtdec). They both use libfdt.
>>
>> I regard fdt_support as part of the former, and fdtdec (decode-only
>> helper functions) as part of the latter. At present when you turn on
>> CONFIG_OF_LIBFDT you get everything, but we could provide finer
>> granularity for platforms which only want to decode an FDT for
>> run-time config and don't want to mess with it. Since fdt_decode is
>> about 6KB of code that might be useful.
>
> Ahh, I see.  I have not been closely tracking the u-boot config (fdtdec)
> improvements, so I did not recognize it as being part of that effort. That
> makes sense.

You are welcome, thanks.

Regards
Simon

>
> Thanks,
> gvb
>
> [snip]
>

[U-Boot] [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Simon Glass]

Hi Jerry,

On Fri, Dec 2, 2011 at 9:22 AM, Jerry Van Baren <gvb.uboot@gmail.com> wrote:
> On 12/01/2011 11:58 PM, Simon Glass wrote:
>>
>> Hi Jerry,
>>
>> On Thu, Dec 1, 2011 at 7:33 PM, Jerry Van Baren<gvb.uboot@gmail.com>
>> ?wrote:
>
> [snip]
>
>>> FDT helper functions have been accumulating in common/fdt_support.c
>>> rather
>>> than a separate file. ?Simon, what is the history of lib/fdtdec.c? ?Is it
>>> a
>>> shared file from the linux kernel? ?If it is u-boot specific, it would
>>> probably be better to add the functions to fdt_support.c.
>>
>> There are sort-of two FDT strands within U-Boot. The main one is
>> support for putting together an FDT to pass to the kernel (cmd_fdt,
>> fdt_support), and the other is for U-Boot's own use (run-time
>> configuration of U-Boot, fdtdec). They both use libfdt.
>>
>> I regard fdt_support as part of the former, and fdtdec (decode-only
>> helper functions) as part of the latter. At present when you turn on
>> CONFIG_OF_LIBFDT you get everything, but we could provide finer
>> granularity for platforms which only want to decode an FDT for
>> run-time config and don't want to mess with it. Since fdt_decode is
>> about 6KB of code that might be useful.
>
> Ahh, I see. ?I have not been closely tracking the u-boot config (fdtdec)
> improvements, so I did not recognize it as being part of that effort. That
> makes sense.

You are welcome, thanks.

Regards
Simon

>
> Thanks,
> gvb
>
> [snip]
>

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren]

On 12/02/2011 10:00 AM, Simon Glass wrote:
> On Fri, Dec 2, 2011 at 8:10 AM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/01/2011 06:51 PM, Simon Glass wrote:
>>> On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>>> This adds basic support for the Tegra2 USB controller. Board files should
>>>>> call board_usb_init() to set things up.
...
>>>>> +       config->enabled = fdtdec_get_is_enabled(blob, node);
>>>>> +       config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>>>>
>>>> periph-id is a U-Boot specific concept, not HW description. The DT
>>>> shouldn't contain that value.
>>>
>>> It is actually the bit position of the peripheral in the clock
>>> registers, so arguably a hardware description. U-Boot uses this to
>>> efficiently manage peripheral clocks, reset, pinmux, etc.
>>>
>>> How does the kernel figure out the clock register (etc.) to use with a
>>> particular peripheral? Also bear in mind that the intent with U-Boot
>>> is to be a lot more lightweight with these things.
>>
>> The DT binding has to be identical though; U-Boot implementation details
>> aren't supposed to affect the content of the DT.
>>
>> Clock bindings are an area of active development. I haven't been
>> following the progress, but I imagine that the clock controller will
>> define a node per clock, and the devices that consume the clock will
>> refer to that node using a phandle. It's then up to the clock controller
>> driver to extract whatever information it needs from the clock node and
>> map that to an internal periph-id. It's plausible that a legitimate part
>> of the clock binding itself is such a periph-id field, but that should
>> be defined by the clock controller binding, not the peripheral binding.
> 
> OK, well this is an example of where I would like to run with what we
> have, and adjust it later when things are finalized in the kernel.
> 
> I'm not sure about your analysis here actually. The peripherals have a
> selectable source clock and their own divider from that clock, plus
> they have bits for enabling their internal clock and reset. The
> registers for all of these can sort-of be indexed through the
> peripheral ID. I think with this model you would need to have a
> separate clock node for every peripheral, with the peripheral node
> pointing back to that. Perhaps that is what you mean. It means that
> every peripheral has its own node and then a clock node. It probably
> won't be too slow to decode.

re: the last-but-one sentence: Yes, I think that's how it'll work.

>>>>> +int board_usb_init(const void *blob)
>>>>> +{
>>>>> +#ifdef CONFIG_OF_CONTROL
>>>>> +       struct fdt_usb config;
>>>>> +       int clk_done = 0;
>>>>> +       int node, upto = 0;
>>>>> +       unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>>>>> +
>>>>> +       do {
>>>>> +               node = fdtdec_next_alias(blob, "usb",
>>>>> +                                        COMPAT_NVIDIA_TEGRA20_USB, &upto);
>>>>
>>>> Why only initialize USB controllers with aliases? Surely this should
>>>> enumerate all nodes with a specific compatible flag?
>>>
>>> The aliases are (I thought) the official way that device trees specify
>>> device ordering. No we do not enumerate things in U-Boot - there is no
>>> device model as such. We can do this on Tegra, but still need to know
>>> the order to use (i.e. which is port 0).
>>
>> I don't believe the kernel uses the alias for anything at all right now.
>> Instead, it enumerates all nodes that match a certain compatible flag,
>> and instantiates a device for each one it has a driver for. I believe
>> this mode of operation is pretty implicit in DT itself; it's something
>> U-Boot should do too.
> 
> It does this at present with USB. But we want to enumerate the ports
> and know which is port 0, which is port 1, etc. How does the kernel do
> that?

I don't think it cares; it just hosts a number of USB ports, and
peripherals show up on those USB ports. The numbering of the ports is
entirely arbitrary AFAIK.

-- 
nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Fri, Dec 2, 2011 at 12:40 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/02/2011 10:00 AM, Simon Glass wrote:
>> On Fri, Dec 2, 2011 at 8:10 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/01/2011 06:51 PM, Simon Glass wrote:
>>>> On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>>>> This adds basic support for the Tegra2 USB controller. Board files should
>>>>>> call board_usb_init() to set things up.
> ...
>>>>>> + ? ? ? config->enabled = fdtdec_get_is_enabled(blob, node);
>>>>>> + ? ? ? config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>>>>>
>>>>> periph-id is a U-Boot specific concept, not HW description. The DT
>>>>> shouldn't contain that value.
>>>>
>>>> It is actually the bit position of the peripheral in the clock
>>>> registers, so arguably a hardware description. U-Boot uses this to
>>>> efficiently manage peripheral clocks, reset, pinmux, etc.
>>>>
>>>> How does the kernel figure out the clock register (etc.) to use with a
>>>> particular peripheral? Also bear in mind that the intent with U-Boot
>>>> is to be a lot more lightweight with these things.
>>>
>>> The DT binding has to be identical though; U-Boot implementation details
>>> aren't supposed to affect the content of the DT.
>>>
>>> Clock bindings are an area of active development. I haven't been
>>> following the progress, but I imagine that the clock controller will
>>> define a node per clock, and the devices that consume the clock will
>>> refer to that node using a phandle. It's then up to the clock controller
>>> driver to extract whatever information it needs from the clock node and
>>> map that to an internal periph-id. It's plausible that a legitimate part
>>> of the clock binding itself is such a periph-id field, but that should
>>> be defined by the clock controller binding, not the peripheral binding.
>>
>> OK, well this is an example of where I would like to run with what we
>> have, and adjust it later when things are finalized in the kernel.
>>
>> I'm not sure about your analysis here actually. The peripherals have a
>> selectable source clock and their own divider from that clock, plus
>> they have bits for enabling their internal clock and reset. The
>> registers for all of these can sort-of be indexed through the
>> peripheral ID. I think with this model you would need to have a
>> separate clock node for every peripheral, with the peripheral node
>> pointing back to that. Perhaps that is what you mean. It means that
>> every peripheral has its own node and then a clock node. It probably
>> won't be too slow to decode.
>
> re: the last-but-one sentence: Yes, I think that's how it'll work.
>
>>>>>> +int board_usb_init(const void *blob)
>>>>>> +{
>>>>>> +#ifdef CONFIG_OF_CONTROL
>>>>>> + ? ? ? struct fdt_usb config;
>>>>>> + ? ? ? int clk_done = 0;
>>>>>> + ? ? ? int node, upto = 0;
>>>>>> + ? ? ? unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>>>>>> +
>>>>>> + ? ? ? do {
>>>>>> + ? ? ? ? ? ? ? node = fdtdec_next_alias(blob, "usb",
>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?COMPAT_NVIDIA_TEGRA20_USB, &upto);
>>>>>
>>>>> Why only initialize USB controllers with aliases? Surely this should
>>>>> enumerate all nodes with a specific compatible flag?
>>>>
>>>> The aliases are (I thought) the official way that device trees specify
>>>> device ordering. No we do not enumerate things in U-Boot - there is no
>>>> device model as such. We can do this on Tegra, but still need to know
>>>> the order to use (i.e. which is port 0).
>>>
>>> I don't believe the kernel uses the alias for anything at all right now.
>>> Instead, it enumerates all nodes that match a certain compatible flag,
>>> and instantiates a device for each one it has a driver for. I believe
>>> this mode of operation is pretty implicit in DT itself; it's something
>>> U-Boot should do too.
>>
>> It does this at present with USB. But we want to enumerate the ports
>> and know which is port 0, which is port 1, etc. How does the kernel do
>> that?
>
> I don't think it cares; it just hosts a number of USB ports, and
> peripherals show up on those USB ports. The numbering of the ports is
> entirely arbitrary AFAIK.

OK. For the moment in U-Boot we do care, so I will leave the alias
solution in there for now. I may look later at the patch to suppose a
virtual hub on Tegra.

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

Here are my comments on the rest of your email.

On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> This adds basic support for the Tegra2 USB controller. Board files should
>> call board_usb_init() to set things up.
>
> Just a very brief review:
>
>> +/* Put the port into host mode (this only works for USB1) */
>> +static void set_host_mode(struct usb_ctlr *usbctlr)
>> +{
>> + ? ? ? /* Check whether remote host from USB1 is driving VBus */
>> + ? ? ? if (readl(&usbctlr->phy_vbus_sensors) & VBUS_VLD_STS)
>> + ? ? ? ? ? ? ? return;
>> +
>> + ? ? ? /*
>> + ? ? ? ?* If not driving, we set GPIO USB1_VBus_En. Seaboard platform uses
>> + ? ? ? ?* PAD SLXK (GPIO D.00) as USB1_VBus_En Config as GPIO
>> + ? ? ? ?*/
>> + ? ? ? gpio_direction_output(GPIO_PD0, 1);
>
> The GPIO to use here needs to be parameterized; there's no reason to
> believe it'll be the same on all boards (or even that boards will have
> any such GPIO).

I have done this. It is tricky since so far we have no way of setting
up the pinmux for a given GPIO. Rather than bite off that camel I have
just set up the pinmux in the board file.

>
>> +/* Put our ports into host mode */
>> +void usb_set_host_mode(void)
>> +{
>> + ? ? ? if (host_dev_ctlr)
>> + ? ? ? ? ? ? ? set_host_mode(host_dev_ctlr);
>> +}
>
> Why not just call set_host_mode() directly from board_usb_init()?

This function is exported, and the caller doesn't have a pointer to
the host/device-switchable port.

>
>> +#ifndef CONFIG_OF_CONTROL
>> +static int probe_port(struct usb_ctlr *usbctlr, const int params[])
>> +{
>> + ? ? ? enum periph_id id;
>> + ? ? ? int utmi = 0;
>> +
>> + ? ? ? /*
>> + ? ? ? ?* Get the periph id. Port 1 has an internal transceiver, port 3 is
>> + ? ? ? ?* external
>> + ? ? ? ?*/
>> + ? ? ? switch ((u32)usbctlr) {
>> + ? ? ? case TEGRA_USB1_BASE:
>> + ? ? ? ? ? ? ? id = PERIPH_ID_USBD;
>> + ? ? ? ? ? ? ? break;
>> +
>> + ? ? ? case TEGRA_USB3_BASE:
>> + ? ? ? ? ? ? ? id = PERIPH_ID_USB3;
>> + ? ? ? ? ? ? ? utmi = 1;
>> + ? ? ? ? ? ? ? break;
>> +
>> + ? ? ? default:
>> + ? ? ? ? ? ? ? printf("tegrausb: probe_port: no such port %p\n", usbctlr);
>> + ? ? ? ? ? ? ? return -1;
>> + ? ? ? }
>
> What about the other port (USB2)?

I have removed this function.

>
>> +#ifdef CONFIG_OF_CONTROL
>> +int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
>> + ? ? ? ? ? ? ? ? ?struct fdt_usb *config)
>> +{
>> + ? ? ? int clk_node = 0, rate;
>> +
>> + ? ? ? /* Find the parameters for our oscillator frequency */
>> + ? ? ? do {
>> + ? ? ? ? ? ? ? clk_node = fdt_node_offset_by_compatible(blob, clk_node,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "nvidia,tegra20-usbparams");
>> + ? ? ? ? ? ? ? if (clk_node < 0) {
>> + ? ? ? ? ? ? ? ? ? ? ? debug("Cannot find USB params for clock %u",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? osc_frequency_mhz);
>> + ? ? ? ? ? ? ? ? ? ? ? return -FDT_ERR_NOTFOUND;
>> + ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? rate = fdtdec_get_int(blob, clk_node, "osc-frequency", 0);
>> + ? ? ? } while (rate != osc_frequency_mhz);
>> +
>> + ? ? ? config->reg = (struct usb_ctlr *)fdtdec_get_addr(blob, node, "reg");
>> + ? ? ? config->host_mode = fdtdec_get_bool(blob, node, "support-host-mode");
>
> Property "support-host-mode" isn't something that's supported by the
> kernel binding, and needs discussion/ack there.

OK will await comments. The kernel bindings seem very basic at this stage.

>
>> + ? ? ? config->utmi = fdtdec_lookup_phandle(blob, node, "utmi") >= 0;
>
> In the kernel DT binding, this is property "phy_type" with legal values
> "utmi" or "ulpi."

OK I have changed this.

>
>> + ? ? ? config->enabled = fdtdec_get_is_enabled(blob, node);
>> + ? ? ? config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>
> periph-id is a U-Boot specific concept, not HW description. The DT
> shouldn't contain that value.

See my previous comments as to why this is desirable. We can change
over to a clock-based approach once the kernel implements it.

>
>> + ? ? ? if (config->periph_id == -1)
>> + ? ? ? ? ? ? ? return -FDT_ERR_NOTFOUND;
>> +
>> + ? ? ? return fdtdec_get_int_array(blob, clk_node, "params", config->params,
>> + ? ? ? ? ? ? ? ? ? ? ? PARAM_COUNT);
>
> Property "params" (which should probably be named something better
> anyway) isn't something that's supported by the kernel binding, and
> needs discussion/ack there. Instead, I suggest following the kernel's
> approach for now - don't specify these PHY parameters in the binding,
> but rather just apply the defaults, which are apparently suitable for
> all the boards supported by the mainline kernel at least.

As discussed I have changed the name but otherwise left this as is.
since you mentioned that some boards may need to change the parameters
(it seems Tegra3 is different also).

>
>
>> +int board_usb_init(const void *blob)
>> +{
>> +#ifdef CONFIG_OF_CONTROL
>> + ? ? ? struct fdt_usb config;
>> + ? ? ? int clk_done = 0;
>> + ? ? ? int node, upto = 0;
>> + ? ? ? unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>> +
>> + ? ? ? do {
>> + ? ? ? ? ? ? ? node = fdtdec_next_alias(blob, "usb",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?COMPAT_NVIDIA_TEGRA20_USB, &upto);
>
> Why only initialize USB controllers with aliases? Surely this should
> enumerate all nodes with a specific compatible flag?

See my other comments - we want to know that port 0 is USB3 on Seaboard.

>
>> + ? ? ? ? ? ? ? /* The first port we find gets to set the clocks */
>
> Ick.

Well they all uses the same params anyway. It could be refactored to
read the timing separately but it is convenient to have it in one
structure.

>
>> + ? ? ? /* Set up our two ports */
>> +#ifdef CONFIG_TEGRA_USB1_HOST
>> + ? ? ? host_dev_ctlr = (struct usb_ctlr *)TEGRA_USB1_BASE;
>> +#endif
>
> That port is always the host/device switchable controller. Why not
> always make that assignment? The issue here is probably that
> set_host_mode() isn't suitable for all boards. The solution seems to be
> to fix that.

I have removed this code.

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren]

On 12/02/2011 05:59 PM, Simon Glass wrote:
> Hi Stephen,
> 
> Here are my comments on the rest of your email.
> 
> On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>> This adds basic support for the Tegra2 USB controller. Board files should
>>> call board_usb_init() to set things up.

>>> +       config->enabled = fdtdec_get_is_enabled(blob, node);
>>> +       config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>>
>> periph-id is a U-Boot specific concept, not HW description. The DT
>> shouldn't contain that value.
> 
> See my previous comments as to why this is desirable. We can change
> over to a clock-based approach once the kernel implements it.

That will cause backwards-compatibility problems; older FDTs won't work
with newer U-Boots. We should avoid incompatible changes like this if at
all possible.

>>> +int board_usb_init(const void *blob)
>>> +{
>>> +#ifdef CONFIG_OF_CONTROL
>>> +       struct fdt_usb config;
>>> +       int clk_done = 0;
>>> +       int node, upto = 0;
>>> +       unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>>> +
>>> +       do {
>>> +               node = fdtdec_next_alias(blob, "usb",
>>> +                                        COMPAT_NVIDIA_TEGRA20_USB, &upto);
>>
>> Why only initialize USB controllers with aliases? Surely this should
>> enumerate all nodes with a specific compatible flag?
> 
> See my other comments - we want to know that port 0 is USB3 on Seaboard.

Why does U-Boot care? Everything should be enumerating which peripherals
happen to appear on the various USB busses, and not care which host
controller they're attached to.

(With the exception of USB port 0 being device-capable, but that should
be configured by an explicit property, not based on aliases or anything
like that)

-- 
nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 1:33 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/02/2011 05:59 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> Here are my comments on the rest of your email.
>>
>> On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>> This adds basic support for the Tegra2 USB controller. Board files should
>>>> call board_usb_init() to set things up.
>
>>>> + ? ? ? config->enabled = fdtdec_get_is_enabled(blob, node);
>>>> + ? ? ? config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>>>
>>> periph-id is a U-Boot specific concept, not HW description. The DT
>>> shouldn't contain that value.
>>
>> See my previous comments as to why this is desirable. We can change
>> over to a clock-based approach once the kernel implements it.
>
> That will cause backwards-compatibility problems; older FDTs won't work
> with newer U-Boots. We should avoid incompatible changes like this if at
> all possible.

At the moment the fdts are in the U-Boot tree, so we should be able to
change them at the same time. But of course when we update the fdt  we
need to update the U-Boot code. Is there any alternative other than
doing nothing until the kernel decides everything?

>
>>>> +int board_usb_init(const void *blob)
>>>> +{
>>>> +#ifdef CONFIG_OF_CONTROL
>>>> + ? ? ? struct fdt_usb config;
>>>> + ? ? ? int clk_done = 0;
>>>> + ? ? ? int node, upto = 0;
>>>> + ? ? ? unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>>>> +
>>>> + ? ? ? do {
>>>> + ? ? ? ? ? ? ? node = fdtdec_next_alias(blob, "usb",
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?COMPAT_NVIDIA_TEGRA20_USB, &upto);
>>>
>>> Why only initialize USB controllers with aliases? Surely this should
>>> enumerate all nodes with a specific compatible flag?
>>
>> See my other comments - we want to know that port 0 is USB3 on Seaboard.
>
> Why does U-Boot care? Everything should be enumerating which peripherals
> happen to appear on the various USB busses, and not care which host
> controller they're attached to.

At present we do:

'usb start 0'
'usb start 1'

to select between the ports. There is a patch in the works to change
that but it hasn't gone upstream yet, or at least wasn't accepted.

There is a problem with adopting that approach in general - e.g.
*which* UART you write to has a big effect on whether the user sees
any output.

>
> (With the exception of USB port 0 being device-capable, but that should
> be configured by an explicit property, not based on aliases or anything
> like that)

Yes it is - in fact I added that property.

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren]

On 12/05/2011 02:46 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Dec 5, 2011 at 1:33 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/02/2011 05:59 PM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> Here are my comments on the rest of your email.
>>>
>>> On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>>> This adds basic support for the Tegra2 USB controller. Board files should
>>>>> call board_usb_init() to set things up.
>>
>>>>> +       config->enabled = fdtdec_get_is_enabled(blob, node);
>>>>> +       config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>>>>
>>>> periph-id is a U-Boot specific concept, not HW description. The DT
>>>> shouldn't contain that value.
>>>
>>> See my previous comments as to why this is desirable. We can change
>>> over to a clock-based approach once the kernel implements it.
>>
>> That will cause backwards-compatibility problems; older FDTs won't work
>> with newer U-Boots. We should avoid incompatible changes like this if at
>> all possible.
> 
> At the moment the fdts are in the U-Boot tree, so we should be able to
> change them at the same time. But of course when we update the fdt  we
> need to update the U-Boot code. Is there any alternative other than
> doing nothing until the kernel decides everything?

You can choose not to represent these parameters in the device tree at
all, but rather hard-code the values in the driver. This is what the
kernel currently does; as luck would have it, the required values are
identical for all the boards the kernel currently supports. Once that's
all in place and working, we can work on defining a binding for those
parameters and review/implement it in both U-Boot and the kernel.

>>>>> +int board_usb_init(const void *blob)
>>>>> +{
>>>>> +#ifdef CONFIG_OF_CONTROL
>>>>> +       struct fdt_usb config;
>>>>> +       int clk_done = 0;
>>>>> +       int node, upto = 0;
>>>>> +       unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>>>>> +
>>>>> +       do {
>>>>> +               node = fdtdec_next_alias(blob, "usb",
>>>>> +                                        COMPAT_NVIDIA_TEGRA20_USB, &upto);
>>>>
>>>> Why only initialize USB controllers with aliases? Surely this should
>>>> enumerate all nodes with a specific compatible flag?
>>>
>>> See my other comments - we want to know that port 0 is USB3 on Seaboard.
>>
>> Why does U-Boot care? Everything should be enumerating which peripherals
>> happen to appear on the various USB busses, and not care which host
>> controller they're attached to.
> 
> At present we do:
> 
> 'usb start 0'
> 'usb start 1'
> 
> to select between the ports. There is a patch in the works to change
> that but it hasn't gone upstream yet, or at least wasn't accepted.

Can you point out the patch that changes this, and what exactly it
changes. Hopefully it removes the parameter from the "usb start" command.

I'm still not convinced why U-Boot cares about this (as opposed to the
user using U-Boot).

-- 
nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 2:15 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/05/2011 02:46 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Dec 5, 2011 at 1:33 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/02/2011 05:59 PM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> Here are my comments on the rest of your email.
>>>>
>>>> On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>>>> This adds basic support for the Tegra2 USB controller. Board files should
>>>>>> call board_usb_init() to set things up.
>>>
>>>>>> + ? ? ? config->enabled = fdtdec_get_is_enabled(blob, node);
>>>>>> + ? ? ? config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>>>>>
>>>>> periph-id is a U-Boot specific concept, not HW description. The DT
>>>>> shouldn't contain that value.
>>>>
>>>> See my previous comments as to why this is desirable. We can change
>>>> over to a clock-based approach once the kernel implements it.
>>>
>>> That will cause backwards-compatibility problems; older FDTs won't work
>>> with newer U-Boots. We should avoid incompatible changes like this if at
>>> all possible.
>>
>> At the moment the fdts are in the U-Boot tree, so we should be able to
>> change them at the same time. But of course when we update the fdt ?we
>> need to update the U-Boot code. Is there any alternative other than
>> doing nothing until the kernel decides everything?
>
> You can choose not to represent these parameters in the device tree at
> all, but rather hard-code the values in the driver. This is what the
> kernel currently does; as luck would have it, the required values are
> identical for all the boards the kernel currently supports. Once that's
> all in place and working, we can work on defining a binding for those
> parameters and review/implement it in both U-Boot and the kernel.

OK, but how about we have this conversation when things are actually
in the kernel and working? The scheme used in this series (peripheral
IDs) is very handy for U-Boot and avoids this hard-coding that you
refer to.

Also the only way I can see it being hard-coded is by the kernel
looking at the peripheral address and converting this to an ID. That
seems really ugly to me. Or am I missing something?

>
>>>>>> +int board_usb_init(const void *blob)
>>>>>> +{
>>>>>> +#ifdef CONFIG_OF_CONTROL
>>>>>> + ? ? ? struct fdt_usb config;
>>>>>> + ? ? ? int clk_done = 0;
>>>>>> + ? ? ? int node, upto = 0;
>>>>>> + ? ? ? unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>>>>>> +
>>>>>> + ? ? ? do {
>>>>>> + ? ? ? ? ? ? ? node = fdtdec_next_alias(blob, "usb",
>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?COMPAT_NVIDIA_TEGRA20_USB, &upto);
>>>>>
>>>>> Why only initialize USB controllers with aliases? Surely this should
>>>>> enumerate all nodes with a specific compatible flag?
>>>>
>>>> See my other comments - we want to know that port 0 is USB3 on Seaboard.
>>>
>>> Why does U-Boot care? Everything should be enumerating which peripherals
>>> happen to appear on the various USB busses, and not care which host
>>> controller they're attached to.
>>
>> At present we do:
>>
>> 'usb start 0'
>> 'usb start 1'
>>
>> to select between the ports. There is a patch in the works to change
>> that but it hasn't gone upstream yet, or at least wasn't accepted.
>
> Can you point out the patch that changes this, and what exactly it
> changes. Hopefully it removes the parameter from the "usb start" command.

Yes, they are in the Chromium tree:

https://gerrit.chromium.org/gerrit/#change,4951
https://gerrit.chromium.org/gerrit/#change,4981

>
> I'm still not convinced why U-Boot cares about this (as opposed to the
> user using U-Boot).

Well if U-Boot presents the ports in the wrong order, the user's
scripts will fail.

Also, what about the console UART problem? Surely the kernel provides
a way to select the ordering of those? How do I know what UART I am
getting on /dev/ttyS0?

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren]

On 12/05/2011 04:35 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Dec 5, 2011 at 2:15 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/05/2011 02:46 PM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On Mon, Dec 5, 2011 at 1:33 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>> On 12/02/2011 05:59 PM, Simon Glass wrote:
>>>>> Hi Stephen,
>>>>>
>>>>> Here are my comments on the rest of your email.
>>>>>
>>>>> On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>>>>> This adds basic support for the Tegra2 USB controller. Board files should
>>>>>>> call board_usb_init() to set things up.
>>>>
>>>>>>> +       config->enabled = fdtdec_get_is_enabled(blob, node);
>>>>>>> +       config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>>>>>>
>>>>>> periph-id is a U-Boot specific concept, not HW description. The DT
>>>>>> shouldn't contain that value.
>>>>>
>>>>> See my previous comments as to why this is desirable. We can change
>>>>> over to a clock-based approach once the kernel implements it.
>>>>
>>>> That will cause backwards-compatibility problems; older FDTs won't work
>>>> with newer U-Boots. We should avoid incompatible changes like this if at
>>>> all possible.
>>>
>>> At the moment the fdts are in the U-Boot tree, so we should be able to
>>> change them at the same time. But of course when we update the fdt  we
>>> need to update the U-Boot code. Is there any alternative other than
>>> doing nothing until the kernel decides everything?
>>
>> You can choose not to represent these parameters in the device tree at
>> all, but rather hard-code the values in the driver. This is what the
>> kernel currently does; as luck would have it, the required values are
>> identical for all the boards the kernel currently supports. Once that's
>> all in place and working, we can work on defining a binding for those
>> parameters and review/implement it in both U-Boot and the kernel.
> 
> OK, but how about we have this conversation when things are actually
> in the kernel and working? The scheme used in this series (peripheral
> IDs) is very handy for U-Boot and avoids this hard-coding that you
> refer to.

I'd really like to avoid adding stuff to the .dts file when we know we
going to rip it out again ASAP. I'd prefer to incrementally enhance the
.dts bindings always moving forward, and never removing/breaking stuff
if possible.

Now you did point out that the .dts files are currently in U-Boot, so
this is slightly moot since the binding documentation, .dts file and
driver can all be rev'd in sync. However, I hope this will change soon.
Otherwise, every addition to them means changing U-Boot, the Linux
kernel, U-Boot v2, fastboot, quick boot, .....

> Also the only way I can see it being hard-coded is by the kernel
> looking at the peripheral address and converting this to an ID. That
> seems really ugly to me. Or am I missing something?

Well, the Tegra SD/MMC driver works exactly that way (well, mapping an
instance ID to both base address and periph ID), so there's certainly
precedent for this. And that driver is not unique.

>>>>>>> +int board_usb_init(const void *blob)
>>>>>>> +{
>>>>>>> +#ifdef CONFIG_OF_CONTROL
>>>>>>> +       struct fdt_usb config;
>>>>>>> +       int clk_done = 0;
>>>>>>> +       int node, upto = 0;
>>>>>>> +       unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>>>>>>> +
>>>>>>> +       do {
>>>>>>> +               node = fdtdec_next_alias(blob, "usb",
>>>>>>> +                                        COMPAT_NVIDIA_TEGRA20_USB, &upto);
>>>>>>
>>>>>> Why only initialize USB controllers with aliases? Surely this should
>>>>>> enumerate all nodes with a specific compatible flag?
>>>>>
>>>>> See my other comments - we want to know that port 0 is USB3 on Seaboard.
>>>>
>>>> Why does U-Boot care? Everything should be enumerating which peripherals
>>>> happen to appear on the various USB busses, and not care which host
>>>> controller they're attached to.
>>>
>>> At present we do:
>>>
>>> 'usb start 0'
>>> 'usb start 1'
>>>
>>> to select between the ports. There is a patch in the works to change
>>> that but it hasn't gone upstream yet, or at least wasn't accepted.

Judging by the USB driver, there's actually only "usb start" with no
parameter, and ehc-tegra.c:ehci_hcd_init() hard-codes this as starting
port 0. That's a little more severe. Anyway, that implies to me that
tegra-seaboard.dts should set status = "disabled" on all but one port,
because the others will be useless anyway.

>> Can you point out the patch that changes this, and what exactly it
>> changes. Hopefully it removes the parameter from the "usb start" command.
> 
> Yes, they are in the Chromium tree:
> 
> https://gerrit.chromium.org/gerrit/#change,4951
> https://gerrit.chromium.org/gerrit/#change,4981

OK, those look interesting, at least from the commit descriptions. I'd
assert they really should be upstreamed before or as part of the Tegra
USB driver addition, since it makes the whole /aliases thing completely
irrelevant for USB.

>> I'm still not convinced why U-Boot cares about this (as opposed to the
>> user using U-Boot).
> 
> Well if U-Boot presents the ports in the wrong order, the user's
> scripts will fail.
> 
> Also, what about the console UART problem? Surely the kernel provides
> a way to select the ordering of those? How do I know what UART I am
> getting on /dev/ttyS0?

That's a question my subconscious has been asking me for a while to. The
answer is that it's all very accidental...

The kernel serial driver needs a clock-frequency property. If it isn't
present, the driver won't initialize. The .dts files in the kernel only
have this property for serial ports that are hooked up on the given
board. In the case (Paz00 a/k/a Toshiba AC100) where multiple serial
ports have this property, the useful one just happens to be the one the
kernel processes first. So, it all just happens to work out. I have
since at least posted patches to add explicit status = "disabled" for
the ports that aren't hooked up, but we should start thinking about how
to use /aliases to force a particular enumeration order rather than
relying on whatever is making it work accidentally.

-- 
nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 4:17 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/05/2011 04:35 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Dec 5, 2011 at 2:15 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/05/2011 02:46 PM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On Mon, Dec 5, 2011 at 1:33 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>> On 12/02/2011 05:59 PM, Simon Glass wrote:
>>>>>> Hi Stephen,
>>>>>>
>>>>>> Here are my comments on the rest of your email.
>>>>>>
>>>>>> On Mon, Nov 28, 2011 at 11:21 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>>>> On 11/23/2011 08:54 PM, Simon Glass wrote:
>>>>>>>> This adds basic support for the Tegra2 USB controller. Board files should
>>>>>>>> call board_usb_init() to set things up.
>>>>>
>>>>>>>> + ? ? ? config->enabled = fdtdec_get_is_enabled(blob, node);
>>>>>>>> + ? ? ? config->periph_id = fdtdec_get_int(blob, node, "periph-id", -1);
>>>>>>>
>>>>>>> periph-id is a U-Boot specific concept, not HW description. The DT
>>>>>>> shouldn't contain that value.
>>>>>>
>>>>>> See my previous comments as to why this is desirable. We can change
>>>>>> over to a clock-based approach once the kernel implements it.
>>>>>
>>>>> That will cause backwards-compatibility problems; older FDTs won't work
>>>>> with newer U-Boots. We should avoid incompatible changes like this if at
>>>>> all possible.
>>>>
>>>> At the moment the fdts are in the U-Boot tree, so we should be able to
>>>> change them at the same time. But of course when we update the fdt ?we
>>>> need to update the U-Boot code. Is there any alternative other than
>>>> doing nothing until the kernel decides everything?
>>>
>>> You can choose not to represent these parameters in the device tree at
>>> all, but rather hard-code the values in the driver. This is what the
>>> kernel currently does; as luck would have it, the required values are
>>> identical for all the boards the kernel currently supports. Once that's
>>> all in place and working, we can work on defining a binding for those
>>> parameters and review/implement it in both U-Boot and the kernel.
>>
>> OK, but how about we have this conversation when things are actually
>> in the kernel and working? The scheme used in this series (peripheral
>> IDs) is very handy for U-Boot and avoids this hard-coding that you
>> refer to.
>
> I'd really like to avoid adding stuff to the .dts file when we know we
> going to rip it out again ASAP. I'd prefer to incrementally enhance the
> .dts bindings always moving forward, and never removing/breaking stuff
> if possible.
>
> Now you did point out that the .dts files are currently in U-Boot, so
> this is slightly moot since the binding documentation, .dts file and
> driver can all be rev'd in sync. However, I hope this will change soon.
> Otherwise, every addition to them means changing U-Boot, the Linux
> kernel, U-Boot v2, fastboot, quick boot, .....

OK well I can remove the USB params and put in the C file with an
#ifdef for T30. Ick.

>
>> Also the only way I can see it being hard-coded is by the kernel
>> looking at the peripheral address and converting this to an ID. That
>> seems really ugly to me. Or am I missing something?
>
> Well, the Tegra SD/MMC driver works exactly that way (well, mapping an
> instance ID to both base address and periph ID), so there's certainly
> precedent for this. And that driver is not unique.

I don't know if I can NAK a comment but I would like to NAK this one.

>
>>>>>>>> +int board_usb_init(const void *blob)
>>>>>>>> +{
>>>>>>>> +#ifdef CONFIG_OF_CONTROL
>>>>>>>> + ? ? ? struct fdt_usb config;
>>>>>>>> + ? ? ? int clk_done = 0;
>>>>>>>> + ? ? ? int node, upto = 0;
>>>>>>>> + ? ? ? unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
>>>>>>>> +
>>>>>>>> + ? ? ? do {
>>>>>>>> + ? ? ? ? ? ? ? node = fdtdec_next_alias(blob, "usb",
>>>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?COMPAT_NVIDIA_TEGRA20_USB, &upto);
>>>>>>>
>>>>>>> Why only initialize USB controllers with aliases? Surely this should
>>>>>>> enumerate all nodes with a specific compatible flag?
>>>>>>
>>>>>> See my other comments - we want to know that port 0 is USB3 on Seaboard.
>>>>>
>>>>> Why does U-Boot care? Everything should be enumerating which peripherals
>>>>> happen to appear on the various USB busses, and not care which host
>>>>> controller they're attached to.
>>>>
>>>> At present we do:
>>>>
>>>> 'usb start 0'
>>>> 'usb start 1'
>>>>
>>>> to select between the ports. There is a patch in the works to change
>>>> that but it hasn't gone upstream yet, or at least wasn't accepted.
>
> Judging by the USB driver, there's actually only "usb start" with no
> parameter, and ehc-tegra.c:ehci_hcd_init() hard-codes this as starting
> port 0. That's a little more severe. Anyway, that implies to me that
> tegra-seaboard.dts should set status = "disabled" on all but one port,
> because the others will be useless anyway.
>
>>> Can you point out the patch that changes this, and what exactly it
>>> changes. Hopefully it removes the parameter from the "usb start" command.
>>
>> Yes, they are in the Chromium tree:
>>
>> https://gerrit.chromium.org/gerrit/#change,4951
>> https://gerrit.chromium.org/gerrit/#change,4981
>
> OK, those look interesting, at least from the commit descriptions. I'd
> assert they really should be upstreamed before or as part of the Tegra
> USB driver addition, since it makes the whole /aliases thing completely
> irrelevant for USB.

No, that needs to be decoupled from this in my view. That is a large
and invasive change which is AFAIK only useful for Tegra. It's not at
all clear it will be accepted.

>
>>> I'm still not convinced why U-Boot cares about this (as opposed to the
>>> user using U-Boot).
>>
>> Well if U-Boot presents the ports in the wrong order, the user's
>> scripts will fail.
>>
>> Also, what about the console UART problem? Surely the kernel provides
>> a way to select the ordering of those? How do I know what UART I am
>> getting on /dev/ttyS0?
>
> That's a question my subconscious has been asking me for a while to. The
> answer is that it's all very accidental...
>
> The kernel serial driver needs a clock-frequency property. If it isn't
> present, the driver won't initialize. The .dts files in the kernel only
> have this property for serial ports that are hooked up on the given
> board. In the case (Paz00 a/k/a Toshiba AC100) where multiple serial
> ports have this property, the useful one just happens to be the one the
> kernel processes first. So, it all just happens to work out. I have
> since at least posted patches to add explicit status = "disabled" for
> the ports that aren't hooked up, but we should start thinking about how
> to use /aliases to force a particular enumeration order rather than
> relying on whatever is making it work accidentally.

Ickity ick. I think I'll keep my aliases if it's ok with you?

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren]

On 12/05/2011 06:14 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Dec 5, 2011 at 4:17 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/05/2011 04:35 PM, Simon Glass wrote:
...
>> I'd really like to avoid adding stuff to the .dts file when we know we
>> going to rip it out again ASAP. I'd prefer to incrementally enhance the
>> .dts bindings always moving forward, and never removing/breaking stuff
>> if possible.
>>
>> Now you did point out that the .dts files are currently in U-Boot, so
>> this is slightly moot since the binding documentation, .dts file and
>> driver can all be rev'd in sync. However, I hope this will change soon.
>> Otherwise, every addition to them means changing U-Boot, the Linux
>> kernel, U-Boot v2, fastboot, quick boot, .....
> 
> OK well I can remove the USB params and put in the C file with an
> #ifdef for T30. Ick.

There's no T30 support in mainline U-Boot, so I think you can avoid any
ifdefs.

>>> Also the only way I can see it being hard-coded is by the kernel
>>> looking at the peripheral address and converting this to an ID. That
>>> seems really ugly to me. Or am I missing something?
>>
>> Well, the Tegra SD/MMC driver works exactly that way (well, mapping an
>> instance ID to both base address and periph ID), so there's certainly
>> precedent for this. And that driver is not unique.
> 
> I don't know if I can NAK a comment but I would like to NAK this one.

I don't know what that means; that you believe my statement is
incorrect, or you don't like the argument I'm basing on it or ...?

>>>>> At present we do:
>>>>>
>>>>> 'usb start 0'
>>>>> 'usb start 1'
>>>>>
>>>>> to select between the ports. There is a patch in the works to change
>>>>> that but it hasn't gone upstream yet, or at least wasn't accepted.
>>>> Can you point out the patch that changes this, and what exactly it
>>>> changes. Hopefully it removes the parameter from the "usb start" command.
>>>
>>> Yes, they are in the Chromium tree:
>>>
>>> https://gerrit.chromium.org/gerrit/#change,4951
>>> https://gerrit.chromium.org/gerrit/#change,4981
>>
>> OK, those look interesting, at least from the commit descriptions. I'd
>> assert they really should be upstreamed before or as part of the Tegra
>> USB driver addition, since it makes the whole /aliases thing completely
>> irrelevant for USB.
> 
> No, that needs to be decoupled from this in my view. That is a large
> and invasive change which is AFAIK only useful for Tegra. It's not at
> all clear it will be accepted.

Surely it's useful for any SoC that has multiple USB controllers, and
where the user may plug peripherals into more than 1 of them. I assume
that's common. Or am I misunderstanding what those patches do?

>>>> I'm still not convinced why U-Boot cares about this (as opposed to the
>>>> user using U-Boot).
>>>
>>> Well if U-Boot presents the ports in the wrong order, the user's
>>> scripts will fail.
>>>
>>> Also, what about the console UART problem? Surely the kernel provides
>>> a way to select the ordering of those? How do I know what UART I am
>>> getting on /dev/ttyS0?
>>
>> That's a question my subconscious has been asking me for a while to. The
>> answer is that it's all very accidental...
>>
>> The kernel serial driver needs a clock-frequency property. If it isn't
>> present, the driver won't initialize. The .dts files in the kernel only
>> have this property for serial ports that are hooked up on the given
>> board. In the case (Paz00 a/k/a Toshiba AC100) where multiple serial
>> ports have this property, the useful one just happens to be the one the
>> kernel processes first. So, it all just happens to work out. I have
>> since at least posted patches to add explicit status = "disabled" for
>> the ports that aren't hooked up, but we should start thinking about how
>> to use /aliases to force a particular enumeration order rather than
>> relying on whatever is making it work accidentally.
> 
> Ickity ick. I think I'll keep my aliases if it's ok with you?

Having aliases is fine.

Using aliases to name the devices which get instantiated through
standardized enumeration mechanisms is fine.

To the best of my knowledge, restricting device enumeration based on the
aliases is non-standard, hence wrong.

As I mentioned elsewhere, the patches only allow one to actually use usb
controller 0; ehci-tegra.c's ehci_hcd_init() hard-codes port 0 when
calling tegrausb_start_port(). Rather than relying on non-standard
aliases usage to restrict the set of USB devices that get instantiated,
why not just add status = "disabled" to all but one USB host's node in
each board's .dts file; that's the standard way to disable devices.

-- 
nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Tue, Dec 6, 2011 at 12:42 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/05/2011 06:14 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Dec 5, 2011 at 4:17 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/05/2011 04:35 PM, Simon Glass wrote:
> ...
>>> I'd really like to avoid adding stuff to the .dts file when we know we
>>> going to rip it out again ASAP. I'd prefer to incrementally enhance the
>>> .dts bindings always moving forward, and never removing/breaking stuff
>>> if possible.
>>>
>>> Now you did point out that the .dts files are currently in U-Boot, so
>>> this is slightly moot since the binding documentation, .dts file and
>>> driver can all be rev'd in sync. However, I hope this will change soon.
>>> Otherwise, every addition to them means changing U-Boot, the Linux
>>> kernel, U-Boot v2, fastboot, quick boot, .....
>>
>> OK well I can remove the USB params and put in the C file with an
>> #ifdef for T30. Ick.
>
> There's no T30 support in mainline U-Boot, so I think you can avoid any
> ifdefs.
>
>>>> Also the only way I can see it being hard-coded is by the kernel
>>>> looking at the peripheral address and converting this to an ID. That
>>>> seems really ugly to me. Or am I missing something?
>>>
>>> Well, the Tegra SD/MMC driver works exactly that way (well, mapping an
>>> instance ID to both base address and periph ID), so there's certainly
>>> precedent for this. And that driver is not unique.
>>
>> I don't know if I can NAK a comment but I would like to NAK this one.
>
> I don't know what that means; that you believe my statement is
> incorrect, or you don't like the argument I'm basing on it or ...?

What happens is that the SD/MMC driver (dating from pre-FDT days) has
a hard-coded base address and peripheral ID, based on an instance ID
(0, 1, 2).

I would expect that we want the FDT to control all of this - it
already has the base address, can have the peripheral ID, and the
instance ID (ordering) should be set by the FDT not hard-coded IMO.
That's the reason for my NAK comment. It just seems completely wrong
to duplicate this information in the driver and require the instance
ordering to be hard-coded in the driver. It means that boards that
want to change this ordering must fiddle around in the driver to make
this work.

Also this is U-Boot, not the kernel. Commands like 'ext2load' require
that you pass the instance ID to indicate which device to use.

>
>>>>>> At present we do:
>>>>>>
>>>>>> 'usb start 0'
>>>>>> 'usb start 1'
>>>>>>
>>>>>> to select between the ports. There is a patch in the works to change
>>>>>> that but it hasn't gone upstream yet, or at least wasn't accepted.
>>>>> Can you point out the patch that changes this, and what exactly it
>>>>> changes. Hopefully it removes the parameter from the "usb start" command.
>>>>
>>>> Yes, they are in the Chromium tree:
>>>>
>>>> https://gerrit.chromium.org/gerrit/#change,4951
>>>> https://gerrit.chromium.org/gerrit/#change,4981
>>>
>>> OK, those look interesting, at least from the commit descriptions. I'd
>>> assert they really should be upstreamed before or as part of the Tegra
>>> USB driver addition, since it makes the whole /aliases thing completely
>>> irrelevant for USB.
>>
>> No, that needs to be decoupled from this in my view. That is a large
>> and invasive change which is AFAIK only useful for Tegra. It's not at
>> all clear it will be accepted.
>
> Surely it's useful for any SoC that has multiple USB controllers, and
> where the user may plug peripherals into more than 1 of them. I assume
> that's common. Or am I misunderstanding what those patches do?

I don't believe it has been an issue with other SOCs, but I don't know
for sure. Anyway, it is sideways to this issue. I agree it should be
upstreamed but a separate series IMO.

>
>>>>> I'm still not convinced why U-Boot cares about this (as opposed to the
>>>>> user using U-Boot).
>>>>
>>>> Well if U-Boot presents the ports in the wrong order, the user's
>>>> scripts will fail.
>>>>
>>>> Also, what about the console UART problem? Surely the kernel provides
>>>> a way to select the ordering of those? How do I know what UART I am
>>>> getting on /dev/ttyS0?
>>>
>>> That's a question my subconscious has been asking me for a while to. The
>>> answer is that it's all very accidental...
>>>
>>> The kernel serial driver needs a clock-frequency property. If it isn't
>>> present, the driver won't initialize. The .dts files in the kernel only
>>> have this property for serial ports that are hooked up on the given
>>> board. In the case (Paz00 a/k/a Toshiba AC100) where multiple serial
>>> ports have this property, the useful one just happens to be the one the
>>> kernel processes first. So, it all just happens to work out. I have
>>> since at least posted patches to add explicit status = "disabled" for
>>> the ports that aren't hooked up, but we should start thinking about how
>>> to use /aliases to force a particular enumeration order rather than
>>> relying on whatever is making it work accidentally.
>>
>> Ickity ick. I think I'll keep my aliases if it's ok with you?
>
> Having aliases is fine.
>
> Using aliases to name the devices which get instantiated through
> standardized enumeration mechanisms is fine.
>
> To the best of my knowledge, restricting device enumeration based on the
> aliases is non-standard, hence wrong.

OK, so is your objection that we ignore a peripheral that has no
alias? If I change the code to locate these and add them at the end
would that be better?

>
> As I mentioned elsewhere, the patches only allow one to actually use usb
> controller 0; ehci-tegra.c's ehci_hcd_init() hard-codes port 0 when
> calling tegrausb_start_port(). Rather than relying on non-standard
> aliases usage to restrict the set of USB devices that get instantiated,
> why not just add status = "disabled" to all but one USB host's node in
> each board's .dts file; that's the standard way to disable devices.

I can do that, but how do I solve the ordering problem?

Regards,
Simon

>
> --
> nvpublic

Re: [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren]

On 12/06/2011 02:23 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Tue, Dec 6, 2011 at 12:42 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> On 12/05/2011 06:14 PM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On Mon, Dec 5, 2011 at 4:17 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>> On 12/05/2011 04:35 PM, Simon Glass wrote:
...
>>>>> Also the only way I can see it being hard-coded is by the kernel
>>>>> looking at the peripheral address and converting this to an ID. That
>>>>> seems really ugly to me. Or am I missing something?
>>>>
>>>> Well, the Tegra SD/MMC driver works exactly that way (well, mapping an
>>>> instance ID to both base address and periph ID), so there's certainly
>>>> precedent for this. And that driver is not unique.
>>>
>>> I don't know if I can NAK a comment but I would like to NAK this one.
>>
>> I don't know what that means; that you believe my statement is
>> incorrect, or you don't like the argument I'm basing on it or ...?
> 
> What happens is that the SD/MMC driver (dating from pre-FDT days) has
> a hard-coded base address and peripheral ID, based on an instance ID
> (0, 1, 2).

That's basically the exact same thing, it's just the exact fields that
are the key into and output from the table are different.

> I would expect that we want the FDT to control all of this - it
> already has the base address, can have the peripheral ID, and the
> instance ID (ordering) should be set by the FDT not hard-coded IMO.
> That's the reason for my NAK comment. It just seems completely wrong
> to duplicate this information in the driver and require the instance
> ordering to be hard-coded in the driver. It means that boards that
> want to change this ordering must fiddle around in the driver to make
> this work.
> 
> Also this is U-Boot, not the kernel. Commands like 'ext2load' require
> that you pass the instance ID to indicate which device to use.

SD/MMC is a very different use-case, and not a good analogy to USB.

With SD, the user always wants to identify a specific device that they
know contains their files.

With USB, the user doesn't care that there are multiple USB host
controllers in the SoC; they just want to plug in their device somewhere
and have it work. Having to decide which USB controller to enable at a
time is pretty hokey, given there's unlikely to be any indication at all
on the PCB or case which ports map to which USB controllers.

What the user might care about is selecting a enumerated particular USB
device. They'd only know which one after looking at some "lsusb" command
(or whatever it's called in U-Boot) in the general case.

Hence, I assert that USB host controller number is completely
irrelevant, and all should be active at once.

This of course is all somewhat moot given that I pointed out
ehci-tegra.c only supports one of the hosts anyway...

> OK, so is your objection that we ignore a peripheral that has no
> alias?

Yes.

And that enumeration is based on alias nodes not enumerating all nodes
that match the compatible flag.

> If I change the code to locate these and add them at the end
> would that be better?

Sure.

I think the best implementation would be to enumerate everything solely
based on compatible flags, and allowing "usb start" to accept either an
alias name (which would be fixed) or a DT unit address or node name
(which would be fixed) or a controller index (which might vary based on
enumeration order) to select the controller.

The next best implementation would be to enumerate solely based on
compatible flags, then sort the list based on alias, thus allowing alias
to cause a static order.

However, enumerating based on alias, then enumerating based on
compatible flags and adding any missing nodes would be fine.

>> As I mentioned elsewhere, the patches only allow one to actually use usb
>> controller 0; ehci-tegra.c's ehci_hcd_init() hard-codes port 0 when
>> calling tegrausb_start_port(). Rather than relying on non-standard
>> aliases usage to restrict the set of USB devices that get instantiated,
>> why not just add status = "disabled" to all but one USB host's node in
>> each board's .dts file; that's the standard way to disable devices.
> 
> I can do that, but how do I solve the ordering problem?

If you only have one enabled controller in the .dts file, there can't be
any ordering issue since there is always only 1 controller.

-- 
nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren]

On 12/06/2011 02:23 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Tue, Dec 6, 2011 at 12:42 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/05/2011 06:14 PM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On Mon, Dec 5, 2011 at 4:17 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>> On 12/05/2011 04:35 PM, Simon Glass wrote:
...
>>>>> Also the only way I can see it being hard-coded is by the kernel
>>>>> looking at the peripheral address and converting this to an ID. That
>>>>> seems really ugly to me. Or am I missing something?
>>>>
>>>> Well, the Tegra SD/MMC driver works exactly that way (well, mapping an
>>>> instance ID to both base address and periph ID), so there's certainly
>>>> precedent for this. And that driver is not unique.
>>>
>>> I don't know if I can NAK a comment but I would like to NAK this one.
>>
>> I don't know what that means; that you believe my statement is
>> incorrect, or you don't like the argument I'm basing on it or ...?
> 
> What happens is that the SD/MMC driver (dating from pre-FDT days) has
> a hard-coded base address and peripheral ID, based on an instance ID
> (0, 1, 2).

That's basically the exact same thing, it's just the exact fields that
are the key into and output from the table are different.

> I would expect that we want the FDT to control all of this - it
> already has the base address, can have the peripheral ID, and the
> instance ID (ordering) should be set by the FDT not hard-coded IMO.
> That's the reason for my NAK comment. It just seems completely wrong
> to duplicate this information in the driver and require the instance
> ordering to be hard-coded in the driver. It means that boards that
> want to change this ordering must fiddle around in the driver to make
> this work.
> 
> Also this is U-Boot, not the kernel. Commands like 'ext2load' require
> that you pass the instance ID to indicate which device to use.

SD/MMC is a very different use-case, and not a good analogy to USB.

With SD, the user always wants to identify a specific device that they
know contains their files.

With USB, the user doesn't care that there are multiple USB host
controllers in the SoC; they just want to plug in their device somewhere
and have it work. Having to decide which USB controller to enable at a
time is pretty hokey, given there's unlikely to be any indication at all
on the PCB or case which ports map to which USB controllers.

What the user might care about is selecting a enumerated particular USB
device. They'd only know which one after looking at some "lsusb" command
(or whatever it's called in U-Boot) in the general case.

Hence, I assert that USB host controller number is completely
irrelevant, and all should be active at once.

This of course is all somewhat moot given that I pointed out
ehci-tegra.c only supports one of the hosts anyway...

> OK, so is your objection that we ignore a peripheral that has no
> alias?

Yes.

And that enumeration is based on alias nodes not enumerating all nodes
that match the compatible flag.

> If I change the code to locate these and add them at the end
> would that be better?

Sure.

I think the best implementation would be to enumerate everything solely
based on compatible flags, and allowing "usb start" to accept either an
alias name (which would be fixed) or a DT unit address or node name
(which would be fixed) or a controller index (which might vary based on
enumeration order) to select the controller.

The next best implementation would be to enumerate solely based on
compatible flags, then sort the list based on alias, thus allowing alias
to cause a static order.

However, enumerating based on alias, then enumerating based on
compatible flags and adding any missing nodes would be fine.

>> As I mentioned elsewhere, the patches only allow one to actually use usb
>> controller 0; ehci-tegra.c's ehci_hcd_init() hard-codes port 0 when
>> calling tegrausb_start_port(). Rather than relying on non-standard
>> aliases usage to restrict the set of USB devices that get instantiated,
>> why not just add status = "disabled" to all but one USB host's node in
>> each board's .dts file; that's the standard way to disable devices.
> 
> I can do that, but how do I solve the ordering problem?

If you only have one enabled controller in the .dts file, there can't be
any ordering issue since there is always only 1 controller.

-- 
nvpublic

Re: [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Wed, Dec 7, 2011 at 3:46 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/06/2011 02:23 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Tue, Dec 6, 2011 at 12:42 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/05/2011 06:14 PM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On Mon, Dec 5, 2011 at 4:17 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>> On 12/05/2011 04:35 PM, Simon Glass wrote:
> ...
>>>>>> Also the only way I can see it being hard-coded is by the kernel
>>>>>> looking at the peripheral address and converting this to an ID. That
>>>>>> seems really ugly to me. Or am I missing something?
>>>>>
>>>>> Well, the Tegra SD/MMC driver works exactly that way (well, mapping an
>>>>> instance ID to both base address and periph ID), so there's certainly
>>>>> precedent for this. And that driver is not unique.
>>>>
>>>> I don't know if I can NAK a comment but I would like to NAK this one.
>>>
>>> I don't know what that means; that you believe my statement is
>>> incorrect, or you don't like the argument I'm basing on it or ...?
>>
>> What happens is that the SD/MMC driver (dating from pre-FDT days) has
>> a hard-coded base address and peripheral ID, based on an instance ID
>> (0, 1, 2).
>
> That's basically the exact same thing, it's just the exact fields that
> are the key into and output from the table are different.
>
>> I would expect that we want the FDT to control all of this - it
>> already has the base address, can have the peripheral ID, and the
>> instance ID (ordering) should be set by the FDT not hard-coded IMO.
>> That's the reason for my NAK comment. It just seems completely wrong
>> to duplicate this information in the driver and require the instance
>> ordering to be hard-coded in the driver. It means that boards that
>> want to change this ordering must fiddle around in the driver to make
>> this work.
>>
>> Also this is U-Boot, not the kernel. Commands like 'ext2load' require
>> that you pass the instance ID to indicate which device to use.
>
> SD/MMC is a very different use-case, and not a good analogy to USB.
>
> With SD, the user always wants to identify a specific device that they
> know contains their files.
>
> With USB, the user doesn't care that there are multiple USB host
> controllers in the SoC; they just want to plug in their device somewhere
> and have it work. Having to decide which USB controller to enable at a
> time is pretty hokey, given there's unlikely to be any indication at all
> on the PCB or case which ports map to which USB controllers.
>
> What the user might care about is selecting a enumerated particular USB
> device. They'd only know which one after looking at some "lsusb" command
> (or whatever it's called in U-Boot) in the general case.
>
> Hence, I assert that USB host controller number is completely
> irrelevant, and all should be active at once.

In our case we do not want to look at the USB port the contains the 3G
modem, for example, just the external one which can contain a USB
stick.

>
> This of course is all somewhat moot given that I pointed out
> ehci-tegra.c only supports one of the hosts anyway...
>
>> OK, so is your objection that we ignore a peripheral that has no
>> alias?
>
> Yes.

OK. We will ignore it anyway since USB2 is marked as disabled, but I
agree that isn't true in general.

>
> And that enumeration is based on alias nodes not enumerating all nodes
> that match the compatible flag.
>
>> If I change the code to locate these and add them at the end
>> would that be better?
>
> Sure.
>
> I think the best implementation would be to enumerate everything solely
> based on compatible flags, and allowing "usb start" to accept either an
> alias name (which would be fixed) or a DT unit address or node name
> (which would be fixed) or a controller index (which might vary based on
> enumeration order) to select the controller.
>
> The next best implementation would be to enumerate solely based on
> compatible flags, then sort the list based on alias, thus allowing alias
> to cause a static order.
>
> However, enumerating based on alias, then enumerating based on
> compatible flags and adding any missing nodes would be fine.

Well ok. Since this is basically a small *addition* to the code
(scanning things that don't have an alias), and will have no effect
with the device tree as included in this series, I would like to do
this as a follow-on patch after the series. I hope you can live with
that also?

>
>>> As I mentioned elsewhere, the patches only allow one to actually use usb
>>> controller 0; ehci-tegra.c's ehci_hcd_init() hard-codes port 0 when
>>> calling tegrausb_start_port(). Rather than relying on non-standard
>>> aliases usage to restrict the set of USB devices that get instantiated,
>>> why not just add status = "disabled" to all but one USB host's node in
>>> each board's .dts file; that's the standard way to disable devices.
>>
>> I can do that, but how do I solve the ordering problem?
>
> If you only have one enabled controller in the .dts file, there can't be
> any ordering issue since there is always only 1 controller.

I really was referring to how it should be done. We will enable two
points. Only one is currently accessible due to us needing either a
parameter to 'usb start' or the virtual ehci hub code. But USB
upstreaming is not complete after this series - it is only a step in
the process. I feel that 14 patches is big enough for a series :-)

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Wed, Dec 7, 2011 at 3:46 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/06/2011 02:23 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Tue, Dec 6, 2011 at 12:42 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/05/2011 06:14 PM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On Mon, Dec 5, 2011 at 4:17 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>> On 12/05/2011 04:35 PM, Simon Glass wrote:
> ...
>>>>>> Also the only way I can see it being hard-coded is by the kernel
>>>>>> looking at the peripheral address and converting this to an ID. That
>>>>>> seems really ugly to me. Or am I missing something?
>>>>>
>>>>> Well, the Tegra SD/MMC driver works exactly that way (well, mapping an
>>>>> instance ID to both base address and periph ID), so there's certainly
>>>>> precedent for this. And that driver is not unique.
>>>>
>>>> I don't know if I can NAK a comment but I would like to NAK this one.
>>>
>>> I don't know what that means; that you believe my statement is
>>> incorrect, or you don't like the argument I'm basing on it or ...?
>>
>> What happens is that the SD/MMC driver (dating from pre-FDT days) has
>> a hard-coded base address and peripheral ID, based on an instance ID
>> (0, 1, 2).
>
> That's basically the exact same thing, it's just the exact fields that
> are the key into and output from the table are different.
>
>> I would expect that we want the FDT to control all of this - it
>> already has the base address, can have the peripheral ID, and the
>> instance ID (ordering) should be set by the FDT not hard-coded IMO.
>> That's the reason for my NAK comment. It just seems completely wrong
>> to duplicate this information in the driver and require the instance
>> ordering to be hard-coded in the driver. It means that boards that
>> want to change this ordering must fiddle around in the driver to make
>> this work.
>>
>> Also this is U-Boot, not the kernel. Commands like 'ext2load' require
>> that you pass the instance ID to indicate which device to use.
>
> SD/MMC is a very different use-case, and not a good analogy to USB.
>
> With SD, the user always wants to identify a specific device that they
> know contains their files.
>
> With USB, the user doesn't care that there are multiple USB host
> controllers in the SoC; they just want to plug in their device somewhere
> and have it work. Having to decide which USB controller to enable at a
> time is pretty hokey, given there's unlikely to be any indication at all
> on the PCB or case which ports map to which USB controllers.
>
> What the user might care about is selecting a enumerated particular USB
> device. They'd only know which one after looking at some "lsusb" command
> (or whatever it's called in U-Boot) in the general case.
>
> Hence, I assert that USB host controller number is completely
> irrelevant, and all should be active at once.

In our case we do not want to look at the USB port the contains the 3G
modem, for example, just the external one which can contain a USB
stick.

>
> This of course is all somewhat moot given that I pointed out
> ehci-tegra.c only supports one of the hosts anyway...
>
>> OK, so is your objection that we ignore a peripheral that has no
>> alias?
>
> Yes.

OK. We will ignore it anyway since USB2 is marked as disabled, but I
agree that isn't true in general.

>
> And that enumeration is based on alias nodes not enumerating all nodes
> that match the compatible flag.
>
>> If I change the code to locate these and add them at the end
>> would that be better?
>
> Sure.
>
> I think the best implementation would be to enumerate everything solely
> based on compatible flags, and allowing "usb start" to accept either an
> alias name (which would be fixed) or a DT unit address or node name
> (which would be fixed) or a controller index (which might vary based on
> enumeration order) to select the controller.
>
> The next best implementation would be to enumerate solely based on
> compatible flags, then sort the list based on alias, thus allowing alias
> to cause a static order.
>
> However, enumerating based on alias, then enumerating based on
> compatible flags and adding any missing nodes would be fine.

Well ok. Since this is basically a small *addition* to the code
(scanning things that don't have an alias), and will have no effect
with the device tree as included in this series, I would like to do
this as a follow-on patch after the series. I hope you can live with
that also?

>
>>> As I mentioned elsewhere, the patches only allow one to actually use usb
>>> controller 0; ehci-tegra.c's ehci_hcd_init() hard-codes port 0 when
>>> calling tegrausb_start_port(). Rather than relying on non-standard
>>> aliases usage to restrict the set of USB devices that get instantiated,
>>> why not just add status = "disabled" to all but one USB host's node in
>>> each board's .dts file; that's the standard way to disable devices.
>>
>> I can do that, but how do I solve the ordering problem?
>
> If you only have one enabled controller in the .dts file, there can't be
> any ordering issue since there is always only 1 controller.

I really was referring to how it should be done. We will enable two
points. Only one is currently accessible due to us needing either a
parameter to 'usb start' or the virtual ehci hub code. But USB
upstreaming is not complete after this series - it is only a step in
the process. I feel that 14 patches is big enough for a series :-)

Regards,
Simon

>
> --
> nvpublic

Re: [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren]

On 12/08/2011 02:24 PM, Simon Glass wrote:
> On Wed, Dec 7, 2011 at 3:46 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> On 12/06/2011 02:23 PM, Simon Glass wrote:
...
>> I think the best implementation would be to enumerate everything solely
>> based on compatible flags, and allowing "usb start" to accept either an
>> alias name (which would be fixed) or a DT unit address or node name
>> (which would be fixed) or a controller index (which might vary based on
>> enumeration order) to select the controller.
>>
>> The next best implementation would be to enumerate solely based on
>> compatible flags, then sort the list based on alias, thus allowing alias
>> to cause a static order.
>>
>> However, enumerating based on alias, then enumerating based on
>> compatible flags and adding any missing nodes would be fine.
> 
> Well ok. Since this is basically a small *addition* to the code
> (scanning things that don't have an alias), and will have no effect
> with the device tree as included in this series, I would like to do
> this as a follow-on patch after the series. I hope you can live with
> that also?

I suppose it'll have to do.

It's totally the wrong way to go about it though, and will provide a bad
example that'll probably end up proliferating itself through the code
since it's the first example.

In other words rather than:

Now: Scan /aliases for USB, add then all.

Later: Scan device nodes for any that weren't in /aliases, add them all

I'd prefer to see:

Now: Scan /aliases for USB, add then all.

Later: Remove all the /aliases scanning code, fix the code to scan all
child nodes of the SoC node, find all nodes matching the USB compatible
flag, add them all. While adding a USB controller, check the /alias node
and honor any alias there if there is one.

-- 
nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Stephen Warren]

On 12/08/2011 02:24 PM, Simon Glass wrote:
> On Wed, Dec 7, 2011 at 3:46 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/06/2011 02:23 PM, Simon Glass wrote:
...
>> I think the best implementation would be to enumerate everything solely
>> based on compatible flags, and allowing "usb start" to accept either an
>> alias name (which would be fixed) or a DT unit address or node name
>> (which would be fixed) or a controller index (which might vary based on
>> enumeration order) to select the controller.
>>
>> The next best implementation would be to enumerate solely based on
>> compatible flags, then sort the list based on alias, thus allowing alias
>> to cause a static order.
>>
>> However, enumerating based on alias, then enumerating based on
>> compatible flags and adding any missing nodes would be fine.
> 
> Well ok. Since this is basically a small *addition* to the code
> (scanning things that don't have an alias), and will have no effect
> with the device tree as included in this series, I would like to do
> this as a follow-on patch after the series. I hope you can live with
> that also?

I suppose it'll have to do.

It's totally the wrong way to go about it though, and will provide a bad
example that'll probably end up proliferating itself through the code
since it's the first example.

In other words rather than:

Now: Scan /aliases for USB, add then all.

Later: Scan device nodes for any that weren't in /aliases, add them all

I'd prefer to see:

Now: Scan /aliases for USB, add then all.

Later: Remove all the /aliases scanning code, fix the code to scan all
child nodes of the SoC node, find all nodes matching the USB compatible
flag, add them all. While adding a USB controller, check the /alias node
and honor any alias there if there is one.

-- 
nvpublic

Re: [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Mon, Dec 12, 2011 at 10:18 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/08/2011 02:24 PM, Simon Glass wrote:
>> On Wed, Dec 7, 2011 at 3:46 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/06/2011 02:23 PM, Simon Glass wrote:
> ...
>>> I think the best implementation would be to enumerate everything solely
>>> based on compatible flags, and allowing "usb start" to accept either an
>>> alias name (which would be fixed) or a DT unit address or node name
>>> (which would be fixed) or a controller index (which might vary based on
>>> enumeration order) to select the controller.
>>>
>>> The next best implementation would be to enumerate solely based on
>>> compatible flags, then sort the list based on alias, thus allowing alias
>>> to cause a static order.
>>>
>>> However, enumerating based on alias, then enumerating based on
>>> compatible flags and adding any missing nodes would be fine.
>>
>> Well ok. Since this is basically a small *addition* to the code
>> (scanning things that don't have an alias), and will have no effect
>> with the device tree as included in this series, I would like to do
>> this as a follow-on patch after the series. I hope you can live with
>> that also?
>
> I suppose it'll have to do.
>
> It's totally the wrong way to go about it though, and will provide a bad
> example that'll probably end up proliferating itself through the code
> since it's the first example.
>
> In other words rather than:
>
> Now: Scan /aliases for USB, add then all.
>
> Later: Scan device nodes for any that weren't in /aliases, add them all
>
> I'd prefer to see:
>
> Now: Scan /aliases for USB, add then all.
>
> Later: Remove all the /aliases scanning code, fix the code to scan all
> child nodes of the SoC node, find all nodes matching the USB compatible
> flag, add them all. While adding a USB controller, check the /alias node
> and honor any alias there if there is one.

OK, perhaps I am thinking too closely about an efficient algorithm as
separate from the required function. This patch series does the 'Now'
but, and the 'Later' bit can be done as you say I think. Issues that
come include someone having an alias for port 2 but not 1, and having
to detect this and move things down at the end. I will need to think
about support in fdtdec to make this easy. That's why I want to do it
later as this patch series is already large and we are well aware of
its limitations in several areas (e.g. there is no way to use port 1
in U-Boot yet).

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH 10/14] tegra: usb: Add support for USB peripheral

[Simon Glass]

Hi Stephen,

On Mon, Dec 12, 2011 at 10:18 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/08/2011 02:24 PM, Simon Glass wrote:
>> On Wed, Dec 7, 2011 at 3:46 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/06/2011 02:23 PM, Simon Glass wrote:
> ...
>>> I think the best implementation would be to enumerate everything solely
>>> based on compatible flags, and allowing "usb start" to accept either an
>>> alias name (which would be fixed) or a DT unit address or node name
>>> (which would be fixed) or a controller index (which might vary based on
>>> enumeration order) to select the controller.
>>>
>>> The next best implementation would be to enumerate solely based on
>>> compatible flags, then sort the list based on alias, thus allowing alias
>>> to cause a static order.
>>>
>>> However, enumerating based on alias, then enumerating based on
>>> compatible flags and adding any missing nodes would be fine.
>>
>> Well ok. Since this is basically a small *addition* to the code
>> (scanning things that don't have an alias), and will have no effect
>> with the device tree as included in this series, I would like to do
>> this as a follow-on patch after the series. I hope you can live with
>> that also?
>
> I suppose it'll have to do.
>
> It's totally the wrong way to go about it though, and will provide a bad
> example that'll probably end up proliferating itself through the code
> since it's the first example.
>
> In other words rather than:
>
> Now: Scan /aliases for USB, add then all.
>
> Later: Scan device nodes for any that weren't in /aliases, add them all
>
> I'd prefer to see:
>
> Now: Scan /aliases for USB, add then all.
>
> Later: Remove all the /aliases scanning code, fix the code to scan all
> child nodes of the SoC node, find all nodes matching the USB compatible
> flag, add them all. While adding a USB controller, check the /alias node
> and honor any alias there if there is one.

OK, perhaps I am thinking too closely about an efficient algorithm as
separate from the required function. This patch series does the 'Now'
but, and the 'Later' bit can be done as you say I think. Issues that
come include someone having an alias for port 2 but not 1, and having
to detect this and move things down at the end. I will need to think
about support in fdtdec to make this easy. That's why I want to do it
later as this patch series is already large and we are well aware of
its limitations in several areas (e.g. there is no way to use port 1
in U-Boot yet).

Regards,
Simon

>
> --
> nvpublic