[PATCH v2 0/2] add support for Phytec PCM-049 and PCM-959

[PATCH v2 0/2] add support for Phytec PCM-049 and PCM-959

[Colin Foster]

As should be clear for the title and patch title, this is adding initial
support for the OMAP 4460 SOM and dev kit for Phytec's PCM959 evaluation
kit.

The PCM049 is a legacy SOM offered by Phytec:
https://www.phytec.com/legacy-soms/
There was a vendor BSP offered by Phytec, but that never entered the
Device Tree era. This patch is meant to change that.

My development bootloader has moved to U-Boot, but I have verified
Barebox works in the past. When booting from SD card, either bootloader
should work. When booting from Barebox, the NAND OOB layout is
incompatible between the bootloader and the kernel.

I haven't had any OOB / ECC errors in the NAND at all, which was my
main concern. Due to that, I'm submitting this as a patch instead of
an RFC. Hardware ECC correction seems to be fully functional.



v1->v2
    * Almost everything moved into the SOM (PCM-049) .dtsi. Only the
      LED chip is dev-board specific.
    * Fix pinmux associations (*pmx_core was applying *pmx_wkup
      entries... I'm surprised that didn't cause more issues)
    * Documentation added

    * Updates from review:
    *   Board compatible strings added
    *   Hyphen / underscore changes
    *   Remove unnecessary status="okay" entries
    *   Generic names used (regulator, led-1, etc.)


Colin Foster (2):
  dt-bindings: arm: omap: add phytec pcm-049 som and pcm-959 dev board
  arm: dts: omap4: pcm959: add initial support for phytec pcm959

 .../devicetree/bindings/arm/omap/omap.txt     |   3 +
 arch/arm/boot/dts/Makefile                    |   1 +
 arch/arm/boot/dts/omap4-phytec-pcm-049.dtsi   | 412 ++++++++++++++++++
 arch/arm/boot/dts/omap4-phytec-pcm-959.dts    |  48 ++
 4 files changed, 464 insertions(+)
 create mode 100644 arch/arm/boot/dts/omap4-phytec-pcm-049.dtsi
 create mode 100644 arch/arm/boot/dts/omap4-phytec-pcm-959.dts

-- 
2.25.1

[PATCH v2 1/2] dt-bindings: arm: omap: add phytec pcm-049 som and pcm-959 dev board

[Colin Foster]

Add documentation for additional OMAP SOMs and development platforms,
provided by Phytec as the PCM-049 SOM and the PCM-959 development kit.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---

v1->v2
    * New patch

---
 Documentation/devicetree/bindings/arm/omap/omap.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/omap/omap.txt b/Documentation/devicetree/bindings/arm/omap/omap.txt
index fa8b31660cad..0a28215dfa12 100644
--- a/Documentation/devicetree/bindings/arm/omap/omap.txt
+++ b/Documentation/devicetree/bindings/arm/omap/omap.txt
@@ -131,6 +131,9 @@ Boards (incomplete list of examples):
 - OMAP4 PandaBoard : Low cost community board
   compatible = "ti,omap4-panda", "ti,omap4430", "ti,omap4"
 
+- OMAP4 PCM-959 : Commercial dev kit with PCM-049 SOM
+  compatible = "phytec,pcm959", "phytec,pcm049", "ti,omap4460", "ti,omap4430", "ti,omap4";
+
 - OMAP4 DuoVero with Parlor : Commercial expansion board with daughter board
   compatible = "gumstix,omap4-duovero-parlor", "gumstix,omap4-duovero", "ti,omap4430", "ti,omap4";
 
-- 
2.25.1

[PATCH v2 2/2] arm: dts: omap4: pcm959: add initial support for phytec pcm959

[Colin Foster]

The Phytec PCM-959 is a development platform for the Phytec PCM-049 SOM.
Add initial functionality for the board. The verified interfaces and
peripherals are listed below for the SOM (PCM-049) and the dev board
(PCM-959)

The omap2plus_defconfig was used for testing. Only the On-board LEDs
required CONFIG_LEDS_PCA9532 addition.

PCM-049:
i2c1
  * EEPROM at 0x50
  * TMP102 (hwmon) at 0x4b
twl6030
GPMC
  * Ethernet
  * Flash
Serial (ttyS2 console)

PCM959:
MMC1
On-board LEDs (with CONFIG_LEDS_PCA9532)

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---

v1->v2
    * Almost everything moved to the SOM (PCM049)
    * Fix where omap_pmx_wkup were actually getting applied to
      omap_pmx_core.
    * Use hyphens instead of underscores for names
    * Remove unnecessary entries of "status=okay"
    * Use generic "regulator" and "led" names
    * add compatible strings for the dev board and SOM

---
 arch/arm/boot/dts/Makefile                  |   1 +
 arch/arm/boot/dts/omap4-phytec-pcm-049.dtsi | 412 ++++++++++++++++++++
 arch/arm/boot/dts/omap4-phytec-pcm-959.dts  |  48 +++
 3 files changed, 461 insertions(+)
 create mode 100644 arch/arm/boot/dts/omap4-phytec-pcm-049.dtsi
 create mode 100644 arch/arm/boot/dts/omap4-phytec-pcm-959.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 6aa7dc4db2fc..ae8338cb5bb1 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -968,6 +968,7 @@ dtb-$(CONFIG_ARCH_OMAP4) += \
 	omap4-panda.dtb \
 	omap4-panda-a4.dtb \
 	omap4-panda-es.dtb \
+	omap4-phytec-pcm-959.dtb \
 	omap4-sdp.dtb \
 	omap4-sdp-es23plus.dtb \
 	omap4-var-dvk-om44.dtb \
