[U-Boot] [PATCH v2 0/17] tegra: Add fdt definitions and USB driver

[U-Boot] [PATCH v2 0/17] tegra: Add fdt definitions and USB driver

[Simon Glass]

This series brings in the kernel fdt file, adds USB definitions support
to it, and provides a working USB driver for Tegra2 Seaboard.

(I have done this in one series since otherwise most of the fdt additions
will just look like dead code.)

The driver requires CONFIG_OF_CONTROL and a device tree to operate.

Some enhancements to fdtdec are required to make this easier, and these
are included in the series also. I have had to bring in basic GPIO
support due to the request to put the USB VBUS into the fdt.

Since the kernel recently got a very minimal USB binding, I have started
with that and extended it where appropriate.

Changes in v2:
- Use "okay" rather than "ok" in fdt to signify an enabled node
- Add header prototype for fdtdec_next_compatible()
- Change fdtdec_next_alias() to increment 'upto' only on success
- Rename get_prop_len() to get_prop_check_min_len()
- New patch to add gpio_request() to asm-generic header
- Pulled USB fdt definitions from newer for-next instead of master
- Change fdt property usbparmas to usbtiming
- Add details for port USB2
- Use "okay" instead of "ok" for fdt node status
- Remove 0x from fdt aliases
- Tidy code style in USB buffer alignment code
- Display prominent warning when alignment code is triggered
- Rename params to timing
- Store entire fdt config in port list, not just register pointer
- Remove non-fdt operation of USB, since it is not needed
- Decode USB VBUS GPIO from the fdt
- Decode phy type differently (to match new kernel fdt)
- Rename tegra20-usb to tegra20-ehcui (to match new kernel fdt)
- Improve debug() printouts in case of failure to init USB
- Add setting of pinmux for USB VBUS GPIO
- Remove unneeded CONFIG_TEGRA_USBx defines

Simon Glass (17):
  fdt: Tidy up a few fdtdec problems
  fdt: Add functions to access phandles, arrays and bools
  Add gpio_request() to asm-generic header
  fdt: Add basic support for decoding GPIO definitions
  arm: fdt: Ensure that an embedded fdt is word-aligned
  arm: fdt: Add skeleton device tree file from kernel
  tegra: fdt: Add Tegra2x device tree file from kernel
  tegra: fdt: Add device tree file for Tegra2 Seaboard from kernel
  tegra: usb: fdt: Add additional device tree definitions for USB ports
  tegra: usb: fdt: Add USB definitions for Tegra2 Seaboard
  usb: Add support for data alignment
  usb: Add support for txfifo threshold
  tegra: usb: Add support for Tegra USB peripheral
  tegra: usb: Add USB support to nvidia boards
  tegra: usb: Add common USB defines for tegra2 boards
  tegra: usb: Enable USB on Seaboard
  tegra: fdt: Enable FDT support for Seaboard

 README                                    |    9 +
 arch/arm/cpu/armv7/tegra2/Makefile        |    4 +-
 arch/arm/cpu/armv7/tegra2/config.mk       |    2 +
 arch/arm/cpu/armv7/tegra2/usb.c           |  398 +++++++++++++++++++++++++++++
 arch/arm/cpu/armv7/u-boot.lds             |    5 +
 arch/arm/dts/skeleton.dtsi                |   13 +
 arch/arm/dts/tegra20.dtsi                 |  238 +++++++++++++++++
 arch/arm/include/asm/arch-tegra2/tegra2.h |    2 +
 arch/arm/include/asm/arch-tegra2/usb.h    |  255 ++++++++++++++++++
 board/nvidia/common/board.c               |    8 +
 board/nvidia/dts/tegra2-seaboard.dts      |   48 ++++
 drivers/usb/host/Makefile                 |    1 +
 drivers/usb/host/ehci-hcd.c               |   36 +++
 drivers/usb/host/ehci-tegra.c             |   63 +++++
 drivers/usb/host/ehci.h                   |    6 +-
 dts/Makefile                              |    2 +-
 include/asm-generic/gpio.h                |   10 +
 include/configs/seaboard.h                |   12 +
 include/configs/tegra2-common.h           |   14 +
 include/fdtdec.h                          |  113 ++++++++-
 lib/fdtdec.c                              |  182 +++++++++++++-
 21 files changed, 1408 insertions(+), 13 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/tegra2/usb.c
 create mode 100644 arch/arm/dts/skeleton.dtsi
 create mode 100644 arch/arm/dts/tegra20.dtsi
 create mode 100644 arch/arm/include/asm/arch-tegra2/usb.h
 create mode 100644 board/nvidia/dts/tegra2-seaboard.dts
 create mode 100644 drivers/usb/host/ehci-tegra.c

-- 
1.7.3.1

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

[Simon Glass]

This fixes five 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. This is needed
only because the first fdt_compat_id is defined to be invalid.
4. Added a header prototype for fdtdec_next_compatible()
5. Change fdtdec_next_alias() to only increment its 'upto' parameter
on success, to make the display error messages in the caller easier.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Use "okay" rather than "ok" in fdt to signify an enabled node
- Add header prototype for fdtdec_next_compatible()
- Change fdtdec_next_alias() to increment 'upto' only on success

 include/fdtdec.h |   23 +++++++++++++++++++----
 lib/fdtdec.c     |   15 +++++++++------
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index d871cdd..49dbac4 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -82,6 +82,21 @@ int fdtdec_next_alias(const void *blob, const char *name,
 		enum fdt_compat_id id, int *upto);
 
 /**
+ * Find the next compatible node for a peripheral.
+ *
+ * Do the first call with node = 0. This function will return a pointer to
+ * the next compatible node. Next time you call this function, pass the
+ * value returned, and the next node will be provided.
+ *
+ * @param blob		FDT blob to use
+ * @param node		Start node for search
+ * @param id		Compatible ID to look for (enum fdt_compat_id)
+ * @return offset of next compatible node, or -FDT_ERR_NOTFOUND if no more
+ */
+int fdtdec_next_compatible(const void *blob, int node,
+		enum fdt_compat_id id);
+
+/**
  * Look up an address property in a node and return it as an address.
  * The property must hold either one address with no trailing data or
  * one address with a length. This is only tested on 32-bit machines.
@@ -112,14 +127,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..a0a34e4 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 0 == strcmp(cell, "okay");
+	return 1;
 }
 
 enum fdt_compat_id fd_dec_lookup(const void *blob, int node)
@@ -122,14 +123,16 @@ int fdtdec_next_alias(const void *blob, const char *name,
 	/* snprintf() is not available */
 	assert(strlen(name) < MAX_STR_LEN);
 	sprintf(str, "%.*s%d", MAX_STR_LEN, name, *upto);
-	(*upto)++;
 	node = find_alias_node(blob, str);
 	if (node < 0)
 		return node;
 	err = fdt_node_check_compatible(blob, node, compat_names[id]);
 	if (err < 0)
 		return err;
-	return err ? -FDT_ERR_NOTFOUND : node;
+	if (err)
+		return -FDT_ERR_NOTFOUND;
+	(*upto)++;
+	return node;
 }
 
 /*
@@ -140,7 +143,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 v2 01/17] fdt: Tidy up a few fdtdec problems

[Stephen Warren]

On 12/02/2011 07:11 PM, Simon Glass wrote:
...
> +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 0 == strcmp(cell, "okay");
> +	return 1;
>  }

The kernel accepts both okay (standard) and ok (non-standard). I assume
the latter is required for some in-use-but-technically-incorrect .dts
files. I imagine U-Boot should act like the kernel here.

(Sorry if I wasn't clear here before; that's certainly what I intended
to mean)

-- 
nvpublic

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

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 1:27 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/02/2011 07:11 PM, Simon Glass wrote:
> ...
>> +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 0 == strcmp(cell, "okay");
>> + ? ? return 1;
>> ?}
>
> The kernel accepts both okay (standard) and ok (non-standard). I assume
> the latter is required for some in-use-but-technically-incorrect .dts
> files. I imagine U-Boot should act like the kernel here.

Given that we are just starting out with fdt, do you think it would be
better to not bloat the code in this way? I don't mind either way -
just asking :-)

Regards,
Simon

>
> (Sorry if I wasn't clear here before; that's certainly what I intended
> to mean)
>
> --
> nvpublic

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

[Stephen Warren]

On 12/05/2011 02:40 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Dec 5, 2011 at 1:27 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>> ...
>>> +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 0 == strcmp(cell, "okay");
>>> +     return 1;
>>>  }
>>
>> The kernel accepts both okay (standard) and ok (non-standard). I assume
>> the latter is required for some in-use-but-technically-incorrect .dts
>> files. I imagine U-Boot should act like the kernel here.
> 
> Given that we are just starting out with fdt, do you think it would be
> better to not bloat the code in this way? I don't mind either way -
> just asking :-)

My point is that there are probably .dts files using "ok" instead of
"okay" or the kernel wouldn't support "ok". People will probably want to
use those with U-Boot without changing anything else. So, U-Boot should
interpret the FDT in the same way as the kernel.

-- 
nvpublic

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

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 2:07 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/05/2011 02:40 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Dec 5, 2011 at 1:27 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>> ...
>>>> +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 0 == strcmp(cell, "okay");
>>>> + ? ? return 1;
>>>> ?}
>>>
>>> The kernel accepts both okay (standard) and ok (non-standard). I assume
>>> the latter is required for some in-use-but-technically-incorrect .dts
>>> files. I imagine U-Boot should act like the kernel here.
>>
>> Given that we are just starting out with fdt, do you think it would be
>> better to not bloat the code in this way? I don't mind either way -
>> just asking :-)
>
> My point is that there are probably .dts files using "ok" instead of
> "okay" or the kernel wouldn't support "ok". People will probably want to
> use those with U-Boot without changing anything else. So, U-Boot should
> interpret the FDT in the same way as the kernel.

OK, how about:
		return 0 == strncmp(cell, "ok", 2);

(I do feel that if you do this sort of thing you end up with people
using 'ok' even in new fdts, since they look at code like this and
think it is fine)

Regards,
Simon

>
> --
> nvpublic

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

[Scott Wood]

On 12/05/2011 04:11 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Dec 5, 2011 at 2:07 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> My point is that there are probably .dts files using "ok" instead of
>> "okay" or the kernel wouldn't support "ok". People will probably want to
>> use those with U-Boot without changing anything else. So, U-Boot should
>> interpret the FDT in the same way as the kernel.

The kernel has to deal with real Open Firmware systems, some of which
pass buggy trees.  U-Boot should not blindly imitate all of Linux's
workarounds.

> OK, how about:
> 		return 0 == strncmp(cell, "ok", 2);
> 
> (I do feel that if you do this sort of thing you end up with people
> using 'ok' even in new fdts, since they look at code like this and
> think it is fine)

Indeed.

-Scott

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

[Stephen Warren]

On 12/05/2011 03:18 PM, Scott Wood wrote:
> On 12/05/2011 04:11 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Dec 5, 2011 at 2:07 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> My point is that there are probably .dts files using "ok" instead of
>>> "okay" or the kernel wouldn't support "ok". People will probably want to
>>> use those with U-Boot without changing anything else. So, U-Boot should
>>> interpret the FDT in the same way as the kernel.
> 
> The kernel has to deal with real Open Firmware systems, some of which
> pass buggy trees.  U-Boot should not blindly imitate all of Linux's
> workarounds.

If it's certain that we'll never see anyone writing FDTs with "ok" in
them instead of "okay" (because such FDTs were only autogenerated by
Open Firmware), then I'm fine with the original code.

>> OK, how about:
>> 		return 0 == strncmp(cell, "ok", 2);
>>
>> (I do feel that if you do this sort of thing you end up with people
>> using 'ok' even in new fdts, since they look at code like this and
>> think it is fine)

That'd be buggy since it'd allow a lot more than ok/okay. A comment in
the code would avoid the issue.

-- 
nvpublic

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

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 2:25 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/05/2011 03:18 PM, Scott Wood wrote:
>> On 12/05/2011 04:11 PM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On Mon, Dec 5, 2011 at 2:07 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>> My point is that there are probably .dts files using "ok" instead of
>>>> "okay" or the kernel wouldn't support "ok". People will probably want to
>>>> use those with U-Boot without changing anything else. So, U-Boot should
>>>> interpret the FDT in the same way as the kernel.
>>
>> The kernel has to deal with real Open Firmware systems, some of which
>> pass buggy trees. ?U-Boot should not blindly imitate all of Linux's
>> workarounds.
>
> If it's certain that we'll never see anyone writing FDTs with "ok" in
> them instead of "okay" (because such FDTs were only autogenerated by
> Open Firmware), then I'm fine with the original code.
>
>>> OK, how about:
>>> ? ? ? ? ? ? ?return 0 == strncmp(cell, "ok", 2);
>>>
>>> (I do feel that if you do this sort of thing you end up with people
>>> using 'ok' even in new fdts, since they look at code like this and
>>> think it is fine)
>
> That'd be buggy since it'd allow a lot more than ok/okay. A comment in
> the code would avoid the issue.

Hmm, ok I will revert to "okay" and add a comment.

Regards,
Simon

>
> --
> nvpublic

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

[Simon Glass]

Add a function to look up 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, although there is no
actual boolean type in U-Boot.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Rename get_prop_len() to get_prop_check_min_len()

 include/fdtdec.h |   40 ++++++++++++++++++++++++++
 lib/fdtdec.c     |   83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 0 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 49dbac4..974520a 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -141,3 +141,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,
+		u32 *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 a0a34e4..8f972b7 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -148,3 +148,86 @@ 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_check_min_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,
+		u32 *array, int count)
+{
+	const u32 *cell;
+	int i, err = 0;
+
+	debug("%s: %s\n", __func__, prop_name);
+	cell = get_prop_check_min_len(blob, node, prop_name,
+				      sizeof(u32) * 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 v2 02/17] fdt: Add functions to access phandles, arrays and bools

[Stephen Warren]

On 12/02/2011 07:11 PM, Simon Glass wrote:
> Add a function to look up 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, although there is no
> actual boolean type in U-Boot.

> +/**
> + * 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;
> +}

I'm still slightly worried that this implementation may interpret the
FDT differently to the kernel. At least the code I've written for
boolean properties /only/ looks at the presence of the property, and
never at the contents even if there is one.

That said, the ePAPR specification does only say that boolean properties
/might/ have an empty value, thus implying that a non-zero length is
permissible. so, perhaps this is fine.

Actually no, I'm more than worried now I think it through. You'd argued
for being able to write 0/1 to the property so that U-Boot could modify
the value in-place without having to add/remove nodes to/from the FDT
image, but rather just change their content. However, if this modified
FDT is then passed to the kernel (rather than some other fresh FDT),
then it's imperative that U-Boot and the kernel represent boolean
properties in the exact same way. Hence, we either can't have U-Boot use
this edit-in-place optimization, or U-Boot needs some "cleanup" of the
FDT image before passing it to the kernel. However, the latter is
impossible, since by then since the boot-the-kernel code has no idea
whether a property is a boolean or not, and hence implementing such a
cleanup is impossible.

-- 
nvpublic

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

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 1:59 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/02/2011 07:11 PM, Simon Glass wrote:
>> Add a function to look up 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, although there is no
>> actual boolean type in U-Boot.
>
>> +/**
>> + * 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;
>> +}
>
> I'm still slightly worried that this implementation may interpret the
> FDT differently to the kernel. At least the code I've written for
> boolean properties /only/ looks at the presence of the property, and
> never at the contents even if there is one.
>
> That said, the ePAPR specification does only say that boolean properties
> /might/ have an empty value, thus implying that a non-zero length is
> permissible. so, perhaps this is fine.

OK

>
> Actually no, I'm more than worried now I think it through. You'd argued
> for being able to write 0/1 to the property so that U-Boot could modify
> the value in-place without having to add/remove nodes to/from the FDT
> image, but rather just change their content. However, if this modified
> FDT is then passed to the kernel (rather than some other fresh FDT),
> then it's imperative that U-Boot and the kernel represent boolean
> properties in the exact same way. Hence, we either can't have U-Boot use
> this edit-in-place optimization, or U-Boot needs some "cleanup" of the
> FDT image before passing it to the kernel. However, the latter is
> impossible, since by then since the boot-the-kernel code has no idea
> whether a property is a boolean or not, and hence implementing such a
> cleanup is impossible.

Hang on - this fdt cannot be passed to the kernel! It is for U-Boot's
use, for it's own configuration. There is no mechanism to fix up
U-Boot's internal fdt and pass it to the kernel. U-Boot scripts can't
even find its address!

That side of U-Boot is a complete different use of fdt and I have been
careful (so far) to keep them apart.

Regards,
Simon

>
> --
> nvpublic

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

[Stephen Warren]

On 12/05/2011 03:07 PM, Simon Glass wrote:
> On Mon, Dec 5, 2011 at 1:59 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>> +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;
>>> +}
>>
>> I'm still slightly worried that this implementation may interpret the
>> FDT differently to the kernel. At least the code I've written for
>> boolean properties /only/ looks at the presence of the property, and
>> never at the contents even if there is one.
>>
>> That said, the ePAPR specification does only say that boolean properties
>> /might/ have an empty value, thus implying that a non-zero length is
>> permissible. so, perhaps this is fine.
> 
> OK
> 
>>
>> Actually no, I'm more than worried now I think it through. You'd argued
>> for being able to write 0/1 to the property so that U-Boot could modify
>> the value in-place without having to add/remove nodes to/from the FDT
>> image, but rather just change their content. However, if this modified
>> FDT is then passed to the kernel (rather than some other fresh FDT),
>> then it's imperative that U-Boot and the kernel represent boolean
>> properties in the exact same way. Hence, we either can't have U-Boot use
>> this edit-in-place optimization, or U-Boot needs some "cleanup" of the
>> FDT image before passing it to the kernel. However, the latter is
>> impossible, since by then since the boot-the-kernel code has no idea
>> whether a property is a boolean or not, and hence implementing such a
>> cleanup is impossible.
> 
> Hang on - this fdt cannot be passed to the kernel! It is for U-Boot's
> use, for it's own configuration. There is no mechanism to fix up
> U-Boot's internal fdt and pass it to the kernel. U-Boot scripts can't
> even find its address!
> 
> That side of U-Boot is a complete different use of fdt and I have been
> careful (so far) to keep them apart.

Yes, that's certainly the way it is right now.

I'd heard some discussion on changing that, and allowing U-Boot to pass
its internal configuration FDT to the kernel instead of loading it from
disk a second time. However, it looks like that discussion was purely
internal to NVIDIA and might have been motivated by some confusion
rather than actual intent.

As such, my comments about fixing stuff up to pass to the kernel don't
apply.

Still, I think we should be able pass the same FDT to U-Boot and the
kernel, and as such they should both interpret boolean properties in the
same way. This patch doesn't do that.

-- 
nvpublic

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

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 2:36 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/05/2011 03:07 PM, Simon Glass wrote:
>> On Mon, Dec 5, 2011 at 1:59 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>>> +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;
>>>> +}
>>>
>>> I'm still slightly worried that this implementation may interpret the
>>> FDT differently to the kernel. At least the code I've written for
>>> boolean properties /only/ looks at the presence of the property, and
>>> never at the contents even if there is one.
>>>
>>> That said, the ePAPR specification does only say that boolean properties
>>> /might/ have an empty value, thus implying that a non-zero length is
>>> permissible. so, perhaps this is fine.
>>
>> OK
>>
>>>
>>> Actually no, I'm more than worried now I think it through. You'd argued
>>> for being able to write 0/1 to the property so that U-Boot could modify
>>> the value in-place without having to add/remove nodes to/from the FDT
>>> image, but rather just change their content. However, if this modified
>>> FDT is then passed to the kernel (rather than some other fresh FDT),
>>> then it's imperative that U-Boot and the kernel represent boolean
>>> properties in the exact same way. Hence, we either can't have U-Boot use
>>> this edit-in-place optimization, or U-Boot needs some "cleanup" of the
>>> FDT image before passing it to the kernel. However, the latter is
>>> impossible, since by then since the boot-the-kernel code has no idea
>>> whether a property is a boolean or not, and hence implementing such a
>>> cleanup is impossible.
>>
>> Hang on - this fdt cannot be passed to the kernel! It is for U-Boot's
>> use, for it's own configuration. There is no mechanism to fix up
>> U-Boot's internal fdt and pass it to the kernel. U-Boot scripts can't
>> even find its address!
>>
>> That side of U-Boot is a complete different use of fdt and I have been
>> careful (so far) to keep them apart.
>
> Yes, that's certainly the way it is right now.
>
> I'd heard some discussion on changing that, and allowing U-Boot to pass
> its internal configuration FDT to the kernel instead of loading it from
> disk a second time. However, it looks like that discussion was purely
> internal to NVIDIA and might have been motivated by some confusion
> rather than actual intent.
>
> As such, my comments about fixing stuff up to pass to the kernel don't
> apply.

OK

>
> Still, I think we should be able pass the same FDT to U-Boot and the
> kernel, and as such they should both interpret boolean properties in the
> same way. This patch doesn't do that.

OK, I will change it, and start a separate thread on
devicetree-discuss about fdtget/put.

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH v2 03/17] Add gpio_request() to asm-generic header

[Simon Glass]

This function should also be part of the GPIO API, so add it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- New patch to add gpio_request() to asm-generic header

 include/asm-generic/gpio.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index a1ebb28..c1d697f 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -72,3 +72,13 @@ int gpio_get_value(int gp);
  * @return 0 if ok, -1 on error
  */
 int gpio_set_value(int gp, int value);
