Re: [PATCH v4 2/2] arm64: dts: imx8mp-msc-sm2s: Add device trees for MSC SM2S-IMX8PLUS SoM and carrier board

[Martyn Welch]

On Fri, 2022-08-12 at 16:42 +0300, Krzysztof Kozlowski wrote:
> On 12/08/2022 14:35, Martyn Welch wrote:
> > On Fri, 2022-08-12 at 12:47 +0300, Krzysztof Kozlowski wrote:
> > > On 12/08/2022 11:41, Martyn Welch wrote:
> > > > Add device trees for one of a number of MSC's (parent company,
> > > > Avnet)
> > > > variants of the SM2S-IMX8PLUS system on module along with the
> > > > compatible
> > > > SM2S-SK-AL-EP1 carrier board. As the name suggests, this family
> > > > of
> > > > SoMs use
> > > > the NXP i.MX8MP SoC and provide the SMARC module interface.
> > > > 
> > > > Signed-off-by: Martyn Welch <martyn.welch@xxxxxxxxxxxxx>
> > > > ---
> > > > 
> > > > Changes in v2
> > > >   - Added compatibles
> > > >   - Removed underscores from node names
> > > >   - Make node names more generic
> > > >   - Reorder properties
> > > >   - Fix issues found by dtbs_check in these files
> > > > 
> > > > Changes in v3:
> > > >   - Switched to avnet vendor string in compatibles
> > > >   - Corrected patch description
> > > > 
> > > > Changes in v4:
> > > >   - Switched from phy-reset-gpios to reset-gpios, removing
> > > > duplication
> > > >   - Removed unneeded sdma1 node
> > > > 
> > > >  arch/arm64/boot/dts/freescale/Makefile        |   1 +
> > > >  .../freescale/imx8mp-msc-sm2s-14N0600E.dts    |  72 ++
> > > >  .../dts/freescale/imx8mp-msc-sm2s-ep1.dts     |  53 ++
> > > >  .../boot/dts/freescale/imx8mp-msc-sm2s.dtsi   | 812
> > > > ++++++++++++++++++
> > > >  4 files changed, 938 insertions(+)
> > > >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-msc-
> > > > sm2s-
> > > > 14N0600E.dts
> > > >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-msc-
> > > > sm2s-
> > > > ep1.dts
> > > >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-msc-
> > > > sm2s.dtsi
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/freescale/Makefile
> > > > b/arch/arm64/boot/dts/freescale/Makefile
> > > > index 8bf7f7ecebaa..139c8b95c9c9 100644
> > > > --- a/arch/arm64/boot/dts/freescale/Makefile
> > > > +++ b/arch/arm64/boot/dts/freescale/Makefile
> > > > @@ -83,6 +83,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mn-venice-
> > > > gw7902.dtb
> > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-dhcom-pdk2.dtb
> > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk.dtb
> > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-icore-mx8mp-edimm2.2.dtb
> > > > +dtb-$(CONFIG_ARCH_MXC) += imx8mp-msc-sm2s-ep1.dtb
> > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-phyboard-pollux-rdk.dtb
> > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-tqma8mpql-mba8mpxl.dtb
> > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-venice-gw74xx.dtb
> > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s-
> > > > 14N0600E.dts b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s-
> > > > 14N0600E.dts
> > > > new file mode 100644
> > > > index 000000000000..9e976e8baaee
> > > > --- /dev/null
> > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s-
> > > > 14N0600E.dts
> > > > @@ -0,0 +1,72 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (C) 2022 Avnet Embedded GmbH
> > > > + */
> > > > +/dts-v1/;
> > > > +
> > > > +#include "imx8mp-msc-sm2s.dtsi"
> > > > +
> > > > +/ {
> > > > +       model = "MSC SM2S-IMX8PLUS-QC6-14N0600E SoM";
> > > > +       compatible = "avnet,sm2s-imx8mp-14N0600E", "avnet,sm2s-
> > > > imx8mp",
> > > > +                    "fsl,imx8mp";
> > > 
> > > This does not match your bindings. Please test your DTS.
> > > 
> > 
> > Hi Krzysztof,
> > 
> > I'm not sure I follow. This is the DTS for the SoM. 
> 
> SoMs usually do not have DTSes because they cannot be run on their
> own.
> SoMs almost always require a baseboard/carrier. Therefore this should
> not be DTS, but that was not my comment.
> 

OK, so does making this as dtsi resolve the issue for you? I assume as
a dtsi I would also need to remove the model and compatible?

> > The only way I can
> > test the SoM at the moment is on combination with the "EP1" carrier
> > board. 
> 
> ... so you basically say it cannot be a DTS.
> 
> > That has been tested. The strings match those specified in the
> > bindings unless I'm being blind to something.
> 
> Test the DTS - make dtbs_check (there are several
> variations/arguments/helpers):
> Documentation/devicetree/bindings/writing-schema.rst
> 

Yep, ran that.

> > 
> > I guess I can build the DTB for just the SoM 
> 
> But you just did it, didn't you? This is a DTS.
> 

As you can see from the patch, I didn't add that file directly to the
Makefile, so dtbs_check didn't check it directly.

> > 
> > and boot with that or
> > thinking about it, rename this as a .dtsi, given that it's unlikely
> > that anyone is going to have a carrier barebones enough that it
> > could
> > be considered just the SoM?
> 
> Anyway, I wanted DT bindings tests for DTS. Not actual tests on
> hardware, because the compatibles do not matter in that aspect.
> 

The tests threw quite a few errors that seemed to be related to the
imx8mp.dtsi. The only ones that seemed to be related to the files I've
created seem to be the result of including optional pins in the pin
muxing, which need to be there AFAIK, but seem to be resulting in
warnings from the tool.

Martyn

> Best regards,
> Krzysztof