Re: [PATCH 4/4] dts: iot2050: Support IOT2050-SM variant

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

On 18/12/2023 17:36, Jan Kiszka wrote:
> From: Baocheng Su <baocheng.su@siemens.com>
> 
> Main differences between the new variant and Advanced PG2:

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

> 
> 1. Arduino interface is removed. Instead, an new ASIC is added for
>    communicating with PLC 1200 signal modules.
> 2. USB 3.0 type A connector is removed, only USB 2.0 type A connector is
>    avaiable.
> 3. DP interface is tailored down. Instead, to communicate with the
>    PLC 1200 signal modules, a USB 3.0 type B connector is added but the
>    signal is not USB.
> 4. DDR size is increased to 4 GB.
> 5. Two sensors are added, one tilt sensor and one light sensor.
> 
> The light sensor it not yet added to the DT at this stage as it depends
> on to-be-added bindings.
> 
> Co-developed-by: Chao Zeng <chao.zeng@siemens.com>
> Signed-off-by: Chao Zeng <chao.zeng@siemens.com>
> Co-developed-by: Li Hua Qian <huaqian.li@siemens.com>
> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> Signed-off-by: Baocheng Su <baocheng.su@siemens.com>
> [Jan: rebase over arduino refactoring, split-out light sensor]
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/arm64/boot/dts/ti/Makefile               |   1 +
>  .../dts/ti/k3-am6548-iot2050-advanced-sm.dts  | 210 ++++++++++++++++++
>  2 files changed, 211 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dts
> 
> diff --git a/arch/arm64/boot/dts/ti/Makefile b/arch/arm64/boot/dts/ti/Makefile
> index 77a347f9f47d..9b15eaad284c 100644
> --- a/arch/arm64/boot/dts/ti/Makefile
> +++ b/arch/arm64/boot/dts/ti/Makefile
> @@ -53,6 +53,7 @@ dtb-$(CONFIG_ARCH_K3) += k3-am6528-iot2050-basic-pg2.dtb
>  dtb-$(CONFIG_ARCH_K3) += k3-am6548-iot2050-advanced.dtb
>  dtb-$(CONFIG_ARCH_K3) += k3-am6548-iot2050-advanced-m2.dtb
>  dtb-$(CONFIG_ARCH_K3) += k3-am6548-iot2050-advanced-pg2.dtb
> +dtb-$(CONFIG_ARCH_K3) += k3-am6548-iot2050-advanced-sm.dtb
>  dtb-$(CONFIG_ARCH_K3) += k3-am654-base-board.dtb
>  dtb-$(CONFIG_ARCH_K3) += k3-am654-gp-evm.dtb
>  dtb-$(CONFIG_ARCH_K3) += k3-am654-evm.dtb
> diff --git a/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dts b/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dts
> new file mode 100644
> index 000000000000..ab3eef683890
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dts
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) Siemens AG, 2023
> + *
> + * Authors:
> + *   Baocheng Su <baocheng.su@siemens.com>
> + *   Chao Zeng <chao.zeng@siemens.com>
> + *   Huaqian Li <huaqian.li@siemens.com>
> + *
> + * AM6548-based (quad-core) IOT2050 SM variant, Product Generation 2
> + * 4 GB RAM, 16 GB eMMC, USB-serial converter on connector X30
> + *
> + * Product homepage:
> + * https://new.siemens.com/global/en/products/automation/pc-based/iot-gateways/simatic-iot2050.html
> + */
> +
> +/dts-v1/;
> +
> +#include "k3-am6548-iot2050-advanced-common.dtsi"
> +#include "k3-am65-iot2050-common-pg2.dtsi"
> +
> +/ {
> +	compatible = "siemens,iot2050-advanced-sm", "ti,am654";
> +	model = "SIMATIC IOT2050 Advanced SM";
> +
> +	memory@80000000 {
> +		device_type = "memory";
> +		/* 4G RAM */
> +		reg = <0x00000000 0x80000000 0x00000000 0x80000000>,
> +		      <0x00000008 0x80000000 0x00000000 0x80000000>;
> +	};
> +
> +	aliases {
> +		spi1 = &main_spi0;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&leds_pins_default>, <&user1_led_pins>;
> +
> +		user-led1-red {

led-0

> +			gpios = <&wkup_gpio0 52 GPIO_ACTIVE_HIGH>;

Missing function, missing color. Color goes as property, not as node name.

> +		};
> +
> +		user-led1-green {

led-1

> +			gpios = <&wkup_gpio0 53 GPIO_ACTIVE_HIGH>;

Ditto


> +
> +&dwc3_0 {
> +	assigned-clock-parents = <&k3_clks 151 4>,  /* set REF_CLK to 20MHz i.e. PER0_PLL/48 */
> +				 <&k3_clks 151 9>;  /* set PIPE3_TXB_CLK to CLK_12M_RC/256 (for HS only) */
> +	/delete-property/ phys;
> +	/delete-property/ phy-names;

If your board need to remove phys from the SoC node, something is wrong.
Either your board or SoC.

Any removal of properties in DTS is weird and unexpected. It deserves
comments.

> +};
> +
> +&usb0 {
> +	maximum-speed = "high-speed";
> +	/delete-property/ snps,dis-u1-entry-quirk;
> +	/delete-property/ snps,dis-u2-entry-quirk;

Same questions. If SoC has quirks, how can your board be fixed? It's SoC
property... or you are using different SoC.

Best regards,
Krzysztof