Re: [PATCH v2 4/4] ARM: i.MX: Add support for ZII RDU1 board

[Lucas Stach <l.stach@pengutronix.de>]

Hi Andrey,

Am Mittwoch, den 20.06.2018, 12:24 -0700 schrieb Andrey Smirnov:
> ZII RDU1 is a i.MX51 based, Babbage board derivative supported by
> upstream kernel. This commit adds support for it to Barebox.
> 
> > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  arch/arm/boards/Makefile                      |   1 +
>  arch/arm/boards/zii-imx51-rdu1/Makefile       |   2 +
>  arch/arm/boards/zii-imx51-rdu1/board.c        | 100 ++++++++++++++++++
>  .../flash-header-imx51-zii-rdu1.imxcfg        |  60 +++++++++++
>  arch/arm/boards/zii-imx51-rdu1/lowlevel.c     |  46 ++++++++
>  arch/arm/configs/imx_v7_defconfig             |   1 +
>  arch/arm/dts/Makefile                         |   1 +
>  arch/arm/dts/imx51-zii-rdu1.dts               |  46 ++++++++
>  arch/arm/mach-imx/Kconfig                     |   5 +
>  images/Makefile.imx                           |   5 +
>  10 files changed, 267 insertions(+)
>  create mode 100644 arch/arm/boards/zii-imx51-rdu1/Makefile
>  create mode 100644 arch/arm/boards/zii-imx51-rdu1/board.c
>  create mode 100644 arch/arm/boards/zii-imx51-rdu1/flash-header-imx51-zii-rdu1.imxcfg
>  create mode 100644 arch/arm/boards/zii-imx51-rdu1/lowlevel.c
>  create mode 100644 arch/arm/dts/imx51-zii-rdu1.dts
> 
> diff --git a/arch/arm/boards/Makefile b/arch/arm/boards/Makefile
> index f1dc4c685..e5d217f52 100644
> --- a/arch/arm/boards/Makefile
> +++ b/arch/arm/boards/Makefile
> > @@ -150,5 +150,6 @@ obj-$(CONFIG_MACH_VSCOM_BALTOS)			+= vscom-baltos/
> >  obj-$(CONFIG_MACH_QEMU_VIRT64)			+= qemu-virt64/
> >  obj-$(CONFIG_MACH_WARP7)			+= element14-warp7/
> >  obj-$(CONFIG_MACH_VF610_TWR)			+= freescale-vf610-twr/
> > +obj-$(CONFIG_MACH_ZII_RDU1)			+= zii-imx51-rdu1/
> >  obj-$(CONFIG_MACH_ZII_RDU2)			+= zii-imx6q-rdu2/
> >  obj-$(CONFIG_MACH_ZII_VF610_DEV)		+= zii-vf610-dev/
> diff --git a/arch/arm/boards/zii-imx51-rdu1/Makefile b/arch/arm/boards/zii-imx51-rdu1/Makefile
> new file mode 100644
> index 000000000..01c7a259e
> --- /dev/null
> +++ b/arch/arm/boards/zii-imx51-rdu1/Makefile
> @@ -0,0 +1,2 @@
> +obj-y += board.o
> +lwl-y += lowlevel.o
> diff --git a/arch/arm/boards/zii-imx51-rdu1/board.c b/arch/arm/boards/zii-imx51-rdu1/board.c
> new file mode 100644
> index 000000000..c09285aa4
> --- /dev/null
> +++ b/arch/arm/boards/zii-imx51-rdu1/board.c
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright (C) 2015 Nikita Yushchenko, CogentEmbedded, Inc
> + * Copyright (C) 2015 Andrey Gusakov, CogentEmbedded, Inc
> + * Copyright (C) 2007 Sascha Hauer, Pengutronix
> + *
> + * 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; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * 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) "zii-rdu1: " fmt

I don't think we want that. At least we don't do it on RDU2, but I
think Chris has an option here.

> +#include <common.h>
> +#include <init.h>
> +#include <mach/bbu.h>
> +#include <libfile.h>
> +#include <mach/imx5.h>
> +
> > +#define ZII_RDU1_DATAFLASH		"/dev/dataflash0"
> > +#define ZII_RDU1_DATAFLASH_CONFIG	ZII_RDU1_DATAFLASH ".config"
> +
> +/**
> + * zii_rdu1_bbu_spi_update - RDU1 specific BBU handler
> + *
> > + * @handler:	BBU handler pointer passed down by BBU framework
> > + * @data:	BBU data pointer passed down by BBU framework
> + *
> + * RDU1 design chose to use first page of the onboard dataflash to
> + * store vendor board-specific paramters, which is problematic because
> + * imx_bbu_internal_v1_update() will not spare that region when
> + * peforming the update.
> + *
> + * This functions works as a thin wrapper around
> + * imx_bbu_internal_v1_update() and saves the data before and restores
> + * it after, keeping board specific data intact.
> + */

In general I like the idea of replacing the update handler in the
board, as it shows clearly that this is some board specific quirk.

