[PATCH 1/1] TARGET: Add new device support under target IPQ806x for Linksys E8350

[mail at adrianschmutzler.de]

> > --- /dev/null
> > +++ b/target/linux/ipq806x/base-files/etc/rc.button/reset
>
> What's that for? Implementation of reset button is available with KEY_RESTART OOTB.
> -> NOTE: Linksys E8350 uses ubi volumes not jffs2 . Until the "factory reset" script properly handle such volumes(which is not the case at the moment). I've made ipq806x copy of it with added specific actions for linksys,e8350. The ipq806x copy of "reset" > script do not break any old dependencies and have to be removed as redundant once there is a proper factory reset script for UBI volumes is in place. This have to be assigned as task to the maintainer of the "reset" script. It is not a bug but lacking of new features of the current reset script.

So, what you want to tell me is that the reset button is broken on _all_ devices using ubi volumes? Then, please fix it for all of them (in a separate patch, like you did for the wifi up issue), instead of adding a "workaround" just for your device at hand.

> > --- /dev/null
> > +++ b/target/linux/ipq806x/base-files/lib/upgrade/fwtool.sh
>
> Same here. Why do you copy that stuff over?
> -> NEED DEFINITION: the device uses one main "UBI" volume with kernel,roofs,rootfs_data in it. The sysupgrade script has lack of feature to remove the "metadata" trailer: "nand.sh" -> nand_upgrade_ubinized -> no medata in the sysupgrade image should be there before "ubiformat "${mtddev}" -y -f "${ubi_file}"" command!
>
> -> To overcome this limitation I use "fwtool" with option -t. 
> -> fwtool.sh is coping the fwtool to /tmp location ( all the rest is not touched ) and than the metadata is removed from platform.sh script running fwtool -t option. fwtool is not available after the switch to sysupgrade ramfs rootfs and the copy to /tmp directory preserves it.
> May be there is a better approach (use variable to remove the "metadata" before "nand_upgrade_ubinized") but this has to be assigned to the maintainer of "nand.sh" script which does not properly handle the removal of metadata in "nand_upgrade_ubinized" function. (nand.sh bug or feature?)

I'm not an UBI/NAND expert. However, I don't really believe that there is no better way to achieve this, particularly given that this is not the first ubi device added to OpenWrt.
As with my previous comment, this looks to me like a general, not single-device-related problem, so please address it that way.

This is particularly important if you touch sysupgrade components. This is really complicated enough, I don't want to see different local flavors of certain scripts on top of that.

Please note that it appears your responses never made it to the openwrt-devel list. Either your address is subject to some kind of greylisting and they will just be delayed, or there is another problem there. The mail address in Cc seems correct, though.

Best

Adrian


