[PATCH 4/4] arm64: dts: freescale: Add support for phyBOARD-Pollux-i.MX8MP

Mon Dec 7 08:35:33 EST 2020

Hello Krzysztof,

Am Montag, den 07.12.2020, 13:09 +0100 schrieb Krzysztof Kozlowski:
> On Fri, Dec 04, 2020 at 09:33:02PM +0100, Teresa Remmet wrote:
> > Add initial support for phyBOARD-Pollux-i.MX8MP.
> > Supported basic features:
> > 	* eMMC
> > 	* i2c EEPROM
> > 	* i2c RTC
> > 	* i2c LED
> > 	* PMIC
> > 	* debug UART
> > 	* SD card
> > 	* 1Gbit Ethernet (fec)
> > 	* watchdog
> > 
> > Signed-off-by: Teresa Remmet <t.remmet at phytec.de>
> > ---
> >  arch/arm64/boot/dts/freescale/Makefile             |   1 +
> >  .../dts/freescale/imx8mp-phyboard-pollux-rdk.dts   |  16 ++
> >  .../boot/dts/freescale/imx8mp-phyboard-pollux.dtsi | 152
> > ++++++++++
> >  .../boot/dts/freescale/imx8mp-phycore-som.dtsi     | 319
> > +++++++++++++++++++++
> >  4 files changed, 488 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-phyboard-
> > pollux-rdk.dts
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-phyboard-
> > pollux.dtsi
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-phycore-
> > som.dtsi
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/Makefile
> > b/arch/arm64/boot/dts/freescale/Makefile
> > index acfb8af45912..a43b496678be 100644
> > --- a/arch/arm64/boot/dts/freescale/Makefile
> > +++ b/arch/arm64/boot/dts/freescale/Makefile
> > @@ -37,6 +37,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mn-evk.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mn-ddr4-evk.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mn-var-som-symphony.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk.dtb
> > +dtb-$(CONFIG_ARCH_MXC) += imx8mp-phyboard-pollux-rdk.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-hummingboard-pulse.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-
> > rdk.dts b/arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-
> > rdk.dts
> > new file mode 100644
> > index 000000000000..dd64be32c99d
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts
> > @@ -0,0 +1,16 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 PHYTEC Messtechnik GmbH
> > + * Author: Teresa Remmet <t.remmet at phytec.de>
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "imx8mp-phycore-som.dtsi"
> > +#include "imx8mp-phyboard-pollux.dtsi"
> > +
> > +/ {
> > +	model = "PHYTEC phyBOARD-Pollux i.MX8MP";
> > +	compatible = "phytec,imx8mp-phyboard-pollux-rdk",
> > +		     "phytec,imx8mp-phycore-som", "fsl,imx8mp";
> 
> This is the purpose of this file? Why having a DTS to include DTSI
> only?
> Usually there is just DTSI for SOM and DTS fot the board.

we have different options for the SoMs. Like SPI-NOR flash mounted or
not. We usually add this to the SoM include, but disable it. We enable
this in the dts if mounted. This makes it easy to generate different
device trees for different SoM options. So far upstream is not every
feature supported. So we don't do anything in the dts yet. But I want
to setup the layout already.

I hope this makes it clear.

> 
> > +};
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-phyboard-
> > pollux.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-phyboard-
> > pollux.dtsi
> > new file mode 100644
> > index 000000000000..dbeaa27eb043
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux.dtsi
> > @@ -0,0 +1,152 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 PHYTEC Messtechnik GmbH
> > + * Author: Teresa Remmet <t.remmet at phytec.de>
> > + */
> > +
> > +#include <dt-bindings/leds/leds-pca9532.h>
> > +#include <dt-bindings/pwm/pwm.h>
> > +
> > +/ {
> > +	chosen {
> > +		stdout-path = &uart2;
> > +	};
> > +
> > +	reg_usdhc2_vmmc: regulator-usdhc2 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "VSD_3V3";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>;
> > +		enable-active-high;
> > +		startup-delay-us = <100>;
> > +		off-on-delay-us = <12000>;
> > +	};
> > +};
> > +
> > +&i2c2 {
> > +	clock-frequency = <400000>;
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_i2c2>;
> > +	pinctrl-1 = <&pinctrl_i2c2_gpio>;
> > +	sda-gpios = <&gpio5 17 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +	scl-gpios = <&gpio5 16 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +	status = "okay";
> > +
> > +	eeprom at 51 {
> > +		compatible = "atmel,24c02";
> > +		reg = <0x51>;
> > +		pagesize = <16>;
> > +		status = "okay";
> > +	};
> > +
> > +	leddimmer at 62 {
> 
> I think name "leds" is more appropriate.

okay I will change it.

> 
> > +		compatible = "nxp,pca9533";
> > +		reg = <0x62>;
> > +		status = "okay";
> > +
> > +		led1 {
> > +			label = "red:user1";
> > +			type = <PCA9532_TYPE_LED>;
> > +		};
> > +
> > +		led2 {
> > +			label = "green:user2";
> > +			type = <PCA9532_TYPE_LED>;
> > +		};
> > +
> > +		led3 {
> > +			label = "blue:user3";
> > +			type = <PCA9532_TYPE_LED>;
> > +		};
> > +	};
> > +};
> > +
> > +&snvs_pwrkey {
> > +	status = "okay";
> > +};
> > +
> > +/* debug console */
> > +&uart2 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_uart2>;
> > +	status = "okay";
> > +};
> > +
> > +/* SD-Card */
> > +&usdhc2 {
> > +	pinctrl-names = "default", "state_100mhz", "state_200mhz";
> > +	pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_pins>;
> > +	pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_pins>;
> > +	pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_pins>;
> > +	cd-gpios = <&gpio2 12 GPIO_ACTIVE_LOW>;
> > +	vmmc-supply = <&reg_usdhc2_vmmc>;
> > +	bus-width = <4>;
> > +	status = "okay";
> > +};
> > +
> > +&iomuxc {
> > +	pinctrl_i2c2: i2c2grp {
> > +		fsl,pins = <
> > +			MX8MP_IOMUXC_I2C2_SCL__I2C2_SCL		0x4
> > 00001c3
> > +			MX8MP_IOMUXC_I2C2_SDA__I2C2_SDA		0x4
> > 00001c3
> > +		>;
> > +	};
> > +
> > +	pinctrl_i2c2_gpio: i2c2gpiogrp {
> > +		fsl,pins = <
> > +			MX8MP_IOMUXC_I2C2_SCL__GPIO5_IO16	0x1e3
> > +			MX8MP_IOMUXC_I2C2_SDA__GPIO5_IO17	0x1e3
> > +		>;
> > +	};
> > +
> > +	pinctrl_uart2: uart2grp {
> > +		fsl,pins = <
> > +			MX8MP_IOMUXC_UART2_RXD__UART2_DCE_RX	0x4
> > 9
> > +			MX8MP_IOMUXC_UART2_TXD__UART2_DCE_TX	0x4
> > 9
> > +		>;
> > +	};
> > +
> > +	pinctrl_usdhc2_pins: usdhc2-gpiogrp {
> > +		fsl,pins = <
> > +			MX8MP_IOMUXC_SD2_CD_B__GPIO2_IO12	0x1c4
> > +			MX8MP_IOMUXC_SD2_RESET_B__GPIO2_IO19	0x4
> > 1
> > +		>;
> > +	};
> > +
> > +	pinctrl_usdhc2: usdhc2grp {
> > +		fsl,pins = <
> > +			MX8MP_IOMUXC_SD2_CLK__USDHC2_CLK	0x190
> > +			MX8MP_IOMUXC_SD2_CMD__USDHC2_CMD	0x1d0
> > +			MX8MP_IOMUXC_SD2_DATA0__USDHC2_DATA0	0x1
> > d0
> > +			MX8MP_IOMUXC_SD2_DATA1__USDHC2_DATA1	0x1
> > d0
> > +			MX8MP_IOMUXC_SD2_DATA2__USDHC2_DATA2	0x1
> > d0
> > +			MX8MP_IOMUXC_SD2_DATA3__USDHC2_DATA3	0x1
> > d0
> > +			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT	0xc
> > 1
> > +		>;
> > +	};
> > +
> > +	pinctrl_usdhc2_100mhz: usdhc2-100mhzgrp {
> > +		fsl,pins = <
> > +			MX8MP_IOMUXC_SD2_CLK__USDHC2_CLK	0x194
> > +			MX8MP_IOMUXC_SD2_CMD__USDHC2_CMD	0x1d4
> > +			MX8MP_IOMUXC_SD2_DATA0__USDHC2_DATA0	0x1
> > d4
> > +			MX8MP_IOMUXC_SD2_DATA1__USDHC2_DATA1	0x1
> > d4
> > +			MX8MP_IOMUXC_SD2_DATA2__USDHC2_DATA2	0x1
> > d4
> > +			MX8MP_IOMUXC_SD2_DATA3__USDHC2_DATA3	0x1
> > d4
> > +			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT	0xc
> > 1
> > +		>;
> > +	};
> > +
> > +	pinctrl_usdhc2_200mhz: usdhc2-200mhzgrp {
> > +		fsl,pins = <
> > +			MX8MP_IOMUXC_SD2_CLK__USDHC2_CLK	0x196
> > +			MX8MP_IOMUXC_SD2_CMD__USDHC2_CMD	0x1d6
> > +			MX8MP_IOMUXC_SD2_DATA0__USDHC2_DATA0	0x1
> > d6
> > +			MX8MP_IOMUXC_SD2_DATA1__USDHC2_DATA1	0x1
> > d6
> > +			MX8MP_IOMUXC_SD2_DATA2__USDHC2_DATA2	0x1
> > d6
> > +			MX8MP_IOMUXC_SD2_DATA3__USDHC2_DATA3	0x1
> > d6
> > +			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT	0xc
> > 1
> > +		>;
> > +	};
> > +};
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-phycore-som.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mp-phycore-som.dtsi
> > new file mode 100644
> > index 000000000000..e1fdfebd8142
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-phycore-som.dtsi
> > @@ -0,0 +1,319 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 PHYTEC Messtechnik GmbH
> > + * Author: Teresa Remmet <t.remmet at phytec.de>
> > + */
> > +
> > +#include <dt-bindings/net/ti-dp83867.h>
> > +#include "imx8mp.dtsi"
> > +
> > +/ {
> > +	model = "PHYTEC phyCORE-i.MX8MP";
> > +	compatible = "phytec,imx8mp-phycore-som", "fsl,imx8mp";
> > +
> > +	aliases {
> > +		rtc0 = &rv3028;
> > +		rtc1 = &snvs_rtc;
> > +	};
> > +
> > +	memory at 40000000 {
> > +		device_type = "memory";
> > +		reg = <0x0 0x40000000 0 0x80000000>;
> > +	};
> > +
> > +	rtcclkout: rv3028-clkout {
> 
> Is it really a separate oscillator giving 32 kHz? Or maybe this is
> actually part of PMIC?

It is a clock out of the used i2c rtc. Which I actually trying to
disable. As it is not connected. But it is enabled as default.

> 
> In any case node name should reflect class of a device so just
> "clock".

Changing that.

> 
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <32768>;
> > +		clock-output-names = "rv3028-clkout";
> > +		status = "disabled";
> > +	};
> > +};
> > +
> > +&A53_0 {
> > +	cpu-supply = <&buck2>;
> > +};
> > +
> > +&A53_1 {
> > +	cpu-supply = <&buck2>;
> > +};
> > +
> > +&A53_2 {
> > +	cpu-supply = <&buck2>;
> > +};
> > +
> > +&A53_3 {
> > +	cpu-supply = <&buck2>;
> > +};
> > +
> > +/* ethernet 1 */
> > +&fec {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_fec>;
> > +	phy-mode = "rgmii-id";
> > +	phy-handle = <&ethphy1>;
> > +	fsl,magic-packet;
> > +	status = "okay";
> > +
> > +	mdio {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		ethphy1: ethernet-phy at 0 {
> > +			compatible = "ethernet-phy-ieee802.3-c22";
> > +			reg = <0>;
> > +			interrupt-parent = <&gpio1>;
> > +			interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
> > +			ti,rx-internal-delay =
> > <DP83867_RGMIIDCTL_2_00_NS>;
> > +			ti,tx-internal-delay =
> > <DP83867_RGMIIDCTL_2_00_NS>;
> > +			ti,fifo-depth =
> > <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
> > +			ti,clk-output-sel = <DP83867_CLK_O_SEL_OFF>;
> > +			enet-phy-lane-no-swap;
> > +		};
> > +	};
> > +};
> > +
> > +&i2c1 {
> > +	clock-frequency = <400000>;
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_i2c1>;
> > +	pinctrl-1 = <&pinctrl_i2c1_gpio>;
> > +	sda-gpios = <&gpio5 15 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +	scl-gpios = <&gpio5 14 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +	status = "okay";
> > +
> > +	pmic: pca9450 at 25 {
> 
> Node name: pmic.

Will be fixed.

> 
> > +		reg = <0x25>;
> > +		compatible = "nxp,pca9450c";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_pmic>;
> > +		interrupt-parent = <&gpio4>;
> > +		interrupts = <18 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +		regulators {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			buck1: BUCK1 {
> > +				reg = <0>;
> 
> Does this all pass dtc W=1 builds and dtschema? The node name does
> not
> have unit address. Bindings explicitly do not allow it... This looks
> wrong.

The reg property is a leftover and not correct. I will remove this and
others below. Same with #address-cells and #size-cells.

> 
> > +				regulator-compatible = "BUCK1";
> > +				regulator-min-microvolt = <600000>;
> > +				regulator-max-microvolt = <2187500>;
> > +				regulator-boot-on;
> > +				regulator-always-on;
> > +				regulator-ramp-delay = <3125>;
> > +			};
> > +
> > +			buck2: BUCK2 {
> > +				reg = <1>;
> > +				regulator-compatible = "BUCK2";
> > +				regulator-min-microvolt = <600000>;
> > +				regulator-max-microvolt = <2187500>;
> > +				regulator-boot-on;
> > +				regulator-always-on;
> > +				regulator-ramp-delay = <3125>;
> > +			};
> > +
> > +			buck4: BUCK4 {
> > +				reg = <3>;
> > +				regulator-compatible = "BUCK4";
> > +				regulator-min-microvolt = <600000>;
> > +				regulator-max-microvolt = <3400000>;
> > +				regulator-boot-on;
> > +				regulator-always-on;
> > +			};
> > +
> > +			buck5: BUCK5 {
> > +				reg = <4>;
> > +				regulator-compatible = "BUCK5";
> > +				regulator-min-microvolt = <600000>;
> > +				regulator-max-microvolt = <3400000>;
> > +				regulator-boot-on;
> > +				regulator-always-on;
> > +			};
> > +
> > +			buck6: BUCK6 {
> > +				reg = <5>;
> > +				regulator-compatible = "BUCK6";
> > +				regulator-min-microvolt = <600000>;
> > +				regulator-max-microvolt = <3400000>;
> > +				regulator-boot-on;
> > +				regulator-always-on;
> > +			};
> > +
> > +			ldo1: LDO1 {
> > +				reg = <6>;
> > +				regulator-compatible = "LDO1";
> > +				regulator-min-microvolt = <1600000>;
> > +				regulator-max-microvolt = <3300000>;
> > +				regulator-boot-on;
> > +				regulator-always-on;
> > +			};
> > +
> > +			ldo2: LDO2 {
> > +				reg = <7>;
> > +				regulator-compatible = "LDO2";
> > +				regulator-min-microvolt = <800000>;
> > +				regulator-max-microvolt = <1150000>;
> > +				regulator-boot-on;
> > +				regulator-always-on;
> > +			};
> > +
> > +			ldo3: LDO3 {
> > +				reg = <8>;
> > +				regulator-compatible = "LDO3";
> > +				regulator-min-microvolt = <800000>;
> > +				regulator-max-microvolt = <3300000>;
> > +				regulator-boot-on;
> > +				regulator-always-on;
> > +			};
> > +
> > +			ldo4: LDO4 {
> > +				reg = <9>;
> > +				regulator-compatible = "LDO4";
> > +				regulator-min-microvolt = <800000>;
> > +				regulator-max-microvolt = <3300000>;
> > +				regulator-boot-on;
> > +				regulator-always-on;
> > +			};
> > +
> > +			ldo5: LDO5 {
> > +				reg = <10>;
> > +				regulator-compatible = "LDO5";
> > +				regulator-min-microvolt = <1800000>;
> > +				regulator-max-microvolt = <3300000>;
> > +			};
> > +		};
> > +	};
> > +
> > +	eeprom at 51 {
> > +		compatible = "atmel,24c32";
> > +		reg = <0x51>;
> > +		pagesize = <32>;
> > +		status = "okay";
> > +	};
> > +
> > +	rv3028: rtc at 52 {
> > +		compatible = "microcrystal,rv3028";
> > +		reg = <0x52>;
> > +		trickle-resistor-ohms = <1000>;
> > +		enable-level-switching-mode;
> > +		clocks = <&rtcclkout>;
> > +		status = "okay";
> > +	};
> > +};
> > +
> > +/* eMMC */
> > +&usdhc3 {
> > +	pinctrl-names = "default", "state_100mhz", "state_200mhz";
> > +	pinctrl-0 = <&pinctrl_usdhc3>;
> > +	pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
> > +	pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
> > +	bus-width = <8>;
> > +	non-removable;
> > +	status = "okay";
> > +};
> > +
> > +&wdog1 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_wdog>;
> > +	fsl,ext-reset-output;
> > +	status = "okay";
> > +};
> > +
> > +&iomuxc {
> > +	pinctrl_fec: fecgrp {
> > +		fsl,pins = <
> > +			MX8MP_IOMUXC_SAI1_RXD2__ENET1_MDC		0x3
> > +			MX8MP_IOMUXC_SAI1_RXD3__ENET1_MDIO		0x3
> > +			MX8MP_IOMUXC_SAI1_RXD4__ENET1_RGMII_RD0		
> > 0x91
> > +			MX8MP_IOMUXC_SAI1_RXD5__ENET1_RGMII_RD1		
> > 0x91
> > +			MX8MP_IOMUXC_SAI1_RXD6__ENET1_RGMII_RD2		
> > 0x91
> > +			MX8MP_IOMUXC_SAI1_RXD7__ENET1_RGMII_RD3		
> > 0x91
> > +			MX8MP_IOMUXC_SAI1_TXC__ENET1_RGMII_RXC		
> > 0x91
> > +			MX8MP_IOMUXC_SAI1_TXFS__ENET1_RGMII_RX_CTL	0x9
> > 1
> > +			MX8MP_IOMUXC_SAI1_TXD0__ENET1_RGMII_TD0		
> > 0x1f
> > +			MX8MP_IOMUXC_SAI1_TXD1__ENET1_RGMII_TD1		
> > 0x1f
> > +			MX8MP_IOMUXC_SAI1_TXD2__ENET1_RGMII_TD2		
> > 0x1f
> > +			MX8MP_IOMUXC_SAI1_TXD3__ENET1_RGMII_TD3		
> > 0x1f
> > +			MX8MP_IOMUXC_SAI1_TXD4__ENET1_RGMII_TX_CTL	0x1
> > f
> > +			MX8MP_IOMUXC_SAI1_TXD5__ENET1_RGMII_TXC		
> > 0x1f
> > +			MX8MP_IOMUXC_GPIO1_IO15__GPIO1_IO15		0x1
> > 1
> > +		>;
> > +	};
> > +
> > +	pinctrl_i2c1: i2c1grp {
> > +		fsl,pins = <
> > +			MX8MP_IOMUXC_I2C1_SCL__I2C1_SCL		0x4
> > 00001c3
> > +			MX8MP_IOMUXC_I2C1_SDA__I2C1_SDA		0x4
> > 00001c3
> > +		>;
> > +	};
> > +
> > +	pinctrl_i2c1_gpio: i2c1gpiogrp {
> > +		fsl,pins = <
> > +			MX8MP_IOMUXC_I2C1_SCL__GPIO5_IO14	0x1e3
> > +			MX8MP_IOMUXC_I2C1_SDA__GPIO5_IO15	0x1e3
> > +		>;
> > +	};
> > +
> > +
> 
> Too many line breaks.

Oh, I will remove this.

Thank you for the review.

Teresa
> 
> Best regards,
> Krzysztof