+
+/**
+ * Request a gpio. This should be called before any of the other functions
+ * are used on this gpio.
+ *
+ * @param gp	GPIO number
+ * @param label	User label for this GPIO
+ * @return 0 if ok, -1 on error
+ */
+int gpio_request(unsigned gpio, const char *label);
-- 
1.7.3.1

[U-Boot] [PATCH v2 04/17] fdt: Add basic support for decoding GPIO definitions

[Simon Glass]

This adds some support into fdtdec for reading GPIO definitions from
the fdt. We permit up to FDT_GPIO_MAX GPIOs in the system. Each GPIO
is of the form:

gpio-function-name = <phandle gpio_num flags>;

where:

phandle is a pointer to the GPIO node
gpio_num is the number of the GPIO (0 to 223)
flags is some flags, proposed as follows:

   bit    meaning
   0      0=input, 1=output
   1      for output only: inital value of output
   2      0=polarity normal, 1=active low (inverted)

An example is:

enable-propounder = <&gpio 43 1>;

which means that GPIO 43 is an output and we can enable the propounder by
setting gpio 43 high.

Two main functions are provided:

fdtdec_decode_gpio() reads a GPIO property from an fdt node and decodes it
into a structure.

fdtdec_setup_gpio() sets up the GPIO by calling gpio_request and
gpio_direction_input/output() for you.

Both functions can cope with the property being missing, which is taken to
mean that that GPIO function is not available or is not needed.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 include/fdtdec.h |   49 ++++++++++++++++++++++++++++++++
 lib/fdtdec.c     |   83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+), 0 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 974520a..57db42b 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -61,6 +61,26 @@ enum fdt_compat_id {
 	COMPAT_COUNT,
 };
 
+/* For now we allow 224 GPIOs. We can extend this later if required */
+enum {
+	FDT_GPIO_NONE = 255,	/* an invalid GPIO used to end our list */
+	FDT_GPIO_MAX  = 224,	/* maximum valid GPIO number */
+
+	FDT_GPIO_OUTPUT	= 1 << 0,	/* set as output (else input) */
+	FDT_GPIO_HIGH	= 1 << 1,	/* set output as high (else low) */
+	FDT_GPIO_ACTIVE_LOW = 1 << 2,	/* input is active low (else high) */
+};
+
+/* This is the state of a GPIO pin as defined by the fdt */
+struct fdt_gpio_state {
+	const char *name;	/* name of the fdt property defining this */
+	u8 gpio;		/* GPIO number, or FDT_GPIO_NONE if none */
+	u8 flags;		/* FDT_GPIO_... flags */
+};
+
+/* This tells us whether a fdt_gpio_state record is valid or not */
+#define fdt_gpio_isvalid(x) ((x)->gpio != FDT_GPIO_NONE)
+
 /**
  * Find the next numbered alias for a peripheral. This is used to enumerate
  * all the peripherals of a certain type.
@@ -181,3 +201,32 @@ int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
  * @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);
+
+/**
+ * Decode a single GPIOs from an FDT.
+ *
+ * If the property is not found, then the GPIO structure will still be
+ * initialised, with gpio set to FDT_GPIO_NONE. This makes it easy to
+ * provide optional GPIOs.
+ *
+ * @param blob		FDT blob to use
+ * @param node		Node to look at
+ * @param prop_name	Node property name
+ * @param gpio		gpio elements to fill from FDT
+ * @return 0 if ok, -FDT_ERR_NOTFOUND if the property is missing.
+ */
+int fdtdec_decode_gpio(const void *blob, int node, const char *prop_name,
+		struct fdt_gpio_state *gpio);
+
+/**
+ * Set up a GPIO pin according to the provided gpio information. This sets it
+ * to either input or output. If an output, then the defined value is
+ * assigned.
+ *
+ * If the gpio is FDT_GPIO_NONE, no action is taken. This makes it easy to
+ * deal with optional GPIOs.
+ *
+ * @param gpio		GPIO info to use for set up
+ * @return 0 if all ok or gpio was FDT_GPIO_NONE; -1 on error
+ */
+int fdtdec_setup_gpio(struct fdt_gpio_state *gpio);
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 8f972b7..8eed752 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -24,6 +24,9 @@
 #include <libfdt.h>
 #include <fdtdec.h>
 
+/* we need the generic GPIO interface here */
+#include <asm-generic/gpio.h>
+
 DECLARE_GLOBAL_DATA_PTR;
 
 /*
@@ -231,3 +234,83 @@ int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
 
 	return 1;
 }
+
+/**
+ * Decode a list of GPIOs from an FDT. This creates a list of GPIOs with no
+ * terminating item.
+ *
+ * @param blob		FDT blob to use
+ * @param node		Node to look at
+ * @param prop_name	Node property name
+ * @param gpio		Array of gpio elements to fill from FDT. This will be
+ *			untouched if either 0 or an error is returned
+ * @param max_count	Maximum number of elements allowed
+ * @return number of GPIOs read if ok, -FDT_ERR_BADLAYOUT if max_count would
+ * be exceeded, or -FDT_ERR_NOTFOUND if the property is missing.
+ */
+static int fdtdec_decode_gpios(const void *blob, int node,
+		const char *prop_name, struct fdt_gpio_state *gpio,
+		int max_count)
+{
+	const struct fdt_property *prop;
+	const u32 *cell;
+	const char *name;
+	int len, i;
+
+	debug("%s: %s\n", __func__, prop_name);
+	assert(max_count > 0);
+	prop = fdt_get_property(blob, node, prop_name, &len);
+	if (!prop) {
+		debug("FDT: %s: property '%s' missing\n", __func__, prop_name);
+		return -FDT_ERR_NOTFOUND;
+	}
+
+	/* We will use the name to tag the GPIO */
+	name = fdt_string(blob, prop->nameoff);
+	cell = (u32 *)prop->data;
+	len /= sizeof(u32) * 3;		/* 3 cells per GPIO record */
+	if (len > max_count) {
+		debug("FDT: %s: too many GPIOs / cells for "
+			"property '%s'\n", __func__, prop_name);
+		return -FDT_ERR_BADLAYOUT;
+	}
+
+	/* Read out the GPIO data from the cells */
+	for (i = 0; i < len; i++, cell += 3) {
+		gpio[i].gpio = fdt32_to_cpu(cell[1]);
+		gpio[i].flags = fdt32_to_cpu(cell[2]);
+		gpio[i].name = name;
+	}
+
+	return len;
+}
+
+int fdtdec_decode_gpio(const void *blob, int node, const char *prop_name,
+		struct fdt_gpio_state *gpio)
+{
+	int err;
+
+	debug("%s: %s\n", __func__, prop_name);
+	gpio->gpio = FDT_GPIO_NONE;
+	err = fdtdec_decode_gpios(blob, node, prop_name, gpio, 1);
+	return err == 1 ? 0 : err;
+}
+
+int fdtdec_setup_gpio(struct fdt_gpio_state *gpio)
+{
+	/*
+	 * Return success if there is no GPIO defined. This is used for
+	 * optional GPIOs)
+	 */
+	if (!fdt_gpio_isvalid(gpio))
+		return 0;
+
+	if (gpio_request(gpio->gpio, gpio->name))
+		return -1;
+	if (gpio->flags & FDT_GPIO_OUTPUT) {
+		return gpio_direction_output(gpio->gpio,
+					     gpio->flags & FDT_GPIO_HIGH);
+	} else {
+		return gpio_direction_input(gpio->gpio);
+	}
+}
-- 
1.7.3.1

[U-Boot] [PATCH v2 04/17] fdt: Add basic support for decoding GPIO definitions

[Stephen Warren]

On 12/02/2011 07:11 PM, Simon Glass wrote:
> This adds some support into fdtdec for reading GPIO definitions from
> the fdt. We permit up to FDT_GPIO_MAX GPIOs in the system. Each GPIO
> is of the form:
> 
> gpio-function-name = <phandle gpio_num flags>;
> 
> where:
> 
> phandle is a pointer to the GPIO node
> gpio_num is the number of the GPIO (0 to 223)
> flags is some flags, proposed as follows:
> 
>    bit    meaning
>    0      0=input, 1=output
>    1      for output only: inital value of output
>    2      0=polarity normal, 1=active low (inverted)

The meaning of the flags (and even whether there are any flags any if so
how many cells there are to contain them) is defined by the GPIO
controller's binding. It's not something that can be interpreted in
isolation by a generic DT parsing function. See for example #gpio-cells
in tegra20.dtsi's gpio node and kernel file
Documentation/devicetree/bindings/gpio/gpio_nvidia.txt.

This implies that a lot of the code isn't correct, but I haven't
explicitly mentioned this everywhere for brevity.

> An example is:
> 
> enable-propounder = <&gpio 43 1>;

I /think/ convention is to name such properties foo-gpios rather than
just foo. Still, this is only a comment on the example and not the code,
since the complete property name is passed into the new functions by the
caller.

> +/* For now we allow 224 GPIOs. We can extend this later if required */
> +enum {
> +	FDT_GPIO_NONE = 255,	/* an invalid GPIO used to end our list */

Can't you use 0 for that? (the kernel currently uses -1, but it seems
there's agreement that was a mistake). If you use 255, the number will
have to keep getting bumped as more complex systems become supported. If
not 0, perhaps U32_MAX or whatever the relevant ${type}_MAX is?

-- 
nvpublic

[U-Boot] [PATCH v2 04/17] fdt: Add basic support for decoding GPIO definitions

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 1:46 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/02/2011 07:11 PM, Simon Glass wrote:
>> This adds some support into fdtdec for reading GPIO definitions from
>> the fdt. We permit up to FDT_GPIO_MAX GPIOs in the system. Each GPIO
>> is of the form:
>>
>> gpio-function-name = <phandle gpio_num flags>;
>>
>> where:
>>
>> phandle is a pointer to the GPIO node
>> gpio_num is the number of the GPIO (0 to 223)
>> flags is some flags, proposed as follows:
>>
>> ? ?bit ? ?meaning
>> ? ?0 ? ? ?0=input, 1=output
>> ? ?1 ? ? ?for output only: inital value of output
>> ? ?2 ? ? ?0=polarity normal, 1=active low (inverted)
>
> The meaning of the flags (and even whether there are any flags any if so
> how many cells there are to contain them) is defined by the GPIO
> controller's binding. It's not something that can be interpreted in
> isolation by a generic DT parsing function. See for example #gpio-cells
> in tegra20.dtsi's gpio node and kernel file
> Documentation/devicetree/bindings/gpio/gpio_nvidia.txt.

I see this in my version:

Required properties:
- compatible : "nvidia,tegra20-gpio"
- #gpio-cells : Should be two. The first cell is the pin number and the
  second cell is used to specify optional parameters:
  - bit 0 specifies polarity (0 for normal, 1 for inverted)
- gpio-controller : Marks the device node as a GPIO controller.

so how do I go about adding the other two bits?

>
> This implies that a lot of the code isn't correct, but I haven't
> explicitly mentioned this everywhere for brevity.

Well it's ok - these flags are only used in one place, and only the
input/output flag in fact.

It would be nice to use constants for these in the fdt instead of
numbers. Is that in progress?

enable-propounder = <&gpio 43 OUTPUT>;

>
>> An example is:
>>
>> enable-propounder = <&gpio 43 1>;
>
> I /think/ convention is to name such properties foo-gpios rather than
> just foo. Still, this is only a comment on the example and not the code,
> since the complete property name is passed into the new functions by the
> caller.

ok, will update.

>
>> +/* For now we allow 224 GPIOs. We can extend this later if required */
>> +enum {
>> + ? ? FDT_GPIO_NONE = 255, ? ?/* an invalid GPIO used to end our list */
>
> Can't you use 0 for that? (the kernel currently uses -1, but it seems
> there's agreement that was a mistake). If you use 255, the number will
> have to keep getting bumped as more complex systems become supported. If
> not 0, perhaps U32_MAX or whatever the relevant ${type}_MAX is?

But 0 is a valid GPIO isn't it?

I currently use the max value available to the u8. We can change it at
will when we update the u8 type to u16 which is why I made it a
constant.

Thanks for all your help with this.

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH v2 04/17] fdt: Add basic support for decoding GPIO definitions

[Stephen Warren]

On 12/05/2011 02:56 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Dec 5, 2011 at 1:46 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>> This adds some support into fdtdec for reading GPIO definitions from
>>> the fdt. We permit up to FDT_GPIO_MAX GPIOs in the system. Each GPIO
>>> is of the form:
>>>
>>> gpio-function-name = <phandle gpio_num flags>;
>>>
>>> where:
>>>
>>> phandle is a pointer to the GPIO node
>>> gpio_num is the number of the GPIO (0 to 223)
>>> flags is some flags, proposed as follows:
>>>
>>>    bit    meaning
>>>    0      0=input, 1=output
>>>    1      for output only: inital value of output
>>>    2      0=polarity normal, 1=active low (inverted)
>>
>> The meaning of the flags (and even whether there are any flags any if so
>> how many cells there are to contain them) is defined by the GPIO
>> controller's binding. It's not something that can be interpreted in
>> isolation by a generic DT parsing function. See for example #gpio-cells
>> in tegra20.dtsi's gpio node and kernel file
>> Documentation/devicetree/bindings/gpio/gpio_nvidia.txt.
> 
> I see this in my version:
> 
> Required properties:
> - compatible : "nvidia,tegra20-gpio"
> - #gpio-cells : Should be two. The first cell is the pin number and the
>   second cell is used to specify optional parameters:
>   - bit 0 specifies polarity (0 for normal, 1 for inverted)
> - gpio-controller : Marks the device node as a GPIO controller.
> 
> so how do I go about adding the other two bits?

I don't think you would. Input vs. output and output value are set up by
APIs such as gpio_direction_input/output based on what the driver wants
to do with the GPIOs.

>>> +/* For now we allow 224 GPIOs. We can extend this later if required */
>>> +enum {
>>> +     FDT_GPIO_NONE = 255,    /* an invalid GPIO used to end our list */
>>
>> Can't you use 0 for that? (the kernel currently uses -1, but it seems
>> there's agreement that was a mistake). If you use 255, the number will
>> have to keep getting bumped as more complex systems become supported. If
>> not 0, perhaps U32_MAX or whatever the relevant ${type}_MAX is?
> 
> But 0 is a valid GPIO isn't it?

Well, it depends how you define your numbering scheme. It may well be!

There are many ways of representing a GPIO:

* GPIO n on a specific controller (of which there may be many). This is
what DT GPIO bindings use.

* A system-wide GPIO ID, in which case the numbering is "virtual" (e.g.
a concatenation of the GPIOs on all the present controllers), and you
can choose to start the first controller's GPIOs at 0, 1, 1000 etc.,
thus leaving -1, 0, -n..999 etc. as invalid GPIOs. This is what the
Linux kernel's gpiolib uses (and some say this global numbering scheme
was a mistake).

> I currently use the max value available to the u8. We can change it at
> will when we update the u8 type to u16 which is why I made it a
> constant.

include/asm-generic/gpio.h seems to use an int to represent a GPIO. I'd
suggest these APIs do the same, rather than use a u8.

-- 
nvpublic

[U-Boot] [PATCH v2 04/17] fdt: Add basic support for decoding GPIO definitions

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 2:22 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/05/2011 02:56 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Dec 5, 2011 at 1:46 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>>> This adds some support into fdtdec for reading GPIO definitions from
>>>> the fdt. We permit up to FDT_GPIO_MAX GPIOs in the system. Each GPIO
>>>> is of the form:
>>>>
>>>> gpio-function-name = <phandle gpio_num flags>;
>>>>
>>>> where:
>>>>
>>>> phandle is a pointer to the GPIO node
>>>> gpio_num is the number of the GPIO (0 to 223)
>>>> flags is some flags, proposed as follows:
>>>>
>>>> ? ?bit ? ?meaning
>>>> ? ?0 ? ? ?0=input, 1=output
>>>> ? ?1 ? ? ?for output only: inital value of output
>>>> ? ?2 ? ? ?0=polarity normal, 1=active low (inverted)
>>>
>>> The meaning of the flags (and even whether there are any flags any if so
>>> how many cells there are to contain them) is defined by the GPIO
>>> controller's binding. It's not something that can be interpreted in
>>> isolation by a generic DT parsing function. See for example #gpio-cells
>>> in tegra20.dtsi's gpio node and kernel file
>>> Documentation/devicetree/bindings/gpio/gpio_nvidia.txt.
>>
>> I see this in my version:
>>
>> Required properties:
>> - compatible : "nvidia,tegra20-gpio"
>> - #gpio-cells : Should be two. The first cell is the pin number and the
>> ? second cell is used to specify optional parameters:
>> ? - bit 0 specifies polarity (0 for normal, 1 for inverted)
>> - gpio-controller : Marks the device node as a GPIO controller.
>>
>> so how do I go about adding the other two bits?
>
> I don't think you would. Input vs. output and output value are set up by
> APIs such as gpio_direction_input/output based on what the driver wants
> to do with the GPIOs.

Fair enough. I am wanting to create a way for more information to be
provided about a GPIO so that it can be set up automatically ready for
use (reduces code size).

>
>>>> +/* For now we allow 224 GPIOs. We can extend this later if required */
>>>> +enum {
>>>> + ? ? FDT_GPIO_NONE = 255, ? ?/* an invalid GPIO used to end our list */
>>>
>>> Can't you use 0 for that? (the kernel currently uses -1, but it seems
>>> there's agreement that was a mistake). If you use 255, the number will
>>> have to keep getting bumped as more complex systems become supported. If
>>> not 0, perhaps U32_MAX or whatever the relevant ${type}_MAX is?
>>
>> But 0 is a valid GPIO isn't it?
>
> Well, it depends how you define your numbering scheme. It may well be!
>
> There are many ways of representing a GPIO:
>
> * GPIO n on a specific controller (of which there may be many). This is
> what DT GPIO bindings use.
>
> * A system-wide GPIO ID, in which case the numbering is "virtual" (e.g.
> a concatenation of the GPIOs on all the present controllers), and you
> can choose to start the first controller's GPIOs at 0, 1, 1000 etc.,
> thus leaving -1, 0, -n..999 etc. as invalid GPIOs. This is what the
> Linux kernel's gpiolib uses (and some say this global numbering scheme
> was a mistake).

Well maybe it was a mistake, but it seems painful for the user to
translate GPIO numbers in this way. U-Boot's GPIO command takes a GPIO
number, which starts at zero.

>
>> I currently use the max value available to the u8. We can change it at
>> will when we update the u8 type to u16 which is why I made it a
>> constant.
>
> include/asm-generic/gpio.h seems to use an int to represent a GPIO. I'd
> suggest these APIs do the same, rather than use a u8.

Do you mean the fdt_gpio_state structure? I have not used u8 for any
function calls and would not.

This adds 3 bytes for every entry. What is the benefit? People get
upset when we waste memory!

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH v2 04/17] fdt: Add basic support for decoding GPIO definitions

[Stephen Warren]

On 12/05/2011 03:52 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Dec 5, 2011 at 2:22 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/05/2011 02:56 PM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On Mon, Dec 5, 2011 at 1:46 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>>>> This adds some support into fdtdec for reading GPIO definitions from
>>>>> the fdt. We permit up to FDT_GPIO_MAX GPIOs in the system. Each GPIO
>>>>> is of the form:
>>>>>
>>>>> gpio-function-name = <phandle gpio_num flags>;
>>>>>
>>>>> where:
>>>>>
>>>>> phandle is a pointer to the GPIO node
>>>>> gpio_num is the number of the GPIO (0 to 223)
>>>>> flags is some flags, proposed as follows:
>>>>>
>>>>>    bit    meaning
>>>>>    0      0=input, 1=output
>>>>>    1      for output only: inital value of output
>>>>>    2      0=polarity normal, 1=active low (inverted)
>>>>
>>>> The meaning of the flags (and even whether there are any flags any if so
>>>> how many cells there are to contain them) is defined by the GPIO
>>>> controller's binding. It's not something that can be interpreted in
>>>> isolation by a generic DT parsing function. See for example #gpio-cells
>>>> in tegra20.dtsi's gpio node and kernel file
>>>> Documentation/devicetree/bindings/gpio/gpio_nvidia.txt.
>>>
>>> I see this in my version:
>>>
>>> Required properties:
>>> - compatible : "nvidia,tegra20-gpio"
>>> - #gpio-cells : Should be two. The first cell is the pin number and the
>>>   second cell is used to specify optional parameters:
>>>   - bit 0 specifies polarity (0 for normal, 1 for inverted)
>>> - gpio-controller : Marks the device node as a GPIO controller.
>>>
>>> so how do I go about adding the other two bits?
>>
>> I don't think you would. Input vs. output and output value are set up by
>> APIs such as gpio_direction_input/output based on what the driver wants
>> to do with the GPIOs.
> 
> Fair enough. I am wanting to create a way for more information to be
> provided about a GPIO so that it can be set up automatically ready for
> use (reduces code size).

At least in this case, I don't think it makes sense to do that. The FDT
is about representing that a particular GPIO is a VBUS GPIO. That
doesn't mean the GPIO /has/ to be an output driven high; that's only
true if the driver is enabled and chooses to configure that port as a
host port, not a device port.

If you wanted to represent GPIOs that were always configured to a
specific output value in DT, I think that'd be an unrelated binding
somewhere other than the USB bus's vbus-gpios property, since it'd have
a completely different semantic meaning.

>> include/asm-generic/gpio.h seems to use an int to represent a GPIO. I'd
>> suggest these APIs do the same, rather than use a u8.
> 
> Do you mean the fdt_gpio_state structure?

Yes.

> I have not used u8 for any
> function calls and would not.
> 
> This adds 3 bytes for every entry. What is the benefit? People get
> upset when we waste memory!

Well, U-boot has already chosen to use an int to represent a GPIO ID.
Given that, I assert that all places that store a GPIO ID should use the
same type. And realistically, we're only talking about a handful of
instances here, and any bloat is completely limited to those platforms
that use this feature, and linear with the number of GPIOs.

-- 
nvpublic

[U-Boot] [PATCH v2 04/17] fdt: Add basic support for decoding GPIO definitions

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 3:03 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/05/2011 03:52 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Dec 5, 2011 at 2:22 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/05/2011 02:56 PM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On Mon, Dec 5, 2011 at 1:46 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>>>>> This adds some support into fdtdec for reading GPIO definitions from
>>>>>> the fdt. We permit up to FDT_GPIO_MAX GPIOs in the system. Each GPIO
>>>>>> is of the form:
>>>>>>
>>>>>> gpio-function-name = <phandle gpio_num flags>;
>>>>>>
>>>>>> where:
>>>>>>
>>>>>> phandle is a pointer to the GPIO node
>>>>>> gpio_num is the number of the GPIO (0 to 223)
>>>>>> flags is some flags, proposed as follows:
>>>>>>
>>>>>> ? ?bit ? ?meaning
>>>>>> ? ?0 ? ? ?0=input, 1=output
>>>>>> ? ?1 ? ? ?for output only: inital value of output
>>>>>> ? ?2 ? ? ?0=polarity normal, 1=active low (inverted)
>>>>>
>>>>> The meaning of the flags (and even whether there are any flags any if so
>>>>> how many cells there are to contain them) is defined by the GPIO
>>>>> controller's binding. It's not something that can be interpreted in
>>>>> isolation by a generic DT parsing function. See for example #gpio-cells
>>>>> in tegra20.dtsi's gpio node and kernel file
>>>>> Documentation/devicetree/bindings/gpio/gpio_nvidia.txt.
>>>>
>>>> I see this in my version:
>>>>
>>>> Required properties:
>>>> - compatible : "nvidia,tegra20-gpio"
>>>> - #gpio-cells : Should be two. The first cell is the pin number and the
>>>> ? second cell is used to specify optional parameters:
>>>> ? - bit 0 specifies polarity (0 for normal, 1 for inverted)
>>>> - gpio-controller : Marks the device node as a GPIO controller.
>>>>
>>>> so how do I go about adding the other two bits?
>>>
>>> I don't think you would. Input vs. output and output value are set up by
>>> APIs such as gpio_direction_input/output based on what the driver wants
>>> to do with the GPIOs.
>>
>> Fair enough. I am wanting to create a way for more information to be
>> provided about a GPIO so that it can be set up automatically ready for
>> use (reduces code size).
>
> At least in this case, I don't think it makes sense to do that. The FDT
> is about representing that a particular GPIO is a VBUS GPIO. That
> doesn't mean the GPIO /has/ to be an output driven high; that's only
> true if the driver is enabled and chooses to configure that port as a
> host port, not a device port.
>
> If you wanted to represent GPIOs that were always configured to a
> specific output value in DT, I think that'd be an unrelated binding
> somewhere other than the USB bus's vbus-gpios property, since it'd have
> a completely different semantic meaning.

I feel that it is useful to have a generic gpio setup function which
can at least set gpio to input or output. We could even make it
optional with an additional flag. IMO a lot of the reason for only
having one flag is that no one can OR together two decimal numbers
(i.e. we need symbolic constants in the fdt).

But for now I will drop this comment and the fdtdec_setup_gpio()
function. This will increase code size slightly since every driver
will need to call gpio_direction_input/output() manually.

>
>>> include/asm-generic/gpio.h seems to use an int to represent a GPIO. I'd
>>> suggest these APIs do the same, rather than use a u8.
>>
>> Do you mean the fdt_gpio_state structure?
>
> Yes.
>
>> I have not used u8 for any
>> function calls and would not.
>>
>> This adds 3 bytes for every entry. What is the benefit? People get
>> upset when we waste memory!
>
> Well, U-boot has already chosen to use an int to represent a GPIO ID.
> Given that, I assert that all places that store a GPIO ID should use the
> same type. And realistically, we're only talking about a handful of
> instances here, and any bloat is completely limited to those platforms
> that use this feature, and linear with the number of GPIOs.

OK I have changed this. Each structure element is now 4 bytes (50%)
bigger than before but I don't think this will add up to more than a
few hundred bytes all up.

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH v2 04/17] fdt: Add basic support for decoding GPIO definitions

[Mike Frysinger]

On Monday 05 December 2011 17:52:01 Simon Glass wrote:
> On Mon, Dec 5, 2011 at 2:22 PM, Stephen Warren wrote:
> > On 12/05/2011 02:56 PM, Simon Glass wrote:
> > * A system-wide GPIO ID, in which case the numbering is "virtual" (e.g.
> > a concatenation of the GPIOs on all the present controllers), and you
> > can choose to start the first controller's GPIOs at 0, 1, 1000 etc.,
> > thus leaving -1, 0, -n..999 etc. as invalid GPIOs. This is what the
> > Linux kernel's gpiolib uses (and some say this global numbering scheme
> > was a mistake).
> 
> Well maybe it was a mistake, but it seems painful for the user to
> translate GPIO numbers in this way. U-Boot's GPIO command takes a GPIO
> number, which starts at zero.

u-boot should simply follow what linux is doing as it keeps things a hell of a 
lot simpler for us: the code is done, people moving between the worlds don't 
get surprised and screw something up, and we don't accidentally forget "oh, in 
u-boot we do XXX instead of YYY" and screw things up ourselves.

> >> I currently use the max value available to the u8. We can change it at
> >> will when we update the u8 type to u16 which is why I made it a
> >> constant.
> > 
> > include/asm-generic/gpio.h seems to use an int to represent a GPIO. I'd
> > suggest these APIs do the same, rather than use a u8.
> 
> Do you mean the fdt_gpio_state structure? I have not used u8 for any
> function calls and would not.

the asm-generic/gpio.h using "int" as a gpio is wrong.  it should be 
"unsigned".  there's a patch somewhere to fix this.

> This adds 3 bytes for every entry. What is the benefit? People get
> upset when we waste memory!

some systems have more than 256 GPIOs.  it's actually not that hard to hit the 
limit.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111205/fa62d320/attachment.pgp>

[U-Boot] [PATCH v2 04/17] fdt: Add basic support for decoding GPIO definitions

[Simon Glass]

Hi Mike,

On Mon, Dec 5, 2011 at 7:55 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday 05 December 2011 17:52:01 Simon Glass wrote:
>> On Mon, Dec 5, 2011 at 2:22 PM, Stephen Warren wrote:
>> > On 12/05/2011 02:56 PM, Simon Glass wrote:
>> > * A system-wide GPIO ID, in which case the numbering is "virtual" (e.g.
>> > a concatenation of the GPIOs on all the present controllers), and you
>> > can choose to start the first controller's GPIOs at 0, 1, 1000 etc.,
>> > thus leaving -1, 0, -n..999 etc. as invalid GPIOs. This is what the
>> > Linux kernel's gpiolib uses (and some say this global numbering scheme
>> > was a mistake).
>>
>> Well maybe it was a mistake, but it seems painful for the user to
>> translate GPIO numbers in this way. U-Boot's GPIO command takes a GPIO
>> number, which starts at zero.
>
> u-boot should simply follow what linux is doing as it keeps things a hell of a
> lot simpler for us: the code is done, people moving between the worlds don't
> get surprised and screw something up, and we don't accidentally forget "oh, in
> u-boot we do XXX instead of YYY" and screw things up ourselves.

Good, I think that is what we are doing. But if Linux changes the GPIO
numbering we may need to do something.

>
>> >> I currently use the max value available to the u8. We can change it at
>> >> will when we update the u8 type to u16 which is why I made it a
>> >> constant.
>> >
>> > include/asm-generic/gpio.h seems to use an int to represent a GPIO. I'd
>> > suggest these APIs do the same, rather than use a u8.
>>
>> Do you mean the fdt_gpio_state structure? I have not used u8 for any
>> function calls and would not.
>
> the asm-generic/gpio.h using "int" as a gpio is wrong. ?it should be
> "unsigned". ?there's a patch somewhere to fix this.

OK, will wait to see it.

>
>> This adds 3 bytes for every entry. What is the benefit? People get
>> upset when we waste memory!
>
> some systems have more than 256 GPIOs. ?it's actually not that hard to hit the
> limit.
> -mike

Not for me so far, but I don't doubt it. I have changed it to uint in
the series.

Regards,
Simon

[U-Boot] [PATCH v2 05/17] 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 v2 06/17] arm: fdt: Add skeleton device tree file from kernel

[Simon Glass]

This was taken from commit b48c54e2 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

[U-Boot] [PATCH v2 07/17] tegra: fdt: Add Tegra2x device tree file from kernel

[Simon Glass]

This was taken from commit b48c54e2 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           |  169 +++++++++++++++++++++++++++++++++++
 2 files changed, 171 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..6146d24
--- /dev/null
+++ b/arch/arm/dts/tegra20.dtsi
@@ -0,0 +1,169 @@
+/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 >;
+	};
+
+	usb at c5000000 {
+		compatible = "nvidia,tegra20-ehci", "usb-ehci";
+		reg = <0xc5000000 0x4000>;
+		interrupts = < 52 >;
+		phy_type = "utmi";
+	};
+
+	usb at c5004000 {
+		compatible = "nvidia,tegra20-ehci", "usb-ehci";
+		reg = <0xc5004000 0x4000>;
+		interrupts = < 53 >;
+		phy_type = "ulpi";
+	};
+
+	usb at c5008000 {
+		compatible = "nvidia,tegra20-ehci", "usb-ehci";
+		reg = <0xc5008000 0x4000>;
+		interrupts = < 129 >;
+		phy_type = "utmi";
+	};
+
+};
+
-- 
1.7.3.1

[U-Boot] [PATCH v2 08/17] tegra: fdt: Add device tree file for Tegra2 Seaboard from kernel

[Simon Glass]

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

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Pulled USB fdt definitions from newer for-next instead of master

 board/nvidia/dts/tegra2-seaboard.dts |   36 ++++++++++++++++++++++++++++++++++
 1 files changed, 36 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..dde5d03
--- /dev/null
+++ b/board/nvidia/dts/tegra2-seaboard.dts
@@ -0,0 +1,36 @@
+/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;
+	};
+
+	usb at c5000000 {
+		nvidia,vbus-gpio = <&gpio 24 0>; /* PD0 */
+	};
+};
-- 
1.7.3.1

[U-Boot] [PATCH v2 09/17] tegra: usb: fdt: Add additional device tree definitions for USB ports

[Simon Glass]

This adds peripheral IDs and timing information to the USB part of the
device tree for U-Boot.

The peripheral IDs provide easy access to clock registers. We will likely
remove this in favor of a full clock tree when it is available in the
kernel (but probably still retain the peripheral ID, just move it into
a clock node).

The USB timing information does apparently vary between boards sometimes,
so is include in the fdt for convenience.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Change fdt property usbparmas to usbtiming
- Add details for port USB2

 arch/arm/dts/tegra20.dtsi |   69 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index 6146d24..d7ab843 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -144,11 +144,78 @@
 		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
+ */
+	usbtiming at 0 {
+		compatible = "nvidia,tegra20-usbtiming";
+		osc-frequency = <13000000>;
+		/* DivN, DivM, DivP, CPCON, LFCON, Delays     Debounce, Bias */
+		timing = <0x3c0 0x0d 0x00 0xc 0  0x02 0x33 0x05 0x7f 0x7ef4 5>;
+	};
+
+	usbtiming at 1 {
+		compatible = "nvidia,tegra20-usbtiming";
+		osc-frequency = <19200000>;
+		timing = <0x0c8 0x04 0x00 0x3 0  0x03 0x4b 0x06 0xbb 0xbb80 7>;
+	};
+
+	usbtiming at 2 {
+		compatible = "nvidia,tegra20-usbtiming";
+		osc-frequency = <12000000>;
+		timing = <0x3c0 0x0c 0x00 0xc 0  0x02 0x2f 0x04 0x76 0x7530 5>;
+	};
+
+	usbtiming at 3 {
+		compatible = "nvidia,tegra20-usbtiming";
+		osc-frequency = <26000000>;
+		timing = <0x3c0 0x1a 0x00 0xc 0  0x04 0x66 0x09 0xfe 0xfde8 9>;
+	};
+
 	usb at c5000000 {
 		compatible = "nvidia,tegra20-ehci", "usb-ehci";
 		reg = <0xc5000000 0x4000>;
 		interrupts = < 52 >;
 		phy_type = "utmi";
+		periph-id = <22>;	// PERIPH_ID_USBD
 	};
 
 	usb at c5004000 {
@@ -156,6 +223,7 @@
 		reg = <0xc5004000 0x4000>;
 		interrupts = < 53 >;
 		phy_type = "ulpi";
+		periph-id = <58>;	// PERIPH_ID_USB2
 	};
 
 	usb at c5008000 {
@@ -163,6 +231,7 @@
 		reg = <0xc5008000 0x4000>;
 		interrupts = < 129 >;
 		phy_type = "utmi";
+		periph-id = <59>;	// PERIPH_ID_USB3
 	};
 
 };
-- 
1.7.3.1

[U-Boot] [PATCH v2 09/17] tegra: usb: fdt: Add additional device tree definitions for USB ports

[Stephen Warren]

On 12/02/2011 07:11 PM, Simon Glass wrote:
> This adds peripheral IDs and timing information to the USB part of the
> device tree for U-Boot.
> 
> The peripheral IDs provide easy access to clock registers. We will likely
> remove this in favor of a full clock tree when it is available in the
> kernel (but probably still retain the peripheral ID, just move it into
> a clock node).
> 
> The USB timing information does apparently vary between boards sometimes,
> so is include in the fdt for convenience.

Which parts vary? Most of it is PLL configuration, and it seems
basically impossible for that to vary yet still create the correct
output frequency.

> +/* This table has USB timing parameters for each Oscillator frequency we

This comment block should be indented to match the nodes.

> + * 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)

Those two sets of data seem like they should simply be part of the clock
driver. Some part of the system boot or USB init process needs to say
"enable the USB clocks", and that code needs to write those hard-coded
values into the relevant clock registers (or calculate them at run-time;
whatever). This stuff can't vary by board.

> + * 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.

      ^ typo

> + *  Reference frequency     13.0MHz      19.2MHz      12.0MHz      26.0MHz

If this data varies at all, then those two sets of data seem
port-specific to me, and hence should be moved into a per-port property.
Having a single global set of parameters that gets applied to all ports
at once doesn't make sense to me, if the data varies.

I imagine the values are board specific too, and hence shouldn't be in
tegra20.dtsi but rather in tegra-seaboard.dts.

The values in these fields should be specific in a way that's agnostic
to the reference clock, e.g. as a number of mS/nS. That way, you'd just
specify the single set of values to use, and the driver would do the
calculations to map the time domain into the reference-clock-specific
values to put into the registers.

> + */
> +
> +	usbtiming at 0 {
> +		compatible = "nvidia,tegra20-usbtiming";
> +		osc-frequency = <13000000>;
> +		/* DivN, DivM, DivP, CPCON, LFCON, Delays     Debounce, Bias */
> +		timing = <0x3c0 0x0d 0x00 0xc 0  0x02 0x33 0x05 0x7f 0x7ef4 5>;
> +	};
> +
> +	usbtiming at 1 {
> +		compatible = "nvidia,tegra20-usbtiming";
> +		osc-frequency = <19200000>;
> +		timing = <0x0c8 0x04 0x00 0x3 0  0x03 0x4b 0x06 0xbb 0xbb80 7>;
> +	};
> +
> +	usbtiming at 2 {
> +		compatible = "nvidia,tegra20-usbtiming";
> +		osc-frequency = <12000000>;
> +		timing = <0x3c0 0x0c 0x00 0xc 0  0x02 0x2f 0x04 0x76 0x7530 5>;
> +	};
> +
> +	usbtiming at 3 {
> +		compatible = "nvidia,tegra20-usbtiming";
> +		osc-frequency = <26000000>;
> +		timing = <0x3c0 0x1a 0x00 0xc 0  0x04 0x66 0x09 0xfe 0xfde8 9>;
> +	};

You can't include those timing nodes right there, because the unit
addresses (@0, ... @3) aren't physical addresses in the same scheme as
all the other nodes.

