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