diff mbox series

[OpenWrt-Devel] mvebu: add support for GL.iNet GL-MV1000

Message ID 1586773611-27431-1-git-send-email-li.zhang@gl-inet.com
State New
Headers show
Series [OpenWrt-Devel] mvebu: add support for GL.iNet GL-MV1000 | expand

Commit Message

Li Zhang April 13, 2020, 10:26 a.m. UTC
This patch adds supports for GL-MV1000.

Specification:
	- SOC: Marvell Armada 88F3720 (1GHz)
	- Flash: 16MB (W25Q128FWSIG)
	- RAM: 1GB DDR4
	- Ethernet: 3x GE (1 WAN + 2 LAN)
	- EMMC: 8GB EMMC (KLM8G1GETF-B041)
	- MicroSD: 1x microSD slot
	- USB: 1x USB 2.0 port(TypeA),1x USB 3.0 port(TypeC)
	- Button: 1x reset button,1x slide switch
	- LED: 3x greed LED
	- UART: 1x UART on PCB (JP1: 3.3V, RX, TX, GND)

Signed-off-by: Li Zhang <li.zhang@gl-inet.com>
---
 package/boot/uboot-envtools/files/mvebu            |  3 +
 .../cortexa53/base-files/etc/board.d/02_network    |  3 +-
 .../boot/dts/marvell/armada-gl-mv1000-emmc.dts     | 68 ++++++++++++++++++++++
 target/linux/mvebu/image/Makefile                  |  9 +++
 target/linux/mvebu/image/cortexa53.mk              |  9 +++
 target/linux/mvebu/image/gen_mvebu_sdcard_img.sh   |  6 ++
 .../mvebu/image/generic-arm64-emmc.bootscript      | 12 ++++
 7 files changed, 109 insertions(+), 1 deletion(-)
 create mode 100644 target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
 create mode 100644 target/linux/mvebu/image/generic-arm64-emmc.bootscript

Comments

Tomasz Maciej Nowak April 13, 2020, 8:14 p.m. UTC | #1
Hi Li Zhang.

This version looks much better than the first one,
some comments inline.

W dniu 13.04.2020 o 12:26, Li Zhang pisze:
> This patch adds supports for GL-MV1000.
> 
> Specification:
> 	- SOC: Marvell Armada 88F3720 (1GHz)
> 	- Flash: 16MB (W25Q128FWSIG)
> 	- RAM: 1GB DDR4
> 	- Ethernet: 3x GE (1 WAN + 2 LAN)
> 	- EMMC: 8GB EMMC (KLM8G1GETF-B041)
> 	- MicroSD: 1x microSD slot
> 	- USB: 1x USB 2.0 port(TypeA),1x USB 3.0 port(TypeC)
> 	- Button: 1x reset button,1x slide switch
> 	- LED: 3x greed LED
> 	- UART: 1x UART on PCB (JP1: 3.3V, RX, TX, GND)
> 
> Signed-off-by: Li Zhang <li.zhang@gl-inet.com>
> ---
>  package/boot/uboot-envtools/files/mvebu            |  3 +
>  .../cortexa53/base-files/etc/board.d/02_network    |  3 +-
>  .../boot/dts/marvell/armada-gl-mv1000-emmc.dts     | 68 ++++++++++++++++++++++
>  target/linux/mvebu/image/Makefile                  |  9 +++
>  target/linux/mvebu/image/cortexa53.mk              |  9 +++
>  target/linux/mvebu/image/gen_mvebu_sdcard_img.sh   |  6 ++
>  .../mvebu/image/generic-arm64-emmc.bootscript      | 12 ++++
>  7 files changed, 109 insertions(+), 1 deletion(-)
>  create mode 100644 target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
>  create mode 100644 target/linux/mvebu/image/generic-arm64-emmc.bootscript
> 
> diff --git a/package/boot/uboot-envtools/files/mvebu b/package/boot/uboot-envtools/files/mvebu
> index 7902384..9d23c77 100644
> --- a/package/boot/uboot-envtools/files/mvebu
> +++ b/package/boot/uboot-envtools/files/mvebu
> @@ -24,6 +24,9 @@ globalscale,espressobin-v7-emmc|\
>  marvell,armada8040-mcbin)
>  	ubootenv_add_uci_config "/dev/mtd0" "0x3f0000" "0x10000" "0x10000" "1"
>  	;;
> +glinet,gl-mv1000-emmc)
> +	ubootenv_add_uci_config "/dev/mtd1" "0x0" "0x8000" "0x8000" "1"
> +	;;
>  linksys,caiman|\
>  linksys,cobra|\
>  linksys,shelby)
> diff --git a/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network b/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
> index ba4b930..e5ff667 100755
> --- a/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
> +++ b/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
> @@ -14,7 +14,8 @@ case "$board" in
>  globalscale,espressobin|\
>  globalscale,espressobin-emmc|\
>  globalscale,espressobin-v7|\
> -globalscale,espressobin-v7-emmc)
> +globalscale,espressobin-v7-emmc|\
> +glinet,gl-mv1000-emmc)
>  	ucidef_set_interfaces_lan_wan "lan0 lan1" "wan"
>  	;;
>  marvell,armada-3720-db|\
> diff --git a/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts b/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
> new file mode 100644
> index 0000000..fe01dfe
> --- /dev/null
> +++ b/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
> @@ -0,0 +1,68 @@

Please add a license in SPDX format, a common one is dual MIT and
GPL-2.0+.

> +/*
> + * Device Tree file for GL.iNet GL-MV1000
> + */
> +
> +#include "armada-3720-espressobin.dts"

This is different device from ESPRESSObin altogether, please make it
stand-alone dts (copy espressobin dts, add required nodes and
remove/disable nodes for devices not present on the GL-MV1000).
Would be good to add all LEDs and buttons as nodes, which seem to be
GPIO controlled as in this picture:
https://static.gl-inet.com/docs/en/3/hardware/mv1000/mv1000.png
but that's not mandatory.

> +
> +/ {
> +       model = "GL.inet GL-MV1000 (Marvell)";

Instead of Marvell more apropriate would be Brume, since this is used
on Your website.

> +       compatible = "glinet,gl-mv1000-emmc";

Add here to compatible array in second place "marvell,armada3720".
Are there any boards in cutomers hands which do not have SD card slot
or eMMC soldered? There is no point in indicating the eMMC presence
when all sold boards are the same. In case of ESPRESSObin it's there
because various hardware versions. So please remove all -emmc suffixes
from files and file names across this patch if that's the case. One
exeption would be if U-Boot searches for exctly this name
(armada-gl-mv1000-emmc.dts) when booting, but looking at the usage of
boot.scr that's not the case.

> +};
> +
> +&spi0 {
> +        status = "okay";
> +
> +        flash@0 {
> +                reg = <0>;
> +                compatible = "jedec,spi-nor";
> +                spi-max-frequency = <104000000>;
> +                m25p,fast-read;
> +                partitions {
> +                        compatible = "fixed-partitions";
> +                        #address-cells = <1>;
> +                        #size-cells = <1>;
> +
> +                        partition@0 {
> +                                label = "u-boot";
> +                                reg = <0 0xf0000>;
> +                        };
> +
> +                        partition@f0000 {
> +                                label = "u-boot-env";
> +                                reg = <0Xf0000 0x8000>;
> +                        };
> +
> +                        art: partition@f8000 {
> +                                label = "art";

The name of this partition is rather strange, since the hardware
specification doesn't mention any PCIe or SDIO connected wireless
cards (don't know if there's any USB card without eeprom). Is
there any calibration data stored or only MAC address/addresses?
Does U-Boot also read information from this partition? Anyway
that's not a remark to change that name, I'm just curious.

> +                                reg = <0xf8000 0x8000>;
> +                        };

On previous patch, there were additionnal partitions in dts
without -emmc suffix : dtb, firmware. If that space would be
unused it's better to add them here, so later You could
introduce images that could be written in SPI flash.

> +
> +               };
> +        };
> +};
> +
> +&sdhci1 {
> +        wp-inverted;
> +        bus-width = <4>;
> +        cd-gpios = <&gpionb 17 GPIO_ACTIVE_LOW>;
> +        marvell,pad-type = "sd";
> +        no-1-8-v;
> +        vqmmc-supply = <&vcc_sd_reg1>;
> +        status = "okay";
> +};
> +
> +
> +&sdhci0 {
> +        bus-width = <8>;
> +        mmc-ddr-1_8v;
> +        mmc-hs400-1_8v;
> +        non-removable;
> +        no-sd;
> +        no-sdio;
> +        marvell,pad-type = "fixed-1-8v";
> +        status = "okay";
> +};
> +
> +&eth0 {
> +	mtd-mac-address = <&art 0x0>;
> +};
> diff --git a/target/linux/mvebu/image/Makefile b/target/linux/mvebu/image/Makefile
> index ef92748..b848049 100644
> --- a/target/linux/mvebu/image/Makefile
> +++ b/target/linux/mvebu/image/Makefile
> @@ -107,6 +107,15 @@ define Device/Default-arm64
>    KERNEL := kernel-bin
>  endef
>  
> +define Device/Default-arm64-emmc

