[OpenWrt-Devel,3/3] ath79: add support for Teltonika RUT955

The Teltonika RUT955 is an industrial 2G/3G/4G WiFi router with
various additional inputs and outputs.

Specification:

- 550/400/200 MHz (CPU/DDR/AHB)
- 128 MB of RAM (DDR2)
- 16 MB of FLASH (SPI NOR)
- 4x 10/100 Mbps Ethernet, with passive PoE support on LAN1
- 2T2R 2,4 GHz (AR9344)
- built-in 3G module (example: Qutectel EC-25EU)
- RS232 on D-Sub9 port (Cypress ACM via USB, /dev/ttyACM0)
- RS422/RS485 (AR934x high speed UART, /dev/ttyATH1)
- analog 0-9V input (MCP3221)
- various digital inputs and outputs incl. a relay
- 2x miniSIM slot (can be swapped via GPIO)
- 2x RP-SMA/F (Wi-Fi), 2x SMA/F (3G), 1x GPS
- 2x 74HC595 shift registers providing 16 GPOs
- 12x LED (4 are driven by AR9344, 7 by 74HC595)
- 1x button (reset)
- DC jack for main power input (9-30 V)
- debugging UART available on PCB edge connector

Serial console (/dev/ttyS0) pinout:

- RX: pin1 (square) on top side of the main PCB (AR9344 is on top)
- TX: pin1 (square) on bottom side

Flash instruction:

Vendor firmware is based on OpenWrt CC release. Use the "factory" image
directly in GUI (make sure to uncheck "keep settings") or in U-Boot web
based recovery. To avoid any problems, make sure to first update vendor
firmware to latest version - "factory" image was successfully tested on
device running "RUT9XX_R_00.06.051" firmware and U-Boot "3.0.2".

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 target/linux/ath79/dts/ar9344_tlt_rut955.dts  | 301 ++++++++++++++++++
 .../generic/base-files/etc/board.d/02_network |   5 +
 target/linux/ath79/image/generic.mk           |  37 +++
 3 files changed, 343 insertions(+)
 create mode 100644 target/linux/ath79/dts/ar9344_tlt_rut955.dts

Hi Daniel,

