[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[Anton Vorontsov]

Linux understands "host" (default), "peripheral" and "otg" (broken).
Though, U-Boot doesn't restrict dr_mode variable to these values (think
of renames in future).

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 board/freescale/mpc837xerdb/mpc837xerdb.c |    1 +
 common/fdt_support.c                      |   25 +++++++++++++++++++++++++
 include/configs/MPC837XERDB.h             |    1 +
 include/fdt_support.h                     |    6 ++++++
 4 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/board/freescale/mpc837xerdb/mpc837xerdb.c b/board/freescale/mpc837xerdb/mpc837xerdb.c
index 992c9cf..0ccac7b 100644
--- a/board/freescale/mpc837xerdb/mpc837xerdb.c
+++ b/board/freescale/mpc837xerdb/mpc837xerdb.c
@@ -148,5 +148,6 @@ void ft_board_setup(void *blob, bd_t *bd)
 	ft_pci_setup(blob, bd);
 #endif
 	ft_cpu_setup(blob, bd);
+	fdt_fixup_dr_usb(blob, bd);
 }
 #endif /* CONFIG_OF_BOARD_SETUP */
diff --git a/common/fdt_support.c b/common/fdt_support.c
index 69eb667..10b66fc 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -615,3 +615,28 @@ void fdt_fixup_ethernet(void *fdt, bd_t *bd)
 	}
 }
 #endif
+
+#ifdef CONFIG_HAS_FSL_DR_USB
+void fdt_fixup_dr_usb(void *blob, bd_t *bd)
+{
+	char *mode;
+	const char *compat = "fsl-usb2-dr";
+	const char *prop = "dr_mode";
+	int node_offset;
+	int err;
+
+	mode = getenv("usb_dr_mode");
+	if (!mode)
+		return;
+
+	node_offset = fdt_node_offset_by_compatible(blob, 0, compat);
+	if (node_offset < 0)
+		printf("WARNING: could not find compatible node %s: %s.\n",
+			compat, fdt_strerror(node_offset));
+
+	err = fdt_setprop(blob, node_offset, prop, mode, strlen(mode) + 1);
+	if (err < 0)
+		printf("WARNING: could not set %s for %s: %s.\n",
+		       prop, compat, fdt_strerror(err));
+}
+#endif /* CONFIG_HAS_FSL_DR_USB */
diff --git a/include/configs/MPC837XERDB.h b/include/configs/MPC837XERDB.h
index 033a5fb..2287fbc 100644
--- a/include/configs/MPC837XERDB.h
+++ b/include/configs/MPC837XERDB.h
@@ -576,6 +576,7 @@
  */
 #define CONFIG_ENV_OVERWRITE
 
+#define CONFIG_HAS_FSL_DR_USB
 #define CONFIG_HAS_ETH0				/* add support for "ethaddr" */
 #define CONFIG_ETHADDR	00:04:9f:ef:04:01
 #define CONFIG_HAS_ETH1				/* add support for "eth1addr" */
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 7836f28..c10de8a 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -50,6 +50,12 @@ int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
 			 const void *val, int len, int create);
 void fdt_fixup_qe_firmware(void *fdt);
 
+#ifdef CONFIG_HAS_FSL_DR_USB
+void fdt_fixup_dr_usb(void *blob, bd_t *bd);
+#else
+static inline void fdt_fixup_dr_usb(void *blob, bd_t *bd) {}
+#endif /* CONFIG_HAS_FSL_DR_USB */
+
 #ifdef CONFIG_OF_HAS_UBOOT_ENV
 int fdt_env(void *fdt);
 #endif
-- 
1.5.2.2

[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[Kim Phillips]

On Fri, 14 Mar 2008 23:20:18 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:


> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 7836f28..c10de8a 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -50,6 +50,12 @@ int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
>  			 const void *val, int len, int create);
>  void fdt_fixup_qe_firmware(void *fdt);
>  
> +#ifdef CONFIG_HAS_FSL_DR_USB
> +void fdt_fixup_dr_usb(void *blob, bd_t *bd);
> +#else
> +static inline void fdt_fixup_dr_usb(void *blob, bd_t *bd) {}
> +#endif /* CONFIG_HAS_FSL_DR_USB */
> +

