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

[Sascha Hauer <s.hauer@pengutronix.de>]

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