Re: [PATCH 2/5] dts: imx95-phyflex-libra-rdk: Add dts for phyFLEX-i.MX 95 Libra RDK

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] arm64: dts: freescale: duplicate usdhc2-200mhzgrp node overwrites usdhc2 pins
- [High] arm64: dts: freescale: missing OF graph ports on M.2 connectors disables radios
- [High] arm64: dts: freescale: descending cooling-levels ending in 0 breaks pwm-fan
- [Medium] arm64: dts: freescale: misplaced external-facing and wake-gpios on pcie0/pcie1
- [Medium] arm64: dts: freescale: stale gpio2 gpio-line-names collide with gpio_expander2
--

commit 01cb871eca78a3c506a85dce8f3e949939c4a0cb
Author: Benjamin Hahn <B.Hahn@phytec.de>

dts: imx95-phyflex-libra-rdk: Add dts for phyFLEX-i.MX 95 Libra RDK

This commit adds the device tree source files for the PHYTEC phyFLEX Libra
RDK baseboard (revision 1618.2) and phyFLEX-i.MX 95 FPSC Gamma SoM (revision
1620.3).

> diff --git a/arch/arm64/boot/dts/freescale/imx95-phyflex-fpsc-g-som.dtsi b/arch/arm64/boot/dts/freescale/imx95-phyflex-fpsc-g-som.dtsi
> new file mode 100644
> index 0000000000000..4b37d76cced1b
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx95-phyflex-fpsc-g-som.dtsi
> @@ -0,0 +1,736 @@

[ ... ]