> +
>  	usb at c5000000 {
>  		compatible = "nvidia,tegra20-ehci", "usb-ehci";
>  		reg = <0xc5000000 0x4000>;
>  		interrupts = < 52 >;
>  		phy_type = "utmi";
> +		periph-id = <22>;	// PERIPH_ID_USBD

Given this is a temporary U-Boot-specific solution, can the property be
named u-boot,periph-id so it's obvious that when writing a .dts for the
kernel only, you don't care about this value.

Personally, I'd suggest not having this property at all, but rather have
the driver maintain a base_address -> periph_id mapping table
internally. That table can be removed once the clock/reset bindings are
all set up, and the information can be obtained there instead.

-- 
nvpublic

[U-Boot] [PATCH v2 09/17] tegra: usb: fdt: Add additional device tree definitions for USB ports

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 3:25 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/02/2011 07:11 PM, Simon Glass wrote:
>> This adds peripheral IDs and timing information to the USB part of the
>> device tree for U-Boot.
>>
>> The peripheral IDs provide easy access to clock registers. We will likely
>> remove this in favor of a full clock tree when it is available in the
>> kernel (but probably still retain the peripheral ID, just move it into
>> a clock node).
>>
>> The USB timing information does apparently vary between boards sometimes,
>> so is include in the fdt for convenience.
>
> Which parts vary? Most of it is PLL configuration, and it seems
> basically impossible for that to vary yet still create the correct
> output frequency.

See here for T30 differences, for example.

https://gerrit.chromium.org/gerrit/#patch,sidebyside,12440,1,board/nvidia/cardhu/tegra30.dtsi

>
>> +/* This table has USB timing parameters for each Oscillator frequency we
>
> This comment block should be indented to match the nodes.
>
>> + * 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)
>
> Those two sets of data seem like they should simply be part of the clock
> driver. Some part of the system boot or USB init process needs to say
> "enable the USB clocks", and that code needs to write those hard-coded
> values into the relevant clock registers (or calculate them at run-time;
> whatever). This stuff can't vary by board.

We don't have a USB clock driver. This driver is in CPU code, so I am
not suggesting that it varies by board, only by SOC. This stuff is in
the SOC dts.
>
>> + * 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.
>
> ? ? ?^ typo
>
>> + * ?Reference frequency ? ? 13.0MHz ? ? ?19.2MHz ? ? ?12.0MHz ? ? ?26.0MHz
>
> If this data varies at all, then those two sets of data seem
> port-specific to me, and hence should be moved into a per-port property.
> Having a single global set of parameters that gets applied to all ports
> at once doesn't make sense to me, if the data varies.

The hardware doesn't support this does it?

>
> I imagine the values are board specific too, and hence shouldn't be in
> tegra20.dtsi but rather in tegra-seaboard.dts.

I believe they vary by SOC, not board.

>
> The values in these fields should be specific in a way that's agnostic
> to the reference clock, e.g. as a number of mS/nS. That way, you'd just
> specify the single set of values to use, and the driver would do the
> calculations to map the time domain into the reference-clock-specific
> values to put into the registers.

Yes I agree. I don't have time to write this. Shall we go forward with
what we have, and Nvidia can tidy up later?

>
>> + */
>> +
>> + ? ? usbtiming at 0 {
>> + ? ? ? ? ? ? compatible = "nvidia,tegra20-usbtiming";
>> + ? ? ? ? ? ? osc-frequency = <13000000>;
>> + ? ? ? ? ? ? /* DivN, DivM, DivP, CPCON, LFCON, Delays ? ? Debounce, Bias */
>> + ? ? ? ? ? ? timing = <0x3c0 0x0d 0x00 0xc 0 ?0x02 0x33 0x05 0x7f 0x7ef4 5>;
>> + ? ? };
>> +
>> + ? ? usbtiming at 1 {
>> + ? ? ? ? ? ? compatible = "nvidia,tegra20-usbtiming";
>> + ? ? ? ? ? ? osc-frequency = <19200000>;
>> + ? ? ? ? ? ? timing = <0x0c8 0x04 0x00 0x3 0 ?0x03 0x4b 0x06 0xbb 0xbb80 7>;
>> + ? ? };
>> +
>> + ? ? usbtiming at 2 {
>> + ? ? ? ? ? ? compatible = "nvidia,tegra20-usbtiming";
>> + ? ? ? ? ? ? osc-frequency = <12000000>;
>> + ? ? ? ? ? ? timing = <0x3c0 0x0c 0x00 0xc 0 ?0x02 0x2f 0x04 0x76 0x7530 5>;
>> + ? ? };
>> +
>> + ? ? usbtiming at 3 {
>> + ? ? ? ? ? ? compatible = "nvidia,tegra20-usbtiming";
>> + ? ? ? ? ? ? osc-frequency = <26000000>;
>> + ? ? ? ? ? ? timing = <0x3c0 0x1a 0x00 0xc 0 ?0x04 0x66 0x09 0xfe 0xfde8 9>;
>> + ? ? };
>
> You can't include those timing nodes right there, because the unit
> addresses (@0, ... @3) aren't physical addresses in the same scheme as
> all the other nodes.

Shall I put them inside some other 'timing' node? Bear in mind we
don't have clock stuff yet.

>
>> +
>> ? ? ? usb at c5000000 {
>> ? ? ? ? ? ? ? compatible = "nvidia,tegra20-ehci", "usb-ehci";
>> ? ? ? ? ? ? ? reg = <0xc5000000 0x4000>;
>> ? ? ? ? ? ? ? interrupts = < 52 >;
>> ? ? ? ? ? ? ? phy_type = "utmi";
>> + ? ? ? ? ? ? periph-id = <22>; ? ? ? // PERIPH_ID_USBD
>
> Given this is a temporary U-Boot-specific solution, can the property be
> named u-boot,periph-id so it's obvious that when writing a .dts for the
> kernel only, you don't care about this value.

ok. I suggest the kernel does something similar.

>
> Personally, I'd suggest not having this property at all, but rather have
> the driver maintain a base_address -> periph_id mapping table
> internally. That table can be removed once the clock/reset bindings are
> all set up, and the information can be obtained there instead.

I really want to avoid pushing SOC things into the drivers! The SOC
code provides clock/reset functions. It seems useful to have a way of
identifying all the peripherals in the SOC by number.

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH v2 09/17] tegra: usb: fdt: Add additional device tree definitions for USB ports

[Stephen Warren]

On 12/05/2011 05:55 PM, Simon Glass wrote:
> On Mon, Dec 5, 2011 at 3:25 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>> This adds peripheral IDs and timing information to the USB part of the
>>> device tree for U-Boot.
>>>
>>> The peripheral IDs provide easy access to clock registers. We will likely
>>> remove this in favor of a full clock tree when it is available in the
>>> kernel (but probably still retain the peripheral ID, just move it into
>>> a clock node).
>>>
>>> The USB timing information does apparently vary between boards sometimes,
>>> so is include in the fdt for convenience.
>>
>> Which parts vary? Most of it is PLL configuration, and it seems
>> basically impossible for that to vary yet still create the correct
>> output frequency.
> 
> See here for T30 differences, for example.
> 
> https://gerrit.chromium.org/gerrit/#patch,sidebyside,12440,1,board/nvidia/cardhu/tegra30.dtsi

OK, so that varies by SoC not board.

>>> +/* 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)
>>
>> Those two sets of data seem like they should simply be part of the clock
>> driver. Some part of the system boot or USB init process needs to say
>> "enable the USB clocks", and that code needs to write those hard-coded
>> values into the relevant clock registers (or calculate them at run-time;
>> whatever). This stuff can't vary by board.
> 
> We don't have a USB clock driver. This driver is in CPU code, so I am
> not suggesting that it varies by board, only by SOC. This stuff is in
> the SOC dts.
>>
>>> + * 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.
>>
>>      ^ typo
>>
>>> + *  Reference frequency     13.0MHz      19.2MHz      12.0MHz      26.0MHz
>>
>> If this data varies at all, then those two sets of data seem
>> port-specific to me, and hence should be moved into a per-port property.
>> Having a single global set of parameters that gets applied to all ports
>> at once doesn't make sense to me, if the data varies.
> 
> The hardware doesn't support this does it?

It looks like it doesn't; there are two debounce count registers and
each port (or something) can select which of those two debounce values
is used.

>> I imagine the values are board specific too, and hence shouldn't be in
>> tegra20.dtsi but rather in tegra-seaboard.dts.
> 
> I believe they vary by SOC, not board.

>> The values in these fields should be specific in a way that's agnostic
>> to the reference clock, e.g. as a number of mS/nS. That way, you'd just
>> specify the single set of values to use, and the driver would do the
>> calculations to map the time domain into the reference-clock-specific
>> values to put into the registers.
> 
> Yes I agree. I don't have time to write this. Shall we go forward with
> what we have, and Nvidia can tidy up later?

I definitely oppose that; the current bindings are simply not
conceptually correct. I'm opposed to merging something that's known to
be broken even if there's a clear intent/timeframe to fix it; it'd be
better to just defer merging it. (Well, even better to fix it to be
right an merge it!)

As a blanket statement, the USB driver shouldn't programming PLLs. This
applies whether the PLL configuration is hard-coded in the driver, is
parsed from DT, or whatever. We should definitely remove all the PLL
related stuff from the DT bindings, and have some core or board specific
code set up the PLLs. That code can be replaced by DT clock parsing when
its ready. The USB driver shouldn't be impacted by that change at all.

Re: the debounce values: The kernel doesn't touch those registers as far
as I can tell and everything works. I'd suggest completely removing that
from the DT bindings for now. It's quite possible it only works because
the HW register defaults are correct (or at least work OK) for a 12MHz
reference crystal, and all boards supported by the kernel just happen to
use that reference frequency, but I think the same is probably true for
(Tegra boards in) U-Boot right now too.

Taking that approach, you can completely remove the contentious parts of
the DT binding and defer the problem until more infra-structure is in
place to support those features.

>>> +
>>>       usb at c5000000 {
>>>               compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>>               reg = <0xc5000000 0x4000>;
>>>               interrupts = < 52 >;
>>>               phy_type = "utmi";
>>> +             periph-id = <22>;       // PERIPH_ID_USBD
>>
>> Given this is a temporary U-Boot-specific solution, can the property be
>> named u-boot,periph-id so it's obvious that when writing a .dts for the
>> kernel only, you don't care about this value.
> 
> ok. I suggest the kernel does something similar.

The kernel will use the standardized clock bindings once they're ready
and we convert Tegra over to use them. The kernel is extremely unlikely
to ever use "periph-id" or "u-boot,periph-id".

Right now, the kernel's clock driver contains a mapping table from
device name (e.g. tegra-ehci.2) to clock name (e.g. usb3). This allows
the kernel USB driver to work without any explicit periph-id or similar
DT property.

I'd strongly suggest that's the cleanest transition plan for U-Boot
until the clock bindings are complete, and they are added to
tegra20.dtsi, and U-Boot can parse them, since it avoids temporarily
defining DT properties to WAR the issue, and then yanking them out
later. This mapping would be a part of the core Tegra SoC code,
completely isolated from the USB driver.

-- 
nvpublic

[U-Boot] [PATCH v2 09/17] tegra: usb: fdt: Add additional device tree definitions for USB ports

[Simon Glass]

Hi Stephen,

On Tue, Dec 6, 2011 at 12:28 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/05/2011 05:55 PM, Simon Glass wrote:
>> On Mon, Dec 5, 2011 at 3:25 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>>> This adds peripheral IDs and timing information to the USB part of the
>>>> device tree for U-Boot.
>>>>
>>>> The peripheral IDs provide easy access to clock registers. We will likely
>>>> remove this in favor of a full clock tree when it is available in the
>>>> kernel (but probably still retain the peripheral ID, just move it into
>>>> a clock node).
>>>>
>>>> The USB timing information does apparently vary between boards sometimes,
>>>> so is include in the fdt for convenience.
>>>
>>> Which parts vary? Most of it is PLL configuration, and it seems
>>> basically impossible for that to vary yet still create the correct
>>> output frequency.
>>
>> See here for T30 differences, for example.
>>
>> https://gerrit.chromium.org/gerrit/#patch,sidebyside,12440,1,board/nvidia/cardhu/tegra30.dtsi
>
> OK, so that varies by SoC not board.

Yes that's why I put it in tegra20.dtsi. But I have removed it now.

>
>>>> +/* 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)
>>>
>>> Those two sets of data seem like they should simply be part of the clock
>>> driver. Some part of the system boot or USB init process needs to say
>>> "enable the USB clocks", and that code needs to write those hard-coded
>>> values into the relevant clock registers (or calculate them at run-time;
>>> whatever). This stuff can't vary by board.
>>
>> We don't have a USB clock driver. This driver is in CPU code, so I am
>> not suggesting that it varies by board, only by SOC. This stuff is in
>> the SOC dts.
>>>
>>>> + * 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.
>>>
>>> ? ? ?^ typo
>>>
>>>> + * ?Reference frequency ? ? 13.0MHz ? ? ?19.2MHz ? ? ?12.0MHz ? ? ?26.0MHz
>>>
>>> If this data varies at all, then those two sets of data seem
>>> port-specific to me, and hence should be moved into a per-port property.
>>> Having a single global set of parameters that gets applied to all ports
>>> at once doesn't make sense to me, if the data varies.
>>
>> The hardware doesn't support this does it?
>
> It looks like it doesn't; there are two debounce count registers and
> each port (or something) can select which of those two debounce values
> is used.
>
>>> I imagine the values are board specific too, and hence shouldn't be in
>>> tegra20.dtsi but rather in tegra-seaboard.dts.
>>
>> I believe they vary by SOC, not board.
>
>>> The values in these fields should be specific in a way that's agnostic
>>> to the reference clock, e.g. as a number of mS/nS. That way, you'd just
>>> specify the single set of values to use, and the driver would do the
>>> calculations to map the time domain into the reference-clock-specific
>>> values to put into the registers.
>>
>> Yes I agree. I don't have time to write this. Shall we go forward with
>> what we have, and Nvidia can tidy up later?
>
> I definitely oppose that; the current bindings are simply not
> conceptually correct. I'm opposed to merging something that's known to
> be broken even if there's a clear intent/timeframe to fix it; it'd be
> better to just defer merging it. (Well, even better to fix it to be
> right an merge it!)
>
> As a blanket statement, the USB driver shouldn't programming PLLs. This
> applies whether the PLL configuration is hard-coded in the driver, is
> parsed from DT, or whatever. We should definitely remove all the PLL
> related stuff from the DT bindings, and have some core or board specific
> code set up the PLLs. That code can be replaced by DT clock parsing when
> its ready. The USB driver shouldn't be impacted by that change at all.
>
> Re: the debounce values: The kernel doesn't touch those registers as far
> as I can tell and everything works. I'd suggest completely removing that
> from the DT bindings for now. It's quite possible it only works because
> the HW register defaults are correct (or at least work OK) for a 12MHz
> reference crystal, and all boards supported by the kernel just happen to
> use that reference frequency, but I think the same is probably true for
> (Tegra boards in) U-Boot right now too.
>
> Taking that approach, you can completely remove the contentious parts of
> the DT binding and defer the problem until more infra-structure is in
> place to support those features.

Sorry - I think this was on another thread - but I am removing the fdt
bindings for USB timings from this series. We can deal with it later
as needed.

>
>>>> +
>>>> ? ? ? usb at c5000000 {
>>>> ? ? ? ? ? ? ? compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>>> ? ? ? ? ? ? ? reg = <0xc5000000 0x4000>;
>>>> ? ? ? ? ? ? ? interrupts = < 52 >;
>>>> ? ? ? ? ? ? ? phy_type = "utmi";
>>>> + ? ? ? ? ? ? periph-id = <22>; ? ? ? // PERIPH_ID_USBD
>>>
>>> Given this is a temporary U-Boot-specific solution, can the property be
>>> named u-boot,periph-id so it's obvious that when writing a .dts for the
>>> kernel only, you don't care about this value.
>>
>> ok. I suggest the kernel does something similar.
>
> The kernel will use the standardized clock bindings once they're ready
> and we convert Tegra over to use them. The kernel is extremely unlikely
> to ever use "periph-id" or "u-boot,periph-id".

What is the time frame on this working be completed and merged?\

>
> Right now, the kernel's clock driver contains a mapping table from
> device name (e.g. tegra-ehci.2) to clock name (e.g. usb3). This allows
> the kernel USB driver to work without any explicit periph-id or similar
> DT property.

Where does tegra-ehci.2 come from? I don't see that in the fdt.

U-Boot itself does not have a device name or a clock name, or even the
concept of a clock. FDT control has only just arrived in U-Boot and it
needs to fit with what exists in the code base. There needs to be a
multiple sub-arch refactoring effort which I am sure will be taken up
once the kernel completes its work. But this is not just an FDT thing.

>
> I'd strongly suggest that's the cleanest transition plan for U-Boot
> until the clock bindings are complete, and they are added to
> tegra20.dtsi, and U-Boot can parse them, since it avoids temporarily
> defining DT properties to WAR the issue, and then yanking them out
> later. This mapping would be a part of the core Tegra SoC code,
> completely isolated from the USB driver.

Tegra U-Boot already uses peripheral IDs internally - it's
considerably more efficient to be able to refer to a peripheral by a
number than a string. Putting these in the fdt makes it easy for fdt
code to use internal clocks, etc. One way or another the driver code
needs to figure out the peripheral ID. Please have a look at
arch/arm/cpu/armv7/tegra2/clock.c.

I hope that our efforts can continue while we wait for the kernel to
implement clocks. We can then look at writing the required plumbing
code which presumably will be common across ARM architectures, not
tegra-specific. That is going to need review within U-Boot also, and
it is not going to happen overnight. Efficiency is much more of a
concern and no BSS is available when initial fdt decoding starts.

We will want to incorporate these changes with the pinmux stuff also,
since that is already required to do all of this properly.

The one-line addition per peripheral and can easier be converted to
use some other approach later. I've added a u-boot prefix on the
property to clearly show that this is a U-Boot property. I think this
is a reasonable solution.

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH v2 09/17] tegra: usb: fdt: Add additional device tree definitions for USB ports

[Stephen Warren]

On 12/06/2011 02:09 PM, Simon Glass wrote:
> On Tue, Dec 6, 2011 at 12:28 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/05/2011 05:55 PM, Simon Glass wrote:
>>> On Mon, Dec 5, 2011 at 3:25 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>>>> This adds peripheral IDs and timing information to the USB part of the
>>>>> device tree for U-Boot.
>>>>>
>>>>> The peripheral IDs provide easy access to clock registers. We will likely
>>>>> remove this in favor of a full clock tree when it is available in the
>>>>> kernel (but probably still retain the peripheral ID, just move it into
>>>>> a clock node).
>>>>>
>>>>> The USB timing information does apparently vary between boards sometimes,
>>>>> so is include in the fdt for convenience.

>>>>>       usb at c5000000 {
>>>>>               compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>>>>               reg = <0xc5000000 0x4000>;
>>>>>               interrupts = < 52 >;
>>>>>               phy_type = "utmi";
>>>>> +             periph-id = <22>;       // PERIPH_ID_USBD
>>>>
>>>> Given this is a temporary U-Boot-specific solution, can the property be
>>>> named u-boot,periph-id so it's obvious that when writing a .dts for the
>>>> kernel only, you don't care about this value.
>>>
>>> ok. I suggest the kernel does something similar.
>>
>> The kernel will use the standardized clock bindings once they're ready
>> and we convert Tegra over to use them. The kernel is extremely unlikely
>> to ever use "periph-id" or "u-boot,periph-id".
> 
> What is the time frame on this working be completed and merged?

Sorry, I have no idea. I've been focusing on other subsystems (pinmux,
audio) and haven't been following the clock stuff at all. Hopefully
someone will start driving Tegra kernel towards common clock soon, but I
don't think exactly who and when has been nailed down yet.

>> Right now, the kernel's clock driver contains a mapping table from
>> device name (e.g. tegra-ehci.2) to clock name (e.g. usb3). This allows
>> the kernel USB driver to work without any explicit periph-id or similar
>> DT property.
> 
> Where does tegra-ehci.2 come from? I don't see that in the fdt.

Pre-DT, everything was instantiated from platform devices. Each one had
a name ("tegra-ehci") and an instance number ("2"), which concatenate to
"tegra-ehci.2". All the clocks (and I think other resources like
regulators) in the kernel were marked as being for use by a particular
device name. For example in arch/arm/mach-tegra/tegra2_clocks.c:

static struct clk tegra_list_clks[] = {
...
        PERIPH_CLK("usb3",      "tegra-ehci.2", ...),

With DT, the device names typically don't follow this format (in this
case, it'd be something more like "/usb at c5008000"). However, this
prevented the clock lookups by device name from working, so a temporary
scheme was put in place to keep the same device names. This is driven by
"AUXDATA", for example in arch/arm/mach-tegra/board-dt.c:


struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
...
//             compatible, unit address, device name
OF_DEV_AUXDATA("nvidia,tegra20-ehci", TEGRA_USB3_BASE, "tegra-ehci.2",

This means that any device with the given compatible property value, the
given unit address will be named accordingly.

This allows the existing clock/regulator lookups to work unmodified.

Once DT bindings are in place for clocks, regulators, etc., the clock
tables can be derived from DT, phandles will be used to match clocks and
devices rather than device names, and the AUXDATA table can go away.

The equivalent in U-Boot would be a table that maps from driver type
(e.g. COMPAT_NVIDIA_TEGRA20_USB or perhaps NVIDIA_TEGRA20_USB?) and
address to periph id. Again, once the clock bindings are complete and
the nodes present in the .dts file, that mapping table can be removed
and everything will work based on phandles.

I'd like to point out here that everything is in a pretty big state of
flux/development, since DT support for ARM is new. Temporary workarounds
like AUXDATA allow us to make as much work as possible using device
tree, but without having to put temporary nodes/properties into the .dts
files themselves. That way, the DT bindings will only ever get added to
in a compatible fashion, rather than going through multiple incompatible
sets of requirements.

-- 
nvpublic

[U-Boot] [PATCH v2 09/17] tegra: usb: fdt: Add additional device tree definitions for USB ports

[Simon Glass]

Hi Stephen,

On Wed, Dec 7, 2011 at 3:36 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/06/2011 02:09 PM, Simon Glass wrote:
>> On Tue, Dec 6, 2011 at 12:28 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/05/2011 05:55 PM, Simon Glass wrote:
>>>> On Mon, Dec 5, 2011 at 3:25 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>>>>> This adds peripheral IDs and timing information to the USB part of the
>>>>>> device tree for U-Boot.
>>>>>>
>>>>>> The peripheral IDs provide easy access to clock registers. We will likely
>>>>>> remove this in favor of a full clock tree when it is available in the
>>>>>> kernel (but probably still retain the peripheral ID, just move it into
>>>>>> a clock node).
>>>>>>
>>>>>> The USB timing information does apparently vary between boards sometimes,
>>>>>> so is include in the fdt for convenience.
>
>>>>>> ? ? ? usb at c5000000 {
>>>>>> ? ? ? ? ? ? ? compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>>>>> ? ? ? ? ? ? ? reg = <0xc5000000 0x4000>;
>>>>>> ? ? ? ? ? ? ? interrupts = < 52 >;
>>>>>> ? ? ? ? ? ? ? phy_type = "utmi";
>>>>>> + ? ? ? ? ? ? periph-id = <22>; ? ? ? // PERIPH_ID_USBD
>>>>>
>>>>> Given this is a temporary U-Boot-specific solution, can the property be
>>>>> named u-boot,periph-id so it's obvious that when writing a .dts for the
>>>>> kernel only, you don't care about this value.
>>>>
>>>> ok. I suggest the kernel does something similar.
>>>
>>> The kernel will use the standardized clock bindings once they're ready
>>> and we convert Tegra over to use them. The kernel is extremely unlikely
>>> to ever use "periph-id" or "u-boot,periph-id".
>>
>> What is the time frame on this working be completed and merged?
>
> Sorry, I have no idea. I've been focusing on other subsystems (pinmux,
> audio) and haven't been following the clock stuff at all. Hopefully
> someone will start driving Tegra kernel towards common clock soon, but I
> don't think exactly who and when has been nailed down yet.
>
>>> Right now, the kernel's clock driver contains a mapping table from
>>> device name (e.g. tegra-ehci.2) to clock name (e.g. usb3). This allows
>>> the kernel USB driver to work without any explicit periph-id or similar
>>> DT property.
>>
>> Where does tegra-ehci.2 come from? I don't see that in the fdt.
>
> Pre-DT, everything was instantiated from platform devices. Each one had
> a name ("tegra-ehci") and an instance number ("2"), which concatenate to
> "tegra-ehci.2". All the clocks (and I think other resources like
> regulators) in the kernel were marked as being for use by a particular
> device name. For example in arch/arm/mach-tegra/tegra2_clocks.c:
>
> static struct clk tegra_list_clks[] = {
> ...
> ? ? ? ?PERIPH_CLK("usb3", ? ? ?"tegra-ehci.2", ...),
>
> With DT, the device names typically don't follow this format (in this
> case, it'd be something more like "/usb at c5008000"). However, this
> prevented the clock lookups by device name from working, so a temporary
> scheme was put in place to keep the same device names. This is driven by
> "AUXDATA", for example in arch/arm/mach-tegra/board-dt.c:
>
>
> struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
> ...
> // ? ? ? ? ? ? compatible, unit address, device name
> OF_DEV_AUXDATA("nvidia,tegra20-ehci", TEGRA_USB3_BASE, "tegra-ehci.2",
>
> This means that any device with the given compatible property value, the
> given unit address will be named accordingly.
>
> This allows the existing clock/regulator lookups to work unmodified.
>
> Once DT bindings are in place for clocks, regulators, etc., the clock
> tables can be derived from DT, phandles will be used to match clocks and
> devices rather than device names, and the AUXDATA table can go away.
>
> The equivalent in U-Boot would be a table that maps from driver type
> (e.g. COMPAT_NVIDIA_TEGRA20_USB or perhaps NVIDIA_TEGRA20_USB?) and
> address to periph id. Again, once the clock bindings are complete and
> the nodes present in the .dts file, that mapping table can be removed
> and everything will work based on phandles.
>
> I'd like to point out here that everything is in a pretty big state of
> flux/development, since DT support for ARM is new. Temporary workarounds
> like AUXDATA allow us to make as much work as possible using device
> tree, but without having to put temporary nodes/properties into the .dts
> files themselves. That way, the DT bindings will only ever get added to
> in a compatible fashion, rather than going through multiple incompatible
> sets of requirements.

Gosh.

I have to say that I feel that peripheral IDs are the best solution
for Tegra U-Boot until everything is worked out in the kernel.

We can't rely on phandles since they don't exist without an fdt,
unless we mandate that everyone must use an fdt. I don't feel
comfortable doing that until things are a bit more stable with all the
things you are working on.

I really can't see why we want to put a table in U-Boot which does a
mapping that is clear a hardware feature and IMO belongs in the fdt
(why repeat peripheral addresses in the code and the fdt?).

Plus I still don't have an answer to my question about how we can
ensure that instance 0 is a particular device. The fact that this just
happens to work in the kernel for uarts, and shouldn't matter for
everything else, is not good enough in U-Boot since scripts etc. rely
on instance mapping being stable.

So for now I think we should simply add a peripheral ID to the fdt,
use the alias mechanism (perhaps augmented by scanning peripherals not
mentioned by an alias if you like) and be done with it.

Regards,
Simon

>
> --
> nvpublic

[U-Boot] [PATCH v2 09/17] tegra: usb: fdt: Add additional device tree definitions for USB ports

[Stephen Warren]

On 12/08/2011 02:10 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Wed, Dec 7, 2011 at 3:36 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/06/2011 02:09 PM, Simon Glass wrote:
>>> On Tue, Dec 6, 2011 at 12:28 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>> On 12/05/2011 05:55 PM, Simon Glass wrote:
>>>>> On Mon, Dec 5, 2011 at 3:25 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>>>>>> This adds peripheral IDs and timing information to the USB part of the
>>>>>>> device tree for U-Boot.
>>>>>>>
>>>>>>> The peripheral IDs provide easy access to clock registers. We will likely
>>>>>>> remove this in favor of a full clock tree when it is available in the
>>>>>>> kernel (but probably still retain the peripheral ID, just move it into
>>>>>>> a clock node).
>>>>>>>
>>>>>>> The USB timing information does apparently vary between boards sometimes,
>>>>>>> so is include in the fdt for convenience.
>>
>>>>>>>       usb at c5000000 {
>>>>>>>               compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>>>>>>               reg = <0xc5000000 0x4000>;
>>>>>>>               interrupts = < 52 >;
>>>>>>>               phy_type = "utmi";
>>>>>>> +             periph-id = <22>;       // PERIPH_ID_USBD
>>>>>>
>>>>>> Given this is a temporary U-Boot-specific solution, can the property be
>>>>>> named u-boot,periph-id so it's obvious that when writing a .dts for the
>>>>>> kernel only, you don't care about this value.
>>>>>
>>>>> ok. I suggest the kernel does something similar.
>>>>
>>>> The kernel will use the standardized clock bindings once they're ready
>>>> and we convert Tegra over to use them. The kernel is extremely unlikely
>>>> to ever use "periph-id" or "u-boot,periph-id".
>>>
>>> What is the time frame on this working be completed and merged?
>>
>> Sorry, I have no idea. I've been focusing on other subsystems (pinmux,
>> audio) and haven't been following the clock stuff at all. Hopefully
>> someone will start driving Tegra kernel towards common clock soon, but I
>> don't think exactly who and when has been nailed down yet.
>>
>>>> Right now, the kernel's clock driver contains a mapping table from
>>>> device name (e.g. tegra-ehci.2) to clock name (e.g. usb3). This allows
>>>> the kernel USB driver to work without any explicit periph-id or similar
>>>> DT property.
>>>
>>> Where does tegra-ehci.2 come from? I don't see that in the fdt.
>>
>> Pre-DT, everything was instantiated from platform devices. Each one had
>> a name ("tegra-ehci") and an instance number ("2"), which concatenate to
>> "tegra-ehci.2". All the clocks (and I think other resources like
>> regulators) in the kernel were marked as being for use by a particular
>> device name. For example in arch/arm/mach-tegra/tegra2_clocks.c:
>>
>> static struct clk tegra_list_clks[] = {
>> ...
>>        PERIPH_CLK("usb3",      "tegra-ehci.2", ...),
>>
>> With DT, the device names typically don't follow this format (in this
>> case, it'd be something more like "/usb at c5008000"). However, this
>> prevented the clock lookups by device name from working, so a temporary
>> scheme was put in place to keep the same device names. This is driven by
>> "AUXDATA", for example in arch/arm/mach-tegra/board-dt.c:
>>
>>
>> struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
>> ...
>> //             compatible, unit address, device name
>> OF_DEV_AUXDATA("nvidia,tegra20-ehci", TEGRA_USB3_BASE, "tegra-ehci.2",
>>
>> This means that any device with the given compatible property value, the
>> given unit address will be named accordingly.
>>
>> This allows the existing clock/regulator lookups to work unmodified.
>>
>> Once DT bindings are in place for clocks, regulators, etc., the clock
>> tables can be derived from DT, phandles will be used to match clocks and
>> devices rather than device names, and the AUXDATA table can go away.
>>
>> The equivalent in U-Boot would be a table that maps from driver type
>> (e.g. COMPAT_NVIDIA_TEGRA20_USB or perhaps NVIDIA_TEGRA20_USB?) and
>> address to periph id. Again, once the clock bindings are complete and
>> the nodes present in the .dts file, that mapping table can be removed
>> and everything will work based on phandles.
>>
>> I'd like to point out here that everything is in a pretty big state of
>> flux/development, since DT support for ARM is new. Temporary workarounds
>> like AUXDATA allow us to make as much work as possible using device
>> tree, but without having to put temporary nodes/properties into the .dts
>> files themselves. That way, the DT bindings will only ever get added to
>> in a compatible fashion, rather than going through multiple incompatible
>> sets of requirements.
> 
> Gosh.
> 
> I have to say that I feel that peripheral IDs are the best solution
> for Tegra U-Boot until everything is worked out in the kernel.

The problem here is that it requires the DT to change incompatibly
later; it adds a property to the DT now that will be at best
meaningless/unused in the future.

If we simply don't add anything to the DT now, there's nothing to remove
from the DT later. Newer U-Boots might require additional information in
the DT (i.e. perhaps rely on full clock bindings) but won't deprecate
any existing fields.

> We can't rely on phandles since they don't exist without an fdt,
> unless we mandate that everyone must use an fdt. I don't feel
> comfortable doing that until things are a bit more stable with all the
> things you are working on.

Sure, phandles won't work until the complete clock binding is implemnted.

> I really can't see why we want to put a table in U-Boot which does a
> mapping that is clear a hardware feature and IMO belongs in the fdt
> (why repeat peripheral addresses in the code and the fdt?).

It's a HW feature of the clock/reset controller, not the USB controller.

> Plus I still don't have an answer to my question about how we can
> ensure that instance 0 is a particular device.

As I said before, in the context of USB (where IIRC the question was
asked), you can enable just a single USB controller. The code only
supports a single controller anyway.

For SD/MMC, it does make sense to statically name some/all devices. That
is what /aliases is for. It's just that as I said, /aliases is meant to
control naming of devices that have been enumerated, not control the
enumeration itself.

-- 
nvpublic

[U-Boot] [PATCH v2 09/17] tegra: usb: fdt: Add additional device tree definitions for USB ports

[Simon Glass]

Hi Stephen,

On Mon, Dec 12, 2011 at 10:13 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/08/2011 02:10 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Wed, Dec 7, 2011 at 3:36 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/06/2011 02:09 PM, Simon Glass wrote:
>>>> On Tue, Dec 6, 2011 at 12:28 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>> On 12/05/2011 05:55 PM, Simon Glass wrote:
>>>>>> On Mon, Dec 5, 2011 at 3:25 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>>>>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>>>>>>> This adds peripheral IDs and timing information to the USB part of the
>>>>>>>> device tree for U-Boot.
>>>>>>>>
>>>>>>>> The peripheral IDs provide easy access to clock registers. We will likely
>>>>>>>> remove this in favor of a full clock tree when it is available in the
>>>>>>>> kernel (but probably still retain the peripheral ID, just move it into
>>>>>>>> a clock node).
>>>>>>>>
>>>>>>>> The USB timing information does apparently vary between boards sometimes,
>>>>>>>> so is include in the fdt for convenience.
>>>
>>>>>>>> ? ? ? usb at c5000000 {
>>>>>>>> ? ? ? ? ? ? ? compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>>>>>>> ? ? ? ? ? ? ? reg = <0xc5000000 0x4000>;
>>>>>>>> ? ? ? ? ? ? ? interrupts = < 52 >;
>>>>>>>> ? ? ? ? ? ? ? phy_type = "utmi";
>>>>>>>> + ? ? ? ? ? ? periph-id = <22>; ? ? ? // PERIPH_ID_USBD
>>>>>>>
>>>>>>> Given this is a temporary U-Boot-specific solution, can the property be
>>>>>>> named u-boot,periph-id so it's obvious that when writing a .dts for the
>>>>>>> kernel only, you don't care about this value.
>>>>>>
>>>>>> ok. I suggest the kernel does something similar.
>>>>>
>>>>> The kernel will use the standardized clock bindings once they're ready
>>>>> and we convert Tegra over to use them. The kernel is extremely unlikely
>>>>> to ever use "periph-id" or "u-boot,periph-id".
>>>>
>>>> What is the time frame on this working be completed and merged?
>>>
>>> Sorry, I have no idea. I've been focusing on other subsystems (pinmux,
>>> audio) and haven't been following the clock stuff at all. Hopefully
>>> someone will start driving Tegra kernel towards common clock soon, but I
>>> don't think exactly who and when has been nailed down yet.
>>>
>>>>> Right now, the kernel's clock driver contains a mapping table from
>>>>> device name (e.g. tegra-ehci.2) to clock name (e.g. usb3). This allows
>>>>> the kernel USB driver to work without any explicit periph-id or similar
>>>>> DT property.
>>>>
>>>> Where does tegra-ehci.2 come from? I don't see that in the fdt.
>>>
>>> Pre-DT, everything was instantiated from platform devices. Each one had
>>> a name ("tegra-ehci") and an instance number ("2"), which concatenate to
>>> "tegra-ehci.2". All the clocks (and I think other resources like
>>> regulators) in the kernel were marked as being for use by a particular
>>> device name. For example in arch/arm/mach-tegra/tegra2_clocks.c:
>>>
>>> static struct clk tegra_list_clks[] = {
>>> ...
>>> ? ? ? ?PERIPH_CLK("usb3", ? ? ?"tegra-ehci.2", ...),
>>>
>>> With DT, the device names typically don't follow this format (in this
>>> case, it'd be something more like "/usb at c5008000"). However, this
>>> prevented the clock lookups by device name from working, so a temporary
>>> scheme was put in place to keep the same device names. This is driven by
>>> "AUXDATA", for example in arch/arm/mach-tegra/board-dt.c:
>>>
>>>
>>> struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
>>> ...
>>> // ? ? ? ? ? ? compatible, unit address, device name
>>> OF_DEV_AUXDATA("nvidia,tegra20-ehci", TEGRA_USB3_BASE, "tegra-ehci.2",
>>>
>>> This means that any device with the given compatible property value, the
>>> given unit address will be named accordingly.
>>>
>>> This allows the existing clock/regulator lookups to work unmodified.
>>>
>>> Once DT bindings are in place for clocks, regulators, etc., the clock
>>> tables can be derived from DT, phandles will be used to match clocks and
>>> devices rather than device names, and the AUXDATA table can go away.
>>>
>>> The equivalent in U-Boot would be a table that maps from driver type
>>> (e.g. COMPAT_NVIDIA_TEGRA20_USB or perhaps NVIDIA_TEGRA20_USB?) and
>>> address to periph id. Again, once the clock bindings are complete and
>>> the nodes present in the .dts file, that mapping table can be removed
>>> and everything will work based on phandles.
>>>
>>> I'd like to point out here that everything is in a pretty big state of
>>> flux/development, since DT support for ARM is new. Temporary workarounds
>>> like AUXDATA allow us to make as much work as possible using device
>>> tree, but without having to put temporary nodes/properties into the .dts
>>> files themselves. That way, the DT bindings will only ever get added to
>>> in a compatible fashion, rather than going through multiple incompatible
>>> sets of requirements.
>>
>> Gosh.
>>
>> I have to say that I feel that peripheral IDs are the best solution
>> for Tegra U-Boot until everything is worked out in the kernel.
>
> The problem here is that it requires the DT to change incompatibly
> later; it adds a property to the DT now that will be at best
> meaningless/unused in the future.
>
> If we simply don't add anything to the DT now, there's nothing to remove
> from the DT later. Newer U-Boots might require additional information in
> the DT (i.e. perhaps rely on full clock bindings) but won't deprecate
> any existing fields.

I really don't see the difference between changing the code later and
changing the FDT later. The full clock bindings could be ages away. We
have already given it a u-boot prefix. We might even decided to keep
it for efficiency reasons.

>
>> We can't rely on phandles since they don't exist without an fdt,
>> unless we mandate that everyone must use an fdt. I don't feel
>> comfortable doing that until things are a bit more stable with all the
>> things you are working on.
>
> Sure, phandles won't work until the complete clock binding is implemnted.

No I mean they won't work if someone isn't using CONFIG_OF_CONTROL and
thus doesn't have a device tree at all. This option is optional in
U-Boot, and in fact has only been in mainline for 5 minutes, and there
are no users up until this patch. FDT still needs to prove its worth
in U-Boot.

>
>> I really can't see why we want to put a table in U-Boot which does a
>> mapping that is clear a hardware feature and IMO belongs in the fdt
>> (why repeat peripheral addresses in the code and the fdt?).
>
> It's a HW feature of the clock/reset controller, not the USB controller.

I think I was responding to your request to have tables in U-Boot like:

<device_type>, <peripheral_address> -> clock ID

My objection is that <device_type> is already encoded in the
compatible string, <peripheral_address> is in the reg property, and we
can avoid the same altogether by putting the peripheral ID in the FDT
too. In fact I thought we already agreed on u-boot,periph-id.

>
>> Plus I still don't have an answer to my question about how we can
>> ensure that instance 0 is a particular device.
>
> As I said before, in the context of USB (where IIRC the question was
> asked), you can enable just a single USB controller. The code only
> supports a single controller anyway.

Yes but it must be the correct controller. In other words, on Seaboard
the side controller must be first for USB to work.

>
> For SD/MMC, it does make sense to statically name some/all devices. That
> is what /aliases is for. It's just that as I said, /aliases is meant to
> control naming of devices that have been enumerated, not control the
> enumeration itself.

And UARTs I think. So I think this means we are happy with the aliases
approach, and we are now just discussing the algorithm.

Regards,
SImon

>
> --
> nvpublic

[U-Boot] [PATCH v2 10/17] tegra: usb: fdt: Add USB definitions for Tegra2 Seaboard

[Simon Glass]

We set up two USB ports, one of which can be host or device.
For some reason the kernel version does enable both ports.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Use "okay" instead of "ok" for fdt node status
- Remove 0x from fdt aliases

 board/nvidia/dts/tegra2-seaboard.dts |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/board/nvidia/dts/tegra2-seaboard.dts b/board/nvidia/dts/tegra2-seaboard.dts
index dde5d03..19c6503 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 c5008000";
+                usb1 = "/usb at c5000000";
+        };
+
 	memory {
 		device_type = "memory";
 		reg = < 0x00000000 0x40000000 >;
@@ -31,6 +37,12 @@
 	};
 
 	usb at c5000000 {
-		nvidia,vbus-gpio = <&gpio 24 0>; /* PD0 */
+		nvidia,vbus-gpio = <&gpio 24 1>; /* PD0 high to enable vbus */
+		support-host-mode;
+		status = "okay";
+	};
+
+	usb at c5008000 {
+		status = "okay";
 	};
 };