diff --git a/arch/arm/boot/dts/omap4-phytec-pcm-049.dtsi b/arch/arm/boot/dts/omap4-phytec-pcm-049.dtsi
new file mode 100644
index 000000000000..314b37e51082
--- /dev/null
+++ b/arch/arm/boot/dts/omap4-phytec-pcm-049.dtsi
@@ -0,0 +1,412 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Innovative Advantage, Inc.
+ */
+#include <dt-bindings/input/input.h>
+
+/ {
+	memory@80000000 {
+		device_type = "memory";
+		reg = <0x80000000 0x40000000>; /* 1 GB */
+	};
+
+	reserved-memory {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		dsp_memory_region: dsp-memory@98000000 {
+			compatible = "shared-dma-pool";
+			reg = <0x98000000 0x800000>;
+			reusable;
+			status = "okay";
+		};
+
+		ipu_memory_region: ipu-memory@98800000 {
+			compatible = "shared-dma-pool";
+			reg = <0x98800000 0x7000000>;
+			reusable;
+			status = "okay";
+		};
+	};
+
+	chosen {
+		stdout-path = &uart3;
+	};
+
+	leds: leds {
+		compatible = "gpio-leds";
+		pinctrl-names = "default";
+		pinctrl-0 = <
+			&led_gpio_pins
+		>;
+
+		led-0 {
+			label = "modul:red:status1";
+			gpios = <&gpio5 0x18 GPIO_ACTIVE_HIGH>; /* GPIO 152 */
+			linux,default-trigger = "heartbeat";
+		};
+
+		led-1 {
+			label = "modul:green:status2";
+			gpios = <&gpio5 0x19 GPIO_ACTIVE_HIGH>; /* GPIO 153 */
+			linux,default-trigger = "mmc0";
+		};
+	};
+};
+
+&gpio1_target {
+	ti,no-reset-on-init;
+};
+
+&omap4_pmx_core {
+	pinctrl-names = "default";
+	pinctrl-0 = <
+		&tps62361_pins
+	>;
+
+	i2c1_pins: pinmux-i2c1-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x122, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c1_scl */
+			OMAP4_IOPAD(0x124, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c1_sda */
+		>;
+	};
+
+	i2c3_pins: pinmux-i2c3-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x12a, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c3_scl */
+			OMAP4_IOPAD(0x12c, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c3_sda */
+		>;
+	};
+
+	i2c4_pins: pinmux-i2c4-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x12e, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c4_scl */
+			OMAP4_IOPAD(0x130, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c4_sda */
+		>;
+	};
+
+	uart1_pins: pinmux_uart1_pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x126, PIN_INPUT_PULLUP | MUX_MODE1)	/* uart1_rx */
+			OMAP4_IOPAD(0x128, PIN_INPUT_PULLUP | MUX_MODE1)	/* uart1_tx */
+		>;
+	};
+
+	uart2_pins: pinmux-uart2-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x118, PIN_INPUT_PULLUP | MUX_MODE0)	/* uart2_cts */
+			OMAP4_IOPAD(0x11a, PIN_OUTPUT | MUX_MODE0)		/* uart2_rts */
+			OMAP4_IOPAD(0x11c, PIN_INPUT | MUX_MODE0)		/* uart2_rx */
+			OMAP4_IOPAD(0x11e, PIN_OUTPUT | MUX_MODE0)		/* uart2_tx */
+		>;
+	};
+
+	uart3_pins: pinmux-uart3-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x140, PIN_INPUT_PULLUP | MUX_MODE0)	/* uart3_cts */
+			OMAP4_IOPAD(0x142, PIN_OUTPUT | MUX_MODE0)		/* uart3_rts */
+			OMAP4_IOPAD(0x144, PIN_INPUT | MUX_MODE0)		/* uart3_rx */
+			OMAP4_IOPAD(0x146, PIN_OUTPUT | MUX_MODE0)		/* uart3_tx */
+		>;
+	};
+
+	led_gpio_pins: pinmux-leds-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x156, PIN_OUTPUT | MUX_MODE3)	/* gpio_152 */
+			OMAP4_IOPAD(0x158, PIN_OUTPUT | MUX_MODE3)	/* gpio_153 */
+		>;
+	};
+
+	pinctrl_tempsense: pinmux-pinctrl-tempsense-pins{
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x154, PIN_INPUT_PULLUP | MUX_MODE3)	/* gpio_151 */
+		>;
+	};
+
+	gpmc_pins: gpmc-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x40, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad0 */
+			OMAP4_IOPAD(0x42, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad1 */
+			OMAP4_IOPAD(0x44, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad2 */
+			OMAP4_IOPAD(0x46, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad3 */
+			OMAP4_IOPAD(0x48, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad4 */
+			OMAP4_IOPAD(0x4a, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad5 */
+			OMAP4_IOPAD(0x4c, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad6 */
+			OMAP4_IOPAD(0x4e, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad7 */
+			OMAP4_IOPAD(0x50, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad8 */
+			OMAP4_IOPAD(0x52, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad9 */
+			OMAP4_IOPAD(0x54, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad10 */
+			OMAP4_IOPAD(0x56, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad11 */
+			OMAP4_IOPAD(0x58, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad12 */
+			OMAP4_IOPAD(0x5a, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad13 */
+			OMAP4_IOPAD(0x5c, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad14 */
+			OMAP4_IOPAD(0x5e, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad15 */
+
+			OMAP4_IOPAD(0x60, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a16 */
+			OMAP4_IOPAD(0x62, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a17 */
+			OMAP4_IOPAD(0x64, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a18 */
+			OMAP4_IOPAD(0x66, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a19 */
+			OMAP4_IOPAD(0x68, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a20 */
+			OMAP4_IOPAD(0x6a, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a21 */
+			OMAP4_IOPAD(0x6c, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a22 */
+			OMAP4_IOPAD(0x6e, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a23 */
+
+			OMAP4_IOPAD(0x82, PIN_OUTPUT_PULLDOWN | MUX_MODE0)	/* gpmc_noe */
+			OMAP4_IOPAD(0x84, PIN_OUTPUT_PULLDOWN | MUX_MODE0)	/* gpmc_nwe */
+
+			OMAP4_IOPAD(0x7c, PIN_OUTPUT_PULLDOWN | MUX_MODE0)	/* gpmc_nwp */
+			OMAP4_IOPAD(0x80, PIN_OUTPUT_PULLDOWN | MUX_MODE0)	/* gpmc_nadv_ale */
+			OMAP4_IOPAD(0x86, PIN_OUTPUT_PULLDOWN | MUX_MODE0)	/* gpmc_nbe0_cle */
+			OMAP4_IOPAD(0x8a, PIN_INPUT_PULLUP | MUX_MODE0)		/* gpmc_wait0 */
+			OMAP4_IOPAD(0x8c, PIN_INPUT_PULLUP | MUX_MODE0)		/* gpmc_wait1 */
+
+			OMAP4_IOPAD(0x74, PIN_OUTPUT_PULLUP | MUX_MODE0)	/* gpmc_ncs0 */
+			OMAP4_IOPAD(0x76, PIN_OUTPUT_PULLUP | MUX_MODE0)	/* gpmc_ncs1 */
+			OMAP4_IOPAD(0x92, PIN_OUTPUT_PULLUP | MUX_MODE0)	/* gpmc_ncs5 */
+		>;
+	};
+
+	ethernet_pins: ethernet-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x114, PIN_INPUT | MUX_MODE3)		/* gpio_121 */
+		>;
+	};
+
+	tps62361_pins: pinmux-tps62361-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x19c, PIN_OUTPUT_PULLUP | MUX_MODE3)	/* gpio_182 */
+		>;
+	};
+
+	mmc1_pins: pinmux-mmc1-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x0e2, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_clk */
+			OMAP4_IOPAD(0x0e4, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_cmd */
+			OMAP4_IOPAD(0x0e6, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat0 */
+			OMAP4_IOPAD(0x0e8, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat1 */
+			OMAP4_IOPAD(0x0ea, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat2 */
+			OMAP4_IOPAD(0x0ec, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat3 */
+		>;
+	};
+};
+
+&omap4_pmx_wkup {
+	pinctrl-names = "default";
+	pinctrl-0= <
+		&smartreflex_i2c_pins
+		&fref_xtal_in_pins
+		&fref_clk3_out_pins
+		&sys_pins
+	>;
+
+	ethernet_wkgpio_pins: pinmux-ethernet-wkup-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x66, PIN_OUTPUT | MUX_MODE3)		/* gpio_wk29 */
+		>;
+	};
+
+	smartreflex_i2c_pins: pinmux-smartreflex-i2c-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x4a, PIN_INPUT_PULLUP | MUX_MODE0)		/* sr_scl */
+			OMAP4_IOPAD(0x4c, PIN_INPUT_PULLUP | MUX_MODE0)		/* sr_sda */
+		>;
+	};
+
+	fref_xtal_in_pins: pinmux-fref-xtal-in-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x4e, PIN_OUTPUT | MUX_MODE0)		/* fref_xtal_in */
+		>;
+	};
+
+	fref_clk3_out_pins: pinmux-usb-refclk-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x58, PIN_OUTPUT | MUX_MODE0)		/* fref_clk3_out */
+		>;
+	};
+
+	sys_pins: pinmux-sys-pins {
+		pinctrl-single,pins = <
+			OMAP4_IOPAD(0x5e, PIN_INPUT | MUX_MODE0)		/* sys_32k */
+			OMAP4_IOPAD(0x60, PIN_OUTPUT | MUX_MODE0)		/* sys_nrespwron */
+			OMAP4_IOPAD(0x62, PIN_OUTPUT | MUX_MODE0)		/* sys_nreswarm */
+			OMAP4_IOPAD(0x64, PIN_OUTPUT_PULLUP | MUX_MODE0)	/* sys_pwr_req */
+			OMAP4_IOPAD(0x68, PIN_INPUT | MUX_MODE0)		/* sys_boot6 */
+			OMAP4_IOPAD(0x6a, PIN_INPUT | MUX_MODE0)		/* sys_boot7 */
+		>;
+	};
+};
+
+&i2c1 {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c1_pins>;
+
+	clock-frequency = <400000>;
+
+	twl: twl@48 {
+		compatible = "ti,twl6030";
+		reg = <0x48>;
+		interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; /* IRQ_SYS_1N cascaded to gic */
+	};
+
+	core_vdd_reg: regulator@60 {
+		compatible = "ti,tps62361";
+		reg = <0x60>;
+
+		regulator-name = "tps62361-vout";
+		regulator-min-microvolt = <500000>;
+		regulator-max-microvolt = <1500000>;
+		regulator-coupled-max-spread = <300000>;
+		regulator-max-step-microvolt = <100000>;
+		regulator-boot-on;
+		regulator-always-on;
+		ti,vsel0-gpio = <&gpio5 22 GPIO_ACTIVE_HIGH>;
+		ti,vsel0-state-high;
+	};
+
+	temperature-sensor@4b {
+		compatible = "ti,tmp102";
+		reg = <0x4b>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_tempsense>;
+		interrupt-parent = <&gpio5>;
+		interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
+		#thermal-sensor-cells = <1>;
+	};
+
+	eeprom@50 {
+		compatible = "atmel,24c32";
+		reg = <0x50>;
+	};
+};
+
+#include "twl6030.dtsi"
+#include "twl6030_omap4.dtsi"
+
+&i2c2 {
+	status = "disabled";
+};
+
+&i2c3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c3_pins>;
+	status = "okay";
+
+	clock-frequency = <100000>;
+};
+
+&i2c4 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c4_pins>;
+	status = "disabled";
+
+	clock-frequency = <400000>;
+};
+
+&vmmc {
+	ti,retain-on-reset;
+};
+
+&mmc1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc1_pins>;
+
+	vmmc-supply = <&vmmc>;
+	bus-width = <4>;
+	status = "okay";
+};
+
+&mmc2 {
+	status = "disabled";
+};
+
+&mmc3 {
+	status = "disabled";
+};
+
+&mmc4 {
+	status = "disabled";
+};
+
+&mmc5 {
+	status = "disabled";
+};
+
+&uart1 {
+	status = "okay";
+};
+
+&uart2 {
+	status = "okay";
+};
+
+&uart3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart3_pins>;
+	status = "okay";
+};
+
+&uart4 {
+	status = "disabled";
+};
+
+&elm {
+	status = "okay";
+};
+
+#include "omap-gpmc-smsc9221.dtsi"
+
+&gpmc {
+	ranges = <5 0 0x2c000000 0x01000000>,
+		 <0 0 0x08000000 0x01000000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <
+		&gpmc_pins
+		>;
+	status = "okay";
+
+	nandflash: nand@0,0 {
+		compatible = "ti,omap2-nand";
+		reg = <0 0 4>;
+		interrupt-parent = <&gpmc>;
+		rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>;
+		nand-bus-width = <16>;
+		ti,nand-ecc-opt = "bch8";
+		ti,elm-id=<&elm>;
+		linux,mtd-name = "micron,nand";
+		gpmc,device-nand = "true";
+		gpmc,device-width = <1>;
+
+		gpmc,sync-clk-ps = <0>;
+		gpmc,cs-on-ns = <0>;
+		gpmc,cs-rd-off-ns = <44>;
+		gpmc,cs-wr-off-ns = <44>;
+		gpmc,adv-rd-off-ns = <34>;
+		gpmc,adv-wr-off-ns = <44>;
+		gpmc,we-off-ns = <40>;
+		gpmc,oe-off-ns = <54>;
+		gpmc,access-ns = <64>;
+		gpmc,rd-cycle-ns = <82>;
+		gpmc,wr-cycle-ns = <82>;
+		gpmc,wr-access-ns = <40>;
+		gpmc,wr-data-mux-bus-ns = <0>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+	};
+
+	ethernet@gpmc {
+		reg = <5 0 0xff>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <
+			&ethernet_pins
+			&ethernet_wkgpio_pins
+		>;
+
+		/* Either GPIO 103 or GPIO 121. Use 121 to match the reference design */
+		interrupt-parent = <&gpio4>;
+		interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+	};
+};
diff --git a/arch/arm/boot/dts/omap4-phytec-pcm-959.dts b/arch/arm/boot/dts/omap4-phytec-pcm-959.dts
new file mode 100644
index 000000000000..f323d64660d7
--- /dev/null
+++ b/arch/arm/boot/dts/omap4-phytec-pcm-959.dts
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Innovative Advantage, Inc.
+ */
+/dts-v1/;
+
+#include <dt-bindings/leds/leds-pca9532.h>
+#include "omap4460.dtsi"
+#include "omap4-phytec-pcm-049.dtsi"
+
+/ {
+	model = "Phytec PCM-959 Eval Board";
+	compatible = "phytec,pcm959", "phytec,pcm049", "ti,omap4460", "ti,omap4430", "ti,omap4";
+};
+
+&i2c4 {
+	status = "okay";
+
+	leddim: led@62 {
+		compatible = "nxp,pca9533";
+		reg = <0x62>;
+
+		led-1 {
+			label = "board:red:free_use1";
+			linux,default-trigger = "none";
+			type = <PCA9532_TYPE_LED>;
+		};
+
+		led-2 {
+			label = "board:yellow:free_use2";
+			linux,default-trigger = "none";
+			type = <PCA9532_TYPE_LED>;
+		};
+
+		led-3 {
+			label = "board:yellow:free_use3";
+			linux,default-trigger = "none";
+			type = <PCA9532_TYPE_LED>;
+		};
+
+		led-4 {
+			label = "board:green:free_use4";
+			linux,default-trigger = "none";
+			type = <PCA9532_TYPE_LED>;
+		};
+	};
+};
+
-- 
2.25.1

Re: [PATCH v2 1/2] dt-bindings: arm: omap: add phytec pcm-049 som and pcm-959 dev board

[Krzysztof Kozlowski]

On 24/11/2022 00:32, Colin Foster wrote:
> Add documentation for additional OMAP SOMs and development platforms,
> provided by Phytec as the PCM-049 SOM and the PCM-959 development kit.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
> 
> v1->v2


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Would be nice to convert the file to DT Schema.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] arm: dts: omap4: pcm959: add initial support for phytec pcm959

[Krzysztof Kozlowski]

On 24/11/2022 00:32, Colin Foster wrote:
> The Phytec PCM-959 is a development platform for the Phytec PCM-049 SOM.
> Add initial functionality for the board. The verified interfaces and
> peripherals are listed below for the SOM (PCM-049) and the dev board
> (PCM-959)
> 
> The omap2plus_defconfig was used for testing. Only the On-board LEDs
> required CONFIG_LEDS_PCA9532 addition.
> 

Thank you for your patch. There is something to discuss/improve.

> +
> +	reserved-memory {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		dsp_memory_region: dsp-memory@98000000 {
> +			compatible = "shared-dma-pool";
> +			reg = <0x98000000 0x800000>;
> +			reusable;
> +			status = "okay";

Okay is by default, why adding it? Was it disabled in DTSI file?

> +		};
> +
> +		ipu_memory_region: ipu-memory@98800000 {
> +			compatible = "shared-dma-pool";
> +			reg = <0x98800000 0x7000000>;
> +			reusable;
> +			status = "okay";
> +		};
> +	};
> +
> +	chosen {
> +		stdout-path = &uart3;
> +	};
> +
> +	leds: leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <
> +			&led_gpio_pins
> +		>;

That's unusual syntax.

pinctrl-0 = <&led_gpio_pins>;


> +
> +		led-0 {
> +			label = "modul:red:status1";

You should rather use function and color. label is slowly getting
deprecated.

> +			gpios = <&gpio5 0x18 GPIO_ACTIVE_HIGH>; /* GPIO 152 */
> +			linux,default-trigger = "heartbeat";
> +		};
> +
> +		led-1 {
> +			label = "modul:green:status2";
> +			gpios = <&gpio5 0x19 GPIO_ACTIVE_HIGH>; /* GPIO 153 */
> +			linux,default-trigger = "mmc0";
> +		};
> +	};
> +};
> +
> +&gpio1_target {
> +	ti,no-reset-on-init;
> +};
> +
> +&omap4_pmx_core {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <
> +		&tps62361_pins

Ditto.

> +	>;
> +
> +	i2c1_pins: pinmux-i2c1-pins {
> +		pinctrl-single,pins = <

These can stay like this. But it's exception.

> +			OMAP4_IOPAD(0x122, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c1_scl */
> +			OMAP4_IOPAD(0x124, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c1_sda */
> +		>;
> +	};
> +
> +	i2c3_pins: pinmux-i2c3-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x12a, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c3_scl */
> +			OMAP4_IOPAD(0x12c, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c3_sda */
> +		>;
> +	};
> +
> +	i2c4_pins: pinmux-i2c4-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x12e, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c4_scl */
> +			OMAP4_IOPAD(0x130, PIN_INPUT_PULLUP | MUX_MODE0)	/* i2c4_sda */
> +		>;
> +	};
> +
> +	uart1_pins: pinmux_uart1_pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x126, PIN_INPUT_PULLUP | MUX_MODE1)	/* uart1_rx */
> +			OMAP4_IOPAD(0x128, PIN_INPUT_PULLUP | MUX_MODE1)	/* uart1_tx */
> +		>;
> +	};
> +
> +	uart2_pins: pinmux-uart2-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x118, PIN_INPUT_PULLUP | MUX_MODE0)	/* uart2_cts */
> +			OMAP4_IOPAD(0x11a, PIN_OUTPUT | MUX_MODE0)		/* uart2_rts */
> +			OMAP4_IOPAD(0x11c, PIN_INPUT | MUX_MODE0)		/* uart2_rx */
> +			OMAP4_IOPAD(0x11e, PIN_OUTPUT | MUX_MODE0)		/* uart2_tx */
> +		>;
> +	};
> +
> +	uart3_pins: pinmux-uart3-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x140, PIN_INPUT_PULLUP | MUX_MODE0)	/* uart3_cts */
> +			OMAP4_IOPAD(0x142, PIN_OUTPUT | MUX_MODE0)		/* uart3_rts */
> +			OMAP4_IOPAD(0x144, PIN_INPUT | MUX_MODE0)		/* uart3_rx */
> +			OMAP4_IOPAD(0x146, PIN_OUTPUT | MUX_MODE0)		/* uart3_tx */
> +		>;
> +	};
> +
> +	led_gpio_pins: pinmux-leds-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x156, PIN_OUTPUT | MUX_MODE3)	/* gpio_152 */
> +			OMAP4_IOPAD(0x158, PIN_OUTPUT | MUX_MODE3)	/* gpio_153 */
> +		>;
> +	};
> +
> +	pinctrl_tempsense: pinmux-pinctrl-tempsense-pins{
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x154, PIN_INPUT_PULLUP | MUX_MODE3)	/* gpio_151 */
> +		>;
> +	};
> +
> +	gpmc_pins: gpmc-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x40, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad0 */
> +			OMAP4_IOPAD(0x42, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad1 */
> +			OMAP4_IOPAD(0x44, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad2 */
> +			OMAP4_IOPAD(0x46, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad3 */
> +			OMAP4_IOPAD(0x48, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad4 */
> +			OMAP4_IOPAD(0x4a, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad5 */
> +			OMAP4_IOPAD(0x4c, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad6 */
> +			OMAP4_IOPAD(0x4e, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad7 */
> +			OMAP4_IOPAD(0x50, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad8 */
> +			OMAP4_IOPAD(0x52, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad9 */
> +			OMAP4_IOPAD(0x54, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad10 */
> +			OMAP4_IOPAD(0x56, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad11 */
> +			OMAP4_IOPAD(0x58, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad12 */
> +			OMAP4_IOPAD(0x5a, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad13 */
> +			OMAP4_IOPAD(0x5c, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad14 */
> +			OMAP4_IOPAD(0x5e, PIN_INPUT_PULLDOWN | MUX_MODE0)	/* gpmc_ad15 */
> +
> +			OMAP4_IOPAD(0x60, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a16 */
> +			OMAP4_IOPAD(0x62, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a17 */
> +			OMAP4_IOPAD(0x64, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a18 */
> +			OMAP4_IOPAD(0x66, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a19 */
> +			OMAP4_IOPAD(0x68, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a20 */
> +			OMAP4_IOPAD(0x6a, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a21 */
> +			OMAP4_IOPAD(0x6c, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a22 */
> +			OMAP4_IOPAD(0x6e, PIN_OUTPUT | MUX_MODE0)		/* gpmc_a23 */
> +
> +			OMAP4_IOPAD(0x82, PIN_OUTPUT_PULLDOWN | MUX_MODE0)	/* gpmc_noe */
> +			OMAP4_IOPAD(0x84, PIN_OUTPUT_PULLDOWN | MUX_MODE0)	/* gpmc_nwe */
> +
> +			OMAP4_IOPAD(0x7c, PIN_OUTPUT_PULLDOWN | MUX_MODE0)	/* gpmc_nwp */
> +			OMAP4_IOPAD(0x80, PIN_OUTPUT_PULLDOWN | MUX_MODE0)	/* gpmc_nadv_ale */
> +			OMAP4_IOPAD(0x86, PIN_OUTPUT_PULLDOWN | MUX_MODE0)	/* gpmc_nbe0_cle */
> +			OMAP4_IOPAD(0x8a, PIN_INPUT_PULLUP | MUX_MODE0)		/* gpmc_wait0 */
> +			OMAP4_IOPAD(0x8c, PIN_INPUT_PULLUP | MUX_MODE0)		/* gpmc_wait1 */
> +
> +			OMAP4_IOPAD(0x74, PIN_OUTPUT_PULLUP | MUX_MODE0)	/* gpmc_ncs0 */
> +			OMAP4_IOPAD(0x76, PIN_OUTPUT_PULLUP | MUX_MODE0)	/* gpmc_ncs1 */
> +			OMAP4_IOPAD(0x92, PIN_OUTPUT_PULLUP | MUX_MODE0)	/* gpmc_ncs5 */
> +		>;
> +	};
> +
> +	ethernet_pins: ethernet-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x114, PIN_INPUT | MUX_MODE3)		/* gpio_121 */
> +		>;
> +	};
> +
> +	tps62361_pins: pinmux-tps62361-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x19c, PIN_OUTPUT_PULLUP | MUX_MODE3)	/* gpio_182 */
> +		>;
> +	};
> +
> +	mmc1_pins: pinmux-mmc1-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x0e2, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_clk */
> +			OMAP4_IOPAD(0x0e4, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_cmd */
> +			OMAP4_IOPAD(0x0e6, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat0 */
> +			OMAP4_IOPAD(0x0e8, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat1 */
> +			OMAP4_IOPAD(0x0ea, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat2 */
> +			OMAP4_IOPAD(0x0ec, PIN_INPUT_PULLUP | MUX_MODE0)	/* sdmmc1_dat3 */
> +		>;
> +	};
> +};
> +
> +&omap4_pmx_wkup {
> +	pinctrl-names = "default";
> +	pinctrl-0= <
> +		&smartreflex_i2c_pins
> +		&fref_xtal_in_pins
> +		&fref_clk3_out_pins
> +		&sys_pins

Squash the lines.

> +	>;
> +
> +	ethernet_wkgpio_pins: pinmux-ethernet-wkup-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x66, PIN_OUTPUT | MUX_MODE3)		/* gpio_wk29 */
> +		>;
> +	};
> +
> +	smartreflex_i2c_pins: pinmux-smartreflex-i2c-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x4a, PIN_INPUT_PULLUP | MUX_MODE0)		/* sr_scl */
> +			OMAP4_IOPAD(0x4c, PIN_INPUT_PULLUP | MUX_MODE0)		/* sr_sda */
> +		>;
> +	};
> +
> +	fref_xtal_in_pins: pinmux-fref-xtal-in-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x4e, PIN_OUTPUT | MUX_MODE0)		/* fref_xtal_in */
> +		>;
> +	};
> +
> +	fref_clk3_out_pins: pinmux-usb-refclk-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x58, PIN_OUTPUT | MUX_MODE0)		/* fref_clk3_out */
> +		>;
> +	};
> +
> +	sys_pins: pinmux-sys-pins {
> +		pinctrl-single,pins = <
> +			OMAP4_IOPAD(0x5e, PIN_INPUT | MUX_MODE0)		/* sys_32k */
> +			OMAP4_IOPAD(0x60, PIN_OUTPUT | MUX_MODE0)		/* sys_nrespwron */
> +			OMAP4_IOPAD(0x62, PIN_OUTPUT | MUX_MODE0)		/* sys_nreswarm */
> +			OMAP4_IOPAD(0x64, PIN_OUTPUT_PULLUP | MUX_MODE0)	/* sys_pwr_req */
> +			OMAP4_IOPAD(0x68, PIN_INPUT | MUX_MODE0)		/* sys_boot6 */
> +			OMAP4_IOPAD(0x6a, PIN_INPUT | MUX_MODE0)		/* sys_boot7 */
> +		>;
> +	};
> +};
> +
> +&i2c1 {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c1_pins>;
> +
> +	clock-frequency = <400000>;
> +
> +	twl: twl@48 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +		compatible = "ti,twl6030";
> +		reg = <0x48>;
> +		interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; /* IRQ_SYS_1N cascaded to gic */
> +	};
> +
> +	core_vdd_reg: regulator@60 {
> +		compatible = "ti,tps62361";
> +		reg = <0x60>;
> +
> +		regulator-name = "tps62361-vout";
> +		regulator-min-microvolt = <500000>;
> +		regulator-max-microvolt = <1500000>;
> +		regulator-coupled-max-spread = <300000>;
> +		regulator-max-step-microvolt = <100000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +		ti,vsel0-gpio = <&gpio5 22 GPIO_ACTIVE_HIGH>;
> +		ti,vsel0-state-high;
> +	};
> +
> +	temperature-sensor@4b {
> +		compatible = "ti,tmp102";
> +		reg = <0x4b>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_tempsense>;
> +		interrupt-parent = <&gpio5>;
> +		interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
> +		#thermal-sensor-cells = <1>;
> +	};
> +
> +	eeprom@50 {
> +		compatible = "atmel,24c32";
> +		reg = <0x50>;
> +	};
> +};
> +
> +#include "twl6030.dtsi"
> +#include "twl6030_omap4.dtsi"

Includes are usually at beginning. Is it a convention for all OMAP DTSes
to add it in the middle?

> +
> +&i2c2 {
> +	status = "disabled";
> +};
> +
> +&i2c3 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c3_pins>;
> +	status = "okay";
> +
> +	clock-frequency = <100000>;
> +};
> +
> +&i2c4 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c4_pins>;
> +	status = "disabled";
> +
> +	clock-frequency = <400000>;
> +};
> +
> +&vmmc {
> +	ti,retain-on-reset;
> +};
> +
> +&mmc1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc1_pins>;
> +
> +	vmmc-supply = <&vmmc>;
> +	bus-width = <4>;
> +	status = "okay";
> +};
> +
> +&mmc2 {
> +	status = "disabled";
> +};
> +
> +&mmc3 {
> +	status = "disabled";
> +};
> +
> +&mmc4 {
> +	status = "disabled";
> +};
> +
> +&mmc5 {
> +	status = "disabled";
> +};
> +
> +&uart1 {
> +	status = "okay";
> +};
> +
> +&uart2 {
> +	status = "okay";
> +};
> +
> +&uart3 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart3_pins>;
> +	status = "okay";
> +};
> +
> +&uart4 {
> +	status = "disabled";
> +};
> +
> +&elm {
> +	status = "okay";
> +};
> +
> +#include "omap-gpmc-smsc9221.dtsi"
> +
> +&gpmc {
> +	ranges = <5 0 0x2c000000 0x01000000>,
> +		 <0 0 0x08000000 0x01000000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <
> +		&gpmc_pins
> +		>;

Merge/squash lines.

> +	status = "okay";
> +
> +	nandflash: nand@0,0 {
> +		compatible = "ti,omap2-nand";
> +		reg = <0 0 4>;
> +		interrupt-parent = <&gpmc>;
> +		rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>;
> +		nand-bus-width = <16>;
> +		ti,nand-ecc-opt = "bch8";
> +		ti,elm-id=<&elm>;
> +		linux,mtd-name = "micron,nand";
> +		gpmc,device-nand = "true";
> +		gpmc,device-width = <1>;
> +
> +		gpmc,sync-clk-ps = <0>;
> +		gpmc,cs-on-ns = <0>;
> +		gpmc,cs-rd-off-ns = <44>;
> +		gpmc,cs-wr-off-ns = <44>;
> +		gpmc,adv-rd-off-ns = <34>;
> +		gpmc,adv-wr-off-ns = <44>;
> +		gpmc,we-off-ns = <40>;
> +		gpmc,oe-off-ns = <54>;
> +		gpmc,access-ns = <64>;
> +		gpmc,rd-cycle-ns = <82>;
> +		gpmc,wr-cycle-ns = <82>;
> +		gpmc,wr-access-ns = <40>;
> +		gpmc,wr-data-mux-bus-ns = <0>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +	};
> +
> +	ethernet@gpmc {
> +		reg = <5 0 0xff>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <
> +			&ethernet_pins
> +			&ethernet_wkgpio_pins
> +		>;
> +
> +		/* Either GPIO 103 or GPIO 121. Use 121 to match the reference design */
> +		interrupt-parent = <&gpio4>;
> +		interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
> +	};
> +};
> diff --git a/arch/arm/boot/dts/omap4-phytec-pcm-959.dts b/arch/arm/boot/dts/omap4-phytec-pcm-959.dts
> new file mode 100644
> index 000000000000..f323d64660d7
> --- /dev/null
> +++ b/arch/arm/boot/dts/omap4-phytec-pcm-959.dts
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Innovative Advantage, Inc.
> + */
> +/dts-v1/;
> +
> +#include <dt-bindings/leds/leds-pca9532.h>
> +#include "omap4460.dtsi"
> +#include "omap4-phytec-pcm-049.dtsi"
> +
> +/ {
> +	model = "Phytec PCM-959 Eval Board";
> +	compatible = "phytec,pcm959", "phytec,pcm049", "ti,omap4460", "ti,omap4430", "ti,omap4";
> +};
> +
> +&i2c4 {
> +	status = "okay";
> +
> +	leddim: led@62 {
> +		compatible = "nxp,pca9533";
> +		reg = <0x62>;
> +
> +		led-1 {
> +			label = "board:red:free_use1";
> +			linux,default-trigger = "none";
> +			type = <PCA9532_TYPE_LED>;
> +		};
> +
> +		led-2 {
> +			label = "board:yellow:free_use2";
> +			linux,default-trigger = "none";
> +			type = <PCA9532_TYPE_LED>;
> +		};
> +
> +		led-3 {
> +			label = "board:yellow:free_use3";
> +			linux,default-trigger = "none";
> +			type = <PCA9532_TYPE_LED>;
> +		};
> +
> +		led-4 {
> +			label = "board:green:free_use4";
> +			linux,default-trigger = "none";
> +			type = <PCA9532_TYPE_LED>;
> +		};
> +	};
> +};
> +

Drop trailing new line.


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: arm: omap: add phytec pcm-049 som and pcm-959 dev board

[Colin Foster]

On Thu, Nov 24, 2022 at 12:53:32PM +0100, Krzysztof Kozlowski wrote:
> On 24/11/2022 00:32, Colin Foster wrote:
> > Add documentation for additional OMAP SOMs and development platforms,
> > provided by Phytec as the PCM-049 SOM and the PCM-959 development kit.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> > 
> > v1->v2
> 
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Would be nice to convert the file to DT Schema.

Agreed. Thanks for the Ack despite this.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 2/2] arm: dts: omap4: pcm959: add initial support for phytec pcm959

[Colin Foster]

Hi Krzysztof,

I appreciate your feedback!

On Thu, Nov 24, 2022 at 12:57:48PM +0100, Krzysztof Kozlowski wrote:
> On 24/11/2022 00:32, Colin Foster wrote:
> > The Phytec PCM-959 is a development platform for the Phytec PCM-049 SOM.
> > Add initial functionality for the board. The verified interfaces and
> > peripherals are listed below for the SOM (PCM-049) and the dev board
> > (PCM-959)
> > 
> > The omap2plus_defconfig was used for testing. Only the On-board LEDs
> > required CONFIG_LEDS_PCA9532 addition.
> > 
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +
> > +	reserved-memory {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		ranges;
> > +
> > +		dsp_memory_region: dsp-memory@98000000 {
> > +			compatible = "shared-dma-pool";
> > +			reg = <0x98000000 0x800000>;
> > +			reusable;
> > +			status = "okay";
> 
> Okay is by default, why adding it? Was it disabled in DTSI file?

No. This (and a lot of your points) are based on the fact that I mostly
started from omap4-panda-common.dtsi. I'll remove this and the one below
(and any others I come across, armed with this information).

> 
> > +		};
> > +
> > +		ipu_memory_region: ipu-memory@98800000 {
> > +			compatible = "shared-dma-pool";
> > +			reg = <0x98800000 0x7000000>;
> > +			reusable;
> > +			status = "okay";
> > +		};
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = &uart3;
> > +	};
> > +
> > +	leds: leds {
> > +		compatible = "gpio-leds";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <
> > +			&led_gpio_pins
> > +		>;
> 
> That's unusual syntax.
> 
> pinctrl-0 = <&led_gpio_pins>;

Noted. I'll fix it up. In the unlikely scenario that someone else tries
to support something similar to the OMAP4 pandaboard, it might be worth
sweeping through those .dts[,i] files and applying these suggestions.

> 
> 
> > +
> > +		led-0 {
> > +			label = "modul:red:status1";
> 
> You should rather use function and color. label is slowly getting
> deprecated.

Thanks for this info. I didn't know this.

For background, the naming was intended to remain consistent with the
vendor BSP, but there isn't _really_ a need for me to do that.

...

>+
> > +&omap4_pmx_wkup {
> > +	pinctrl-names = "default";
> > +	pinctrl-0= <
> > +		&smartreflex_i2c_pins
> > +		&fref_xtal_in_pins
> > +		&fref_clk3_out_pins
> > +		&sys_pins
> 
> Squash the lines.
> 
> > +	>;
...
> > +
> > +&i2c1 {
> > +	status = "okay";
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&i2c1_pins>;
> > +
> > +	clock-frequency = <400000>;
> > +
> > +	twl: twl@48 {
> 
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

My apologies that you had to send this to me again. I'll make this
change here as well, noting it is another divergence from pandaboard
(which I understand is a good thing)

> 
> > +		compatible = "ti,twl6030";
> > +		reg = <0x48>;
> > +		interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; /* IRQ_SYS_1N cascaded to gic */
> > +	};
> > +
> > +	core_vdd_reg: regulator@60 {
> > +		compatible = "ti,tps62361";
> > +		reg = <0x60>;
> > +
> > +		regulator-name = "tps62361-vout";
> > +		regulator-min-microvolt = <500000>;
> > +		regulator-max-microvolt = <1500000>;
> > +		regulator-coupled-max-spread = <300000>;
> > +		regulator-max-step-microvolt = <100000>;
> > +		regulator-boot-on;
> > +		regulator-always-on;
> > +		ti,vsel0-gpio = <&gpio5 22 GPIO_ACTIVE_HIGH>;
> > +		ti,vsel0-state-high;
> > +	};
> > +
> > +	temperature-sensor@4b {
> > +		compatible = "ti,tmp102";
> > +		reg = <0x4b>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_tempsense>;
> > +		interrupt-parent = <&gpio5>;
> > +		interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
> > +		#thermal-sensor-cells = <1>;
> > +	};
> > +
> > +	eeprom@50 {
> > +		compatible = "atmel,24c32";
> > +		reg = <0x50>;
> > +	};
> > +};
> > +
> > +#include "twl6030.dtsi"
> > +#include "twl6030_omap4.dtsi"
> 
> Includes are usually at beginning. Is it a convention for all OMAP DTSes
> to add it in the middle?

I thought it was curious as well. My hunch was these includes needed to
be after the twl node in order to resolve a reference earlier in the
file. I understand that to be incorrect... but thinking out loud maybe
that was the case for early implementations of DTC...?

I'll move it to the top and make sure it works. But yes, every .dts[i]
of the OMAP4 that use twl6030.dtsi all have these include near the end
of the file (omap4-sdp.dts, opam4-kc1.dts, omap4-var-som-om44.dtsi,
omap4-panda-common.dtsi, omap4-duovero.dtsi). "twl" node names... etc.

...

>> +
> > +&gpmc {
> > +	ranges = <5 0 0x2c000000 0x01000000>,
> > +		 <0 0 0x08000000 0x01000000>;
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <
> > +		&gpmc_pins
> > +		>;
> 
> Merge/squash lines.

Ack

...
> > diff --git a/arch/arm/boot/dts/omap4-phytec-pcm-959.dts b/arch/arm/boot/dts/omap4-phytec-pcm-959.dts
> > new file mode 100644
> > index 000000000000..f323d64660d7
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/omap4-phytec-pcm-959.dts
> > @@ -0,0 +1,48 @@
...
> > +
> > +		led-4 {
> > +			label = "board:green:free_use4";
> > +			linux,default-trigger = "none";
> > +			type = <PCA9532_TYPE_LED>;
> > +		};
> > +	};
> > +};
> > +
> 
> Drop trailing new line.

Ack

> 
> 
> Best regards,
> Krzysztof
>