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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Krzysztof,

Am Montag, den 07.12.2020, 14:46 +0100 schrieb Krzysztof Kozlowski:
> 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@xxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxx>
> > > > + */
> > > > +
> > > > +/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.

Ok, then i will change it now like you suggested and rework it later
after more features are available.

> 
> > 
> > > 
> > > > +};
> > > > 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@xxxxxxxxx>
> > > > + */
> > > > +
> > > > +#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@51 {
> > > > +		compatible = "atmel,24c02";
> > > > +		reg = <0x51>;
> > > > +		pagesize = <16>;
> > > > +		status = "okay";
> > > > +	};
> > > > +
> > > > +	leddimmer@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@xxxxxxxxx>
> > > > + */
> > > > +
> > > > +#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@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,

I measured it. I could see that the clock was being disabled. But yes
it does not feel like correct solution and needs more investigation.
I was not able to find the correct property modification.
Will remove this for now and find a proper solution afterwards. It does
not have impact on the functionality if it is enabled or not.
So I will remove the clock part in v2.

Regards,
Teresa

> 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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux