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

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Quoting Ahmad Fatoum (2022-12-01 17:10:20)
> 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.

Polyhex also make a SOM with the brand DEBIX.
- https://debix.io/hardware/debix-som-a-io-board.html

But perhaps it will be fine, as that will be
"polyhex,imx8mp-debix-som" (and perhaps they'll do an A/B variant too?)

--
Kieran


> 
> > +
> > +     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@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 |
>