Re: [PATCH v2 5/6] riscv: dts: spacemit: k1-musepi-pro: enable PCIe ports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `vpcie3v3-supply` property is incorrectly assigned to the top-level PCIe host controller nodes (`&pcie1` and `&pcie2`), which will cause a `dtbs_check` schema validation failure.
- [Low] Grammar and phrasing errors in the commit message.
--

commit 2a8e8221b4fb4ddc072a4a8a753e3e9b7e8f1d06
Author: Andre Heider <a.heider@gmail.com>

riscv: dts: spacemit: k1-musepi-pro: enable PCIe ports

This commit enables the two PCIe controllers and their PHYs for the
SpacemiT K1 MusePI Pro board. The PCIe ports are routed to the M.2 M-key
connector and the PCIe slot, and fixed regulators are provided for the
3.3V supplies.

> Enable the two PCIe controller along with and their associated PHYs. They
> are routed to the M.2 M-key connector and to the PCIe slot.

This isn't a bug, but there's a typo in the commit message. The word
"controller" should be plural, and there is an extra "and" after "with".

> diff --git a/arch/riscv/boot/dts/spacemit/k1-musepi-pro.dts b/arch/riscv/boot/dts/spacemit/k1-musepi-pro.dts
> index 9c90c2817ecb7..2d3e30f0bd806 100644
> --- a/arch/riscv/boot/dts/spacemit/k1-musepi-pro.dts
> +++ b/arch/riscv/boot/dts/spacemit/k1-musepi-pro.dts

[ ... ]

> +&pcie1_phy {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie1_3_cfg>;
> +	status = "okay";
> +};
> +
> +&pcie1_port {
> +	phys = <&pcie1_phy>;
> +	vpcie3v3-supply = <&pcie_vcc_3v3>;
> +};
> +
> +&pcie1 {
> +	vpcie3v3-supply = <&pcie_vcc_3v3>;

Does this cause a dtbs_check schema validation failure?

According to the SpacemiT PCIe device tree binding schema, the
vpcie3v3-supply property is strictly defined under the patternProperties
for the child PCI bridge node. The top-level host controller node
specifies unevaluatedProperties: false and does not define vpcie3v3-supply.

> +	status = "okay";
> +};
> +
> +&pcie2_phy {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie2_4_cfg>;
> +	status = "okay";
> +};
> +
> +&pcie2_port {
> +	phys = <&pcie2_phy>;
> +	vpcie3v3-supply = <&mpcie_vcc_3v3>;
> +};
> +
> +&pcie2 {
> +	vpcie3v3-supply = <&mpcie_vcc_3v3>;

Is this redundancy added to work around an existing issue in the SpacemiT
K1 PCIe driver?

The driver currently fetches the regulator from the root controller device
instead of the port node, but duplicating the property in the device tree
violates the bindings.

Should the driver be updated to parse the child node instead of duplicating
the property here in the root nodes?

> +	status = "okay";
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511111116.1109643-1-a.heider@gmail.com?part=5