several comments inline below.

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Daniel Golle
> Sent: Sonntag, 2. Februar 2020 17:42
> To: openwrt-devel@lists.openwrt.org
> Subject: [OpenWrt-Devel] [PATCH 3/3] ath79: add support for Teltonika
> RUT955
> 
> The Teltonika RUT955 is an industrial 2G/3G/4G WiFi router with various
> additional inputs and outputs.
> 
> Specification:
> 
> - 550/400/200 MHz (CPU/DDR/AHB)
> - 128 MB of RAM (DDR2)
> - 16 MB of FLASH (SPI NOR)
> - 4x 10/100 Mbps Ethernet, with passive PoE support on LAN1
> - 2T2R 2,4 GHz (AR9344)
> - built-in 3G module (example: Qutectel EC-25EU)
> - RS232 on D-Sub9 port (Cypress ACM via USB, /dev/ttyACM0)
> - RS422/RS485 (AR934x high speed UART, /dev/ttyATH1)
> - analog 0-9V input (MCP3221)
> - various digital inputs and outputs incl. a relay
> - 2x miniSIM slot (can be swapped via GPIO)
> - 2x RP-SMA/F (Wi-Fi), 2x SMA/F (3G), 1x GPS
> - 2x 74HC595 shift registers providing 16 GPOs
> - 12x LED (4 are driven by AR9344, 7 by 74HC595)
> - 1x button (reset)
> - DC jack for main power input (9-30 V)
> - debugging UART available on PCB edge connector
> 
> Serial console (/dev/ttyS0) pinout:
> 
> - RX: pin1 (square) on top side of the main PCB (AR9344 is on top)
> - TX: pin1 (square) on bottom side
> 
> Flash instruction:
> 
> Vendor firmware is based on OpenWrt CC release. Use the "factory" image
> directly in GUI (make sure to uncheck "keep settings") or in U-Boot web
> based recovery. To avoid any problems, make sure to first update vendor
> firmware to latest version - "factory" image was successfully tested on device
> running "RUT9XX_R_00.06.051" firmware and U-Boot "3.0.2".
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  target/linux/ath79/dts/ar9344_tlt_rut955.dts  | 301 ++++++++++++++++++
>  .../generic/base-files/etc/board.d/02_network |   5 +
>  target/linux/ath79/image/generic.mk           |  37 +++
>  3 files changed, 343 insertions(+)
>  create mode 100644 target/linux/ath79/dts/ar9344_tlt_rut955.dts
> 
> diff --git a/target/linux/ath79/dts/ar9344_tlt_rut955.dts
> b/target/linux/ath79/dts/ar9344_tlt_rut955.dts
> new file mode 100644
> index 0000000000..06d18f8d26
> --- /dev/null
> +++ b/target/linux/ath79/dts/ar9344_tlt_rut955.dts
> @@ -0,0 +1,301 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT /dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +
> +#include "ar9344.dtsi"
> +
> +/ {
> +	model = "Teltonika RUT955";
> +	compatible = "tlt,rut955", "qca,ar9344";

I would prefer having the full vendor name here (teltonika,rut955), as we tried to avoid abbreviations in names for ath79 wherever possible (with the exception of ubnt).

> +
> +	aliases {
> +		serial0 = &uart;
> +		serial1 = &hs_uart;
> +		led-boot = &led_system_green;
> +		led-failsafe = &led_system_red;
> +		led-running = &led_system_green;
> +		led-upgrade = &led_system_red;
> +		label-mac-device = &eth1;
> +	};
> +
> +	i2c {
> +		compatible = "i2c-gpio";
> +		scl-gpios = <&gpio 16
> (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> +		sda-gpios = <&gpio 17
> (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		hwmon@4d {
> +			compatible = "microchip,mcp3221";
> +			reg = <0x4d>;
> +		};
> +	};
> +
> +	gpio_ext_spi {
> +		compatible = "spi-gpio";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pmx_led_spi_gpio>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		sck-gpios = <&gpio 4 GPIO_ACTIVE_HIGH>;     // 74HC595
> SRCLK (Serial Clock)
> +		mosi-gpios = <&gpio 12 GPIO_ACTIVE_HIGH>;   // 74HC595
> SER (Serial)
> +		cs-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;     // 74HC595
> RCLK (Register Clock)
> +		num-chipselects = <1>;
> +
> +		gpio_ext: gpio_ext@0 {
> +			compatible = "fairchild,74hc595";
> +			reg = <0>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			registers-number = <2>;
> +			spi-max-frequency = <10000000>;
> +			gpio-line-names = "signal_bar0", "signal_bar1",
> "signal_bar2", "signal_bar3",
> +				"signal_bar4", "status_red", "status_green",
> "sim_sel",
> +				"", "relay", "modem_vbus", "modem_rst",
> +				"", "", "", "";
> +		};
> +	};
> +
> +	reg_usb_modem_vbus: reg_usb_modem_vbus {
> +		compatible = "regulator-fixed";
> +		regulator-name = "usb_modem_vbus";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		gpio = <&gpio_ext 10 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +		regulator-always-on;
> +		regulator-boot-on;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		signal0 {
> +			label = "rut955:green:signal1";

I'd prefer having the same numbering for node and label here (signal0<>signal1).

> +			gpios = <&gpio_ext 0 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +		};
> +
> +		signal1 {
> +			label = "rut955:green:signal2";
> +			gpios = <&gpio_ext 1 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +		};
> +
> +		signal2 {
> +			label = "rut955:green:signal3";
> +			gpios = <&gpio_ext 2 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +		};
> +
> +		signal3 {
> +			label = "rut955:green:signal4";
> +			gpios = <&gpio_ext 3 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +		};
> +
> +		signal4 {
> +			label = "rut955:green:signal5";
> +			gpios = <&gpio_ext 4 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +		};
> +
> +		led_system_red: systemred {

Please add an underscore: system_red

> +			label = "rut955:green:red";

This should be rut955:red:system ?

> +			gpios = <&gpio_ext 5 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";

Default-state "off" is default, so I think this is not needed (also above).

> +		};
> +
> +		led_system_green: systemgreen {
> +			label = "rut955:green:system";
> +			gpios = <&gpio_ext 6 GPIO_ACTIVE_HIGH>;
> +			default-state = "on";
> +		};
> +	};
> +
> +	keys {
> +		compatible = "gpio-keys";
> +
> +		reset {
> +			linux,code = <KEY_RESTART>;
> +			gpios = <&gpio 15 GPIO_ACTIVE_HIGH>;
> +			debounce-interval = <60>;
> +		};
> +	};
> +};
> +
> +&gpio {
> +	gpio-line-names = "", "wan_led", "input", "mmc_cs",
> +		"leds_sck", "", "", "",
> +		"", "", "", "",
> +		"leds_mosi", "lan2_led", "lan1_led", "",
> +		"i2c_scl", "i2c_sda", "", "DIN2",
> +		"spi?", "DIN1", "lan3_led";
> +};
> +
> +&ref {
> +	clock-frequency = <40000000>;
> +};
> +
> +&uart {
> +	status = "okay";
> +};
> +
> +&hs_uart {
> +	status = "okay";

Style nitpick: Typically we have an empty line after status.

> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pmx_uart2>;
> +};
> +
> +&spi {
> +	cs-gpios = <0>, <0>;
> +	num-cs = <2>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&jtag_disable_pins>, <&pmx_spi_cs1>;
> +
> +	status = "okay";
> +

Style nitpick: Typically, status comes first.

> +	flash@0 {
> +		compatible = "jedec,spi-nor";
> +		reg = <0>;
> +		spi-max-frequency = <25000000>;
> +
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			partition@0 {
> +				label = "u-boot";
> +				reg = <0x0 0x20000>;
> +				read-only;
> +			};
> +
> +			config: partition@20000 {
> +				label = "config";
> +				reg = <0x20000 0x10000>;
> +				read-only;
> +			};
> +
> +			art: partition@30000 {
> +				label = "art";
> +				reg = <0x30000 0x10000>;
> +				read-only;
> +			};
> +
> +			partition@40000 {
> +				label = "firmware";
> +				reg = <0x40000 0xf30000>;
> +				compatible = "tplink,firmware";
> +			};
> +
> +			partition@f70000 {
> +				label = "event-log";
> +				reg = <0xf70000 0x80000>;
> +			};

There are 0x10000 left empty at the end?
Is this writeable by intention?

> +		};
> +	};
> +
> +	microsd@1 {
> +		compatible = "mmc-spi-slot";
> +		spi-max-frequency = <25000000>;
> +		reg = <1>;
> +		voltage-ranges = <3200 3400>;
> +		broken-cd;
> +		status = "disabled";
> +	};
> +};
> +
> +&usb {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	status = "okay";
> +
> +	port@1 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <1>;
> +
> +		hub_port1: port@1 { // user USB port
> +			compatible = "usb-a-connector";
> +			reg = <1>;
> +		};
> +
> +		hub_port2: port@2 { // N/C
> +			reg = <2>;
> +		};
> +
> +		hub_port3: port@3 { // Cypress CDC-ACM serial (RS-232 D-
> Sub9)
> +			reg = <3>;
> +		};
> +
> +		hub_port4: port@4 { // Quectel EC-25 modem
> +			reg = <4>;
> +			vbus-supply = <&reg_usb_modem_vbus>;
> +		};
> +	};
> +};
> +
> +&usb_phy {
> +	status = "okay";
> +};
> +
> +&wmac {
> +	status = "okay";
> +
> +	mtd-cal-data = <&art 0x1000>;
> +	mtd-mac-address = <&config 0x0>;
> +	mtd-mac-address-increment = <2>;

No other addresses available in flash? art 0x1002 not usable?

> +};
> +
> +&eth1 {
> +	status = "okay";
> +
> +	mtd-mac-address = <&config 0x0>;
> +
> +	gmac-config {
> +		device = <&gmac>;
> +		switch-phy-swap = <0>;
> +		switch-only-mode = <1>;
> +	};
> +};
> +
> +&eth0 {
> +	status = "okay";
> +
> +	phy-handle = <&swphy4>;
> +
> +	mtd-mac-address = <&config 0x0>;
> +	mtd-mac-address-increment = <1>;
> +};
> +
> +&builtin_switch {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pmx_leds_switch>;
> +};
> +
> +&pinmux {
> +	pmx_spi_cs1: pinmux_spi_cs1 {
> +		pinctrl-single,bits = <0x0 0x07000000 0xff000000>;
> +	};
> +
> +	pmx_led_spi_gpio: pinmux_led_spi_gpio {
> +		pinctrl-single,bits = <0x4 0x0 0xff>,
> +					<0xc 0x0 0xff>,
> +					<0x14 0x0 0xff>;
> +	};
> +
> +	pmx_leds_switch: pinmux_leds_switch {
> +		pinctrl-single,bits =  <0x0 0x00002d00 0x0000ff00>,
> +					<0xc 0x002c2b00 0x00ffff00>,
> +					<0x14 0x002a0000 0x00ff0000>;
> +	};
> +
> +	pmx_uart2: pinmux_uart2 {
> +		pinctrl-single,bits = <0x10 0x4f000000 0xff000000>,
> +				<0x3c 0x000b0000 0x00ff0000>;
> +	};
> +};
> diff --git a/target/linux/ath79/generic/base-files/etc/board.d/02_network
> b/target/linux/ath79/generic/base-files/etc/board.d/02_network
> index 4630cf8447..4b75dc0359 100755
> --- a/target/linux/ath79/generic/base-files/etc/board.d/02_network
> +++ b/target/linux/ath79/generic/base-files/etc/board.d/02_network
> @@ -210,6 +210,11 @@ ath79_setup_interfaces()
>  		ucidef_add_switch "switch0" \
>  			"0@eth0" "1:lan" "2:lan" "3:wan"
>  		;;
> +	tlt,rut955)
> +		ucidef_set_interface_wan "eth1"
> +		ucidef_add_switch "switch0" \
> +			"0@eth0" "2:lan:3" "3:lan:2" "4:lan:1"
> +		;;
>  	tplink,archer-a7-v5|\
>  	tplink,archer-c6-v2|\
>  	tplink,archer-c6-v2-us|\
> diff --git a/target/linux/ath79/image/generic.mk
> b/target/linux/ath79/image/generic.mk
> index 1bc7b2d68e..ab11120da8 100644
> --- a/target/linux/ath79/image/generic.mk
> +++ b/target/linux/ath79/image/generic.mk
> @@ -36,6 +36,11 @@ define Build/addpattern
>  	-mv "$@.new" "$@"
>  endef
> 
> +define Build/append-md5sum-bin
> +	$(STAGING_DIR_HOST)/bin/mkhash md5 $@ | sed 's/../\\\\x&/g' |\
> +		xargs echo -ne >> $@
> +endef
> +
>  define Build/cybertan-trx
>  	@echo -n '' > $@-empty.bin
>  	-$(STAGING_DIR_HOST)/bin/trx -o $@.new \ @@ -73,6 +78,17 @@
> define Build/pisen_wmb001n-factory
>    rm -rf "$@.tmp"
>  endef
> 
> +define Build/teltonika-fw-fake-checksum
> +	# Teltonika U-Boot web based firmware upgrade/recovery routine
> compares
> +	# 16 bytes from md5sum1[16] field in TP-Link v1 header (offset: 76
> bytes
> +	# from begin of the firmware file) with 16 bytes stored just before
> +	# 0xdeadc0de marker. Values are only compared, MD5 sum is not
> verified.
> +	let \
> +		offs="$$(stat -c%s $@) - 20"; \
> +		dd if=$@ bs=1 count=16 skip=76 |\
> +		dd of=$@ bs=1 count=16 seek=$$offs conv=notrunc endef
> +
>  define Device/seama
>    KERNEL := kernel-bin | append-dtb | relocate-kernel | lzma
>    KERNEL_INITRAMFS := $$(KERNEL) | seama @@ -1044,6 +1060,27 @@
> define Device/sitecom_wlr-7100  endef  TARGET_DEVICES += sitecom_wlr-
> 7100
> 
> +define Device/tlt_rut955
> +  SOC := ar9344
> +  DEVICE_TITLE := Teltonika RUT955

Please use newer syntax:

DEVICE_VENDOR := Teltonika
DEVICE_MODEL := RUT955

> +  DEVICE_PACKAGES := kmod-usb2 kmod-usb-acm  kmod-usb-net-qmi-
> wwan

Double space after -acm; I'd like a line break here.

> +kmod-usb-serial-option kmod-hwmon-mcp3021 uqmi -uboot-envtools
> +  IMAGE_SIZE := 15552k
> +  TPLINK_HWID := 0x35000001
> +  TPLINK_HWREV := 0x1
> +  TPLINK_HEADER_VERSION := 1
> +  KERNEL := kernel-bin | append-dtb | lzma | tplink-v1-header
> +  KERNEL_INITRAMFS := kernel-bin | append-dtb | lzma | uImage lzma
> +  IMAGES := sysupgrade.bin factory.bin

Could also use "IMAGES += factory.bin" here.

Best

Adrian

> +  IMAGE/factory.bin := append-kernel | pad-to $$$$(BLOCKSIZE) | append-
> rootfs |\
> +	pad-rootfs | teltonika-fw-fake-checksum | append-string master |\
> +	append-md5sum-bin | check-size $$$$(IMAGE_SIZE)
> +  IMAGE/sysupgrade.bin := append-kernel | pad-to $$$$(BLOCKSIZE) |\
> +	append-rootfs | pad-rootfs | append-metadata |\
> +	check-size $$$$(IMAGE_SIZE)
> +  SUPPORTED_DEVICES += rut900
> +endef
> +TARGET_DEVICES += tlt_rut955
> +
>  define Device/trendnet_tew-823dru
>    SOC := qca9558
>    DEVICE_VENDOR := Trendnet
> --
> 2.25.0
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Hi Daniel, Adrian,

See my comments inline.

On 02.02.2020 17:41, Daniel Golle wrote:
> The Teltonika RUT955 is an industrial 2G/3G/4G WiFi router with
> various additional inputs and outputs.
> 
> Specification:
> 
> - 550/400/200 MHz (CPU/DDR/AHB)
> - 128 MB of RAM (DDR2)
> - 16 MB of FLASH (SPI NOR)
> - 4x 10/100 Mbps Ethernet, with passive PoE support on LAN1
> - 2T2R 2,4 GHz (AR9344)

Have you tested RF sensitivity? I remember external GPIO-driven LNA in 
RUT900 has to be defined, otherwise the radio was almost deaf.

> - built-in 3G module (example: Qutectel EC-25EU)

Typo: s/Qutectel/Quectel

I think the only 3G-based one in whole series is RUT900.
EC25E is a 4G one.

> - RS232 on D-Sub9 port (Cypress ACM via USB, /dev/ttyACM0)
> - RS422/RS485 (AR934x high speed UART, /dev/ttyATH1)
> - analog 0-9V input (MCP3221)
> - various digital inputs and outputs incl. a relay
> - 2x miniSIM slot (can be swapped via GPIO)
> - 2x RP-SMA/F (Wi-Fi), 2x SMA/F (3G), 1x GPS
> - 2x 74HC595 shift registers providing 16 GPOs
> - 12x LED (4 are driven by AR9344, 7 by 74HC595)
> - 1x button (reset)
> - DC jack for main power input (9-30 V)
> - debugging UART available on PCB edge connector
> 
> Serial console (/dev/ttyS0) pinout:
> 
> - RX: pin1 (square) on top side of the main PCB (AR9344 is on top)
> - TX: pin1 (square) on bottom side
> 
> Flash instruction:
> 
> Vendor firmware is based on OpenWrt CC release. Use the "factory" image
> directly in GUI (make sure to uncheck "keep settings") or in U-Boot web
> based recovery. To avoid any problems, make sure to first update vendor
> firmware to latest version - "factory" image was successfully tested on
> device running "RUT9XX_R_00.06.051" firmware and U-Boot "3.0.2".
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>   target/linux/ath79/dts/ar9344_tlt_rut955.dts  | 301 ++++++++++++++++++
>   .../generic/base-files/etc/board.d/02_network |   5 +
>   target/linux/ath79/image/generic.mk           |  37 +++
>   3 files changed, 343 insertions(+)
>   create mode 100644 target/linux/ath79/dts/ar9344_tlt_rut955.dts
> 
> diff --git a/target/linux/ath79/dts/ar9344_tlt_rut955.dts b/target/linux/ath79/dts/ar9344_tlt_rut955.dts
> new file mode 100644
> index 0000000000..06d18f8d26
> --- /dev/null
> +++ b/target/linux/ath79/dts/ar9344_tlt_rut955.dts
> @@ -0,0 +1,301 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +
> +#include "ar9344.dtsi"
> +
> +/ {
> +	model = "Teltonika RUT955";
> +	compatible = "tlt,rut955", "qca,ar9344";

I know that Teltonika uses this prefix in their software but maybe as 
Adrian suggested, full name would fit better here? I'm fine with both.

At least, until it's in [0], we can use whatever fits.

> +
> +	aliases {
> +		serial0 = &uart;
> +		serial1 = &hs_uart;
> +		led-boot = &led_system_green;
> +		led-failsafe = &led_system_red;
> +		led-running = &led_system_green;
> +		led-upgrade = &led_system_red;
> +		label-mac-device = &eth1;
> +	};
> +
> +	i2c {
> +		compatible = "i2c-gpio";
> +		scl-gpios = <&gpio 16 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> +		sda-gpios = <&gpio 17 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		hwmon@4d {
> +			compatible = "microchip,mcp3221";
> +			reg = <0x4d>;
> +		};
> +	};
> +
> +	gpio_ext_spi {
> +		compatible = "spi-gpio";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pmx_led_spi_gpio>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		sck-gpios = <&gpio 4 GPIO_ACTIVE_HIGH>;     // 74HC595 SRCLK (Serial Clock)
> +		mosi-gpios = <&gpio 12 GPIO_ACTIVE_HIGH>;   // 74HC595 SER (Serial)
> +		cs-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;     // 74HC595 RCLK (Register Clock)
> +		num-chipselects = <1>;
> +
> +		gpio_ext: gpio_ext@0 {
> +			compatible = "fairchild,74hc595";
> +			reg = <0>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			registers-number = <2>;
> +			spi-max-frequency = <10000000>;
> +			gpio-line-names = "signal_bar0", "signal_bar1", "signal_bar2", "signal_bar3",
> +				"signal_bar4", "status_red", "status_green", "sim_sel",
> +				"", "relay", "modem_vbus", "modem_rst",
> +				"", "", "", "";

I don't think we use this property anywhere else in ath79 (or even other 
DTS-based targets?) so far and AFAIK it doesn't have any usage before we 
start using new char dev for GPIOs (libgpiod).

Anyway, wouldn't it be more readable to at least add "led" to names of 
lines driving LEDs?

Also, maybe it would make sense to provide access to SIM select and 
relay lines in user-space (with gpio-export in DTS or in UCI)?

> +		};
> +	};
> +
> +	reg_usb_modem_vbus: reg_usb_modem_vbus {
> +		compatible = "regulator-fixed";
> +		regulator-name = "usb_modem_vbus";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		gpio = <&gpio_ext 10 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +		regulator-always-on;
> +		regulator-boot-on;
> +	};

