* [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile
@ 2020-09-05 12:07 Andre Heider
2020-09-06 9:32 ` Pali Rohár
2020-09-11 4:35 ` [PATCH v2] " Andre Heider
0 siblings, 2 replies; 21+ messages in thread
From: Andre Heider @ 2020-09-05 12:07 UTC (permalink / raw)
To: u-boot
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
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)
{
--
2.28.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-05 12:07 [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile Andre Heider
@ 2020-09-06 9:32 ` Pali Rohár
2020-09-06 16:12 ` Marek Behun
2020-09-06 18:44 ` Andre Heider
2020-09-11 4:35 ` [PATCH v2] " Andre Heider
1 sibling, 2 replies; 21+ messages in thread
From: Pali Rohár @ 2020-09-06 9:32 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-06 9:32 ` Pali Rohár
@ 2020-09-06 16:12 ` Marek Behun
2020-09-06 18:48 ` Andre Heider
2020-09-06 18:44 ` Andre Heider
1 sibling, 1 reply; 21+ messages in thread
From: Marek Behun @ 2020-09-06 16:12 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-06 9:32 ` Pali Rohár
2020-09-06 16:12 ` Marek Behun
@ 2020-09-06 18:44 ` Andre Heider
2020-09-07 7:58 ` Pali Rohár
1 sibling, 1 reply; 21+ messages in thread
From: Andre Heider @ 2020-09-06 18:44 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-06 16:12 ` Marek Behun
@ 2020-09-06 18:48 ` Andre Heider
2020-09-06 18:56 ` Marek Behun
0 siblings, 1 reply; 21+ messages in thread
From: Andre Heider @ 2020-09-06 18:48 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-06 18:48 ` Andre Heider
@ 2020-09-06 18:56 ` Marek Behun
2020-09-07 5:20 ` Andre Heider
0 siblings, 1 reply; 21+ messages in thread
From: Marek Behun @ 2020-09-06 18:56 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-06 18:56 ` Marek Behun
@ 2020-09-07 5:20 ` Andre Heider
0 siblings, 0 replies; 21+ messages in thread
From: Andre Heider @ 2020-09-07 5:20 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-06 18:44 ` Andre Heider
@ 2020-09-07 7:58 ` Pali Rohár
2020-09-07 8:46 ` Andre Heider
0 siblings, 1 reply; 21+ messages in thread
From: Pali Rohár @ 2020-09-07 7:58 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-07 7:58 ` Pali Rohár
@ 2020-09-07 8:46 ` Andre Heider
2020-09-07 8:52 ` Pali Rohár
0 siblings, 1 reply; 21+ messages in thread
From: Andre Heider @ 2020-09-07 8:46 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-07 8:46 ` Andre Heider
@ 2020-09-07 8:52 ` Pali Rohár
2020-09-07 16:51 ` Andre Heider
0 siblings, 1 reply; 21+ messages in thread
From: Pali Rohár @ 2020-09-07 8:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-07 8:52 ` Pali Rohár
@ 2020-09-07 16:51 ` Andre Heider
0 siblings, 0 replies; 21+ messages in thread
From: Andre Heider @ 2020-09-07 16:51 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-05 12:07 [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile Andre Heider
2020-09-06 9:32 ` Pali Rohár
@ 2020-09-11 4:35 ` Andre Heider
2020-09-11 13:02 ` Gérald Kerma
` (2 more replies)
1 sibling, 3 replies; 21+ messages in thread
From: Andre Heider @ 2020-09-11 4:35 UTC (permalink / raw)
To: u-boot
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.
Fixes booting GRUB EFI arm64 on Fedora.
Reported-by: Dennis Gilmore <dennis@ausil.us>
Signed-off-by: Andre Heider <a.heider@gmail.com>
---
v2:
- enable BOARD_LATE_INIT only for espressobin, via defconfig
- don't overwrite a set/saved $fdtfile
This still has the issue that $fdtfile doesn't show up after `env
default -a`. Pali may look into that, but the infrastructure for it
needs to be created first.
Until then, this can be considered as v2010.10 material as it fixes
booting distros relying on $fdtfile.
Tested myself on a v5 board without eMMC.
Tested by G?rald on v7 emmc, v7/ddr4 detection confirmed working.
board/Marvell/mvebu_armada-37xx/board.c | 47 +++++++++++++++++++++
configs/mvebu_espressobin-88f3720_defconfig | 1 +
2 files changed, 48 insertions(+)
diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c
index 90bfc139aa..73d69e0388 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,36 @@ int board_init(void)
return 0;
}
+#ifdef CONFIG_BOARD_LATE_INIT
+int board_late_init(void)
+{
+ bool ddr4, emmc;
+
+ if (env_get("fdtfile"))
+ return 0;
+
+ 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;
+}
+#endif
+
/* Board specific AHCI / SATA enable code */
int board_ahci_enable(void)
{
diff --git a/configs/mvebu_espressobin-88f3720_defconfig b/configs/mvebu_espressobin-88f3720_defconfig
index 5e9fcd1f26..559aeb076f 100644
--- a/configs/mvebu_espressobin-88f3720_defconfig
+++ b/configs/mvebu_espressobin-88f3720_defconfig
@@ -85,3 +85,4 @@ CONFIG_USB_ETHER_SMSC95XX=y
CONFIG_SHA1=y
CONFIG_SHA256=y
CONFIG_NET_RANDOM_ETHADDR=y
+CONFIG_BOARD_LATE_INIT=y
--
2.28.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-11 4:35 ` [PATCH v2] " Andre Heider
@ 2020-09-11 13:02 ` Gérald Kerma
2020-09-24 10:38 ` Stefan Roese
2020-09-25 7:46 ` Pali Rohár
2 siblings, 0 replies; 21+ messages in thread
From: Gérald Kerma @ 2020-09-11 13:02 UTC (permalink / raw)
To: u-boot
Le 11/09/2020 ? 06:35, Andre Heider a ?crit?:
> 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.
>
> Fixes booting GRUB EFI arm64 on Fedora.
>
> Reported-by: Dennis Gilmore <dennis@ausil.us>
> Signed-off-by: Andre Heider <a.heider@gmail.com>
> ---
>
> v2:
> - enable BOARD_LATE_INIT only for espressobin, via defconfig
> - don't overwrite a set/saved $fdtfile
>
> This still has the issue that $fdtfile doesn't show up after `env
> default -a`. Pali may look into that, but the infrastructure for it
> needs to be created first.
>
> Until then, this can be considered as v2010.10 material as it fixes
> booting distros relying on $fdtfile.
>
> Tested myself on a v5 board without eMMC.
> Tested by G?rald on v7 emmc, v7/ddr4 detection confirmed working.
>
> board/Marvell/mvebu_armada-37xx/board.c | 47 +++++++++++++++++++++
> configs/mvebu_espressobin-88f3720_defconfig | 1 +
> 2 files changed, 48 insertions(+)
>
> diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c
> index 90bfc139aa..73d69e0388 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,36 @@ int board_init(void)
> return 0;
> }
>
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> + bool ddr4, emmc;
> +
> + if (env_get("fdtfile"))
> + return 0;
> +
> + 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;
> +}
> +#endif
> +
> /* Board specific AHCI / SATA enable code */
> int board_ahci_enable(void)
> {
> diff --git a/configs/mvebu_espressobin-88f3720_defconfig b/configs/mvebu_espressobin-88f3720_defconfig
> index 5e9fcd1f26..559aeb076f 100644
> --- a/configs/mvebu_espressobin-88f3720_defconfig
> +++ b/configs/mvebu_espressobin-88f3720_defconfig
> @@ -85,3 +85,4 @@ CONFIG_USB_ETHER_SMSC95XX=y
> CONFIG_SHA1=y
> CONFIG_SHA256=y
> CONFIG_NET_RANDOM_ETHADDR=y
> +CONFIG_BOARD_LATE_INIT=y
|Tested-by: G?rald Kerma <gandalf@gk2.net>|
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-11 4:35 ` [PATCH v2] " Andre Heider
2020-09-11 13:02 ` Gérald Kerma
@ 2020-09-24 10:38 ` Stefan Roese
2020-09-25 7:46 ` Pali Rohár
2 siblings, 0 replies; 21+ messages in thread
From: Stefan Roese @ 2020-09-24 10:38 UTC (permalink / raw)
To: u-boot
On 11.09.20 06:35, 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.
>
> Fixes booting GRUB EFI arm64 on Fedora.
>
> Reported-by: Dennis Gilmore <dennis@ausil.us>
> Signed-off-by: Andre Heider <a.heider@gmail.com>
Applied to u-boot-marvell/master
Thanks,
Stefan
> ---
>
> v2:
> - enable BOARD_LATE_INIT only for espressobin, via defconfig
> - don't overwrite a set/saved $fdtfile
>
> This still has the issue that $fdtfile doesn't show up after `env
> default -a`. Pali may look into that, but the infrastructure for it
> needs to be created first.
>
> Until then, this can be considered as v2010.10 material as it fixes
> booting distros relying on $fdtfile.
>
> Tested myself on a v5 board without eMMC.
> Tested by G?rald on v7 emmc, v7/ddr4 detection confirmed working.
>
> board/Marvell/mvebu_armada-37xx/board.c | 47 +++++++++++++++++++++
> configs/mvebu_espressobin-88f3720_defconfig | 1 +
> 2 files changed, 48 insertions(+)
>
> diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c
> index 90bfc139aa..73d69e0388 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,36 @@ int board_init(void)
> return 0;
> }
>
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> + bool ddr4, emmc;
> +
> + if (env_get("fdtfile"))
> + return 0;
> +
> + 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;
> +}
> +#endif
> +
> /* Board specific AHCI / SATA enable code */
> int board_ahci_enable(void)
> {
> diff --git a/configs/mvebu_espressobin-88f3720_defconfig b/configs/mvebu_espressobin-88f3720_defconfig
> index 5e9fcd1f26..559aeb076f 100644
> --- a/configs/mvebu_espressobin-88f3720_defconfig
> +++ b/configs/mvebu_espressobin-88f3720_defconfig
> @@ -85,3 +85,4 @@ CONFIG_USB_ETHER_SMSC95XX=y
> CONFIG_SHA1=y
> CONFIG_SHA256=y
> CONFIG_NET_RANDOM_ETHADDR=y
> +CONFIG_BOARD_LATE_INIT=y
>
Viele Gr??e,
Stefan
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-11 4:35 ` [PATCH v2] " Andre Heider
2020-09-11 13:02 ` Gérald Kerma
2020-09-24 10:38 ` Stefan Roese
@ 2020-09-25 7:46 ` Pali Rohár
2020-09-26 9:09 ` Andre Heider
2 siblings, 1 reply; 21+ messages in thread
From: Pali Rohár @ 2020-09-25 7:46 UTC (permalink / raw)
To: u-boot
On Friday 11 September 2020 06:35:10 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.
>
> Fixes booting GRUB EFI arm64 on Fedora.
>
> Reported-by: Dennis Gilmore <dennis@ausil.us>
> Signed-off-by: Andre Heider <a.heider@gmail.com>
> ---
>
> v2:
> - enable BOARD_LATE_INIT only for espressobin, via defconfig
> - don't overwrite a set/saved $fdtfile
>
> This still has the issue that $fdtfile doesn't show up after `env
> default -a`. Pali may look into that, but the infrastructure for it
> needs to be created first.
>
> Until then, this can be considered as v2010.10 material as it fixes
> booting distros relying on $fdtfile.
>
> Tested myself on a v5 board without eMMC.
> Tested by G?rald on v7 emmc, v7/ddr4 detection confirmed working.
>
> board/Marvell/mvebu_armada-37xx/board.c | 47 +++++++++++++++++++++
> configs/mvebu_espressobin-88f3720_defconfig | 1 +
> 2 files changed, 48 insertions(+)
>
> diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c
> index 90bfc139aa..73d69e0388 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,36 @@ int board_init(void)
> return 0;
> }
>
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> + bool ddr4, emmc;
> +
> + if (env_get("fdtfile"))
> + return 0;
> +
> + 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");
Maybe stupid question, but are not we able to detect if eMMC is
connected or not at runtime? That could simplify setup and avoid usage
of additional special DTS file for eMMC in U-Boot.
> +
> + 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;
> +}
> +#endif
> +
> /* Board specific AHCI / SATA enable code */
> int board_ahci_enable(void)
> {
> diff --git a/configs/mvebu_espressobin-88f3720_defconfig b/configs/mvebu_espressobin-88f3720_defconfig
> index 5e9fcd1f26..559aeb076f 100644
> --- a/configs/mvebu_espressobin-88f3720_defconfig
> +++ b/configs/mvebu_espressobin-88f3720_defconfig
> @@ -85,3 +85,4 @@ CONFIG_USB_ETHER_SMSC95XX=y
> CONFIG_SHA1=y
> CONFIG_SHA256=y
> CONFIG_NET_RANDOM_ETHADDR=y
> +CONFIG_BOARD_LATE_INIT=y
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-25 7:46 ` Pali Rohár
@ 2020-09-26 9:09 ` Andre Heider
2020-10-02 10:49 ` Pali Rohár
0 siblings, 1 reply; 21+ messages in thread
From: Andre Heider @ 2020-09-26 9:09 UTC (permalink / raw)
To: u-boot
On 25/09/2020 09:46, Pali Roh?r wrote:
> On Friday 11 September 2020 06:35:10 Andre Heider wrote:
...
>>
>> +#ifdef CONFIG_BOARD_LATE_INIT
>> +int board_late_init(void)
>> +{
>> + bool ddr4, emmc;
>> +
>> + if (env_get("fdtfile"))
>> + return 0;
>> +
>> + 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");
>
> Maybe stupid question, but are not we able to detect if eMMC is
> connected or not at runtime? That could simplify setup and avoid usage
> of additional special DTS file for eMMC in U-Boot.
At some point I wondered about the same.
IIRC armbian just enables it and uses one u-boot binary everywhere. A
non-existing chip won't get detected, so that shouldn't be a problem.
But I think it has more to do with enabling/powering up hardware blocks,
which is not wanted or may even problematic.
Regards,
Andre
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-09-26 9:09 ` Andre Heider
@ 2020-10-02 10:49 ` Pali Rohár
2020-10-03 7:17 ` Andre Heider
0 siblings, 1 reply; 21+ messages in thread
From: Pali Rohár @ 2020-10-02 10:49 UTC (permalink / raw)
To: u-boot
On Saturday 26 September 2020 11:09:59 Andre Heider wrote:
> On 25/09/2020 09:46, Pali Roh?r wrote:
> > On Friday 11 September 2020 06:35:10 Andre Heider wrote:
> ...
> > > +#ifdef CONFIG_BOARD_LATE_INIT
> > > +int board_late_init(void)
> > > +{
> > > + bool ddr4, emmc;
> > > +
> > > + if (env_get("fdtfile"))
> > > + return 0;
> > > +
> > > + 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");
> >
> > Maybe stupid question, but are not we able to detect if eMMC is
> > connected or not at runtime? That could simplify setup and avoid usage
> > of additional special DTS file for eMMC in U-Boot.
>
> At some point I wondered about the same.
>
> IIRC armbian just enables it and uses one u-boot binary everywhere. A
> non-existing chip won't get detected, so that shouldn't be a problem.
>
> But I think it has more to do with enabling/powering up hardware blocks,
> which is not wanted or may even problematic.
In U-Boot such functionality may be implemented in board_fix_fdt()
function which allows U-Boot to modify its Device tree prior using it.
You have already wrote code which is doing V5 vs V7 detection, so
detecting eMMC is the last thing which is missing to have just one
U-Boot DTS file and therefore only one U-Boot binary for Espressobin.
> Regards,
> Andre
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-10-02 10:49 ` Pali Rohár
@ 2020-10-03 7:17 ` Andre Heider
2020-10-03 8:50 ` Pali Rohár
0 siblings, 1 reply; 21+ messages in thread
From: Andre Heider @ 2020-10-03 7:17 UTC (permalink / raw)
To: u-boot
On 02/10/2020 12:49, Pali Roh?r wrote:
> On Saturday 26 September 2020 11:09:59 Andre Heider wrote:
>> On 25/09/2020 09:46, Pali Roh?r wrote:
>>> On Friday 11 September 2020 06:35:10 Andre Heider wrote:
>> ...
>>>> +#ifdef CONFIG_BOARD_LATE_INIT
>>>> +int board_late_init(void)
>>>> +{
>>>> + bool ddr4, emmc;
>>>> +
>>>> + if (env_get("fdtfile"))
>>>> + return 0;
>>>> +
>>>> + 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");
>>>
>>> Maybe stupid question, but are not we able to detect if eMMC is
>>> connected or not at runtime? That could simplify setup and avoid usage
>>> of additional special DTS file for eMMC in U-Boot.
>>
>> At some point I wondered about the same.
>>
>> IIRC armbian just enables it and uses one u-boot binary everywhere. A
>> non-existing chip won't get detected, so that shouldn't be a problem.
>>
>> But I think it has more to do with enabling/powering up hardware blocks,
>> which is not wanted or may even problematic.
>
> In U-Boot such functionality may be implemented in board_fix_fdt()
> function which allows U-Boot to modify its Device tree prior using it.
>
> You have already wrote code which is doing V5 vs V7 detection, so
> detecting eMMC is the last thing which is missing to have just one
> U-Boot DTS file and therefore only one U-Boot binary for Espressobin.
That implies setting status=okay for the emmc node, which then powers up
that block. I don't know if that might be problematic. Can we just do that?
The emmc detection would also add some delay to the boot process.
At least it should be powered down if no emmc chip has been detected,
but I didn't check if that happens right now.
Regards,
Andre
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-10-03 7:17 ` Andre Heider
@ 2020-10-03 8:50 ` Pali Rohár
2020-10-05 16:20 ` Andre Heider
0 siblings, 1 reply; 21+ messages in thread
From: Pali Rohár @ 2020-10-03 8:50 UTC (permalink / raw)
To: u-boot
On Saturday 03 October 2020 09:17:35 Andre Heider wrote:
> On 02/10/2020 12:49, Pali Roh?r wrote:
> > On Saturday 26 September 2020 11:09:59 Andre Heider wrote:
> > > On 25/09/2020 09:46, Pali Roh?r wrote:
> > > > On Friday 11 September 2020 06:35:10 Andre Heider wrote:
> > > ...
> > > > > +#ifdef CONFIG_BOARD_LATE_INIT
> > > > > +int board_late_init(void)
> > > > > +{
> > > > > + bool ddr4, emmc;
> > > > > +
> > > > > + if (env_get("fdtfile"))
> > > > > + return 0;
> > > > > +
> > > > > + 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");
> > > >
> > > > Maybe stupid question, but are not we able to detect if eMMC is
> > > > connected or not at runtime? That could simplify setup and avoid usage
> > > > of additional special DTS file for eMMC in U-Boot.
> > >
> > > At some point I wondered about the same.
> > >
> > > IIRC armbian just enables it and uses one u-boot binary everywhere. A
> > > non-existing chip won't get detected, so that shouldn't be a problem.
> > >
> > > But I think it has more to do with enabling/powering up hardware blocks,
> > > which is not wanted or may even problematic.
> >
> > In U-Boot such functionality may be implemented in board_fix_fdt()
> > function which allows U-Boot to modify its Device tree prior using it.
> >
> > You have already wrote code which is doing V5 vs V7 detection, so
> > detecting eMMC is the last thing which is missing to have just one
> > U-Boot DTS file and therefore only one U-Boot binary for Espressobin.
>
> That implies setting status=okay for the emmc node, which then powers up
> that block. I don't know if that might be problematic. Can we just do that?
No. I mean to detect presence of eMMC in board_fix_fdt() and then set
tatus=okay only if check passed.
> The emmc detection would also add some delay to the boot process.
>
> At least it should be powered down if no emmc chip has been detected, but I
> didn't check if that happens right now.
>
> Regards,
> Andre
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-10-03 8:50 ` Pali Rohár
@ 2020-10-05 16:20 ` Andre Heider
2020-10-06 11:02 ` Pali Rohár
0 siblings, 1 reply; 21+ messages in thread
From: Andre Heider @ 2020-10-05 16:20 UTC (permalink / raw)
To: u-boot
On 03/10/2020 10:50, Pali Roh?r wrote:
> On Saturday 03 October 2020 09:17:35 Andre Heider wrote:
>> On 02/10/2020 12:49, Pali Roh?r wrote:
>>> On Saturday 26 September 2020 11:09:59 Andre Heider wrote:
>>>> On 25/09/2020 09:46, Pali Roh?r wrote:
>>>>> On Friday 11 September 2020 06:35:10 Andre Heider wrote:
>>>> ...
>>>>>> +#ifdef CONFIG_BOARD_LATE_INIT
>>>>>> +int board_late_init(void)
>>>>>> +{
>>>>>> + bool ddr4, emmc;
>>>>>> +
>>>>>> + if (env_get("fdtfile"))
>>>>>> + return 0;
>>>>>> +
>>>>>> + 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");
>>>>>
>>>>> Maybe stupid question, but are not we able to detect if eMMC is
>>>>> connected or not at runtime? That could simplify setup and avoid usage
>>>>> of additional special DTS file for eMMC in U-Boot.
>>>>
>>>> At some point I wondered about the same.
>>>>
>>>> IIRC armbian just enables it and uses one u-boot binary everywhere. A
>>>> non-existing chip won't get detected, so that shouldn't be a problem.
>>>>
>>>> But I think it has more to do with enabling/powering up hardware blocks,
>>>> which is not wanted or may even problematic.
>>>
>>> In U-Boot such functionality may be implemented in board_fix_fdt()
>>> function which allows U-Boot to modify its Device tree prior using it.
>>>
>>> You have already wrote code which is doing V5 vs V7 detection, so
>>> detecting eMMC is the last thing which is missing to have just one
>>> U-Boot DTS file and therefore only one U-Boot binary for Espressobin.
>>
>> That implies setting status=okay for the emmc node, which then powers up
>> that block. I don't know if that might be problematic. Can we just do that?
>
> No. I mean to detect presence of eMMC in board_fix_fdt() and then set
> tatus=okay only if check passed.
Yes, but how do you detect the emmc then? Enabling it in u-boot's dts
and calling mmc_init() on it would be the easy way, but open coding all
the required parts to do that with status=disabled could get rather big
and/or unmaintanable (I didn't check what exactly would be required)?
>
>> The emmc detection would also add some delay to the boot process.
>>
>> At least it should be powered down if no emmc chip has been detected, but I
>> didn't check if that happens right now.
>>
>> Regards,
>> Andre
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] arm: mvebu: Espressobin: Set environment variable fdtfile
2020-10-05 16:20 ` Andre Heider
@ 2020-10-06 11:02 ` Pali Rohár
0 siblings, 0 replies; 21+ messages in thread
From: Pali Rohár @ 2020-10-06 11:02 UTC (permalink / raw)
To: u-boot
On Monday 05 October 2020 18:20:17 Andre Heider wrote:
> On 03/10/2020 10:50, Pali Roh?r wrote:
> > On Saturday 03 October 2020 09:17:35 Andre Heider wrote:
> > > On 02/10/2020 12:49, Pali Roh?r wrote:
> > > > On Saturday 26 September 2020 11:09:59 Andre Heider wrote:
> > > > > On 25/09/2020 09:46, Pali Roh?r wrote:
> > > > > > On Friday 11 September 2020 06:35:10 Andre Heider wrote:
> > > > > ...
> > > > > > > +#ifdef CONFIG_BOARD_LATE_INIT
> > > > > > > +int board_late_init(void)
> > > > > > > +{
> > > > > > > + bool ddr4, emmc;
> > > > > > > +
> > > > > > > + if (env_get("fdtfile"))
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + 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");
> > > > > >
> > > > > > Maybe stupid question, but are not we able to detect if eMMC is
> > > > > > connected or not at runtime? That could simplify setup and avoid usage
> > > > > > of additional special DTS file for eMMC in U-Boot.
> > > > >
> > > > > At some point I wondered about the same.
> > > > >
> > > > > IIRC armbian just enables it and uses one u-boot binary everywhere. A
> > > > > non-existing chip won't get detected, so that shouldn't be a problem.
> > > > >
> > > > > But I think it has more to do with enabling/powering up hardware blocks,
> > > > > which is not wanted or may even problematic.
> > > >
> > > > In U-Boot such functionality may be implemented in board_fix_fdt()
> > > > function which allows U-Boot to modify its Device tree prior using it.
> > > >
> > > > You have already wrote code which is doing V5 vs V7 detection, so
> > > > detecting eMMC is the last thing which is missing to have just one
> > > > U-Boot DTS file and therefore only one U-Boot binary for Espressobin.
> > >
> > > That implies setting status=okay for the emmc node, which then powers up
> > > that block. I don't know if that might be problematic. Can we just do that?
> >
> > No. I mean to detect presence of eMMC in board_fix_fdt() and then set
> > tatus=okay only if check passed.
>
> Yes, but how do you detect the emmc then? Enabling it in u-boot's dts and
> calling mmc_init() on it would be the easy way, but open coding all the
> required parts to do that with status=disabled could get rather big and/or
> unmaintanable (I didn't check what exactly would be required)?
Hm... seems that it would be really easier to enable eMMC in U-Boot DTS,
let U-Boot to try initialize eMMC and in case it fails, then disable
eMMC drivers &?devices and disable it also in FTB blob.
For powering up eMMC there is function comphy_emmc_power_up(), so it
would be needed to write power_down equivalent. And for xenon_sdhci.c
would be needed to write .remove callback (for disabling eMMC).
> >
> > > The emmc detection would also add some delay to the boot process.
> > >
> > > At least it should be powered down if no emmc chip has been detected, but I
> > > didn't check if that happens right now.
> > >
> > > Regards,
> > > Andre
>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-10-06 11:02 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-05 12:07 [PATCH] arm: mvebu: Espressobin: Set environment variable fdtfile Andre Heider
2020-09-06 9:32 ` Pali Rohár
2020-09-06 16:12 ` Marek Behun
2020-09-06 18:48 ` Andre Heider
2020-09-06 18:56 ` Marek Behun
2020-09-07 5:20 ` Andre Heider
2020-09-06 18:44 ` Andre Heider
2020-09-07 7:58 ` Pali Rohár
2020-09-07 8:46 ` Andre Heider
2020-09-07 8:52 ` Pali Rohár
2020-09-07 16:51 ` Andre Heider
2020-09-11 4:35 ` [PATCH v2] " Andre Heider
2020-09-11 13:02 ` Gérald Kerma
2020-09-24 10:38 ` Stefan Roese
2020-09-25 7:46 ` Pali Rohár
2020-09-26 9:09 ` Andre Heider
2020-10-02 10:49 ` Pali Rohár
2020-10-03 7:17 ` Andre Heider
2020-10-03 8:50 ` Pali Rohár
2020-10-05 16:20 ` Andre Heider
2020-10-06 11:02 ` Pali Rohár
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.