tegra: Add fdt definitions and USB driver

On 12/02/2011 07:11 PM, Simon Glass wrote:
...

Quoted 11 lines

+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

Hi Stephen,

Quoted 14 lines · Stephen Warren

On 12/02/2011 07:11 PM, Simon Glass wrote:
...

Quoted 11 lines

lib/fdtdec.c +3 -2

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

Quoted 3 lines

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

Quoted 21 lines · Simon Glass

Hi Stephen,

Quoted 17 lines · Stephen Warren

On 12/02/2011 07:11 PM, Simon Glass wrote:
...

Quoted 11 lines

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

Quoted 3 lines

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

Hi Stephen,

Quoted 4 lines · Stephen Warren

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)

Quoted 3 lines · Simon Glass

Hi Stephen,

On Mon, Dec 5, 2011 at 2:07 PM, Stephen Warren <[email protected]> wrote:

Quoted 4 lines

Quoted 4 lines

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.

Quoted 6 lines

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)

Quoted 4 lines · Scott Wood

Quoted 3 lines · Simon Glass

Hi Stephen,

On Mon, Dec 5, 2011 at 2:07 PM, Stephen Warren <[email protected]> wrote:

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

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.

Quoted 3 lines

Quoted 3 lines

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

Quoted 3 lines

Quoted 3 lines

(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

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.

lib/fdtdec.c +15 -0

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

Quoted 20 lines · 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.

lib/fdtdec.c +15 -0

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

lib/fdtdec.c int fdtdec_check_fdt(void) · line 233

Quoted 10 lines

lib/fdtdec.c +10 -0

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

Quoted 21 lines · Stephen Warren

Quoted 4 lines · Simon Glass

lib/fdtdec.c +4 -0

+int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
+{
+     const s32 *cell;
+     int len;

lib/fdtdec.c int fdtdec_check_fdt(void) · line 233

Quoted 10 lines

lib/fdtdec.c +10 -0

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

Quoted 3 lines

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

Quoted 13 lines

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.

Quoted 42 lines · Simon Glass

Quoted 21 lines · Stephen Warren

Quoted 4 lines · Simon Glass

(quoted diff) +4 -0

+int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
+{
+     const s32 *cell;
+     int len;

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 3 lines

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

Quoted 13 lines

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.

Quoted 7 lines

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

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

Quoted 8 lines · 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:

Quoted 8 lines

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.

Quoted 4 lines

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.

include/fdtdec.h enum fdt_compat_id { · line 66

Quoted 4 lines

include/fdtdec.h +3 -0

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

Hi Stephen,

Quoted 18 lines · Stephen Warren

Quoted 8 lines · 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:

Quoted 8 lines

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)

Quoted 6 lines

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?

Quoted 8 lines

include/fdtdec.h enum fdt_compat_id { · line 66

Quoted 3 lines

include/fdtdec.h +3 -0

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

Quoted 37 lines · Simon Glass

Hi Stephen,

Quoted 18 lines · Stephen Warren

Quoted 8 lines · 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:

Quoted 8 lines

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)

Quoted 6 lines

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.

Quoted 1 line

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.

Quoted 11 lines

Quoted 9 lines

Review

Quoted 4 lines

(quoted diff) +3 -0

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

Quoted 4 lines

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

Hi Stephen,

Quoted 40 lines · Stephen Warren

Quoted 38 lines · Simon Glass

Hi Stephen,

Quoted 18 lines · Stephen Warren

Quoted 8 lines · 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:

Quoted 8 lines

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)

Quoted 6 lines

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?

Quoted 3 lines

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

Quoted 2 lines

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!

Quoted 43 lines · Simon Glass

Hi Stephen,

Quoted 40 lines · Stephen Warren

Quoted 38 lines · Simon Glass

Hi Stephen,

Quoted 18 lines · Stephen Warren

Quoted 8 lines · 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:

Quoted 8 lines

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)

Quoted 6 lines

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?

Quoted 7 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 3 lines

I have not used u8 for any
function calls and would not.

Quoted 2 lines

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

Quoted 7 lines · Stephen Warren

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.

Quoted 6 lines

Quoted 3 lines

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!

Quoted 9 lines · On Monday 05 December 2011 17:52:01 Simon Glass wrote:

Quoted 7 lines · Stephen Warren

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

Quoted 3 lines

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.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 3 lines

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

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.

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

+ */

arch/arm/dts/tegra20.dtsi +15 -0

+
+	usbtiming@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@1 {
+		compatible = "nvidia,tegra20-usbtiming";
+		osc-frequency = <19200000>;
+		timing = <0x0c8 0x04 0x00 0x3 0  0x03 0x4b 0x06 0xbb 0xbb80 7>;
+	};
+
+	usbtiming@2 {

Quoted 8 lines · 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).

Quoted 2 lines

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.

Quoted 2 lines

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

This comment block should be indented to match the nodes.

Quoted 13 lines

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

Quoted 8 lines

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

Quoted 17 lines

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

Quoted 1 line

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

Quoted 23 lines

+ */

arch/arm/dts/tegra20.dtsi +21 -0

+
+     usbtiming@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@1 {
+             compatible = "nvidia,tegra20-usbtiming";
+             osc-frequency = <19200000>;
+             timing = <0x0c8 0x04 0x00 0x3 0  0x03 0x4b 0x06 0xbb 0xbb80 7>;
+     };
+
+     usbtiming@2 {
+             compatible = "nvidia,tegra20-usbtiming";
+             osc-frequency = <12000000>;
+             timing = <0x3c0 0x0c 0x00 0xc 0  0x02 0x2f 0x04 0x76 0x7530 5>;
+     };
+
+     usbtiming@3 {

Quoted 4 lines

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

Quoted 8 lines

+
      usb@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

Quoted 12 lines · Stephen Warren

Quoted 10 lines · 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.

Quoted 3 lines

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

Quoted 23 lines

Quoted 18 lines

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

Quoted 4 lines

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

Quoted 5 lines

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.

Quoted 20 lines

Quoted 17 lines

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

Quoted 6 lines

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 5 lines

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?

Quoted 8 lines

Quoted 7 lines

arch/arm/dts/tegra20.dtsi +2 -0

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

Quoted 3 lines

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.

Quoted 17 lines · Simon Glass

Quoted 15 lines · Stephen Warren

Quoted 10 lines · 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.

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.

Quoted 3 lines

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.

Quoted 53 lines

Quoted 24 lines

Quoted 15 lines

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

Quoted 8 lines

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

Quoted 5 lines

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.

Quoted 20 lines

Quoted 17 lines

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

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 13 lines

Quoted 12 lines

Quoted 8 lines

+
      usb@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.

Quoted 1 line

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

Quoted 25 lines · Stephen Warren

Quoted 24 lines · Simon Glass

Quoted 22 lines · Stephen Warren

Quoted 11 lines · 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.

Quoted 6 lines

      usb@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.

Quoted 5 lines

Quoted 1 line

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?

Quoted 4 lines

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.

Quoted 26 lines · Simon Glass

Quoted 25 lines · Stephen Warren

Quoted 24 lines · Simon Glass

Quoted 22 lines · Stephen Warren

Quoted 11 lines · 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.

Quoted 6 lines

      usb@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.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 4 lines

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

Hi Stephen,

Quoted 84 lines · Stephen Warren

Quoted 26 lines · Simon Glass

Quoted 25 lines · Stephen Warren

Quoted 24 lines · Simon Glass

Quoted 22 lines · Stephen Warren

Quoted 11 lines · 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.

Quoted 6 lines

      usb@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.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 4 lines

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

Quoted 7 lines

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.

Quoted 95 lines · Simon Glass

Hi Stephen,

Quoted 84 lines · Stephen Warren

Quoted 26 lines · Simon Glass

Quoted 25 lines · Stephen Warren

Quoted 24 lines · Simon Glass

Quoted 22 lines · Stephen Warren

Quoted 11 lines · 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.

Quoted 6 lines

      usb@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.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 4 lines

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

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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

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

board/nvidia/dts/tegra2-seaboard.dts +4 -0

+	};
+
+	usb@c5008000 {
+		status = "okay";
 	};
 };

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 <[email protected]>

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