IIRC, with this approach, user would need to unload USB host driver to 
be able to disable VBUS (and thus 'hard-reset' modem). I would consider 
using "gpio-export" to give user control over that, especially if it 
controls power _only_ for the modem.

Also, see (long) discussion about this subject here: [1]

> +
> +	leds {
> +		compatible = "gpio-leds";

Any reason to use h/w control on WAN/LANs LEDs instead of defining them 
as gpio-leds and thus allow users to have control on them? I suppose we 
don't have a general rule here, though.

> +
> +		signal0 {

Could you keep it the same as in RUT900? Count from 1, not 0?

> +			label = "rut955:green:signal1";
> +			gpios = <&gpio_ext 0 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +		};
> +
> +		signal1 {
> +			label = "rut955:green:signal2";
> +			gpios = <&gpio_ext 1 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +		};
> +
> +		signal2 {
> +			label = "rut955:green:signal3";
> +			gpios = <&gpio_ext 2 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +		};
> +
> +		signal3 {
> +			label = "rut955:green:signal4";
> +			gpios = <&gpio_ext 3 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +		};
> +
> +		signal4 {
> +			label = "rut955:green:signal5";
> +			gpios = <&gpio_ext 4 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +		};
> +
> +		led_system_red: systemred {
> +			label = "rut955:green:red";
> +			gpios = <&gpio_ext 5 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +		};
> +
> +		led_system_green: systemgreen {
> +			label = "rut955:green:system";
> +			gpios = <&gpio_ext 6 GPIO_ACTIVE_HIGH>;
> +			default-state = "on";
> +		};
> +	};
> +
> +	keys {
> +		compatible = "gpio-keys";
> +
> +		reset {

Please, add here also label = "reset". I think this property is optional 
but without it, at least in debugfs (/sys/kernel/debug/gpio), it will 
get meaningless name from parent node's compat string:

gpio-15  (                    |gpio-keys           ) in  lo IRQ

> +			linux,code = <KEY_RESTART>;
> +			gpios = <&gpio 15 GPIO_ACTIVE_HIGH>;

Are you sure it's active high? In RUT900 it's set to active low: [2].

> +			debounce-interval = <60>;
> +		};
> +	};
> +};
> +
> +&gpio {
> +	gpio-line-names = "", "wan_led", "input", "mmc_cs",
> +		"leds_sck", "", "", "",
> +		"", "", "", "",
> +		"leds_mosi", "lan2_led", "lan1_led", "",
> +		"i2c_scl", "i2c_sda", "", "DIN2",
> +		"spi?", "DIN1", "lan3_led";

"spi?"?

I'm wondering if we really need this 'gpio-line-names' now?

> +};
> +
> +&ref {
> +	clock-frequency = <40000000>;
> +};
> +
> +&uart {
> +	status = "okay";
> +};
> +
> +&hs_uart {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pmx_uart2>;
> +};
> +
> +&spi {
> +	cs-gpios = <0>, <0>;
> +	num-cs = <2>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&jtag_disable_pins>, <&pmx_spi_cs1>;
> +
> +	status = "okay";
> +
> +	flash@0 {
> +		compatible = "jedec,spi-nor";
> +		reg = <0>;
> +		spi-max-frequency = <25000000>;
> +
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			partition@0 {
> +				label = "u-boot";
> +				reg = <0x0 0x20000>;
> +				read-only;
> +			};
> +
> +			config: partition@20000 {
> +				label = "config";
> +				reg = <0x20000 0x10000>;
> +				read-only;
> +			};
> +
> +			art: partition@30000 {
> +				label = "art";
> +				reg = <0x30000 0x10000>;
> +				read-only;
> +			};
> +
> +			partition@40000 {
> +				label = "firmware";
> +				reg = <0x40000 0xf30000>;
> +				compatible = "tplink,firmware";
> +			};
> +
> +			partition@f70000 {
> +				label = "event-log";
> +				reg = <0xf70000 0x80000>;

As Adrian wrote, this should be 0x90000 (576k, same as in RUT900).

> +			};
> +		};
> +	};
> +
> +	microsd@1 {
> +		compatible = "mmc-spi-slot";
> +		spi-max-frequency = <25000000>;
> +		reg = <1>;
> +		voltage-ranges = <3200 3400>;
> +		broken-cd;
> +		status = "disabled";
> +	};
> +};
> +
> +&usb {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	status = "okay";
> +
> +	port@1 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <1>;
> +
> +		hub_port1: port@1 { // user USB port

Maybe a "label" property would be better here instead of a comment?

> +			compatible = "usb-a-connector";
> +			reg = <1>;
> +		};
> +
> +		hub_port2: port@2 { // N/C
> +			reg = <2>;
> +		};

I'm not sure if not populated connectors should be mentioned in DTS?

> +
> +		hub_port3: port@3 { // Cypress CDC-ACM serial (RS-232 D-Sub9)

Label?

> +			reg = <3>;
> +		};
> +
> +		hub_port4: port@4 { // Quectel EC-25 modem
> +			reg = <4>;
> +			vbus-supply = <&reg_usb_modem_vbus>;
> +		};

Also "hub_portX" aren't referenced anywhere, you can drop these labels.

> +	};
> +};
> +
> +&usb_phy {
> +	status = "okay";
> +};
> +
> +&wmac {
> +	status = "okay";
> +
> +	mtd-cal-data = <&art 0x1000>;
> +	mtd-mac-address = <&config 0x0>;
> +	mtd-mac-address-increment = <2>;
> +};
> +
> +&eth1 {

Why did you change nodes order here (eth1 before eth0)?

> +	status = "okay";
> +
> +	mtd-mac-address = <&config 0x0>;
> +
> +	gmac-config {
> +		device = <&gmac>;
> +		switch-phy-swap = <0>;
> +		switch-only-mode = <1>;

Have you tested that network configuration? Enabling 'switch-only-mode' 
doesn't make sense as it will make all internal switch PHYs connect to 
GMAC1 only.

> +	};
> +};
> +
> +&eth0 {
> +	status = "okay";
> +
> +	phy-handle = <&swphy4>;
> +
> +	mtd-mac-address = <&config 0x0>;
> +	mtd-mac-address-increment = <1>;
> +};
> +
> +&builtin_switch {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pmx_leds_switch>;
> +};
> +
> +&pinmux {
> +	pmx_spi_cs1: pinmux_spi_cs1 {
> +		pinctrl-single,bits = <0x0 0x07000000 0xff000000>;
> +	};
> +
> +	pmx_led_spi_gpio: pinmux_led_spi_gpio {
> +		pinctrl-single,bits = <0x4 0x0 0xff>,
> +					<0xc 0x0 0xff>,
> +					<0x14 0x0 0xff>;
> +	};
> +
> +	pmx_leds_switch: pinmux_leds_switch {
> +		pinctrl-single,bits =  <0x0 0x00002d00 0x0000ff00>,
> +					<0xc 0x002c2b00 0x00ffff00>,
> +					<0x14 0x002a0000 0x00ff0000>;
> +	};
> +
> +	pmx_uart2: pinmux_uart2 {
> +		pinctrl-single,bits = <0x10 0x4f000000 0xff000000>,
> +				<0x3c 0x000b0000 0x00ff0000>;

I'm wondering, are these pins also configured to output and input in 
GPIO_OE and GPIO_IN registers?

> +	};
> +};
> diff --git a/target/linux/ath79/generic/base-files/etc/board.d/02_network b/target/linux/ath79/generic/base-files/etc/board.d/02_network
> index 4630cf8447..4b75dc0359 100755
> --- a/target/linux/ath79/generic/base-files/etc/board.d/02_network
> +++ b/target/linux/ath79/generic/base-files/etc/board.d/02_network
> @@ -210,6 +210,11 @@ ath79_setup_interfaces()
>   		ucidef_add_switch "switch0" \
>   			"0@eth0" "1:lan" "2:lan" "3:wan"
>   		;;
> +	tlt,rut955)
> +		ucidef_set_interface_wan "eth1"
> +		ucidef_add_switch "switch0" \
> +			"0@eth0" "2:lan:3" "3:lan:2" "4:lan:1"
> +		;;
>   	tplink,archer-a7-v5|\
>   	tplink,archer-c6-v2|\
>   	tplink,archer-c6-v2-us|\
> diff --git a/target/linux/ath79/image/generic.mk b/target/linux/ath79/image/generic.mk
> index 1bc7b2d68e..ab11120da8 100644
> --- a/target/linux/ath79/image/generic.mk
> +++ b/target/linux/ath79/image/generic.mk
> @@ -36,6 +36,11 @@ define Build/addpattern
>   	-mv "$@.new" "$@"
>   endef
>   
> +define Build/append-md5sum-bin
> +	$(STAGING_DIR_HOST)/bin/mkhash md5 $@ | sed 's/../\\\\x&/g' |\
> +		xargs echo -ne >> $@
> +endef
> +
>   define Build/cybertan-trx
>   	@echo -n '' > $@-empty.bin
>   	-$(STAGING_DIR_HOST)/bin/trx -o $@.new \
> @@ -73,6 +78,17 @@ define Build/pisen_wmb001n-factory
>     rm -rf "$@.tmp"
>   endef
>   
> +define Build/teltonika-fw-fake-checksum
> +	# Teltonika U-Boot web based firmware upgrade/recovery routine compares
> +	# 16 bytes from md5sum1[16] field in TP-Link v1 header (offset: 76 bytes
> +	# from begin of the firmware file) with 16 bytes stored just before
> +	# 0xdeadc0de marker. Values are only compared, MD5 sum is not verified.
> +	let \
> +		offs="$$(stat -c%s $@) - 20"; \
> +		dd if=$@ bs=1 count=16 skip=76 |\
> +		dd of=$@ bs=1 count=16 seek=$$offs conv=notrunc
> +endef
> +
>   define Device/seama
>     KERNEL := kernel-bin | append-dtb | relocate-kernel | lzma
>     KERNEL_INITRAMFS := $$(KERNEL) | seama
> @@ -1044,6 +1060,27 @@ define Device/sitecom_wlr-7100
>   endef
>   TARGET_DEVICES += sitecom_wlr-7100
>   
> +define Device/tlt_rut955
> +  SOC := ar9344
> +  DEVICE_TITLE := Teltonika RUT955
> +  DEVICE_PACKAGES := kmod-usb2 kmod-usb-acm  kmod-usb-net-qmi-wwan kmod-usb-serial-option kmod-hwmon-mcp3021 uqmi -uboot-envtools
> +  IMAGE_SIZE := 15552k
> +  TPLINK_HWID := 0x35000001
> +  TPLINK_HWREV := 0x1
> +  TPLINK_HEADER_VERSION := 1
> +  KERNEL := kernel-bin | append-dtb | lzma | tplink-v1-header
> +  KERNEL_INITRAMFS := kernel-bin | append-dtb | lzma | uImage lzma
> +  IMAGES := sysupgrade.bin factory.bin
> +  IMAGE/factory.bin := append-kernel | pad-to $$$$(BLOCKSIZE) | append-rootfs |\
> +	pad-rootfs | teltonika-fw-fake-checksum | append-string master |\
> +	append-md5sum-bin | check-size $$$$(IMAGE_SIZE)
> +  IMAGE/sysupgrade.bin := append-kernel | pad-to $$$$(BLOCKSIZE) |\
> +	append-rootfs | pad-rootfs | append-metadata |\
> +	check-size $$$$(IMAGE_SIZE)
> +  SUPPORTED_DEVICES += rut900

