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.
>
> >
> > > +};
> > > 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 = <®_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,
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
