[U-Boot] [PATCH 09/12] aspeed/ast2500: Common board init functions for ast2500 based boards

Simon Glass
2017-01-14T17:14:16Z

Hi Maxim,

On 4 January 2017 at 12:46, Maxim Sloyko <maxims@google.com> wrote:
> Signed-off-by: Maxim Sloyko <maxims@google.com>
> ---
>
> arch/arm/mach-aspeed/Makefile | 2 +-
> arch/arm/mach-aspeed/ast2500-board.c | 74 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 75 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm/mach-aspeed/ast2500-board.c
>
> diff --git a/arch/arm/mach-aspeed/Makefile b/arch/arm/mach-aspeed/Makefile
> index 1f7af71b03..9d29ff7f6f 100644
> --- a/arch/arm/mach-aspeed/Makefile
> +++ b/arch/arm/mach-aspeed/Makefile
> @@ -5,4 +5,4 @@
> #
>
> obj-$(CONFIG_ARCH_ASPEED) += ast_wdt.o
> -obj-$(CONFIG_ASPEED_AST2500) += ast2500/
> +obj-$(CONFIG_ASPEED_AST2500) += ast2500/ ast2500-board.o
> diff --git a/arch/arm/mach-aspeed/ast2500-board.c b/arch/arm/mach-aspeed/ast2500-board.c
> new file mode 100644
> index 0000000000..5b2b0ce132
> --- /dev/null
> +++ b/arch/arm/mach-aspeed/ast2500-board.c
> @@ -0,0 +1,74 @@
> +#include <common.h>
> +#include <asm/arch/timer.h>
> +#include <asm/arch/wdt.h>
> +#include <asm/io.h>
> +#include <dm.h>
> +#include <dm/uclass.h>
> +#include <linux/err.h>
> +#include <ram.h>
> +#include <timer.h>

Check include file order.

> +
> +/* Second Watchdog Timer by default is configured

/*
* Second watchdog...
* ...
*/

Please check globally. Try to use more columns if you can.

> + * to trigger secondary boot source.
> + */
> +#define AST_2ND_BOOT_WDT (1)

Remove () around simple constants.

> +
> +/* Third Watchdog Timer by default is configured
> + * to toggle Flash address mode switch before reset.
> + */
> +#define AST_FLASH_ADDR_DETECT_WDT (2)
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +void lowlevel_init(void)
> +{
> + /*
> + * These two watchdogs need to be stopped as soon as possible,
> + * otherwise the board might hang. By default they are set to
> + * a very short timeout and even simple debug write to serial
> + * console early in the init process might cause them to fire.
> + */
> + struct ast_wdt *flash_addr_wdt =
> + (struct ast_wdt *)(WDT_BASE +
> + sizeof(struct ast_wdt) *
> + AST_FLASH_ADDR_DETECT_WDT);
> +
> + clrbits_le32(&flash_addr_wdt->ctrl, WDT_CTRL_EN);
> +
> +#ifndef CONFIG_FIRMWARE_2ND_BOOT
> + struct ast_wdt *sec_boot_wdt =
> + (struct ast_wdt *)(WDT_BASE +
> + sizeof(struct ast_wdt) *
> + AST_2ND_BOOT_WDT);
> +
> + clrbits_le32(&sec_boot_wdt->ctrl, WDT_CTRL_EN);
> +#endif

Seems this should call a helper function in ast_wdt.c or similar.

> +}
> +
> +int board_init(void)
> +{
> + gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;

blank line

> + return 0;
> +}
> +
> +int dram_init(void)
> +{
> + struct udevice *dev;
> + int ret = uclass_get_device(UCLASS_RAM, 0, &dev);

blank line, and better to separate decl and code in this case, since
you check it below.

> + if (ret) {
> + debug("DRAM FAIL1\r\n");
> + return ret;
> + }
> +
> + struct ram_info ram;

move to top of function

> + ret = ram_get_info(dev, &ram);
> + if (ret) {
> + debug("DRAM FAIL2\r\n");
> + return ret;
> + }
> +

drop extra blank line

> +
> + gd->ram_size = ram.size;

blank line

> + return 0;
> +}
> +
> --
> 2.11.0.390.gc69c2f50cf-goog
>

REgards,
Simon