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

[Andre Heider <a.heider@gmail.com>]

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.

> 
> Marek
>