Re: [PATCH 2/2] arm64: dts: imx8mm: Add Gateworks IMX8MM Development Kits

From: Krzysztof Kozlowski
Date: Mon Dec 28 2020 - 14:20:24 EST

On Mon, Dec 28, 2020 at 11:02:40AM -0800, Tim Harvey wrote:
> > +
> > > +&i2c1 {
> > > + clock-frequency = <100000>;
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&pinctrl_i2c1>;
> > > + status = "okay";
> > > +
> > > + gsc: gsc@20 {
> >
> > Node name should describe class of a device so maybe
> > "system-controller"?
> >
>
> The node-name here must match 'gsc' due to the bindings. The name
> stands for 'Gateworks System Controller'.

Indeed, so it must stay like this.
> > +
> > > + fan-controller@0 {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> >
> > These do not seem needed.
>
> They are required per
> Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml but they
> should be inherited from the parent node right? So perhaps
> gateworks-gsc.yaml needs to remove them?

Maybe they shouldn't be in the bindings, either? These are only for
nodes which have childre. The fan-controller does not have them.

>
> >
> > > + compatible = "gw,gsc-fan";
> > > + reg = <0x0a>;
> > > + };
> > > + };
> > > +
> > > + gsc_gpio: pca9555@23 {
> >
> > Lookes like gpio-controller, so "gpio". Please run make dtbs_check to be
> > sure your board validate against dt-schema.
>
> Yes, I see these warnings now with 'make dtbs_check W=1'

dtbs_check without W=1.
dtbs with W=1.
These are different tools.

>
> Note when I do run 'make dtbs_check W=1' I see many warnings which
> seem invalid that I have to skip over:
> /usr/src/linux/arch/arm64/boot/dts/freescale/imx8mm-venice-gw71xx-0x.dt.yaml: pi
> nctrl@30330000: fec1grp:fsl,pins:0: [104, 720, 0, 0, 0, 3, 108, 724, 1216, 0, 1,
> 3, 112, 728, 0, 0, 0, 31, 116, 732, 0, 0, 0, 31, 120, 736, 0, 0, 0,
> 31, 124, 740, 0, 0, 0, 31, 156, 772, 0, 0, 0, 145, 152, 768, 0, 0, 0,
> 145, 148, 764, 0, 0, 0, 145, 144, 760, 0, 0, 0, 145, 132, 748, 0, 0,
> 0, 31, 140, 756, 0, 0, 0, 145, 136, 752, 0, 0, 0, 145, 128, 744, 0, 0,
> 0, 31, 244, 860, 0, 5, 0, 25] is too long
> From schema:
> /usr/src/linux/Documentation/devicetree/bindings/pinctrl/fsl,imx8mm-pinctrl.yaml
> /usr/src/linux/arch/arm64/boot/dts/freescale/imx8mm-venice-gw71xx-0x.dt.yaml:
> pinctrl@30330000: fec1grp:fsl,pins:0: Additional items are not allowed
> (108, 724, 1216, 0, 1, 3, 112, 728, 0, 0, 0, 31, 116, 732, 0, 0, 0,
> 31, 120, 736, 0, 0, 0, 31, 124, 740, 0, 0, 0, 31, 156, 772, 0, 0, 0,
> 145, 152, 768, 0, 0, 0, 145, 148, 764, 0, 0, 0, 145, 144, 760, 0, 0,
> 0, 145, 132, 748, 0, 0, 0, 31, 140, 756, 0, 0, 0, 145, 136, 752, 0, 0,
> 0, 145, 128, 744, 0, 0, 0, 31, 244, 860, 0, 5, 0, 25 were unexpected)
> ^^^ It seems maybe something is too restrictive with
> Documentation/devicetree/bindings/pinctrl/fsl,imx8mm-pinctrl.yaml that
> doesn't allow an array of pins?

This is expected because dt-schema does not accept pin groups in one
array (<..., ....>). Compare your warnings with some other recent imx8mm
designs. If you introduce new, these have to be fixed.

>
> I see lots of warnings about imx8mm gpmi as well but they don't have
> anything to do with my dtbs.
>
> >
> > > + compatible = "nxp,pca9555";
> > > + reg = <0x23>;
> > > + gpio-controller;
> > > + #gpio-cells = <2>;
> > > + interrupt-parent = <&gsc>;
> > > + interrupts = <4>;
> > > + };
> > > +
> <snip>
> > > +
> > > +&iomuxc {
> > > + pinctrl_fec1: fec1grp {
> > > + fsl,pins = <
> > > + MX8MM_IOMUXC_ENET_MDC_ENET1_MDC 0x3
> > > + MX8MM_IOMUXC_ENET_MDIO_ENET1_MDIO 0x3
> > > + MX8MM_IOMUXC_ENET_TD3_ENET1_RGMII_TD3 0x1f
> > > + MX8MM_IOMUXC_ENET_TD2_ENET1_RGMII_TD2 0x1f
> > > + MX8MM_IOMUXC_ENET_TD1_ENET1_RGMII_TD1 0x1f
> > > + MX8MM_IOMUXC_ENET_TD0_ENET1_RGMII_TD0 0x1f
> > > + MX8MM_IOMUXC_ENET_RD3_ENET1_RGMII_RD3 0x91
> > > + MX8MM_IOMUXC_ENET_RD2_ENET1_RGMII_RD2 0x91
> > > + MX8MM_IOMUXC_ENET_RD1_ENET1_RGMII_RD1 0x91
> > > + MX8MM_IOMUXC_ENET_RD0_ENET1_RGMII_RD0 0x91
> > > + MX8MM_IOMUXC_ENET_TXC_ENET1_RGMII_TXC 0x1f
> > > + MX8MM_IOMUXC_ENET_RXC_ENET1_RGMII_RXC 0x91
> > > + MX8MM_IOMUXC_ENET_RX_CTL_ENET1_RGMII_RX_CTL 0x91
> > > + MX8MM_IOMUXC_ENET_TX_CTL_ENET1_RGMII_TX_CTL 0x1f
> > > + MX8MM_IOMUXC_NAND_ALE_GPIO3_IO0 0x19
> > > + >;
> > > + };
> > > +
> > > + pinctrl_gsc: gscirq {
> >
> > grp suffix. I think dtbs_check should point this out.
>
> ok, I do see this warning with 'make dtbs_check W=1'. I will fix the
> node names for the iomux pin groups
>
> <snip>
> >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw71xx-0x.dts b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw71xx-0x.dts
> > > new file mode 100644
> > > index 00000000..3f88c4a
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw71xx-0x.dts
> > > @@ -0,0 +1,19 @@
> > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > +/*
> > > + * Copyright 2020 Gateworks Corporation
> > > + */
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include "imx8mm.dtsi"
> > > +#include "imx8mm-venice-gw700x.dtsi"
> > > +#include "imx8mm-venice-gw71xx.dtsi"
> > > +
> > > +/ {
> > > + model = "Gateworks Venice GW71xx-0x i.MX8MM Development Kit";
> > > + compatible = "gw,imx8mm-gw71xx-0x", "fsl,imx8mm";
> > > +
> > > + chosen {
> > > + stdout-path = &uart2;
> > > + };
> > > +};
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw71xx.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw71xx.dtsi
> > > new file mode 100644
> > > index 00000000..a4eeb0d
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw71xx.dtsi
> >
> > This file is included only once, so it should be directly inside imx8mm-venice-gw71xx-0x.dts. Do not create empty DTS just to include DTSI and nothing more. It's really over-complicating simple hierarchy.
>
> The reason for this is that the '0x' on the end is the SoM and in the
> future there will be a '1x', '2x' SoM so imagine:
> imx8mm-venice-gw71xx-2x.dts being the 'kit' that combines the
> imx8mm-venice-gw71xx.dtsi baseboard and imx8mm-venice-gw702x.dtsi SoM.
> Our numbering scheme has SoM's as gw70xx. I will add something to the
> commit log that explains this.

If you really intend to upstream these future designs, then it's fine.
Have in mind that if you do not upstream them, anyone can simplify later
the hierarchy by renaming imx8mm-venice-gw71xx.dtsi into
imx8mm-venice-gw71xx.dts.

>
> >
> > > @@ -0,0 +1,186 @@
> > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > +/*
> > > + * Copyright 2020 Gateworks Corporation
> > > + */
> > > +
> > > +/ {
> > > + aliases {
> > > + usb0 = &usbotg1;
> > > + usb1 = &usbotg2;
> > > + };
> > > +
> > > + leds {
> >
> > led-controller
>
> ok - will fix
>
> >
> > > + compatible = "gpio-leds";
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&pinctrl_gpio_leds>;
> > > +
> > > + user1 { /* GRN */
> >
> > led-0
>
> ok, will fix
>
> >
> > > + label = "user1";
> >
> > label is now deprecated. See the common bindings.
> >
>
> ok, will fix by removing deprecated label and adding color/function props
>
> > > + gpios = <&gpio5 5 GPIO_ACTIVE_HIGH>;
> > > + default-state = "on";
> > > + linux,default-trigger = "heartbeat";
> > > + };
> > > +
> <snip>
> > > +&i2c2 {
> > > + clock-frequency = <400000>;
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&pinctrl_i2c2>;
> > > + status = "okay";
> > > +
> > > + accel@19 {
> >
> > accelerometer
> >
>
> ok - will fix
>
> You also had asked why ecspi2, i2c3 were enabled (with no child nodes)
> and why uart1/uart3 were enabled on the carrier boards.
>
> The carrier boards have off-board connectors for ecspi2, i2c3 so I
> include them on the baseboard so that the Linux SPI/I2C userspace
> API's could be used.

Sure, this makes sense. Can you add a comment to such nodes like:
"UART on header"

Customers usually copy the reference designs (boards) so it is usually
useful to explain why something on carrier board was enabled, even
though nothing is attached.

Best regards,
Krzysztof