Please, drop this line.
I will add support for RUT900 with a separate patch.

> +endef
> +TARGET_DEVICES += tlt_rut955
> +
>   define Device/trendnet_tew-823dru
>     SOC := qca9558
>     DEVICE_VENDOR := Trendnet
> 

[0] 
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/vendor-prefixes.yaml

[1] 
https://www.mail-archive.com/openwrt-devel@lists.openwrt.org/msg49220.html

[2] 
https://github.com/openwrt/openwrt/blob/master/target/linux/ar71xx/files/arch/mips/ath79/mach-rut9xx.c#L111

On Mon, Feb 03, 2020 at 11:51:29PM +0100, Piotr Dymacz wrote:
> Hi Daniel, Adrian,
> 
> See my comments inline.
> 
> On 02.02.2020 17:41, Daniel Golle wrote:
> > The Teltonika RUT955 is an industrial 2G/3G/4G WiFi router with
> > various additional inputs and outputs.
> > 
> > Specification:
> > 
> > - 550/400/200 MHz (CPU/DDR/AHB)
> > - 128 MB of RAM (DDR2)
> > - 16 MB of FLASH (SPI NOR)
> > - 4x 10/100 Mbps Ethernet, with passive PoE support on LAN1
> > - 2T2R 2,4 GHz (AR9344)
> 
> Have you tested RF sensitivity? I remember external GPIO-driven LNA in
> RUT900 has to be defined, otherwise the radio was almost deaf.
> 
> > - built-in 3G module (example: Qutectel EC-25EU)
> 
> Typo: s/Qutectel/Quectel
> 
> I think the only 3G-based one in whole series is RUT900.
> EC25E is a 4G one.
> 
> > - RS232 on D-Sub9 port (Cypress ACM via USB, /dev/ttyACM0)
> > - RS422/RS485 (AR934x high speed UART, /dev/ttyATH1)
> > - analog 0-9V input (MCP3221)
> > - various digital inputs and outputs incl. a relay
> > - 2x miniSIM slot (can be swapped via GPIO)
> > - 2x RP-SMA/F (Wi-Fi), 2x SMA/F (3G), 1x GPS
> > - 2x 74HC595 shift registers providing 16 GPOs
> > - 12x LED (4 are driven by AR9344, 7 by 74HC595)
> > - 1x button (reset)
> > - DC jack for main power input (9-30 V)
> > - debugging UART available on PCB edge connector
> > 
> > Serial console (/dev/ttyS0) pinout:
> > 
> > - RX: pin1 (square) on top side of the main PCB (AR9344 is on top)
> > - TX: pin1 (square) on bottom side
> > 
> > Flash instruction:
> > 
> > Vendor firmware is based on OpenWrt CC release. Use the "factory" image
> > directly in GUI (make sure to uncheck "keep settings") or in U-Boot web
> > based recovery. To avoid any problems, make sure to first update vendor
> > firmware to latest version - "factory" image was successfully tested on
> > device running "RUT9XX_R_00.06.051" firmware and U-Boot "3.0.2".
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >   target/linux/ath79/dts/ar9344_tlt_rut955.dts  | 301 ++++++++++++++++++
> >   .../generic/base-files/etc/board.d/02_network |   5 +
> >   target/linux/ath79/image/generic.mk           |  37 +++
> >   3 files changed, 343 insertions(+)
> >   create mode 100644 target/linux/ath79/dts/ar9344_tlt_rut955.dts
> > 
> > diff --git a/target/linux/ath79/dts/ar9344_tlt_rut955.dts b/target/linux/ath79/dts/ar9344_tlt_rut955.dts
> > new file mode 100644
> > index 0000000000..06d18f8d26
> > --- /dev/null
> > +++ b/target/linux/ath79/dts/ar9344_tlt_rut955.dts
> > @@ -0,0 +1,301 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> > +/dts-v1/;
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/input/input.h>
> > +
> > +#include "ar9344.dtsi"
> > +
> > +/ {
> > +	model = "Teltonika RUT955";
> > +	compatible = "tlt,rut955", "qca,ar9344";
> 
> I know that Teltonika uses this prefix in their software but maybe as Adrian
> suggested, full name would fit better here? I'm fine with both.
> 
> At least, until it's in [0], we can use whatever fits.
> 
> > +
> > +	aliases {
> > +		serial0 = &uart;
> > +		serial1 = &hs_uart;
> > +		led-boot = &led_system_green;
> > +		led-failsafe = &led_system_red;
> > +		led-running = &led_system_green;
> > +		led-upgrade = &led_system_red;
> > +		label-mac-device = &eth1;
> > +	};
> > +
> > +	i2c {
> > +		compatible = "i2c-gpio";
> > +		scl-gpios = <&gpio 16 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> > +		sda-gpios = <&gpio 17 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		hwmon@4d {
> > +			compatible = "microchip,mcp3221";
> > +			reg = <0x4d>;
> > +		};
> > +	};
> > +
> > +	gpio_ext_spi {
> > +		compatible = "spi-gpio";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pmx_led_spi_gpio>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		sck-gpios = <&gpio 4 GPIO_ACTIVE_HIGH>;     // 74HC595 SRCLK (Serial Clock)
> > +		mosi-gpios = <&gpio 12 GPIO_ACTIVE_HIGH>;   // 74HC595 SER (Serial)
> > +		cs-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;     // 74HC595 RCLK (Register Clock)
> > +		num-chipselects = <1>;
> > +
> > +		gpio_ext: gpio_ext@0 {
> > +			compatible = "fairchild,74hc595";
> > +			reg = <0>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			registers-number = <2>;
> > +			spi-max-frequency = <10000000>;
> > +			gpio-line-names = "signal_bar0", "signal_bar1", "signal_bar2", "signal_bar3",
> > +				"signal_bar4", "status_red", "status_green", "sim_sel",
> > +				"", "relay", "modem_vbus", "modem_rst",
> > +				"", "", "", "";
> 
> I don't think we use this property anywhere else in ath79 (or even other
> DTS-based targets?) so far and AFAIK it doesn't have any usage before we
> start using new char dev for GPIOs (libgpiod).

My intention is to avoid deprecated kernel interfaces in favor of
using the 
> 
> Anyway, wouldn't it be more readable to at least add "led" to names of lines
> driving LEDs?

The gpio-line-names is the suggested interface of the kernel to
apply labels to GPIO lines the user can see in /sys/kernel/debug/gpio.
I agree we should use libgpiod or any other way to later on refer to
such label in UCI.

> 
> Also, maybe it would make sense to provide access to SIM select and relay
> lines in user-space (with gpio-export in DTS or in UCI)?

I'll add uci-defaults for gpio_switches as a temporary fix until we
have proper handling allowing to identify GPIO lines by name rather
than a (potentilly non-constant) number.

> 
> > +		};
> > +	};
> > +
> > +	reg_usb_modem_vbus: reg_usb_modem_vbus {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "usb_modem_vbus";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		gpio = <&gpio_ext 10 GPIO_ACTIVE_HIGH>;
> > +		enable-active-high;
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +	};
> 
> IIRC, with this approach, user would need to unload USB host driver to be
> able to disable VBUS (and thus 'hard-reset' modem). I would consider using
> "gpio-export" to give user control over that, especially if it controls
> power _only_ for the modem.

