[PATCH] ARM: dts: aspeed-g6: add pcie-lpc and pcie-kcs4

[PATCH] ARM: dts: aspeed-g6: add pcie-lpc and pcie-kcs4

[Grégoire Layet]

Add pcie_lpc node and pcie_kcs4 child to the ast2600 g6 common dtsi.

The ASPEED AST2600 has a PCIe to LPC controller. It includes a KCS
interface on channel 4. This is a fully KCS-compatible interface
that is exposed over PCIe.

This can be used by the host for IPMI when the PCIe BMC Device is
activated.

While the datasheet provides interrupt numbers for KCS channels 1, 2,
3 and 4 over PCI, not all 4 are described in the "PCIe to LPC Controller"
section. Since only the KCS channel 4 is described, only this channel is
added.

Ordered next to existing lpc node for address ordering.
Use existing "aspeed,ast2600-lpc-v2" compatible string.
The pcie_kcs4 uses existing "aspeed,ast2600-kcs-bmc" compatible as it's a
standard KCS interface.

Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 56bb3b0444f7..ac351f01048f 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -658,6 +658,21 @@ ibt: ibt@140 {
 				};
 			};

+			pcie_lpc: pcie-lpc@1e789800 {
+				compatible = "aspeed,ast2600-lpc-v2", "simple-mfd", "syscon";
+				reg = <0x1e789800 0x800>;
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0x0 0x1e789800 0x800>;
+
+				pcie_kcs4: pcie-kcs@114 {
+					compatible = "aspeed,ast2600-kcs-bmc";
+					reg = <0x114 0x1>, <0x118 0x1>, <0x11c 0x1>;
+					interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>;
+					status = "disabled";
+				};
+			};
+
 			sdc: sdc@1e740000 {
 				compatible = "aspeed,ast2600-sd-controller";
 				reg = <0x1e740000 0x100>;

base-commit: 03f906d8f5541e8bb741035981304feceed5993d
--
2.54.0

Re: [PATCH] ARM: dts: aspeed-g6: add pcie-lpc and pcie-kcs4

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The newly added `pcie_lpc` node's memory region overlaps completely with the upper half of the existing `lpc` node's memory region.
- [Low] The nodes `pcie_lpc` and `pcie_kcs4` use specific, non-generic node names (`pcie-lpc` and `pcie-kcs`), violating Devicetree specification rules.
--

--- Patch [1]: [PATCH] ARM: dts: aspeed-g6: add pcie-lpc and pcie-kcs4 ---
commit 2d0eb7dfc73068ee32b00e22cfaac3679c6e6dbd
Author: Grégoire Layet <gregoire.layet@9elements.com>

ARM: dts: aspeed-g6: add pcie-lpc and pcie-kcs4

This commit adds a pcie_lpc node and a pcie_kcs4 child node to the ast2600
common dtsi. It allows exposing a fully KCS-compatible interface over PCIe,
which can be used by the host for IPMI.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index 56bb3b0444f7e..ac351f01048fe 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi

[ ... ]

> @@ -658,6 +658,21 @@ ibt: ibt@140 {
>  				};
>  			};
>  
> +			pcie_lpc: pcie-lpc@1e789800 {

[Severity: Low]
Is this node name aligned with Devicetree naming specifications?

Using "pcie-lpc" describes the specific bus connection rather than a generic
functional name like "lpc".

> +				compatible = "aspeed,ast2600-lpc-v2", "simple-mfd", "syscon";
> +				reg = <0x1e789800 0x800>;

[Severity: Medium]
Will this reg value cause a resource reservation conflict?

The existing lpc@1e789000 node is defined earlier in this file with a size of
0x1000:

			lpc: lpc@1e789000 {
				compatible = "aspeed,ast2600-lpc-v2", "simple-mfd", "syscon";
				reg = <0x1e789000 0x1000>;

This means the existing lpc node spans up to 0x1e78a000, which completely
overlaps the 0x1e789800 through 0x1e789fff memory range claimed by this
newly added node.

Could the original node's size be reduced to 0x800 to prevent the overlap?

> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				ranges = <0x0 0x1e789800 0x800>;
> +
> +				pcie_kcs4: pcie-kcs@114 {

[Severity: Low]
Does this child node also need a more generic node name like "kcs@114"
instead of "pcie-kcs@114"?

> +					compatible = "aspeed,ast2600-kcs-bmc";
> +					reg = <0x114 0x1>, <0x118 0x1>, <0x11c 0x1>;
> +					interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>;
> +					status = "disabled";
> +				};
> +			};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/de49f2032f5b1f170161dcd6191c0daa48f0b0c2.1784117715.git.gregoire.layet@9elements.com?part=1

Re: [PATCH] ARM: dts: aspeed-g6: add pcie-lpc and pcie-kcs4

[Tan Siewert]

> Add pcie_lpc node and pcie_kcs4 child to the ast2600 g6 common dtsi.
> 
> The ASPEED AST2600 has a PCIe to LPC controller. It includes a KCS
> interface on channel 4. This is a fully KCS-compatible interface
> that is exposed over PCIe.
> 
> This can be used by the host for IPMI when the PCIe BMC Device is
> activated.
> 
> While the datasheet provides interrupt numbers for KCS channels 1, 2,
> 3 and 4 over PCI, not all 4 are described in the "PCIe to LPC Controller"
> section. Since only the KCS channel 4 is described, only this channel is
> added.
> 
> Ordered next to existing lpc node for address ordering.
> Use existing "aspeed,ast2600-lpc-v2" compatible string.
> The pcie_kcs4 uses existing "aspeed,ast2600-kcs-bmc" compatible as it's a
> standard KCS interface.
> 
> Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
>
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index 56bb3b0444f7..ac351f01048f 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> @@ -658,6 +658,21 @@ ibt: ibt@140 {
>  				};
>  			};
>  
> +			pcie_lpc: pcie-lpc@1e789800 {

lpc@1e789000 already maps 0x1e789000-0x1e78a000 and 0x1e789914 falls inside it,
so you're describing a second `ast2600-lpc-v2` node which is unnecessary.

Suggestion: Merge pcie_kcs4 into lpc@1e789000 and use 914 as offset. That way
you don't accidentally cause an overlap for the devices if you describe more in
the future.

	Tan

-- 
Tan Siewert <tan.siewert@9elements.com>