| Message ID | 20200905120744.634300-1-a.heider@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | arm: mvebu: Espressobin: Set environment variable fdtfile | expand |
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. > 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? > + > + 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. > + return 0; > +} > + > /* Board specific AHCI / SATA enable code */ > int board_ahci_enable(void) > { > -- > 2.28.0 >
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. 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. Marek
On 06/09/2020 11:32, Pali Rohár 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. Right you are, fixed locally. > >> 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? > >> + >> + 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. Indeed, it shouldn't wipe a saved $fdtfile. Fixed locally with adding this hunk to the start of the function: + if (env_get("fdtfile")) + return 0; CC'ed Baruch, since I looked at the clearfog implementation which has the same bug. > 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. It would be possible to avoid the above runtime detection and just add: "fdtfile=marvell/" CONFIG_DEFAULT_DEVICE_TREE ".dtb\0" to CONFIG_EXTRA_ENV_SETTINGS instead. But for that, the u-boot dtb variants need to match Linux', which they currently do not. I posted a set which adds "-emmc.dts". But "-v7.dts" and "v7-emmc.dts" are still missing here, and I don't think it makes sense to add them, because all they're doing is relabeling the switch ports. Or maybe it's desired to have all dts files from Linux here too, even though we technically don't need a binary for each board? > >> + return 0; >> +} >> + >> /* Board specific AHCI / SATA enable code */ >> int board_ahci_enable(void) >> { >> -- >> 2.28.0 >> Thanks, Andre
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 >
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
Hi Marek, On 06/09/2020 20:56, Marek Behun wrote: > 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 Oh sorry, I misread the first time. I see what you mean, and there're boards which do just that, and there're boards which do it like my patch. But in the end it would just use another envvar, with the added complexity of a little scripting. At first glance, plain $fdtfile might be more straight forward? Is there another advantage of your approach I'm not seeing yet? Thanks, Andre
On Sunday 06 September 2020 20:44:50 Andre Heider wrote: > On 06/09/2020 11:32, Pali Rohár wrote: > > On Saturday 05 September 2020 14:07:44 Andre Heider wrote: > > > + > > > + 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. > > Indeed, it shouldn't wipe a saved $fdtfile. Fixed locally with adding this > hunk to the start of the function: > + if (env_get("fdtfile")) > + return 0; This has still one issue: 'env default -a' does not restore original value. I would expect that 'env default -a' resets these values to correct default. > CC'ed Baruch, since I looked at the clearfog implementation which has the > same bug.
Hi Pali, On 07/09/2020 09:58, Pali Rohár wrote: > On Sunday 06 September 2020 20:44:50 Andre Heider wrote: >> On 06/09/2020 11:32, Pali Rohár wrote: >>> On Saturday 05 September 2020 14:07:44 Andre Heider wrote: >>>> + >>>> + 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. >> >> Indeed, it shouldn't wipe a saved $fdtfile. Fixed locally with adding this >> hunk to the start of the function: >> + if (env_get("fdtfile")) >> + return 0; > > This has still one issue: 'env default -a' does not restore original > value. I would expect that 'env default -a' resets these values to > correct default. there doesn't seem to be a way to archive that programmatically? The default argument needs to be known at compile time. Regards, Andre
On Monday 07 September 2020 10:46:37 Andre Heider wrote: > Hi Pali, > > On 07/09/2020 09:58, Pali Rohár wrote: > > On Sunday 06 September 2020 20:44:50 Andre Heider wrote: > > > On 06/09/2020 11:32, Pali Rohár wrote: > > > > On Saturday 05 September 2020 14:07:44 Andre Heider wrote: > > > > > + > > > > > + 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. > > > > > > Indeed, it shouldn't wipe a saved $fdtfile. Fixed locally with adding this > > > hunk to the start of the function: > > > + if (env_get("fdtfile")) > > > + return 0; > > > > This has still one issue: 'env default -a' does not restore original > > value. I would expect that 'env default -a' resets these values to > > correct default. > > there doesn't seem to be a way to archive that programmatically? > The default argument needs to be known at compile time. So what about fixing it instead of adding new hacks? Currently default env needs to be known at compile time due to assignment to static const storage. But this can be changed and e.g. board could could provide weak function which returns additional variables/value which uboot main code can use for default settings. > Regards, > Andre
On 07/09/2020 10:52, Pali Rohár wrote: > On Monday 07 September 2020 10:46:37 Andre Heider wrote: >> Hi Pali, >> >> On 07/09/2020 09:58, Pali Rohár wrote: >>> On Sunday 06 September 2020 20:44:50 Andre Heider wrote: >>>> On 06/09/2020 11:32, Pali Rohár wrote: >>>>> On Saturday 05 September 2020 14:07:44 Andre Heider wrote: >>>>>> + >>>>>> + 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. >>>> >>>> Indeed, it shouldn't wipe a saved $fdtfile. Fixed locally with adding this >>>> hunk to the start of the function: >>>> + if (env_get("fdtfile")) >>>> + return 0; >>> >>> This has still one issue: 'env default -a' does not restore original >>> value. I would expect that 'env default -a' resets these values to >>> correct default. >> >> there doesn't seem to be a way to archive that programmatically? >> The default argument needs to be known at compile time. > > So what about fixing it instead of adding new hacks? Right, so for the occasional u-boot hacker like me it's not obvious how to solve a certain problem. Usually I check how other boards solve it. By your definition of hack other boards are full of it. > Currently default env needs to be known at compile time due to > assignment to static const storage. But this can be changed and e.g. > board could could provide weak function which returns additional > variables/value which uboot main code can use for default settings. Sounds like you have an idea how to solve it nicely, so please go ahead. >> Regards, >> Andre
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 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; + + 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"); + + return 0; +} + /* Board specific AHCI / SATA enable code */ int board_ahci_enable(void) {
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(+)