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

[Marek Behun]

On Sun, 6 Sep 2020 20:48:57 +0200
Andre Heider <a.heider at 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 at 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 at 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