Re: [PATCH] arm64: dts: ti: Add support for Siemens IOT2050 boards

Jan,

A few quick scan comments below, you might need to post based off
5.12-rc1 once available..

Also, I see a bit of warnings with dtbs_check, which probably needs a
little more digging into (pcie insists to get a device_type property,
etc..)

you could use kernel_patch_verify or https://github.com/nmenon/kernel_patch_verify/blob/master/Dockerbuild.md

it throws up a report like this https://pastebin.ubuntu.com/p/SdkZr432z3/

So, many of my comments below are just first pass parse of that log -> I
usually do recommend building with W=2 and dtbs_check (with yamlint etc)
to make sure things are a bit sane. Will be good to have additional
eyes.

On 11:21-20210209, Jan Kiszka wrote:

From: Jan Kiszka <jan.kiszka@siemens.com>

Add support for two Siemens SIMATIC IOT2050 variants, Basic and
Advanced. They are based on the TI AM6528 and AM6548 SOCs.

Based on original version by Le Jin.

Might be good to add links to the boards as well (if available), for
future reference.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Will be nice to see at least a pastebin link for a bootlog on the boards
in the cover-letter / diffstat section with the v2 - for reference.

- In a separate patch, ./Documentation/devicetree/bindings/vendor-prefixes.yaml -> Could you
  make sure we add 'siemens' there?
- and, lets move the bindings to it's own patch, since that is how Rob
 prefers to review in https://patchwork.ozlabs.org/project/devicetree-bindings/list/

Both of these patches will need Rob to ack. I think I should be able
to pick the first one up as well to reduce dependency, but we can
check with Rob in case there is a preference.

- drop the EOL to club am65 dtbs close to each other

+dtb-$(CONFIG_ARCH_K3) += k3-am6528-iot2050-basic.dtb
+

- Drop this EOL as well. Something like this:

dtb-$(CONFIG_ARCH_K3) += k3-am654-base-board.dtb
dtb-$(CONFIG_ARCH_K3) += k3-am6528-iot2050-basic.dtb
dtb-$(CONFIG_ARCH_K3) += k3-am6548-iot2050-advanced.dtb

dtb-$(CONFIG_ARCH_K3) += k3-j721e-common-proc-board.dtb

Optional: might be nice to add a oneliner comment for reuse scope..

