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

Krzysztof Kozlowski krzk at kernel.org
Mon Dec 7 08:46:52 EST 2020 

On Mon, Dec 07, 2020 at 02:35:33PM +0100, Teresa Remmet wrote:
> 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.

You make the upstream DTSes more complicated to make it easier for
downstream. No, this does not work this way. You can either upstream
other DTSes so such split will make sense, or this contribution should
make sense in the upstreamed state.

In the second case, by "matching upstreamed state" I mean that you
organize your DTSes in a way they make sense for upstream, for example
one DTSI for the SOM and one DTS for the board using it.

> 
> > 
> > > +};
> > > 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.

This does not make sense at all:
1. This is a node without any reference to hardware,
2. It is being disabled in DTS so it will not have any effect in kernel
   therefore will not have any impact on real hardware,
3. The RV3028 RTC takes it as phandle... but RV3028 RTC is a clock
   provider, not a consumer.

If you want to disable RV3028 RTC clock, you need to modify it's
properties, not add fake nodes.

Best regards,
Krzysztof

More information about the linux-arm-kernel mailing list