this looks like a prime candidate for a weak function (which would also
eliminate the need for the new CONFIG_HAS_FSL_DR_USB introduced here).

Kim

[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[Anton Vorontsov]

On Wed, Mar 19, 2008 at 08:35:58PM -0500, Kim Phillips wrote:
> On Fri, 14 Mar 2008 23:20:18 +0300
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> 
> > diff --git a/include/fdt_support.h b/include/fdt_support.h
> > index 7836f28..c10de8a 100644
> > --- a/include/fdt_support.h
> > +++ b/include/fdt_support.h
> > @@ -50,6 +50,12 @@ int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
> >  			 const void *val, int len, int create);
> >  void fdt_fixup_qe_firmware(void *fdt);
> >  
> > +#ifdef CONFIG_HAS_FSL_DR_USB
> > +void fdt_fixup_dr_usb(void *blob, bd_t *bd);
> > +#else
> > +static inline void fdt_fixup_dr_usb(void *blob, bd_t *bd) {}
> > +#endif /* CONFIG_HAS_FSL_DR_USB */
> > +
> 
> this looks like a prime candidate for a weak function (which would also
> eliminate the need for the new CONFIG_HAS_FSL_DR_USB introduced here).

Are you sure it's supposed to work like this?.. I don't see the linker
dropping unused weak symbols with the patch below.

diff --git a/board/freescale/mpc837xerdb/mpc837xerdb.c b/board/freescale/mpc837xerdb/mpc837xerdb.c
index d091538..70f52da 100644
--- a/board/freescale/mpc837xerdb/mpc837xerdb.c
+++ b/board/freescale/mpc837xerdb/mpc837xerdb.c
@@ -13,6 +13,7 @@
  */
 
 #include <common.h>
+#include <fdt_support.h>
 #include <i2c.h>
 #include <asm/io.h>
 #include <asm/fsl_serdes.h>
@@ -197,5 +198,6 @@ void ft_board_setup(void *blob, bd_t *bd)
 	ft_pci_setup(blob, bd);
 #endif
 	ft_cpu_setup(blob, bd);
+	/* fdt_fixup_dr_usb(blob); */
 }
 #endif /* CONFIG_OF_BOARD_SETUP */
diff --git a/common/fdt_support.c b/common/fdt_support.c
index 69eb667..22a2d99 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -615,3 +615,26 @@ void fdt_fixup_ethernet(void *fdt, bd_t *bd)
 	}
 }
 #endif
+
+void __attribute__((weak)) fdt_fixup_dr_usb(void *fdt)
+{
+	char *mode;
+	const char *compat = "fsl-usb2-dr";
+	const char *prop = "dr_mode";
+	int node_offset;
+	int err;
+
+	mode = getenv("usb_dr_mode");
+	if (!mode)
+		return;
+
+	node_offset = fdt_node_offset_by_compatible(fdt, 0, compat);
+	if (node_offset < 0)
+		printf("WARNING: could not find compatible node %s: %s.\n",
+			compat, fdt_strerror(node_offset));
+
+	err = fdt_setprop(fdt, node_offset, prop, mode, strlen(mode) + 1);
+	if (err < 0)
+		printf("WARNING: could not set %s for %s: %s.\n",
+		       prop, compat, fdt_strerror(err));
+}
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 7836f28..9e980b8 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -49,6 +49,7 @@ void fdt_fixup_ethernet(void *fdt, bd_t *bd);
 int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
 			 const void *val, int len, int create);
 void fdt_fixup_qe_firmware(void *fdt);
+void __attribute__((weak)) fdt_fixup_dr_usb(void *fdt);
 
 #ifdef CONFIG_OF_HAS_UBOOT_ENV
 int fdt_env(void *fdt);
-- 
1.5.2.2

