| Message ID | 11e0b0c8b828254567a8ff89820c067cacad2150.1702917360.git.jan.kiszka@siemens.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: iot2050: Add support for new SM variant | expand |
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
On 19.12.23 08:54, Krzysztof Kozlowski wrote: > 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 > This is adjusting the existing LED nodes in k3-am65-iot2050-common.dtsi, not introducing new ones. We can add the color properties in a separate patch, but the node names are now part of the kernel ABI. Changing them would break existing userland. > >> + >> +&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. This goes along disabling USB3 which is by default enabled via k3-am65-iot2050-common-pg2.dtsi > >> +}; >> + >> +&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. > Same story. Baocheng, Zeng Chao, correct me if there is more behind that. Jan
On 19.12.23 09:48, Krzysztof Kozlowski wrote: > On 19/12/2023 09:22, Jan Kiszka wrote: >>> >>>> + gpios = <&wkup_gpio0 53 GPIO_ACTIVE_HIGH>; >>> >>> Ditto >>> >> >> This is adjusting the existing LED nodes in k3-am65-iot2050-common.dtsi, >> not introducing new ones. We can add the color properties in a separate > > > Then why aren't you overriding by phandle/label? > We could do that as well if we added labels first (they don't exist so far). Not seeing any difference, though. >> patch, but the node names are now part of the kernel ABI. Changing them >> would break existing userland. > > You mean label. Why node names became the ABI? Which interface exposes them? root@iot2050-debian:~# ls -l /sys/class/leds/ total 0 lrwxrwxrwx 1 root root 0 Dec 19 08:55 mmc0:: -> ../../devices/platform/bus@100000/4fa0000.mmc/leds/mmc0:: lrwxrwxrwx 1 root root 0 Dec 19 08:55 mmc1:: -> ../../devices/platform/bus@100000/4f80000.mmc/leds/mmc1:: lrwxrwxrwx 1 root root 0 Dec 14 21:12 status-led-green -> ../../devices/platform/leds/leds/status-led-green lrwxrwxrwx 1 root root 0 Dec 19 08:55 status-led-red -> ../../devices/platform/leds/leds/status-led-red lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led1-green -> ../../devices/platform/leds/leds/user-led1-green lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led1-red -> ../../devices/platform/leds/leds/user-led1-red lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led2-green -> ../../devices/platform/leds/leds/user-led2-green lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led2-red -> ../../devices/platform/leds/leds/user-led2-red >>>> + >>>> +&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. >> >> This goes along disabling USB3 which is by default enabled via >> k3-am65-iot2050-common-pg2.dtsi > > Isn't this mistake? Common part enables only these pieces which are > working in common hardware SoM. If your common part of hardware, which > DTSI should represent, has USB3 then why is it being disabled here? If > common hardware design does not have USB3, then why is it being enabled > in DTSI? It's a trade-off between adding yet another dtsi for those widely common bits vs. adjusting the differences of only one variant from that. We do the same for the Display Port so far. Jan
On 19/12/2023 10:03, Jan Kiszka wrote: > On 19.12.23 09:48, Krzysztof Kozlowski wrote: >> On 19/12/2023 09:22, Jan Kiszka wrote: >>>> >>>>> + gpios = <&wkup_gpio0 53 GPIO_ACTIVE_HIGH>; >>>> >>>> Ditto >>>> >>> >>> This is adjusting the existing LED nodes in k3-am65-iot2050-common.dtsi, >>> not introducing new ones. We can add the color properties in a separate >> >> >> Then why aren't you overriding by phandle/label? >> > > We could do that as well if we added labels first (they don't exist so > far). Not seeing any difference, though. > >>> patch, but the node names are now part of the kernel ABI. Changing them >>> would break existing userland. >> >> You mean label. Why node names became the ABI? Which interface exposes them? > > root@iot2050-debian:~# ls -l /sys/class/leds/ > total 0 > lrwxrwxrwx 1 root root 0 Dec 19 08:55 mmc0:: -> ../../devices/platform/bus@100000/4fa0000.mmc/leds/mmc0:: > lrwxrwxrwx 1 root root 0 Dec 19 08:55 mmc1:: -> ../../devices/platform/bus@100000/4f80000.mmc/leds/mmc1:: > lrwxrwxrwx 1 root root 0 Dec 14 21:12 status-led-green -> ../../devices/platform/leds/leds/status-led-green > lrwxrwxrwx 1 root root 0 Dec 19 08:55 status-led-red -> ../../devices/platform/leds/leds/status-led-red > lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led1-green -> ../../devices/platform/leds/leds/user-led1-green > lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led1-red -> ../../devices/platform/leds/leds/user-led1-red > lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led2-green -> ../../devices/platform/leds/leds/user-led2-green > lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led2-red -> ../../devices/platform/leds/leds/user-led2-red > >>>>> + >>>>> +&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. >>> >>> This goes along disabling USB3 which is by default enabled via >>> k3-am65-iot2050-common-pg2.dtsi >> >> Isn't this mistake? Common part enables only these pieces which are >> working in common hardware SoM. If your common part of hardware, which >> DTSI should represent, has USB3 then why is it being disabled here? If >> common hardware design does not have USB3, then why is it being enabled >> in DTSI? > > It's a trade-off between adding yet another dtsi for those widely > common bits vs. adjusting the differences of only one variant from You don't need to add one more DTSI to achieve proper architecture of DTS/DTSI split. > that. We do the same for the Display Port so far. DTSI represents common piece of hardware, like SoM or re-usable blocks, not trade-off. Best regards, Krzysztof
On 19.12.23 10:50, Krzysztof Kozlowski wrote: > On 19/12/2023 10:03, Jan Kiszka wrote: >> On 19.12.23 09:48, Krzysztof Kozlowski wrote: >>> On 19/12/2023 09:22, Jan Kiszka wrote: >>>>> >>>>>> + gpios = <&wkup_gpio0 53 GPIO_ACTIVE_HIGH>; >>>>> >>>>> Ditto >>>>> >>>> >>>> This is adjusting the existing LED nodes in k3-am65-iot2050-common.dtsi, >>>> not introducing new ones. We can add the color properties in a separate >>> >>> >>> Then why aren't you overriding by phandle/label? >>> >> >> We could do that as well if we added labels first (they don't exist so >> far). Not seeing any difference, though. > > Confusion? Your code suggests new node, thus you got review like you got. > >> >>>> patch, but the node names are now part of the kernel ABI. Changing them >>>> would break existing userland. >>> >>> You mean label. Why node names became the ABI? Which interface exposes them? >> >> root@iot2050-debian:~# ls -l /sys/class/leds/ >> total 0 >> lrwxrwxrwx 1 root root 0 Dec 19 08:55 mmc0:: -> ../../devices/platform/bus@100000/4fa0000.mmc/leds/mmc0:: >> lrwxrwxrwx 1 root root 0 Dec 19 08:55 mmc1:: -> ../../devices/platform/bus@100000/4f80000.mmc/leds/mmc1:: >> lrwxrwxrwx 1 root root 0 Dec 14 21:12 status-led-green -> ../../devices/platform/leds/leds/status-led-green >> lrwxrwxrwx 1 root root 0 Dec 19 08:55 status-led-red -> ../../devices/platform/leds/leds/status-led-red >> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led1-green -> ../../devices/platform/leds/leds/user-led1-green >> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led1-red -> ../../devices/platform/leds/leds/user-led1-red >> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led2-green -> ../../devices/platform/leds/leds/user-led2-green >> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led2-red -> ../../devices/platform/leds/leds/user-led2-red > > I replied too fast previous and did not include answer here: > > You have label for that... Somehow all these nodes are half-baked, > without all the expected properties and now you call node name as ABI. > The node name is not the ABI. Well, existing userspace uses those names, and adding the properties would break that interface. Now, does Linux do that? Jan
On 19.12.23 10:54, Jan Kiszka wrote: > On 19.12.23 10:50, Krzysztof Kozlowski wrote: >> On 19/12/2023 10:03, Jan Kiszka wrote: >>> On 19.12.23 09:48, Krzysztof Kozlowski wrote: >>>> On 19/12/2023 09:22, Jan Kiszka wrote: >>>>>> >>>>>>> + gpios = <&wkup_gpio0 53 GPIO_ACTIVE_HIGH>; >>>>>> >>>>>> Ditto >>>>>> >>>>> >>>>> This is adjusting the existing LED nodes in k3-am65-iot2050-common.dtsi, >>>>> not introducing new ones. We can add the color properties in a separate >>>> >>>> >>>> Then why aren't you overriding by phandle/label? >>>> >>> >>> We could do that as well if we added labels first (they don't exist so >>> far). Not seeing any difference, though. >> >> Confusion? Your code suggests new node, thus you got review like you got. >> >>> >>>>> patch, but the node names are now part of the kernel ABI. Changing them >>>>> would break existing userland. >>>> >>>> You mean label. Why node names became the ABI? Which interface exposes them? >>> >>> root@iot2050-debian:~# ls -l /sys/class/leds/ >>> total 0 >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 mmc0:: -> ../../devices/platform/bus@100000/4fa0000.mmc/leds/mmc0:: >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 mmc1:: -> ../../devices/platform/bus@100000/4f80000.mmc/leds/mmc1:: >>> lrwxrwxrwx 1 root root 0 Dec 14 21:12 status-led-green -> ../../devices/platform/leds/leds/status-led-green >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 status-led-red -> ../../devices/platform/leds/leds/status-led-red >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led1-green -> ../../devices/platform/leds/leds/user-led1-green >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led1-red -> ../../devices/platform/leds/leds/user-led1-red >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led2-green -> ../../devices/platform/leds/leds/user-led2-green >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led2-red -> ../../devices/platform/leds/leds/user-led2-red >> >> I replied too fast previous and did not include answer here: >> >> You have label for that... Somehow all these nodes are half-baked, >> without all the expected properties and now you call node name as ABI. >> The node name is not the ABI. > > Well, existing userspace uses those names, and adding the properties > would break that interface. Now, does Linux do that? > Obviously, we could deviate from the existing naming scheme only for the new variant, keeping it for the other 5, but that will be "fun" to maintain. Jan
On 19/12/2023 10:54, Jan Kiszka wrote: >>>> You mean label. Why node names became the ABI? Which interface exposes them? >>> >>> root@iot2050-debian:~# ls -l /sys/class/leds/ >>> total 0 >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 mmc0:: -> ../../devices/platform/bus@100000/4fa0000.mmc/leds/mmc0:: >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 mmc1:: -> ../../devices/platform/bus@100000/4f80000.mmc/leds/mmc1:: >>> lrwxrwxrwx 1 root root 0 Dec 14 21:12 status-led-green -> ../../devices/platform/leds/leds/status-led-green >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 status-led-red -> ../../devices/platform/leds/leds/status-led-red >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led1-green -> ../../devices/platform/leds/leds/user-led1-green >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led1-red -> ../../devices/platform/leds/leds/user-led1-red >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led2-green -> ../../devices/platform/leds/leds/user-led2-green >>> lrwxrwxrwx 1 root root 0 Dec 19 08:55 user-led2-red -> ../../devices/platform/leds/leds/user-led2-red >> >> I replied too fast previous and did not include answer here: >> >> You have label for that... Somehow all these nodes are half-baked, >> without all the expected properties and now you call node name as ABI. >> The node name is not the ABI. > > Well, existing userspace uses those names, and adding the properties > would break that interface. Now, does Linux do that? I don't think you understood the concept. There is no change for userspace. Same interface, same names. No ABI break. Anyway, changing them is not part of this patchset since these are not new nodes. Best regards, Krzysztof
On 19.12.23 16:39, Krzysztof Kozlowski wrote: > On 19/12/2023 16:37, Jan Kiszka wrote: >>>>> >>>>> You have label for that... Somehow all these nodes are half-baked, >>>>> without all the expected properties and now you call node name as ABI. >>>>> The node name is not the ABI. >>>> >>>> Well, existing userspace uses those names, and adding the properties >>>> would break that interface. Now, does Linux do that? >>> >>> I don't think you understood the concept. There is no change for >>> userspace. Same interface, same names. No ABI break. >> >> I do understand the impact very well: >> open("/sys/class/leds/user-led1-red") has to work for all the variants, >> consistently and backward-compatible for userspace. > > And it will. The name is the same. Nope, it's not - I tried that already :) root@iot2050-debian:~# ls -l /sys/class/leds/ total 0 lrwxrwxrwx 1 root root 0 Dec 19 09:49 green:indicator -> ../../devices/platform/leds/leds/green:indicator lrwxrwxrwx 1 root root 0 Dec 19 09:49 green:status -> ../../devices/platform/leds/leds/green:status lrwxrwxrwx 1 root root 0 Dec 19 09:49 mmc0:: -> ../../devices/platform/bus@100000/4fa0000.mmc/leds/mmc0:: lrwxrwxrwx 1 root root 0 Dec 19 09:49 mmc1:: -> ../../devices/platform/bus@100000/4f80000.mmc/leds/mmc1:: lrwxrwxrwx 1 root root 0 Dec 19 09:49 red:indicator -> ../../devices/platform/leds/leds/red:indicator lrwxrwxrwx 1 root root 0 Dec 19 09:49 red:indicator_1 -> ../../devices/platform/leds/leds/red:indicator_1 lrwxrwxrwx 1 root root 0 Dec 19 09:49 red:indicator_2 -> ../../devices/platform/leds/leds/red:indicator_2 lrwxrwxrwx 1 root root 0 Dec 19 09:49 red:status -> ../../devices/platform/leds/leds/red:status Jan
On 19/12/2023 16:48, Jan Kiszka wrote: > On 19.12.23 16:42, Krzysztof Kozlowski wrote: >> On 19/12/2023 16:40, Jan Kiszka wrote: >>> On 19.12.23 16:39, Krzysztof Kozlowski wrote: >>>> On 19/12/2023 16:37, Jan Kiszka wrote: >>>>>>>> >>>>>>>> You have label for that... Somehow all these nodes are half-baked, >>>>>>>> without all the expected properties and now you call node name as ABI. >>>>>>>> The node name is not the ABI. >>>>>>> >>>>>>> Well, existing userspace uses those names, and adding the properties >>>>>>> would break that interface. Now, does Linux do that? >>>>>> >>>>>> I don't think you understood the concept. There is no change for >>>>>> userspace. Same interface, same names. No ABI break. >>>>> >>>>> I do understand the impact very well: >>>>> open("/sys/class/leds/user-led1-red") has to work for all the variants, >>>>> consistently and backward-compatible for userspace. >>>> >>>> And it will. The name is the same. >>> >>> Nope, it's not - I tried that already :) >>> >>> root@iot2050-debian:~# ls -l /sys/class/leds/ >>> total 0 >>> lrwxrwxrwx 1 root root 0 Dec 19 09:49 green:indicator -> ../../devices/platform/leds/leds/green:indicator >> >> And how does your DTS look like? >> >> Because I also tried and it is exactly the same. >> > > I played with > > diff --git a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi > index 402afa4bc1b6..a791444eeb93 100644 > --- a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi > +++ b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi > @@ -10,6 +10,7 @@ > */ > > #include "k3-am654.dtsi" > +#include <dt-bindings/leds/common.h> > #include <dt-bindings/phy/phy.h> > #include <dt-bindings/net/ti-dp83867.h> > > @@ -84,27 +85,39 @@ leds { > pinctrl-0 = <&leds_pins_default>; > > status-led-red { > + color = <LED_COLOR_ID_RED>; > + function = LED_FUNCTION_STATUS; > gpios = <&wkup_gpio0 32 GPIO_ACTIVE_HIGH>; > panic-indicator; And where is the label property? Please read my message again: >> You: >> patch, but the node names are now part of the kernel ABI. Changing them would break existing userland. > Me: > You mean label. Why node names became the ABI? Which interface exposes them? >> You: >> root@iot2050-debian:~# ls -l /sys/class/leds/ > Me: > I replied too fast previous and did not include answer here: > You have label for that... So again: The stable ABI is fulfilled by using label property. Not the Devicetree "label" phandle in front of the node, but the dedicated property. Best regards, Krzysztof
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 { + gpios = <&wkup_gpio0 52 GPIO_ACTIVE_HIGH>; + }; + + user-led1-green { + gpios = <&wkup_gpio0 53 GPIO_ACTIVE_HIGH>; + }; + }; +}; + +&main_pmx0 { + main_pcie_enable_pins_default: main-pcie-enable-default-pins { + pinctrl-single,pins = < + AM65X_IOPAD(0x01d8, PIN_OUTPUT, 7) /* (AH12) GPIO1_22 */ + >; + }; + + main_spi0_pins: main-spi0-default-pins { + pinctrl-single,pins = < + AM65X_IOPAD(0x01c4, PIN_INPUT, 0) /* (AH13) SPI0_CLK */ + AM65X_IOPAD(0x01c8, PIN_INPUT, 0) /* (AE13) SPI0_D0 */ + AM65X_IOPAD(0x01cc, PIN_INPUT, 0) /* (AD13) SPI0_D1 */ + AM65X_IOPAD(0x01bc, PIN_OUTPUT, 0) /* (AG13) SPI0_CS0 */ + >; + }; +}; + +&main_pmx1 { + asic_spi_mux_ctrl_pin: asic-spi-mux-ctrl-default-pins { + pinctrl-single,pins = < + AM65X_IOPAD(0x0010, PIN_OUTPUT, 7) /* (D21) GPIO1_86 */ + >; + }; +}; + +&wkup_pmx0 { + user1_led_pins: user1-led-default-pins { + pinctrl-single,pins = < + /* (AB1) WKUP_UART0_RXD:WKUP_GPIO0_52, as USER 1 led red */ + AM65X_WKUP_IOPAD(0x00a0, PIN_OUTPUT, 7) + /* (AB5) WKUP_UART0_TXD:WKUP_GPIO0_53, as USER 1 led green */ + AM65X_WKUP_IOPAD(0x00a4, PIN_OUTPUT, 7) + >; + }; + + soc_asic_pins: soc-asic-default-pins { + pinctrl-single,pins = < + AM65X_WKUP_IOPAD(0x0044, PIN_INPUT, 7) /* (P4) WKUP_GPIO0_29 */ + AM65X_WKUP_IOPAD(0x0048, PIN_INPUT, 7) /* (P5) WKUP_GPIO0_30 */ + AM65X_WKUP_IOPAD(0x004c, PIN_INPUT, 7) /* (P1) WKUP_GPIO0_31 */ + >; + }; +}; + +&main_gpio0 { + gpio-line-names = "main_gpio0-base"; +}; + +&main_gpio1 { + pinctrl-names = "default"; + pinctrl-0 = + <&cp2102n_reset_pin_default>, + <&main_pcie_enable_pins_default>, + <&asic_spi_mux_ctrl_pin>; + gpio-line-names = + /* 0..9 */ + "", "", "", "", "", "", "", "", "", "", + /* 10..19 */ + "", "", "", "", "", "", "", "", "", "", + /* 20..29 */ + "", "", "", "", "CP2102N-RESET", "", "", "", "", "", + /* 30..39 */ + "", "", "", "", "", "", "", "", "", "", + /* 40..49 */ + "", "", "", "", "", "", "", "", "", "", + /* 50..59 */ + "", "", "", "", "", "", "", "", "", "", + /* 60..69 */ + "", "", "", "", "", "", "", "", "", "", + /* 70..79 */ + "", "", "", "", "", "", "", "", "", "", + /* 80..86 */ + "", "", "", "", "", "", "ASIC-spi-mux-ctrl"; +}; + +&wkup_gpio0 { + pinctrl-names = "default"; + pinctrl-0 = + <&push_button_pins_default>, + <&db9_com_mode_pins_default>, + <&soc_asic_pins>; + gpio-line-names = + /* 0..9 */ + "wkup_gpio0-base", "", "", "", "UART0-mode1", "UART0-mode0", + "UART0-enable", "UART0-terminate", "", "WIFI-disable", + /* 10..19 */ + "", "", "", "", "", "", "", "", "", "", + /* 20..29 */ + "", "", "", "", "", "USER-button", "", "", "","ASIC-gpio-0", + /* 30..31 */ + "ASIC-gpio-1", "ASIC-gpio-2"; +}; + +&main_spi0 { + pinctrl-names = "default"; + pinctrl-0 = <&main_spi0_pins>; + + #address-cells = <1>; + #size-cells= <0>; +}; + +&mcu_spi0 { + pinctrl-names = "default"; + pinctrl-0 = <&mcu_spi0_pins_default>; +}; + +&main_i2c3 { + accelerometer: lsm6dso@6a { + compatible = "st,lsm6dso"; + reg = <0x6a>; + }; + + /delete-node/ edp-bridge@f; +}; + +&dss { + status = "disabled"; +}; + +&dss_ports { + /delete-node/ port@1; +}; + +&serdes0 { + assigned-clocks = <&k3_clks 153 4>, <&serdes0 AM654_SERDES_CMU_REFCLK>; + assigned-clock-parents = <&k3_clks 153 8>, <&k3_clks 153 4>; +}; + +&serdes1 { + status = "disabled"; +}; + +&pcie0_rc { + pinctrl-names = "default"; + pinctrl-0 = <&minipcie_pins_default>; + + num-lanes = <1>; + phys = <&serdes0 PHY_TYPE_PCIE 1>; + phy-names = "pcie-phy0"; + reset-gpios = <&wkup_gpio0 27 GPIO_ACTIVE_HIGH>; + status = "okay"; +}; + +&pcie1_rc { + status = "disabled"; +}; + +&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; +}; + +&usb0 { + maximum-speed = "high-speed"; + /delete-property/ snps,dis-u1-entry-quirk; + /delete-property/ snps,dis-u2-entry-quirk; +};