[PATCH v2] ARM: i.MX: Add liteboard support

[PATCH v2] ARM: i.MX: Add liteboard support

[Marcin Niestroj]

liteboard is a development board which uses liteSOM as its
base. liteSOM can't exist on its own, but is used as part
of other boards - it only contains processor and memory.

Hardware specification:
 * liteSOM:
   - i.MX6UL
   - 256M or 512M DDR3 RAM
   - eMMC (uSDHC2)
 * Ethernet PHY
 * USB host (usb_otg1)
 * MicroSD slot (uSDHC1)

Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
---
Hi,

It has been a while before posting first patch [1]. Finally I have
found more time to finish that work.

[1] https://www.spinics.net/lists/u-boot-v2/msg33201.html

Changes v1 -> v2:
 * rebased on current master
 * use for loop over boot_sources[] (previously named envs[])
   (suggested by Andrey)
 * do not configure RX UART in lowlevel.c
   (suggested by Andrey)
 * use putchar() instead of putc_ll(), so we depend on
   CONFIG_PBL_CONSOLE instead of CONFIG_DEBUG_LL
   (suggested by Andrey)
 * only setup UART in case of CONFIG_PBL_CONSOLE=y
   (suggested by Andrey)
 * move relocate_to_current_adr(), setup_c(), barrier()
   inside setup_uart() (suggested by Andrey)
 * do not disable watchdog powerdown counters (I couldn't
   find good reason to disable them)
 * put partition definitions in dts file inside 'partitions'
   node

 arch/arm/boards/Makefile                      |   1 +
 arch/arm/boards/grinn-liteboard/Makefile      |   2 +
 arch/arm/boards/grinn-liteboard/board.c       | 110 ++++++++++++++++++
 .../flash-header-liteboard-256mb.imxcfg       |   6 +
 .../flash-header-liteboard-512mb.imxcfg       |   6 +
 .../grinn-liteboard/flash-header-liteboard.h  |  68 +++++++++++
 arch/arm/boards/grinn-liteboard/lowlevel.c    |  82 +++++++++++++
 arch/arm/configs/imx_v7_defconfig             |   1 +
 arch/arm/dts/Makefile                         |   1 +
 arch/arm/dts/imx6ul-liteboard.dts             |  96 +++++++++++++++
 arch/arm/mach-imx/Kconfig                     |   4 +
 images/Makefile.imx                           |  10 ++
 12 files changed, 387 insertions(+)
 create mode 100644 arch/arm/boards/grinn-liteboard/Makefile
 create mode 100644 arch/arm/boards/grinn-liteboard/board.c
 create mode 100644 arch/arm/boards/grinn-liteboard/flash-header-liteboard-256mb.imxcfg
 create mode 100644 arch/arm/boards/grinn-liteboard/flash-header-liteboard-512mb.imxcfg
 create mode 100644 arch/arm/boards/grinn-liteboard/flash-header-liteboard.h
 create mode 100644 arch/arm/boards/grinn-liteboard/lowlevel.c
 create mode 100644 arch/arm/dts/imx6ul-liteboard.dts

diff --git a/arch/arm/boards/Makefile b/arch/arm/boards/Makefile
index c737cf341..601691543 100644
--- a/arch/arm/boards/Makefile
+++ b/arch/arm/boards/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_MACH_GE863)			+= telit-evk-pro3/
 obj-$(CONFIG_MACH_GK802)			+= gk802/
 obj-$(CONFIG_MACH_GLOBALSCALE_GURUPLUG)		+= globalscale-guruplug/
 obj-$(CONFIG_MACH_GLOBALSCALE_MIRABOX)		+= globalscale-mirabox/
+obj-$(CONFIG_MACH_GRINN_LITEBOARD)		+= grinn-liteboard/
 obj-$(CONFIG_MACH_GUF_CUPID)			+= guf-cupid/
 obj-$(CONFIG_MACH_GUF_SANTARO)			+= guf-santaro/
 obj-$(CONFIG_MACH_GUF_VINCELL)			+= guf-vincell/