+ * Authors:
+ *   Le Jin [off-list ref]
+ *   Jan Kiszka [off-list ref]
+ */
+
+/dts-v1/;
+
+#include "k3-am654.dtsi"
+#include <dt-bindings/phy/phy.h>
+
+/ {
+	aliases {
+		spi0 = &mcu_spi0;
+	};
+
+	chosen {
+		stdout-path = "serial3:115200n8";
+		bootargs = "earlycon=ns16550a,mmio32,0x02800000";

serial3 is main_uart1, did you mean 0x02810000 instead of 0x02800000 ?

+	};
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		secure_ddr: secure_ddr@9e800000 {

"_" is not something we prefer for node names, so something like
	secure_ddr: secure-ddr@...

+			reg = <0 0x9e800000 0 0x01800000>; /* for OP-TEE */
+			alignment = <0x1000>;
+			no-map;
+		};
+
+		mcu_r5fss0_core0_dma_memory_region: r5f-dma-memory@a0000000 {
+			compatible = "shared-dma-pool";
+			reg = <0 0xa0000000 0 0x100000>;
+			no-map;
+		};
+
+		mcu_r5fss0_core0_memory_region: r5f-memory@a0100000 {
+			compatible = "shared-dma-pool";
+			reg = <0 0xa0100000 0 0xf00000>;
+			no-map;
+		};
+
+		mcu_r5fss0_core1_dma_memory_region: r5f-dma-memory@a1000000 {
+			compatible = "shared-dma-pool";
+			reg = <0 0xa1000000 0 0x100000>;
+			no-map;
+		};
+
+		mcu_r5fss0_core1_memory_region: r5f-memory@a1100000 {
+			compatible = "shared-dma-pool";
+			reg = <0 0xa1100000 0 0xf00000>;
+			no-map;
+		};
+
+		rtos_ipc_memory_region: ipc-memories@a2000000 {
+			reg = <0x00 0xa2000000 0x00 0x00200000>;
+			alignment = <0x1000>;
+			no-map;
+		};
+	};
+
+	gpio_leds {

just 'leds'?

+		compatible = "gpio-leds";
+		pinctrl-names = "default";
+		pinctrl-0 = <&leds_pins_default>;
+
+		status-led-red {
+			gpios = <&wkup_gpio0 32 GPIO_ACTIVE_HIGH>;
+			panic-indicator;
+			linux,default-trigger = "gpio";
+		};
+
+		status-led-green {
+			gpios = <&wkup_gpio0 24 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "gpio";
+		};
+
+		user-led1-red {
+			gpios = <&pcal9535_3 14 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "gpio";
+		};
+
+		user-led1-green {
+			gpios = <&pcal9535_2 15 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "gpio";
+		};
+
+		user-led2-red {
+			gpios = <&wkup_gpio0 17 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "gpio";
+		};
+
+		user-led2-green {
+			gpios = <&wkup_gpio0 22 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "gpio";

are you sure this is "gpio" ? dtbs_check reports should be one of:
	['backlight', 'default-on', 'heartbeat', 'disk-activity', 'ide-disk', 'timer', 'pattern']

+		};
+	};
+
+	dp_refclk: clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <19200000>;
+	};
+};
+
+&wkup_pmx0 {
+	wkup_i2c0_pins_default: wkup_i2c0_pins_default {

	Here, and else where:
		wkup_i2c0_pins_default: wkup-i2c0-pins-default

Prefer we dont use _ in node names (rest of the pinmux node names as
		well).

+		pinctrl-single,pins = <
+			AM65X_WKUP_IOPAD(0x00e0, PIN_INPUT,  0)  /* (AC7) WKUP_I2C0_SCL */
+			AM65X_WKUP_IOPAD(0x00e4, PIN_INPUT,  0)  /* (AD6) WKUP_I2C0_SDA */
+		>;
+	};
+
+	mcu_i2c0_pins_default: mcu_i2c0_pins_default {
+		pinctrl-single,pins = <
+			AM65X_IOPAD(0x0070, PIN_INPUT,  5)  /* (R25) I2C2_SDA */

[... similar issues with '_' elsewhere.. ]

+		>;
+	};
+};
+
+&main_pmx1 {
+	main_i2c0_pins_default: main-i2c0-pins-default {

	these look fine..

+		pinctrl-single,pins = <
+			AM65X_IOPAD(0x0000, PIN_INPUT,  0)  /* (D20) I2C0_SCL */
+			AM65X_IOPAD(0x0004, PIN_INPUT,  0)  /* (C21) I2C0_SDA */
+		>;
+	};
+
+	main_i2c1_pins_default: main-i2c1-pins-default {
+		pinctrl-single,pins = <
+			AM65X_IOPAD(0x0008, PIN_INPUT,  0)  /* (B21) I2C1_SCL */
+			AM65X_IOPAD(0x000c, PIN_INPUT,  0)  /* (E21) I2C1_SDA */
+		>;
+	};
+
+	ecap0_pins_default: ecap0-pins-default {
+		pinctrl-single,pins = <
+			AM65X_IOPAD(0x0010, PIN_INPUT,  0)  /* (D21) ECAP0_IN_APWM_OUT */
+		>;
+	};
+};
+
+&wkup_uart0 {
+	/* Wakeup UART is used by System firmware */
+	status = "disabled";

In case of reservation for firmware usage:
status = "reserved";

[...]

+
+&wkup_gpio0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <
+		&arduino_io_d2_to_d3_pins_default
+		&arduino_i2c_aio_switch_pins_default
+		&arduino_io_oe_pins_default
+		&push_button_pins_default
+		&db9_com_mode_pins_default
+	>;
+	gpio-line-names =
+		"wkup_gpio0-base", "", "", "", "UART0-mode1", "UART0-mode0",
+			"UART0-enable", "UART0-terminate", "", "WIFI-disable",
+		"", "", "", "", "", "", "", "", "", "",
+		"", "A4A5-I2C-mux", "", "", "", "USER-button", "", "", "","IO0",
+		"IO1", "IO2", "", "IO3", "IO17-direction",
+			"A5", "IO16-direction", "IO15-direction",
+			"IO14-direction", "A3",
+		"", "IO18-direction", "A4", "A2", "A1",
+			"A0", "", "", "IO13", "IO11",
+		"IO12", "IO10";

Any chance of intending this consistently?

+};
+
+&wkup_i2c0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&wkup_i2c0_pins_default>;
+	clock-frequency = <400000>;
+};
+
+&mcu_i2c0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mcu_i2c0_pins_default>;
+	clock-frequency = <400000>;
+
+	psu: tps62363@60 {
+		compatible = "ti,tps62363";
+		reg =  <0x60>;
+		regulator-name = "tps62363-vout";
+		regulator-min-microvolt = <500000>;
+		regulator-max-microvolt = <1500000>;
+		regulator-boot-on;
+		/* ti,vsel0-gpio = <&gpio1 16 0>; */
+		/* ti,vsel1-gpio = <&gpio1 17 0>; */

Could you drop the commented out properties: above and later?

+		ti,vsel0-state-high;
+		ti,vsel1-state-high;
+		/* ti,enable-pull-down; */
+		/* ti,enable-force-pwm; */
+		ti,enable-vout-discharge;
+	};
+
+	/*D4200*/

