Message-ID: <20250912105449.70717d80@donnerap>
Date: Fri, 12 Sep 2025 10:54:49 +0100
From: Andre Przywara <andre.przywara@....com>
To: "J. Neuschäfer via B4 Relay"
 <devnull+j.ne.posteo.net@...nel.org>
Cc: j.ne@...teo.net, 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 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 = <&reg_dcdca>;
> +};
> +
> +&ehci0 {
> +	status = "okay";
> +};
> +
> +&ehci3 {
> +	status = "okay";
> +};
> +
> +/* TODO: EMAC1 connected to AC200 PHY */
> +
> +&gpu {
> +	mali-supply = <&reg_dcdcc>;
> +	status = "okay";
> +};
> +
> +&ir {
> +	status = "okay";
> +};
> +
> +&mmc0 {
> +	vmmc-supply = <&reg_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.

> +};
> +
> +&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.

> +};
> +
> +&mmc2 {
> +	vmmc-supply = <&reg_aldo1>;
> +	vqmmc-supply = <&reg_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.

> +};
> +
> +&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 = <&reg_vcc5v>;
> +		vinb-supply = <&reg_vcc5v>;
> +		vinc-supply = <&reg_vcc5v>;
> +		vind-supply = <&reg_vcc5v>;
> +		vine-supply = <&reg_vcc5v>;
> +		aldoin-supply = <&reg_vcc5v>;
> +		bldoin-supply = <&reg_vcc5v>;
> +		cldoin-supply = <&reg_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