Re: [PATCH 2/2] arm64: dts: freescale: Add imx8mp-venice-gw7905-2x
From: Tim Harvey
Date: Fri May 12 2023 - 11:08:18 EST
On Thu, May 11, 2023 at 10:23 AM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 11/05/2023 19:10, Tim Harvey wrote:
> > The Gateworks imx8mp-venice-gw7905-2x consists of a SOM + baseboard.
> >
> > The GW702x SOM contains the following:
> > - i.MX8M Plus SoC
> > - LPDDR4 memory
> > - eMMC Boot device
> > - Gateworks System Controller (GSC) with integrated EEPROM, button
> > controller, and ADC's
> > - PMIC
> > - RGMII PHY (eQoS)
> > - SOM connector providing:
> > - eQoS GbE MII
> > - 1x SPI
> > - 2x I2C
> > - 4x UART
> > - 2x USB 3.0
> > - 1x PCI
> > - 1x SDIO (4-bit 3.3V)
> > - 1x SDIO (4-bit 3.3V/1.8V)
> > - GPIO
> >
> > The GW7905 Baseboard contains the following:
> > - GPS
> > - microSD
> > - off-board I/O connector with I2C, SPI, GPIO
> > - EERPOM
> > - PCIe clock generator
> > - 1x full-length miniPCIe socket with PCI/USB3 (via mux) and USB2.0
> > - 1x half-length miniPCIe socket with USB2.0 and USB3.0
> > - USB 3.0 HUB
> > - USB Type-C with USB PD Sink capability and peripheral support
> > - USB Type-C with USB 3.0 host support
> >
> > Signed-off-by: Tim Harvey <tharvey@xxxxxxxxxxxxx>
> > ---
> > .../dts/freescale/imx8mp-venice-gw702x.dtsi | 589 ++++++++++++++++++
> > .../dts/freescale/imx8mp-venice-gw7905-2x.dts | 28 +
> > .../dts/freescale/imx8mp-venice-gw7905.dtsi | 358 +++++++++++
>
Hi Krzysztof,
Thanks for the review!
>
> How do you compile it? Missing Makefile. This also suggests that maybe
> you did not test it with dtbs_check...
>
I am in the habbit of using 'make dtbs W=1' and 'make dtbs_check' but
I accidently put the Makefile change in a future commit. With this new
board we add a new SOM compatible with 3 other baseboards and a new
baseboard compatible with one other SOM so there will be 4 more boards
added shortly: imx8mm-venice-gw7905-0x, imx8mp-venice-gw71xx-2x,
imx8mp-venice-gw72xx-2x, imx8mp-venice-gw73xx-2x. I assume its still
best to submit each of those as a 2-part series (add the binding, then
add the dt) instead of bulking multiple boards into one submission
correct?
>
> > 3 files changed, 975 insertions(+)
> > create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-venice-gw702x.dtsi
> > create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-venice-gw7905-2x.dts
> > create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-venice-gw7905.dtsi
>
> ...
>
> > + gsc: gsc@20 {
> > + compatible = "gw,gsc";
> > + reg = <0x20>;
> > + pinctrl-0 = <&pinctrl_gsc>;
> > + interrupt-parent = <&gpio2>;
> > + interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
> > + interrupt-controller;
> > + #interrupt-cells = <1>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + adc {
> > + compatible = "gw,gsc-adc";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + channel@6 {
> > + gw,mode = <0>;
> > + reg = <0x06>;
> > + label = "temp";
> > + };
> > +
> > + channel@8 {
> > + gw,mode = <1>;
> > + reg = <0x08>;
> > + label = "vdd_bat";
> > + };
> > +
> > + channel@16 {
> > + gw,mode = <4>;
> > + reg = <0x16>;
> > + label = "fan_tach";
> > + };
> > +
> > + channel@82 {
> > + gw,mode = <2>;
> > + reg = <0x82>;
> > + label = "vdd_vin";
> > + gw,voltage-divider-ohms = <22100 1000>;
> > + };
> > +
> > + channel@84 {
> > + gw,mode = <2>;
> > + reg = <0x84>;
> > + label = "vdd_adc1";
> > + gw,voltage-divider-ohms = <10000 10000>;
> > + };
> > +
> > + channel@86 {
> > + gw,mode = <2>;
> > + reg = <0x86>;
> > + label = "vdd_adc2";
> > + gw,voltage-divider-ohms = <10000 10000>;
> > + };
> > +
> > + channel@88 {
> > + gw,mode = <2>;
> > + reg = <0x88>;
> > + label = "vdd_1p0";
> > + };
> > +
> > + channel@8c {
> > + gw,mode = <2>;
> > + reg = <0x8c>;
> > + label = "vdd_1p8";
> > + };
> > +
> > + channel@8e {
> > + gw,mode = <2>;
> > + reg = <0x8e>;
> > + label = "vdd_2p5";
> > + };
> > +
> > + channel@90 {
> > + gw,mode = <2>;
> > + reg = <0x90>;
> > + label = "vdd_3p3";
> > + gw,voltage-divider-ohms = <10000 10000>;
> > + };
> > +
> > + channel@92 {
> > + gw,mode = <2>;
> > + reg = <0x92>;
> > + label = "vdd_dram";
> > + };
> > +
> > + channel@98 {
> > + gw,mode = <2>;
> > + reg = <0x98>;
> > + label = "vdd_soc";
> > + };
> > +
> > + channel@9a {
> > + gw,mode = <2>;
> > + reg = <0x9a>;
> > + label = "vdd_arm";
> > + };
> > +
> > + channel@a2 {
> > + gw,mode = <2>;
> > + reg = <0xa2>;
> > + label = "vdd_gsc";
> > + gw,voltage-divider-ohms = <10000 10000>;
> > + };
> > + };
> > +
> > + fan-controller@0 {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
>
> Why do you need these two? I know binding expects them, but why? Anyway
> compatible is first, reg is second property.
I never needed them for GSC functionality but ended up having to add
them to the gateworks-gsc.yaml to make binding checks happy.
When I was working on gateworks-gsc.yaml I was getting the following
error until I added #address-cells=1 and #size-cells=0:
> $ make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> CHKDT Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> SCHEMA Documentation/devicetree/bindings/processed-schema.yaml
> DTC Documentation/devicetree/bindings/mfd/gateworks-gsc.example.dt.yaml
> Documentation/devicetree/bindings/mfd/gateworks-gsc.example.dts:58.21-34: Warning (reg_format): /example-0/i2c/gsc@20/fan-controller@2c:reg: property has invalid length (4 bytes) (#address-cells == 2, #size-cells == 1)
I didn't completely understand the issue and dt_binding_check no
longer complains with the above if I remove the requirement so it
seems I should submit the following patch along with removing the
properties from all the dt's that have the fan-controller node:
diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
index acb9c54942d9..dc379f3ebf24 100644
--- a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
+++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
@@ -122,12 +122,6 @@ patternProperties:
compatible:
const: gw,gsc-fan
- "#address-cells":
- const: 1
-
- "#size-cells":
- const: 0
-
reg:
description: The fan controller base address
maxItems: 1
@@ -135,8 +129,6 @@ patternProperties:
required:
- compatible
- reg
- - "#address-cells"
- - "#size-cells"
required:
- compatible
@@ -194,8 +186,6 @@ examples:
};
fan-controller@2c {
- #address-cells = <1>;
- #size-cells = <0>;
compatible = "gw,gsc-fan";
reg = <0x2c>;
};
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-venice-gw702x.dtsi b/arch/arm6
4/boot/dts/freescale/imx8mp-venice-gw702x.dtsi
index 4fca4aae8f72..74b0fda235ed 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-venice-gw702x.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp-venice-gw702x.dtsi
@@ -222,8 +222,6 @@ channel@a2 {
};
fan-controller@0 {
- #address-cells = <1>;
- #size-cells = <0>;
compatible = "gw,gsc-fan";
reg = <0x0a>;
};
Would that make sense?
Is it that the fan-controller because it does not have addressable
child nodes does not need address-cells?
>
>
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-venice-gw7905-2x.dts b/arch/arm64/boot/dts/freescale/imx8mp-venice-gw7905-2x.dts
> > new file mode 100644
> > index 000000000000..4a1bbbbe19e6
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-venice-gw7905-2x.dts
> > @@ -0,0 +1,28 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright 2023 Gateworks Corporation
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "imx8mp.dtsi"
> > +#include "imx8mp-venice-gw702x.dtsi"
> > +#include "imx8mp-venice-gw7905.dtsi"
> > +
> > +/ {
> > + model = "Gateworks Venice GW7905-2x i.MX8MP Development Kit";
> > + compatible = "gateworks,imx8mp-gw7905-2x", "fsl,imx8mp";
> > +
> > + chosen {
> > + stdout-path = &uart2;
> > + };
> > +};
> > +
> > +/* Disable SOM interfaces not used on baseboard */
> > +&eqos {
> > + status = "disabled";
> > +};
> > +
> > +&usdhc1 {
> > + status = "disabled";
> > +};
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-venice-gw7905.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-venice-gw7905.dtsi
> > new file mode 100644
> > index 000000000000..479190a6391f
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-venice-gw7905.dtsi
> > @@ -0,0 +1,358 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright 2023 Gateworks Corporation
> > + */
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/leds/common.h>
> > +#include <dt-bindings/phy/phy-imx8-pcie.h>
> > +
> > +/ {
> > + aliases {
> > + ethernet0 = &eqos;
> > + };
> > +
> > + led-controller {
> > + compatible = "gpio-leds";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_gpio_leds>;
> > +
> > + led-0 {
> > + function = LED_FUNCTION_STATUS;
> > + color = <LED_COLOR_ID_GREEN>;
> > + gpios = <&gpio4 22 GPIO_ACTIVE_HIGH>;
> > + default-state = "on";
> > + linux,default-trigger = "heartbeat";
> > + };
> > +
> > + led-1 {
> > + function = LED_FUNCTION_STATUS;
> > + color = <LED_COLOR_ID_RED>;
> > + gpios = <&gpio4 27 GPIO_ACTIVE_HIGH>;
> > + default-state = "off";
> > + };
> > + };
> > +
> > + pcie0_refclk: pcie0-refclk {
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + clock-frequency = <100000000>;
> > + };
> > +
> > + pps {
> > + compatible = "pps-gpio";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_pps>;
> > + gpios = <&gpio4 21 GPIO_ACTIVE_HIGH>;
> > + status = "okay";
> > + };
> > +
> > + reg_usb2_vbus: regulator-usb2-vbus {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_reg_usb2_en>;
> > + compatible = "regulator-fixed";
> > + regulator-name = "usb2_vbus";
> > + gpio = <&gpio4 12 GPIO_ACTIVE_HIGH>;
> > + enable-active-high;
> > + regulator-min-microvolt = <5000000>;
> > + regulator-max-microvolt = <5000000>;
> > + };
> > +
> > + reg_usdhc2_vmmc: regulator-usdhc2 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_reg_usdhc2_vmmc>;
> > + compatible = "regulator-fixed";
> > + regulator-name = "SD2_3P3V";
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>;
> > + enable-active-high;
> > + };
> > +
>
> Drop stay blank line
will do in v2.
Best Regards,
Tim
>
> > +};
> > +
> > +/* off-board header */
> > +&ecspi2 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_spi2>;
> > + cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
> > + status = "okay";
> > +};
>
>
>
> Best regards,
> Krzysztof
>