Add a space prefix and postfix? (here and below)
/* D4200 */

+	pcal9535_1: gpio@20 {
+		compatible = "nxp,pcal9535";
+		reg = <0x20>;
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-line-names =
+			"A0-pull", "A1-pull", "A2-pull", "A3-pull", "A4-pull",
+			"A5-pull", "", "",
+			"IO14-enable", "IO15-enable", "IO16-enable",
+			"IO17-enable", "IO18-enable", "IO19-enable";
+	};
+
+	/*D4201*/
+	pcal9535_2: gpio@21 {
+		compatible = "nxp,pcal9535";
+		reg = <0x21>;
+		#gpio-cells = <2>;
+		gpio-controller;
+		gpio-line-names =
+			"IO0-direction", "IO1-direction", "IO2-direction",
+			"IO3-direction", "IO4-direction", "IO5-direction",
+			"IO6-direction", "IO7-direction",
+			"IO8-direction", "IO9-direction", "IO10-direction",
+			"IO11-direction", "IO12-direction", "IO13-direction",
+			"IO19-direction";
+	};
+

	[...]

+
+&pcie1_rc {
+	pinctrl-names = "default";
+	pinctrl-0 = <&minipcie_pins_default>;
+
+	num-lanes = <1>;
+	phys = <&serdes1 PHY_TYPE_PCIE 0>;
+	phy-names = "pcie-phy0";
+	reset-gpios = <&wkup_gpio0 27 GPIO_ACTIVE_HIGH>;

schema seems to want a device_type, and there seems to be some warnings
	on serdes as well.. might be something to check up on..

Will be nice to explain the difference between basic and advanced dts.

Are they two different boards? looks like the basic is using a part spin
with a single cluster, perhaps? I guess links might help..

+ * Authors:
+ *   Le Jin [off-list ref]
+ *   Jan Kiszka [off-list ref]
+ */
+
+/dts-v1/;
+
+#include "k3-am65-iot2050-common.dtsi"
+
+/ {
+	compatible = "siemens,iot2050-basic", "ti,am654";
+	model = "SIMATIC IOT2050 Basic";
+
+	memory@80000000 {
+		device_type = "memory";
+		/* 1G RAM */
+		reg = <0x00000000 0x80000000 0x00000000 0x40000000>;
+	};
+
+	cpus {
+		cpu-map {
+			/delete-node/ cluster1;
+		};
+		/delete-node/ cpu@100;
+		/delete-node/ cpu@101;
+	};

Personally, I'd prefer this (handling efuse spins in board files or
even overlays) instead of having to create 100s of dtsi per SoC for
every permutation & combination of TI efused devices and handle these
in board files. I do see examples of similar usage elsewhere in:

$ git grep /delete-node/ arch/arm64/boot/dts/

But, if someone has a different opinion, feel free to pipe up with a
reasonable way to prevent file explosion.

[...]

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D