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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] arm64: dts: freescale: missing OF graph ports on M.2 connectors
  disable radios
- [High] arm64: dts: freescale: usb2 missing dr_mode, vbus-supply, and
  pinctrl_usb2
- [Medium] arm64: dts: freescale: eeprom@58 uses 4 KiB 24c32 compatible
  for 32B ID page
- [Medium] arm64: dts: imx95-phyflex-libra-rdk: descending
  cooling-levels breaks pwm-fan
--

commit 831b01c8ff6e8f1f1ec13e354da0a278c7f495db
Author: Benjamin Hahn <B.Hahn@phytec.de>

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

This commit adds device tree support for the PHYTEC phyFLEX-i.MX 95 FPSC
Gamma SoM and the phyFLEX Libra RDK baseboard.

> 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..7cc4fa0104c36
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx95-phyflex-fpsc-g-som.dtsi

[ ... ]

> +	/* User EEPROM ID page */
> +	eeprom@58 {
> +		compatible = "atmel,24c32";
> +		reg = <0x58>;
> +		pagesize = <32>;
> +		vcc-supply = <&reg_nvcc_aon>;
> +	};

[Severity: Medium]
Should eeprom@58 use compatible = "atmel,24c32d-wl" (or specify size = <32>)
instead of "atmel,24c32"?

"atmel,24c32" registers a 4096-byte NVMEM device for the 32-byte
identification page, so reads past 32 bytes wrap around and writes at
offsets >= 0x400 set address bit A10, which permanently locks the
identification page on M24C32-D devices.

[ ... ]

> 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..ccd05932e9557
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx95-phyflex-libra-rdk.dts

[ ... ]

> +	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 m2-key-m-connector and m2-key-e-connector missing OF graph ports linking
them to pcie1_port0 and pcie0_port0?

When pwrseq_pcie_m2_probe() binds to m2-key-e-connector, it requests
w-disable1-gpios and w-disable2-gpios with GPIOD_OUT_HIGH, holding both
active-low disable lines asserted.  Because neither the connector nodes nor
pcie0_port0 and pcie1_port0 define OF graph endpoints, slot_pwrctrl_probe()
and pwrseq_pcie_m2_match() never match the PCIe consumers to enable the
sequencers and deassert W_DISABLE1#.

[ ... ]

> +	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: Medium]
Does using descending cooling-levels = <255 166 96 0> with normal PWM
polarity break pwm-fan suspend/shutdown and cooling state tracking?

On suspend and shutdown, pwm_fan_power_off() disables TPM5_CH3, which lets
the 0x51e pad pull-down on IMX95_PAD_GPIO_IO26__TPM5_CH3 pull the inverted
fan control line low and spin the fan at 100%.  In addition,
pwm_fan_update_state() assumes cooling-levels is sorted in ascending order
(pwm < ctx->pwm_fan_cooling_levels[i + 1]), so writing pwm1 or pwm1_enable
via sysfs records the wrong cooling state and causes pwm_fan_set_cur_state()
to skip thermal transitions.

Using PWM_POLARITY_INVERTED with ascending levels and a non-zero off state
(such as cooling-levels = <1 96 166 255>) or a pull-up on
IMX95_PAD_GPIO_IO26__TPM5_CH3 avoids disabling the output into a pull-down.

[ ... ]

> +&usb2 {
> +	disable-over-current;
> +	status = "okay";
> +};

[ ... ]

> +&usbphynop {
> +	vbus-supply = <&reg_vdd_5v0>;
> +};

[Severity: High]
Is usb2 missing dr_mode = "host", pinctrl_usb2, and its VBUS supply?

Without dr_mode, ci_get_platdata() defaults to USB_DR_MODE_OTG, and because
no ID pin or role switch is wired to usb2, ci_hdrc_otg_fsm_init() leaves the
controller in peripheral B-idle mode instead of host mode.  Furthermore,
fsl,imx95-usb does not set CI_HDRC_PHY_VBUS_CONTROL, so ehci_ci_portpower()
only controls vbus-supply on &usb2, while placing vbus-supply = <&reg_vdd_5v0>
on &usbphynop causes usb_phy_gen_create_phy() to claim reg_vdd_5v0 via
devm_regulator_get_exclusive() and block other consumers such as pwm-fan.
Finally, pinctrl_usb2 in imx95-phyflex-fpsc-g-som.dtsi is never referenced.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-wip-bhahn-imx95_upstreaming-v2-0-3ef58af5e689@phytec.de?part=2