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

[Jerry Van Baren <gerald.vanbaren@ge.com>]

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)