Please don't add generic difinition for single device where only
image name and recipe are slightly changed, please remove it.

> +  BOOT_SCRIPT := generic-arm64-emmc
> +  DEVICE_DTS_DIR := $(DTS_DIR)/marvell
> +  IMAGES := emmc.img
> +  IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata
> +  KERNEL_NAME := Image
> +  KERNEL := kernel-bin
> +endef
> +
>  define Device/NAND-128K
>    BLOCKSIZE := 128k
>    PAGESIZE := 2048
> diff --git a/target/linux/mvebu/image/cortexa53.mk b/target/linux/mvebu/image/cortexa53.mk
> index 77f5c79..4dfd0b4 100644
> --- a/target/linux/mvebu/image/cortexa53.mk
> +++ b/target/linux/mvebu/image/cortexa53.mk
> @@ -69,3 +69,12 @@ define Device/methode_udpu
>    BOOT_SCRIPT := udpu
>  endef
>  TARGET_DEVICES += methode_udpu
> +
> +define Device/glinet_gl-mv1000-emmc
> +  $(call Device/Default-arm64-emmc)

Please call "Default-arm64" here. You can override image recipe
here adding (don't know if that'll be necessary):
BOOT_SCRIPT := generic-arm64-emmc
IMAGES := emmc.img
IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata

> +  DEVICE_TITLE := GL.iNet GL-MV1000 EMMC
> +  DEVICE_DTS := armada-gl-mv1000-emmc
> +  SUPPORTED_DEVICES := glinet,gl-mv1000-emmc
> +endef
> +TARGET_DEVICES += glinet_gl-mv1000-emmc
> +
> diff --git a/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh b/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
> index 842e591..50028fe 100755
> --- a/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
> +++ b/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
> @@ -51,6 +51,12 @@ while [ "$#" -ge 3 ]; do
>  	shift; shift; shift
>  done
>  
> +model=''
> +model=$(echo $OUTFILE | grep "gl-mv1000-emmc")
> +[ "$model" != "" ] && {
> +	ptgen_args="$ptgen_args -t 83 -p 7093504"

Please don't add device hacks here it's ment to be as generic
as posible. Instead, add possibility to override CONFIG_TARGET_ROOTFS_PARTSIZE
as parameter to "boot-img-ext4" command.

> +}
> +
>  head=16
>  sect=63
>  
> diff --git a/target/linux/mvebu/image/generic-arm64-emmc.bootscript b/target/linux/mvebu/image/generic-arm64-emmc.bootscript
> new file mode 100644
> index 0000000..4de4d39
> --- /dev/null
> +++ b/target/linux/mvebu/image/generic-arm64-emmc.bootscript
> @@ -0,0 +1,12 @@
> +setenv bootargs "root=/dev/mmcblk0p2 rw rootwait"
> +
> +if test -n "${console}"; then
> +	setenv bootargs "${bootargs} ${console}"
> +fi
> +
> +setenv mmcdev 0

Does the U-Boot only load "boot.scr" from first mmcdev
(I assume that's eMMC) or also searches for it on SD card? If
searched on both devices, there's no need to add this file, use
generic-arm64.bootscript. That way image will be applicable to
both storage mediums, just write this information in commit
message to inform users. If U-Boot searches for "boot.scr" only
on eMMC, then rename this file to gl-mv1000.bootscript and set
it in BOOT_SCRIPT variable.
 
> +
> +load mmc ${mmcdev}:1 ${fdt_addr} @DTB@.dtb
> +load mmc ${mmcdev}:1 ${kernel_addr} Image
> +
> +booti ${kernel_addr} - ${fdt_addr}
> 

Regards, Tomasz
Li Zhang April 14, 2020, 4:20 a.m. UTC | #2
Hi Tomasz,
Thank you very much for you correction.

> +};
> +
> +&spi0 {
> +        status = "okay";
> +
> +        flash@0 {
> +                reg = <0>;
> +                compatible = "jedec,spi-nor";
> +                spi-max-frequency = <104000000>;
> +                m25p,fast-read;
> +                partitions {
> +                        compatible = "fixed-partitions";
> +                        #address-cells = <1>;
> +                        #size-cells = <1>;
> +
> +                        partition@0 {
> +                                label = "u-boot";
> +                                reg = <0 0xf0000>;
> +                        };
> +
> +                        partition@f0000 {
> +                                label = "u-boot-env";
> +                                reg = <0Xf0000 0x8000>;
> +                        };
> +
> +                        art: partition@f8000 {
> +                                label = "art";

The name of this partition is rather strange, since the hardware
specification doesn't mention any PCIe or SDIO connected wireless
cards (don't know if there's any USB card without eeprom). Is
there any calibration data stored or only MAC address/addresses?
Does U-Boot also read information from this partition? Anyway
that's not a remark to change that name, I'm just curious.
--->MAC,SN,DDNS and ther private information are stored in the art.

On previous patch, there were additionnal partitions in dts
without -emmc suffix : dtb, firmware. If that space would be
unused it's better to add them here, so later You could
introduce images that could be written in SPI flash.
--->There is a short backup firmware in additionnal partitions of flash,normally it's not visible.In addition The offical firmware is stored in EMMC,So the previous file(gl-mv1000.dts) was deleted.

> +define Device/Default-arm64-emmc

Please don't add generic difinition for single device where only
image name and recipe are slightly changed, please remove it.

> +  BOOT_SCRIPT := generic-arm64-emmc
> +  DEVICE_DTS_DIR := $(DTS_DIR)/marvell
> +  IMAGES := emmc.img
> +  IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata
> +  KERNEL_NAME := Image
> +  KERNEL := kernel-bin
> +endef
> +
>  define Device/NAND-128K
>    BLOCKSIZE := 128k
>    PAGESIZE := 2048
> diff --git a/target/linux/mvebu/image/cortexa53.mk b/target/linux/mvebu/image/cortexa53.mk
> index 77f5c79..4dfd0b4 100644
> --- a/target/linux/mvebu/image/cortexa53.mk
> +++ b/target/linux/mvebu/image/cortexa53.mk
> @@ -69,3 +69,12 @@ define Device/methode_udpu
>    BOOT_SCRIPT := udpu
>  endef
>  TARGET_DEVICES += methode_udpu
> +
> +define Device/glinet_gl-mv1000-emmc
> +  $(call Device/Default-arm64-emmc)

Please call "Default-arm64" here. You can override image recipe
here adding (don't know if that'll be necessary):
--->1,It is used to distinguish between sd card and emmc.So It can let users better reconginze that it can be applied to EMMC rather than just sd card.
--->2,Currently,The uboot does not support 'firmware-gzip' upgrades .So it can not recover system via uboot, when the user's EMMC firmware fails to boot.
--->3,Could i create another file(gl-mv1000.mk) to include 'Default-arm64-emmc'?


Thank you!


Li Zhang | Software Engineer
li.zhang@gl-inet.com 
GL.iNet  WiFi for Things
Website: www.gl-inet.com   |   LinkedIn: gl-inet.com   |   Tel: +86-755-8660-6126
Room 305-306, Skyworth Digital Building , Shiyan Street, Baoan District, Shenzhen, China
Email Disclaimer: The content of this email is confidential and intended for the recipient specified in message only. It is strictly forbidden to share any part of this message with any third party, without a written consent of the sender. If you received this message by mistake, please reply to this message and follow with its deletion, so that we can ensure such a mistake does not occur in the future.
 
From: Tomasz Maciej Nowak
Date: 2020-04-14 04:14
To: Li Zhang; openwrt-devel
Subject: Re: [OpenWrt-Devel] [PATCH] mvebu: add support for GL.iNet GL-MV1000
Hi Li Zhang.
 
This version looks much better than the first one,
some comments inline.
 
W dniu 13.04.2020 o 12:26, Li Zhang pisze:
> This patch adds supports for GL-MV1000.
> 
> Specification:
> - SOC: Marvell Armada 88F3720 (1GHz)
> - Flash: 16MB (W25Q128FWSIG)
> - RAM: 1GB DDR4
> - Ethernet: 3x GE (1 WAN + 2 LAN)
> - EMMC: 8GB EMMC (KLM8G1GETF-B041)
> - MicroSD: 1x microSD slot
> - USB: 1x USB 2.0 port(TypeA),1x USB 3.0 port(TypeC)
> - Button: 1x reset button,1x slide switch
> - LED: 3x greed LED
> - UART: 1x UART on PCB (JP1: 3.3V, RX, TX, GND)
> 
> Signed-off-by: Li Zhang <li.zhang@gl-inet.com>
> ---
>  package/boot/uboot-envtools/files/mvebu            |  3 +
>  .../cortexa53/base-files/etc/board.d/02_network    |  3 +-
>  .../boot/dts/marvell/armada-gl-mv1000-emmc.dts     | 68 ++++++++++++++++++++++
>  target/linux/mvebu/image/Makefile                  |  9 +++
>  target/linux/mvebu/image/cortexa53.mk              |  9 +++
>  target/linux/mvebu/image/gen_mvebu_sdcard_img.sh   |  6 ++
>  .../mvebu/image/generic-arm64-emmc.bootscript      | 12 ++++
>  7 files changed, 109 insertions(+), 1 deletion(-)
>  create mode 100644 target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
>  create mode 100644 target/linux/mvebu/image/generic-arm64-emmc.bootscript
> 
> diff --git a/package/boot/uboot-envtools/files/mvebu b/package/boot/uboot-envtools/files/mvebu
> index 7902384..9d23c77 100644
> --- a/package/boot/uboot-envtools/files/mvebu
> +++ b/package/boot/uboot-envtools/files/mvebu
> @@ -24,6 +24,9 @@ globalscale,espressobin-v7-emmc|\
>  marvell,armada8040-mcbin)
>  ubootenv_add_uci_config "/dev/mtd0" "0x3f0000" "0x10000" "0x10000" "1"
>  ;;
> +glinet,gl-mv1000-emmc)
> + ubootenv_add_uci_config "/dev/mtd1" "0x0" "0x8000" "0x8000" "1"
> + ;;
>  linksys,caiman|\
>  linksys,cobra|\
>  linksys,shelby)
> diff --git a/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network b/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
> index ba4b930..e5ff667 100755
> --- a/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
> +++ b/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
> @@ -14,7 +14,8 @@ case "$board" in
>  globalscale,espressobin|\
>  globalscale,espressobin-emmc|\
>  globalscale,espressobin-v7|\
> -globalscale,espressobin-v7-emmc)
> +globalscale,espressobin-v7-emmc|\
> +glinet,gl-mv1000-emmc)
>  ucidef_set_interfaces_lan_wan "lan0 lan1" "wan"
>  ;;
>  marvell,armada-3720-db|\
> diff --git a/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts b/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
> new file mode 100644
> index 0000000..fe01dfe
> --- /dev/null
> +++ b/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
> @@ -0,0 +1,68 @@
 
