Re: [PATCH v2 2/2] ARM: dts: aspeed: Add NVIDIA MGX Vera C2 BMC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ARM: dts: aspeed: vr-c2-bmc mac2 pinctrl conflicts with gpio0 mux pins
- [Low] ARM: dts: aspeed: vr-c2: unreferenced framebuffer shared-dma-pool node
--

commit 07cf82ff2465e47eff9ccc7235e61fcff4d5b27a
Author: Jacky Huang <jackyhuang@nvidia.com>

ARM: dts: aspeed: Add NVIDIA MGX Vera C2 BMC

This commit adds the device tree for the NVIDIA MGX Vera C2 BMC based on
the ASPEED AST2600 SoC.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-nvidia-vr-c2-bmc.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-nvidia-vr-c2-bmc.dts
> new file mode 100644
> index 0000000000000..a044dfa004fc1
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-nvidia-vr-c2-bmc.dts

[ ... ]

> +	reserved-memory {
> +		ranges;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		framebuffer@9f000000 {
> +			reg = <0x9f000000 0x01000000>;
> +			no-map;
> +		};
> +
> +		ramoops@a0000000 {
> +			compatible = "ramoops";
> +			reg = <0xa0000000 0x100000>;
> +			max-reason = <2>;
> +			record-size = <0x10000>;
> +		};
> +
> +		framebuffer {
> +			compatible = "shared-dma-pool";
> +			alignment = <0x01000000>;
> +			reusable;
> +			size = <0x01000000>;
> +		};

[Severity: Low]
Is this framebuffer shared-dma-pool node intentionally left unreferenced?

During boot, fdt_scan_reserved_mem() -> __reserved_mem_alloc_size() ->
rmem_cma_setup() allocates 16 MiB from memblock for this pool. Because
linux,cma-default is not set, rmem_cma_setup() only stores the struct cma
pointer in rmem->priv for per-device attachment via a memory-region
phandle in of_reserved_mem_device_init_by_idx().

However, this node has no phandle label, no device node references it via
memory-region, and &gfx is not enabled in this device tree, so no device
can allocate DMA buffers from this 16 MiB pool. Should this node be
removed, or labeled and referenced by an enabled &gfx node?

[ ... ]

> +&gpio0 {
> +	gpio-line-names =
> +		/* A0-A7 */
> +		"", "", "", "", "", "", "", "",
> +
> +		/* B0-B7 */
> +		"", "", "", "", "", "", "", "",
> +
> +		/* C0-C7 */
> +		"SGPIO_I2C_MUX_SEL-O",
> +		"", "", "", "", "", "", "",
> +
> +		/* D0-D7 */
> +		"", "", "",
> +		"UART1_MUX_SEL-O",

[ ... ]

> +/* NCSI MAC */
> +&mac2 {
> +	phy-mode = "rmii";
> +	pinctrl-0 = <&pinctrl_rmii3_default>;
> +	pinctrl-names = "default";
> +	use-ncsi;
> +
> +	status = "okay";
> +};

[Severity: Medium]
Does pinctrl_rmii3_default on &mac2 conflict with the gpio0 lines
SGPIO_I2C_MUX_SEL-O (GPIOC0) and UART1_MUX_SEL-O (GPIOD3)?

In drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c, the RMII3 group includes
balls H24 (GPIOC0 / RMII3RCLKO) and E26 (GPIOD3 / RMII3RXER):

    GROUP_DECL(RMII3, H24, J22, H22, H23, G23, F23, F26, F25, E26);
    GROUP_DECL(NCSI3, J22, H22, H23, G23, F23, F26, F25, E26);

When &mac2 probes with pinctrl_rmii3_default, pinmux_enable_setting()
claims both H24 and E26 for 1e670000.ethernet and muxes H24 as the 50 MHz
RMII3RCLKO output. Because aspeed_g6_pinmux_ops sets .strict = true, any
subsequent GPIO request for GPIOC0 or GPIOD3 via aspeed_gpio_request() ->
pinctrl_gpio_request() -> pin_request() will fail with -EINVAL.

Since &mac2 uses NCSI, should it use pinctrl_ncsi3_default so H24
(GPIOC0) is not claimed as RMII3RCLKO, and is GPIOD3 (E26) expected to be
used as a GPIO while NCSI3 is active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793705210.git.jackyhuang@nvidia.com?part=2