[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[Kim Phillips]

On Mon, 24 Mar 2008 17:44:19 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> On Wed, Mar 19, 2008 at 08:35:58PM -0500, Kim Phillips wrote:
> > On Fri, 14 Mar 2008 23:20:18 +0300
> > Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > 
> > 
> > > diff --git a/include/fdt_support.h b/include/fdt_support.h
> > > index 7836f28..c10de8a 100644
> > > --- a/include/fdt_support.h
> > > +++ b/include/fdt_support.h
> > > @@ -50,6 +50,12 @@ int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
> > >  			 const void *val, int len, int create);
> > >  void fdt_fixup_qe_firmware(void *fdt);
> > >  
> > > +#ifdef CONFIG_HAS_FSL_DR_USB
> > > +void fdt_fixup_dr_usb(void *blob, bd_t *bd);
> > > +#else
> > > +static inline void fdt_fixup_dr_usb(void *blob, bd_t *bd) {}
> > > +#endif /* CONFIG_HAS_FSL_DR_USB */
> > > +
> > 
> > this looks like a prime candidate for a weak function (which would also
> > eliminate the need for the new CONFIG_HAS_FSL_DR_USB introduced here).
> 
> Are you sure it's supposed to work like this?.. I don't see the linker
> dropping unused weak symbols with the patch below.

with something like the following I get 24 bytes more text in
fdt_support.o, and 200 total bytes added 83xx-wide:

diff --git a/board/freescale/mpc837xerdb/mpc837xerdb.c b/board/freescale/mpc837xerdb/mpc837xerdb.c
index a3b20b8..e8d130c 100644
--- a/board/freescale/mpc837xerdb/mpc837xerdb.c
+++ b/board/freescale/mpc837xerdb/mpc837xerdb.c
@@ -13,6 +13,7 @@
  */
 
 #include <common.h>
+#include <fdt_support.h>
 #include <i2c.h>
 #include <asm/io.h>
 #include <spd_sdram.h>
@@ -153,6 +154,7 @@ int misc_init_r(void)
 }
 
 #if defined(CONFIG_OF_BOARD_SETUP)
+extern void fdt_fixup_dr_usb(void *);
 
 void ft_board_setup(void *blob, bd_t *bd)
 {
@@ -160,5 +162,6 @@ void ft_board_setup(void *blob, bd_t *bd)
 	ft_pci_setup(blob, bd);
 #endif
 	ft_cpu_setup(blob, bd);
+	fdt_fixup_dr_usb(blob);
 }
 #endif /* CONFIG_OF_BOARD_SETUP */
diff --git a/common/fdt_support.c b/common/fdt_support.c
index 69eb667..355b741 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -615,3 +615,10 @@ void fdt_fixup_ethernet(void *fdt, bd_t *bd)
 	}
 }
 #endif
+
+void __fdt_fixup_dr_usb(void *fdt)
+{
+	/* add your own */
+}
+void fdt_fixup_dr_usb(void *fdt) __attribute__((weak,
+	alias("__fdt_fixup_dr_usb")));
diff --git a/cpu/mpc83xx/fdt.c b/cpu/mpc83xx/fdt.c
index 6f55932..3224286 100644
--- a/cpu/mpc83xx/fdt.c
+++ b/cpu/mpc83xx/fdt.c
@@ -68,4 +68,27 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 	fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize);
 }
+
+void fdt_fixup_dr_usb(void *fdt)
+{
+	char *mode;
+	const char *compat = "fsl-usb2-dr";
+	const char *prop = "dr_mode";
+	int node_offset;
+	int err;
+
+	mode = getenv("usb_dr_mode");
+	if (!mode)
+		return;
+
+	node_offset = fdt_node_offset_by_compatible(fdt, 0, compat);
+	if (node_offset < 0)
+		printf("WARNING: could not find compatible node %s: %s.\n",
+			compat, fdt_strerror(node_offset));
+
+	err = fdt_setprop(fdt, node_offset, prop, mode, strlen(mode) + 1);
+	if (err < 0)
+		printf("WARNING: could not set %s for %s: %s.\n",
+		       prop, compat, fdt_strerror(err));
+}
 #endif /* CONFIG_OF_LIBFDT */

but the QE based 83xx SoCs don't use this fixup.  So while
CONFIG_HAS_FSL_DR_USB gives you more control, I doubt saving 200 bytes
justifies the new config either.  Either way really, but I tend to be
against any new CONFIG_s.

Kim

