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

From: Nishanth Menon
Date: Tue Feb 09 2021 - 09:45:47 EST

Next message: Catangiu, Adrian Costin: "Re: [PATCH v5 1/2] drivers/misc: sysgenid: add system generation id driver"
Previous message: Leo Yan: "Re: [PATCH] perf tools: Fix arm64 build error with gcc-11"
In reply to: Jan Kiszka: "[PATCH] arm64: dts: ti: Add support for Siemens IOT2050 boards"
Next in thread: Jan Kiszka: "Re: [PATCH] arm64: dts: ti: Add support for Siemens IOT2050 boards"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---

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.

> .../devicetree/bindings/arm/ti/k3.yaml | 2 +
> arch/arm64/boot/dts/ti/Makefile | 4 +
> .../boot/dts/ti/k3-am65-iot2050-common.dtsi | 649 ++++++++++++++++++
> .../boot/dts/ti/k3-am6528-iot2050-basic.dts | 56 ++
> .../dts/ti/k3-am6548-iot2050-advanced.dts | 57 ++
> 5 files changed, 768 insertions(+)
> create mode 100644 arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> create mode 100644 arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dts
> create mode 100644 arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dts
>
> diff --git a/Documentation/devicetree/bindings/arm/ti/k3.yaml b/Documentation/devicetree/bindings/arm/ti/k3.yaml
> index c6e1c1e63e43..b1ab0cf4a2d6 100644
> --- a/Documentation/devicetree/bindings/arm/ti/k3.yaml
> +++ b/Documentation/devicetree/bindings/arm/ti/k3.yaml
> @@ -23,6 +23,8 @@ properties:
> items:
> - enum:
> - ti,am654-evm
> + - siemens,iot2050-basic
> + - siemens,iot2050-advanced

- 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.

> - const: ti,am654
>
> - description: K3 J721E SoC
> diff --git a/arch/arm64/boot/dts/ti/Makefile b/arch/arm64/boot/dts/ti/Makefile
> index 65506f21ba30..928ea26ce250 100644
> --- a/arch/arm64/boot/dts/ti/Makefile
> +++ b/arch/arm64/boot/dts/ti/Makefile
> @@ -8,6 +8,10 @@
>
> dtb-$(CONFIG_ARCH_K3) += k3-am654-base-board.dtb
>

- 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

> +dtb-$(CONFIG_ARCH_K3) += k3-am6548-iot2050-advanced.dtb
> +
> dtb-$(CONFIG_ARCH_K3) += k3-j721e-common-proc-board.dtb
>
> dtb-$(CONFIG_ARCH_K3) += k3-j7200-common-proc-board.dtb
> diff --git a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> new file mode 100644
> index 000000000000..de05937dbb60
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> @@ -0,0 +1,649 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) Siemens AG, 2018-2021
> + *

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

> + * Authors:
> + * Le Jin <le.jin@xxxxxxxxxxx>
> + * Jan Kiszka <jan.kiszk@xxxxxxxxxxx>
> + */
> +
> +/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..

> +};
> +
> +&pcie1_ep {
> + status = "disabled";
> +};
> diff --git a/arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dts b/arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dts
> new file mode 100644
> index 000000000000..bb9ab4fdd74e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dts
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) Siemens AG, 2018-2021
> + *

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 <le.jin@xxxxxxxxxxx>
> + * Jan Kiszka <jan.kiszk@xxxxxxxxxxx>
> + */
> +
> +/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.

> +};
> +
> +/* eMMC */
> +&sdhci0 {
> + status = "disabled";
> +};
> +
> +&main_pmx0 {
> + main_uart0_pins_default: main_uart0_pins_default {
> + pinctrl-single,pins = <
> + AM65X_IOPAD(0x01e4, PIN_INPUT, 0) /* (AF11) UART0_RXD */
> + AM65X_IOPAD(0x01e8, PIN_OUTPUT, 0) /* (AE11) UART0_TXD */
> + AM65X_IOPAD(0x01ec, PIN_INPUT, 0) /* (AG11) UART0_CTSn */
> + AM65X_IOPAD(0x01f0, PIN_OUTPUT, 0) /* (AD11) UART0_RTSn */
> + AM65X_IOPAD(0x0188, PIN_INPUT, 1) /* (D25) UART0_DCDn */
> + AM65X_IOPAD(0x018c, PIN_INPUT, 1) /* (B26) UART0_DSRn */
> + AM65X_IOPAD(0x0190, PIN_OUTPUT, 1) /* (A24) UART0_DTRn */
> + AM65X_IOPAD(0x0194, PIN_INPUT, 1) /* (E24) UART0_RIN */
> + >;
> + };
> +};
> +
> +&main_uart0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&main_uart0_pins_default>;
> +};
> diff --git a/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dts b/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dts
> new file mode 100644
> index 000000000000..aa1ef081ef22
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dts
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) Siemens AG, 2018-2021
> + *
> + * Authors:
> + * Le Jin <le.jin@xxxxxxxxxxx>
> + * Jan Kiszka <jan.kiszk@xxxxxxxxxxx>
> + */
> +
> +/dts-v1/;
> +
> +#include "k3-am65-iot2050-common.dtsi"
[...]

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

Next message: Catangiu, Adrian Costin: "Re: [PATCH v5 1/2] drivers/misc: sysgenid: add system generation id driver"
Previous message: Leo Yan: "Re: [PATCH] perf tools: Fix arm64 build error with gcc-11"
In reply to: Jan Kiszka: "[PATCH] arm64: dts: ti: Add support for Siemens IOT2050 boards"
Next in thread: Jan Kiszka: "Re: [PATCH] arm64: dts: ti: Add support for Siemens IOT2050 boards"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]