Re: [PATCH v2 2/4] arm64: dts: freescale: Add support for the Variscite DART-MX8M-PLUS SoM

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Laurent,

On 25.10.23 18:50, Laurent Pinchart wrote:
> +	reg_eqos_phy: regulator-eqos-phy {
> +		compatible = "regulator-fixed";
> +		regulator-name = "eqos-phy";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&gpio2 20 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +		regulator-always-on;

Apparently, https://lore.kernel.org/all/20230721110345.3925719-1-m.felsch@pengutronix.de/
didn't make it upstream. Perhaps you mentioning that you could use this would help get
it unstuck? :)

> +&eqos {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_eqos>;
> +	phy-mode = "rgmii";
> +	phy-handle = <&ethphy0>;
> +	status = "okay";
> +
> +	mdio {
> +		compatible = "snps,dwmac-mdio";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		ethphy0: ethernet-phy@0 {
> +			compatible = "ethernet-phy-ieee802.3-c22";
> +			reg = <0>;
> +			eee-broken-1000t;
> +			reset-gpios = <&gpio2 11 GPIO_ACTIVE_LOW>;

Nitpick: Separate pinctrl entry for PHY GPIOs that's added to the PHY node?
Makes it easier to check that all used signals are indeed muxed.

> +	pmic@25 {
> +		compatible = "nxp,pca9450c";
> +		reg = <0x25>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_pmic>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> +
> +		regulators {
> +			BUCK1 {
> +				regulator-name = "BUCK1";
> +				regulator-min-microvolt = <600000>;
> +				regulator-max-microvolt = <2187500>;

Nitpick: These may be the limits of what the BUCK can output, but they
don't look like a safe operating range for the board. The Linux driver already
has ranges hardcoded to cover what's possible by the hardware, so if you specify
regulator range here, it should pertain to what the board and SoC are designed
to handle.

> +/* eMMC */
> +&usdhc3 {
> +	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;

no-sd
no-sdio

may give you a tiny bit of speedup during probe, if you know that there will
always be an eMMC here.

> +	pinctrl_i2c1: i2c1grp {
> +		fsl,pins = <
> +			MX8MP_IOMUXC_I2C1_SCL__I2C1_SCL					0x400001c2
> +			MX8MP_IOMUXC_I2C1_SDA__I2C1_SDA					0x400001c2
> +		>;
> +	};
> +
> +	pinctrl_i2c1_gpio: i2c1gpiogrp {
> +		fsl,pins = <
> +			MX8MP_IOMUXC_I2C1_SCL__GPIO5_IO14				0x1c2
> +			MX8MP_IOMUXC_I2C1_SDA__GPIO5_IO15				0x1c2

This surprises me. I'd expect that the SION bit needs to be set for GPIO bus recovery.

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 |