diff --git a/arch/arm/boards/grinn-liteboard/Makefile b/arch/arm/boards/grinn-liteboard/Makefile
new file mode 100644
index 000000000..01c7a259e
--- /dev/null
+++ b/arch/arm/boards/grinn-liteboard/Makefile
@@ -0,0 +1,2 @@
+obj-y += board.o
+lwl-y += lowlevel.o
diff --git a/arch/arm/boards/grinn-liteboard/board.c b/arch/arm/boards/grinn-liteboard/board.c
new file mode 100644
index 000000000..695b24384
--- /dev/null
+++ b/arch/arm/boards/grinn-liteboard/board.c
@@ -0,0 +1,110 @@
+/*
+ * Copyright (C) 2018 Grinn
+ *
+ * Author: Marcin Niestroj <m.niestroj@grinn-global.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 in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#define pr_fmt(fmt) "liteboard: " fmt
+
+#include <bootsource.h>
+#include <common.h>
+#include <envfs.h>
+#include <init.h>
+#include <mach/bbu.h>
+#include <mach/imx6.h>
+#include <malloc.h>
+#include <mfd/imx6q-iomuxc-gpr.h>
+#include <of.h>
+
+static void bbu_register_handler_sd(bool is_boot_source)
+{
+	imx6_bbu_internal_mmc_register_handler("sd", "/dev/mmc0.barebox",
+				is_boot_source ? BBU_HANDLER_FLAG_DEFAULT : 0);
+}
+
+static void bbu_register_handler_emmc(bool is_boot_source)
+{
+	int emmc_boot_flag = 0, emmc_flag = 0;
+	const char *bootpart;
+	struct device_d *dev;
+	int ret;
+
+	if (!is_boot_source)
+		goto bbu_register;
+
+	dev = get_device_by_name("mmc1");
+	if (!dev) {
+		pr_warn("Failed to get eMMC device\n");
+		goto bbu_register;
+	}
+
+	ret = device_detect(dev);
+	if (ret) {
+		pr_warn("Failed to probe eMMC\n");
+		goto bbu_register;
+	}
+
+	bootpart = dev_get_param(dev, "boot");
+	if (!bootpart) {
+		pr_warn("Failed to get eMMC boot configuration\n");
+		goto bbu_register;
+	}
+
+	if (!strncmp(bootpart, "boot", 4))
+		emmc_boot_flag |= BBU_HANDLER_FLAG_DEFAULT;
+	else
+		emmc_flag |= BBU_HANDLER_FLAG_DEFAULT;
+
+bbu_register:
+	imx6_bbu_internal_mmc_register_handler("emmc", "/dev/mmc1.barebox",
+					emmc_flag);
+	imx6_bbu_internal_mmcboot_register_handler("emmc-boot", "mmc1",
+					emmc_boot_flag);
+}
+
+static const struct {
+	const char *name;
+	const char *env;
+	void (*bbu_register_handler)(bool);
+} boot_sources[] = {
+	{"SD",   "/chosen/environment-sd",   bbu_register_handler_sd},
+	{"eMMC", "/chosen/environment-emmc", bbu_register_handler_emmc},
+};
+
+static int liteboard_devices_init(void)
+{
+	int boot_source_idx = 0;
+	int ret;
+	int i;
+
+	if (!of_machine_is_compatible("grinn,imx6ul-liteboard"))
+		return 0;
+
+	barebox_set_hostname("liteboard");
+
+	if (bootsource_get() == BOOTSOURCE_MMC)
+		boot_source_idx = bootsource_get_instance();
+
+	ret = of_device_enable_path(boot_sources[boot_source_idx].env);
+	if (ret < 0)
+		pr_warn("Failed to enable environment partition '%s' (%d)\n",
+			boot_sources[boot_source_idx].env, ret);
+
+	pr_notice("Using environment in %s\n",
+		boot_sources[boot_source_idx].name);
+
+	for (i = 0; i < ARRAY_SIZE(boot_sources); i++)
+		boot_sources[i].bbu_register_handler(boot_source_idx == i);
+
+	return 0;
+}
+device_initcall(liteboard_devices_init);
diff --git a/arch/arm/boards/grinn-liteboard/flash-header-liteboard-256mb.imxcfg b/arch/arm/boards/grinn-liteboard/flash-header-liteboard-256mb.imxcfg
new file mode 100644
index 000000000..1b980c784
--- /dev/null
+++ b/arch/arm/boards/grinn-liteboard/flash-header-liteboard-256mb.imxcfg
@@ -0,0 +1,6 @@
+
+#define SETUP_MDASP_MDCTL		\
+	wm 32 0x021B0040 0x00000047;	\
+	wm 32 0x021B0000 0x83180000
+
+#include "flash-header-liteboard.h"
diff --git a/arch/arm/boards/grinn-liteboard/flash-header-liteboard-512mb.imxcfg b/arch/arm/boards/grinn-liteboard/flash-header-liteboard-512mb.imxcfg
new file mode 100644
index 000000000..c93a2cc0f
--- /dev/null
+++ b/arch/arm/boards/grinn-liteboard/flash-header-liteboard-512mb.imxcfg
@@ -0,0 +1,6 @@
+
+#define SETUP_MDASP_MDCTL		\
+	wm 32 0x021B0040 0x0000004F;	\
+	wm 32 0x021B0000 0x84180000
+
+#include "flash-header-liteboard.h"
diff --git a/arch/arm/boards/grinn-liteboard/flash-header-liteboard.h b/arch/arm/boards/grinn-liteboard/flash-header-liteboard.h
new file mode 100644
index 000000000..60a39f524
--- /dev/null
+++ b/arch/arm/boards/grinn-liteboard/flash-header-liteboard.h
@@ -0,0 +1,68 @@
+
+loadaddr 0x80000000
+soc imx6
+dcdofs 0x400
+
+wm 32 0x020c4068 0xffffffff
+wm 32 0x020c406c 0xffffffff
+wm 32 0x020c4070 0xffffffff
+wm 32 0x020c4074 0xffffffff
+wm 32 0x020c4078 0xffffffff
+wm 32 0x020c407c 0xffffffff
+wm 32 0x020c4080 0xffffffff
+
+wm 32 0x020E04B4 0x000C0000
+wm 32 0x020E04AC 0x00000000
+wm 32 0x020E027C 0x00000030
+wm 32 0x020E0250 0x00000030
+wm 32 0x020E024C 0x00000030
+wm 32 0x020E0490 0x00000030
+wm 32 0x020E0288 0x00000030
+wm 32 0x020E0270 0x00000000
+wm 32 0x020E0260 0x00000030
+wm 32 0x020E0264 0x00000030
+wm 32 0x020E04A0 0x00000030
+wm 32 0x020E0494 0x00020000
+wm 32 0x020E0280 0x00000030
+wm 32 0x020E0284 0x00000030
+wm 32 0x020E04B0 0x00020000
+wm 32 0x020E0498 0x00000030
+wm 32 0x020E04A4 0x00000030
+wm 32 0x020E0244 0x00000030
+wm 32 0x020E0248 0x00000030
+wm 32 0x021B001C 0x00008000
+wm 32 0x021B0800 0xA1390003
+wm 32 0x021B080C 0x00000000
+wm 32 0x021B083C 0x41480148
+wm 32 0x021B0848 0x40403E42
+wm 32 0x021B0850 0x40405852
+wm 32 0x021B081C 0x33333333
+wm 32 0x021B0820 0x33333333
+wm 32 0x021B082C 0xf3333333
+wm 32 0x021B0830 0xf3333333
+wm 32 0x021B08C0 0x00922012
+wm 32 0x021B0858 0x00000F00
+wm 32 0x021B08b8 0x00000800
+wm 32 0x021B0004 0x0002002D
+wm 32 0x021B0008 0x1B333030
+wm 32 0x021B000C 0x676B52F3
+wm 32 0x021B0010 0xB66D0B63
+wm 32 0x021B0014 0x01FF00DB
+wm 32 0x021B0018 0x00211740
+wm 32 0x021B001C 0x00008000
+wm 32 0x021B002C 0x000026D2
+wm 32 0x021B0030 0x006B1023
+
+SETUP_MDASP_MDCTL
+
+wm 32 0x021b0890 0x00400A38
+wm 32 0x021B001C 0x02008032
+wm 32 0x021B001C 0x00008033
+wm 32 0x021B001C 0x00048031
+wm 32 0x021B001C 0x15208030
+wm 32 0x021B001C 0x04008040
+wm 32 0x021B0020 0x00007800
+wm 32 0x021B0818 0x00000227
+wm 32 0x021B0004 0x0002556D
+wm 32 0x021B0404 0x00011006
+wm 32 0x021B001C 0x00000000
diff --git a/arch/arm/boards/grinn-liteboard/lowlevel.c b/arch/arm/boards/grinn-liteboard/lowlevel.c
new file mode 100644
index 000000000..331ccc228
--- /dev/null
+++ b/arch/arm/boards/grinn-liteboard/lowlevel.c
@@ -0,0 +1,82 @@
+/*
+ * Copyright (C) 2018 Grinn
+ *
+ * Author: Marcin Niestroj <m.niestroj@grinn-global.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 in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#include <debug_ll.h>
+#include <common.h>
+#include <linux/sizes.h>
+#include <io.h>
+#include <image-metadata.h>
+#include <asm/barebox-arm-head.h>
+#include <asm/barebox-arm.h>
+#include <asm/sections.h>
+#include <asm/cache.h>
+#include <asm/mmu.h>
+#include <mach/esdctl.h>
+#include <mach/imx6.h>
+
+static inline void setup_uart(void)
+{
+	void __iomem *iomuxbase = IOMEM(MX6_IOMUXC_BASE_ADDR);
+	void __iomem *uart = IOMEM(MX6_UART1_BASE_ADDR);
+
+	relocate_to_current_adr();
+	setup_c();
+	barrier();
+
+	imx6_ungate_all_peripherals();
+
+	writel(0x0, iomuxbase + 0x84);
+	writel(0x1b0b1, iomuxbase + 0x0310);
+
+	imx6_uart_setup(uart);
+
+	pbl_set_putc(imx_uart_putc, uart);
+
+	putchar('>');
+}
+
+BAREBOX_IMD_TAG_STRING(liteboard_memsize_SZ_256M, IMD_TYPE_PARAMETER,
+		"memsize=256", 0);
+BAREBOX_IMD_TAG_STRING(liteboard_memsize_SZ_512M, IMD_TYPE_PARAMETER,
+		"memsize=512", 0);
+
+extern char __dtb_imx6ul_liteboard_start[];
+
+static void __noreturn start_imx6_liteboard(void)
+{
+	imx6ul_cpu_lowlevel_init();
+
+	arm_setup_stack(0x00910000 - 8);
+
+	arm_early_mmu_cache_invalidate();
+
+	if (IS_ENABLED(CONFIG_PBL_CONSOLE))
+		setup_uart();
+
+	imx6ul_barebox_entry(__dtb_imx6ul_liteboard_start +
+			get_runtime_offset());
+}
+
+#define LITEBOARD_ENTRY(name, memory_size)				\
+	ENTRY_FUNCTION(name, r0, r1, r2)				\
+	{								\
+		IMD_USED(liteboard_memsize_##memory_size);		\
+									\
+		start_imx6_liteboard();					\
+	}
+
+
+LITEBOARD_ENTRY(start_imx6ul_liteboard_256mb, SZ_256M);
+LITEBOARD_ENTRY(start_imx6ul_liteboard_512mb, SZ_512M);
diff --git a/arch/arm/configs/imx_v7_defconfig b/arch/arm/configs/imx_v7_defconfig
index 0fc3c9c50..bf84dfa9f 100644
--- a/arch/arm/configs/imx_v7_defconfig
+++ b/arch/arm/configs/imx_v7_defconfig
@@ -41,6 +41,7 @@ CONFIG_MACH_ZII_VF610_DEV=y
 CONFIG_MACH_PHYTEC_PHYCORE_IMX7=y
 CONFIG_MACH_FREESCALE_MX7_SABRESD=y
 CONFIG_MACH_NXP_IMX6ULL_EVK=y
+CONFIG_MACH_GRINN_LITEBOARD=y
 CONFIG_IMX_IIM=y
 CONFIG_IMX_IIM_FUSE_BLOW=y
 CONFIG_THUMB2_BAREBOX=y
diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
index 1caeca35b..b3b562917 100644
--- a/arch/arm/dts/Makefile
+++ b/arch/arm/dts/Makefile
@@ -27,6 +27,7 @@ pbl-dtb-$(CONFIG_MACH_FREESCALE_MX7_SABRESD) += imx7d-sdb.dtb.o
 pbl-dtb-$(CONFIG_MACH_GK802) += imx6q-gk802.dtb.o
 pbl-dtb-$(CONFIG_MACH_GLOBALSCALE_GURUPLUG) += kirkwood-guruplug-server-plus-bb.dtb.o
 pbl-dtb-$(CONFIG_MACH_GLOBALSCALE_MIRABOX) += armada-370-mirabox-bb.dtb.o
+pbl-dtb-$(CONFIG_MACH_GRINN_LITEBOARD) += imx6ul-liteboard.dtb.o
 pbl-dtb-$(CONFIG_MACH_GUF_SANTARO) += imx6q-guf-santaro.dtb.o
 pbl-dtb-$(CONFIG_MACH_GUF_VINCELL) += imx53-guf-vincell.dtb.o imx53-guf-vincell-lt.dtb.o
 pbl-dtb-$(CONFIG_MACH_GW_VENTANA) += imx6q-gw54xx.dtb.o
diff --git a/arch/arm/dts/imx6ul-liteboard.dts b/arch/arm/dts/imx6ul-liteboard.dts
new file mode 100644
index 000000000..03a4bfc78
--- /dev/null
+++ b/arch/arm/dts/imx6ul-liteboard.dts
@@ -0,0 +1,96 @@
+/*
+ * Copyright 2018 Grinn
+ *
+ * Author: Marcin Niestroj <m.niestroj@grinn-global.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License
+ *     version 2 as published by the Free Software Foundation.
+ *
+ *     This file is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <arm/imx6ul-liteboard.dts>
+
+/ {
+	chosen {
+		environment-sd {
+			compatible = "barebox,environment";
+			device-path = &environment_sd;
+			status = "disabled";
+		};
+
+		environment-emmc {
+			compatible = "barebox,environment";
+			device-path = &environment_emmc;
+			status = "disabled";
+		};
+	};
+};
+
+&usdhc1 {
+	partitions {
+		compatible = "fixed-partitions";
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		partition@0 {
+			label = "barebox";
+			reg = <0x0 0xe0000>;
+		};
+
+		environment_sd: partition@e0000 {
+			label = "barebox-environment";
+			reg = <0xe0000 0x20000>;
+		};
+	};
+};
+
+&usdhc2 {
+	partitions {
+		compatible = "fixed-partitions";
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		partition@0 {
+			label = "barebox";
+			reg = <0x0 0xe0000>;
+		};
+
+		environment_emmc: partition@e0000 {
+			label = "barebox-environment";
+			reg = <0xe0000 0x20000>;
+		};
+	};
+};
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index 63a92bd5b..4eabd938f 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -454,6 +454,10 @@ config MACH_NXP_IMX8MQ_EVK
 	select FIRMWARE_IMX8MQ_ATF
 	select ARM_SMCCC
 
+config MACH_GRINN_LITEBOARD
+	bool "Grinn liteboard"
+	select ARCH_IMX6UL
+
 endif
 
 # ----------------------------------------------------------
diff --git a/images/Makefile.imx b/images/Makefile.imx
index 341ce8506..9b5cd577d 100644
--- a/images/Makefile.imx
+++ b/images/Makefile.imx
@@ -486,6 +486,16 @@ CFG_start_imx6dl_samx6i.pblx.imximg = $(board)/kontron-samx6i/flash-header-samx6
 FILE_barebox-imx6dl-samx6i.img = start_imx6dl_samx6i.pblx.imximg
 image-$(CONFIG_MACH_KONTRON_SAMX6I) += barebox-imx6dl-samx6i.img
 
+pblx-$(CONFIG_MACH_GRINN_LITEBOARD) += start_imx6ul_liteboard_256mb
+CFG_start_imx6ul_liteboard_256mb.pblx.imximg = $(board)/grinn-liteboard/flash-header-liteboard-256mb.imxcfg
+FILE_barebox-grinn-liteboard-256mb.img = start_imx6ul_liteboard_256mb.pblx.imximg
+image-$(CONFIG_MACH_GRINN_LITEBOARD) += barebox-grinn-liteboard-256mb.img
+
+pblx-$(CONFIG_MACH_GRINN_LITEBOARD) += start_imx6ul_liteboard_512mb
+CFG_start_imx6ul_liteboard_512mb.pblx.imximg = $(board)/grinn-liteboard/flash-header-liteboard-512mb.imxcfg
+FILE_barebox-grinn-liteboard-512mb.img = start_imx6ul_liteboard_512mb.pblx.imximg
+image-$(CONFIG_MACH_GRINN_LITEBOARD) += barebox-grinn-liteboard-512mb.img
+
 pblx-$(CONFIG_MACH_GW_VENTANA) += start_imx6q_gw54xx_1gx64
 CFG_start_imx6q_gw54xx_1gx64.pblx.imximg = $(board)/gateworks-ventana/flash-header-ventana-quad-1gx64.imxcfg
 FILE_barebox-gateworks-imx6q-ventana-1gx64.img = start_imx6q_gw54xx_1gx64.pblx.imximg
-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] ARM: i.MX: Add liteboard support

[Sascha Hauer]

Hi Marcin,

Looks fine overall, two small things to note.

On Fri, Oct 19, 2018 at 04:12:42PM +0200, Marcin Niestroj wrote:
> +static void bbu_register_handler_sd(bool is_boot_source)
> +{
> +	imx6_bbu_internal_mmc_register_handler("sd", "/dev/mmc0.barebox",
> +				is_boot_source ? BBU_HANDLER_FLAG_DEFAULT : 0);
> +}
> +
> +static void bbu_register_handler_emmc(bool is_boot_source)
> +{
> +	int emmc_boot_flag = 0, emmc_flag = 0;
> +	const char *bootpart;
> +	struct device_d *dev;
> +	int ret;
> +
> +	if (!is_boot_source)
> +		goto bbu_register;
> +
> +	dev = get_device_by_name("mmc1");
> +	if (!dev) {
> +		pr_warn("Failed to get eMMC device\n");
> +		goto bbu_register;
> +	}
> +
> +	ret = device_detect(dev);
> +	if (ret) {
> +		pr_warn("Failed to probe eMMC\n");
> +		goto bbu_register;
> +	}
> +
> +	bootpart = dev_get_param(dev, "boot");
> +	if (!bootpart) {
> +		pr_warn("Failed to get eMMC boot configuration\n");
> +		goto bbu_register;
> +	}
> +
> +	if (!strncmp(bootpart, "boot", 4))
> +		emmc_boot_flag |= BBU_HANDLER_FLAG_DEFAULT;
> +	else
> +		emmc_flag |= BBU_HANDLER_FLAG_DEFAULT;
> +
> +bbu_register:
> +	imx6_bbu_internal_mmc_register_handler("emmc", "/dev/mmc1.barebox",
> +					emmc_flag);
> +	imx6_bbu_internal_mmcboot_register_handler("emmc-boot", "mmc1",
> +					emmc_boot_flag);
> +}

I'm not sure if the way you switch the default handler to the current
bootsource might not be a bit confusing. For example if you start from
eMMC user partition and execute the mmcboot handler once then the
default will change from user partition to boot next time. Also if you
think of the SD slot as a temporary boot source to recover the eMMC
bootloader then you might want to keep the default boot slot as eMMC
even if you are booting from SD currently.

I'll merge it as is if you think it's fine, but otherwise I suggest to
keep the default as eMMC boot partition. Maybe the others are not even
needed at all. Less choices make it easier for the user to pick the
right one ;)

> +
> +static const struct {
> +	const char *name;
> +	const char *env;
> +	void (*bbu_register_handler)(bool);
> +} boot_sources[] = {
> +	{"SD",   "/chosen/environment-sd",   bbu_register_handler_sd},
> +	{"eMMC", "/chosen/environment-emmc", bbu_register_handler_emmc},
> +};
> +
> +static int liteboard_devices_init(void)
> +{
> +	int boot_source_idx = 0;
> +	int ret;
> +	int i;
> +
> +	if (!of_machine_is_compatible("grinn,imx6ul-liteboard"))
> +		return 0;
> +
> +	barebox_set_hostname("liteboard");
> +
> +	if (bootsource_get() == BOOTSOURCE_MMC)
> +		boot_source_idx = bootsource_get_instance();
> +
> +	ret = of_device_enable_path(boot_sources[boot_source_idx].env);

You should check for index out of bounds before using boot_source_idx as
an array index, even when you know you only have 0 and 1 as boot
sources.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] ARM: i.MX: Add liteboard support

[Marcin Niestrój]

Hi Sascha,

Thanks for review! See my comments below.

Sascha Hauer <s.hauer@pengutronix.de> writes:

> Hi Marcin,
>
> Looks fine overall, two small things to note.
>
> On Fri, Oct 19, 2018 at 04:12:42PM +0200, Marcin Niestroj wrote:
>> +static void bbu_register_handler_sd(bool is_boot_source)
>> +{
>> +	imx6_bbu_internal_mmc_register_handler("sd", "/dev/mmc0.barebox",
>> +				is_boot_source ? BBU_HANDLER_FLAG_DEFAULT : 0);
>> +}
>> +
>> +static void bbu_register_handler_emmc(bool is_boot_source)
>> +{
>> +	int emmc_boot_flag = 0, emmc_flag = 0;
>> +	const char *bootpart;
>> +	struct device_d *dev;
>> +	int ret;
>> +
>> +	if (!is_boot_source)
>> +		goto bbu_register;
>> +
>> +	dev = get_device_by_name("mmc1");
>> +	if (!dev) {
>> +		pr_warn("Failed to get eMMC device\n");
>> +		goto bbu_register;
>> +	}
>> +
>> +	ret = device_detect(dev);
>> +	if (ret) {
>> +		pr_warn("Failed to probe eMMC\n");
>> +		goto bbu_register;
>> +	}
>> +
>> +	bootpart = dev_get_param(dev, "boot");
>> +	if (!bootpart) {
>> +		pr_warn("Failed to get eMMC boot configuration\n");
>> +		goto bbu_register;
>> +	}
>> +
>> +	if (!strncmp(bootpart, "boot", 4))
>> +		emmc_boot_flag |= BBU_HANDLER_FLAG_DEFAULT;
>> +	else
>> +		emmc_flag |= BBU_HANDLER_FLAG_DEFAULT;
>> +
>> +bbu_register:
>> +	imx6_bbu_internal_mmc_register_handler("emmc", "/dev/mmc1.barebox",
>> +					emmc_flag);
>> +	imx6_bbu_internal_mmcboot_register_handler("emmc-boot", "mmc1",
>> +					emmc_boot_flag);
>> +}
>
> I'm not sure if the way you switch the default handler to the current
> bootsource might not be a bit confusing. For example if you start from
> eMMC user partition and execute the mmcboot handler once then the
> default will change from user partition to boot next time.

I am not sure what introduces more confusion here: choosing default
handler based on eMMC boot configuration or implicitly switching to eMMC
boot partitions when running mmcboot handler (even if they were disabled
before running that handler). I think it is the latter.

Maybe we should only switch between boot0 <-> boot1 partitions, but do
not change eMMC boot configuration when none of boo0 and boot1 were
configured? I looks logical that `barebox_update ...` command should
only update bootloader on specified target, no implicit boot source
changes.

> Also if you think of the SD slot as a temporary boot source to recover
> the eMMC bootloader then you might want to keep the default boot slot
> as eMMC even if you are booting from SD currently.

If eMMC has valid bootloader (at least valid header with DCD for NXP
processors), then SD card will not be booted without changing boot
configuration on dip switch.

In case of no valid bootloader on eMMC processor will actually try to
boot from SD card, but this is due the fact that SD card is on esdhc0
interface on this board, which is tried when configured boot source did
not work.

>
> I'll merge it as is if you think it's fine, but otherwise I suggest to
> keep the default as eMMC boot partition. Maybe the others are not even
> needed at all.

I've looked at other boards and there is no consistency across them, i.e
some boards set always the same handler as default, some choose default
handler based on bootsource.

> Less choices make it easier for the user to pick the right one ;)

But at the same time on development boards we also want to give users
options they can easily test, before designing their own solution :)

>
>> +
>> +static const struct {
>> +	const char *name;
>> +	const char *env;
>> +	void (*bbu_register_handler)(bool);
>> +} boot_sources[] = {
>> +	{"SD",   "/chosen/environment-sd",   bbu_register_handler_sd},
>> +	{"eMMC", "/chosen/environment-emmc", bbu_register_handler_emmc},
>> +};
>> +
>> +static int liteboard_devices_init(void)
>> +{
>> +	int boot_source_idx = 0;
>> +	int ret;
>> +	int i;
>> +
>> +	if (!of_machine_is_compatible("grinn,imx6ul-liteboard"))
>> +		return 0;
>> +
>> +	barebox_set_hostname("liteboard");
>> +
>> +	if (bootsource_get() == BOOTSOURCE_MMC)
>> +		boot_source_idx = bootsource_get_instance();
>> +
>> +	ret = of_device_enable_path(boot_sources[boot_source_idx].env);
>
> You should check for index out of bounds before using boot_source_idx as
> an array index, even when you know you only have 0 and 1 as boot
> sources.

Okay, I will fix that in v3.

>
> Sascha


-- 
Regards,
Marcin

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] ARM: i.MX: Add liteboard support

[Sascha Hauer]

Hi Marcin,

On Fri, Nov 02, 2018 at 07:11:09PM +0100, Marcin Niestrój wrote:
> >> +bbu_register:
> >> +	imx6_bbu_internal_mmc_register_handler("emmc", "/dev/mmc1.barebox",
> >> +					emmc_flag);
> >> +	imx6_bbu_internal_mmcboot_register_handler("emmc-boot", "mmc1",
> >> +					emmc_boot_flag);
> >> +}
> >
> > I'm not sure if the way you switch the default handler to the current
> > bootsource might not be a bit confusing. For example if you start from
> > eMMC user partition and execute the mmcboot handler once then the
> > default will change from user partition to boot next time.
> 
> I am not sure what introduces more confusion here: choosing default
> handler based on eMMC boot configuration or implicitly switching to eMMC
> boot partitions when running mmcboot handler (even if they were disabled
> before running that handler). I think it is the latter.
> 
> Maybe we should only switch between boot0 <-> boot1 partitions, but do
> not change eMMC boot configuration when none of boo0 and boot1 were
> configured? I looks logical that `barebox_update ...` command should
> only update bootloader on specified target, no implicit boot source
> changes.
> 
> > Also if you think of the SD slot as a temporary boot source to recover
> > the eMMC bootloader then you might want to keep the default boot slot
> > as eMMC even if you are booting from SD currently.
> 
> If eMMC has valid bootloader (at least valid header with DCD for NXP
> processors), then SD card will not be booted without changing boot
> configuration on dip switch.
> 
> In case of no valid bootloader on eMMC processor will actually try to
> boot from SD card, but this is due the fact that SD card is on esdhc0
> interface on this board, which is tried when configured boot source did
> not work.
> 
> >
> > I'll merge it as is if you think it's fine, but otherwise I suggest to
> > keep the default as eMMC boot partition. Maybe the others are not even
> > needed at all.
> 
> I've looked at other boards and there is no consistency across them, i.e
> some boards set always the same handler as default, some choose default
> handler based on bootsource.

Indeed, unfortunately there is no consistency.

> 
> > Less choices make it easier for the user to pick the right one ;)
> 
> But at the same time on development boards we also want to give users
> options they can easily test, before designing their own solution :)

As said, these were only suggestions, I'm fine with either way.

> > You should check for index out of bounds before using boot_source_idx as
> > an array index, even when you know you only have 0 and 1 as boot
> > sources.
> 
> Okay, I will fix that in v3.

I'll wait for v3 then.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox