[PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile

Marek Behun
2020-09-06T18:56:04Z

On Sun, 6 Sep 2020 20:48:57 +0200
Andre Heider <a.heider@gmail.com> wrote:

> On 06/09/2020 18:12, Marek Behun wrote:
> > On Sun, 6 Sep 2020 11:32:47 +0200
> > Pali Roh?r <pali@kernel.org> wrote:
> >
> >> Adding Marek to loop.
> >>
> >> On Saturday 05 September 2020 14:07:44 Andre Heider wrote:
> >>> Required for the generic distro mechanism.
> >>>
> >>> Linux ships with 4 variants:
> >>> marvell/armada-3720-espressobin-v7-emmc.dtb
> >>> marvell/armada-3720-espressobin-v7.dtb
> >>> marvell/armada-3720-espressobin-emmc.dtb
> >>> marvell/armada-3720-espressobin.dtb
> >>>
> >>> Use available information to determine the appropriate filename.
> >>>
> >>> Tested on a v5 board without eMMC.
> >>>
> >>> Signed-off-by: Andre Heider <a.heider@gmail.com>
> >>> ---
> >>> arch/arm/mach-mvebu/Kconfig | 1 +
> >>> board/Marvell/mvebu_armada-37xx/board.c | 42 +++++++++++++++++++++++++
> >>> 2 files changed, 43 insertions(+)
> >>>
> >>> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> >>> index 0d8e0922a2..31f5d26dc2 100644
> >>> --- a/arch/arm/mach-mvebu/Kconfig
> >>> +++ b/arch/arm/mach-mvebu/Kconfig
> >>> @@ -100,6 +100,7 @@ config TARGET_HELIOS4
> >>> config TARGET_MVEBU_ARMADA_37XX
> >>> bool "Support Armada 37xx platforms"
> >>> select ARMADA_3700
> >>> + select BOARD_LATE_INIT
> >>
> >> This should go into espressobin defconfig file. Other Armada 37xx board
> >> do not need to have this option enabled.
> >
> > I agree with this.
> >
> >>> imply SCSI
> >>>
> >>> config TARGET_DB_88F6720
> >>> diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c
> >>> index 90bfc139aa..3bf0a08897 100644
> >>> --- a/board/Marvell/mvebu_armada-37xx/board.c
> >>> +++ b/board/Marvell/mvebu_armada-37xx/board.c
> >>> @@ -5,6 +5,7 @@
> >>>
> >>> #include <common.h>
> >>> #include <dm.h>
> >>> +#include <env.h>
> >>> #include <i2c.h>
> >>> #include <init.h>
> >>> #include <phy.h>
> >>> @@ -50,6 +51,22 @@ DECLARE_GLOBAL_DATA_PTR;
> >>> #define MVEBU_G2_SMI_PHY_CMD_REG (24)
> >>> #define MVEBU_G2_SMI_PHY_DATA_REG (25)
> >>>
> >>> +/*
> >>> + * Memory Controller Registers
> >>> + *
> >>> + * Assembled based on public information:
> >>> + * https://gitlab.nic.cz/turris/mox-boot-builder/-/blob/master/wtmi/main.c#L332-336
> >>> + * https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell/blob/mv_ddr-armada-18.12/drivers/mv_ddr_mc6.h#L309-L332
> >>> + *
> >>> + * And checked against the written register values for the various topologies:
> >>> + * https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell/blob/mv_ddr-armada-atf-mainline/a3700/mv_ddr_tim.h
> >>> + */
> >>> +#define A3700_CH0_MC_CTRL2_REG MVEBU_REGISTER(0x002c4)
> >>> +#define A3700_MC_CTRL2_SDRAM_TYPE_MASK 0xf
> >>> +#define A3700_MC_CTRL2_SDRAM_TYPE_OFFS 4
> >>> +#define A3700_MC_CTRL2_SDRAM_TYPE_DDR3 2
> >>> +#define A3700_MC_CTRL2_SDRAM_TYPE_DDR4 3
> >>> +
> >>> int board_early_init_f(void)
> >>> {
> >>> return 0;
> >>> @@ -63,6 +80,31 @@ int board_init(void)
> >>> return 0;
> >>> }
> >>>
> >>> +int board_late_init(void)
> >>> +{
> >>> + bool ddr4, emmc;
> >>> +
> >>> + if (!of_machine_is_compatible("globalscale,espressobin"))
> >>> + return 0;
> >>> +
> >>> + /* If the memory controller has been configured for DDR4, we're running on v7 */
> >>> + ddr4 = ((readl(A3700_CH0_MC_CTRL2_REG) >> A3700_MC_CTRL2_SDRAM_TYPE_OFFS)
> >>> + & A3700_MC_CTRL2_SDRAM_TYPE_MASK) == A3700_MC_CTRL2_SDRAM_TYPE_DDR4;
> >>
> >> Marek, is this correct way to determining V5 vs V7?
> >
> > If for all ESPRESSObin board versions it is true that
> > "all v7 boards are DDR4 only and all v5 boards are DDR3 only"
> > then yes, you can differentiate with this code.
>
> I believe that's the case.
> There may be other ways to detect the board, this was just the most
> obvious one to me.
>
> > This register is set
> > early in boot process, by the code in TIM image, and if it was set
> > incorrectly, the board would not boot into U-Boot.
> >
> >>> +
> >>> + emmc = of_machine_is_compatible("globalscale,espressobin-emmc");
> >>> +
> >>> + if (ddr4 && emmc)
> >>> + env_set("fdtfile", "marvell/armada-3720-espressobin-v7-emmc.dtb");
> >>> + else if (ddr4)
> >>> + env_set("fdtfile", "marvell/armada-3720-espressobin-v7.dtb");
> >>> + else if (emmc)
> >>> + env_set("fdtfile", "marvell/armada-3720-espressobin-emmc.dtb");
> >>> + else
> >>> + env_set("fdtfile", "marvell/armada-3720-espressobin.dtb");
> >>
> >> This code would overwrite user's value of fdtfile variable. And any
> >> change done by saveenv for fdtfile would be lost. I do not think this is
> >> correct way as it would break booting other distributions/forks (e.g.
> >> Marvell one) which DTB file has different name.
> >>
> >> Also user would not be able to adjust usage of its own DTB file if this
> >> code would overwrite it at every boot.
> >>
> >> Cannot be put value of this variable only to default set? Like all other
> >> variables? So value would be restored/overwritten only by 'env default'
> >> call.
> >
> > There are special U-Boot variables $soc, $board, and $boardver.
> > In include/config_distro_bootcmd.h look at line
> > "setenv efi_fdtfile ${soc}-${board}${boardver}.dtb; "
> > I think you should be setting $boardver env variable in your code, and
> > have default bootscript somehow build fdtfile name from this.
>
> That's protected by
> #if defined(CONFIG_ARM) && !defined(CONFIG_ARM64)
> though, so 32bit only.

Hi,
I meant it as an inspiration, not that that line of code should be used
by your board...
Marek