I tied the regulator to the corresponding port of the USB hub as that
seemed like the most clean thing to do.

> 
> Also, see (long) discussion about this subject here: [1]

You are right that this limits user control and I've also been involved
in that debate and 
> 
> > +
> > +	leds {
> > +		compatible = "gpio-leds";
> 
> Any reason to use h/w control on WAN/LANs LEDs instead of defining them as
> gpio-leds and thus allow users to have control on them? I suppose we don't
> have a general rule here, though.

It's what the vendor-firmware does as well and as those LEDs are right
next to the corresponding RJ-45 socket and there are plenty of
user-controlled LEDs, I thought I'd just keep it like that.

> 
> > +
> > +		signal0 {
> 
> Could you keep it the same as in RUT900? Count from 1, not 0?

Ack.

> 
> > +			label = "rut955:green:signal1";
> > +			gpios = <&gpio_ext 0 GPIO_ACTIVE_HIGH>;
> > +			default-state = "off";
> > +		};
> > +
> > +		signal1 {
> > +			label = "rut955:green:signal2";
> > +			gpios = <&gpio_ext 1 GPIO_ACTIVE_HIGH>;
> > +			default-state = "off";
> > +		};
> > +
> > +		signal2 {
> > +			label = "rut955:green:signal3";
> > +			gpios = <&gpio_ext 2 GPIO_ACTIVE_HIGH>;
> > +			default-state = "off";
> > +		};
> > +
> > +		signal3 {
> > +			label = "rut955:green:signal4";
> > +			gpios = <&gpio_ext 3 GPIO_ACTIVE_HIGH>;
> > +			default-state = "off";
> > +		};
> > +
> > +		signal4 {
> > +			label = "rut955:green:signal5";
> > +			gpios = <&gpio_ext 4 GPIO_ACTIVE_HIGH>;
> > +			default-state = "off";
> > +		};
> > +
> > +		led_system_red: systemred {
> > +			label = "rut955:green:red";
> > +			gpios = <&gpio_ext 5 GPIO_ACTIVE_HIGH>;
> > +			default-state = "off";
> > +		};
> > +
> > +		led_system_green: systemgreen {
> > +			label = "rut955:green:system";
> > +			gpios = <&gpio_ext 6 GPIO_ACTIVE_HIGH>;
> > +			default-state = "on";
> > +		};
> > +	};
> > +
> > +	keys {
> > +		compatible = "gpio-keys";
> > +
> > +		reset {
> 
> Please, add here also label = "reset". I think this property is optional but
> without it, at least in debugfs (/sys/kernel/debug/gpio), it will get
> meaningless name from parent node's compat string:
> 
> gpio-15  (                    |gpio-keys           ) in  lo IRQ
> 
> > +			linux,code = <KEY_RESTART>;
> > +			gpios = <&gpio 15 GPIO_ACTIVE_HIGH>;
> 
> Are you sure it's active high? In RUT900 it's set to active low: [2].

It was working, but that doesn't mean it was right.
I'll do another round of testing to figure it out.

> 
> > +			debounce-interval = <60>;
> > +		};
> > +	};
> > +};
> > +
> > +&gpio {
> > +	gpio-line-names = "", "wan_led", "input", "mmc_cs",
> > +		"leds_sck", "", "", "",
> > +		"", "", "", "",
> > +		"leds_mosi", "lan2_led", "lan1_led", "",
> > +		"i2c_scl", "i2c_sda", "", "DIN2",
> > +		"spi?", "DIN1", "lan3_led";
> 
> "spi?"?
> 
> I'm wondering if we really need this 'gpio-line-names' now?

They shows up in /sys/kernel/debug/gpio, so it is useful imho.


> 
> > +};
> > +
> > +&ref {
> > +	clock-frequency = <40000000>;
> > +};
> > +
> > +&uart {
> > +	status = "okay";
> > +};
> > +
> > +&hs_uart {
> > +	status = "okay";
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pmx_uart2>;
> > +};
> > +
> > +&spi {
> > +	cs-gpios = <0>, <0>;
> > +	num-cs = <2>;
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&jtag_disable_pins>, <&pmx_spi_cs1>;
> > +
> > +	status = "okay";
> > +
> > +	flash@0 {
> > +		compatible = "jedec,spi-nor";
> > +		reg = <0>;
> > +		spi-max-frequency = <25000000>;
> > +
> > +		partitions {
> > +			compatible = "fixed-partitions";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +
> > +			partition@0 {
> > +				label = "u-boot";
> > +				reg = <0x0 0x20000>;
> > +				read-only;
> > +			};
> > +
> > +			config: partition@20000 {
> > +				label = "config";
> > +				reg = <0x20000 0x10000>;
> > +				read-only;
> > +			};
> > +
> > +			art: partition@30000 {
> > +				label = "art";
> > +				reg = <0x30000 0x10000>;
> > +				read-only;
> > +			};
> > +
> > +			partition@40000 {
> > +				label = "firmware";
> > +				reg = <0x40000 0xf30000>;
> > +				compatible = "tplink,firmware";
> > +			};
> > +
> > +			partition@f70000 {
> > +				label = "event-log";
> > +				reg = <0xf70000 0x80000>;
> 
> As Adrian wrote, this should be 0x90000 (576k, same as in RUT900).

Right, my bad.

> 
> > +			};
> > +		};
> > +	};
> > +
> > +	microsd@1 {
> > +		compatible = "mmc-spi-slot";
> > +		spi-max-frequency = <25000000>;
> > +		reg = <1>;
> > +		voltage-ranges = <3200 3400>;
> > +		broken-cd;
> > +		status = "disabled";
> > +	};
> > +};
> > +
> > +&usb {
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	status = "okay";
> > +
> > +	port@1 {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		reg = <1>;
> > +
> > +		hub_port1: port@1 { // user USB port
> 
> Maybe a "label" property would be better here instead of a comment?

Ok.

> 
> > +			compatible = "usb-a-connector";
> > +			reg = <1>;
> > +		};
> > +
> > +		hub_port2: port@2 { // N/C
> > +			reg = <2>;
> > +		};
> 
> I'm not sure if not populated connectors should be mentioned in DTS?
> 
> > +
> > +		hub_port3: port@3 { // Cypress CDC-ACM serial (RS-232 D-Sub9)
> 
> Label?
> 
> > +			reg = <3>;
> > +		};
> > +
> > +		hub_port4: port@4 { // Quectel EC-25 modem
> > +			reg = <4>;
> > +			vbus-supply = <&reg_usb_modem_vbus>;
> > +		};
> 
> Also "hub_portX" aren't referenced anywhere, you can drop these labels.
> 
> > +	};
> > +};
> > +
> > +&usb_phy {
> > +	status = "okay";
> > +};
> > +
> > +&wmac {
> > +	status = "okay";
> > +
> > +	mtd-cal-data = <&art 0x1000>;
> > +	mtd-mac-address = <&config 0x0>;
> > +	mtd-mac-address-increment = <2>;
> > +};
> > +
> > +&eth1 {
> 
> Why did you change nodes order here (eth1 before eth0)?
> 
> > +	status = "okay";
> > +
> > +	mtd-mac-address = <&config 0x0>;
> > +
> > +	gmac-config {
> > +		device = <&gmac>;
> > +		switch-phy-swap = <0>;
> > +		switch-only-mode = <1>;
> 
> Have you tested that network configuration? Enabling 'switch-only-mode'
> doesn't make sense as it will make all internal switch PHYs connect to GMAC1
> only.

I copied this from similar (TP-Link) devices, all ports (incl. WAN)
were working. I'll give it a try with those attributes removed to see
if they are actually needed.

> 
> > +	};
> > +};
> > +
> > +&eth0 {
> > +	status = "okay";
> > +
> > +	phy-handle = <&swphy4>;
> > +
> > +	mtd-mac-address = <&config 0x0>;
> > +	mtd-mac-address-increment = <1>;
> > +};
> > +
> > +&builtin_switch {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pmx_leds_switch>;
> > +};
> > +
> > +&pinmux {
> > +	pmx_spi_cs1: pinmux_spi_cs1 {
> > +		pinctrl-single,bits = <0x0 0x07000000 0xff000000>;
> > +	};
> > +
> > +	pmx_led_spi_gpio: pinmux_led_spi_gpio {
> > +		pinctrl-single,bits = <0x4 0x0 0xff>,
> > +					<0xc 0x0 0xff>,
> > +					<0x14 0x0 0xff>;
> > +	};
> > +
> > +	pmx_leds_switch: pinmux_leds_switch {
> > +		pinctrl-single,bits =  <0x0 0x00002d00 0x0000ff00>,
> > +					<0xc 0x002c2b00 0x00ffff00>,
> > +					<0x14 0x002a0000 0x00ff0000>;
> > +	};
> > +
> > +	pmx_uart2: pinmux_uart2 {
> > +		pinctrl-single,bits = <0x10 0x4f000000 0xff000000>,
> > +				<0x3c 0x000b0000 0x00ff0000>;
> 
> I'm wondering, are these pins also configured to output and input in GPIO_OE
> and GPIO_IN registers?

GPIO_OE and GPIO_IN are not range of the pinmux which starts at
0x1804002c. Hence it cannot be set using that pinmux, but it's possible
by gpio-hogs. As the RS485 serial right now still doesn't work
(supposedly for lack-of-clock-signal-reasons) it's hard to test if
anything additional is needed at this point.

> 
> > +	};
> > +};
> > diff --git a/target/linux/ath79/generic/base-files/etc/board.d/02_network b/target/linux/ath79/generic/base-files/etc/board.d/02_network
> > index 4630cf8447..4b75dc0359 100755
> > --- a/target/linux/ath79/generic/base-files/etc/board.d/02_network
> > +++ b/target/linux/ath79/generic/base-files/etc/board.d/02_network
> > @@ -210,6 +210,11 @@ ath79_setup_interfaces()
> >   		ucidef_add_switch "switch0" \
> >   			"0@eth0" "1:lan" "2:lan" "3:wan"
> >   		;;
> > +	tlt,rut955)
> > +		ucidef_set_interface_wan "eth1"
> > +		ucidef_add_switch "switch0" \
> > +			"0@eth0" "2:lan:3" "3:lan:2" "4:lan:1"
> > +		;;
> >   	tplink,archer-a7-v5|\
> >   	tplink,archer-c6-v2|\
> >   	tplink,archer-c6-v2-us|\
> > diff --git a/target/linux/ath79/image/generic.mk b/target/linux/ath79/image/generic.mk
> > index 1bc7b2d68e..ab11120da8 100644
> > --- a/target/linux/ath79/image/generic.mk
> > +++ b/target/linux/ath79/image/generic.mk
> > @@ -36,6 +36,11 @@ define Build/addpattern
> >   	-mv "$@.new" "$@"
> >   endef
> > +define Build/append-md5sum-bin
> > +	$(STAGING_DIR_HOST)/bin/mkhash md5 $@ | sed 's/../\\\\x&/g' |\
> > +		xargs echo -ne >> $@
> > +endef
> > +
> >   define Build/cybertan-trx
> >   	@echo -n '' > $@-empty.bin
> >   	-$(STAGING_DIR_HOST)/bin/trx -o $@.new \
> > @@ -73,6 +78,17 @@ define Build/pisen_wmb001n-factory
> >     rm -rf "$@.tmp"
> >   endef
> > +define Build/teltonika-fw-fake-checksum
> > +	# Teltonika U-Boot web based firmware upgrade/recovery routine compares
> > +	# 16 bytes from md5sum1[16] field in TP-Link v1 header (offset: 76 bytes
> > +	# from begin of the firmware file) with 16 bytes stored just before
> > +	# 0xdeadc0de marker. Values are only compared, MD5 sum is not verified.
> > +	let \
> > +		offs="$$(stat -c%s $@) - 20"; \
> > +		dd if=$@ bs=1 count=16 skip=76 |\
> > +		dd of=$@ bs=1 count=16 seek=$$offs conv=notrunc
> > +endef
> > +
> >   define Device/seama
> >     KERNEL := kernel-bin | append-dtb | relocate-kernel | lzma
> >     KERNEL_INITRAMFS := $$(KERNEL) | seama
> > @@ -1044,6 +1060,27 @@ define Device/sitecom_wlr-7100
> >   endef
> >   TARGET_DEVICES += sitecom_wlr-7100
> > +define Device/tlt_rut955
> > +  SOC := ar9344
> > +  DEVICE_TITLE := Teltonika RUT955
> > +  DEVICE_PACKAGES := kmod-usb2 kmod-usb-acm  kmod-usb-net-qmi-wwan kmod-usb-serial-option kmod-hwmon-mcp3021 uqmi -uboot-envtools
> > +  IMAGE_SIZE := 15552k
> > +  TPLINK_HWID := 0x35000001
> > +  TPLINK_HWREV := 0x1
> > +  TPLINK_HEADER_VERSION := 1
> > +  KERNEL := kernel-bin | append-dtb | lzma | tplink-v1-header
> > +  KERNEL_INITRAMFS := kernel-bin | append-dtb | lzma | uImage lzma
> > +  IMAGES := sysupgrade.bin factory.bin
> > +  IMAGE/factory.bin := append-kernel | pad-to $$$$(BLOCKSIZE) | append-rootfs |\
> > +	pad-rootfs | teltonika-fw-fake-checksum | append-string master |\
> > +	append-md5sum-bin | check-size $$$$(IMAGE_SIZE)
> > +  IMAGE/sysupgrade.bin := append-kernel | pad-to $$$$(BLOCKSIZE) |\
> > +	append-rootfs | pad-rootfs | append-metadata |\
> > +	check-size $$$$(IMAGE_SIZE)
> > +  SUPPORTED_DEVICES += rut900
> 
> Please, drop this line.
> I will add support for RUT900 with a separate patch.