-- 
1.7.3.1

[U-Boot] [PATCH v2 10/17] tegra: usb: fdt: Add USB definitions for Tegra2 Seaboard

[Stephen Warren]

On 12/02/2011 07:11 PM, Simon Glass wrote:
> We set up two USB ports, one of which can be host or device.
> For some reason the kernel version does enable both ports.

> +++ 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 c5008000";
> +                usb1 = "/usb at c5000000";
> +        };
> +

That's indented with spaces instead of TABs.

>  	memory {
>  		device_type = "memory";
>  		reg = < 0x00000000 0x40000000 >;
> @@ -31,6 +37,12 @@
>  	};
>  
>  	usb at c5000000 {
> -		nvidia,vbus-gpio = <&gpio 24 0>; /* PD0 */
> +		nvidia,vbus-gpio = <&gpio 24 1>; /* PD0 high to enable vbus */
> +		support-host-mode;
> +		status = "okay";

That's the default, and tegra20.dtsi doesn't specify status =
"disabled", so the status properties are redundant. You may want to
specify a disabled status in tegra-seaboard.dts for the other USB port
if it isn't used though.

> +	};
> +
> +	usb at c5008000 {
> +		status = "okay";
>  	};
>  };

-- 
nvpublic

[U-Boot] [PATCH v2 11/17] usb: Add support for data alignment

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

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Tidy code style in USB buffer alignment code
- Display prominent warning when alignment code is triggered

 README                      |    6 ++++++
 drivers/usb/host/ehci-hcd.c |   29 +++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/README b/README