> @@ -0,0 +1,67 @@
> +fwtool_check_signature() {
> + [ $# -gt 1 ] && return 1
> +
> + [ ! -x /usr/bin/ucert ] && {
> + if [ "$REQUIRE_IMAGE_SIGNATURE" = 1 ]; then
> + return 1
> + else
> + return 0
> + fi
> + }
> +
> + if ! fwtool -q -s /tmp/sysupgrade.ucert "$1"; then
> + echo "Image signature not found"
> + [ "$REQUIRE_IMAGE_SIGNATURE" = 1 -a "$FORCE" != 1 ] && {
> + echo "Use sysupgrade -F to override this check when
> downgrading or flashing to vendor firmware"
> + }
> + [ "$REQUIRE_IMAGE_SIGNATURE" = 1 ] && return 1
> + return 0
> + fi
> +
> + fwtool -q -T -s /dev/null "$1" | \
> + ucert -V -m - -c "/tmp/sysupgrade.ucert" -P /etc/opkg/keys
> +
> + return $?
> +}
> +
> +fwtool_check_image() {
> + [ $# -gt 1 ] && return 1
> +
> + . /usr/share/libubox/jshn.sh
> +
> + if ! fwtool -q -i /tmp/sysupgrade.meta "$1"; then
> + echo "Image metadata not found"
> + [ "$REQUIRE_IMAGE_METADATA" = 1 -a "$FORCE" != 1 ] && {
> + echo "Use sysupgrade -F to override this check when
> downgrading or flashing to vendor firmware"
> + }
> + [ "$REQUIRE_IMAGE_METADATA" = 1 ] && return 1
> + return 0
> + fi
> + # For UBI volumes we need fwtool on later stage to remove the
> metadata - via platform.sh!!!
> + cp /usr/bin/fwtool /tmp/.
> +
> + json_load "$(cat /tmp/sysupgrade.meta)" || {
> + echo "Invalid image metadata"
> + return 1
> + }
> +
> + device="$(cat /tmp/sysinfo/board_name)"
> +
> + json_select supported_devices || return 1
> +
> + json_get_keys dev_keys
> + for k in $dev_keys; do
> + json_get_var dev "$k"
> + [ "$dev" = "$device" ] && return 0
> + done
> +
> + echo "Device $device not supported by this image"
> + echo -n "Supported devices:"
> + for k in $dev_keys; do
> + json_get_var dev "$k"
> + echo -n " $dev"
> + done
> + echo
> +
> + return 1
> +}
> diff --git a/target/linux/ipq806x/base-files/lib/upgrade/platform.sh
> b/target/linux/ipq806x/base-files/lib/upgrade/platform.sh
> index 560e64af3a..50a7ade058 100644
> --- a/target/linux/ipq806x/base-files/lib/upgrade/platform.sh
> +++ b/target/linux/ipq806x/base-files/lib/upgrade/platform.sh
> @@ -27,6 +27,11 @@ platform_do_upgrade() {
> zyxel,nbg6817)
> zyxel_do_upgrade "$1"
> ;;
> + linksys,e8350)
> + # first remove metadata trailer from the UBI volume
> + /tmp/fwtool -q -t -i /dev/null "$1"
This looks strange.
- see the comment above

> + nand_do_upgrade "$1"
> + ;;
> linksys,ea7500-v1 |\
> linksys,ea8500)
> platform_do_upgrade_linksys "$1"
> diff --git a/target/linux/ipq806x/files-5.4/arch/arm/boot/dts/qcom-ipq8064-
> e8350.dts b/target/linux/ipq806x/files-5.4/arch/arm/boot/dts/qcom-
> ipq8064-e8350.dts
> new file mode 100644
> index 0000000000..0af1d39880
> --- /dev/null
> +++ b/target/linux/ipq806x/files-5.4/arch/arm/boot/dts/qcom-ipq8064-e835
> +++ 0.dts
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT #include
> +"qcom-ipq8064-v2.0.dtsi"
> +
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/soc/qcom,tcsr.h>
> +
> +/ {
> + model = "Linksys EA8300 V1 WiFi Router";

So, is it V1? Then add that to the compatible etc.: linksys,e8350-v1
-> I've not sen V2 yet, so I can remove the versioning of this device in the updated patch.

> + compatible = "linksys,e8350", "qcom,ipq8064";
> +
> + memory at 0 {
> + reg = <0x42000000 0x1e000000>;
> + device_type = "memory";
> + };
> +
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + rsvd at 41200000 {
> + reg = <0x41200000 0x300000>;
> + no-map;
> + };
> + };
> +
> + aliases {
> + serial0 = &gsbi4_serial;

This line can be dropped, it has been moved to the DTSI recently.
-> OK, so it will be in the updated patch

> +
> + led-boot = &led_power;
> + led-failsafe = &led_power;
> + led-running = &led_power;
> + led-upgrade = &led_power;
> + };
> +
> + keys {
> + compatible = "gpio-keys";
> + pinctrl-0 = <&button_pins>;
> + pinctrl-names = "default";
> +
> + reset {

Your indent is broken/mixed, same goes for empty lines between nodes.
DTS should have tab indent only, and nodes should be separated by empty lines.
-> will be fixed in the updated patch

> + label = "reset";
> + gpios = <&qcom_pinmux 68 GPIO_ACTIVE_LOW>;
> + linux,code = <KEY_RESTART>;
> + };
> +
> + wps {
> + label = "wps";
> + gpios = <&qcom_pinmux 65 GPIO_ACTIVE_LOW>;
> + linux,code = <KEY_WPS_BUTTON>;
> + };
> + wifi {
> + label = "wifi";
> + gpios = <&qcom_pinmux 67 GPIO_ACTIVE_LOW>;
> + linux,code = <KEY_RFKILL>;
> + };
> + };
> +
> + nand-controller at 1ac00000 {
> + compatible = "qcom,ipq806x-nand";
> + reg = <0x1ac00000 0x800>;
> +
> + clocks = <&gcc EBI2_CLK>,
> + <&gcc EBI2_AON_CLK>;
> + clock-names = "core", "aon";
> +
> + dmas = <&adm_dma 3>;
> + dma-names = "rxtx";
> + qcom,cmd-crci = <15>;
> + qcom,data-crci = <3>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + nand at 0 {
> + reg = <0>;
> +
> + nand-ecc-strength = <4>;
> + nand-bus-width = <8>;
> +
> + partitions {
> + compatible = "fixed-partitions";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + partition at 0 {
> + label = "ubi";
> + reg = <0 0x4000000>;
> + };
> + partition at 4000000 {
> + label = "extra";
> + reg = <0x4000000 0x4000000>;
> + };
> + };
> + };
> + };
> +
> +
> + leds {
> + compatible = "gpio-leds";
> + pinctrl-0 = <&led_pins>;
> + pinctrl-names = "default";
> +
> + led_power: power {
> + label = "e8350:green:power";
> + gpios = <&qcom_pinmux 26 GPIO_ACTIVE_HIGH>;
> + default-state = "keep";
> + };
> +
> + wps {
> + label = "e8350:green:wps";
> + gpios = <&qcom_pinmux 53 GPIO_ACTIVE_HIGH>;
> + };
> +
> + wifi {
> + label = "e8350:green:wifi";
> + gpios = <&qcom_pinmux 54 GPIO_ACTIVE_HIGH>;
> + };
> +

drop this empty line.

> + };
> +};
> +
> +&qcom_pinmux {
> + button_pins: button_pins {
> + mux {
> + pins = "gpio68","gpio65", "gpio67";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-pull-up;
> + };
> + };
> +
> + led_pins: led_pins {
> + mux {
> + pins = "gpio26","gpio53", "gpio54";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-pull-up;
> + };
> + };
> +};
> +
> +&gsbi4 {
> + qcom,mode = <GSBI_PROT_I2C_UART>;
> + status = "okay";
> + serial at 16340000 {
> + status = "okay";
> + };
> + /*
> + * The i2c device on gsbi4 should not be enabled.
> + * On ipq806x designs gsbi4 i2c is meant for exclusive
> + * RPM usage. Turning this on in kernel manifests as
> + * i2c failure for the RPM.
> + */
> +};
> +&gsbi5 {
> + qcom,mode = <GSBI_PROT_SPI>;
> + status = "okay";
> +
> + spi5: spi at 1a280000 {
> + status = "okay";
> +
> + pinctrl-0 = <&spi_pins>;
> + pinctrl-names = "default";
> +
> + cs-gpios = <&qcom_pinmux 20 GPIO_ACTIVE_HIGH>;
> +
> + m25p80 at 0 {
> + compatible = "jedec,spi-nor";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + spi-max-frequency = <51200000>;
> + reg = <0>;
> +
> + partitions {
> + compatible = "qcom,smem";
> + };
> + };
> + };
> +};
> +
> +&sata_phy {
> + status = "okay";
> +};
> +
> +&sata {
> + status = "okay";
> +};
> +
> +&usb3_0 {
> + clocks = <&gcc USB30_1_MASTER_CLK>;
> + status = "okay";
> +};
> +
> +&usb3_1 {
> + clocks = <&gcc USB30_0_MASTER_CLK>;
> + status = "okay";
> +};
> +
> +&pcie0 {
> + status = "okay";
> + force_gen1 = <1>;
> +};
> +
> +&pcie1 {
> + status = "okay";
> +};
> +
> +&pcie2 {
> + status = "okay";
> +};
> +
> +&mdio0 {
> + status = "okay";
> +
> + pinctrl-0 = <&mdio0_pins>;
> + pinctrl-names = "default";
> +
> + phy0: ethernet-phy at 0 {
> + reg = <0>;
> + qca,ar8327-initvals = <
> + 0x00004 0x7600000 /* PAD0_MODE */
> + 0x00008 0x1000000 /* PAD5_MODE */
> + 0x0000c 0x80 /* PAD6_MODE */
> + 0x00010 0x2613a0 /* PWS_REG */
> + 0x000e4 0x6a545 /* MAC_POWER_SEL */
> + 0x000e0 0xc74164de /* SGMII_CTRL */
> + 0x0007c 0x4e /* PORT0_STATUS */
> + 0x00094 0x4e /* PORT6_STATUS */
> + >;
> + };
> +};
> +
> +&gmac1 {
> + status = "okay";
> + phy-mode = "rgmii";
> + qcom,id = <1>;
> +
> + pinctrl-0 = <&rgmii2_pins>;
> + pinctrl-names = "default";
> +
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> +};
> +
> +&gmac2 {
> + status = "okay";
> + phy-mode = "sgmii";
> + qcom,id = <2>;
> +
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> +};
> +
> +&tcsr {
> + qcom,usb-ctrl-select = <TCSR_USB_SELECT_USB3_DUAL>;
> + compatible = "qcom,tcsr";
> +};
> +
> +&adm_dma {
> + status = "okay";
> +};
> diff --git a/target/linux/ipq806x/image/Makefile
> b/target/linux/ipq806x/image/Makefile
> index 5cc66daa21..42512ed423 100644
> --- a/target/linux/ipq806x/image/Makefile
> +++ b/target/linux/ipq806x/image/Makefile
> @@ -13,6 +13,17 @@ define Build/buffalo-rootfs-cksum
> ) >> $@
> endef
> 
> +# tune addpattern for Linksys E8350 fw pattern generation define
> +Build/linksys-bin
> + $(STAGING_DIR_HOST)/bin/addpattern -p $(FW_DEVICE_ID) -v
> $(FW_VERSION) $(if $(SERIAL),-s $(SERIAL)) -i $@ -o $@.new
> + mv $@.new $@
> +endef
> +# Add Linksys E8350 fw header generator from the linksys gpl code: to
> +upgrade openwrt factory image over the native Linksys WEB interface
> define Build/addfwhdr
> + -$(STAGING_DIR_HOST)/bin/linksys/e8350/addfwhdr -i $@ -o $@.new

