Skip to contentSkip to conversation

ARM: dts: aspeed-g6: Add nodes for i3c controllers

PATCH New Jeremy Kerr 2 participants 5 messages May 1, 2024 – May 2, 2024

Subsystems (2)
Device tree bindings Arm/Aspeed machine support
lore
Share & tools
lore

Jeremy Kerr

author
#0 lore raw

PATCH 1/1 ARM: dts: aspeed-g6: Add nodes for i3c controllers

Add the i3c controller devices to the ast2600 g6 common dts. We add all 6 busses to the common g6 definition, but leave disabled through the status property, to be enabled per-platform.

Signed-off-by Jeremy Kerr <[email protected]>
---
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 93 +++++++++++++++++++++++++
 1 file changed, 93 insertions(+)
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi +93 -0
index 29f94696d8b1..f9d01599a965 100644
@@ -866,6 +866,13 @@ i2c: bus at 1e78a000 {
 				ranges = <0 0x1e78a000 0x1000>;
 			};
 
+			i3c: bus at 1e7a0000 {
+				compatible = "simple-bus";
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0 0x1e7a0000 0x8000>;
+			};
+
 			fsim0: fsi at 1e79b000 {
 				compatible = "aspeed,ast2600-fsi-master", "fsi-master";
 				reg = <0x1e79b000 0x94>;

@@ -1125,3 +1132,89 @@ i2c15: i2c-bus at 800 {
 		status = "disabled";
 	};
 };
+
+&i3c {
+	i3c_global: i3c-global {
+		compatible = "aspeed,ast2600-i3c-global", "syscon", "simple-mfd";
+		reg = <0x0 0x1000>;
+		resets = <&syscon ASPEED_RESET_I3C_DMA>;
+	};
+
+	i3c0: i3c at 2000 {
+		compatible = "aspeed,ast2600-i3c";
+		reg = <0x2000 0x1000>;
+		#address-cells = <3>;
+		#size-cells = <0>;
+		clocks = <&syscon ASPEED_CLK_GATE_I3C0CLK>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i3c1_default>;
+		interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
+		aspeed,global-regs = <&i3c_global 0>;
+		status = "disabled";
+	};
+
+	i3c1: i3c at 3000 {
+		compatible = "aspeed,ast2600-i3c";
+		reg = <0x3000 0x1000>;
+		#address-cells = <3>;
+		#size-cells = <0>;
+		clocks = <&syscon ASPEED_CLK_GATE_I3C1CLK>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i3c2_default>;
+		interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
+		aspeed,global-regs = <&i3c_global 1>;
+		status = "disabled";
+	};
+
+	i3c2: i3c at 4000 {
+		compatible = "aspeed,ast2600-i3c";
+		reg = <0x4000 0x1000>;
+		#address-cells = <3>;
+		#size-cells = <0>;
+		clocks = <&syscon ASPEED_CLK_GATE_I3C2CLK>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i3c3_default>;
+		interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
+		aspeed,global-regs = <&i3c_global 2>;
+		status = "disabled";
+	};
+
+	i3c3: i3c at 5000 {
+		compatible = "aspeed,ast2600-i3c";
+		reg = <0x5000 0x1000>;
+		#address-cells = <3>;
+		#size-cells = <0>;
+		clocks = <&syscon ASPEED_CLK_GATE_I3C3CLK>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i3c4_default>;
+		interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
+		aspeed,global-regs = <&i3c_global 3>;
+		status = "disabled";
+	};
+
+	i3c4: i3c at 6000 {
+		compatible = "aspeed,ast2600-i3c";
+		reg = <0x6000 0x1000>;
+		#address-cells = <3>;
+		#size-cells = <0>;
+		clocks = <&syscon ASPEED_CLK_GATE_I3C4CLK>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i3c5_default>;
+		interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
+		aspeed,global-regs = <&i3c_global 4>;
+		status = "disabled";
+	};
+
+	i3c5: i3c at 7000 {
+		compatible = "aspeed,ast2600-i3c";
+		reg = <0x7000 0x1000>;
+		#address-cells = <3>;
+		#size-cells = <0>;
+		clocks = <&syscon ASPEED_CLK_GATE_I3C5CLK>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_i3c6_default>;
+		interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>;
+		aspeed,global-regs = <&i3c_global 5>;
+		status = "disabled";
+	};
+};
Signature · 1 line
2.39.2
#1 lore raw

PATCH 1/1 ARM: dts: aspeed-g6: Add nodes for i3c controllers

Quoted 14 lines · Jeremy Kerr

Add the i3c controller devices to the ast2600 g6 common dts. We add all 6 busses to the common g6 definition, but leave disabled through the status property, to be enabled per-platform.

Signed-off-by Jeremy Kerr <[email protected]>
---
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 93 +++++++++++++++++++++++++
 1 file changed, 93 insertions(+)
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi +0 -0
index 29f94696d8b1..f9d01599a965 100644

