Re: [PATCH 3/3] ARM: dts: marvell: add support for D-Link DNS-320L

[Krzysztof Kozlowski <krzk@kernel.org>]

On 29/06/2024 13:34, Zoltan HERPAI wrote:
> Dual-slot NAS based on Marvell Kirkwood.
> 
> Specifications:
>  - Marvell 88F6702 @1GHz
>  - 256Mb RAM
>  - 128Mb NAND
>  - 1x GbE LAN (Marvell 88E1318R)
>  - 1x USB 2.0
>  - 2x SATA
>  - Weltrend WT69P3 ("supervisor" MCU chip)
>  - Serial on J2 (115200,8n1)
>  - Newer bootROM so kwboot-ing via serial is possible
> 
> Signed-off-by: Zoltan HERPAI <wigyori@uid0.hu>
> ---
>  arch/arm/boot/dts/marvell/Makefile            |   1 +
>  .../arm/boot/dts/marvell/kirkwood-dns320l.dts | 197 ++++++++++++++++++
>  2 files changed, 198 insertions(+)
>  create mode 100644 arch/arm/boot/dts/marvell/kirkwood-dns320l.dts
> 
> diff --git a/arch/arm/boot/dts/marvell/Makefile b/arch/arm/boot/dts/marvell/Makefile
> index 1e0f5ff492f7..cadd4039b783 100644
> --- a/arch/arm/boot/dts/marvell/Makefile
> +++ b/arch/arm/boot/dts/marvell/Makefile
> @@ -92,6 +92,7 @@ dtb-$(CONFIG_MACH_KIRKWOOD) += \
>  	kirkwood-db-88f6282.dtb \
>  	kirkwood-dir665.dtb \
>  	kirkwood-dns320.dtb \
> +	kirkwood-dns320l.dtb \
>  	kirkwood-dns325.dtb \
>  	kirkwood-dockstar.dtb \
>  	kirkwood-dreamplug.dtb \
> diff --git a/arch/arm/boot/dts/marvell/kirkwood-dns320l.dts b/arch/arm/boot/dts/marvell/kirkwood-dns320l.dts
> new file mode 100644
> index 000000000000..82a2d60376f7
> --- /dev/null
> +++ b/arch/arm/boot/dts/marvell/kirkwood-dns320l.dts
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Device Tree file for D-Link DNS-320L
> + *
> + * Copyright (C) 2024, Zoltan HERPAI <wigyori@uid0.hu>
> + * Copyright (C) 2015, Sunke Schluters <sunke-dev@schlueters.de>
> + *
> + * This file is based on the works of:
> + * - Sunke Schluters <sunke-dev@schlueters.de>
> + *   - https://github.com/scus1/dns320l/blob/master/kernel/dts/kirkwood-dns320l.dts
> + * - Andreas Bohler <dev@aboehler.at>:
> + *   - http://www.aboehler.at/doku/doku.php/projects:dns320l
> + *   - http://www.aboehler.at/hg/linux-dns320l/file/ba7a60ad7687/linux-3.12/kirkwood-dns320l.dts
> + */
> +
> +/dts-v1/;
> +
> +#include "kirkwood.dtsi"
> +#include "kirkwood-6281.dtsi"
> +
> +/ {
> +	model = "D-Link DNS-320L";
> +	compatible = "dlink,dns320l", "marvell,kirkwood-88f6702", "marvell,kirkwood";
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0x00000000 0x10000000>;
> +	};
> +
> +	chosen {
> +		bootargs = "console=ttyS0,115200n8 earlyprintk";
> +		stdout-path = &uart0;
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		#address-cells = <1>;
> +		#size-cells = <0>;

Nope, these cannot be there.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> +		pinctrl-0 = <&pmx_buttons>;
> +		pinctrl-names = "default";
> +
> +		button@1 {
> +			label = "Reset push button";
> +			linux,code = <KEY_RESTART>;
> +			gpios = <&gpio0 28 1>;
> +		};
> +
> +		button@2 {
> +			label = "USB unmount button";
> +			linux,code = <KEY_EJECTCD>;
> +			gpios = <&gpio0 27 1>;
> +		};
> +	};
> +
> +	gpio-leds {
> +		compatible = "gpio-leds";
> +		pinctrl-0 = <&pmx_leds>;
> +		pinctrl-names = "default";
> +
> +		blue-usb {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> +			label = "dns320l:usb:blue";
> +			gpios = <&gpio0 25 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "usbport";
> +		};
> +
> +		orange-usb {
> +			label = "dns320l:usb:orange";
> +			gpios = <&gpio0 26 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		orange-l-hdd {
> +			label = "dns320l:orange:l_hdd";
> +			gpios = <&gpio0 23 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		orange-r-hdd {
> +			label = "dns320l:orange:r_hdd";
> +			gpios = <&gpio0 22 GPIO_ACTIVE_HIGH>;
> +		};
> +	};
> +
> +	ocp@f1000000 {

Why you are not overriding by label/phandle?

> +		sata@80000 {
> +			status = "okay";
> +			nr-ports = <2>;
> +		};
> +
> +		serial@12000 {
> +			status = "okay";
> +		};
> +
> +		serial@12100 {
> +			pinctrl-0 = <&pmx_uart1>;
> +			pinctrl-names = "default";
> +			status = "okay";
> +		};
> +	};
> +
> +	regulators {

No, there is no such node/bus. Drop

> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-0 = <&pmx_power_sata>;
> +		pinctrl-names = "default";
> +
> +		sata_power: regulator@1 {
> +			compatible = "regulator-fixed";
> +			reg = <1>;
> +			regulator-name = "SATA Power";
> +			regulator-min-microvolt = <5000000>;
> +			regulator-max-microvolt = <5000000>;
> +			enable-active-high;
> +			regulator-always-on;
> +			regulator-boot-on;
> +			gpio = <&gpio0 24 0>;
> +		};
> +	};
> +};
> +


Best regards,
Krzysztof