* [PATCH] ARM: dts: aspeed-g6: Add nodes for i3c controllers
@ 2024-05-01 3:38 Jeremy Kerr
2024-05-01 10:24 ` Krzysztof Kozlowski
0 siblings, 1 reply; 5+ messages in thread
From: Jeremy Kerr @ 2024-05-01 3:38 UTC (permalink / raw)
To: linux-aspeed
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 <jk@codeconstruct.com.au>
---
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 93 +++++++++++++++++++++++++
1 file changed, 93 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 29f94696d8b1..f9d01599a965 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -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";
+ };
+};
--
2.39.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH] ARM: dts: aspeed-g6: Add nodes for i3c controllers
2024-05-01 3:38 [PATCH] ARM: dts: aspeed-g6: Add nodes for i3c controllers Jeremy Kerr
@ 2024-05-01 10:24 ` Krzysztof Kozlowski
2024-05-01 11:17 ` Jeremy Kerr
0 siblings, 1 reply; 5+ messages in thread
From: Krzysztof Kozlowski @ 2024-05-01 10:24 UTC (permalink / raw)
To: linux-aspeed
On 01/05/2024 05:38, Jeremy Kerr wrote:
> 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 <jk@codeconstruct.com.au>
> ---
> arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 93 +++++++++++++++++++++++++
> 1 file changed, 93 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index 29f94696d8b1..f9d01599a965 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> @@ -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.
> + #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 {
????
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] ARM: dts: aspeed-g6: Add nodes for i3c controllers
2024-05-01 10:24 ` Krzysztof Kozlowski
@ 2024-05-01 11:17 ` Jeremy Kerr
2024-05-01 16:45 ` Krzysztof Kozlowski
0 siblings, 1 reply; 5+ messages in thread
From: Jeremy Kerr @ 2024-05-01 11:17 UTC (permalink / raw)
To: linux-aspeed
Hi Krzysztof,
> > --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> > +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> > @@ -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.
> > +???????????????????????????????#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 {
>
> ????
>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] ARM: dts: aspeed-g6: Add nodes for i3c controllers
2024-05-01 11:17 ` Jeremy Kerr
@ 2024-05-01 16:45 ` Krzysztof Kozlowski
2024-05-02 11:10 ` Jeremy Kerr
0 siblings, 1 reply; 5+ messages in thread
From: Krzysztof Kozlowski @ 2024-05-01 16:45 UTC (permalink / raw)
To: linux-aspeed
On 01/05/2024 13:17, Jeremy Kerr wrote:
> Hi Krzysztof,
>
>>> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
>>> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
>>> @@ -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.
>
>>> +???????????????????????????????#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 {
>>
>> ????
>>
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-05-02 11:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-01 3:38 [PATCH] ARM: dts: aspeed-g6: Add nodes for i3c controllers Jeremy Kerr
2024-05-01 10:24 ` Krzysztof Kozlowski
2024-05-01 11:17 ` Jeremy Kerr
2024-05-01 16:45 ` Krzysztof Kozlowski
2024-05-02 11:10 ` Jeremy Kerr
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).