[U-Boot] Patch to add support for BAV335x boards

Tue Feb 3 12:57:39 CET 2015

Dear Gilles,

In message <3EFCB679-8DF9-43F2-B3F4-FBC00E578061 at gigadevices.com> you wrote:
> 
> I just added support for the BAV335x OEM boards =
> http://birdland.com/oem/bav335x-network-processor/
> 
> This patch was generated with the patman tool which reported no checks, =
> warnings nor errors.
> 
> The patch is relative to the v2015.01 tag.

THis being a version 2 of the patch, you should have marked it as such
in the Subject, and provides a Changelog.

> Content-Disposition: attachment;
> 	filename=0001-Adding-Support-for-BAV335x-boards.patch
> Content-Type: application/octet-stream;
> 	name="0001-Adding-Support-for-BAV335x-boards.patch"
> Content-Transfer-Encoding: quoted-printable

Also, I asked you before NOT to send patches as MIME attachments.
Please make sure to send them inline, ideally using git-send-email.

This being a TI based board, you should have sent it to the TI
custodian.

> From a4d163542a7acbcc1ac5f520f41966f373b9f307 Mon Sep 17 00:00:00 2001
> From: Gilles Gameiro <gilles at gigadevices.com>
> Date: Sun, 1 Feb 2015 19:30:01 -0800
> Subject: [PATCH] Adding Support for BAV335x boards
> 
> Signed-off-by: Gilles Gameiro <gilles at gigadevices.com>

Please add a description of the board to the commit message.

> @@ -969,6 +982,7 @@ source "board/taskit/stamp9g20/Kconfig"
>  source "board/tbs/tbs2910/Kconfig"
>  source "board/ti/am335x/Kconfig"
>  source "board/ti/am43xx/Kconfig"
> +source "board/birdland/bav335x/Kconfig"
>  source "board/ti/ti814x/Kconfig"
>  source "board/ti/ti816x/Kconfig"
>  source "board/ti/tnetv107xevm/Kconfig"

Please keep lists sorted.

> +config CONS_INDEX
> +	int "UART used for console"
> +	range 1 6
> +	default 1
> +	help
> +	  The AM335x SoC has a total of 6 UARTs (UART0 to UART5 as referenced
> +	  in documentation, etc) available to it.  Depending on your specific
> +	  board you may want something other than UART0 as for example the IDK
> +	  uses UART3 so enter 4 here.

Is this useful and correct information for your board?

> --- /dev/null
> +++ b/board/birdland/bav335x/Makefile
> @@ -0,0 +1,11 @@
> +#
> +# Makefile
> +#
> +# Copyright (C) 2012-2014, Birdland Audio - http://birdland.com/oem
> +#

SPDX ID missing.

> +++ b/board/birdland/bav335x/README
> @@ -0,0 +1,31 @@
> +Summary
> +=======

SPDX ID missing.  Please fix globally for all new files.

> +static __maybe_unused struct ctrl_dev *cdev =
> +		(struct ctrl_dev *)CTRL_DEVICE_BASE;
> +
> +
> +
> +/*

Excessive white space - please use a single blank line.  Please fix globally.

> +		if (header.version[1] == 'A') {
> +			if (debug)
> +				puts("=== Detected Board model BAV335x Rev.A");

Use debug() instead.  Please fix globally.

...
> +void sdram_init(void)
> +{
> +	config_ddr(400, &ioregs_bonelt,
> +		   &ddr3_bav335x_data,
> +		   &ddr3_bav335x_cmd_ctrl_data,
> +		   &ddr3_bav335x_emif_reg_data, 0);
> +}
> +#endif

It might be useful to add a comment so it is easier to see which "#if"
this belongs to.

> +	/* Default manufacturing address; used when no EE or invalid */
> +	n = 0;
> +	mac_addr[0] = 0;
> +	mac_addr[1] = 0x20;
> +	mac_addr[2] = 0x18;
> +	mac_addr[3] = 0x1C;
> +	mac_addr[4] = 0x00;
> +	mac_addr[5] = 0x01;

Please never, never ever hard-wire MAC addresses into U-Boot.

Also, the 002018 prefix appears to be owned by Cis Technology Inc.;
do you have the rights to use it?

> --- /dev/null
> +++ b/board/birdland/bav335x/mux.c
> @@ -0,0 +1,206 @@
> +/*
> + * mux.c
> + *
> + * Copyright (c) 2012-2014 Birdland Audio - http://birdland.com/oem
> + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.

Please use SPDX ID instead.  

What exactly is the difference in this file compared to
board/ti/am335x/mux.c ?

> +/*
> + * The AM335x GP EVM, if daughter card(s) are connected, can have 8
> + * different profiles.  These profiles determine what peripherals are
> + * valid and need pinmux to be configured.
> + */

