Hi Ahmad - thanks for the review
On 01/12/2022 17:10, Ahmad Fatoum wrote:
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.
Fair enough, though there's quite a lot else on the base board which
we've left off simply because we're not currently using it. I'm inclined
to treat this the same for now.
+// 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.
Sure, makes sense to me
+
+ 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 = <ðphy0>;
+ status = "okay";
+
+ mdio {
+ compatible = "snps,dwmac-mdio";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ethphy0: ethernet-phy@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.
Sure
+ 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?
I confess I'm unsure here, the number here comes from the downstream
.dts, which in this instance I've trusted...is there any way I can check?
+&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.
Ah both for other elements of the board which aren't included in this
set, as they don't work properly yet. Apologies; I'll remove those.
+ 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?
I don't actually...it's present in the imx8mp-evk.dts which this is
based on, but in addition to not seeing any difference there the SD card
still seems fine as far as I can tell (same read / write speed in
practice) - I'll take it out, thanks
+ status = "okay";
+};
+
+/* eMMc */
eMMC
Ack
+&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