[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[Anton Vorontsov]

On Mon, Mar 24, 2008 at 06:00:49PM -0500, Kim Phillips wrote:
> On Mon, 24 Mar 2008 17:44:19 +0300
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > On Wed, Mar 19, 2008 at 08:35:58PM -0500, Kim Phillips wrote:
> > > On Fri, 14 Mar 2008 23:20:18 +0300
> > > Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > > 
> > > 
> > > > diff --git a/include/fdt_support.h b/include/fdt_support.h
> > > > index 7836f28..c10de8a 100644
> > > > --- a/include/fdt_support.h
> > > > +++ b/include/fdt_support.h
> > > > @@ -50,6 +50,12 @@ int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
> > > >  			 const void *val, int len, int create);
> > > >  void fdt_fixup_qe_firmware(void *fdt);
> > > >  
> > > > +#ifdef CONFIG_HAS_FSL_DR_USB
> > > > +void fdt_fixup_dr_usb(void *blob, bd_t *bd);
> > > > +#else
> > > > +static inline void fdt_fixup_dr_usb(void *blob, bd_t *bd) {}
> > > > +#endif /* CONFIG_HAS_FSL_DR_USB */
> > > > +
> > > 
> > > this looks like a prime candidate for a weak function (which would also
> > > eliminate the need for the new CONFIG_HAS_FSL_DR_USB introduced here).
> > 
> > Are you sure it's supposed to work like this?.. I don't see the linker
> > dropping unused weak symbols with the patch below.
> 
> with something like the following I get 24 bytes more text in
> fdt_support.o, and 200 total bytes added 83xx-wide:
> 
[...]
> but the QE based 83xx SoCs don't use this fixup.  So while
> CONFIG_HAS_FSL_DR_USB gives you more control, I doubt saving 200 bytes
> justifies the new config either.  Either way really, but I tend to be
> against any new CONFIG_s.

Hm... maybe then it's better to place fdt_fixup_usb_dr function
inside the fdt_support.h, marking it with static inline -- it's used
just once per board anyway..? Then neither space wasted nor the new
configs introduced.

-- 
Anton Vorontsov
email: cboumailru at gmail.com
irc://irc.freenode.net/bd2

[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[Kim Phillips]

On Tue, 25 Mar 2008 16:48:54 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> On Mon, Mar 24, 2008 at 06:00:49PM -0500, Kim Phillips wrote:
> > On Mon, 24 Mar 2008 17:44:19 +0300
> > Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > 
> > > On Wed, Mar 19, 2008 at 08:35:58PM -0500, Kim Phillips wrote:
> > > > On Fri, 14 Mar 2008 23:20:18 +0300
> > > > Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > > > 
> > > > 
> > > > > diff --git a/include/fdt_support.h b/include/fdt_support.h
> > > > > index 7836f28..c10de8a 100644
> > > > > --- a/include/fdt_support.h
> > > > > +++ b/include/fdt_support.h
> > > > > @@ -50,6 +50,12 @@ int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
> > > > >  			 const void *val, int len, int create);
> > > > >  void fdt_fixup_qe_firmware(void *fdt);
> > > > >  
> > > > > +#ifdef CONFIG_HAS_FSL_DR_USB
> > > > > +void fdt_fixup_dr_usb(void *blob, bd_t *bd);
> > > > > +#else
> > > > > +static inline void fdt_fixup_dr_usb(void *blob, bd_t *bd) {}
> > > > > +#endif /* CONFIG_HAS_FSL_DR_USB */
> > > > > +
> > > > 
> > > > this looks like a prime candidate for a weak function (which would also
> > > > eliminate the need for the new CONFIG_HAS_FSL_DR_USB introduced here).
> > > 
> > > Are you sure it's supposed to work like this?.. I don't see the linker
> > > dropping unused weak symbols with the patch below.
> > 
> > with something like the following I get 24 bytes more text in
> > fdt_support.o, and 200 total bytes added 83xx-wide:
> > 
> [...]
> > but the QE based 83xx SoCs don't use this fixup.  So while
> > CONFIG_HAS_FSL_DR_USB gives you more control, I doubt saving 200 bytes
> > justifies the new config either.  Either way really, but I tend to be
> > against any new CONFIG_s.
> 
> Hm... maybe then it's better to place fdt_fixup_usb_dr function
> inside the fdt_support.h, marking it with static inline -- it's used
> just once per board anyway..? Then neither space wasted nor the new
> configs introduced.

I'd like to see that.  Do you want to send a patch?

Kim

[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[Anton Vorontsov]

On Tue, Mar 25, 2008 at 10:42:11AM -0500, Kim Phillips wrote:
[...]
> > > but the QE based 83xx SoCs don't use this fixup.  So while
> > > CONFIG_HAS_FSL_DR_USB gives you more control, I doubt saving 200 bytes
> > > justifies the new config either.  Either way really, but I tend to be
> > > against any new CONFIG_s.
> > 
> > Hm... maybe then it's better to place fdt_fixup_usb_dr function
> > inside the fdt_support.h, marking it with static inline -- it's used
> > just once per board anyway..? Then neither space wasted nor the new
> > configs introduced.
> 
> I'd like to see that.  Do you want to send a patch?

Sure, here it is.

- - - -
From: Anton Vorontsov <avorontsov@ru.mvista.com>
Subject: mpc83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

Linux understands "host" (default), "peripheral" and "otg" (broken).
Though, U-Boot doesn't restrict dr_mode variable to these values (think
of renames in future).

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 board/freescale/mpc837xerdb/mpc837xerdb.c |    2 ++
 include/fdt_support.h                     |   24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/board/freescale/mpc837xerdb/mpc837xerdb.c b/board/freescale/mpc837xerdb/mpc837xerdb.c
index d091538..70f52da 100644
--- a/board/freescale/mpc837xerdb/mpc837xerdb.c
+++ b/board/freescale/mpc837xerdb/mpc837xerdb.c
@@ -13,6 +13,7 @@
  */
 
 #include <common.h>
+#include <fdt_support.h>
 #include <i2c.h>
 #include <asm/io.h>
 #include <asm/fsl_serdes.h>
@@ -197,5 +198,6 @@ void ft_board_setup(void *blob, bd_t *bd)
 	ft_pci_setup(blob, bd);
 #endif
 	ft_cpu_setup(blob, bd);
+	fdt_fixup_dr_usb(blob);
 }
 #endif /* CONFIG_OF_BOARD_SETUP */
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 7836f28..933ef41 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -27,6 +27,7 @@
 #ifdef CONFIG_OF_LIBFDT
 
 #include <fdt.h>
