Re: [PATCH v5 2/3] ARM: dts: aspeed: Add Intel AVCRP BMC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ARM: dts: aspeed: avcrp: uart3/4 pinmux conflicts and pinctrl
  GPIO names
- [Low] ARM: dts: aspeed: avcrp: unnecessary RMII RCLK clock specified
  on RGMII mac1
--

commit 60b4388d543f21749b3a54e80fb2aad89bf0d6d7
Author: Dawid Glazik <dawid.glazik@linux.intel.com>

ARM: dts: aspeed: Add Intel AVCRP BMC

This commit adds the device tree for the Intel AVCRP BMC platform based on
the ASPEED AST2600 SoC.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-intel-avcrp.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-intel-avcrp.dts
> new file mode 100644
> index 0000000000000..eb73d64412228
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-intel-avcrp.dts
[ ... ]
> +&gpio0 {
> +	gpio-line-names =
> +		/*A0-A7*/ "", "", "", "", "SMB_CPU_PIROM_SCL", "SMB_CPU_PIROM_SDA",
> +				"SMB_IPMB_STBY_LVC3_SCL", "SMB_IPMB_STBY_LVC3_SDA",
> +		/*B0-B7*/ "", "", "", "FM_CPU1_ERR2_LVT3_N", "", "",
> +				"FM_BMC_BMCINIT_R", "FP_ID_LED_N",
[ ... ]
> +		/*L0-L7*/ "", "", "", "", "PREQ_N", "TCK_MUX_SEL", "", "",
> +		/*M0-M7*/ "", "", "", "", "", "", "SPA_SOUT", "SPA_SIN",
[ ... ]
> +&mac1 {
> +	clocks = <&syscon ASPEED_CLK_GATE_MAC2CLK>,
> +		 <&syscon ASPEED_CLK_MAC2RCLK>;
> +	clock-names = "MACCLK", "RCLK";
> +	phy-handle = <&ethphy1>;
> +	phy-mode = "rgmii-id";
> +	pinctrl-0 = <&pinctrl_rgmii2_default>;
> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +};

[Severity: Low]
Does &mac1 need the second clock entry here?  In aspeed-g6.dtsi, &mac1
already sets clocks to ASPEED_CLK_GATE_MAC2CLK, and the optional second
clock (ASPEED_CLK_MAC2RCLK) gates the 50MHz reference clock output for
rmii mode, which is unused when phy-mode is "rgmii-id".

[ ... ]

> +&uart1 {
> +	status = "okay";
> +};
> +
> +&uart3 {
> +	status = "okay";
> +};
> +
> +&uart4 {
> +	status = "okay";
> +};
> +
> +&uart5 {
> +	status = "okay";
> +};
> +
> +&uart_routing {
> +	status = "okay";
> +};

[Severity: High]
Should &uart3 and &uart4 override pinctrl-0 = <>; when used with
&uart_routing on this board?

In aspeed-g6.dtsi, &uart3 defaults to pinctrl_txd3_default and
pinctrl_rxd3_default (pins L4 and L5), and &uart4 defaults to
pinctrl_txd4_default and pinctrl_rxd4_default (pins B6 and B7).  Without
clearing pinctrl-0, probing &uart3 and &uart4 muxes those balls away from
&gpio0, driving serial tx onto the board lines named in gpio-line-names on
B6 and L4 and causing gpio requests on B6, B7, L4 and L5 to fail with
-EINVAL under strict pinmux.

In addition, &gpio0 still sets gpio-line-names on pins A4-A7 and M6-M7,
which are claimed by the default pinctrl of &i2c12, &i2c13 and &uart1.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794242015.git.dawid.glazik@linux.intel.com?part=2