index fda0190..3194846 100644
--- a/README
+++ b/README
@@ -1096,6 +1096,12 @@ 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. Since we want all callers to
+		align data for us, we display a warning when the alignment
+		code is triggered.
+
 - 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..23458b6 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -322,6 +322,24 @@ 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);
+
+	if (unaligned) {
+		printf("EHCI: Unaligned buffer: performance will suffer\n");
+		align_buf = malloc(length + CONFIG_USB_EHCI_DATA_ALIGN);
+		if (!align_buf)
+			return -1;
+		buffer = (void *)(((ulong)align_buf +
+			CONFIG_USB_EHCI_DATA_ALIGN - 1) &
+				~(CONFIG_USB_EHCI_DATA_ALIGN - 1));
+		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 +531,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;
-- 
1.7.3.1

[U-Boot] [PATCH v2 11/17] usb: Add support for data alignment

[Remy Bohmer]

Hi,

2011/12/3 Simon Glass <sjg@chromium.org>:
> 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.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v2:
> - Tidy code style in USB buffer alignment code
> - Display prominent warning when alignment code is triggered
>
> ?README ? ? ? ? ? ? ? ? ? ? ?| ? ?6 ++++++
> ?drivers/usb/host/ehci-hcd.c | ? 29 +++++++++++++++++++++++++++++
> ?2 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/README b/README
> index fda0190..3194846 100644
> --- a/README
> +++ b/README
> @@ -1096,6 +1096,12 @@ 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. Since we want all callers to
> + ? ? ? ? ? ? ? align data for us, we display a warning when the alignment
> + ? ? ? ? ? ? ? code is triggered.
> +
> ?- 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..23458b6 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -322,6 +322,24 @@ 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);
> +
> + ? ? ? if (unaligned) {
> + ? ? ? ? ? ? ? printf("EHCI: Unaligned buffer: performance will suffer\n");
> + ? ? ? ? ? ? ? align_buf = malloc(length + CONFIG_USB_EHCI_DATA_ALIGN);
> + ? ? ? ? ? ? ? if (!align_buf)
> + ? ? ? ? ? ? ? ? ? ? ? return -1;
> + ? ? ? ? ? ? ? buffer = (void *)(((ulong)align_buf +
> + ? ? ? ? ? ? ? ? ? ? ? CONFIG_USB_EHCI_DATA_ALIGN - 1) &
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ~(CONFIG_USB_EHCI_DATA_ALIGN - 1));
> + ? ? ? ? ? ? ? if (usb_pipeout(pipe))
> + ? ? ? ? ? ? ? ? ? ? ? memcpy(buffer, orig_buf, length);
> + ? ? ? }

I do not really like this solution. Maybe we can introduce something
like an usbbuf_alloc() function that takes care of the alignment.
This saves the memcpy() for every transmit. I know it has more impact
if we do it like that, but I think it is cleaner.

Kind regards,

Remy

[U-Boot] [PATCH v2 11/17] usb: Add support for data alignment

[Simon Glass]

Hi Remy,

On Sun, Dec 4, 2011 at 3:13 AM, Remy Bohmer <linux@bohmer.net> wrote:
> Hi,
>
> 2011/12/3 Simon Glass <sjg@chromium.org>:
>> 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.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>> Changes in v2:
>> - Tidy code style in USB buffer alignment code
>> - Display prominent warning when alignment code is triggered
>>
>> ?README ? ? ? ? ? ? ? ? ? ? ?| ? ?6 ++++++
>> ?drivers/usb/host/ehci-hcd.c | ? 29 +++++++++++++++++++++++++++++
>> ?2 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/README b/README
>> index fda0190..3194846 100644
>> --- a/README
>> +++ b/README
>> @@ -1096,6 +1096,12 @@ 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. Since we want all callers to
>> + ? ? ? ? ? ? ? align data for us, we display a warning when the alignment
>> + ? ? ? ? ? ? ? code is triggered.
>> +
>> ?- 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..23458b6 100644
>> --- a/drivers/usb/host/ehci-hcd.c
>> +++ b/drivers/usb/host/ehci-hcd.c
>> @@ -322,6 +322,24 @@ 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);
>> +
>> + ? ? ? if (unaligned) {
>> + ? ? ? ? ? ? ? printf("EHCI: Unaligned buffer: performance will suffer\n");
>> + ? ? ? ? ? ? ? align_buf = malloc(length + CONFIG_USB_EHCI_DATA_ALIGN);
>> + ? ? ? ? ? ? ? if (!align_buf)
>> + ? ? ? ? ? ? ? ? ? ? ? return -1;
>> + ? ? ? ? ? ? ? buffer = (void *)(((ulong)align_buf +
>> + ? ? ? ? ? ? ? ? ? ? ? CONFIG_USB_EHCI_DATA_ALIGN - 1) &
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ~(CONFIG_USB_EHCI_DATA_ALIGN - 1));
>> + ? ? ? ? ? ? ? if (usb_pipeout(pipe))
>> + ? ? ? ? ? ? ? ? ? ? ? memcpy(buffer, orig_buf, length);
>> + ? ? ? }
>
> I do not really like this solution. Maybe we can introduce something
> like an usbbuf_alloc() function that takes care of the alignment.
> This saves the memcpy() for every transmit. I know it has more impact
> if we do it like that, but I think it is cleaner.

This is called from usb_bulk_msg() which is called from usb_storage
and USB ethernet drivers. A similar problem happened in the MMC stack
and was fixed by changing all the callers to cache-align their
buffers.

Do you mean that you want a patch which changes the callers so that
they align their data to a cache boundary as with MMC? Given the small
number of callers at this stage it shouldn't be too hard. Then we
could drop this patch.

Please let me know.

Regards,
Simon

>
> Kind regards,
>
> Remy