+#include <libfdt.h>
 
 int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force);
 void do_fixup_by_path(void *fdt, const char *path, const char *prop,
@@ -64,5 +65,28 @@ void ft_cpu_setup(void *blob, bd_t *bd);
 void ft_pci_setup(void *blob, bd_t *bd);
 #endif
 
+static inline void fdt_fixup_dr_usb(void *fdt)
+{
+	char *mode;
+	const char *compat = "fsl-usb2-dr";
+	const char *prop = "dr_mode";
+	int node_offset;
+	int err;
+
+	mode = getenv("usb_dr_mode");
+	if (!mode)
+		return;
+
+	node_offset = fdt_node_offset_by_compatible(fdt, 0, compat);
+	if (node_offset < 0)
+		printf("WARNING: could not find compatible node %s: %s.\n",
+			compat, fdt_strerror(node_offset));
+
+	err = fdt_setprop(fdt, node_offset, prop, mode, strlen(mode) + 1);
+	if (err < 0)
+		printf("WARNING: could not set %s for %s: %s.\n",
+		       prop, compat, fdt_strerror(err));
+}
+
 #endif /* ifdef CONFIG_OF_LIBFDT */
 #endif /* ifndef __FDT_SUPPORT_H */
-- 
1.5.2.2

[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[Jerry Van Baren]

Anton Vorontsov wrote:
> On Tue, Mar 25, 2008 at 10:42:11AM -0500, Kim Phillips wrote:
> [...]
>>>> but the QE based 83xx SoCs don't use this fixup.  So while
>>>> CONFIG_HAS_FSL_DR_USB gives you more control, I doubt saving 200 bytes
>>>> justifies the new config either.  Either way really, but I tend to be
>>>> against any new CONFIG_s.
>>> Hm... maybe then it's better to place fdt_fixup_usb_dr function
>>> inside the fdt_support.h, marking it with static inline -- it's used
>>> just once per board anyway..? Then neither space wasted nor the new
>>> configs introduced.
>> I'd like to see that.  Do you want to send a patch?
> 
> Sure, here it is.
> 
> - - - -
> From: Anton Vorontsov <avorontsov@ru.mvista.com>
> Subject: mpc83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role
> 
> Linux understands "host" (default), "peripheral" and "otg" (broken).
> Though, U-Boot doesn't restrict dr_mode variable to these values (think
> of renames in future).
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  board/freescale/mpc837xerdb/mpc837xerdb.c |    2 ++
>  include/fdt_support.h                     |   24 ++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/board/freescale/mpc837xerdb/mpc837xerdb.c b/board/freescale/mpc837xerdb/mpc837xerdb.c
> index d091538..70f52da 100644
> --- a/board/freescale/mpc837xerdb/mpc837xerdb.c
> +++ b/board/freescale/mpc837xerdb/mpc837xerdb.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <common.h>
> +#include <fdt_support.h>
>  #include <i2c.h>
>  #include <asm/io.h>
>  #include <asm/fsl_serdes.h>
> @@ -197,5 +198,6 @@ void ft_board_setup(void *blob, bd_t *bd)
>  	ft_pci_setup(blob, bd);
>  #endif
>  	ft_cpu_setup(blob, bd);
> +	fdt_fixup_dr_usb(blob);
>  }
>  #endif /* CONFIG_OF_BOARD_SETUP */
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 7836f28..933ef41 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -27,6 +27,7 @@
>  #ifdef CONFIG_OF_LIBFDT
>  
>  #include <fdt.h>
> +#include <libfdt.h>
>  
>  int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force);
>  void do_fixup_by_path(void *fdt, const char *path, const char *prop,
> @@ -64,5 +65,28 @@ void ft_cpu_setup(void *blob, bd_t *bd);
>  void ft_pci_setup(void *blob, bd_t *bd);
>  #endif
>  
> +static inline void fdt_fixup_dr_usb(void *fdt)
> +{
> +	char *mode;
> +	const char *compat = "fsl-usb2-dr";
> +	const char *prop = "dr_mode";
> +	int node_offset;
> +	int err;
> +
> +	mode = getenv("usb_dr_mode");
> +	if (!mode)
> +		return;
> +
> +	node_offset = fdt_node_offset_by_compatible(fdt, 0, compat);
> +	if (node_offset < 0)
> +		printf("WARNING: could not find compatible node %s: %s.\n",
> +			compat, fdt_strerror(node_offset));
> +
> +	err = fdt_setprop(fdt, node_offset, prop, mode, strlen(mode) + 1);
> +	if (err < 0)
> +		printf("WARNING: could not set %s for %s: %s.\n",
> +		       prop, compat, fdt_strerror(err));
> +}
> +
>  #endif /* ifdef CONFIG_OF_LIBFDT */
>  #endif /* ifndef __FDT_SUPPORT_H */

I have reservations on this:
1) We are adding code to a header file (arguably unavoidable, saving 
grace is that it is static inline so it presumably doesn't bloat boards 
that don't use it).

2) This is CPU-specific in a general purpose fdt_support.h header file.

Is anybody else's tail tingling[1]?

Best regards,
gvb

[1] http://en.wikipedia.org/wiki/Verne_(Over_the_Hedge)