I don't fully agree on the implementation of the handler though.
Overwriting and restoring the config area still has the risk of
destroying this data, leaving the unit in a bricked state.

As the config area is page aligned, there is no need to touch it at all
(I think). All you need to do is to point the BBU target at the barebox
partition on the dataflash and truncate the barebox image in the custom
handler by changing data->image and data->len in the custom RDU1
handler.

> +static int zii_rdu1_bbu_spi_update(struct bbu_handler *handler,
> > +				   struct bbu_data *data)
> +{
> > +	uint8_t *config;
> > +	size_t size;
> > +	int ret;
> +
> > +	config = read_file(ZII_RDU1_DATAFLASH_CONFIG, &size);
> > +	if (!config)
> > +		return -EIO;
> +
> > +	ret = imx_bbu_internal_v1_update(handler, data);
> > +	if (ret < 0) {
> > +		pr_warn("Failed to update '%s'\n", handler->name);
> > +		/*
> > +		 * At this point we don't know what state the config
> > +		 * partition was left in, so we fall through and
> > +		 * re-write it with a known good data
> > +		 */
> > +	}
> +
> > +	ret = write_file_flash(ZII_RDU1_DATAFLASH_CONFIG, config, size);
> > +	if (ret < 0)
> > +		return ret;
> +
> > +	return 0;
> +}
> +
> +static int zii_rdu1_init(void)
> +{
> > +	struct bbu_handler *handler;
> +
> > +	if (!of_machine_is_compatible("zii,imx51-rdu1"))
> > +		return 0;
> +
> > +	imx51_babbage_power_init();
> +
> > +	barebox_set_hostname("rdu1");
> +
> > +	imx51_bbu_internal_mmc_register_handler("mmc", "/dev/mmc0", 0);
> > +	imx51_bbu_internal_spi_i2c_register_handler("spi",
> > +						    ZII_RDU1_DATAFLASH,
> > +						    BBU_HANDLER_FLAG_DEFAULT);
> > +	/*
> > +	 * Overload freshly registered SPI update handler to deal with
> > +	 * RDU1 quirk (see above for description)
> > +	 */
> > +	handler = bbu_find_handler_by_device(ZII_RDU1_DATAFLASH);
> > +	if (WARN_ON(!handler))
> > +		return -EINVAL;
> +
> > +	handler->handler = zii_rdu1_bbu_spi_update;
> +
> > +	return 0;
> +}
> +coredevice_initcall(zii_rdu1_init);

[...]

> diff --git a/arch/arm/boards/zii-imx51-rdu1/lowlevel.c b/arch/arm/boards/zii-imx51-rdu1/lowlevel.c
> new file mode 100644
> index 000000000..c28ca8653
> --- /dev/null
> +++ b/arch/arm/boards/zii-imx51-rdu1/lowlevel.c
> @@ -0,0 +1,46 @@
> +#include <debug_ll.h>
> +#include <mach/clock-imx51_53.h>
> +#include <mach/iomux-mx51.h>
> +#include <common.h>
> +#include <mach/esdctl.h>
> +#include <mach/generic.h>
> +#include <asm/barebox-arm-head.h>
> +#include <asm/barebox-arm.h>
> +
> +static inline void setup_uart(void)
> +{
> > +	void __iomem *iomuxbase = IOMEM(MX51_IOMUXC_BASE_ADDR);
> > +	void __iomem *ccmbase = IOMEM(MX51_CCM_BASE_ADDR);
> +
> > +	/*
> > +	 * Restore CCM values that might be changed by the Mask ROM
> > +	 * code.
> > +	 *
> > +	 * Source: RealView debug scripts provided by Freescale
> > +	 */
> > +	writel(MX5_CCM_CBCDR_RESET_VALUE,  ccmbase + MX5_CCM_CBCDR);
> > +	writel(MX5_CCM_CSCMR1_RESET_VALUE, ccmbase + MX5_CCM_CSCMR1);
> > +	writel(MX5_CCM_CSCDR1_RESET_VALUE, ccmbase + MX5_CCM_CSCDR1);
> +
> > +	imx_setup_pad(iomuxbase, MX51_PAD_UART1_TXD__UART1_TXD);
> +
> > +	imx51_uart_setup_ll();
> +
> > +	putc_ll('>');
> +}
> +
> +extern char __dtb_imx51_zii_rdu1_start[];
> +
> +ENTRY_FUNCTION(start_imx51_zii_rdu1, r0, r1, r2)
> +{
> > +	void *fdt;
> +
> > +	imx5_cpu_lowlevel_init();
> +
> > +	if (IS_ENABLED(CONFIG_DEBUG_LL))
> > +		setup_uart();
> +
> > +	fdt = __dtb_imx51_zii_rdu1_start + get_runtime_offset();
> +
> > +	imx51_barebox_entry(fdt);
> +}
> \ No newline at end of file

Add the newline to make git happy, please. There are more places like
this.

Regards,
Lucas

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