| 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: <20250915150914.75e1df2b@donnerap>
Date: Mon, 15 Sep 2025 15:09:14 +0100
From: Andre Przywara <andre.przywara@....com>
To: "J. Neuschäfer" <j.ne@...teo.net>
Cc: "J. Neuschäfer via B4 Relay"
<devnull+j.ne.posteo.net@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec
<jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, Maxime
Ripard <mripard@...nel.org>, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] arm64: dts: allwinner: h313: Add Amediatech X96Q
On Mon, 15 Sep 2025 13:40:15 +0000
J. Neuschäfer <j.ne@...teo.net> wrote:
> On Fri, Sep 12, 2025 at 10:54:49AM +0100, Andre Przywara wrote:
> > On Fri, 12 Sep 2025 01:52:10 +0200
> > J. Neuschäfer via B4 Relay <devnull+j.ne.posteo.net@...nel.org> wrote:
> >
> > Hi,
> >
> > many thanks for posting the DT, I really wish more people would do that!
> >
> > > From: "J. Neuschäfer" <j.ne@...teo.net>
> > >
> > > The X96Q is a set-top box with an H313 SoC, AXP305 PMIC, 1 or 2 GiB RAM,
> > > 8 or 16 GiB eMMC flash, 2x USB A, Micro-SD, HDMI, Ethernet, audio/video
> > > output, and infrared input.
> > >
> > > https://x96mini.com/products/x96q-tv-box-android-10-set-top-box
> > >
> > > Tested, works:
> > > - debug UART
> > > - status LED
> > > - USB ports in host mode
> > > - MicroSD
> > > - eMMC
> > > - recovery button hidden behind audio/video port
> > > - analog audio (line out)
> > >
> > > Does not work:
> > > - Ethernet (requires AC200 MFD/EPHY driver)
> > > - analog video output (requires AC200 driver)
> > > - HDMI audio/video output
> > >
> > > Untested:
> > > - "OTG" USB port in device mode
> > > - built-in IR receiver
> > > - external IR receiver
> > > - WLAN (requires out-of-tree XRadio driver)
> > >
> > > Table of regulators on the downstream kernel, for reference:
> > >
> > > vcc-5v 1 15 0 unknown 5000mV 0mA 5000mV 5000mV
> > > dcdca 0 0 0 unknown 900mV 0mA 0mV 0mV
> > > dcdcb 0 0 0 unknown 1350mV 0mA 0mV 0mV
> > > dcdcc 0 0 0 unknown 900mV 0mA 0mV 0mV
> > > dcdcd 0 0 0 unknown 1500mV 0mA 0mV 0mV
> > > dcdce 0 0 0 unknown 3300mV 0mA 0mV 0mV
> > > aldo1 0 0 0 unknown 3300mV 0mA 0mV 0mV
> > > aldo2 0 0 0 unknown 700mV 0mA 0mV 0mV
> > > aldo3 0 0 0 unknown 700mV 0mA 0mV 0mV
> > > bldo1 0 0 0 unknown 1800mV 0mA 0mV 0mV
> > > bldo2 0 0 0 unknown 1800mV 0mA 0mV 0mV
> > > bldo3 0 0 0 unknown 700mV 0mA 0mV 0mV
> > > bldo4 0 0 0 unknown 700mV 0mA 0mV 0mV
> > > cldo1 0 0 0 unknown 2500mV 0mA 0mV 0mV
> > > cldo2 0 0 0 unknown 700mV 0mA 0mV 0mV
> > > cldo3 0 0 0 unknown 700mV 0mA 0mV 0mV
> > >
> > > Signed-off-by: J. Neuschäfer <j.ne@...teo.net>
> > > ---
> > > arch/arm64/boot/dts/allwinner/Makefile | 1 +
> > > arch/arm64/boot/dts/allwinner/sun50i-h313-x96q.dts | 235 +++++++++++++++++++++
> > > 2 files changed, 236 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > > index 780aeba0f3a4e14d69c9602e37b8d299165507b9..2edfa7bf4ab31c4aa934da98e5e042edc9aaf600 100644
> > > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > > @@ -41,6 +41,7 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-pine-h64-model-b.dtb
> > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-tanix-tx6.dtb
> > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-tanix-tx6-mini.dtb
> > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h313-tanix-tx1.dtb
> > > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h313-x96q.dtb
> > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-bigtreetech-cb1-manta.dtb
> > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-bigtreetech-pi.dtb
> > > dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-orangepi-zero2.dtb
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h313-x96q.dts b/arch/arm64/boot/dts/allwinner/sun50i-h313-x96q.dts
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..9534eb03b89557f2545af5af7cf43390be722cf0
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h313-x96q.dts
> > > @@ -0,0 +1,235 @@
> > > +// SPDX-License-Identifier: (GPL-2.0-or-later OR MIT)
> > > +/*
> > > + * Copyright (C) 2025 J. Neuschäfer <j.ne@...teo.net>
> > > + */
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include "sun50i-h616.dtsi"
> > > +#include "sun50i-h616-cpu-opp.dtsi"
> > > +
> > > +#include <dt-bindings/gpio/gpio.h>
> > > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +#include <dt-bindings/input/linux-event-codes.h>
> > > +#include <dt-bindings/leds/common.h>
> > > +
> > > +/ {
> > > + model = "X96Q";
> > > + compatible = "amediatech,x96q", "allwinner,sun50i-h616";
> > > +
> > > + aliases {
> > > + mmc0 = &mmc0;
> > > + mmc1 = &mmc1;
> > > + mmc2 = &mmc2;
> >
> > We don't do mmc aliases in the upstream DTs. Long story, but you should
> > not need them. I guess you want to disagree ;-),
>
> I've seen mmc aliases before, and I've seen the annoying effects of an
> unstable probe order, so this is mostly of thing of habit.
Yes, as I said, it's a long story ;-) IIUC Rockchip does it, we
traditionally don't. There are arguments for both ways, and the decision
for sunxi was to not do it.
> > in this case U-Boot has
> > you covered, by adding the aliases during build time: just use
> > $fdtcontroladdr, as you should do anyway.
>
> I'm using a recent upstream U-Boot with a FIT image ('make image.fit' in
> the Linux source tree). In this case U-Boot does not add mmc aliases.
> Is there a special Kconfig option I need to enable in U-Boot?
U-Boot now automatically sync's the kernel DTs into the U-Boot tree, so
U-Boot is using the exact same DT for itself - and can pass that on to any
kernel. The trick is to just not load a DTB, but instead forward the
address of the one U-Boot is already using: $fdtcontroladdr.
If you are using EFI boot, this is done automatically, if you are using
the traditional booti method, you just change the last parameter:
=> booti $kernel_addr_r $ramdisk_addr_r:$filesize $fdtcontroladdr
I don't know how this works exactly with FIT images, so if you can
override the DTB in there or not include it in the first place. The
concept of bundling a particular DTB with a kernel is admittedly a long
standing practise in the embedded world, but does not make much sense
outside of it (in a single image, distribution world), and I like to treat
the embedded use case as a special case of a more generic approach.
> What do you mean by 'build time'? U-Boot's?
When U-Boot builds the DTB it uses itself, it will add some nodes, mostly
from arch/arm/dts/sunxi-u-boot.dtsi.
And because U-Boot relies on the MMC aliases internally, we add them
there. So if you pass on that DT, you will get the aliases.
Cheers,
Andre
> > > + serial0 = &uart0;
> > > + };
> [...]
> > > +&mmc0 {
> > > + vmmc-supply = <®_aldo1>;
> > > + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */
> > > + disable-wp;
> > > + bus-width = <4>;
> > > + max-frequency = <150000000>;
> >
> > That line is already in the .dtsi file, so redundant.
>
> Good point, removing.
> >
> > > + status = "okay";
> > > + /* µSD */
> >
> > If we really need this comment, it should be above, right after the
> > "&mmc0 {". And I wonder if it should be "microSD" instead.
>
> Good points, I'll do both.
>
> > > +};
> > > +
> > > +&mmc1 {
> > > + /* TODO: XRadio XR819 WLAN */
> >
> > Either you just keep the comment, an mention mmc1, but don't reference the
> > node, or you add the properties that you know of already, like
> > vmmc-supply, vqmmc-supply, mmc-pwrseq, bus-width, non-removable.
> > But this "empty reference with a comment" is somewhat odd.
>
> I'll change it to a pure comment for now.
>
> > > +};
> > > +
> > > +&mmc2 {
> > > + vmmc-supply = <®_aldo1>;
> > > + vqmmc-supply = <®_bldo1>;
> > > + non-removable;
> > > + cap-mmc-hw-reset;
> > > + mmc-ddr-1_8v;
> > > + mmc-hs200-1_8v;
> > > + bus-width = <8>;
> > > + max-frequency = <100000000>;
> >
> > Are you sure you need that?
>
> I found it in the downstream DT. The eMMC probes fine without it, so
> I'll remove it. The eMMC datasheet (assuming I found the right one),
> claims a maximum of 200 MHz, so the 100 MHz restriction seems
> unnecessary.
>
> >
> > > + status = "okay";
> > > + /* eMMC */
> >
> > Please move that comment up.
>
> Will do.
>
>
> [...]
> > > + regulators {
> > > + reg_dcdca: dcdca {
> > > + regulator-always-on;
> > > + regulator-min-microvolt = <810000>;
> > > + regulator-max-microvolt = <1100000>;
> > > + regulator-name = "vdd-cpu";
> > > + };
> > > +
> > > + dcdcb {
> > > + /* unused */
> > > + };
> > > +
> > > + reg_dcdcc: dcdcc {
> > > + regulator-always-on;
> > > + regulator-min-microvolt = <810000>;
> > > + regulator-max-microvolt = <990000>;
> > > + regulator-name = "vdd-gpu-sys";
> > > + };
> > > +
> > > + dcdcd {
> > > + regulator-always-on;
> >
> > Why is this always on? What happens if you remove that line or turn it off?
> > For always-on regulators we either need a comment saying why, or, better,
> > have an explanatory regulator-name (like above).
> > Is that for DRAM, by any chance (1.5V for DDR3 chips)?
>
> The observed behavior is that the system halts when dcdcd is disabled.
> The relationship is a bit hard to determine without schematics, but the
> DDR3 chips (Micron MT41J256M4) do indeed use 1.5V. Since this is the
> only source of 1.5V, I think it's a safe guess that this is indeed the
> DRAM regulator.
>
> >
> > Cheers,
> > Andre
>
> Thanks for the review!
> J. Neuschäfer
Powered by blists - more mailing lists