[PATCH v4 3/3] arm64: dts: Add device tree for the Debix Model A Board

[Ahmad Fatoum]

Hello Daniel,

On 17.10.22 17:10, Daniel Scally wrote:
> Add a device tree file describing the Debix Model A board from
> Polyhex Technology Co.

Thanks for your patch. Some minor comments below.

> Changes in v3 (Laurent):
> 
>     - Added IOB copyright notice
>     - Removed the eth node for the connector that's on the separate I/O
>     board

I'd have left the FEC node in and described the PHY, but left the FEC disabled.
Only the magnetics are on the expansion board, while the PHY is on the
base board.

> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2019 NXP
> + * Copyright 2022 Ideas on Board Oy
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/usb/pd.h>
> +
> +#include "imx8mp.dtsi"
> +
> +/ {
> +	model = "Polyhex Debix Model A i.MX8MPlus board";
> +	compatible = "polyhex,imx8mp-debix-model-a", "fsl,imx8mp";

I see that Model A and Model B share the same SoC and PCB. Could you
add polyhex,imx8mp-debix as a second compatible? That way, bootloader
may match against that compatible when they support both.
You'll need to adjust the binding accordingly.

> +
> +	chosen {
> +		stdout-path = &uart2;
> +	};
> +
> +	gpio-leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpio_led>;
> +
> +		status-led {
> +			function = LED_FUNCTION_POWER;
> +			color = <LED_COLOR_ID_RED>;
> +			gpios = <&gpio3 16 GPIO_ACTIVE_HIGH>;
> +			default-state = "on";
> +		};
> +	};
> +
> +	reg_usdhc2_vmmc: regulator-usdhc2 {
> +		compatible = "regulator-fixed";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_reg_usdhc2_vmmc>;
> +		regulator-name = "VSD_3V3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +	};
> +};
> +
> +&A53_0 {
> +	cpu-supply = <&buck2>;
> +};
> +
> +&A53_1 {
> +	cpu-supply = <&buck2>;
> +};
> +
> +&A53_2 {
> +	cpu-supply = <&buck2>;
> +};
> +
> +&A53_3 {
> +	cpu-supply = <&buck2>;
> +};
> +
> +&eqos {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_eqos>;
> +	phy-connection-type = "rgmii-id";
> +	phy-handle = <&ethphy0>;
> +	status = "okay";
> +
> +	mdio {
> +		compatible = "snps,dwmac-mdio";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		ethphy0: ethernet-phy at 0 {

Could you append a /* RTL8211E */ comment here? This can be very useful for others
who need to bring up the same chip in the future.

> +			compatible = "ethernet-phy-ieee802.3-c22";
> +			reg = <0>;

Is the PHY really at address 0 or does it just answer at this address
because it's the broadcast address?


> +&iomuxc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_hog>;
> +
> +	pinctrl_hog: hoggrp {
> +		fsl,pins = <
> +			MX8MP_IOMUXC_HDMI_DDC_SCL__HDMIMIX_HDMI_SCL			0x400001c3
> +			MX8MP_IOMUXC_HDMI_DDC_SDA__HDMIMIX_HDMI_SDA			0x400001c3
> +			MX8MP_IOMUXC_HDMI_HPD__HDMIMIX_HDMI_HPD				0x40000019
> +			MX8MP_IOMUXC_HDMI_CEC__HDMIMIX_HDMI_CEC				0x40000019

Why do you hog these?

> +	pinctrl_usb1_vbus: usb1grp {

This is unused.

> +	pinctrl_usdhc2: usdhc2grp {
> +		fsl,pins = <
> +			MX8MP_IOMUXC_SD2_CLK__USDHC2_CLK				0x190
> +			MX8MP_IOMUXC_SD2_CMD__USDHC2_CMD				0x1d0
> +			MX8MP_IOMUXC_SD2_DATA0__USDHC2_DATA0				0x1d0
> +			MX8MP_IOMUXC_SD2_DATA1__USDHC2_DATA1				0x1d0
> +			MX8MP_IOMUXC_SD2_DATA2__USDHC2_DATA2				0x1d0
> +			MX8MP_IOMUXC_SD2_DATA3__USDHC2_DATA3				0x1d0
> +			MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT				0xc1

Just to make sure this doesn't fry SD-Cards by mistake: VSELECT is indeed
connected to a 1.8V/3.3V switch powering vqmmc?

> +/* SD Card */
> +&usdhc2 {
> +	assigned-clocks = <&clk IMX8MP_CLK_USDHC2>;
> +	assigned-clock-rates = <400000000>;

I wonder why this is necessary. Do you see a difference
in /sys/kernel/debug/mmcX/ios between having this and leaving
it out?

> +	status = "okay";
> +};
> +
> +/* eMMc */

eMMC

> +&usdhc3 {
> +	assigned-clocks = <&clk IMX8MP_CLK_USDHC3>;
> +	assigned-clock-rates = <400000000>;
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz";
> +	pinctrl-0 = <&pinctrl_usdhc3>;
> +	pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
> +	pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
> +	bus-width = <8>;
> +	non-removable;
> +	status = "okay";
> +};
> +
> +&wdog1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_wdog>;
> +	fsl,ext-reset-output;
> +	status = "okay";
> +};


Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |