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

Andre Heider
2020-09-06T18:48:57Z

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
>