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

[Krzysztof Kozlowski <krzk@kernel.org>]

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@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@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.

[...]

> > > > > +	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?

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.

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