[PATCH v4 3/3] arm64: dts: Add device tree for the Debix Model A Board
Dan Scally
dan.scally at ideasonboard.com
Tue Dec 6 02:25:08 PST 2022
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 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.
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
>
More information about the linux-arm-kernel
mailing list