arch/arm/boot/dts/aspeed/aspeed-g6.dtsi i2c: bus at 1e78a000 { · line 870

Quoted 5 lines
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi +2 -0
@@ -866,6 +866,13 @@ i2c: bus at 1e78a000 {
 				ranges = <0 0x1e78a000 0x1000>;
 			};
 
+			i3c: bus at 1e7a0000 {
+				compatible = "simple-bus";

What bus is it? Why is it even needed? If it is i3c, then for sure compatible is wrong.

Quoted 9 lines
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi +5 -0
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0 0x1e7a0000 0x8000>;
+			};
+
 			fsim0: fsi at 1e79b000 {
 				compatible = "aspeed,ast2600-fsi-master", "fsi-master";
 				reg = <0x1e79b000 0x94>;

arch/arm/boot/dts/aspeed/aspeed-g6.dtsi i2c15: i2c-bus at 800 { · line 1136

Quoted 5 lines
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi +2 -0
@@ -1125,3 +1132,89 @@ i2c15: i2c-bus at 800 {
 		status = "disabled";
 	};
 };
+
+&i3c {

????

That's not how we construct DTS. Overrides/extends of nodes are for boards, not within DTSI.

Please provide full correct definition IN ONE place. See DTS coding style.

Best regards,
Krzysztof

author
#2 lore raw

PATCH 1/1 ARM: dts: aspeed-g6: Add nodes for i3c controllers

Hi Krzysztof,

Quoted 11 lines
Quoted 8 lines
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi +0 -0
@@ -866,6 +866,13 @@ i2c: bus at 1e78a000 {
 ????????????????????????????????ranges = <0 0x1e78a000 0x1000>;
 ????????????????????????};

?
+???????????????????????i3c: bus at 1e7a0000 { +???????????????????????????????compatible = "simple-bus";

What bus is it? Why is it even needed? If it is i3c, then for sure compatible is wrong.

This is not the i3c bus, it's the MMIO mapping that allows us to specify the individual i3c controller mappings as sensible offsets into the main address space. Did you miss the ranges property there?

This is following the existing design for the i2c controllers.

Quoted 17 lines
Quoted 9 lines

+???????????????????????????????#address-cells = <1>; +???????????????????????????????#size-cells = <1>; +???????????????????????????????ranges = <0 0x1e7a0000 0x8000>; +???????????????????????};
+
????????????????????????fsim0: fsi at 1e79b000 { ????????????????????????????????compatible = "aspeed,ast2600-fsi-master", "fsi-master"; ????????????????????????????????reg = <0x1e79b000 0x94>;

arch/arm/boot/dts/aspeed/aspeed-g6.dtsi i2c15: i2c-bus at 800 { · line 1136

Quoted 5 lines
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi +2 -0
@@ -1125,3 +1132,89 @@ i2c15: i2c-bus at 800 {
 ????????????????status = "disabled";
 ????????};
 ?};
+
+&i3c {

????

Quoted 2 lines

That's not how we construct DTS.? Overrides/extends of nodes are for boards, not within DTSI.

The overrides are occurring at the &i3cX labels, not &i3c. Platform level dts just connect at those labels to define overrides for each bus:

    &i3c0 {
            status = "okay";
            mctp-controller;
    };

    &i3c1 {
            status = "okay";
            mctp-controller;
    };

There is existing precedence for this layout; the i2c and pinctrl mappings already use dtsi-internal labels. It keeps the bus definitions more manageable.

Cheers,

Jeremy

#3 lore raw

PATCH 1/1 ARM: dts: aspeed-g6: Add nodes for i3c controllers

Quoted 37 lines · Jeremy Kerr

Hi Krzysztof,

Quoted 11 lines
Quoted 8 lines
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi +0 -0
@@ -866,6 +866,13 @@ i2c: bus at 1e78a000 {
 ????????????????????????????????ranges = <0 0x1e78a000 0x1000>;
 ????????????????????????};

?
+???????????????????????i3c: bus at 1e7a0000 { +???????????????????????????????compatible = "simple-bus";

What bus is it? Why is it even needed? If it is i3c, then for sure compatible is wrong.

This is not the i3c bus, it's the MMIO mapping that allows us to specify the individual i3c controller mappings as sensible offsets into the main address space. Did you miss the ranges property there?

This is following the existing design for the i2c controllers.

Quoted 17 lines
Quoted 9 lines

+???????????????????????????????#address-cells = <1>; +???????????????????????????????#size-cells = <1>; +???????????????????????????????ranges = <0 0x1e7a0000 0x8000>; +???????????????????????};
+
????????????????????????fsim0: fsi at 1e79b000 { ????????????????????????????????compatible = "aspeed,ast2600-fsi-master", "fsi-master"; ????????????????????????????????reg = <0x1e79b000 0x94>;

arch/arm/boot/dts/aspeed/aspeed-g6.dtsi i2c15: i2c-bus at 800 { · line 1136

Quoted 5 lines
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi +2 -0
@@ -1125,3 +1132,89 @@ i2c15: i2c-bus at 800 {
 ????????????????status = "disabled";
 ????????};
 ?};
+
+&i3c {

????

Quoted 5 lines
Quoted 2 lines

That's not how we construct DTS.? Overrides/extends of nodes are for boards, not within DTSI.

The overrides are occurring at the &i3cX labels, not &i3c. Platform level dts just connect at those labels to define overrides for each bus:

You miss the point. Look how DTS is constructed, read DTS coding style.

Your first node is empty and that is not readable.

Best regards,
Krzysztof

author
#4 lore raw

PATCH 1/1 ARM: dts: aspeed-g6: Add nodes for i3c controllers

Hi Krysztof,

Quoted 1 line

Your first node is empty and that is not readable.

I'd argue that separating the i3c definitions makes the source more readable (granted, at the cognitive expense of having to dereference a label), but ok.

I'll send a v2 with the bus definitions inline, but first reworking the existing i2c definitions to use the same format.

Cheers,

Jeremy