Is this relevant to your board?

> diff --git a/board/birdland/bav335x/u-boot.lds b/board/birdland/bav335x/u-boot.lds
> new file mode 100644
> index 0000000..fc80f21
> --- /dev/null
> +++ b/board/birdland/bav335x/u-boot.lds

Do you really need a private linker script?  Why?

> +	/*
> +	 * Deprecated: this MMU section is used by pxa at present but
> +	 * should not be used by new boards/CPUs.
> +	 */

Did you read that?

> --- /dev/null
> +++ b/include/configs/bav335x.h
> @@ -0,0 +1,633 @@
> +/*
> + * bav335x.h
> + *
> + * Copyright (c) 2012-2014 Birdland Audio - http://birdland.com/oem
> + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

SPDX ID ....

> +#define MACH_TYPE_TIAM335EVM		3589	/* Until the next sync */
> +#define CONFIG_MACH_TYPE		MACH_TYPE_TIAM335EVM

But this is NOT a TI AM335X EVM board, it is your own machine.  You
should not hijack foreign IDs.

> +/* Always 128 KiB env size */
> +#define CONFIG_ENV_SIZE			(128 << 10)

This makes no sense.  You will NEVER need that much.  If you make the
environment so big without need, it will only slow down booting.

> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +DEFAULT_LINUX_BOOT_ENV \
> +"boot_fdt=try\0" \
> +"bootpart=0:2\0" \
> +"bootdir=\0" \
...

Please use proper indentation.

> +#define MTDPARTS_DEFAULT  \
> +	"mtdparts=nand.0:" \
> +	"128k(NAND.SPL)," \
> +	"128k(NAND.SPL.backup1)," \
> +	"128k(NAND.SPL.backup2)," \
> +	"128k(NAND.SPL.backup3)," \
> +	"256k(NAND.u-boot-spl-os)," \
> +	"1m(NAND.u-boot)," \
> +	"128k(NAND.u-boot-env)," \
> +	"128k(NAND.u-boot-env.backup1)," \
> +	"8m(NAND.kernel)," \
> +	"-(NAND.rootfs)"
...
> +/*
> + * Default to using SPI for environment, etc.
> + * 0x000000 - 0x020000 : SPL (128KiB)
> + * 0x020000 - 0x0A0000 : U-Boot (512KiB)
> + * 0x0A0000 - 0x0BFFFF : First copy of U-Boot Environment (128KiB)
> + * 0x0C0000 - 0x0DFFFF : Second copy of U-Boot Environment (128KiB)
> + * 0x0E0000 - 0x442000 : Linux Kernel
> + * 0x442000 - 0x800000 : Userland
> + */

Are these settings in any way related and/or kept in sync?

> +#define CONFIG_ENV_OFFSET		(768 << 10) /* 768 KiB in */
> +#define CONFIG_ENV_OFFSET_REDUND	(896 << 10) /* 896 KiB in */

Hm...  768kB = 0xC0000, 896 kB = 0xE0000

This is not what you wrote before???

> +#define MTDIDS_DEFAULT			"nor0=m25p80-flash.0"
> +#define MTDPARTS_DEFAULT		"mtdparts=m25p80-flash.0:128k(SPL)," \
> +					"512k(u-boot),128k(u-boot-env1)," \
> +					"128k(u-boot-env2),3464k(kernel)," \
> +					"-(rootfs)"

See MTDPARTS_DEFAULT further above?  What happens if you have
CONFIG_NAND and CONFIG_SPI_BOOT both set?

> +/*
> + * NOR Size = 16 MiB
> + * Number of Sectors/Blocks = 128
> + * Sector Size = 128 KiB
> + * Word length = 16 bits
> + * Default layout:
> + * 0x000000 - 0x07FFFF : U-Boot (512 KiB)
> + * 0x080000 - 0x09FFFF : First copy of U-Boot Environment (128 KiB)
> + * 0x0A0000 - 0x0BFFFF : Second copy of U-Boot Environment (128 KiB)
> + * 0x0C0000 - 0x4BFFFF : Linux Kernel (4 MiB)
> + * 0x4C0000 - 0xFFFFFF : Userland (11 MiB + 256 KiB)

Oh, yet another set of settings...

This config file is a bit too complicated, don't you think so?
I recommend to perform a serious clean-up.  As is, it is unreadable
and unmaintainable.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The White Rabbit put on his spectacles. "Where shall I begin,  please
your Majesty ?" he asked.
"Begin at the beginning,", the King said, very gravely,  "and  go  on
till you come to the end: then stop."                -- Lewis Carroll