> +&scmi_iomuxc {

[ ... ]

> +	pinctrl_usdhc2_200mhz: usdhc2-200mhzgrp {
> +		fsl,pins = <
> +			IMX95_PAD_SD2_CD_B__USDHC2_CD_B		0x31e	/* CD */
> +			IMX95_PAD_SD2_CLK__USDHC2_CLK		0x158e	/* CLK */
> +			IMX95_PAD_SD2_CMD__USDHC2_CMD		0x138e	/* CMD */
> +			IMX95_PAD_SD2_DATA0__USDHC2_DATA0	0x138e	/* DATA0 */
> +			IMX95_PAD_SD2_DATA1__USDHC2_DATA1	0x138e	/* DATA1 */
> +			IMX95_PAD_SD2_DATA2__USDHC2_DATA2	0x138e	/* DATA2 */
> +			IMX95_PAD_SD2_DATA3__USDHC2_DATA3	0x138e	/* DATA3 */
> +			IMX95_PAD_SD2_VSELECT__USDHC2_VSELECT	0x51e
> +		>;
> +		bootph-all;
> +	};
> +
> +	pinctrl_usdhc3: usdhc3grp {
> +		fsl,pins = <
> +			IMX95_PAD_SD3_CLK__USDHC3_CLK		0x159e	/* SDIO_CLK */
> +			IMX95_PAD_SD3_CMD__USDHC3_CMD		0x139e	/* SDIO_CMD */
> +			IMX95_PAD_SD3_DATA0__USDHC3_DATA0	0x139e	/* SDIO_DATA0 */
> +			IMX95_PAD_SD3_DATA1__USDHC3_DATA1	0x139e	/* SDIO_DATA1 */
> +			IMX95_PAD_SD3_DATA2__USDHC3_DATA2	0x139e	/* SDIO_DATA2 */
> +			IMX95_PAD_SD3_DATA3__USDHC3_DATA3	0x139e	/* SDIO_DATA3 */
> +		>;
> +	};
> +
> +	pinctrl_usdhc3_100mhz: usdhc3-100mhzgrp {
> +		fsl,pins = <
> +			IMX95_PAD_SD3_CLK__USDHC3_CLK		0x159e	/* SDIO_CLK */
> +			IMX95_PAD_SD3_CMD__USDHC3_CMD		0x139e	/* SDIO_CMD */
> +			IMX95_PAD_SD3_DATA0__USDHC3_DATA0	0x139e	/* SDIO_DATA0 */
> +			IMX95_PAD_SD3_DATA1__USDHC3_DATA1	0x139e	/* SDIO_DATA1 */
> +			IMX95_PAD_SD3_DATA2__USDHC3_DATA2	0x139e	/* SDIO_DATA2 */
> +			IMX95_PAD_SD3_DATA3__USDHC3_DATA3	0x139e	/* SDIO_DATA3 */
> +		>;
> +	};
> +
> +	pinctrl_usdhc3_200mhz: usdhc2-200mhzgrp {
> +		fsl,pins = <
> +			IMX95_PAD_SD3_CLK__USDHC3_CLK		0x159e	/* SDIO_CLK */
> +			IMX95_PAD_SD3_CMD__USDHC3_CMD		0x139e	/* SDIO_CMD */
> +			IMX95_PAD_SD3_DATA0__USDHC3_DATA0	0x139e	/* SDIO_DATA0 */
> +			IMX95_PAD_SD3_DATA1__USDHC3_DATA1	0x139e	/* SDIO_DATA1 */
> +			IMX95_PAD_SD3_DATA2__USDHC3_DATA2	0x139e	/* SDIO_DATA2 */
> +			IMX95_PAD_SD3_DATA3__USDHC3_DATA3	0x139e	/* SDIO_DATA3 */
> +		>;
> +	};

[Severity: High]
Should pinctrl_usdhc3_200mhz be named usdhc3-200mhzgrp instead of
usdhc2-200mhzgrp?

Because both pinctrl_usdhc2_200mhz and pinctrl_usdhc3_200mhz use the node
name usdhc2-200mhzgrp under &scmi_iomuxc, dtc merges them into a single
node and overwrites the SD2 fsl,pins array in pinctrl_usdhc2_200mhz with
the SD3 pins from pinctrl_usdhc3_200mhz.

When usdhc2 (which has sd-uhs-sdr104 enabled) switches to its state_200mhz
pinctrl state, it will attempt to claim the SD3 pads instead of configuring
the SD2 pads, breaking 200MHz SD card operation and conflicting with usdhc3.

Also, both pinctrl_usdhc2_200mhz and pinctrl_usdhc3_200mhz appear to use
100MHz pad settings (0x158e/0x138e and 0x159e/0x139e) rather than 200MHz
slew rate settings (such as 0x15fe/0x13fe used in pinctrl_usdhc1_200mhz).

> diff --git a/arch/arm64/boot/dts/freescale/imx95-phyflex-libra-rdk.dts b/arch/arm64/boot/dts/freescale/imx95-phyflex-libra-rdk.dts
> new file mode 100644
> index 0000000000000..55d49f8109567
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx95-phyflex-libra-rdk.dts
> @@ -0,0 +1,558 @@

[ ... ]

> +	m2-key-m-connector {
> +		compatible = "pcie-m2-m-connector";
> +		clocks = <&rv3028>;
> +		i2c-parent = <&lpi2c5>;
> +		vpcie3v3-supply = <&reg_vdd_3v3>;
> +	};
> +
> +	m2-key-e-connector {
> +		compatible = "pcie-m2-e-connector";
> +		clocks = <&rv3028>;
> +		i2c-parent = <&lpi2c3>;
> +		sdio-reset-gpios = <&gpio_expander2 6 GPIO_ACTIVE_LOW>;
> +		sdio-wake-gpios = <&gpio_expander2 5 GPIO_ACTIVE_LOW>;
> +		uart-wake-gpios = <&gpio_expander1 13 GPIO_ACTIVE_LOW>;
> +		vpcie3v3-supply = <&reg_vdd_3v3>;
> +		w-disable1-gpios = <&gpio_expander1 9 GPIO_ACTIVE_LOW>;
> +		w-disable2-gpios = <&gpio_expander1 10 GPIO_ACTIVE_LOW>;
> +	};

[Severity: High]
Are the OF graph ports missing from m2-key-m-connector and
m2-key-e-connector?

When pwrseq_pcie_m2_probe() in drivers/power/sequencing/pwrseq-pcie-m2.c
probes m2-key-e-connector, it requests w-disable1-gpios and w-disable2-gpios
with GPIOD_OUT_HIGH, asserting the active-low W_DISABLE1# and W_DISABLE2#
lines and disabling the M.2 module's Wi-Fi and Bluetooth radios:

drivers/power/sequencing/pwrseq-pcie-m2.c:pwrseq_pcie_m2_probe() {
	...
	ctx->w_disable1_gpio = devm_gpiod_get_optional(dev, "w-disable1",
						       GPIOD_OUT_HIGH);
	...
	ctx->w_disable2_gpio = devm_gpiod_get_optional(dev, "w-disable2",
						       GPIOD_OUT_HIGH);
}

Those lines are only deasserted when a consumer matches via
pwrseq_pcie_m2_match(), which walks the connector's OF graph endpoints via
for_each_endpoint_of_node(ctx->of_node, endpoint). Without the ports nodes
linking m2-key-e-connector and m2-key-m-connector to the PCIe Root Ports,
UART, and SDIO controller (which are also required by
pcie-m2-e-connector.yaml and pcie-m2-m-connector.yaml), no consumer can
match and the radios remain permanently disabled.

> +	osc_mclk: oscillator-mclk {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <19200000>;
> +		status = "disabled";
> +	};
> +
> +	osc_mclk_gated: oscillator-mclk-gated {
> +		compatible = "gpio-gate-clock";
> +		#clock-cells = <0>;
> +		clocks = <&osc_mclk>;
> +		enable-gpios = <&gpio_expander1 8 GPIO_ACTIVE_HIGH>;
> +		status = "disabled";
> +	};
> +
> +	fan: pwm-fan {
> +		compatible = "pwm-fan";
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +		#cooling-cells = <2>;
> +		/* startup 35%, duty cycle 30%-100%;
> +		 * polarity inversion does not appear to work
> +		 */
> +		cooling-levels = <255 166 96 0>;
> +		/* fan-supply = <&reg_vdd_5v0>;  probe fails with this set */
> +		pwms = <&tpm5 3 86958 0>;
> +		pulses-per-revolution = <2>;
> +	};

[Severity: High]
Will cooling-levels = <255 166 96 0> turn the fan off when the thermal zone
reaches its highest temperature trip point (trip4 at 75C)?

In drivers/hwmon/pwm-fan.c, __set_pwm() treats a PWM value of 0 as fan
power-off rather than driving a 0% duty cycle:

drivers/hwmon/pwm-fan.c:__set_pwm() {
	...
	if (pwm > 0) {
		...
		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
		ret = pwm_apply_might_sleep(ctx->pwm, state);
		if (ret)
			return ret;
		ret = pwm_fan_power_on(ctx);
	} else {
		ret = pwm_fan_power_off(ctx, false);
	}
	...
}

When cooling state 3 selects cooling-levels[3] == 0, pwm_fan_power_off() sets
state->enabled = false. In drivers/pwm/pwm-imx-tpm.c, pwm_imx_tpm_apply_hw()
disables the TPM5_CH3 output driver when state->enabled is false, allowing the
pad pull-up on IMX95_PAD_GPIO_IO26__TPM5_CH3 (0x51e) to pull the inverted PWM
line high and stop the fan at 75C.

Additionally, pwm_fan_probe() sets the initial state to
cooling_levels[pwm_fan_max_state] (0), so the PWM channel is not enabled at
boot, and pwm_fan_update_state() expects cooling-levels to be ordered in
ascending order. Using a non-zero minimum value (such as 1) or configuring
active-high/inverted PWM handling would avoid disabling the channel at state 3.

[ ... ]

> +&gpio2 {
> +	gpio-line-names = "", "", "", "", "",
> +			  "", "", "", "", "",
> +			  "", "", "", "", "",
> +			  "", "RGMII2_nINT", "GPIO4", "RTC_INT", "",
> +			  "LVDS1_BL_EN";
> +};
> +
> +&lpi2c1 {
> +	temperature-sensor@4f {
> +		compatible = "nxp,p3t1755";
> +		reg = <0x4f>;
> +		vs-supply = <&reg_vdd_1v8>;
> +	};
> +};
> +
> +&lpi2c3 {
> +	status = "okay";
> +
> +	gpio_expander2: gpio@21 {
> +		compatible = "ti,tcal6416";
> +		reg = <0x21>;
> +		interrupt-parent = <&gpio2>;
> +		interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
> +		#interrupt-cells = <2>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		gpio-line-names = "CAN1_STBY", "CAN2_STBY", "NC",
> +				  "KEY-E_nALERT", "KEY-M_nALERT",
> +				  "SDIO_nWAKE", "SDIO_nRESET", "HDMI_EN",
> +				  "RTC_nINT", "LVDS1_BL_EN", "SFP_TX_FAULT",
> +				  "SFP_TX_DISABLE", "SFP_MOD_ABS", "SFP_RS0",
> +				  "SFP_RX_LOSS", "SFP_RS1";

[Severity: Medium]
Should the &gpio2 gpio-line-names override in imx95-phyflex-libra-rdk.dts be
dropped?

On the SoM (imx95-phyflex-fpsc-g-som.dtsi), GPIO2 lines 18 and 20 are muxed to
SPI3_CS and SPI3_MOSI for lpspi4, while on the baseboard RTC_nINT and
LVDS1_BL_EN are on gpio_expander2 lines 8 and 9. Keeping "RTC_INT" and
"LVDS1_BL_EN" on &gpio2 duplicates the "LVDS1_BL_EN" line name and causes
userspace lookups by name to match gpio2 line 20 instead of gpio_expander2
line 9.

[ ... ]

> +/* M.2 Key-E PCIe */
> +&pcie0 {
> +	external-facing;
> +	supports-clkreq;
> +	wake-gpios = <&gpio_expander1 11 GPIO_ACTIVE_LOW>;
> +	status = "okay";
> +};
> +
> +&pcie0_port0 {
> +	reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
> +	vpcie3v3-supply = <&reg_vdd_3v3>;
> +};
> +
> +/* M.2 Key-M PCIe */
> +&pcie1 {
> +	external-facing;
> +	supports-clkreq;
> +	wake-gpios = <&gpio_expander1 12 GPIO_ACTIVE_LOW>;
> +	status = "okay";
> +};
> +
> +&pcie1_port0 {
> +	reset-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>;
> +	vpcie3v3-supply = <&reg_vdd_3v3>;
> +};

[Severity: Medium]
Should external-facing and wake-gpios be placed on &pcie0_port0 and
&pcie1_port0 rather than on &pcie0 and &pcie1?

In drivers/pci/of.c, pci_set_bus_of_node() only checks external-facing when
bus->self is non-NULL (reading from the Root Port node rather than the host
bridge node), and pci_configure_of_wake_gpio() requests wake-gpios from the
enumerated pci_dev's OF node (&pcie0_port0 and &pcie1_port0). The
fsl,imx6q-pcie.yaml binding also only allows pci-pci-bridge.yaml properties
like external-facing and wake-gpios on the Root Port child node.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-wip-bhahn-imx95_upstreaming-v1-0-1c1cebb5be99@phytec.de?part=2