Ack.

> 
> > +endef
> > +TARGET_DEVICES += tlt_rut955
> > +
> >   define Device/trendnet_tew-823dru
> >     SOC := qca9558
> >     DEVICE_VENDOR := Trendnet
> > 
> 
> [0] https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/vendor-prefixes.yaml
> 
> [1]
> https://www.mail-archive.com/openwrt-devel@lists.openwrt.org/msg49220.html
> 
> [2] https://github.com/openwrt/openwrt/blob/master/target/linux/ar71xx/files/arch/mips/ath79/mach-rut9xx.c#L111
> 
> -- 
> Cheers,
> Piotr
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

On Sun, Feb 02, 2020 at 06:15:41PM +0100, mail@adrianschmutzler.de wrote:
> Hi Daniel,
> 
> several comments inline below.
> > +	compatible = "tlt,rut955", "qca,ar9344";
> 
> I would prefer having the full vendor name here (teltonika,rut955), as we tried to avoid abbreviations in names for ath79 wherever possible (with the exception of ubnt).

Ack.

> > ...
> > +	leds {
> > +		compatible = "gpio-leds";
> > +
> > +		signal0 {
> > +			label = "rut955:green:signal1";
> 
> I'd prefer having the same numbering for node and label here (signal0<>signal1).

Ack.
> > ...
> > +
> > +		led_system_red: systemred {
> 
> Please add an underscore: system_red
Ack.

> 
> > +			label = "rut955:green:red";
> 
> This should be rut955:red:system ?

Oh yes. Ack.

> > ...
> > +			gpios = <&gpio_ext 5 GPIO_ACTIVE_HIGH>;
> > +			default-state = "off";
> 
> Default-state "off" is default, so I think this is not needed (also above).

Ack, removed.

> > ...
> > +			partition@f70000 {
> > +				label = "event-log";
> > +				reg = <0xf70000 0x80000>;
> > +			};
> 
> There are 0x10000 left empty at the end?
> Is this writeable by intention?

No, length should be 0x90000 instead, was a typo.
Also set read-only as this is not being used by OpenWrt.

> > +&wmac {
> > +	status = "okay";
> > +
> > +	mtd-cal-data = <&art 0x1000>;
> > +	mtd-mac-address = <&config 0x0>;
> > +	mtd-mac-address-increment = <2>;
> 
> No other addresses available in flash? art 0x1002 not usable?

00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
*
00001000  02 02 00 02 03 04 05 06  00 30 3a 30 32 3a 30 33  |.........0:02:03|
00001010  3a 30 34 3a 30 35 3a 30  36 00 00 00 00 00 1f 00  |:04:05:06.......|

Doesn't look useful to me :(
> 
> Please use newer syntax:
> 
> DEVICE_VENDOR := Teltonika
> DEVICE_MODEL := RUT955

Ack.

> 
> > +  DEVICE_PACKAGES := kmod-usb2 kmod-usb-acm  kmod-usb-net-qmi-
> > wwan
> 
> Double space after -acm; I'd like a line break here.

Fixed.

> 
> > +kmod-usb-serial-option kmod-hwmon-mcp3021 uqmi -uboot-envtools
> > +  IMAGE_SIZE := 15552k
> > +  TPLINK_HWID := 0x35000001
> > +  TPLINK_HWREV := 0x1
> > +  TPLINK_HEADER_VERSION := 1
> > +  KERNEL := kernel-bin | append-dtb | lzma | tplink-v1-header
> > +  KERNEL_INITRAMFS := kernel-bin | append-dtb | lzma | uImage lzma
> > +  IMAGES := sysupgrade.bin factory.bin
> 
> Could also use "IMAGES += factory.bin" here.

True.

I will repost v2 shortly after testing it with all changes suggested
by Piotr and you applied.

Thanks to both of you for the instant review!


Cheers


Daniel

> 
> Best
> 
> Adrian
>

Hi Piotr,

On Mon, Feb 03, 2020 at 11:51:29PM +0100, Piotr Dymacz wrote:
> Hi Daniel, Adrian,
> 
> See my comments inline.
> 
> On 02.02.2020 17:41, Daniel Golle wrote:
> > The Teltonika RUT955 is an industrial 2G/3G/4G WiFi router with
> > various additional inputs and outputs.
> > 
> > Specification:
> > 
> > - 550/400/200 MHz (CPU/DDR/AHB)
> > - 128 MB of RAM (DDR2)
> > - 16 MB of FLASH (SPI NOR)
> > - 4x 10/100 Mbps Ethernet, with passive PoE support on LAN1
> > - 2T2R 2,4 GHz (AR9344)
> 
> Have you tested RF sensitivity? I remember external GPIO-driven LNA in
> RUT900 has to be defined, otherwise the radio was almost deaf.

Seems like RUT955 hasn't got that external LNA.
The GPIO line (2) used for that in mach-rut9xx.c is used for DIN3
digital input on the RUT955 board...
Signal level seems very ok:
laptop 4m away from device:		signal: -38 [-42, -41] dBm
phone in other room (>6m + wall):	signal: -55 [-65, -55] dBm


Cheers


Daniel