[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[Kim Phillips]

On Tue, 25 Mar 2008 19:06:34 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> On Tue, Mar 25, 2008 at 10:42:11AM -0500, Kim Phillips wrote:
> [...]
> > > > but the QE based 83xx SoCs don't use this fixup.  So while
> > > > CONFIG_HAS_FSL_DR_USB gives you more control, I doubt saving 200 bytes
> > > > justifies the new config either.  Either way really, but I tend to be
> > > > against any new CONFIG_s.
> > > 
> > > Hm... maybe then it's better to place fdt_fixup_usb_dr function
> > > inside the fdt_support.h, marking it with static inline -- it's used
> > > just once per board anyway..? Then neither space wasted nor the new
> > > configs introduced.
> > 
> > I'd like to see that.  Do you want to send a patch?
> 
> Sure, here it is.
> 
> - - - -
> From: Anton Vorontsov <avorontsov@ru.mvista.com>
> Subject: mpc83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role
> 
> Linux understands "host" (default), "peripheral" and "otg" (broken).
> Though, U-Boot doesn't restrict dr_mode variable to these values (think
> of renames in future).
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  board/freescale/mpc837xerdb/mpc837xerdb.c |    2 ++
>  include/fdt_support.h                     |   24 ++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/board/freescale/mpc837xerdb/mpc837xerdb.c b/board/freescale/mpc837xerdb/mpc837xerdb.c
> index d091538..70f52da 100644
> --- a/board/freescale/mpc837xerdb/mpc837xerdb.c
> +++ b/board/freescale/mpc837xerdb/mpc837xerdb.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <common.h>
> +#include <fdt_support.h>
>  #include <i2c.h>
>  #include <asm/io.h>
>  #include <asm/fsl_serdes.h>
> @@ -197,5 +198,6 @@ void ft_board_setup(void *blob, bd_t *bd)
>  	ft_pci_setup(blob, bd);
>  #endif
>  	ft_cpu_setup(blob, bd);
> +	fdt_fixup_dr_usb(blob);
>  }
>  #endif /* CONFIG_OF_BOARD_SETUP */
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 7836f28..933ef41 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -27,6 +27,7 @@
>  #ifdef CONFIG_OF_LIBFDT
>  
>  #include <fdt.h>
> +#include <libfdt.h>
>  
>  int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force);
>  void do_fixup_by_path(void *fdt, const char *path, const char *prop,
> @@ -64,5 +65,28 @@ void ft_cpu_setup(void *blob, bd_t *bd);
>  void ft_pci_setup(void *blob, bd_t *bd);
>  #endif
>  
> +static inline void fdt_fixup_dr_usb(void *fdt)
> +{
> +	char *mode;
> +	const char *compat = "fsl-usb2-dr";
> +	const char *prop = "dr_mode";
> +	int node_offset;
> +	int err;
> +
> +	mode = getenv("usb_dr_mode");
> +	if (!mode)
> +		return;
> +
> +	node_offset = fdt_node_offset_by_compatible(fdt, 0, compat);
> +	if (node_offset < 0)
> +		printf("WARNING: could not find compatible node %s: %s.\n",
> +			compat, fdt_strerror(node_offset));
> +
> +	err = fdt_setprop(fdt, node_offset, prop, mode, strlen(mode) + 1);
> +	if (err < 0)
> +		printf("WARNING: could not set %s for %s: %s.\n",
> +		       prop, compat, fdt_strerror(err));
> +}
> +
>  #endif /* ifdef CONFIG_OF_LIBFDT */
>  #endif /* ifndef __FDT_SUPPORT_H */

<sigh> yeah, that's too much code for a header file though.

I think your original patch was the more prudent approach, it follows
existing practice and it fits with the assumption that there'll be a
usb driver one day :).  I'll put it in my queue and apply it unless
anyone has further comments.

Kim

[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[Jerry Van Baren]

Kim Phillips wrote:

[snip]

> <sigh> yeah, that's too much code for a header file though.
> 
> I think your original patch was the more prudent approach, it follows
> existing practice and it fits with the assumption that there'll be a
> usb driver one day :).  I'll put it in my queue and apply it unless
> anyone has further comments.
> 
> Kim

Yup, Kim's tail was tingling too.  :-D

Best regards,
gvb

[U-Boot-Users] [PATCH 6/7] 83xx/fdt_support: let user specifiy FSL USB Dual-Role controller role

[Kim Phillips]

On Fri, 14 Mar 2008 23:20:18 +0300
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> Linux understands "host" (default), "peripheral" and "otg" (broken).
> Though, U-Boot doesn't restrict dr_mode variable to these values (think
> of renames in future).
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  board/freescale/mpc837xerdb/mpc837xerdb.c |    1 +
>  common/fdt_support.c                      |   25 +++++++++++++++++++++++++
>  include/configs/MPC837XERDB.h             |    1 +
>  include/fdt_support.h                     |    6 ++++++
>  4 files changed, 33 insertions(+), 0 deletions(-)
> 
applied, thanks.

Kim