Re: [PATCH v2 2/2] arm: dts: nxp: imx: Add devicetree files for M48

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] pinctrl: imx: out-of-bounds heap write during pinctrl mmio parsing
- [High] drm/imx: imx-ldb probe failure due to missing fsl,data-width property
- [Medium] dt: missing nvmem-cells for FEC MAC address provisioning
- [Low] build: missing U-Boot dtsi include and Makefile target for imx6qp
- [Low] dt: invalid syntax with duplicate properties under U-Boot macro
- [Low] dt: dead display-timings node in dw-hdmi configuration
--

commit 5077f8eda8c37343a057ce35121fe2403593e1fb
Author: Petko Manolov <petko.manolov@konsulko.com>

arm: dts: nxp: imx: Add devicetree files for M48

M48 is a heterogeneous board made by Draeger that is based on two
independent cpus, that are monitoring each other. This patch adds the
DT for the two variants of M48 that are based on imx6q and imx6dl.

> diff --git a/arch/arm/boot/dts/nxp/imx/Makefile b/arch/arm/boot/dts/nxp/imx/Makefile
> index 1a2539fa19b44..44b3932562358 100644
> --- a/arch/arm/boot/dts/nxp/imx/Makefile
> +++ b/arch/arm/boot/dts/nxp/imx/Makefile
> @@ -220,6 +220,8 @@ dtb-$(CONFIG_SOC_IMX6Q) += \
>  	imx6q-dmo-edmqmx6.dtb \
>  	imx6q-dms-ba16.dtb \
>  	imx6q-ds.dtb \
> +	imx6q-draeger-m48.dtb \
> +	imx6dl-draeger-m48.dtb \

[Severity: Low]
Is the imx6qp-draeger-m48.dtb variant intentionally missing from this list?

Because it is excluded from the build, it manifests as dead code and masks a
compilation error where the imx6qp variant includes a nonexistent U-Boot
dtsi.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6dl-draeger-m48.dts b/arch/arm/boot/dts/nxp/imx/imx6dl-draeger-m48.dts
> new file mode 100644
> index 0000000000000..45809770d16f3
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6dl-draeger-m48.dts
[ ... ]
> +#define MX6QDL_PAD_CTL_GRP_DDR_TYPE_RGMII        0x768 0xfff 0x000 0x000C0000 0x0
> +#define MX6QDL_PAD_CTL_GRP_RGMII_TERM            0x788 0xfff 0x000 0x00000200 0x0

[Severity: High]
Does this custom macro define an invalid mux_reg offset (0x768) that
translates to an out-of-bounds pin ID?

When the device tree assigns these out-of-bounds register offsets to the
standard fsl,pins pinctrl group enetgrp, the driver calculates an array index
by dividing mux_reg (0x768) by 4, yielding pin_id = 474.

In drivers/pinctrl/freescale/pinctrl-imx.c:imx_pinctrl_parse_pin_mmio():

    *pin_id = (mux_reg != -1) ? mux_reg / 4 : conf_reg / 4;
    pin_reg = &ipctl->pin_regs[*pin_id];
    pin->pin = *pin_id;
    pin_reg->mux_reg = mux_reg;

The ipctl->pin_regs array is allocated with info->npins (which is 216 for
i.MX6Q/DL). Writing to ipctl->pin_regs[474] writes strictly past the end of
the heap allocation, leading to memory corruption during early boot.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6qdl-draeger-m48.dtsi b/arch/arm/boot/dts/nxp/imx/imx6qdl-draeger-m48.dtsi
> new file mode 100644
> index 0000000000000..bcfe7b2556cbd
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6qdl-draeger-m48.dtsi
[ ... ]
> +&fec {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_m48_enet>;
> +	phy-mode = "rgmii-id";
> +	phy-handle = <&ethphy>;
> +	status = "okay";

[Severity: Medium]
Is this missing the standard nvmem-cells bindings for FEC MAC address
provisioning?

The Linux fec driver relies on nvmem-cells and nvmem-mac-interfaces to
retrieve the programmed MAC address automatically. Since this relies on a
downstream U-Boot alias (eeprom_mac = &eeprom_mac;) instead, the kernel will
fail to extract the MAC address from the EEPROM and will assign a random MAC
address to the Ethernet interface.

[ ... ]
> +&hdmi {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_m48_hdmi>;
> +
> +	display-timings {
> +		native-mode = <&hdmi0>;
> +
> +		hdmi0: hdmi {
> +			clock-frequency = <65000000>;

[Severity: Low]
Are these display timings dead code?

This display-timings node is embedded directly within the &hdmi controller
node, which is unsupported and unparsed by the dw-hdmi driver. The i.MX6 HDMI
DRM driver retrieves timings via DDC I2C (EDID) or DRM panel bridge bindings,
so these provided HDMI resolutions and timings are completely ignored.

[ ... ]
> +&ldb {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_m48_lvds>;
> +
> +	lvds-channel@0 {
> +		fsl,data-mapping = "spwg";
> +		fsl,default-data-width = <24>;

[Severity: High]
Will the use of a made-up fsl,default-data-width property instead of
fsl,data-width cause the imx-ldb probe to fail?

During probe, of_get_bus_format() reads a default datawidth of 0, fails to
find a matching 0-bit format for "spwg" in imx_ldb_bit_mappings, and returns
-ENOENT.

In drivers/gpu/drm/imx/ipuv3/imx-ldb.c:imx_ldb_bind():

    if (bus_format < 0) {
        dev_err(dev, "could not determine data mapping: %d\n",
            bus_format);
        ret = bus_format;
        goto free_child;
    }

Because -ENOENT evaluates to < 0 and is not explicitly handled as an optional
-EINVAL fallback, the driver aborts with a "could not determine data mapping"
error, causing the LVDS display output to completely fail.

[ ... ]
> +	config {
> +		#ifdef LEGACY_BOOT
> +		bootcmd = "startM48;errorMsg";
> +			bootcmd = "startKernel;errorMsg";
> +		#endif
> +
> +	};

[Severity: Low]
Does this duplicate bootcmd property declaration cause an invalid syntax
error?

If the Device Tree is preprocessed with -DLEGACY_BOOT, the Device Tree
Compiler (DTC) will error out due to duplicate properties within the same
block.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6qp-draeger-m48.dts b/arch/arm/boot/dts/nxp/imx/imx6qp-draeger-m48.dts
> new file mode 100644
> index 0000000000000..ab331596abd98
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6qp-draeger-m48.dts
[ ... ]
> +#include "imx6qdl-draeger-m48.dtsi"
> +#include "imx6qdl-draeger-m48-u-boot.dtsi"

[Severity: Low]
Does this reference a nonexistent file?

This includes a downstream U-Boot artifact (imx6qdl-draeger-m48-u-boot.dtsi)
which does not exist in the upstream tree. If built out-of-tree or enabled
later, it will immediately trigger a fatal preprocessor error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924183855.1393302-1-petko.manolov@konsulko.com?part=2