| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<TY2PPF5CB9A1BE626A2F0F6307461D8F64BF2F0A@TY2PPF5CB9A1BE6.apcprd06.prod.outlook.com>
Date: Thu, 23 Oct 2025 07:37:49 +0000
From: Ryan Chen <ryan_chen@...eedtech.com>
To: Arnd Bergmann <arnd@...db.de>, BMC-SW <BMC-SW@...eedtech.com>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Joel Stanley <joel@....id.au>, Andrew Jeffery
<andrew@...econstruct.com.au>, Jeremy Kerr <jk@...econstruct.com.au>, Lee
Jones <lee@...nel.org>, Catalin Marinas <catalin.marinas@....com>, Will
Deacon <will@...nel.org>, Bjorn Andersson <bjorn.andersson@....qualcomm.com>,
Geert Uytterhoeven <geert@...ux-m68k.org>, Nishanth Menon <nm@...com>,
Nícolas F. R. A. Prado <nfraprado@...labora.com>,
Taniya Das <quic_tdas@...cinc.com>, "Lad, Prabhakar"
<prabhakar.mahadev-lad.rj@...renesas.com>, Kuninori Morimoto
<kuninori.morimoto.gx@...esas.com>, Eric Biggers <ebiggers@...nel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
<linux-aspeed@...ts.ozlabs.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v6 4/6] arm64: dts: aspeed: Add initial AST2700 SoC device
tree
Hi Arnd,
Thanks for the detailed review and explanations.
> Subject: Re: [PATCH v6 4/6] arm64: dts: aspeed: Add initial AST2700 SoC device
> tree
>
> On Wed, Oct 22, 2025, at 09:05, Ryan Chen wrote:
> > Add initial device tree for the ASPEED 8th BMC SoC family.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@...eedtech.com>
>
> I think this is the place where you'd want to put some information about the
> chip itself. I know what it is, but others may not know anything about it.
>
I've included basic information about the AST27xx family in the cover letter:
AST27XX SOC Family
- https://www.aspeedtech.com/server_ast2700/
- https://www.aspeedtech.com/server_ast2720/
- https://www.aspeedtech.com/server_ast2750/
I will add here in next submit.
> > + aliases {
> > + serial0 = &uart0;
> > + serial1 = &uart1;
> > + serial2 = &uart2;
> > + serial3 = &uart3;
> > + serial4 = &uart4;
> > + serial5 = &uart5;
> > + serial6 = &uart6;
> > + serial7 = &uart7;
> > + serial8 = &uart8;
> > + serial9 = &uart9;
> > + serial10 = &uart10;
> > + serial11 = &uart11;
> > + serial12 = &uart12;
> > + serial13 = &uart13;
> > + serial14 = &uart14;
> > + };
>
> This looks like you just list all the uarts that are present on the chip, which is
> not how the aliases are meant to be used. Move this block into the board
> specific file and only list the ones that are actually enabled on that particular
> board.
>
> In particular, the alias names are meant to be local to the board and don't
> usually correspond to the numbering inside of the chip. In the defconfig, we
> currently set CONFIG_SERIAL_8250_NR_UARTS=8, which is enough for any
> board we support so far, but that means only the first
> 8 aliases in the list will actually work.
Understood. I'll move the aliases block from the SoC dtsi into the
EVB board dts. For the EVB, UART12 is used as the default console,
and the board labels match the SoC numbering, so I plan to keep:
Does that look acceptable?
ast2700-evb.dts
aliases {
serial0 = &uart0;
serial1 = &uart1;
serial2 = &uart2;
serial3 = &uart3;
serial4 = &uart4;
serial5 = &uart5;
serial6 = &uart6;
serial7 = &uart7;
serial8 = &uart8;
serial9 = &uart9;
serial10 = &uart10;
serial11 = &uart11;
serial12 = &uart12;
serial13 = &uart13;
serial14 = &uart14;
}
>
> > + soc0: soc@...00000 {
> > + compatible = "simple-bus";
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges = <0x0 0x0 0x0 0x10000000 0x0 0x4000000>;
> > +
> > + intc0: interrupt-controller@...00000 {
> > + compatible = "aspeed,ast2700-intc0";
> > + reg = <0 0x12100000 0 0x4000>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges = <0x0 0x0 0x12100000 0x4000>;
>
> This doesn't seem to add up: you define a local register space for the soc from
> 0x0 to 0x4000000, but the registers of the child devices are above 0x4000000.
>
> I suspect that you forgot to adjust all the addresses in the child devices to be
> inside of that range.
Yes, that's a mistake. I'll fix the ranges property in
soc0 to properly cover all child registers.
>
> > + soc1: soc@...00000 {
> > + compatible = "simple-bus";
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges = <0x0 0x0 0x0 0x14000000 0x0 0x10000000>;
>
> This probably needs some explanation: why are there two 'soc@...'
> devices? Is this literally two chips in the system, or are you describing two
> buses inside of the same SoC?
The AST2700 is two soc connection with a property bus.
Sharing some decode registers. Each have it own ahb bus.
>
> > +
> > + mdio0: mdio@...40000 {
> > + compatible = "aspeed,ast2600-mdio";
> > + reg = <0 0x14040000 0 0x8>;
> > + resets = <&syscon1 SCU1_RESET_MII>;
> > + status = "disabled";
> > + };
>
> I see that you use the old compatible="aspeed,ast2600-mdio" string exclusively
> here. While this works, I would suggest you list both a more specific
> "aspeed,ast2700-mdio" string to refer to the version in this chip as well as the
> fallback "aspeed,ast2600-mdio" string as the generic identifier.
>
> The binding obviously has to describe both in that case, but the driver does not
> need to be modified as long as both behave the same way.
Thanks, will submit ast2700-mdio.
Question, should I add in here patch series?
Or go for another patch thread?
>
> > +
> > + syscon1: syscon@...02000 {
> > + compatible = "aspeed,ast2700-scu1", "syscon", "simple-mfd";
> > + reg = <0x0 0x14c02000 0x0 0x1000>;
> > + ranges = <0x0 0x0 0x14c02000 0x1000>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + #clock-cells = <1>;
> > + #reset-cells = <1>;
> > +
> > + scu_ic2: interrupt-controller@100 {
> > + compatible = "aspeed,ast2700-scu-ic2";
> > + reg = <0x100 0x8>;
> > + #interrupt-cells = <1>;
> > + interrupts-extended = <&intc1_5 0>;
> > + interrupt-controller;
> > + };
> > +
> > + scu_ic3: interrupt-controller@108 {
> > + compatible = "aspeed,ast2700-scu-ic3";
> > + reg = <0x108 0x8>;
> > + #interrupt-cells = <1>;
> > + interrupts-extended = <&intc1_5 26>;
> > + interrupt-controller;
> > + };
>
> This looks a bit silly to be honest: you have two separate devices that each have
> a single register and a different compatible string?
Yes, it have difference register define in each scu-ic#. That is compatible with design.
https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-aspeed-scu-ic.c#L45-L48
>
> Also you claim to be compatible with "syscon" but nothing actually refers to the
> syscon node in that form?
There is another submit ongoing in pinctrl which will use syscon.
https://lwn.net/ml/all/20250829073030.2749482-2-billy_tsai@aspeedtech.com/
Could I keep it? or I should remove it?
>
> Arnd
Powered by blists - more mailing lists