Please add a license in SPDX format, a common one is dual MIT and
GPL-2.0+.
 
> +/*
> + * Device Tree file for GL.iNet GL-MV1000
> + */
> +
> +#include "armada-3720-espressobin.dts"
 
This is different device from ESPRESSObin altogether, please make it
stand-alone dts (copy espressobin dts, add required nodes and
remove/disable nodes for devices not present on the GL-MV1000).
Would be good to add all LEDs and buttons as nodes, which seem to be
GPIO controlled as in this picture:
https://static.gl-inet.com/docs/en/3/hardware/mv1000/mv1000.png
but that's not mandatory.
 
> +
> +/ {
> +       model = "GL.inet GL-MV1000 (Marvell)";
 
Instead of Marvell more apropriate would be Brume, since this is used
on Your website.
 
> +       compatible = "glinet,gl-mv1000-emmc";
 
Add here to compatible array in second place "marvell,armada3720".
Are there any boards in cutomers hands which do not have SD card slot
or eMMC soldered? There is no point in indicating the eMMC presence
when all sold boards are the same. In case of ESPRESSObin it's there
because various hardware versions. So please remove all -emmc suffixes
from files and file names across this patch if that's the case. One
exeption would be if U-Boot searches for exctly this name
(armada-gl-mv1000-emmc.dts) when booting, but looking at the usage of
boot.scr that's not the case.
 
> +};
> +
> +&spi0 {
> +        status = "okay";
> +
> +        flash@0 {
> +                reg = <0>;
> +                compatible = "jedec,spi-nor";
> +                spi-max-frequency = <104000000>;
> +                m25p,fast-read;
> +                partitions {
> +                        compatible = "fixed-partitions";
> +                        #address-cells = <1>;
> +                        #size-cells = <1>;
> +
> +                        partition@0 {
> +                                label = "u-boot";
> +                                reg = <0 0xf0000>;
> +                        };
> +
> +                        partition@f0000 {
> +                                label = "u-boot-env";
> +                                reg = <0Xf0000 0x8000>;
> +                        };
> +
> +                        art: partition@f8000 {
> +                                label = "art";
 
The name of this partition is rather strange, since the hardware
specification doesn't mention any PCIe or SDIO connected wireless
cards (don't know if there's any USB card without eeprom). Is
there any calibration data stored or only MAC address/addresses?
Does U-Boot also read information from this partition? Anyway
that's not a remark to change that name, I'm just curious.
 
> +                                reg = <0xf8000 0x8000>;
> +                        };
 
On previous patch, there were additionnal partitions in dts
without -emmc suffix : dtb, firmware. If that space would be
unused it's better to add them here, so later You could
introduce images that could be written in SPI flash.
 
> +
> +               };
> +        };
> +};
> +
> +&sdhci1 {
> +        wp-inverted;
> +        bus-width = <4>;
> +        cd-gpios = <&gpionb 17 GPIO_ACTIVE_LOW>;
> +        marvell,pad-type = "sd";
> +        no-1-8-v;
> +        vqmmc-supply = <&vcc_sd_reg1>;
> +        status = "okay";
> +};
> +
> +
> +&sdhci0 {
> +        bus-width = <8>;
> +        mmc-ddr-1_8v;
> +        mmc-hs400-1_8v;
> +        non-removable;
> +        no-sd;
> +        no-sdio;
> +        marvell,pad-type = "fixed-1-8v";
> +        status = "okay";
> +};
> +
> +&eth0 {
> + mtd-mac-address = <&art 0x0>;
> +};
> diff --git a/target/linux/mvebu/image/Makefile b/target/linux/mvebu/image/Makefile
> index ef92748..b848049 100644
> --- a/target/linux/mvebu/image/Makefile
> +++ b/target/linux/mvebu/image/Makefile
> @@ -107,6 +107,15 @@ define Device/Default-arm64
>    KERNEL := kernel-bin
>  endef
>  
> +define Device/Default-arm64-emmc
 
Please don't add generic difinition for single device where only
image name and recipe are slightly changed, please remove it.
 
> +  BOOT_SCRIPT := generic-arm64-emmc
> +  DEVICE_DTS_DIR := $(DTS_DIR)/marvell
> +  IMAGES := emmc.img
> +  IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata
> +  KERNEL_NAME := Image
> +  KERNEL := kernel-bin
> +endef
> +
>  define Device/NAND-128K
>    BLOCKSIZE := 128k
>    PAGESIZE := 2048
> diff --git a/target/linux/mvebu/image/cortexa53.mk b/target/linux/mvebu/image/cortexa53.mk
> index 77f5c79..4dfd0b4 100644
> --- a/target/linux/mvebu/image/cortexa53.mk
> +++ b/target/linux/mvebu/image/cortexa53.mk
> @@ -69,3 +69,12 @@ define Device/methode_udpu
>    BOOT_SCRIPT := udpu
>  endef
>  TARGET_DEVICES += methode_udpu
> +
> +define Device/glinet_gl-mv1000-emmc
> +  $(call Device/Default-arm64-emmc)
 
Please call "Default-arm64" here. You can override image recipe
here adding (don't know if that'll be necessary):
BOOT_SCRIPT := generic-arm64-emmc
IMAGES := emmc.img
IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata
 
> +  DEVICE_TITLE := GL.iNet GL-MV1000 EMMC
> +  DEVICE_DTS := armada-gl-mv1000-emmc
> +  SUPPORTED_DEVICES := glinet,gl-mv1000-emmc
> +endef
> +TARGET_DEVICES += glinet_gl-mv1000-emmc
> +
> diff --git a/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh b/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
> index 842e591..50028fe 100755
> --- a/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
> +++ b/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
> @@ -51,6 +51,12 @@ while [ "$#" -ge 3 ]; do
>  shift; shift; shift
>  done
>  
> +model=''
> +model=$(echo $OUTFILE | grep "gl-mv1000-emmc")
> +[ "$model" != "" ] && {
> + ptgen_args="$ptgen_args -t 83 -p 7093504"
 
Please don't add device hacks here it's ment to be as generic
as posible. Instead, add possibility to override CONFIG_TARGET_ROOTFS_PARTSIZE
as parameter to "boot-img-ext4" command.
 
> +}
> +
>  head=16
>  sect=63
>  
> diff --git a/target/linux/mvebu/image/generic-arm64-emmc.bootscript b/target/linux/mvebu/image/generic-arm64-emmc.bootscript
> new file mode 100644
> index 0000000..4de4d39
> --- /dev/null
> +++ b/target/linux/mvebu/image/generic-arm64-emmc.bootscript
> @@ -0,0 +1,12 @@
> +setenv bootargs "root=/dev/mmcblk0p2 rw rootwait"
> +
> +if test -n "${console}"; then
> + setenv bootargs "${bootargs} ${console}"
> +fi
> +
> +setenv mmcdev 0
 
Does the U-Boot only load "boot.scr" from first mmcdev
(I assume that's eMMC) or also searches for it on SD card? If
searched on both devices, there's no need to add this file, use
generic-arm64.bootscript. That way image will be applicable to
both storage mediums, just write this information in commit
message to inform users. If U-Boot searches for "boot.scr" only
on eMMC, then rename this file to gl-mv1000.bootscript and set
it in BOOT_SCRIPT variable.
> +
> +load mmc ${mmcdev}:1 ${fdt_addr} @DTB@.dtb
> +load mmc ${mmcdev}:1 ${kernel_addr} Image
> +
> +booti ${kernel_addr} - ${fdt_addr}
> 
 
Regards, Tomasz
Tomasz Maciej Nowak April 14, 2020, 6:53 p.m. UTC | #3
W dniu 14.04.2020 o 06:20, Li.zhang pisze:
> Hi Tomasz,
> Thank you very much for you correction.
> 
>> +};
>> +
>> +&spi0 {
>> +        status = "okay";
>> +
>> +        flash@0 {
>> +                reg = <0>;
>> +                compatible = "jedec,spi-nor";
>> +                spi-max-frequency = <104000000>;
>> +                m25p,fast-read;
>> +                partitions {
>> +                        compatible = "fixed-partitions";
>> +                        #address-cells = <1>;
>> +                        #size-cells = <1>;
>> +
>> +                        partition@0 {
>> +                                label = "u-boot";
>> +                                reg = <0 0xf0000>;
>> +                        };
>> +
>> +                        partition@f0000 {
>> +                                label = "u-boot-env";
>> +                                reg = <0Xf0000 0x8000>;
>> +                        };
>> +
>> +                        art: partition@f8000 {
>> +                                label = "art";
> 
> The name of this partition is rather strange, since the hardware
> specification doesn't mention any PCIe or SDIO connected wireless
> cards (don't know if there's any USB card without eeprom). Is
> there any calibration data stored or only MAC address/addresses?
> Does U-Boot also read information from this partition? Anyway
> that's not a remark to change that name, I'm just curious.
> --->MAC,SN,DDNS and ther private information are stored in the art.

Ok, still that name is weird, but that's matter of taste.

> 
> On previous patch, there were additionnal partitions in dts
> without -emmc suffix : dtb, firmware. If that space would be
> unused it's better to add them here, so later You could
> introduce images that could be written in SPI flash.
> --->There is a short backup firmware in additionnal partitions of flash,normally it's not visible.In addition The offical firmware is stored in EMMC,So the previous file(gl-mv1000.dts) was deleted.

Then it would be better to specify those partitions, as some users could simply
overwrite this thinking that's unoccupied space.

> 
>> +define Device/Default-arm64-emmc
> 
> Please don't add generic difinition for single device where only
> image name and recipe are slightly changed, please remove it.
> 
>> +  BOOT_SCRIPT := generic-arm64-emmc
>> +  DEVICE_DTS_DIR := $(DTS_DIR)/marvell
>> +  IMAGES := emmc.img
>> +  IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata
>> +  KERNEL_NAME := Image
>> +  KERNEL := kernel-bin
>> +endef
>> +
>>  define Device/NAND-128K
>>    BLOCKSIZE := 128k
>>    PAGESIZE := 2048
>> diff --git a/target/linux/mvebu/image/cortexa53.mk b/target/linux/mvebu/image/cortexa53.mk
>> index 77f5c79..4dfd0b4 100644
>> --- a/target/linux/mvebu/image/cortexa53.mk
>> +++ b/target/linux/mvebu/image/cortexa53.mk
>> @@ -69,3 +69,12 @@ define Device/methode_udpu
>>    BOOT_SCRIPT := udpu
>>  endef
>>  TARGET_DEVICES += methode_udpu
>> +
>> +define Device/glinet_gl-mv1000-emmc
>> +  $(call Device/Default-arm64-emmc)
> 
> Please call "Default-arm64" here. You can override image recipe
> here adding (don't know if that'll be necessary):
> --->1,It is used to distinguish between sd card and emmc.So It can let users better reconginze that it can be applied to EMMC rather than just sd card.

But with this patch, the image has in name "emmc" only, how would users know that
they can use it for SD card? If both images are interchangable You have to extend
commit message with instuctions, that single image can be used on both mediums
and how to use them (the SD card is rather obvious, but more explanation is
needed for the eMMC).
Crucial question is left unanswered: Does the U-Boot by default boot OpenWrt from
eMMC only or eMMC and SD card? And if boot from both, which is booted first?

> --->2,Currently,The uboot does not support 'firmware-gzip' upgrades .So it can not recover system via uboot, when the user's EMMC firmware fails to boot.

Ok.

> --->3,Could i create another file(gl-mv1000.mk) to include 'Default-arm64-emmc'?

No, please don't. You can always override IMAGES variable and add multiple IMAGE recipes in device
definition, there is no reason to add a common stub used ONLY by one device.

> 
> 
> Thank you!
> 
> 
> Li Zhang | Software Engineer
> li.zhang@gl-inet.com 
> GL.iNet  WiFi for Things
> Website: www.gl-inet.com   |   LinkedIn: gl-inet.com   |   Tel: +86-755-8660-6126
> Room 305-306, Skyworth Digital Building , Shiyan Street, Baoan District, Shenzhen, China
> Email Disclaimer: The content of this email is confidential and intended for the recipient specified in message only. It is strictly forbidden to share any part of this message with any third party, without a written consent of the sender. If you received this message by mistake, please reply to this message and follow with its deletion, so that we can ensure such a mistake does not occur in the future.
>  
> From: Tomasz Maciej Nowak
> Date: 2020-04-14 04:14
> To: Li Zhang; openwrt-devel
> Subject: Re: [OpenWrt-Devel] [PATCH] mvebu: add support for GL.iNet GL-MV1000
> Hi Li Zhang.
>  
> This version looks much better than the first one,
> some comments inline.
>  
> W dniu 13.04.2020 o 12:26, Li Zhang pisze:
>> This patch adds supports for GL-MV1000.
>>
>> Specification:
>> - SOC: Marvell Armada 88F3720 (1GHz)
>> - Flash: 16MB (W25Q128FWSIG)
>> - RAM: 1GB DDR4
>> - Ethernet: 3x GE (1 WAN + 2 LAN)
>> - EMMC: 8GB EMMC (KLM8G1GETF-B041)
>> - MicroSD: 1x microSD slot
>> - USB: 1x USB 2.0 port(TypeA),1x USB 3.0 port(TypeC)
>> - Button: 1x reset button,1x slide switch
>> - LED: 3x greed LED
>> - UART: 1x UART on PCB (JP1: 3.3V, RX, TX, GND)
>>
>> Signed-off-by: Li Zhang <li.zhang@gl-inet.com>
>> ---
>>  package/boot/uboot-envtools/files/mvebu            |  3 +
>>  .../cortexa53/base-files/etc/board.d/02_network    |  3 +-
>>  .../boot/dts/marvell/armada-gl-mv1000-emmc.dts     | 68 ++++++++++++++++++++++
>>  target/linux/mvebu/image/Makefile                  |  9 +++
>>  target/linux/mvebu/image/cortexa53.mk              |  9 +++
>>  target/linux/mvebu/image/gen_mvebu_sdcard_img.sh   |  6 ++
>>  .../mvebu/image/generic-arm64-emmc.bootscript      | 12 ++++
>>  7 files changed, 109 insertions(+), 1 deletion(-)
>>  create mode 100644 target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
>>  create mode 100644 target/linux/mvebu/image/generic-arm64-emmc.bootscript
>>
>> diff --git a/package/boot/uboot-envtools/files/mvebu b/package/boot/uboot-envtools/files/mvebu
>> index 7902384..9d23c77 100644
>> --- a/package/boot/uboot-envtools/files/mvebu
>> +++ b/package/boot/uboot-envtools/files/mvebu
>> @@ -24,6 +24,9 @@ globalscale,espressobin-v7-emmc|\
>>  marvell,armada8040-mcbin)
>>  ubootenv_add_uci_config "/dev/mtd0" "0x3f0000" "0x10000" "0x10000" "1"
>>  ;;
>> +glinet,gl-mv1000-emmc)
>> + ubootenv_add_uci_config "/dev/mtd1" "0x0" "0x8000" "0x8000" "1"
>> + ;;
>>  linksys,caiman|\
>>  linksys,cobra|\
>>  linksys,shelby)
>> diff --git a/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network b/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
>> index ba4b930..e5ff667 100755
>> --- a/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
>> +++ b/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
>> @@ -14,7 +14,8 @@ case "$board" in
>>  globalscale,espressobin|\
>>  globalscale,espressobin-emmc|\
>>  globalscale,espressobin-v7|\
>> -globalscale,espressobin-v7-emmc)
>> +globalscale,espressobin-v7-emmc|\
>> +glinet,gl-mv1000-emmc)
>>  ucidef_set_interfaces_lan_wan "lan0 lan1" "wan"
>>  ;;
>>  marvell,armada-3720-db|\
>> diff --git a/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts b/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
>> new file mode 100644
>> index 0000000..fe01dfe
>> --- /dev/null
>> +++ b/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
>> @@ -0,0 +1,68 @@
>  
> Please add a license in SPDX format, a common one is dual MIT and
> GPL-2.0+.
>  
>> +/*
>> + * Device Tree file for GL.iNet GL-MV1000
>> + */
>> +
>> +#include "armada-3720-espressobin.dts"
>  
> This is different device from ESPRESSObin altogether, please make it
> stand-alone dts (copy espressobin dts, add required nodes and
> remove/disable nodes for devices not present on the GL-MV1000).
> Would be good to add all LEDs and buttons as nodes, which seem to be
> GPIO controlled as in this picture:
> https://static.gl-inet.com/docs/en/3/hardware/mv1000/mv1000.png
> but that's not mandatory.
>  
>> +
>> +/ {
>> +       model = "GL.inet GL-MV1000 (Marvell)";
>  
> Instead of Marvell more apropriate would be Brume, since this is used
> on Your website.
>  
>> +       compatible = "glinet,gl-mv1000-emmc";
>  
> Add here to compatible array in second place "marvell,armada3720".
> Are there any boards in cutomers hands which do not have SD card slot
> or eMMC soldered? There is no point in indicating the eMMC presence
> when all sold boards are the same. In case of ESPRESSObin it's there
> because various hardware versions. So please remove all -emmc suffixes
> from files and file names across this patch if that's the case. One
> exeption would be if U-Boot searches for exctly this name
> (armada-gl-mv1000-emmc.dts) when booting, but looking at the usage of
> boot.scr that's not the case.
>  
>> +};
>> +
>> +&spi0 {
>> +        status = "okay";
>> +
>> +        flash@0 {
>> +                reg = <0>;
>> +                compatible = "jedec,spi-nor";
>> +                spi-max-frequency = <104000000>;
>> +                m25p,fast-read;
>> +                partitions {
>> +                        compatible = "fixed-partitions";
>> +                        #address-cells = <1>;
>> +                        #size-cells = <1>;
>> +
>> +                        partition@0 {
>> +                                label = "u-boot";
>> +                                reg = <0 0xf0000>;
>> +                        };
>> +
>> +                        partition@f0000 {
>> +                                label = "u-boot-env";
>> +                                reg = <0Xf0000 0x8000>;
>> +                        };
>> +
>> +                        art: partition@f8000 {
>> +                                label = "art";
>  
> The name of this partition is rather strange, since the hardware
> specification doesn't mention any PCIe or SDIO connected wireless
> cards (don't know if there's any USB card without eeprom). Is
> there any calibration data stored or only MAC address/addresses?
> Does U-Boot also read information from this partition? Anyway
> that's not a remark to change that name, I'm just curious.
>  
>> +                                reg = <0xf8000 0x8000>;
>> +                        };
>  
> On previous patch, there were additionnal partitions in dts
> without -emmc suffix : dtb, firmware. If that space would be
> unused it's better to add them here, so later You could
> introduce images that could be written in SPI flash.
>  
>> +
>> +               };
>> +        };
>> +};
>> +
>> +&sdhci1 {
>> +        wp-inverted;
>> +        bus-width = <4>;
>> +        cd-gpios = <&gpionb 17 GPIO_ACTIVE_LOW>;
>> +        marvell,pad-type = "sd";
>> +        no-1-8-v;
>> +        vqmmc-supply = <&vcc_sd_reg1>;
>> +        status = "okay";
>> +};
>> +
>> +
>> +&sdhci0 {
>> +        bus-width = <8>;
>> +        mmc-ddr-1_8v;
>> +        mmc-hs400-1_8v;
>> +        non-removable;
>> +        no-sd;
>> +        no-sdio;
>> +        marvell,pad-type = "fixed-1-8v";
>> +        status = "okay";
>> +};
>> +
>> +&eth0 {
>> + mtd-mac-address = <&art 0x0>;
>> +};
>> diff --git a/target/linux/mvebu/image/Makefile b/target/linux/mvebu/image/Makefile
>> index ef92748..b848049 100644
>> --- a/target/linux/mvebu/image/Makefile
>> +++ b/target/linux/mvebu/image/Makefile
>> @@ -107,6 +107,15 @@ define Device/Default-arm64
>>    KERNEL := kernel-bin
>>  endef
>>  
>> +define Device/Default-arm64-emmc
>  
> Please don't add generic difinition for single device where only
> image name and recipe are slightly changed, please remove it.
>  
>> +  BOOT_SCRIPT := generic-arm64-emmc
>> +  DEVICE_DTS_DIR := $(DTS_DIR)/marvell
>> +  IMAGES := emmc.img
>> +  IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata
>> +  KERNEL_NAME := Image
>> +  KERNEL := kernel-bin
>> +endef
>> +
>>  define Device/NAND-128K
>>    BLOCKSIZE := 128k
>>    PAGESIZE := 2048
>> diff --git a/target/linux/mvebu/image/cortexa53.mk b/target/linux/mvebu/image/cortexa53.mk
>> index 77f5c79..4dfd0b4 100644
>> --- a/target/linux/mvebu/image/cortexa53.mk
>> +++ b/target/linux/mvebu/image/cortexa53.mk
>> @@ -69,3 +69,12 @@ define Device/methode_udpu
>>    BOOT_SCRIPT := udpu
>>  endef
>>  TARGET_DEVICES += methode_udpu
>> +
>> +define Device/glinet_gl-mv1000-emmc
>> +  $(call Device/Default-arm64-emmc)
>  
> Please call "Default-arm64" here. You can override image recipe
> here adding (don't know if that'll be necessary):
> BOOT_SCRIPT := generic-arm64-emmc
> IMAGES := emmc.img
> IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata
>  
>> +  DEVICE_TITLE := GL.iNet GL-MV1000 EMMC
>> +  DEVICE_DTS := armada-gl-mv1000-emmc
>> +  SUPPORTED_DEVICES := glinet,gl-mv1000-emmc
>> +endef
>> +TARGET_DEVICES += glinet_gl-mv1000-emmc
>> +
>> diff --git a/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh b/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
>> index 842e591..50028fe 100755
>> --- a/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
>> +++ b/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
>> @@ -51,6 +51,12 @@ while [ "$#" -ge 3 ]; do
>>  shift; shift; shift
>>  done
>>  
>> +model=''
>> +model=$(echo $OUTFILE | grep "gl-mv1000-emmc")
>> +[ "$model" != "" ] && {
>> + ptgen_args="$ptgen_args -t 83 -p 7093504"
>  
> Please don't add device hacks here it's ment to be as generic
> as posible. Instead, add possibility to override CONFIG_TARGET_ROOTFS_PARTSIZE
> as parameter to "boot-img-ext4" command.
>  
>> +}
>> +
>>  head=16
>>  sect=63
>>  
>> diff --git a/target/linux/mvebu/image/generic-arm64-emmc.bootscript b/target/linux/mvebu/image/generic-arm64-emmc.bootscript
>> new file mode 100644
>> index 0000000..4de4d39
>> --- /dev/null
>> +++ b/target/linux/mvebu/image/generic-arm64-emmc.bootscript
>> @@ -0,0 +1,12 @@
>> +setenv bootargs "root=/dev/mmcblk0p2 rw rootwait"
>> +
>> +if test -n "${console}"; then
>> + setenv bootargs "${bootargs} ${console}"
>> +fi
>> +
>> +setenv mmcdev 0
>  
> Does the U-Boot only load "boot.scr" from first mmcdev
> (I assume that's eMMC) or also searches for it on SD card? If
> searched on both devices, there's no need to add this file, use
> generic-arm64.bootscript. That way image will be applicable to
> both storage mediums, just write this information in commit
> message to inform users. If U-Boot searches for "boot.scr" only
> on eMMC, then rename this file to gl-mv1000.bootscript and set
> it in BOOT_SCRIPT variable.
>> +
>> +load mmc ${mmcdev}:1 ${fdt_addr} @DTB@.dtb
>> +load mmc ${mmcdev}:1 ${kernel_addr} Image
>> +
>> +booti ${kernel_addr} - ${fdt_addr}
>>
>  
> Regards, Tomasz
>
Li Zhang April 15, 2020, 8:44 a.m. UTC | #4
Hi Tomasz,
Crucial question is left unanswered: Does the U-Boot by default boot OpenWrt from
eMMC only or eMMC and SD card? And if boot from both, which is booted first?
---> Boot openwrt from EMMC by default. If boot from both,Emmc is booted first.

Regards,Li


Li Zhang | Software Engineer
li.zhang@gl-inet.com 
GL.iNet  WiFi for Things
Website: www.gl-inet.com   |   LinkedIn: gl-inet.com   |   Tel: +86-755-8660-6126
Room 305-306, Skyworth Digital Building , Shiyan Street, Baoan District, Shenzhen, China
Email Disclaimer: The content of this email is confidential and intended for the recipient specified in message only. It is strictly forbidden to share any part of this message with any third party, without a written consent of the sender. If you received this message by mistake, please reply to this message and follow with its deletion, so that we can ensure such a mistake does not occur in the future.
 
From: Tomasz Maciej Nowak
Date: 2020-04-15 02:53
To: Li.zhang; openwrt-devel
Subject: Re: [OpenWrt-Devel] [PATCH] mvebu: add support for GL.iNet GL-MV1000
W dniu 14.04.2020 o 06:20, Li.zhang pisze:
> Hi Tomasz,
> Thank you very much for you correction.
> 
>> +};
>> +
>> +&spi0 {
>> +        status = "okay";
>> +
>> +        flash@0 {
>> +                reg = <0>;
>> +                compatible = "jedec,spi-nor";
>> +                spi-max-frequency = <104000000>;
>> +                m25p,fast-read;
>> +                partitions {
>> +                        compatible = "fixed-partitions";
>> +                        #address-cells = <1>;
>> +                        #size-cells = <1>;
>> +
>> +                        partition@0 {
>> +                                label = "u-boot";
>> +                                reg = <0 0xf0000>;
>> +                        };
>> +
>> +                        partition@f0000 {
>> +                                label = "u-boot-env";
>> +                                reg = <0Xf0000 0x8000>;
>> +                        };
>> +
>> +                        art: partition@f8000 {
>> +                                label = "art";
> 
> The name of this partition is rather strange, since the hardware
> specification doesn't mention any PCIe or SDIO connected wireless
> cards (don't know if there's any USB card without eeprom). Is
> there any calibration data stored or only MAC address/addresses?
> Does U-Boot also read information from this partition? Anyway
> that's not a remark to change that name, I'm just curious.
> --->MAC,SN,DDNS and ther private information are stored in the art.
 
Ok, still that name is weird, but that's matter of taste.
 
> 
> On previous patch, there were additionnal partitions in dts
> without -emmc suffix : dtb, firmware. If that space would be
> unused it's better to add them here, so later You could
> introduce images that could be written in SPI flash.
> --->There is a short backup firmware in additionnal partitions of flash,normally it's not visible.In addition The offical firmware is stored in EMMC,So the previous file(gl-mv1000.dts) was deleted.
 
Then it would be better to specify those partitions, as some users could simply
overwrite this thinking that's unoccupied space.
 
> 
>> +define Device/Default-arm64-emmc
> 
> Please don't add generic difinition for single device where only
> image name and recipe are slightly changed, please remove it.
> 
>> +  BOOT_SCRIPT := generic-arm64-emmc
>> +  DEVICE_DTS_DIR := $(DTS_DIR)/marvell
>> +  IMAGES := emmc.img
>> +  IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata
>> +  KERNEL_NAME := Image
>> +  KERNEL := kernel-bin
>> +endef
>> +
>>  define Device/NAND-128K
>>    BLOCKSIZE := 128k
>>    PAGESIZE := 2048
>> diff --git a/target/linux/mvebu/image/cortexa53.mk b/target/linux/mvebu/image/cortexa53.mk
>> index 77f5c79..4dfd0b4 100644
>> --- a/target/linux/mvebu/image/cortexa53.mk
>> +++ b/target/linux/mvebu/image/cortexa53.mk
>> @@ -69,3 +69,12 @@ define Device/methode_udpu
>>    BOOT_SCRIPT := udpu
>>  endef
>>  TARGET_DEVICES += methode_udpu
>> +
>> +define Device/glinet_gl-mv1000-emmc
>> +  $(call Device/Default-arm64-emmc)
> 
> Please call "Default-arm64" here. You can override image recipe
> here adding (don't know if that'll be necessary):
> --->1,It is used to distinguish between sd card and emmc.So It can let users better reconginze that it can be applied to EMMC rather than just sd card.
 
But with this patch, the image has in name "emmc" only, how would users know that
they can use it for SD card? If both images are interchangable You have to extend
commit message with instuctions, that single image can be used on both mediums
and how to use them (the SD card is rather obvious, but more explanation is
needed for the eMMC).
Crucial question is left unanswered: Does the U-Boot by default boot OpenWrt from
eMMC only or eMMC and SD card? And if boot from both, which is booted first?
 
> --->2,Currently,The uboot does not support 'firmware-gzip' upgrades .So it can not recover system via uboot, when the user's EMMC firmware fails to boot.
 
Ok.
 
> --->3,Could i create another file(gl-mv1000.mk) to include 'Default-arm64-emmc'?
 
No, please don't. You can always override IMAGES variable and add multiple IMAGE recipes in device
definition, there is no reason to add a common stub used ONLY by one device.
 
> 
> 
> Thank you!
> 
> 
> Li Zhang | Software Engineer
> li.zhang@gl-inet.com 
> GL.iNet  WiFi for Things
> Website: www.gl-inet.com   |   LinkedIn: gl-inet.com   |   Tel: +86-755-8660-6126
> Room 305-306, Skyworth Digital Building , Shiyan Street, Baoan District, Shenzhen, China
> Email Disclaimer: The content of this email is confidential and intended for the recipient specified in message only. It is strictly forbidden to share any part of this message with any third party, without a written consent of the sender. If you received this message by mistake, please reply to this message and follow with its deletion, so that we can ensure such a mistake does not occur in the future.
>  
> From: Tomasz Maciej Nowak
> Date: 2020-04-14 04:14
> To: Li Zhang; openwrt-devel
> Subject: Re: [OpenWrt-Devel] [PATCH] mvebu: add support for GL.iNet GL-MV1000
> Hi Li Zhang.
>  
> This version looks much better than the first one,
> some comments inline.
>  
> W dniu 13.04.2020 o 12:26, Li Zhang pisze:
>> This patch adds supports for GL-MV1000.
>>
>> Specification:
>> - SOC: Marvell Armada 88F3720 (1GHz)
>> - Flash: 16MB (W25Q128FWSIG)
>> - RAM: 1GB DDR4
>> - Ethernet: 3x GE (1 WAN + 2 LAN)
>> - EMMC: 8GB EMMC (KLM8G1GETF-B041)
>> - MicroSD: 1x microSD slot
>> - USB: 1x USB 2.0 port(TypeA),1x USB 3.0 port(TypeC)
>> - Button: 1x reset button,1x slide switch
>> - LED: 3x greed LED
>> - UART: 1x UART on PCB (JP1: 3.3V, RX, TX, GND)
>>
>> Signed-off-by: Li Zhang <li.zhang@gl-inet.com>
>> ---
>>  package/boot/uboot-envtools/files/mvebu            |  3 +
>>  .../cortexa53/base-files/etc/board.d/02_network    |  3 +-
>>  .../boot/dts/marvell/armada-gl-mv1000-emmc.dts     | 68 ++++++++++++++++++++++
>>  target/linux/mvebu/image/Makefile                  |  9 +++
>>  target/linux/mvebu/image/cortexa53.mk              |  9 +++
>>  target/linux/mvebu/image/gen_mvebu_sdcard_img.sh   |  6 ++
>>  .../mvebu/image/generic-arm64-emmc.bootscript      | 12 ++++
>>  7 files changed, 109 insertions(+), 1 deletion(-)
>>  create mode 100644 target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
>>  create mode 100644 target/linux/mvebu/image/generic-arm64-emmc.bootscript
>>
>> diff --git a/package/boot/uboot-envtools/files/mvebu b/package/boot/uboot-envtools/files/mvebu
>> index 7902384..9d23c77 100644
>> --- a/package/boot/uboot-envtools/files/mvebu
>> +++ b/package/boot/uboot-envtools/files/mvebu
>> @@ -24,6 +24,9 @@ globalscale,espressobin-v7-emmc|\
>>  marvell,armada8040-mcbin)
>>  ubootenv_add_uci_config "/dev/mtd0" "0x3f0000" "0x10000" "0x10000" "1"
>>  ;;
>> +glinet,gl-mv1000-emmc)
>> + ubootenv_add_uci_config "/dev/mtd1" "0x0" "0x8000" "0x8000" "1"
>> + ;;
>>  linksys,caiman|\
>>  linksys,cobra|\
>>  linksys,shelby)
>> diff --git a/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network b/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
>> index ba4b930..e5ff667 100755
>> --- a/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
>> +++ b/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
>> @@ -14,7 +14,8 @@ case "$board" in
>>  globalscale,espressobin|\
>>  globalscale,espressobin-emmc|\
>>  globalscale,espressobin-v7|\
>> -globalscale,espressobin-v7-emmc)
>> +globalscale,espressobin-v7-emmc|\
>> +glinet,gl-mv1000-emmc)
>>  ucidef_set_interfaces_lan_wan "lan0 lan1" "wan"
>>  ;;
>>  marvell,armada-3720-db|\
>> diff --git a/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts b/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
>> new file mode 100644
>> index 0000000..fe01dfe
>> --- /dev/null
>> +++ b/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
>> @@ -0,0 +1,68 @@
>  
> Please add a license in SPDX format, a common one is dual MIT and
> GPL-2.0+.
>  
>> +/*
>> + * Device Tree file for GL.iNet GL-MV1000
>> + */
>> +
>> +#include "armada-3720-espressobin.dts"
>  
> This is different device from ESPRESSObin altogether, please make it
> stand-alone dts (copy espressobin dts, add required nodes and
> remove/disable nodes for devices not present on the GL-MV1000).
> Would be good to add all LEDs and buttons as nodes, which seem to be
> GPIO controlled as in this picture:
> https://static.gl-inet.com/docs/en/3/hardware/mv1000/mv1000.png
> but that's not mandatory.
>  
>> +
>> +/ {
>> +       model = "GL.inet GL-MV1000 (Marvell)";
>  
> Instead of Marvell more apropriate would be Brume, since this is used
> on Your website.
>  
>> +       compatible = "glinet,gl-mv1000-emmc";
>  
> Add here to compatible array in second place "marvell,armada3720".
> Are there any boards in cutomers hands which do not have SD card slot
> or eMMC soldered? There is no point in indicating the eMMC presence
> when all sold boards are the same. In case of ESPRESSObin it's there
> because various hardware versions. So please remove all -emmc suffixes
> from files and file names across this patch if that's the case. One
> exeption would be if U-Boot searches for exctly this name
> (armada-gl-mv1000-emmc.dts) when booting, but looking at the usage of
> boot.scr that's not the case.
>  
>> +};
>> +
>> +&spi0 {
>> +        status = "okay";
>> +
>> +        flash@0 {
>> +                reg = <0>;
>> +                compatible = "jedec,spi-nor";
>> +                spi-max-frequency = <104000000>;
>> +                m25p,fast-read;
>> +                partitions {
>> +                        compatible = "fixed-partitions";
>> +                        #address-cells = <1>;
>> +                        #size-cells = <1>;
>> +
>> +                        partition@0 {
>> +                                label = "u-boot";
>> +                                reg = <0 0xf0000>;
>> +                        };
>> +
>> +                        partition@f0000 {
>> +                                label = "u-boot-env";
>> +                                reg = <0Xf0000 0x8000>;
>> +                        };
>> +
>> +                        art: partition@f8000 {
>> +                                label = "art";
>  
> The name of this partition is rather strange, since the hardware
> specification doesn't mention any PCIe or SDIO connected wireless
> cards (don't know if there's any USB card without eeprom). Is
> there any calibration data stored or only MAC address/addresses?
> Does U-Boot also read information from this partition? Anyway
> that's not a remark to change that name, I'm just curious.
>  
>> +                                reg = <0xf8000 0x8000>;
>> +                        };
>  
> On previous patch, there were additionnal partitions in dts
> without -emmc suffix : dtb, firmware. If that space would be
> unused it's better to add them here, so later You could
> introduce images that could be written in SPI flash.
>  
>> +
>> +               };
>> +        };
>> +};
>> +
>> +&sdhci1 {
>> +        wp-inverted;
>> +        bus-width = <4>;
>> +        cd-gpios = <&gpionb 17 GPIO_ACTIVE_LOW>;
>> +        marvell,pad-type = "sd";
>> +        no-1-8-v;
>> +        vqmmc-supply = <&vcc_sd_reg1>;
>> +        status = "okay";
>> +};
>> +
>> +
>> +&sdhci0 {
>> +        bus-width = <8>;
>> +        mmc-ddr-1_8v;
>> +        mmc-hs400-1_8v;
>> +        non-removable;
>> +        no-sd;
>> +        no-sdio;
>> +        marvell,pad-type = "fixed-1-8v";
>> +        status = "okay";
>> +};
>> +
>> +&eth0 {
>> + mtd-mac-address = <&art 0x0>;
>> +};
>> diff --git a/target/linux/mvebu/image/Makefile b/target/linux/mvebu/image/Makefile
>> index ef92748..b848049 100644
>> --- a/target/linux/mvebu/image/Makefile
>> +++ b/target/linux/mvebu/image/Makefile
>> @@ -107,6 +107,15 @@ define Device/Default-arm64
>>    KERNEL := kernel-bin
>>  endef
>>  
>> +define Device/Default-arm64-emmc
>  
> Please don't add generic difinition for single device where only
> image name and recipe are slightly changed, please remove it.
>  
>> +  BOOT_SCRIPT := generic-arm64-emmc
>> +  DEVICE_DTS_DIR := $(DTS_DIR)/marvell
>> +  IMAGES := emmc.img
>> +  IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata
>> +  KERNEL_NAME := Image
>> +  KERNEL := kernel-bin
>> +endef
>> +
>>  define Device/NAND-128K
>>    BLOCKSIZE := 128k
>>    PAGESIZE := 2048
>> diff --git a/target/linux/mvebu/image/cortexa53.mk b/target/linux/mvebu/image/cortexa53.mk
>> index 77f5c79..4dfd0b4 100644
>> --- a/target/linux/mvebu/image/cortexa53.mk
>> +++ b/target/linux/mvebu/image/cortexa53.mk
>> @@ -69,3 +69,12 @@ define Device/methode_udpu
>>    BOOT_SCRIPT := udpu
>>  endef
>>  TARGET_DEVICES += methode_udpu
>> +
>> +define Device/glinet_gl-mv1000-emmc
>> +  $(call Device/Default-arm64-emmc)
>  
> Please call "Default-arm64" here. You can override image recipe
> here adding (don't know if that'll be necessary):
> BOOT_SCRIPT := generic-arm64-emmc
> IMAGES := emmc.img
> IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata
>  
>> +  DEVICE_TITLE := GL.iNet GL-MV1000 EMMC
>> +  DEVICE_DTS := armada-gl-mv1000-emmc
>> +  SUPPORTED_DEVICES := glinet,gl-mv1000-emmc
>> +endef
>> +TARGET_DEVICES += glinet_gl-mv1000-emmc
>> +
>> diff --git a/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh b/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
>> index 842e591..50028fe 100755
>> --- a/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
>> +++ b/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
>> @@ -51,6 +51,12 @@ while [ "$#" -ge 3 ]; do
>>  shift; shift; shift
>>  done
>>  
>> +model=''
>> +model=$(echo $OUTFILE | grep "gl-mv1000-emmc")
>> +[ "$model" != "" ] && {
>> + ptgen_args="$ptgen_args -t 83 -p 7093504"
>  
> Please don't add device hacks here it's ment to be as generic
> as posible. Instead, add possibility to override CONFIG_TARGET_ROOTFS_PARTSIZE
> as parameter to "boot-img-ext4" command.
>  
>> +}
>> +
>>  head=16
>>  sect=63
>>  
>> diff --git a/target/linux/mvebu/image/generic-arm64-emmc.bootscript b/target/linux/mvebu/image/generic-arm64-emmc.bootscript
>> new file mode 100644
>> index 0000000..4de4d39
>> --- /dev/null
>> +++ b/target/linux/mvebu/image/generic-arm64-emmc.bootscript
>> @@ -0,0 +1,12 @@
>> +setenv bootargs "root=/dev/mmcblk0p2 rw rootwait"
>> +
>> +if test -n "${console}"; then
>> + setenv bootargs "${bootargs} ${console}"
>> +fi
>> +
>> +setenv mmcdev 0
>  
> Does the U-Boot only load "boot.scr" from first mmcdev
> (I assume that's eMMC) or also searches for it on SD card? If
> searched on both devices, there's no need to add this file, use
> generic-arm64.bootscript. That way image will be applicable to
> both storage mediums, just write this information in commit
> message to inform users. If U-Boot searches for "boot.scr" only
> on eMMC, then rename this file to gl-mv1000.bootscript and set
> it in BOOT_SCRIPT variable.
>> +
>> +load mmc ${mmcdev}:1 ${fdt_addr} @DTB@.dtb
>> +load mmc ${mmcdev}:1 ${kernel_addr} Image
>> +
>> +booti ${kernel_addr} - ${fdt_addr}
>>
>  
> Regards, Tomasz
>
Piotr Dymacz April 15, 2020, 9:36 a.m. UTC | #5
Hi Tomasz, Li,

On 14.04.2020 20:53, Tomasz Maciej Nowak wrote:
> W dniu 14.04.2020 o 06:20, Li.zhang pisze:
>> Hi Tomasz,
>> Thank you very much for you correction.
>> 
>>> +};
>>> +
>>> +&spi0 {
>>> +        status = "okay";
>>> +
>>> +        flash@0 {
>>> +                reg = <0>;
>>> +                compatible = "jedec,spi-nor";
>>> +                spi-max-frequency = <104000000>;
>>> +                m25p,fast-read;
>>> +                partitions {
>>> +                        compatible = "fixed-partitions";
>>> +                        #address-cells = <1>;
>>> +                        #size-cells = <1>;
>>> +
>>> +                        partition@0 {
>>> +                                label = "u-boot";
>>> +                                reg = <0 0xf0000>;
>>> +                        };
>>> +
>>> +                        partition@f0000 {
>>> +                                label = "u-boot-env";
>>> +                                reg = <0Xf0000 0x8000>;
>>> +                        };
>>> +
>>> +                        art: partition@f8000 {
>>> +                                label = "art";
>> 
>> The name of this partition is rather strange, since the hardware
>> specification doesn't mention any PCIe or SDIO connected wireless
>> cards (don't know if there's any USB card without eeprom). Is
>> there any calibration data stored or only MAC address/addresses?
>> Does U-Boot also read information from this partition? Anyway
>> that's not a remark to change that name, I'm just curious.
>> --->MAC,SN,DDNS and ther private information are stored in the art.
> 
> Ok, still that name is weird, but that's matter of taste.

ART has a specific meaning (Atheros Radio Test) and it looks just wrong 
here, not just weird.

Why not change it to something less misleading?
Maybe 'factory' or 'vendor'?
Li Zhang April 15, 2020, 9:51 a.m. UTC | #6
Hi Piotr,Tomasz:

ART has a specific meaning (Atheros Radio Test) and it looks just wrong
here, not just weird.
 
Why not change it to something less misleading?
Maybe 'factory' or 'vendor'?

Thank you very much for your advise.Next time,I'll change it.


Li Zhang | Software Engineer
li.zhang@gl-inet.com 
GL.iNet  WiFi for Things
Website: www.gl-inet.com   |   LinkedIn: gl-inet.com   |   Tel: +86-755-8660-6126
Room 305-306, Skyworth Digital Building , Shiyan Street, Baoan District, Shenzhen, China
Email Disclaimer: The content of this email is confidential and intended for the recipient specified in message only. It is strictly forbidden to share any part of this message with any third party, without a written consent of the sender. If you received this message by mistake, please reply to this message and follow with its deletion, so that we can ensure such a mistake does not occur in the future.
 
From: Piotr Dymacz
Date: 2020-04-15 17:36
To: Tomasz Maciej Nowak; Li.zhang; openwrt-devel
Subject: Re: [OpenWrt-Devel] [PATCH] mvebu: add support for GL.iNet GL-MV1000
Hi Tomasz, Li,
 
On 14.04.2020 20:53, Tomasz Maciej Nowak wrote:
> W dniu 14.04.2020 o 06:20, Li.zhang pisze:
>> Hi Tomasz,
>> Thank you very much for you correction.
>> 
>>> +};
>>> +
>>> +&spi0 {
>>> +        status = "okay";
>>> +
>>> +        flash@0 {
>>> +                reg = <0>;
>>> +                compatible = "jedec,spi-nor";
>>> +                spi-max-frequency = <104000000>;
>>> +                m25p,fast-read;
>>> +                partitions {
>>> +                        compatible = "fixed-partitions";
>>> +                        #address-cells = <1>;
>>> +                        #size-cells = <1>;
>>> +
>>> +                        partition@0 {
>>> +                                label = "u-boot";
>>> +                                reg = <0 0xf0000>;
>>> +                        };
>>> +
>>> +                        partition@f0000 {
>>> +                                label = "u-boot-env";
>>> +                                reg = <0Xf0000 0x8000>;
>>> +                        };
>>> +
>>> +                        art: partition@f8000 {
>>> +                                label = "art";
>> 
>> The name of this partition is rather strange, since the hardware
>> specification doesn't mention any PCIe or SDIO connected wireless
>> cards (don't know if there's any USB card without eeprom). Is
>> there any calibration data stored or only MAC address/addresses?
>> Does U-Boot also read information from this partition? Anyway
>> that's not a remark to change that name, I'm just curious.
>> --->MAC,SN,DDNS and ther private information are stored in the art.
> 
> Ok, still that name is weird, but that's matter of taste.
 
ART has a specific meaning (Atheros Radio Test) and it looks just wrong 
here, not just weird.
 
Why not change it to something less misleading?
Maybe 'factory' or 'vendor'?
diff mbox series

Patch

diff --git a/package/boot/uboot-envtools/files/mvebu b/package/boot/uboot-envtools/files/mvebu
index 7902384..9d23c77 100644
--- a/package/boot/uboot-envtools/files/mvebu
+++ b/package/boot/uboot-envtools/files/mvebu
@@ -24,6 +24,9 @@  globalscale,espressobin-v7-emmc|\
 marvell,armada8040-mcbin)
 	ubootenv_add_uci_config "/dev/mtd0" "0x3f0000" "0x10000" "0x10000" "1"
 	;;
+glinet,gl-mv1000-emmc)
+	ubootenv_add_uci_config "/dev/mtd1" "0x0" "0x8000" "0x8000" "1"
+	;;
 linksys,caiman|\
 linksys,cobra|\
 linksys,shelby)
diff --git a/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network b/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
index ba4b930..e5ff667 100755
--- a/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
+++ b/target/linux/mvebu/cortexa53/base-files/etc/board.d/02_network
@@ -14,7 +14,8 @@  case "$board" in
 globalscale,espressobin|\
 globalscale,espressobin-emmc|\
 globalscale,espressobin-v7|\
-globalscale,espressobin-v7-emmc)
+globalscale,espressobin-v7-emmc|\
+glinet,gl-mv1000-emmc)
 	ucidef_set_interfaces_lan_wan "lan0 lan1" "wan"
 	;;
 marvell,armada-3720-db|\
diff --git a/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts b/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
new file mode 100644
index 0000000..fe01dfe
--- /dev/null
+++ b/target/linux/mvebu/files-4.19/arch/arm64/boot/dts/marvell/armada-gl-mv1000-emmc.dts
@@ -0,0 +1,68 @@ 
+/*
+ * Device Tree file for GL.iNet GL-MV1000
+ */
+
+#include "armada-3720-espressobin.dts"
+
+/ {
+       model = "GL.inet GL-MV1000 (Marvell)";
+       compatible = "glinet,gl-mv1000-emmc";
+};
+
+&spi0 {
+        status = "okay";
+
+        flash@0 {
+                reg = <0>;
+                compatible = "jedec,spi-nor";
+                spi-max-frequency = <104000000>;
+                m25p,fast-read;
+                partitions {
+                        compatible = "fixed-partitions";
+                        #address-cells = <1>;
+                        #size-cells = <1>;
+
+                        partition@0 {
+                                label = "u-boot";
+                                reg = <0 0xf0000>;
+                        };
+
+                        partition@f0000 {
+                                label = "u-boot-env";
+                                reg = <0Xf0000 0x8000>;
+                        };
+
+                        art: partition@f8000 {
+                                label = "art";
+                                reg = <0xf8000 0x8000>;
+                        };
+
+               };
+        };
+};
+
+&sdhci1 {
+        wp-inverted;
+        bus-width = <4>;
+        cd-gpios = <&gpionb 17 GPIO_ACTIVE_LOW>;
+        marvell,pad-type = "sd";
+        no-1-8-v;
+        vqmmc-supply = <&vcc_sd_reg1>;
+        status = "okay";
+};
+
+
+&sdhci0 {
+        bus-width = <8>;
+        mmc-ddr-1_8v;
+        mmc-hs400-1_8v;
+        non-removable;
+        no-sd;
+        no-sdio;
+        marvell,pad-type = "fixed-1-8v";
+        status = "okay";
+};
+
+&eth0 {
+	mtd-mac-address = <&art 0x0>;
+};
diff --git a/target/linux/mvebu/image/Makefile b/target/linux/mvebu/image/Makefile
index ef92748..b848049 100644
--- a/target/linux/mvebu/image/Makefile
+++ b/target/linux/mvebu/image/Makefile
@@ -107,6 +107,15 @@  define Device/Default-arm64
   KERNEL := kernel-bin
 endef
 
+define Device/Default-arm64-emmc
+  BOOT_SCRIPT := generic-arm64-emmc
+  DEVICE_DTS_DIR := $(DTS_DIR)/marvell
+  IMAGES := emmc.img
+  IMAGE/emmc.img := boot-scr | boot-img-ext4 | sdcard-img-ext4 | append-metadata
+  KERNEL_NAME := Image
+  KERNEL := kernel-bin
+endef
+
 define Device/NAND-128K
   BLOCKSIZE := 128k
   PAGESIZE := 2048
diff --git a/target/linux/mvebu/image/cortexa53.mk b/target/linux/mvebu/image/cortexa53.mk
index 77f5c79..4dfd0b4 100644
--- a/target/linux/mvebu/image/cortexa53.mk
+++ b/target/linux/mvebu/image/cortexa53.mk
@@ -69,3 +69,12 @@  define Device/methode_udpu
   BOOT_SCRIPT := udpu
 endef
 TARGET_DEVICES += methode_udpu
+
+define Device/glinet_gl-mv1000-emmc
+  $(call Device/Default-arm64-emmc)
+  DEVICE_TITLE := GL.iNet GL-MV1000 EMMC
+  DEVICE_DTS := armada-gl-mv1000-emmc
+  SUPPORTED_DEVICES := glinet,gl-mv1000-emmc
+endef
+TARGET_DEVICES += glinet_gl-mv1000-emmc
+
diff --git a/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh b/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
index 842e591..50028fe 100755
--- a/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
+++ b/target/linux/mvebu/image/gen_mvebu_sdcard_img.sh
@@ -51,6 +51,12 @@  while [ "$#" -ge 3 ]; do
 	shift; shift; shift
 done
 
+model=''
+model=$(echo $OUTFILE | grep "gl-mv1000-emmc")
+[ "$model" != "" ] && {
+	ptgen_args="$ptgen_args -t 83 -p 7093504"
+}
+
 head=16
 sect=63
 
diff --git a/target/linux/mvebu/image/generic-arm64-emmc.bootscript b/target/linux/mvebu/image/generic-arm64-emmc.bootscript
new file mode 100644
index 0000000..4de4d39
--- /dev/null
+++ b/target/linux/mvebu/image/generic-arm64-emmc.bootscript
@@ -0,0 +1,12 @@ 
+setenv bootargs "root=/dev/mmcblk0p2 rw rootwait"
+
+if test -n "${console}"; then
+	setenv bootargs "${bootargs} ${console}"
+fi
+
+setenv mmcdev 0
+
+load mmc ${mmcdev}:1 ${fdt_addr} @DTB@.dtb
+load mmc ${mmcdev}:1 ${kernel_addr} Image
+
+booti ${kernel_addr} - ${fdt_addr}