README +0 -0

index fda0190..3194846 100644
@@ -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

drivers/usb/host/ehci-hcd.c +7 -0

index 2197119..23458b6 100644
@@ -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) {

Hi,

2011/12/3 Simon Glass <[email protected]>:

Quoted 53 lines

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 <[email protected]>

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

README +0 -0

index fda0190..3194846 100644
@@ -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

drivers/usb/host/ehci-hcd.c +14 -0

index 2197119..23458b6 100644
@@ -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));

drivers/usb/host/ehci-hcd.c ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, · line 341

Quoted 3 lines

drivers/usb/host/ehci-hcd.c +3 -0

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

Quoted 5 lines

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.

Hi Simon,

2011/12/6 Simon Glass <[email protected]>:

Quoted 11 lines

Quoted 5 lines

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.

Quoted 6 lines

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

drivers/usb/host/ehci.h +0 -0

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

drivers/usb/host/ehci.h struct ehci_hcor { · line 83

Quoted 13 lines · Simon Glass

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

drivers/usb/host/ehci.h +1 -1

index 3d0ad0c..cc00ce4 100644
@@ -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_ ...

drivers/usb/host/ehci.h struct ehci_hcor { · line 87

Quoted 4 lines

drivers/usb/host/ehci.h +4 -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.

Quoted 4 lines

drivers/usb/host/ehci.h +0 -0

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

Quoted 17 lines · Simon Glass

drivers/usb/host/ehci.h struct ehci_hcor { · line 83

Quoted 13 lines

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

drivers/usb/host/ehci.h +1 -1

index 3d0ad0c..cc00ce4 100644
@@ -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_ ...

Quoted 7 lines

drivers/usb/host/ehci.h struct ehci_hcor { · line 87

Quoted 4 lines

drivers/usb/host/ehci.h +4 -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?

Simon Glass wrote at Monday, December 05, 2011 7:03 PM:

Quoted 35 lines · Stephen Warren

Quoted 17 lines · Simon Glass

drivers/usb/host/ehci.h struct ehci_hcor { · line 83

Quoted 13 lines

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

drivers/usb/host/ehci.h +1 -1

index 3d0ad0c..cc00ce4 100644
@@ -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_ ...

Quoted 7 lines

drivers/usb/host/ehci.h struct ehci_hcor { · line 87

Quoted 4 lines

drivers/usb/host/ehci.h +4 -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;

Quoted 3 lines

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

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 <[email protected]>

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