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

Tue Dec 8 07:23:54 EST 2020

Hello Krzysztof,

Am Dienstag, den 08.12.2020, 13:00 +0100 schrieb Krzysztof Kozlowski:
> On Tue, Dec 08, 2020 at 12:53:22PM +0100, Teresa Remmet wrote:
> > 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 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.
> > 
> > Ok, then i will change it now like you suggested and rework it
> > later
> > after more features are available.
> 
> If you submit two DTSes using the phyboard DTSI, it will be enough to
> justify that split.

Yes, but I don't have any features yet where the DTS files would differ
. So this would not make sense now.

> 
> [...]
> 
> > > > > > +	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.
> 
> Mhmm... I assume you also measured it without this clock-dance in DTS
> and the clock was on in such case?

Yes, the clock was on. 

> 
> It is pretty confusing... The RV3028 registers a clock provider and
> its
> clock will be disabled by the core because it is not used. Adding a
> clock consumer to RV3028 should not change here anything because
> RV3028
> does not use this clock. Adding a fixed clock without reference to HW
> also should not change here anything.

True. I will need to investigate here more why it is not being disabled
as it is not used.

Teresa

> 
> Anyway, your RV3028 node with a clock phandle would not pass dtschema
> check so it's a hint you are doing something not correct for Linux
> kernel.
> 
> Best regards,
> Krzysztof
> 