I didn't find addfwhdr anywhere?
-> you have already found it in a separate patch - tools-firmware-utils ;)

> \
> + ;mv "$@.new" "$@"
> +endef

Since this is the first Linksys device needing that stuff after several others were added to the target, one should name them more specificly as the current names imply that those apply to all Linksys devices which is just not the case.

> +
> define Device/Default
> PROFILES := Default
> KERNEL_DEPENDS = $$(wildcard $(DTS_DIR)/$$(DEVICE_DTS).dts)
> @@ -109,6 +120,26 @@ define Device/compex_wpq864 endef
> TARGET_DEVICES += compex_wpq864
> 
> +define Device/linksys_e8350
> + $(call Device/LegacyImage)
> + BOARD_NAME = e8350
> + SUPPORTED_DEVICES += e8350
Drop BOARD_NAME and SUPPORTED_DEVICES.
-> what about the metadata generation, will it be affected?

> + DEVICE_VENDOR := Linksys
> + DEVICE_MODEL := E8350
> + DEVICE_VARIANT := v1

Okay, so this should definitely be called linksys_e8350-v1 and Linksys,e8350-v1.

> + SOC := qcom-ipq8064
> + FW_VERSION := v1.0.03.003
> + FW_DEVICE_ID := 8350
> + PAGESIZE := 2048
> + BLOCKSIZE := 128k
> + KERNEL_IN_UBI := 1
> + IMAGES = factory.bin sysupgrade.ubi
> + IMAGE/sysupgrade.ubi := append-ubi | check-size 0x04000000 |
> append-metadata
> + IMAGE/factory.bin := append-ubi | check-size 0x04000000 | addfwhdr |
> linksys-bin
> + DEVICE_PACKAGES := ath10k-firmware-qca988x-ct endef
> +TARGET_DEVICES += linksys_e8350
> +
> define Device/linksys_ea7500-v1
> $(call Device/LegacyImage)
> DEVICE_VENDOR := Linksys
> diff --git a/target/linux/ipq806x/patches-5.4/0069-arm-boot-add-dts-
> files.patch b/target/linux/ipq806x/patches-5.4/0069-arm-boot-add-dts-
> files.patch
> index 8cdd198c29..8e4a3fd876 100644
> --- a/target/linux/ipq806x/patches-5.4/0069-arm-boot-add-dts-files.patch
> +++ b/target/linux/ipq806x/patches-5.4/0069-arm-boot-add-dts-files.patch
> @@ -10,7 +10,7 @@ Signed-off-by: John Crispin <john at phrozen.org>
> 
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> -@@ -843,6 +843,19 @@ dtb-$(CONFIG_ARCH_QCOM) += \
> +@@ -843,6 +843,20 @@ dtb-$(CONFIG_ARCH_QCOM) += \
> qcom-ipq4019-ap.dk07.1-c1.dtb \
> qcom-ipq4019-ap.dk07.1-c2.dtb \
> qcom-ipq8064-ap148.dtb \
> @@ -19,6 +19,7 @@ Signed-off-by: John Crispin <john at phrozen.org>
> + qcom-ipq8064-db149.dtb \
> + qcom-ipq8064-ap161.dtb \
> + qcom-ipq8064-ea7500-v1.dtb \
> ++ qcom-ipq8064-e8350.dtb \

Sorting.

Best

Adrian

> + qcom-ipq8064-ea8500.dtb \
> + qcom-ipq8064-r7500.dtb \
> + qcom-ipq8064-r7500v2.dtb \
> --
> 2.27.0
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openpgp-digital-signature.asc
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.openwrt.org/pipermail/openwrt-devel/attachments/20200720/03058910/attachment-0001.sig>