| 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: <CAGb2v67dp8V4A86yyaixN9oOgBzMpLJ0ZxnDLng8mO2tkEqYUg@mail.gmail.com>
Date: Sat, 13 Sep 2025 17:36:12 +0800
From: Chen-Yu Tsai <wens@...e.org>
To: Andre Przywara <andre.przywara@....com>,
J. Neuschäfer via B4 Relay <devnull+j.ne.posteo.net@...nel.org>,
j.ne@...teo.net
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.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 Fri, Sep 12, 2025 at 5:54 PM Andre Przywara <andre.przywara@....com> 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 ;-), in this case U-Boot has
> you covered, by adding the aliases during build time: just use
> $fdtcontroladdr, as you should do anyway.
>
> > + serial0 = &uart0;
> > + };
> > +
> > + chosen {
> > + stdout-path = "serial0:115200n8";
> > + };
> > +
> > + reg_vcc5v: vcc5v {
> > + /* board wide 5V supply directly from the DC input */
> > + compatible = "regulator-fixed";
> > + regulator-name = "vcc-5v";
> > + regulator-min-microvolt = <5000000>;
> > + regulator-max-microvolt = <5000000>;
> > + regulator-always-on;
> > + };
> > +
> > + gpio-keys {
> > + compatible = "gpio-keys";
> > +
> > + key-recovery {
> > + label = "Recovery";
> > + linux,code = <KEY_VENDOR>;
> > + gpios = <&pio 7 9 GPIO_ACTIVE_LOW>;
> > + };
> > + };
> > +
> > + leds {
> > + compatible = "gpio-leds";
> > +
> > + led-0 {
> > + color = <LED_COLOR_ID_BLUE>;
> > + gpios = <&pio 7 6 GPIO_ACTIVE_LOW>;
> > + default-state = "on";
> > + };
> > + };
> > +};
> > +
> > +&codec {
> > + allwinner,audio-routing = "Line Out", "LINEOUT";
> > + status = "okay";
> > +};
> > +
> > +&cpu0 {
> > + cpu-supply = <®_dcdca>;
> > +};
> > +
> > +&ehci0 {
> > + status = "okay";
> > +};
> > +
> > +&ehci3 {
> > + status = "okay";
> > +};
> > +
> > +/* TODO: EMAC1 connected to AC200 PHY */
> > +
> > +&gpu {
> > + mali-supply = <®_dcdcc>;
> > + status = "okay";
> > +};
> > +
> > +&ir {
> > + status = "okay";
> > +};
> > +
> > +&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.
>
> > + 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.
Yes. Please use ASCII in the code if possible, since some of us have
setups that don't quite work well with extended character sets.
> > +};
> > +
> > +&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'd say just fill it in completely so that the mmc host is enabled and
the SDIO card is detected. Missing driver support for the chip is a
different issue, but since this is an enumerable bus it shouldn't prevent
you from describing everything already.
> > +};
> > +
> > +&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?
>
> > + status = "okay";
> > + /* eMMC */
>
> Please move that comment up.
I don't think it's necessary though. hs200 and 8-bit width would make
it obvious that it's an eMMC.
> > +};
> > +
> > +&ohci0 {
> > + status = "okay";
> > +};
> > +
> > +&ohci3 {
> > + status = "okay";
> > +};
> > +
> > +&r_i2c {
> > + status = "okay";
> > +
> > + axp305: pmic@36 {
> > + compatible = "x-powers,axp305", "x-powers,axp805",
> > + "x-powers,axp806";
> > + interrupt-controller;
> > + #interrupt-cells = <1>;
> > + reg = <0x36>;
> > +
> > + x-powers,self-working-mode;
> > + vina-supply = <®_vcc5v>;
> > + vinb-supply = <®_vcc5v>;
> > + vinc-supply = <®_vcc5v>;
> > + vind-supply = <®_vcc5v>;
> > + vine-supply = <®_vcc5v>;
> > + aldoin-supply = <®_vcc5v>;
> > + bldoin-supply = <®_vcc5v>;
> > + cldoin-supply = <®_vcc5v>;
> > +
> > + 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)?
>
> Cheers,
> Andre
>
> > + regulator-min-microvolt = <1500000>;
> > + regulator-max-microvolt = <1500000>;
> > + };
> > +
> > + dcdce {
> > + /* unused */
> > + };
> > +
> > + reg_aldo1: aldo1 {
> > + regulator-always-on;
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + regulator-name = "vcc3v3";
> > + };
> > +
> > + aldo2 {
> > + /* unused */
> > + };
> > +
> > + aldo3 {
> > + /* unused */
> > + };
> > +
> > + reg_bldo1: bldo1 {
> > + regulator-always-on;
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > + regulator-name = "vcc1v8";
> > + };
> > +
> > + bldo2 {
> > + /* unused */
> > + };
> > +
> > + bldo3 {
> > + /* unused */
> > + };
> > +
> > + bldo4 {
> > + /* unused */
> > + };
> > +
> > + cldo1 {
> > + /* unused */
> > + };
> > +
> > + cldo2 {
> > + /* unused */
> > + };
> > +
> > + cldo3 {
> > + /* unused */
> > + };
> > + };
> > + };
> > +};
> > +
> > +&uart0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&uart0_ph_pins>;
> > + status = "okay";
> > +};
> > +
> > +&usbotg {
> > + dr_mode = "host"; /* USB A type receptacle */
> > + status = "okay";
> > +};
> > +
> > +&usbphy {
> > + status = "okay";
> > +};
> >
>
>
Powered by blists - more mailing lists