On Tue, Dec 06, 2022 at 10:25:08AM +0000, Dan Scally wrote: > 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. Overall I think it's a good strategy, if we haven't tested a peripheral we shouldn't include it yet. Missing pieces can be added later. Of course if there are pieces that are easy to test already, we can include them. > >> +// 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"; > >> +}; -- Regards, Laurent Pinchart