| 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:
<TY2PPF5CB9A1BE6CF8336D211641A18E2DEF2F1A@TY2PPF5CB9A1BE6.apcprd06.prod.outlook.com>
Date: Fri, 24 Oct 2025 03:54:55 +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
> Subject: Re: [PATCH v6 4/6] arm64: dts: aspeed: Add initial AST2700 SoC device
> tree
>
> On Thu, Oct 23, 2025, at 09:37, Ryan Chen wrote:
> >
> >> > + 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;
> > }
>
> I think this would be broken for the defconfig if the consol is on serial12. I
> would recommend using serial0 as the console, like
>
> aliases {
> serial0 = &uart12;
> }
>
> in this case. If additional uarts are enabled, add those as further aliases.
Understood, I will update.
>
> >>
> >> > + 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.
>
> I don't understand your explanation,
Let me clarify more clearly:
The AST2700 is a dual-SoC architecture, consisting of two interconnected SoCs,
referred to as SoC0 and SoC1. Each SoC has its own clock/reset domains.
They are connected through an internal "property bus",
which is Aspeed's internal interconnect providing shared
address decoding and communication between the two SoCs.
>
> >>
> >> > +
> >> > + 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?
>
> Since there is no corresponding driver change, I would keep the binding change
> as a patch in this series.
Sorry, I am wondering, I will follow Andrew advice.
Submit ast2700-mdio to net-next go out another thread.
And put submit link in cover-letter in next version.
Is it ok?
>
> >> > +
> >> > + 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-aspe
> > ed-scu-ic.c#L45-L48
>
> Right, if the driver already has this design, it does make sense to not change it
> for the new generation. For a newly added driver I would probably do it
> differently.
Thanks
>
> >> 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?
>
> The version of the driver you are linking does not appear to use syscon, maybe
> this is an artifact from a previous version?
>
> If so, you can drop it. On the other hand, this does seem to be a classic syscon
> device and keeping it marked that way is not harmful, just redundant if you
> actually use the more specific compatible string.
Sorry, I may not point right link
https://patchwork.ozlabs.org/project/linux-aspeed/patch/20250829073030.2749482-4-billy_tsai@aspeedtech.com/
aspeed_g7_soc0_pinctrl_probe -> aspeed_pinctrl_probe
https://github.com/torvalds/linux/blob/master/drivers/pinctrl/aspeed/pinctrl-aspeed.c#L456
That will use syscon to regmap.
>
>
> Arnd
Powered by blists - more mailing lists