* [U-Boot] Patch to add support for BAV335x boards
2015-02-03 7:40 [U-Boot] Patch to add support for BAV335x boards Gilles
@ 2015-02-03 11:57 ` Wolfgang Denk
2015-02-03 19:52 ` Albert ARIBAUD
1 sibling, 0 replies; 3+ messages in thread
From: Wolfgang Denk @ 2015-02-03 11:57 UTC (permalink / raw)
To: u-boot
Dear Gilles,
In message <3EFCB679-8DF9-43F2-B3F4-FBC00E578061@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@gigadevices.com>
> Date: Sun, 1 Feb 2015 19:30:01 -0800
> Subject: [PATCH] Adding Support for BAV335x boards
>
> Signed-off-by: Gilles Gameiro <gilles@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
^ permalink raw reply [flat|nested] 3+ messages in thread* [U-Boot] Patch to add support for BAV335x boards
2015-02-03 7:40 [U-Boot] Patch to add support for BAV335x boards Gilles
2015-02-03 11:57 ` Wolfgang Denk
@ 2015-02-03 19:52 ` Albert ARIBAUD
1 sibling, 0 replies; 3+ messages in thread
From: Albert ARIBAUD @ 2015-02-03 19:52 UTC (permalink / raw)
To: u-boot
Hello Gilles,
On Mon, 2 Feb 2015 23:40:02 -0800, Gilles <gilles@gigadevices.com>
wrote:
> Hi Folks,
>
> 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.
Your mail's subject should be the one produced by patman, and the patch
should be in the mail body, not in an attachment.
Actually, patman would have handled all this for you and more (CC:s,
etc) if it had not been prevented from sending the mail itself.
Did patman abort before it could send the mail? If so, then you should
fix your patman configuration; or at least, you could send the mail
through git send-email (which you might have to install in addition to
git itself).
Please read tools/patman/README and set up your patman -- that'll save
time to both you and the list.
> Cheers,
> Gilles
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 3+ messages in thread