[U-Boot] [PATCH v2 11/17] usb: Add support for data alignment

[Remy Bohmer]

Hi Simon,

2011/12/6 Simon Glass <sjg@chromium.org>:
>>
>> I do not really like this solution. Maybe we can introduce something
>> like an usbbuf_alloc() function that takes care of the alignment.
>> This saves the memcpy() for every transmit. I know it has more impact
>> if we do it like that, but I think it is cleaner.
>
> This is called from usb_bulk_msg() which is called from usb_storage
> and USB ethernet drivers. A similar problem happened in the MMC stack
> and was fixed by changing all the callers to cache-align their
> buffers.
>
> Do you mean that you want a patch which changes the callers so that
> they align their data to a cache boundary as with MMC? Given the small
> number of callers at this stage it shouldn't be too hard. Then we
> could drop this patch.
>
> Please let me know.

That is fine with me too.

Kind regards,

Remy

[U-Boot] [PATCH v2 11/17] usb: Add support for data alignment

[Simon Glass]

Hi Remy,

On Sat, Dec 10, 2011 at 8:04 AM, Remy Bohmer <linux@bohmer.net> wrote:
> Hi Simon,
>
> 2011/12/6 Simon Glass <sjg@chromium.org>:
>>>
>>> I do not really like this solution. Maybe we can introduce something
>>> like an usbbuf_alloc() function that takes care of the alignment.
>>> This saves the memcpy() for every transmit. I know it has more impact
>>> if we do it like that, but I think it is cleaner.
>>
>> This is called from usb_bulk_msg() which is called from usb_storage
>> and USB ethernet drivers. A similar problem happened in the MMC stack
>> and was fixed by changing all the callers to cache-align their
>> buffers.
>>
>> Do you mean that you want a patch which changes the callers so that
>> they align their data to a cache boundary as with MMC? Given the small
>> number of callers at this stage it shouldn't be too hard. Then we
>> could drop this patch.
>>
>> Please let me know.
>
> That is fine with me too.

OK I will look at that. I dropped this patch from the v3 series.

Regards,
Simon

>
> Kind regards,
>
> Remy

[U-Boot] [PATCH v2 12/17] usb: Add support for txfifo threshold

[Simon Glass]

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                      |    3 +++
 drivers/usb/host/ehci-hcd.c |    7 +++++++
 drivers/usb/host/ehci.h     |    6 +++++-
 3 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/README b/README
index 3194846..7df3aa1 100644
--- a/README
+++ b/README
@@ -1102,6 +1102,9 @@ The following options need to be configured:
 		align data for us, we display a warning when the alignment
 		code is triggered.
 
+		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 23458b6..3040a6d 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;
 }
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 v2 12/17] usb: Add support for txfifo threshold

[Stephen Warren]

On 12/02/2011 07:11 PM, Simon Glass wrote:
> CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the txfilltuning
> field in the EHCI controller on reset.

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

Why not remove _reserved_0_ ...

> +	uint32_t or_burstsize;
> +	uint32_t or_txfilltuning;
> +#define TXFIFO_THRESH(p)		((p & 0x3f) << 16)
> +	uint32_t _reserved_1_[6];

... and make _reserved_1_ 1 element bigger and keep it named _reserved_?
The result would be a little simpler.

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

-- 
nvpublic

[U-Boot] [PATCH v2 12/17] usb: Add support for txfifo threshold

[Simon Glass]

Hi Stephen,

On Mon, Dec 5, 2011 at 3:32 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/02/2011 07:11 PM, Simon Glass wrote:
>> CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the txfilltuning
>> field in the EHCI controller on reset.
>
>> 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_;
>
> Why not remove _reserved_0_ ...
>
>> + ? ? uint32_t or_burstsize;
>> + ? ? uint32_t or_txfilltuning;
>> +#define TXFIFO_THRESH(p) ? ? ? ? ? ? ((p & 0x3f) << 16)
>> + ? ? uint32_t _reserved_1_[6];
>
> ... and make _reserved_1_ 1 element bigger and keep it named _reserved_?
> The result would be a little simpler.

Sorry I'm a bit stuck with that one. I have:

	uint32_t or_asynclistaddr;
	uint32_t _reserved_0_;
	uint32_t or_burstsize;
	uint32_t or_txfilltuning;
	uint32_t _reserved_1_[6];
	uint32_t or_configflag;

How can I remove _reserved_0_? I would need to replace it with
something, as need or_burstsize to stay where it is. Can you please
explain a little more?

Thanks,
Simon

>
>> ? ? ? 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];
>
> --
> nvpublic

[U-Boot] [PATCH v2 12/17] usb: Add support for txfifo threshold

[Stephen Warren]

Simon Glass wrote at Monday, December 05, 2011 7:03 PM:
> On Mon, Dec 5, 2011 at 3:32 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > On 12/02/2011 07:11 PM, Simon Glass wrote:
> >> CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the txfilltuning
> >> field in the EHCI controller on reset.
> >
> >> 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_;
> >
> > Why not remove _reserved_0_ ...
> >
> >> + ? ? uint32_t or_burstsize;
> >> + ? ? uint32_t or_txfilltuning;
> >> +#define TXFIFO_THRESH(p) ? ? ? ? ? ? ((p & 0x3f) << 16)
> >> + ? ? uint32_t _reserved_1_[6];
> >
> > ... and make _reserved_1_ 1 element bigger and keep it named _reserved_?
> > The result would be a little simpler.
> 
> Sorry I'm a bit stuck with that one. I have:
> 
> 	uint32_t or_asynclistaddr;
> 	uint32_t _reserved_0_;
> 	uint32_t or_burstsize;
> 	uint32_t or_txfilltuning;
> 	uint32_t _reserved_1_[6];
> 	uint32_t or_configflag;
> 
> How can I remove _reserved_0_? I would need to replace it with
> something, as need or_burstsize to stay where it is. Can you please
> explain a little more?

Oh right, this is a HW structure, so the layout is fixed? In which case
the patch is fine. I wonder why all the fields weren't just defined from
the start rather than being marked "reserved" though; perhaps it'd be
best to update the code to completely flesh out the HW description at
some point. Not necessarily in this patch though.

-- 
nvpublic

[U-Boot] [PATCH v2 12/17] usb: Add support for txfifo threshold

[Simon Glass]

Hi Stephen,

On Tue, Dec 6, 2011 at 10:58 AM, Stephen Warren <swarren@nvidia.com> wrote:
> Simon Glass wrote at Monday, December 05, 2011 7:03 PM:
>> On Mon, Dec 5, 2011 at 3:32 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> > On 12/02/2011 07:11 PM, Simon Glass wrote:
>> >> CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the txfilltuning
>> >> field in the EHCI controller on reset.
>> >
>> >> 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_;
>> >
>> > Why not remove _reserved_0_ ...
>> >
>> >> + ? ? uint32_t or_burstsize;
>> >> + ? ? uint32_t or_txfilltuning;
>> >> +#define TXFIFO_THRESH(p) ? ? ? ? ? ? ((p & 0x3f) << 16)
>> >> + ? ? uint32_t _reserved_1_[6];
>> >
>> > ... and make _reserved_1_ 1 element bigger and keep it named _reserved_?
>> > The result would be a little simpler.
>>
>> Sorry I'm a bit stuck with that one. I have:
>>
>> ? ? ? uint32_t or_asynclistaddr;
>> ? ? ? uint32_t _reserved_0_;
>> ? ? ? uint32_t or_burstsize;
>> ? ? ? uint32_t or_txfilltuning;
>> ? ? ? uint32_t _reserved_1_[6];
>> ? ? ? uint32_t or_configflag;
>>
>> How can I remove _reserved_0_? I would need to replace it with
>> something, as need or_burstsize to stay where it is. Can you please
>> explain a little more?
>
> Oh right, this is a HW structure, so the layout is fixed? In which case
> the patch is fine. I wonder why all the fields weren't just defined from
> the start rather than being marked "reserved" though; perhaps it'd be
> best to update the code to completely flesh out the HW description at
> some point. Not necessarily in this patch though.

Yes that's right. Will leave this patch as is and resend series.

There are two sides to this: some people will say they want everything
in there in case they want to access a particular field later. Others
will say that it adds confusion and dead code. I think I'll sit on the
fence.

Regards,
Simon

>
> --
> nvpublic
>

[U-Boot] [PATCH v2 13/17] tegra: usb: Add support for Tegra 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>
---
Changes in v2:
- Rename params to timing
- Store entire fdt config in port list, not just register pointer
- Remove non-fdt operation of USB, since it is not needed
- Decode USB VBUS GPIO from the fdt
- Decode phy type differently (to match new kernel fdt)
- Rename tegra20-usb to tegra20-ehcui (to match new kernel fdt)
- Improve debug() printouts in case of failure to init USB

 arch/arm/cpu/armv7/tegra2/Makefile        |    4 +-
 arch/arm/cpu/armv7/tegra2/usb.c           |  398 +++++++++++++++++++++++++++++
 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, 724 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..f2173a7
--- /dev/null
+++ b/arch/arm/cpu/armv7/tegra2/usb.c
@@ -0,0 +1,398 @@
+/*
+ * 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 */
+};
+
+/* 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 a USB port */
+struct fdt_usb {
+	struct usb_ctlr *reg;   /* address of registers in physical memory */
+	u32 timing[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 */
+	struct fdt_gpio_state vbus_gpio;	/* GPIO for vbus enable */
+};
+
+static struct fdt_usb 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 fdt_usb *host_dev_ctlr;
+
+/* 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 fdt_usb *config)
+{
+	/* Check whether remote host from USB1 is driving VBus */
+	if (readl(&config->reg->phy_vbus_sensors) & VBUS_VLD_STS)
+		return;
+
+	/*
+	 * If not driving, we set GPIO USB1_VBus_En. We assume that the
+	 * pinmux is set up correctly for this.
+	 */
+	if (fdt_gpio_isvalid(&config->vbus_gpio)) {
+		fdtdec_setup_gpio(&config->vbus_gpio);
+		gpio_direction_output(config->vbus_gpio.gpio, 1);
+	}
+}
+
+/* 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 u32 *timing)
+{
+	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,
+		timing[PARAM_STABLE_COUNT] << UTMIP_PLLU_STABLE_COUNT_SHIFT);
+	clrsetbits_le32(&val, UTMIP_PLL_ACTIVE_DLY_COUNT_MASK,
+		timing[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,
+		timing[PARAM_ENABLE_DELAY_COUNT] <<
+			UTMIP_PLLU_ENABLE_DLY_COUNT_SHIFT);
+	clrsetbits_le32(&val, UTMIP_XTAL_FREQ_COUNT_MASK,
+		timing[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,
+		timing[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,
+		timing[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 u32 timing[])
+{
+	clock_start_pll(CLOCK_ID_USB,
+		timing[PARAM_DIVM], timing[PARAM_DIVN], timing[PARAM_DIVP],
+		timing[PARAM_CPCON], timing[PARAM_LFCON]);
+}
+
+/**
+ * Add a new USB port to the list of available ports.
+ *
+ * @param config	USB port configuration
+ * @return 0 if ok, -1 if error (too many ports)
+ */
+static int add_port(struct fdt_usb *config)
+{
+	struct usb_ctlr *usbctlr = config->reg;
+
+	if (port_count == USB_PORTS_MAX) {
+		debug("tegrausb: Cannot register more than %d ports\n",
+		      USB_PORTS_MAX);
+		return -1;
+	}
+	init_usb_controller(config->periph_id, usbctlr, config->timing);
+	if (config->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);
+	}
+	if (config->host_mode) {
+		/* Only one host-dev port is supported */
+		if (host_dev_ctlr)
+			return -1;
+		host_dev_ctlr = &port[port_count];
+	}
+	port[port_count++] = *config;
+	return 0;
+}
+
+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;
+}
+
+int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
+		   struct fdt_usb *config)
+{
+	int clk_node = 0, rate;
+	const char *phy;
+
+	/* Find the parameters for our oscillator frequency */
+	do {
+		clk_node = fdt_node_offset_by_compatible(blob, clk_node,
+					"nvidia,tegra20-usbtiming");
+		if (clk_node < 0) {
+			debug("Cannot find USB timing 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");
+	phy = fdt_getprop(blob, node, "phy_type", NULL);
+	config->utmi = phy && 0 == strcmp("utmi", phy);
+	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;
+	fdtdec_decode_gpio(blob, node, "nvidia,vbus-gpio", &config->vbus_gpio);
+
+	return fdtdec_get_int_array(blob, clk_node, "timing", config->timing,
+			PARAM_COUNT);
+}
+
+int board_usb_init(const void *blob)
+{
+	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) {
+			debug("Cannot find usb%d alias in fdt\n", upto);
+			break;
+		}
+		if (fdt_decode_usb(blob, node, osc_freq, &config)) {
+			debug("Cannot decode USB node %s\n",
+			      fdt_get_name(blob, node, NULL));
+			return -1;
+		}
+		if (!config.enabled) {
+			debug("Skipping USB node %s since it is disabled\n",
+			      fdt_get_name(blob, node, NULL));
+			continue;
+		}
+
+		/* The first port we find gets to set the clocks */
+		if (!clk_done) {
+			config_clock(config.timing);
+			clk_done = 1;
+		}
+		if (add_port(&config))
+			return -1;
+	} while (node);
+	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 57db42b..9871b81 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 8eed752..509b81f 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -37,6 +37,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-ehci"),
 };
 
 /**
-- 
1.7.3.1

[U-Boot] [PATCH v2 13/17] tegra: usb: Add support for Tegra USB peripheral

[Remy Bohmer]

Hi,

2011/12/3 Simon Glass <sjg@chromium.org>:
> 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>
> ---
> Changes in v2:
> - Rename params to timing
> - Store entire fdt config in port list, not just register pointer
> - Remove non-fdt operation of USB, since it is not needed
> - Decode USB VBUS GPIO from the fdt
> - Decode phy type differently (to match new kernel fdt)
> - Rename tegra20-usb to tegra20-ehcui (to match new kernel fdt)
> - Improve debug() printouts in case of failure to init USB
>

> ?drivers/usb/host/Makefile ? ? ? ? ? ? ? ? | ? ?1 +
> ?drivers/usb/host/ehci-tegra.c ? ? ? ? ? ? | ? 63 +++++

As for this USB part:
Acked-by: Remy Bohmer <linux@bohmer.net>

Kind regards,

Remy

[U-Boot] [PATCH v2 14/17] 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>
---
Changes in v2:
- Add setting of pinmux for USB VBUS GPIO

 board/nvidia/common/board.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
index f39fb24..c7a33c3 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,13 @@ int board_init(void)
 	/* boot param addr */
 	gd->bd->bi_boot_params = (NV_PA_SDRAM_BASE + 0x100);
 
+#ifdef CONFIG_USB_EHCI_TEGRA
+	/* For USB GPIO PD0. for now, since we have no pinmux in fdt */
+	pinmux_tristate_disable(PINGRP_SLXK);
+
+	board_usb_init(gd->fdt_blob);
+#endif
+
 	return 0;
 }
 
-- 
1.7.3.1

[U-Boot] [PATCH v2 15/17] 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 v2 16/17] 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>
---
Changes in v2:
- Remove unneeded CONFIG_TEGRA_USBx defines

 include/configs/seaboard.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h
index 261f952..b6d9f7a 100644
--- a/include/configs/seaboard.h
+++ b/include/configs/seaboard.h
@@ -72,4 +72,11 @@
 
 #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
+
 #endif /* __CONFIG_H */
-- 
1.7.3.1

[U-Boot] [PATCH v2 17/17] 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 |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h
index b6d9f7a..1dc775a 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) # "
-- 
1.7.3.1