Re: [PATCH v2 1/1] ARM: dts: add support for mws4 board

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 29/05/2022 22:37, Marian Ulbricht wrote:
> Mws4 is an ARM based nuclear probe hardware designed and used by
> German government "Bundesamt fuer Strahlenschutz" to monitor
> nuclear activity.
> 
> Signed-off-by: Marian Ulbricht <ulbricht@innoroute.de>
> ---
> 
> Changes in v2:
> * cosmetics
> 
> Changes in v1:
> * add dts
> 
>  arch/arm/boot/dts/omap3-mws4.dts | 328 +++++++++++++++++++++++++++++++
>  1 file changed, 328 insertions(+)
>  create mode 100644 arch/arm/boot/dts/omap3-mws4.dts
> 
> diff --git a/arch/arm/boot/dts/omap3-mws4.dts b/arch/arm/boot/dts/omap3-mws4.dts
> new file mode 100644
> index 000000000000..680a8e4ee816
> --- /dev/null
> +++ b/arch/arm/boot/dts/omap3-mws4.dts
> @@ -0,0 +1,328 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * Modified 2015 by Bernhard Gaetzschmann, Ultratronik from Beagleboard xM
> + *
> + * Modified 2022 Marian Ulbricht ulbricht@innoroute.de
> + *
> + */
> +/dts-v1/;
> +#include "omap36xx.dtsi"
> +
> +/ {
> +	model = "Ultratronik BFS MWS4";
> +	compatible = "ti,omap3-mws4", "ti,omap36xx", "ti,omap3";

The board compatible still looks wrong. According to model it is not the
TI who made it. You need to use proper vendor prefix. Vendor prefix and
board compatible should be documented in the bindings. For the board
compatible this is Documentation/devicetree/bindings/arm/omap/omap.txt I
guess.

> +	cpus {
> +		cpu@0 {

Please override by label.

> +			cpu0-supply = <&vcc>;
> +		};
> +	};
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0x80000000 0x10000000>; // 256 MB
> +	};
> +
> +	aliases {

Aliases go first.

> +		i2c0 = &i2c1;
> +		i2c1 = &i2c2;
> +		i2c2 = &i2c3;
> +		serial0 = &uart1;
> +		serial1 = &uart2;
> +		serial2 = &uart3;
> +		mmc1 = &mmc1;
> +	};
> +
> +	netcard: AX88796BLI@ffdf0000 {
> +		compatible = "ax88796_dt";

This did not improve since last time... the node name does not match
Devicetree spec (generic node name like "ethernet", only lowercase
characters). The compatible is not documented. In fact, it is not a
proper compatible.

Checkpatch should warn you about this.

> +		reg = <0xffdf0000 0x1000> ;
> +
> +};
> +
> +	watchdog_max: watchdog {
> +		compatible = "linux,wdt-gpio";
> +		gpios = <&gpio4 21 1>; //117

Please use GPIO flags for the flags.

> +		hw_algo = "toggle";
> +		always-running;
> +		hw_margin_ms = <900>;
> +	};
> +
> +	hsusb1_phy: hsusb1_phy {

Override by labels.

> +		status = "disabled";
> +	};
> +
> +	/* HS USB Host PHY on PORT 2 */
> +	hsusb2_phy: hsusb2_phy {

The same.

> +		status = "disabled";
> +	};
> +
> +	/* fixed 19.2MHz oscillator */
> +	hfclk_19m2: oscillator {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <19200000>;
> +	};
> +
> +leds {

Please spend some time to make this code look like normal DTS.
Indentation is easy to fix, you do not reviewers to point it out.

> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&led_pins>;
> +		led_sm {

Generic node names, no underscores in node names. So led-0.

> +			label = "led_sm";
> +			gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* 101 */
> +			default-state = "on";
> +		};
> +
> +		led1 {

led-1

and so one.
> +			label = "led1";
> +			gpios = <&gpio4 6 GPIO_ACTIVE_HIGH>; /* 102 */
> +			linux,default-trigger = "cpu";
> +		};
> +
> +		led2 {
> +			label = "led2";
> +			gpios = <&gpio4 7 GPIO_ACTIVE_HIGH>; /* 103 */
> +			linux,default-trigger = "mmc0";

Add function and color properties where applicable.

> +		};
> +
> +		led3 {
> +			label = "led3";
> +			gpios = <&gpio4 8 GPIO_ACTIVE_HIGH>; /* 104 */
> +			linux,default-trigger = "usb-host";
> +		};
> +
> +		led_usb {
> +			label = "led_usb";
> +			gpios = <&gpio4 14 GPIO_ACTIVE_HIGH>; /* 110 */
> +			default-state = "on";
> +		};
> +	};
> +};
> +
> +&gpmc {
> +	ranges = <0 0 0x30000000 0x1000000	/* CS0 space, 16MB */
> +		  255 0 0x6e000000 0x02d4>; /* register space */
> +
> +	/* Chip select 0 */
> +	nand@0,0 {
> +		compatible = "ti,omap2-nand";
> +		reg = <0 0 4		/* NAND I/O window, 4 bytes */
> +		       255 0 0x02d4>; /* GPMC register space */
> +		interrupts = <20>;
> +		ti,nand-ecc-opt = "bch4";
> +		nand-bus-width = <16>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		gpmc,cs-on-ns = <0>;
> +		gpmc,cs-rd-off-ns = <36>;
> +		gpmc,cs-wr-off-ns = <36>;
> +		gpmc,adv-on-ns = <6>;
> +		gpmc,adv-rd-off-ns = <24>;
> +		gpmc,adv-wr-off-ns = <36>;
> +		gpmc,oe-on-ns = <6>;
> +		gpmc,oe-off-ns = <48>;
> +		gpmc,we-on-ns = <6>;
> +		gpmc,we-off-ns = <30>;
> +		gpmc,rd-cycle-ns = <72>;
> +		gpmc,wr-cycle-ns = <72>;
> +		gpmc,access-ns = <54>;
> +		gpmc,wr-access-ns = <30>;
> +
> +		partition@0 {
> +			label = "X-Loader";
> +			reg = <0 0x40000>;
> +		};
> +
> +		partition@40000 {
> +			label = "U-Boot";
> +			reg = <0x40000 0x100000>;
> +		};
> +
> +		partition@100000 {
> +			label = "U-Boot Env";
> +			reg = <0x100000 0x120000>;
> +		};
> +
> +		partition@120000 {
> +			label = "dt";
> +			reg = <0x120000 0x140000>;
> +		};
> +
> +		partition@140000 {
> +			label = "Kernel";
> +			reg = <0x140000 0xB40000>;
> +		};
> +
> +		partition@B40000 {
> +			label = "Filesystem";
> +			reg = <0xB40000 0xf4c0000>;
> +		};
> +	};
> +};
> +
> +&usbhshost {
> +	status = "disabled";
> +};
> +
> +&omap3_pmx_core {
> +		mmc1_pins: pinmux_mmc1_pins {

No underscores in node names. Hyphens instead.

> +		pinctrl-single,pins = <
> +			OMAP3_CORE1_IOPAD(0x2144, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_clk.sdmmc1_clk */
> +			OMAP3_CORE1_IOPAD(0x2146, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_cmd.sdmmc1_cmd */
> +			OMAP3_CORE1_IOPAD(0x2148, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat0.sdmmc1_dat0 */
> +			OMAP3_CORE1_IOPAD(0x214a, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat1.sdmmc1_dat1 */
> +			OMAP3_CORE1_IOPAD(0x214c, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat2.sdmmc1_dat2 */
> +			OMAP3_CORE1_IOPAD(0x214e, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat3.sdmmc1_dat3 */
> +		>;
> +	};
> +
> +	wd_pins: pinmux_wd_pins {
> +		pinctrl-single,pins = <
> +			OMAP3_CORE1_IOPAD(0x213e, PIN_OUTPUT | MUX_MODE4)	// CONTROL_PADCONF_MCBSP2_CLKX	0x013E
> +		>;
> +	};
> +
> +	i2c1_pins: pinmux_i2c1_pins {
> +		pinctrl-single,pins = <
> +			OMAP3_CORE1_IOPAD(0x21ba, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c1_scl */
> +			OMAP3_CORE1_IOPAD(0x21bc, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c1_sda */
> +		>;
> +	};
> +
> +	hsusb0_pins: pinmux_hsusb0_pins {
> +		pinctrl-single,pins = <
> +			OMAP3_CORE1_IOPAD(0x21a2, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* hsusb0_clk.hsusb0_clk */
> +			OMAP3_CORE1_IOPAD(0x21a4, PIN_OUTPUT | MUX_MODE0)					/* hsusb0_stp.hsusb0_stp */
> +			OMAP3_CORE1_IOPAD(0x21a6, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* hsusb0_dir.hsusb0_dir */
> +			OMAP3_CORE1_IOPAD(0x21a8, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* hsusb0_nxt.hsusb0_nxt */
> +			OMAP3_CORE1_IOPAD(0x21aa, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* hsusb0_data0.hsusb2_data0 */
> +			OMAP3_CORE1_IOPAD(0x21ac, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* hsusb0_data1.hsusb0_data1 */
> +			OMAP3_CORE1_IOPAD(0x21ae, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* hsusb0_data2.hsusb0_data2 */
> +			OMAP3_CORE1_IOPAD(0x21b0, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* hsusb0_data7.hsusb0_data3 */
> +			OMAP3_CORE1_IOPAD(0x21b2, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* hsusb0_data7.hsusb0_data4 */
> +			OMAP3_CORE1_IOPAD(0x21b4, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* hsusb0_data7.hsusb0_data5 */
> +			OMAP3_CORE1_IOPAD(0x21b6, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* hsusb0_data7.hsusb0_data6 */
> +			OMAP3_CORE1_IOPAD(0x21b8, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* hsusb0_data7.hsusb0_data7 */
> +		>;
> +	};
> +
> +	led_pins: pinmux_led_pins {
> +		pinctrl-single,pins = <
> +			OMAP3_CORE1_IOPAD(0x211a, PIN_OUTPUT | MUX_MODE4)	/* gpio101*/
> +			OMAP3_CORE1_IOPAD(0x211c, PIN_OUTPUT | MUX_MODE4)	/* gpio102*/
> +			OMAP3_CORE1_IOPAD(0x211e, PIN_OUTPUT | MUX_MODE4)	/* gpio103 */
> +			OMAP3_CORE1_IOPAD(0x2120, PIN_OUTPUT | MUX_MODE4)	/* gpio103 */
> +		>;
> +	};
> +
> +	gpio_pins: pinmux_gpio_pins {
> +		pinctrl-single,pins = <
> +			OMAP3_CORE1_IOPAD(0x2122, PIN_INPUT | MUX_MODE4)	/* gpio105 spontanmeldung */
> +			OMAP3_CORE1_IOPAD(0x212c, PIN_OUTPUT | MUX_MODE4)	/* gpio110 usb2_en */
> +			OMAP3_CORE1_IOPAD(0x218C, PIN_OUTPUT | MUX_MODE4)	/* GPO0 */
> +			OMAP3_CORE1_IOPAD(0x218E, PIN_OUTPUT | MUX_MODE4)	/* GPO1 */
> +			OMAP3_CORE1_IOPAD(0x2190, PIN_OUTPUT | MUX_MODE4)	/* GPO2 */
> +			OMAP3_CORE1_IOPAD(0x2192, PIN_OUTPUT | MUX_MODE4)	/* GPO3 */
> +			OMAP3_CORE1_IOPAD(0x2194, PIN_OUTPUT | MUX_MODE4)	/* GPO4 */
> +			OMAP3_CORE1_IOPAD(0x2196, PIN_OUTPUT | MUX_MODE4)	/* GPO5 */
> +			OMAP3_CORE1_IOPAD(0x2198, PIN_OUTPUT | MUX_MODE4)	/* GPO6 */
> +			OMAP3_CORE1_IOPAD(0x2116, PIN_INPUT | MUX_MODE4)	/* IN1 */
> +			OMAP3_CORE1_IOPAD(0x2118, PIN_INPUT | MUX_MODE4)	/* IN2 */
> +			OMAP3_CORE1_IOPAD(0x2124, PIN_INPUT | MUX_MODE4)	/* IN3 */
> +			OMAP3_CORE1_IOPAD(0x2126, PIN_INPUT | MUX_MODE4)	/* IN4 */
> +			OMAP3_CORE1_IOPAD(0x2128, PIN_INPUT | MUX_MODE4)	/* IN5 */
> +			OMAP3_CORE1_IOPAD(0x25F4, PIN_OUTPUT | MUX_MODE4)	/* RES_RS232 */
> +			OMAP3_CORE1_IOPAD(0x25F6, PIN_OUTPUT | MUX_MODE4)	/* HUB_RESET */
> +		>;
> +	};
> +};
> +
> +&i2c1 {
> +	pinctrl-names = "default";
> +
> +	pinctrl-0 = <&i2c1_pins>;
> +	clock-frequency = <100000>;
> +	twl: twl@48 {

Generic node name representing class of a device.

> +		reg = <0x48>;
> +		interrupts = <7>; /* SYS_NIRQ cascaded to intc */
> +		interrupt-parent = <&intc>;
> +		clocks = <&hfclk_19m2>;
> +		clock-names = "fck";
> +		twl_power: power {
> +			compatible = "ti,twl4030-power";
> +			ti,system-power-controller;
> +		};
> +	};
> +};
> +
> +&i2c2 {
> +	clock-frequency = <100000>;
> +	rtc8564: rtc8564@51 {

The same.

> +		compatible = "epson,rtc8564";
> +		reg = <0x51>;
> +		#clock-cells = <0>;
> +	};
> +};
> +
> +&i2c3 {
> +	clock-frequency = <100000>;
> +};
> +
> +#include "twl4030.dtsi"
> +#include "twl4030_omap3.dtsi"

Includes go on top usually.

Best regards,
Krzysztof