On 04.03.21 07:58, Vignesh Raghavendra wrote: > Hi, > > On 2/12/21 1:02 AM, 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 GP and AM6548 SOCs HS, thus >> differ in their number of cores and availability of security features. >> Furthermore the Advanced version comes with more RAM, an eMMC and a few >> internal differences. >> >> Based on original version by Le Jin. >> >> Link: https://new.siemens.com/global/en/products/automation/pc-based/iot-gateways/simatic-iot2050.html >> Link: https://github.com/siemens/meta-iot2050 >> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx> > > Reviewed-by: Vignesh Raghavendra <vigneshr@xxxxxx> > Thanks! > Few minor comments below: > > [...] > >> + >> +&mcu_i2c0 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&mcu_i2c0_pins_default>; >> + clock-frequency = <400000>; >> + >> + psu: tps62363@60 { > > Please use generic node names: > > psu: regulator@60 { ... }; > Done. >> + compatible = "ti,tps62363"; >> + reg = <0x60>; >> + regulator-name = "tps62363-vout"; >> + regulator-min-microvolt = <500000>; >> + regulator-max-microvolt = <1500000>; >> + regulator-boot-on; >> + ti,vsel0-state-high; >> + ti,vsel1-state-high; >> + ti,enable-vout-discharge; >> + }; >> + >> + /* 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"; >> + }; >> + >> + /* D4202 */ >> + pcal9535_3: gpio@25 { >> + compatible = "nxp,pcal9535"; >> + reg = <0x25>; >> + #gpio-cells = <2>; >> + gpio-controller; >> + gpio-line-names = >> + "IO0-pull", "IO1-pull", "IO2-pull", "IO3-pull", >> + "IO4-pull", "IO5-pull", "IO6-pull", "IO7-pull", >> + "IO8-pull", "IO9-pull", "IO10-pull", "IO11-pull", >> + "IO12-pull", "IO13-pull"; >> + }; >> +}; > > [...] > >> +&dwc3_0 { >> + status = "okay"; >> +}; >> + >> +&usb0_phy { >> + status = "okay"; >> +}; >> + > > These nodes are enabled by default right? Above is redundant. Seems like historic left-overs here - fixed. > >> +&usb0 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&usb0_pins_default>; >> + dr_mode = "host"; >> +}; >> + >> +&dwc3_1 { >> + status = "okay"; >> +}; >> + >> +&usb1_phy { >> + status = "okay"; >> +}; >> + > > Same here... Also fixed. > >> +&usb1 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&usb1_pins_default>; >> + dr_mode = "host"; >> +}; >> + > > [...] > >> +&mcu_spi0 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&mcu_spi0_pins_default>; >> + >> + #address-cells = <1>; >> + #size-cells= <0>; >> + ti,pindir-d0-out-d1-in = <1>; >> + >> + spidev@0 { >> + compatible = "rohm,dh2228fv"; >> + spi-max-frequency = <20000000>; >> + reg = <0>; >> + }; > > Is the device really dh2228fv? At least to my understanding, "rohm,dh2228fv" is commonly used for declaring spidev, and this is what we need for userland here. > >> +}; >> + >> +&tscadc0 { >> + status = "disabled"; >> +}; >> + >> +&tscadc1 { >> + adc { >> + ti,adc-channels = <0 1 2 3 4 5>; >> + }; >> +}; >> + >> +&ospi0 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&mcu_fss0_ospi0_pins_default>; >> + >> + flash@0 { >> + compatible = "jedec,spi-nor"; >> + reg = <0x0>; >> + spi-tx-bus-width = <1>; >> + spi-rx-bus-width = <1>; >> + spi-max-frequency = <50000000>; >> + cdns,tshsl-ns = <60>; >> + cdns,tsd2d-ns = <60>; >> + cdns,tchsh-ns = <60>; >> + cdns,tslch-ns = <60>; >> + cdns,read-delay = <2>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + }; >> +}; >> + >> +&dss { >> + status = "okay"; >> + > > Node is enabled by default. Please drop above line for consistency. Done. > >> + pinctrl-names = "default"; >> + pinctrl-0 = <&dss_vout1_pins_default>; >> + >> + assigned-clocks = <&k3_clks 67 2>; >> + assigned-clock-parents = <&k3_clks 67 5>; >> +}; >> + >> +&dss_ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + port@1 { >> + reg = <1>; >> + >> + dpi_out: endpoint { >> + remote-endpoint = <&bridge_in>; >> + }; >> + }; >> +}; >> + > > [...] > > Regards > Vignesh > Thanks again, v3 will come after some testing. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
